[Python] Adds allOf/oneOf/anyOf composed models #4446

spacether · 2019-11-10T23:45:15Z

This PR adds composed schema (allOf/oneOf/anyOf) support to the python-experimental generator. All composed schema instances will now store instances of their composed schema models under the hood, then use them when we get attributes, set attributes, or convert the model instance to a dict for serialization.
Common model methods have been moved into parent classes with a net reduction of -3,800 lines

Test Verification

In depth tests on the Dog allOf class have been added at:

https://github.com/spacether/openapi-generator/blob/allof_anyof_oneof_addition/samples/client/petstore/python-experimental/test/test_dog.py#L33

A test showing that allOf parameters are accessible on an instance of Dog is at:

https://github.com/spacether/openapi-generator/blob/allof_anyof_oneof_addition/samples/client/petstore/python-experimental/tests/test_deserialization.py#L120

Circular Reference Problem and Solution

Note: class names in dataTypes must look like pet.Pet so we can load modules with lines like
from package.models import pet
at the top of the module, then we can use pet.Pet as the data type later in the model definition.
This is specifically needed for models with composed schemas because those models require circular references.

For example:
Animal uses a Cat discriminator, so the animal module imports cat
The cat module uses allOf Animal in its composed schema definition so it imports animal

The only way to make the circular imports work is using the below two techniques:

in cat load the animal module in with a try except import error catch, where if we get an import error we will set the animal variable to the partially loaded animal module in sys
delay our usage of the composed schemas (animal.Animal) by putting them in the model _composed_schemas method. That way, the code is not run until the method is invoked. If we put animal.Animal into Cat's module level constants, because the animal module is not yet fully loaded, animal.Animal does not exist and the code would not run.

Related Issues

This may fix the below issues if the end users use the python-experimental generator:

Potential Future Work

If this PR is accepted, later on I can submit a PR adding a python-experimental client example for the openapi spec demonstrating that oneOf and anyOf also work.

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project before.
Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
Copy 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) @slash-arun (2019/11)

Authors of related work:
@rienafairefr per #2170

eak24 · 2019-11-19T21:39:56Z

Very excited to see this go in! Is there anything I can do to help it along?

spacether · 2019-11-19T22:00:01Z

Very excited to see this go in! Is there anything I can do to help it along?

Just waiting on a review and approval on this PR

spacether · 2019-11-22T17:45:59Z

Python team:
@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @slash-arun (2019/11)
Gentle reminder that this still needs a review

Adding composed schema models adds a significant openapi spec feature to our python client ecosystem
If it would help I can screen share with anyone to walk them through the updates.

spacether · 2019-11-30T20:55:35Z

Python team:
@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @slash-arun (2019/11)
Gentle reminder that this still needs a review

jimschubert

Overall, I think this looks good. I'm not current with dynamic programming paradigms in Python, so I can't comment on the design decisions. It's similar to what I've seen in Ruby and PHP code that I've worked on, though. The "safe import" is similar to things I've seen in node.js. It will be good to get this merged and get community feedback.

As an experimental generator, I think it's safe to merge into any version. However, the README_common rename will affect the stable python generators, and would need to be changed back before merge (see inline comment). The rest of my comments are trivial code quality suggestions.

jimschubert · 2019-12-04T00:37:22Z

...erator/src/main/java/org/openapitools/codegen/languages/PythonClientExperimentalCodegen.java

@@ -65,6 +65,9 @@ public PythonClientExperimentalCodegen() {
        modelTemplateFiles.remove("model.mustache");
        modelTemplateFiles.put("python-experimental/model.mustache", ".py");

+        modelTestTemplateFiles.remove("model_test.mustache", ".py");


Side note (not blocking for this PR): we should probably add a helper to remove by template name. For modelTestTemplateFiles, I think it's pretty simple, but the supportingFiles.remove calls later in this file might fail on hash conflicts or if a property is added to SupportingFile without updating hash/equals.

That sounds like a good idea. Let's keep that as a future improvement in a separate PR.

...erator/src/main/java/org/openapitools/codegen/languages/PythonClientExperimentalCodegen.java

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

samples/client/petstore/python-experimental/docs/UserApi.md

eak24 · 2019-12-05T05:28:26Z

@spacether I just tried this PR out and I ran into the following issue. When specifying a discriminator property that is defined in the superclass, during deserialization, the client is unable to find the mentioned method in the list of attributes. Would there be a simple way to search through that list of attributes down to the root type? Example of a failing schema:

    SuperClass:
      type: object
      properties:
        myTypeDiscriminator:
          type: string
    SubClass:
      type: object
      properties:
        myTypeDiscriminator:
          type: string
      allOf:
        - $ref: '#/components/schemas/SuperClass'
    SubSubClass:
      type: object
      allOf:
        - $ref: '#/components/schemas/SubClass'
      discriminator:
        propertyName: 'myType'
        mapping:
          ...

The issue is that when SubSubClass attempts to resolve the myType property, it only looks in its own attribute map, and doesn't seem to look in the properties pulled in from the super. Let me know if this isn't clear enough. Certainly not a blocker - just wanted to mention this functionality is lacking.

spacether · 2019-12-05T16:10:42Z

@ethan92429 thank you for letting me know about this.
Let me think about this and consider how to get it working.
My addition is not yet considering discriminator class mapping. In the future I plan on adding it.
This diff: #4454
Is working on improving ancestor handling in composed schema models.

spacether · 2019-12-07T05:42:33Z

@ethan92429 I got ancestor inheritance working by changing my mustache template vars variables into requiredVars and optionalVars. In the test folder I added a Child composed schema model showing it working at:

@ethan92429 Can you explain your example more? I don't understand it.
Do the parent class and the grandparent class need to share the same property name or does my example cover your use case?
Are you sure that your discriminator usage is correct?
Where does your example include the myType variable in the ancestor models as a schema that is selected by the discriminator?
If you only use allOf, why use a discriminator? The example I see here show the discriminator applying to schemas at the same level as the discriminator: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#discriminatorObject

eak24 · 2019-12-07T13:36:57Z

Hi @spacether glad to hear that you got this working - just to clarify the example that I couldn't get to work (I'm trying again with the example specified in the OAS)

components:
  schemas:
    Animal:
      type: object
      required:
      - pet_type
      properties:
        pet_type:
          type: string
    Pet:
      type: object
      allOf:
      - $ref: '#/components/schemas/Animal'
      discriminator:
        propertyName: pet_type
        mapping:
          cachorro: Dog
    Cat:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Cat`
        properties:
          name:
            type: string
    Dog:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Dog`
        properties:
          bark:
            type: string
    Lizard:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Lizard`
        properties:
          lovesRocks:
            type: boolean

Basically, I have one single property that will be used as a discriminator defined in the base class of my deeply nested polymorphic tree (pet_type on Animal), and I need all the polymorphic children (Pet) to use that property as their discriminator, but the Pet model can't find it because it is an inherited property. Put simply, I need inherited properties to be considered as valid discriminator mappers if they are included in allOf. Does that clarify? I was able to actually bypass this by just including pet_type in every model that I export, but it is a bit verbose.

spacether · 2019-12-08T19:22:10Z

@ethan92429 your example is not working because of a bug in swagger-parser. I have opened a ticket to track that bug here: swagger-api/swagger-parser#1269
I am still working on a way to get your example to work.
@ethan92429 if you delete Animal and update your Pet class to be:

    Pet:
      type: object
      required:
      - pet_type
      properties:
        pet_type:
          type: string
      discriminator: pet_type

Then your example should work.
A discriminator using an inherited property should work though, still working on it...

…rialize_lizard, adds setting discriminator using allOf schema

spacether · 2019-12-08T22:02:44Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

                            LOGGER.debug("discriminator is set to null (not correctly set earlier): {}", name);
-                            m.discriminator = createDiscriminator(name, schema);
+                            m.discriminator = createDiscriminator(name, innerSchema);


Per here: https://swagger.io/specification/#discriminatorObject
We should be able to set the discriminator on the parent composed schema, but that's not yet working because of this bug in swagger parser: swagger-api/swagger-parser#1269
In place of having discriminator at the root level of a composed schema, we should be able to use it from our definition in our allOf schemas.
I updated this code to set the discriminator on self if one of its ancestor allOf schemas defines discriminator.
Please see my ParentPet class which demonstrates this usage at: https://github.com/spacether/openapi-generator/blob/allof_anyof_oneof_addition/modules/openapi-generator/src/test/resources/2_0/python-client-experimental/petstore-with-fake-endpoints-models-for-testing.yaml#L2072

NOTE: the original invocation here m.discriminator = createDiscriminator(name, schema); did not do anything because we already ran that exact line above in line 1772.

I think this seems reasonable, but I'd like to have someone else on the @OpenAPITools/generator-core-team review as well since it affects all generators.

spacether · 2019-12-08T22:04:24Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

                        }

                        if (innerSchema.getXml() != null) {
                            m.xmlPrefix = innerSchema.getXml().getPrefix();
                            m.xmlNamespace = innerSchema.getXml().getNamespace();
                            m.xmlName = innerSchema.getXml().getName();
                        }
+                        if (modelDiscriminators > 1) {
+                            LOGGER.error("Allof composed schema is inherriting >1 descriminator. Only use one discriminator: {}", composed);
+                        }


We should error out if the user tries to define multiple discriminators in one schema.

spacether · 2019-12-08T22:09:00Z

@ethan92429 in this branch I:

got your example working
added it to the python-experimental sample spec https://github.com/spacether/openapi-generator/blob/allof_anyof_oneof_addition/modules/openapi-generator/src/test/resources/2_0/python-client-experimental/petstore-with-fake-endpoints-models-for-testing.yaml#L2072
added a test of it https://github.com/spacether/openapi-generator/blob/allof_anyof_oneof_addition/samples/client/petstore/python-experimental/tests/test_deserialization.py#L124

spacether · 2019-12-09T01:31:52Z

@jimschubert can you give this another look? Your feedback has been addressed and all tests are passing.

jimschubert

I've skimmed the Python code since it's not my forte. My concerns and previous comments have been addressed. The explanation of the change in DefaultCodegen seems reasonable but I'd also like someone else on the core team to review since I have not worked much on the code processing allOf and discriminators.

edit: if there's no response in a day or two from others, I'll go ahead and merge since the change in the file is so small.

eak24 · 2019-12-29T00:28:35Z

@spacether I've been playing around with python-experimental and ran into a hitch with using it for my spec. The problem is that with the changes to have the class property types used directly, circular type referencing or recursive definitions doesn't seem to work anymore:

components:
  schemas:
    Dog:
      properties:
        enemy:
          type: object
          $ref: '#/components/schemas/Dog'

This should be a valid spec, but the generated python attempts to reference the Dog class in the definition of the Dog class - which clearly doesn't work. This causes a circular import. This looks to be a regression from just the plain python generator - which uses strings to define openApi types. What are your thoughts on this?

spacether · 2019-12-29T06:03:10Z

@ethan92429 can you file a bug on your use case where a model has a property which is of type self?
I can fix it by storing a model's parameter types as a class method rather than a dict. Then they will not be loaded at module load time.

spacether changed the title ~~Adds composed model support for allOf oneOf anyOf~~ [python-experimental] Adds allOf/oneOf/anyOf Nov 10, 2019

spacether force-pushed the allof_anyof_oneof_addition branch from 5bd3c25 to 8047162 Compare November 15, 2019 17:38

spacether changed the title ~~[python-experimental] Adds allOf/oneOf/anyOf~~ [python-experimental] Adds allOf/oneOf/anyOf composed models Nov 18, 2019

xtellurian mentioned this pull request Nov 23, 2019

Can't query time series with the Python SDK amphoradata/python-sdk#2

Closed

sebastien-rosset mentioned this pull request Nov 26, 2019

[Python] conditionally set auth attributes #4605

Closed

5 tasks

spacether changed the title ~~[python-experimental] Adds allOf/oneOf/anyOf composed models~~ [Python] Adds allOf/oneOf/anyOf composed models Dec 3, 2019

spacether force-pushed the allof_anyof_oneof_addition branch from 991d2b3 to b523f4b Compare December 3, 2019 17:24

spacether mentioned this pull request Dec 3, 2019

[Python] Add 'supportLargeModel' tag to handle unusually large OAS data type #4628

Closed

5 tasks

spacether force-pushed the allof_anyof_oneof_addition branch from b523f4b to 5aff153 Compare December 3, 2019 17:53

jimschubert reviewed Dec 4, 2019

View reviewed changes

spacether commented Dec 4, 2019

View reviewed changes

samples/client/petstore/python-experimental/docs/UserApi.md Outdated Show resolved Hide resolved

Adds composed model support for allOf oneOf anyOf

5dfe18e

Moves discriminator into discriminator() class method, adds test_dese…

f5fec62

…rialize_lizard, adds setting discriminator using allOf schema

spacether force-pushed the allof_anyof_oneof_addition branch from 888c2e0 to f5fec62 Compare December 8, 2019 21:54

spacether commented Dec 8, 2019

View reviewed changes

Fixes typo in error message

bb66bd3

jimschubert approved these changes Dec 12, 2019

View reviewed changes

jimschubert merged commit 789f1a0 into OpenAPITools:master Dec 15, 2019

jimschubert added the Client: Python label Dec 15, 2019

jimschubert added this to the 4.2.3 milestone Dec 15, 2019

spacether mentioned this pull request Dec 20, 2019

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

Closed

4 tasks

eak24 mentioned this pull request Dec 29, 2019

[Python] Recursive/Cyclic definitions cause import error in Python-experimental #4885

Closed

spacether deleted the allof_anyof_oneof_addition branch January 2, 2020 00:15

eak24 mentioned this pull request Jan 2, 2020

[Python] Multilevel inheritance discriminator support #4912

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Python] Adds allOf/oneOf/anyOf composed models #4446

[Python] Adds allOf/oneOf/anyOf composed models #4446

spacether commented Nov 10, 2019 •

edited

Loading

eak24 commented Nov 19, 2019

spacether commented Nov 19, 2019 •

edited

Loading

spacether commented Nov 22, 2019

spacether commented Nov 30, 2019

jimschubert left a comment

jimschubert Dec 4, 2019

spacether Dec 4, 2019

eak24 commented Dec 5, 2019

spacether commented Dec 5, 2019

spacether commented Dec 7, 2019 •

edited

Loading

eak24 commented Dec 7, 2019 •

edited

Loading

spacether commented Dec 8, 2019 •

edited

Loading

spacether Dec 8, 2019 •

edited

Loading

jimschubert Dec 12, 2019

spacether Dec 8, 2019

spacether commented Dec 8, 2019

spacether commented Dec 9, 2019

jimschubert left a comment •

edited

Loading

eak24 commented Dec 29, 2019

spacether commented Dec 29, 2019

[Python] Adds allOf/oneOf/anyOf composed models #4446

[Python] Adds allOf/oneOf/anyOf composed models #4446

Conversation

spacether commented Nov 10, 2019 • edited Loading

Test Verification

Circular Reference Problem and Solution

Related Issues

Potential Future Work

eak24 commented Nov 19, 2019

spacether commented Nov 19, 2019 • edited Loading

spacether commented Nov 22, 2019

spacether commented Nov 30, 2019

jimschubert left a comment

Choose a reason for hiding this comment

jimschubert Dec 4, 2019

Choose a reason for hiding this comment

spacether Dec 4, 2019

Choose a reason for hiding this comment

eak24 commented Dec 5, 2019

spacether commented Dec 5, 2019

spacether commented Dec 7, 2019 • edited Loading

eak24 commented Dec 7, 2019 • edited Loading

spacether commented Dec 8, 2019 • edited Loading

spacether Dec 8, 2019 • edited Loading

Choose a reason for hiding this comment

jimschubert Dec 12, 2019

Choose a reason for hiding this comment

spacether Dec 8, 2019

Choose a reason for hiding this comment

spacether commented Dec 8, 2019

spacether commented Dec 9, 2019

jimschubert left a comment • edited Loading

Choose a reason for hiding this comment

eak24 commented Dec 29, 2019

spacether commented Dec 29, 2019

spacether commented Nov 10, 2019 •

edited

Loading

spacether commented Nov 19, 2019 •

edited

Loading

spacether commented Dec 7, 2019 •

edited

Loading

eak24 commented Dec 7, 2019 •

edited

Loading

spacether commented Dec 8, 2019 •

edited

Loading

spacether Dec 8, 2019 •

edited

Loading

jimschubert left a comment •

edited

Loading