Skip to content

[typescript-fetch] Fix discriminator mapping name #4340

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

Conversation

eriktim
Copy link
Contributor

@eriktim eriktim commented Oct 31, 2019

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

The typescript-fetch generator incorrectly using the modelName as the identifier for tagged unions (discriminators). This PR patches that by using the mappingName instead, as this may have been set to an arbitrary value.

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx @macjohnny @nicokoenig @topce @akehir

@auto-labeler
Copy link

auto-labeler bot commented Oct 31, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@eriktim eriktim added this to the 4.2.1 milestone Oct 31, 2019
@macjohnny
Copy link
Member

thanks for the PR. is mappingName always set? what is its value?

@eriktim
Copy link
Contributor Author

eriktim commented Nov 1, 2019

@macjohnny Yes, in the discriminator.mappedModels it is always set. There are some other edge cases with discriminators that do not seem to work at the moment. I'll create new PRs once/if I run into them.

This fix is for the "Mapping Type Names" in https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

@Bessonov
Copy link

Bessonov commented Nov 1, 2019

@eriktim I'm not sure that it's related, but I still get generated converters, which doesn't exists in models. Reproduction:
https://github.com/Bessonov/openapi-bug

git clone https://github.com/Bessonov/openapi-bug
cd openapi-bug
./test.sh

@eriktim
Copy link
Contributor Author

eriktim commented Nov 3, 2019

Ahh I actually do not use oneOf in my spec file. My patch is only for the case where you don't.

But the union type that's being created in your case is great. It seems to lack proper ToJSON/FromJSON functions though. I'm gonna see if I can create another PR for that.

@eriktim eriktim deleted the fix-typescript-discriminator-mapping-name branch November 3, 2019 07:12
@Bessonov
Copy link

Bessonov commented Nov 3, 2019

@eriktim Thank you for your response! I've experimenting a little bit and come out with following content of modelOneOf.mustache:

{{#hasImports}}
import {
    {{#imports}}
    {{{.}}},
    {{/imports}}
} from './';

{{/hasImports}}
{{#discriminator}}
import {
{{#discriminator.mappedModels}}
    {{modelName}}ToJSON,
    {{modelName}}FromJSONTyped,
{{/discriminator.mappedModels}}
} from './';

{{/discriminator}}
/**
 * @type {{classname}}{{#description}}
 * {{{description}}}{{/description}}
 * @export
 */
export type {{classname}} = {{#oneOf}}{{{.}}}{{^-last}} | {{/-last}}{{/oneOf}};

export function {{classname}}FromJSON(json: any): {{classname}} {
    return {{classname}}FromJSONTyped(json, false);
}

export function {{classname}}FromJSONTyped(json: any, ignoreDiscriminator: boolean): {{classname}} {
{{#discriminator}}
    if (!ignoreDiscriminator) {
{{#discriminator.mappedModels}}
        if (json['{{discriminator.propertyName}}'] === '{{mappingName}}') {
            return {{modelName}}FromJSONTyped(json, true);
        }
{{/discriminator.mappedModels}}
    }
{{/discriminator}}
    throw new Error('unknown discriminator');
}

export function {{classname}}ToJSON(value?: {{classname}} | null): any {
    if (value === undefined) {
        return undefined;
    }
    if (value === null) {
        return null;
    }
    {{#discriminator}}
    {{#discriminator.mappedModels}}
    if (value.{{discriminator.propertyName}} === '{{mappingName}}') {
        return {{modelName}}ToJSON(value);
    }
    {{/discriminator.mappedModels}}
    {{/discriminator}}
    throw new Error('unknown discriminator');
}

@eriktim
Copy link
Contributor Author

eriktim commented Nov 3, 2019

Haha nice, I've come up something similar! Wanted to do a final check tomorrow.

@eriktim
Copy link
Contributor Author

eriktim commented Nov 3, 2019

@Bessonov if you want you can already make a PR. I'll probably add some small comments tomorrow. Otherwise I'll create one tomorrow.

@Bessonov
Copy link

Bessonov commented Nov 3, 2019

@eriktim do that in most convenient way for you, I'm not blocked anymore 👍

jimschubert added a commit that referenced this pull request Nov 3, 2019
* 'master' of github.com:OpenAPITools/openapi-generator: (88 commits)
  smaller tests, better code format (#4355)
  csharp-netcore: Replace null literals with default (#4345)
  [core] consider polymorphism when computing unused schemas (#4335)
  Fix issue 4326 forward throws for delegate to main method (#4327)
  [kotlin][client] annotate api exceptions (#4339)
  refactor java-vertx-web parameters and bugfix on non primitive parameter (#4353)
  Remove deprecated API use of ObjectFactory.property() (#2613) (#4352)
  [python][metadata]: Adding license and author fields (#4318)
  [Python] Avoid pep8 violation (#4316)
  [JS] Update package.json (#4261)
  Add slash-arun to Python technical committee (#4354)
  [typescript-fetch] Fix discriminator mapping name (#4340)
  fix security alerts reported by github (#4344)
  fix cpp-restbed-server json field serialization #4320 (#4323)
  typescript-angular: fix oneOf and anyOf generates incorrect model for primitive types (#4341)
  fix(typescript-angular): do not call .toISOString() on a string (#4330) (#4337)
  update samples (#4334)
  Prepare 4.2.0 release (#4333)
  [FEATURE][Haskell] Haskell-Servant serves static files (#4058)
  [FEATURE][Haskell] Add Middleware support for the haskell servant generator (#4056)
  ...
jimschubert added a commit that referenced this pull request Nov 3, 2019
* master: (142 commits)
  smaller tests, better code format (#4355)
  csharp-netcore: Replace null literals with default (#4345)
  [core] consider polymorphism when computing unused schemas (#4335)
  Fix issue 4326 forward throws for delegate to main method (#4327)
  [kotlin][client] annotate api exceptions (#4339)
  refactor java-vertx-web parameters and bugfix on non primitive parameter (#4353)
  Remove deprecated API use of ObjectFactory.property() (#2613) (#4352)
  [python][metadata]: Adding license and author fields (#4318)
  [Python] Avoid pep8 violation (#4316)
  [JS] Update package.json (#4261)
  Add slash-arun to Python technical committee (#4354)
  [typescript-fetch] Fix discriminator mapping name (#4340)
  fix security alerts reported by github (#4344)
  fix cpp-restbed-server json field serialization #4320 (#4323)
  typescript-angular: fix oneOf and anyOf generates incorrect model for primitive types (#4341)
  fix(typescript-angular): do not call .toISOString() on a string (#4330) (#4337)
  update samples (#4334)
  Prepare 4.2.0 release (#4333)
  [FEATURE][Haskell] Haskell-Servant serves static files (#4058)
  [FEATURE][Haskell] Add Middleware support for the haskell servant generator (#4056)
  ...
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Nov 3, 2019
* master: (141 commits)
  smaller tests, better code format (OpenAPITools#4355)
  csharp-netcore: Replace null literals with default (OpenAPITools#4345)
  [core] consider polymorphism when computing unused schemas (OpenAPITools#4335)
  Fix issue 4326 forward throws for delegate to main method (OpenAPITools#4327)
  [kotlin][client] annotate api exceptions (OpenAPITools#4339)
  refactor java-vertx-web parameters and bugfix on non primitive parameter (OpenAPITools#4353)
  Remove deprecated API use of ObjectFactory.property() (OpenAPITools#2613) (OpenAPITools#4352)
  [python][metadata]: Adding license and author fields (OpenAPITools#4318)
  [Python] Avoid pep8 violation (OpenAPITools#4316)
  [JS] Update package.json (OpenAPITools#4261)
  Add slash-arun to Python technical committee (OpenAPITools#4354)
  [typescript-fetch] Fix discriminator mapping name (OpenAPITools#4340)
  fix security alerts reported by github (OpenAPITools#4344)
  fix cpp-restbed-server json field serialization OpenAPITools#4320 (OpenAPITools#4323)
  typescript-angular: fix oneOf and anyOf generates incorrect model for primitive types (OpenAPITools#4341)
  fix(typescript-angular): do not call .toISOString() on a string (OpenAPITools#4330) (OpenAPITools#4337)
  update samples (OpenAPITools#4334)
  Prepare 4.2.0 release (OpenAPITools#4333)
  [FEATURE][Haskell] Haskell-Servant serves static files (OpenAPITools#4058)
  [FEATURE][Haskell] Add Middleware support for the haskell servant generator (OpenAPITools#4056)
  ...
@eriktim
Copy link
Contributor Author

eriktim commented Nov 6, 2019

@Bessonov see #4387

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