Skip to content

Conversation

@joscha
Copy link
Contributor

@joscha joscha commented Aug 28, 2024

When a discriminator is a pure enum, currently the typescript generator assigns a string to it, which is the name of the class (this.discriminator = "<ClassName>").
See:
Screenshot 2024-08-28 at 5 26 19 PM
and:
image

Because the stringified classname and the enum type are usually incompatible (unless there is an enum value which happens to be the class name) a type error is produced.

This pull request wraps this line in an additional guard.

Please note that the fix does not do any introspection on a potential overlap of the classname with valid enum values as any such assignment, whilst syntactically valid, would most likely not make any semantical sense.

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
    
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the [technical committee]: @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) @davidgamero (2022/03) @mkusaka (2022/04)

{{/discriminatorValue}}
{{/allVars}}
{{#discriminatorName}}
{{^discriminator.isEnum}}
Copy link
Contributor Author

@joscha joscha Aug 28, 2024

Choose a reason for hiding this comment

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

this is the meaningful change of this PR.

I can see here:

{{#discriminator}}
{{#discriminator.isEnum}}
this.{{{discriminatorName}}} = this.getClass().getSimpleName();
{{/discriminator.isEnum}}
{{/discriminator}}

that other generators guard the opposite case. If I misunderstand what the fix is, please let me know. An alternative could also be to make the discriminator type always a string/symbol and only assign the stringified value of each enum value.

Copy link
Member

Choose a reason for hiding this comment

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

so what would be the discriminator.isEnum case?

Copy link
Contributor Author

@joscha joscha Aug 29, 2024

Choose a reason for hiding this comment

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

The discriminator is the type member and type is of type InteractionTypeEnum, thus the template string discriminator.isEnum would evaluate to true, hence with the template negation, it would not emit the guarded class name assignment (e.g. it would omit this.{{discriminatorName}} = "{{classname}}";)

Copy link
Contributor Author

@joscha joscha Aug 29, 2024

Choose a reason for hiding this comment

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

basically before the change we'd produce code that looks like this:

export class Interaction {
    'type': InteractionTypeEnum;
    static readonly discriminator: string | undefined = "type";
    public constructor() {
        this.type = "Interaction";
    }
}

export enum InteractionTypeEnum {
    Call = 'call',
    Email = 'email',
    Meeting = 'meeting',
    ChatMessage = 'chat-message'
}

(note how the string "Interaction" is not a valid value of InteractionTypeEnum)

After the changes in this PR it would be:

export class Interaction {
    'type': InteractionTypeEnum;
    static readonly discriminator: string | undefined = "type";
    public constructor() {
    }
}

export enum InteractionTypeEnum {
    Call = 'call',
    Email = 'email',
    Meeting = 'meeting',
    ChatMessage = 'chat-message'
}

E.g. the meaningful diff is:

export class Interaction {
    'type': InteractionTypeEnum;
    static readonly discriminator: string | undefined = "type";
    public constructor() {
-        this.type = "Interaction";
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the explanation! I wonder why there even is such a default value assigned, do you happen to know?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not, sorry, I am assuming it is a remnant from making sure each class has a sane default value to discriminate on, however the implementation of it doesn't make sense. It works fine when the discriminator is of string type, which is the majority of implementations I've seen, but it obviously doesn't work any more as soon as it's a fixed enum.

@@ -0,0 +1,77 @@
openapi: 3.0.0
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 an extract of the OpenAPI spec of https://developer.affinity.co/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for the noise that introducing a new sample fixture produces, I couldn't find a sample OpenAPI spec that shows the issue I am trying to fix. If you know of one that has a discriminator which is an enum, please feel free to point me to it, then I might be able to use it instead and reduce the footprint of this PR a bit.

Comment on lines +67 to +68
public constructor() {
}
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 meaningful part of the fixture (the omission of this.type = "Interaction")

@macjohnny macjohnny merged commit 4238f17 into OpenAPITools:master Aug 29, 2024
@joscha joscha deleted the joscha/do-not-assign-string-to-enum branch August 29, 2024 12:57
joscha added a commit to joscha/openapi-generator that referenced this pull request Aug 30, 2024
macjohnny pushed a commit that referenced this pull request Aug 30, 2024
* Add oneOf model for Typescript generator

* Update import procces: For oneOfs only import $refs within the oneOf

* Remove new line after description

* Update samples

* Typescript: Update model.mustache

* Typescript: Remove emun from oneOf models

* Update samples

* Typescript oneOf: add discriminator and update deserialize procces

* Typescript: update tests

* Typescript oneOf: move type mapping to models

* Typescript: Update samples

* Typescript: Update type of mappig

* Typescript: Update type of mappig

* Typescript oneOf: update deserializing logic

* Typescript: Update samples

* Revert "[typescript] fix: `enum` can not receive stringified class name (#19481)"

This reverts commit 4238f17.

* [typescript] chore: update fixtures

---------

Co-authored-by: ksvirkou-hubspot <[email protected]>
@wing328 wing328 added this to the 7.9.0 milestone Oct 7, 2024
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