From fffeaeaeddc8111b59239c4bf16501955bbd6b64 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 12 Sep 2024 17:41:40 -0700 Subject: [PATCH 1/5] fix class generation for nullable enums --- end_to_end_tests/baseline_openapi_3.0.json | 13 ++++- end_to_end_tests/baseline_openapi_3.1.yaml | 12 ++++- .../my_test_api_client/models/__init__.py | 2 + .../my_test_api_client/models/a_model.py | 52 ++++++++++++++++++ .../models/a_model_nullable_enum_inline.py | 9 ++++ .../parser/properties/enum_property.py | 53 ++++++++++++------- .../test_properties/test_enum_property.py | 21 ++++++++ .../test_parser/test_properties/test_init.py | 19 +++++-- 8 files changed, 155 insertions(+), 26 deletions(-) create mode 100644 end_to_end_tests/golden-record/my_test_api_client/models/a_model_nullable_enum_inline.py diff --git a/end_to_end_tests/baseline_openapi_3.0.json b/end_to_end_tests/baseline_openapi_3.0.json index f34f27366..8a2306ada 100644 --- a/end_to_end_tests/baseline_openapi_3.0.json +++ b/end_to_end_tests/baseline_openapi_3.0.json @@ -1660,7 +1660,9 @@ "model", "nullable_model", "one_of_models", - "nullable_one_of_models" + "nullable_one_of_models", + "nullable_enum_as_ref", + "nullable_enum_inline" ], "type": "object", "properties": { @@ -1830,6 +1832,14 @@ } ], "nullable": true + }, + "nullable_enum_as_ref": { + "$ref": "#/components/schemas/AnEnumWithNull" + }, + "nullable_enum_inline": { + "type": "string", + "enum": ["FIRST_VALUE", "SECOND_VALUE", null], + "nullable": true } }, "description": "A Model for testing all the ways custom objects can be used ", @@ -1850,6 +1860,7 @@ "SECOND_VALUE", null ], + "nullable": true, "description": "For testing Enums with mixed string / null values " }, "AnEnumWithOnlyNull": { diff --git a/end_to_end_tests/baseline_openapi_3.1.yaml b/end_to_end_tests/baseline_openapi_3.1.yaml index 13d267b77..a8910578b 100644 --- a/end_to_end_tests/baseline_openapi_3.1.yaml +++ b/end_to_end_tests/baseline_openapi_3.1.yaml @@ -1650,7 +1650,9 @@ info: "model", "nullable_model", "one_of_models", - "nullable_one_of_models" + "nullable_one_of_models", + "nullable_enum_as_ref", + "nullable_enum_inline" ], "type": "object", "properties": { @@ -1840,6 +1842,13 @@ info: "$ref": "#/components/schemas/ModelWithUnionProperty" } ] + }, + "nullable_enum_as_ref": { + "$ref": "#/components/schemas/AnEnumWithNull" + }, + "nullable_enum_inline": { + "type": ["string", "null"], + "enum": ["FIRST_VALUE", "SECOND_VALUE", null] } }, "description": "A Model for testing all the ways custom objects can be used ", @@ -1855,6 +1864,7 @@ info: }, "AnEnumWithNull": { "title": "AnEnumWithNull", + "type": ["string", "null"], "enum": [ "FIRST_VALUE", "SECOND_VALUE", 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 7435983e3..c0ad148c2 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 @@ -4,6 +4,7 @@ from .a_discriminated_union_type_2 import ADiscriminatedUnionType2 from .a_form_data import AFormData from .a_model import AModel +from .a_model_nullable_enum_inline import AModelNullableEnumInline from .a_model_with_properties_reference_that_are_not_object import AModelWithPropertiesReferenceThatAreNotObject from .all_of_has_properties_but_no_type import AllOfHasPropertiesButNoType from .all_of_has_properties_but_no_type_type_enum import AllOfHasPropertiesButNoTypeTypeEnum @@ -91,6 +92,7 @@ "AllOfSubModel", "AllOfSubModelTypeEnum", "AModel", + "AModelNullableEnumInline", "AModelWithPropertiesReferenceThatAreNotObject", "AnAllOfEnum", "AnArrayWithACircularRefInItemsObjectAdditionalPropertiesAItem", diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/a_model.py b/end_to_end_tests/golden-record/my_test_api_client/models/a_model.py index d14160bf8..0bc4be405 100644 --- a/end_to_end_tests/golden-record/my_test_api_client/models/a_model.py +++ b/end_to_end_tests/golden-record/my_test_api_client/models/a_model.py @@ -4,8 +4,10 @@ from attrs import define as _attrs_define from dateutil.parser import isoparse +from ..models.a_model_nullable_enum_inline import AModelNullableEnumInline from ..models.an_all_of_enum import AnAllOfEnum from ..models.an_enum import AnEnum +from ..models.an_enum_with_null import AnEnumWithNull from ..models.different_enum import DifferentEnum from ..types import UNSET, Unset @@ -33,6 +35,8 @@ class AModel: nullable_one_of_models (Union['FreeFormModel', 'ModelWithUnionProperty', None]): model (ModelWithUnionProperty): nullable_model (Union['ModelWithUnionProperty', None]): + nullable_enum_as_ref (Union[AnEnumWithNull, None]): For testing Enums with mixed string / null values + nullable_enum_inline (Union[AModelNullableEnumInline, None]): any_value (Union[Unset, Any]): an_optional_allof_enum (Union[Unset, AnAllOfEnum]): nested_list_of_enums (Union[Unset, List[List[DifferentEnum]]]): @@ -57,6 +61,8 @@ class AModel: nullable_one_of_models: Union["FreeFormModel", "ModelWithUnionProperty", None] model: "ModelWithUnionProperty" nullable_model: Union["ModelWithUnionProperty", None] + nullable_enum_as_ref: Union[AnEnumWithNull, None] + nullable_enum_inline: Union[AModelNullableEnumInline, None] an_allof_enum_with_overridden_default: AnAllOfEnum = AnAllOfEnum.OVERRIDDEN_DEFAULT any_value: Union[Unset, Any] = UNSET an_optional_allof_enum: Union[Unset, AnAllOfEnum] = UNSET @@ -122,6 +128,18 @@ def to_dict(self) -> Dict[str, Any]: else: nullable_model = self.nullable_model + nullable_enum_as_ref: Union[None, str] + if isinstance(self.nullable_enum_as_ref, AnEnumWithNull): + nullable_enum_as_ref = self.nullable_enum_as_ref.value + else: + nullable_enum_as_ref = self.nullable_enum_as_ref + + nullable_enum_inline: Union[None, str] + if isinstance(self.nullable_enum_inline, AModelNullableEnumInline): + nullable_enum_inline = self.nullable_enum_inline.value + else: + nullable_enum_inline = self.nullable_enum_inline + any_value = self.any_value an_optional_allof_enum: Union[Unset, str] = UNSET @@ -199,6 +217,8 @@ def to_dict(self) -> Dict[str, Any]: "nullable_one_of_models": nullable_one_of_models, "model": model, "nullable_model": nullable_model, + "nullable_enum_as_ref": nullable_enum_as_ref, + "nullable_enum_inline": nullable_enum_inline, } ) if any_value is not UNSET: @@ -342,6 +362,36 @@ def _parse_nullable_model(data: object) -> Union["ModelWithUnionProperty", None] nullable_model = _parse_nullable_model(d.pop("nullable_model")) + def _parse_nullable_enum_as_ref(data: object) -> Union[AnEnumWithNull, None]: + if data is None: + return data + try: + if not isinstance(data, str): + raise TypeError() + componentsschemas_an_enum_with_null = AnEnumWithNull(data) + + return componentsschemas_an_enum_with_null + except: # noqa: E722 + pass + return cast(Union[AnEnumWithNull, None], data) + + nullable_enum_as_ref = _parse_nullable_enum_as_ref(d.pop("nullable_enum_as_ref")) + + def _parse_nullable_enum_inline(data: object) -> Union[AModelNullableEnumInline, None]: + if data is None: + return data + try: + if not isinstance(data, str): + raise TypeError() + nullable_enum_inline = AModelNullableEnumInline(data) + + return nullable_enum_inline + except: # noqa: E722 + pass + return cast(Union[AModelNullableEnumInline, None], data) + + nullable_enum_inline = _parse_nullable_enum_inline(d.pop("nullable_enum_inline")) + any_value = d.pop("any_value", UNSET) _an_optional_allof_enum = d.pop("an_optional_allof_enum", UNSET) @@ -469,6 +519,8 @@ def _parse_not_required_nullable_model(data: object) -> Union["ModelWithUnionPro nullable_one_of_models=nullable_one_of_models, model=model, nullable_model=nullable_model, + nullable_enum_as_ref=nullable_enum_as_ref, + nullable_enum_inline=nullable_enum_inline, any_value=any_value, an_optional_allof_enum=an_optional_allof_enum, nested_list_of_enums=nested_list_of_enums, diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/a_model_nullable_enum_inline.py b/end_to_end_tests/golden-record/my_test_api_client/models/a_model_nullable_enum_inline.py new file mode 100644 index 000000000..21634624e --- /dev/null +++ b/end_to_end_tests/golden-record/my_test_api_client/models/a_model_nullable_enum_inline.py @@ -0,0 +1,9 @@ +from enum import Enum + + +class AModelNullableEnumInline(str, Enum): + FIRST_VALUE = "FIRST_VALUE" + SECOND_VALUE = "SECOND_VALUE" + + def __str__(self) -> str: + return str(self.value) diff --git a/openapi_python_client/parser/properties/enum_property.py b/openapi_python_client/parser/properties/enum_property.py index 0f0db0d61..98b611eca 100644 --- a/openapi_python_client/parser/properties/enum_property.py +++ b/openapi_python_client/parser/properties/enum_property.py @@ -9,7 +9,6 @@ from ... import Config, utils from ... import schema as oai -from ...schema import DataType from ..errors import PropertyError from .none import NoneProperty from .protocol import PropertyProtocol, Value @@ -43,7 +42,7 @@ class EnumProperty(PropertyProtocol): } @classmethod - def build( # noqa: PLR0911 + def build( cls, *, data: oai.Schema, @@ -75,9 +74,10 @@ def build( # noqa: PLR0911 # So instead, if null is a possible value, make the property nullable. # Mypy is not smart enough to know that the type is right though unchecked_value_list = [value for value in enum if value is not None] # type: ignore + allow_null = len(unchecked_value_list) < len(enum) # It's legal to have an enum that only contains null as a value, we don't bother constructing an enum for that - if len(unchecked_value_list) == 0: + if len(unchecked_value_list) == 0 and allow_null: return ( NoneProperty.build( name=name, @@ -102,21 +102,6 @@ def build( # noqa: PLR0911 Union[List[int], List[str]], unchecked_value_list ) # We checked this with all the value_types stuff - if len(value_list) < len(enum): # Only one of the values was None, that becomes a union - data.oneOf = [ - oai.Schema(type=DataType.NULL), - data.model_copy(update={"enum": value_list, "default": data.default}), - ] - data.enum = None - return UnionProperty.build( - data=data, - name=name, - required=required, - schemas=schemas, - parent_name=parent_name, - config=config, - ) - class_name = data.title or name if parent_name: class_name = f"{utils.pascal_case(parent_name)}{utils.pascal_case(class_name)}" @@ -150,8 +135,38 @@ def build( # noqa: PLR0911 return checked_default, schemas prop = evolve(prop, default=checked_default) + # Now, if one of the values was None, wrap the type we just made in a union with None. We're + # constructing the UnionProperty directly instead of using UnionProperty.build() because we + # do *not* want the usual union behavior of creating ThingType1, ThingType2, etc. + returned_prop: EnumProperty | UnionProperty | PropertyError + if allow_null: + none_prop = NoneProperty.build( + name=name, + required=required, + default=None, + python_name=prop.python_name, + description=None, + example=None, + ) + assert not isinstance(none_prop, PropertyError) + union_prop = UnionProperty( + name=name, + required=required, + default=checked_default, + python_name=prop.python_name, + description=data.description, + example=data.example, + inner_properties=[ + none_prop, + prop, + ], + ) + returned_prop = union_prop + else: + returned_prop = prop + schemas = evolve(schemas, classes_by_name={**schemas.classes_by_name, class_info.name: prop}) - return prop, schemas + return returned_prop, schemas def convert_value(self, value: Any) -> Value | PropertyError | None: if value is None or isinstance(value, Value): diff --git a/tests/test_parser/test_properties/test_enum_property.py b/tests/test_parser/test_properties/test_enum_property.py index 704f48b3b..b7d9fe9ec 100644 --- a/tests/test_parser/test_properties/test_enum_property.py +++ b/tests/test_parser/test_properties/test_enum_property.py @@ -1,6 +1,8 @@ import openapi_python_client.schema as oai from openapi_python_client.parser.errors import PropertyError from openapi_python_client.parser.properties import EnumProperty, Schemas +from openapi_python_client.parser.properties.none import NoneProperty +from openapi_python_client.parser.properties.union import UnionProperty def test_conflict(config): @@ -66,3 +68,22 @@ def test_unsupported_type(config): ) assert isinstance(err, PropertyError) + + +def test_nullable_enum(config): + data = oai.Schema( + type="string", + enum=["a", "b", None], + nullable=True, + ) + schemas = Schemas() + + p, _ = EnumProperty.build( + data=data, name="prop1", required=True, schemas=schemas, parent_name="parent", config=config + ) + + assert isinstance(p, UnionProperty) + assert len(p.inner_properties) == 2 # noqa: PLR2004 + assert isinstance(p.inner_properties[0], NoneProperty) + assert isinstance(p.inner_properties[1], EnumProperty) + assert p.inner_properties[1].class_info.name == "ParentProp1" diff --git a/tests/test_parser/test_properties/test_init.py b/tests/test_parser/test_properties/test_init.py index 3290dcd39..161c41640 100644 --- a/tests/test_parser/test_properties/test_init.py +++ b/tests/test_parser/test_properties/test_init.py @@ -403,14 +403,23 @@ def test_property_from_data_str_enum(self, enum_property_factory, config): "ParentAnEnum": prop, } + @pytest.mark.parametrize( + "desc,extra_props", + [ + ("3_1_implicit_type", {}), + ("3_1_explicit_type", {"type": ["string", "null"]}), + ("3_0_implicit_type", {"nullable": True}), + ("3_0_explicit_type", {"type": "string", "nullable": True}), + ], + ) def test_property_from_data_str_enum_with_null( - self, enum_property_factory, union_property_factory, none_property_factory, config + self, desc, extra_props, enum_property_factory, union_property_factory, none_property_factory, config ): from openapi_python_client.parser.properties import Class, Schemas, property_from_data from openapi_python_client.schema import Schema existing = enum_property_factory() - data = Schema(title="AnEnum", enum=["A", "B", "C", None], default="B") + data = Schema(title="AnEnum", enum=["A", "B", "C", None], default="B", **extra_props) name = "my_enum" required = True @@ -423,16 +432,16 @@ def test_property_from_data_str_enum_with_null( # None / null is removed from enum, and property is now nullable assert isinstance(prop, UnionProperty), "Enums with None should be converted to UnionProperties" enum_prop = enum_property_factory( - name="my_enum_type_1", + name=name, required=required, values={"A": "A", "B": "B", "C": "C"}, class_info=Class(name="ParentAnEnum", module_name="parent_an_enum"), value_type=str, default="ParentAnEnum.B", ) - none_property = none_property_factory(name="my_enum_type_0", required=required) + none_property = none_property_factory(name=name, required=required) assert prop == union_property_factory( - name=name, default="ParentAnEnum.B", inner_properties=[none_property, enum_prop] + name=name, default="ParentAnEnum.B", inner_properties=[none_property, enum_prop], required=required ) assert schemas != new_schemas, "Provided Schemas was mutated" assert new_schemas.classes_by_name == { From f741fec46a74950e14bb1fbee4288075e53004ab Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Thu, 12 Sep 2024 18:32:37 -0700 Subject: [PATCH 2/5] changeset --- .changeset/nullable_enum_fix.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/nullable_enum_fix.md diff --git a/.changeset/nullable_enum_fix.md b/.changeset/nullable_enum_fix.md new file mode 100644 index 000000000..e47425399 --- /dev/null +++ b/.changeset/nullable_enum_fix.md @@ -0,0 +1,7 @@ +--- +default: patch +--- + +# Fix class generation for nullable enums in OpenAPI 3.0 + +Fixed issue #1120, where enum types with `nullable: true` did not work correctly if `type` was also specified. From 500f363938308bec46c8363f921790d05056dcd2 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 13 Sep 2024 19:45:56 -0700 Subject: [PATCH 3/5] rework union type logic to preserve original type name for class when possible --- .changeset/nullable_enum_fix.md | 7 -- .changeset/union_fixes.md | 7 ++ .../my_test_api_client/models/__init__.py | 10 +- .../my_test_api_client/models/a_model.py | 8 +- .../body_upload_file_tests_upload_post.py | 4 +- .../my_test_api_client/models/extended.py | 8 +- .../models/model_with_any_json_properties.py | 30 +++--- ...ny_json_properties_additional_property.py} | 10 +- ...ions_simple_before_complex_response_200.py | 26 ++--- ...ns_simple_before_complex_response_200a.py} | 10 +- .../parser/properties/enum_property.py | 54 ++++------- .../parser/properties/has_named_class.py | 8 ++ .../parser/properties/model_property.py | 4 +- .../parser/properties/union.py | 54 ++++++++--- .../test_parser/test_properties/test_init.py | 32 +++++- .../test_parser/test_properties/test_union.py | 97 +++++++++++++++++++ 16 files changed, 257 insertions(+), 112 deletions(-) delete mode 100644 .changeset/nullable_enum_fix.md create mode 100644 .changeset/union_fixes.md rename end_to_end_tests/golden-record/my_test_api_client/models/{model_with_any_json_properties_additional_property_type_0.py => model_with_any_json_properties_additional_property.py} (82%) rename end_to_end_tests/golden-record/my_test_api_client/models/{post_responses_unions_simple_before_complex_response_200a_type_1.py => post_responses_unions_simple_before_complex_response_200a.py} (89%) create mode 100644 openapi_python_client/parser/properties/has_named_class.py diff --git a/.changeset/nullable_enum_fix.md b/.changeset/nullable_enum_fix.md deleted file mode 100644 index e47425399..000000000 --- a/.changeset/nullable_enum_fix.md +++ /dev/null @@ -1,7 +0,0 @@ ---- -default: patch ---- - -# Fix class generation for nullable enums in OpenAPI 3.0 - -Fixed issue #1120, where enum types with `nullable: true` did not work correctly if `type` was also specified. diff --git a/.changeset/union_fixes.md b/.changeset/union_fixes.md new file mode 100644 index 000000000..099ef37eb --- /dev/null +++ b/.changeset/union_fixes.md @@ -0,0 +1,7 @@ +--- +default: patch +--- + +# Fix class generation for some union types + +Fixed issue #1120, where certain combinations of types-- such as a `oneOf` between a model or an enum and null, or the OpenAPI 3.0 equivalent of using `nullable: true`-- could cause unnecessary suffixes like "Type0" to be added to the class name, and/or could cause extra copies of the class to be generated. 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 4c46cf9fa..0879c1e47 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 @@ -53,7 +53,7 @@ ) from .model_with_additional_properties_refed import ModelWithAdditionalPropertiesRefed from .model_with_any_json_properties import ModelWithAnyJsonProperties -from .model_with_any_json_properties_additional_property_type_0 import ModelWithAnyJsonPropertiesAdditionalPropertyType0 +from .model_with_any_json_properties_additional_property import ModelWithAnyJsonPropertiesAdditionalProperty from .model_with_backslash_in_description import ModelWithBackslashInDescription from .model_with_circular_ref_a import ModelWithCircularRefA from .model_with_circular_ref_b import ModelWithCircularRefB @@ -81,8 +81,8 @@ from .post_naming_property_conflict_with_import_body import PostNamingPropertyConflictWithImportBody from .post_naming_property_conflict_with_import_response_200 import PostNamingPropertyConflictWithImportResponse200 from .post_responses_unions_simple_before_complex_response_200 import PostResponsesUnionsSimpleBeforeComplexResponse200 -from .post_responses_unions_simple_before_complex_response_200a_type_1 import ( - PostResponsesUnionsSimpleBeforeComplexResponse200AType1, +from .post_responses_unions_simple_before_complex_response_200a import ( + PostResponsesUnionsSimpleBeforeComplexResponse200A, ) from .test_inline_objects_body import TestInlineObjectsBody from .test_inline_objects_response_200 import TestInlineObjectsResponse200 @@ -134,7 +134,7 @@ "ModelWithAdditionalPropertiesInlinedAdditionalProperty", "ModelWithAdditionalPropertiesRefed", "ModelWithAnyJsonProperties", - "ModelWithAnyJsonPropertiesAdditionalPropertyType0", + "ModelWithAnyJsonPropertiesAdditionalProperty", "ModelWithBackslashInDescription", "ModelWithCircularRefA", "ModelWithCircularRefB", @@ -162,7 +162,7 @@ "PostNamingPropertyConflictWithImportBody", "PostNamingPropertyConflictWithImportResponse200", "PostResponsesUnionsSimpleBeforeComplexResponse200", - "PostResponsesUnionsSimpleBeforeComplexResponse200AType1", + "PostResponsesUnionsSimpleBeforeComplexResponse200A", "TestInlineObjectsBody", "TestInlineObjectsResponse200", "ValidationError", diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/a_model.py b/end_to_end_tests/golden-record/my_test_api_client/models/a_model.py index d54720758..2960c50df 100644 --- a/end_to_end_tests/golden-record/my_test_api_client/models/a_model.py +++ b/end_to_end_tests/golden-record/my_test_api_client/models/a_model.py @@ -353,9 +353,9 @@ def _parse_nullable_model(data: object) -> Union["ModelWithUnionProperty", None] try: if not isinstance(data, dict): raise TypeError() - nullable_model_type_1 = ModelWithUnionProperty.from_dict(data) + nullable_model = ModelWithUnionProperty.from_dict(data) - return nullable_model_type_1 + return nullable_model except: # noqa: E722 pass return cast(Union["ModelWithUnionProperty", None], data) @@ -498,9 +498,9 @@ def _parse_not_required_nullable_model(data: object) -> Union["ModelWithUnionPro try: if not isinstance(data, dict): raise TypeError() - not_required_nullable_model_type_1 = ModelWithUnionProperty.from_dict(data) + not_required_nullable_model = ModelWithUnionProperty.from_dict(data) - return not_required_nullable_model_type_1 + return not_required_nullable_model except: # noqa: E722 pass return cast(Union["ModelWithUnionProperty", None, Unset], data) diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/body_upload_file_tests_upload_post.py b/end_to_end_tests/golden-record/my_test_api_client/models/body_upload_file_tests_upload_post.py index d7fbbf835..9c1a1e7e6 100644 --- a/end_to_end_tests/golden-record/my_test_api_client/models/body_upload_file_tests_upload_post.py +++ b/end_to_end_tests/golden-record/my_test_api_client/models/body_upload_file_tests_upload_post.py @@ -304,9 +304,9 @@ def _parse_some_nullable_object(data: object) -> Union["BodyUploadFileTestsUploa try: if not isinstance(data, dict): raise TypeError() - some_nullable_object_type_0 = BodyUploadFileTestsUploadPostSomeNullableObject.from_dict(data) + some_nullable_object = BodyUploadFileTestsUploadPostSomeNullableObject.from_dict(data) - return some_nullable_object_type_0 + return some_nullable_object except: # noqa: E722 pass return cast(Union["BodyUploadFileTestsUploadPostSomeNullableObject", None], data) diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/extended.py b/end_to_end_tests/golden-record/my_test_api_client/models/extended.py index f8bc76a1a..985249a8e 100644 --- a/end_to_end_tests/golden-record/my_test_api_client/models/extended.py +++ b/end_to_end_tests/golden-record/my_test_api_client/models/extended.py @@ -361,9 +361,9 @@ def _parse_nullable_model(data: object) -> Union["ModelWithUnionProperty", None] try: if not isinstance(data, dict): raise TypeError() - nullable_model_type_1 = ModelWithUnionProperty.from_dict(data) + nullable_model = ModelWithUnionProperty.from_dict(data) - return nullable_model_type_1 + return nullable_model except: # noqa: E722 pass return cast(Union["ModelWithUnionProperty", None], data) @@ -506,9 +506,9 @@ def _parse_not_required_nullable_model(data: object) -> Union["ModelWithUnionPro try: if not isinstance(data, dict): raise TypeError() - not_required_nullable_model_type_1 = ModelWithUnionProperty.from_dict(data) + not_required_nullable_model = ModelWithUnionProperty.from_dict(data) - return not_required_nullable_model_type_1 + return not_required_nullable_model except: # noqa: E722 pass return cast(Union["ModelWithUnionProperty", None, Unset], data) diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/model_with_any_json_properties.py b/end_to_end_tests/golden-record/my_test_api_client/models/model_with_any_json_properties.py index 6e669914a..89498e3bd 100644 --- a/end_to_end_tests/golden-record/my_test_api_client/models/model_with_any_json_properties.py +++ b/end_to_end_tests/golden-record/my_test_api_client/models/model_with_any_json_properties.py @@ -4,9 +4,7 @@ from attrs import field as _attrs_field if TYPE_CHECKING: - from ..models.model_with_any_json_properties_additional_property_type_0 import ( - ModelWithAnyJsonPropertiesAdditionalPropertyType0, - ) + from ..models.model_with_any_json_properties_additional_property import ModelWithAnyJsonPropertiesAdditionalProperty T = TypeVar("T", bound="ModelWithAnyJsonProperties") @@ -17,17 +15,17 @@ class ModelWithAnyJsonProperties: """ """ additional_properties: Dict[ - str, Union["ModelWithAnyJsonPropertiesAdditionalPropertyType0", List[str], bool, float, int, str] + str, Union["ModelWithAnyJsonPropertiesAdditionalProperty", List[str], bool, float, int, str] ] = _attrs_field(init=False, factory=dict) def to_dict(self) -> Dict[str, Any]: - from ..models.model_with_any_json_properties_additional_property_type_0 import ( - ModelWithAnyJsonPropertiesAdditionalPropertyType0, + from ..models.model_with_any_json_properties_additional_property import ( + ModelWithAnyJsonPropertiesAdditionalProperty, ) field_dict: Dict[str, Any] = {} for prop_name, prop in self.additional_properties.items(): - if isinstance(prop, ModelWithAnyJsonPropertiesAdditionalPropertyType0): + if isinstance(prop, ModelWithAnyJsonPropertiesAdditionalProperty): field_dict[prop_name] = prop.to_dict() elif isinstance(prop, list): field_dict[prop_name] = prop @@ -39,8 +37,8 @@ def to_dict(self) -> Dict[str, Any]: @classmethod def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T: - from ..models.model_with_any_json_properties_additional_property_type_0 import ( - ModelWithAnyJsonPropertiesAdditionalPropertyType0, + from ..models.model_with_any_json_properties_additional_property import ( + ModelWithAnyJsonPropertiesAdditionalProperty, ) d = src_dict.copy() @@ -51,13 +49,13 @@ def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T: def _parse_additional_property( data: object, - ) -> Union["ModelWithAnyJsonPropertiesAdditionalPropertyType0", List[str], bool, float, int, str]: + ) -> Union["ModelWithAnyJsonPropertiesAdditionalProperty", List[str], bool, float, int, str]: try: if not isinstance(data, dict): raise TypeError() - additional_property_type_0 = ModelWithAnyJsonPropertiesAdditionalPropertyType0.from_dict(data) + additional_property = ModelWithAnyJsonPropertiesAdditionalProperty.from_dict(data) - return additional_property_type_0 + return additional_property except: # noqa: E722 pass try: @@ -69,7 +67,7 @@ def _parse_additional_property( except: # noqa: E722 pass return cast( - Union["ModelWithAnyJsonPropertiesAdditionalPropertyType0", List[str], bool, float, int, str], data + Union["ModelWithAnyJsonPropertiesAdditionalProperty", List[str], bool, float, int, str], data ) additional_property = _parse_additional_property(prop_dict) @@ -85,13 +83,11 @@ def additional_keys(self) -> List[str]: def __getitem__( self, key: str - ) -> Union["ModelWithAnyJsonPropertiesAdditionalPropertyType0", List[str], bool, float, int, str]: + ) -> Union["ModelWithAnyJsonPropertiesAdditionalProperty", List[str], bool, float, int, str]: return self.additional_properties[key] def __setitem__( - self, - key: str, - value: Union["ModelWithAnyJsonPropertiesAdditionalPropertyType0", List[str], bool, float, int, str], + self, key: str, value: Union["ModelWithAnyJsonPropertiesAdditionalProperty", List[str], bool, float, int, str] ) -> None: self.additional_properties[key] = value diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/model_with_any_json_properties_additional_property_type_0.py b/end_to_end_tests/golden-record/my_test_api_client/models/model_with_any_json_properties_additional_property.py similarity index 82% rename from end_to_end_tests/golden-record/my_test_api_client/models/model_with_any_json_properties_additional_property_type_0.py rename to end_to_end_tests/golden-record/my_test_api_client/models/model_with_any_json_properties_additional_property.py index 6ae70905e..cd6cc851a 100644 --- a/end_to_end_tests/golden-record/my_test_api_client/models/model_with_any_json_properties_additional_property_type_0.py +++ b/end_to_end_tests/golden-record/my_test_api_client/models/model_with_any_json_properties_additional_property.py @@ -3,11 +3,11 @@ from attrs import define as _attrs_define from attrs import field as _attrs_field -T = TypeVar("T", bound="ModelWithAnyJsonPropertiesAdditionalPropertyType0") +T = TypeVar("T", bound="ModelWithAnyJsonPropertiesAdditionalProperty") @_attrs_define -class ModelWithAnyJsonPropertiesAdditionalPropertyType0: +class ModelWithAnyJsonPropertiesAdditionalProperty: """ """ additional_properties: Dict[str, str] = _attrs_field(init=False, factory=dict) @@ -21,10 +21,10 @@ def to_dict(self) -> Dict[str, Any]: @classmethod def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T: d = src_dict.copy() - model_with_any_json_properties_additional_property_type_0 = cls() + model_with_any_json_properties_additional_property = cls() - model_with_any_json_properties_additional_property_type_0.additional_properties = d - return model_with_any_json_properties_additional_property_type_0 + model_with_any_json_properties_additional_property.additional_properties = d + return model_with_any_json_properties_additional_property @property def additional_keys(self) -> List[str]: diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/post_responses_unions_simple_before_complex_response_200.py b/end_to_end_tests/golden-record/my_test_api_client/models/post_responses_unions_simple_before_complex_response_200.py index 0b6a29243..3c07edb03 100644 --- a/end_to_end_tests/golden-record/my_test_api_client/models/post_responses_unions_simple_before_complex_response_200.py +++ b/end_to_end_tests/golden-record/my_test_api_client/models/post_responses_unions_simple_before_complex_response_200.py @@ -4,8 +4,8 @@ from attrs import field as _attrs_field if TYPE_CHECKING: - from ..models.post_responses_unions_simple_before_complex_response_200a_type_1 import ( - PostResponsesUnionsSimpleBeforeComplexResponse200AType1, + from ..models.post_responses_unions_simple_before_complex_response_200a import ( + PostResponsesUnionsSimpleBeforeComplexResponse200A, ) @@ -16,19 +16,19 @@ class PostResponsesUnionsSimpleBeforeComplexResponse200: """ Attributes: - a (Union['PostResponsesUnionsSimpleBeforeComplexResponse200AType1', str]): + a (Union['PostResponsesUnionsSimpleBeforeComplexResponse200A', str]): """ - a: Union["PostResponsesUnionsSimpleBeforeComplexResponse200AType1", str] + a: Union["PostResponsesUnionsSimpleBeforeComplexResponse200A", str] additional_properties: Dict[str, Any] = _attrs_field(init=False, factory=dict) def to_dict(self) -> Dict[str, Any]: - from ..models.post_responses_unions_simple_before_complex_response_200a_type_1 import ( - PostResponsesUnionsSimpleBeforeComplexResponse200AType1, + from ..models.post_responses_unions_simple_before_complex_response_200a import ( + PostResponsesUnionsSimpleBeforeComplexResponse200A, ) a: Union[Dict[str, Any], str] - if isinstance(self.a, PostResponsesUnionsSimpleBeforeComplexResponse200AType1): + if isinstance(self.a, PostResponsesUnionsSimpleBeforeComplexResponse200A): a = self.a.to_dict() else: a = self.a @@ -45,22 +45,22 @@ def to_dict(self) -> Dict[str, Any]: @classmethod def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T: - from ..models.post_responses_unions_simple_before_complex_response_200a_type_1 import ( - PostResponsesUnionsSimpleBeforeComplexResponse200AType1, + from ..models.post_responses_unions_simple_before_complex_response_200a import ( + PostResponsesUnionsSimpleBeforeComplexResponse200A, ) d = src_dict.copy() - def _parse_a(data: object) -> Union["PostResponsesUnionsSimpleBeforeComplexResponse200AType1", str]: + def _parse_a(data: object) -> Union["PostResponsesUnionsSimpleBeforeComplexResponse200A", str]: try: if not isinstance(data, dict): raise TypeError() - a_type_1 = PostResponsesUnionsSimpleBeforeComplexResponse200AType1.from_dict(data) + a = PostResponsesUnionsSimpleBeforeComplexResponse200A.from_dict(data) - return a_type_1 + return a except: # noqa: E722 pass - return cast(Union["PostResponsesUnionsSimpleBeforeComplexResponse200AType1", str], data) + return cast(Union["PostResponsesUnionsSimpleBeforeComplexResponse200A", str], data) a = _parse_a(d.pop("a")) diff --git a/end_to_end_tests/golden-record/my_test_api_client/models/post_responses_unions_simple_before_complex_response_200a_type_1.py b/end_to_end_tests/golden-record/my_test_api_client/models/post_responses_unions_simple_before_complex_response_200a.py similarity index 89% rename from end_to_end_tests/golden-record/my_test_api_client/models/post_responses_unions_simple_before_complex_response_200a_type_1.py rename to end_to_end_tests/golden-record/my_test_api_client/models/post_responses_unions_simple_before_complex_response_200a.py index 601d17cf8..a59c0962a 100644 --- a/end_to_end_tests/golden-record/my_test_api_client/models/post_responses_unions_simple_before_complex_response_200a_type_1.py +++ b/end_to_end_tests/golden-record/my_test_api_client/models/post_responses_unions_simple_before_complex_response_200a.py @@ -3,11 +3,11 @@ from attrs import define as _attrs_define from attrs import field as _attrs_field -T = TypeVar("T", bound="PostResponsesUnionsSimpleBeforeComplexResponse200AType1") +T = TypeVar("T", bound="PostResponsesUnionsSimpleBeforeComplexResponse200A") @_attrs_define -class PostResponsesUnionsSimpleBeforeComplexResponse200AType1: +class PostResponsesUnionsSimpleBeforeComplexResponse200A: """ """ additional_properties: Dict[str, Any] = _attrs_field(init=False, factory=dict) @@ -21,10 +21,10 @@ def to_dict(self) -> Dict[str, Any]: @classmethod def from_dict(cls: Type[T], src_dict: Dict[str, Any]) -> T: d = src_dict.copy() - post_responses_unions_simple_before_complex_response_200a_type_1 = cls() + post_responses_unions_simple_before_complex_response_200a = cls() - post_responses_unions_simple_before_complex_response_200a_type_1.additional_properties = d - return post_responses_unions_simple_before_complex_response_200a_type_1 + post_responses_unions_simple_before_complex_response_200a.additional_properties = d + return post_responses_unions_simple_before_complex_response_200a @property def additional_keys(self) -> List[str]: diff --git a/openapi_python_client/parser/properties/enum_property.py b/openapi_python_client/parser/properties/enum_property.py index 01eefb43b..02e0f0ca2 100644 --- a/openapi_python_client/parser/properties/enum_property.py +++ b/openapi_python_client/parser/properties/enum_property.py @@ -1,5 +1,8 @@ from __future__ import annotations +from openapi_python_client.parser.properties.has_named_class import HasNamedClass +from openapi_python_client.schema.data_type import DataType + __all__ = ["EnumProperty", "ValueType"] from typing import Any, ClassVar, List, Union, cast @@ -19,7 +22,7 @@ @define -class EnumProperty(PropertyProtocol): +class EnumProperty(PropertyProtocol, HasNamedClass): """A property that should use an enum""" name: str @@ -42,7 +45,7 @@ class EnumProperty(PropertyProtocol): } @classmethod - def build( + def build( # noqa: PLR0911 cls, *, data: oai.Schema, @@ -102,6 +105,21 @@ def build( Union[List[int], List[str]], unchecked_value_list ) # We checked this with all the value_types stuff + if allow_null: # Only one of the values was None, that becomes a union + data.oneOf = [ + oai.Schema(type=DataType.NULL), + data.model_copy(update={"enum": value_list, "default": data.default}), + ] + data.enum = None + return UnionProperty.build( + data=data, + name=name, + required=required, + schemas=schemas, + parent_name=parent_name, + config=config, + ) + class_name = data.title or name if parent_name: class_name = f"{utils.pascal_case(parent_name)}{utils.pascal_case(class_name)}" @@ -135,38 +153,8 @@ def build( return checked_default, schemas prop = evolve(prop, default=checked_default) - # Now, if one of the values was None, wrap the type we just made in a union with None. We're - # constructing the UnionProperty directly instead of using UnionProperty.build() because we - # do *not* want the usual union behavior of creating ThingType1, ThingType2, etc. - returned_prop: EnumProperty | UnionProperty | PropertyError - if allow_null: - none_prop = NoneProperty.build( - name=name, - required=required, - default=None, - python_name=prop.python_name, - description=None, - example=None, - ) - assert not isinstance(none_prop, PropertyError) - union_prop = UnionProperty( - name=name, - required=required, - default=checked_default, - python_name=prop.python_name, - description=data.description, - example=data.example, - inner_properties=[ - none_prop, - prop, - ], - ) - returned_prop = union_prop - else: - returned_prop = prop - schemas = evolve(schemas, classes_by_name={**schemas.classes_by_name, class_info.name: prop}) - return returned_prop, schemas + return prop, schemas def convert_value(self, value: Any) -> Value | PropertyError | None: if value is None or isinstance(value, Value): diff --git a/openapi_python_client/parser/properties/has_named_class.py b/openapi_python_client/parser/properties/has_named_class.py new file mode 100644 index 000000000..d24b77420 --- /dev/null +++ b/openapi_python_client/parser/properties/has_named_class.py @@ -0,0 +1,8 @@ +from typing import Protocol, runtime_checkable + +from .schemas import Class + + +@runtime_checkable +class HasNamedClass(Protocol): + class_info: Class diff --git a/openapi_python_client/parser/properties/model_property.py b/openapi_python_client/parser/properties/model_property.py index 0dc13be54..66a4d7a2d 100644 --- a/openapi_python_client/parser/properties/model_property.py +++ b/openapi_python_client/parser/properties/model_property.py @@ -5,6 +5,8 @@ from attrs import define, evolve +from openapi_python_client.parser.properties.has_named_class import HasNamedClass + from ... import Config, utils from ... import schema as oai from ...utils import PythonIdentifier @@ -15,7 +17,7 @@ @define -class ModelProperty(PropertyProtocol): +class ModelProperty(PropertyProtocol, HasNamedClass): """A property which refers to another Schema""" name: str diff --git a/openapi_python_client/parser/properties/union.py b/openapi_python_client/parser/properties/union.py index 8b7b02a48..3c490ddd5 100644 --- a/openapi_python_client/parser/properties/union.py +++ b/openapi_python_client/parser/properties/union.py @@ -5,6 +5,10 @@ from attr import define, evolve +from openapi_python_client.parser.properties.has_named_class import HasNamedClass +from openapi_python_client.schema.openapi_schema_pydantic.reference import Reference +from openapi_python_client.schema.openapi_schema_pydantic.schema import Schema + from ... import Config from ... import schema as oai from ...utils import PythonIdentifier @@ -47,25 +51,45 @@ def build( """ from . import property_from_data - sub_properties: list[PropertyProtocol] = [] - type_list_data = [] - if isinstance(data.type, list): + if isinstance(data.type, list) and not (data.anyOf or data.oneOf): for _type in data.type: type_list_data.append(data.model_copy(update={"type": _type, "default": None})) - for i, sub_prop_data in enumerate(chain(data.anyOf, data.oneOf, type_list_data)): - sub_prop, schemas = property_from_data( - name=f"{name}_type_{i}", - required=True, - data=sub_prop_data, - schemas=schemas, - parent_name=parent_name, - config=config, - ) - if isinstance(sub_prop, PropertyError): - return PropertyError(detail=f"Invalid property in union {name}", data=sub_prop_data), schemas - sub_properties.append(sub_prop) + def process_items( + preserve_name_for_item: Schema | Reference | None = None, + ) -> tuple[list[PropertyProtocol] | PropertyError, Schemas]: + props: list[PropertyProtocol] = [] + new_schemas = schemas + items_with_classes: list[Schema | Reference] = [] + for i, sub_prop_data in enumerate(chain(data.anyOf, data.oneOf, type_list_data)): + sub_prop_name = name if sub_prop_data is preserve_name_for_item else f"{name}_type_{i}" + sub_prop, new_schemas = property_from_data( + name=sub_prop_name, + required=True, + data=sub_prop_data, + schemas=new_schemas, + parent_name=parent_name, + config=config, + ) + if isinstance(sub_prop, PropertyError): + return PropertyError(detail=f"Invalid property in union {name}", data=sub_prop_data), new_schemas + if isinstance(sub_prop, HasNamedClass): + items_with_classes.append(sub_prop_data) + props.append(sub_prop) + + if (not preserve_name_for_item) and (len(items_with_classes) == 1): + # After our first pass, if it turns out that there was exactly one enum or model in the list, + # then we'll do a second pass where we use the original name for that item instead of a + # "xyz_type_n" synthetic name. Enum and model are the only types that would get their own + # Python class. + return process_items(items_with_classes[0]) + + return props, new_schemas + + sub_properties, schemas = process_items() + if isinstance(sub_properties, PropertyError): + return sub_properties, schemas def flatten_union_properties(sub_properties: list[PropertyProtocol]) -> list[PropertyProtocol]: flattened = [] diff --git a/tests/test_parser/test_properties/test_init.py b/tests/test_parser/test_properties/test_init.py index a649653f1..147926dc7 100644 --- a/tests/test_parser/test_properties/test_init.py +++ b/tests/test_parser/test_properties/test_init.py @@ -12,6 +12,9 @@ StringProperty, UnionProperty, ) +from openapi_python_client.parser.properties.float import FloatProperty +from openapi_python_client.parser.properties.int import IntProperty +from openapi_python_client.parser.properties.none import NoneProperty 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 @@ -440,7 +443,7 @@ def test_property_from_data_str_enum_with_null( value_type=str, default=Value(python_code="ParentAnEnum.B", raw_value="B"), ) - none_property = none_property_factory(name=name, required=required) + none_property = none_property_factory(name=f"{name}_type_0", required=required) assert prop == union_property_factory( name=name, default=Value(python_code="ParentAnEnum.B", raw_value="B"), @@ -721,6 +724,33 @@ def test_property_from_data_list_of_types(self, config): assert isinstance(response, UnionProperty) assert len(response.inner_properties) == 2 + assert isinstance(response.inner_properties[0], FloatProperty) + assert isinstance(response.inner_properties[1], NoneProperty) + + def test_property_from_data_list_of_types_and_oneof(self, config): + from openapi_python_client.parser.properties import Schemas, property_from_data + + name = "union_prop" + required = True + data = oai.Schema( + type=[DataType.NUMBER, DataType.NULL], + anyOf=[ + oai.Schema(type=DataType.NUMBER), + oai.Schema(type=DataType.INTEGER), # this is OK because an integer is also a number + oai.Schema(type=DataType.NULL), + ], + ) + schemas = Schemas() + + response = property_from_data( + name=name, required=required, data=data, schemas=schemas, parent_name="parent", config=config + )[0] + + assert isinstance(response, UnionProperty) + assert len(response.inner_properties) == 3 + assert isinstance(response.inner_properties[0], FloatProperty) + assert isinstance(response.inner_properties[1], IntProperty) + assert isinstance(response.inner_properties[2], NoneProperty) def test_property_from_data_union_of_one_element(self, model_property_factory, config): from openapi_python_client.parser.properties import Schemas, property_from_data diff --git a/tests/test_parser/test_properties/test_union.py b/tests/test_parser/test_properties/test_union.py index acbbd06d6..1b8d9f103 100644 --- a/tests/test_parser/test_properties/test_union.py +++ b/tests/test_parser/test_properties/test_union.py @@ -1,8 +1,12 @@ 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.enum_property import EnumProperty +from openapi_python_client.parser.properties.model_property import ModelProperty from openapi_python_client.parser.properties.protocol import Value +from openapi_python_client.parser.properties.schemas import Class from openapi_python_client.schema import DataType, ParameterLocation +from openapi_python_client.utils import ClassName def test_property_from_data_union(union_property_factory, date_time_property_factory, string_property_factory, config): @@ -33,6 +37,99 @@ def test_property_from_data_union(union_property_factory, date_time_property_fac assert s == Schemas() +def test_name_is_preserved_if_union_has_one_model(config): + from openapi_python_client.parser.properties import Schemas, property_from_data + + parent_name = "parent" + name = "prop_1" + required = True + data = oai.Schema( + oneOf=[ + oai.Schema(type=DataType.OBJECT), + oai.Schema(type=DataType.STRING), + ], + ) + expected_model_class = Class(name=ClassName("ParentProp1", ""), module_name="parent_prop_1") + + p, s = property_from_data( + name=name, required=required, data=data, schemas=Schemas(), parent_name=parent_name, config=config + ) + + assert isinstance(p, UnionProperty) + assert len(p.inner_properties) == 2 + prop1 = p.inner_properties[0] + assert isinstance(prop1, ModelProperty) + assert prop1.name == name + assert prop1.class_info == expected_model_class + assert p.inner_properties[1].name == f"{name}_type_1" + + assert s == Schemas(classes_by_name={expected_model_class.name: prop1}, models_to_process=[prop1]) + + +def test_name_is_preserved_if_union_has_one_enum(config): + from openapi_python_client.parser.properties import Schemas, property_from_data + + parent_name = "parent" + name = "prop_1" + required = True + data = oai.Schema( + oneOf=[ + oai.Schema(type=DataType.INTEGER, enum=[10, 20]), + oai.Schema(type=DataType.STRING), + ], + ) + expected_enum_class = Class(name=ClassName("ParentProp1", ""), module_name="parent_prop_1") + + p, s = property_from_data( + name=name, required=required, data=data, schemas=Schemas(), parent_name=parent_name, config=config + ) + + assert isinstance(p, UnionProperty) + assert len(p.inner_properties) == 2 + prop1 = p.inner_properties[0] + assert isinstance(prop1, EnumProperty) + assert prop1.name == name + assert prop1.class_info == expected_enum_class + assert p.inner_properties[1].name == f"{name}_type_1" + + assert s == Schemas(classes_by_name={expected_enum_class.name: prop1}) + + +def test_name_is_preserved_if_union_has_multiple_models_or_enums(config): + from openapi_python_client.parser.properties import Schemas, property_from_data + + parent_name = "parent" + name = "prop_1" + required = True + data = oai.Schema( + oneOf=[ + oai.Schema(type=DataType.OBJECT), + oai.Schema(type=DataType.INTEGER, enum=[10, 20]), + ], + ) + expected_model_class = Class(name=ClassName("ParentProp1Type0", ""), module_name="parent_prop_1_type_0") + expected_enum_class = Class(name=ClassName("ParentProp1Type1", ""), module_name="parent_prop_1_type_1") + + p, s = property_from_data( + name=name, required=required, data=data, schemas=Schemas(), parent_name=parent_name, config=config + ) + + assert isinstance(p, UnionProperty) + assert len(p.inner_properties) == 2 + [prop1, prop2] = p.inner_properties + assert isinstance(prop1, ModelProperty) + assert prop1.name == f"{name}_type_0" + assert prop1.class_info == expected_model_class + assert isinstance(prop2, EnumProperty) + assert prop2.name == f"{name}_type_1" + assert prop2.class_info == expected_enum_class + + assert s == Schemas( + classes_by_name={expected_model_class.name: prop1, expected_enum_class.name: prop2}, + models_to_process=[prop1], + ) + + def test_build_union_property_invalid_property(config): name = "bad_union" required = True From 203e58b9dd9616785c05689857cfcb1e1265d425 Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Mon, 18 Nov 2024 11:58:43 -0800 Subject: [PATCH 4/5] cleanup/comments --- .../parser/properties/union.py | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/openapi_python_client/parser/properties/union.py b/openapi_python_client/parser/properties/union.py index 3c490ddd5..fc81f55bb 100644 --- a/openapi_python_client/parser/properties/union.py +++ b/openapi_python_client/parser/properties/union.py @@ -53,17 +53,27 @@ def build( type_list_data = [] if isinstance(data.type, list) and not (data.anyOf or data.oneOf): + # The schema specifies "type:" with a list of allowable types. If there is *not* also an "anyOf" + # or "oneOf", then we should treat that as a shorthand for a oneOf where each variant is just + # a single "type:". For example: + # {"type": ["string", "int"]} becomes + # {"oneOf": [{"type": "string"}, {"type": "int"}]} + # However, if there *is* also an "anyOf" or "oneOf" list, then the information from "type:" is + # redundant since every allowable variant type is already fully described in the list. for _type in data.type: type_list_data.append(data.model_copy(update={"type": _type, "default": None})) + # Here we're copying properties from the top-level union schema that might apply to one + # of the type variants, like "format" for a string. But we don't copy "default" because + # default values will be handled at the top level by the UnionProperty. def process_items( - preserve_name_for_item: Schema | Reference | None = None, + use_original_name_for: oai.Schema | oai.Reference | None = None, ) -> tuple[list[PropertyProtocol] | PropertyError, Schemas]: props: list[PropertyProtocol] = [] new_schemas = schemas - items_with_classes: list[Schema | Reference] = [] + schemas_with_classes: list[oai.Schema | oai.Reference] = [] for i, sub_prop_data in enumerate(chain(data.anyOf, data.oneOf, type_list_data)): - sub_prop_name = name if sub_prop_data is preserve_name_for_item else f"{name}_type_{i}" + sub_prop_name = name if sub_prop_data is use_original_name_for else f"{name}_type_{i}" sub_prop, new_schemas = property_from_data( name=sub_prop_name, required=True, @@ -75,15 +85,19 @@ def process_items( if isinstance(sub_prop, PropertyError): return PropertyError(detail=f"Invalid property in union {name}", data=sub_prop_data), new_schemas if isinstance(sub_prop, HasNamedClass): - items_with_classes.append(sub_prop_data) + schemas_with_classes.append(sub_prop_data) props.append(sub_prop) - if (not preserve_name_for_item) and (len(items_with_classes) == 1): - # After our first pass, if it turns out that there was exactly one enum or model in the list, - # then we'll do a second pass where we use the original name for that item instead of a - # "xyz_type_n" synthetic name. Enum and model are the only types that would get their own - # Python class. - return process_items(items_with_classes[0]) + if (not use_original_name_for) and len(schemas_with_classes) == 1: + # An example of this scenario is a oneOf where one of the variants is an inline enum or + # model, and the other is a simple value like null. If the name of the union property is + # "foo" then it's desirable for the enum or model class to be named "Foo", not "FooType1". + # So, we'll do a second pass where we tell ourselves to use the original property name + # for that item instead of "{name}_type_{i}". + # This only makes a functional difference if the variant was an inline schema, because + # we wouldn't be generating a class otherwise, but even if it wasn't inline this will + # save on pointlessly long variable names inside from_dict/to_dict. + return process_items(use_original_name_for=schemas_with_classes[0]) return props, new_schemas From 62e0c0244c704021320adfed91351bf952962ffc Mon Sep 17 00:00:00 2001 From: Eli Bishop Date: Fri, 6 Dec 2024 10:45:06 -0800 Subject: [PATCH 5/5] clarify special-casing logic for nullable object/enum --- .../parser/properties/union.py | 74 ++++++++++++++----- .../test_parser/test_properties/test_union.py | 10 +-- 2 files changed, 58 insertions(+), 26 deletions(-) diff --git a/openapi_python_client/parser/properties/union.py b/openapi_python_client/parser/properties/union.py index fc81f55bb..7a3fb085d 100644 --- a/openapi_python_client/parser/properties/union.py +++ b/openapi_python_client/parser/properties/union.py @@ -1,13 +1,11 @@ from __future__ import annotations from itertools import chain -from typing import Any, ClassVar, cast +from typing import Any, Callable, ClassVar, cast from attr import define, evolve from openapi_python_client.parser.properties.has_named_class import HasNamedClass -from openapi_python_client.schema.openapi_schema_pydantic.reference import Reference -from openapi_python_client.schema.openapi_schema_pydantic.schema import Schema from ... import Config from ... import schema as oai @@ -66,14 +64,46 @@ def build( # of the type variants, like "format" for a string. But we don't copy "default" because # default values will be handled at the top level by the UnionProperty. + def _add_index_suffix_to_variant_names(index: int) -> str: + return f"{name}_type_{index}" + + def _add_index_suffix_to_variant_names(index: int) -> str: + return f"{name}_type_{index}" + def process_items( - use_original_name_for: oai.Schema | oai.Reference | None = None, + variant_name_from_index_func: Callable[[int], str] = _add_index_suffix_to_variant_names, ) -> tuple[list[PropertyProtocol] | PropertyError, Schemas]: props: list[PropertyProtocol] = [] new_schemas = schemas - schemas_with_classes: list[oai.Schema | oai.Reference] = [] for i, sub_prop_data in enumerate(chain(data.anyOf, data.oneOf, type_list_data)): - sub_prop_name = name if sub_prop_data is use_original_name_for else f"{name}_type_{i}" + sub_prop_name = variant_name_from_index_func(i) + + # The sub_prop_name logic is what makes this a bit complicated. That value is used only + # if sub_prop is an *inline* schema and needs us to make up a name for it. For instance, + # in the following schema-- + # + # MyModel: + # properties: + # unionThing: + # oneOf: + # - type: object + # properties: ... + # - type: object + # properties: ... + # + # --both of the variants under oneOf are inline schemas. And since they're objects, we + # will be creating model classes for them, which need names. Inline schemas are named by + # concatenating names of parents; so, when we're in UnionProperty.build() for unionThing, + # the value of "name" is "my_model_union_thing", and then we set sub_prop_name to + # "my_model_union_thing_type_0" and "my_model_union_thing_type_1" for the two variants, + # and their model classes will be MyModelUnionThingType0 and MyModelUnionThingType1. + # + # However, in this example, if the second variant was just a scalar type instead of an + # object (like "type: null" or "type: string"), so that the first variant is the only + # one that needs a class... then it would be friendlier to call the first variant's + # class just MyModelUnionThing, not MyModelUnionThingType0. We'll check for that special + # case below; we can't know if that's the situation until after we've processed them all. + sub_prop, new_schemas = property_from_data( name=sub_prop_name, required=True, @@ -84,26 +114,30 @@ def process_items( ) if isinstance(sub_prop, PropertyError): return PropertyError(detail=f"Invalid property in union {name}", data=sub_prop_data), new_schemas - if isinstance(sub_prop, HasNamedClass): - schemas_with_classes.append(sub_prop_data) props.append(sub_prop) - if (not use_original_name_for) and len(schemas_with_classes) == 1: - # An example of this scenario is a oneOf where one of the variants is an inline enum or - # model, and the other is a simple value like null. If the name of the union property is - # "foo" then it's desirable for the enum or model class to be named "Foo", not "FooType1". - # So, we'll do a second pass where we tell ourselves to use the original property name - # for that item instead of "{name}_type_{i}". - # This only makes a functional difference if the variant was an inline schema, because - # we wouldn't be generating a class otherwise, but even if it wasn't inline this will - # save on pointlessly long variable names inside from_dict/to_dict. - return process_items(use_original_name_for=schemas_with_classes[0]) - return props, new_schemas - sub_properties, schemas = process_items() + sub_properties, new_schemas = process_items() + # Here's the check for the special case described above. If just one of the variants is + # an inline schema whose name matters, then we'll re-process them to simplify the naming. + # Unfortunately we do have to re-process them all; we can't just modify that one variant + # in place, because new_schemas already contains several references to its old name. + if ( + not isinstance(sub_properties, PropertyError) + and len([p for p in sub_properties if isinstance(p, HasNamedClass)]) == 1 + ): + def _use_same_name_as_parent_for_that_one_variant(index: int) -> str: + for i, p in enumerate(sub_properties): + if i == index and isinstance(p, HasNamedClass): + return name + return _add_index_suffix_to_variant_names(index) + + sub_properties, new_schemas = process_items(_use_same_name_as_parent_for_that_one_variant) + if isinstance(sub_properties, PropertyError): return sub_properties, schemas + schemas = new_schemas def flatten_union_properties(sub_properties: list[PropertyProtocol]) -> list[PropertyProtocol]: flattened = [] diff --git a/tests/test_parser/test_properties/test_union.py b/tests/test_parser/test_properties/test_union.py index 1b8d9f103..decf6c412 100644 --- a/tests/test_parser/test_properties/test_union.py +++ b/tests/test_parser/test_properties/test_union.py @@ -37,7 +37,7 @@ def test_property_from_data_union(union_property_factory, date_time_property_fac assert s == Schemas() -def test_name_is_preserved_if_union_has_one_model(config): +def test_name_is_preserved_if_union_is_nullable_model(config): from openapi_python_client.parser.properties import Schemas, property_from_data parent_name = "parent" @@ -46,7 +46,7 @@ def test_name_is_preserved_if_union_has_one_model(config): data = oai.Schema( oneOf=[ oai.Schema(type=DataType.OBJECT), - oai.Schema(type=DataType.STRING), + oai.Schema(type=DataType.NULL), ], ) expected_model_class = Class(name=ClassName("ParentProp1", ""), module_name="parent_prop_1") @@ -61,12 +61,11 @@ def test_name_is_preserved_if_union_has_one_model(config): assert isinstance(prop1, ModelProperty) assert prop1.name == name assert prop1.class_info == expected_model_class - assert p.inner_properties[1].name == f"{name}_type_1" assert s == Schemas(classes_by_name={expected_model_class.name: prop1}, models_to_process=[prop1]) -def test_name_is_preserved_if_union_has_one_enum(config): +def test_name_is_preserved_if_union_is_nullable_enum(config): from openapi_python_client.parser.properties import Schemas, property_from_data parent_name = "parent" @@ -75,7 +74,7 @@ def test_name_is_preserved_if_union_has_one_enum(config): data = oai.Schema( oneOf=[ oai.Schema(type=DataType.INTEGER, enum=[10, 20]), - oai.Schema(type=DataType.STRING), + oai.Schema(type=DataType.NULL), ], ) expected_enum_class = Class(name=ClassName("ParentProp1", ""), module_name="parent_prop_1") @@ -90,7 +89,6 @@ def test_name_is_preserved_if_union_has_one_enum(config): assert isinstance(prop1, EnumProperty) assert prop1.name == name assert prop1.class_info == expected_enum_class - assert p.inner_properties[1].name == f"{name}_type_1" assert s == Schemas(classes_by_name={expected_enum_class.name: prop1})