Use enum type for discriminator #13846

bernie-schelberg-mywave · 2022-10-28T02:23:50Z

As described in #12412, it's reasonably common to define the valid options for a discriminator property as an enum. However, the current versions of the generator assume the discriminator property is always string. This change uses the specified type when it is explicitly defined as a separate type in the schema. I attempted to also handle the scenario where the enum property was specified inline, but it was too complicated for me.

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master (6.1.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

bernie-schelberg-mywave · 2022-10-28T03:21:57Z

@cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09)

ddoming73 · 2022-11-02T09:13:07Z

Any chance of this PR being merged soon?

welshm

The code itself seems good, I think we'll need to see what the regenerated samples look like for other servers

welshm · 2022-11-02T12:12:16Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

+        // FIXME: there are other ways to define the type of the discriminator property (inline
+        //  for example). Handling those scenarios is too complicated for me, I'm leaving it for
+        //  the future..
+        String propertyType =
+            Optional.ofNullable(schema.getProperties())
+                .map(p -> (Schema) p.get(discPropName))
+                .map(Schema::get$ref)
+                .map(ModelUtils::getSimpleRef)
+                .orElseGet(() -> typeMapping.get("string"));
+        discriminator.setPropertyType(propertyType);


This will impact more than just the JavaSpring generation - can you regenerate the samples and tests to see what impact this has on other clients?

@welshm I've run the commands in the PR instructions above. No files changed to commit to Git. I'm not certain that it worked properly, but the scripts seemed to run without errors.

welshm · 2022-11-02T12:19:03Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

+        //  the future..
+        String propertyType =
+            Optional.ofNullable(schema.getProperties())
+                .map(p -> (Schema) p.get(discPropName))


nit: I know this isn't a variable you named, but I think full naming is better to disambiguate. discriminatorPropertyName might be more clear what the variable clearly represents

Variable has been renamed, please review again

welshm

Code look great! Can you run the commands in the PR template to regenerate the samples?

@wing328 will need to approve running the tests

cachescrubber · 2022-11-03T08:36:36Z

modules/openapi-generator/src/test/resources/3_0/oneOfDiscriminator.yaml

+      properties:
+        fruitType:
+          $ref: "#/components/schemas/FruitTypeEnum"
+      required: fruitType


The required property must be an array.

Sorry, I see the problem now. I've pushed a fix.

cachescrubber · 2022-11-03T08:41:37Z

modules/openapi-generator/src/test/resources/3_0/oneOfDiscriminator.yaml

+        - $ref: '#/components/schemas/BananaOneOfEnumMappingDisc'
+      discriminator:
+        propertyName: fruitType
+        mapping:


The mapping to the enum value might cause problems with the auto generated JsonSubType.

I actually generated a sample Project:

@JsonIgnoreProperties( value = "fruitType", // ignore manually set fruitType, it will be automatically generated by Jackson during serialization allowSetters = true // allows the fruitType to be set during deserialization ) @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "fruitType", visible = true) @JsonSubTypes({ @JsonSubTypes.Type(value = AppleOneOfEnumMappingDisc.class, name = "APPLE"), @JsonSubTypes.Type(value = AppleOneOfEnumMappingDisc.class, name = "AppleOneOfEnumMappingDisc"), @JsonSubTypes.Type(value = BananaOneOfEnumMappingDisc.class, name = "BANANA"), @JsonSubTypes.Type(value = BananaOneOfEnumMappingDisc.class, name = "BananaOneOfEnumMappingDisc") }) @Generated(value = "org.openapitools.codegen.languages.SpringCodegen", date = "2022-11-03T09:30:55.032940+01:00[Europe/Berlin]") public interface FruitOneOfEnumMappingDisc { public FruitTypeEnum getFruitType(); }

@welshm Isn't there another issue / pr dealing with duplicated JsonSubTypes?

@bernie-schelberg-mywave Could you please add a enum based discriminator to Ione of the generated sample projects (for example bin/configs/spring-boot-oneof.yaml)?

I actually generated a sample Project:

@JsonIgnoreProperties( value = "fruitType", // ignore manually set fruitType, it will be automatically generated by Jackson during serialization allowSetters = true // allows the fruitType to be set during deserialization ) @JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "fruitType", visible = true) @JsonSubTypes({ @JsonSubTypes.Type(value = AppleOneOfEnumMappingDisc.class, name = "APPLE"), @JsonSubTypes.Type(value = AppleOneOfEnumMappingDisc.class, name = "AppleOneOfEnumMappingDisc"), @JsonSubTypes.Type(value = BananaOneOfEnumMappingDisc.class, name = "BANANA"), @JsonSubTypes.Type(value = BananaOneOfEnumMappingDisc.class, name = "BananaOneOfEnumMappingDisc") }) @Generated(value = "org.openapitools.codegen.languages.SpringCodegen", date = "2022-11-03T09:30:55.032940+01:00[Europe/Berlin]") public interface FruitOneOfEnumMappingDisc { public FruitTypeEnum getFruitType(); }

@welshm Isn't there another issue / pr dealing with duplicated JsonSubTypes?

I'm running openapi-generator version 6.2.0 and the above is the current behaviour when you add a mapping to the discriminator. Both the derived classes and the mapping values are added as JsonSubTypes.

The documentations seems to indicate that this is the expected behaviour.

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 AND Codegen validates that oneOf and anyOf schemas contain the required discriminator and throws an error if the discriminator is missing.

@cachescrubber Yes, there's another project for duplicated JsonSubTypes: #13815

Done (added enum based discriminator to samples)

bernie-schelberg-mywave · 2022-11-07T11:43:55Z

@welshm @cachescrubber @wing328 Sorry for the delay, I've managed to generate samples (I added an example using an enum mapping), please review again.

GavinRay97 · 2022-11-08T16:45:54Z

Thanks a ton for this @bernie-schelberg-mywave, this will help us out a ton at Hasura with our public SDK types, which currently fail to generate properly due to having a lot of polymorphic types using enum discriminants.

We have a query language IR where we distinguish between types of operations like binary_op, unary_op, binary_array_op, and operators like eq, neq, lte etc.

https://github.com/hasura/graphql-engine/blob/ed8e81270ed623a37530a23aedebe3d1b47b3b80/dc-agents/dc-api-types/src/agent.openapi.json#L1225-L1261

This fails pretty miserably in OpenAPI Generator currently:

      "Expression": {
        "discriminator": {
          "mapping": {
            "and": "AndExpression",
            "binary_arr_op": "ApplyBinaryArrayComparisonOperator",
            "binary_op": "ApplyBinaryComparisonOperator",
            "exists": "ExistsExpression",
            "not": "NotExpression",
            "or": "OrExpression",
            "unary_op": "ApplyUnaryComparisonOperator"
          },
          "propertyName": "type"
        },
        "oneOf": [
          {
            "$ref": "#/components/schemas/ExistsExpression"
          },
          // <SNIPPED>
         ]
        },
      "BinaryComparisonOperator": {
        "additionalProperties": true,
        "anyOf": [
          {
            "enum": [
              "less_than",
              "less_than_or_equal",
              "greater_than",
              "greater_than_or_equal",
              "equal"
            ],
            "type": "string"
          },
          {
            "type": "string"
          }
        ]
      },

bernie-schelberg-mywave · 2022-11-21T21:11:50Z

@welshm @cachescrubber @wing328 is there anything else I can do to help move this PR forward?

wing328 · 2022-11-25T02:39:14Z

cc @OpenAPITools/generator-core-team as the change impacts default codegen.

bernie-schelberg-mywave · 2022-11-25T04:52:20Z

@wing328 thanks for running the workflows, I saw there was a failure, so merged the master branch and re-generated the samples. Now I have a CircleCI failure, but I don't believe that it's because of the changes I've made. The error appears to be caused by a Java version incompatibility, although it builds using version 1.8 on my machine. I'm unable to run the samples.circleci profile on my local (arm64e-powered) machine. Can you provide any clue as to what I can do to resolve the failure?

wing328 · 2022-12-06T05:35:24Z

Hi @bernie-schelberg-mywave thanks again for the PR.

Can you please PM me via Slack for some quick questions if you don't mind?

ref: https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g

nitish-atomic · 2022-12-07T08:17:27Z

@wing328 is there any timeline for having this in any release??

vojkozel96 · 2023-01-20T13:22:49Z

Hi, do we have any progress here? We miss this functionality and it would help us a lot!

ludovicianul · 2023-02-09T08:44:37Z

Is this planned to be merged? Seems like a key missing functionality? Is there a workaround for it that anyone is aware of (other than leaving the discriminator as String)?

maidi29 · 2023-02-21T09:25:56Z

Any updates on this? 👀

ludovicianul · 2023-02-21T10:09:18Z

As a workaround, I'm using a custom template for the oneof_interface.mustache.

bernie-schelberg-mywave · 2023-02-22T01:52:42Z

As far as I know, the plan is to include this as part of the version 7.0 release.

# Conflicts: # modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java

# Conflicts: # samples/openapi3/server/petstore/spring-boot-oneof/src/main/java/org/openapitools/model/Foo.java # samples/openapi3/server/petstore/spring-boot-oneof/src/main/java/org/openapitools/model/FooRef.java

...t/petstore/swift5/validation/PetstoreClient/Classes/OpenAPIs/URLSessionImplementations.swift

...les/openapi3/server/petstore/spring-boot-oneof/src/main/java/org/openapitools/model/Foo.java

…um discriminator

bernie-schelberg-mywave · 2023-03-07T22:28:38Z

...s/openapi3/server/petstore/spring-boot-oneof/src/main/java/org/openapitools/model/Apple.java

+   */
+  public Apple(Integer seeds) {
+    this.seeds = seeds;
+    this.fruitType = fruitType;


Initialising fruitType like this is weird, but is a separate issue I think. If a class implements an interface which has a oneOf with a discriminator, the discriminator property is only half considered "required". The template adds initialisation of the field, but omits the constructor parameter. Arguably, the field should be initialised with the correct discriminator value, it should not be a constructor parameter at all.

bernie-schelberg-mywave · 2023-03-07T22:36:45Z

I've switched the branch from master to 7.0.x on advice that this should be targeted for the 7.0.0 release. I added some better testing in the samples. Please review again and advise if there is anything else I can do to help move this forward.

cc @wing328 @jimschubert @cbornet @jmini @etherealjoy @spacether

effervescentia · 2023-03-24T18:13:18Z

I was so excited to find this pull request 🙏 very eager to have this functionality added
unfortunately without it the typescript-fetch generator just creates uncompilable garbage (it's 99% there, just missing some type guards and trying to spread a non-object value into an object) so we aren't able to use it for the other endpoints until this feature has been released.

Thanks for your continued work on this @bernie-schelberg-mywave

effervescentia · 2023-04-04T18:56:54Z

Hi @wing328 @jimschubert @cbornet @jmini @etherealjoy @spacether, I don't mean to be a pest, but is there a timeline we can expect to have this change merged?

Without this fix generators like the typscript-fetch generator will output uncompilable code with missing references and type-unsafe operations.
For example this minimal schema will generate an SDK that fails to compile. I'm using this configuration file which results in uncompilable code.

Each entry in the union creates a file that looks like this (the output of the 'foo' entry)

The union itself creates a file with the invalid operations and missing references.

specifically:

the typeguards (instanceOfTestControllerBadUnion200ResponseValueOneOf*) do not exist so the import fails
the *FromJSONTyped functions have a string union return type (which is not accurate, they return objects) but are being used in a spread operation, which results in a compiler error

wing328 · 2023-07-03T16:02:02Z

@bernie-schelberg-mywave thanks again for the PR. would you be able to resolve the merge conflicts for the last time? I'll merge it once the merge conflicts have been resolved.

# Conflicts: # modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

wing328 · 2023-07-04T15:25:36Z

as discussed the CI failure is not related to this change.

i'll look into the CI failures after merging this PR.

srix55 · 2023-07-08T06:59:21Z

Hi @wing328 This fix isn't in 7.0.0-Beta? I am not seeing the changes there - it still says string required for me.

yarinsa · 2023-08-28T08:37:21Z

Today when using discriminator a new type is generated. How can I use it and reference it to the original type?
Meaning

enum System:
 AWS = 'aws'
 Azure = 'azure'

type Specs<S extends System> = { [key: string] : any; system: S }

type AWSSpecs = { account_id: number } & Specs<System.AWS>
type AZureSpecs = { client_id: number } & Specs<System.Azure>

type SystemSpecsMapper : Record<System, Specs<System>> = {
 [System.AWS]: AWSSpecs
 [System.Azure]: AzureSpecs
}

Request<S extends System> {
  name: S
  specs: SystemSpecsMapper<S>
}

Now when using discriminator with union (python) I am getting a whole new enum with a super long name,which has nothing to do with the original enum type

Fix OpenAPITools#12412 Use enum type for discriminator

9f35097

welshm reviewed Nov 2, 2022

View reviewed changes

Rename variable

74093a7

welshm reviewed Nov 2, 2022

View reviewed changes

cachescrubber suggested changes Nov 3, 2022

View reviewed changes

bernie-schelberg-mywave added 2 commits November 7, 2022 21:21

Fix required property in test resource

d53f07f

Add example of enum mapping to samples

0cf0de7

bernie-schelberg-mywave added 2 commits November 25, 2022 13:47

Merge branch 'master' into issue12412

2519d25

Generate samples

e07aca2

bernie-schelberg-mywave added 4 commits February 23, 2023 08:32

Merge branch 'master' into issue12412

c8b4a9f

# Conflicts: # modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java

Generate samples after merging master

0e08a4a

Merge branch '7.0.x' into issue12412

d1964ab

# Conflicts: # samples/openapi3/server/petstore/spring-boot-oneof/src/main/java/org/openapitools/model/Foo.java # samples/openapi3/server/petstore/spring-boot-oneof/src/main/java/org/openapitools/model/FooRef.java

Regenerate samples

e0f1b1c

bernie-schelberg-mywave requested a review from jimschubert as a code owner March 6, 2023 05:52

bernie-schelberg-mywave changed the base branch from master to 7.0.x March 6, 2023 05:52

Fix raw use of parameterized class

7857121