Handled by the NOC shift
", + "extrainfo": { + "extrainfofield": "Handled by the NOC shift
", } }, ) @@ -1420,23 +1405,25 @@ of the library surface: except GlpiError as exc: print(f"GLPI call failed: {exc}") -This is not yet the client's entire failure surface. The client is still -built on ``requests``, and network-level faults -- connection failures, -DNS errors, timeouts -- still propagate as ``requests`` exceptions today -rather than a :class:`~glpi_python_client.GlpiError` subclass. Catch -``requests.RequestException`` alongside :class:`~glpi_python_client.GlpiError` -if you need to handle those too: +That single ``except`` clause covers network-level faults too. +Connection failures, DNS errors and timeouts are translated into +:class:`~glpi_python_client.GlpiTransportError` -- or its +:class:`~glpi_python_client.GlpiTimeoutError` subclass for a timeout -- +so you never need to import the HTTP library to catch them. The original +transport exception stays attached as ``__cause__`` for debugging, and +these faults are retried three times before they surface: .. code-block:: python - import requests - from glpi_python_client import GlpiClient, GlpiError + from glpi_python_client import GlpiClient, GlpiTimeoutError, GlpiTransportError client = GlpiClient.from_env() try: ticket = client.get_ticket(42) - except (GlpiError, requests.RequestException) as exc: - print(f"GLPI call failed: {exc}") + except GlpiTimeoutError as exc: + print(f"GLPI was too slow: {exc} (cause: {exc.__cause__!r})") + except GlpiTransportError as exc: + print(f"GLPI was unreachable: {exc}") A handful of sites also deliberately still raise bare ``RuntimeError`` (using a closed client, a missing v1 document session, a partially @@ -1495,7 +1482,8 @@ a failed response. It logs a warning and falls through to a fresh token acquisition, which carries its own independent 3-attempt retry decorator. The refresh method's own retry decorator only retries a network-level fault on the refresh request itself (a -``requests.RequestException`` raised before any response is received) — +:class:`~glpi_python_client.GlpiTransportError` raised before any +response is received) — it does **not** retry a :class:`~glpi_python_client.GlpiServerError` from the fall-through, since that failure is already being retried by the nested acquisition call. A persistent 5xx encountered while diff --git a/glpi_python_client/__init__.py b/glpi_python_client/__init__.py index d3c0b66..fd49b9c 100644 --- a/glpi_python_client/__init__.py +++ b/glpi_python_client/__init__.py @@ -2,10 +2,13 @@ The package re-exports two client classes: -* :class:`GlpiClient` — synchronous, blocking client (single source of - truth for endpoint behaviour). -* :class:`AsyncGlpiClient` — asynchronous facade that wraps each - synchronous method into a coroutine. +* :class:`GlpiClient` — synchronous, blocking client. +* :class:`AsyncGlpiClient` — asynchronous client doing real + non-blocking I/O on the event loop. + +They expose identical endpoint surfaces because they are the same +code: the async tree is hand-written and the sync one is generated +from it. Neither is a wrapper around the other. The ``api_schema`` and ``custom_schema`` Pydantic models are also re-exported so downstream users can import them from a single stable @@ -14,6 +17,7 @@ from __future__ import annotations +from glpi_python_client._async.clients import AsyncGlpiClient from glpi_python_client._errors import ( GlpiAuthError, GlpiError, @@ -25,7 +29,7 @@ GlpiTransportError, GlpiValidationError, ) -from glpi_python_client.clients import AsyncGlpiClient, GlpiClient +from glpi_python_client._sync.clients import GlpiClient from glpi_python_client.models import ( DeleteDocument, DeleteEntity, diff --git a/glpi_python_client/_async/__init__.py b/glpi_python_client/_async/__init__.py new file mode 100644 index 0000000..7fb9b80 --- /dev/null +++ b/glpi_python_client/_async/__init__.py @@ -0,0 +1,15 @@ +"""Hand-written client tree. + +This package is the single source of truth for client code. Its sibling +``glpi_python_client._sync`` is generated from it by ``unasync_build.py`` +and checked in; CI regenerates and diffs to keep the two from drifting. + +Prose written here is copied verbatim into the generated tree -- the +codegen rewrites tokens, not sentences. Docstrings under this package are +therefore worded so they read correctly on both surfaces: describe *what* a +helper does, and leave whether it blocks or awaits to the signature. +""" + +from __future__ import annotations + +__all__: list[str] = [] diff --git a/glpi_python_client/_async/_concurrency.py b/glpi_python_client/_async/_concurrency.py new file mode 100644 index 0000000..4b3bd32 --- /dev/null +++ b/glpi_python_client/_async/_concurrency.py @@ -0,0 +1,59 @@ +"""Asyncio concurrency primitives for the async client tree. + +This module and its ``_sync`` twin are the only files maintained by hand on +both sides of the codegen. Everything else under ``_sync/`` is generated +from ``_async/`` by :mod:`unasync`, which rewrites tokens and strips +``async``/``await``. That works for syntax; it cannot work here, because the +two surfaces need genuinely *different primitives*, not differently-spelled +ones: + +* A fan-out is :func:`asyncio.gather` on this side and plain sequential + evaluation on the other. ``unasync`` leaves ``asyncio.gather`` untouched, + so a generated twin would call it from synchronous code and break. +* The auth lock must be an :class:`asyncio.Lock` here and a + :class:`threading.Lock` there -- see :data:`Lock` for why substituting one + for the other is wrong in *both* directions. + +Keeping both twins tiny is deliberate: hand-maintained duplication is a +liability, so it is confined to the smallest possible surface. +""" + +from __future__ import annotations + +import asyncio +from typing import Any + +#: Lock type guarding OAuth token acquisition on the async surface. +#: +#: An :class:`asyncio.Lock`, and the ``_sync`` twin uses a +#: :class:`threading.Lock`. Neither choice is substitutable for the other, +#: which is exactly why this is hand-written: +#: +#: * A :class:`threading.Lock` here would **deadlock**. The lock is held +#: across an ``await``, so a second task blocking on it blocks the whole +#: event loop -- and the task holding it can then never resume to release +#: it. A single-threaded loop has no way out of that. +#: * An :class:`asyncio.Lock` on the sync side would be worse than useless: +#: it is bound to the loop that first contends it, so sharing one client +#: across threads raises ``RuntimeError: ... bound to a different event +#: loop`` and can deadlock a thread permanently. The failure is latent -- +#: ``acquire()`` only looks up the loop on the *contended* path, so +#: uncontended use passes and tests stay green. +Lock = asyncio.Lock + + +async def gather(*awaitables: Any) -> list[Any]: + """Run ``awaitables`` concurrently and return their results in order. + + The ``_sync`` twin takes already-computed values and simply returns + them. That is not a stub: once ``unasync`` strips the ``await`` from a + call site, each argument expression evaluates eagerly at the point it is + written, which *is* sequential execution. So the same call shape -- + ``await gather(self.a(), self.b())`` -- means "concurrently" here and + "one after the other" there, with no change to the calling code. + """ + + return list(await asyncio.gather(*awaitables)) + + +__all__ = ["Lock", "gather"] diff --git a/glpi_python_client/auth/__init__.py b/glpi_python_client/_async/auth/__init__.py similarity index 56% rename from glpi_python_client/auth/__init__.py rename to glpi_python_client/_async/auth/__init__.py index 98746fe..d4731b6 100644 --- a/glpi_python_client/auth/__init__.py +++ b/glpi_python_client/_async/auth/__init__.py @@ -1,13 +1,13 @@ """Public authentication exports for the GLPI client package. The authentication package owns the OAuth2 token manager used by the -asynchronous ``GlpiClient`` and the legacy v1 session wrapper used solely by +GLPI client and the legacy v1 session wrapper used solely by the management document upload mixin. """ from __future__ import annotations -from glpi_python_client.auth._v1_session import GLPIV1Session -from glpi_python_client.auth.auth import GLPITokenManager +from glpi_python_client._async.auth._v1_session import GLPIV1Session +from glpi_python_client._async.auth.auth import GLPITokenManager __all__ = ["GLPITokenManager", "GLPIV1Session"] diff --git a/glpi_python_client/_async/auth/_v1_session.py b/glpi_python_client/_async/auth/_v1_session.py new file mode 100644 index 0000000..a80f788 --- /dev/null +++ b/glpi_python_client/_async/auth/_v1_session.py @@ -0,0 +1,466 @@ +"""GLPI v1 REST session used for legacy endpoints not exposed by v2. + +Two consumers currently share this session: + +* the management :class:`DocumentMixin` for the multipart + ``POST /Document`` upload (the v2 API does not advertise a binary + upload route), and +* the :class:`PluginFieldsMixin` for the GLPI "Fields" plugin endpoints + (``PluginFieldsContainer``, ``PluginFieldsField`` and the per-item + value itemtypes), which the v2 contract does not surface at all. + +The session wrapper owns the authenticated v1 lifecycle (init, refresh, +kill) and exposes the typed ``upload_document`` helper plus the generic +``request_json`` JSON-only HTTP helper that newer mixins build on. + +Retry policy +------------ +Every public dispatch helper (``_init_session``, ``request_json``, +``upload_document``) carries the same :mod:`tenacity` retry decorator +used by the v2 transport: three attempts spaced by three seconds, +triggered by :class:`~glpi_python_client.GlpiTransportError` (network +faults) and +:class:`~glpi_python_client.GlpiServerError` (which +:func:`finalize_request_response` raises for 5xx server errors), with +``reraise=True`` so the real error surfaces once retries are exhausted. +Not every :class:`ValueError` subclass is retried, only the one named in +the predicate above: :class:`~glpi_python_client.GlpiServerError` (5xx) +*is* a ``ValueError`` and *is* retried by this decorator. +:class:`~glpi_python_client.GlpiStatusError` subclasses for 4xx statuses, +:class:`~glpi_python_client.GlpiValidationError`, and +:class:`~glpi_python_client.GlpiProtocolError` are also ``ValueError`` +subclasses but are not in the retry predicate, so they surface +immediately without a retry. +""" + +from __future__ import annotations + +import json +import logging +from datetime import datetime, timedelta, timezone +from typing import Any, cast + +import httpx +from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed + +from glpi_python_client._async.clients.commons._config import build_http_session +from glpi_python_client._async.clients.commons._http import ( + ensure_response_status, + finalize_request_response, + response_json_or_empty, + transport_error_from, +) +from glpi_python_client._errors import ( + GlpiProtocolError, + GlpiServerError, + GlpiTransportError, + GlpiValidationError, +) + +logger = logging.getLogger(__name__) + +_DEFAULT_SESSION_REFRESH_INTERVAL_SECONDS = 15 * 60 +_AUTH_FAILURE_STATUS_CODES = frozenset({401, 403}) +#: Retry policy for the v1 session, expressed in library-owned types. +#: +#: Mirrors the v2 transport policy deliberately: naming the HTTP library's +#: own exception base here would make the retries stop matching — silently — +#: the next time the transport changes. +_RETRY_ON_NETWORK_ERRORS = retry( + retry=retry_if_exception_type((GlpiTransportError, GlpiServerError)), + stop=stop_after_attempt(3), + wait=wait_fixed(3), + reraise=True, +) + + +class GLPIV1Session: + """Authenticated GLPI v1 REST session limited to document upload. + + The session takes care of token initialisation, periodic refresh, retry on + auth failure, and best-effort cleanup. Only the upload endpoint is exposed + because the rest of the high-level client uses the v2 API exclusively. + """ + + def __init__( + self, + *, + base_url: str, + user_token: str, + app_token: str | None = None, + verify_ssl: bool = True, + session_refresh_interval_seconds: int = ( + _DEFAULT_SESSION_REFRESH_INTERVAL_SECONDS + ), + ) -> None: + self._base_url = base_url.rstrip("/") + self._user_token = user_token + self._app_token = app_token + if session_refresh_interval_seconds < 1: + raise GlpiValidationError( + "session_refresh_interval_seconds must be a positive integer" + ) + self._session_refresh_interval = timedelta( + seconds=session_refresh_interval_seconds + ) + + # 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 + + async def _dispatch(self, method: str, url: str, **kwargs: Any) -> httpx.Response: + """Send one raw v1 HTTP call, translating transport faults. + + Every call this session makes goes through here so network failures + surface as :class:`~glpi_python_client.GlpiTransportError` rather than + as the HTTP library's own exception type. That is what lets the retry + predicate above name a library-owned type, and it keeps callers from + having to import the HTTP library to catch a connection failure. + + Raises + ------ + GlpiTransportError + When the request never produced a response. + """ + + try: + return await self._http.request(method.upper(), url, **kwargs) + except httpx.HTTPError as exc: + raise transport_error_from(exc, method=method, url=url) from exc + + @_RETRY_ON_NETWORK_ERRORS + async def _init_session(self) -> None: + """Acquire one fresh GLPI v1 session token via ``GET /initSession``. + + The call replaces any existing session state and stores the + authentication timestamp used by the refresh-interval check. + Network errors and 5xx responses are retried; 4xx and payload + errors propagate immediately as :class:`ValueError`. + """ + + headers: dict[str, str] = { + "Content-Type": "application/json", + "Accept": "application/json", + "Authorization": f"user_token {self._user_token}", + } + if self._app_token: + headers["App-Token"] = self._app_token + + url = f"{self._base_url}/initSession" + response = await self._dispatch("GET", url, headers=headers, timeout=30) + finalize_request_response( + response, + method="get", + url=url, + success_statuses=(200,), + logger=logger, + ) + ensure_response_status( + response, + success_statuses=(200,), + failure_message="GLPI v1 initSession failed", + ) + + token = response.json().get("session_token") + if not token: + raise GlpiProtocolError("GLPI v1 initSession returned no session_token") + + self._session_token = str(token) + self._session_started_at = datetime.now(tz=timezone.utc) + logger.info("GLPI v1 session initialised.") + + async def _ensure_session(self) -> None: + """Lazily initialise or renew the v1 session when the token is stale. + + The helper is called by every authenticated request before any header + construction so callers never have to manage the lifecycle directly. + """ + + if self._session_token is None: + await self._init_session() + return + if self._is_session_stale(): + logger.info("GLPI v1 session reached refresh interval; renewing session.") + await self._renew_session() + + def _is_session_stale(self) -> bool: + """Return whether the current v1 session token must be renewed. + + Stale tokens are determined by the configured refresh interval relative + to the timestamp the current token was acquired. + """ + + if self._session_started_at is None: + return True + return datetime.now(tz=timezone.utc) >= ( + self._session_started_at + self._session_refresh_interval + ) + + def _session_headers(self) -> dict[str, str]: + """Return the GLPI v1 headers carrying the current session token. + + The helper assumes ``_ensure_session`` has already been called so the + session token state is valid. + """ + + headers: dict[str, str] = { + "Session-Token": str(self._session_token), + "Accept": "application/json", + } + if self._app_token: + headers["App-Token"] = self._app_token + return headers + + async def _renew_session(self) -> None: + """Drop the current GLPI v1 session token and acquire a new one. + + The previous token is best-effort killed so the GLPI server can release + the associated session state immediately. ``_init_session`` will set + the new token on success or raise, leaving the existing state + untouched on failure (the retry decorator handles transients). + """ + + if self._session_token is not None: + try: + await self._dispatch( + "GET", + f"{self._base_url}/killSession", + headers=self._session_headers(), + timeout=10, + ) + except Exception: + logger.warning("Failed to kill stale GLPI v1 session.", exc_info=True) + await self._init_session() + + async def _headers(self) -> dict[str, str]: + """Return ready-to-use authenticated GLPI v1 request headers. + + The helper is the single header entry-point used by all authenticated + v1 calls so token lifecycle handling stays centralised. + """ + + await self._ensure_session() + return self._session_headers() + + async def _authenticated_request( + self, + method: str, + url: str, + *, + success_statuses: tuple[int, ...], + headers: dict[str, str] | None = None, + **kwargs: Any, + ) -> httpx.Response: + """Send one authenticated GLPI v1 request and finalize the response. + + When the GLPI server rejects the current token the helper renews + the session and retries the request once. The returned response + has already been passed through :func:`finalize_request_response` + so 5xx errors surface as + :class:`~glpi_python_client.GlpiServerError` for the outer + tenacity retry to catch; non-success statuses outside the + ``success_statuses`` set are logged but otherwise returned for + the caller to validate with :func:`ensure_response_status`. + """ + + request_headers = {**await self._headers(), **(headers or {})} + # 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 = await self._dispatch(verb, url, headers=request_headers, **kwargs) + if _is_auth_failure_response(response): + logger.warning( + "GLPI v1 session token was rejected; refreshing session and " + "retrying request once." + ) + await self._renew_session() + request_headers = {**await self._headers(), **(headers or {})} + response = await self._dispatch( + verb, url, headers=request_headers, **kwargs + ) + return finalize_request_response( + response, + method=method, + url=url, + success_statuses=success_statuses, + logger=logger, + ) + + async def close(self) -> None: + """Kill the v1 session token and release the underlying HTTP session. + + Cleanup is best-effort: any failure during ``killSession`` is logged + and the local session state is still cleared. + """ + + try: + if self._session_token is not None: + await self._dispatch( + "GET", + f"{self._base_url}/killSession", + headers=self._session_headers(), + timeout=10, + ) + logger.info("GLPI v1 session killed.") + except Exception: + logger.warning("Failed to kill GLPI v1 session.", exc_info=True) + finally: + self._session_token = None + self._session_started_at = None + await self._http.aclose() + + @_RETRY_ON_NETWORK_ERRORS + async def request_json( + self, + method: str, + path: str, + *, + params: dict[str, object] | None = None, + json_body: dict[str, object] | None = None, + success_statuses: tuple[int, ...] = (200, 201, 204, 206), + failure_message: str | None = None, + ) -> object: + """Send one JSON-only authenticated request to the GLPI v1 API. + + The helper centralises session-token handling, the one-shot retry + on token rejection, status validation and JSON parsing so callers + can stay focused on their endpoint semantics. Network errors and + 5xx responses are retried; 4xx and payload errors propagate + immediately as :class:`ValueError`. + + Parameters + ---------- + method : str + HTTP verb (``"GET"``, ``"POST"``, ``"PUT"``, ``"DELETE"``). + path : str + Resource path appended to the v1 base URL (without leading + slash, e.g. ``"PluginFieldsContainer"``). + params : dict[str, object] | None, optional + Query-string parameters forwarded to the HTTP transport. + json_body : dict[str, object] | None, optional + JSON body serialised into the request when set. The + ``Content-Type: application/json`` header is added + automatically. + success_statuses : tuple[int, ...], optional + HTTP status codes considered successful (default covers the + CRUD codes returned by the v1 API). + failure_message : str | None, optional + Prefix used in the :class:`~glpi_python_client.GlpiStatusError` + raised on a non-success status. Defaults to ``"GLPI v1 + {METHOD} {path} failed"``. + + Returns + ------- + object + Parsed JSON body for non-empty responses; an empty ``dict`` + when the body is empty or contains only whitespace. + + Raises + ------ + GlpiStatusError + If the v1 server returns a non-success HTTP status outside + the 5xx range (narrows to :class:`~glpi_python_client.GlpiAuthError` + or :class:`~glpi_python_client.GlpiNotFoundError` where the + status allows it). Inherits from ``ValueError``. + GlpiServerError + If the v1 server persistently returns a 5xx status after this + decorator's retries are exhausted. + """ + + url = f"{self._base_url}/{path.lstrip('/')}" + kwargs: dict[str, Any] = {"timeout": 30} + if params is not None: + kwargs["params"] = params + headers: dict[str, str] = {} + if json_body is not None: + kwargs["content"] = json.dumps(json_body) + headers["Content-Type"] = "application/json" + response = await self._authenticated_request( + method, + url, + success_statuses=success_statuses, + headers=headers or None, + **kwargs, + ) + ensure_response_status( + response, + success_statuses=success_statuses, + failure_message=failure_message + or f"GLPI v1 {method.upper()} {path} failed", + ) + return response_json_or_empty(response) + + @_RETRY_ON_NETWORK_ERRORS + async def upload_document( + self, + filename: str, + content: bytes, + mime_type: str, + *, + document_name: str | None = None, + ticket_id: int | None = None, + entity_id: int | None = None, + ) -> dict[str, object]: + """Upload one binary document via ``POST /Document``. + + The legacy v1 endpoint uses a multipart upload manifest so the GLPI + server can create the document, link it to the optional parent ticket, + and assign it to the provided entity in a single round-trip. Network + errors and 5xx responses are retried; 4xx and payload errors + propagate immediately as :class:`ValueError`. + """ + + manifest_input: dict[str, object] = { + "name": document_name or filename, + "_filename": [filename], + } + if entity_id is not None: + manifest_input["entities_id"] = int(entity_id) + if ticket_id is not None: + manifest_input["itemtype"] = "Ticket" + manifest_input["items_id"] = int(ticket_id) + manifest_input["tickets_id"] = int(ticket_id) + manifest = json.dumps({"input": manifest_input}) + response = await self._authenticated_request( + "POST", + f"{self._base_url}/Document", + success_statuses=(200, 201), + files=[ + ("uploadManifest", (None, manifest, "application/json")), + ("filename[]", (filename, content, mime_type)), + ], + timeout=60, + ) + ensure_response_status( + response, + success_statuses=(200, 201), + failure_message="GLPI v1 document upload failed", + ) + payload = response.json() + if not isinstance(payload, dict): + raise GlpiProtocolError( + "GLPI v1 document upload returned unexpected payload: " + f"{type(payload).__name__}" + ) + logger.info("GLPI v1 document uploaded: id=%s", payload.get("id")) + return cast(dict[str, object], payload) + + +def _is_auth_failure_response(response: httpx.Response) -> bool: + """Return whether one GLPI v1 response means the session token is invalid. + + Both HTTP-level rejection and the ``ERROR_SESSION_TOKEN_INVALID`` payload + marker emitted by the GLPI v1 API are considered auth failures. + """ + + if response.status_code in _AUTH_FAILURE_STATUS_CODES: + return True + return "ERROR_SESSION_TOKEN_INVALID" in str(response.text or "") + + +__all__ = ["GLPIV1Session"] diff --git a/glpi_python_client/_async/auth/auth.py b/glpi_python_client/_async/auth/auth.py new file mode 100644 index 0000000..2ae6840 --- /dev/null +++ b/glpi_python_client/_async/auth/auth.py @@ -0,0 +1,416 @@ +"""OAuth2 token management for the high-level GLPI clients. + +The token manager centralizes credential validation, token acquisition, +refresh behavior, and lifecycle cleanup so both sync and async clients can +share the same authenticated session state. +""" + +from __future__ import annotations + +import logging +from datetime import datetime, timedelta, timezone + +import httpx +from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed + +from glpi_python_client._async.clients.commons._config import build_http_session +from glpi_python_client._async.clients.commons._http import transport_error_from +from glpi_python_client._errors import ( + GlpiServerError, + GlpiTransportError, + GlpiValidationError, + status_error_class, +) + +logger = logging.getLogger(__name__) + + +def validate_credentials( + *, + client_id: str | None, + client_secret: str | None, + username: str | None, + password: str | None, +) -> None: + """Check that the supplied OAuth credential sets are complete. + + GLPI authentication accepts client credentials, user credentials, or + both. Partial pairs are rejected here so the token request path does not + fail later with a less actionable error. + + This is a free function rather than only a method so callers can check a + configuration *before* committing resources to it. That ordering matters: + the client used to build its HTTP session first and unwind it in an + ``except`` clause when validation failed, which is not expressible on the + async surface -- an ``httpx.AsyncClient`` has no synchronous close, and a + constructor cannot await one. Validating up front removes the need to + unwind anything. + + Raises + ------ + GlpiValidationError + If either pair is half-supplied, or if neither pair is supplied. + """ + + missing_client_fields = [ + name + for name, value in { + "client_id": client_id, + "client_secret": client_secret, + }.items() + if value is None + ] + missing_user_fields = [ + name + for name, value in { + "username": username, + "password": password, + }.items() + if value is None + ] + + has_client_fields = len(missing_client_fields) < 2 + has_user_fields = len(missing_user_fields) < 2 + + if has_client_fields and missing_client_fields: + raise GlpiValidationError( + "GLPI OAuth client credentials must include both client_id " + "and client_secret." + ) + if has_user_fields and missing_user_fields: + raise GlpiValidationError( + "GLPI user credentials must include both username and password." + ) + if not (client_id is not None and client_secret is not None) and not ( + username is not None and password is not None + ): + raise GlpiValidationError( + "GLPI authentication requires either client_id/client_secret, " + "username/password, or both." + ) + + +class GLPITokenManager: + """OAuth2 token manager for the GLPI API. + + Parameters + ---------- + token_url : str + Full URL of the GLPI token endpoint. + client_id : str | None, optional + OAuth2 client ID. Provide it together with ``client_secret`` when the + GLPI instance requires client authentication. + client_secret : str | None, optional + OAuth2 client secret. Provide it together with ``client_id``. + username : str | None, optional + Username for the password grant flow. Provide it together with + ``password``. + password : str | None, optional + Password for the password grant flow. Provide it together with + ``username``. + session : httpx.AsyncClient | None, optional + Existing HTTP client to reuse. + auth_token_refresh : int | None, optional + Maximum token age in seconds before a refresh is attempted. ``None`` + disables interval-based refreshes. + """ + + def __init__( + self, + token_url: str, + client_id: str | None = None, + client_secret: str | None = None, + username: str | None = None, + password: str | None = None, + session: httpx.AsyncClient | None = None, + auth_token_refresh: int | None = None, + ) -> None: + self._token_url = token_url + self._client_id = client_id + self._client_secret = client_secret + self._username = username + self._password = password + self._owns_session = session is None + self._session = session or build_http_session(verify_ssl=True) + self._auth_token_refresh_interval = _refresh_interval(auth_token_refresh) + + self._validate_credentials() + + self.access_token: str | None = None + self.refresh_token: str | None = None + self.token_expires_at: datetime | None = None + self.token_updated_at: datetime | None = None + + async def _post_token_request(self, data: dict[str, str]) -> httpx.Response: + """POST the OAuth token endpoint, translating transport faults. + + Network failures surface as + :class:`~glpi_python_client.GlpiTransportError` so the retry + predicates below can name a library-owned type and callers never have + to import the HTTP library to catch a connection failure. + + Raises + ------ + GlpiTransportError + When the token request never produced a response. + """ + + try: + return await self._session.post(self._token_url, data=data, timeout=30) + except httpx.HTTPError as exc: + raise transport_error_from(exc, method="post", url=self._token_url) from exc + + @property + def auth_token_refresh(self) -> int | None: + """Return the proactive refresh delay configured for this manager. + + The public property keeps the original integer value used at + construction time instead of exposing the internal ``timedelta`` + representation. + """ + + if self._auth_token_refresh_interval is None: + return None + return int(self._auth_token_refresh_interval.total_seconds()) + + def _validate_credentials(self) -> None: + """Validate that the configured OAuth credential sets are complete.""" + + validate_credentials( + client_id=self._client_id, + client_secret=self._client_secret, + username=self._username, + password=self._password, + ) + + @property + def _has_client_credentials(self) -> bool: + return self._client_id is not None and self._client_secret is not None + + @property + def _has_user_credentials(self) -> bool: + return self._username is not None and self._password is not None + + def _build_token_request_data(self) -> dict[str, str]: + """Build the form payload sent to the OAuth token endpoint. + + The payload shape depends on whether the manager is using the password + grant or pure client-credentials flow, and it includes client + credentials when that pair is configured. + """ + + data: dict[str, str] = {"scope": "api"} + if self._has_user_credentials: + assert self._username is not None + assert self._password is not None + data["grant_type"] = "password" + data["username"] = self._username + data["password"] = self._password + else: + data["grant_type"] = "client_credentials" + + if self._has_client_credentials: + assert self._client_id is not None + assert self._client_secret is not None + data["client_id"] = self._client_id + data["client_secret"] = self._client_secret + + return data + + def _store_token_data( + self, token_data: dict[str, object], label: str = "acquired" + ) -> None: + """Store token data from an OAuth2 response. + + Parameters + ---------- + token_data : dict[str, object] + Token endpoint JSON response. + label : str, optional + Label for log messages. + + Returns + ------- + None + Mutates instance state. + """ + + self.access_token = str(token_data.get("access_token") or "") or None + refresh_token = str(token_data.get("refresh_token") or "").strip() + if refresh_token: + self.refresh_token = refresh_token + expires_in = int(str(token_data.get("expires_in") or 3600)) + now = datetime.now(tz=timezone.utc) + self.token_updated_at = now + self.token_expires_at = now + timedelta(seconds=expires_in) + logger.info("GLPI OAuth token %s successfully.", label) + + def logout(self) -> None: + """Discard the currently cached OAuth tokens. + + Returns + ------- + None + Clears in-memory token state. + """ + + self.access_token = None + self.refresh_token = None + self.token_expires_at = None + self.token_updated_at = None + + async def close(self) -> None: + """Release token-manager resources. + + Returns + ------- + None + Performs a local logout and closes any owned HTTP session. + """ + + self.logout() + if self._owns_session: + await self._session.aclose() + + def _should_refresh_by_interval(self, now: datetime) -> bool: + """Return whether the configured proactive refresh interval elapsed. + + This check supplements token-expiry handling so long-lived clients can + refresh credentials before the server-side expiry timestamp is reached. + """ + + if self._auth_token_refresh_interval is None or self.token_updated_at is None: + return False + return now >= self.token_updated_at + self._auth_token_refresh_interval + + @retry( + retry=retry_if_exception_type((GlpiTransportError, GlpiServerError)), + stop=stop_after_attempt(3), + wait=wait_fixed(3), + reraise=True, + ) + async def _acquire_token(self) -> None: + """Acquire an OAuth2 access token using the configured auth flow. + + Returns + ------- + None + Stores the new access token. + + Raises + ------ + GlpiAuthError + If GLPI rejects the credentials (401/403). Not retried. + GlpiServerError + If the token endpoint fails (5xx). Retried up to 3 attempts. + GlpiStatusError + If the token endpoint returns any other unexpected status. Not + retried. + """ + + data = self._build_token_request_data() + response = await self._post_token_request(data) + if 200 <= response.status_code < 300: + self._store_token_data(response.json()) + return + try: + error_detail = response.json() + except Exception: + error_detail = response.text + error_class = status_error_class(response.status_code) + raise error_class( + f"GLPI OAuth token returned {response.status_code}: {error_detail}", + status_code=response.status_code, + url=self._token_url, + response_text=str(error_detail), + ) + + @retry( + retry=retry_if_exception_type(GlpiTransportError), + stop=stop_after_attempt(3), + wait=wait_fixed(3), + reraise=True, + ) + async def _refresh_access_token(self) -> None: + """Refresh the OAuth2 access token using the stored refresh token. + + Returns + ------- + None + Stores the refreshed token or acquires a new one. + + Raises + ------ + GlpiAuthError + If GLPI rejects the credentials while refreshing (401/403). This + method does not raise directly on a non-2xx response: it logs a + warning and falls through to a nested :meth:`_acquire_token` + call, which raises. That nested call is not retried by either + decorator, so a persistent 401 costs 1 refresh POST + 1 acquire + POST (2 total) before this propagates. + GlpiServerError + If the token endpoint fails (5xx) while refreshing. This + method's own retry decorator only matches + ``GlpiTransportError`` (network-level faults), not + ``GlpiServerError``, so it does not retry the fall-through to + :meth:`_acquire_token`. The nested call carries its own + independent decorator, which does retry ``GlpiServerError`` up + to 3 attempts. A persistent 5xx therefore costs exactly 1 + refresh POST + 3 nested acquire POSTs = 4 POST requests, not the + 12 an earlier, less precise predicate produced by retrying the + already-retried nested failure a second time. + GlpiStatusError + If the token endpoint returns any other unexpected status while + refreshing, raised by the nested :meth:`_acquire_token` call. + Not retried. + """ + + if not self.refresh_token: + await self._acquire_token() + return + + data = { + "grant_type": "refresh_token", + "refresh_token": self.refresh_token, + } + if self._has_client_credentials: + assert self._client_id is not None + assert self._client_secret is not None + data["client_id"] = self._client_id + data["client_secret"] = self._client_secret + response = await self._post_token_request(data) + if 200 <= response.status_code < 300: + self._store_token_data(response.json(), label="refreshed") + return + logger.warning("Token refresh failed, acquiring new token...") + await self._acquire_token() + + async def ensure_token(self) -> None: + """Ensure a valid OAuth2 access token is available. + + Returns + ------- + None + Updates token state when needed. + """ + + if not self.access_token: + await self._acquire_token() + return + + now = datetime.now(tz=timezone.utc) + token_expired = ( + self.token_expires_at is not None and now >= self.token_expires_at + ) + if token_expired or self._should_refresh_by_interval(now): + await self._refresh_access_token() + + +def _refresh_interval(value: int | None) -> timedelta | None: + if value is None: + return None + if value < 1: + raise GlpiValidationError( + "auth_token_refresh must be a positive integer or None" + ) + return timedelta(seconds=value) diff --git a/glpi_python_client/_async/clients/__init__.py b/glpi_python_client/_async/clients/__init__.py new file mode 100644 index 0000000..ada209a --- /dev/null +++ b/glpi_python_client/_async/clients/__init__.py @@ -0,0 +1,17 @@ +"""Client class for one GLPI surface. + +The concrete client composes every per-endpoint mixin from +:mod:`glpi_python_client._async.clients.api`, the aggregated helpers from +:mod:`glpi_python_client._async.clients.custom`, and the transport mixin +from :mod:`glpi_python_client._async.clients.commons`. + +Only one of the two client trees is written by hand; the other is +generated from it. Both expose the same endpoint surface, so the choice +between them is purely about the caller's runtime model. +""" + +from __future__ import annotations + +from glpi_python_client._async.clients.client import AsyncGlpiClient + +__all__ = ["AsyncGlpiClient"] diff --git a/glpi_python_client/clients/_base_client.py b/glpi_python_client/_async/clients/_base_client.py similarity index 90% rename from glpi_python_client/clients/_base_client.py rename to glpi_python_client/_async/clients/_base_client.py index 5febafb..90c9c40 100644 --- a/glpi_python_client/clients/_base_client.py +++ b/glpi_python_client/_async/clients/_base_client.py @@ -1,9 +1,9 @@ -"""Shared construction logic for the synchronous and asynchronous clients. +"""Shared construction logic for the GLPI client. The :class:`_BaseGlpiClient` mixin holds the constructor signature, the resource-bundle assignment, and the :meth:`from_env` classmethod that -both :class:`~glpi_python_client.clients.sync_client.GlpiClient` and -:class:`~glpi_python_client.clients.async_client.AsyncGlpiClient` use. +both :class:`~glpi_python_client.GlpiClient` and +:class:`~glpi_python_client.AsyncGlpiClient` use. Lifecycle helpers (``close``, ``__enter__``/``__exit__`` versus ``__aenter__``/``__aexit__``) stay on the concrete subclasses because they differ between the sync and async surfaces. @@ -14,7 +14,6 @@ import logging import os import sys -import threading from typing import TYPE_CHECKING if sys.version_info >= (3, 11): @@ -22,7 +21,8 @@ else: # pragma: no cover - fallback for Python 3.10 from typing_extensions import Self -from glpi_python_client.clients.commons._config import ( +from glpi_python_client._async._concurrency import Lock +from glpi_python_client._async.clients.commons._config import ( build_client_env_config, build_client_resources, ) @@ -37,8 +37,8 @@ class _BaseGlpiClient: """Shared construction helpers for the GLPI client variants. The mixin assigns the resource bundle returned by - :func:`build_client_resources` and the header/lock/state attributes - used by :class:`~glpi_python_client.clients.commons._transport.TransportMixin`. + :func:`build_client_resources` and the header, lock and state + attributes the transport mixin declares. """ def __init__( @@ -124,7 +124,7 @@ def __init__( self.glpi_profile = glpi_profile self.entity_recursive = entity_recursive self.language = language - self._auth_lock = threading.Lock() + self._auth_lock = Lock() self._closed = False @classmethod @@ -152,9 +152,8 @@ def from_env( prefix : str, optional Common prefix shared by every environment variable name. **overrides : object - Keyword overrides forwarded to :meth:`__init__`. The - asynchronous client accepts an additional ``executor`` - keyword here. + Keyword overrides forwarded to :meth:`__init__`; the + keyword overrides are forwarded verbatim. Returns ------- diff --git a/glpi_python_client/_async/clients/api/__init__.py b/glpi_python_client/_async/clients/api/__init__.py new file mode 100644 index 0000000..7bd032e --- /dev/null +++ b/glpi_python_client/_async/clients/api/__init__.py @@ -0,0 +1,54 @@ +"""Per-endpoint API mixins backed by the ``api_schema`` Pydantic models. + +The mixins under this package mirror the endpoints documented in +``docs/glpi_api_contract.json`` one for one. They wrap the +transport helpers from :mod:`glpi_python_client._async.clients.commons` and exchange +typed ``Getc
"}, - ) - - client._get_request = _get # type: ignore[method-assign] - ctx = await client.get_ticket_context(42) - assert ctx.ticket.id == 42 - assert any("Timeline/Task" in c for c in calls) - assert any("Timeline/Followup" in c for c in calls) - assert any("Timeline/Solution" in c for c in calls) - assert any("Timeline/Document" in c for c in calls) - await client.close() - - -async def test_async_get_task_statistics_with_tickets_uses_gather() -> None: - """The async override fetches per-ticket tasks concurrently.""" - - client = make_async_client() - seen: list[str] = [] - - def _get( - endpoint: str, params: Any = None, skip_entity: bool = False - ) -> FakeResponse: - seen.append(endpoint) - return FakeResponse(status_code=200, payload=[]) - - client._get_request = _get # type: ignore[method-assign] - stats = await client.get_task_statistics([1, 2, 3]) - assert stats["ticket_count"] == 3 - assert stats["task_count"] == 0 - assert sum(1 for e in seen if "/Timeline/Task" in e) == 3 - await client.close() - - -async def test_async_get_task_statistics_empty_short_circuits() -> None: - """An empty ticket list returns zeroed totals without HTTP traffic.""" - - client = make_async_client() - - def _fail(*_args: Any, **_kwargs: Any) -> FakeResponse: - pytest.fail("no HTTP call expected for empty ticket list") - - client._get_request = _fail # type: ignore[method-assign] - stats = await client.get_task_statistics([]) - assert stats == { - "ticket_count": 0, - "task_count": 0, - "total_duration": 0, - "duration_by_user": {}, - "duration_by_ticket": {}, - } - await client.close() - - -async def test_async_close_closes_v1_session_when_configured() -> None: - """The async ``close`` also closes the optional v1 fallback session.""" - - client = AsyncGlpiClient( - glpi_api_url="https://glpi.example.test/api.php/v2", - username="u", - password="p", - v1_base_url="https://glpi.example.test/apirest.php", - v1_user_token="user-token", - v1_app_token="app-token", - ) - assert client._v1 is not None - await client.close() - assert client._closed is True - - -async def test_async_from_env_accepts_executor() -> None: - """``AsyncGlpiClient.from_env`` accepts an executor and forwards it.""" - - env = { - "GLPI_API_URL": "https://glpi.example.test/api.php/v2", - "GLPI_USERNAME": "u", - "GLPI_PASSWORD": "p", - } - with ThreadPoolExecutor(max_workers=1) as pool: - client = AsyncGlpiClient.from_env(env=env, executor=pool) - try: - assert client._executor is pool - finally: - await client.close() - - -async def test_async_generator_wrapper_yields_then_stops_default_executor() -> None: - """The bridge wrapper drives a sync generator function to completion.""" - - from glpi_python_client.clients.commons._async_bridge import ( - AsyncBridge, - _make_async_generator_wrapper, - ) - - def sync_gen(self: AsyncBridge, n: int) -> Any: - for i in range(n): - yield [i] - - wrapper = _make_async_generator_wrapper(sync_gen) - - class _Owner(AsyncBridge): - pass - - owner = _Owner() - collected: list[list[int]] = [] - async for batch in wrapper(owner, 3): - collected.append(batch) - assert collected == [[0], [1], [2]] - - -async def test_async_generator_wrapper_with_executor() -> None: - """The wrapper dispatches generator advancement to the supplied executor.""" - - from glpi_python_client.clients.commons._async_bridge import ( - AsyncBridge, - _make_async_generator_wrapper, - ) - - captured_threads: list[str] = [] - - def sync_gen(self: AsyncBridge) -> Any: - import threading - - captured_threads.append(threading.current_thread().name) - yield ["one"] - captured_threads.append(threading.current_thread().name) - - wrapper = _make_async_generator_wrapper(sync_gen) - - with ThreadPoolExecutor(max_workers=1, thread_name_prefix="glpi-gen") as pool: - - class _Owner(AsyncBridge): - pass - - owner = _Owner() - owner._executor = pool - batches: list[list[str]] = [] - async for batch in wrapper(owner): - batches.append(batch) - assert batches == [["one"]] - assert captured_threads - assert all(name.startswith("glpi-gen") for name in captured_threads) - - -# --------------------------------------------------------------------------- -# AsyncPaginationMixin — iter_search_tickets -# --------------------------------------------------------------------------- - - -async def test_async_iter_search_tickets_single_page() -> None: - """A response shorter than batch_size yields one batch then stops.""" - - client = make_async_client() - call_count = 0 - - async def fake_search( - rsql_filter: str = "", - *, - limit: int = 50, - start: int = 0, - sort: str | None = None, - fields: tuple[str, ...] = (), - ) -> list[Any]: - nonlocal call_count - call_count += 1 - return [{"id": 1, "name": "t1", "content": "c"}] - - client.search_tickets = fake_search # type: ignore[method-assign] - batches: list[Any] = [] - async for batch in client.iter_search_tickets("status==1", batch_size=50): - batches.append(batch) - assert call_count == 1 - assert len(batches) == 1 - assert len(batches[0]) == 1 - await client.close() - - -async def test_async_iter_search_tickets_multi_page_stops_on_short_batch() -> None: - """Iteration stops after the first batch shorter than batch_size.""" - - client = make_async_client() - responses = [ - [ - {"id": 1, "name": "a", "content": "c"}, - {"id": 2, "name": "b", "content": "c"}, - ], - [{"id": 3, "name": "c", "content": "c"}], - ] - call_count = 0 - - async def fake_search( - rsql_filter: str = "", - *, - limit: int = 50, - start: int = 0, - sort: str | None = None, - fields: tuple[str, ...] = (), - ) -> list[Any]: - nonlocal call_count - result = responses[min(call_count, len(responses) - 1)] - call_count += 1 - return result - - client.search_tickets = fake_search # type: ignore[method-assign] - batches: list[Any] = [] - async for batch in client.iter_search_tickets("", batch_size=2): - batches.append(batch) - assert call_count == 2 - assert len(batches) == 2 - assert sum(len(b) for b in batches) == 3 - await client.close() - - -async def test_async_iter_search_tickets_empty_page_not_yielded() -> None: - """An empty response is not yielded but still terminates the loop.""" - - client = make_async_client() - - async def fake_search( - rsql_filter: str = "", - *, - limit: int = 50, - start: int = 0, - sort: str | None = None, - fields: tuple[str, ...] = (), - ) -> list[Any]: - return [] - - client.search_tickets = fake_search # type: ignore[method-assign] - batches: list[Any] = [] - async for batch in client.iter_search_tickets("status==1", batch_size=50): - batches.append(batch) - assert batches == [] - await client.close() - - -# --------------------------------------------------------------------------- -# AsyncPaginationMixin — iter_search_users -# --------------------------------------------------------------------------- - - -async def test_async_iter_search_users_single_page() -> None: - """A response shorter than batch_size yields one batch then stops.""" - - client = make_async_client() - call_count = 0 - - async def fake_search( - rsql_filter: str = "", - *, - limit: int = 50, - start: int = 0, - skip_entity: bool = False, - ) -> list[Any]: - nonlocal call_count - call_count += 1 - return [{"id": 1, "username": "alice"}] - - client.search_users = fake_search # type: ignore[method-assign] - batches: list[Any] = [] - async for batch in client.iter_search_users("username==alice", batch_size=50): - batches.append(batch) - assert call_count == 1 - assert len(batches) == 1 - await client.close() - - -async def test_async_iter_search_users_multi_page_stops_on_short_batch() -> None: - """Iteration stops after the first short user batch.""" - - client = make_async_client() - responses = [ - [{"id": 1, "username": "alice"}, {"id": 2, "username": "bob"}], - [{"id": 3, "username": "carol"}], - ] - call_count = 0 - - async def fake_search( - rsql_filter: str = "", - *, - limit: int = 50, - start: int = 0, - skip_entity: bool = False, - ) -> list[Any]: - nonlocal call_count - result = responses[min(call_count, len(responses) - 1)] - call_count += 1 - return result - - client.search_users = fake_search # type: ignore[method-assign] - batches: list[Any] = [] - async for batch in client.iter_search_users("", batch_size=2): - batches.append(batch) - assert call_count == 2 - assert sum(len(b) for b in batches) == 3 - await client.close() - - -# --------------------------------------------------------------------------- -# AsyncPaginationMixin — iter_search_entities -# --------------------------------------------------------------------------- - - -async def test_async_iter_search_entities_single_page() -> None: - """A response shorter than batch_size yields one batch then stops.""" - - client = make_async_client() - call_count = 0 - - async def fake_search( - rsql_filter: str = "", - *, - limit: int = 50, - start: int = 0, - ) -> list[Any]: - nonlocal call_count - call_count += 1 - return [{"id": 1, "name": "root"}] - - client.search_entities = fake_search # type: ignore[method-assign] - batches: list[Any] = [] - async for batch in client.iter_search_entities("", batch_size=50): - batches.append(batch) - assert call_count == 1 - assert len(batches) == 1 - await client.close() - - -async def test_async_iter_search_entities_multi_page_stops_on_short_batch() -> None: - """Iteration stops after the first short entity batch.""" - - client = make_async_client() - responses = [ - [{"id": 1, "name": "a"}, {"id": 2, "name": "b"}], - [{"id": 3, "name": "c"}], - ] - call_count = 0 - - async def fake_search( - rsql_filter: str = "", - *, - limit: int = 50, - start: int = 0, - ) -> list[Any]: - nonlocal call_count - result = responses[min(call_count, len(responses) - 1)] - call_count += 1 - return result - - client.search_entities = fake_search # type: ignore[method-assign] - batches: list[Any] = [] - async for batch in client.iter_search_entities("", batch_size=2): - batches.append(batch) - assert call_count == 2 - assert sum(len(b) for b in batches) == 3 - await client.close() - - -# --------------------------------------------------------------------------- -# AsyncStatisticsMixin — get_task_durations with entity_id -# --------------------------------------------------------------------------- - - -async def test_async_get_task_durations_with_entity_id() -> None: - """Providing entity_id builds the entity filter without an HTTP lookup.""" - - client = make_async_client() - search_calls: list[str] = [] - - async def fake_search_tickets(rsql_filter: str = "", **kwargs: Any) -> list[Any]: - search_calls.append(rsql_filter) - return [] - - async def fake_iter( - rsql_filter: str = "", - **kwargs: Any, - ) -> Any: - return - yield # make it an async generator - - async def fake_task_stats(ticket_ids: list[int]) -> Any: - return { - "total_duration": 0, - "task_count": 0, - "duration_by_user": {}, - "duration_by_ticket": {}, - } - - client.search_tickets = fake_search_tickets # type: ignore[method-assign] - client.iter_search_tickets = fake_iter # type: ignore[method-assign] - client.get_task_statistics = fake_task_stats # type: ignore[method-assign] - - result = await client.get_task_durations(entity_id=5) - assert result["total_duration"] == 0 - assert any("entities_id==5" in c for c in search_calls) or True - await client.close() - - -# --------------------------------------------------------------------------- -# AsyncStatisticsMixin — get_ticket_statistics -# --------------------------------------------------------------------------- - - -async def test_async_get_ticket_statistics_returns_summary() -> None: - """get_ticket_statistics awaits search_tickets and summarises results.""" - - client = make_async_client() - - async def fake_search_tickets(rsql_filter: str = "", **kwargs: Any) -> list[Any]: - # Return empty list — _summarize_tickets([]) is valid and avoids - # constructing full GetTicket model objects in this smoke test. - return [] - - client.search_tickets = fake_search_tickets # type: ignore[method-assign] - - result = await client.get_ticket_statistics() - assert isinstance(result, dict) - assert "entities" in result - await client.close() - - -async def test_async_get_ticket_statistics_with_entity_name_no_match() -> None: - """When entity lookup returns nothing, an empty dict is returned early.""" - - client = make_async_client() - - async def fake_search_entities(rsql_filter: str = "", **kwargs: Any) -> list[Any]: - return [] - - client.search_entities = fake_search_entities # type: ignore[method-assign] - - result = await client.get_ticket_statistics(entity_name="nonexistent") - assert result == {"entities": {}} - await client.close() - - -async def test_async_get_ticket_statistics_with_entity_id() -> None: - """Providing entity_id builds the filter without calling search_entities.""" - - client = make_async_client() - entity_calls: list[str] = [] - - async def fake_search_entities(**kwargs: Any) -> list[Any]: - entity_calls.append("called") - return [] - - async def fake_search_tickets(rsql_filter: str = "", **kwargs: Any) -> list[Any]: - return [] - - client.search_entities = fake_search_entities # type: ignore[method-assign] - client.search_tickets = fake_search_tickets # type: ignore[method-assign] - - result = await client.get_ticket_statistics(entity_id=3) - assert entity_calls == [] - assert "entities" in result - await client.close() - - -# --------------------------------------------------------------------------- -# AsyncStatisticsMixin — get_user_activity -# --------------------------------------------------------------------------- - - -async def test_async_get_user_activity_raises_without_criteria() -> None: - """``GlpiValidationError`` is raised when no user criteria are supplied. - - ``GlpiValidationError`` inherits ``ValueError`` so existing callers that - catch the broader type keep working. - """ - - client = make_async_client() - with pytest.raises(GlpiValidationError, match="At least one of") as excinfo: - await client.get_user_activity() - assert isinstance(excinfo.value, ValueError) - await client.close() - - -async def test_async_get_user_activity_by_user_id() -> None: - """get_user_activity accepts user_id and returns a UserActivityResult.""" - - client = make_async_client() - - async def fake_iter_tickets(rsql_filter: str = "", **kwargs: Any) -> Any: - if False: - yield [] - - async def fake_task_durations(**kwargs: Any) -> Any: - from glpi_python_client.clients.custom._statistics import TaskDurationsResult - - return TaskDurationsResult( - start_date="2025-01-01", - end_date="2025-01-31", - total_duration=0, - task_count=0, - duration_by_user={}, - duration_by_entity={}, - tasks=None, - ) - - client.iter_search_tickets = fake_iter_tickets # type: ignore[method-assign] - client.get_task_durations = fake_task_durations # type: ignore[method-assign] - client._v1 = _FakeV1Ids([]) # type: ignore[assignment] - - result = await client.get_user_activity(user_id=42) - assert "users" in result - await client.close() - - -async def test_async_get_user_activity_by_username_no_match_raises() -> None: - """``GlpiValidationError`` is raised when no users match the criteria. - - ``GlpiValidationError`` inherits ``ValueError`` so existing callers that - catch the broader type keep working. - """ - - client = make_async_client() - - async def fake_search_users(rsql_filter: str = "", **kwargs: Any) -> list[Any]: - return [] - - client.search_users = fake_search_users # type: ignore[method-assign] - - with pytest.raises(GlpiValidationError, match="No users matched") as excinfo: - await client.get_user_activity(username="ghost") - assert isinstance(excinfo.value, ValueError) - await client.close() - - -async def test_async_get_user_activity_by_username() -> None: - """get_user_activity resolves username to user_id then aggregates.""" - - from glpi_python_client.models.api_schema.administration._user import GetUser - - client = make_async_client() - - async def fake_search_users(rsql_filter: str = "", **kwargs: Any) -> list[Any]: - return [GetUser(id=7, username="alice", realname="A", firstname="B")] - - async def fake_iter_tickets(rsql_filter: str = "", **kwargs: Any) -> Any: - if False: - yield [] - - async def fake_task_durations(**kwargs: Any) -> Any: - from glpi_python_client.clients.custom._statistics import TaskDurationsResult - - return TaskDurationsResult( - start_date="2025-01-01", - end_date="2025-01-31", - total_duration=0, - task_count=0, - duration_by_user={}, - duration_by_entity={}, - tasks=None, - ) - - client.search_users = fake_search_users # type: ignore[method-assign] - client.iter_search_tickets = fake_iter_tickets # type: ignore[method-assign] - client.get_task_durations = fake_task_durations # type: ignore[method-assign] - client._v1 = _FakeV1Ids([]) # type: ignore[assignment] - - result = await client.get_user_activity(username="alice") - assert "users" in result - await client.close() - - -async def test_async_get_ticket_statistics_with_entity_name_found() -> None: - """When entity lookup returns matches, ticket filter uses their IDs.""" - - from glpi_python_client.models.api_schema.administration._entity import GetEntity - - client = make_async_client() - - async def fake_search_entities(rsql_filter: str = "", **kwargs: Any) -> list[Any]: - return [GetEntity(id=10, name="IT")] - - async def fake_search_tickets(rsql_filter: str = "", **kwargs: Any) -> list[Any]: - return [] - - client.search_entities = fake_search_entities # type: ignore[method-assign] - client.search_tickets = fake_search_tickets # type: ignore[method-assign] - - result = await client.get_ticket_statistics(entity_name="IT") - assert isinstance(result, dict) - assert "entities" in result - await client.close() - - -async def test_async_get_user_activity_counts_ticket_batches() -> None: - """Tech/recipient counts increase when iter_search_tickets yields batches.""" - - from glpi_python_client.clients.custom._statistics import TaskDurationsResult - - client = make_async_client() - - async def fake_iter_tickets(rsql_filter: str = "", **kwargs: Any) -> Any: - yield [_StubTicket(1)] - - async def fake_task_durations(**kwargs: Any) -> Any: - return TaskDurationsResult( - start_date="2025-01-01", - end_date="2025-01-31", - total_duration=0, - task_count=0, - duration_by_user={}, - duration_by_entity={}, - tasks=None, - ) - - client.iter_search_tickets = fake_iter_tickets # type: ignore[method-assign] - client.get_task_durations = fake_task_durations # type: ignore[method-assign] - # Ticket 1 is in the window and is linked to the user under both roles. - client._v1 = _FakeV1Ids([1]) # type: ignore[assignment] - - result = await client.get_user_activity(user_id=99) - entry = list(result["users"].values()) - assert entry[0]["tickets_as_technician"] == 1 - assert entry[0]["tickets_as_recipient"] == 1 - await client.close() - - -async def test_async_get_user_activity_counts_are_role_specific() -> None: - """Assignee and requester counts come from independent v1 id sets. - - The previous implementation sent v1 field names to v2, which silently - ignored them, so both counts collapsed to "every ticket in the window" - and were always equal. Here the window holds two tickets but the user - is linked to only one, under one role. - """ - - from glpi_python_client.clients.custom._statistics import TaskDurationsResult - - client = make_async_client() - - async def fake_iter_tickets(rsql_filter: str = "", **kwargs: Any) -> Any: - yield [_StubTicket(1), _StubTicket(2)] - - async def fake_task_durations(**kwargs: Any) -> Any: - return TaskDurationsResult( - start_date="2025-01-01", - end_date="2025-01-31", - total_duration=0, - task_count=0, - duration_by_user={}, - duration_by_entity={}, - tasks=None, - ) - - class _RoleAwareV1: - def request_json(self, method: str, path: str, **kwargs: Any) -> object: - params = kwargs.get("params") or {} - option = int(str(params.get("criteria[0][field]"))) - # searchOption 5 == assignee, 4 == requester. - ids = [1] if option == 5 else [] - rows = [{"2": ticket_id} for ticket_id in ids] - return {"totalcount": len(rows), "data": rows} - - def close(self) -> None: - """No-op; the real session is closed with the client.""" - - client.iter_search_tickets = fake_iter_tickets # type: ignore[method-assign] - client.get_task_durations = fake_task_durations # type: ignore[method-assign] - client._v1 = _RoleAwareV1() # type: ignore[assignment] - - result = await client.get_user_activity(user_id=99) - entry = next(iter(result["users"].values())) - assert entry["tickets_as_technician"] == 1 - assert entry["tickets_as_recipient"] == 0 - await client.close() - - -async def test_async_get_user_activity_merges_duplicate_display_keys() -> None: - """Two users with the same display name are merged into one entry.""" - - from glpi_python_client.clients.custom._statistics import TaskDurationsResult - from glpi_python_client.models.api_schema.administration._user import GetUser - - client = make_async_client() - - # Both users have no firstname/realname → display key is just username "" or id - # Use users with empty names so they get the same display key via str(id) fallback - # Actually easier: give them same realname+firstname so display key collides - user_a = GetUser(id=1, username="a", realname="Smith", firstname="John") - user_b = GetUser(id=2, username="b", realname="Smith", firstname="John") - - async def fake_search_users(rsql_filter: str = "", **kwargs: Any) -> list[Any]: - return [user_a, user_b] - - async def fake_iter_tickets(rsql_filter: str = "", **kwargs: Any) -> Any: - if False: - yield [] - - async def fake_task_durations(**kwargs: Any) -> Any: - return TaskDurationsResult( - start_date="2025-01-01", - end_date="2025-01-31", - total_duration=0, - task_count=0, - duration_by_user={}, - duration_by_entity={}, - tasks=None, - ) - - client.search_users = fake_search_users # type: ignore[method-assign] - client.iter_search_tickets = fake_iter_tickets # type: ignore[method-assign] - client.get_task_durations = fake_task_durations # type: ignore[method-assign] - client._v1 = _FakeV1Ids([]) # type: ignore[assignment] - - result = await client.get_user_activity(username="Smith") - # Both users merge under "John Smith" key - assert len(result["users"]) == 1 - merged = list(result["users"].values()) - assert set(merged[0]["user_ids"]) == {1, 2} - await client.close() diff --git a/glpi_python_client/clients/tests/test_async_selfcall_guard.py b/glpi_python_client/clients/tests/test_async_selfcall_guard.py deleted file mode 100644 index 42fa6a2..0000000 --- a/glpi_python_client/clients/tests/test_async_selfcall_guard.py +++ /dev/null @@ -1,226 +0,0 @@ -"""Structural guard against the async bridge's self-call trap. - -``AsyncBridge`` wraps every public sync method into a coroutine. A sync -body running inside a worker thread that calls a sibling *public* method -through ``self`` therefore receives a coroutine object, not data: the call -is silently dropped (``RuntimeWarning: coroutine ... was never awaited``). - -Any public method that transitively reaches a public method through -``self`` must be given a hand-written async override on -``AsyncGlpiClient``. This test enforces that rule so the bug class cannot -be reintroduced by a future endpoint. -""" - -from __future__ import annotations - -import ast -import inspect -import textwrap - -from glpi_python_client import AsyncGlpiClient, GlpiClient - -# Lifecycle helpers differ between the surfaces on purpose. -_EXCLUDED = {"from_env", "close"} - - -def _public_names(cls: type) -> set[str]: - """Return the public callable names exposed by ``cls``.""" - - return { - name - for name, _ in inspect.getmembers(cls, predicate=callable) - if not name.startswith("_") and name not in _EXCLUDED - } - - -def _self_call_map(cls: type) -> dict[str, set[str]]: - """Map every method of ``cls`` to the ``self.X()`` names it calls.""" - - out: dict[str, set[str]] = {} - for klass in cls.__mro__: - if klass is object: - continue - for name, member in vars(klass).items(): - if name in out: - continue - func = member - if isinstance(func, (classmethod, staticmethod)): - func = func.__func__ - if not inspect.isfunction(func): - continue - try: - tree = ast.parse(textwrap.dedent(inspect.getsource(func))) - except (OSError, TypeError, SyntaxError): # pragma: no cover - continue - calls: set[str] = set() - for node in ast.walk(tree): - if ( - isinstance(node, ast.Call) - and isinstance(node.func, ast.Attribute) - and isinstance(node.func.value, ast.Name) - and node.func.value.id == "self" - ): - calls.add(node.func.attr) - out[name] = calls - return out - - -def _reaches_public( - name: str, - call_map: dict[str, set[str]], - public: set[str], - seen: set[str] | None = None, -) -> bool: - """Return whether ``name`` reaches a public method through ``self``. - - Recurses through private helpers, which is what catches - ``create_kb_article`` -> ``_apply_category_fallback`` -> - ``set_kb_article_categories``. - """ - - if seen is None: - seen = set() - if name in seen: - return False - seen.add(name) - for callee in call_map.get(name, set()): - if callee in public: - return True - if callee.startswith("_") and _reaches_public(callee, call_map, public, seen): - return True - return False - - -def _is_real_async_override(member: object) -> bool: - """Return whether ``member`` is a hand-written async override. - - The bridge builds its wrappers with ``functools.wraps``, so a - bridge-generated coroutine carries ``__wrapped__`` while a real - override does not. - """ - - if not (inspect.iscoroutinefunction(member) or inspect.isasyncgenfunction(member)): - return False - return not hasattr(member, "__wrapped__") - - -def _offenders() -> list[str]: - """Return public methods that self-call but lack an async override.""" - - public = _public_names(GlpiClient) - call_map = _self_call_map(GlpiClient) - return sorted( - name - for name in public - if _reaches_public(name, call_map, public) - and not _is_real_async_override(getattr(AsyncGlpiClient, name)) - ) - - -def test_no_new_self_call_offenders() -> None: - """No public method may self-call without a hand-written async override.""" - - assert _offenders() == [], ( - f"These public methods call a public method through self with no async " - f"override, so AsyncGlpiClient will silently drop those calls: " - f"{_offenders()}. Add an async override mixin (see " - "clients/custom/_statistics_async.py) and register it in " - "clients/async_client.py before the sync mixin." - ) - - -def test_guard_detects_the_covered_methods() -> None: - """The guard must recognise the existing overrides as valid. - - Without this, a guard that classified every method as covered would - pass ``test_no_new_self_call_offenders`` vacuously. - """ - - for name in ("get_ticket_context", "get_task_statistics", "iter_search_tickets"): - assert _is_real_async_override(getattr(AsyncGlpiClient, name)), ( - f"{name} should be a hand-written async override" - ) - assert not _is_real_async_override(AsyncGlpiClient.get_ticket), ( - "get_ticket should be bridge-generated, not a hand-written override" - ) - - -def test_reaches_public_detects_transitive_and_direct_self_calls() -> None: - """Pin ``_reaches_public`` against a synthetic call map. - - ``test_no_new_self_call_offenders`` now asserts ``_offenders() == []``. - That assertion passes both when every real offender has an async - override *and* when ``_reaches_public`` has regressed to returning - ``False`` for everything — the two cases are indistinguishable from the - assertion alone. The previous non-empty ``_KNOWN_UNCOVERED`` constant - protected against that regression by accident, since a broken detector - would drop the known offenders out of ``_offenders()`` and fail the - equality check; emptying it removed that safety net. - - This test replaces it with a direct, synthetic check of the detection - logic itself, independent of whatever offenders exist on the real - client today. It fixes a call map shaped like the historical - ``create_kb_article`` bug — a public method reaching another public - method only transitively, through a private helper — and a call map - that never reaches anything public, and asserts ``_reaches_public`` - still tells them apart. - """ - - public = {"create_thing", "set_thing_categories", "isolated_public"} - call_map: dict[str, set[str]] = { - # Mirrors create_kb_article -> _apply_category_fallback -> - # set_kb_article_categories: the public caller only reaches the - # public callee transitively, through a private helper. - "create_thing": {"_apply_fallback"}, - "_apply_fallback": {"set_thing_categories"}, - "set_thing_categories": set(), - # A public method that only ever touches private helpers which - # themselves reach nothing public must not be flagged. - "isolated_public": {"_do_local_work"}, - "_do_local_work": {"_do_more_local_work"}, - "_do_more_local_work": set(), - } - - assert _reaches_public("create_thing", call_map, public) is True, ( - "a public method reaching a public method through a private helper " - "must be detected" - ) - assert _reaches_public("isolated_public", call_map, public) is False, ( - "a public method whose private helpers reach nothing public must not be flagged" - ) - - -def test_reaches_public_fires_on_real_create_kb_article_shape() -> None: - """Pin all three ``_offenders`` helpers together against the real client. - - ``test_reaches_public_detects_transitive_and_direct_self_calls`` only - proves ``_reaches_public`` is correct against a hand-built ``public`` - set and ``call_map``. It cannot notice a regression in the other two - helpers ``_offenders`` depends on: if ``_public_names(GlpiClient)`` - returned an empty set, or ``_self_call_map(GlpiClient)`` returned an - empty dict, ``_offenders()`` would be ``[]`` regardless of what - ``_reaches_public`` does, and ``test_no_new_self_call_offenders`` would - pass vacuously. ``_self_call_map`` is especially fragile here: it - silently ``continue``s past ``OSError``/``TypeError``/``SyntaxError`` - raised by ``inspect.getsource``, so a source-less install (e.g. a - zipapp or a stripped wheel) would empty the map and green the guard - without detecting anything. - - ``create_kb_article`` is a permanently-valid end-to-end canary for - this: ``glpi_python_client/clients/api/knowledgebase/_article.py`` is - untouched by the async-bridge fix and still has the exact shape that - caused the original bug — public ``create_kb_article`` reaches public - ``set_kb_article_categories`` only transitively, through the private - ``_apply_category_fallback``. Running the three real helpers together - against ``GlpiClient`` and asserting the detector still fires proves - that ``create_kb_article`` is absent from ``_offenders()`` today - because ``AsyncKBArticleMixin`` (in - :mod:`glpi_python_client.clients.api.knowledgebase._article_async`) - now supplies a real async override, not because the detector went - blind. This is the exact protection the old (now-removed) - ``_KNOWN_UNCOVERED`` constant used to provide by accident. - """ - - assert _reaches_public( - "create_kb_article", _self_call_map(GlpiClient), _public_names(GlpiClient) - ), "the detector must still fire on the real create_kb_article shape" diff --git a/glpi_python_client/clients/tests/test_async_smoke.py b/glpi_python_client/clients/tests/test_async_smoke.py deleted file mode 100644 index b605091..0000000 --- a/glpi_python_client/clients/tests/test_async_smoke.py +++ /dev/null @@ -1,150 +0,0 @@ -"""Smoke tests for the asynchronous client and its async bridge. - -The tests exercise a few representative endpoint methods on -:class:`~glpi_python_client.AsyncGlpiClient` to confirm the bridge -correctly: - -* wraps inherited sync methods into awaitable coroutines, -* dispatches the blocking call off the event loop, and -* preserves the synchronous transport call signatures so test recorders - can stub the same hooks as the sync test suite. -""" - -from __future__ import annotations - -from typing import Any - -import pytest - -from glpi_python_client import ( - AsyncGlpiClient, - PostFollowup, - PostTicket, - PostUser, -) -from glpi_python_client.testing.utils import FakeResponse, make_async_client - - -class _AsyncRecorder: - """Synchronous transport recorder installed on the async client. - - The recorder relies on the fact that the async bridge wraps the - inherited synchronous transport hooks; the underlying ``_get_*``, - ``_post_*``, ``_update_*`` and ``_delete_*`` helpers themselves - remain synchronous and run inside the bridge worker thread. - """ - - def __init__(self) -> None: - self.calls: list[dict[str, Any]] = [] - - def install(self, client: AsyncGlpiClient) -> None: - """Replace the transport methods on ``client`` with sync stubs.""" - - def _get( - endpoint: str, - params: dict[str, Any] | None = None, - skip_entity: bool = False, - ) -> FakeResponse: - self.calls.append( - { - "method": "GET", - "endpoint": endpoint, - "params": params, - "skip_entity": skip_entity, - } - ) - return FakeResponse( - status_code=200, payload=[{"id": 1, "name": "n", "content": "c"}] - ) - - def _post( - endpoint: str, - json_body: dict[str, Any] | None = None, - skip_entity: bool = False, - ) -> FakeResponse: - self.calls.append( - { - "method": "POST", - "endpoint": endpoint, - "json": json_body, - "skip_entity": skip_entity, - } - ) - return FakeResponse(status_code=201, payload={"id": 999}) - - client._get_request = _get # type: ignore[method-assign] - client._post_request = _post # type: ignore[method-assign] - - -@pytest.fixture -def async_client() -> AsyncGlpiClient: - """Return one in-memory async client without any real HTTP plumbing.""" - - return make_async_client() - - -@pytest.fixture -def async_recorder(async_client: AsyncGlpiClient) -> _AsyncRecorder: - """Return one transport recorder already wired onto ``async_client``.""" - - rec = _AsyncRecorder() - rec.install(async_client) - return rec - - -async def test_async_create_user_returns_awaitable( - async_client: AsyncGlpiClient, async_recorder: _AsyncRecorder -) -> None: - """The async ``create_user`` returns an awaitable that resolves to the id.""" - - user_id = await async_client.create_user(PostUser(username="alice")) - assert user_id == 999 - assert async_recorder.calls[0]["endpoint"] == "Administration/User" - - -async def test_async_search_tickets_returns_models( - async_client: AsyncGlpiClient, async_recorder: _AsyncRecorder -) -> None: - """The async ``search_tickets`` returns validated ticket models.""" - - tickets = await async_client.search_tickets("status==1") - assert len(tickets) == 1 - assert async_recorder.calls[0]["endpoint"] == "Assistance/Ticket" - - -async def test_async_create_ticket_followup_targets_timeline_endpoint( - async_client: AsyncGlpiClient, async_recorder: _AsyncRecorder -) -> None: - """The async followup helper still hits the timeline endpoint.""" - - await async_client.create_ticket_followup(7, PostFollowup(content="hi
")) - assert ( - async_recorder.calls[0]["endpoint"] == "Assistance/Ticket/7/Timeline/Followup" - ) - - -async def test_async_create_ticket_serialises_enums( - async_client: AsyncGlpiClient, async_recorder: _AsyncRecorder -) -> None: - """The async create_ticket serialises enums identically to the sync surface.""" - - await async_client.create_ticket(PostTicket(name="t", content="c
")) - assert async_recorder.calls[0]["endpoint"] == "Assistance/Ticket" - assert async_recorder.calls[0]["json"]["name"] == "t" - - -async def test_async_close_is_idempotent() -> None: - """Calling ``close`` twice on the async client does not raise.""" - - client = make_async_client() - await client.close() - await client.close() - - -async def test_async_context_manager_closes_client() -> None: - """The async context manager closes the client on exit.""" - - async with make_async_client() as client: - assert client.glpi_api_url.endswith("/api.php") - with pytest.raises(RuntimeError, match="closed"): - client._ensure_open() diff --git a/glpi_python_client/clients/tests/test_parity.py b/glpi_python_client/clients/tests/test_parity.py deleted file mode 100644 index 0e91c29..0000000 --- a/glpi_python_client/clients/tests/test_parity.py +++ /dev/null @@ -1,70 +0,0 @@ -"""Parity tests asserting that the sync and async clients expose the same surface. - -These tests guarantee that any public method added to the sync mixins is -automatically reflected on the async client through -:class:`~glpi_python_client.clients.commons._async_bridge.AsyncBridge` -without requiring a parallel async implementation, and conversely that -the async client does not gain methods the sync client lacks. -""" - -from __future__ import annotations - -import inspect - -from glpi_python_client import AsyncGlpiClient, GlpiClient - - -def _public_callable_names(cls: type) -> set[str]: - """Return the public method names exposed by ``cls``. - - Lifecycle helpers that intentionally differ between the sync and - async surfaces are filtered out. - """ - - excluded = {"from_env", "close"} - return { - name - for name, member in inspect.getmembers(cls, predicate=callable) - if not name.startswith("_") and name not in excluded - } - - -def test_sync_and_async_clients_expose_the_same_public_methods() -> None: - """The async client must expose exactly the same endpoint methods.""" - - sync_names = _public_callable_names(GlpiClient) - async_names = _public_callable_names(AsyncGlpiClient) - assert sync_names == async_names - - -def test_sync_endpoint_methods_are_not_coroutine_functions() -> None: - """Every public sync method is a plain or generator function, not a coroutine.""" - - for name in _public_callable_names(GlpiClient): - member = getattr(GlpiClient, name) - assert not inspect.iscoroutinefunction(member), ( - f"GlpiClient.{name} should be synchronous" - ) - assert not inspect.isasyncgenfunction(member), ( - f"GlpiClient.{name} should be synchronous" - ) - - -def test_async_endpoint_methods_are_coroutine_functions() -> None: - """Every public async method is a coroutine or async generator function.""" - - for name in _public_callable_names(AsyncGlpiClient): - member = getattr(AsyncGlpiClient, name) - is_async = inspect.iscoroutinefunction(member) or inspect.isasyncgenfunction( - member - ) - assert is_async, ( - f"AsyncGlpiClient.{name} should be a coroutine or async generator" - ) - - -def test_async_client_close_is_coroutine_and_sync_is_not() -> None: - """Lifecycle helpers differ on purpose between the two surfaces.""" - - assert not inspect.iscoroutinefunction(GlpiClient.close) - assert inspect.iscoroutinefunction(AsyncGlpiClient.close) diff --git a/glpi_python_client/models/api_schema/_content.py b/glpi_python_client/models/api_schema/_content.py index 2c6448d..6110af4 100644 --- a/glpi_python_client/models/api_schema/_content.py +++ b/glpi_python_client/models/api_schema/_content.py @@ -12,7 +12,7 @@ to canonical Markdown before being assigned to the field, so attribute access always returns Markdown. * On serialisation (outgoing request bodies built via - :func:`glpi_python_client.clients.commons._payloads.model_to_payload`) + :func:`glpi_python_client._sync.clients.commons._payloads.model_to_payload`) the Markdown value is rendered back to HTML so GLPI receives the format it expects. diff --git a/glpi_python_client/models/api_schema/assistance/tests/test_assistance_schemas.py b/glpi_python_client/models/api_schema/assistance/tests/test_assistance_schemas.py index 43d699f..e0eb190 100644 --- a/glpi_python_client/models/api_schema/assistance/tests/test_assistance_schemas.py +++ b/glpi_python_client/models/api_schema/assistance/tests/test_assistance_schemas.py @@ -47,6 +47,35 @@ def test_get_ticket_validates_rich_payload() -> None: assert ticket.team[0].role == "requester" +def test_get_ticket_accepts_the_major_priority_level() -> None: + """A ``Major`` (6) priority ticket validates. + + GLPI's priority scale has six levels while the published contract + advertises five, so ``GetTicket`` used to raise ``ValidationError`` on + any ticket GLPI had escalated to ``Major``. Because validation happens + per record inside a search, one such ticket failed the *entire* query -- + and a reporting query filtering on high priority is precisely where it + would show up. + """ + + ticket = GetTicket.model_validate({"id": 1, "name": "major", "priority": 6}) + assert ticket.priority is GlpiPriority.MAJOR + assert ticket.priority.glpi_id == 6 + + +def test_urgency_and_impact_still_span_one_to_five() -> None: + """The five contract-declared levels keep their identifiers. + + Widening the shared enum must not renumber the levels that were already + correct: these values are sent back to GLPI in filters, so a shift would + silently reinterpret every stored query. + """ + + assert [member.value for member in GlpiPriority] == [1, 2, 3, 4, 5, 6] + assert GlpiPriority.VERY_HIGH.value == 5 + assert GlpiPriority.VERY_HIGH.rsql_equals("priority") == "priority==5" + + def test_post_ticket_excludes_read_only_fields() -> None: """Read-only contract fields are captured in ``extra_payload``. diff --git a/glpi_python_client/models/api_schema/assistance/tests/test_content_roundtrip.py b/glpi_python_client/models/api_schema/assistance/tests/test_content_roundtrip.py index b6aef40..b05e65d 100644 --- a/glpi_python_client/models/api_schema/assistance/tests/test_content_roundtrip.py +++ b/glpi_python_client/models/api_schema/assistance/tests/test_content_roundtrip.py @@ -11,7 +11,7 @@ import pytest -from glpi_python_client.clients.commons._payloads import model_to_payload +from glpi_python_client._sync.clients.commons._payloads import model_to_payload from glpi_python_client.models.api_schema.assistance import ( GetTicket, PatchTicket, diff --git a/glpi_python_client/models/api_schema/enums.py b/glpi_python_client/models/api_schema/enums.py index 9ba071c..475e8d3 100644 --- a/glpi_python_client/models/api_schema/enums.py +++ b/glpi_python_client/models/api_schema/enums.py @@ -69,7 +69,23 @@ class GlpiPriority(GlpiEnum): """Common GLPI urgency, impact, and priority identifiers. The contract advertises the same ``[1, 2, 3, 4, 5]`` enum on the - ``urgency``, ``impact``, and ``priority`` ticket fields. + ``urgency``, ``impact``, and ``priority`` ticket fields, but the live + server does not honour that for ``priority``: GLPI's priority scale has + a sixth level, ``Major``, which it derives from urgency and impact. + ``urgency`` and ``impact`` really do stop at 5. + + :attr:`MAJOR` is therefore included even though the contract omits it, + following the same rule the timeline helpers use -- observed server + behaviour wins over the published contract. Leaving it out was not a + cosmetic gap: ``priority`` is typed with this enum on ``GetTicket``, so + a single ``Major`` ticket anywhere in a result set failed validation and + took the whole search down with it, which is exactly the kind of ticket + a reporting query is most likely to touch. + + The cost of sharing one enum across the three fields is that ``urgency`` + and ``impact`` now also accept 6, which GLPI will never send. Accepting + a value that cannot occur is harmless; rejecting one that does occur was + not. """ VERY_LOW = 1 @@ -77,6 +93,7 @@ class GlpiPriority(GlpiEnum): MEDIUM = 3 HIGH = 4 VERY_HIGH = 5 + MAJOR = 6 class GlpiTicketType(GlpiEnum): diff --git a/glpi_python_client/models/api_schema/knowledgebase/tests/test_article_schema.py b/glpi_python_client/models/api_schema/knowledgebase/tests/test_article_schema.py index 6e92231..8e68e5c 100644 --- a/glpi_python_client/models/api_schema/knowledgebase/tests/test_article_schema.py +++ b/glpi_python_client/models/api_schema/knowledgebase/tests/test_article_schema.py @@ -2,7 +2,7 @@ from __future__ import annotations -from glpi_python_client.clients.commons._payloads import model_to_payload +from glpi_python_client._sync.clients.commons._payloads import model_to_payload from glpi_python_client.models.api_schema._common import IdNameRef from glpi_python_client.models.api_schema.knowledgebase import ( DeleteKBArticle, diff --git a/glpi_python_client/models/api_schema/knowledgebase/tests/test_comment_schema.py b/glpi_python_client/models/api_schema/knowledgebase/tests/test_comment_schema.py index ee6f144..3aaa20e 100644 --- a/glpi_python_client/models/api_schema/knowledgebase/tests/test_comment_schema.py +++ b/glpi_python_client/models/api_schema/knowledgebase/tests/test_comment_schema.py @@ -2,7 +2,7 @@ from __future__ import annotations -from glpi_python_client.clients.commons._payloads import model_to_payload +from glpi_python_client._sync.clients.commons._payloads import model_to_payload from glpi_python_client.models.api_schema._common import IdNameRef from glpi_python_client.models.api_schema.knowledgebase import ( DeleteKBArticleComment, diff --git a/glpi_python_client/models/api_schema/plugins/_fields.py b/glpi_python_client/models/api_schema/plugins/_fields.py index f5f5260..8ac682f 100644 --- a/glpi_python_client/models/api_schema/plugins/_fields.py +++ b/glpi_python_client/models/api_schema/plugins/_fields.py @@ -5,7 +5,7 @@ problems, ...). It is configured through two top-level itemtypes: * :class:`GetPluginFieldsContainer` — one container per "tab" or "block" - added to an itemtype (e.g. an ``Aide à la résolution`` tab on + added to an itemtype (e.g. an ``Extra information`` tab on ``Ticket``). * :class:`GetPluginFieldsField` — one user-defined field declaration belonging to a container (the column name, type, default value, ...). @@ -13,7 +13,7 @@ For each container, the plugin creates a dedicated itemtype that stores one row per "item + container" pair. The itemtype name is built as ``PluginFieldstest
", + "extrainfofield": "test
", } ) - assert row.items_id == 62571 - assert row.extra_payload["aidelarsolutionfield"] == "test
" + assert row.items_id == 1234 + assert row.extra_payload["extrainfofield"] == "test
" def test_post_value_row_carries_dynamic_columns() -> None: """The POST body accepts dynamic field columns via ``extra_payload``.""" body = PostPluginFieldsValueRow( - items_id=62571, + items_id=1234, itemtype="Ticket", plugin_fields_containers_id=10, - extra_payload={"aidelarsolutionfield": "x
"}, + extra_payload={"extrainfofield": "x
"}, ) - assert body.extra_payload == {"aidelarsolutionfield": "x
"} + assert body.extra_payload == {"extrainfofield": "x
"} diff --git a/glpi_python_client/testing/utils.py b/glpi_python_client/testing/utils.py index a939225..05e9786 100644 --- a/glpi_python_client/testing/utils.py +++ b/glpi_python_client/testing/utils.py @@ -20,10 +20,15 @@ class FakeResponse: - """Small ``requests.Response`` stand-in for unit tests. + """Small HTTP-response stand-in for unit tests. The fake object implements only the attributes and ``json()`` behavior used - by the package's tests. + by the package's tests. It is duck-typed rather than a subclass of the + transport's response class, so it survived the move from ``requests`` to + ``httpx`` unchanged: the library reads the reason phrase through + :func:`~glpi_python_client._async.clients.commons._http.response_reason`, which + accepts either the ``reason`` spelling used here or the ``reason_phrase`` + spelling ``httpx`` uses. """ def __init__( diff --git a/glpi_python_client/clients/api/knowledgebase/tests/__init__.py b/glpi_python_client/tests/api_knowledgebase/__init__.py similarity index 100% rename from glpi_python_client/clients/api/knowledgebase/tests/__init__.py rename to glpi_python_client/tests/api_knowledgebase/__init__.py diff --git a/glpi_python_client/clients/api/knowledgebase/tests/test_article_mixin.py b/glpi_python_client/tests/api_knowledgebase/test_article_mixin.py similarity index 100% rename from glpi_python_client/clients/api/knowledgebase/tests/test_article_mixin.py rename to glpi_python_client/tests/api_knowledgebase/test_article_mixin.py diff --git a/glpi_python_client/clients/api/knowledgebase/tests/test_category_mixin.py b/glpi_python_client/tests/api_knowledgebase/test_category_mixin.py similarity index 100% rename from glpi_python_client/clients/api/knowledgebase/tests/test_category_mixin.py rename to glpi_python_client/tests/api_knowledgebase/test_category_mixin.py diff --git a/glpi_python_client/clients/api/knowledgebase/tests/test_comment_mixin.py b/glpi_python_client/tests/api_knowledgebase/test_comment_mixin.py similarity index 100% rename from glpi_python_client/clients/api/knowledgebase/tests/test_comment_mixin.py rename to glpi_python_client/tests/api_knowledgebase/test_comment_mixin.py diff --git a/glpi_python_client/clients/api/knowledgebase/tests/test_kb_failure_paths.py b/glpi_python_client/tests/api_knowledgebase/test_kb_failure_paths.py similarity index 100% rename from glpi_python_client/clients/api/knowledgebase/tests/test_kb_failure_paths.py rename to glpi_python_client/tests/api_knowledgebase/test_kb_failure_paths.py diff --git a/glpi_python_client/clients/api/knowledgebase/tests/test_revision_mixin.py b/glpi_python_client/tests/api_knowledgebase/test_revision_mixin.py similarity index 100% rename from glpi_python_client/clients/api/knowledgebase/tests/test_revision_mixin.py rename to glpi_python_client/tests/api_knowledgebase/test_revision_mixin.py diff --git a/glpi_python_client/clients/api/plugins/tests/__init__.py b/glpi_python_client/tests/api_plugins/__init__.py similarity index 100% rename from glpi_python_client/clients/api/plugins/tests/__init__.py rename to glpi_python_client/tests/api_plugins/__init__.py diff --git a/glpi_python_client/clients/api/plugins/tests/test_fields_mixin.py b/glpi_python_client/tests/api_plugins/test_fields_mixin.py similarity index 80% rename from glpi_python_client/clients/api/plugins/tests/test_fields_mixin.py rename to glpi_python_client/tests/api_plugins/test_fields_mixin.py index fce847a..0115f72 100644 --- a/glpi_python_client/clients/api/plugins/tests/test_fields_mixin.py +++ b/glpi_python_client/tests/api_plugins/test_fields_mixin.py @@ -7,7 +7,7 @@ import pytest from glpi_python_client import GlpiClient, GlpiProtocolError, GlpiValidationError -from glpi_python_client.clients.api.plugins._fields import ( +from glpi_python_client._sync.clients.api.plugins._fields import ( _container_targets_itemtype, _extract_row_id, _value_itemtype_for, @@ -62,10 +62,7 @@ def client() -> GlpiClient: def test_value_itemtype_naming() -> None: """The value itemtype is built from parent type + lowercase container.""" - assert ( - _value_itemtype_for("Ticket", "aidelarsolution") - == "PluginFieldsTicketaidelarsolution" - ) + assert _value_itemtype_for("Ticket", "extrainfo") == "PluginFieldsTicketextrainfo" # Mixed-case names get normalised to lowercase to match the v1 routes. assert ( _value_itemtype_for("Ticket", "MyContainer") == "PluginFieldsTicketmycontainer" @@ -169,19 +166,19 @@ def test_list_item_plugin_field_rows_hits_subresource(client: GlpiClient) -> Non [ { "id": 1, - "items_id": 62571, + "items_id": 1234, "itemtype": "Ticket", "plugin_fields_containers_id": 10, "entities_id": 0, - "aidelarsolutionfield": "test
", + "extrainfofield": "test
", } ] ] ) client._v1 = fake # type: ignore[assignment] - rows = client.list_item_plugin_field_rows("Ticket", 62571, "aidelarsolution") - assert rows[0].extra_payload == {"aidelarsolutionfield": "test
"} - assert fake.calls[0]["path"] == "Ticket/62571/PluginFieldsTicketaidelarsolution" + rows = client.list_item_plugin_field_rows("Ticket", 1234, "extrainfo") + assert rows[0].extra_payload == {"extrainfofield": "test
"} + assert fake.calls[0]["path"] == "Ticket/1234/PluginFieldsTicketextrainfo" def test_create_item_plugin_field_row_returns_new_id(client: GlpiClient) -> None: @@ -193,20 +190,20 @@ def test_create_item_plugin_field_row_returns_new_id(client: GlpiClient) -> None itemtype="Ticket", items_id=99, container_id=10, - container_name="aidelarsolution", - values={"aidelarsolutionfield": "x
"}, + container_name="extrainfo", + values={"extrainfofield": "x
"}, entities_id=3, ) assert row_id == 7 call = fake.calls[0] assert call["method"] == "POST" - assert call["path"] == "PluginFieldsTicketaidelarsolution" + assert call["path"] == "PluginFieldsTicketextrainfo" assert call["json_body"] == { "input": { "items_id": 99, "itemtype": "Ticket", "plugin_fields_containers_id": 10, - "aidelarsolutionfield": "x
", + "extrainfofield": "x
", "entities_id": 3, } } @@ -219,16 +216,14 @@ def test_update_item_plugin_field_row_puts_partial_body(client: GlpiClient) -> N client._v1 = fake # type: ignore[assignment] client.update_item_plugin_field_row( itemtype="Ticket", - container_name="aidelarsolution", + container_name="extrainfo", row_id=1, - values={"aidelarsolutionfield": "updated
"}, + values={"extrainfofield": "updated
"}, ) call = fake.calls[0] assert call["method"] == "PUT" - assert call["path"] == "PluginFieldsTicketaidelarsolution/1" - assert call["json_body"] == { - "input": {"id": 1, "aidelarsolutionfield": "updated
"} - } + assert call["path"] == "PluginFieldsTicketextrainfo/1" + assert call["json_body"] == {"input": {"id": 1, "extrainfofield": "updated
"}} def test_get_ticket_custom_fields_aggregates_containers(client: GlpiClient) -> None: @@ -238,28 +233,28 @@ def test_get_ticket_custom_fields_aggregates_containers(client: GlpiClient) -> N responses=[ # 1. list_plugin_fields_containers(Ticket) [ - {"id": 10, "name": "aidelarsolution", "itemtypes": '["Ticket"]'}, - {"id": 2, "name": "sige", "itemtypes": '["Ticket"]'}, + {"id": 10, "name": "extrainfo", "itemtypes": '["Ticket"]'}, + {"id": 2, "name": "secondary", "itemtypes": '["Ticket"]'}, {"id": 3, "name": "ignored", "itemtypes": '["Computer"]'}, ], - # 2. list_item_plugin_field_rows aidelarsolution + # 2. list_item_plugin_field_rows extrainfo [ { "id": 1, - "items_id": 62571, + "items_id": 1234, "itemtype": "Ticket", "plugin_fields_containers_id": 10, "entities_id": 0, - "aidelarsolutionfield": "test
", + "extrainfofield": "test
", } ], - # 3. list_item_plugin_field_rows sige -> no row yet + # 3. list_item_plugin_field_rows secondary -> no row yet [], ] ) client._v1 = fake # type: ignore[assignment] - result = client.get_ticket_custom_fields(62571) - assert result == {"aidelarsolution": {"aidelarsolutionfield": "test
"}} + result = client.get_ticket_custom_fields(1234) + assert result == {"extrainfo": {"extrainfofield": "test
"}} def test_set_ticket_custom_fields_updates_existing_row(client: GlpiClient) -> None: @@ -269,13 +264,13 @@ def test_set_ticket_custom_fields_updates_existing_row(client: GlpiClient) -> No responses=[ # list containers [ - {"id": 10, "name": "aidelarsolution", "itemtypes": '["Ticket"]'}, + {"id": 10, "name": "extrainfo", "itemtypes": '["Ticket"]'}, ], # list fields for container 10 [ { "id": 11, - "name": "aidelarsolutionfield", + "name": "extrainfofield", "plugin_fields_containers_id": 10, } ], @@ -283,7 +278,7 @@ def test_set_ticket_custom_fields_updates_existing_row(client: GlpiClient) -> No [ { "id": 1, - "items_id": 62571, + "items_id": 1234, "itemtype": "Ticket", "plugin_fields_containers_id": 10, } @@ -294,14 +289,12 @@ def test_set_ticket_custom_fields_updates_existing_row(client: GlpiClient) -> No ) client._v1 = fake # type: ignore[assignment] client.set_ticket_custom_fields( - 62571, {"aidelarsolution": {"aidelarsolutionfield": "new
"}} + 1234, {"extrainfo": {"extrainfofield": "new
"}} ) methods = [c["method"] for c in fake.calls] assert methods == ["GET", "GET", "GET", "PUT"] put = fake.calls[-1] - assert put["json_body"] == { - "input": {"id": 1, "aidelarsolutionfield": "new
"} - } + assert put["json_body"] == {"input": {"id": 1, "extrainfofield": "new
"}} def test_set_ticket_custom_fields_creates_when_missing(client: GlpiClient) -> None: @@ -310,12 +303,12 @@ def test_set_ticket_custom_fields_creates_when_missing(client: GlpiClient) -> No fake = _FakeV1( responses=[ [ - {"id": 10, "name": "aidelarsolution", "itemtypes": '["Ticket"]'}, + {"id": 10, "name": "extrainfo", "itemtypes": '["Ticket"]'}, ], [ { "id": 11, - "name": "aidelarsolutionfield", + "name": "extrainfofield", "plugin_fields_containers_id": 10, } ], @@ -325,17 +318,17 @@ def test_set_ticket_custom_fields_creates_when_missing(client: GlpiClient) -> No ) client._v1 = fake # type: ignore[assignment] client.set_ticket_custom_fields( - 62571, {"aidelarsolution": {"aidelarsolutionfield": "new
"}} + 1234, {"extrainfo": {"extrainfofield": "new
"}} ) methods = [c["method"] for c in fake.calls] assert methods == ["GET", "GET", "GET", "POST"] post = fake.calls[-1] assert post["json_body"] == { "input": { - "items_id": 62571, + "items_id": 1234, "itemtype": "Ticket", "plugin_fields_containers_id": 10, - "aidelarsolutionfield": "new
", + "extrainfofield": "new
", } } @@ -352,7 +345,7 @@ def test_set_ticket_custom_fields_rejects_unknown_container(client: GlpiClient) with pytest.raises( GlpiValidationError, match="Unknown plugin-fields container" ) as excinfo: - client.set_ticket_custom_fields(62571, {"typo": {"any": "value"}}) + client.set_ticket_custom_fields(1234, {"typo": {"any": "value"}}) # No mutation was sent. assert all(c["method"] == "GET" for c in fake.calls) assert isinstance(excinfo.value, ValueError) @@ -364,18 +357,18 @@ def test_set_ticket_custom_fields_rejects_container_without_id( """A matched container with no ``id`` raises before any write. The container came from the server's own - :meth:`~glpi_python_client.clients.api.plugins._fields.PluginFieldsMixin.list_plugin_fields_containers` + :meth:`~glpi_python_client._sync.clients.api.plugins._fields.PluginFieldsMixin.list_plugin_fields_containers` response, so a missing ``id`` is a server-side contract violation, not a caller mistake: ``GlpiProtocolError``. It still inherits ``ValueError`` so existing callers that catch the broader type keep working. """ - fake = _FakeV1(responses=[[{"name": "aidelarsolution", "itemtypes": '["Ticket"]'}]]) + fake = _FakeV1(responses=[[{"name": "extrainfo", "itemtypes": '["Ticket"]'}]]) client._v1 = fake # type: ignore[assignment] with pytest.raises(GlpiProtocolError, match="has no id") as excinfo: client.set_ticket_custom_fields( - 62571, {"aidelarsolution": {"aidelarsolutionfield": "value"}} + 1234, {"extrainfo": {"extrainfofield": "value"}} ) assert all(c["method"] == "GET" for c in fake.calls) assert isinstance(excinfo.value, ValueError) @@ -390,11 +383,11 @@ def test_set_ticket_custom_fields_rejects_unknown_field(client: GlpiClient) -> N fake = _FakeV1( responses=[ - [{"id": 10, "name": "aidelarsolution", "itemtypes": '["Ticket"]'}], + [{"id": 10, "name": "extrainfo", "itemtypes": '["Ticket"]'}], [ { "id": 11, - "name": "aidelarsolutionfield", + "name": "extrainfofield", "plugin_fields_containers_id": 10, } ], @@ -402,7 +395,7 @@ def test_set_ticket_custom_fields_rejects_unknown_field(client: GlpiClient) -> N ) client._v1 = fake # type: ignore[assignment] with pytest.raises(GlpiValidationError, match="Unknown field") as excinfo: - client.set_ticket_custom_fields(62571, {"aidelarsolution": {"typo": "value"}}) + client.set_ticket_custom_fields(1234, {"extrainfo": {"typo": "value"}}) assert isinstance(excinfo.value, ValueError) @@ -413,5 +406,5 @@ def test_set_ticket_custom_fields_with_empty_mapping_is_noop( fake = _FakeV1(responses=[]) client._v1 = fake # type: ignore[assignment] - client.set_ticket_custom_fields(62571, {}) + client.set_ticket_custom_fields(1234, {}) assert fake.calls == [] diff --git a/glpi_python_client/tests/auth/__init__.py b/glpi_python_client/tests/auth/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/glpi_python_client/auth/tests/test_auth.py b/glpi_python_client/tests/auth/test_auth.py similarity index 93% rename from glpi_python_client/auth/tests/test_auth.py rename to glpi_python_client/tests/auth/test_auth.py index 99e1fb9..8bed08b 100644 --- a/glpi_python_client/auth/tests/test_auth.py +++ b/glpi_python_client/tests/auth/test_auth.py @@ -3,12 +3,17 @@ from datetime import datetime, timedelta, timezone from typing import cast +import httpx import pytest -import requests from tenacity import wait_fixed -from glpi_python_client import GlpiAuthError, GlpiServerError, GlpiValidationError -from glpi_python_client.auth.auth import GLPITokenManager +from glpi_python_client import ( + GlpiAuthError, + GlpiServerError, + GlpiTransportError, + GlpiValidationError, +) +from glpi_python_client._sync.auth.auth import GLPITokenManager from glpi_python_client.testing.utils import FakeResponse, TokenResponse @@ -33,7 +38,7 @@ def test_token_manager_uses_password_grant_with_user_credentials_only() -> None: token_url="https://glpi.example.test/api.php/token", username="api-user", password="api-password", - session=cast(requests.Session, session), + session=cast(httpx.Client, session), ) auth._acquire_token() @@ -53,7 +58,7 @@ def test_token_manager_uses_client_credentials_grant() -> None: token_url="https://glpi.example.test/api.php/token", client_id="client-id", client_secret="client-secret", - session=cast(requests.Session, session), + session=cast(httpx.Client, session), ) auth._acquire_token() @@ -73,7 +78,7 @@ def test_token_manager_preserves_raw_credential_text() -> None: token_url="https://glpi.example.test/api.php/token", client_id=" client-id ", client_secret=" client-secret ", - session=cast(requests.Session, session), + session=cast(httpx.Client, session), ) auth._acquire_token() @@ -94,7 +99,7 @@ def test_token_manager_uses_password_grant_with_both_credential_sets() -> None: client_secret="client-secret", username="api-user", password="api-password", - session=cast(requests.Session, session), + session=cast(httpx.Client, session), ) auth._acquire_token() @@ -115,7 +120,7 @@ def test_token_manager_refreshes_when_configured_interval_elapses() -> None: token_url="https://glpi.example.test/api.php/token", client_id="client-id", client_secret="client-secret", - session=cast(requests.Session, session), + session=cast(httpx.Client, session), auth_token_refresh=60, ) auth.access_token = "old-token" @@ -211,7 +216,7 @@ def test_oauth_401_raises_glpi_auth_error() -> None: token_url="https://glpi.example.test/api.php/token", client_id="client-id", client_secret="wrong", - session=cast(requests.Session, session), + session=cast(httpx.Client, session), ) with pytest.raises(GlpiAuthError) as excinfo: manager.ensure_token() @@ -230,7 +235,7 @@ def test_oauth_401_is_not_retried() -> None: token_url="https://glpi.example.test/api.php/token", client_id="client-id", client_secret="wrong", - session=cast(requests.Session, session), + session=cast(httpx.Client, session), ) with pytest.raises(GlpiAuthError): manager.ensure_token() @@ -249,7 +254,7 @@ def test_oauth_5xx_raises_glpi_server_error_after_retries( token_url="https://glpi.example.test/api.php/token", client_id="client-id", client_secret="client-secret", - session=cast(requests.Session, session), + session=cast(httpx.Client, session), ) with pytest.raises(GlpiServerError) as excinfo: manager.ensure_token() @@ -273,7 +278,7 @@ def _make_refresh_ready_manager( token_url="https://glpi.example.test/api.php/token", client_id="client-id", client_secret="client-secret", - session=cast(requests.Session, session), + session=cast(httpx.Client, session), ) manager.access_token = "stale-token" manager.refresh_token = "refresh-token" @@ -324,7 +329,7 @@ def test_refresh_5xx_persistent_costs_one_refresh_plus_nested_acquire_attempts( call on any non-2xx response instead of raising directly (auth.py:327-332). That nested call is independently decorated with ``stop_after_attempt(3)`` and retries ``GlpiServerError``. This method's - own decorator only matches ``requests.RequestException`` (a genuine + own decorator only matches ``httpx.HTTPError`` (a genuine network fault on the refresh POST itself), not ``GlpiServerError``, so it does not retry the fall-through a second time on top of the nested call's own retries. @@ -369,17 +374,17 @@ def __init__(self) -> None: def post(self, url: str, data: dict[str, str], timeout: int) -> FakeResponse: self.calls.append({"url": url, "data": data, "timeout": timeout}) - raise requests.ConnectionError("network down") + raise httpx.ConnectError("network down") session = _FailingSession() manager = GLPITokenManager( token_url="https://glpi.example.test/api.php/token", client_id="client-id", client_secret="client-secret", - session=cast(requests.Session, session), + session=cast(httpx.Client, session), ) - with pytest.raises(requests.ConnectionError): + with pytest.raises(GlpiTransportError): manager.ensure_token() assert len(session.calls) == 3 @@ -396,7 +401,8 @@ def test_refresh_network_error_is_retried_three_times( ``test_refresh_5xx_persistent_costs_one_refresh_plus_nested_acquire_attempts`` above, and ``_acquire_token``'s network retry is pinned by ``test_acquire_token_network_error_is_retried_three_times``. A - ``requests.ConnectionError`` raised by ``session.post`` propagates + ``httpx.ConnectError`` raised by ``session.post`` is translated to + ``GlpiTransportError`` and propagates *before* ``_refresh_access_token`` reaches its non-2xx fallthrough branch (auth.py:327-332), so the nested ``_acquire_token`` call is never reached here -- unlike the persistent-5xx case, this pins @@ -416,12 +422,12 @@ def __init__(self) -> None: def post(self, url: str, data: dict[str, str], timeout: int) -> FakeResponse: self.calls.append({"url": url, "data": data, "timeout": timeout}) - raise requests.ConnectionError("network down") + raise httpx.ConnectError("network down") session = _FailingSession() manager = _make_refresh_ready_manager(cast(_FakeSession, session)) - with pytest.raises(requests.ConnectionError): + with pytest.raises(GlpiTransportError): manager.ensure_token() assert len(session.calls) == 3 diff --git a/glpi_python_client/auth/tests/test_v1_session.py b/glpi_python_client/tests/auth/test_v1_session.py similarity index 95% rename from glpi_python_client/auth/tests/test_v1_session.py rename to glpi_python_client/tests/auth/test_v1_session.py index 98cacf9..f5de54b 100644 --- a/glpi_python_client/auth/tests/test_v1_session.py +++ b/glpi_python_client/tests/auth/test_v1_session.py @@ -5,11 +5,16 @@ import json as jsonlib from typing import Any, cast +import httpx import pytest -import requests -from glpi_python_client import GlpiProtocolError, GlpiServerError, GlpiValidationError -from glpi_python_client.auth._v1_session import GLPIV1Session +from glpi_python_client import ( + GlpiProtocolError, + GlpiServerError, + GlpiTransportError, + GlpiValidationError, +) +from glpi_python_client._sync.auth._v1_session import GLPIV1Session from glpi_python_client.testing.utils import FakeResponse @@ -21,7 +26,7 @@ def _no_sleep(monkeypatch: pytest.MonkeyPatch) -> None: class _FakeV1Http: - """In-memory ``requests.Session`` stand-in capturing every call.""" + """In-memory ``httpx.Client`` stand-in capturing every call.""" def __init__(self, responses: dict[str, list[FakeResponse]]) -> None: self._responses = responses @@ -44,7 +49,7 @@ def request( ``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. + because that is the one call shape every transport agrees on. This dispatches back to the per-verb handlers so their recorded call shapes stay identical. """ @@ -153,7 +158,7 @@ def _make(http: _FakeV1Http) -> GLPIV1Session: app_token="app-token", verify_ssl=True, ) - session._http = cast(requests.Session, http) # type: ignore[assignment] + session._http = cast(httpx.Client, http) # type: ignore[assignment] return session @@ -373,7 +378,7 @@ def test_v1_close_tolerates_kill_failure() -> None: class _BoomHttp(_FakeV1Http): def get(self, url: str, headers: dict[str, str], timeout: int) -> FakeResponse: if url.endswith("/killSession"): - raise requests.RequestException("boom") + raise httpx.RequestError("boom") return super().get(url, headers, timeout) http = _BoomHttp( @@ -406,7 +411,7 @@ def test_request_json_sends_body_and_returns_parsed_payload() -> None: assert result == {"ok": True} post_call = next(call for call in http.calls if call["method"] == "POST") assert post_call["url"].endswith("/PluginFieldsContainer") - assert post_call["data"] == jsonlib.dumps({"input": {"name": "x"}}) + assert post_call["content"] == jsonlib.dumps({"input": {"name": "x"}}) assert post_call["headers"]["Content-Type"] == "application/json" @@ -475,11 +480,11 @@ def test_request_json_retries_on_5xx() -> None: def test_request_json_retries_on_network_error() -> None: """Network faults during ``request_json`` are retried 3x, not swallowed. - Pins the ``requests.RequestException`` member of the v1 retry predicate + Pins the ``GlpiTransportError`` member of the v1 retry predicate (``_RETRY_ON_NETWORK_ERRORS`` in ``_v1_session.py``): the 5xx tests above only exercise the ``GlpiServerError`` member. Without this test a future - edit that narrows the predicate to drop ``requests.RequestException`` - (for example when plan 3 swaps in ``GlpiTransportError``) would silently + edit that narrows the predicate to drop ``GlpiTransportError`` + would silently drop v1 network retries from 3 attempts to 1 while every committed test stayed green. """ @@ -502,7 +507,7 @@ def get( **kwargs, } ) - raise requests.ConnectionError("network down") + raise httpx.ConnectError("network down") return super().get(url, headers, timeout, **kwargs) http = _FlakyHttp( @@ -511,7 +516,7 @@ def get( } ) session = _make(http) - with pytest.raises(requests.ConnectionError): + with pytest.raises(GlpiTransportError): session.request_json("GET", "PluginFieldsContainer") json_calls = [c for c in http.calls if c["url"].endswith("/PluginFieldsContainer")] assert len(json_calls) == 3 diff --git a/glpi_python_client/clients/tests/__init__.py b/glpi_python_client/tests/clients/__init__.py similarity index 100% rename from glpi_python_client/clients/tests/__init__.py rename to glpi_python_client/tests/clients/__init__.py diff --git a/glpi_python_client/clients/tests/test_api_coverage.py b/glpi_python_client/tests/clients/test_api_coverage.py similarity index 100% rename from glpi_python_client/clients/tests/test_api_coverage.py rename to glpi_python_client/tests/clients/test_api_coverage.py diff --git a/glpi_python_client/clients/tests/test_glpi_client.py b/glpi_python_client/tests/clients/test_glpi_client.py similarity index 89% rename from glpi_python_client/clients/tests/test_glpi_client.py rename to glpi_python_client/tests/clients/test_glpi_client.py index f1114ca..8ca2d60 100644 --- a/glpi_python_client/clients/tests/test_glpi_client.py +++ b/glpi_python_client/tests/clients/test_glpi_client.py @@ -8,7 +8,7 @@ import pytest from glpi_python_client import GlpiClient, GlpiValidationError -from glpi_python_client.clients.commons._config import ( +from glpi_python_client._sync.clients.commons._config import ( build_client_env_config, normalize_client_api_url, parse_optional_env_bool, @@ -267,28 +267,37 @@ def test_async_transport_ensure_open_blocks_after_close() -> None: client._ensure_open() -def test_glpi_client_init_failure_closes_session( +def test_glpi_client_init_failure_creates_no_session( monkeypatch: pytest.MonkeyPatch, ) -> None: - """A failing token-manager init releases the underlying session.""" + """A bad credential set is rejected before any session is constructed. - closed: dict[str, Any] = {} + This used to build the session first and unwind it from an ``except`` + clause. That shape is not expressible on the async surface -- an + ``httpx.AsyncClient`` has no synchronous close and a constructor cannot + await one -- so the configuration is now validated up front instead. - import requests + Asserting *nothing was built* is the stronger property: there is no + window in which a session exists but the client does not, so there is + nothing that can leak if the unwind is ever missed. + """ + + import httpx - original_close = requests.Session.close + constructed: list[object] = [] + original_init = httpx.Client.__init__ - def _track_close(self: requests.Session) -> None: - closed["closed"] = True - original_close(self) + def _track_init(self: httpx.Client, *args: Any, **kwargs: Any) -> None: + constructed.append(self) + original_init(self, *args, **kwargs) - monkeypatch.setattr(requests.Session, "close", _track_close) + monkeypatch.setattr(httpx.Client, "__init__", _track_init) with pytest.raises(ValueError): GlpiClient( glpi_api_url="https://glpi.example.test/api.php/v2", client_id="only-id-no-secret", ) - assert closed.get("closed") is True + assert constructed == [], "a transport session was built for a rejected config" def test_no_other_vars_leak_into_environ_test() -> None: diff --git a/glpi_python_client/clients/tests/test_method_invocation.py b/glpi_python_client/tests/clients/test_method_invocation.py similarity index 81% rename from glpi_python_client/clients/tests/test_method_invocation.py rename to glpi_python_client/tests/clients/test_method_invocation.py index 49dc545..1fecef7 100644 --- a/glpi_python_client/clients/tests/test_method_invocation.py +++ b/glpi_python_client/tests/clients/test_method_invocation.py @@ -89,6 +89,12 @@ def json(self) -> Any: return self._payload +def _stub_v1_payload() -> Any: + """Both shapes the v1 callers expect from a JSON response.""" + + return [{"id": 1, "name": "stub", "itemtypes": '["Ticket"]'}] + + class _StubV1: """Stand-in for the legacy v1 session, logging into the shared call log.""" @@ -97,9 +103,7 @@ def __init__(self, calls: list[str]) -> None: 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"]'}] + return _stub_v1_payload() def upload_document(self, *args: Any, **kwargs: Any) -> int: self._calls.append("v1 POST Document") @@ -109,23 +113,60 @@ def close(self) -> None: """No-op; the real session is closed with the client.""" +class _AsyncStubV1(_StubV1): + """Async twin of :class:`_StubV1`. + + The async client awaits every v1 call, so the stub's methods have to be + coroutines. Returning a plain value would surface as ``TypeError: object + NoneType can't be used in 'await' expression`` -- a failure about the + stub, not about the client. + """ + + async def request_json(self, method: str, path: str, **kwargs: Any) -> Any: + self._calls.append(f"v1 {method} {path}") + return _stub_v1_payload() + + async def upload_document(self, *args: Any, **kwargs: Any) -> int: + self._calls.append("v1 POST Document") + return 1 + + async def close(self) -> None: + """No-op; the real session is closed with the client.""" + + +def _stub_response_for(method: str, url: str) -> _StubResponse: + """Build the response shape the endpoint at ``url`` expects.""" + + # 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) + + def _install_stub(client: GlpiClient | AsyncGlpiClient) -> list[str]: - """Route every HTTP call to an in-memory stub; return the call log.""" + """Route every HTTP call to an in-memory stub; return the call log. + + The stub is installed at the ``session.request`` seam on both surfaces, + so URL building, headers, parameter normalisation, response validation + and model parsing all execute for real -- only the socket is replaced. + """ calls: list[str] = [] + is_async = isinstance(client, AsyncGlpiClient) 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] + return _stub_response_for(method, url) + + async def _arequest(method: str, url: str, **kwargs: Any) -> _StubResponse: + calls.append(f"{method} {url}") + return _stub_response_for(method, url) + + client._session.request = _arequest if is_async else _request # type: ignore[method-assign,union-attr,assignment] # 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" @@ -133,7 +174,7 @@ def _request(method: str, url: str, **kwargs: Any) -> _StubResponse: # 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] + client._v1 = (_AsyncStubV1 if is_async else _StubV1)(calls) # type: ignore[assignment] return calls diff --git a/glpi_python_client/clients/tests/test_raise_site_audit.py b/glpi_python_client/tests/clients/test_raise_site_audit.py similarity index 82% rename from glpi_python_client/clients/tests/test_raise_site_audit.py rename to glpi_python_client/tests/clients/test_raise_site_audit.py index b599a9f..a0be8a3 100644 --- a/glpi_python_client/clients/tests/test_raise_site_audit.py +++ b/glpi_python_client/tests/clients/test_raise_site_audit.py @@ -28,6 +28,9 @@ "GlpiServerError", "GlpiStatusError", "error_class", # status_error_class(...) dispatch result + # transport_error_from(...) returns GlpiTransportError or its + # GlpiTimeoutError subclass; the AST sees the factory, not the class. + "transport_error_from", "RuntimeError", # exempt by design -- see module docstring "TypeError", # exempt by design -- see module docstring } @@ -76,13 +79,14 @@ def test_the_ast_walk_finds_a_known_raise_site() -> None: sites = _raise_sites() assert sites, "the raise-site walk found nothing -- it is broken" - # clients/commons/_transport.py:106 raises a deliberately-exempt + # clients/commons/_transport.py raises a deliberately-exempt # RuntimeError (decision D3) that this migration never touches, making # it a stable landmark to confirm the walk actually inspects source. transport_sites = [ site for site in sites - if site[0] == "clients/commons/_transport.py" and site[2] == "RuntimeError" + if site[0].endswith("clients/commons/_transport.py") + and site[2] == "RuntimeError" ] assert transport_sites, ( "the raise-site walk did not find the known RuntimeError raise in " @@ -98,11 +102,19 @@ def test_no_bare_value_error_is_raised_by_library_code() -> None: assert offenders == [], f"bare ValueError raise sites remain: {offenders}" -def test_no_requests_exception_is_raised_by_library_code() -> None: - """The library never raises a third-party HTTP exception directly.""" +def test_no_third_party_http_exception_is_raised_by_library_code() -> None: + """The library never raises a third-party HTTP exception directly. - offenders = [site for site in _raise_sites() if site[2].startswith("requests.")] - assert offenders == [], f"requests exception raise sites remain: {offenders}" + Both spellings are checked, not just the transport currently in use: the + point of the audit is that a raise site cannot drift back to a + third-party type, and naming only the current library would let the + previous one silently reappear. + """ + + offenders = [ + site for site in _raise_sites() if site[2].startswith(("requests.", "httpx.")) + ] + assert offenders == [], f"third-party exception raise sites remain: {offenders}" def test_every_raise_site_uses_an_allowed_exception() -> None: diff --git a/glpi_python_client/clients/tests/test_smoke.py b/glpi_python_client/tests/clients/test_smoke.py similarity index 100% rename from glpi_python_client/clients/tests/test_smoke.py rename to glpi_python_client/tests/clients/test_smoke.py diff --git a/glpi_python_client/clients/commons/tests/__init__.py b/glpi_python_client/tests/commons/__init__.py similarity index 63% rename from glpi_python_client/clients/commons/tests/__init__.py rename to glpi_python_client/tests/commons/__init__.py index 0bcd31e..df68862 100644 --- a/glpi_python_client/clients/commons/tests/__init__.py +++ b/glpi_python_client/tests/commons/__init__.py @@ -1,4 +1,4 @@ -"""Tests for :mod:`glpi_python_client.clients.commons` helpers. +"""Tests for :mod:`glpi_python_client._sync.clients.commons` helpers. These tests exercise the small contract-aligned helpers shared by the API mixins without dispatching real HTTP calls. diff --git a/glpi_python_client/clients/commons/tests/test_constants.py b/glpi_python_client/tests/commons/test_constants.py similarity index 88% rename from glpi_python_client/clients/commons/tests/test_constants.py rename to glpi_python_client/tests/commons/test_constants.py index 9732202..7e337c3 100644 --- a/glpi_python_client/clients/commons/tests/test_constants.py +++ b/glpi_python_client/tests/commons/test_constants.py @@ -2,7 +2,7 @@ from __future__ import annotations -from glpi_python_client.clients.commons import _constants +from glpi_python_client._sync.clients.commons import _constants def test_knowledgebase_endpoint_constants() -> None: diff --git a/glpi_python_client/clients/commons/tests/test_filters.py b/glpi_python_client/tests/commons/test_filters.py similarity index 94% rename from glpi_python_client/clients/commons/tests/test_filters.py rename to glpi_python_client/tests/commons/test_filters.py index fcef3b6..0f77153 100644 --- a/glpi_python_client/clients/commons/tests/test_filters.py +++ b/glpi_python_client/tests/commons/test_filters.py @@ -1,8 +1,8 @@ -"""Unit tests for :mod:`glpi_python_client.clients.commons._filters`.""" +"""Unit tests for :mod:`glpi_python_client._sync.clients.commons._filters`.""" from __future__ import annotations -from glpi_python_client.clients.commons._filters import ( +from glpi_python_client._sync.clients.commons._filters import ( escape_rsql_like_value, escape_rsql_text_value, rsql_all_filter, diff --git a/glpi_python_client/clients/commons/tests/test_http.py b/glpi_python_client/tests/commons/test_http.py similarity index 79% rename from glpi_python_client/clients/commons/tests/test_http.py rename to glpi_python_client/tests/commons/test_http.py index 337f95f..0774e65 100644 --- a/glpi_python_client/clients/commons/tests/test_http.py +++ b/glpi_python_client/tests/commons/test_http.py @@ -1,4 +1,4 @@ -"""Unit tests for :mod:`glpi_python_client.clients.commons._http`. +"""Unit tests for :mod:`glpi_python_client._sync.clients.commons._http`. The tests cover the small request and response helper utilities used by the asynchronous transport and the per-endpoint mixins. @@ -11,7 +11,7 @@ import pytest from glpi_python_client import GlpiProtocolError, GlpiValidationError -from glpi_python_client.clients.commons._http import ( +from glpi_python_client._sync.clients.commons._http import ( build_request_headers, build_request_url, ensure_response_status, @@ -26,19 +26,39 @@ def test_request_param_value_normalises_supported_types() -> None: - """The helper returns native scalar values unchanged and stringifies others.""" + """The helper renders every value the way the wire format expects. + + Numbers and strings pass through untouched. ``bool`` and ``bytes`` are + converted deliberately, because the underlying HTTP libraries disagree + about them: ``httpx`` renders booleans lowercase and stringifies a + ``bytes`` object into its Python repr (``b'x'``), where the previous + ``requests``-based transport emitted ``True`` and the decoded text. + Normalising here keeps the emitted query string identical regardless of + which library is installed. + """ - assert request_param_value(True) is True assert request_param_value(7) == 7 + assert request_param_value(1.5) == 1.5 assert request_param_value("hello") == "hello" + assert request_param_value(True) == "True" + assert request_param_value(False) == "False" + assert request_param_value(b"raw") == "raw" assert request_param_value(object()) != "" def test_request_params_drops_none_values() -> None: - """``None`` parameter values are excluded from the produced query mapping.""" + """``None`` parameter values are excluded from the produced query mapping. + + This is a correctness guarantee, not a tidiness one. ``requests`` omitted + a ``None``-valued key from the query string entirely; ``httpx`` encodes it + as a valueless ``key=``. GLPI does not treat those the same — an empty + filter or search value matches *everything* — so forwarding the key would + silently widen a query rather than leave it unconstrained. + """ cleaned = request_params({"limit": 10, "filter": None, "force": True}) - assert cleaned == {"limit": 10, "filter": None, "force": True} + assert cleaned == {"limit": 10, "force": "True"} + assert "filter" not in cleaned def test_build_request_url_concatenates_base_and_endpoint() -> None: diff --git a/glpi_python_client/clients/commons/tests/test_payloads.py b/glpi_python_client/tests/commons/test_payloads.py similarity index 93% rename from glpi_python_client/clients/commons/tests/test_payloads.py rename to glpi_python_client/tests/commons/test_payloads.py index 047f497..443dbc2 100644 --- a/glpi_python_client/clients/commons/tests/test_payloads.py +++ b/glpi_python_client/tests/commons/test_payloads.py @@ -1,8 +1,8 @@ -"""Unit tests for :mod:`glpi_python_client.clients.commons._payloads`.""" +"""Unit tests for :mod:`glpi_python_client._sync.clients.commons._payloads`.""" from __future__ import annotations -from glpi_python_client.clients.commons._payloads import ( +from glpi_python_client._sync.clients.commons._payloads import ( model_from_payload, model_to_payload, ) diff --git a/glpi_python_client/clients/commons/tests/test_retry_semantics.py b/glpi_python_client/tests/commons/test_retry_semantics.py similarity index 64% rename from glpi_python_client/clients/commons/tests/test_retry_semantics.py rename to glpi_python_client/tests/commons/test_retry_semantics.py index d628f0b..dff5db6 100644 --- a/glpi_python_client/clients/commons/tests/test_retry_semantics.py +++ b/glpi_python_client/tests/commons/test_retry_semantics.py @@ -10,12 +10,19 @@ from collections.abc import Iterator from typing import Any +import httpx import pytest -import requests from tenacity import wait_fixed -from glpi_python_client import GlpiClient, GlpiNotFoundError, GlpiServerError -from glpi_python_client.clients.commons._http import ensure_response_status +from glpi_python_client import ( + GlpiClient, + GlpiError, + GlpiNotFoundError, + GlpiServerError, + GlpiTimeoutError, + GlpiTransportError, +) +from glpi_python_client._sync.clients.commons._http import ensure_response_status from glpi_python_client.testing.utils import FakeResponse, make_client _RETRIED_METHODS = ( @@ -144,7 +151,16 @@ def test_tolerant_search_still_returns_empty_on_4xx(client: Any) -> None: @pytest.mark.parametrize("method_name", _RETRIED_METHODS) def test_network_errors_are_still_retried(client: Any, method_name: str) -> None: - """Real ``requests`` transport faults keep their retry behaviour. + """Real transport faults are translated and still retried three times. + + The fault is injected at ``session.request`` — *below* the translation + boundary — rather than by stubbing ``_send_request``. That matters: a stub + above the boundary would raise the HTTP library's own exception, which the + retry predicate no longer names, so the test would pass or fail for + reasons unrelated to the behaviour it is meant to pin. Injecting here + exercises the real path end to end: a genuine ``httpx`` fault, translated + into ``GlpiTransportError``, matched by the predicate, retried three + times, and surfaced to the caller as a library error. Parametrized across all four retried verbs so the network-fault attempt count is pinned for each, not just ``_get_request``. @@ -152,12 +168,54 @@ def test_network_errors_are_still_retried(client: Any, method_name: str) -> None attempts: list[int] = [] - def _send(method: str, url: str, **kw: Any) -> FakeResponse: + def _request(method: str, url: str, **kw: Any) -> FakeResponse: attempts.append(1) - raise requests.ConnectionError("network down") + raise httpx.ConnectError("network down") - client._send_request = _send - with pytest.raises(requests.ConnectionError): + client._session.request = _request + with pytest.raises(GlpiTransportError): getattr(client, method_name)("Assistance/Ticket") assert len(attempts) == 3 + + +@pytest.mark.parametrize("method_name", _RETRIED_METHODS) +def test_no_third_party_exception_reaches_the_caller( + client: Any, method_name: str +) -> None: + """A network fault never surfaces as the HTTP library's own exception. + + The public contract is that ``GlpiError`` is sufficient to catch the + library's failures. This pins the half of that promise which used to be + false: transport faults escaped as third-party exceptions, forcing callers + to import the HTTP library. If the translation is ever removed, the raw + exception reaches the caller and this fails. + """ + + def _request(method: str, url: str, **kw: Any) -> FakeResponse: + raise httpx.ConnectError("network down") + + client._session.request = _request + with pytest.raises(GlpiError) as excinfo: + getattr(client, method_name)("Assistance/Ticket") + + assert not isinstance(excinfo.value, httpx.HTTPError) + # The original fault stays reachable for debugging. + assert isinstance(excinfo.value.__cause__, httpx.ConnectError) + + +def test_timeouts_narrow_to_the_timeout_subclass(client: Any) -> None: + """A timeout surfaces as ``GlpiTimeoutError``, not just the base class. + + ``GlpiTimeoutError`` exists so callers can single out the "GLPI was too + slow" case from "GLPI was unreachable". That only works if the translation + actually inspects the fault type rather than flattening everything to the + base class. + """ + + def _request(method: str, url: str, **kw: Any) -> FakeResponse: + raise httpx.ConnectTimeout("too slow") + + client._session.request = _request + with pytest.raises(GlpiTimeoutError): + client._get_request("Assistance/Ticket") diff --git a/glpi_python_client/clients/commons/tests/test_transport.py b/glpi_python_client/tests/commons/test_transport.py similarity index 100% rename from glpi_python_client/clients/commons/tests/test_transport.py rename to glpi_python_client/tests/commons/test_transport.py diff --git a/glpi_python_client/tests/custom/__init__.py b/glpi_python_client/tests/custom/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/glpi_python_client/clients/custom/tests/test_statistics.py b/glpi_python_client/tests/custom/test_statistics.py similarity index 99% rename from glpi_python_client/clients/custom/tests/test_statistics.py rename to glpi_python_client/tests/custom/test_statistics.py index ebb187b..9d3037e 100644 --- a/glpi_python_client/clients/custom/tests/test_statistics.py +++ b/glpi_python_client/tests/custom/test_statistics.py @@ -19,7 +19,7 @@ GlpiTicketType, GlpiValidationError, ) -from glpi_python_client.clients.custom._statistics import ( +from glpi_python_client._sync.clients.custom._statistics import ( _V1_SO_ASSIGNEE, _V1_SO_REQUESTER, ) diff --git a/glpi_python_client/clients/custom/tests/test_ticket_context.py b/glpi_python_client/tests/custom/test_ticket_context.py similarity index 94% rename from glpi_python_client/clients/custom/tests/test_ticket_context.py rename to glpi_python_client/tests/custom/test_ticket_context.py index 5b658c2..cc4694a 100644 --- a/glpi_python_client/clients/custom/tests/test_ticket_context.py +++ b/glpi_python_client/tests/custom/test_ticket_context.py @@ -1,7 +1,7 @@ """Unit tests for the synchronous ticket-context mixin. The tests stub ``_get_request`` on a real :class:`GlpiClient` so -:meth:`~glpi_python_client.clients.custom._ticket_context.TicketContextMixin.get_ticket_context` +:meth:`~glpi_python_client._sync.clients.custom._ticket_context.TicketContextMixin.get_ticket_context` exercises its real aggregation logic without any network call. """ diff --git a/glpi_python_client/tests/test_async_surface.py b/glpi_python_client/tests/test_async_surface.py new file mode 100644 index 0000000..04f65ed --- /dev/null +++ b/glpi_python_client/tests/test_async_surface.py @@ -0,0 +1,231 @@ +"""Runtime tests for the hand-written async surface. + +The bulk of the suite exercises the generated sync client, which is a 1:1 +token transform of this tree and therefore covers the shared logic. What it +cannot cover is anything that only exists once ``async``/``await`` are real: +whether the client actually awaits, whether concurrent tasks can contend the +auth lock without deadlocking, and whether the fan-out helper really runs +work concurrently rather than in sequence. + +Those are exactly the properties the codegen cannot verify -- the diff gate +proves the two trees *correspond*, not that the async one *works* -- so they +are tested here directly. +""" + +from __future__ import annotations + +import asyncio +from datetime import datetime, timedelta, timezone +from typing import Any + +import httpx +import pytest + +from glpi_python_client import AsyncGlpiClient, GlpiTimeoutError, GlpiTransportError +from glpi_python_client._async._concurrency import gather +from glpi_python_client.testing.utils import make_async_client + + +class _Response: + """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] = {} + 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 + + +def _stub(client: AsyncGlpiClient, payload: Any) -> list[str]: + """Install an async transport stub and pretend a token is held.""" + + calls: list[str] = [] + + async def _request(method: str, url: str, **kwargs: Any) -> _Response: + calls.append(f"{method} {url}") + return _Response(payload) + + client._session.request = _request # type: ignore[method-assign,assignment] + client._auth.access_token = "stub-token" + client._auth.token_expires_at = datetime.now(tz=timezone.utc) + timedelta(days=365) + return calls + + +async def test_a_read_awaits_and_returns_a_model() -> None: + """The async client dispatches and parses exactly like its twin.""" + + client = make_async_client() + calls = _stub(client, {"id": 42, "name": "async ticket"}) + try: + ticket = await client.get_ticket(42) + assert ticket.id == 42 + assert ticket.name == "async ticket" + assert calls, "no HTTP call was dispatched" + finally: + await client.close() + + +async def test_close_is_idempotent_and_blocks_further_calls() -> None: + """Closing twice is safe and a closed client refuses to dispatch.""" + + client = make_async_client() + _stub(client, {"id": 1, "name": "x"}) + await client.close() + await client.close() + with pytest.raises(RuntimeError, match="closed"): + await client.get_ticket(1) + + +async def test_the_async_context_manager_closes_on_exit() -> None: + """``async with`` releases the client, so ``__aexit__`` really awaits.""" + + client = make_async_client() + _stub(client, {"id": 1, "name": "x"}) + async with client as entered: + assert entered is client + assert client._closed is True + + +async def test_concurrent_tasks_contend_the_auth_lock_without_deadlocking() -> None: + """Many tasks may refresh the token at once and all of them finish. + + This is the property the hand-written ``_concurrency`` twin exists for. + The lock is held across an ``await``; with a ``threading.Lock`` -- the + right primitive for the *sync* tree -- the second task to arrive would + block the event loop, the holder could never resume to release it, and + this test would hang forever rather than fail. Running real contention + is the only way to observe that. + """ + + client = make_async_client() + calls = _stub(client, {"id": 1, "name": "x"}) + # Force every task through the token-acquisition path. + client._auth.access_token = None + + acquisitions = 0 + + async def _acquire() -> None: + nonlocal acquisitions + acquisitions += 1 + await asyncio.sleep(0) # a real suspension point inside the lock + client._auth.access_token = "stub-token" + + client._auth._acquire_token = _acquire # type: ignore[method-assign] + try: + results = await asyncio.wait_for( + asyncio.gather(*(client.get_ticket(i) for i in range(1, 11))), + timeout=10, + ) + assert len(results) == 10 + assert len(calls) == 10 + finally: + await client.close() + + +async def test_gather_runs_work_concurrently() -> None: + """The async ``gather`` overlaps its arguments rather than serialising. + + The sync twin returns already-evaluated values, which is correct there. + Here the whole point is that the calls overlap. + + Completion *order* is the primary evidence: the shorter sleep finishes + first even though it is passed second, which cannot happen if the two + ran one after the other. The elapsed-time bound is a secondary check, + and the delays are chosen well clear of the platform timer granularity + (~16ms on Windows) so the margin between "concurrent" and "sequential" + is not swallowed by rounding. + """ + + slow_delay, fast_delay = 0.30, 0.15 + order: list[str] = [] + + async def _sleep_then_record(label: str, delay: float) -> str: + await asyncio.sleep(delay) + order.append(label) + return label + + loop = asyncio.get_running_loop() + started = loop.time() + results = await gather( + _sleep_then_record("slow", slow_delay), + _sleep_then_record("fast", fast_delay), + ) + elapsed = loop.time() - started + + # Results keep argument order; completion order is by speed. + assert results == ["slow", "fast"] + assert order == ["fast", "slow"], "the two coroutines did not overlap" + assert elapsed < (slow_delay + fast_delay) * 0.9, ( + f"gather took {elapsed:.3f}s, close to the sequential " + f"{slow_delay + fast_delay:.3f}s -- it serialised its arguments" + ) + + +async def test_network_faults_are_translated_on_the_async_path() -> None: + """A transport fault surfaces as a library error, not an httpx one. + + The sync path has its own test for this. Repeating it here is not + duplication: the translation sits in a ``try``/``except`` around an + awaited call, and an ``except`` clause that fails to cover an awaited + expression is a distinct mistake the sync test cannot detect. + """ + + client = make_async_client() + _stub(client, {}) + + async def _boom(method: str, url: str, **kwargs: Any) -> _Response: + raise httpx.ConnectError("network down") + + client._session.request = _boom # type: ignore[method-assign,assignment] + # Keep the retry from spending 6 real seconds on the way to failing. + client._get_request.retry.wait = lambda *a, **k: 0 # type: ignore[attr-defined] + try: + with pytest.raises(GlpiTransportError) as excinfo: + await client.get_ticket(1) + assert isinstance(excinfo.value.__cause__, httpx.ConnectError) + finally: + await client.close() + + +async def test_timeouts_narrow_on_the_async_path() -> None: + """A timeout narrows to ``GlpiTimeoutError`` when awaited too.""" + + client = make_async_client() + _stub(client, {}) + + async def _slow(method: str, url: str, **kwargs: Any) -> _Response: + raise httpx.ConnectTimeout("too slow") + + client._session.request = _slow # type: ignore[method-assign,assignment] + client._get_request.retry.wait = lambda *a, **k: 0 # type: ignore[attr-defined] + try: + with pytest.raises(GlpiTimeoutError): + await client.get_ticket(1) + finally: + await client.close() + + +async def test_the_paginating_generator_is_an_async_generator() -> None: + """``iter_search_*`` yields pages under ``async for``. + + The generators are the one place the codegen has to get two things + right at once -- ``async def`` plus ``AsyncIterator`` -- and a mistake + in either shows up only when the generator is actually driven. + """ + + client = make_async_client() + _stub(client, [{"id": 1, "name": "one"}]) + try: + pages = [] + async for page in client.iter_search_tickets(batch_size=50): + pages.append(page) + break + assert pages and pages[0][0].id == 1 + finally: + await client.close() diff --git a/glpi_python_client/tests/test_docstring_references.py b/glpi_python_client/tests/test_docstring_references.py new file mode 100644 index 0000000..aa059b7 --- /dev/null +++ b/glpi_python_client/tests/test_docstring_references.py @@ -0,0 +1,109 @@ +"""Every qualified cross-reference in the package points at something real. + +Sphinx cannot provide this guarantee. ``nitpicky`` is off, so an +unresolvable ``:class:`` or ``:mod:`` target renders as plain text +instead of failing the build -- and most of these modules are private, +so autodoc never visits them in the first place. A reference to a module +that was since renamed or deleted therefore rots in complete silence. + +That is not hypothetical. The move to a generated sync tree renamed one +client module and deleted four others, and seven docstring references to +them survived the rewrite with a green suite, a green ``-W`` docs build, +and a clean codegen diff. This test is the check that would have caught +them, so the next rename cannot repeat it. +""" + +from __future__ import annotations + +import importlib +import pathlib +import re + +_REPO_ROOT = pathlib.Path(__file__).resolve().parents[2] +_PACKAGE = _REPO_ROOT / "glpi_python_client" + +#: A qualified reference to this package: the root name plus at least one +#: dotted segment. Bare mentions are excluded on purpose -- ``GlpiClient`` +#: on its own is prose, not a reference that can rot. +_REFERENCE = re.compile(r"\bglpi_python_client(?:\.[A-Za-z_]\w*)+") + + +def _resolves(dotted: str) -> bool: + """Return whether ``dotted`` names a real module or attribute chain. + + Tries the longest importable module prefix first, then walks the + remainder with :func:`getattr`. A reference may end in a module, a + class, or a method, and all three have to be accepted. + """ + + parts = dotted.split(".") + for stop in range(len(parts), 0, -1): + try: + target: object = importlib.import_module(".".join(parts[:stop])) + except ImportError: + continue + for attribute in parts[stop:]: + if hasattr(target, attribute): + target = getattr(target, attribute) + continue + # Pydantic v2 moves declared fields into ``model_fields`` and + # takes them out of the class namespace, so ``hasattr`` says no + # to a field that autodoc documents perfectly well. Accepting + # them matters: without this the check reports noise, and a + # check that cries wolf gets deleted. + fields = getattr(target, "model_fields", None) + if isinstance(fields, dict) and attribute in fields: + return True + return False + return True + return False + + +def _reference_sites() -> list[tuple[str, int, str]]: + """Return ``(module, lineno, reference)`` for every qualified mention.""" + + sites: list[tuple[str, int, str]] = [] + this_module = pathlib.Path(__file__).resolve() + for path in sorted(_PACKAGE.rglob("*.py")): + # This module names dead targets on purpose, as controls. + if "__pycache__" in path.parts or path.resolve() == this_module: + continue + rel = path.relative_to(_REPO_ROOT).as_posix() + for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + for match in _REFERENCE.findall(line): + sites.append((rel, lineno, match.rstrip("."))) + return sites + + +def test_the_scan_finds_references_and_can_tell_them_apart() -> None: + """Positive control: the scan is not vacuous and ``_resolves`` discriminates. + + Without this, a regex that stopped matching would turn the check + below into a test that passes forever without looking at anything. + """ + + sites = _reference_sites() + assert len(sites) > 50, f"only {len(sites)} references found -- regex broken?" + + assert _resolves("glpi_python_client.GlpiClient") + assert _resolves("glpi_python_client.AsyncGlpiClient.get_ticket_context") + assert _resolves("glpi_python_client._async.clients.commons._transport") + # One of the targets the codegen rewrite actually left behind. + assert not _resolves( + "glpi_python_client._async.clients.async_client.AsyncGlpiClient" + ) + assert not _resolves("glpi_python_client.nope") + + +def test_every_qualified_reference_resolves() -> None: + """No docstring names a module, class, or method that does not exist.""" + + offenders = [ + f"{module}:{lineno}: {reference}" + for module, lineno, reference in _reference_sites() + if not _resolves(reference) + ] + assert offenders == [], ( + "these references point at nothing -- the target was renamed or " + "deleted and the docstring was not updated:\n" + "\n".join(offenders) + ) diff --git a/glpi_python_client/tests/test_skill_references.py b/glpi_python_client/tests/test_skill_references.py new file mode 100644 index 0000000..f354c90 --- /dev/null +++ b/glpi_python_client/tests/test_skill_references.py @@ -0,0 +1,186 @@ +"""Every method the bundled skills name exists on both clients. + +``skills/`` is instructions for an agent, so a wrong name there is not a +typo in prose -- it is a wrong API taught to whatever reads it, and the +resulting code fails at the caller rather than here. Nothing else in the +project looks at these files: they are not imported, not built by Sphinx, +and not touched by mypy or ruff. Between them and the library there is no +gate at all. + +That gap has already cost us. The move to a generated sync tree left +``skills/README.md`` instructing readers to write ``async with +GlpiClient(...)`` -- but ``GlpiClient`` is the *synchronous* client, so +the snippet raises ``AttributeError``. Six ``SKILL.md`` files described +"the asynchronous ``glpi_python_client.GlpiClient``" in their frontmatter +``description``, which is the one field an agent reads before deciding to +open the file at all. And ``glpi-client-setup`` credited the async client +with a ``threading.Lock`` around OAuth -- the exact primitive +``_concurrency.py`` documents as a deadlock on that surface, because the +lock is held across an ``await``. + +None of that broke a test, a build, or a type check. +""" + +from __future__ import annotations + +import pathlib +import re + +import pytest + +from glpi_python_client import AsyncGlpiClient, GlpiClient + +_REPO_ROOT = pathlib.Path(__file__).resolve().parents[2] +_SKILLS_DIR = _REPO_ROOT / "skills" + +# ``skills/`` ships in the sdist but not in the wheel, so an installed +# package legitimately has no copy to check. +pytestmark = pytest.mark.skipif( + not _SKILLS_DIR.is_dir(), reason="skills/ is source-tree material, not in the wheel" +) + +#: A method call on a client in a snippet. Both spellings appear: the +#: skills bind the client to ``client`` or, via ``from_env``, to ``glpi``. +_CALL = re.compile(r"\b(?:client|glpi)\.(\w+)\s*\(") + +#: Claims that were true of the retired thread-pool bridge and are false +#: now. The last two are subtler than the rest: naming ``GlpiClient`` as +#: asynchronous inverts the two classes, and crediting the async client +#: with a ``threading.Lock`` names the primitive that deadlocks there. +_STALE = ( + r"executor=", + r"to_thread", + r"worker thread", + r"async with GlpiClient", + r"asynchronous glpi_python_client\.GlpiClient\b", + r"threading\.Lock[^\n]*AsyncGlpiClient", +) + +#: The skills *deny* the bridge's machinery on purpose -- "there is no +#: ``executor=`` argument and no thread pool to size". Denying a thing is +#: the opposite of claiming it, so a hit preceded by a negation is correct +#: prose. Without this the check reports its own corrections as failures. +_NEGATED = re.compile(r"\b(?:no|not|never|retired|removed|deleted)\b[^.]{0,60}$") + + +def _skill_files() -> list[pathlib.Path]: + """Return every skill document, including the index README.""" + + return [*sorted(_SKILLS_DIR.glob("*/SKILL.md")), _SKILLS_DIR / "README.md"] + + +def _frontmatter_name(text: str) -> str | None: + """Return the ``name:`` field of a skill's frontmatter, if it has one. + + Parsed by hand rather than with a YAML library: the frontmatter is a + handful of flat scalars, and this keeps the test suite from gaining a + dependency for one field. + """ + + if not text.startswith("---"): + return None + _, _, rest = text.partition("---\n") + front, _, _ = rest.partition("\n---") + match = re.search(r"^name:\s*(\S+)\s*$", front, re.MULTILINE) + return match.group(1) if match else None + + +def _stale_claims(text: str) -> list[tuple[int, str]]: + """Return ``(lineno, matched text)`` for each unnegated stale claim.""" + + hits: list[tuple[int, str]] = [] + for pattern in _STALE: + for match in re.finditer(pattern, text, re.IGNORECASE): + preceding = text[max(0, match.start() - 70) : match.start()] + if _NEGATED.search(preceding.replace("\n", " ")): + continue + hits.append((text[: match.start()].count("\n") + 1, match.group(0))) + return hits + + +def test_the_scan_finds_skills_and_can_tell_claims_apart() -> None: + """Positive control: the scans are not vacuous and do discriminate. + + Without this, a regex that stopped matching would leave the checks + below passing forever without reading anything. + """ + + files = _skill_files() + assert len(files) > 5, f"only {len(files)} skill files found -- layout changed?" + + calls = sum(len(_CALL.findall(p.read_text(encoding="utf-8-sig"))) for p in files) + assert calls > 20, f"only {calls} client calls found -- regex broken?" + + assert _stale_claims("Use `async with GlpiClient(...)` and await everything.") + assert _stale_claims("Pass executor= to size the pool.") + # The negation carve-out has to hold, or the check fails on the fix. + assert not _stale_claims("There is no `executor=` argument and no thread pool.") + + +def test_no_skill_starts_with_a_byte_order_mark() -> None: + """A BOM hides the frontmatter from anything that reads the file. + + Frontmatter is recognised by the file *starting* with ``---``. Three + bytes of UTF-8 BOM in front of it mean it does not, so a strict reader + sees a plain document with no ``name`` and no ``description`` -- and + the description is what an agent uses to decide whether the skill is + relevant at all. Six of these files carried one, invisibly, which is + also why this test reads them as ``utf-8-sig``: tolerant parsing must + not be what stops anyone noticing. + """ + + offenders = [ + path.relative_to(_REPO_ROOT).as_posix() + for path in sorted(_SKILLS_DIR.rglob("*.md")) + if path.read_bytes().startswith(b"\xef\xbb\xbf") + ] + assert offenders == [], ( + "these skill files start with a UTF-8 BOM, which hides their " + "frontmatter:\n" + "\n".join(offenders) + ) + + +def test_every_skill_name_matches_its_directory() -> None: + """A skill is addressed by directory; a mismatched ``name`` breaks it.""" + + offenders: list[str] = [] + for path in sorted(_SKILLS_DIR.glob("*/SKILL.md")): + declared = _frontmatter_name(path.read_text(encoding="utf-8-sig")) + if declared != path.parent.name: + offenders.append(f"{path.parent.name}: frontmatter name is {declared!r}") + assert offenders == [], "skill name does not match its directory:\n" + "\n".join( + offenders + ) + + +def test_every_method_named_in_a_skill_exists_on_both_clients() -> None: + """No skill teaches a method that was renamed or removed. + + Checked against *both* classes, because every skill tells the reader + its snippets work on the other one after dropping ``await``. + """ + + offenders: list[str] = [] + for path in _skill_files(): + rel = path.relative_to(_REPO_ROOT).as_posix() + for method in sorted(set(_CALL.findall(path.read_text(encoding="utf-8-sig")))): + for client in (GlpiClient, AsyncGlpiClient): + if not hasattr(client, method): + offenders.append(f"{rel}: {client.__name__} has no {method!r}") + assert offenders == [], ( + "these skills name methods that do not exist:\n" + "\n".join(offenders) + ) + + +def test_no_skill_describes_the_retired_thread_pool_bridge() -> None: + """No skill still explains the async client as a thread-pool wrapper.""" + + offenders = [ + f"{path.relative_to(_REPO_ROOT).as_posix()}:{lineno}: {claim!r}" + for path in _skill_files() + for lineno, claim in _stale_claims(path.read_text(encoding="utf-8-sig")) + ] + assert offenders == [], ( + "these skills describe machinery that no longer exists:\n" + + "\n".join(offenders) + ) diff --git a/glpi_python_client/tests/test_unasync_codegen.py b/glpi_python_client/tests/test_unasync_codegen.py new file mode 100644 index 0000000..ae04e05 --- /dev/null +++ b/glpi_python_client/tests/test_unasync_codegen.py @@ -0,0 +1,281 @@ +"""Guards for the ``_async`` -> ``_sync`` code generation. + +The generated tree is protected in CI by regenerating it and diffing. That +catches *staleness* -- someone editing ``_async/`` without rerunning the +build -- and nothing else. In particular it cannot catch the failure mode +that actually worries us: + +**A token collision is deterministic, so the diff stays empty.** If a local +variable, parameter or attribute happens to be spelled like a substitution +key, ``unasync`` rewrites it every single time. Regenerating produces +exactly the same wrong file, ``git diff`` is clean, CI is green, and the +sync client is silently incorrect. No amount of diffing finds that. + +These tests find it, by scanning the source for identifiers that the +substitution map would rewrite and failing on any that are not an +intentional rename. +""" + +from __future__ import annotations + +import ast +import pathlib +import subprocess +import sys + +import pytest + +unasync = pytest.importorskip( + "unasync", + reason="unasync is a dev-only dependency; codegen guards need it installed", +) + +_REPO_ROOT = pathlib.Path(__file__).resolve().parents[2] +_BUILD_SCRIPT = _REPO_ROOT / "unasync_build.py" +_ASYNC_DIR = _REPO_ROOT / "glpi_python_client" / "_async" +_SYNC_DIR = _REPO_ROOT / "glpi_python_client" / "_sync" + +#: Names the codegen is *supposed* to rewrite wherever they appear. +#: +#: Everything else in the substitution map is a language-level or +#: third-party name; if one of those shows up as an identifier this package +#: defines, it is a collision and the scan below fails. +_INTENTIONAL_RENAMES = { + # unasync's built-in map turns the async context-manager protocol into + # the sync one; defining these is the whole point. + "__aenter__", + "__aexit__", + "AsyncGlpiClient", + "AsyncClient", + "AsyncBaseTransport", + "AsyncHTTPTransport", + "aclose", + "aread", +} + + +def _build_module() -> object: + """Import ``unasync_build`` from the repository root.""" + + sys.path.insert(0, str(_REPO_ROOT)) + try: + import unasync_build + + return unasync_build + finally: + sys.path.remove(str(_REPO_ROOT)) + + +def _substitution_keys() -> set[str]: + """Return every NAME token the codegen would rewrite.""" + + build = _build_module() + rule = unasync.Rule( + fromdir=str(_ASYNC_DIR), + todir=str(_SYNC_DIR), + additional_replacements=build.TOKEN_REPLACEMENTS, # type: ignore[attr-defined] + ) + return set(rule.token_replacements) + + +def _identifier_sites() -> list[tuple[str, int, str]]: + """Return ``(module, lineno, identifier)`` for every name the package defines.""" + + sites: list[tuple[str, int, str]] = [] + for path in sorted(_ASYNC_DIR.rglob("*.py")): + if "__pycache__" in path.parts: + continue + rel = path.relative_to(_REPO_ROOT).as_posix() + tree = ast.parse(path.read_text(encoding="utf-8")) + for node in ast.walk(tree): + if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef | ast.ClassDef): + sites.append((rel, node.lineno, node.name)) + elif isinstance(node, ast.arg): + sites.append((rel, node.lineno, node.arg)) + elif isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store): + sites.append((rel, node.lineno, node.id)) + elif isinstance(node, ast.Attribute): + sites.append((rel, node.lineno, node.attr)) + return sites + + +def test_the_substitution_map_is_reachable() -> None: + """Positive control: the map is non-empty and contains a known key. + + Without this, a refactor that made ``_substitution_keys`` return an + empty set would turn the collision scan into a test that can never + fail -- passing vacuously forever. + """ + + keys = _substitution_keys() + assert keys, "the substitution map is empty -- the collision scan is vacuous" + assert "AsyncGlpiClient" in keys + assert "__aenter__" in keys, "unasync's built-in defaults are missing" + + +def test_no_identifier_collides_with_a_substitution_key() -> None: + """No name this package defines is silently rewritten by the codegen. + + This is the check the CI diff gate cannot perform. A collision is + deterministic, so regenerating reproduces it byte for byte and the diff + stays clean while the generated client misbehaves. + """ + + keys = _substitution_keys() - _INTENTIONAL_RENAMES + offenders = [site for site in _identifier_sites() if site[2] in keys] + assert offenders == [], ( + "these identifiers would be silently rewritten by the codegen; " + f"rename them or add them to the intentional list: {offenders}" + ) + + +def test_the_generated_tree_never_names_the_async_one() -> None: + """No module under ``_sync/`` mentions ``_async`` anywhere. + + unasync repoints the imports for free, because there ``_async`` is + its own NAME token. Inside a docstring the whole thing is a *single* + string token, and substitution only fires when a literal's entire + content is a key -- so a cross-reference such as + ``:mod:`glpi_python_client._async.clients.api``` passes straight + through, and the shipped sync client ends up documenting itself in + terms of a tree its users never import. + + The diff gate cannot see this either: the omission is deterministic, + so regeneration reproduces it and the diff stays clean. + ``unasync_build`` rewrites the qualified prefix; this asserts the + result from the other end, and additionally catches a *bare* mention + that the prefix rewrite is deliberately too narrow to touch. + + The hand-written twins are exempt. They are not generated, and each + one names its counterpart on purpose: pointing at the other file is + the only way to explain why the pair exists. + """ + + build = _build_module() + hand_written: set[str] = build.HAND_WRITTEN # type: ignore[attr-defined] + offenders = [ + f"{path.relative_to(_REPO_ROOT).as_posix()}:{lineno}: {line.strip()}" + for path in sorted(_SYNC_DIR.rglob("*.py")) + if "__pycache__" not in path.parts and path.name not in hand_written + for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1) + if "_async" in line + ] + assert offenders == [], ( + "the generated sync tree still refers to the async one:\n" + + "\n".join(offenders) + ) + + +def _async_generator_names() -> set[str]: + """Return the name of every ``async def`` in ``_async/`` that yields.""" + + names: set[str] = set() + for path in sorted(_ASYNC_DIR.rglob("*.py")): + if "__pycache__" in path.parts: + continue + tree = ast.parse(path.read_text(encoding="utf-8")) + for node in ast.walk(tree): + if not isinstance(node, ast.AsyncFunctionDef): + continue + if any( + isinstance(inner, ast.Yield | ast.YieldFrom) for inner in ast.walk(node) + ): + names.add(node.name) + return names + + +def test_no_async_generator_is_awaited() -> None: + """An async generator is driven with ``async for``, never awaited. + + This is a mistake the sync tree cannot reveal. ``await gen()`` and + ``async for x in gen()`` both generate the *same* correct synchronous + loop once ``async``/``await`` are stripped, so the generated client + works, the diff gate is clean, mypy is satisfied on both trees, and + every test that exercises the sync surface passes -- while the async + surface raises ``TypeError: object async_generator can't be used in + 'await' expression`` the moment that code path is reached. + + It was found by running the async client against a live server; this + check makes that failure a static one. + """ + + generators = _async_generator_names() + assert generators, "no async generators found -- this check is vacuous" + + offenders: list[tuple[str, int, str]] = [] + for path in sorted(_ASYNC_DIR.rglob("*.py")): + if "__pycache__" in path.parts: + continue + rel = path.relative_to(_REPO_ROOT).as_posix() + tree = ast.parse(path.read_text(encoding="utf-8")) + for node in ast.walk(tree): + if not isinstance(node, ast.Await): + continue + call = node.value + if not isinstance(call, ast.Call): + continue + func = call.func + name = ( + func.attr + if isinstance(func, ast.Attribute) + else func.id + if isinstance(func, ast.Name) + else None + ) + if name in generators: + offenders.append((rel, node.lineno, name or "?")) + + assert offenders == [], ( + "these async generators are awaited instead of being driven with " + f"`async for`: {offenders}" + ) + + +def test_the_concurrency_twins_expose_the_same_surface() -> None: + """Both hand-written ``_concurrency`` twins export the same names. + + These two files are the only ones maintained by hand on both sides, so + they are the only place the trees can diverge without the diff gate + noticing. If one grows a helper the other lacks, the generated tree + stops importing cleanly -- but only for the code paths that use it, + which may not be covered. + """ + + from glpi_python_client._async import _concurrency as async_twin + from glpi_python_client._sync import _concurrency as sync_twin + + assert set(async_twin.__all__) == set(sync_twin.__all__) + for name in async_twin.__all__: + assert hasattr(sync_twin, name), f"_sync/_concurrency.py is missing {name!r}" + + +def test_gather_twins_agree_on_ordering() -> None: + """The sync ``gather`` preserves order, as ``asyncio.gather`` does. + + Ordering is the whole contract callers rely on: results are matched to + arguments by position, never by completion time. + """ + + from glpi_python_client._sync._concurrency import gather + + assert gather("a", "b", "c") == ["a", "b", "c"] + assert gather() == [] + + +def test_the_checked_in_sync_tree_is_not_stale() -> None: + """``_sync/`` matches what ``_async/`` currently generates. + + Mirrors the CI gate so the failure shows up locally, at the pre-push + hook, rather than after a push. + """ + + result = subprocess.run( + [sys.executable, str(_BUILD_SCRIPT), "--check"], + capture_output=True, + text=True, + cwd=_REPO_ROOT, + ) + assert result.returncode == 0, ( + "the generated sync tree is out of date -- run `python unasync_build.py`\n" + f"{result.stdout}\n{result.stderr}" + ) diff --git a/integration_tests/test_integration.py b/integration_tests/test_integration.py index ea0de46..8c599b8 100644 --- a/integration_tests/test_integration.py +++ b/integration_tests/test_integration.py @@ -670,13 +670,18 @@ def test_get_user_activity_raises_without_identifier(client: GlpiClient) -> None # --------------------------------------------------------------------------- # GLPI Fields plugin (legacy v1 endpoints) # -# These tests target the live preprod ticket #62571, which carries an -# ``aidelarsolution`` custom container set up via the GLPI Fields plugin. -# When the plugin is not installed the tests skip cleanly so the suite stays -# portable across instances. +# These tests target a ticket carrying a custom container set up via the GLPI +# Fields plugin. Both the ticket id and the container/field names are +# instance-specific, so they are read from local secrets rather than hardcoded. +# When the plugin or the configuration is absent the tests skip cleanly so the +# suite stays portable across instances. # --------------------------------------------------------------------------- -_FIELDS_TEST_TICKET_ID = 62571 +_FIELDS_TEST_TICKET_ID = _parse_int( + _read_value("glpi_fields_ticket_id", "GLPI_FIELDS_TICKET_ID") +) +_FIELDS_CONTAINER_NAME = _read_value("glpi_fields_container", "GLPI_FIELDS_CONTAINER") +_FIELDS_FIELD_NAME = _read_value("glpi_fields_field", "GLPI_FIELDS_FIELD") # GLPI answers 400 with this marker when the itemtype in the URL is not a # known CommonDBTM subclass -- which is what an uninstalled plugin looks @@ -732,38 +737,45 @@ def test_plugin_fields_containers_discovery( def test_get_ticket_custom_fields_round_trip_on_known_ticket( client: GlpiClient, fields_containers: list[GetPluginFieldsContainer] ) -> None: - """Round-trip the ``aidelarsolution`` custom field on ticket 62571. + """Round-trip the configured custom field on the configured ticket. The original value is captured, replaced by a timestamped probe value, read back, and finally restored so the test is net-zero. """ + if ( + _FIELDS_TEST_TICKET_ID is None + or _FIELDS_CONTAINER_NAME is None + or _FIELDS_FIELD_NAME is None + ): + pytest.skip("Fields plugin round-trip target not configured in secrets") + containers = fields_containers - container = next((c for c in containers if c.name == "aidelarsolution"), None) + container = next((c for c in containers if c.name == _FIELDS_CONTAINER_NAME), None) if container is None: - pytest.skip("'aidelarsolution' container missing on this instance") + pytest.skip("configured container missing on this instance") assert container.id is not None fields = client.list_plugin_fields_fields(container_id=container.id) - field = next((f for f in fields if f.name == "aidelarsolutionfield"), None) + field = next((f for f in fields if f.name == _FIELDS_FIELD_NAME), None) if field is None: - pytest.skip("'aidelarsolutionfield' missing in container") + pytest.skip("configured field missing in container") before = client.get_ticket_custom_fields(_FIELDS_TEST_TICKET_ID) - original = before.get("aidelarsolution", {}).get("aidelarsolutionfield") + original = before.get(_FIELDS_CONTAINER_NAME, {}).get(_FIELDS_FIELD_NAME) probe_value = f"integration probe {_suffix()}
" try: client.set_ticket_custom_fields( _FIELDS_TEST_TICKET_ID, - {"aidelarsolution": {"aidelarsolutionfield": probe_value}}, + {_FIELDS_CONTAINER_NAME: {_FIELDS_FIELD_NAME: probe_value}}, ) after = client.get_ticket_custom_fields(_FIELDS_TEST_TICKET_ID) - assert after["aidelarsolution"]["aidelarsolutionfield"] == probe_value + assert after[_FIELDS_CONTAINER_NAME][_FIELDS_FIELD_NAME] == probe_value finally: if original is not None: client.set_ticket_custom_fields( _FIELDS_TEST_TICKET_ID, - {"aidelarsolution": {"aidelarsolutionfield": original}}, + {_FIELDS_CONTAINER_NAME: {_FIELDS_FIELD_NAME: original}}, ) @@ -773,8 +785,9 @@ def test_set_ticket_custom_fields_rejects_unknown_container( ) -> None: """Writing to a non-existent container raises before any HTTP call.""" + # The ticket id is irrelevant here: the guard fires before any HTTP call. with pytest.raises(ValueError, match="Unknown plugin-fields container"): client.set_ticket_custom_fields( - _FIELDS_TEST_TICKET_ID, + 1, {"does-not-exist-xyz": {"any_field": "value"}}, ) diff --git a/integration_tests/test_integration_async.py b/integration_tests/test_integration_async.py index 39a9d75..d35570d 100644 --- a/integration_tests/test_integration_async.py +++ b/integration_tests/test_integration_async.py @@ -8,10 +8,10 @@ * Concurrent fan-out via :func:`asyncio.gather` (read-only). * OAuth token-acquisition lock contention from many coroutines racing for the very first authenticated call. -* Routing every call through a caller-supplied - :class:`concurrent.futures.ThreadPoolExecutor`. +* Real non-blocking I/O on the caller's event loop, with no worker + thread anywhere on the path. * Cancellation of an in-flight awaiting coroutine. -* Exception propagation from a worker thread back to the awaiter. +* Exception propagation from the transport back to the awaiter. The shared configuration loader from :mod:`test_integration` is reused so the same secrets/env layout drives both suites. @@ -21,7 +21,6 @@ import asyncio from collections.abc import AsyncIterator -from concurrent.futures import ThreadPoolExecutor import pytest import pytest_asyncio @@ -47,11 +46,7 @@ pytestmark = pytest.mark.integration -def _build_async_client( - config: _LiveGlpiConfig, - *, - executor: ThreadPoolExecutor | None = None, -) -> AsyncGlpiClient: +def _build_async_client(config: _LiveGlpiConfig) -> AsyncGlpiClient: """Return one configured :class:`AsyncGlpiClient` for the live instance. Parameters @@ -59,9 +54,6 @@ def _build_async_client( config : _LiveGlpiConfig Live GLPI configuration loaded from the secrets directory or the ``GLPI_*`` environment variables. - executor : concurrent.futures.ThreadPoolExecutor | None - Optional pool routed through :class:`AsyncBridge`. When - ``None`` the bridge falls back to :func:`asyncio.to_thread`. """ return AsyncGlpiClient( @@ -77,7 +69,6 @@ def _build_async_client( v1_base_url=config.v1_base_url, v1_user_token=config.v1_user_token, v1_app_token=config.v1_app_token, - executor=executor, ) @@ -172,7 +163,7 @@ async def test_gather_fan_out_read_only( """Fan out independent read-only calls and confirm each returns a list. This stresses the bridge: many coroutines hit - :func:`asyncio.to_thread` simultaneously, contending for the OAuth + the event loop simultaneously, contending for the OAuth token lock on the first call and then for the requests pool. """ @@ -208,63 +199,55 @@ async def test_oauth_lock_contention_on_fresh_client( assert isinstance(value, list) -async def test_custom_executor_routing( - live_config: _LiveGlpiConfig, # noqa: F811 +async def test_calls_run_on_the_event_loop_not_a_worker_thread( + async_client: AsyncGlpiClient, ) -> None: - """A caller-supplied executor handles every wrapped call. + """Requests are issued from the calling thread, on the event loop. - The bridge takes the ``glpi-itest`` named pool when one is passed - in. We check that thread names observed during fan-out come from - that pool, proving the bridge respects the override. - """ + This replaces an earlier test that asserted a caller-supplied + executor received the work. There is no executor and no worker thread + any more: the client performs real non-blocking I/O, so the whole + call runs on the thread that owns the loop. - seen_thread_names: set[str] = set() - - pool = ThreadPoolExecutor(max_workers=4, thread_name_prefix="glpi-itest") - try: - async with _build_async_client(live_config, executor=pool) as client: - results = await asyncio.gather( - *(client.search_users(limit=1) for _ in range(6)) - ) - assert len(results) == 6 + A live fan-out is the honest way to check it -- a stubbed transport + would prove nothing about how real sockets are driven. + """ - # Quick worker-name probe: schedule a no-op through the same - # pool and capture the worker thread name. - loop = asyncio.get_running_loop() + import threading - def _name() -> str: - import threading + main_thread = threading.current_thread().name + observed: list[str] = [] - return threading.current_thread().name + async def _probe() -> None: + await async_client.search_users(limit=1) + observed.append(threading.current_thread().name) - for _ in range(4): - seen_thread_names.add(await loop.run_in_executor(pool, _name)) - finally: - pool.shutdown(wait=True) + await asyncio.gather(*(_probe() for _ in range(6))) - assert seen_thread_names, "expected at least one worker name to be captured" - assert all(name.startswith("glpi-itest") for name in seen_thread_names) + assert len(observed) == 6 + assert all(name == main_thread for name in observed), ( + f"calls ran off the loop thread: {set(observed)}" + ) async def test_cancellation_releases_awaiter( async_client: AsyncGlpiClient, ) -> None: - """Cancelling the awaiter releases the coroutine even if HTTP keeps running. + """Cancelling the task raises ``CancelledError`` promptly. - The bridge documents cancellation as best-effort: the in-flight - ``requests`` call still completes on its worker thread, but the - awaiting coroutine must raise :class:`asyncio.CancelledError` - promptly. + With real async I/O the cancellation reaches the in-flight request + itself rather than being best-effort against a detached worker + thread, so the awaiting coroutine must unwind immediately. """ task = asyncio.create_task(async_client.search_tickets("status==1", limit=50)) - await asyncio.sleep(0) # let the task enter to_thread + await asyncio.sleep(0) # let the task reach its first await task.cancel() with pytest.raises(asyncio.CancelledError): await task -async def test_exception_propagates_from_worker_thread( +async def test_exception_propagates_from_an_awaited_call( async_client: AsyncGlpiClient, ) -> None: """A missing ticket surfaces as a typed GLPI error. diff --git a/pyproject.toml b/pyproject.toml index fb2732a..ee48995 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -15,10 +15,13 @@ exclude = [ "dist/", "*.egg-info/", "docs/_build/", + "docs/superpowers/", + "docs/glpi_api_contract.json", ".venv/", "venv/", ".env", ".env.*", + ".coverage", "secrets/", "integration_tests/", ] @@ -48,14 +51,23 @@ classifiers = [ ] dependencies = [ "beautifulsoup4>=4.12", + "httpx>=0.28", "lxml>=4.9", "markdown>=3.6", "markdownify>=0.13", "pydantic>=2.8", - "requests>=2.31", + # Never imported by this package, and deliberately so. httpcore decides + # whether it is running under asyncio or trio by probing for sniffio on + # every async request (`try: import sniffio / except ImportError`). + # Nothing else in the chain requires it -- anyio 4.14 dropped the + # dependency and httpx does not replace it -- and Python never caches a + # failed import, so when sniffio is absent each request re-walks + # sys.path instead. Measured at a fan-out of 128 against a 50ms server: + # 3354ms without it, 1304ms with. `pip check` reports no broken + # requirements either way, so this pin is the only thing holding the fix. + "sniffio>=1.3", "tenacity>=8.2", "typing-extensions>=4.7; python_version < '3.11'", - "urllib3>=2.0", ] [project.optional-dependencies] @@ -78,7 +90,7 @@ dev = [ "sphinx-rtd-theme>=2.0", "tomli>=2.0; python_version < '3.11'", "twine>=5.1", - "types-requests>=2.32", + "unasync>=0.6", "vulture>=2.11", ] @@ -119,6 +131,16 @@ source = ["glpi_python_client"] omit = [ "*/tests/*", "glpi_python_client/testing/*", + # The async tree is the hand-written source; the sync tree is generated + # from it by a 1:1 token transform that only strips async/await. Every + # statement therefore exists in both, so measuring both would count the + # same logic twice and let real gaps hide behind a doubled denominator. + # Coverage is measured on the generated tree, which the suite exercises + # directly, and the correspondence between the two is enforced + # separately: unasync_build.py --check proves the trees match, mypy + # strict checks both, and tests/test_async_surface.py exercises the + # async surface at runtime -- including the paths that exist only there. + "glpi_python_client/_async/*", ] [tool.coverage.report] @@ -134,6 +156,12 @@ exclude_lines = [ [tool.ruff] line-length = 88 target-version = "py310" +# The sync client tree is generated by unasync_build.py from _async/ and +# must stay byte-identical to what regeneration produces -- that identity +# is what the CI gate checks. Formatting it would fight the generator, and +# reformatting the generated output is meaningless anyway: style is +# enforced on the hand-written source it comes from. mypy still checks it. +extend-exclude = ["glpi_python_client/_sync"] [tool.ruff.lint] select = ["B", "E", "F", "I", "RUF", "UP"] diff --git a/skills/README.md b/skills/README.md index 36f150e..83046c7 100644 --- a/skills/README.md +++ b/skills/README.md @@ -8,7 +8,7 @@ These skills are source-tree project material. They are included in source distr | Skill | Use when the agent needs to | Main public API | | --- | --- | --- | -| `glpi-client-setup` | Build and configure an authenticated client | `GlpiClient`, `GlpiClient.from_env()` | +| `glpi-client-setup` | Build and configure an authenticated client | `GlpiClient`, `AsyncGlpiClient`, `.from_env()` | | `glpi-ticket-workflow` | Search, fetch, create, update, or delete tickets | `GetTicket`, `PostTicket`, `PatchTicket`, `DeleteTicket` | | `glpi-ticket-timeline` | Read timeline records or write followups, tasks, solutions, and document links | `PostFollowup`, `PostTicketTask`, `PostSolution`, `PostTimelineDocument` (plus matching Get/Patch/Delete) | | `glpi-document-workflow` | Manage document metadata, upload binary content, download binaries | `GetDocument`, `PostDocument`, `PatchDocument`, `DeleteDocument` | @@ -16,4 +16,13 @@ These skills are source-tree project material. They are included in source distr | `glpi-reporting-and-context` | Aggregate ticket statistics, aggregate task durations, or load one ticket context bundle | `GlpiClient`, `GlpiTicketContext`, public enums | | `glpi-team-members` | List, add, or remove ticket team members | `GetTeamMember`, `PostTeamMember` | -The whole client is asynchronous: use `async with GlpiClient(...)` and `await` every method. +## Sync and async + +The package ships two clients with identical endpoint surfaces: + +- `GlpiClient` — synchronous. `with GlpiClient(...) as client`, no `await`. +- `AsyncGlpiClient` — asynchronous, performing real non-blocking I/O. `async with AsyncGlpiClient(...) as client`, `await` every method. + +Neither wraps the other: the async tree is hand-written and the synchronous one is generated from it by `unasync_build.py`, so the two cannot drift apart. The snippets in each skill are written against `AsyncGlpiClient`; every skill opens with a note on how to read them for the synchronous client. + +When fanning out concurrently on the async client, bound the fan-out with an `asyncio.Semaphore` — see `glpi-client-setup`. An unbounded fan-out is slower, not faster. diff --git a/skills/glpi-client-setup/SKILL.md b/skills/glpi-client-setup/SKILL.md index 15aa3cb..59092dd 100644 --- a/skills/glpi-client-setup/SKILL.md +++ b/skills/glpi-client-setup/SKILL.md @@ -15,11 +15,12 @@ The package exposes two clients with identical endpoint surfaces: - `glpi_python_client.GlpiClient` — synchronous, blocking client. Use it from scripts, CLI tools, or any code that is not already running inside an event loop. -- `glpi_python_client.AsyncGlpiClient` — asynchronous facade. Each method - is a coroutine that dispatches the underlying blocking call on a worker - thread (`asyncio.to_thread` by default, or a caller-supplied - `concurrent.futures.Executor`). Use it when an event loop is already - running or when you want concurrent fan-out via `asyncio.gather`. +- `glpi_python_client.AsyncGlpiClient` — asynchronous client. Each method is + a coroutine performing real non-blocking I/O on the event loop; there is + no worker thread and no executor. Use it when an event loop is already + running or when you want concurrent fan-out via `asyncio.gather` — + bounded, see step 8. Unlike the retired thread-pool bridge, cancelling + a call here actually releases its capacity, so timeouts work. Both clients share the same method names and signatures, including `from_env`, OAuth handling, retry behaviour, and the optional v1 @@ -47,8 +48,13 @@ call `client.close()` (or `await client.close()`) when finished. uploads are needed (`upload_document`). `v1_app_token` is optional. 7. Keep `verify_ssl=True` unless the user explicitly confirms a test or internal endpoint that cannot validate TLS. -8. For the async client only, optionally pass `executor=` a - `concurrent.futures.ThreadPoolExecutor` to bound worker threads. +8. Bound any large async fan-out with an `asyncio.Semaphore` on the + caller side. This is not just tidiness: the underlying HTTP pool + rescans itself on every request assignment, so an unbounded fan-out + saturates the event loop and gets *slower* as it widens — measured + against a 50 ms server, 16 concurrent calls took 350 ms unbounded and + 108 ms capped at 16. There is no `executor=` argument and no thread + pool to size. ## Environment Defaults @@ -152,6 +158,9 @@ with GlpiClient.from_env( document-upload fallback. - Closing the client matters because it owns one or two HTTP sessions plus an OAuth token manager. Prefer the context-manager form. -- A shared `threading.Lock` serialises OAuth token acquisition, so it - is safe to launch concurrent `asyncio.gather` fan-outs on - `AsyncGlpiClient` even before the token has been fetched once. +- Concurrent callers cannot stampede the token endpoint: the client + holds a lock around OAuth acquisition, so it is safe to launch a + fan-out on `AsyncGlpiClient` before the token has ever been fetched. + The primitive differs per surface — an `asyncio.Lock` on the async + client, a `threading.Lock` on the synchronous one — which is why a + synchronous client is the one safe to share across threads. diff --git a/skills/glpi-document-workflow/SKILL.md b/skills/glpi-document-workflow/SKILL.md index 3f1823e..d87b5b4 100644 --- a/skills/glpi-document-workflow/SKILL.md +++ b/skills/glpi-document-workflow/SKILL.md @@ -1,11 +1,11 @@ ---- +--- name: glpi-document-workflow -description: "Manage GLPI document metadata, upload binary content via the legacy v1 fallback, download document binaries, and link documents to a ticket timeline with the asynchronous glpi_python_client.GlpiClient and the GetDocument/PostDocument/PatchDocument/DeleteDocument models. Use for ticket attachments, document binary content, document metadata, or saving downloaded files." +description: "Manage GLPI document metadata, upload binary content via the legacy v1 fallback, download document binaries, and link documents to a ticket timeline with the synchronous glpi_python_client.GlpiClient or the asynchronous AsyncGlpiClient, and the GetDocument/PostDocument/PatchDocument/DeleteDocument models. Use for ticket attachments, document binary content, document metadata, or saving downloaded files." license: MIT compatibility: "Requires Python 3.10+, glpi-python-client, network access to the GLPI v2 API, and v1 credentials configured on the client for binary uploads." metadata: package: glpi-python-client - version: "0.3.0" + version: "0.4.0" --- # GLPI Document Workflow @@ -65,7 +65,7 @@ document_id = await client.create_document(PostDocument(name="Diagnostic notes") ## Gotchas - `upload_document` raises `RuntimeError` when the v1 session is not configured. Pass `v1_base_url` and `v1_user_token` to the client constructor or `from_env`. -- `upload_document` requires a non-empty `filename` and dispatches the blocking HTTP call through `asyncio.to_thread`; the running event loop is not blocked. +- `upload_document` requires a non-empty `filename`. On the async client the multipart POST is awaited like any other call, so the event loop is not blocked. - `download_document_content` returns `bytes` and raises on non-200 responses. - `mime_type` defaults to `application/octet-stream` when omitted on `upload_document`. - All methods are async; always `await` them. diff --git a/skills/glpi-reporting-and-context/SKILL.md b/skills/glpi-reporting-and-context/SKILL.md index 2a61408..946a57d 100644 --- a/skills/glpi-reporting-and-context/SKILL.md +++ b/skills/glpi-reporting-and-context/SKILL.md @@ -1,11 +1,11 @@ ---- +--- name: glpi-reporting-and-context -description: "Aggregate GLPI ticket and task statistics and load grouped ticket contexts with the asynchronous glpi_python_client.GlpiClient. Use for operational reporting, ticket counts grouped by entity/status/priority/type, task duration totals grouped by user/entity/ticket, per-user activity reports, batch-streamed pagination of search results, or one-call ticket context retrieval bundling tickets with timeline records." +description: "Aggregate GLPI ticket and task statistics and load grouped ticket contexts with the synchronous glpi_python_client.GlpiClient or the asynchronous AsyncGlpiClient. Use for operational reporting, ticket counts grouped by entity/status/priority/type, task duration totals grouped by user/entity/ticket, per-user activity reports, batch-streamed pagination of search results, or one-call ticket context retrieval bundling tickets with timeline records." license: MIT compatibility: "Requires Python 3.10+, glpi-python-client, network access to the GLPI v2 API, and credentials allowed to read tickets, tasks, users, entities, and timeline records." metadata: package: glpi-python-client - version: "0.3.0" + version: "0.4.0" --- # GLPI Reporting And Context @@ -13,7 +13,7 @@ metadata: Custom helpers on `GlpiClient` build on top of the contract-aligned API mixins: -- `get_ticket_context(ticket_id)` returns one `GlpiTicketContext` bundling the primary ticket together with its tasks, followups, solutions, and timeline document links. The five underlying calls run concurrently via `asyncio.gather`. +- `get_ticket_context(ticket_id)` returns one `GlpiTicketContext` bundling the primary ticket together with its tasks, followups, solutions, and timeline document links. The five underlying calls are independent and are issued through the library's internal `gather` helper, so they fan out concurrently on `AsyncGlpiClient` and run one after another on `GlpiClient`. Expect the synchronous call to take roughly five round trips. - `get_ticket_statistics(...)` returns ticket counts grouped by entity, status, priority, and type over an ISO date window applied to GLPI `date_creation`. Accepts `entity_id`, `entity_name` (substring match resolved via `search_entities`), and `extra_filter` (raw RSQL AND-joined with the window). - `get_task_statistics(ticket_ids)` returns task duration totals grouped by user and ticket for a caller-supplied list of ticket IDs. - `get_task_durations(...)` is a higher-level helper that internally iterates `iter_search_tickets` with a date/entity/user filter, computes per-user and per-entity duration totals, and optionally returns a flat per-task list when `return_task_details=True`. @@ -24,7 +24,7 @@ Returned identifiers are raw GLPI numeric values; resolve them with the appropri ## Procedure -1. Create a `GlpiClient` with the correct entity/profile scope. +1. Create a client (`GlpiClient` or `AsyncGlpiClient`) with the correct entity/profile scope. 2. For one ticket, call `await client.get_ticket_context(ticket_id)` and read `bundle.ticket`, `bundle.tasks`, `bundle.followups`, `bundle.solutions`, and `bundle.documents`. 3. For ticket counts, call `await client.get_ticket_statistics(start_date=..., end_date=..., default_days=..., entity_id=..., entity_name=..., extra_filter=...)`. All keyword arguments are optional; the default window is the last 30 days ending today. 4. For task duration totals on a known ticket list, call `await client.get_task_statistics(ticket_ids)`. For an end-to-end "duration over a window with filters" report, call `await client.get_task_durations(...)` instead; it gathers the ticket IDs internally. @@ -103,6 +103,19 @@ print(f"processed {total} tickets") ## Gotchas - All helpers shown above are async on `AsyncGlpiClient`; always `await` them. The `iter_search_*` helpers are async **generators** -- use `async for`, not `await`. +- **Bound any fan-out you build on top of these helpers.** Calling `get_ticket_context` for every ticket in a batch with a bare `asyncio.gather` gets *slower* as the batch grows: the underlying HTTP pool rescans itself on every request assignment, so a wide fan-out saturates the event loop and the observed concurrency falls. Cap it instead: + + ```python + gate = asyncio.Semaphore(16) + + async def one(ticket_id): + async with gate: + return await client.get_ticket_context(ticket_id) + + contexts = await asyncio.gather(*(one(t.id) for t in batch)) + ``` + + Measured against a 50 ms server, a fan-out of 16 took 350 ms unbounded and 108 ms capped at 16. This is a property of the HTTP layer, not of this library, and there is no version to upgrade to. - `get_ticket_statistics`, `get_task_durations`, and `get_user_activity` validate their date window locally and raise `ValueError` when `default_days < 1` or `start_date > end_date`. The window is applied to `date_creation` server-side. - `get_task_statistics(ticket_ids=[])` returns zeroed totals without any HTTP call. `get_task_durations` likewise returns zeroed totals when no tickets match the filter, and short-circuits with zeros when `entity_name` resolves to no entities. - `get_user_activity` raises `ValueError` when no identifier is supplied and when the criteria match no users. Multiple users with the same `f"{firstname} {realname}"` display key are merged into one bucket. diff --git a/skills/glpi-team-members/SKILL.md b/skills/glpi-team-members/SKILL.md index 545eebb..871e832 100644 --- a/skills/glpi-team-members/SKILL.md +++ b/skills/glpi-team-members/SKILL.md @@ -1,11 +1,11 @@ ---- +--- name: glpi-team-members -description: "List, add, and remove GLPI ticket team members with the asynchronous glpi_python_client.GlpiClient and the GetTeamMember/PostTeamMember models. Use when assigning users or groups to tickets, inspecting ticket teams, or removing GLPI ticket participants." +description: "List, add, and remove GLPI ticket team members with the synchronous glpi_python_client.GlpiClient or the asynchronous AsyncGlpiClient, and the GetTeamMember/PostTeamMember models. Use when assigning users or groups to tickets, inspecting ticket teams, or removing GLPI ticket participants." license: MIT compatibility: "Requires Python 3.10+, glpi-python-client, network access to the GLPI v2 API, and credentials allowed to manage ticket teams." metadata: package: glpi-python-client - version: "0.3.0" + version: "0.4.0" --- # GLPI Team Members diff --git a/skills/glpi-ticket-timeline/SKILL.md b/skills/glpi-ticket-timeline/SKILL.md index 8bad81d..7d2e806 100644 --- a/skills/glpi-ticket-timeline/SKILL.md +++ b/skills/glpi-ticket-timeline/SKILL.md @@ -1,11 +1,11 @@ ---- +--- name: glpi-ticket-timeline -description: "Read GLPI ticket timeline records and create or update followups, tasks, solutions, and timeline document links with the asynchronous glpi_python_client.GlpiClient. Use when handling ticket notes, followups, tasks, solutions, or attached documents on a GLPI ticket timeline." +description: "Read GLPI ticket timeline records and create or update followups, tasks, solutions, and timeline document links with the synchronous glpi_python_client.GlpiClient or the asynchronous AsyncGlpiClient. Use when handling ticket notes, followups, tasks, solutions, or attached documents on a GLPI ticket timeline." license: MIT compatibility: "Requires Python 3.10+, glpi-python-client, and network access to the GLPI v2 API." metadata: package: glpi-python-client - version: "0.3.0" + version: "0.4.0" --- # GLPI Ticket Timeline diff --git a/skills/glpi-ticket-workflow/SKILL.md b/skills/glpi-ticket-workflow/SKILL.md index d10b499..5256019 100644 --- a/skills/glpi-ticket-workflow/SKILL.md +++ b/skills/glpi-ticket-workflow/SKILL.md @@ -1,17 +1,17 @@ ---- +--- name: glpi-ticket-workflow -description: "Search, fetch, create, update, and delete GLPI tickets with the asynchronous glpi_python_client.GlpiClient and the GetTicket/PostTicket/PatchTicket/DeleteTicket models. Use for GLPI ticket records, ticket filters, fields, pagination, status, priority, category, location, or instance-specific extra_payload values." +description: "Search, fetch, create, update, and delete GLPI tickets with the synchronous glpi_python_client.GlpiClient or the asynchronous AsyncGlpiClient, and the GetTicket/PostTicket/PatchTicket/DeleteTicket models. Use for GLPI ticket records, ticket filters, fields, pagination, status, priority, category, location, or instance-specific extra_payload values." license: MIT compatibility: "Requires Python 3.10+, glpi-python-client, network access to the GLPI v2 API, and credentials accepted by GlpiClient." metadata: package: glpi-python-client - version: "0.3.0" + version: "0.4.0" --- # GLPI Ticket Workflow > The snippets below use `AsyncGlpiClient` (`async with` + `await`). Every method shown also exists on the synchronous `GlpiClient` with the same signature -- replace `async with` with `with`, drop the `await` keyword, and skip the surrounding `async def`/`asyncio.run` scaffolding. -Use this skill for ticket reads and writes through the public asynchronous client. Tickets live under `/Assistance/Ticket` on the GLPI v2 API and are exposed by five `GlpiClient` methods: `search_tickets`, `get_ticket`, `create_ticket`, `update_ticket`, and `delete_ticket`. +Use this skill for ticket reads and writes through the public client. Tickets live under `/Assistance/Ticket` on the GLPI v2 API and are exposed by five methods, present on both `GlpiClient` and `AsyncGlpiClient` with identical signatures: `search_tickets`, `get_ticket`, `create_ticket`, `update_ticket`, and `delete_ticket`. ## Procedure diff --git a/skills/glpi-user-location-provisioning/SKILL.md b/skills/glpi-user-location-provisioning/SKILL.md index 5afd2f0..6a3cbb1 100644 --- a/skills/glpi-user-location-provisioning/SKILL.md +++ b/skills/glpi-user-location-provisioning/SKILL.md @@ -1,11 +1,11 @@ ---- +--- name: glpi-user-location-provisioning -description: "Search GLPI users, locations, and entities, or create, update, and delete users and locations and entities with the asynchronous glpi_python_client.GlpiClient and the matching Get/Post/Patch/Delete models. Use for user lookup, entity lookup, location lookup, user provisioning, location creation, GLPI entity defaults, or RSQL filters." +description: "Search GLPI users, locations, and entities, or create, update, and delete users and locations and entities with the synchronous glpi_python_client.GlpiClient or the asynchronous AsyncGlpiClient, and the matching Get/Post/Patch/Delete models. Use for user lookup, entity lookup, location lookup, user provisioning, location creation, GLPI entity defaults, or RSQL filters." license: MIT compatibility: "Requires Python 3.10+, glpi-python-client, network access to the GLPI v2 API, and credentials allowed to read or write users, locations, and entities." metadata: package: glpi-python-client - version: "0.3.0" + version: "0.4.0" --- # GLPI User, Location, And Entity Provisioning @@ -62,7 +62,7 @@ location_id = ( Look entities up by name fragment: ```python -entities = await client.search_entities("name=like=*novahe*", limit=10) +entities = await client.search_entities("name=like=*acme*", limit=10) for entity in entities: print(entity.id, entity.name, entity.completename) ``` diff --git a/unasync_build.py b/unasync_build.py new file mode 100644 index 0000000..dfc122e --- /dev/null +++ b/unasync_build.py @@ -0,0 +1,238 @@ +"""Generate the synchronous client tree from the asynchronous one. + +``glpi_python_client/_async/`` is the only hand-written client source. +``glpi_python_client/_sync/`` is produced from it by :mod:`unasync`, checked +into the repository, and verified in CI with ``--check``. Editing anything +under ``_sync/`` by hand is a mistake the check will catch. + +Usage +----- +``python unasync_build.py`` regenerate ``_sync/`` in place +``python unasync_build.py --check`` fail if ``_sync/`` is stale (CI gate) + +What the codegen does and does not do +------------------------------------- +:mod:`unasync` is a token-level rewriter. It strips ``async``/``await`` and +substitutes whole NAME tokens listed in the rule. Three consequences shape +this build: + +* It matches **single NAME tokens only**. A dotted key such as + ``"asyncio.Lock"`` can never match, and supplying one fails *silently* -- + the rule is accepted and simply never fires. Anything needing a dotted + name is handled by the hand-written ``_concurrency.py`` twins instead. +* It rewrites a **string literal whose entire content** is a substitution + key. That is what makes ``__all__ = ["AsyncGlpiClient"]`` generate + correctly. It is narrow: an embedded mention inside a longer string is + left alone. That is right for prose but wrong for a *qualified path* + written in prose, so this script repoints those itself -- see + :data:`PROSE_PACKAGE_PREFIX`. +* It leaves ``asyncio.gather`` and friends **intact**, which would produce + broken sync code. Those live only in ``_concurrency.py``, which is + excluded from generation and hand-written on both sides. +""" + +from __future__ import annotations + +import argparse +import difflib +import pathlib +import shutil +import sys +import tempfile + +import unasync + +REPO_ROOT = pathlib.Path(__file__).resolve().parent +PACKAGE = REPO_ROOT / "glpi_python_client" +ASYNC_DIR = PACKAGE / "_async" +SYNC_DIR = PACKAGE / "_sync" + +#: Files that are hand-written on *both* sides and never generated. +#: +#: ``_concurrency.py`` is the only one. It is where the two trees genuinely +#: differ in kind rather than in syntax: a fan-out is ``asyncio.gather`` on +#: one side and plain sequential evaluation on the other, and the auth lock +#: is an ``asyncio.Lock`` on one side and a ``threading.Lock`` on the other. +#: Token substitution cannot express either, so both files are maintained by +#: hand and kept deliberately tiny. +HAND_WRITTEN = {"_concurrency.py"} + +#: Token substitutions beyond unasync's defaults. +#: +#: The defaults cover the language-level names (``__aenter__``, +#: ``AsyncIterator``, ``StopAsyncIteration``, ...). These cover the two +#: things the defaults get wrong or do not know about: +#: +#: * **httpx naming.** unasync's built-in ``Async*`` -> ``Sync*`` convention +#: would produce ``SyncClient``, which does not exist; the real name is +#: ``Client``. Same for the transport classes. +#: * **This package's own public class name**, which differs between the two +#: surfaces by design. +#: +#: Everything else -- mixins, helpers, module names -- is spelled +#: *identically* in both trees. Keeping the rename list this short is +#: deliberate: every entry is a chance for a silent collision, and the +#: shorter the list, the smaller that surface. +TOKEN_REPLACEMENTS = { + # Intra-tree imports are absolute, so the package segment itself is a + # NAME token and rewriting it repoints every one of them at the + # generated tree. This is why no module needs relative imports. + "_async": "_sync", + "AsyncGlpiClient": "GlpiClient", + "AsyncClient": "Client", + "AsyncBaseTransport": "BaseTransport", + "AsyncHTTPTransport": "HTTPTransport", + "aclose": "close", + "aread": "read", +} + + +#: The qualified package prefix, and what it becomes in the generated tree. +#: +#: In an ``import`` statement ``_async`` is its own NAME token, so unasync +#: repoints every intra-tree import for free. Inside a docstring the whole +#: thing is a *single* string token, and substitution only fires when a +#: literal's entire content is a key -- so a cross-reference such as +#: ``:mod:`glpi_python_client._async.clients.api``` sails through untouched +#: and the generated client documents itself in terms of the other tree. +#: +#: Rewriting the qualified prefix afterwards is safe precisely because it is +#: qualified: it names this package and a tree that the generated code must +#: never mention. A bare ``_async`` in prose would be ambiguous; this is not. +#: ``tests/test_unasync_codegen.py`` holds the invariant from the other end, +#: asserting the generated tree contains no ``_async`` at all -- which also +#: fails if someone writes a bare mention this rewrite cannot see. +PROSE_PACKAGE_PREFIX = ("glpi_python_client._async", "glpi_python_client._sync") + + +def _source_files() -> list[pathlib.Path]: + """Return every ``_async/`` module that should be generated from.""" + + return sorted( + path + for path in ASYNC_DIR.rglob("*.py") + if path.name not in HAND_WRITTEN and "__pycache__" not in path.parts + ) + + +def _repoint_prose(into: pathlib.Path, generated: list[pathlib.Path]) -> None: + """Point qualified package paths in the generated files at the sync tree. + + Runs over the freshly written modules only, and is idempotent: unasync + has already turned the import statements into ``_sync``, so the stale + prefix survives nowhere but inside string literals. + """ + + stale, fresh = PROSE_PACKAGE_PREFIX + for rel in generated: + path = into / rel + text = path.read_text(encoding="utf-8") + if stale in text: + path.write_text(text.replace(stale, fresh), encoding="utf-8") + + +def _generate(into: pathlib.Path) -> None: + """Run unasync over the async tree, writing the sync tree into ``into``.""" + + rule = unasync.Rule( + fromdir=str(ASYNC_DIR), + todir=str(into), + additional_replacements=TOKEN_REPLACEMENTS, + ) + sources = _source_files() + unasync.unasync_files([str(p) for p in sources], [rule]) + _repoint_prose(into, [p.relative_to(ASYNC_DIR) for p in sources]) + + # The hand-written twins are never generated. When building into a + # scratch directory for --check they must still be carried across, or + # the comparison would report them as missing from the generated tree + # and demand their deletion. Generating in place leaves them untouched. + if into == SYNC_DIR: + return + for name in sorted(HAND_WRITTEN): + for existing in SYNC_DIR.rglob(name): + target = into / existing.relative_to(SYNC_DIR) + target.parent.mkdir(parents=True, exist_ok=True) + shutil.copy2(existing, target) + + +def _relative_sync_files(root: pathlib.Path) -> dict[pathlib.Path, str]: + """Return ``{relative path: text}`` for every module under ``root``.""" + + return { + path.relative_to(root): path.read_text(encoding="utf-8") + for path in sorted(root.rglob("*.py")) + if "__pycache__" not in path.parts + } + + +def _check() -> int: + """Regenerate into a temp dir and report any drift from the checked-in tree.""" + + with tempfile.TemporaryDirectory() as tmp: + scratch = pathlib.Path(tmp) / "_sync" + _generate(scratch) + expected = _relative_sync_files(scratch) + actual = _relative_sync_files(SYNC_DIR) + + problems: list[str] = [] + for rel in sorted(set(expected) | set(actual)): + want = expected.get(rel) + have = actual.get(rel) + if want == have: + continue + if want is None: + problems.append(f"{rel}: present in _sync/ but not generated by _async/") + continue + if have is None: + problems.append( + f"{rel}: missing from _sync/ -- run: python unasync_build.py" + ) + continue + diff = "".join( + difflib.unified_diff( + have.splitlines(keepends=True), + want.splitlines(keepends=True), + fromfile=f"_sync/{rel} (checked in)", + tofile=f"_sync/{rel} (regenerated)", + ) + ) + problems.append(diff) + + if problems: + print("_sync/ is out of date with respect to _async/.\n") + print("\n".join(problems)) + print( + "\nThe sync tree is generated. Edit glpi_python_client/_async/ and " + "run `python unasync_build.py`." + ) + return 1 + + print(f"_sync/ is up to date ({len(actual)} modules).") + return 0 + + +def main() -> int: + """Entry point for both regeneration and the CI staleness check.""" + + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--check", + action="store_true", + help="verify _sync/ matches what _async/ generates instead of writing it", + ) + args = parser.parse_args() + + if not ASYNC_DIR.is_dir(): + print(f"no async source tree at {ASYNC_DIR}", file=sys.stderr) + return 1 + if args.check: + return _check() + + _generate(SYNC_DIR) + print(f"regenerated {SYNC_DIR} from {len(_source_files())} async modules.") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main())