fix: better support for merging properties with allOf

Author: eli-bl

openapi_python_client/parser/properties/merge_properties.py

@@ -0,0 +1,167 @@
+
+from collections.abc import Callable
+from typing import Any
+
+from attr import evolve
+
+from openapi_python_client.parser.properties.any import AnyProperty
+from openapi_python_client.parser.properties.enum_property import EnumProperty, ValueType
+from openapi_python_client.parser.properties.float import FloatProperty
+from openapi_python_client.parser.properties.int import IntProperty
+from openapi_python_client.parser.properties.list_property import ListProperty
+from openapi_python_client.parser.properties.protocol import PropertyProtocol
+from openapi_python_client.parser.properties.string import StringProperty
+
+
+def merge_properties(prop1: PropertyProtocol, prop2: PropertyProtocol) -> PropertyProtocol:
+    """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.
+    
+    OpenAPI defines allOf in terms of validation behavior: the input must pass the validation rules
+    defined in all of the listed schemas. Our task here is slightly more difficult, since we must end
+    up with a single Property object that will be used to generate a single class property in the
+    generated code. Due to limitations of our internal model, this may not be possible for some
+    combinations of property attributes that OpenAPI supports (for instance, we have no way to represent
+    a string property that must match two different regexes).
+
+    Properties can also have attributes that do not represent validation rules, such as "description"
+    and "example". OpenAPI does not define any overriding/aggregation rules for these in allOf. The
+    implementation here is, assuming prop1 and prop2 are in the same order that the schemas were in the
+    allOf, any such attributes that prop2 specifies will override the ones from prop1.
+
+    Any failure is thrown as a ValueError.
+    """
+    if isinstance(prop1, EnumProperty) or isinstance(prop2, EnumProperty):
+        return _merge_with_enum(prop1, prop2)
+
+    if prop1.__class__ == prop2.__class__:
+        return _merge_same_type(prop1, prop2)
+    
+    if isinstance(prop1, AnyProperty) or isinstance(prop2, AnyProperty):
+        return _merge_with_any(prop1, prop2)
+    
+    if _is_numeric(prop1) and _is_numeric(prop2):
+        return _merge_numeric(prop1, prop2)
+
+    raise ValueError("defined with two incompatible types")
+
+
+def _is_numeric(prop: PropertyProtocol) -> bool:
+    return isinstance(prop, IntProperty) or isinstance(prop, FloatProperty)
+
+
+def _merge_same_type(prop1: PropertyProtocol, prop2: PropertyProtocol) -> PropertyProtocol:
+    if prop1 == prop2:
+        # It's always OK to redefine a property with everything exactly the same
+        return prop1
+
+    if isinstance(prop1, StringProperty):
+        return _merge_string(prop1, prop2)
+
+    if isinstance(prop1, ListProperty):
+        # There's no clear way to represent the intersection of two different list types. Fail in this case.
+        if prop1.inner_property != prop2.inner_property:
+            raise ValueError("can't redefine a list property with a different element type")
+
+    # For all other property types, there aren't any special attributes that affect validation, so just
+    # apply the rules for common attributes like "description".
+    return _merge_common_attributes(prop1, prop2)
+
+
+def _merge_string(prop1: StringProperty, prop2: StringProperty) -> StringProperty:
+    # If max_length was specified for both, the smallest value takes precedence. If only one of them
+    # specifies it, _combine_values will pick that one.
+    max_length: int | None = _combine_values(prop1.max_length, prop2.max_length, lambda a, b: min([a, b]))
+
+    # If pattern was specified for both, we have a problem. OpenAPI has no logical objection to this;
+    # it would just mean the value must match both of the patterns to be valid. But we have no way to
+    # represent this in our internal model currently.
+    pattern: str | None | ValueError = _combine_values(
+        prop1.pattern,
+        prop2.pattern,
+        lambda a, b: ValueError("specified two different regex patterns")
+    )
+    if isinstance(pattern, ValueError):
+        raise pattern
+    
+    return _merge_common_attributes(evolve(prop1, max_length=max_length, pattern=pattern), prop2)
+
+
+def _merge_numeric(prop1: PropertyProtocol, prop2: PropertyProtocol) -> IntProperty:
+    # Here, one of the properties was defined as "int" and the other was just a general number (which
+    # we call FloatProperty). "Must be integer" is the stricter validation rule, so the result must be
+    # an IntProperty.
+    int_prop = prop1 if isinstance(prop1, IntProperty) else prop2
+    result = _merge_common_attributes(int_prop, prop1, prop2)
+    if result.default is not None:
+        if isinstance(result.default, float) and not result.default.is_integer():
+            raise ValueError(f"default value {result.default} is not valid for an integer property")
+    return result
+    
+
+def _merge_with_any(prop1: PropertyProtocol, prop2: PropertyProtocol) -> PropertyProtocol:
+    # AnyProperty implies no validation rules for a value, so merging it with any other type just means
+    # we should use the validation rules for the other type and the result should not be an AnyProperty.
+    non_any_prop = prop2 if isinstance(prop1, AnyProperty) else prop1
+    return _merge_common_attributes(non_any_prop, prop1, prop2)
+
+
+def _merge_with_enum(prop1: PropertyProtocol, prop2: PropertyProtocol) -> EnumProperty:
+    if isinstance(prop1, EnumProperty) and isinstance(prop2, EnumProperty):
+        # We want the narrowest validation rules that fit both, so use whichever values list is a
+        # subset of the other.
+        values: dict[str, ValueType]
+        if _values_are_subset(prop1, prop2):
+            values = prop1.values
+        elif _values_are_subset(prop2, prop1):
+            values = prop2.values
+        else:
+            raise ValueError("can't redefine an enum property with incompatible lists of values")
+        return _merge_common_attributes(evolve(prop1, values=values), prop2)
+
+    # If enum values were specified for just one of the properties, use those.
+    enum_prop = prop1 if isinstance(prop1, EnumProperty) else prop2
+    non_enum_prop = prop2 if isinstance(prop1, EnumProperty) else prop1
+    if (
+        (isinstance(non_enum_prop, IntProperty) and enum_prop.value_type is int) or
+        (isinstance(non_enum_prop, StringProperty) and enum_prop.value_type is str)
+    ):
+        return _merge_common_attributes(enum_prop, prop1, prop2)
+    raise ValueError("defined with two incompatible types")
+
+
+def _merge_common_attributes(base: PropertyProtocol, *extend_with: PropertyProtocol) -> PropertyProtocol:
+    """Create a new instance based on base, overriding basic attributes with values from extend_with, in order.
+    
+    For "default", "description", and "example", a non-None value overrides any value from a previously
+    specified property. The behavior is similar to using the spread operator with dictionaries, except
+    that None means "not specified".
+
+    For "required", any True value overrides all other values (a property that was previously required
+    cannot become optional).
+    """
+    current = base
+    for override in extend_with:
+        current = evolve(
+            current,
+            required=current.required or override.required,
+            default = override.default or current.default,
+            description = override.description or current.description,
+            example = override.example or current.example,
+    )
+    return current
+
+
+def _values_are_subset(prop1: EnumProperty, prop2: EnumProperty) -> bool:
+    return set(prop1.values.items()) <= set(prop2.values.items())
+
+
+def _combine_values(value1: Any, value2: Any, combinator: Callable) -> Any:
+    if value1 == value2:
+        return value1
+    if value1 is None:
+        return value2
+    if value2 is None:
+        return value1
+    return combinator(value1, value2)

openapi_python_client/parser/properties/model_property.py

@@ -5,12 +5,13 @@
 
 from attrs import define, evolve
 
+from openapi_python_client.parser.properties.merge_properties import merge_properties
+
 from ... import Config, utils
 from ... import schema as oai
 from ...utils import PythonIdentifier
 from ..errors import ParseError, PropertyError
 from .any import AnyProperty
-from .enum_property import EnumProperty
 from .protocol import PropertyProtocol, Value
 from .schemas import Class, ReferencePath, Schemas, parse_reference_path
 
@@ -216,59 +217,6 @@ def get_type_string(
 from .property import Property  # noqa: E402
 
 
-def _values_are_subset(first: EnumProperty, second: EnumProperty) -> bool:
-    return set(first.values.items()) <= set(second.values.items())
-
-
-def _types_are_subset(first: EnumProperty, second: Property) -> bool:
-    from . import IntProperty, StringProperty
-
-    if first.value_type is int and isinstance(second, IntProperty):
-        return True
-    if first.value_type is str and isinstance(second, StringProperty):
-        return True
-    return False
-
-
-def _enum_subset(first: Property, second: Property) -> EnumProperty | None:
-    """Return the EnumProperty that is the subset of the other, if possible."""
-
-    if isinstance(first, EnumProperty):
-        if isinstance(second, EnumProperty):
-            if _values_are_subset(first, second):
-                return first
-            if _values_are_subset(second, first):
-                return second
-            return None
-        return first if _types_are_subset(first, second) else None
-
-    if isinstance(second, EnumProperty) and _types_are_subset(second, first):
-        return second
-    return None
-
-
-def _merge_properties(first: Property, second: Property) -> Property | PropertyError:
-    required = first.required or second.required
-
-    err = None
-
-    if first.__class__ == second.__class__:
-        first = evolve(first, required=required)
-        second = evolve(second, required=required)
-        if first == second:
-            return first
-        err = PropertyError(header="Cannot merge properties", detail="Properties has conflicting values")
-
-    enum_subset = _enum_subset(first, second)
-    if enum_subset is not None:
-        return evolve(enum_subset, required=required)
-
-    return err or PropertyError(
-        header="Cannot merge properties",
-        detail=f"{first.__class__}, {second.__class__}Properties have incompatible types",
-    )
-
-
 def _resolve_naming_conflict(first: Property, second: Property, config: Config) -> PropertyError | None:
     first.set_python_name(first.name, config=config, skip_snake_case=True)
     second.set_python_name(second.name, config=config, skip_snake_case=True)
@@ -307,23 +255,24 @@ def _add_if_no_conflict(new_prop: Property) -> PropertyError | None:
         nonlocal properties
 
         name_conflict = properties.get(new_prop.name)
-        merged_prop_or_error = _merge_properties(name_conflict, new_prop) if name_conflict else new_prop
-        if isinstance(merged_prop_or_error, PropertyError):
-            merged_prop_or_error.header = (
-                f"Found conflicting properties named {new_prop.name} when creating {class_name}"
+        try:
+            merged_prop = merge_properties(name_conflict, new_prop) if name_conflict else new_prop
+        except ValueError as e:
+            return PropertyError(
+                header=f"Found conflicting properties named {new_prop.name} when creating {class_name}",
+                detail=str(e),
             )
-            return merged_prop_or_error
 
         for other_prop in properties.values():
-            if other_prop.name == merged_prop_or_error.name:
+            if other_prop.name == merged_prop.name:
                 continue  # Same property, probably just got merged
-            if other_prop.python_name != merged_prop_or_error.python_name:
+            if other_prop.python_name != merged_prop.python_name:
                 continue
-            naming_error = _resolve_naming_conflict(merged_prop_or_error, other_prop, config)
+            naming_error = _resolve_naming_conflict(merged_prop, other_prop, config)
             if naming_error is not None:
                 return naming_error
 
-        properties[merged_prop_or_error.name] = merged_prop_or_error
+        properties[merged_prop.name] = merged_prop
         return None
 
     unprocessed_props = data.properties or {}