Union variants use Title names instead of indices (#1301)

Replaces #962 with a rebase & a couple tweaks

Co-authored-by: Gibryon Bhojraj <gib.bhojraj@wallaroo.ai>

Author: dbanty

.changeset/change_some_union_variant_names.md

@@ -0,0 +1,18 @@
+---
+default: major
+---
+
+# Change some union variant names
+
+When creating a union with `oneOf`, `anyOf`, or a list of `type`, the name of each variant used to be `type_{index}`
+where the index is based on the order of the types in the union.
+
+This made some modules difficult to understand, what is a `my_type_type_0` after all?
+It also meant that reordering union members, while not a breaking change to the API, _would_ be a breaking change 
+for generated clients.
+
+Now, if an individual variant has a `title` attribute, that `title` will be used in the name instead.
+This is only an enhancement for documents which use `title` in union variants, and only a breaking change for 
+_inline models_ (not `#/components/schemas` which should already have used more descriptive names).
+
+Thanks @wallagib for PR #962!

end_to_end_tests/baseline_openapi_3.0.json

@@ -2171,6 +2171,7 @@
             "oneOf": [
               {
                 "type": "object",
+                "title": "Apples",
                 "properties": {
                   "apples": {
                     "type": "string"
@@ -2179,6 +2180,7 @@
               },
               {
                 "type": "object",
+                "title": "Bananas",
                 "properties": {
                   "bananas": {
                     "type": "string"

end_to_end_tests/baseline_openapi_3.1.yaml

@@ -2157,6 +2157,7 @@ info:
           "oneOf": [
             {
               "type": "object",
+              "title": "Apples",
               "properties": {
                 "apples": {
                   "type": "string"
@@ -2165,6 +2166,7 @@ info:
             },
             {
               "type": "object",
+              "title": "Bananas",
               "properties": {
                 "bananas": {
                   "type": "string"

end_to_end_tests/golden-record/my_test_api_client/models/__init__.py

@@ -72,8 +72,8 @@
 from .model_with_recursive_ref_in_additional_properties import ModelWithRecursiveRefInAdditionalProperties
 from .model_with_union_property import ModelWithUnionProperty
 from .model_with_union_property_inlined import ModelWithUnionPropertyInlined
-from .model_with_union_property_inlined_fruit_type_0 import ModelWithUnionPropertyInlinedFruitType0
-from .model_with_union_property_inlined_fruit_type_1 import ModelWithUnionPropertyInlinedFruitType1
+from .model_with_union_property_inlined_apples import ModelWithUnionPropertyInlinedApples
+from .model_with_union_property_inlined_bananas import ModelWithUnionPropertyInlinedBananas
 from .none import None_
 from .octet_stream_tests_octet_stream_post_response_200 import OctetStreamTestsOctetStreamPostResponse200
 from .post_bodies_multiple_data_body import PostBodiesMultipleDataBody
@@ -155,8 +155,8 @@
     "ModelWithRecursiveRefInAdditionalProperties",
     "ModelWithUnionProperty",
     "ModelWithUnionPropertyInlined",
-    "ModelWithUnionPropertyInlinedFruitType0",
-    "ModelWithUnionPropertyInlinedFruitType1",
+    "ModelWithUnionPropertyInlinedApples",
+    "ModelWithUnionPropertyInlinedBananas",
     "None_",
     "OctetStreamTestsOctetStreamPostResponse200",
     "PostBodiesMultipleDataBody",

end_to_end_tests/golden-record/my_test_api_client/models/model_with_union_property_inlined.py

@@ -6,8 +6,8 @@
 from ..types import UNSET, Unset
 
 if TYPE_CHECKING:
-    from ..models.model_with_union_property_inlined_fruit_type_0 import ModelWithUnionPropertyInlinedFruitType0
-    from ..models.model_with_union_property_inlined_fruit_type_1 import ModelWithUnionPropertyInlinedFruitType1
+    from ..models.model_with_union_property_inlined_apples import ModelWithUnionPropertyInlinedApples
+    from ..models.model_with_union_property_inlined_bananas import ModelWithUnionPropertyInlinedBananas
 
 
 T = TypeVar("T", bound="ModelWithUnionPropertyInlined")
@@ -17,18 +17,18 @@
 class ModelWithUnionPropertyInlined:
     """
     Attributes:
-        fruit (Union['ModelWithUnionPropertyInlinedFruitType0', 'ModelWithUnionPropertyInlinedFruitType1', Unset]):
+        fruit (Union['ModelWithUnionPropertyInlinedApples', 'ModelWithUnionPropertyInlinedBananas', Unset]):
     """
 
-    fruit: Union["ModelWithUnionPropertyInlinedFruitType0", "ModelWithUnionPropertyInlinedFruitType1", Unset] = UNSET
+    fruit: Union["ModelWithUnionPropertyInlinedApples", "ModelWithUnionPropertyInlinedBananas", Unset] = UNSET
 
     def to_dict(self) -> dict[str, Any]:
-        from ..models.model_with_union_property_inlined_fruit_type_0 import ModelWithUnionPropertyInlinedFruitType0
+        from ..models.model_with_union_property_inlined_apples import ModelWithUnionPropertyInlinedApples
 
         fruit: Union[Unset, dict[str, Any]]
         if isinstance(self.fruit, Unset):
             fruit = UNSET
-        elif isinstance(self.fruit, ModelWithUnionPropertyInlinedFruitType0):
+        elif isinstance(self.fruit, ModelWithUnionPropertyInlinedApples):
             fruit = self.fruit.to_dict()
         else:
             fruit = self.fruit.to_dict()
@@ -43,29 +43,29 @@ def to_dict(self) -> dict[str, Any]:
 
     @classmethod
     def from_dict(cls: type[T], src_dict: Mapping[str, Any]) -> T:
-        from ..models.model_with_union_property_inlined_fruit_type_0 import ModelWithUnionPropertyInlinedFruitType0
-        from ..models.model_with_union_property_inlined_fruit_type_1 import ModelWithUnionPropertyInlinedFruitType1
+        from ..models.model_with_union_property_inlined_apples import ModelWithUnionPropertyInlinedApples
+        from ..models.model_with_union_property_inlined_bananas import ModelWithUnionPropertyInlinedBananas
 
         d = dict(src_dict)
 
         def _parse_fruit(
             data: object,
-        ) -> Union["ModelWithUnionPropertyInlinedFruitType0", "ModelWithUnionPropertyInlinedFruitType1", Unset]:
+        ) -> Union["ModelWithUnionPropertyInlinedApples", "ModelWithUnionPropertyInlinedBananas", Unset]:
             if isinstance(data, Unset):
                 return data
             try:
                 if not isinstance(data, dict):
                     raise TypeError()
-                fruit_type_0 = ModelWithUnionPropertyInlinedFruitType0.from_dict(data)
+                fruit_apples = ModelWithUnionPropertyInlinedApples.from_dict(data)
 
-                return fruit_type_0
+                return fruit_apples
             except:  # noqa: E722
                 pass
             if not isinstance(data, dict):
                 raise TypeError()
-            fruit_type_1 = ModelWithUnionPropertyInlinedFruitType1.from_dict(data)
+            fruit_bananas = ModelWithUnionPropertyInlinedBananas.from_dict(data)
 
-            return fruit_type_1
+            return fruit_bananas
 
         fruit = _parse_fruit(d.pop("fruit", UNSET))
 

end_to_end_tests/golden-record/my_test_api_client/models/model_with_union_property_inlined_fruit_type_0.py renamed to end_to_end_tests/golden-record/my_test_api_client/models/model_with_union_property_inlined_apples.py

@@ -6,11 +6,11 @@
 
 from ..types import UNSET, Unset
 
-T = TypeVar("T", bound="ModelWithUnionPropertyInlinedFruitType0")
+T = TypeVar("T", bound="ModelWithUnionPropertyInlinedApples")
 
 
 @_attrs_define
-class ModelWithUnionPropertyInlinedFruitType0:
+class ModelWithUnionPropertyInlinedApples:
     """
     Attributes:
         apples (Union[Unset, str]):
@@ -35,12 +35,12 @@ def from_dict(cls: type[T], src_dict: Mapping[str, Any]) -> T:
         d = dict(src_dict)
         apples = d.pop("apples", UNSET)
 
-        model_with_union_property_inlined_fruit_type_0 = cls(
+        model_with_union_property_inlined_apples = cls(
             apples=apples,
         )
 
-        model_with_union_property_inlined_fruit_type_0.additional_properties = d
-        return model_with_union_property_inlined_fruit_type_0
+        model_with_union_property_inlined_apples.additional_properties = d
+        return model_with_union_property_inlined_apples
 
     @property
     def additional_keys(self) -> list[str]:

end_to_end_tests/golden-record/my_test_api_client/models/model_with_union_property_inlined_fruit_type_1.py renamed to end_to_end_tests/golden-record/my_test_api_client/models/model_with_union_property_inlined_bananas.py

@@ -6,11 +6,11 @@
 
 from ..types import UNSET, Unset
 
-T = TypeVar("T", bound="ModelWithUnionPropertyInlinedFruitType1")
+T = TypeVar("T", bound="ModelWithUnionPropertyInlinedBananas")
 
 
 @_attrs_define
-class ModelWithUnionPropertyInlinedFruitType1:
+class ModelWithUnionPropertyInlinedBananas:
     """
     Attributes:
         bananas (Union[Unset, str]):
@@ -35,12 +35,12 @@ def from_dict(cls: type[T], src_dict: Mapping[str, Any]) -> T:
         d = dict(src_dict)
         bananas = d.pop("bananas", UNSET)
 
-        model_with_union_property_inlined_fruit_type_1 = cls(
+        model_with_union_property_inlined_bananas = cls(
             bananas=bananas,
         )
 
-        model_with_union_property_inlined_fruit_type_1.additional_properties = d
-        return model_with_union_property_inlined_fruit_type_1
+        model_with_union_property_inlined_bananas.additional_properties = d
+        return model_with_union_property_inlined_bananas
 
     @property
     def additional_keys(self) -> list[str]:

openapi_python_client/parser/properties/union.py

@@ -28,7 +28,14 @@ class UnionProperty(PropertyProtocol):
 
     @classmethod
     def build(
-        cls, *, data: oai.Schema, name: str, required: bool, schemas: Schemas, parent_name: str, config: Config
+        cls,
+        *,
+        data: oai.Schema,
+        name: str,
+        required: bool,
+        schemas: Schemas,
+        parent_name: str,
+        config: Config,
     ) -> tuple[UnionProperty | PropertyError, Schemas]:
         """
         Create a `UnionProperty` the right way.
@@ -55,16 +62,30 @@ def build(
                 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)):
+            # If a schema has a unique title property, we can use that to carry forward a descriptive name instead of "type_0"
+            subscript: str
+            if (
+                isinstance(sub_prop_data, oai.Schema)
+                and sub_prop_data.title is not None
+                and sub_prop_data.title != data.title
+            ):
+                subscript = sub_prop_data.title
+            else:
+                subscript = f"type_{i}"
+
             sub_prop, schemas = property_from_data(
-                name=f"{name}_type_{i}",
+                name=f"{name}_{subscript}",
                 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
+                return (
+                    PropertyError(detail=f"Invalid property in union {name}", data=sub_prop_data),
+                    schemas,
+                )
             sub_properties.append(sub_prop)
 
         def flatten_union_properties(sub_properties: list[PropertyProtocol]) -> list[PropertyProtocol]:
@@ -108,7 +129,12 @@ def convert_value(self, value: Any) -> Value | None | PropertyError:
 
     def _get_inner_type_strings(self, json: bool) -> set[str]:
         return {
-            p.get_type_string(no_optional=True, json=json, quoted=not p.is_base_type) for p in self.inner_properties
+            p.get_type_string(
+                no_optional=True,
+                json=json,
+                quoted=not p.is_base_type,
+            )
+            for p in self.inner_properties
         }
 
     @staticmethod

tests/conftest.py

@@ -6,7 +6,7 @@
 import pytest
 from mypy.semanal_shared import Protocol
 
-from openapi_python_client import Config, MetaType
+from openapi_python_client import Config, MetaType, utils
 from openapi_python_client import schema as oai
 from openapi_python_client.config import ConfigFile
 from openapi_python_client.parser.properties import (
@@ -321,5 +321,5 @@ def _common_kwargs(kwargs: dict[str, Any]) -> dict[str, Any]:
         **kwargs,
     }
     if not kwargs.get("python_name"):
-        kwargs["python_name"] = kwargs["name"]
+        kwargs["python_name"] = utils.PythonIdentifier(value=kwargs["name"], prefix="")
     return kwargs

tests/test_parser/test_properties/test_union.py

@@ -1,6 +1,6 @@
 import openapi_python_client.schema as oai
 from openapi_python_client.parser.errors import ParseError
-from openapi_python_client.parser.properties import Schemas, UnionProperty
+from openapi_python_client.parser.properties import Schemas, UnionProperty, property_from_data
 from openapi_python_client.schema import DataType, ParameterLocation
 
 
@@ -28,3 +28,49 @@ def test_not_required_in_path(config):
 
     err = prop.validate_location(ParameterLocation.PATH)
     assert isinstance(err, ParseError)
+
+
+def test_union_oneOf_descriptive_type_name(
+    union_property_factory, date_time_property_factory, string_property_factory, config
+):
+    nested_schema_variant_A = oai.Schema(type=DataType.STRING, title="A")
+    nested_schema_variant_B = oai.Schema(type=DataType.STRING, title="B")
+    nested_schema_variant_2 = oai.Schema(type=DataType.STRING)
+    nested_schema_variant_C = oai.Schema(type=DataType.STRING, title="C")
+    nested_schema_variant_4 = oai.Schema(type=DataType.STRING)
+
+    name = "union_prop"
+    required = True
+    data = oai.Schema(
+        anyOf=[
+            # AnyOf retains the old naming convention
+            nested_schema_variant_C,
+            nested_schema_variant_4,
+        ],
+        oneOf=[
+            # OneOf fields that define their own titles will have those titles as their Type names
+            nested_schema_variant_A,
+            nested_schema_variant_B,
+            nested_schema_variant_2,
+            oai.Schema(type=DataType.STRING, schema_format="date-time"),
+        ],
+    )
+    expected = union_property_factory(
+        name=name,
+        required=required,
+        inner_properties=[
+            string_property_factory(name=f"{name}_C"),
+            string_property_factory(name=f"{name}_type_1"),
+            string_property_factory(name=f"{name}_A"),
+            string_property_factory(name=f"{name}_B"),
+            string_property_factory(name=f"{name}_type_4"),
+            date_time_property_factory(name=f"{name}_type_5"),
+        ],
+    )
+
+    p, s = property_from_data(
+        name=name, required=required, data=data, schemas=Schemas(), parent_name="parent", config=config
+    )
+
+    assert p == expected
+    assert s == Schemas()