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] GetItem not working for Client generated allOf model and broken since 5.2.0 #12239

Merged
merged 7 commits into from
May 28, 2022

Conversation

the-akhil-nair
Copy link
Contributor

Problem:
The issue is due to the creation of self._var_name_to_model_instances while doing the deserialization of the data.

Earlier the Python SDK code was using get_var_name_to_model_instances function which was adding var name to model instances that contain it.

Now the var_name_to_model_instances is populated with self and composed_instance and there will be no check if a variable name is present in composed_instances or not.

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 responses from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    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 bypassing the relevant config(s) as an argument to the script, for example, ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
    @taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @arun-nalla (2019/11) @spacether (2019/11) @wing328

Fixes #11299

…stances while doing the deserialization of the data.

Earlier the Python SDK code was using get_var_name_to_model_instances function which was adding var name to model instances that contain it. So <class 'openapi_client.model.stream_options_all_of'> will not part of mapping in self._var_name_to_model_instances for variable name stream_options.

Now with the latest Python SDK code following is the way through which var_name_to_model_instances is created:

    for prop_name in model_args:
        if prop_name not in discarded_args:
            var_name_to_model_instances[prop_name] = [self] + composed_instances
Now as we can see that the var_name_to_model_instances is populated with self and composed_instance which will also contain stream_options_all_of as a composed instance and there will be no check that if stream_options is present in composed_instances or not.

As there is no attribute_mapping found for stream_options in stream_options_all_of, the type for stream_options will be treated as dict for mapping stream_options_all_of as mentioned by @Chekov2k.

So what I suggest is the following code:

    for prop_name in model_args:
        if prop_name not in discarded_args:
           var_name_to_model_instances[prop_name] = [self] + list(
                filter(
                    lambda x: prop_name in x.openapi_types, composed_instances))
This way we can check if the property name is present in that composed instance or not. If it's okay for @spacether I can raise a PR for this.
Added samples, test cases to validate all_of schema.
…into getiem_all_of_bug

# Conflicts:
#	samples/openapi3/client/petstore/python/.openapi-generator/FILES
#	samples/openapi3/client/petstore/python/README.md
#	samples/openapi3/client/petstore/python/petstore_api/models/__init__.py
Updated docs and samples.
Updated test cases, docs and samples.
var_name_to_model_instances[prop_name] = [self] + composed_instances
var_name_to_model_instances[prop_name] = [self] + list(
filter(
lambda x: prop_name in x.openapi_types, composed_instances))
Copy link
Contributor

Choose a reason for hiding this comment

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

So technically in every instance of the payload prop_name should be present but this code did work before so I am okay with this being merged back in.

@spacether spacether added this to the 6.0.1 milestone May 28, 2022
Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

Thank you for your PR. Looks good.

@spacether spacether merged commit e823290 into OpenAPITools:master May 28, 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.

[BUG][Python] Client generated allOf model broken since 5.2.0
2 participants