fix!: Turn all tags (endpoints) into valid Python identifiers [WIP]

Author: dbanty

openapi_python_client/config.py

@@ -20,9 +20,6 @@ class Config(BaseModel):
     @staticmethod
     def load_from_path(path: Path) -> "Config":
         """Creates a Config from provided JSON or YAML file and sets a bunch of globals from it"""
-        from . import utils
-
         config_data = yaml.safe_load(path.read_text())
         config = Config(**config_data)
-        utils.FIELD_PREFIX = config.field_prefix
         return config

openapi_python_client/parser/openapi.py

@@ -11,6 +11,7 @@
 from ..config import Config
 from .errors import GeneratorError, ParseError, PropertyError
 from .properties import Class, EnumProperty, ModelProperty, Property, Schemas, build_schemas, property_from_data
+from .properties.property import _PythonIdentifier, to_valid_python_identifier
 from .responses import Response, response_from_data
 
 
@@ -30,9 +31,9 @@ class EndpointCollection:
     @staticmethod
     def from_data(
         *, data: Dict[str, oai.PathItem], schemas: Schemas, config: Config
-    ) -> Tuple[Dict[str, "EndpointCollection"], Schemas]:
+    ) -> Tuple[Dict[_PythonIdentifier, "EndpointCollection"], Schemas]:
         """Parse the openapi paths data to get EndpointCollections by tag"""
-        endpoints_by_tag: Dict[str, EndpointCollection] = {}
+        endpoints_by_tag: Dict[_PythonIdentifier, EndpointCollection] = {}
 
         methods = ["get", "put", "post", "delete", "options", "head", "patch", "trace"]
 
@@ -41,7 +42,7 @@ def from_data(
                 operation: Optional[oai.Operation] = getattr(path_data, method)
                 if operation is None:
                     continue
-                tag = utils.snake_case((operation.tags or ["default"])[0])
+                tag = to_valid_python_identifier((operation.tags or ["default"])[0], prefix="tag")
                 collection = endpoints_by_tag.setdefault(tag, EndpointCollection(tag=tag))
                 endpoint, schemas = Endpoint.from_data(
                     data=operation, path=path, method=method, tag=tag, schemas=schemas, config=config
@@ -261,8 +262,8 @@ def _add_parameters(
             if prop.python_name in used_python_names:
                 duplicate, duplicate_location = used_python_names[prop.python_name]
                 if duplicate.python_name == prop.python_name:  # Existing should be converted too for consistency
-                    duplicate.set_python_name(f"{duplicate.python_name}_{duplicate_location}")
-                prop.set_python_name(f"{prop.python_name}_{param.param_in}")
+                    duplicate.set_python_name(f"{duplicate.python_name}_{duplicate_location}", config=config)
+                prop.set_python_name(f"{prop.python_name}_{param.param_in}", config=config)
             else:
                 used_python_names[prop.python_name] = (prop, param.param_in)
 
@@ -340,7 +341,7 @@ class GeneratorData:
     version: str
     models: Iterator[ModelProperty]
     errors: List[ParseError]
-    endpoint_collections_by_tag: Dict[str, EndpointCollection]
+    endpoint_collections_by_tag: Dict[_PythonIdentifier, EndpointCollection]
     enums: Iterator[EnumProperty]
 
     @staticmethod

openapi_python_client/parser/properties/model_property.py

@@ -7,7 +7,7 @@
 from ... import schema as oai
 from ... import utils
 from ..errors import ParseError, PropertyError
-from .property import Property
+from .property import Property, to_valid_python_identifier
 from .schemas import Class, Schemas, parse_reference_path
 
 
@@ -210,6 +210,7 @@ def build_model_property(
         required=required,
         name=name,
         additional_properties=additional_properties,
+        python_name=to_valid_python_identifier(name, prefix=config.field_prefix),
     )
     if class_info.name in schemas.classes_by_name:
         error = PropertyError(data=data, detail=f'Attempted to generate duplicate models with name "{class_info.name}"')

openapi_python_client/parser/properties/property.py

@@ -1,8 +1,27 @@
-from typing import ClassVar, Optional, Set
+from typing import ClassVar, NewType, Optional, Set
 
 import attr
 
-from ... import utils
+from ... import Config
+from ...utils import fix_keywords, fix_reserved_words, sanitize, snake_case
+
+_PythonIdentifier = NewType("_PythonIdentifier", str)
+
+
+def to_valid_python_identifier(value: str, prefix: str) -> _PythonIdentifier:
+    """
+    Given a string, attempt to coerce it into a valid Python identifier by stripping out invalid characters and, if
+    necessary, prepending a prefix.
+
+    See:
+        https://docs.python.org/3/reference/lexical_analysis.html#identifiers
+    """
+    new_value = fix_reserved_words(fix_keywords(snake_case(sanitize(value))))
+
+    if new_value.isidentifier():
+        return _PythonIdentifier(new_value)
+
+    return _PythonIdentifier(f"{prefix}{new_value}")
 
 
 @attr.s(auto_attribs=True, frozen=True)
@@ -26,16 +45,13 @@ class Property:
     _type_string: ClassVar[str] = ""
     _json_type_string: ClassVar[str] = ""  # Type of the property after JSON serialization
     default: Optional[str] = attr.ib()
-    python_name: str = attr.ib(init=False)
+    python_name: _PythonIdentifier
 
     template: ClassVar[Optional[str]] = None
     json_is_dict: ClassVar[bool] = False
 
-    def __attrs_post_init__(self) -> None:
-        self.set_python_name(self.name)
-
-    def set_python_name(self, new_name: str) -> None:
-        object.__setattr__(self, "python_name", utils.to_valid_python_identifier(utils.snake_case(new_name)))
+    def set_python_name(self, new_name: str, config: Config) -> None:
+        object.__setattr__(self, "python_name", to_valid_python_identifier(new_name, prefix=config.field_prefix))
 
     def get_base_type_string(self) -> str:
         return self._type_string

openapi_python_client/utils.py

@@ -55,23 +55,3 @@ def kebab_case(value: str) -> str:
 
 def remove_string_escapes(value: str) -> str:
     return value.replace('"', r"\"")
-
-
-# This can be changed by config.Config.load_config
-FIELD_PREFIX = "field_"
-
-
-def to_valid_python_identifier(value: str) -> str:
-    """
-    Given a string, attempt to coerce it into a valid Python identifier by stripping out invalid characters and, if
-    necessary, prepending a prefix.
-
-    See:
-        https://docs.python.org/3/reference/lexical_analysis.html#identifiers
-    """
-    new_value = fix_reserved_words(fix_keywords(sanitize(value)))
-
-    if new_value.isidentifier():
-        return new_value
-
-    return f"{FIELD_PREFIX}{new_value}"

tests/conftest.py

@@ -26,8 +26,11 @@ def _factory(**kwargs):
             "optional_properties": [],
             "relative_imports": set(),
             "additional_properties": False,
+            "python_name": "",
             **kwargs,
         }
+        if not kwargs["python_name"]:
+            kwargs["python_name"] = kwargs["name"]
         return ModelProperty(**kwargs)
 
     return _factory
@@ -53,6 +56,8 @@ def _factory(**kwargs):
             "value_type": str,
             **kwargs,
         }
+        if not kwargs.get("python_name"):
+            kwargs["python_name"] = kwargs["name"]
         return EnumProperty(**kwargs)
 
     return _factory

tests/test_config.py

@@ -22,11 +22,9 @@ def test_load_from_path(mocker):
 
     config = Config.load_from_path(fake_path)
     safe_load.assert_called()
-    assert utils.FIELD_PREFIX == "blah"
+    assert config.field_prefix == "blah"
     assert config.class_overrides["Class1"] == override1
     assert config.class_overrides["Class2"] == override2
     assert config.project_name_override == "project-name"
     assert config.package_name_override == "package_name"
     assert config.package_version_override == "package_version"
-
-    utils.FIELD_PREFIX = "field_"

tests/test_parser/test_openapi.py

@@ -460,12 +460,12 @@ def test__add_responses(self, mocker):
         response_1 = Response(
             status_code=200,
             source="source",
-            prop=DateTimeProperty(name="datetime", required=True, nullable=False, default=None),
+            prop=DateTimeProperty(name="datetime", required=True, nullable=False, default=None, python_name="datetime"),
         )
         response_2 = Response(
             status_code=404,
             source="source",
-            prop=DateProperty(name="date", required=True, nullable=False, default=None),
+            prop=DateProperty(name="date", required=True, nullable=False, default=None, python_name="date"),
         )
         response_from_data = mocker.patch(
             f"{MODULE_NAME}.response_from_data", side_effect=[(response_1, schemas_1), (response_2, schemas_2)]
@@ -1004,7 +1004,7 @@ def test_from_data_tags_snake_case_sanitizer(self, mocker):
 
         path_1_put = oai.Operation.construct()
         path_1_post = oai.Operation.construct(tags=["AMF Subscription Info (Document)", "tag_3"])
-        path_2_get = oai.Operation.construct()
+        path_2_get = oai.Operation.construct(tags=["3. ABC"])
         data = {
             "path_1": oai.PathItem.construct(post=path_1_post, put=path_1_put),
             "path_2": oai.PathItem.construct(get=path_2_get),
@@ -1039,16 +1039,17 @@ def test_from_data_tags_snake_case_sanitizer(self, mocker):
                     config=config,
                 ),
                 mocker.call(
-                    data=path_2_get, path="path_2", method="get", tag="default", schemas=schemas_2, config=config
+                    data=path_2_get, path="path_2", method="get", tag="tag3_abc", schemas=schemas_2, config=config
                 ),
             ],
         )
         assert result == (
             {
-                "default": EndpointCollection("default", endpoints=[endpoint_1, endpoint_3]),
+                "default": EndpointCollection("default", endpoints=[endpoint_1]),
                 "amf_subscription_info_document": EndpointCollection(
                     "amf_subscription_info_document", endpoints=[endpoint_2]
                 ),
+                "tag3_abc": EndpointCollection("tag3_abc", endpoints=[endpoint_3])
             },
             schemas_3,
         )

tests/test_parser/test_properties/test_init.py

@@ -38,7 +38,7 @@ def test_get_type_string(self, mocker, nullable, required, no_optional, json, ex
 
         mocker.patch.object(Property, "_type_string", "TestType")
         mocker.patch.object(Property, "_json_type_string", "str")
-        p = Property(name="test", required=required, default=None, nullable=nullable)
+        p = Property(name="test", required=required, default=None, nullable=nullable, python_name="test")
         assert p.get_type_string(no_optional=no_optional, json=json) == expected
 
     @pytest.mark.parametrize(

tests/test_parser/test_properties/test_model_property.py

@@ -84,7 +84,13 @@ class TestBuildModelProperty:
             (False, False),
             (
                 oai.Schema.construct(type="string"),
-                StringProperty(name="AdditionalProperty", required=True, nullable=False, default=None),
+                StringProperty(
+                    name="AdditionalProperty",
+                    required=True,
+                    nullable=False,
+                    default=None,
+                    python_name="additional_property",
+                ),
             ),
         ],
     )

tests/test_parser/test_properties/test_property.py

@@ -0,0 +1,18 @@
+from openapi_python_client.parser.properties.property import to_valid_python_identifier
+
+
+class TestToValidPythonIdentifier:
+    def test_valid_identifier_is_not_changed(self):
+        assert to_valid_python_identifier("valid_field", prefix="field") == "valid_field"
+
+    def test_numbers_are_prefixed(self):
+        assert to_valid_python_identifier("1", prefix="field") == "field1"
+
+    def test_invalid_symbols_are_stripped(self):
+        assert to_valid_python_identifier("$abc", "prefix") == "abc"
+
+    def test_keywords_are_postfixed(self):
+        assert to_valid_python_identifier("for", "prefix") == "for_"
+
+    def test_empty_is_prefixed(self):
+        assert to_valid_python_identifier("", "something") == "something"

tests/test_templates/test_property_templates/test_date_property/test_date_property.py

@@ -2,16 +2,21 @@
 
 import jinja2
 
+from openapi_python_client.parser.properties import DateProperty
 
-def test_required_not_nullable():
-    from openapi_python_client.parser.properties import DateProperty
 
-    prop = DateProperty(
+def date_property(required=True, nullable=True, default=None) -> DateProperty:
+    return DateProperty(
         name="a_prop",
-        required=True,
-        nullable=False,
-        default=None,
+        required=required,
+        nullable=nullable,
+        default=default,
+        python_name="a_prop",
     )
+
+
+def test_required_not_nullable():
+    prop = date_property(nullable=False)
     here = Path(__file__).parent
     templates_dir = here.parent.parent.parent.parent / "openapi_python_client" / "templates"
 
@@ -28,14 +33,8 @@ def test_required_not_nullable():
 
 
 def test_required_nullable():
-    from openapi_python_client.parser.properties import DateProperty
 
-    prop = DateProperty(
-        name="a_prop",
-        required=True,
-        nullable=True,
-        default=None,
-    )
+    prop = date_property()
     here = Path(__file__).parent
     templates_dir = here.parent.parent.parent.parent / "openapi_python_client" / "templates"
 
@@ -52,14 +51,7 @@ def test_required_nullable():
 
 
 def test_optional_nullable():
-    from openapi_python_client.parser.properties import DateProperty
-
-    prop = DateProperty(
-        name="a_prop",
-        required=False,
-        nullable=True,
-        default=None,
-    )
+    prop = date_property(required=False)
     here = Path(__file__).parent
     templates_dir = here.parent.parent.parent.parent / "openapi_python_client" / "templates"
 

tests/test_utils.py

@@ -76,13 +76,6 @@ def test__fix_reserved_words(reserved_word: str, expected: str):
     assert utils.fix_reserved_words(reserved_word) == expected
 
 
-def test_to_valid_python_identifier():
-    assert utils.to_valid_python_identifier("valid") == "valid"
-    assert utils.to_valid_python_identifier("1") == "field_1"
-    assert utils.to_valid_python_identifier("$") == "field_"
-    assert utils.to_valid_python_identifier("for") == "for_"
-
-
 @pytest.mark.parametrize(
     "before, after",
     [