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

Provide/document alternative to TypeFactory#constructType(Type type, Class<?> contextType) in 2.7 #1087

Closed
sdeleuze opened this issue Jan 18, 2016 · 8 comments
Milestone

Comments

@sdeleuze
Copy link

In Spring Framework 4.2 and previous versions, we are using TypeFactory#constructType(Type type, Class<?> contextType) that is now removed in Jackson 2.7, and using TypeFactory#constructType(Type type) is not enough since it doesn't work for some of our use cases with parametrized Spring MVC controllers.

Is there an alternative to TypeFactory#constructType(Type type, Class<?> contextType) in Jackson 2.7? The code for this method is still there but commented, any chance to restore it in 2.7.1 to avoid breaking all applications using Spring Framework 4.x + Jackson 2.7?

@cowtowncoder
Copy link
Member

Removal was accidental, as per #1079; I will add it back as deprecated for 2.7.1. This is the way it should have been in 2.7.0. Apologies for that.
One caveat: the operation of the method may not work exactly the way as previously, unfortunately, due to changes in type resolution itself. So: this will avoid immediate (and catastrophic) startup breakage, but there are benefits to upgrading code that was using the method.
It might be possible to improve on this handling, if you find problems; if so, please add notes here. It would be good to avoid unnecessary breakages to allow some amount of flexibility wrt Jackson databind versions in use.

Now, as to replacement method... the problem with method was that the full type resolution context can not be fully created with given parameters. For specific case of method parameter or return types, or field types, what is needed is the type of declaring Class, and with that, JavaType.getBindings() may be called to get bindings to pass to constructType(Class, TypeBindings).
Alternatively it is possible to do full introspection for [Basic]BeanDescription, and then AnnotatedMethod and AnnotatedField will give direct access to resolved JavaType (since they retain references to JavaType of declaring class).

Let me know if you have trouble figuring out replacement(s); this is bit tricky area. But on plus side, type resolution should FINALLY work the way it should always have -- there are no open issues related to type variable aliasing or other resolution issues. So I don't anticipate breaking changes in this area for future minor releases.

@sdeleuze
Copy link
Author

Thanks for your feedback, and thanks for add it back in 2.7.1 as deprecated.

For the replacement method (since our goal in Spring 4.3 is to avoid using deprecated methods when Jackson 2.7 is used), I have changed:

protected JavaType getJavaType(Type type, Class<?> contextClass) {
    TypeFactory typeFactory = this.objectMapper.getTypeFactory();
    return typeFactory.constructType(type, contextClass)
}

To

protected JavaType getJavaType(Type type, Class<?> contextClass) {
    TypeFactory typeFactory = this.objectMapper.getTypeFactory();
    Type superclass = (contextClass == null ? null : contextClass.getGenericSuperclass());
    TypeBindings bindings = (superclass == null ? TypeBindings.emptyBindings() :
        typeFactory.constructType(superclass).getBindings());
    return typeFactory.constructType(type, bindings);
}

It seems to work well in our unit tests (I will do more tests with various use cases). Is it what you had in mind in you first proposal?

@cowtowncoder
Copy link
Member

@sdeleuze close; I think resolving of contextClass directly into JavaType would work better, although it really depends on where type is actually declared. Context to give should be class in which thing (method, field) that type is associated with is declared, but due to generic parameters, that class could be one of super types of contextClass.

Anyway, if code you have works fine, it is probably reasonable thing to use.

@sdeleuze
Copy link
Author

In fact, I initially tried resolving directly contextClass into a JavaType but it did not work in my test, so there is maybe something to polish in the new type resolution mechanism, since it worked in previous versions with TypeFactory#constructType(Type type, Class<?> contextType).

To give you more details, here is my use case. I have the following controller classes:

public abstract class MyParameterizedController<DTO extends Identifiable> {
    public void handleDto(HttpEntity<DTO> dto) {}
}

public class MySimpleParameterizedController extends MyParameterizedController<SimpleBean> {
}

In that test, we try to resolve the generic parameter of the method retrieved thanks to MySimpleParameterizedController.class.getMethod("handleDto", HttpEntity.class). When getJavaType() is invoked, type is a sun.reflect.generics.reflectiveObjects.TypeVariableImpl that represent DTO and contextClass is my MySimpleParameterizedController class.

Both Jackson 2.6 TypeFactory#constructType(Type type, Class<?> contextType) and my draft fix with contextClass.getGenericSuperclass() are returning a SimpleBean JavaType.

But if I resolve directly contextClass into a JavaType, the JavaType returned is DTO not SimpleBean and my test fails. Could it be possible to tune the new type resolution mechanism to make it work like 2.6 when resolving the JavaType using contextClass, without requiring contextClass.getGenericSuperclass() that is likely to create some regression and is not very intuitive?

@cowtowncoder
Copy link
Member

@sdeleuze Well, keep in mind that the method as is deprecated for the reason it can not actually be fully implemented with information given: it is possible to pass inconsistent arguments, but no way to really detect whether they are or are not compatible. So change to replace that on caller side needs to be bigger. Problem is often that method itself may not have enough context to do such decision.

In your specific case, resolution should start with MySimpleParameterizedController.class becoming JavaType, then finding method handleDto as AnnotatedMethod (annotation access is not what matters, but rather type resolution) from within that JavaType. Or, to find type parameters, find JavaType for MyParameterizedController; super class / interfaces are available via JavaType.

The reason type parameters from MySimpleParameterizedController do not work is that the method itself is not declared there, and binding of parameters happens at specific class.

I hope my explanation at least points in right direction... it's bit difficult to explain. But basically active type parameter name to resolved type bindings vary between layers of inheritance hierarchy; older code assumed there's one flat binding, but this is not true. Now that layers are exactly resolved, it is however necessary to find the proper layer. It can be more work, but on plus side it actually then fully works.

@sdeleuze
Copy link
Author

Thanks for your detailed feedback, much appreciated.

sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Jan 23, 2016
AbstractJackson2HttpMessageConverter now implements its own
TypeVariable resolution algorithm since it is now deprecated
and has not the same behavior in Jackson 2.7.
See FasterXML/jackson-databind#1087
for more details.

The dependency on jackson-datatype-jdk7 has been removed since
it is now provided by default in jackson-databind.

Issues: SPR-13483, SPR-13728
@sdeleuze
Copy link
Author

I have implemented the TypeVariable resolution algorithm on Spring Framework side with support just for the use case we need, as detailed in https://jira.spring.io/browse/SPR-13728, so I close this issue.

@cowtowncoder
Copy link
Member

@sdeleuze Thank you for working through this, and apologies for the hassle. As I said, I hope to keep API stable in this are from this point on.

cowtowncoder added a commit that referenced this issue Jan 24, 2016
@cowtowncoder cowtowncoder added this to the 2.7.1 milestone Jan 24, 2016
sdeleuze added a commit to sdeleuze/spring-framework that referenced this issue Jan 25, 2016
AbstractJackson2HttpMessageConverter now implements its own
TypeVariable resolution algorithm since in Jackson 2.7 it is now
deprecated and has not the same behavior .
See FasterXML/jackson-databind#1087
for more details.

The dependency on jackson-datatype-jdk7 has been removed since
it is now provided by default in the jackson-databind module.

Issues: SPR-13483, SPR-13728
aryehpro added a commit to aryehpro/jackson-databind that referenced this issue Jan 25, 2016
# By Tatu Saloranta (113) and others
# Via Tatu Saloranta
* 'master' of https://github.com/FasterXML/jackson-databind: (124 commits)
  Minor addition related to FasterXML#1087: resolve context type, assuming type bindings from that are expected to work.
  Add unit test for FasterXML#999
  minor warnings cleanup
  Add Javadoc badge with automatic version detection
  Fix FasterXML#1083
  Add failing test for FasterXML#1083
  add a unit test to verify that Object Id works via AtomicReference too
  Minor javadoc improvement wrt FasterXML#1076, making `SimpleType.construct(Class)` deprecated (was not yet, for some reason, should have been)
  Fix FasterXML#1078
  Fix FasterXML#1079
  [maven-release-plugin] prepare for next development iteration
  [maven-release-plugin] prepare release jackson-databind-2.7.0
  prepare for 2.7.0 final
  Fix FasterXML#1073
  Try to reproduce FasterXML#1074
  javadoc trimming
  Try to reproduce FasterXML#825 again, still passes.
  minor improvement to ensure base64 encoding uses configured setting
  Undo part of change done for StringDeserializer; caused issues with XML handling
  further minor cleanups to cleanup
  ...
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

No branches or pull requests

2 participants