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

TypeFactory cannot convert Collection sub-type without type parameters to canonical form and back #3108

Closed
lbilger opened this issue Apr 7, 2021 · 7 comments
Milestone

Comments

@lbilger
Copy link

lbilger commented Apr 7, 2021

Describe the bug
When constructing a type using constructType and getting its canonical String representation, a type parameter is added to classes that implement a Collection type, even if the class itself does not have any type parameters. Trying to read the result back using constructFromCanonical fails with java.lang.IllegalArgumentException: Cannot create TypeBindings for class kotlin.collections.EmptyList with 1 type parameter: class expects 0.

Version information
Tested with 2.11.4 and 2.12.2.

To Reproduce

See the following test written in Kotlin:

class JacksonTypeFactoryTest {
    private val objectMapper = ObjectMapper()

    @Test
    fun `EmptyList type can be converted to canonical string and back`() {
        val canonical = objectMapper.typeFactory.constructType(emptyList<String>().javaClass).toCanonical()
        println(canonical)
        val type = objectMapper.typeFactory.constructFromCanonical(canonical)
        println(type)
    }

    @Test
    fun `StringList type can be converted to canonical string and back`() {
        val canonical = objectMapper.typeFactory.constructType(StringList::class.java).toCanonical()
        println(canonical)
        val type = objectMapper.typeFactory.constructFromCanonical(canonical)
        println(type)
    }

    @Test
    fun `Non-collection type can be converted to canonical string and back`() {
        val canonical = objectMapper.typeFactory.constructType(ConcreteType::class.java).toCanonical()
        println(canonical)
        val type = objectMapper.typeFactory.constructFromCanonical(canonical)
        println(type)
    }

    class StringList : ArrayList<String>()

    open class ParamType<T>

    class ConcreteType : ParamType<Int>()
}

The first two tests fail, while the third one succeeds. So it seems to be a problem only with types that extend Collection.

Expected behavior
TypeFactory should always produce canonical type names that it can parse again.

Additional context
Although this doesn't seem to be a Kotlin-specific problem, it gains relevance by the fact that Kotlin uses the EmptyList class to optimize empty lists.

This is similar to #1415 but different in that I'm not using constructCollectionType here.

@lbilger lbilger added the to-evaluate Issue that has been received but not yet evaluated label Apr 7, 2021
@cowtowncoder cowtowncoder added 2.12 and removed to-evaluate Issue that has been received but not yet evaluated labels Apr 7, 2021
@cowtowncoder
Copy link
Member

Assuming this is not Kotlin-specific, it'd be great to be able to reproduce with plain Java, to add test here (tests that use Kotlin would need to go under Kotlin module).
Seems like tests 2 and 3 would be easy to convert.

@lbilger
Copy link
Author

lbilger commented Apr 7, 2021

Thanks for the fast answer! Following is the test converted to Java. The first one would only work if you have the Kotlin libs on the class path, but you can just remove it.

public class JacksonTypeFactoryTest {
    private final ObjectMapper objectMapper = new ObjectMapper();

    @Test
    public void empty_list() {
        String canonical = objectMapper.getTypeFactory().constructType(CollectionsKt.<String>emptyList().getClass()).toCanonical();
        System.out.println(canonical);
        JavaType type = objectMapper.getTypeFactory().constructFromCanonical(canonical);
        System.out.println(type);
    }

    @Test
    public void string_list() {
        String canonical = objectMapper.getTypeFactory().constructType(StringList.class).toCanonical();
        System.out.println(canonical);
        JavaType type = objectMapper.getTypeFactory().constructFromCanonical(canonical);
        System.out.println(type);
    }

    @Test
    public void non_collection() {
        String canonical = objectMapper.getTypeFactory().constructType(ConcreteType.class).toCanonical();
        System.out.println(canonical);
        JavaType type = objectMapper.getTypeFactory().constructFromCanonical(canonical);
        System.out.println(type);
    }

    static class StringList extends ArrayList<String> {}

    static class ParamType<T> {}

    static class ConcreteType extends ParamType<Integer> {}
}

@cowtowncoder
Copy link
Member

Thanks! It seems likely that addressing problems shown by 2 Java-only cases might handle Kotlin case too.

@cowtowncoder
Copy link
Member

I can reproduce this. Yeah, this will be very difficult to resolve... I'll see what I can do, but I would recommend that no code outside of Jackson core functionality tries to use canonical name for JavaType at all; handling is bit too specialized for the limited use case of Jackson itself. Whereas java-classmate would (for example) properly bind type parameters to actual type, Jackson's Collection-/Map(Like)Type are more interested in type-bindings of key/content value types.

@cowtowncoder cowtowncoder added 2.13 and removed 2.12 labels Apr 10, 2021
@cowtowncoder cowtowncoder added this to the 2.12.3 milestone Apr 11, 2021
@cowtowncoder cowtowncoder changed the title TypeFactory cannot convert collection type without type parameters to canonical form and back TypeFactory cannot convert Collection sub-type without type parameters to canonical form and back Apr 11, 2021
@cowtowncoder
Copy link
Member

Fixed this in 2.12 for 2.12.3, wrt. "simple" cases presented: works by checking that the number of type parameters JavaType instance provides matches what declaration of the "raw class" has.

The problem fundamentally is that CollectionType, for example, expects exactly 1 parameter which is introspected from binding of underlying implementation of java.util.Collection -- but JavaType's "raw class" may be anything else, and could have one unrelated type variable.

I'll have to think about what to do wrt canonical name in general for 3.0: it is really only ever needed for one quite specific case -- that of EnumSet/EnumMap handling for polymorphic cases.
It may be used for other generic polymorphic types but I am bit doubtful that it would actually really work.

@lbilger
Copy link
Author

lbilger commented Apr 14, 2021

Thanks for fixing this so quickly! I found this bug through a standard software I'm using that's internally using Jackson to serialize/deserialize data. I'll have a look at how exactly they are using the canonical name and point them to your comment about not using it outside Jackson itself.

@cowtowncoder
Copy link
Member

@lbilger no problem! And yes, it is probably good to have a look -- there may be legit need there, and since the methods are public I can see why they may seem like good options. But if at all possible I would suggest trying to avoid use.

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