search

FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.53k stars 1.38k forks source link

Add @Inherited annotation to all Jackson annotations #198 #3408

Open emmersonbonnat-tw opened 2 years ago

emmersonbonnat-tw commented 2 years ago

We migrated to Micronaut 3.3.3 from 2.x recently and since Micronaut 3.0 there is a breaking change in Annotation Inheritance. Due to this the Jackson annotations that are not annotated with @java.lang.annotation.Inherited are not inherited from parent classes and interfaces.

Can you add @inherited annotation to all Jackson annotations?

cowtowncoder commented 2 years ago

This could go in 2.14; not sure how much time I personally have but could help with PRs; meaning "contributions welcome".

Note that there are annotations both in the main annotations package (jackson-annotations) and some here within jackson-databind. Also it's probably worth considering whether there are annotations that should NOT be marked as inheritable -- I suspect some would be.

My only concern is whether there might be regressions due to handling by other frameworks. I don't know if there is any way to check this.

Other than that no objections: Jackson itself does not care about this annotation so behavior would not be changed.

yawkat commented 2 years ago

@cowtowncoder I would think it would have an impact on jackson, because @Inherited affects the behavior of the normal reflection getAnnotation

cowtowncoder commented 2 years ago

@yawkat I may be wrong here but since Jackson handles inheritance on its own it uses "bulk" access methods for Class annotations (for which @Inherited may be considered) which I think do not process @Inherited. Direct getAnnotation() is only used for Members (Fields, Methods, Constructors), and for those I think @Inherited is not processed in any way (which is a major reason why Jackson had to handle inheritance on its own in the first place).

But if this did have an effect the main concern would be mix-in processing which could definitely be broken.

I guess whoever worked on this would need to add non-trivial testing for Class annotations wrt mix-ins to ensure there is no regression.

yawkat commented 2 years ago

Ah that makes sense.

There are definitely some micronaut bugs in this area also, since we introspect the annotations ourselves too, but don't follow jacksons annotation inheritance behavior properly. It may be safer to fix this on the micronaut side of things.

(I don't know about other frameworks)

rearl commented 2 years ago

What is the status of this issue? Is it still planned for 2.14?

We'd like to see @Inherited at least on @JsonFilter and it would be nice to have it on@JsonIgnore. In general, I think it would make sense to have @Inherited on all annotations possible.

cowtowncoder commented 2 years ago

@rearl The issue is the fact that Jackson itself has no benefit from this at all (inheritance is implemented explicitly without regards to this annotation, to support field/method/constructor annotation inheritance, not just class ones); but the potential benefits and risks are for other libraries/frameworks... and we don't have good ways to really gauge that part at all.

So no current plans unless someone steps up and:

  1. Proposes changes (PR against jackson-annotations and/or jackson-databind)
  2. Tests with some popular framework/library to show benefits (and explain those in PR)

I do not have either time or itch for doing this: I am not against it, but if this is to be done, someone else needs to be driving the change. My main concern is that there could be potential downsides if some frameworks (say, Swagger generators?) were not counting on getting inheritance.

Timing-wise it ideally would have done before 2.14.0-rc1, but if it was done really quickly now, I am thinking of 2.14.0-rc2 within a week or two.