Openapi-generator: [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 https://github.com/OpenAPITools/openapi-generator/pull/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