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 @JsonIgnoreProperties and @JsonPropertyOrder merge (optionally), not just override #82

Open
cowtowncoder opened this issue Feb 10, 2016 · 3 comments

Comments

@cowtowncoder
Copy link
Member

A small subset of array-valued annotations would benefit from ability to append/merge their values.
List includes at least:

  • @JsonIgnoreProperties (default: simple union/merge)
  • @JsonPropertyOrder (default: simple append?)

In these cases, current behavior of full replacement is probably not what is usually the expected behavior.
But additionally we should probably also allow alternate choice of "override"/"overwrite".

One obvious challenge is that since Annotation types can not be instantiated, we may have to figure out whether we could use value object approach (see @JacksonAnnotationValue for details) to help here.

The main challenge here is to combine two important aspects:

  1. Try to keep annotation-merging/inheritance-handling/resolution code annotation agnostic and fully generic (that is, logic need not know semantics of actual annotation tpes)
  2. Fully handle merging so that un-merged set of annotations need not be exposed to calling code.

So: big part of this is to see if merging is practical, and how.

@bjconlan-fc
Copy link

Yes this is particularly important in xml where element ordering breaks any contract of the xsd (as per usage in the jaxb module)

@vetler
Copy link

vetler commented Oct 3, 2019

Recently ran into this problem via FasterXML/jackson-module-jaxb-annotations#66, and was wondering why the proposed fix there couldn't be implemented?

While I understand the need for a better solution of merging annotations, the proposed fix wouldn't make this worse, as it only applies to XmlType and JaxbAnnotationIntrospector.findSerializationPropertyOrder which doesn't take any of the other annotations into account anyway.

If this would be acceptable as an interim solution, I'd be happy to implement it.

@cowtowncoder
Copy link
Member Author

The reason I did not accept proposed fix is because it is against design of AnnotationIntrospector: it should not handle flattening of annotations or traversal; that is what processing before AI should do, if it is to be done. AI does not -- for example -- have knowledge of mix-in annotations; nor would it handle precedence of chained AIs properly.
I do not want one-off implementations for each annotation type, and AnnotationIntrospector implementation, as I will be responsible for maintaining these changes, and problems that may arise from compatibility with other functionality. This is wider area than what is of concern to original author who needs to just consider their use case (this is not meant as critique at all, just explaining difference in responsibilities).

So if merging was to be implemented -- something that is, I think, a good idea -- I was thinking it should be done by code in AnnotatedClass (and related), using ideally meta-annotations to guide the logic, so that it would not have to know semantics of the annotation types. The best time to do this would be for 3.0.0 version (master branches of Jackson components).
There is already quite a bit of logic for resolving annotations in hierarchy, so I think more general approach would just include thinking of possible meta-annotations to add (to specify which Jackson annotations are mergeable, and if there are multiple ways to merge, mode of merging), and the modify logic.

This is not the only way to solve the problem, and I am open to other proposals. It just should be something more generic than specific annotation type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants