Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}

Expand All @@ -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 %}
Expand All @@ -72,6 +85,7 @@ else:
{%- elif ns.contains_properties_without_transform %}
{{ destination }} = {{ source }}
{%- endif %}
{% endif %}
{% endmacro %}


Expand Down
Original file line number Diff line number Diff line change
@@ -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()'