[Python][test] oneOf with overlapping types

[jirikuncar]

tl;dr Shows a serialization problem of oneOf models in python-experimental with overlapping types.

Minimal specification

Two models with overlapping field definition of named enum type.

openapi: 3.0.1
info:
  title: fruity
  version: 0.0.1
paths:
  /:
    get:
      responses:
        "200":
          description: desc
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/fruit"
components:
  schemas:
    fruit:
      title: fruit
      oneOf:
        - $ref: "#/components/schemas/apple"
        - $ref: "#/components/schemas/banana"
    appleColor:
      type: string
      enum:
        - yellow
        - green
        - red
    apple:
      title: apple
      type: object
      properties:
        kind:
          type: string
        color:
          $ref: '#/components/schemas/appleColor'
    bananaColor:
      type: string
      enum:
        - yellow
        - green
    banana:
      title: banana
      type: object
      properties:
        count:
          type: number
        color:
          $ref: '#/components/schemas/bananaColor'

Problem

The generated model contains all fields and the overlapping field has wrong type.

    @cached_property
    def openapi_types():
        """..."""
        return {
            'kind': (str,),  # noqa: E501
            'color': (banana_color.BananaColor,),  # noqa: E501
            'count': (float,),  # noqa: E501
        }

     # ...

     attribute_map = {
        'kind': 'kind',  # noqa: E501
        'color': 'color',  # noqa: E501
        'count': 'count',  # noqa: E501
    }

This prevents deserialization of {"kind": "sweet", "color": "red"} response.

How do I get expected behavior?

Simply remove definition of openapi_types and attribute_map and let the oneOf models do the job.

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
• Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. 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/config/java*. For Windows users, please run the script in Git BASH.
• File the PR against the correct branch: master
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @sebastien-rosset

[sebastien-rosset]

Thank you for the explanation. This is similar to a problem that was also identified in Java jersey2. AFAIK, in case of composed schemas, the following use cases are not currently handled:

1. Same property name appears in two different schemas with different types.
2. Same property name, different constraints, e.g. maxItems, minItems, maximum, minimum, pattern...

The problem exists for anyOf, oneOf and allOf, not just oneOf. A schema can have a combination of anyOf, oneOf and allOf, and conceivable the same property could appear multiple times across all three.

When two properties with the same name are specified, afaik the "last one wins", i.e. the last referenced schema in the OAS document wins.

the "last" property wins

[jirikuncar]

The correct solution would include all properties defined directly on the model and not in any oneOf one.

[spacether]

A composed schema can have properties at the base level and and can have oneOf defined. To cover that case we do need to define attribute_map here. The composed schema is more strict and can require:

  ComposedSchema:
    properties:
      someVar:
        type: string
    oneOf:
      - Apple

And Apple will accept any type for someVar because it is accepted as an additionalProperty, but in ComposedSchema we need to check the type of someVar to make sure that it is type str in python.
We can also have a schema like:

  ComposedSchema:
    allOf:
       - Plant
    oneOf:
      - Apple

In which case the composed schema should also store the parameter names in the
attribute_map for Plan + Apple.

General question about when this problem arises.

Can't this happen for all of these cases for required + optional variables:

1. oneOf schemas have a variable with conflicting type
2. anyOf schemas have a variable of conflicting type
3. Does our generator allow type collisions between variables in (allOf + oneOf) or (allOf + anyOf)? It should not, that violates the openapi spec.

You have also revealed that the docstring for this function is incorrect for these 1-3 use cases.
Python-experimental has previously addressed issues with composed schema inputs and tweaking them in the past with the method addNullDefaultToOneOfAnyOfReqProps. It goes through and sets composed schema required inputs to have a default null value in the composed schema init signature. This makes those inputs named arguments and makes them optional for users to pass them in. This is because:
we can have 2 oneOf schemas, schemaA, and schemaB each with 1 required variable, varA, varB. If we are only making an instance of schemaA, then varB is not required. So we set them all to null, and when we make an instance of schemaA, it requires that we pass in varA.

How about we do some similar tweaking at the composed schema variable level here?
We could

• look for variable type collisions in the oneOf/anyOf variables of a composed schema
• update their data types at the composed schema level to be a tuple of the allowed types
• this lets us keep using the mustache template as-is
• this keeps our documentation correct and lets our users know that multiple types can be passed in for these variables

What do you think?

[jirikuncar]

1. oneOf schemas have a variable with conflicting type

This can happen and we should support it.

2. anyOf schemas have a variable of conflicting type

It can happen. I am not sure how it should be resolved when a value matches multiple conflicting types.

3. Does our generator allow type collisions between variables in (allOf + oneOf) or (allOf + anyOf)? It should not, that violates the openapi spec.

You are right that It should not.

I am not sure about the solution though. I would completely remove validation on base schema for properties defined in XOf models.

---

I agree that we should not merge this mustache template patch. I was looking for a quick solution that would unblock me so I can at least generate code that can deserialize server response without raising an exception.

[sebastien-rosset]

Does our generator allow type collisions between variables in (allOf + oneOf) or (allOf + anyOf)? It should not, that violates the openapi spec.

One can intentionally or accidentally write a OAS document which is 1) syntactically compliant with the JSON schema specification and 2) will not match any input payload. It's an interesting question whether it's even possible to analyze a schema and conclude whether there is at least one matching input data.
IMO this should be handled with the --validation argument, and the code generator should still accept the OAS document. I.e. you want to know if a OAS doc is syntactically valid.

Consider pattern. There is an infinite number of regex that will not match any input string, including the empty string. Here is one simple example:

type: string
pattern: .^

[jirikuncar]

I have a quick update for templates that works for me ™️. I could not find out how to get directly defined properties on the model.

[spacether]

Thank you for the explanation. This is similar to a problem that was also identified in Java jersey2. AFAIK, in case of composed schemas, the following use cases are not currently handled:

1. Same property name appears in two different schemas with different types.

2. Same property name, different constraints, e.g. maxItems, minItems, maximum, minimum, pattern...

The problem exists for anyOf, oneOf and allOf, not just oneOf. A schema can have a combination of anyOf, oneOf and allOf, and conceivable the same property could appear multiple times across all three.

When two properties with the same name are specified, afaik the "last one wins", i.e. the last referenced schema in the OAS document wins.

the "last" property wins

My understanding is that this is the cause. In java the codegenmodel pools all variable inputs from all oneOf anyOf allOf models as inputs in the composed model. So we can have collisions. Maybe we should store plural types for CodegenParameter and CodegenProperty. What do you think @wing328 @jimschubert

[sebastien-rosset]

As a side note, the schema may be brittle if two oneOf child schemas define a appleColor and bananaColor that share common values such as "yellow", unless there are other required distinguishing constraints. The issue raised here is still valid.

[spacether]

2. Same property name, different constraints, e.g. maxItems, minItems, maximum, minimum, pattern...

For the record python-experimental handles this use case.
We would store the property in two model instances and differing validation would be checked in each model when the composed model is instantiated.

[sebastien-rosset]

2. Same property name, different constraints, e.g. maxItems, minItems, maximum, minimum, pattern...

For the record python-experimental handles this use case.
We would store the property in two model instances and differing validation would be checked in each model when the composed model is instantiated.

Below is oneOf schema where the breed property can be either:

1. A string of 1 to 10 [a-z] characters, or
2. A string of 11 to 20 [a-z] characters.

I know, there is a much simpler way to write the schema, this is just an example to test the edge cases.

    TestOneOf:
      oneOf:
        - type: object
          properties:
            breed:
              type: string
              pattern: '[a-z]{1,10}'
        - type: object
          properties:
            breed:
              type: string
              pattern: '[a-z]{11,20}'

Here is the generated python code.

class TestOneOf(ModelComposed):
    validations = {
        ('breed',): {
            'regex': {
                # NOTE The last one wins. Doesn't that mean the generated code will always match the input data against this pattern?
                'pattern': r'[a-z]{11,20}',  # noqa: E501
            },
        },
    }

I'm not saying this breaks my use cases.

[spacether]

Here is the generated python code.

class TestOneOf(ModelComposed):
    validations = {
        ('breed',): {
            'regex': {
                # NOTE The last one wins. Doesn't that mean the generated code will always match the input data against this pattern?
                'pattern': r'[a-z]{11,20}',  # noqa: E501
            },
        },
    }

I'm not saying this breaks my use cases.

Ah, my mistake python-experimental does not have this working yet. It looks like I need to combine or strip out oneOf and anyOf validations at the composed schema level. Then it should work. Thank you for pointing that out.

[jimschubert]

I scanned through this, and I think the approach makes sense. Were there additional requests to be made before merge (@spacether and @sebastien-rosset)?

There are a handful of proposed changes, were you wanting to change those before merge @jirikuncar ?

[spacether]

The mustache changes will break functionality for composed schemas that include properties.

The changes omit properties variables that are defined like this:

components:
  schemas:
    ComposedModelWithProperties:
      properties:
        - color:
        type: string
      oneOf:
        - Cat
        - Dog

Can you add an example like I have shown above and show that color is present in ComposedModelWithProperties.openapi_types?

• Could we fix this in composed schemas by just iterating over vars to make openapi_types?
  • Are the properties only loaded into vars vs requredVars/optionalVars/allVars?
• How about also changing the validations in a composed schema to only iterate over those vars?

[jirikuncar]

@jimschubert all changes has been resolved by upgrading to latest master.

@spacether it looks like you have doing a big refactoring in #7274. I will wait until it's merged before adding more changes to this PR.

[spacether]

@spacether it looks like you have doing a big refactoring in #7274. I will wait until it's merged before adding more changes to this PR.

Thank you for your patience. 7274 was just merged so you can continue work here.

[jimschubert]

@spacether You'd requested changes on this PR. Do you have any remaining concerns? I'd like to get it in before addition conflicts emerge.

[spacether]

Yes, my change requested comment still applies.
Why not iterate over vars to only define types core parameters in a composed schema that are defined using the composed schema key of properties?
The new code that we have now:

• uses special handling when we have oneOf schemas
• if we have oneOf + properties in a composed schema that breaks that use case
• what about i we only have only anyOf schemas?
• what about if we have oneOf + allOf?

Iterating over vars should handle all of these above use case and prevent type conflicts if vars only contains the vars from composed schema properties.
If we iterate over vars, then the data types for conflicting oneOf types will be defined in the oneOf schemas, which should work. The problem that we have today is that a parameter has its type defined in the composed schema AND schemas oneOfA and oneOfB. With my suggestion, this conflicting parameter will be an additionalProperty in the composed schema and will have individual type definition in oneOfA + oneOfB.

To do this, in modules/openapi-generator/src/main/resources/python/python-experimental/model_templates/model_composed.mustache
we need to iterate over vars when defining openapi_type and validations. And perhaps when defining attribute_map. Not sure about attribute_map. That map just goes from python name to spec var name.

[spacether]

FYI I am working on helping on this. I duplicated your branch and I am working on implementing my iterate over vars suggestion in https://github.com/spacether/openapi-generator/tree/composed_schema_overlapping_type_fix_from_jr

[spacether]

I am working on fixing this in two PRS:

1. fix additionalProperties in models [python] Fixes additional_properties_type for models #8802
   right now some models have additionalPropertiesType being defined as None when it should have a real value
2. make the CodegenModel/CodegenProperty/CodegenParameter/CodegenResponse.vars only contain the properties defined in that schema Adds fix for composed schema model vars #8816

[spacether]

Closing due to inactivity