Allow EntityMetamodel to be decorated without breaking ID resolution - #4772
Conversation
hjohn
added
Priority 2: Should
| configuration.getComponent(MessageConverter.class), | ||
| new AnnotationBasedEntityIdResolver<>() | ||
| ); | ||
| return new AnnotationBasedEntityIdResolver<>(); |
There was a problem hiding this comment.
This class is basically now 100% indentical to AnnotationBasedEntityIdResolverDefinition -- I guess we could remove one of them...
32eaea1 to
15a4697
Compare
smcvb
left a comment
smcvb
left a comment
There was a problem hiding this comment.
Bunch of nits, as always. Biggest pointer is the removal. I think we should deprecate it instead with a clear explanation why. Other than that, I see how this will resolve #4765 without the additional interface, which is great!
| * @author Mitchell Herrijgers | ||
| * @since 5.0.0 | ||
| */ | ||
| public class AnnotatedEntityIdResolver<ID> implements EntityIdResolver<ID>, DescribableComponent { |
There was a problem hiding this comment.
I'd rather have us mark this as @Deprecated since 5.3.0, with a bit of JavaDoc attached to it why.
There was a problem hiding this comment.
It is deprecated now :)
| configuration.getComponent(MessageConverter.class), | ||
| new AnnotationBasedEntityIdResolver<>() | ||
| ); | ||
| return new AnnotationBasedEntityIdResolver<>(); |
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| @ExtendWith(MockitoExtension.class) | ||
| class AnnotatedEntityIdResolverTest { |
There was a problem hiding this comment.
If we don't remove the class but deprecate it instead, I think we should keep the test as well.
| */ | ||
| <E, ID> EntityIdResolver<ID> createIdResolver( | ||
| Class<E> entityType, | ||
| Class<ID> idType, | ||
| AnnotatedEntityMetamodel<E> entityMetamodel, | ||
| EntityMetamodel<E> entityMetamodel, |
There was a problem hiding this comment.
Although an API change, I think this change is fair to make. I mean, any AnnotatedEntityMetamodel anybody might have passed was already an EntityMetamodel implementation. Thus any custom EntityIdResolverDefinition a user may have created (although I doubt it that this happened) that did expect the AnnotatedEntityMetamodel could still cast themselves to resolve it.
There was a problem hiding this comment.
Yeah, if anyone did implement it, they would have to change their signature, but that would be a trivial fix.
15a4697 to
f284a21
Compare
smcvb
left a comment
smcvb
left a comment
There was a problem hiding this comment.
My concerns have been addressed, hence I'm approving this pull request.
2 participants
Problem
AnnotatedEventSourcedEntityModulecontained a hard cast toAnnotatedEntityMetamodel<E>when building theEntityIdResolver. This made theEntityMetamodelcomponent impossible to decorate or replace: any plain wrapper would cause aClassCastExceptionat startup. The root cause was thatEntityIdResolverDefinition.createIdResolverrequiredAnnotatedEntityMetamodel<E>as a parameter, leaking an annotation-specific concrete type into what should be a generic SPI.Solution
Representation info (the mapping from message
QualifiedNameto expected payloadClass) is now sourced from theAnnotatedEntityMetamodelbuilt and cached byAnnotatedEventSourcedEntityModuleitself, rather than from whatever component ends up registered in configuration. A newRepresentationConvertingEntityIdResolverwraps the delegate resolver and applies payload conversion using this cached provider before delegating ID extraction. TheEntityMetamodelcomponent in configuration is passed toEntityIdResolverDefinition.createIdResolveras the plainEntityMetamodel<E>interface — no cast required — and sinceAnnotatedEntityIdResolverDefinitiononly needs to perform annotation-based ID extraction (the conversion step having moved to the wrapper), it simplifies to returning anAnnotationBasedEntityIdResolverdirectly.Impact
EntityIdResolverDefinition.createIdResolvernow acceptsEntityMetamodel<E>instead ofAnnotatedEntityMetamodel<E>, which is a minor breaking change for any custom implementations of the interface.AnnotatedEntityMetamodelis marked@Internalas it is no longer part of the public API surface.AnnotatedEntityIdResolveris deprecated as its responsibility has been absorbed by the new wrapper class.