[BUG][python] allOf with subobjects generates "Invalid inputs"

[mfmarche]

Bug Report Checklist

• Have you provided a full/minimal spec to reproduce the issue?
• Have you validated the input using an OpenAPI validator (example)?
• Have you tested with the latest master to confirm the issue still exists?
• Have you searched for related issues/PRs?
• What's the actual output vs expected output?
• [Optional] Sponsorship to speed up the bug fix or feature request (example)

Description

The current testcases for allOf in petstore seem to work on the assumption that passing model arguments in init() can be passed to all objects in the allOf.

In particular, the method "validate_get_composed_info" description says:

Exceptions are raised if:
- 0 or > 1 oneOf schema matches the model_args input data
- no anyOf schema matches the model_args input data
- any of the allOf schemas do not match the model_args input data

This seems to suggest that I cannot instantiate a composedObject (via its constructor), with arguments that are associated with a particular object in the oneOf list.

I made a very simple change to the petstore api model spec to highlight this:

+    SpecialAttributes:
+      type: string
+      minLength: 1
+      maxLength: 100
     ShapeInterface:
       properties:
         shapeType:
           type: string
+        name:
+          $ref: '#/components/schemas/SpecialAttributes'
       required:
         - shapeType

Adding a name to ShapeInterface, with min/max arguments now makes it ModelNormal, (rather than a simple string).

instantiating this:

equilateral_triangle.EquilateralTriangle(triangle_type="EquilateralTriangle", shape_type="Triangle", name=SpecialAttributes("aname"))

gives the following error:

petstore_api.exceptions.ApiValueError: Invalid inputs given to generate an instance of 'TriangleInterface'. The input data was invalid for the allOf schema 'TriangleInterface' in the composed schema 'EquilateralTriangle'. Error=Invalid type for variable 'name'. Required value type is one of [NoneType, bool, date, datetime, dict, float, int, list, str] and passed type was SpecialAttributes at ['name']

I believe this error is due to the "SpecialAttributes" property, which is valid for ShapeInterface, but not valid for the TriangleInterface. So the loop of passing the constructor arguments to each allOf class hits the TriangleInterfac, and then errors out. If SpecialAttribute was a normal string, it would work, since that is a primitive value type.

openapi-generator version

master

Suggest a fix

Its not clear what the correct solution is. It appears to me that the premise of allOf compostion currently works by way of "additionalTypes" being passed into each allOf object, and as long as the types are of primitive types, it is able to instantiate this composed objects. The oneOf, anyOf works since it is able to catch the exception and continue. With the allOf implementation, since all constructor arguments are passed into the list of allOf objects, there is a potential for an argument to not be succesful since it doesn't understand that type.

I suspect with the current allOf implementation, there is a side effect that all arguments/properties make there way to all allOf objects, as additionalTypes.

Is it possible that this type of constructor instantiation is not fesible, and possible only via deserialization?
For example, I also tried the following:

        tri = equilateral_triangle.EquilateralTriangle(triangle_type="EquilateralTriangle", shape_type="Triangle")
        tri["name"] = SpecialAttributes("something")

but this also failed with:

petstore_api.exceptions.ApiTypeError: Invalid type for variable 'name'. Required value type is one of [NoneType, bool, date, datetime, dict, float, int, list, str] and passed type was SpecialAttributes at ['name']

This is possibly a different issue, but I'm now unclear as to any workaround.

Since the composed object knows about each argument, shouldn't the composed object prune the argument list based on which allOf object it is passing the information to? It would introspect each allOf object and determine the arguments that intersect.

[spacether]

EquilateralTriangle is defined as:

    EquilateralTriangle:
      allOf:
        - $ref: '#/components/schemas/ShapeInterface'
        - $ref: '#/components/schemas/TriangleInterface'

So any payload that you pass in to EquilateralTriangle must also work when validated separately and passed into:

• ShapeInterface
  AND
• TriangleInterface

TriangleInterface is defined as:

    TriangleInterface:
      properties:
        triangleType:
          type: string
      required:
        - triangleType

Which should allow name in as an additionalProperty.
SpecialAttributes is a primitive model and when its type is checked under the hood, our code should see it as str which is allowed in the additionalProperties definition. So maybe our type checking code isn't doing a good enough job.
We have the fn get_simple_class to go from SpecialAttributes -> str.

[mfmarche]

if that fix occured (get_simple_class), that would handle this case. But what if SpecialAttributes with a Normal or Compound class, what happens next? How is that passed through to all the allOf classes?

Also, what is the desired behavior if the instances allOf has "additionalProperties" set to False, and it receives a type it does not know about (such as this case). I believe it would throw an exception. Is it expected that you must have additional property types set to True for all allOf instances? I don't quite understand the behavior of duplicating properties across all allOf structures, even though a particular structure has properly defined the property, why would that have to trickle and pollute other instances in the allOf list.

[spacether]

With additionalProperties set to false the schema/model does not accept any properties that do not exist in the schema.

For a Normal class (object based) get_simple class should return dict to work with additionalProperties.
For a Composed class, it could be any type, int/str/float/None/bool/list/dict. It depends upon the composition definition and the input payload.

If must be included in the other allOfs because all schemas are validated separately.
Here's an exampled:

A:
  type: object
B:
  type: object
  additionalProperties:
    oneof:
      - type:int
      - type: str
C:
  type: object
  additionalProperties:
    oneof:
      - type: str
D:
  type: object
  additionalProperties:
    oneof:
      - type: str, format: datetime
E:
  allOf:
  - A
  - B
  - C
  - D

A: an object with any values is allowed in
B: an object with integer or string values are allowed in
C: an object with string values are allowed in
D: an object with string values that are iso datetimes are allowed in
E: any type is allowed in as a value (and all of the allOf schemas require type object, so only type object will pass validation of A-D)
It's not pollution its additional constraints that other schemas impose.
Ideally we would store all the payload values in one place rather than multiple instances. That's what I am working on doing in python-experimental one still has to verify that all validations pass in each schema one by one though.

[mfmarche]

In your example, I presume then that the datetime object would only be permitted into object D, and I would also presume, that datetime would still be passed in as arguments to A, B and C, however, it would not be accepted, and I assume then, it would not create an exception correct? In this case, that value would only be stored in D.

With regard to the pollution however, I don't see how the data is not duplicated across multple objects. For example for types that are the same across the structures, and if A B and C had str named properties named a_value, b_value, and c_value, and E was constructed with:

E(a_value="a", b_value="b", c_value="c"), I presume that the data would end up everywhere, such as:

A["a_value"] = "a"
A["b_value"] = "b"
A["c_value"] = "c"

(also across B, C and D objects)

[spacether]

Datetimes are stored as string types in openapi schema, so they would be allowed in to A, B, and C.
So for pullution you mean data storage duplication. Yes that is a drawback of the current python generator. It is duplicated and stored in multiple places.
dict(a_value='a', b_value='b', c_value='c') would be stored in the composed schema and in A,B,C,D instances under the hood yes. That's one reason that I'm working on python-experimental; to eliminate that storage duplication.

[mfmarche]

Ok thanks for that explanation. I understand its duplicated now (wrongly). The solution I presented I believe solves this, albeit likely not the depth as you are undertaking in the experiental changes.

However, since it knows the name of the property, it can very easily see if the object accepts that, since it doesn't need to send that into the instances. There is also this check to handle addtional types as well in find_args_intersection:

elif cls.additional_properties_type and type(value) in cls.additional_properties_type:
    #allow additional properties, if declared, to pass through            
    intersected_args[key] = value

That specifically allows additional properties to pass through, since it might be required to store them. But certainly not for types/classes it has no idea about.

With regard to your comment about get_simple_class, its not exactly clear what the intended behavior should be then for classes/objects that are passed in.

At moment its doing this:

    if isinstance(input_value, type):
        # input_value is a class
        return input_value

Does it need to return something like input_value.to_dict()? (if type is OpenApiModel).

[spacether]

So get_simple_class is used to turn a value into a class for comparison. The logic may need to be updated to return the ModelSimple class and the base type that the model is representing. So maybe get_simple_class(StringModel('yo')) should result in (str, ModelSImple). Whichc could be checked to see that it matches up with a str allowed type or a StringModel type depending on if the schema explicitly defined it as StringModel or it is being allowed in with additionalProperties with any type.

This is complicated by the fact that we represent schemas types as model classes or python primitive types. The logic may need to be sharpened to better cover all use cases.

One should not need to return input_value.to_dict() in get_simple_class be cause the results are used to check if the input value is the correct type or needs coercion.

[spacether]

This use case will work in the python-experimental generator. Please use it
Because all inputs passed into __new__

• are converted back into python primitive types
• and any needed validation is run using those primitive types