[REQ] Cleanup Jackson type info mess

[rpost]

General question: why some java generators use JsonTypeInfo.As.EXISTING_PROPERTY and some JsonTypeInfo.As.PROPERTY?

More specific question: why spring-java generator for following specification snippet:

    Service:
      type: object
      discriminator:
        propertyName: '@type'
      required:
        - '@type'
      properties:
        '@type':
          description: JSON type discriminator, must contain object type name
          type: string
    ConcreteService:
      allOf:
        - $ref: '#/components/schemas/Service'
        - type: object

creates sources as follows:

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "@type", visible = true)
@JsonSubTypes({
  @JsonSubTypes.Type(value = ConcreteService.class, name = "ConcreteService"),
})

public class Service   {
  @JsonProperty("@type")
  private String atType;

?

Why is discriminator value exposed as object property? It should be invisibile from java developer point of view. What is even worse is that I have to manually set proper type in order to have @type filled in serialized json.

Considering yaml above I would expect that

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "@type", visible = true)
@JsonSubTypes({
  @JsonSubTypes.Type(value = ConcreteService.class, name = "ConcreteService"),
})

public class Service   {
// no @type property!

is created.

[marcelstoer]

I've been there some three years ago: https://stackoverflow.com/questions/50163882/use-swagger-openapi-discriminator-so-that-jackson-serializes-object-correctly, take note of the follow-up issues in the comment.

• [Java] Generator produces a dedicated field for the discriminator swagger-api/swagger-codegen#8137
• JsonTypeInfo.As.PROPERTY type id, property with same name, result in duplicate JSON property FasterXML/jackson-databind#2022

[rpost]

I changed my mind about how type discriminators should be configured.

Verba docent, exempla trahunt:

This test summarizes current behaviour for generators which use EXISTING_PROPERTY:

package current;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class CurrentBehaviour {

    ObjectMapper mapper = new ObjectMapper();

    @Test
    void testSerialization() throws JsonProcessingException {
        String json = mapper.writeValueAsString(new Child());
        Assertions.assertEquals("{\"@type\":null}", json); //not optimal solution: developer has to set atType manually

        Child child = new Child();
        child.atType = "Child"; //bad! manual and error prone
        json = mapper.writeValueAsString(child);
        Assertions.assertEquals("{\"@type\":\"Child\"}", json);
    }

    @Test
    void testDeserialization() throws JsonProcessingException {
        Child parsed = mapper.readValue("{\"@type\":\"Child\"}", Child.class);
        Assertions.assertNotNull(parsed);
        Assertions.assertEquals("Child", parsed.atType);
    }

}

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "@type", visible = true)
@JsonSubTypes({
        @JsonSubTypes.Type(value = Child.class, name = "Child"),
})
class Parent {
    @JsonProperty("@type")
    String atType;
}

class Child extends Parent {
}

This is my proposal how to improve:

package suggested;

import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class Suggestion {

    ObjectMapper mapper = new ObjectMapper();

    @Test
    void testSerialization() throws JsonProcessingException {
        String json = mapper.writeValueAsString(new Child());
        Assertions.assertEquals("{\"@type\":\"Child\"}", json); // type discriminator handled by jackson itself!

        Child child = new Child();
        child.atType = "Child"; // even if developers touches this field manually...
        json = mapper.writeValueAsString(child);
        Assertions.assertEquals("{\"@type\":\"Child\"}", json); // ... jackson serializes it only once

        child = new Child();
        child.atType = "nonsense"; // even if developers breaks this ...
        json = mapper.writeValueAsString(child);
        Assertions.assertEquals("{\"@type\":\"Child\"}", json); // ... jackson just ignores error
    }

    @Test
    void testDeserialization() throws JsonProcessingException {
        Child parsed = mapper.readValue("{\"@type\":\"Child\"}", Child.class);
        Assertions.assertNotNull(parsed);
        Assertions.assertEquals("Child", parsed.atType);
    }

}

@JsonIgnoreProperties(value = "@type", allowSetters = true)
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "@type", visible = true)
@JsonSubTypes({
        @JsonSubTypes.Type(value = Child.class, name = "Child"),
})
class Parent {
    @JsonProperty("@type")
    String atType;
}

class Child extends Parent {
}

This configures Jackson so that it both:

• fills field in class during deserialization
• automatically fills json property with correct value during serialization

Are you open for MR with such change?

[rpost]

Ok, one part solved, we are closer to "good" but still not yet there.

To proceed further we have to clarify what is openapi spec valid and what not.

For sure this is one is valid and should be supported:

openapi: 3.0.0
info:
  title: Sample API
  version: 1.0.0
paths: {}
components:
  schemas:
    Dog:
      allOf:
        - $ref: '#/components/schemas/Animal'
        - type: object
          properties:
            breed:
              type: string
    Cat:
      allOf:
        - $ref: '#/components/schemas/Animal'
        - type: object
          properties:
            breed:
              type: string
    Animal:
      type: object
      discriminator:
        propertyName: className
      required:
        - className
      properties:
        className:
          type: string
        color:
          type: string
          default: red

But what if Animal object doesn't have className property:

openapi: 3.0.0
info:
  title: Sample API
  version: 1.0.0
paths: {}
components:
  schemas:
    Dog:
      allOf:
        - $ref: '#/components/schemas/Animal'
        - type: object
          properties:
            breed:
              type: string
    Cat:
      allOf:
        - $ref: '#/components/schemas/Animal'
        - type: object
          properties:
            breed:
              type: string
    Animal:
      type: object
      discriminator:
        propertyName: className
      required:
        - className
      properties:
        color:
          type: string
          default: red

Is this valid?

Why I'm asking? Seems like some Java generators assume first approach (the ones that had EXISTING_PROPERTY before my changes) and some assume second one.

Let's take jaxrs-cxf-client for instance.

Given first yaml as input, generated classes look as follow:

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "className", visible = true)
@JsonSubTypes({
  @JsonSubTypes.Type(value = Cat.class, name = "Cat"),
  @JsonSubTypes.Type(value = Dog.class, name = "Dog"),
})
public class Animal  {
  
  @ApiModelProperty(required = true, value = "")
  private String className;

  @ApiModelProperty(value = "")
  private String color = "red";
//...
}

and

public class Cat extends Animal {
  
  @ApiModelProperty(value = "")
  private String breed;
//...
}

Now, when I try to serialize:

        String s = new ObjectMapper().writeValueAsString(new Cat());
        System.err.println(s);

I'm getting {"className":"Cat","className":null,"color":"red","breed":null} which is obviously wrong. Interestingly the same generator fed with second yaml works fine (ie: there is no duplication of className field in serialized json). Even more interestingly second yaml combined with spring generator creates classes which fail to deserialize {"className":"Cat","className":null,"color":"red","breed":null} : Unrecognized field "className" (class de.telekom.hpbxcc.mate.client.model.Cat), not marked as ignorable (2 known properties: "color", "breed"])

Long story short: to move forward and clean this chaos we need to decide whether second one yaml is valid and supported

[rpost]

related: #3796

[cookeac]

I'm working with a group of participants on an open source project and dealing with the same issue.
The first yaml that @rpost has listed is our understanding of what should be valid according to the OpenAPI spec, and the second yaml is what many Java developers have needed to implement until now so that openapi-generator would generate the correct serialisation/deserialisation. The problem is that the second yaml does not seem to align with the spec, and does not produce proper code for other languages.
Moving to support the first yaml is the right move - but do you need to also support the second yaml for backwards compatibility for Java-only use?

[rpost]

Thank you @cookeac for your opinion. Sad thing is that apparently no one else (from maintainers) is interested in this topic. Pity, I was eager to clean this up.

[nickshoe]

I think that we should follow the "valid OpenAPI spec" path and consider these two requirements:

1. we want the serialization process to produce a JSON key indicating the object type, as specified with @JsonSubTypes annotation, without having to specify an additional property in the POJO since Java objects already have this information represented by their (sub)classes
2. we want the deserialization process to produce POJO instances of the correct subclasses, we're not interested at all in an additional property in the POJO, because we have Java objects (hence, we don't care about deserialization)

I'm starting to think that this is rather a problem with how Jackson handles serialization/deserialization itself: the PROPERTY strategy seems to be working just fine during serialization, but requires a regular POJO property during deserialization. Anyways, there is a workaround for the deserialization issue: #4588 (comment).

So, my proposal is:

• use PROPERTY strategy
• don't use an additional property in the POJO
• configure Jackson to ignore unrecognized fields during deserialization

    ObjectMapper om = new ObjectMapper();
    om.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
    ...

[nickshoe]

Another proposal (but I haven't tested it yet):

• use PROPERTY strategy and set visible = false (see here)
• don't use an additional property in the POJO

By setting visible = false the deserialization process should not complain about unrecognized fields/properties. This way, there's no need to change the ObjectMapper configuration as I've pointed out in my previous comment.