Skip to content
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.

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

Make the content registered with appendAnnotationIntrospector work correctly as a fallback. #3758

Closed
k163377 opened this issue Jan 28, 2023 · 7 comments
Labels
to-evaluate Issue that has been received but not yet evaluated

Comments

@k163377
Copy link
Contributor

k163377 commented Jan 28, 2023

Is your feature request related to a problem? Please describe.
AnnotationIntrospector registered with appendAnnotationIntrospector is expected to be called after all higher priority AnnotationIntrospectors have been processed.
For example, if a JsonCreator is present in the class, the findCreatorAnnotation function is not expected to be called.

On the other hand, in the current situation, all AnnotationIntrospectors will try to process one target.

Describe the solution you'd like
Ensure that AnnotationIntrospectors registered with appendAnnotationIntrospector are called after all AnnotationIntrospectors with higher priority have been processed.

Usage example
By changing the behavior in this way, processing related to the case where nothing is specified can be implemented.
In the findJsonCreator function example, such an implementation currently requires checking that the class does not contain any other JsonCreator.
Also, it is not possible to implement the function in such a way that it does not overwrite a JsonCreator that is specified with a higher priority by another module.

Additional context
This would require a huge change, such as eliminating AnnotationIntrospectorPair.
Probably not possible to achieve this until Jackson 3.0.

@k163377 k163377 added the to-evaluate Issue that has been received but not yet evaluated label Jan 28, 2023
@cowtowncoder
Copy link
Member

I don't quite understand suggested problem here. AnnotationIntrospectorPair handles calls to lower priority introspectors based on results from higher priority ones; merging information if necessary (and if possible). This is done on per-annotation basis (one by one).

Also keep in mind that although name of class is AnnotationIntrospector, the idea is that information used may come from alternate sources, not just annotations.

Would it be possible to show an example of specific problem?

@k163377
Copy link
Contributor Author

k163377 commented Jan 28, 2023

This is the case for the following implementation(KotlinNamesAnnotationIntrospector is registered by appendAnnotationIntrospector)
https://github.com/FasterXML/jackson-module-kotlin/blob/062176cbf53e6a60b1a1d9080708a4c168b6a17a/src/main/kotlin/com/fasterxml/jackson/module/kotlin/KotlinNamesAnnotationIntrospector.kt#L81-L88

This process is intended to "use the Kotlin Primary Constructor if no other JsonCreator is specified.
However, this implementation has a problem that it overrides the Creator specified by any other method than using the JsonCreator annotation.
I don't think this problem can be solved with the current Jackson mechanism.

Also, I personally feel that the findCreatorAnnotation function should not be called in such cases.

@cowtowncoder
Copy link
Member

@k163377 I think Kotlin module is really abusing expected usage of AnnotationIntrospector: method really should not use any complicated logic but simply indicate if marker annotation (or its equivalent) is seen. It may be that changes would be needed for constructor detection in jackson-databind and so on, but I do not think trying to add complex logic in AnnotationIntrospector should be done.

Kotlin module has often added elaborate (but not always sensible or maintainable) workarounds instead of trying to work in improving introspection with databind. Same sometimes happens with Scala module, but to a lesser degree.
Both of these do need more advanced handling than Java, although with Java 17 Record type this is changing a bit.

@k163377
Copy link
Contributor Author

k163377 commented Feb 4, 2023

I think this is not a kotlin-module specific problem, but a JsonCreator handling design issue.
The current design has the following problems

  • Cannot prioritize between OriginalMarker and JsonCreator.
    • Even when simply detecting annotations, the priority level changes depending on whether the annotation is given to the constructor or the factory function.
  • It is difficult to strictly apply the constraint that JsonCreator is limited to one per class.

Personally, I think it would be more correct if this process were done on a per-class basis.

(I agree that the scope of benefit from this problem being solved is limited.)

@cowtowncoder
Copy link
Member

@k163377 I am sorry but I really do not understand what OriginalMarker means here; or why you think JsonCreator was not processed on per-class basis. AnnotationIntrospector is only called to get markers on creators (constructors, factory methods) and it's then up to caller to figure out the whole picture. It is not up to AnnotationIntrospector to do ANY processing combining different annotations or accessors. That's up to Bean[De]SerializerFactory or BasicBeanDescription. Their handling gets complicated to be sure, and logic for Java handling differs from one needed by Kotlin module.

But I think one mistake Kotlin module may be doing is to do much in its AnnotationIntrospector implementation: those methods really should absolutely not try to access information across different methods and fields, but only on specific one being asked about.

@cowtowncoder
Copy link
Member

I don't think approach here is doable; will close. I think problems (which are real for sure) need to be addressed differently, possibly changing AnnotationIntrospector implementations and/or usage.

@yihtserns
Copy link
Contributor

yihtserns commented Jun 20, 2024

FYI, I think #4584 will address this issue. So once that is done, I think most of the things in KotlinNamesAnnotationIntrospector will no longer be necessary. 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to-evaluate Issue that has been received but not yet evaluated
Projects
None yet
Development

No branches or pull requests

3 participants