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

[rienafairefr]

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, ./bin/openapi3/{LANG}-petstore.sh, ./bin/security/{LANG}-petstore.sh and ./bin/openapi3/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.
  @wing328 @ci

Description of the PR

I saw that allOf/oneOf/anyOf was not yet fully handled in general, and in particular for my use case, in the Python client codegen. This PR is a tentative to handle deserialization of models that use oneOf/anyOf/allOf, by making use of the allOf/oneOf/anyOf fields in CodegenModel

I've added a composed_hierarchy to the model, which is specified when the underlying openapi model uses oneOf/allOf/anyOf, if there is no discriminator field. When receiving some data, to deserialize it to the correct class, we try to match the incoming data to all possible subclasse

• oneOf: we raise an exception if 0 or more than one of them match
• allOf: we raise an exception if not all of them match
• anyOf: we raise an exception if 0 of them match

with oneOf/anyOf, the instance that is returned will be of the correct subclass, and with allOf, we return an instance of the current model class.

The case of a more advanced schema (when more than one of allOf/oneOf/anyOf is used) is not handled.
Caveat: cases of inline models might not be handled properly in some or all of the cases.

[tomghyselinck]

Hi @rienafairefr,

Thank you very much for this PR!

I have been doing some tests on your code.

It seems to work on basic situations, but I still have some troubles with special/extended cases.

The final class implementation still seems to miss some attributes of the base class when using multi-level inheritance.
In the example below MyImpl will miss the properties implType and commonData from BaseClass.

Also when referencing SubClass in the OpenAPI yaml file as operation response and request body it won't be able to properly discriminate. At least one reason is that the required property of the base class (implType from BaseClass in our example) is missing. In particular for the request body, it will be the server implementation which will fail because the implType property is not given to the server.

Python server comparison

Please note that the Python server (python-flask) implementation defines similar inheritance on allOf/oneOf/anyOf which seems to solve many of these issues.

This implementation also avoids issues with oneOf of which the items has required fields (it assigns self._some_attribute instead of self.some_attribute (the latter fails because it's a property which does some checks on the value which is being set).

It there a possibility to sync the client implementation with the server implementation regarding model definitions?
@wing328: I read that the Python server and client implementations will converge some time? Is that correct? I personally believe that would be a very good approach.

Another final note

The "child" (or "final") implementations in the Python models (either client or server) could give a proper default value to the discriminator property. (when one of their parent(s) has a discriminator)
For example:

• SubClass.__init__(implType='SubClass', ...) instead of SubClass.__init__(implType=None, ...)
• MyImpl.__init__(implType='MyImpl', ...) instead of MyImpl.__init__(implType=None, ...)

This would be very convenient for users of the API so they won't need to provide that field (which is mostly the same as the class name). Especially when they need to provide a new item to a Python function (i.e. REST operation)

Example (stripped very much):

BaseClass:
  discriminator:
    propertyName: implType
  properties:
    implType:
      type: string
    commonData:
      type: string
SubClass:
  discriminator:
    propertyName: implType
  allOf:
  - $ref: BaseClass
  - properties:
      moreCommonData:
        type: string
MyImpl:
  allOf:
  - $ref: SubClass
  - properties:
      implData:
        type: string

With best regards,
Tom.

[rienafairefr]

Thanks for the review @tomghyselinck
You mentioned the python-flask implementation but I couldn't find where the allOf/oneOf/anyOf are handled there. I'd gladly use the python-flask models implementation and sync up, for sure 👍
I sent my inheritance-using spec to python-flask and couldn't find a place where a deserialization from json would give us a "child" concrete type instead of a parent type, maybe I just didn't look at the right places ^^

[tomghyselinck]

Hi @rienafairefr,

I was a little bit confused about the python-flask code regarding allOf/oneOf/anyOf support.
I was wrong about that, I still need to add model conversion myself in the "controller" implementations.

def child_post(base_class):  # noqa: E501
    """Create a new child

    :param base_class: Child description
    :type base_class: dict | bytes

    :rtype: BaseClass
    """
    # XXX - Generated by openapi-generator:
    if connexion.request.is_json:
        base_class = BaseClass.from_dict(connexion.request.get_json())  # noqa: E501
        # XXX - Added by us:
        if type(base_class) is BaseClass:
            # XXX - We need the final implementation => resolve ourselves...
            if base_class.impl_type == 'SomeChild':
                base_class = SomeChild.from_dict(connexion.request.get_json())  # noqa: E501
            if base_class.impl_type == 'OtherChild':
                base_class = OtherChild.from_dict(connexion.request.get_json())  # noqa: E501

    # Continue with the implementation...

We will probably need another PR for the server controllers properly supporting allOf/oneOf/anyOf.

I made a review on the client implementation.

With best regards,
Tom.

[tomghyselinck]

Hi @rienafairefr,

Are you aware of #2049 which should also fix the support for allOf?
Does this also fix your needs for oneOf / anyOf?

With best regards,
Tom.

[rienafairefr]

Hi @tomghyselinck, I looked at #2049 but I don't think the cases of anyOf/oneOf are handled inthere :-|

[spacether]

@rienafairefr can you describe what your code does for the case of the Dog class?
It looks like it attempts to instantiate all child classes and returns an exception if the instantiation fails, is that right?
How do we ensure that the instance of Dog will have the variables defined in the Dog class and the child Animal class? We could accomplish this by using allVars.
@tomghyselinck do you have an alternative suggestion to using allVars?
One solution would be to make a class Dog which inherited from allOf classes.
Not sure how one would handle intersecting classes, like two classes that both include the property red. The inheritance solution may not work for more complex composed cases.

How about the below proposal (store the children)?

For the Dog class instance, store the below instances of child classes:

• Animal
  note: breed validation is stored in the Dog class.

The Dog needs properties added from those child classes.
And if a property's value is changed, the new value should pass the required validation in the child class.
Any serialization of Dog would create the dict using the properties in the current class
and merge in the dicts of Animal and any other child class instances.
This method also lets use preserve child class validation.
If a Dog property in Animal is changed, change the value in the corresponding child instances so we preserve their validation.
This would require storing a dog_property to dog_child_class_instance mapping.

[spacether]

One more use case that I'm not sure it this PR covers:
@rienafairefr if we input prop_c='some value' into a model and prop_c does not exist in the composed class nor its child classes are we raising an exception? Where?

ChildA
  prop_a

ChildB
  prop_b

Parent
  all_of:
    - ChildA
    - ChildB

parent = Parent(prop_a='a', prop_b='b', prop_c='c')

[spacether]

@rienafairefr the feature looks like it's almost there. Can you add tests to this PR that demonstrate instantiation and property access of Dog or other composed schemas?

[rienafairefr]

@spacether Json schemas encompass more stuff than just inheritance like in the oop world, so even though it's tempting, we can't really do only that.

for allOf, yeah, we could have instances of the 'child' classes, and delegate to those. For anyOf or oneOf it would be possible too, but a bit more useless because anyOf or oneOf are instance of only one of the sub-classes.
In the current situation, properties from the composed child schemas are dumped in the same place, maybe there is something to do here. We'd have to kinda keep track of where each model property comes from.

There is already a deserialization of Dog instances (e.g. test_deserialize_dict_str_dog ), but it's using the disciminator thing. The standard petstore spec doesn't have anyOf or oneOf. I'll add that to the 3_0/petstore-with-fake-endpoints-models-for-testing.yaml spec and add a unit test in the openapi3/petstore python client

[spacether]

@rienafairefr thank you for explaining how oop inheritance won't work here.

Doesn't anyOf allow for one or more child schema matches? That's what this source describes: https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/
If dog only returns an animal animal instance without also setting breed on the dog instance, isn't it incorrectly building the dog instance?

Adding those tests in the openapi python client sample sounds great!

[tomghyselinck]

@tomghyselinck do you have an alternative suggestion to using allVars?
One solution would be to make a class Dog which inherited from allOf classes.
Not sure how one would handle intersecting classes, like two classes that both include the property red. The inheritance solution may not work for more complex composed cases.

Hi @spacether,

Sorry for the late reply.

What do you mean with alternative to using allVars? AFAIK this PR uses vars and not allVars?

[spacether]

@tomghyselinck do you have an alternative suggestion to using allVars?
One solution would be to make a class Dog which inherited from allOf classes.
Not sure how one would handle intersecting classes, like two classes that both include the property red. The inheritance solution may not work for more complex composed cases.

Hi @spacether,

Sorry for the late reply.

What do you mean with alternative to using allVars? AFAIK this PR uses vars and not allVars?

Hi @rienafairefr and @tomghyselinck I want to make sure that this implementation covers the case that I described above for accessing all properties on a model instance that has composed schema.
If we switch to allVars when using allOf I think all of those properties will be accessible, but using allVars is not the only way to do this. Let's look at a current example, how it is currently working and what the desired behavior is in my opinion.

Looking at Dog and Cat:
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml#L1266
Their schemas are:

  Dog:
    allOf:
      - $ref: '#/definitions/Animal'
      - type: object
        properties:
          breed:
            type: string
  Cat:
    allOf:
      - $ref: '#/definitions/Animal'
      - type: object
        properties:
          declawed:
            type: boolean
  Animal:
    type: object
    discriminator: className
    required:
      - className
    properties:
      className:
        type: string
      color:
        type: string
        default: 'red'

A Dog instance should have the properties:

• breed (optional)
• className (required)
• color (optional)

A Cat instance should have the properties:

• declawed (optional)
• className (required)
• color (optional)

Only className is required per allOf:
https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/
The allOf composed schema is part of Swagger 2.0 spec.

Looking at our tests, I don't see any tests for instance properties on Dog and Cat instances.
Can we add them?

• Cat has no tests on it
  https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/python/test/test_cat.py
• We do test Dog, but the test is not checking to see that breed, className, and color are present
  https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/python/tests/test_deserialization.py

What do you think?
Does what I've described make sense?
Do you agree with how I've interpreted the Dog and Cat schemas?

[spacether]

oneOf/anyOf/allOf composed models have been added in the python-experimental generator in this diff: #4446
That generator inherrits much of its code from the python generator and should be a drop in replacement for devs using the python generator.

[rienafairefr]

Thanks @spacether I'll test out he python-experimental generator, this PR can probably be closed

[spacether]

@rienafairefr thank you for this PR.
I am working on cleaning up old python PRs.
What would you like to do with this one; work on integrating it or closing it?

[rienafairefr]

I think the python-experimental generator handled that issue, so I recommend closing this PR