[Python] add additionalProperties support, Option2 #2049

spacether · 2019-02-04T03:07:52Z

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: 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.
@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01)

Description of the PR

Adds support for additionalProperties to the Python generator
Fixes inheritance in python when AllOf is used
Adds additionalProperties codegenParameter to the model that the mustache files have access to in case we want to use the schema for examples in the future
Adds the ability to access and assign values to a model instance using brackets. model_instance['var_name'] = var_val
Simplifies model instance variable storage by storing all values in a data dict. This will make it easier to send Nullable value=None items, because items in the data store are correct
Switches parameter and discriminator typing to use actual python classes
Adds type checking with TypeError/ValueError/KeyError exceptions which tell where in the data the problem is (data is in the exception string and exception.path_to_item)
- Model instances can be instantiated with type checking with the _check_type variable
- Endpoint hits can check type before sending out data with the _check_type variable
- Deserialization automatically checks type on all received data

Issues that will be closed if this is merged:

Inheritance not working: [Python] Inheritance doesn't work #623
When model only has allOf, it is blank: [Python] Generated model for component with "allOf" has no attributes #453
Fixes additionalproperties: [BUG] [PYTHON] additionalProperties not supported #2028

New Changes

Model variable types, discriminators values, and response types are now all classes
api_client call_api method keyword argument changed from response_type to response_types_mixed
Mode instantiation:
- required arguments with no enums of length one are positional
- required arguments WITH enums of length one are set as keyword arguments with defaults = the enum value
- option arguments are keyword argument **kwargs
- This should be completely backwards compatible if people are using all keyword arguments which still works

Breaking Changes:

Received date and date-time data that is not parsable will now raise an ApiValueError which can be captured with ValueError, Exception, ApiValueError, or OpenApiException. Date/date-time variable will not fall back to string values. Instead one must set the type of that variable in the spec equal to {} which allows that variable to hold any of the openapi types. An example of this can be seen at: samples/client/petstore/python/tests/test_deserialization.py::test_deserialize_str_to_date_failure

Note:

This is an alternative solution to this PR: #2029

ivan-gomes · 2019-02-04T18:07:50Z

modules/openapi-generator/src/main/resources/python/model.mustache

+        passed_type = self.recursive_type(value)
+        if type(name) != str:
+            raise TypeError('Variable name must be type string and %s was not' % name)
+        elif passed_type != required_type and check_type:


Thank you very much for creating this pull request! I'm the author of #2028 and #2029 and I think the solution you have here is cleaner and more robust.

Just one minor issue I've noticed so far. I tested it with my use case of

additionalProperties: type: "object"

which caused this error to be raised when doing model["foo"] = "bar"with ValueError: Variable value must be type object but you passed in str. It looks like the type checking isn't taking into account subclasses. I would really appreciate if you could address this use case. I would be happy to test for you as well.

Hi Ivan. You're welcome. Thank you for yours. We both had the same idea to add a dict to store the values, this pull request just implemented it as a general case. Thanks for the heads up about that failing case. To fix it, I will add more samples. For the example you gave the code was right to raise an error, because you should have passed in:
model["foo"] = {"some_var": "bar"}
But the error message was wrong in the type that it was demanding. It should have wanted a dict rather than an object. We collapse complex schema types down elsewhere, so I will find that code and use it.

The intent of additionalProperties with type object is that any value, regardless of type, is accepted. Upon review of the OpenAPI spec I should be using additionalProperties: true to represent that intent (source: https://swagger.io/docs/specification/data-models/dictionaries/). I will try this case out for us.

When additionalProperties: true is used, the codegen treats it as if additionalProperties is not enabled. I suspect that the fact that its type isn't defined causes it to be ignored.

Ivan, what does inspecting the values in the openapi spec show when we set additionalProperties to either of the below values?

additionalProperties: true

additionalProperties: {}

If we see it in those results, we can handle it in the java code.
One can inspect the values in the openapi spec with the below line:

java -DdebugOpenAPI -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -t modules/openapi-generator/src/main/resources/python -i modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml -g python -o samples/client/petstore/python -DpackageName=petstore_api

Ivan I am figuring out how to add this empty object -> all types.
This is also complicated by the fact that any type can have nullable set to true, which would mean that we could accept types like (string) (string OR None) or all the way up to (string, boolean, integer, float, list, dict, None).

For now I will leave nullable out because it is a 3.0 spec feature.
We still will have cases were "str|bool|int|float|list|dict" would be the type or you could have that nested as the innermost value in a nested list or dict schema, like:
dict(str, dict(str, str|bool|int|float|list|dict))

@wing328 @ivan-gomes I am still working on this. Please refrain from merging until I have updated this PR. My update will include the type=object additionalProperties feature.

Roger. Please let me know if there is anything I can do to help.

@ivan-gomes will do.
Still to do is:

Ensure deserialization works w/ tests (only file tests remaining)

Fix the other language tests that I broke by changing the additionalpropertiesclass property names

@wing328 @ivan-gomes I am nearing completion on this task.
To get this feature to work I added typing of all parameters and discriminators using real python classes and type checking. Soon I will get this PR submitted.
This is a big change and I welcome all feedback and discussion on it.

…er a variable name

…s on all model parameters

…use in model.to_dict and serialization

…aises ApiTypeError, openapi3 python nullable class tests added

… and code, partial work on api_client deserialization

…pi_client, tests not passing yet

…pass

…t_enum_test

ivan-gomes · 2019-03-06T19:20:26Z

Ran it against the API spec that caused me to create the issue, and it works great! Thanks so much for your effort.

My humble opinion is that the changes involved are quite interconnected and trying to retroactively decouple them into separate PRs would be significant work with little practical purpose.

spacether · 2019-03-12T03:06:51Z

Per a discussion with @wing328 this PR is a bit big. I will break it up to smaller pieces for ease of review.

tomghyselinck · 2019-03-13T14:27:23Z

Hi @spacether,

I have made some additional changes to make things work for (multi-level) inheritance:
https://github.com/tomghyselinck/openapi-generator/tree/python_additionalprops_option2

I use this in combination with #2170 to also make the disciminator.mapping work as expected.
I also made a small change on that one: See rienafairefr#2 (based on https://github.com/tomghyselinck/openapi-generator/tree/python-discriminator-map)

With best regards,
Tom.

spacether · 2019-03-13T14:46:37Z

Thanks for doing that @tomghyselinck Can you explain how you can use vars rather than allvars in the model mustache file for allof models?
Do you mind if we incorporate your updates in a PR in or along with the above 5 smaller suggested PRs.

tomghyselinck · 2019-03-13T16:47:21Z

Hi @spacether,

Thank you for your quick response.

Regarding the `allVars`:

A language-specific implementation of DefaultCodegen can set the supportsInheritance, (and eventually supportsMultipleInheritance) to true when it supports inheritance in the generated models.
In that case the properties from "parent" schemas are only added to the allVars (and not to the vars).

The PythonClientCodegen did set supportsInheritance to true so it needed to use the allVars.
IMHO this is not the intent of the supportsInheritance vs. allVars use.

Please note that I also discovered an issue in the use of allVars: DefaultCodegen.fromModel does not call postProcessModelProperty on the allVars.
This especially causes issues when postProcessModelProperty adds additional item to for example CodeGroperty.vendorExtensions.
In particular we had an issue where our regex pattern was not set (vendorExtensions.x-regex and vendorExtensions.x-modifiers)

I have not yet created a BUG report for the latter issue.

Incorpration

Please go ahead and incorporating my changes in the way that fits best for you 😉.

tomghyselinck · 2019-03-13T17:42:37Z

Hi @spacether,

I just want to let you know that there is still a problem with deserializing enum models.
I have been working on it, but I did not find a proper solution yet.

I have a quick and dirty workaround:
tomghyselinck@40658ca

There are two alternative workarounds (or solutions?) in this commit:

Change the dataType to (MyEnumType, str) instead of (MyEnumType, )
Change the dataType to (str, ) instead of (MyEnumType, ) (commented out)

Both alternative changes trigger a warning WARN o.o.codegen.DefaultCodegen - allOf with multiple schemas defined. Using only the first one: <some_type>. To fully utilize allOf, please use $ref instead of inline schema definition.
I have no clue why yet...

With best regards,
Tom.

spacether · 2019-03-13T18:24:07Z

Thanks @tomghyselinck
Yes, I know about that. My above comment where we fold non object models into their referenced models and moving enum allowable values to the class level (the first time that I have mentioned this) should fix that.
class_name.allowed_values['enum_propery'] = [
'option 1 value',
'option 2 value'
]

Or maybe we should keep enum only classes like shown here: https://docs.python.org/3/library/enum.html
I need to think about this more.

Some non-object models are:

type string/array/number etc enum models
type string/array/number etc and contain enums and other validation
type string/array/number etc and contain NO enums and contain other validation

Are we able to tell the difference between the three above 3 options from the current debugged model description?
It feels like making enum classes is overly complicated when the above solution stores the same data and simplifies the type declaration.
Another question, for non-object model number 2, where do we store additional validation? The enum class or the class that uses it?
What do you think @tomghyselinck

tomghyselinck · 2019-03-13T19:19:20Z

Thank you for the explanation.

(I intended to ask if #1991 was the solution for this, but my comment was already in the air 🙂)

spacether · 2019-03-18T03:58:13Z

Break out PR-statuses:

[APPROVED + MERGED] python create exceptions module where all exceptions will inherit from OpenAPiException Adds exceptions module to python clients #2393
[APPROVED + MERGED] add additionalProperties examples Updates the toInstantiationType method in the csharp generator, Adds spec additionalProperties + nullable examples #2405
[SUBMITTED] python handle non-object models with validations and enums [python-experimental] generate model if type != object if enums/validations exist #2757
python change types from strings to classes + add type checking
python add aditionalproperties feature and aditionalproperties where type == {} which accepts all types

mverderese · 2019-03-29T23:24:52Z

Hey @spacether! This is exactly the functionality I'm looking for in the codegen tools for the python client. I have a model defined with a couple of rigid properties, and then want to be able to support additional properties of a certain type. If that's not possible, then aliasing a Dict[str, <type>] as a Model would be great. I'm a little lost as to what the status of this PR is, so any clarity would be greatly appreciated! Also if you need any help getting this over the finish-line, let me know!

spacether · 2019-03-30T05:03:14Z

Hey @mvrderese this PR is being broken up into smaller pieces so it is easier to review. I would welcome some help. If you want to collaborate please shoot me an email through the link at the bottom of my page here https://spacether.github.io/

ivan-gomes · 2019-04-23T17:24:14Z

Hi @spacether. Can we get an update on where we are on this feature? It addresses the last blocker I have on migrating a project to openapi-generator.

spacether · 2019-04-23T20:31:18Z

Hi @spacether. Can we get an update on where we are on this feature? It addresses the last blocker I have on migrating a project to openapi-generator.

Hey @ivan-gomes I am still working on this.
The remaining tasks are:

handle non-object type models
- STATUS: PR is in merged here: [python-experimental] generate model if type != object if enums/validations exist #2757
convert data types to classes + add additionalProperties
- STATUS: SUBMITTED at [Python-experimental] types now classes, adds additionalProperties handling #4154

wing328 · 2019-05-20T02:18:36Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/PythonClientCodegen.java

+    public String getTypeString(Schema p, String prefix, String suffix) {
+        // this is used to set dataType, which defines a python tuple of classes
+        String typeSuffix = suffix;
+        if (suffix == ")") {


Please use ")".equals(suffix) instead

Updated per feedback here https://github.com/spacether/openapi-generator/tree/issue_1991_uses_class_types

wing328 · 2019-05-20T02:19:00Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/PythonClientCodegen.java

+            return prefix + "[" + getTypeString(inner, "(", ")") + "]" + typeSuffix;
+        }
+        String baseType = getSimpleTypeDeclaration(p);
+        if (baseType == "file") {


There's no file type in OAS v3 as it has been replaced by "type: string, format: binary".

What about using ModelUtils.isBinarySchema to check instead

This code only exists to handle v2 file schemas, so applying it to ModelUtils.isBinarySchema would target a different use case than we want. I am replacing this with ModelUtils.isFileSchema(p) here https://github.com/spacether/openapi-generator/tree/issue_1991_uses_class_types

wing328 · 2019-05-20T02:21:43Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/PythonClientCodegen.java

+
+    @Override
+    public String toInstantiationType(Schema property) {
+        if (property instanceof ArraySchema || property instanceof MapSchema || property.getAdditionalProperties() != null) {


Please use ModelUtils.isArraySchema instead of property instanceof ArraySchema. Same for the map schema check using instanceof at the moment.

Implemented this feedback here https://github.com/spacether/openapi-generator/tree/issue_1991_uses_class_types

spacether · 2019-09-01T07:22:14Z

Unable to get necessary reviews and approvals on this PR and feature break out PRs.

ivan-gomes · 2019-09-10T17:03:58Z

That is a real shame considering this is a very painful bug and you put a lot of work into fixing it. Is there anything that can be done? Tagging @wing328 for feedback as well.

spacether · 2019-09-10T17:24:53Z

This is something that I really wanted to complete.
Feedback was that my original PR was too big so I broke it into pieces.
Feedback was then that I should put it in a new generator which I did.
My last PR was open for a month ready for merge with pings asking for review. This feature branch represents 100+ hours of investment of my time. But if it is not reviewed and approved there is nothing I can do. It makes me feel bad when I have made such an investment (100+ hours of dev time) and the code is ignored for a month. The last commitment was a review by Friday and the review was not done.

spacether · 2020-04-15T00:05:57Z

The code in this PR was merged into master in the new generator python-experimental

spacether changed the title ~~Python additionalprops option2~~ Python add additionalProperties support, Option2 Feb 4, 2019

spacether changed the title ~~Python add additionalProperties support, Option2~~ [Python] add additionalProperties support, Option2 Feb 4, 2019

ivan-gomes reviewed Feb 4, 2019

View reviewed changes

spacether mentioned this pull request Feb 6, 2019

[BUG] [Python] Models with only additionalProperties are not generated #2078

Closed

6 tasks

wing328 added Client: Python WIP Work in Progress labels Feb 9, 2019

spacether force-pushed the python_additionalprops_option2 branch 2 times, most recently from 3f6d7b8 to 3901164 Compare February 18, 2019 20:02

spacether added 22 commits March 2, 2019 12:40

Adds additionalproperties feature in python

5bc9540

Adds ensure-up-to-date updates

64d5444

Adds docstring description of the model argument inputs

ec73f56

Adds ensure up to date oepnapi3 python updates

5cdfe51

Adds test fix

0a52027

Adds fix for Shippable, gives the additionalProperties CodegenParamet…

8be21b0

…er a variable name

Adds function to set freeform types to string of all types

ba7ea49

Adds postProcessAllModels function which processes and fixes dataType…

88f8197

…s on all model parameters

Adds models with additionalproperties of each type and model tests

0590d05

Adds _check_type parameter to model, adds additionalproperties tests

372ae60

Creates utils module, adds additionalproperties map for serialization

5b4f108

Removes additional_properties_map, creates model_to_dict function to …

2e33cf1

…use in model.to_dict and serialization

Improves docstring for model_to_dict

983f570

Adds class type definition in models

f218ed5

Adds datetime and date type classes, adds general OpenApiException, r…

7656756

…aises ApiTypeError, openapi3 python nullable class tests added

Adds class imports to models, python generator uses dataType for docs…

7ac9285

… and code, partial work on api_client deserialization

Model discriminator now stores classes, adds __deserialize_model in a…

67dbb9c

…pi_client, tests not passing yet

Creates exceptions module, all deserialization tests in py2 except 1 …

9680dfd

…pass

Adds validate_and_convert_types, python 2.0 tests pass except for tes…

05fe1aa

…t_enum_test

Adds anytype deserialization tests in python swagger client

e8f97fa

Fixes typos

59b2d4c

Adds file deserialization test

e158601

tomghyselinck mentioned this pull request Mar 11, 2019

[Python] handle oneOf/allOf/anyOf composed schemas in the Python client #2121

Closed

4 tasks

This was referenced Mar 14, 2019

Adds exceptions module to python clients #2393

Merged

Updates the toInstantiationType method in the csharp generator, Adds spec additionalProperties + nullable examples #2405

Merged

spacether mentioned this pull request Apr 28, 2019

[python-experimental] generate model if type != object if enums/validations exist #2757

Merged

4 tasks

wing328 reviewed May 20, 2019

View reviewed changes

patrickkwang mentioned this pull request Aug 12, 2019

additionalProperties? NCATSTranslator/ReasonerAPI#79

Closed

spacether closed this Sep 1, 2019

spacether mentioned this pull request Oct 14, 2019

[Python-experimental] types now classes, adds additionalProperties handling #4154

Merged

5 tasks

ivan-gomes mentioned this pull request Jan 29, 2020

[PYTHON] Issue #2028 - additionalProperties support #2029

Closed

4 tasks

spacether mentioned this pull request May 22, 2020

[codegen][python-experimental] Composed schema with additionalProperties #6290

Merged

5 tasks

spacether deleted the python_additionalprops_option2 branch September 1, 2020 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Python] add additionalProperties support, Option2 #2049

[Python] add additionalProperties support, Option2 #2049

spacether commented Feb 4, 2019 •

edited

Loading

ivan-gomes Feb 4, 2019 •

edited

Loading

spacether Feb 4, 2019 •

edited

Loading

ivan-gomes Feb 4, 2019

ivan-gomes Feb 4, 2019

spacether Feb 4, 2019 •

edited

Loading

spacether Feb 5, 2019

spacether Feb 9, 2019

ivan-gomes Feb 11, 2019

spacether Feb 11, 2019 •

edited

Loading

spacether Feb 28, 2019 •

edited

Loading

ivan-gomes commented Mar 6, 2019

spacether commented Mar 12, 2019

tomghyselinck commented Mar 13, 2019

spacether commented Mar 13, 2019 •

edited

Loading

tomghyselinck commented Mar 13, 2019

tomghyselinck commented Mar 13, 2019

spacether commented Mar 13, 2019 •

edited

Loading

tomghyselinck commented Mar 13, 2019

spacether commented Mar 18, 2019 •

edited

Loading

mverderese commented Mar 29, 2019

spacether commented Mar 30, 2019

ivan-gomes commented Apr 23, 2019

spacether commented Apr 23, 2019 •

edited

Loading

wing328 May 20, 2019

spacether Sep 30, 2019

wing328 May 20, 2019 •

edited

Loading

spacether Sep 29, 2019 •

edited

Loading

wing328 May 20, 2019

spacether Sep 30, 2019

spacether commented Sep 1, 2019

ivan-gomes commented Sep 10, 2019

spacether commented Sep 10, 2019 •

edited

Loading

spacether commented Apr 15, 2020

[Python] add additionalProperties support, Option2 #2049

[Python] add additionalProperties support, Option2 #2049

Conversation

spacether commented Feb 4, 2019 • edited Loading

PR checklist

Description of the PR

Issues that will be closed if this is merged:

New Changes

Breaking Changes:

Note:

ivan-gomes Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

spacether Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether Feb 11, 2019 • edited Loading

Choose a reason for hiding this comment

spacether Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

ivan-gomes commented Mar 6, 2019

spacether commented Mar 12, 2019

tomghyselinck commented Mar 13, 2019

spacether commented Mar 13, 2019 • edited Loading

tomghyselinck commented Mar 13, 2019

Regarding the allVars:

Incorpration

tomghyselinck commented Mar 13, 2019

spacether commented Mar 13, 2019 • edited Loading

tomghyselinck commented Mar 13, 2019

spacether commented Mar 18, 2019 • edited Loading

mverderese commented Mar 29, 2019

spacether commented Mar 30, 2019

ivan-gomes commented Apr 23, 2019

spacether commented Apr 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wing328 May 20, 2019 • edited Loading

Choose a reason for hiding this comment

spacether Sep 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacether commented Sep 1, 2019

ivan-gomes commented Sep 10, 2019

spacether commented Sep 10, 2019 • edited Loading

spacether commented Apr 15, 2020

spacether commented Feb 4, 2019 •

edited

Loading

ivan-gomes Feb 4, 2019 •

edited

Loading

spacether Feb 4, 2019 •

edited

Loading

spacether Feb 4, 2019 •

edited

Loading

spacether Feb 11, 2019 •

edited

Loading

spacether Feb 28, 2019 •

edited

Loading

spacether commented Mar 13, 2019 •

edited

Loading

Regarding the `allVars`:

spacether commented Mar 13, 2019 •

edited

Loading

spacether commented Mar 18, 2019 •

edited

Loading

spacether commented Apr 23, 2019 •

edited

Loading

wing328 May 20, 2019 •

edited

Loading

spacether Sep 29, 2019 •

edited

Loading

spacether commented Sep 10, 2019 •

edited

Loading