misc implementation changes

Author: eli-bl

.changeset/fix-model-override.md

@@ -4,4 +4,6 @@ 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.
+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. Previously, if the property's type was A in the first schema and B in the second, the resulting schema would use type A for the property.
+
+The new behavior is, that the generator will test whether one of the types A/B is derived from the other. "Derived" here means that the result of `allOf[A, B]` would be exactly identical to B. If so, it will use the class name of B. If not, it will attempt to merge A and B with the usual `allOf` logic to create a new inline schema.

openapi_python_client/parser/properties/merge_properties.py

@@ -1,9 +1,13 @@
 from __future__ import annotations
+from itertools import chain
 
+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.model_property import ModelProperty
+from openapi_python_client.parser.properties.model_property import ModelDetails, ModelProperty, _gather_property_data
+from openapi_python_client.parser.properties.schemas import Class, Schemas
 
 __all__ = ["merge_properties"]
 
@@ -27,7 +31,12 @@
 STRING_WITH_FORMAT_TYPES = (DateProperty, DateTimeProperty, FileProperty)
 
 
-def merge_properties(prop1: Property, prop2: Property) -> Property | PropertyError:  # noqa: PLR0911
+def merge_properties(
+    prop1: Property,
+    prop2: Property,
+    parent_name: str,
+    config: Config,
+) -> Property | PropertyError:  # noqa: PLR0911
     """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.
@@ -54,7 +63,7 @@ def merge_properties(prop1: Property, prop2: Property) -> Property | PropertyErr
     if isinstance(prop1, EnumProperty) or isinstance(prop2, EnumProperty):
         return _merge_with_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:
@@ -68,7 +77,7 @@ 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
 
@@ -77,10 +86,10 @@ def _merge_same_type(prop1: Property, prop2: Property) -> Property | None | Prop
         return prop1
 
     if isinstance(prop1, ModelProperty) and isinstance(prop2, ModelProperty):
-        return _merge_models(prop1, prop2)
+        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,22 +99,67 @@ 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:
+def _merge_models(prop1: ModelProperty, prop2: ModelProperty, parent_name: str, config: Config) -> 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():
+            # 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)
-    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)
+
+    # Detect whether one of the schemas is derived from the other-- that is, if it is (or is equivalent
+    # to) the result of taking the other type and adding/modifying properties with allOf. If so, then
+    # we can simply use the class of the derived type. We will still call _merge_common_attributes in
+    # case any metadata like "description" has been modified.
+    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_data = _gather_property_data(merged_props.values(), Schemas())
+    
+    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_details = ModelDetails(
+        required_properties=prop_data.required_props,
+        optional_properties=prop_data.optional_props,
+        additional_properties=None,
+        relative_imports=prop_data.relative_imports,
+        lazy_imports=prop_data.lazy_imports,
+    )
+    prop = ModelProperty(
+        class_info=class_info,
+        data=prop2.data,  # TODO: not sure what this should be
+        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:
@@ -190,17 +244,19 @@ 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:
+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)]:
+            if not [p1 for p1 in extended_list if _property_is_extension_of(p2, p1, parent_name, config)]:
                 return False
         return True
 
-    return _list_is_extension_of(
+    return _properties_are_extension_of(
         extended_model.required_properties, base_model.required_properties
-    ) and _list_is_extension_of(extended_model.optional_properties, base_model.optional_properties)
+    ) and _properties_are_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
+def _property_is_extension_of(extended_prop: PropertyProtocol, base_prop: PropertyProtocol, 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
+    )

openapi_python_client/parser/properties/model_property.py

@@ -2,7 +2,7 @@
 
 from dataclasses import dataclass
 from itertools import chain
-from typing import Any, ClassVar, NamedTuple
+from typing import Any, ClassVar, Iterable, NamedTuple
 
 from attrs import define, evolve
 
@@ -261,7 +261,7 @@ class _PropertyData(NamedTuple):
     schemas: Schemas
 
 
-def _process_properties(  # noqa: PLR0912, PLR0911
+def _process_properties(  # noqa: PLR0911
     *,
     data: oai.Schema,
     schemas: Schemas,
@@ -273,15 +273,13 @@ def _process_properties(  # noqa: PLR0912, PLR0911
     from .merge_properties import merge_properties
 
     properties: dict[str, Property] = {}
-    relative_imports: set[str] = set()
-    lazy_imports: set[str] = set()
     required_set = set(data.required or [])
 
     def _add_if_no_conflict(new_prop: Property) -> PropertyError | None:
         nonlocal properties
 
         name_conflict = properties.get(new_prop.name)
-        merged_prop = merge_properties(name_conflict, new_prop) if name_conflict else new_prop
+        merged_prop = merge_properties(name_conflict, new_prop, class_name, config) if name_conflict else new_prop
         if isinstance(merged_prop, PropertyError):
             merged_prop.header = f"Found conflicting properties named {new_prop.name} when creating {class_name}"
             return merged_prop
@@ -340,17 +338,22 @@ def _add_if_no_conflict(new_prop: Property) -> PropertyError | None:
         if isinstance(prop_or_error, PropertyError):
             return prop_or_error
 
+    return _gather_property_data(properties.values(), schemas)
+
+
+def _gather_property_data(properties: Iterable[Property], schemas: Schemas) -> _PropertyData:
     required_properties = []
     optional_properties = []
-    for prop in properties.values():
+    relative_imports: set[str] = set()
+    lazy_imports: set[str] = set()
+    for prop in properties:
         if prop.required:
             required_properties.append(prop)
         else:
             optional_properties.append(prop)
 
         lazy_imports.update(prop.get_lazy_imports(prefix=".."))
         relative_imports.update(prop.get_imports(prefix=".."))
-
     return _PropertyData(
         optional_props=optional_properties,
         required_props=required_properties,