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

Annotations are lost in the case of duplicate methods #868

Merged

Conversation

christophercurrie
Copy link
Member

This, I believe, is the root cause of a number of the "inconsistent behavior" bugs in the Scala module. For background, a normal Scala class:

class Foo(val bar: Int)

will have a single accessor method visible to the JVM, bar(). ScalaAnnotationIntrospector will report an implicit name of "bar" for this method, which enables its use as a a getter.

Scala provides an annotation for end users to provide compatibility for libraries that are not Scala aware:

class Foo(@scala.beans.BeanProperty val bar: Int)

This class will have two accessors, bar() and getBar(). The first will be reported and enabled as above, and the second will be detected by Jackson's normal property detection. They'll be merged into the same property, and the duplicate resolution code in POJOPropertyBuilder#getGetter will resolve the duplication and discard the extras.

The error is in merging annotations between the duplicates, and in merging annotations from creators into the AnnotatedMethod instances that represent them. The problem with the current code is that it only merges annotations from and into the last detected getter (the head of the linked list). Since reflection method order is non-deterministic in JVM 7+, which of the duplicates gets the annotations merged in is essentially random, though various platforms demonstrate tendencies in one direction or the other.

In the "happy" case, getBar is detected last; since it's at the front of the linked list, it will get receive the annotations from the creator. getBar is considered "better" by getGetter and is retained, so the annotation is visible to code looking for it.

In the "unhappy" case, bar is detected last; it will receive the annotations from the creator, but getBar will not. getBar will be chosen by getGetter, and so the creator annotations will be lost, and code looking for it will fail to do so.

The fix covers this use case, and also covers the additional use case that annotations on bar and getBar were not mutually visible.

The tests use 'isBar' to simulate the implicit naming that occurs in the Scala module. The linked list class also needed to be made protected to be visible to the tests.

@cowtowncoder
Copy link
Member

Ouch.

Ok, it is great that the root cause is now known and especially to have a fix for the problem.
I have some concerns on solution (perhaps since I haven't fully checked out the code flow, or misremember soemthing), but given timing I think it's best that I merge this. Introspection will need to be rewritten substantiatlly for 2.7 again, to support earlier merging of annotations for creators.

My specific concern I think is just that the most straight-forward (conceptually) fix would seem to be to ensure proper ordering of accessors, and maybe do pruning and merging only after full pruning.
It seems like this fix could still leave ordering of annotations undefined, if there is an overlap, although it would not drop them

So, anyway, great detective work, and I think this happened just in time :)

cowtowncoder added a commit that referenced this pull request Jul 17, 2015
Annotations are lost in the case of duplicate methods
@cowtowncoder cowtowncoder merged commit 4333eb6 into FasterXML:master Jul 17, 2015
cowtowncoder added a commit that referenced this pull request Jul 17, 2015
@VinRoh
Copy link

VinRoh commented Jan 31, 2017

I have the same problem but with @JasonRootName annotation .This is happening on a highly critical Java project. The response of RestService is a POJO ServiceResponse.java with the @JasonRootName(value=serviceResponse). As expected most of the time the json response returned is correct as "serviceResponse" but fails once in 50 deployments and send the Json response as "ServiceResponse" which is the class name. The Jackson version used is 2.6.1 . Is there any issue with the JacksonAnnotationIntrospector.java and AnnotationClass.java under the com.fasterxml.jackson.databind.introspect package??

@cowtowncoder
Copy link
Member

@VinRoh despite superficial similarities I don't think this is likely related to your issue. There are no known issue with annotation introspector (it is stateless), but theoretically lazy loading in AnnotatedClass could be related.
Thank you for filing the issue for jackson-databind which is directly relevant.

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

Successfully merging this pull request may close these issues.

None yet

3 participants