better support for merging properties with allOf (#1096)

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]>

Author: eli-bl

.changeset/improved_property_merging.md

@@ -0,0 +1,20 @@
+---
+default: minor
+---
+
+# Improved property-merging behavior with `allOf`
+
+When using `allOf` to extend a base object type, `openapi-python-client` is now able to handle some kinds of modifications to an existing property that would have previously caused an error:
+
+- Overriding attributes that do not affect validation, such as `description`.
+- Combining properties that this generator ignores, like `maxLength` or `pattern`.
+- Combining a generic numeric type with `int` (resulting in `int`).
+- Adding a `format` to a string.
+- Combining `any` with a specific type (resulting in that specific type).
+- Adding or overriding a `default`
+
+> [!NOTE]
+> `pattern` and `max_length` are no longer fields on `StringProperty`, which may impact custom templates.
+
+This also fixes a bug where properties of inline objects (as opposed to references) were not using the
+merge logic, but were simply overwriting previous definitions of the same property.

end_to_end_tests/baseline_openapi_3.0.json

@@ -2152,6 +2152,56 @@
           "additionalProperties": {}
         }
       },
+      "ModelWithMergedProperties": {
+        "title": "ModelWithMergedProperties",
+        "allOf": [
+          {
+            "type": "object",
+            "properties": {
+              "simpleString": {
+                "type": "string",
+                "description": "base simpleString description"
+              },
+              "stringToEnum": {
+                "type": "string",
+                "default": "a"
+              },
+              "stringToDate": {
+                "type": "string"
+              },
+              "numberToInt": {
+                "type": "number"
+              },
+              "anyToString": {}
+            }
+          },
+          {
+            "type": "object",
+            "properties": {
+              "simpleString": {
+                "type": "string",
+                "description": "extended simpleString description",
+                "default": "new default"
+              },
+              "stringToEnum": {
+                "type": "string",
+                "enum": ["a", "b"]
+              },
+              "stringToDate": {
+                "type": "string",
+                "format": "date"
+              },
+              "numberToInt": {
+                "type": "integer"
+              },
+              "anyToString": {
+                "type": "string",
+                "default": "x"
+              }
+            }
+          }
+        ]
+      },
       "ModelWithPrimitiveAdditionalProperties": {
         "title": "ModelWithPrimitiveAdditionalProperties",
         "type": "object",

end_to_end_tests/baseline_openapi_3.1.yaml

@@ -2146,6 +2146,56 @@ info:
         "additionalProperties": { }
       }
     },
+    "ModelWithMergedProperties": {
+      "title": "ModelWithMergedProperties",
+      "allOf": [
+        {
+          "type": "object",
+          "properties": {
+            "simpleString": {
+              "type": "string",
+              "description": "base simpleString description"
+            },
+            "stringToEnum": {
+              "type": "string",
+              "default": "a"
+            },
+            "stringToDate": {
+              "type": "string"
+            },
+            "numberToInt": {
+              "type": "number"
+            },
+            "anyToString": {}
+          }
+        },
+        {
+          "type": "object",
+          "properties": {
+            "simpleString": {
+              "type": "string",
+              "description": "extended simpleString description",
+              "default": "new default"
+            },
+            "stringToEnum": {
+              "type": "string",
+              "enum": ["a", "b"]
+            },
+            "stringToDate": {
+              "type": "string",
+              "format": "date"
+            },
+            "numberToInt": {
+              "type": "integer"
+            },
+            "anyToString": {
+              "type": "string",
+              "default": "x"
+            }
+          }
+        }
+      ]
+    },
     "ModelWithPrimitiveAdditionalProperties": {
       "title": "ModelWithPrimitiveAdditionalProperties",
       "type": "object",

end_to_end_tests/golden-record/my_test_api_client/models/__init__.py

@@ -60,6 +60,8 @@
 from .model_with_circular_ref_in_additional_properties_b import ModelWithCircularRefInAdditionalPropertiesB
 from .model_with_date_time_property import ModelWithDateTimeProperty
 from .model_with_discriminated_union import ModelWithDiscriminatedUnion
+from .model_with_merged_properties import ModelWithMergedProperties
+from .model_with_merged_properties_string_to_enum import ModelWithMergedPropertiesStringToEnum
 from .model_with_no_properties import ModelWithNoProperties
 from .model_with_primitive_additional_properties import ModelWithPrimitiveAdditionalProperties
 from .model_with_primitive_additional_properties_a_date_holder import ModelWithPrimitiveAdditionalPropertiesADateHolder
@@ -138,6 +140,8 @@
     "ModelWithCircularRefInAdditionalPropertiesB",
     "ModelWithDateTimeProperty",
     "ModelWithDiscriminatedUnion",
+    "ModelWithMergedProperties",
+    "ModelWithMergedPropertiesStringToEnum",
     "ModelWithNoProperties",
     "ModelWithPrimitiveAdditionalProperties",
     "ModelWithPrimitiveAdditionalPropertiesADateHolder",

end_to_end_tests/golden-record/my_test_api_client/models/model_with_merged_properties.py

@@ -0,0 +1,112 @@
+import datetime
+from typing import Any, Dict, List, Type, TypeVar, Union
+
+from attrs import define as _attrs_define
+from attrs import field as _attrs_field
+from dateutil.parser import isoparse
+
+from ..models.model_with_merged_properties_string_to_enum import ModelWithMergedPropertiesStringToEnum
+from ..types import UNSET, Unset
+
+T = TypeVar("T", bound="ModelWithMergedProperties")
+
+
+@_attrs_define
+class ModelWithMergedProperties:
+    """
+    Attributes:
+        simple_string (Union[Unset, str]): extended simpleString description Default: 'new default'.
+        string_to_enum (Union[Unset, ModelWithMergedPropertiesStringToEnum]):  Default:
+            ModelWithMergedPropertiesStringToEnum.A.
+        string_to_date (Union[Unset, datetime.date]):
+        number_to_int (Union[Unset, int]):
+        any_to_string (Union[Unset, str]):  Default: 'x'.
+    """
+
+    simple_string: Union[Unset, str] = "new default"
+    string_to_enum: Union[Unset, ModelWithMergedPropertiesStringToEnum] = ModelWithMergedPropertiesStringToEnum.A
+    string_to_date: Union[Unset, datetime.date] = UNSET
+    number_to_int: Union[Unset, int] = UNSET
+    any_to_string: Union[Unset, str] = "x"
+    additional_properties: Dict[str, Any] = _attrs_field(init=False, factory=dict)
+
+    def to_dict(self) -> Dict[str, Any]:
+        simple_string = self.simple_string
+
+        string_to_enum: Union[Unset, str] = UNSET
+        if not isinstance(self.string_to_enum, Unset):
+            string_to_enum = self.string_to_enum.value
+
+        string_to_date: Union[Unset, str] = UNSET
+        if not isinstance(self.string_to_date, Unset):
+            string_to_date = self.string_to_date.isoformat()
+
+        number_to_int = self.number_to_int
+
+        any_to_string = self.any_to_string
+
+        field_dict: Dict[str, Any] = {}
+        field_dict.update(self.additional_properties)
+        field_dict.update({})
+        if simple_string is not UNSET:
+            field_dict["simpleString"] = simple_string
+        if string_to_enum is not UNSET:
+            field_dict["stringToEnum"] = string_to_enum
+        if string_to_date is not UNSET:
+            field_dict["stringToDate"] = string_to_date
+        if number_to_int is not UNSET:
+            field_dict["numberToInt"] = number_to_int
+        if any_to_string is not UNSET:
+            field_dict["anyToString"] = any_to_string
+
+        return field_dict
+
+    @classmethod
+    def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T:
+        d = src_dict.copy()
+        simple_string = d.pop("simpleString", UNSET)
+
+        _string_to_enum = d.pop("stringToEnum", UNSET)
+        string_to_enum: Union[Unset, ModelWithMergedPropertiesStringToEnum]
+        if isinstance(_string_to_enum, Unset):
+            string_to_enum = UNSET
+        else:
+            string_to_enum = ModelWithMergedPropertiesStringToEnum(_string_to_enum)
+
+        _string_to_date = d.pop("stringToDate", UNSET)
+        string_to_date: Union[Unset, datetime.date]
+        if isinstance(_string_to_date, Unset):
+            string_to_date = UNSET
+        else:
+            string_to_date = isoparse(_string_to_date).date()
+
+        number_to_int = d.pop("numberToInt", UNSET)
+
+        any_to_string = d.pop("anyToString", UNSET)
+
+        model_with_merged_properties = cls(
+            simple_string=simple_string,
+            string_to_enum=string_to_enum,
+            string_to_date=string_to_date,
+            number_to_int=number_to_int,
+            any_to_string=any_to_string,
+        )
+
+        model_with_merged_properties.additional_properties = d
+        return model_with_merged_properties
+
+    @property
+    def additional_keys(self) -> List[str]:
+        return list(self.additional_properties.keys())
+
+    def __getitem__(self, key: str) -> Any:
+        return self.additional_properties[key]
+
+    def __setitem__(self, key: str, value: Any) -> None:
+        self.additional_properties[key] = value
+
+    def __delitem__(self, key: str) -> None:
+        del self.additional_properties[key]
+
+    def __contains__(self, key: str) -> bool:
+        return key in self.additional_properties

end_to_end_tests/golden-record/my_test_api_client/models/model_with_merged_properties_string_to_enum.py

@@ -0,0 +1,9 @@
+from enum import Enum
+
+
+class ModelWithMergedPropertiesStringToEnum(str, Enum):
+    A = "a"
+    B = "b"
+
+    def __str__(self) -> str:
+        return str(self.value)

openapi_python_client/parser/openapi.py

@@ -155,7 +155,7 @@ def _add_responses(
                     ParseError(
                         detail=(
                             f"Invalid response status code {code} (not a valid HTTP "
-                            f"status code), response will be ommitted from generated "
+                            f"status code), response will be omitted from generated "
                             f"client"
                         )
                     )
@@ -175,7 +175,7 @@ def _add_responses(
                     ParseError(
                         detail=(
                             f"Cannot parse response for status code {status_code}{detail_suffix}, "
-                            f"response will be ommitted from generated client"
+                            f"response will be omitted from generated client"
                         ),
                         data=response.data,
                     )

openapi_python_client/parser/properties/__init__.py

@@ -83,7 +83,6 @@ def _string_based_property(
         name=name,
         default=data.default,
         required=required,
-        pattern=data.pattern,
         python_name=python_name,
         description=data.description,
         example=data.example,

openapi_python_client/parser/properties/enum_property.py

@@ -1,6 +1,6 @@
 from __future__ import annotations
 
-__all__ = ["EnumProperty"]
+__all__ = ["EnumProperty", "ValueType"]
 
 from typing import Any, ClassVar, List, Union, cast
 
@@ -164,6 +164,12 @@ def convert_value(self, value: Any) -> Value | PropertyError | None:
                 return PropertyError(detail=f"Value {value} is not valid for enum {self.name}")
         return PropertyError(detail=f"Cannot convert {value} to enum {self.name} of type {self.value_type}")
 
+    def default_to_raw(self) -> ValueType | None:
+        if self.default is None:
+            return None
+        key = self.default.split(".")[1]
+        return self.values[key]
+
     def get_base_type_string(self, *, quoted: bool = False) -> str:
         return self.class_info.name
 

openapi_python_client/parser/properties/int.py

@@ -60,10 +60,13 @@ def convert_value(cls, value: Any) -> Value | None | PropertyError:
             return value
         if isinstance(value, str):
             try:
-                int(value)
+                value = float(value)
             except ValueError:
                 return PropertyError(f"Invalid int value: {value}")
-            return Value(value)
+        if isinstance(value, float):
+            as_int = int(value)
+            if value == as_int:
+                value = as_int
         if isinstance(value, int) and not isinstance(value, bool):
             return Value(str(value))
         return PropertyError(f"Invalid int value: {value}")