OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.58k stars 6.52k forks source link

[BUG] DefaultCodegen.java does not implement legacyDiscriminatorBehavior as documented. #15246

Open LennardF1989 opened 1 year ago

LennardF1989 commented 1 year ago

NOTE! Please scroll down to after the renaming of this issue!

Given the following snippet:

{
      "BaseComponent": {
        "required": [
          "typeAlias"
        ],
        "type": "object",
        "properties": {
          "typeAlias": {
            "minLength": 1,
            "type": "string",
            "readOnly": true
          },
          "fieldAlias": {
            "type": "string",
            "nullable": true
          }
        },
        "additionalProperties": false,
        "discriminator": {
          "propertyName": "typeAlias",
          "mapping": {
            "header": "#/components/schemas/HeaderComponent",
            "rte": "#/components/schemas/RichTextComponent",
            "image": "#/components/schemas/ImageComponent"
          }
        }
      },
      "HeaderComponent": {
        "required": [
          "typeAlias"
        ],
        "type": "object",
        "allOf": [
          {
            "$ref": "#/components/schemas/BaseComponent"
          }
        ],
        "properties": {
          "typeAlias": {
            "minLength": 1,
            "type": "string",
            "readOnly": true
          },
          "text": {
            "type": "string",
            "nullable": true
          }
        },
        "additionalProperties": false
      }
}

opentools-generator v6.5.0 will generate something like this: image

However, I did not intend to have the "HeaderComponent", "ImageComponent" and "RichTextComponent" to be part of the alias-mapping.

My feeling is the $ref back to the BaseComponent throws the generator off. Not sure if it's an issue with the typescript-fetch templates or the generator logic itself.

LennardF1989 commented 1 year ago

Additional information. Above is with legacyDiscriminatorBehavior set to false.

https://openapi-generator.tech/docs/generators/typescript-fetch/ mentions that when it's set to false it will do the following:

The mapping in the discriminator includes any descendent schemas that allOf inherit from self, any oneOf schemas, any anyOf schemas, any x-discriminator-values, and the discriminator mapping schemas in the OAS document

Now that I've re-read that sentence a few times, above screenshot is actually intended... Meaning this bug is void, correct?

Something is definitely up, this is what CodegenDiscriminator.java says: image

Looks like the docs has it inverted, also note the OR (slightly cut off in the screenshot) - it's either the mapping entries themselves OR the childschema's, not both.

Is there a way to control this behavior? I don't like the default legacyDiscriminatorBehavior either, as it generates objects (if you console.log them) that have all properties, and not just the one from the discriminator. It's annoying...

Also, the instanceOf* functions make absolutely no sense. It should check all values, now it will only check for the discriminator. So an ImageComponent is an instanceOf an HeaderComponent with this logic now...

export function instanceOfHeaderComponent(value: object): boolean {
    let isInstance = true;
    isInstance = isInstance && "typeAlias" in value;

    return isInstance;
}
LennardF1989 commented 1 year ago

Looks like the behavior comes from the AbstractTypeScriptClientCodegen.java: image

This function is called a bunch of times.

I suspect the culprit is here: image

When legacyDiscriminatorBehavior is set to false, as described in my earlier screenshot from the codebase, I would expect this to just skip the inherited discriminator property - now it will find out the child-class (eg. HeaderComponent) doesn't have a discriminator section and decide to include the model.classname as a possibility.

EDIT: Just confirmed the flag is definitely inverted. If I give all subTypes another "x-discriminator-value", it will be included in the switch-statement of my first post.

LennardF1989 commented 1 year ago

Alright, so I tracked it down.

One of my API-calls has an array of BaseComponent as a response. This property has a oneOf so you can see the possible models it can return in the Swagger UI.

"components": {
  "type": "array",
  "items": {
    "oneOf": [
      {
        "$ref": "#/components/schemas/HeaderComponent"
      },
      {
        "$ref": "#/components/schemas/RichTextComponent"
      },
      {
        "$ref": "#/components/schemas/ImageComponent"
      }
    ]
  },
  "nullable": true
}

However, this causes the typescript-fetch client to generate the Inner-file you can see above (based on the modelOneOf.mustache file). which in turned causes it to ignore the mapping that already exists and include the additional types.

I feel this is a bug (or at least undesirable - why have the spec generate a discriminator that won't ever be used or valid), where the mappedModels of the discriminator should not include the modelName, just because it's referenced. I feel this related to recursiveGetDiscriminator and runs deeper than just typescript-fetch.

LennardF1989 commented 1 year ago

Renamed this bug to what it's actually about.

Please refer to this: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenDiscriminator.java#L30-L32

This comment specifically says one OR the other, not both. oneOf is not even mentioned as a possibility.

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L3536-L3543

This bit does not take that into account and will just decide that when a another schema has a oneOf relation, to also add those to the discriminator. But this is a false assumption if a mapping is defined.

An easy fix would be to change the if-check to this:

if (ModelUtils.isComposedSchema(schema) && !this.getLegacyDiscriminatorBehavior() && uniqueDescendants.isEmpty()) {

If nothing of the previous resulting in any mappings to be discovered AND legacyDiscriminatorBehavior is disbled only then actually do this.

This seems like a breaking change so probably needs an additional property to manage this accordingly.