refactor: change TypeTransformerRegistry to use a Map instead of Arraylist - #5929
Open
mokhairymahmoud wants to merge 3 commits into
Open
refactor: change TypeTransformerRegistry to use a Map instead of Arraylist#5929mokhairymahmoud wants to merge 3 commits into
mokhairymahmoud wants to merge 3 commits into
Conversation
mokhairymahmoud
added
core feature
refactoring
mokhairymahmoud
changed the title
ndr-brt
reviewed
Jul 31, 2026
| @@ -31,7 +29,7 @@ | |||
|
|
|||
| public class TypeTransformerRegistryImpl implements TypeTransformerRegistry { | |||
| private final Map<String, Class<?>> aliases = new HashMap<>(); | |||
Member
There was a problem hiding this comment.
it's been a while that I see this aliases unused field, since you're here, could you please delete it?
| @@ -0,0 +1,43 @@ | |||
| /* | |||
| * Copyright (c) 2022 - 2023 Bayerische Motoren Werke Aktiengesellschaft (BMW AG) | |||
| @Override | ||
| public void register(TypeTransformer<?, ?> transformer) { | ||
| this.transformers.add(transformer); | ||
| transformers.computeIfAbsent(transformer.getInputType(), key -> new HashMap<>()) |
Member
There was a problem hiding this comment.
I think it could worth to add a warning log in case of transformer override
Contributor
Author
There was a problem hiding this comment.
Already did, thanks.
Comment on lines
+62
to
+81
| 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)); | ||
| } |
Member
There was a problem hiding this comment.
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
Contributor
Author
There was a problem hiding this comment.
Your implementation is cleaner so I applied it. thanks
mokhairymahmoud
force-pushed
the
feat/transformerRegistry
branch
from
August 3, 2026 14:31
d1f66f2 to
705fd59
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR changes/adds
This PR is to change the current TypeTransformerRegistry from an ArrayList to be a Map. This guarantees the existence of only one unique transformer type in the registry and make the transformer selection more deterministic. The PR is the implementation of this DR
Why it does that
The current way of tackling the typeTransformerRegistry is based on ArrayList with a selection mechanism based on findAny() which makes it indeterministic to select a specific transformer specially if there are two of the same type.
Linked Issue(s)
Related to #5921
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.