Derived route names that collide get _2, _3 suffixes (#28)

http2py/api_pkg_maker.py

@@ -84,8 +84,9 @@ def {func_name}(*args, **kwargs):
     return api.routes[{func_name!r}](*args, **kwargs)
 """
 
-# The first http2py release with ``HttpClient.routes``, which generated packages call.
-MIN_HTTP2PY_VERSION = "0.1.37"
+# The first http2py release whose route names are what generated packages call:
+# ``HttpClient.routes``, with names unique among routes (``get_a_b_2``) and ``get_root``.
+MIN_HTTP2PY_VERSION = "0.1.40"
 
 SETUP_PY = """from setuptools import setup
 

http2py/client.py

@@ -13,6 +13,7 @@
 from http2py.authentication import mk_auth, DFLT_CONFIG_FILENAME
 from http2py.py2request import (
     DFLT_BINARY_DUMPS,
+    _route_name,
     mk_method_spec_from_openapi_method_spec,
     mk_request_function,
 )
@@ -112,6 +113,19 @@ def __init__(
         if self.refresh_inputs is None:
             raise ValueError("No refresh inputs provided when instantiating HttpClient")
         self.routes = {}
+        # Names derived from the spec must avoid every explicit name, and a suffixed
+        # name must not take the natural name of a route that comes later.
+        self._explicit_route_names, self._natural_route_names = set(), set()
+        for pathname, path_spec in openapi_spec["paths"].items():
+            for http_method, spec in path_spec.items():
+                if not isinstance(spec, dict):
+                    continue
+                if spec.get("x-method_name"):
+                    self._explicit_route_names.add(spec["x-method_name"])
+                else:
+                    self._natural_route_names.add(
+                        _route_name(spec, http_method, pathname, self.base_url + pathname)
+                    )
         for pathname, path_spec in openapi_spec["paths"].items():
             url_template = self.base_url + pathname
             for http_method, openapi_method_spec in path_spec.items():
@@ -160,6 +174,10 @@ def register_method(
             content_type=content_type,
             pathname=pathname,
         )
+        if not openapi_method_spec.get("x-method_name"):
+            method_spec["method_name"] = self._unique_derived_name(
+                method_spec["method_name"], url_template
+            )
         func = mk_request_function(
             method_spec,
             dispatch=self.handle_request,
@@ -191,6 +209,30 @@ def register_method(
                 stacklevel=3,
             )
 
+    def _unique_derived_name(self, name, url_template):
+        """``name``, a name derived from the spec (operationId, or method and path),
+        made unique among the routes so far and the explicit ``x-method_name`` ones: the
+        first route keeps it, later ones get ``_2``, ``_3``, ... in spec order (so
+        a route added before another with the same name renames the other; give
+        routes an ``x-method_name`` to pin their names). Explicit names are never
+        changed. A client that skips ``__init__`` knows no explicit names yet."""
+        taken = set(getattr(self, "routes", ())) | getattr(
+            self, "_explicit_route_names", set()
+        )
+        if name not in taken:
+            return name
+        taken |= getattr(self, "_natural_route_names", set())
+        n = 2
+        while f"{name}_{n}" in taken:
+            n += 1
+        unique = f"{name}_{n}"
+        warnings.warn(
+            f"Another route is already named {name!r}: {url_template!r} is named "
+            f"{unique!r} (give each route its own x-method_name to choose).",
+            stacklevel=4,
+        )
+        return unique
+
     def _can_be_route_attribute(self, name):
         """Whether a route called ``name`` may be set as an attribute of the client:
         the name is public, and is not an attribute the client works with already

http2py/py2request.py

@@ -673,7 +673,8 @@ def _route_name(openapi_method_spec, method, pathname, url_template):
     name = _identifier(openapi_method_spec.get("operationId") or "")
     if pathname is None:
         pathname = urlparse(url_template).path
-    return name or default_path_to_func_name(method, pathname)
+    # the root path would be named after the bare method (``get``)
+    return name or default_path_to_func_name(method, pathname.strip("/") or "root")
 
 
 def mk_method_spec_from_openapi_method_spec(

tests/test_route_namespace.py

@@ -174,3 +174,99 @@ def test_a_second_route_of_the_same_name_replaces_the_first_with_a_warning():
     name = other["get"]["x-method_name"]
     assert client.routes[name].method_spec["url_template"].endswith("/plain2")
     assert getattr(client, name) is client.routes[name]
+
+
+def _spec_with_routes(*routes):
+    """A spec whose routes are ``(path, operationId or None, x-method_name or None)``."""
+    spec = copy.deepcopy(SPEC)
+    template = spec["paths"]["/plain"]["get"]
+    spec["paths"] = {}
+    for path, operation_id, method_name in routes:
+        op = copy.deepcopy(template)
+        op.pop("x-method_name", None)
+        if operation_id:
+            op["operationId"] = operation_id
+        if method_name:
+            op["x-method_name"] = method_name
+        spec["paths"][path] = {"get": op}
+    return spec
+
+
+def test_derived_names_that_collide_are_suffixed_in_spec_order():
+    # i2mint/http2py#28
+    spec = _spec_with_routes(
+        ("/a-b", None, None),
+        ("/a_b", None, None),
+        ("/a/b", None, None),
+        ("/u/{id}", None, None),
+        ("/u/id", None, None),
+        ("/x", "list-things", None),
+        ("/y", "list_things", None),
+        ("/z", "get_zz", None),
+        ("/zz", None, None),
+    )
+    with pytest.warns(UserWarning, match="get_a_b_2"):
+        client = _client(spec)
+    assert list(client.routes) == [
+        "get_a_b",
+        "get_a_b_2",
+        "get_a_b_3",
+        "get_u_id",
+        "get_u_id_2",
+        "list_things",
+        "list_things_2",
+        "get_zz",
+        "get_zz_2",
+    ]
+    for name, route in client.routes.items():
+        assert route.__name__ == name and getattr(client, name) is route
+
+
+def test_explicit_names_are_never_suffixed_and_keep_their_name_over_derived():
+    spec = _spec_with_routes(
+        ("/a-b", None, None),  # derived get_a_b, but an explicit one claims it
+        ("/other", None, "get_a_b"),
+    )
+    client = _client(spec)
+    assert list(client.routes) == ["get_a_b_2", "get_a_b"]
+    assert client.routes["get_a_b"].method_spec["url_template"].endswith("/other")
+
+
+def test_two_explicit_names_that_collide_still_warn_and_the_last_wins():
+    spec = _spec_with_routes(("/p", None, "same"), ("/q", None, "same"))
+    with pytest.warns(UserWarning, match="Two routes are named 'same'"):
+        client = _client(spec)
+    assert list(client.routes) == ["same"]
+
+
+def test_api_pkg_files_keeps_every_colliding_route():
+    spec = _spec_with_routes(("/a-b", None, None), ("/a_b", None, None))
+    with pytest.warns(UserWarning):
+        files = api_pkg_files(spec, pkg_name="genpkg_d")
+    funcs = files["genpkg_d/funcs.py"]
+    assert "def get_a_b(" in funcs and "def get_a_b_2(" in funcs
+
+
+def test_root_path_is_not_named_after_the_bare_method():
+    client = _client(_spec_with_routes(("/", None, None)))
+    assert list(client.routes) == ["get_root"]
+
+
+def test_a_suffix_does_not_take_the_natural_name_of_a_later_route():
+    client = _client(
+        _spec_with_routes(("/a-b", None, None), ("/a_b", None, None), ("/a_b_2", None, None))
+    )
+    assert list(client.routes) == ["get_a_b", "get_a_b_3", "get_a_b_2"]
+    assert client.routes["get_a_b_2"].method_spec["url_template"].endswith("/a_b_2")
+
+
+def test_root_path_and_a_root_route_are_told_apart():
+    client = _client(_spec_with_routes(("/root", None, None), ("/", None, None)))
+    assert sorted(client.routes) == ["get_root", "get_root_2"]
+
+
+def test_openapi_to_py_keeps_the_bare_root_name():
+    from http2py.py2request import openapi_to_py
+
+    spec = _spec_with_routes(("/", None, None), ("/root", None, None))
+    assert sorted(openapi_to_py(spec)) == ["get", "get_root"]