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 Client [v2.X] Added inheritance/polymorphism (allOf) to python client #11968

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mjschuetze102
Copy link
Contributor

@mjschuetze102 mjschuetze102 commented Nov 11, 2022

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 and ./bin/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: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This takes some of the changes made to OpenAPI v3.0 and expands on them to add inheritance/ polymorphism to Swagger 2.0

Fixes: #3904, #10912, #11225
Similar fixes could be applied to solve several issues for v3.X

@mjschuetze102 mjschuetze102 changed the title Python Client [v2.X] Added inheritance (allOf) to python client Python Client [v2.X] Added inheritance/polymorphism (allOf) to python client Nov 11, 2022
Comment on lines +263 to +269
@Override
protected void addImport(CodegenModel m, String type) {
if (m.parent != null && m.parent.equals(type)) {
super.addImport(m, type);
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only adds the parent class for importing as the python client does not need to know the type of the values it works with, but does need to import the parent class

Comment on lines +39 to +47
{{#allVars}}
'{{name}}': '{{{datatype}}}'{{#hasMore}},{{/hasMore}}
{{/vars}}
{{/allVars}}
}

attribute_map = {
{{#vars}}
{{#allVars}}
'{{name}}': '{{baseName}}'{{#hasMore}},{{/hasMore}}
{{/vars}}
{{/allVars}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the way the deserializer works in the api_client, the parent values must be present in swagger_types and attribute_map

@@ -52,7 +54,7 @@ class {{classname}}(object):
}
{{/discriminator}}

def __init__(self{{#vars}}, {{name}}={{#defaultValue}}{{{defaultValue}}}{{/defaultValue}}{{^defaultValue}}None{{/defaultValue}}{{/vars}}, _configuration=None): # noqa: E501
def __init__(self{{#allVars}}, {{name}}={{#defaultValue}}{{{defaultValue}}}{{/defaultValue}}{{^defaultValue}}None{{/defaultValue}}{{/allVars}}, _configuration=None): # noqa: E501
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly because the values for the model are defined with the constructor rather than using setters for each property, the parent vars must be passed in the constructor as well as the properties belonging to this model

Comment on lines 78 to 81
{{#parent}}
super().__init__({{#parentModel.vars}}{{name}}{{#hasMore}}, {{/hasMore}}{{/parentModel.vars}}) # noqa: E501

{{/parent}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parent constructor is called with the values passed in to the constructor of the child

Copy link

Choose a reason for hiding this comment

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

I believe this should go up to the first line below the init function so that calling the parent constructor does not wipe out any properties that were overridden by the current child class.

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 believe I had reasoned that it didn't matter much because the code was using vars for the child class properties and parentModel.vars for properties on the parent. Since the child class never worked with parentModel.vars directly it would be fine. However, the discriminator (and any other code that is added in the future) might not realize this, so I changed the order of events

@jcgentr
Copy link

jcgentr commented Jan 20, 2023

Can this be merged? This would be extremely helpful for Python devs using this tool.

Relocated the parent constructor call so that values set at the child level are not overwritten by values set at the parent level
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is polymorphism supported in code generation?
2 participants