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

Deserialization of @JsonTypeInfo annotated type fails with missing type id even for explicit concrete subtypes #2968

Closed
kilink opened this issue Dec 3, 2020 · 16 comments · Fixed by #3803
Labels
2.15 Issues planned for 2.15 most-wanted Tag to indicate that there is heavy user +1'ing action
Milestone

Comments

@kilink
Copy link

kilink commented Dec 3, 2020

When attempting to deserialize to a concrete class that is part of a polymorphic type hierarchy, an InvalidTypeIdException is thrown if the JSON does not contain the type id field. Example:

@JsonTypeInfo(
    use = JsonTypeInfo.Id.NAME,
    property = "type")
@JsonSubTypes({
    @JsonSubTypes.Type(value = Foo.class, name = "foo"),
    @JsonSubTypes.Type(value = Bar.class, name = "bar")})
public interface Base {}

public class Foo implements Base {}

public class Bar implements Base {}

ObjectMapper mapper = new ObjectMapper();

mapper.readerFor(Foo.class).readValue("{}"); // throws InvalidTypeIdException
mapper.readValue("{}", Foo.class); // throws InvalidTypeIdException

While I understand why this happens, as Jackson is finding the JsonTypeInfo / JsonSubTypes annotations on the interface, it is counterintuitive to me. In this instance, I am instructing the mapper as to the specific concrete class to deserialize to, so consulting those annotations seems unnecessary. Perhaps checking if the class / type supplied to readerFor / readValue matches exactly one of the classes listed in JsonSubType could be a fallback if the type id property is not found?

So far, the only workaround I've found is to do something like this:

@JsonTypeInfo(use = JsonTypeInfo.Id.NONE)
public class Foo implements Base {}

but then that means serializing a Foo instance would not get the type id property. Perhaps a custom TypeIdResolver or SubTypeResolver could also be used, but having the described behavior baked in seems like a sensible default to me. Thoughts?

@kilink kilink added the to-evaluate Issue that has been received but not yet evaluated label Dec 3, 2020
@kupci
Copy link
Member

kupci commented Dec 4, 2020

I'd be interested in more about the particular use case for this, but setting that aside for the moment, I could possibly see one benefit, of a small performance gain, i.e. bypass the annotations check if there's nothing supplied.

Perhaps checking if the class / type supplied to readerFor / readValue matches exactly one of the classes listed in JsonSubType could be a fallback if the type id property is not found?

Assuming this is a reasonable improvement, given the above, do you have a PR to show your thinking on this? Also (going back to the use case), not clear on how big of a hit this would be, i.e. how often would this apply? This seems to be a very narrow scenario.

@cowtowncoder
Copy link
Member

Ok, I think there already an issue for improving handling in this case (will see if I can find it). It would be nice to make type id optional for concrete case but it seems rather difficult to make work reliably due to delegating nature of things.

@kilink
Copy link
Author

kilink commented Dec 4, 2020

Here's an example. We have a tool that generates Java / Kotlin classes from GraphQL schemas, so given an example trivial schema:

interface Animal {
  name: String
}

type Dog implements Animal {
  name: String
  barkVolume: Int
}
        
type Cat implements Animal {
  name: String
  lives: Int
}

type Query {
  animals: [Animal]
  dogs: [Dog]
  cats: [Cat]
}
       

We might generate the following for the Animal interface so that query for animals works correctly:

@JsonTypeInfo(
    use = JsonTypeInfo.Id.NAME,
    include = JsonTypeInfo.As.PROPERTY,
    property = "__typename"
)
@JsonSubTypes({
    @JsonSubTypes.Type(value = Dog.class, name = "Dog"),
    @JsonSubTypes.Type(value = Cat.class, name = "Cat")
})
public interface Animal {
  String getName();
}

Now when queries are also built using our codegen tool, we can always arrange for the __typename meta field to be returned, but some users may opt to only use the generated types and build their own queries by hand. To us the generated class even when querying for a specific type like Dog, they need to remember to also always ask for __typename, which seems a little strange to have to do if they are indicating at time of deserialization they know the concrete type based on the query they're performing.

I can also think of non-GraphQL scenarios, where the client is using generated code and doing similar queries for REST endpoints like /animals, /dogs, and the server component is perhaps not using the same classes at all or is written in a different language. Yes, the /dogs endpoint could return JSON with type ids, but it seems redundant to have to do that when the client is specifically asking for a "concrete type" in this case.

@kilink
Copy link
Author

kilink commented Dec 4, 2020

Assuming this is a reasonable improvement, given the above, do you have a PR to show your thinking on this? Also (going back to the use case), not clear on how big of a hit this would be, i.e. how often would this apply? This seems to be a very narrow scenario.

I have not implemented a fix for this yet, but did begin digging around to see if it's possible. I tried to give some more context on the use case in my previous comment.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 4, 2020

Could not find clear example, but #2063 is similar.

I would be open to a PR that would accept missing type id in case of concrete subtype being requested, and have been planning on working on making this possible (but it did not make it in 2.12).

@kupci Annotation processing would not be optimized: they are checked when constructing (de)serializers, once, and never during runtime.

@cowtowncoder cowtowncoder changed the title Deserialization fails for explicit concrete subtypes Deserialization of @JsonTypeInfo annotated type fails with missing type id even for explicit concrete subtypes Dec 4, 2020
@cowtowncoder
Copy link
Member

Since I cannot quite find a more up-to-date issue, I will mark this one as "most-wanted" one -- and may also close older ones that seem like they would be solved with this one.

@cowtowncoder cowtowncoder added 2.13 most-wanted Tag to indicate that there is heavy user +1'ing action and removed to-evaluate Issue that has been received but not yet evaluated labels Dec 4, 2020
@cowtowncoder cowtowncoder added 2.14 and removed 2.13 labels Jul 15, 2021
@alessiostalla
Copy link

Another related use case may be the following:

@JsonTypeInfo(
    use = JsonTypeInfo.Id.NAME,
    include = JsonTypeInfo.As.PROPERTY)
abstract class MyGeneralDomainObject {
    String id; //property
}

class A extends MyGeneralDomainObject {
    MyGeneralDomainObject obj; //here, we need type information
}

class B extends MyGeneralDomainObject {
    A propertyWithExplicitType; //here, we know the type is A, so if A doesn't have subclasses, why should we require type information in the JSON?
}

@wetneb
Copy link

wetneb commented Mar 30, 2022

I would also find this feature very useful.

I want to share a workaround which seems to work fine in my case: to annotate the subclasses with the same @JsonTypeInfo, but including a defaultImpl. For instance:

// Top-level interface

@JsonTypeInfo(
    use = JsonTypeInfo.Id.NAME,
    include = JsonTypeInfo.As.PROPERTY,
    property = "__typename"
)
@JsonSubTypes({
    @JsonSubTypes.Type(value = Dog.class, name = "Dog"),
    @JsonSubTypes.Type(value = Cat.class, name = "Cat")
})
public interface Animal {
  String getName();
}

// Subclasses

@JsonTypeInfo(
    use = JsonTypeInfo.Id.NAME,
    include = JsonTypeInfo.As.PROPERTY,
    property = "__typename",
    defaultImpl = Dog.class
)
@JsonSubTypes({
    @JsonSubTypes.Type(value = Dog.class, name = "Dog")
})
public class Dog implements Animal {
    // ...
}

@JsonTypeInfo(
    use = JsonTypeInfo.Id.NAME,
    include = JsonTypeInfo.As.PROPERTY,
    property = "__typename",
    defaultImpl = Cat.class
)
@JsonSubTypes({
    @JsonSubTypes.Type(value = Cat.class, name = "Cat")
})
public class Cat implements Animal {
   // ...
}

It is not exactly pretty, but it seems to be working as expected.

@SebastianOltmanns
Copy link

This feature would also help me a lot.

Let me shortly explain my use case. We have the interface Animal and two implementing classes Cat and Dog.

@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, include = JsonTypeInfo.As.PROPERTY, property = "_class")
public abstract class Animal {
  public String name;
}

public class Cat implements Animal {}

public class Dog implements Animal {}

I have a HTTP Interface, where the client can request multiple Animals (GET /animals), which would result in a response like this:

[
{"_class": "Cat.class", "name": "foo"},
{"_class": "Cat.class", "name": "bar"},
{"_class": "Dog.class", "name": "bark"},
]

Here it is very important to include _class, because only with this the client can distinguish the two classes.

I also have HTTP endpoints to add a Cat (POST /cat), and one to add a Dog (POST /dog).
When using the POST /cat endpoint, I do not need to know about the _class attribute, because I expect the sender to send a Cat. I want the client to be able to send the body {"name": "meow"} when doing the POST /cat request, instead of having to send {"_class": "Cat.class", "name": "meow"}

As a workaround, I currently do this (thanks to the comment of @wetneb ), which is not the end of the world for me, but I definitly do not like it.

@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, include = JsonTypeInfo.As.PROPERTY, property = "_class")
public abstract class Animal {
  public String name;
}

@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, include = JsonTypeInfo.As.PROPERTY, property = "_class", defaultImpl = Cat.class)
public class Cat implements Animal {}

@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, include = JsonTypeInfo.As.PROPERTY, property = "_class", defaultImpl = Dog.class)
public class Dog implements Animal {}

@paulux84
Copy link

paulux84 commented Feb 9, 2023

Hi all, any news on this issue? Is marked as most-wanted for version 2.14 but is still open.
Thanks

@cowtowncoder
Copy link
Member

@paulux84 No, unfortunately I haven't had any time to look into this particular issue. PRs welcome; will add label.

@cowtowncoder cowtowncoder added the pr-welcome Issue for which progress most likely if someone submits a Pull Request label Feb 13, 2023
@JooHyukKim
Copy link
Member

JooHyukKim commented Mar 4, 2023

I started working on this issue on PR #3803 But it turns out there is quite a number of tests proving otherwise, testing that even deserialization of value as explicit concrete subtype should fail.

So to prevent myself from modifying loads of tests just to abort 😅, may I ask you to check if the change at PR #3803 is viable, @cowtowncoder ? 🙏🏼

@cowtowncoder
Copy link
Member

@JooHyukKim Unfortunately I don't think that approach is correct; determination cannot rely on only abstract types using TypeDeserializers -- intermediate concrete (or even base) types are perfectly legal for polymorphic handling.

@JooHyukKim
Copy link
Member

JooHyukKim commented Mar 4, 2023

PR Status

About the third attempt

  • Took the approach of checking whether deserialization target class has TypeResolver attached to it, before throwing InvalidTypeIdException.
  • About last solution, passes all tests only "locally" but fails on the CI 🥲🥲. (Below photos are the CI error log)
  • LATEST UPDATE : current problem might be fixed by fix exception type in tests #3804 . I am waiting for the mentioned PR to be merged. Then I will try again.

image

image

Further plan

  1. Check if we can fix issue with the third attempt.
  2. If failed, try setting deserializer closer to the initialization phase.

@cowtowncoder cowtowncoder added 2.15 Issues planned for 2.15 and removed 2.14 pr-welcome Issue for which progress most likely if someone submits a Pull Request labels Mar 12, 2023
@cowtowncoder cowtowncoder added this to the 2.15.0 milestone Mar 12, 2023
cowtowncoder added a commit that referenced this issue Mar 12, 2023
cowtowncoder added a commit that referenced this issue Mar 13, 2023
@cowtowncoder
Copy link
Member

Ok, yay -- this is now fixed for 2.15, and will be in the first 2.15.0 release candidate!
Big Thank You to @JooHyukKim for the fix!

One future challenge: I was not able to merge the fix cleanly in 3.0 (master) -- handling of polymorphic type resolution was refactored significantly and I could not see simple 1-to-1 translation. For now, then, tests wrt this issue are under failing in master. I will create a new follow-up issue for that.

@cowtowncoder
Copy link
Member

cowtowncoder commented Apr 12, 2023

NOTE: as per #3853 this is OPT-IN for 2.15, so to allow deserialization you HAVE TO enable

MapperFeature.REQUIRE_TYPE_ID_FOR_SUBTYPES

so that the behavior is changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.15 Issues planned for 2.15 most-wanted Tag to indicate that there is heavy user +1'ing action
Projects
None yet
8 participants