fix: Handle duplicate and conflicting properties in allOf

Author: dbanty

openapi_python_client/parser/properties/model_property.py

@@ -1,4 +1,5 @@
-from typing import ClassVar, List, NamedTuple, Optional, Set, Tuple, Union
+from itertools import chain
+from typing import ClassVar, Dict, List, NamedTuple, Optional, Set, Tuple, Union
 
 import attr
 
@@ -54,6 +55,18 @@ def get_imports(self, *, prefix: str) -> Set[str]:
         return imports
 
 
+def _merge_properties(first: Property, second: Property) -> Union[Property, PropertyError]:
+    if first.__class__ != second.__class__:
+        return PropertyError(header="Cannot merge properties", detail="Properties are two different types")
+    nullable = first.nullable and second.nullable
+    required = first.required or second.required
+    first = attr.evolve(first, nullable=nullable, required=required)
+    second = attr.evolve(second, nullable=nullable, required=required)
+    if first != second:
+        return PropertyError(header="Cannot merge properties", detail="Properties has conflicting values")
+    return first
+
+
 class _PropertyData(NamedTuple):
     optional_props: List[Property]
     required_props: List[Property]
@@ -64,33 +77,50 @@ class _PropertyData(NamedTuple):
 def _process_properties(*, data: oai.Schema, schemas: Schemas, class_name: str) -> Union[_PropertyData, PropertyError]:
     from . import property_from_data
 
-    required_properties: List[Property] = []
-    optional_properties: List[Property] = []
+    properties: Dict[str, Property] = {}
     relative_imports: Set[str] = set()
     required_set = set(data.required or [])
 
+    def _check_existing(prop: Property) -> Union[Property, PropertyError]:
+        existing = properties.get(prop.name)
+        prop_or_error = (existing and _merge_properties(existing, prop)) or prop
+        if isinstance(prop_or_error, PropertyError):
+            prop_or_error.header = f"Found conflicting properties named {prop.name} when creating {class_name}"
+            return prop_or_error
+        properties[prop_or_error.name] = prop_or_error
+        return prop_or_error
+
     all_props = data.properties or {}
     for sub_prop in data.allOf or []:
         if isinstance(sub_prop, oai.Reference):
             source_name = Reference.from_ref(sub_prop.ref).class_name
             sub_model = schemas.models.get(source_name)
             if sub_model is None:
                 return PropertyError(f"Reference {sub_prop.ref} not found")
-            required_properties.extend(sub_model.required_properties)
-            optional_properties.extend(sub_model.optional_properties)
-            relative_imports.update(sub_model.relative_imports)
+            for prop in chain(sub_model.required_properties, sub_model.optional_properties):
+                prop_or_error = _check_existing(prop)
+                if isinstance(prop_or_error, PropertyError):
+                    return prop_or_error
         else:
             all_props.update(sub_prop.properties or {})
             required_set.update(sub_prop.required or [])
 
     for key, value in all_props.items():
         prop_required = key in required_set
-        prop, schemas = property_from_data(
+        prop_or_error, schemas = property_from_data(
             name=key, required=prop_required, data=value, schemas=schemas, parent_name=class_name
         )
-        if isinstance(prop, PropertyError):
-            return prop
-        if prop_required and not prop.nullable:
+        if isinstance(prop_or_error, Property):
+            prop_or_error = _check_existing(prop_or_error)
+        if isinstance(prop_or_error, PropertyError):
+            return prop_or_error
+
+        properties[prop_or_error.name] = prop_or_error
+
+    required_properties = []
+    optional_properties = []
+    for prop in properties.values():
+        if prop.required and not prop.nullable:
             required_properties.append(prop)
         else:
             optional_properties.append(prop)

tests/test_parser/test_properties/test_model_property.py

@@ -1,3 +1,5 @@
+from typing import Callable
+
 import pytest
 
 import openapi_python_client.schema as oai
@@ -202,3 +204,110 @@ def test_bad_additional_props_return_error(self):
 
         assert new_schemas == schemas
         assert err == PropertyError(detail="unknown type not_real", data=oai.Schema(type="not_real"))
+
+
+@pytest.fixture
+def model_property() -> Callable[..., ModelProperty]:
+    from openapi_python_client.parser.reference import Reference
+
+    def _factory(**kwargs):
+        kwargs = {
+            "name": "",
+            "description": "",
+            "required": True,
+            "nullable": True,
+            "default": None,
+            "reference": Reference(class_name="", module_name=""),
+            "required_properties": [],
+            "optional_properties": [],
+            "relative_imports": set(),
+            "additional_properties": False,
+            **kwargs,
+        }
+        return ModelProperty(**kwargs)
+
+    return _factory
+
+
+def string_property(**kwargs) -> StringProperty:
+    kwargs = {
+        "name": "",
+        "required": True,
+        "nullable": True,
+        "default": None,
+        **kwargs,
+    }
+    return StringProperty(**kwargs)
+
+
+class TestProcessProperties:
+    def test_conflicting_properties(self, model_property):
+        from openapi_python_client.parser.properties import Schemas
+        from openapi_python_client.parser.properties.model_property import _process_properties
+
+        data = oai.Schema.construct(allOf=[oai.Reference.construct(ref="First"), oai.Reference.construct(ref="Second")])
+        schemas = Schemas(
+            models={
+                "First": model_property(
+                    optional_properties=[StringProperty(name="prop", required=True, nullable=True, default=None)]
+                ),
+                "Second": model_property(
+                    optional_properties=[DateTimeProperty(name="prop", required=True, nullable=True, default=None)]
+                ),
+            }
+        )
+
+        result = _process_properties(data=data, schemas=schemas, class_name="")
+
+        assert isinstance(result, PropertyError)
+
+    def test_duplicate_properties(self, model_property):
+        from openapi_python_client.parser.properties import Schemas
+        from openapi_python_client.parser.properties.model_property import _process_properties
+
+        data = oai.Schema.construct(allOf=[oai.Reference.construct(ref="First"), oai.Reference.construct(ref="Second")])
+        prop = string_property()
+        schemas = Schemas(
+            models={
+                "First": model_property(optional_properties=[prop]),
+                "Second": model_property(optional_properties=[prop]),
+            }
+        )
+
+        result = _process_properties(data=data, schemas=schemas, class_name="")
+
+        assert result.optional_props == [prop], "There should only be one copy of duplicate properties"
+
+    @pytest.mark.parametrize("first_nullable", [True, False])
+    @pytest.mark.parametrize("second_nullable", [True, False])
+    @pytest.mark.parametrize("first_required", [True, False])
+    @pytest.mark.parametrize("second_required", [True, False])
+    def test_mixed_requirements(self, model_property, first_nullable, second_nullable, first_required, second_required):
+        from openapi_python_client.parser.properties import Schemas
+        from openapi_python_client.parser.properties.model_property import _process_properties
+
+        data = oai.Schema.construct(allOf=[oai.Reference.construct(ref="First"), oai.Reference.construct(ref="Second")])
+        schemas = Schemas(
+            models={
+                "First": model_property(
+                    optional_properties=[string_property(required=first_required, nullable=first_nullable)]
+                ),
+                "Second": model_property(
+                    optional_properties=[string_property(required=second_required, nullable=second_nullable)]
+                ),
+            }
+        )
+
+        result = _process_properties(data=data, schemas=schemas, class_name="")
+
+        nullable = first_nullable and second_nullable
+        required = first_required or second_required
+        expected_prop = string_property(
+            nullable=nullable,
+            required=required,
+        )
+
+        if nullable or not required:
+            assert result.optional_props == [expected_prop]
+        else:
+            assert result.required_props == [expected_prop]