diff --git a/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/api/default/post_types_unions_duplicate_types.py b/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/api/default/post_types_unions_duplicate_types.py index f6eabc4a7..d9cbf92ee 100644 --- a/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/api/default/post_types_unions_duplicate_types.py +++ b/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/api/default/post_types_unions_duplicate_types.py @@ -20,7 +20,7 @@ def _get_kwargs( "url": "/types/unions/duplicate-types", } - if isinstance(body, AModel): + if not isinstance(body, Unset): _kwargs["json"] = body.to_dict() headers["Content-Type"] = "application/json" diff --git a/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/a_model.py b/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/a_model.py index 4ca83858f..a7aa02218 100644 --- a/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/a_model.py +++ b/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/a_model.py @@ -179,8 +179,6 @@ def to_dict(self) -> dict[str, Any]: not_required_one_of_models: dict[str, Any] | Unset if isinstance(self.not_required_one_of_models, Unset): not_required_one_of_models = UNSET - elif isinstance(self.not_required_one_of_models, FreeFormModel): - not_required_one_of_models = self.not_required_one_of_models.to_dict() else: not_required_one_of_models = self.not_required_one_of_models.to_dict() diff --git a/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/extended.py b/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/extended.py index cf4966bf0..a6f86ca84 100644 --- a/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/extended.py +++ b/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/extended.py @@ -182,8 +182,6 @@ def to_dict(self) -> dict[str, Any]: not_required_one_of_models: dict[str, Any] | Unset if isinstance(self.not_required_one_of_models, Unset): not_required_one_of_models = UNSET - elif isinstance(self.not_required_one_of_models, FreeFormModel): - not_required_one_of_models = self.not_required_one_of_models.to_dict() else: not_required_one_of_models = self.not_required_one_of_models.to_dict() diff --git a/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/model_with_union_property_inlined.py b/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/model_with_union_property_inlined.py index fbce31507..624710591 100644 --- a/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/model_with_union_property_inlined.py +++ b/end_to_end_tests/golden-records/my-test-api-client/my_test_api_client/models/model_with_union_property_inlined.py @@ -25,15 +25,9 @@ class ModelWithUnionPropertyInlined: fruit: ModelWithUnionPropertyInlinedApples | ModelWithUnionPropertyInlinedBananas | Unset = UNSET def to_dict(self) -> dict[str, Any]: - from ..models.model_with_union_property_inlined_apples import ( - ModelWithUnionPropertyInlinedApples, # noqa: PLC0415 - ) - fruit: dict[str, Any] | Unset if isinstance(self.fruit, Unset): fruit = UNSET - elif isinstance(self.fruit, ModelWithUnionPropertyInlinedApples): - fruit = self.fruit.to_dict() else: fruit = self.fruit.to_dict() diff --git a/openapi_python_client/templates/property_templates/union_property.py.jinja b/openapi_python_client/templates/property_templates/union_property.py.jinja index 45f350f5e..fb9e817d8 100644 --- a/openapi_python_client/templates/property_templates/union_property.py.jinja +++ b/openapi_python_client/templates/property_templates/union_property.py.jinja @@ -40,6 +40,8 @@ def _parse_{{ property.python_name }}(data: object) -> {{ property.get_type_stri {% endmacro %} {% macro transform(property, source, destination, declare_type=True, skip_unset=False) %} +{% set inner_templates = property.inner_properties | map(attribute="template") | unique | list %} +{% set homogeneous_model_union = property.inner_properties and inner_templates == ["model_property.py.jinja"] %} {% set ns = namespace(contains_properties_without_transform = false, contains_modified_properties = not property.required, has_if = false) %} {% if declare_type %}{{ destination }}: {{ property.get_type_string(json=True) | as_unembedded_code }}{% endif %} @@ -48,6 +50,17 @@ if isinstance({{ source }}, Unset): {{ destination }} = UNSET {% set ns.has_if = true %} {% endif %} +{% if homogeneous_model_union %} +{% if property.required %} +{{ destination }} = {{ source }}.to_dict() +{% elif ns.has_if %} +else: + {{ destination }} = {{ source }}.to_dict() +{% else %} +if not isinstance({{ source }}, Unset): + {{ destination }} = {{ source }}.to_dict() +{% endif %} +{% else %} {% for inner_property in property.inner_properties %} {% import "property_templates/" + inner_property.template as inner_template %} {% if not inner_template.transform %} @@ -72,6 +85,7 @@ else: {%- elif ns.contains_properties_without_transform %} {{ destination }} = {{ source }} {%- endif %} +{% endif %} {% endmacro %} diff --git a/tests/test_templates/test_property_templates/test_union_property/test_union_property.py b/tests/test_templates/test_property_templates/test_union_property/test_union_property.py new file mode 100644 index 000000000..0dcaa1c29 --- /dev/null +++ b/tests/test_templates/test_property_templates/test_union_property/test_union_property.py @@ -0,0 +1,92 @@ +"""Tests for union_property.py.jinja's `transform` macro. + +See issue: redundant `isinstance` dispatch when every union member is a model. +""" + +import pytest +from jinja2 import Environment, PackageLoader + +from openapi_python_client import TEMPLATE_FILTERS + + +@pytest.fixture +def jinja_env() -> Environment: + """A jinja2 environment matching the one used for real code generation. + + Needs the `loopcontrols` extension (for `{% continue %}`) which the shared `env` fixture in + `tests/test_templates/conftest.py` does not enable. + """ + env = Environment( + loader=PackageLoader("openapi_python_client"), + trim_blocks=True, + lstrip_blocks=True, + extensions=["jinja2.ext.loopcontrols"], + ) + env.filters.update(TEMPLATE_FILTERS) + return env + + +def test_transform_homogeneous_model_union_skips_isinstance_dispatch( + jinja_env: Environment, union_property_factory, model_property_factory +) -> None: + """When every union member is a model, `.to_dict()` is called unconditionally (no per-member isinstance + dispatch), since every branch's body is identical regardless of which member actually matched. + """ + model_a = model_property_factory(name="model_a", required=True) + model_b = model_property_factory(name="model_b", required=True) + union = union_property_factory(name="both_models", required=False, inner_properties=[model_a, model_b]) + + template = jinja_env.get_template("property_templates/union_property.py.jinja") + result = template.module.transform(union, "self.both_models", "both_models") + + assert "isinstance(self.both_models, Unset)" in result + assert "isinstance(self.both_models, MyClass)" not in result + assert result.count(".to_dict()") == 1 + + +def test_transform_mixed_union_keeps_isinstance_dispatch( + jinja_env: Environment, union_property_factory, model_property_factory, string_property_factory +) -> None: + """Mixed unions (model + non-model members) must keep the existing per-member isinstance dispatch, + since those branches genuinely differ in behavior. + """ + model_a = model_property_factory(name="model_a", required=True) + string_b = string_property_factory(name="string_b", required=True) + union = union_property_factory(name="mixed", required=False, inner_properties=[model_a, string_b]) + + template = jinja_env.get_template("property_templates/union_property.py.jinja") + result = template.module.transform(union, "self.mixed", "mixed") + + assert "isinstance(self.mixed, MyClass)" in result + assert result.count(".to_dict()") == 1 + + +def test_transform_required_homogeneous_model_union_has_no_guard( + jinja_env: Environment, union_property_factory, model_property_factory +) -> None: + """A required homogeneous-model union can never be Unset, so `.to_dict()` is called with no guard at all.""" + model_a = model_property_factory(name="model_a", required=True) + model_b = model_property_factory(name="model_b", required=True) + union = union_property_factory(name="both_models", required=True, inner_properties=[model_a, model_b]) + + template = jinja_env.get_template("property_templates/union_property.py.jinja") + result = template.module.transform(union, "self.both_models", "both_models") + + assert "isinstance" not in result + assert result.strip() == "both_models: dict[str, Any]\nboth_models = self.both_models.to_dict()" + + +def test_transform_skip_unset_homogeneous_model_union_still_guards_unset( + jinja_env: Environment, union_property_factory, model_property_factory +) -> None: + """Request bodies render `transform` with `skip_unset=True` (the `UNSET` default is assigned elsewhere), but an + optional homogeneous-model union can still *be* `Unset` at runtime, so `.to_dict()` must still be guarded -- + unconditionally calling it here would raise `AttributeError`/fail type-checking (`Unset` has no `to_dict`). + """ + model_a = model_property_factory(name="model_a", required=True) + union = union_property_factory(name="body", required=False, inner_properties=[model_a]) + + template = jinja_env.get_template("property_templates/union_property.py.jinja") + result = template.module.transform(union, "body", '_kwargs["json"]', skip_unset=True, declare_type=False) + + assert result.strip() == 'if not isinstance(body, Unset):\n _kwargs["json"] = body.to_dict()'