too strict criteria for merging properties with allOf #1090
Comments
This was referenced Aug 14, 2024
github-merge-queue bot
pushed a commit
that referenced
this issue
Sep 7, 2024
Fixes #1090 (with some limitations - see below) This PR allows the generator to work with some valid schemas where the same property name is specified by more than one schema within an `allOf`, where previously it would have failed. # Background As described in the issue, OpenAPI and JSON Schema define `allOf` in a fairly vague and tolerant way. If a schema is defined like this: ```yaml C: allOf: - $ref: "/path/to/schema/A" - $ref: "/path/to/schema/B" ``` —then the validation for C is "must pass validation for A, and for B". A and B are both objects, and they both have property X, then the value for X must pass validation for A.X and B.X; the validation is the strictest subset of both. This does not map very cleanly to the client generator's internal model. Because its ultimate goal is to producing Python model classes with properties of specific types, we need to be able to "merge" A.X and B.X to produce a single instance of some ___Property class, whose attributes are derived from both. The logic for doing this was narrower than necessary. If, for instance, A.X and B.X only differed in terms of their `description`, or if one of them specified a `default` value and the other did not, the generator would reject them as being too different. # Solution This PR defines a more nuanced property merge logic, as follows: - If both versions of the property are of the same type, the result is a property of the same type. Its validation rules, if any, are the strictest subset of the validation rules for both (for instance, "string with max length 3" and "string with max length 5" produces "string with max length 3"). - Int and number (aka float) properties can always be merged; the result is an int property (since "must be integer" is a stricter rule than "must be a number"). This is only invalid if there is a non-integer default value. - "Any" can be merged with any property type; the result is that property type (since "must be a ___" is stricter than "can be anything"). - For common string-valued attributes such as `description`, earlier values are overridden by later values, on the assumption that the second schema is meant to be an extension or specialization of the first. It also preserves helpful aspects of the original implementation: - Two enums can be merged, if the values for one are a subset of the other; the result uses the subset. - A string enum can be merged with a string, or an int enum with an int; the result is an enum. # Limitations Due to limitations of our internal model, some combinations of properties that are valid in OpenAPI will still be rejected by this implementation: - Two string properties with different `pattern` attributes cannot be merged, even if there are values that could match both of those regexes. - Two list types that contain different schemas cannot be merged, even though logically this should mean "every element in the list must match both of those schemas". - OpenAPI allows `minimum` and `maximum` to be specified for numeric types, but (as far as I can tell) the generator currently ignores these altogether, so the merge logic doesn't take them into account either. # Compatibility This PR preserves the previous schema parsing behavior in all cases where the generator would have previously succeeded. The new behaviors all apply to valid schemas where the generator would have previously failed. --------- Co-authored-by: Dylan Anthony <[email protected]> Co-authored-by: Dylan Anthony <[email protected]>
micha91
pushed a commit
to micha91/openapi-python-client
that referenced
this issue
May 13, 2025
…1096) Fixes openapi-generators#1090 (with some limitations - see below) This PR allows the generator to work with some valid schemas where the same property name is specified by more than one schema within an `allOf`, where previously it would have failed. # Background As described in the issue, OpenAPI and JSON Schema define `allOf` in a fairly vague and tolerant way. If a schema is defined like this: ```yaml C: allOf: - $ref: "/path/to/schema/A" - $ref: "/path/to/schema/B" ``` —then the validation for C is "must pass validation for A, and for B". A and B are both objects, and they both have property X, then the value for X must pass validation for A.X and B.X; the validation is the strictest subset of both. This does not map very cleanly to the client generator's internal model. Because its ultimate goal is to producing Python model classes with properties of specific types, we need to be able to "merge" A.X and B.X to produce a single instance of some ___Property class, whose attributes are derived from both. The logic for doing this was narrower than necessary. If, for instance, A.X and B.X only differed in terms of their `description`, or if one of them specified a `default` value and the other did not, the generator would reject them as being too different. # Solution This PR defines a more nuanced property merge logic, as follows: - If both versions of the property are of the same type, the result is a property of the same type. Its validation rules, if any, are the strictest subset of the validation rules for both (for instance, "string with max length 3" and "string with max length 5" produces "string with max length 3"). - Int and number (aka float) properties can always be merged; the result is an int property (since "must be integer" is a stricter rule than "must be a number"). This is only invalid if there is a non-integer default value. - "Any" can be merged with any property type; the result is that property type (since "must be a ___" is stricter than "can be anything"). - For common string-valued attributes such as `description`, earlier values are overridden by later values, on the assumption that the second schema is meant to be an extension or specialization of the first. It also preserves helpful aspects of the original implementation: - Two enums can be merged, if the values for one are a subset of the other; the result uses the subset. - A string enum can be merged with a string, or an int enum with an int; the result is an enum. # Limitations Due to limitations of our internal model, some combinations of properties that are valid in OpenAPI will still be rejected by this implementation: - Two string properties with different `pattern` attributes cannot be merged, even if there are values that could match both of those regexes. - Two list types that contain different schemas cannot be merged, even though logically this should mean "every element in the list must match both of those schemas". - OpenAPI allows `minimum` and `maximum` to be specified for numeric types, but (as far as I can tell) the generator currently ignores these altogether, so the merge logic doesn't take them into account either. # Compatibility This PR preserves the previous schema parsing behavior in all cases where the generator would have previously succeeded. The new behaviors all apply to valid schemas where the generator would have previously failed. --------- Co-authored-by: Dylan Anthony <[email protected]> Co-authored-by: Dylan Anthony <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
(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:
type, enum values, or required status.Expected behavior:
Observed behavior:
OpenAPI Spec File
https://gist.github.com/eli-bl/3a48142d312578f526992a175c3d1ce4
In this example, the
Catschema is meant to be identical toAnimalexcept that theidproperty 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):
Additional context
As expected, the generator does allow Schema B to further restrict the
enumvalues for the property. However, it does not allow any other changes undertypefor 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, supposeAnimalhas a propertycolorsof typeAnimalPartColors, which in turn has a single propertyeyes(example:"colors": {"eyes": "blue"}). And suppose you define a typeCatPartColorsthat derives fromAnimalPartColorsbut also addsfur("colors": {"eyes": "blue", "fur": "gray"}). The generator correctly allows you to do that— except that will fail if you then try to redefinecolorsinCatto have typeCatPartColors. I believe that this behavior is contrary to the intention of the OpenAPI spec. UsingallOfto 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 forcolorsin aCatare the ones defined inCatPartColors.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
descriptionin 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.The text was updated successfully, but these errors were encountered: