diff --git a/.changeset/improved_property_merging.md b/.changeset/improved_property_merging.md new file mode 100644 index 000000000..68f3b9ed4 --- /dev/null +++ b/.changeset/improved_property_merging.md @@ -0,0 +1,20 @@ +--- +default: minor +--- + +# Improved property-merging behavior with `allOf` + +When using `allOf` to extend a base object type, `openapi-python-client` is now able to handle some kinds of modifications to an existing property that would have previously caused an error: + +- Overriding attributes that do not affect validation, such as `description`. +- Combining properties that this generator ignores, like `maxLength` or `pattern`. +- Combining a generic numeric type with `int` (resulting in `int`). +- Adding a `format` to a string. +- Combining `any` with a specific type (resulting in that specific type). +- Adding or overriding a `default` + +> [!NOTE] +> `pattern` and `max_length` are no longer fields on `StringProperty`, which may impact custom templates. + +This also fixes a bug where properties of inline objects (as opposed to references) were not using the +merge logic, but were simply overwriting previous definitions of the same property. diff --git a/end_to_end_tests/baseline_openapi_3.0.json b/end_to_end_tests/baseline_openapi_3.0.json index 68ff2a9d0..4fd2711cf 100644 --- a/end_to_end_tests/baseline_openapi_3.0.json +++ b/end_to_end_tests/baseline_openapi_3.0.json @@ -2152,6 +2152,56 @@ "additionalProperties": {} } }, + "ModelWithMergedProperties": { + "title": "ModelWithMergedProperties", + "allOf": [ + { + "type": "object", + "properties": { + "simpleString": { + "type": "string", + "description": "base simpleString description" + }, + "stringToEnum": { + "type": "string", + "default": "a" + }, + "stringToDate": { + "type": "string" + }, + "numberToInt": { + "type": "number" + }, + "anyToString": {} + } + }, + { + "type": "object", + "properties": { + "simpleString": { + "type": "string", + "description": "extended simpleString description", + "default": "new default" + }, + "stringToEnum": { + "type": "string", + "enum": ["a", "b"] + }, + "stringToDate": { + "type": "string", + "format": "date" + }, + "numberToInt": { + "type": "integer" + }, + "anyToString": { + "type": "string", + "default": "x" + } + } + } + ] + }, "ModelWithPrimitiveAdditionalProperties": { "title": "ModelWithPrimitiveAdditionalProperties", "type": "object", diff --git a/end_to_end_tests/baseline_openapi_3.1.yaml b/end_to_end_tests/baseline_openapi_3.1.yaml index b39be67c0..c100ed296 100644 --- a/end_to_end_tests/baseline_openapi_3.1.yaml +++ b/end_to_end_tests/baseline_openapi_3.1.yaml @@ -2146,6 +2146,56 @@ info: "additionalProperties": { } } }, + "ModelWithMergedProperties": { + "title": "ModelWithMergedProperties", + "allOf": [ + { + "type": "object", + "properties": { + "simpleString": { + "type": "string", + "description": "base simpleString description" + }, + "stringToEnum": { + "type": "string", + "default": "a" + }, + "stringToDate": { + "type": "string" + }, + "numberToInt": { + "type": "number" + }, + "anyToString": {} + } + }, + { + "type": "object", + "properties": { + "simpleString": { + "type": "string", + "description": "extended simpleString description", + "default": "new default" + }, + "stringToEnum": { + "type": "string", + "enum": ["a", "b"] + }, + "stringToDate": { + "type": "string", + "format": "date" + }, + "numberToInt": { + "type": "integer" + }, + "anyToString": { + "type": "string", + "default": "x" + } + } + } + ] + }, "ModelWithPrimitiveAdditionalProperties": { "title": "ModelWithPrimitiveAdditionalProperties", "type": "object", diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/__init__.py b/end_to_end_tests/golden-record/my_test_api_client/models/__init__.py index d28f05268..7a9c2ad32 100644 --- a/end_to_end_tests/golden-record/my_test_api_client/models/__init__.py +++ b/end_to_end_tests/golden-record/my_test_api_client/models/__init__.py @@ -60,6 +60,8 @@ from .model_with_circular_ref_in_additional_properties_b import ModelWithCircularRefInAdditionalPropertiesB from .model_with_date_time_property import ModelWithDateTimeProperty from .model_with_discriminated_union import ModelWithDiscriminatedUnion +from .model_with_merged_properties import ModelWithMergedProperties +from .model_with_merged_properties_string_to_enum import ModelWithMergedPropertiesStringToEnum from .model_with_no_properties import ModelWithNoProperties from .model_with_primitive_additional_properties import ModelWithPrimitiveAdditionalProperties from .model_with_primitive_additional_properties_a_date_holder import ModelWithPrimitiveAdditionalPropertiesADateHolder @@ -138,6 +140,8 @@ "ModelWithCircularRefInAdditionalPropertiesB", "ModelWithDateTimeProperty", "ModelWithDiscriminatedUnion", + "ModelWithMergedProperties", + "ModelWithMergedPropertiesStringToEnum", "ModelWithNoProperties", "ModelWithPrimitiveAdditionalProperties", "ModelWithPrimitiveAdditionalPropertiesADateHolder", diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/model_with_merged_properties.py b/end_to_end_tests/golden-record/my_test_api_client/models/model_with_merged_properties.py new file mode 100644 index 000000000..bcf1efa88 --- /dev/null +++ b/end_to_end_tests/golden-record/my_test_api_client/models/model_with_merged_properties.py @@ -0,0 +1,112 @@ +import datetime +from typing import Any, Dict, List, Type, TypeVar, Union + +from attrs import define as _attrs_define +from attrs import field as _attrs_field +from dateutil.parser import isoparse + +from ..models.model_with_merged_properties_string_to_enum import ModelWithMergedPropertiesStringToEnum +from ..types import UNSET, Unset + +T = TypeVar("T", bound="ModelWithMergedProperties") + + +@_attrs_define +class ModelWithMergedProperties: + """ + Attributes: + simple_string (Union[Unset, str]): extended simpleString description Default: 'new default'. + string_to_enum (Union[Unset, ModelWithMergedPropertiesStringToEnum]): Default: + ModelWithMergedPropertiesStringToEnum.A. + string_to_date (Union[Unset, datetime.date]): + number_to_int (Union[Unset, int]): + any_to_string (Union[Unset, str]): Default: 'x'. + """ + + simple_string: Union[Unset, str] = "new default" + string_to_enum: Union[Unset, ModelWithMergedPropertiesStringToEnum] = ModelWithMergedPropertiesStringToEnum.A + string_to_date: Union[Unset, datetime.date] = UNSET + number_to_int: Union[Unset, int] = UNSET + any_to_string: Union[Unset, str] = "x" + additional_properties: Dict[str, Any] = _attrs_field(init=False, factory=dict) + + def to_dict(self) -> Dict[str, Any]: + simple_string = self.simple_string + + string_to_enum: Union[Unset, str] = UNSET + if not isinstance(self.string_to_enum, Unset): + string_to_enum = self.string_to_enum.value + + string_to_date: Union[Unset, str] = UNSET + if not isinstance(self.string_to_date, Unset): + string_to_date = self.string_to_date.isoformat() + + number_to_int = self.number_to_int + + any_to_string = self.any_to_string + + field_dict: Dict[str, Any] = {} + field_dict.update(self.additional_properties) + field_dict.update({}) + if simple_string is not UNSET: + field_dict["simpleString"] = simple_string + if string_to_enum is not UNSET: + field_dict["stringToEnum"] = string_to_enum + if string_to_date is not UNSET: + field_dict["stringToDate"] = string_to_date + if number_to_int is not UNSET: + field_dict["numberToInt"] = number_to_int + if any_to_string is not UNSET: + field_dict["anyToString"] = any_to_string + + return field_dict + + @classmethod + def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T: + d = src_dict.copy() + simple_string = d.pop("simpleString", UNSET) + + _string_to_enum = d.pop("stringToEnum", UNSET) + string_to_enum: Union[Unset, ModelWithMergedPropertiesStringToEnum] + if isinstance(_string_to_enum, Unset): + string_to_enum = UNSET + else: + string_to_enum = ModelWithMergedPropertiesStringToEnum(_string_to_enum) + + _string_to_date = d.pop("stringToDate", UNSET) + string_to_date: Union[Unset, datetime.date] + if isinstance(_string_to_date, Unset): + string_to_date = UNSET + else: + string_to_date = isoparse(_string_to_date).date() + + number_to_int = d.pop("numberToInt", UNSET) + + any_to_string = d.pop("anyToString", UNSET) + + model_with_merged_properties = cls( + simple_string=simple_string, + string_to_enum=string_to_enum, + string_to_date=string_to_date, + number_to_int=number_to_int, + any_to_string=any_to_string, + ) + + model_with_merged_properties.additional_properties = d + return model_with_merged_properties + + @property + def additional_keys(self) -> List[str]: + return list(self.additional_properties.keys()) + + def __getitem__(self, key: str) -> Any: + return self.additional_properties[key] + + def __setitem__(self, key: str, value: Any) -> None: + self.additional_properties[key] = value + + def __delitem__(self, key: str) -> None: + del self.additional_properties[key] + + def __contains__(self, key: str) -> bool: + return key in self.additional_properties diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/model_with_merged_properties_string_to_enum.py b/end_to_end_tests/golden-record/my_test_api_client/models/model_with_merged_properties_string_to_enum.py new file mode 100644 index 000000000..5e146c5eb --- /dev/null +++ b/end_to_end_tests/golden-record/my_test_api_client/models/model_with_merged_properties_string_to_enum.py @@ -0,0 +1,9 @@ +from enum import Enum + + +class ModelWithMergedPropertiesStringToEnum(str, Enum): + A = "a" + B = "b" + + def __str__(self) -> str: + return str(self.value) diff --git a/openapi_python_client/parser/openapi.py b/openapi_python_client/parser/openapi.py index 0d6a7424b..9d62e1df5 100644 --- a/openapi_python_client/parser/openapi.py +++ b/openapi_python_client/parser/openapi.py @@ -155,7 +155,7 @@ def _add_responses( ParseError( detail=( f"Invalid response status code {code} (not a valid HTTP " - f"status code), response will be ommitted from generated " + f"status code), response will be omitted from generated " f"client" ) ) @@ -175,7 +175,7 @@ def _add_responses( ParseError( detail=( f"Cannot parse response for status code {status_code}{detail_suffix}, " - f"response will be ommitted from generated client" + f"response will be omitted from generated client" ), data=response.data, ) diff --git a/openapi_python_client/parser/properties/__init__.py b/openapi_python_client/parser/properties/__init__.py index b85dac635..c8777f509 100644 --- a/openapi_python_client/parser/properties/__init__.py +++ b/openapi_python_client/parser/properties/__init__.py @@ -83,7 +83,6 @@ def _string_based_property( name=name, default=data.default, required=required, - pattern=data.pattern, python_name=python_name, description=data.description, example=data.example, diff --git a/openapi_python_client/parser/properties/enum_property.py b/openapi_python_client/parser/properties/enum_property.py index 0f0db0d61..0e5c80588 100644 --- a/openapi_python_client/parser/properties/enum_property.py +++ b/openapi_python_client/parser/properties/enum_property.py @@ -1,6 +1,6 @@ from __future__ import annotations -__all__ = ["EnumProperty"] +__all__ = ["EnumProperty", "ValueType"] from typing import Any, ClassVar, List, Union, cast @@ -164,6 +164,12 @@ def convert_value(self, value: Any) -> Value | PropertyError | None: return PropertyError(detail=f"Value {value} is not valid for enum {self.name}") return PropertyError(detail=f"Cannot convert {value} to enum {self.name} of type {self.value_type}") + def default_to_raw(self) -> ValueType | None: + if self.default is None: + return None + key = self.default.split(".")[1] + return self.values[key] + def get_base_type_string(self, *, quoted: bool = False) -> str: return self.class_info.name diff --git a/openapi_python_client/parser/properties/int.py b/openapi_python_client/parser/properties/int.py index ab7173d3d..b34663a34 100644 --- a/openapi_python_client/parser/properties/int.py +++ b/openapi_python_client/parser/properties/int.py @@ -60,10 +60,13 @@ def convert_value(cls, value: Any) -> Value | None | PropertyError: return value if isinstance(value, str): try: - int(value) + value = float(value) except ValueError: return PropertyError(f"Invalid int value: {value}") - return Value(value) + if isinstance(value, float): + as_int = int(value) + if value == as_int: + value = as_int if isinstance(value, int) and not isinstance(value, bool): return Value(str(value)) return PropertyError(f"Invalid int value: {value}") diff --git a/openapi_python_client/parser/properties/merge_properties.py b/openapi_python_client/parser/properties/merge_properties.py new file mode 100644 index 000000000..f4e72849b --- /dev/null +++ b/openapi_python_client/parser/properties/merge_properties.py @@ -0,0 +1,165 @@ +from __future__ import annotations + +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 + +__all__ = ["merge_properties"] + +from typing import TypeVar, cast + +from attr import evolve + +from ..errors import PropertyError +from . import FloatProperty +from .any import AnyProperty +from .enum_property import EnumProperty +from .int import IntProperty +from .list_property import ListProperty +from .property import Property +from .protocol import PropertyProtocol +from .string import StringProperty + +PropertyT = TypeVar("PropertyT", bound=PropertyProtocol) + + +STRING_WITH_FORMAT_TYPES = (DateProperty, DateTimeProperty, FileProperty) + + +def merge_properties(prop1: Property, prop2: Property) -> 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. + + OpenAPI defines allOf in terms of validation behavior: the input must pass the validation rules + defined in all 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. + """ + if isinstance(prop2, AnyProperty): + return _merge_common_attributes(prop1, prop2) + + if isinstance(prop1, AnyProperty): + # Use the base type of `prop2`, but keep the override order + return _merge_common_attributes(prop2, prop1, prop2) + + if isinstance(prop1, EnumProperty) or isinstance(prop2, EnumProperty): + return _merge_with_enum(prop1, prop2) + + if (merged := _merge_same_type(prop1, prop2)) is not None: + return merged + + if (merged := _merge_numeric(prop1, prop2)) is not None: + return merged + + if (merged := _merge_string_with_format(prop1, prop2)) is not None: + return merged + + return PropertyError( + detail=f"{prop1.get_type_string(no_optional=True)} can't be merged with {prop2.get_type_string(no_optional=True)}" + ) + + +def _merge_same_type(prop1: Property, prop2: Property) -> 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, ListProperty) and isinstance(prop2, ListProperty): + inner_property = merge_properties(prop1.inner_property, prop2.inner_property) # type: ignore + if isinstance(inner_property, PropertyError): + return PropertyError(detail=f"can't merge list properties: {inner_property.detail}") + prop1.inner_property = inner_property + + # 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_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 + # its class, but keep the correct override order for merging the attributes. + if isinstance(prop1, StringProperty) and isinstance(prop2, STRING_WITH_FORMAT_TYPES): + # Use the more specific class as a base, but keep the correct override order + return _merge_common_attributes(prop2, prop1, prop2) + elif isinstance(prop2, StringProperty) and isinstance(prop1, STRING_WITH_FORMAT_TYPES): + return _merge_common_attributes(prop1, prop2) + else: + return None + + +def _merge_numeric(prop1: Property, prop2: Property) -> IntProperty | None | PropertyError: + """Merge IntProperty with FloatProperty""" + if isinstance(prop1, IntProperty) and isinstance(prop2, (IntProperty, FloatProperty)): + return _merge_common_attributes(prop1, prop2) + elif isinstance(prop2, IntProperty) and isinstance(prop1, (IntProperty, FloatProperty)): + # Use the IntProperty as a base since it's more restrictive, but keep the correct override order + return _merge_common_attributes(prop2, prop1, prop2) + else: + return None + + +def _merge_with_enum(prop1: PropertyProtocol, prop2: PropertyProtocol) -> EnumProperty | PropertyError: + 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. + if _values_are_subset(prop1, prop2): + values = prop1.values + class_info = prop1.class_info + elif _values_are_subset(prop2, prop1): + values = prop2.values + class_info = prop2.class_info + else: + return PropertyError(detail="can't redefine an enum property with incompatible lists of values") + return _merge_common_attributes(evolve(prop1, values=values, class_info=class_info), prop2) + + # If enum values were specified for just one of the properties, use those. + enum_prop = prop1 if isinstance(prop1, EnumProperty) else cast(EnumProperty, 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) + return PropertyError( + detail=f"can't combine enum of type {enum_prop.value_type} with {non_enum_prop.get_type_string(no_optional=True)}" + ) + + +def _merge_common_attributes(base: PropertyT, *extend_with: PropertyProtocol) -> PropertyT | PropertyError: + """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: + override_default = current.convert_value(override.default_to_raw()) + if isinstance(override_default, PropertyError): + return override_default + current = evolve( + current, # type: ignore # can't prove that every property type is an attrs class, but it is + 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()) diff --git a/openapi_python_client/parser/properties/model_property.py b/openapi_python_client/parser/properties/model_property.py index 897632fce..0dc13be54 100644 --- a/openapi_python_client/parser/properties/model_property.py +++ b/openapi_python_client/parser/properties/model_property.py @@ -10,7 +10,6 @@ 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 @@ -220,59 +219,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) @@ -301,6 +247,7 @@ def _process_properties( # noqa: PLR0912, PLR0911 roots: set[ReferencePath | utils.ClassName], ) -> _PropertyData | PropertyError: from . import property_from_data + from .merge_properties import merge_properties properties: dict[str, Property] = {} relative_imports: set[str] = set() @@ -311,26 +258,26 @@ 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}" - ) - return merged_prop_or_error + merged_prop = merge_properties(name_conflict, new_prop) 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 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 {} + unprocessed_props: list[tuple[str, oai.Reference | oai.Schema]] = ( + list(data.properties.items()) if data.properties else [] + ) for sub_prop in data.allOf: if isinstance(sub_prop, oai.Reference): ref_path = parse_reference_path(sub_prop.ref) @@ -352,10 +299,10 @@ def _add_if_no_conflict(new_prop: Property) -> PropertyError | None: return err schemas.add_dependencies(ref_path=ref_path, roots=roots) else: - unprocessed_props.update(sub_prop.properties or {}) + unprocessed_props.extend(sub_prop.properties.items() if sub_prop.properties else []) required_set.update(sub_prop.required or []) - for key, value in unprocessed_props.items(): + for key, value in unprocessed_props: prop_required = key in required_set prop_or_error: Property | (PropertyError | None) prop_or_error, schemas = property_from_data( diff --git a/openapi_python_client/parser/properties/protocol.py b/openapi_python_client/parser/properties/protocol.py index e3f2ceaac..14af0c976 100644 --- a/openapi_python_client/parser/properties/protocol.py +++ b/openapi_python_client/parser/properties/protocol.py @@ -177,3 +177,9 @@ def is_base_type(self) -> bool: ListProperty.__name__, UnionProperty.__name__, } + + def default_to_raw(self) -> Any | None: + d = self.default + if not isinstance(d, Value): + return d + return eval(str(d)) # This should be safe because we've already escaped string values diff --git a/openapi_python_client/parser/properties/string.py b/openapi_python_client/parser/properties/string.py index 1afe02c62..f9adf04ae 100644 --- a/openapi_python_client/parser/properties/string.py +++ b/openapi_python_client/parser/properties/string.py @@ -21,8 +21,6 @@ class StringProperty(PropertyProtocol): python_name: PythonIdentifier description: str | None example: str | None - max_length: int | None = None - pattern: str | None = None _type_string: ClassVar[str] = "str" _json_type_string: ClassVar[str] = "str" _allowed_locations: ClassVar[set[oai.ParameterLocation]] = { @@ -41,7 +39,6 @@ def build( python_name: PythonIdentifier, description: str | None, example: str | None, - pattern: str | None = None, ) -> StringProperty | PropertyError: checked_default = cls.convert_value(default) return cls( @@ -51,7 +48,6 @@ def build( python_name=python_name, description=description, example=example, - pattern=pattern, ) @classmethod diff --git a/tests/conftest.py b/tests/conftest.py index 91b67752c..a15a15741 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,6 @@ +import inspect from pathlib import Path -from typing import Any, Callable, Dict +from typing import Any, Callable, Dict, Union import pytest @@ -20,6 +21,8 @@ StringProperty, UnionProperty, ) +from openapi_python_client.parser.properties.float import FloatProperty +from openapi_python_client.parser.properties.protocol import Value from openapi_python_client.schema.openapi_schema_pydantic import Parameter from openapi_python_client.schema.parameter_location import ParameterLocation @@ -67,6 +70,31 @@ def _factory(**kwargs): return _factory +def _simple_factory(cls: type, default_kwargs: Union[dict, Callable[[dict], dict], None] = None): + def _factory(**kwargs): + kwargs = _common_kwargs(kwargs) + defaults = default_kwargs + if defaults: + if callable(defaults): + defaults = defaults(kwargs) + kwargs = {**defaults, **kwargs} + # It's very easy to accidentally set "default" to a raw value rather than a Value in our test + # code, which is never valid but mypy can't catch it for us. So we'll transform it here. + default_value = kwargs.get("default") + if default_value is not None and not isinstance(default_value, Value): + # Some of our property classes have convert_value as a class method; others have it as + # an instance method (because the logic requires knowing the state of the property). We + # can only call it here if it's a class method. + if inspect.ismethod(cls.convert_value) and cls.convert_value.__self__ is cls: + kwargs["default"] = cls.convert_value(default_value) + else: + kwargs["default"] = Value(str(default_value)) + rv = cls(**kwargs) + return rv + + return _factory + + @pytest.fixture def enum_property_factory() -> Callable[..., EnumProperty]: """ @@ -76,17 +104,14 @@ def enum_property_factory() -> Callable[..., EnumProperty]: """ from openapi_python_client.parser.properties import Class - def _factory(**kwargs): - kwargs = _common_kwargs(kwargs) - kwargs = { + return _simple_factory( + EnumProperty, + lambda kwargs: { "class_info": Class(name=kwargs["name"], module_name=kwargs["name"]), "values": {}, "value_type": str, - **kwargs, - } - return EnumProperty(**kwargs) - - return _factory + }, + ) @pytest.fixture @@ -97,11 +122,7 @@ def any_property_factory() -> Callable[..., AnyProperty]: You can pass the same params into this as the AnyProperty constructor to override defaults. """ - def _factory(**kwargs): - kwargs = _common_kwargs(kwargs) - return AnyProperty(**kwargs) - - return _factory + return _simple_factory(AnyProperty) @pytest.fixture @@ -112,56 +133,51 @@ def string_property_factory() -> Callable[..., StringProperty]: You can pass the same params into this as the StringProperty constructor to override defaults. """ - def _factory(**kwargs): - kwargs = _common_kwargs(kwargs) - return StringProperty(**kwargs) - - return _factory + return _simple_factory(StringProperty) @pytest.fixture def int_property_factory() -> Callable[..., IntProperty]: """ - This fixture surfaces in the test as a function which manufactures StringProperties with defaults. + This fixture surfaces in the test as a function which manufactures IntProperties with defaults. - You can pass the same params into this as the StringProperty constructor to override defaults. + You can pass the same params into this as the IntProperty constructor to override defaults. """ - def _factory(**kwargs): - kwargs = _common_kwargs(kwargs) - return IntProperty(**kwargs) + return _simple_factory(IntProperty) - return _factory + +@pytest.fixture +def float_property_factory() -> Callable[..., FloatProperty]: + """ + This fixture surfaces in the test as a function which manufactures FloatProperties with defaults. + + You can pass the same params into this as the FloatProperty constructor to override defaults. + """ + + return _simple_factory(FloatProperty) @pytest.fixture def none_property_factory() -> Callable[..., NoneProperty]: """ - This fixture surfaces in the test as a function which manufactures StringProperties with defaults. + This fixture surfaces in the test as a function which manufactures NoneProperties with defaults. - You can pass the same params into this as the StringProperty constructor to override defaults. + You can pass the same params into this as the NoneProperty constructor to override defaults. """ - def _factory(**kwargs): - kwargs = _common_kwargs(kwargs) - return NoneProperty(**kwargs) - - return _factory + return _simple_factory(NoneProperty) @pytest.fixture def boolean_property_factory() -> Callable[..., BooleanProperty]: """ - This fixture surfaces in the test as a function which manufactures StringProperties with defaults. + This fixture surfaces in the test as a function which manufactures BooleanProperties with defaults. - You can pass the same params into this as the StringProperty constructor to override defaults. + You can pass the same params into this as the BooleanProperty constructor to override defaults. """ - def _factory(**kwargs): - kwargs = _common_kwargs(kwargs) - return BooleanProperty(**kwargs) - - return _factory + return _simple_factory(BooleanProperty) @pytest.fixture @@ -172,11 +188,7 @@ def date_time_property_factory() -> Callable[..., DateTimeProperty]: You can pass the same params into this as the DateTimeProperty constructor to override defaults. """ - def _factory(**kwargs): - kwargs = _common_kwargs(kwargs) - return DateTimeProperty(**kwargs) - - return _factory + return _simple_factory(DateTimeProperty) @pytest.fixture @@ -187,11 +199,7 @@ def date_property_factory() -> Callable[..., DateProperty]: You can pass the same params into this as the DateProperty constructor to override defaults. """ - def _factory(**kwargs): - kwargs = _common_kwargs(kwargs) - return DateProperty(**kwargs) - - return _factory + return _simple_factory(DateProperty) @pytest.fixture @@ -202,11 +210,7 @@ def file_property_factory() -> Callable[..., FileProperty]: You can pass the same params into this as the FileProperty constructor to override defaults. """ - def _factory(**kwargs): - kwargs = _common_kwargs(kwargs) - return FileProperty(**kwargs) - - return _factory + return _simple_factory(FileProperty) @pytest.fixture @@ -217,13 +221,7 @@ def list_property_factory(string_property_factory) -> Callable[..., ListProperty You can pass the same params into this as the ListProperty constructor to override defaults. """ - def _factory(**kwargs): - kwargs = _common_kwargs(kwargs) - if "inner_property" not in kwargs: - kwargs["inner_property"] = string_property_factory() - return ListProperty(**kwargs) - - return _factory + return _simple_factory(ListProperty, {"inner_property": string_property_factory()}) @pytest.fixture @@ -234,13 +232,9 @@ def union_property_factory(date_time_property_factory, string_property_factory) You can pass the same params into this as the UnionProperty constructor to override defaults. """ - def _factory(**kwargs): - kwargs = _common_kwargs(kwargs) - if "inner_properties" not in kwargs: - kwargs["inner_properties"] = [date_time_property_factory(), string_property_factory()] - return UnionProperty(**kwargs) - - return _factory + return _simple_factory( + UnionProperty, {"inner_properties": [date_time_property_factory(), string_property_factory()]} + ) @pytest.fixture diff --git a/tests/test_parser/test_openapi.py b/tests/test_parser/test_openapi.py index ce4f9d5e6..6eeadcd78 100644 --- a/tests/test_parser/test_openapi.py +++ b/tests/test_parser/test_openapi.py @@ -90,7 +90,7 @@ def test__add_responses_status_code_error(self, response_status_code, mocker): assert response.errors == [ ParseError( detail=f"Invalid response status code {response_status_code} (not a valid HTTP status code), " - "response will be ommitted from generated client" + "response will be omitted from generated client" ) ] response_from_data.assert_not_called() @@ -121,12 +121,12 @@ def test__add_responses_error(self, mocker): assert response.errors == [ ParseError( detail="Cannot parse response for status code 200 (some problem), " - "response will be ommitted from generated client", + "response will be omitted from generated client", data=parse_error.data, ), ParseError( detail="Cannot parse response for status code 404 (some problem), " - "response will be ommitted from generated client", + "response will be omitted from generated client", data=parse_error.data, ), ] diff --git a/tests/test_parser/test_properties/test_init.py b/tests/test_parser/test_properties/test_init.py index a30059a93..2efa27744 100644 --- a/tests/test_parser/test_properties/test_init.py +++ b/tests/test_parser/test_properties/test_init.py @@ -11,7 +11,7 @@ StringProperty, UnionProperty, ) -from openapi_python_client.parser.properties.protocol import ModelProperty +from openapi_python_client.parser.properties.protocol import ModelProperty, Value from openapi_python_client.schema import DataType from openapi_python_client.utils import ClassName, PythonIdentifier @@ -747,15 +747,13 @@ def test_no_format(self, string_property_factory, required, config): from openapi_python_client.parser.properties import property_from_data name = "some_prop" - data = oai.Schema.model_construct(type="string", default='"hello world"', pattern="abcdef") + data = oai.Schema.model_construct(type="string", default="hello world") p, _ = property_from_data( name=name, required=required, data=data, parent_name=None, config=config, schemas=Schemas() ) - assert p == string_property_factory( - name=name, required=required, default="'\\\\\"hello world\\\\\"'", pattern=data.pattern - ) + assert p == string_property_factory(name=name, required=required, default="hello world") def test_datetime_format(self, date_time_property_factory, config): from openapi_python_client.parser.properties import property_from_data @@ -768,7 +766,9 @@ def test_datetime_format(self, date_time_property_factory, config): name=name, required=required, data=data, schemas=Schemas(), config=config, parent_name="" ) - assert p == date_time_property_factory(name=name, required=required, default=f"isoparse('{data.default}')") + assert p == date_time_property_factory( + name=name, required=required, default=Value(f"isoparse('{data.default}')") + ) def test_datetime_bad_default(self, config): from openapi_python_client.parser.properties import property_from_data @@ -796,7 +796,9 @@ def test_date_format(self, date_property_factory, config): name=name, required=required, data=data, schemas=Schemas(), config=config, parent_name="" ) - assert p == date_property_factory(name=name, required=required, default=f"isoparse('{data.default}').date()") + assert p == date_property_factory( + name=name, required=required, default=Value(f"isoparse('{data.default}').date()") + ) def test_date_format_bad_default(self, config): from openapi_python_client.parser.properties import property_from_data diff --git a/tests/test_parser/test_properties/test_merge_properties.py b/tests/test_parser/test_properties/test_merge_properties.py new file mode 100644 index 000000000..78f6bf89f --- /dev/null +++ b/tests/test_parser/test_properties/test_merge_properties.py @@ -0,0 +1,237 @@ +from itertools import permutations + +from attr import evolve + +from openapi_python_client.parser.errors import PropertyError +from openapi_python_client.parser.properties.float import FloatProperty +from openapi_python_client.parser.properties.int import IntProperty +from openapi_python_client.parser.properties.merge_properties import merge_properties +from openapi_python_client.parser.properties.protocol import Value +from openapi_python_client.parser.properties.schemas import Class +from openapi_python_client.parser.properties.string import StringProperty + +MODULE_NAME = "openapi_python_client.parser.properties.merge_properties" + + +def test_merge_basic_attributes_same_type( + boolean_property_factory, + int_property_factory, + float_property_factory, + string_property_factory, + list_property_factory, + model_property_factory, +): + basic_props = [ + boolean_property_factory(default="True"), + int_property_factory(default="1"), + float_property_factory(default="1.5"), + string_property_factory(default=StringProperty.convert_value("x")), + list_property_factory(), + model_property_factory(), + ] + for basic_prop in basic_props: + with_required = evolve(basic_prop, required=True) + assert merge_properties(basic_prop, with_required) == with_required + assert merge_properties(with_required, basic_prop) == with_required + without_default = evolve(basic_prop, default=None) + assert merge_properties(basic_prop, without_default) == basic_prop + assert merge_properties(without_default, basic_prop) == basic_prop + with_desc1 = evolve(basic_prop, description="desc1") + with_desc2 = evolve(basic_prop, description="desc2") + assert merge_properties(basic_prop, with_desc1) == with_desc1 + assert merge_properties(with_desc1, basic_prop) == with_desc1 + assert merge_properties(with_desc1, with_desc2) == with_desc2 + + +def test_incompatible_types( + boolean_property_factory, + int_property_factory, + float_property_factory, + string_property_factory, + list_property_factory, + model_property_factory, +): + props = [ + boolean_property_factory(default=True), + int_property_factory(default=1), + float_property_factory(default=1.5), + string_property_factory(default="x"), + list_property_factory(), + model_property_factory(), + ] + + for prop1, prop2 in permutations(props, 2): + if {prop1.__class__, prop2.__class__} == {IntProperty, FloatProperty}: + continue # the int+float case is covered in another test + error = merge_properties(prop1, prop2) + assert isinstance(error, PropertyError), f"Expected {type(prop1)} and {type(prop2)} to be incompatible" + + +def test_merge_int_with_float(int_property_factory, float_property_factory): + int_prop = int_property_factory(description="desc1") + float_prop = float_property_factory(default=2, description="desc2") + + assert merge_properties(int_prop, float_prop) == ( + evolve(int_prop, default=Value("2"), description=float_prop.description) + ) + assert merge_properties(float_prop, int_prop) == evolve(int_prop, default=Value("2")) + + float_prop_with_non_int_default = evolve(float_prop, default=Value("2.5")) + error = merge_properties(int_prop, float_prop_with_non_int_default) + assert isinstance(error, PropertyError), "Expected invalid default to error" + assert error.detail == "Invalid int value: 2.5" + + +def test_merge_with_any( + any_property_factory, + boolean_property_factory, + int_property_factory, + float_property_factory, + string_property_factory, + model_property_factory, +): + original_desc = "description" + props = [ + boolean_property_factory(default="True", description=original_desc), + int_property_factory(default="1", description=original_desc), + float_property_factory(default="1.5", description=original_desc), + string_property_factory(default=StringProperty.convert_value("x"), description=original_desc), + model_property_factory(description=original_desc), + ] + any_prop = any_property_factory() + for prop in props: + assert merge_properties(any_prop, prop) == prop + assert merge_properties(prop, any_prop) == prop + + +def test_merge_enums(enum_property_factory, config): + enum_with_fewer_values = enum_property_factory( + description="desc1", + values={"A": "A", "B": "B"}, + value_type=str, + ) + enum_with_more_values = enum_property_factory( + example="example2", + values={"A": "A", "B": "B", "C": "C"}, + value_type=str, + ) + # Setting class_info separately because it doesn't get initialized by the constructor - we want + # to make sure the right enum class name gets used in the merged property + enum_with_fewer_values.class_info = Class.from_string(string="FewerValuesEnum", config=config) + enum_with_more_values.class_info = Class.from_string(string="MoreValuesEnum", config=config) + + assert merge_properties(enum_with_fewer_values, enum_with_more_values) == evolve( + enum_with_more_values, + values=enum_with_fewer_values.values, + class_info=enum_with_fewer_values.class_info, + description=enum_with_fewer_values.description, + ) + assert merge_properties(enum_with_more_values, enum_with_fewer_values) == evolve( + enum_with_fewer_values, + example=enum_with_more_values.example, + ) + + +def test_merge_string_with_string_enum(string_property_factory, enum_property_factory): + values = {"A": "A", "B": "B"} + string_prop = string_property_factory(default="A", description="desc1", example="example1") + enum_prop = enum_property_factory( + default="test.B", + description="desc2", + example="example2", + values=values, + value_type=str, + ) + + assert merge_properties(string_prop, enum_prop) == evolve(enum_prop, required=True) + assert merge_properties(enum_prop, string_prop) == evolve( + enum_prop, + required=True, + default="test.A", + description=string_prop.description, + example=string_prop.example, + ) + + +def test_merge_int_with_int_enum(int_property_factory, enum_property_factory): + values = {"VALUE_1": 1, "VALUE_2": 2} + int_prop = int_property_factory(default=1, description="desc1", example="example1") + enum_prop = enum_property_factory( + default="test.VALUE_1", + description="desc2", + example="example2", + values=values, + value_type=int, + ) + + assert merge_properties(int_prop, enum_prop) == evolve(enum_prop, required=True) + assert merge_properties(enum_prop, int_prop) == evolve( + enum_prop, required=True, description=int_prop.description, example=int_prop.example + ) + + +def test_merge_with_incompatible_enum( + boolean_property_factory, + int_property_factory, + float_property_factory, + string_property_factory, + enum_property_factory, + model_property_factory, +): + props = [ + boolean_property_factory(), + int_property_factory(), + float_property_factory(), + string_property_factory(), + model_property_factory(), + ] + string_enum_prop = enum_property_factory(value_type=str) + int_enum_prop = enum_property_factory(value_type=int) + for prop in props: + if not isinstance(prop, StringProperty): + assert isinstance(merge_properties(prop, string_enum_prop), PropertyError) + assert isinstance(merge_properties(string_enum_prop, prop), PropertyError) + if not isinstance(prop, IntProperty): + assert isinstance(merge_properties(prop, int_enum_prop), PropertyError) + assert isinstance(merge_properties(int_enum_prop, prop), PropertyError) + + +def test_merge_string_with_formatted_string( + date_property_factory, + date_time_property_factory, + file_property_factory, + string_property_factory, +): + string_prop = string_property_factory(description="a plain string") + string_prop_with_invalid_default = string_property_factory(default="plain string value") + formatted_props = [ + date_property_factory(description="a date"), + date_time_property_factory(description="a datetime"), + file_property_factory(description="a file"), + ] + for formatted_prop in formatted_props: + merged1 = merge_properties(string_prop, formatted_prop) + assert isinstance(merged1, formatted_prop.__class__) + assert merged1.description == formatted_prop.description + + merged2 = merge_properties(formatted_prop, string_prop) + assert isinstance(merged2, formatted_prop.__class__) + assert merged2.description == string_prop.description + + assert isinstance(merge_properties(string_prop_with_invalid_default, formatted_prop), PropertyError) + assert isinstance(merge_properties(formatted_prop, string_prop_with_invalid_default), PropertyError) + + +def test_merge_lists(int_property_factory, list_property_factory, string_property_factory): + string_prop_1 = string_property_factory(description="desc1") + string_prop_2 = string_property_factory(example="desc2") + int_prop = int_property_factory() + list_prop_1 = list_property_factory(inner_property=string_prop_1) + list_prop_2 = list_property_factory(inner_property=string_prop_2) + list_prop_3 = list_property_factory(inner_property=int_prop) + + assert merge_properties(list_prop_1, list_prop_2) == evolve( + list_prop_1, inner_property=merge_properties(string_prop_1, string_prop_2) + ) + + assert isinstance(merge_properties(list_prop_1, list_prop_3), PropertyError) diff --git a/tests/test_parser/test_properties/test_model_property.py b/tests/test_parser/test_properties/test_model_property.py index a7d7c1f23..8adc88e39 100644 --- a/tests/test_parser/test_properties/test_model_property.py +++ b/tests/test_parser/test_properties/test_model_property.py @@ -325,7 +325,7 @@ def test_process_properties_false(self, model_property_factory, config): class TestProcessProperties: def test_conflicting_properties_different_types( - self, model_property_factory, string_property_factory, date_time_property_factory, config + self, model_property_factory, string_property_factory, int_property_factory, config ): data = oai.Schema.model_construct( allOf=[oai.Reference.model_construct(ref="#/First"), oai.Reference.model_construct(ref="#/Second")] @@ -335,9 +335,7 @@ def test_conflicting_properties_different_types( "/First": model_property_factory( required_properties=[], optional_properties=[string_property_factory()] ), - "/Second": model_property_factory( - required_properties=[], optional_properties=[date_time_property_factory()] - ), + "/Second": model_property_factory(required_properties=[], optional_properties=[int_property_factory()]), } ) @@ -403,25 +401,6 @@ def test_reference_not_processed(self, model_property_factory, config): assert isinstance(result, PropertyError) - def test_conflicting_properties_same_types(self, model_property_factory, string_property_factory, config): - data = oai.Schema.model_construct( - allOf=[oai.Reference.model_construct(ref="#/First"), oai.Reference.model_construct(ref="#/Second")] - ) - schemas = Schemas( - classes_by_reference={ - "/First": model_property_factory( - required_properties=[], optional_properties=[string_property_factory(default="abc")] - ), - "/Second": model_property_factory( - required_properties=[], optional_properties=[string_property_factory()] - ), - } - ) - - result = _process_properties(data=data, schemas=schemas, class_name="", config=config, roots={"root"}) - - assert isinstance(result, PropertyError) - def test_allof_string_and_string_enum( self, model_property_factory, enum_property_factory, string_property_factory, config ): @@ -663,6 +642,33 @@ def test_conflicting_property_names(self, config): result = _process_properties(data=data, schemas=schemas, class_name="", config=config, roots={"root"}) assert isinstance(result, PropertyError) + def test_merge_inline_objects(self, model_property_factory, enum_property_factory, config): + data = oai.Schema.model_construct( + allOf=[ + oai.Schema.model_construct( + type="object", + properties={ + "prop1": oai.Schema.model_construct(type="string", default="a"), + }, + ), + oai.Schema.model_construct( + type="object", + properties={ + "prop1": oai.Schema.model_construct(type="string", description="desc"), + }, + ), + ] + ) + schemas = Schemas() + + result = _process_properties(data=data, schemas=schemas, class_name="", config=config, roots={"root"}) + assert not isinstance(result, PropertyError) + assert len(result.optional_props) == 1 + prop1 = result.optional_props[0] + assert isinstance(prop1, StringProperty) + assert prop1.description == "desc" + assert prop1.default == StringProperty.convert_value("a") + class TestProcessModel: def test_process_model_error(self, mocker, model_property_factory, config): diff --git a/tests/test_parser/test_properties/test_union.py b/tests/test_parser/test_properties/test_union.py index d8a5d762c..fa8d954d0 100644 --- a/tests/test_parser/test_properties/test_union.py +++ b/tests/test_parser/test_properties/test_union.py @@ -1,6 +1,7 @@ import openapi_python_client.schema as oai from openapi_python_client.parser.errors import ParseError, PropertyError from openapi_python_client.parser.properties import Schemas, UnionProperty +from openapi_python_client.parser.properties.protocol import Value from openapi_python_client.schema import DataType, ParameterLocation @@ -19,7 +20,7 @@ def test_property_from_data_union(union_property_factory, date_time_property_fac name=name, required=required, inner_properties=[ - string_property_factory(name=f"{name}_type_0", default="'a'"), + string_property_factory(name=f"{name}_type_0", default=Value("'a'")), date_time_property_factory(name=f"{name}_type_1"), ], )