fix logic for merging object schemas within a property

.changeset/fix-model-override.md

@@ -0,0 +1,53 @@
+---
+default: patch
+---
+
+# Fix overriding of object property class
+
+Fixed issue #1123, in which a property could end up with the wrong type when combining two object schemas with `allOf`, if the type of the property was itself an object but had a different schema in each. Example:
+
+```yaml
+  ModelA:
+    properties:
+      status:
+        type: string
+      result:
+        - $ref: "#/components/schemas/BaseResult"
+
+  ModelB:
+    allOf:
+      - $ref: "#/components/schemas/ModelA"
+      - properties:
+          result:
+            - $ref: "#/components/schemas/ExtendedResult"
+
+  ModelC:
+    allOf:
+      - $ref: "#/components/schemas/ModelA"
+      - properties:
+          result:
+            - $ref: "#/components/schemas/UnrelatedResult"
+
+  BaseResult:
+    properties:
+      prop1:
+        type: string
+
+  ExtendedResult:
+    allOf:
+      - $ref: "#/components/schemas/BaseResult"
+      - properties:
+          prop2:
+            type: string
+
+  UnrelatedResult:
+    properties:
+      prop3:
+        type: string
+```
+
+Previously, in the generated classes for both `ModelB` and `ModelC`, the type of `result` was being incorrectly set to `BaseResult`.
+
+The new behavior is, when computing `allOf: [A, B]` where `A` and `B` are both objects, any property `P` whose name exists in both schemas will have a schema equivalent to `allOf: [A.P, B.P]`. This is consistent with the basic definition of `allOf`.
+
+When translating this into Python code, the generator will use a type that correctly describes the combined schema for the property. If the combined schema is exactly equal in shape to either `A.P` or `B.P` (implying that one was already derived from the other using `allOf`) then it will reuse the corresponding Python class. Otherwise it will create a new class, just as it would for an inline schema that used `allOf`. Therefore in the example above, the type of `ModelB.result` is `ExtendedResult`, but the type of `ModelC.result` is a new class called `ModelCResult` that includes all the properties from `BaseResult` and `UnrelatedResult`.

openapi_python_client/parser/properties/merge_properties.py

@@ -1,9 +1,16 @@
 from __future__ import annotations
 
+from itertools import chain
+
+from openapi_python_client import schema as oai
+from openapi_python_client import utils
+from openapi_python_client.config import Config
 from openapi_python_client.parser.properties.date import DateProperty
 from openapi_python_client.parser.properties.datetime import DateTimeProperty
 from openapi_python_client.parser.properties.file import FileProperty
 from openapi_python_client.parser.properties.literal_enum_property import LiteralEnumProperty
+from openapi_python_client.parser.properties.model_property import ModelProperty, _gather_property_data
+from openapi_python_client.parser.properties.schemas import Class
 
 __all__ = ["merge_properties"]
 
@@ -27,7 +34,12 @@
 STRING_WITH_FORMAT_TYPES = (DateProperty, DateTimeProperty, FileProperty)
 
 
-def merge_properties(prop1: Property, prop2: Property) -> Property | PropertyError:  # noqa: PLR0911
+def merge_properties(  # noqa:PLR0911
+    prop1: Property,
+    prop2: Property,
+    parent_name: str,
+    config: Config,
+) -> Property | PropertyError:
     """Attempt to create a new property that incorporates the behavior of both.
 
     This is used when merging schemas with allOf, when two schemas define a property with the same name.
@@ -57,7 +69,7 @@ def merge_properties(prop1: Property, prop2: Property) -> Property | PropertyErr
     if isinstance(prop1, LiteralEnumProperty) or isinstance(prop2, LiteralEnumProperty):
         return _merge_with_literal_enum(prop1, prop2)
 
-    if (merged := _merge_same_type(prop1, prop2)) is not None:
+    if (merged := _merge_same_type(prop1, prop2, parent_name, config)) is not None:
         return merged
 
     if (merged := _merge_numeric(prop1, prop2)) is not None:
@@ -71,16 +83,21 @@ def merge_properties(prop1: Property, prop2: Property) -> Property | PropertyErr
     )
 
 
-def _merge_same_type(prop1: Property, prop2: Property) -> Property | None | PropertyError:
+def _merge_same_type(
+    prop1: Property, prop2: Property, parent_name: str, config: Config
+) -> Property | None | PropertyError:
     if type(prop1) is not type(prop2):
         return None
 
     if prop1 == prop2:
         # It's always OK to redefine a property with everything exactly the same
         return prop1
 
+    if isinstance(prop1, ModelProperty) and isinstance(prop2, ModelProperty):
+        return _merge_models(prop1, prop2, parent_name, config)
+
     if isinstance(prop1, ListProperty) and isinstance(prop2, ListProperty):
-        inner_property = merge_properties(prop1.inner_property, prop2.inner_property)  # type: ignore
+        inner_property = merge_properties(prop1.inner_property, prop2.inner_property, "", config)  # type: ignore
         if isinstance(inner_property, PropertyError):
             return PropertyError(detail=f"can't merge list properties: {inner_property.detail}")
         prop1.inner_property = inner_property
@@ -90,6 +107,62 @@ def _merge_same_type(prop1: Property, prop2: Property) -> Property | None | Prop
     return _merge_common_attributes(prop1, prop2)
 
 
+def _merge_models(
+    prop1: ModelProperty, prop2: ModelProperty, parent_name: str, config: Config
+) -> Property | PropertyError:
+    # The logic here is basically equivalent to what we would do for a schema that was
+    # "allOf: [prop1, prop2]". We apply the property merge logic recursively and create a new third
+    # schema if the result cannot be fully described by one or the other. If it *can* be fully
+    # described by one or the other, then we can simply reuse the class for that one: for instance,
+    # in a common case where B is an object type that extends A and fully includes it, so that
+    # "allOf: [A, B]" would be the same as B, then it's valid to use the existing B model class.
+    # We would still call _merge_common_attributes in that case, to handle metadat like "description".
+    for prop in [prop1, prop2]:
+        if prop.needs_post_processing():
+            # This means not all of the details of the schema have been filled in, possibly due to a
+            # forward reference. That may be resolved in a later pass, but for now we can't proceed.
+            return PropertyError(f"Schema for {prop} in allOf was not processed", data=prop.data)
+
+    if _model_is_extension_of(prop1, prop2, parent_name, config):
+        return _merge_common_attributes(prop1, prop2)
+    elif _model_is_extension_of(prop2, prop1, parent_name, config):
+        return _merge_common_attributes(prop2, prop1, prop2)
+
+    # Neither of the schemas is a superset of the other, so merging them will result in a new type.
+    merged_props: dict[str, Property] = {p.name: p for p in chain(prop1.required_properties, prop1.optional_properties)}
+    for model in [prop1, prop2]:
+        for sub_prop in chain(model.required_properties, model.optional_properties):
+            if sub_prop.name in merged_props:
+                merged_prop = merge_properties(merged_props[sub_prop.name], sub_prop, parent_name, config)
+                if isinstance(merged_prop, PropertyError):
+                    return merged_prop
+                merged_props[sub_prop.name] = merged_prop
+            else:
+                merged_props[sub_prop.name] = sub_prop
+
+    prop_details = _gather_property_data(merged_props.values())
+
+    name = prop2.name
+    class_string = f"{utils.pascal_case(parent_name)}{utils.pascal_case(name)}"
+    class_info = Class.from_string(string=class_string, config=config)
+    roots = prop1.roots.union(prop2.roots).difference({prop1.class_info.name, prop2.class_info.name})
+    roots.add(class_info.name)
+    prop = ModelProperty(
+        class_info=class_info,
+        data=oai.Schema.model_construct(allOf=[prop1.data, prop2.data]),
+        roots=roots,
+        details=prop_details,
+        description=prop2.description or prop1.description,
+        default=None,
+        required=prop2.required or prop1.required,
+        name=name,
+        python_name=utils.PythonIdentifier(value=name, prefix=config.field_prefix),
+        example=prop2.example or prop1.example,
+    )
+
+    return prop
+
+
 def _merge_string_with_format(prop1: Property, prop2: Property) -> Property | None | PropertyError:
     """Merge a string that has no format with a string that has a format"""
     # Here we need to use the DateProperty/DateTimeProperty/FileProperty as the base so that we preserve
@@ -196,3 +269,23 @@ def _merge_common_attributes(base: PropertyT, *extend_with: PropertyProtocol) ->
 
 def _values_are_subset(prop1: EnumProperty, prop2: EnumProperty) -> bool:
     return set(prop1.values.items()) <= set(prop2.values.items())
+
+
+def _model_is_extension_of(
+    extended_model: ModelProperty, base_model: ModelProperty, parent_name: str, config: Config
+) -> bool:
+    def _properties_are_extension_of(extended_list: list[Property], base_list: list[Property]) -> bool:
+        for p2 in base_list:
+            if not [p1 for p1 in extended_list if _property_is_extension_of(p2, p1, parent_name, config)]:
+                return False
+        return True
+
+    return _properties_are_extension_of(
+        extended_model.required_properties, base_model.required_properties
+    ) and _properties_are_extension_of(extended_model.optional_properties, base_model.optional_properties)
+
+
+def _property_is_extension_of(extended_prop: Property, base_prop: Property, parent_name: str, config: Config) -> bool:
+    return base_prop.name == extended_prop.name and (
+        base_prop == extended_prop or merge_properties(base_prop, extended_prop, parent_name, config) == extended_prop
+    )