[BUG] [JavaSpring] Discriminator property gets serialized twice

[nickshoe]

openapi-generator version

3.3.4

Command line used for generation

I'm working with a JHipster API-First project, which uses openapi-generator-maven-plugin (v3.3.4).

Description

By following the OpenAPI 3.x documentation/examples, it seems that the property used as discriminator must also be specified in the properties list of the schema.

Example:

TicketEvent:
  type: object
  description: A generic event
  discriminator:
    propertyName: type
  required:
    - id
    - sequenceNumber
    - timestamp
    - type
  properties:
    id:
      type: integer
      format: int64
      readOnly: true
    sequenceNumber:
      type: integer
      readOnly: true
    timestamp:
      type: string
      format: date-time
      readOnly: true
    type:
      type: string
      readOnly: true
TicketMovedEvent:
  description: A ticket move event
  allOf:
    - $ref: '#/components/schemas/Event'
    - type: object
      required:
        - source
        - target
      properties:
        source:
          $ref: '#/components/schemas/Queue'
        target:
          $ref: '#/components/schemas/Queue'

The generated Java class for the code above seems to differ from the Jackson guidelines for polymorphic type handling with annotations, where the property used as discriminator must not be present in the class.
The generated code also includes this property as a class attribute with getter/setter.

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

public class TicketEvent   {
   ...

   @JsonProperty("type")
   private String type;

This causes the serialization output to include the property twice, as shown below.

{
    ...
    "type": "TicketMovedEvent",
    "type": null,
    ...
}

I've also tried to remove the property from the OpenAPI properties list, leaving intact the discriminator part; this way the generated code corresponds to the Jackson's guidelines and the serialization works just fine. On the other hand, I get the following error during the deserialization process.

com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field "type" (class it.blutec.bludesk.web.api.model.TicketMovedEvent), not marked as ignorable ...

To fix this, i've configured the Jackson ObjectMapper object for ignoring this kind of situations.

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

By the way, this looks like a workaround to me beacuse my OpenAPI file doesn't (strictly) follows the spec.

Suggest a fix

The generator should not include an additional attribute (with getter/setter) for a discriminator property.

[nickshoe]

Maybe it's the same bug of this issue: #2835

[wing328]

If removing discriminator property addresses the issue, we can certainly do that.

[nickshoe]

Manually removing the attribute (plus getter/setter), corresponding to the discriminator property, from the generated class does the trick. As stated above, I've just needed to turn off the DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES configuration on the ObjectMapper.

I've also tried to use JsonTypeInfo.As.EXISTING_PROPERTY in the discriminator's related annotation, but with no success.

I think that the type property only makes sense in the serialized version of an object; as a matter of fact, it conveys what in OO languages is modeled as classes. So I don't see the point in having an attribute in the class representing the class itself.

[nickshoe]

I would like to contribute to the project, but it's my first time ever in the open-source..
If the proposal of removing the discriminator property from the generated class is sensible, I can work on it. Maybe I'll need some advice for getting started on the repo and on the workflow.

[keenanpepper]

@nickshoe I might beat you to it.

[bkabrda]

I think I managed to solve this as part of #5120 - feel free to give it a go (although it's not a part of an existing release, it will be in openapi-generator 4.3.x release).

[keenanpepper]

@bkabrda it seems like the approach does work, but the change from PROPERTY to EXISTING_PROPERTY needs to be made for other generators as well, for example JavaSpring, which we are using. I'll created a PR for this shortly.

[nickshoe]

I've tried to generate the same example above for "spring" target with a 4.3.x build, but still can find the discriminator property in the class.

[nickshoe]

@keenanpepper

In version 4.3.1 (#5243) the Jackson's EXISTING_PROPERTY strategy is being used for serializing type info, maybe to address this issue, but I think that it is not correct.

In fact, this leads to a null value in the corresponding discriminator key in the output JSON.

Looking at the EXISTING_PROPERTY strategy comment in the official doc:

Inclusion mechanism similar to PROPERTY with respect to deserialization; but one that is produced by a "regular" accessible property during serialization. This means that TypeSerializer will do nothing, and expects a property with defined name to be output using some other mechanism (like default POJO property serialization, or custom serializer)

So, the @JsonSubTypes annotations seems to be ignored this way (even though they're actually produced by the Spring generator). (Please, look also at this issue)

I continue to think that we only need to prevent the actual property generation in the generated class code, and keep using the Jackson's PROPERTY strategy (leaving the discriminator key/value creation only to the serialization process).

[keenanpepper]

I had a PR open to do that, but it was significantly more complicated and invasive than the EXISTING_PROPERTY strategy.

Right now I'm using my own fork of openapi-generator that has the @JsonIgnore fix instead, and have a ticket open to replace that with the latest version of openapi-generator from upstream (thus switching to the EXISTING_PROPERTY fix).

[nickshoe]

@keenanpepper and @wing328 the fact is that the latest release broke the type serialization.
As I've mentioned in my previous comment, now the JSON has null values in the key corresponding to the OpenAPI's discriminator property. And my thesis is that the EXISTING_PROPERTY serialization strategy is not suited to to this, or at least not with the current generated code, maybe this strategy was misinterpreted as the name "suggests" what we wanted to achieve (not to generate the discriminator property as a class property) but in fact it has also side-effects.

[bonii-xx]

We tried to update from 4.2.3 to 4.3.1, and I stumbled upon this issue. Our serialization is now broken, because the discriminator field is missing. Before everything worked fine.

We're using this approach, which worked fine in 4.2.3. (no field generated in the java code)

    Foo:
      type: object
      properties:
        label:
          type: string
      discriminator:
        propertyName: type
        mapping:
          bar: '#/components/schemas/Bar'
          baz: '#/components/schemas/Baz'

With the PR that got released with 4.3.0 we now have to explicitly define the discriminator property? Is this intended?

[nickshoe]

@bonii-xx that's exactly what I'm doing with 4.2.2 (or 4.2.3).

Actually, omitting the discriminator property in the properties list is an "hack".
The OpenAPI 3.x spec clearly states:

...you can add the discriminator/propertyName keyword to model definitions. This keyword points to the property that specifies the data type name...

I don't know why they accepted the EXISTING_PROPERTY pull request, it seems to me that this project lacks of supervision.

IMHO explicitely defining the discriminator property is a must, because at a certain point in time an object must be serialized to JSON (or XML) and, without an additional property valued with the class name, the class information is lost. Thus the need for specifying in which key (property) expose this info (string).

[bonii-xx]

My example does not contain the discriminator property in the properties list, though. We had it in there before, but had to remove it due to the original problem of double-serialization. And now it's broken again, so I'm wondering if this was intentional to begin with.
You seem to be addressing a different issue.

[nickshoe]

@bonii-xx the two issues are related, please read all the comments.

[yanlobianchi]

Version 4.3.0 broke my clients too. The include change of JsonTypeInfo.As.PROPERTY to JsonTypeInfo.As.EXISTING_PROPERTY broken them.

When I instantiate a new object of a sub-type class, the discriminator doesn't exists in this object with the new strategy.

A workaround to this is add the discriminator as property in the POJO class, but in this way the client will have to fill this property when instantiate a object.
I really dislike this workaround, to me the old strategy is better, and if the change is necessary, add a option (configOption) to change the include type of serialization is more convenient and attend the two cases.

[wing328]

Please try the latest master or 5.0.0-beta3 if you've not already done so.

[vkennke]

unfortunately 5.0.0-beta3 has the same problem with JsonTypeInfo.As.EXISTING_PROPERTY

[jorgerod]

Hi

I have the same problem and it seems very important to me.

Will it be solved in version 5.0.0?

[matze999]

I have the same problems as mentioned above:
With version 5.0.0, the plain generated code:

• If i define the discriminator field <discriminator_field> explicitly in the spec itis not being filled automatically with the name of the class. Rather it is null.
• When i do not define <discriminator_field> in the spec i get no type information.
• When i (in the generated code) manually replace "@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY, property = "type", visible = true)" with "@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY)" it works and produces a json which includes "@type":"rectangle".
  With that i can serialize/deserialize.
  (This is from my post over at stackoverflow: https://stackoverflow.com/questions/65701626/jackson-not-generating-type-information?noredirect=1)

Anyway, right now i generate the code, change the code as mentioned above and commit it (which i would like to avoid).

[nickshoe]

This might be a possible solution: #4588 (comment)

[ghost]

Just wanted to share my workaround for this (not great...but it's working until this can be fixed properly). This is specifically for folks using Spring Boot 2, but I'm sure will help other Spring and even non-Spring Java project maintainers:

@Configuration
public class JacksonConfiguration {
    @Bean
    Jackson2ObjectMapperBuilderCustomizer jsonCustomizer() {
        return builder -> {
            var m = new SimpleModule();
            m.setSerializerModifier(new BeanSerializerModifier() {
                @Override
                public List<BeanPropertyWriter> changeProperties(
                        SerializationConfig config, BeanDescription beanDesc, List<BeanPropertyWriter> beanProperties
                ) {
                    for (var bpWriter : beanProperties) {
                        if ("_object_type".equals(bpWriter.getName())) {
                            bpWriter.assignNullSerializer(new ObjectTypeNullSerializer());
                        }
                    }
                    return super.changeProperties(config, beanDesc, beanProperties);
                }
            });
            builder.modulesToInstall(m);
        };
    }

    static class ObjectTypeNullSerializer extends JsonSerializer<Object> {
        @Override
        public void serialize(
                Object value, JsonGenerator gen, SerializerProvider serializers
        ) throws IOException {
            gen.writeString(gen.getCurrentValue().getClass().getSimpleName());
        }
    }
}

This assumes a fairly standard model inheritance e.g.:

    FooModelBase:
      type: object
      required:
        - _object_type
      properties:
        _object_type:
          type: string
      discriminator:
        propertyName: _object_type
    FooModelChildOne:
      allOf:
        - $ref: "#/components/schemas/FooModelBase"
        - type: object
          properties:
            bar:
              type: string
            baz:
              type: string
    FooModelChildTwo:
      allOf:
        - $ref: "#/components/schemas/FooModelBase"
        - type: object
          properties:
            zig:
              type: string
            zag:
              type: string

[MazizEsa]

e sam

Could use open api generator template? Change within the template.

[thomasbreitbach]

Any news here? It's super annoying to set all the technical values (discimininator) in the client all the time.

My workaround looks like this:
I declare the discriminator values in the super class and set the values in the inherited class.

FooTo:
    ...
    discriminator:
        propertyName: mytype
        mapping:
            bar: BarTo

BarTo:
    allOf:
        - $ref: '#/components/schemas/FooTo'
        - type: object
              properties:
                mytype:
                  type: string
                  enum:
                    - 'bar'
                  default: 'bar'

Anyway... The clutter and annoying part just moved - from client code to specification

[simosda]

One way to fix this would be to change the generated JsonTypeInfo.As.PROPERTY to JsonTypeInfo.As.EXISTING_PROPERTY inside @JsonTypeInfo:

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "some_property", visible = true)

when changed to

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.EXISTING_PROPERTY , property = "some_property", visible = true)

works as a charm and returns only one property instead of two. So the generating template of the plugin needs to be updated for certain templates. In my case this would be JavaJaxRS/typeInfoAnnotation.mustache.

[amir-ba]

I think the changes made with #9441) removes the JsonTypeInfo.As.EXISTING_PROPERTY from model annotations mustache and replaces it with JsonTypeInfo.As.PROPERTY. I actually could not figure out the motives for this. Does anyone know the background, if not I can create a MR which works just as the previous versions for such objects.
@rpost your feedback is appreciated.

[nickshoe]

@amir-ba, please read this: #3796 (comment).

TL;DR: EXISTING_PROPERTY is intended to be used only for deserialization purposes.

[amir-ba]

@nickshoe thanks for clarifying this for me.

[nickshoe]

I ultimately think that we could solve this by using the PROPERTY ser/des strategy and by putting the visible attribute to false (or without specifying it, since false is its default value).

Does anyone know why visible = true is being used in the current generated code?

This way, serialization and deserialization work without errors. If one needs to know the type of the object, then Java provides this information through classes (and reflection).

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY , property = "some_property", visible = false)

Or, equivalently (omitting the visible attribute):

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY , property = "some_property")

Or, equivalently (include actually defaults to PROPERTY):

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "some_property")

Please consider the example below:

The super-class:

package it.unibo.nickshoe;

import com.fasterxml.jackson.annotation.JsonFormat;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;

import java.time.Instant;

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
@JsonSubTypes({
        @JsonSubTypes.Type(value = StartEvent.class, name = "StartEvent"),
})
public abstract class Event {
    private int id;

    private Instant createdAt;

    public Event() {}

    public void setId(int id) {
        this.id = id;
    }

    public int getId() {
        return id;
    }

    public void setCreatedAt(Instant createdAt) {
        this.createdAt = createdAt;
    }

    public Instant getCreatedAt() {
        return createdAt;
    }
}

The sub-class:

package it.unibo.nickshoe;

public class StartEvent extends Event {

    private int position;

    public StartEvent() {}

    public void setPosition(int position) {
        this.position = position;
    }

    public int getPosition() {
        return position;
    }
}

The test:

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializationFeature;
import com.fasterxml.jackson.databind.json.JsonMapper;
import com.fasterxml.jackson.datatype.jdk8.Jdk8Module;
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
import com.fasterxml.jackson.module.paramnames.ParameterNamesModule;
import it.unibo.nickshoe.Event;
import it.unibo.nickshoe.StartEvent;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

import java.time.Instant;

public class JacksonPROPERTYStrategyTestCase {

    public static final String EXPECTED_START_EVENT_JSON = "{\"type\":\"StartEvent\",\"id\":100,\"createdAt\":\"2022-07-29T21:38:00Z\",\"position\":10}";

    private ObjectMapper mapper = JsonMapper.builder()
            .addModule(new ParameterNamesModule())
            .addModule(new Jdk8Module())
            .addModule(new JavaTimeModule())
            .build()
            .disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS);

    @Test
    public void testDeserialization() throws JsonProcessingException {
        Event deserializedStartEvent = mapper.readValue(EXPECTED_START_EVENT_JSON, Event.class);

        Assertions.assertEquals(deserializedStartEvent.getClass(), StartEvent.class);
    }

    @Test
    public void testSerialization() throws JsonProcessingException {
        StartEvent startEvent = new StartEvent();
        startEvent.setId(100);
        startEvent.setCreatedAt(Instant.parse("2022-07-29T21:38:00.000Z"));
        startEvent.setPosition(10);

        String serializedStartEvent = mapper.writeValueAsString(startEvent);

        Assertions.assertEquals(serializedStartEvent, EXPECTED_START_EVENT_JSON);
    }

}

[twbecker]

This is quite frustrating. IMO the solution needs to be to remove the discriminator property from the generated POJO altogether. The whole purpose of polymorphic types in this case is to encode the type info into the class itself. Having the discriminator property on the class just introduces the possibility that it be set such that it disagrees with the actual class of the object.

[twbecker]

I just noticed this was for Spring - this is an issue for the jaxrs-spec generator also.

[ShauniArima]

Was this issue solved? It is very frustrating to encounter this issue years after it was reported.

[rajat188]

Looks like the issue is not resolved for the latest version as well I am using
id("org.openapi.generator") version "7.5.0"

and still facing the same issue.