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

Fix introspector chaining in JaxbAnnotationIntrospector.hasRequiredMarker() #32

Closed

Conversation

vojtechhabarta
Copy link

Let's say I have a property marked with @JsonProperty(required = true).
I also used objectMapper.registerModules(ObjectMapper.findModules(classLoader)) to register modules but now the property is not considered "required".

That's because there is a chain of JaxbAnnotationIntrospector and JacksonAnnotationIntrospector (in this order) and JaxbAnnotationIntrospector.hasRequiredMarker() returns Boolean.FALSE so JacksonAnnotationIntrospector is not called (in AnnotationIntrospectorPair).

This PR fixes this case, JaxbAnnotationIntrospector.hasRequiredMarker() now returns null so property can be processed by subsequent introspector.

This PR also changes that JaxbAnnotationIntrospector.hasRequiredMarker() now authoritatively returns Boolean.FALSE when annotation is present. This behavior is consistent with JacksonAnnotationIntrospector.

- fixed introspector chaining
- when annotation is present authoritatively return false
@cowtowncoder
Copy link
Member

Looking at code, makes sense. Looks like return values were switched around.
The original concern was probably due to false being the default (since annotation values do not have "undefined" or null possibility). But returning Boolean.FALSE as fallback is wrong no matter what.

One practical note: since I want this to be in 2.9.3, I'll manually merge changes (master is for 3.0.0).

@cowtowncoder cowtowncoder changed the title Fix introspector chaining in JaxbAnnotationIntrospector.hasRequiredMarker() Fix introspector chaining in JaxbAnnotationIntrospector.hasRequiredMarker() Oct 18, 2017
cowtowncoder added a commit that referenced this pull request Oct 18, 2017
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.

2 participants