TypeFactory type resolution broken in 2.7 for generic types when using constructType with context

[Spikhalskiy]

Hi,

Spring uses TypeFactory#constructType to resolve entity class to deserialize into in case of controllers inheritance with Generics on base classes.

I made simplified version and localized issue in provided simple test, which is a naive simulation of what's Spring does.
This test is broken in 2.7.x, 2.8.x, master. Works fine in 2.5.x and 2.6.x.

    public static class BaseController<Entity extends BaseEntity> {
        public void process(Entity entity) {}
    }

    public static class ImplController extends BaseController<ImplEntity> {}

    public static class BaseEntity {}

    public static class ImplEntity extends BaseEntity {}

    @Test
    public void test() throws NoSuchMethodException {
        Method proceed = BaseController.class.getMethod("process", BaseEntity.class);
        Type entityType = proceed.getGenericParameterTypes()[0];

        JavaType resolvedType = new ObjectMapper().getTypeFactory().constructType(entityType, ImplController.class);
        assertEquals(ImplEntity.class, resolvedType.getRawClass());
    }

So, since 2.7 resolvedType.getRawClass() contains BaseEntity.class, which is incorrect with the provided context.

[cowtowncoder]

@Spikhalskiy unfortunately usage in the test is not quite how type resolution is designed to work: all method/field types must be resolved by starting with class (or parameterized type) that contains them, and only this resolution is designed to produce proper type bindings.
While other methods are accessible they are not meant as public API.

There are two possibilities here:

1. Resolving parameter type for process() would work properly if resolving ImplController and finding method process (that is, difference between test and expected usage)
2. Resolving would fail similarly in both cases

I can try modified version just to verify which way it goes.

In case (2) this obviously should be fixed; but (1) is more difficult and there's no guarantee. If it may still be fixed that'd be good of course. Just that there may be separate issues.

[Spikhalskiy]

@cowtowncoder Problem is here https://github.com/spring-projects/spring-framework/blob/4.0.x/spring-web/src/main/java/org/springframework/http/converter/json/MappingJackson2HttpMessageConverter.java#L281

So, if implement the same controllers structure as described in this topic using Spring controllers - getJavaType would get Entity generic parameter of the BaseController (which has actual request mapping) as a type and ImplController as a contextClass. As a result, process method of ImplController during call from Spring gets incompatible entity with new Jackson version - BaseEntity instead of ImplEntity as expected and as it works with Jackson < 2.7.

This version is not EOL. And more modern versions have same code actually: https://github.com/spring-projects/spring-framework/blob/4.1.x/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java#L278
And even the last one:
"The default implementation returns {@code typeFactory.constructType(type, contextClass)}"
https://github.com/spring-projects/spring-framework/blob/4.3.x/spring-web/src/main/java/org/springframework/http/converter/json/AbstractJackson2HttpMessageConverter.java#L313

So, Spring relies on this specific method behavior and behavior changed in 2.7.x branch.
Maybe we can move the current behavior to a private method and use internally and back the original behavior to this public constructType?
So... shorter question - how to make Spring works fine with 2.8 in this aspect as it works with 2.6 and below :)

Read your answer mostly as "now thing works as designed", sorry for too many questions if instead, you think it should be fixed to work with Spring correctly.
Let me know if you need something from me, like a reproducable example using Spring integration. I just tried made a localized version to highlight behavior change.

[cowtowncoder]

@Spikhalskiy Ok, I added another test method to ensure that type resolution itself works correctly; it does.

Now, let me first explain what is happening, and why result does not work.
The problem is that typeBindings specify bindings of type parameters at specific level in type hierarchy: when passing ImplController, there are no bindings because it is not a generic type itself. But more importantly, it is not the class where process() method is defined: that would be BaseController.
Yet you also can not construct JavaType for BaseController either: that is generic but does not have type bindings. The way test would pass is like so:

        JavaType contextType = MAPPER.constructType(ImplController.class);
        // actually declared in base type so...
        contextType = contextType.getSuperClass();
        JavaType resolvedType = MAPPER.getTypeFactory().constructType(entityType, contextType);

and this gives the expected answer, as typeBindings are ones that apply for that method within that type hierarchy.

So why did this work in 2.6 and earlier? Because back then, TypeBindings were constructed over the whole type hierarchy, sort of globally. So declaration at higher level (for ImplController) would contain embedded bindings from the parent. This would have worked here; but also causes issues with type aliasing (where same name gets mapped to different type at different level in type hierarchy).
So although behavior for this case would work out, this is sort of accidental: bindings as passed are from wrong level, but since they "leak(ed)" it works out in the end.

With that explanation, let me re-read your comment to see what could be done here. :)

[cowtowncoder]

@Spikhalskiy Ok. Yes, I understand that breakage in behavior is unfortunate and not Spring's fault. Unfortunately rewrite of type resolution system was difficult and not entirely painless. On plus side it did fix a few long-lived bugs related to type aliasing; on downside there are some usages that can not be supported exactly as is.

Now... conceptually, what is needed is to find the JavaType of class that actually declares the method.
It should be possible to resolve the outermost type (ImplController.class), and traverse super-class(es) until finding one with raw type of class that declared method of which type(s) are to be resolved.

I don't know if there is enough information to do that; this is problem with many frameworks. Unfortunately java.lang.reflect.Type without context is not reliably resolvable, without knowing where it came from.

One other alternative that I can play with a bit is to see if -- just for these two deprecated methods -- it'd be possible and reasonable to see if one of parent types have non-empty TypeBindings, and if so, use that instead, if initial type has empty bindings. That would most likely work better, and pass this particular test?

[cowtowncoder]

@Spikhalskiy Ok. I think hack I added for 2.7.9 / 2.8.6 should help a lot. Going forward new code really should not use these methods, but methods themselves are simple enough to keep until 3.0. Not sure what should be done at that point, since these methods can not be made to work much better (as per earlier explanation), but at the same time since many/most(/all?) Java web service frameworks still pass incomplete information despite it being a well-known issue this is insufficient. Given this maybe such methods are still necessary...

Anyway: thank you again for all your help!

[Spikhalskiy]

@cowtowncoder Thanks a lot for the fix, Tatu! Will check nightly build when get a chance, if the old spring behavior is back. Yep, would be great to keep it until the major release when Spring team will revisit/reimplement integration.

[cowtowncoder]

@Spikhalskiy yea. It might be worth seeing if the work-around I added might be done by caller instead, because that way previously released 2.7.x / 2.8.x versions would also work.

Logic is pretty simple; if Type being passed is not Class, and if TypeBindings for resolved context are empty, try to see if super-class (and in theory maybe super-interface... didn't go there yet, more complex) has non-empty, and if so, use that. This does not guarantee correct resolution but increases chances a lot for common case.