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] Fixes additional_properties_type for models #8802

Merged

Conversation

spacether
Copy link
Contributor

@spacether spacether commented Feb 23, 2021

Before this PR, master branch python client code incorrectly set the additional_properties_type of models to None when it was unset, when it should be set to allow any type. Allowing any type is the default behavior when additionalProperties is unset per openapi versions >= 2.
This PR fixes additionalProperties values for models #8771, correctly setting additional_properties_type.

  • updates sample model docs
  • adds test of additionalProperties in Tag
  • fixes Fruit and GmFruit tests
  • a new v2 python sample has been added for disallowAdditionalPropertiesIfNotPresent=true. So now for v2 specs we have one sample for each boolean value of disallowAdditionalPropertiesIfNotPresent. The standard python v2 sample now uses disallowAdditionalPropertiesIfNotPresent=false (the option is omitted which default to false in the python generator)
  • I have updated the discard_unknown_keys test @sebastien-rosset
  • For v3 specs, where additionalProperties: False works, we have a test of it here
  • For v3 specs where a composed schema contains schemas where additionalProperties is False we have tests here

requiredVars have been moved out of the composed schema init signature

requiredVars have been moved out of the composed schema init signature because the required vars were probably defined in the composed oneOf/anyOf/allOf schemas and this schema does not know about them. Every schema is validated separately. So that means that for a composed schema with no type required or properties or additionalProperties definition inside it:

  • all types are valid
  • there are no required properties
  • for object types, all properties are allowed because additionalProperties omitted defaults to additionalProperties: True which allows string property names and any type of value
  • THEN the payload is checked against any oneOf/anyOf/allOf schemas

This change is needed because in our java code, if a oneOf class include required variables, those variable are set in composed schema that includes them. This is incorrect because those variable are not required in the composed schema, they are optional there.
One can see this by looking at the Fruit is a oneOf apple, banana example.
This PR updates it so apple and banana include required parameters. Before making the composed schema init change, those parameters were incorrectly present as positional arguments in Fruit. That's because the java code added those apple and banana requiredVars into Fruit's requiredVars which is incorrect.

v2 Spec Implications

There is a bug in swagger-parser for v2 specs. With that bug, values of True, False, and unset are all seen as null additionalProperties which now becomes AnyType schema.
Previously, the default behavior was to treat all 3 conditions as setting additional_properties_type to None, which did not allow any additional properties to be sent for a given object payload.
In order to have additionalProperties True/False, and unset work correctly, users need to upgrade their specifications to v3.
If users still need the old behavior where additionalProperties was None for additionalProperties=True/False/unset, then they should set disallowAdditionalPropertiesIfNotPresent to True. Doing that will break composition usages oneOf/anyOf/allOf because for that to work, in the composed schema additionalProperties must be any type to allow in properties not defined in the composed schema.properties.
There's another bug where schemas with unset type (AnyType) have their type set to object. This PR works around that bug. If you need to differentiate between AnyType schemas and free form objects (type: object with no properties) then update your spec to v3 or higher.

Breaking Change with Fallback

For v2 specs that use compositiona and were setting disallowAdditionalPropertiesIfNotPresent=True this is a breaking change because composition (allOf schemas) will stop working. If you need composition to work, you must omit setting disallowAdditionalPropertiesIfNotPresent which will then default it to False. Composed schemas will then work. This conforms with the openapi tech spec.

Note: this different PR #8816 will fix it so the variables defined in any schema/model in python only include variables for schema.properties and do not include composed schema properties.

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/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 by passing 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.1.x, 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.

@spacether spacether marked this pull request as draft February 23, 2021 18:05
@spacether spacether linked an issue Feb 23, 2021 that may be closed by this pull request
6 tasks
@spacether spacether changed the title Fixes additionalProperties values for models, updates docs, adds tag … [python] Fixes additionalProperties values for models, updates docs, adds tag … Feb 25, 2021
@spacether spacether changed the title [python] Fixes additionalProperties values for models, updates docs, adds tag … [python] Fixes additionalProperties values for models Feb 25, 2021
@spacether spacether changed the title [python] Fixes additionalProperties values for models [python] Fixes additional_properties_type for models Feb 25, 2021
@@ -10,7 +10,52 @@
'_additional_properties_model_instances',
])

{{> model_templates/method_init_shared }}
@convert_js_args_to_python_args
def __init__(self, *args, **kwargs): # noqa: E501
Copy link
Contributor Author

@spacether spacether Mar 19, 2021

Choose a reason for hiding this comment

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

requiredVars have been moved out of the init signature because the required vars were probably defined in the composed oneOf/anyOf/allOf schemas and this schema does not know about them. Please read why this was done in this PR's description.

@spacether spacether force-pushed the fixes_python_model_addprops_type branch from 8725fe3 to 106d6f4 Compare March 19, 2021 22:46
@spacether spacether marked this pull request as ready for review March 19, 2021 22:58
@spacether spacether added this to the 5.1.0 milestone Mar 19, 2021
@spacether
Copy link
Contributor Author

spacether commented Mar 20, 2021

This PR is blocked by the fact that v2 specs do not correctly set additionalProperties.
I will write a separate PR to add a fix for v2 specs which allows us to default additionalProperties to {} when the input additionalProperties is False or unset.

Update: I merged my PR that sets model.additionalProperties in #9033
So this is unblocked.

@spacether spacether changed the base branch from master to 5.2.x March 26, 2021 16:16
@spacether spacether closed this Mar 26, 2021
@spacether spacether reopened this Mar 26, 2021
@spacether
Copy link
Contributor Author

Re-opening to run CI against the 5.2.X branch

@spacether spacether merged commit 6cc2706 into OpenAPITools:5.2.x Mar 31, 2021
@spacether spacether deleted the fixes_python_model_addprops_type branch April 1, 2021 16:41
the-akhil-nair added a commit to the-akhil-nair/openapi-generator that referenced this pull request Oct 29, 2021
Problem:
The following PR removed the logic for adding the default value
for required_vars in composed model's __init__ and _from_openapi_data
method due to which it is failing in allOf model class stating missing
required positional argument.

OpenAPITools#8802

Now it will become users responsibility to add the required_vars as it will
become mandatory input in the payload.

Since in our application end users may not be knowing these required_vars
all the time, the SDK failure create an issue for us.

Solution:
The proposed solution is to add a configuration variable: setRequiredVars.
If it is set to true, then the older logic will be used. The older logic
was using to set the default value of required_vars in __init__ and
_from_openapi_data as positional arguement.
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] additional_properties_type not set correctly in models
2 participants