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

Nested type arguments doesn't work with polymorphic types #1604

Closed
Raniz85 opened this issue Apr 13, 2017 · 25 comments
Closed

Nested type arguments doesn't work with polymorphic types #1604

Raniz85 opened this issue Apr 13, 2017 · 25 comments
Milestone

Comments

@Raniz85
Copy link

Raniz85 commented Apr 13, 2017

When using @JsonSubTypes/@JsonTypeInfo on an interface with a generic parameter (de)serialising a subclass that wraps it's own generic parameter in another generic class or interface doesn't work.

It's a bit complex to explain with words so here's a short example:

public class GenericPolymorphismTest {
    @JsonTypeInfo(property = "type", use = JsonTypeInfo.Id.NAME)
    @JsonSubTypes({
            @JsonSubTypes.Type(value = AddToList.class, name = "addToList")
    })
    public interface Operation<T> {
        T perform(T operand);
    }

    public static class AddToList<T> implements Operation<List<T>> {
        T value;
        @Override
        public List<T> perform(List<T> operand) {
            operand.add(value);
            return operand;
        }
    }

    public static class Container {
        Operation<List<UUID>> listOperation;
    }

    private static final ObjectMapper objectMapper = new ObjectMapper();
    static {
        objectMapper.setVisibility(new VisibilityChecker.Std(JsonAutoDetect.Visibility.ANY));
    }

    public static void main(String[] args) throws JsonProcessingException {
        AddToList<UUID> addToList = new AddToList<>();
        addToList.value = UUID.randomUUID();
        Container container = new Container();
        container.listOperation = addToList;

        System.out.println(objectMapper.writeValueAsString(container));
    }
}

This crashes with the following exception:

Exception in thread "main" com.fasterxml.jackson.databind.JsonMappingException: Class java.util.UUID not subtype of [collection type; class java.util.List, contains [simple type, class java.util.UUID]] (through reference chain: GenericPolymorphismTest$Container["listOperation"]->GenericPolymorphismTest$AddToList["value"])
	at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:388)
	at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:348)
	at com.fasterxml.jackson.databind.ser.std.StdSerializer.wrapAndThrow(StdSerializer.java:343)
	at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:698)
	at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeWithType(BeanSerializerBase.java:581)
	at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:706)
	at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:690)
	at com.fasterxml.jackson.databind.ser.BeanSerializer.serialize(BeanSerializer.java:155)
	at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:292)
	at com.fasterxml.jackson.databind.ObjectMapper._configAndWriteValue(ObjectMapper.java:3681)
	at com.fasterxml.jackson.databind.ObjectMapper.writeValueAsString(ObjectMapper.java:3057)
	at GenericPolymorphismTest.main(GenericPolymorphismTest.java:57)
Caused by: java.lang.IllegalArgumentException: Class java.util.UUID not subtype of [collection type; class java.util.List, contains [simple type, class java.util.UUID]]
	at com.fasterxml.jackson.databind.type.TypeFactory.constructSpecializedType(TypeFactory.java:359)
	at com.fasterxml.jackson.databind.cfg.MapperConfig.constructSpecializedType(MapperConfig.java:306)
	at com.fasterxml.jackson.databind.DatabindContext.constructSpecializedType(DatabindContext.java:149)
	at com.fasterxml.jackson.databind.ser.BeanPropertyWriter._findAndAddDynamic(BeanPropertyWriter.java:870)
	at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:682)
	at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:690)
	... 8 more

If I try work around it by making value into a List<T> deserialisation works but serialisation gives me a List<String> instead of List<UUID> so using a list after perform will throw ClassCastExceptions.

@cowtowncoder
Copy link
Member

And this is with late(st) version, like 2.8.8?

@Raniz85
Copy link
Author

Raniz85 commented Apr 14, 2017

This is with 2.8.8

@wlp1979
Copy link

wlp1979 commented Oct 11, 2017

I am able to recreate this bug without using polymorphism. Using the following code, the GoodOuter can be serialized correctly, but BadOuter fails. I've tested this with both 2.8.4 and 2.8.10.

import java.util.ArrayList;
import java.util.List;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;

public class JacksonBug {
	public static class Data<T> {
		private T data;

		public Data(T data) {
			this.data = data;
		}

		public T getData() {
			return data;
		}

		public static <T> Data<List<T>> of(List<T> data) {
			return new DataList<>(data);
		}
	}

	public static class DataList<T> extends Data<List<T>> {

		public DataList(List<T> data) {
			super(data);
		}
	}

	public static class Inner {
		private int index;

		public Inner(int index) {
			this.index = index;
		}

		public int getIndex() {
			return index;
		}
	}

	public static class BadOuter {
		private Data<List<Inner>> inner;

		public BadOuter(Data<List<Inner>> inner) {
			this.inner = inner;
		}

		public Data<List<Inner>> getInner() {
			return inner;
		}
	}

	public static class GoodOuter {
		private DataList<Inner> inner;

		public GoodOuter(DataList<Inner> inner) {
			this.inner = inner;
		}

		public DataList<Inner> getInner() {
			return inner;
		}
	}

	public static void main(String[] args) throws JsonProcessingException {
		List<Inner> inners = new ArrayList<>();
		for (int i = 0; i < 10; i++) {
			inners.add(new Inner(i));
		}
		GoodOuter goodOuter = new GoodOuter(new DataList<>(inners));
		BadOuter badOuter = new BadOuter(Data.of(inners));
		ObjectMapper objectMapper = new ObjectMapper();
		System.out.println("GoodOuter: " + objectMapper.writeValueAsString(goodOuter));
		System.out.println("BadOuter: " + objectMapper.writeValueAsString(badOuter));
	}
}

@cowtowncoder
Copy link
Member

@wlp1979 Thank you for sharing.

cowtowncoder added a commit that referenced this issue Oct 11, 2017
@cowtowncoder
Copy link
Member

Yes, I can reproduce this. Looks like code path may be slightly different, but at least there's something to dig into.

@epollan
Copy link

epollan commented Oct 13, 2017

@cowtowncoder I isolated this down to a change introduced in 2.7.4. Serialization of both of these object graphs succeeds starting back as far as 2.5.3 and works through 2.7.3. I'll look at the changes to databind in 2.7.4 to see if there's something obvious. I'm a long-time Jackson user, but have never had cause to modify the codebase, so it may take me a bit ;)

@epollan
Copy link

epollan commented Oct 13, 2017

Scanning through the diff, this looks fishy:

diff --git a/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java b/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java
index bf01aaef5..88def8c54 100644
--- a/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java
+++ b/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java
@@ -364,19 +364,22 @@ public final class TypeFactory
                 }
             }
             // (3) Sub-class does not take type parameters -- just resolve subtype
-            if (subclass.getTypeParameters().length == 0) {
+            int typeParamCount = subclass.getTypeParameters().length;
+            if (typeParamCount == 0) {
                 newType = _fromClass(null, subclass, TypeBindings.emptyBindings());     
                 break;
             }
-
+            
             // If not, we'll need to do more thorough forward+backwards resolution. Sigh.
             // !!! TODO (as of 28-Jan-2016, at least)
-
+            
             // 20-Oct-2015, tatu: Container, Map-types somewhat special. There is
             //    a way to fully resolve and merge hierarchies; but that gets expensive
             //    so let's, for now, try to create close-enough approximation that
             //    is not 100% same, structurally, but has equivalent information for
             //    our specific neeeds.
+            // 29-Mar-2016, tatu: See [databind#1173]  (and test `TypeResolverTest`)
+            //  for a case where this code does get invoked: not ideal
             if (baseType.isInterface()) {
                 newType = baseType.refine(subclass, TypeBindings.emptyBindings(), null,
                         new JavaType[] { baseType });
@@ -388,7 +391,20 @@ public final class TypeFactory
             if (newType == null) {
                 // But otherwise gets bit tricky, as we need to partially resolve the type hierarchy
                 // (hopefully passing null Class for root is ok)
-                newType = _fromClass(null, subclass, TypeBindings.emptyBindings());        
+                TypeBindings tb = null;
+
+                // 14-Apr-2016, tatu: One possible short-cut; if type parameter counts
+                //   match, chances are they ought to match. Let's take our chances...
+                if (baseType.containedTypeCount() == typeParamCount) {
+                    if (typeParamCount == 1) {
+                        tb = TypeBindings.create(subclass, baseType.containedType(0));
+                    } else if (typeParamCount == 2) {
+                        tb = TypeBindings.create(subclass, baseType.containedType(0),
+                                baseType.containedType(1));
+                    }
+                }
+                newType = _fromClass(null, subclass,
+                        (tb == null) ? TypeBindings.emptyBindings() : tb);
             }
         } while (false);

@cowtowncoder
Copy link
Member

@epollan Excellent detective work. This is bit concerning, mostly since I do not think that undoing change would work here. Code would rather have to do better resolution of type parameterization, ideally "pushing" type variables through hierarchy and get binding that way.

But it does make sense that this would cause false match of bindings.

@cowtowncoder
Copy link
Member

Yes, I think what goes wrong here is resolution of

Data<List<Inner>>

with sub-type of

 DataList

which short-circuits with assumption that matching number of type variables means that simple substitution works; not true in this case.

Proper resolution would resolve original type, retaining type variable, and then sort of back-tracking...
and the reason this is not done is that it's rather difficult thing to do. java-classmate does handle this, but it is unencumbered by API level complexity Jackson's type system has, and code can not be directly used.

This may be very difficult to solve, esp. for 2.9.x (I won't even attempt changing any older versions since risks for change will be non-trivial)

@cowtowncoder
Copy link
Member

And this essentially goes wrong in the fallback case, in which subtype (one being specialized as) is given existing TypeBindings of base type.

Removing handling and only using base type as-is would fix this case, but breaks one of existing tests (#1186), in which (conversely) existing handling works ok.

@epollan
Copy link

epollan commented Oct 16, 2017

Looks like the problem is in TypeFactory._bindingsForSubtype. Specifically, the baseCount == typeParamCount branch is being entered, with the variable's JavaType (Data<List<Inner>>) containing one immediate binding (Data<T> reifies T to List<Inner>), and the Class<?> (DataList<Inner>) also has one type parameter.

@epollan
Copy link

epollan commented Oct 16, 2017

@cowtowncoder is there a README/doc that describes how to get the jackson code compiling locally? The tip of master references a parent pom of com.fasterxml.jackson:jackson-base:3.0.0-SNAPSHOT, which leads to:

[ERROR]     Non-resolvable parent POM: Could not find artifact com.fasterxml.jackson:jackson-base:pom:3.0.0-SNAPSHOT and 'parent.relativePath' points at wrong local POM @ line 5, column 11 -> [Help 2]

I pinned the parent to the latest released version I found on maven central (2.9.2). I then get an error from the enforcer plugin complaining about jacoco:

[INFO] --- maven-enforcer-plugin:3.0.0-M1:enforce (enforce-java) @ jackson-databind ---
[WARNING] Rule 2: org.apache.maven.plugins.enforcer.RequirePluginVersions failed with message:
Some plugins are missing valid versions:(LATEST RELEASE SNAPSHOT are not allowed )
org.jacoco:jacoco-maven-plugin. 	The version currently in use is 0.7.9
[ERROR] Best Practice is to always define plugin versions!

I pinned jacoco to 0.7.9, and now get java 8 compilation errors:

[INFO] --- maven-compiler-plugin:3.2:compile (default-compile) @ jackson-databind ---
[INFO] Changes detected - recompiling the module!
[INFO] Compiling 415 source files to /Users/epollan/src/jackson-databind/target/classes
[INFO] -------------------------------------------------------------
[WARNING] COMPILATION WARNING : 
[INFO] -------------------------------------------------------------
[WARNING] bootstrap class path not set in conjunction with -source 1.7
[INFO] 1 warning
[INFO] -------------------------------------------------------------
[INFO] -------------------------------------------------------------
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /Users/epollan/src/jackson-databind/src/main/java/com/fasterxml/jackson/databind/ext/jdk8/Jdk8StreamSerializer.java:[74,28] lambda expressions are not supported in -source 1.7
  (use -source 8 or higher to enable lambda expressions)
[ERROR] /Users/epollan/src/jackson-databind/src/main/java/com/fasterxml/jackson/databind/ext/jdk8/LongStreamSerializer.java:[37,30] lambda expressions are not supported in -source 1.7
  (use -source 8 or higher to enable lambda expressions)
[ERROR] /Users/epollan/src/jackson-databind/src/main/java/com/fasterxml/jackson/databind/ext/jdk8/IntStreamSerializer.java:[39,30] lambda expressions are not supported in -source 1.7
  (use -source 8 or higher to enable lambda expressions)
[ERROR] /Users/epollan/src/jackson-databind/src/main/java/com/fasterxml/jackson/databind/ext/jdk8/DoubleStreamSerializer.java:[39,30] lambda expressions are not supported in -source 1.7
  (use -source 8 or higher to enable lambda expressions)

I suspect there's some basic local config I need to do to get the jackson-xxx modules building locally...

@cowtowncoder
Copy link
Member

@epollan I would recommend using branch 2.9 actually. I can push snapshots of components for 3.0.0 (that is, master), but that is far away from being released.
There should be no special set up needed in general, but jackson-databind does have some dependencies. With 2.9 it is easier to somewhat reduce that (parent poms can be release versions, not snapshots).

@epollan
Copy link

epollan commented Oct 17, 2017

I checked out the 2.9.2 tag & it builds fine now.

That type binding transfer to the subclass that's at issue here is pretty concerning... I can see how it would help immensely for retaining type parameterization data for collection/map values, but it's so easy for it to do the wrong thing...

Question: what's the implication of the fall-through case (i.e. EMPTY_BINDINGS)? What's lost in the serialization process in this case?

I'm wondering if maybe specializing this branch to just apply to collection/map would work?

@epollan
Copy link

epollan commented Oct 17, 2017

As I suspected, restricting that branch of type binding transfer to just collection-like types causes tests to fail:

Results :

Failed tests: 
  MapEntryFormatTest.testInclusionWithReference:159 expected:<{[]}> but was:<{["entry":{"a":null}]}>
  MapEntryFormatTest.testInclusion:143 expected:<{[]}> but was:<{["entry":{"a":""}]}>
  AbstractTypeMapping1186Test.testDeserializeMyContainer:45 expected:<class com.fasterxml.jackson.databind.jsontype.AbstractTypeMapping1186Test$MyObject> but was:<class java.util.LinkedHashMap>

Tests in error: 
  TypeRefinementForMapTest.testMapRefinement:108 » InvalidDefinition Cannot cons...

Tests run: 2047, Failures: 3, Errors: 1, Skipped: 0

I think what actually needs to happen is to "unwind" the base type's type bindings in this situation. E.g., given A<T> and B<T>, a value of a type defined as C<X> extends A<B<X>> and assigned to a field of A<B<X>> would inherit only the type binding of the field's JavaType to X. Noodling around for implementations of this algorithm that wouldn't be too expensive...

@epollan
Copy link

epollan commented Oct 17, 2017

blargh. search space is large. I'm just now appreciating that the type parameterization info available on JavaType is a list of N-ary trees.

@cowtowncoder
Copy link
Member

Actually I know what the problem is and how to approach it -- did it for java-classmate -- but as things are, code in question only tries simplest of heuristics. This is guaranteed pretty much not to work in general; it can only cover limited number of cases, and perhaps use sub-optimal heuristics for other.

But the approach that would work really is to resolve the subtype (class), but keeping context of resolved super-type (base type), and once resolution hits unbound parent type of same type-erased type, bind it.

For example: we have Data<List<Inner>> as base type, and DataList<?> as subtype.
We would try to resolve DataList<?> and descend down type hierarchy normal way, with incomplete/missing bindings, which would essentially resolve to Data<List<?>>, which for Jackson then becomes Data<List<Object>>. However: during traversal, check for each parent type would check if we happen to have "external" binding: and encountering Data<List<?>> resolving that with bound+resolved Data<List<Inner>>. This should probably do some basic sanity checks... I don't know if it is possible to have inconsistent bindings; or more importantly, if so, is that a problem (given that thanks to Type Erasure, one can have, say, Numbers within what is declared as List<String> -- ability to resolve incompatible types might be a feature, not bug).

Does this make more sense? Challenge within TypeFactory is that of how to pipe through extra bind information -- it may make sense to duplicate part of code path. Or not. Adding more information in TypeContext, cleanly, may not be trivial thing to do, since this is the only code path where it is needed.
Note, too, that type of binding here is different: instead of named type variables to bind this binds specific "layer", by type-erased Class.

For what it is worth, Classmate:

https://github.com/FasterXML/java-classmate

has functionality for this. Whether having a look at it helps or not I don't know; APIs are quite different since Jackson adds more usage-specific semantics (f.ex. CollectionType and MapType being distinct types -- classmate does not do that, but separates by "physical" differences only).

@epollan
Copy link

epollan commented Oct 17, 2017

Yes -- verifying that DataList<?> is equivalent to Data<List<?>> is the key bit. I'm trying to code that up now. Thanks for the pointers to classmate, BTW.

My plan is to code up a guard against the type binding transfer for exactly this case (albeit simplified for the arity-1 type binding case) and see what tests break.

TBH, I think we're going to end up changing our code to avoid the problem. But, I kind of want to dig a little deeper in case there's a reasonable way out of this. At the very least, I'll have greater understanding of the bug and where it might bite me in the future.

@cowtowncoder
Copy link
Member

Yes. So, just to clarify what I meant: existing type resolution works fine, as is, when only resolving a single type. What is needed here is sort of re-parent things (grafting?), and it could either be implemented as separate recursive process (instead of resolving it is actually walking existing type hierarchy, really), or trying to retrofit resolution with this.

There are further challenges in that type objects are (mostly, ideally should be fully) immutable, to figure out what and how to copy.

@epollan
Copy link

epollan commented Oct 17, 2017

The naive fix for the arity 1 type parameterization case works and all the databind tests pass.

The type count == 1 case in _bindingsForSubtype becomes:

if (typeParamCount == 1) {
    JavaType binding = baseType.containedType(0);
    // While the type binding is actually nested with a single inner binding, AND the subtype's definition
    // itself already implies the outermost binding, then...
    while (binding.containedTypeCount() == 1 && impliesBinding(baseType, binding, subclass)) {
        // ...peel off the outermost binding
        binding = binding.containedType(0);
    }
    return TypeBindings.create(subclass, binding);
}

with impliesBinding implemented as:

private boolean impliesBinding(JavaType baseType, JavaType baseTypeBinding, Class<?> subclass) {
    // Should take a A<B<C>>, e.g., and make it A<B<?>>
    JavaType generalizedBase = _fromClass(
        null,
        baseType.getRawClass(),
        TypeBindings.create(
            baseType.getRawClass(),
            _fromClass(
                null,
                baseTypeBinding.getRawClass(),
                TypeBindings.create(
                    baseTypeBinding.getRawClass(),
                    CORE_TYPE_OBJECT
                )
            )
        )
    );
    JavaType wildcardedSubclass = _fromClass(
        null,
        subclass,
        TypeBindings.create(
            subclass,
            CORE_TYPE_OBJECT
        )
    );
    return wildcardedSubclass.findSuperType(baseType.getRawClass()).equals(generalizedBase);
}

Now, whether this can be generalized to arity N type parameterization in a reasonable manner remains to be seen.

@cowtowncoder
Copy link
Member

Brave attempt, but I am almost certain that this will lose type information since it drops anything that base type had, and since subtype is type erased this can not be brought back from anywhere.
It does happen to work case here, mostly since this is for serialization and type information of elements is available.

Still, something along these lines could work. I hope to find time to work a bit on this, since discussion has helped me remember where I left off after 2.7 (and leaving this one known area of concern -- literally the only known gap at this point, wrt type resolution).

@epollan
Copy link

epollan commented Oct 17, 2017

I think I have it generalized. Will clone, branch, and PR tomorrow after cleaning up what I have. I'm not sure familiar with how the TypeBindings computed for the subclass : Class<?> => JavaType conversion process are used, though. The type information dropped could be an issue, but I have all the tests passing (at least in jackson-databind)...

epollan added a commit to epollan/jackson-databind that referenced this issue Oct 18, 2017
cowtowncoder added a commit that referenced this issue Oct 18, 2017
cowtowncoder added a commit that referenced this issue Oct 18, 2017
@cowtowncoder cowtowncoder added this to the 2.9.3 milestone Oct 19, 2017
@cowtowncoder
Copy link
Member

Ok. With enough banging my head against the wall, and then going to java-classmate to remember full resolution algorithm, I was able to implement what I think is a right way to fully solve nested type assignments.

@dnno
Copy link

dnno commented May 9, 2018

Hi,
We're facing a problem in our project when updating to the latest Spring Boot version (1.5.12) which upgrades from Jackson 1.8.10 to 1.8.11: Serialization of some of our classes now doesn't work anymore. Going up to 1.9.5 the same problem still persists for us.

I was able to track this back to your change in this issue. I've isolated the problem to a simple maven based project, you can find it here:

https://github.com/dnno/jackson-serialization-problem/

The classes we try to serialize make use of generics and Jackson now isn't able to handle them anymore. Type resolution stops here:

TypeFactory._verifyAndResolvePlaceholders():476

if (exp.getRawClass() != act.getRawClass()) {  
    return false;  
}

I don't really understand a lot of what's happening here, but what I do see is, that if I manually set the resulting error String to , the serialization will work fine.

Could it be, that it's wrong to just compare for the same raw classes, but instead traverse the type hierarchy? In my case it's comparing a List (actual) to an Object (expected). A successful comparison would make sense for me.

It would be great if you could take a look at our case, let me know if I can provide further information.

@cowtowncoder
Copy link
Member

@dnno Could you please file a new issue for this: even if the problem is related to this one (caused by fix), it is easier to track fix that way. You can add a reference to this issue as likely root cause.

I can't say much except that this could be related to / same as #1964, which will be fixed for 2.9.6

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

5 participants