Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): single value enums (const) are not generated correctly in 3.1 specs #19696

Merged
merged 5 commits into from
Oct 6, 2024

Conversation

joscha
Copy link
Contributor

@joscha joscha commented Sep 26, 2024

This pull request extends the OpenApiNormalizer to translate single const enums (3.1) into an enum with a single value (3.0).
The fix and a unit test is in 9d36e99
The diff of the samples produced by that fix can be seen in 2760c37

Background:

Currently single value enums in 3.1 are not generated correctly.

In 3.1 these two schemas should produce equivalent code:

      SingleValueEnum_3_1:
        required:
          - type      
        properties:
          type:
            const: this-is-my-only-value
            type: string
      SingleValueEnum_3_0:
        required:
          - type
        properties:
          type:
            enum:
              - this-is-my-only-value

(the type property of each model can only ever have the value this-is-my-only-value. For 3.1 specs this seems to be widened to string, which is only technically correct.
It is especially visible in the typescript generator, where values are now widened to string instead of a "this-is-my-only-value" as const type. //cc @macjohnny

Real-world example (a spec that was upgraded from 3.0.1 to 3.1.0:

Screenshot 2024-09-26 at 10 06 41 AM

via planet-a-ventures/affinity-node#49

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/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    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*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language core: @wing328

import { HttpFile } from '../http/http';

export class SingleValueEnum31 {
'type': string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be:

Suggested change
'type': string;
'type': 'this-is-my-only-value';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix in 9d36e99

@joscha

This comment was marked as outdated.

@@ -1251,6 +1251,12 @@ private Schema processNormalize31Spec(Schema schema, Set<Schema> visitedSchemas)
schema.getTypes().remove("null");
}

// process const
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual fix that transforms a single const into an enum with one value, so the 3.1 and 3.0 representations are equivalent

import { HttpFile } from '../http/http';

export class SingleValueEnum31 {
'type': SingleValueEnum31TypeEnum;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the fix (1)

}

export enum SingleValueEnum31TypeEnum {
ThisIsMyOnlyValue = 'this-is-my-only-value'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and (2)

@joscha joscha marked this pull request as ready for review September 29, 2024 21:55
Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@macjohnny macjohnny requested a review from wing328 September 30, 2024 06:47
@wing328
Copy link
Member

wing328 commented Sep 30, 2024

thanks for the PR

cc @OpenAPITools/generator-core-team

@joscha
Copy link
Contributor Author

joscha commented Sep 30, 2024

thanks for the PR

cc @OpenAPITools/generator-core-team

Thank you. Sorry, wasn't aware there was a core team alias, will use that one from now on.

Update: actually, I think this team doesn't exist? But it would be great if it did, and even greater if we had one per generator? We could then update the pull request template as well with the correct mention, which will work as well for additions and removals and people in the respective team can also tailor their notification preferences around it and/or filter emails

@joscha
Copy link
Contributor Author

joscha commented Oct 2, 2024

Is there anything I can do to help this one along? I am somewhat blocked in planet-a-ventures/affinity-node#49 until this issue has been resolved.

@wing328
Copy link
Member

wing328 commented Oct 6, 2024

lgtm. thanks for the PR

@wing328 wing328 merged commit 2f73582 into OpenAPITools:master Oct 6, 2024
15 checks passed
@wing328
Copy link
Member

wing328 commented Oct 7, 2024

fyi. samples updated via c8fad42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants