Skip to content

too strict criteria for merging properties with allOf #1090

Closed
@eli-bl

Description

@eli-bl

(I think that this overlaps with #1083 - feel free to close one or the other as a duplicate - I think there are multiple problems with allOf, and this is a specific one)

Describe the bug
Conditions:

  • Schema B uses allOf to derive properties from Schema A.
  • Schema B also overrides one of those properties with its own definition.
  • The property is different in at least one way not including its type, enum values, or required status.

Expected behavior:

  • The client generator merges the two definitions, and implements validation for the property based on the criteria from both schemas.

Observed behavior:

  • The client generator fails to parse Schema B. The underlying error is "Properties has conflicting values".

OpenAPI Spec File
https://gist.github.com/eli-bl/3a48142d312578f526992a175c3d1ce4

In this example, the Cat schema is meant to be identical to Animal except that the id property must match a specific regex. This is a valid spec according to https://editor.swagger.io, and the OpenAPI 3 spec does not suggest any reason for it to be invalid.

Desktop (please complete the following information):

  • OS: macOS 14.5
  • Python Version: 3.8.15
  • openapi-python-client version 0.21.2

Additional context
As expected, the generator does allow Schema B to further restrict the enum values for the property. However, it does not allow any other changes under type for a property. For properties that are objects, this is especially problematic, since it means that you can't redeclare a property to use an object type that extends its original type with more properties. For instance, suppose Animal has a property colors of type AnimalPartColors, which in turn has a single property eyes (example: "colors": {"eyes": "blue"}). And suppose you define a type CatPartColors that derives from AnimalPartColors but also adds fur ("colors": {"eyes": "blue", "fur": "gray"}). The generator correctly allows you to do that— except that will fail if you then try to redefine colors in Cat to have type CatPartColors. I believe that this behavior is contrary to the intention of the OpenAPI spec. Using allOf to extend an object type with extra properties is a standard pattern, and the expected behavior in this case would be to say that the validation rules for colors in a Cat are the ones defined in CatPartColors.

Any other change to the property also triggers the error even if it would not change the validation behavior of the property. For instance, changing the description (such as, in this example, calling it "unique identifier of the cat" instead of "unique identifier of the animal") is not allowed.

The relevant logic is here: https://github.com/openapi-generators/openapi-python-client/blob/v0.21.2/openapi_python_client/parser/properties/model_property.py#L250

The OpenAPI spec does not clarify what should happen to non-validation-related attributes such as description in this scenario, where it isn't really possible to define the intersection between one definition and the other. An arguably intuitive behavior would be to use the description that is defined latest (in this example, "unique identifier of the cat")-- which is what the behavior of https://editor.swagger.io appears to be.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions