Added ODM Metadata Inheritance fixes #340 #591
Conversation
|
@Rob4001 Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@Rob4001 Thank you for signing the Contributor License Agreement! |
| Entry entity = clazz.getAnnotation(Entry.class); | ||
| if (entity != null) { | ||
| // findAllMergedAnnotations will return set of inherited annotations and apply them from superclass down to subclass | ||
| Set<Entry> entities = AnnotatedElementUtils.findAllMergedAnnotations(clazz,Entry.class); |
There was a problem hiding this comment.
While I agree that we want to resolve inheritance and meta-annotation concerns, I don't think we want to merge multiple Entry annotations. The reason is that each Entry can come with a base and it's not clear which should be picked.
Instead, it may be valuable to look at Spring Security's AuthorizationAnnotationUtil#findUniqueAnnotation for some inspiration on how to resolve the @Entry annotation while still erroring if the result is ambiguous.
There was a problem hiding this comment.
There is a use case where say you have a inetOrgPerson class which inherits from a organizationalPerson class which inherits from a person class. If you want to reference inetOrgPersons in one query, but person in another they could have different Entry annotation values. By enumerating through all annotations from the top of the class tree and overriding with subclasses it still allows this behavior.
There was a problem hiding this comment.
I understand the use case for filters, but the base value remains ambiguous. Which base should you pick? More importantly, how do you reconcile the fact that you pick a base from one @Entry but all the filters from all @Entry annotations?
There was a problem hiding this comment.
Sorry for the long absence. I would argue that the default should be to take the lowest class in the class inheritance tree (closest to the instantiated class type) with the ability for the class writer to disambiguate by defining the base on the subclass.
There was a problem hiding this comment.
lowest class
I agree with this, and I think it should apply consistently for all of Entry's properties. Instead of picking the lowest base and merging all objectClasses in the type hierarchy, the code should pick the lowest Entry and use its base and objectClasses.
with the ability for the class writer to disambiguate by defining the base on the subclass.
This should also apply for objectClasses. Otherwise, it is tricky for the subclass to override objectClasses in the superclasses.
There was a problem hiding this comment.
I don't want to get hung up on this feature, and it merits further discussion. Could we create a separate ticket to discuss how objectClasses inheritance should work? I'm happy to add to that ticket my thoughts and we can continue the conversation over there.
For this ticket, can you please leave inheritance as follows:
- The target class must have an
@Entryannotation or meta-annotation - No parent
@Entryannotations are considered - All reachable fields are collected (what you are already doing here)
The reason for keeping 3 is that it's clear from the ticket that not collecting all reachable fields is the thing that is primarily surprising to folks.
I'm also happy to update the PR with a polish that does the above if you aren't available. If I don't hear from you within a month or so, I'll proceed as I've described so that it can land in time for the 3.3.0 release.
| // There can be only one id field | ||
| throw new MetaDataException( | ||
| String.format("You man have only one field with the %1$s annotation in class %2$s", Id.class, clazz)); | ||
| ReflectionUtils.doWithFields(clazz, new ReflectionUtils.FieldCallback() { |
There was a problem hiding this comment.
It's not clear to me the purpose of this change to use doWithFields. Is this change necessary to address ODM inheritance?
There was a problem hiding this comment.
If I recall correctly, clazz.getDeclaredFields(); will only return fields declaired directly in the class and not any inherited fields. the "doWithFields" method will recurse through the type inheritance tree to get all fields annotateed with spring ldap annotations.
https://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#getDeclaredFields--
core/src/main/java/org/springframework/ldap/odm/core/impl/ObjectMetaData.java
Show resolved
Hide resolved
Fixes #340 by traversing field and annotations on ODM metadata generation