diff --git a/glpi_python_client/auth/_v1_session.py b/glpi_python_client/auth/_v1_session.py index 96c013e..fcf0478 100644 --- a/glpi_python_client/auth/_v1_session.py +++ b/glpi_python_client/auth/_v1_session.py @@ -47,6 +47,7 @@ GlpiServerError, GlpiValidationError, ) +from glpi_python_client.clients.commons._config import build_http_session from glpi_python_client.clients.commons._http import ( ensure_response_status, finalize_request_response, @@ -95,8 +96,10 @@ def __init__( seconds=session_refresh_interval_seconds ) - self._http = requests.Session() - self._http.verify = verify_ssl + # Built through the shared factory so the SSL policy is applied at + # construction: httpx reads ``verify`` only in ``Client.__init__`` + # and silently ignores a later assignment. + self._http = build_http_session(verify_ssl=verify_ssl) self._session_token: str | None = None self._session_started_at: datetime | None = None @@ -236,11 +239,11 @@ def _authenticated_request( """ request_headers = {**self._headers(), **(headers or {})} - request_method = getattr(self._http, method.lower()) - response = cast( - requests.Response, - request_method(url, headers=request_headers, **kwargs), - ) + # Dispatch through ``request(method, ...)`` rather than looking up a + # per-verb attribute: it is the one call shape both transports share, + # and it keeps the verb a value instead of an attribute name. + verb = method.upper() + response = self._http.request(verb, url, headers=request_headers, **kwargs) if _is_auth_failure_response(response): logger.warning( "GLPI v1 session token was rejected; refreshing session and " @@ -248,10 +251,7 @@ def _authenticated_request( ) self._renew_session() request_headers = {**self._headers(), **(headers or {})} - response = cast( - requests.Response, - request_method(url, headers=request_headers, **kwargs), - ) + response = self._http.request(verb, url, headers=request_headers, **kwargs) return finalize_request_response( response, method=method, diff --git a/glpi_python_client/auth/tests/test_v1_session.py b/glpi_python_client/auth/tests/test_v1_session.py index 2cad8bc..98cacf9 100644 --- a/glpi_python_client/auth/tests/test_v1_session.py +++ b/glpi_python_client/auth/tests/test_v1_session.py @@ -32,6 +32,34 @@ def __init__(self, responses: dict[str, list[FakeResponse]]) -> None: def _next(self, key: str) -> FakeResponse: return self._responses[key].pop(0) + def request( + self, + method: str, + url: str, + headers: dict[str, str], + timeout: int = 30, + **kwargs: Any, + ) -> FakeResponse: + """Verb-agnostic entry point mirroring the real dispatch. + + ``GLPIV1Session`` routes authenticated calls through + ``session.request(method, ...)`` rather than a per-verb attribute, + because that is the one call shape ``requests`` and ``httpx`` share. + This dispatches back to the per-verb handlers so their recorded + call shapes stay identical. + """ + + verb = method.upper() + handler = { + "GET": self.get, + "POST": self.post, + "PUT": self.put, + "DELETE": self.delete, + }.get(verb) + if handler is None: + raise AssertionError(f"unexpected verb {verb!r} in fake v1 session") + return handler(url, headers=headers, timeout=timeout, **kwargs) + def get( self, url: str, diff --git a/glpi_python_client/clients/commons/_config.py b/glpi_python_client/clients/commons/_config.py index 3b417bd..ef33a7e 100644 --- a/glpi_python_client/clients/commons/_config.py +++ b/glpi_python_client/clients/commons/_config.py @@ -10,7 +10,7 @@ from collections.abc import Mapping from dataclasses import dataclass -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Protocol import requests import urllib3 @@ -22,6 +22,19 @@ from glpi_python_client.auth.auth import GLPITokenManager +class SessionFactory(Protocol): + """Callable that builds the transport session for a client. + + Declared as a ``Protocol`` rather than a bare ``Callable`` alias so the + keyword-only ``verify_ssl`` argument is part of the contract: the whole + point of this seam is that the SSL policy is supplied *at construction*, + and a positional-argument factory would let that guarantee slip. + """ + + def __call__(self, *, verify_ssl: bool) -> requests.Session: + """Return a session configured for ``verify_ssl``.""" + + @dataclass(frozen=True) class ClientResources: """Runtime resources owned by one async ``GlpiClient`` instance. @@ -49,6 +62,39 @@ def configure_ssl_warning_policy(*, verify_ssl: bool) -> None: urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) +def build_http_session(*, verify_ssl: bool) -> requests.Session: + """Construct the HTTP session used for every GLPI call. + + This is the single place the library instantiates a transport session, + which makes it the seam the transport swap turns on. Two properties + matter and both are the reason this is a function rather than two inline + lines: + + * ``verify`` is applied **as part of construction**. ``requests`` + tolerates assigning it afterwards; ``httpx`` does not — it reads + ``verify`` only in ``Client.__init__`` and a later assignment is + accepted and silently ignored, leaving certificate verification on + when the caller asked for it off. + * Callers that need to intercept traffic (tests, and anything wanting + ``httpx.MockTransport``) can substitute this factory instead of + monkey-patching a session after the fact. + + Parameters + ---------- + verify_ssl : bool + Whether TLS certificates are verified. + + Returns + ------- + requests.Session + A session configured for the requested SSL policy. + """ + + session = requests.Session() + session.verify = verify_ssl + return session + + def build_client_resources( *, glpi_api_url: object, @@ -62,12 +108,21 @@ def build_client_resources( v1_base_url: str | None, v1_user_token: str | None, v1_app_token: str | None, + session_factory: SessionFactory | None = None, ) -> ClientResources: """Build the shared resources required by one async client instance. The helper validates the API URL, configures SSL behaviour, builds the OAuth token manager, and optionally instantiates the legacy v1 session used solely by the document upload mixin. + + Parameters + ---------- + session_factory : SessionFactory | None, optional + Override for :func:`build_http_session`, called with the resolved + ``verify_ssl`` policy. This is the transport-injection seam: it lets + a caller supply a session wired to a stub transport without patching + module globals. ``None`` uses the default factory. """ from glpi_python_client.auth._v1_session import GLPIV1Session @@ -83,8 +138,8 @@ def build_client_resources( ) configure_ssl_warning_policy(verify_ssl=verify_ssl) - session = requests.Session() - session.verify = verify_ssl + factory = session_factory or build_http_session + session = factory(verify_ssl=verify_ssl) try: auth = GLPITokenManager( token_url=f"{normalized_api_url}/token", diff --git a/glpi_python_client/clients/commons/_http.py b/glpi_python_client/clients/commons/_http.py index a662565..8d2c84d 100644 --- a/glpi_python_client/clients/commons/_http.py +++ b/glpi_python_client/clients/commons/_http.py @@ -20,6 +20,32 @@ from glpi_python_client.clients.commons._constants import RequestParamValue +def response_reason(response: requests.Response) -> str: + """Return one response's HTTP reason phrase, whatever the transport. + + ``requests`` spells this ``Response.reason``; ``httpx`` spells it + ``Response.reason_phrase`` and has no ``reason`` attribute at all. Every + read of the phrase goes through this helper so swapping the transport + touches one function instead of every message that quotes it. + + Parameters + ---------- + response : requests.Response + Response to read the reason phrase from. Typed against the current + transport; any object exposing either attribute works at runtime. + + Returns + ------- + str + The reason phrase, or ``""`` when the transport supplies none. + """ + + reason = getattr(response, "reason", None) + if reason is None: + reason = getattr(response, "reason_phrase", None) + return str(reason) if reason else "" + + def request_params( params: dict[str, object] | None, ) -> dict[str, RequestParamValue] | None: @@ -133,7 +159,7 @@ def finalize_request_response( if 500 <= response.status_code < 600: message = ( f"GLPI {method_name} {url} failed with " - f"{response.status_code} {response.reason}" + f"{response.status_code} {response_reason(response)}" ) logger.warning(message) raise GlpiServerError( diff --git a/glpi_python_client/clients/commons/_transport.py b/glpi_python_client/clients/commons/_transport.py index feb6ed0..60737f8 100644 --- a/glpi_python_client/clients/commons/_transport.py +++ b/glpi_python_client/clients/commons/_transport.py @@ -33,7 +33,7 @@ import logging import threading from collections.abc import Callable -from typing import TYPE_CHECKING, TypeVar, cast +from typing import TYPE_CHECKING, Any, TypeVar import requests from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed @@ -150,16 +150,21 @@ def _send_request( self, method: str, url: str, - **kwargs: object, + **kwargs: Any, ) -> requests.Response: - """Dispatch one blocking ``requests`` call. + """Dispatch one blocking HTTP call. The helper exists as an indirection seam so tests can stub HTTP dispatch without monkey-patching the session attribute directly. + + Dispatch goes through ``session.request(method, url, ...)`` rather + than looking up a per-verb attribute. ``request`` is the one call + shape ``requests`` and ``httpx`` agree on, and keeping the verb a + value rather than an attribute name means the transport swap does + not have to reason about dynamic attribute lookup. """ - request_method = getattr(self._session, method) - return cast(requests.Response, request_method(url, **kwargs)) + return self._session.request(method.upper(), url, **kwargs) def _execute_request( self, diff --git a/glpi_python_client/clients/commons/tests/test_transport.py b/glpi_python_client/clients/commons/tests/test_transport.py index 0cb6d24..a9e0474 100644 --- a/glpi_python_client/clients/commons/tests/test_transport.py +++ b/glpi_python_client/clients/commons/tests/test_transport.py @@ -44,16 +44,59 @@ def test_ensure_token_calls_auth_manager(monkeypatch: pytest.MonkeyPatch) -> Non c.close() -def test_send_request_dispatches_to_matching_session_method( +def test_send_request_dispatches_through_session_request( monkeypatch: pytest.MonkeyPatch, ) -> None: - """``_send_request`` calls the method matching the verb on the session.""" + """``_send_request`` routes through ``session.request`` with an upper verb. + + The verb is passed as a *value* to ``request`` rather than being looked + up as a per-verb attribute: that is the one call shape ``requests`` and + ``httpx`` share, so the transport swap does not have to reason about + dynamic attribute lookup. + """ c = make_client() fake = FakeResponse(status_code=200, payload={}) - monkeypatch.setattr(c._session, "get", lambda url, **kw: fake) - result = c._send_request("get", "https://glpi.example.test/api.php/v2/test") + seen: dict[str, object] = {} + + def _request(method: str, url: str, **kw: object) -> FakeResponse: + seen.update({"method": method, "url": url, "kw": kw}) + return fake + + monkeypatch.setattr(c._session, "request", _request) + result = c._send_request( + "get", "https://glpi.example.test/api.php/v2/test", timeout=30 + ) assert result is fake + assert seen["method"] == "GET" + assert seen["url"] == "https://glpi.example.test/api.php/v2/test" + assert seen["kw"] == {"timeout": 30} + c.close() + + +def test_send_request_does_not_use_per_verb_attributes( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Stubbing only ``session.get`` must no longer intercept a GET. + + This pins the seam: if dispatch ever regresses to + ``getattr(session, verb)`` this test fails, because the per-verb + attribute would be used again. + """ + + c = make_client() + sentinel = FakeResponse(status_code=418, payload={}) + monkeypatch.setattr(c._session, "get", lambda url, **kw: sentinel) + captured: dict[str, object] = {} + + def _request(method: str, url: str, **kw: object) -> FakeResponse: + captured["used"] = True + return FakeResponse(status_code=200, payload={}) + + monkeypatch.setattr(c._session, "request", _request) + result = c._send_request("get", "https://glpi.example.test/api.php/v2/test") + assert captured.get("used") is True + assert result is not sentinel c.close() diff --git a/glpi_python_client/clients/tests/test_method_invocation.py b/glpi_python_client/clients/tests/test_method_invocation.py new file mode 100644 index 0000000..49dc545 --- /dev/null +++ b/glpi_python_client/clients/tests/test_method_invocation.py @@ -0,0 +1,286 @@ +"""Invoke every public client method against a stubbed transport. + +This is the regression net for the transport swap and the unasync codegen +that follow. Existing tests cover endpoints one at a time and assert +payload shapes; this one asserts something different and much cheaper to +keep true: that *every* public method on ``GlpiClient`` still reaches the +transport and returns without blowing up, and that ``AsyncGlpiClient`` +exposes the same surface with each member awaitable. + +Why it matters: when the transport is replaced and, later, when the sync +tree is generated from the async one, the failure mode is not a subtly +wrong payload -- it is a method that no longer dispatches at all, or that +raises ``TypeError`` the moment it is called. A per-endpoint suite catches +that only where a test happens to exist. This catches it everywhere, and +it fails loudly on a method that was added without any coverage. + +The stub sits at the ``session.request`` seam, so the real URL building, +header assembly, parameter normalisation, response validation and model +parsing all execute. Only the socket is replaced. +""" + +from __future__ import annotations + +import asyncio +import inspect +from datetime import datetime, timedelta, timezone +from typing import Any, get_type_hints + +import pytest + +from glpi_python_client import AsyncGlpiClient, GlpiClient +from glpi_python_client.testing.utils import make_async_client, make_client + +#: Methods that deliberately never reach the transport, with the reason. +#: Anything else that makes zero HTTP calls is a wiring failure. +_NO_TRANSPORT: dict[str, str] = { + "close": "releases local resources only", + "from_env": "classmethod constructor, performs no I/O", + # Every selector is an optional keyword, so a no-argument call is the + # documented "no criteria" case: it raises GlpiValidationError before + # any request leaves the process. + "get_user_activity": "validates that at least one selector is given first", +} + + +def _public_methods(cls: type) -> list[str]: + """Return every public callable defined on ``cls`` or its mixins.""" + + return sorted( + name + for name in dir(cls) + if not name.startswith("_") and callable(getattr(cls, name, None)) + ) + + +def _payload_for(endpoint: str) -> Any: + """Return a permissive payload that satisfies most GLPI models. + + Collection endpoints answer with a list and item endpoints with an + object, so the stub picks by shape of the request rather than trying to + special-case each of the 85 methods. + """ + + return { + "id": 1, + "name": "stub", + "content": "
stub
", + "username": "stub", + "comment": "stub", + "revision": 1, + "duration": 0, + "tickets_id": 1, + } + + +class _StubResponse: + """Minimal response object covering what the transport layer reads.""" + + def __init__(self, payload: Any, status_code: int = 200) -> None: + self._payload = payload + self.status_code = status_code + self.headers: dict[str, str] = {"Content-Range": "0-0/1"} + self.text = "{}" + self.reason = "OK" + self.url = "https://glpi.example.test/api.php/v2/stub" + self.content = b"{}" + + def json(self) -> Any: + return self._payload + + +class _StubV1: + """Stand-in for the legacy v1 session, logging into the shared call log.""" + + def __init__(self, calls: list[str]) -> None: + self._calls = calls + + def request_json(self, method: str, path: str, **kwargs: Any) -> Any: + self._calls.append(f"v1 {method} {path}") + # Both shapes the v1 callers expect: a list for collection reads and + # a dict carrying an id for writes. + return [{"id": 1, "name": "stub", "itemtypes": '["Ticket"]'}] + + def upload_document(self, *args: Any, **kwargs: Any) -> int: + self._calls.append("v1 POST Document") + return 1 + + def close(self) -> None: + """No-op; the real session is closed with the client.""" + + +def _install_stub(client: GlpiClient | AsyncGlpiClient) -> list[str]: + """Route every HTTP call to an in-memory stub; return the call log.""" + + calls: list[str] = [] + + def _request(method: str, url: str, **kwargs: Any) -> _StubResponse: + calls.append(f"{method} {url}") + # List endpoints must see a list; single-item endpoints an object. + # GLPI collection paths are the ones the client pages over, and they + # are exactly those called without a trailing numeric id. + tail = url.rstrip("/").rsplit("/", 1)[-1] + payload: Any = _payload_for(url) + if not tail.isdigit() and method.upper() == "GET": + payload = [payload] + return _StubResponse(payload) + + client._session.request = _request # type: ignore[method-assign,union-attr] + # Pretend a valid, non-expiring token is already held so no OAuth round + # trip happens and the call log contains only endpoint traffic. + client._auth.access_token = "stub-token" + client._auth.token_expires_at = datetime.now(tz=timezone.utc) + timedelta(days=365) + # Several features (plugin fields, KB category writes, document upload, + # actor statistics) run on the legacy v1 session rather than the v2 + # transport. Stub it into the same log so they are exercised too. + client._v1 = _StubV1(calls) # type: ignore[assignment] + return calls + + +def _argument_for(name: str, annotation: Any) -> Any: + """Synthesize one plausible argument from a parameter's annotation. + + Container checks come first: ``list[int]`` contains the substring + ``int``, so testing scalars first would hand a bare ``1`` to a + parameter expecting a sequence. Containers are also returned non-empty, + because several methods documentedly short-circuit on an empty + collection and would then make no request at all. + """ + + # Pydantic request bodies first -- they are classes, not typing text. + if hasattr(annotation, "model_fields"): + try: + return annotation() + except Exception: + return annotation.model_construct() + + text = str(annotation) + if "list" in text or "Sequence" in text or "tuple" in text: + return [1] if "int" in text else ["stub"] + if "dict" in text or "Mapping" in text: + return {"stub": "stub"} + if "bool" in text: + return False + if "str" in text: + return "stub" + if "int" in text or "float" in text: + return 1 + return 1 + + +def _build_call_args(method: Any) -> dict[str, Any] | None: + """Return kwargs for ``method``, or ``None`` when it cannot be synthesized.""" + + try: + signature = inspect.signature(method) + hints = get_type_hints(method) + except Exception: + return None + + kwargs: dict[str, Any] = {} + for name, parameter in signature.parameters.items(): + if name in {"self", "cls"}: + continue + if parameter.kind in { + inspect.Parameter.VAR_POSITIONAL, + inspect.Parameter.VAR_KEYWORD, + }: + continue + if parameter.default is not inspect.Parameter.empty: + continue + annotation = hints.get(name, parameter.annotation) + try: + kwargs[name] = _argument_for(name, annotation) + except Exception: + return None + return kwargs + + +SYNC_METHODS = _public_methods(GlpiClient) + + +def test_the_public_surface_has_not_silently_shrunk() -> None: + """The client still exposes its full documented method surface. + + Pinned as a lower bound rather than an exact number so adding an + endpoint does not fail the suite, while a codegen step that drops + methods on the floor does. + """ + + assert len(SYNC_METHODS) >= 85, ( + f"GlpiClient exposes {len(SYNC_METHODS)} public methods; expected at " + "least 85. A drop here means the generated surface lost methods." + ) + + +def test_async_client_mirrors_every_sync_method() -> None: + """``AsyncGlpiClient`` exposes the same names, all awaitable.""" + + async_methods = set(_public_methods(AsyncGlpiClient)) + missing = set(SYNC_METHODS) - async_methods + assert not missing, f"AsyncGlpiClient is missing: {sorted(missing)}" + + +@pytest.mark.parametrize("method_name", SYNC_METHODS) +def test_every_public_method_reaches_the_transport(method_name: str) -> None: + """Each public method dispatches an HTTP call and returns a value. + + Failures here mean the method is no longer wired to the transport -- + the exact breakage a transport swap or a codegen regression produces. + """ + + client = make_client() + calls = _install_stub(client) + try: + method = getattr(client, method_name) + kwargs = _build_call_args(method) + if kwargs is None: + pytest.skip(f"{method_name}: arguments could not be synthesized") + + try: + result = method(**kwargs) + # Generators are lazy -- nothing is dispatched until consumed. + if inspect.isgenerator(result): + for _ in result: + break + except (TypeError, NotImplementedError) as exc: + pytest.fail(f"{method_name} is not callable as declared: {exc!r}") + except Exception: + # Validating the generic stub payload against 40-odd different + # models is not what this test is about; reaching the transport + # is. A payload-shape error still proves the method dispatched. + pass + + if method_name in _NO_TRANSPORT: + return + assert calls, ( + f"{method_name} made no HTTP call. Either it is not wired to the " + f"transport, or it belongs in _NO_TRANSPORT with a reason." + ) + finally: + client.close() + + +@pytest.mark.parametrize("method_name", ["get_ticket", "search_tickets", "get_user"]) +def test_async_methods_reach_the_transport(method_name: str) -> None: + """A representative async slice dispatches through the same seam. + + The bridge (and, after the codegen step, the generated async tree) must + reach the transport exactly as the sync client does. + """ + + async def _run() -> list[str]: + client = make_async_client() + calls = _install_stub(client) + try: + method = getattr(client, method_name) + kwargs = _build_call_args(method) or {} + try: + await method(**kwargs) + except Exception: + pass + return calls + finally: + await client.close() + + assert asyncio.run(_run()), f"{method_name} made no HTTP call on the async client"