Skip to content

refactor: change TypeTransformerRegistry to use a Map instead of Arraylist #5929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mokhairymahmoud wants to merge 3 commits into
base: main
Choose a base branch
from
Open

refactor: change TypeTransformerRegistry to use a Map instead of Arraylist #5929

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b37181a
feat: change TypeTransformerRegistry to use a Map instead of Arraylist.
Jul 29, 2026
705fd59
simplified logic for the typetransformer selector.
Aug 3, 2026
32c68de
chore: empty commit
Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 25 additions & 7 deletions ...n/lib/jsonld-lib/src/main/java/org/eclipse/edc/transform/TypeTransformerRegistryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@
import org.eclipse.edc.transform.spi.TypeTransformerRegistry;
import org.jetbrains.annotations.NotNull;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
Expand All @@ -31,7 +29,7 @@

public class TypeTransformerRegistryImpl implements TypeTransformerRegistry {
private final Map<String, Class<?>> aliases = new HashMap<>();

@ndr-brt ndr-brt Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's been a while that I see this aliases unused field, since you're here, could you please delete it?

@mokhairymahmoud mokhairymahmoud Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

private final List<TypeTransformer<?, ?>> transformers = new ArrayList<>();
private final Map<Class<?>, Map<Class<?>, TypeTransformer<?, ?>>> transformers = new HashMap<>();
private final Map<String, TypeTransformerRegistry> contextRegistries = new HashMap<>();
private TypeTransformerRegistry parent;

Expand All @@ -44,7 +42,8 @@ private TypeTransformerRegistryImpl(TypeTransformerRegistry parent) {

@Override
public void register(TypeTransformer<?, ?> transformer) {
this.transformers.add(transformer);
transformers.computeIfAbsent(transformer.getInputType(), key -> new HashMap<>())

@ndr-brt ndr-brt Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could worth to add a warning log in case of transformer override

@mokhairymahmoud mokhairymahmoud Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already did, thanks.

.put(transformer.getOutputType(), transformer);
}

@Override
Expand All @@ -54,14 +53,33 @@ public void register(TypeTransformer<?, ?> transformer) {

@Override
public @NotNull <INPUT, OUTPUT> TypeTransformer<INPUT, OUTPUT> transformerFor(@NotNull INPUT input, @NotNull Class<OUTPUT> outputType) {
return transformers.stream()
.filter(t -> t.getInputType().isInstance(input) && t.getOutputType().equals(outputType))
.findAny()
return findTransformer(input, outputType)
.map(it -> (TypeTransformer<INPUT, OUTPUT>) it)
.or(() -> Optional.ofNullable(parent).map(p -> p.transformerFor(input, outputType)))
.orElseThrow(() -> new EdcException(format("No Transformer registered that can handle %s -> %s", input.getClass(), outputType)));
}

private Optional<TypeTransformer<?, ?>> findTransformer(Object input, Class<?> outputType) {
var inputTypes = transformers.entrySet().stream()
.filter(entry -> entry.getKey().isInstance(input))
.filter(entry -> entry.getValue().containsKey(outputType))
.map(Map.Entry::getKey)
.toList();

var mostSpecificInputTypes = inputTypes.stream()
.filter(candidate -> inputTypes.stream()
.noneMatch(other -> !candidate.equals(other) && candidate.isAssignableFrom(other)))
.toList();

if (mostSpecificInputTypes.size() > 1) {
throw new EdcException(format("Ambiguous transformers registered for %s -> %s", input.getClass(), outputType));
}

return mostSpecificInputTypes.stream()
.findFirst()
.map(inputType -> transformers.get(inputType).get(outputType));
}
Comment on lines +75 to +101

@ndr-brt ndr-brt Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logic is not wrong but I think it could be written in a more readable way - and slightly more performant, by avoiding multiple iterations over inputTypes, using streams reduce function:

    private Optional<TypeTransformer<?, ?>> findTransformer(Object input, Class<?> outputType) {
        return transformers.entrySet().stream()
                .filter(entry -> entry.getKey().isInstance(input))
                .filter(entry -> entry.getValue().containsKey(outputType))
                .map(Map.Entry::getKey)
                .map(Optional::of)
                .reduce(Optional.empty(), (current, candidate) -> {
                    if (current.isEmpty()) {
                        return candidate;
                    }

                    if (candidate.get().isAssignableFrom(current.get())) {
                        return current;
                    }

                    if (current.get().isAssignableFrom(candidate.get())) {
                        return candidate;
                    }

                    throw new EdcException(format("Ambiguous transformers registered for %s -> %s", input.getClass(), outputType));
                })
                .map(transformers::get)
                .map(it -> it.get(outputType));
    }

so the "most specific" logic emerges in the reduce function itself:

  • set the first item as current
  • is the candidate a supertype of current? ignore it
  • is the candidate a subtype of current? set it as current
  • neither of the two? it means that it's a type that's on a different hierarchy: throw exception

the reduce function could also be extracted and called findMostSpecificInputType

@mokhairymahmoud mokhairymahmoud Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your implementation is cleaner so I applied it. thanks


@Override
public <INPUT, OUTPUT> Result<OUTPUT> transform(@NotNull INPUT input, @NotNull Class<OUTPUT> outputType) {
Objects.requireNonNull(input);
Expand Down
43 changes: 43 additions & 0 deletions core/common/lib/jsonld-lib/src/test/java/org/eclipse/edc/transform/TestTypeTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright (c) 2022 - 2023 Bayerische Motoren Werke Aktiengesellschaft (BMW AG)

@ndr-brt ndr-brt Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: copyright

@mokhairymahmoud mokhairymahmoud Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed it

*
* This program and the accompanying materials are made available under the
* terms of the Apache License, Version 2.0 which is available at
* https://www.apache.org/licenses/LICENSE-2.0
*
* SPDX-License-Identifier: Apache-2.0
*
* Contributors:
* Bayerische Motoren Werke Aktiengesellschaft (BMW AG) - initial API and implementation
*
*/

package org.eclipse.edc.transform;

import org.eclipse.edc.transform.spi.TransformerContext;
import org.eclipse.edc.transform.spi.TypeTransformer;

public class TestTypeTransformer<INPUT, OUTPUT> implements TypeTransformer<INPUT, OUTPUT> {
private final Class<INPUT> inputType;
private final Class<OUTPUT> outputType;

public TestTypeTransformer(Class<INPUT> inputType, Class<OUTPUT> outputType) {
this.inputType = inputType;
this.outputType = outputType;
}

@Override
public Class<INPUT> getInputType() {
return inputType;
}

@Override
public Class<OUTPUT> getOutputType() {
return outputType;
}

@Override
public OUTPUT transform(INPUT input, TransformerContext context) {
return null;
}
}
39 changes: 39 additions & 0 deletions ...b/jsonld-lib/src/test/java/org/eclipse/edc/transform/TypeTransformerRegistryImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
import static org.eclipse.edc.junit.assertions.AbstractResultAssert.assertThat;
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verifyNoInteractions;

Expand All @@ -46,6 +47,34 @@ void shouldReturnTheCorrectTransformer() {
assertThat(transformer).isInstanceOf(StringIntegerTypeTransformer.class);
}

@Test
void shouldReplaceTransformerForTheSameInputAndOutputTypes() {
var replacement = new TestTypeTransformer<>(String.class, Integer.class);
registry.register(replacement);

assertThat(registry.transformerFor("a string", Integer.class)).isSameAs(replacement);
}

@Test
void shouldReturnMostSpecificCompatibleTransformer() {
var objectTransformer = new TestTypeTransformer<>(Object.class, Long.class);
var charSequenceTransformer = new TestTypeTransformer<>(CharSequence.class, Long.class);
registry.register(objectTransformer);
registry.register(charSequenceTransformer);

assertThat(registry.transformerFor("a string", Long.class)).isSameAs(charSequenceTransformer);
}

@Test
void shouldThrowExceptionWhenCompatibleTransformersAreAmbiguous() {
registry.register(new TestTypeTransformer<>(CharSequence.class, Long.class));
registry.register(new TestTypeTransformer<>(Comparable.class, Long.class));

assertThatThrownBy(() -> registry.transformerFor("a string", Long.class))
.isInstanceOf(EdcException.class)
.hasMessageContaining("Ambiguous transformers");
}

@Test
void shouldThrowExceptionWhenTransformerDoesNotExist() {
var notString = 4L;
Expand Down Expand Up @@ -96,13 +125,23 @@ void shouldTransformUsingNestedContext() {
TypeTransformer<Integer, String> typeTransformer = mock();
contextRegistry.register(typeTransformer);
registry.register(typeTransformer);
clearInvocations((Object) typeTransformer);

assertThat(nestedContextRegistry.transform(5, String.class))
.isSucceeded().isEqualTo("5");

verifyNoInteractions(typeTransformer);
}

@Test
void shouldOverrideParentTransformer() {
var replacement = new TestTypeTransformer<>(String.class, Integer.class);
contextRegistry.register(replacement);

assertThat(contextRegistry.transformerFor("a string", Integer.class)).isSameAs(replacement);
assertThat(registry.transformerFor("a string", Integer.class)).isInstanceOf(StringIntegerTypeTransformer.class);
}

}

@Nested
Expand Down
Loading