From 9a27db86f5d79369f602f95de432c05f6531030b Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 13 Oct 2026 05:24:02 +0000 Subject: [PATCH 1/2] fix: routes live in client.routes; a route name cannot replace a client attribute (#23) register_method did setattr(self, name, ...) with the spec's x-method_name, so a (possibly remote) spec could replace openapi_spec, session, handle_request... Every route now goes in client.routes; it is also set as an attribute only when its name is public and not already an attribute (a later route of the same name still replaces an earlier one). Otherwise a warning points to client.routes. api_pkg_maker and cli_maker enumerate client.routes, and generated packages call api.routes[name], so a route named like a client attribute still works there. Co-Authored-By: Claude Opus 5.5 --- http2py/api_pkg_maker.py | 10 ++---- http2py/cli_maker.py | 3 +- http2py/client.py | 36 +++++++++++++++++++-- tests/test_route_namespace.py | 61 +++++++++++++++++++++++++++++++++++ 4 files changed, 99 insertions(+), 11 deletions(-) create mode 100644 tests/test_route_namespace.py diff --git a/http2py/api_pkg_maker.py b/http2py/api_pkg_maker.py index 206dcbc..de402e3 100644 --- a/http2py/api_pkg_maker.py +++ b/http2py/api_pkg_maker.py @@ -79,9 +79,9 @@ def wrapper(func): FUNC_TPL = """ -@_apply_sig(api.{func_name}) +@_apply_sig(api.routes[{func_name!r}]) def {func_name}(*args, **kwargs): - return api.{func_name}(*args, **kwargs) + return api.routes[{func_name!r}](*args, **kwargs) """ SETUP_PY = """from setuptools import setup @@ -158,11 +158,7 @@ def object_ref(obj: Union[str, Callable]) -> str: def _route_names(api: HttpClient) -> list: """The method names ``api`` registered, each a plain identifier, since each is written into generated source.""" - names = [ - name - for name, method in vars(api).items() - if getattr(method, "method_spec", None) - ] + names = list(api.routes) for name in names: if not _is_plain_identifier(name): raise ValueError( diff --git a/http2py/cli_maker.py b/http2py/cli_maker.py index 7daeb97..bf6261a 100644 --- a/http2py/cli_maker.py +++ b/http2py/cli_maker.py @@ -143,8 +143,7 @@ def mk_cli( config_filename, binary_loads=binary_loads, ) - for methodname, method in client_details.__dict__.items() - if getattr(method, "method_spec", None) + for method in client_details.routes.values() ] return cw.mk_parser(cli_methods, convention=CLI_CONVENTION) diff --git a/http2py/client.py b/http2py/client.py index 25a2602..7471853 100644 --- a/http2py/client.py +++ b/http2py/client.py @@ -4,6 +4,8 @@ and exposes each declared route as a normal python method. """ +import warnings + from glom import glom from requests import request, get, Session from i2.errors import AuthorizationError @@ -22,6 +24,13 @@ class HttpClient: """ A client class meant as an interface to an HTTP service with one or more routes defined with an OpenAPI spec. + + Every route is in ``client.routes`` (a ``{name: method}`` dict), and is also an + attribute of the client (``client.``) as long as its name neither starts + with ``_`` nor is already an attribute of the client: a spec, possibly served by + a remote, must not be able to replace the client's own attributes (such as + ``session`` or ``handle_request``). A route that cannot be an attribute is still + reachable as ``client.routes[name]``, and a warning says so. """ auth_type = "" @@ -31,6 +40,7 @@ class HttpClient: refresh_url = "" refresh_input_keys = [] session = None + login_args = None # set by init_security; declared so no route can take the name def __init__( self, @@ -96,6 +106,7 @@ def __init__( self.refresh_inputs = session_state.get("refresh_inputs") if self.refresh_inputs is None: raise ValueError("No refresh inputs provided when instantiating HttpClient") + self.routes = {} 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(): @@ -149,7 +160,28 @@ def register_method(self, url_template, http_method, openapi_method_spec): func.method_spec = method_spec func.content_type = content_type funcname = func.__name__ - setattr(self, funcname, func.__get__(self)) + method = func.__get__(self) + can_be_attribute = self._can_be_route_attribute(funcname) + self.routes[funcname] = method + if can_be_attribute: + setattr(self, funcname, method) + else: + warnings.warn( + f"Route {funcname!r} is not set as an attribute of the client: the name " + f"starts with '_' or is already an attribute of {type(self).__name__}. " + f"Call it as client.routes[{funcname!r}](...).", + stacklevel=2, + ) + + 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 already (unless that attribute is + an earlier route of the same name, which a later route replaces).""" + if name.startswith("_"): + return False + if name in self.routes: + return vars(self).get(name) is self.routes[name] + return not hasattr(self, name) def handle_request(self, method, url, **_request_kwargs): self.ensure_login() @@ -211,5 +243,5 @@ def set_profile(self, profile: str, config: str = DFLT_CONFIG_FILENAME): self._login() def is_request_method(self, methodname): - method = getattr(self, methodname) + method = self.routes.get(methodname) or getattr(self, methodname) return method and hasattr(method, "method_spec") diff --git a/tests/test_route_namespace.py b/tests/test_route_namespace.py new file mode 100644 index 0000000..a531d3e --- /dev/null +++ b/tests/test_route_namespace.py @@ -0,0 +1,61 @@ +"""A route's name cannot replace an attribute of ``HttpClient`` (i2mint/http2py#23): +every route is in ``client.routes``, and only names that collide with nothing are +also set as attributes.""" + +from __future__ import annotations + +import copy +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent)) + +from spec_fixture import SPEC # noqa: E402 + +from http2py import HttpClient # noqa: E402 +from http2py.api_pkg_maker import api_pkg_files # noqa: E402 + + +def _spec_naming_plain(name): + spec = copy.deepcopy(SPEC) + spec["paths"]["/plain"]["get"]["x-method_name"] = name + return spec + + +def _client(spec): + return HttpClient(spec, session_state={"session": object(), "refresh_inputs": {}}) + + +@pytest.mark.parametrize( + "name", + ["openapi_spec", "session", "handle_request", "ensure_login", "binary_loads"] + + ["routes", "login_args", "title", "_private"], +) +def test_colliding_route_name_keeps_the_client_attribute(name): + spec = _spec_naming_plain(name) + with pytest.warns(UserWarning, match=f"client.routes\\[{name!r}\\]"): + client = _client(spec) + route = client.routes[name] + assert getattr(route, "method_spec", None) + assert getattr(client, name, None) is not route + assert client.openapi_spec is spec + assert callable(client.handle_request) and callable(client.ensure_login) + + +def test_non_colliding_routes_are_attributes_too(): + client = _client(SPEC) + assert set(client.routes) == {"get_thing", "make", "plain"} + for name, route in client.routes.items(): + assert getattr(client, name) is route + assert client.is_request_method(name) + + +def test_api_pkg_for_a_route_named_like_a_client_attribute(): + # used to crash with "'function' object has no attribute 'get'" + with pytest.warns(UserWarning): + files = api_pkg_files(_spec_naming_plain("openapi_spec"), pkg_name="genpkg_c") + funcs = files["genpkg_c/funcs.py"] + assert "def openapi_spec(" in funcs + assert "api.routes['openapi_spec']" in funcs From 76857d2d9337770a36a0e514b01de97189078c54 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 13 Oct 2026 05:27:49 +0000 Subject: [PATCH 2/2] fix: address adversarial review of #23 - the CLI calls client.routes[name], not getattr (a route named like a client attribute ran the attribute) - title/version/base_url (never read after __init__) can still be route names, so existing clients and generated packages with such routes keep working - collisions are checked on the class and instance dict, not hasattr (a __getattr__ subclass refused every route); routes is created lazily for a subclass that skips HttpClient.__init__ - duplicate route names warn; warnings point at the caller (stacklevel=3); is_request_method looks at the attribute again Co-Authored-By: Claude Opus 5.5 --- http2py/cli_maker.py | 2 +- http2py/client.py | 38 ++++++++++++++++++-------- tests/test_generated_clients.py | 1 + tests/test_route_namespace.py | 47 ++++++++++++++++++++++++++++++++- 4 files changed, 75 insertions(+), 13 deletions(-) diff --git a/http2py/cli_maker.py b/http2py/cli_maker.py index bf6261a..2ff78e6 100644 --- a/http2py/cli_maker.py +++ b/http2py/cli_maker.py @@ -207,7 +207,7 @@ def cli_method(*args, **kwargs): auth_kwargs, expected_auth_kwargs, config_filename, profile ) http_client = HttpClient(openapi_spec, binary_loads=binary_loads, **auth_kwargs) - return getattr(http_client, methodname)(**kwargs) + return http_client.routes[methodname](**kwargs) method_sig.wrap(cli_method) cli_method.__name__ = methodname diff --git a/http2py/client.py b/http2py/client.py index 7471853..9df09ae 100644 --- a/http2py/client.py +++ b/http2py/client.py @@ -27,10 +27,12 @@ class HttpClient: Every route is in ``client.routes`` (a ``{name: method}`` dict), and is also an attribute of the client (``client.``) as long as its name neither starts - with ``_`` nor is already an attribute of the client: a spec, possibly served by - a remote, must not be able to replace the client's own attributes (such as - ``session`` or ``handle_request``). A route that cannot be an attribute is still - reachable as ``client.routes[name]``, and a warning says so. + with ``_`` nor is already an attribute the client works with: a spec, possibly + served by a remote, must not be able to replace the client's own attributes (such + as ``session`` or ``handle_request``). A route that cannot be an attribute is + still reachable as ``client.routes[name]``, and a warning says so. The purely + informational attributes (``title``, ``version``, ``base_url``), which the client + never reads after ``__init__``, can be taken by a route. """ auth_type = "" @@ -41,6 +43,9 @@ class HttpClient: refresh_input_keys = [] session = None login_args = None # set by init_security; declared so no route can take the name + login_response_keys = () # idem + # Set by __init__ but never read afterwards, so a route may take these names. + _route_shadowable_attrs = frozenset({"title", "version", "base_url"}) def __init__( self, @@ -161,6 +166,14 @@ def register_method(self, url_template, http_method, openapi_method_spec): func.content_type = content_type funcname = func.__name__ method = func.__get__(self) + if "routes" not in vars(self): # e.g. a subclass that skips HttpClient.__init__ + self.routes = {} + if funcname in self.routes: + warnings.warn( + f"Two routes are named {funcname!r} ({url_template!r} replaces the " + "earlier one): give each its own x-method_name.", + stacklevel=3, + ) can_be_attribute = self._can_be_route_attribute(funcname) self.routes[funcname] = method if can_be_attribute: @@ -170,18 +183,21 @@ def register_method(self, url_template, http_method, openapi_method_spec): f"Route {funcname!r} is not set as an attribute of the client: the name " f"starts with '_' or is already an attribute of {type(self).__name__}. " f"Call it as client.routes[{funcname!r}](...).", - stacklevel=2, + stacklevel=3, ) 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 already (unless that attribute is - an earlier route of the same name, which a later route replaces).""" + the name is public, and is not an attribute the client works with already + (an earlier route of the same name, or a ``_route_shadowable_attrs`` one, can + be replaced).""" if name.startswith("_"): return False - if name in self.routes: - return vars(self).get(name) is self.routes[name] - return not hasattr(self, name) + if name in self.routes and vars(self).get(name) is self.routes[name]: + return True + if name in self._route_shadowable_attrs: + return True + return not (hasattr(type(self), name) or name in vars(self)) def handle_request(self, method, url, **_request_kwargs): self.ensure_login() @@ -243,5 +259,5 @@ def set_profile(self, profile: str, config: str = DFLT_CONFIG_FILENAME): self._login() def is_request_method(self, methodname): - method = self.routes.get(methodname) or getattr(self, methodname) + method = getattr(self, methodname) return method and hasattr(method, "method_spec") diff --git a/tests/test_generated_clients.py b/tests/test_generated_clients.py index a4c5afa..9d9c515 100644 --- a/tests/test_generated_clients.py +++ b/tests/test_generated_clients.py @@ -125,6 +125,7 @@ def test_dispatch_cli_passes_binary_loads_to_the_client(monkeypatch, tmp_path): class FakeClient: def __init__(self, openapi_spec, **kwargs): seen.update(kwargs) + self.routes = {"plain": self.plain} def plain(self, **kwargs): return "ran" diff --git a/tests/test_route_namespace.py b/tests/test_route_namespace.py index a531d3e..9dddc08 100644 --- a/tests/test_route_namespace.py +++ b/tests/test_route_namespace.py @@ -31,7 +31,7 @@ def _client(spec): @pytest.mark.parametrize( "name", ["openapi_spec", "session", "handle_request", "ensure_login", "binary_loads"] - + ["routes", "login_args", "title", "_private"], + + ["routes", "login_args", "login_response_keys", "verify_cert", "_private"], ) def test_colliding_route_name_keeps_the_client_attribute(name): spec = _spec_naming_plain(name) @@ -59,3 +59,48 @@ def test_api_pkg_for_a_route_named_like_a_client_attribute(): funcs = files["genpkg_c/funcs.py"] assert "def openapi_spec(" in funcs assert "api.routes['openapi_spec']" in funcs + + +@pytest.mark.parametrize("name", ["title", "version", "base_url"]) +def test_informational_attributes_can_be_route_names(name): + import warnings + + with warnings.catch_warnings(): + warnings.simplefilter("error") + client = _client(_spec_naming_plain(name)) + assert getattr(client, name) is client.routes[name] + + +def test_duplicate_route_names_warn(): + with pytest.warns(UserWarning, match="Two routes are named 'get_thing'"): + client = _client(_spec_naming_plain("get_thing")) + assert set(client.routes) == {"get_thing", "make"} + + +def test_cli_calls_the_route_not_the_client_attribute(monkeypatch): + from http2py import cli_maker + + spec = _spec_naming_plain("ensure_login") + with pytest.warns(UserWarning): + route = _client(spec).routes["ensure_login"] + class Sent(Exception): + pass + + def handle_request(self, method, url, **kw): + raise Sent(url) + + monkeypatch.setattr(HttpClient, "handle_request", handle_request) + with pytest.warns(UserWarning), pytest.raises(Sent, match="/plain$"): + cli_method = cli_maker.register_cli_method(spec, route, []) + cli_method(config="no-such-config", api_key="k") + + +def test_subclass_skipping_init_can_register_routes(): + class Bare(HttpClient): + def __init__(self): + self.verify_cert, self.binary_loads, self.binary_dumps = True, None, None + + bare = Bare() + spec = SPEC["paths"]["/plain"]["get"] + bare.register_method("https://example.com/plain", "get", spec) + assert bare.routes["plain"] is bare.plain