Revise AbstractJackson2HttpMessageConverter's generic type adaptation [SPR-13728]

[spring-projects-issues]

Juergen Hoeller opened SPR-13728 and commented

Jackson 2.7 (currently in RC1) has breaking changes in its TypeFactory / TypeBindings API which break our AbstractJackson2HttpMessageConverter's generic type adaptation. Let's sort this out for Spring Framework 4.3, while retaining compatibility with Jackson 2.6+.

---

Affects: 4.2.3

Reference URL: https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.7#fix-generic-type-resolution-mechanism

Issue Links:

• Full support for Jackson 2.7 [SPR-13483] #18062 Full support for Jackson 2.7 ("is depended on by")
• Require Jackson 2.6+, FreeMarker 2.3.21+, XStream 1.4.5+ [SPR-13062] #17654 Require Jackson 2.6+, FreeMarker 2.3.21+, XStream 1.4.5+
• Jackson 2.6.x -> 2.7 deserialize generic fails when calling Spring controller [SPR-14470] #19039 Jackson 2.6.x -> 2.7 deserialize generic fails when calling Spring controller
• Make @ResponseBody method return type available to message converters [SPR-12811] #17408 Make @ResponseBody method return type available to message converters
• Improve Jackson 2.7 compatibility [SPR-13853] #18426 Improve Jackson 2.7 compatibility
• Revise generic type handling in GsonHttpMessageConverter [SPR-15381] #19944 Revise generic type handling in GsonHttpMessageConverter

Referenced from: commits a730e55

[spring-projects-issues]

Juergen Hoeller commented

Sébastien Deleuze, we have a few reports in the meantime which individually point out the incompatibility with Jackson 2.7. Let's prioritize this and aim for early availability in 4.3 snapshots...

Juergen

[spring-projects-issues]

Sébastien Deleuze commented

I have made some tests but this is not obvious to me how to achieve the same thing with Jackson 2.7 for some parameterized controller use cases (like the one tested in HttpEntityMethodProcessorTests#resolveArgumentTypeVariable), so I have asked for some feedbacks on Jackson bugtracker.

[spring-projects-issues]

Sébastien Deleuze commented

The removal of TypeFactory#constructType(Type type, Class<?> contextType) was accidental, it will be back in Jackson 2.7.1 (deprecated) as detailed here. That will avoid fatal startup breakage with Spring 4.2 and previous releases.

That said, in Spring 4.3 I plan to update our current implementation of AbstractJackson2HttpMessageConverter#getJavaType() to use the new API when Jackson 2.7+ is used, to get more accurate type resolution and avoid using the deprecated API.

[spring-projects-issues]

Sébastien Deleuze commented

This draft commit fixes our remaining failing unit tests, and I have asked for a feedback to Jackson lead developer. Jackson 2.6 and 2.7 have different API for type resolution, so I wrapped version specific code in private classes.

We need to wait the upcoming Jackson 2.7.1 release in order to be able to compile the Jackson 2.6 specific code.

[spring-projects-issues]

Juergen Hoeller commented

Sébastien Deleuze, it looks like we could simply process the TypeBindings code versus the old constructType call in an if/else block within our getJavaType method, with no need for inner classes. Checking the boolean flag for the code path to use should be good enough since all the referenced types exist in both versions of Jackson; non-existing methods are not a problem from a bytecode verification perspective.

Juergen

[spring-projects-issues]

Sébastien Deleuze commented

Juergen Hoeller Thanks your detailed feedback, I was not sure. Glad this is possible, that will make the change less intrusive.

[spring-projects-issues]

Sébastien Deleuze commented

It seems we are going to need to revisit our AbstractJackson2HttpMessageConverter#getJavaType() implementation to manage ourself some of the type resolution logic as described by Jackson lead developer in his last feedback. I am going to experiment on that and will post a comment here with an updated commit to review.

[spring-projects-issues]

Sébastien Deleuze commented

I had more thoughts about this issue and as described by Tatu Saloranta, it seems that like in Jackson case, with the current parameters Type type, Class<?> contextClass we don't have enough information to resolve in a reliable way the JavaType in all cases. In the test I mention, it is used to resolve a TypeVariable coming from a method parameter, so in order to resolve that in a reliable way, we would need the specific MethodParameter context parameter ...

That said, the contextClass parameter may be used by implementers to resolve not only TypeVariable from method parameters, and even more important it is not only exposed in AbstractJackson2HttpMessageConverter#getJavaType() but also in GenericHttpMessageConverter methods. So given the impact of such refactoring, maybe that's fine enough to implement in AbstractJackson2HttpMessageConverter#getJavaType() something that does the same thing than Jackson 2.6 implementation of TypeFactory#constructType(Type type, Class<?> contextType) did, even if this solution is theoretically not perfect.

If we chose to keep our current API and behavior, I will update the implementation of my draft commit, since it currently breaks some use cases. It will have to go through the various inheritance level to find the right information I guess.

Any thoughts about that Juergen Hoeller?

[spring-projects-issues]

Juergen Hoeller commented

Sounds alright to me, Sébastien Deleuze. We didn't have an actual issue with type resolution on our end, and we're dealing with a rather narrow case: Spring MVC handler method arguments. So from my perspective, we should be fine with an algorithm that mimics what Jackson 2.6 did; ideally without calling any deprecated methods.

Juergen

[spring-projects-issues]

Sébastien Deleuze commented

I have implemented in this updated draft commit something close to what jackson 2.6 did for TypeVariable, but using our own Spring based implementation. Please notice that we don't need to detect Jackson version anymore since we now always uses the Spring TypeVariable resolution algorithm.

It works fine with our current tests but I would be interested by your feedback Juergen Hoeller, since ResolvableType API and this kind of advanced generic type handling are not my area of expertise. As you suggested initially, it will be interesting to ask feedbacks from our users since there is clearly a risk for regressions here.

[spring-projects-issues]

Juergen Hoeller commented

Sébastien Deleuze, this looks alright for a start. We might roll that algorithm into a dedicated method on ResolvableType itself even; I'll have a look at it from that perspective (under a new JIRA ticket).

In any case, let's get it into master as-is and see whether we have any integration test failures. And for 4.2.x, I suppose we'll simply stick with our existing code, expecting it to work with Jackson 2.7.1+?

Juergen

[spring-projects-issues]

Sébastien Deleuze commented

Indeed, for 4.2.x, Jackson 2.7.1+ will avoid the blocking error we see with 2.7.0, but it is important to notice that TypeVariable may not work as in 2.6 and previous version. Given the fact that 4.2.x does not support Jackson 2.7, and that our new implementation my cause some regression, I think keeping 4.2.x as it is is the best we can do.

This commit has been merged in master.