fix: use correct class name when overriding model property

Author: eli-bl

.changeset/fix-model-override.md

@@ -0,0 +1,7 @@
+---
+default: patch
+---
+
+# Fix overriding of object property class
+
+Fixed issue #1121, where redefining an object property within an `allOf` would not use the correct class name if the property's type was changed from one object type to another.

openapi_python_client/parser/properties/__init__.py

@@ -125,7 +125,7 @@ def _property_from_ref(
     return prop, schemas
 
 
-def property_from_data(  # noqa: PLR0911, PLR0912
+def property_from_data(  # noqa: PLR0911
     name: str,
     required: bool,
     data: oai.Reference | oai.Schema,

openapi_python_client/parser/properties/merge_properties.py

@@ -3,6 +3,7 @@
 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.model_property import ModelProperty
 
 __all__ = ["merge_properties"]
 
@@ -75,6 +76,9 @@ def _merge_same_type(prop1: Property, prop2: Property) -> Property | None | Prop
         # 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)
+
     if isinstance(prop1, ListProperty) and isinstance(prop2, ListProperty):
         inner_property = merge_properties(prop1.inner_property, prop2.inner_property)  # type: ignore
         if isinstance(inner_property, PropertyError):
@@ -86,6 +90,24 @@ def _merge_same_type(prop1: Property, prop2: Property) -> Property | None | Prop
     return _merge_common_attributes(prop1, prop2)
 
 
+def _merge_models(prop1: ModelProperty, prop2: ModelProperty) -> Property | PropertyError:
+    # Ideally, we would treat this case the same as a schema that consisted of "allOf: [prop1, prop2]",
+    # applying the property merge logic recursively and creating a new third schema if the result could
+    # not be fully described by one or the other. But for now we will just handle the common case where
+    # B is an object type that extends A and fully includes it, with no changes to any of A's properties;
+    # in that case, it is valid to just reuse the model class for B.
+    for prop in [prop1, prop2]:
+        if prop.needs_processing():
+            return PropertyError(f"Schema for {prop} in allOf was not processed", data=prop)
+    if _model_is_extension_of(prop1, prop2):
+        extended_model = prop1
+    elif _model_is_extension_of(prop2, prop1):
+        extended_model = prop2
+    else:
+        return PropertyError(detail="unable to merge two unrelated object types for this property")
+    return _merge_common_attributes(extended_model, prop1, prop2)
+
+
 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
@@ -166,3 +188,19 @@ 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) -> bool:
+    def _list_is_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)]:
+                return False
+        return True
+
+    return _list_is_extension_of(
+        extended_model.required_properties, base_model.required_properties
+    ) and _list_is_extension_of(extended_model.optional_properties, base_model.optional_properties)
+
+
+def _property_is_extension_of(extended_prop: PropertyProtocol, base_prop: PropertyProtocol) -> bool:
+    return base_prop.name == extended_prop.name and merge_properties(base_prop, extended_prop) == extended_prop

openapi_python_client/parser/properties/model_property.py

@@ -1,5 +1,6 @@
 from __future__ import annotations
 
+from dataclasses import dataclass
 from itertools import chain
 from typing import Any, ClassVar, NamedTuple
 
@@ -14,6 +15,15 @@
 from .schemas import Class, ReferencePath, Schemas, parse_reference_path
 
 
+@dataclass
+class ModelDetails:
+    required_properties: list[Property] | None = None
+    optional_properties: list[Property] | None = None
+    additional_properties: Property | None = None
+    relative_imports: set[str] | None = None
+    lazy_imports: set[str] | None = None
+
+
 @define
 class ModelProperty(PropertyProtocol):
     """A property which refers to another Schema"""
@@ -27,11 +37,7 @@ class ModelProperty(PropertyProtocol):
     data: oai.Schema
     description: str
     roots: set[ReferencePath | utils.ClassName]
-    required_properties: list[Property] | None
-    optional_properties: list[Property] | None
-    relative_imports: set[str] | None
-    lazy_imports: set[str] | None
-    additional_properties: Property | None
+    details: ModelDetails
     _json_type_string: ClassVar[str] = "Dict[str, Any]"
 
     template: ClassVar[str] = "model_property.py.jinja"
@@ -75,22 +81,18 @@ def build(
                 class_string = title
         class_info = Class.from_string(string=class_string, config=config)
         model_roots = {*roots, class_info.name}
-        required_properties: list[Property] | None = None
-        optional_properties: list[Property] | None = None
-        relative_imports: set[str] | None = None
-        lazy_imports: set[str] | None = None
-        additional_properties: Property | None = None
+        details = ModelDetails()
         if process_properties:
             data_or_err, schemas = _process_property_data(
                 data=data, schemas=schemas, class_info=class_info, config=config, roots=model_roots
             )
             if isinstance(data_or_err, PropertyError):
                 return data_or_err, schemas
-            property_data, additional_properties = data_or_err
-            required_properties = property_data.required_props
-            optional_properties = property_data.optional_props
-            relative_imports = property_data.relative_imports
-            lazy_imports = property_data.lazy_imports
+            property_data, details.additional_properties = data_or_err
+            details.required_properties = property_data.required_props
+            details.optional_properties = property_data.optional_props
+            details.relative_imports = property_data.relative_imports
+            details.lazy_imports = property_data.lazy_imports
             for root in roots:
                 if isinstance(root, utils.ClassName):
                     continue
@@ -100,11 +102,7 @@ def build(
             class_info=class_info,
             data=data,
             roots=model_roots,
-            required_properties=required_properties,
-            optional_properties=optional_properties,
-            relative_imports=relative_imports,
-            lazy_imports=lazy_imports,
-            additional_properties=additional_properties,
+            details=details,
             description=data.description or "",
             default=None,
             required=required,
@@ -125,14 +123,39 @@ def build(
         )
         return prop, schemas
 
+    def needs_processing(self) -> bool:
+        return not (
+            isinstance(self.details.required_properties, list) and isinstance(self.details.optional_properties, list)
+        )
+
+    @property
+    def required_properties(self) -> list[Property]:
+        return self.details.required_properties or []
+
+    @property
+    def optional_properties(self) -> list[Property]:
+        return self.details.optional_properties or []
+
+    @property
+    def additional_properties(self) -> Property | None:
+        return self.details.additional_properties
+
+    @property
+    def relative_imports(self) -> set[str]:
+        return self.details.relative_imports or set()
+
+    @property
+    def lazy_imports(self) -> set[str] | None:
+        return self.details.lazy_imports or set()
+
     @classmethod
     def convert_value(cls, value: Any) -> Value | None | PropertyError:
         if value is not None:
             return PropertyError(detail="ModelProperty cannot have a default value")  # pragma: no cover
         return None
 
     def __attrs_post_init__(self) -> None:
-        if self.relative_imports:
+        if self.details.relative_imports:
             self.set_relative_imports(self.relative_imports)
 
     @property
@@ -175,15 +198,15 @@ def set_relative_imports(self, relative_imports: set[str]) -> None:
         Args:
             relative_imports: The set of relative import strings
         """
-        object.__setattr__(self, "relative_imports", {ri for ri in relative_imports if self.self_import not in ri})
+        self.details.relative_imports = {ri for ri in relative_imports if self.self_import not in ri}
 
     def set_lazy_imports(self, lazy_imports: set[str]) -> None:
         """Set the lazy imports set for this ModelProperty, filtering out self imports
 
         Args:
             lazy_imports: The set of lazy import strings
         """
-        object.__setattr__(self, "lazy_imports", {li for li in lazy_imports if self.self_import not in li})
+        self.details.lazy_imports = {li for li in lazy_imports if self.self_import not in li}
 
     def get_type_string(
         self,
@@ -289,9 +312,7 @@ def _add_if_no_conflict(new_prop: Property) -> PropertyError | None:
             if not isinstance(sub_model, ModelProperty):
                 return PropertyError("Cannot take allOf a non-object")
             # Properties of allOf references first should be processed first
-            if not (
-                isinstance(sub_model.required_properties, list) and isinstance(sub_model.optional_properties, list)
-            ):
+            if sub_model.needs_processing():
                 return PropertyError(f"Reference {sub_model.name} in allOf was not processed", data=sub_prop)
             for prop in chain(sub_model.required_properties, sub_model.optional_properties):
                 err = _add_if_no_conflict(prop)
@@ -437,9 +458,10 @@ def process_model(model_prop: ModelProperty, *, schemas: Schemas, config: Config
 
     property_data, additional_properties = data_or_err
 
-    object.__setattr__(model_prop, "required_properties", property_data.required_props)
-    object.__setattr__(model_prop, "optional_properties", property_data.optional_props)
+    model_prop.details.required_properties = property_data.required_props
+    model_prop.details.optional_properties = property_data.optional_props
+    model_prop.details.additional_properties = additional_properties
     model_prop.set_relative_imports(property_data.relative_imports)
     model_prop.set_lazy_imports(property_data.lazy_imports)
-    object.__setattr__(model_prop, "additional_properties", additional_properties)
+
     return schemas

openapi_python_client/parser/properties/protocol.py

@@ -185,3 +185,7 @@ def is_base_type(self) -> bool:
             ListProperty.__name__,
             UnionProperty.__name__,
         }
+
+    def needs_processing(self) -> bool:
+        """Returns true if the parser should call process_model() on this property in a second pass."""
+        return False

tests/conftest.py

@@ -25,6 +25,7 @@
     UnionProperty,
 )
 from openapi_python_client.parser.properties.float import FloatProperty
+from openapi_python_client.parser.properties.model_property import ModelDetails
 from openapi_python_client.parser.properties.protocol import PropertyType, Value
 from openapi_python_client.schema.openapi_schema_pydantic import Parameter
 from openapi_python_client.schema.parameter_location import ParameterLocation
@@ -64,15 +65,25 @@ def _factory(**kwargs):
             "class_info": Class(name=ClassName("MyClass", ""), module_name=PythonIdentifier("my_module", "")),
             "data": oai.Schema.model_construct(),
             "roots": set(),
-            "required_properties": None,
-            "optional_properties": None,
-            "relative_imports": None,
-            "lazy_imports": None,
-            "additional_properties": None,
             "python_name": "",
             "example": "",
             **kwargs,
         }
+        # shortcuts for setting attributes within ModelDetails
+        if "details" not in kwargs:
+            detail_args = {}
+            for arg_name in [
+                "required_properties",
+                "optional_properties",
+                "additional_properties",
+                "relative_imports",
+                "lazy_imports",
+            ]:
+                if arg_name in kwargs:
+                    detail_args[arg_name] = kwargs[arg_name]
+                    kwargs.pop(arg_name)
+            kwargs["details"] = ModelDetails(**detail_args)
+
         return ModelProperty(**kwargs)
 
     return _factory