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

[Python] handle discriminator inheritance for models #2170

Closed

Conversation

rienafairefr
Copy link
Contributor

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh, ./bin/security/{LANG}-petstore.sh and ./bin/openapi3/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language. @tomplus @Jyhess @wing328

Description of the PR

related issues: #1662 #1663
in the Python models, discriminator_value_class_map wasn't filled when the model had a filled-up discriminator (with or without a mapping). I'm not sure about the children field for the model and what it corresponds, but adding the discriminator mapping in the case this field is not there seems to work.

@wing328
Copy link
Member

wing328 commented Feb 19, 2019

@rienafairefr thanks for the PR. Are you free for a quick chat via https://gitter.im tomorrow (Wed)?

@rienafairefr
Copy link
Contributor Author

Sorry @wing328 took some time to get back to you, yeah ping me up on gitter anytime :-)

@tomghyselinck
Copy link
Contributor

Hi @rienafairefr,

I am testing this PR.

IMHO the mapping should also be applied when there are children defined.

According to OpenAPI Specification 3.0.2:

  • The children can be used for the "schema name" or (default) "implicit" mapping.
  • while the mapping is an additional mapping to identify the (final or upper-layer) objects.

With best regards,
Tom.

@fantavlik
Copy link
Contributor

This is great, thanks @rienafairefr. I do agree with @tomghyselinck that implicit mapping should also be included. To me it appears that a change to the core code is actually needed, the implicit mapping is not being captured here from what I can tell: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L1798-L1825

This is confirmed when I generate this schema:

Thing:
  oneOf:
  - $ref: "#/components/schemas/Foo"
  - $ref: "#/components/schemas/Bar"
  discriminator:
    propertyName: kind

The discriminator template payload is simply:

...
"discriminator" : {
  "propertyName" : "kind",
  "mappedModels" : [ ]
},

Ideally it would be:

...
"discriminator" : {
  "propertyName" : "kind",
  "mappedModels" : [ {
    "mappingName" : "Foo",
    "modelName" : "Foo"
  }, {
    "mappingName" : "Bar",
    "modelName" : "Bar"
  } ]
},

@andreishappy
Copy link

I've run into this issue and would really appreciate a fix. Do you know by any chance when this could be merged? Thanks! ❤️

@spacether
Copy link
Contributor

We have another PR in review which also seeks to improve the discriminator class map here
#4906

@spacether
Copy link
Contributor

This PR has been closed due to inactivity

@spacether spacether closed this Aug 16, 2022
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.

6 participants