diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..33d445e --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,136 @@ +# Changelog + +All notable changes to this project are documented in this file. + +The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). + +## Unreleased + +### Fixed + +- `GLPITokenManager._refresh_access_token`'s retry decorator no longer + retries a `GlpiServerError` from its fall-through to the nested + `_acquire_token()` call. That nested call already carries its own + independent 3-attempt retry decorator for `GlpiServerError`, so the + outer decorator retrying it too meant a persistent 5xx during token + refresh cost 3 (outer attempts) × (1 refresh POST + 3 nested acquire + POSTs) = 12 POST requests and ~33s of `wait_fixed(3)` sleep, instead of + the 3 attempts the retry configuration alone would suggest. The outer + decorator now only retries `requests.RequestException` (a genuine + network fault on the refresh POST itself), which is not covered by the + nested call at all. A persistent 5xx now costs exactly 1 refresh POST + + 3 nested acquire POSTs = 4 POST requests. A persistent 401 (2 POSTs) and + a network error on the refresh POST (3 POSTs) are unaffected. +- `AsyncGlpiClient.create_kb_article` / `update_kb_article` no longer + silently drop `categories`. Both methods called the public + `set_kb_article_categories` through `self` from inside a synchronous + method body; `AsyncBridge.__init_subclass__` wraps every public sync + method into a coroutine, so that call returned an un-awaited coroutine + instead of performing the write. The article was created (or updated) + successfully, a valid id was returned, and no exception was raised — + the category assignment simply never happened. Fixed with hand-written + async overrides in `_article_async.py` that strip `categories` from the + v2 body, run the v2 write in a worker thread, and apply the category + fallback through an awaited call. +- `AsyncGlpiClient.get_ticket_custom_fields` / `set_ticket_custom_fields` + raised `TypeError: 'coroutine' object is not iterable` and were + unusable. Same root cause as above: a sync method reaching a sibling + public method through `self` received a coroutine instead of a result. + Fixed with hand-written async overrides in `_fields_async.py`. +- The integration suite is runnable end-to-end again. Two defects, both in + `integration_tests/` only (no library code involved): + - `test_iter_search_tickets_multi_page` walked *every* matching ticket in + batches of 3 with no upper bound — it was the only one of the suite's + seven `iter_search` loops missing a `break`. Against a real instance + (59,879 matching tickets) that is ~19,960 requests and several hours, + which stalled the whole suite. It now stops after 3 pages and asserts + that ids do not repeat across pages, which actually verifies that the + `start` offset advances — the old unbounded loop asserted only + `isinstance(collected, list)` and so could not have detected a stuck + offset. + - The three GLPI Fields plugin tests failed rather than skipped when the + plugin is not installed. `_skip_when_no_v1` only checked that v1 + *credentials were configured*, never that the *plugin existed*; an + absent plugin makes GLPI reject the `PluginFieldsContainer` itemtype + with a 400 rather than return an empty list. A new `fields_containers` + fixture skips on exactly that signature (400 + + `ERROR_RESOURCE_NOT_FOUND_NOR_COMMONDBTM`) and re-raises anything else. +- `parse_optional_env_int` (environment/config parsing) and + `StatisticsMixin._resolve_window` (the date-window helper behind + `get_ticket_statistics` / `get_task_durations` / `get_user_activity`) + no longer let a malformed value escape as a bare stdlib `ValueError` + from `int()` / `date.fromisoformat()` (e.g. `GLPI_TIMEOUT=abc` or + `get_ticket_statistics(start_date="2026-13-45")`). Both now raise + `GlpiValidationError`, chaining the original error via `from` rather + than swallowing it. Non-breaking: `GlpiValidationError` inherits + `ValueError`, so `except ValueError` still catches it. + +### Added + +- `glpi_python_client/clients/tests/test_async_selfcall_guard.py`: a + structural AST guard that fails the suite if any public method on + `GlpiClient` transitively reaches another public method through a + literal `self.name(...)` call (directly, or via a private helper) + without a corresponding hand-written async override on + `AsyncGlpiClient`. This prevents the same bug class — silent data loss + or a `TypeError` at call time, depending on how the dropped coroutine is + used — from being reintroduced by a future endpoint. +- A public exception hierarchy, exported from the package root: + `GlpiError`, `GlpiTransportError`, `GlpiTimeoutError`, `GlpiStatusError`, + `GlpiAuthError`, `GlpiNotFoundError`, `GlpiServerError`, + `GlpiValidationError` and `GlpiProtocolError`. `GlpiStatusError` and its + subclasses carry `.status_code`, `.url` and `.response_text`. A GLPI 404 + and a bad argument were previously both a bare `ValueError` and could not + be told apart. +- `FakeResponse` (in the public `glpi_python_client.testing` module) gained + a `url` attribute. +- A user-guide "Error handling" section documenting the exception + hierarchy and the retry behaviour for both the transport layer and OAuth + token acquisition/refresh. + +### Changed + +- **Breaking:** a persistent 5xx now raises `GlpiServerError` instead of + `tenacity.RetryError`. The retry decorators gained `reraise=True`. Code + doing `except tenacity.RetryError` and digging out + `.last_attempt.exception()` should now catch `GlpiServerError` directly. +- **Breaking:** unexpected HTTP statuses raise a `GlpiStatusError` subclass; + rejected arguments and configuration raise `GlpiValidationError`; 2xx + responses with an unusable body raise `GlpiProtocolError`. All three + inherit `ValueError`, so existing `except ValueError` handlers keep + working. +- **Breaking:** a non-2xx OAuth token response raises `GlpiAuthError` (401/403) + or `GlpiServerError` (5xx). The token retry decorators had no `retry=` + predicate and therefore retried every failure, including a rejected + credential; a wrong `client_secret` cost 3 attempts and 6 seconds. OAuth + 4xx is now final, matching the rest of the library. OAuth 5xx is still + retried. +- **Breaking:** the private `glpi_python_client.clients.commons._errors` + module and its `remote_error_message` helper are removed. It had no + library call sites, and `reraise=True` leaves it nothing to unwrap. + +### Unchanged (deliberately) + +- Retry semantics: 5xx retried 3 times with a 3-second fixed wait, 4xx never + retried. +- Tolerant search endpoints still return `[]` rather than raising on a 4xx. +- The `TypeError` sites in environment parsing and the `RuntimeError` sites + for closed clients, missing v1 sessions and partial KB failures still + raise those types. `GlpiValidationError` inherits `ValueError`, not + `TypeError`, so converting them would break `except TypeError` callers. +- The transport is still `requests`. Network faults (connection reset, DNS, + timeout) still surface as `requests` exceptions; they become + `GlpiTransportError` / `GlpiTimeoutError` when the transport moves to + httpx, with no change to the class names above. + +### Notes + +- Both fixed bugs shared one root cause: `AsyncBridge` wraps every public + sync method into a coroutine, so a sync method body calling a sibling + public method through `self` (rather than through a hand-written async + override) silently receives a coroutine instead of the real return + value. +- This is a documentation-only release note; **no version was released** + from this branch. The next release is planned as 0.4.0, an httpx + + unasync rewrite that removes `AsyncBridge` entirely, making this class + of bug structurally impossible rather than merely guarded against. diff --git a/docs/api_reference.rst b/docs/api_reference.rst index 7df5bcd..e512a4e 100644 --- a/docs/api_reference.rst +++ b/docs/api_reference.rst @@ -28,6 +28,61 @@ the asynchronous one wraps each synchronous method into a coroutine. :members: :show-inheritance: +Exceptions +---------- + +Exceptions raised for a bad argument, an unexpected HTTP status, or an +unusable response body derive from :class:`GlpiError`. +:class:`GlpiStatusError`, :class:`GlpiValidationError` and +:class:`GlpiProtocolError` also inherit :class:`ValueError` for backwards +compatibility with releases that raised bare ``ValueError``. + +This is not the library's entire failure surface. Network-level faults +(connection failures, DNS errors, timeouts) still propagate as +``requests`` exceptions today: :class:`GlpiTransportError` and +:class:`GlpiTimeoutError` are reserved for that case but are not raised +until a future httpx transport swap. A handful of sites also +deliberately still raise bare ``RuntimeError`` or ``TypeError`` instead +of a library type, so existing ``except RuntimeError`` / ``except +TypeError`` code keeps working. See :ref:`error-handling` in the user +guide for the full picture, including which methods raise which type. + +.. autoexception:: GlpiError + :members: + :show-inheritance: + +.. autoexception:: GlpiTransportError + :members: + :show-inheritance: + +.. autoexception:: GlpiTimeoutError + :members: + :show-inheritance: + +.. autoexception:: GlpiStatusError + :members: + :show-inheritance: + +.. autoexception:: GlpiAuthError + :members: + :show-inheritance: + +.. autoexception:: GlpiNotFoundError + :members: + :show-inheritance: + +.. autoexception:: GlpiServerError + :members: + :show-inheritance: + +.. autoexception:: GlpiValidationError + :members: + :show-inheritance: + +.. autoexception:: GlpiProtocolError + :members: + :show-inheritance: + Aggregated Models ----------------- diff --git a/docs/development.md b/docs/development.md index 81b17f6..03314f1 100644 --- a/docs/development.md +++ b/docs/development.md @@ -62,13 +62,22 @@ python -m pytest transport serialises OAuth token acquisition so concurrent `asyncio.gather` fan-outs on the async client cannot race. - `glpi_python_client.clients.api.*` contains the contract-aligned - synchronous endpoint mixins, grouped by GLPI subtree (administration, - assistance, assistance/timeline, dropdowns, management). + endpoint mixins, grouped by GLPI subtree (administration, assistance, + assistance/timeline, dropdowns, management, knowledgebase, plugins). + Most are synchronous; `knowledgebase/_article_async.py` and + `plugins/_fields_async.py` are hand-written async overrides needed + because their synchronous bodies call a sibling public method through + `self` (see the `clients.custom` entry below for the other reason a + method needs one). - `glpi_python_client.clients.custom` contains custom helpers built on top of the API mixins. Each helper has a synchronous implementation - (`_ticket_context.py`, `_statistics.py`) plus an optional async - override (`_ticket_context_async.py`, `_statistics_async.py`) that - fans the underlying calls out concurrently with `asyncio.gather`. + (`_ticket_context.py`, `_statistics.py`) plus an async override + (`_ticket_context_async.py`, `_statistics_async.py`) that fans the + underlying calls out concurrently with `asyncio.gather`. That is one + of two reasons a method needs a hand-written async override; the + other — a synchronous body calling a sibling public method through + `self` — is why `clients.api.knowledgebase` and `clients.api.plugins` + also ship one (see above). - `glpi_python_client.auth._v1_session` contains the legacy v1 session used for binary document uploads. - `glpi_python_client.models` contains typed request and response @@ -93,7 +102,11 @@ python -m pytest async client picks the new method up automatically through the `AsyncBridge` — do not duplicate the method on a parallel async mixin unless you genuinely need concurrent fan-out (`asyncio.gather`) - inside the method body. + inside the method body, **or** the method calls a sibling public + method through `self` (directly, or transitively via a private + helper): the bridge wraps that sibling into a coroutine, so the + un-awaited call silently drops instead of running. The guard in step + 4 fails the suite if you miss this. 3. Put reusable endpoint names, payload builders, response handling, or pagination logic in the focused `glpi_python_client.clients.commons` helper module named for that @@ -101,7 +114,10 @@ python -m pytest 4. Add unit tests for payload serialization, response parsing, and client behavior. The parity test in `glpi_python_client/clients/tests/test_parity.py` will fail if the - sync and async surfaces diverge. + sync and async surfaces diverge, and + `glpi_python_client/clients/tests/test_async_selfcall_guard.py` will + fail if a public method reaches another public method through `self` + without a hand-written async override. 5. Document the new workflow in `docs/user_guide.rst` or the README. Keep organization-specific defaults outside the package core. diff --git a/docs/user_guide.rst b/docs/user_guide.rst index 77f8ba6..0e0bbd2 100644 --- a/docs/user_guide.rst +++ b/docs/user_guide.rst @@ -42,6 +42,8 @@ The guide is split into the following sections: context view, and the reporting helpers. 6. **End-to-end examples** — full workflows that combine the previous building blocks. +7. **Error handling** — the public exception hierarchy, what each + branch means, and how retries behave. The sample snippets in sections 3 to 6 use the synchronous :class:`GlpiClient`. Every snippet works on the asynchronous client by @@ -1396,4 +1398,113 @@ Example output:: 'duration_by_user': {'22': 4500, '21': 1800}, 'duration_by_ticket': {120: 1800, 121: 900, 122: 1800, 123: 1800}, }, - } \ No newline at end of file + } + +.. _error-handling: + +7. Error handling +----------------- + +Exceptions the client raises for a bad argument, an unexpected HTTP +status, or an unusable response body derive from +:class:`~glpi_python_client.GlpiError`, so one handler covers that part +of the library surface: + +.. code-block:: python + + from glpi_python_client import GlpiClient, GlpiError + + client = GlpiClient.from_env() + try: + ticket = client.get_ticket(42) + 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: + +.. code-block:: python + + import requests + from glpi_python_client import GlpiClient, GlpiError + + client = GlpiClient.from_env() + try: + ticket = client.get_ticket(42) + except (GlpiError, requests.RequestException) as exc: + print(f"GLPI call failed: {exc}") + +A handful of sites also deliberately still raise bare ``RuntimeError`` +(using a closed client, a missing v1 document session, a partially +failed knowledge-base write) or ``TypeError`` (a malformed environment +value) instead of a library type, so ``except RuntimeError`` / ``except +TypeError`` code written against earlier releases keeps working. + +The hierarchy lets you narrow as far as you need: + +.. code-block:: text + + GlpiError + ├── GlpiTransportError reserved for the httpx transport swap; + │ └── GlpiTimeoutError not raised yet -- see the note above + ├── GlpiStatusError GLPI answered with an unexpected status + │ ├── GlpiAuthError 401 / 403 + │ ├── GlpiNotFoundError 404 + │ └── GlpiServerError 5xx (retried up to 3 attempts before it + │ reaches you) + ├── GlpiValidationError the client rejected your argument + └── GlpiProtocolError GLPI answered 2xx with an unusable body + +:class:`~glpi_python_client.GlpiStatusError` carries the diagnostics you +usually want: + +.. code-block:: python + + from glpi_python_client import GlpiNotFoundError + + try: + ticket = client.get_ticket(999999) + except GlpiNotFoundError as exc: + print(exc.status_code) # 404 + print(exc.url) # the absolute URL that was requested + print(exc.response_text) # the response body + +.. note:: + + :class:`~glpi_python_client.GlpiStatusError`, + :class:`~glpi_python_client.GlpiValidationError` and + :class:`~glpi_python_client.GlpiProtocolError` also inherit + :class:`ValueError`. Code written against earlier releases, which + raised bare ``ValueError``, keeps working unchanged. + +Retry behaviour +~~~~~~~~~~~~~~~ + +Each transport and v1-session retry decorator retries a server error +(5xx) up to 3 attempts with a 3-second fixed wait before +:class:`~glpi_python_client.GlpiServerError` reaches you. Client errors +(4xx) are never retried — they cannot succeed on a second attempt. + +OAuth token acquisition follows the same 3-attempt policy, with one +exception: refreshing an already-issued token does not raise directly on +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) — +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 +refreshing therefore costs exactly 1 refresh POST + up to 3 nested +acquisition POSTs = 4 POST requests before +:class:`~glpi_python_client.GlpiServerError` reaches you. A rejected +credential (401/403) is not retried at either layer and fails after at +most 2 POST requests. + +Search methods are deliberately tolerant: ``search_tickets`` and its +siblings return an empty list rather than raising when GLPI rejects the +query. Methods that fetch or mutate one specific record always raise. \ No newline at end of file diff --git a/glpi_python_client/__init__.py b/glpi_python_client/__init__.py index 7aae164..d3c0b66 100644 --- a/glpi_python_client/__init__.py +++ b/glpi_python_client/__init__.py @@ -14,6 +14,17 @@ from __future__ import annotations +from glpi_python_client._errors import ( + GlpiAuthError, + GlpiError, + GlpiNotFoundError, + GlpiProtocolError, + GlpiServerError, + GlpiStatusError, + GlpiTimeoutError, + GlpiTransportError, + GlpiValidationError, +) from glpi_python_client.clients import AsyncGlpiClient, GlpiClient from glpi_python_client.models import ( DeleteDocument, @@ -123,17 +134,26 @@ "GetTicketTask", "GetTimelineDocument", "GetUser", + "GlpiAuthError", "GlpiClient", "GlpiEnum", + "GlpiError", "GlpiGlobalValidation", + "GlpiNotFoundError", "GlpiPriority", + "GlpiProtocolError", + "GlpiServerError", "GlpiSolutionStatus", + "GlpiStatusError", "GlpiTaskState", "GlpiTicketContext", "GlpiTicketStatus", "GlpiTicketType", "GlpiTimelinePosition", + "GlpiTimeoutError", + "GlpiTransportError", "GlpiUserAuthType", + "GlpiValidationError", "IdNameCompletenameRef", "IdNameRef", "IdRef", diff --git a/glpi_python_client/_errors.py b/glpi_python_client/_errors.py new file mode 100644 index 0000000..19026c8 --- /dev/null +++ b/glpi_python_client/_errors.py @@ -0,0 +1,177 @@ +"""Public exception hierarchy raised by :mod:`glpi_python_client`. + +Every exception the client raises for a bad argument, an unexpected HTTP +status, or an unusable response body deliberately derives from +:class:`GlpiError`, so callers can catch that part of the library surface +with a single ``except`` clause. This is not yet the library's entire +failure surface, and importing the underlying HTTP library is still +sometimes necessary: + +* Network-level faults (connection failures, DNS errors, timeouts) still + propagate as ``requests`` exceptions today. :class:`GlpiTransportError` + and :class:`GlpiTimeoutError` are reserved for that case but are not + raised until the httpx transport swap replaces ``requests`` (a later + release); catch ``requests.RequestException`` alongside + :class:`GlpiError` if you need to handle them now. +* A handful of sites also deliberately still raise bare ``RuntimeError`` + (using a closed client, a missing v1 document session, a partially + failed knowledge-base write) or ``TypeError`` (a malformed environment + value) instead of a library type, so ``except RuntimeError`` / ``except + TypeError`` code written against earlier releases is not broken. + +:class:`GlpiStatusError`, :class:`GlpiValidationError` and +:class:`GlpiProtocolError` also inherit :class:`ValueError` so code written +against earlier releases — which raised bare ``ValueError`` — keeps working. +""" + +from __future__ import annotations + +from functools import partial +from typing import Any + + +class GlpiError(Exception): + """Base class for every exception raised by ``glpi_python_client``.""" + + +class GlpiTransportError(GlpiError): + """The HTTP request never produced a response. + + Reserved for connection failures, DNS errors, and other network-level + faults where GLPI returned no status code at all -- but **not raised + yet**. The client is still built on ``requests``, and those faults + currently propagate as ``requests`` exceptions (e.g. + ``requests.ConnectionError``) instead. This class exists for the + httpx transport swap planned for a later release, which will raise it + in their place; until then, catch ``requests.RequestException`` + alongside :class:`GlpiError` if you need to handle network faults. + """ + + +class GlpiTimeoutError(GlpiTransportError): + """The HTTP request exceeded its timeout before GLPI responded. + + Like its parent :class:`GlpiTransportError`, this is reserved for the + httpx transport swap and is **not raised yet**; a timeout today + surfaces as ``requests.Timeout``. + """ + + +class GlpiStatusError(GlpiError, ValueError): + """GLPI answered with an unexpected HTTP status code. + + Parameters + ---------- + message : str + Human-readable description of the failure. + status_code : int + The HTTP status code GLPI returned. + url : str + The absolute URL that was requested. + response_text : str, optional + The (possibly truncated) response body, for diagnostics. + + Attributes + ---------- + status_code : int + The HTTP status code GLPI returned. + url : str + The absolute URL that was requested. + response_text : str + The (possibly truncated) response body, for diagnostics. + """ + + status_code: int + url: str + response_text: str + + def __init__( + self, + message: str, + *, + status_code: int, + url: str, + response_text: str = "", + ) -> None: + super().__init__(message) + self.status_code = status_code + self.url = url + self.response_text = response_text + + def __reduce__(self) -> tuple[Any, ...]: + """Support :mod:`pickle` and :mod:`copy` for keyword-only arguments.""" + + return ( + partial( + type(self), + status_code=self.status_code, + url=self.url, + response_text=self.response_text, + ), + (str(self),), + ) + + +class GlpiAuthError(GlpiStatusError): + """GLPI rejected the credentials or the caller lacks rights (401/403).""" + + +class GlpiNotFoundError(GlpiStatusError): + """GLPI has no resource at the requested URL (404).""" + + +class GlpiServerError(GlpiStatusError): + """GLPI failed to serve the request (5xx). Retried by the transport.""" + + +class GlpiValidationError(GlpiError, ValueError): + """The caller supplied an argument or configuration the client rejects.""" + + +class GlpiProtocolError(GlpiError, ValueError): + """GLPI answered successfully with a body the client cannot use. + + Raised when the server returns a success status but the payload is + missing a documented field or has an unusable shape. The caller did + nothing wrong, so this is deliberately distinct from + :class:`GlpiValidationError`. + """ + + +def status_error_class(status_code: int) -> type[GlpiStatusError]: + """Return the most specific status-error class for one status code. + + Parameters + ---------- + status_code : int + The HTTP status code GLPI returned. + + Returns + ------- + type of GlpiStatusError + :class:`GlpiAuthError` for 401/403, :class:`GlpiNotFoundError` for + 404, :class:`GlpiServerError` for 5xx, and :class:`GlpiStatusError` + for every other unexpected status. + """ + + if status_code in (401, 403): + return GlpiAuthError + if status_code == 404: + return GlpiNotFoundError + if 500 <= status_code < 600: + return GlpiServerError + return GlpiStatusError + + +__all__ = [ + "GlpiAuthError", + "GlpiError", + "GlpiNotFoundError", + "GlpiProtocolError", + "GlpiServerError", + "GlpiStatusError", + "GlpiTimeoutError", + "GlpiTransportError", + "GlpiValidationError", + "status_error_class", +] diff --git a/glpi_python_client/auth/_v1_session.py b/glpi_python_client/auth/_v1_session.py index f278648..96c013e 100644 --- a/glpi_python_client/auth/_v1_session.py +++ b/glpi_python_client/auth/_v1_session.py @@ -18,10 +18,18 @@ 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 exclusively by :class:`requests.RequestException` (which -:func:`finalize_request_response` raises for 5xx server errors). -:class:`ValueError` raised by status-code or payload checks does not -trigger a retry — client-side or 4xx failures are surfaced immediately. +triggered by :class:`requests.RequestException` (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 @@ -34,6 +42,11 @@ import requests from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed +from glpi_python_client._errors import ( + GlpiProtocolError, + GlpiServerError, + GlpiValidationError, +) from glpi_python_client.clients.commons._http import ( ensure_response_status, finalize_request_response, @@ -45,9 +58,10 @@ _DEFAULT_SESSION_REFRESH_INTERVAL_SECONDS = 15 * 60 _AUTH_FAILURE_STATUS_CODES = frozenset({401, 403}) _RETRY_ON_NETWORK_ERRORS = retry( - retry=retry_if_exception_type(requests.RequestException), + retry=retry_if_exception_type((requests.RequestException, GlpiServerError)), stop=stop_after_attempt(3), wait=wait_fixed(3), + reraise=True, ) @@ -74,7 +88,7 @@ def __init__( self._user_token = user_token self._app_token = app_token if session_refresh_interval_seconds < 1: - raise ValueError( + raise GlpiValidationError( "session_refresh_interval_seconds must be a positive integer" ) self._session_refresh_interval = timedelta( @@ -122,7 +136,7 @@ def _init_session(self) -> None: token = response.json().get("session_token") if not token: - raise ValueError("GLPI v1 initSession returned no session_token") + raise GlpiProtocolError("GLPI v1 initSession returned no session_token") self._session_token = str(token) self._session_started_at = datetime.now(tz=timezone.utc) @@ -214,8 +228,9 @@ def _authenticated_request( 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:`requests.HTTPError` for the - outer tenacity retry to catch; non-success statuses outside the + 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`. """ @@ -303,9 +318,9 @@ def request_json( 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:`ValueError` raised on a - non-success status. Defaults to ``"GLPI v1 {METHOD} {path} - failed"``. + Prefix used in the :class:`~glpi_python_client.GlpiStatusError` + raised on a non-success status. Defaults to ``"GLPI v1 + {METHOD} {path} failed"``. Returns ------- @@ -315,10 +330,14 @@ def request_json( Raises ------ - ValueError + GlpiStatusError If the v1 server returns a non-success HTTP status outside - the 5xx range (which surfaces as :class:`requests.HTTPError` - and is retried). + 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('/')}" @@ -392,7 +411,7 @@ def upload_document( ) payload = response.json() if not isinstance(payload, dict): - raise ValueError( + raise GlpiProtocolError( "GLPI v1 document upload returned unexpected payload: " f"{type(payload).__name__}" ) diff --git a/glpi_python_client/auth/auth.py b/glpi_python_client/auth/auth.py index 289207d..3e3bf1e 100644 --- a/glpi_python_client/auth/auth.py +++ b/glpi_python_client/auth/auth.py @@ -11,7 +11,13 @@ from datetime import datetime, timedelta, timezone import requests -from tenacity import retry, stop_after_attempt, wait_fixed +from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed + +from glpi_python_client._errors import ( + GlpiServerError, + GlpiValidationError, + status_error_class, +) logger = logging.getLogger(__name__) @@ -109,16 +115,16 @@ def _validate_credentials(self) -> None: has_user_fields = len(missing_user_fields) < 2 if has_client_fields and missing_client_fields: - raise ValueError( + raise GlpiValidationError( "GLPI OAuth client credentials must include both client_id " "and client_secret." ) if has_user_fields and missing_user_fields: - raise ValueError( + raise GlpiValidationError( "GLPI user credentials must include both username and password." ) if not self._has_client_credentials and not self._has_user_credentials: - raise ValueError( + raise GlpiValidationError( "GLPI authentication requires either client_id/client_secret, " "username/password, or both." ) @@ -223,7 +229,12 @@ def _should_refresh_by_interval(self, now: datetime) -> bool: return False return now >= self.token_updated_at + self._auth_token_refresh_interval - @retry(stop=stop_after_attempt(3), wait=wait_fixed(3)) + @retry( + retry=retry_if_exception_type((requests.RequestException, GlpiServerError)), + stop=stop_after_attempt(3), + wait=wait_fixed(3), + reraise=True, + ) def _acquire_token(self) -> None: """Acquire an OAuth2 access token using the configured auth flow. @@ -234,8 +245,13 @@ def _acquire_token(self) -> None: Raises ------ - ValueError - If token acquisition fails. + 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() @@ -247,11 +263,20 @@ def _acquire_token(self) -> None: error_detail = response.json() except Exception: error_detail = response.text - raise ValueError( - f"GLPI OAuth token returned {response.status_code}: {error_detail}" + 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(stop=stop_after_attempt(3), wait=wait_fixed(3)) + @retry( + retry=retry_if_exception_type(requests.RequestException), + stop=stop_after_attempt(3), + wait=wait_fixed(3), + reraise=True, + ) def _refresh_access_token(self) -> None: """Refresh the OAuth2 access token using the stored refresh token. @@ -259,6 +284,31 @@ def _refresh_access_token(self) -> None: ------- 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 + ``requests.RequestException`` (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: @@ -306,5 +356,7 @@ def _refresh_interval(value: int | None) -> timedelta | None: if value is None: return None if value < 1: - raise ValueError("auth_token_refresh must be a positive integer or None") + raise GlpiValidationError( + "auth_token_refresh must be a positive integer or None" + ) return timedelta(seconds=value) diff --git a/glpi_python_client/auth/tests/test_auth.py b/glpi_python_client/auth/tests/test_auth.py index 2b9fa22..99e1fb9 100644 --- a/glpi_python_client/auth/tests/test_auth.py +++ b/glpi_python_client/auth/tests/test_auth.py @@ -5,7 +5,9 @@ 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.testing.utils import FakeResponse, TokenResponse @@ -137,13 +139,18 @@ def test_token_manager_refreshes_when_configured_interval_elapses() -> None: def test_token_manager_rejects_non_positive_refresh_interval( refresh_interval: int, ) -> None: - with pytest.raises(ValueError, match="auth_token_refresh"): + """``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ + + with pytest.raises(GlpiValidationError, match="auth_token_refresh") as excinfo: GLPITokenManager( token_url="https://glpi.example.test/api.php/token", client_id="client-id", client_secret="client-secret", auth_token_refresh=refresh_interval, ) + assert isinstance(excinfo.value, ValueError) def test_token_manager_logout_clears_cached_tokens() -> None: @@ -179,7 +186,11 @@ def test_token_manager_rejects_incomplete_credentials( kwargs: dict[str, str], message: str, ) -> None: - with pytest.raises(ValueError, match=message): + """``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ + + with pytest.raises(GlpiValidationError, match=message) as excinfo: GLPITokenManager( token_url="https://glpi.example.test/api.php/token", client_id=kwargs.get("client_id"), @@ -187,3 +198,230 @@ def test_token_manager_rejects_incomplete_credentials( username=kwargs.get("username"), password=kwargs.get("password"), ) + assert isinstance(excinfo.value, ValueError) + + +def test_oauth_401_raises_glpi_auth_error() -> None: + """A rejected credential surfaces as ``GlpiAuthError``, not a bare ValueError.""" + + session = _FakeSession( + response=TokenResponse(status_code=401, payload={"error": "invalid_client"}) + ) + manager = GLPITokenManager( + token_url="https://glpi.example.test/api.php/token", + client_id="client-id", + client_secret="wrong", + session=cast(requests.Session, session), + ) + with pytest.raises(GlpiAuthError) as excinfo: + manager.ensure_token() + + assert excinfo.value.status_code == 401 + assert isinstance(excinfo.value, ValueError) + + +def test_oauth_401_is_not_retried() -> None: + """A 4xx from the token endpoint is final; retrying cannot help.""" + + session = _FakeSession( + response=TokenResponse(status_code=401, payload={"error": "invalid_client"}) + ) + manager = GLPITokenManager( + token_url="https://glpi.example.test/api.php/token", + client_id="client-id", + client_secret="wrong", + session=cast(requests.Session, session), + ) + with pytest.raises(GlpiAuthError): + manager.ensure_token() + + assert len(session.calls) == 1 + + +def test_oauth_5xx_raises_glpi_server_error_after_retries( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A 5xx from the token endpoint is retried, then reraised as-is.""" + + monkeypatch.setattr(GLPITokenManager._acquire_token.retry, "wait", wait_fixed(0)) + session = _FakeSession(response=TokenResponse(status_code=503, payload={})) + manager = GLPITokenManager( + token_url="https://glpi.example.test/api.php/token", + client_id="client-id", + client_secret="client-secret", + session=cast(requests.Session, session), + ) + with pytest.raises(GlpiServerError) as excinfo: + manager.ensure_token() + + assert excinfo.value.status_code == 503 + assert len(session.calls) == 3 + + +def _make_refresh_ready_manager( + session: _FakeSession, +) -> GLPITokenManager: + """Return a manager primed so ``ensure_token`` reaches ``_refresh_access_token``. + + ``ensure_token`` only calls ``_refresh_access_token`` when an access + token is already set *and* it is expired (or the proactive interval + elapsed). ``_acquire_token`` is never reached this way, unlike a fresh + manager, whose ``ensure_token`` always takes the acquire path. + """ + + manager = GLPITokenManager( + token_url="https://glpi.example.test/api.php/token", + client_id="client-id", + client_secret="client-secret", + session=cast(requests.Session, session), + ) + manager.access_token = "stale-token" + manager.refresh_token = "refresh-token" + manager.token_updated_at = datetime.now(tz=timezone.utc) - timedelta(hours=2) + manager.token_expires_at = datetime.now(tz=timezone.utc) - timedelta(seconds=1) + return manager + + +def test_refresh_401_stays_final_with_one_nested_acquire_call( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A 4xx during refresh is not retried by either decorator. + + ``_refresh_access_token`` does not raise directly on a non-2xx response: + it logs a warning and falls through to a *nested* ``_acquire_token()`` + call (auth.py:302-303), which carries its own, independent retry + decorator. So even a "not retried" 4xx costs two POSTs -- one for the + failed refresh, one for the nested acquire that raises + ``GlpiAuthError`` -- rather than exactly one. Neither decorator's + predicate matches ``GlpiAuthError``, so the count stops there instead + of multiplying further (contrast with the 5xx case below). + """ + + monkeypatch.setattr(GLPITokenManager._acquire_token.retry, "wait", wait_fixed(0)) + monkeypatch.setattr( + GLPITokenManager._refresh_access_token.retry, "wait", wait_fixed(0) + ) + session = _FakeSession( + response=TokenResponse(status_code=401, payload={"error": "invalid_grant"}) + ) + manager = _make_refresh_ready_manager(session) + + with pytest.raises(GlpiAuthError) as excinfo: + manager.ensure_token() + + assert excinfo.value.status_code == 401 + # 1 refresh POST (logged, falls through) + 1 nested _acquire_token POST + # (raises GlpiAuthError immediately; not retried by either decorator). + assert len(session.calls) == 2 + + +def test_refresh_5xx_persistent_costs_one_refresh_plus_nested_acquire_attempts( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A persistent 5xx during refresh costs 4 POSTs, not 12. + + ``_refresh_access_token`` falls through to a *nested* ``_acquire_token()`` + 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 + 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. + + A persistent 5xx therefore costs exactly 1 refresh POST + 3 nested + acquire POSTs = 4 POST calls -- not the 3 (this method's attempts) x 4 + = 12 that resulted from a previous predicate that also matched + ``GlpiServerError`` here, duplicating the nested retries. This test pins + the fixed count so a future change (e.g. plan 3's httpx swap) cannot + silently reintroduce the multiplication. + """ + + monkeypatch.setattr(GLPITokenManager._acquire_token.retry, "wait", wait_fixed(0)) + monkeypatch.setattr( + GLPITokenManager._refresh_access_token.retry, "wait", wait_fixed(0) + ) + session = _FakeSession(response=TokenResponse(status_code=503, payload={})) + manager = _make_refresh_ready_manager(session) + + with pytest.raises(GlpiServerError) as excinfo: + manager.ensure_token() + + assert excinfo.value.status_code == 503 + assert len(session.calls) == 4 + + +def test_acquire_token_network_error_is_retried_three_times( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A connection-level failure (no HTTP response at all) is retried. + + Mirrors ``test_network_errors_are_still_retried`` in + ``clients/commons/tests/test_retry_semantics.py`` for the transport, but + covers the OAuth token path, which previously had no equivalent test. + """ + + monkeypatch.setattr(GLPITokenManager._acquire_token.retry, "wait", wait_fixed(0)) + + class _FailingSession: + def __init__(self) -> None: + self.calls: list[dict[str, object]] = [] + + 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") + + 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), + ) + + with pytest.raises(requests.ConnectionError): + manager.ensure_token() + + assert len(session.calls) == 3 + + +def test_refresh_network_error_is_retried_three_times( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A connection-level failure during refresh is retried by refresh's own decorator. + + Pins the one predicate member not yet covered by another test: + ``_refresh_access_token``'s network-error retry. The fall-through + ``GlpiServerError`` case is pinned by + ``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 + *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 + refresh's network retry count at 3, not 12. If a future rewrite of the + retry predicate (e.g. the httpx transport swap) drops this to 1 + without remapping the equivalent network exception, this test catches + it with every other test still green. + """ + + monkeypatch.setattr( + GLPITokenManager._refresh_access_token.retry, "wait", wait_fixed(0) + ) + + class _FailingSession: + def __init__(self) -> None: + self.calls: list[dict[str, object]] = [] + + 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") + + session = _FailingSession() + manager = _make_refresh_ready_manager(cast(_FakeSession, session)) + + with pytest.raises(requests.ConnectionError): + manager.ensure_token() + + assert len(session.calls) == 3 diff --git a/glpi_python_client/auth/tests/test_v1_session.py b/glpi_python_client/auth/tests/test_v1_session.py index db1e0a0..2cad8bc 100644 --- a/glpi_python_client/auth/tests/test_v1_session.py +++ b/glpi_python_client/auth/tests/test_v1_session.py @@ -7,8 +7,8 @@ import pytest import requests -import tenacity +from glpi_python_client import GlpiProtocolError, GlpiServerError, GlpiValidationError from glpi_python_client.auth._v1_session import GLPIV1Session from glpi_python_client.testing.utils import FakeResponse @@ -130,15 +130,20 @@ def _make(http: _FakeV1Http) -> GLPIV1Session: def test_v1_session_rejects_bad_refresh_interval() -> None: - """Constructor enforces a positive refresh interval.""" + """Constructor enforces a positive refresh interval. - with pytest.raises(ValueError): + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ + + with pytest.raises(GlpiValidationError) as excinfo: GLPIV1Session( base_url="https://glpi.example.test/apirest.php", user_token="u", app_token="a", session_refresh_interval_seconds=0, ) + assert isinstance(excinfo.value, ValueError) def test_v1_upload_acquires_session_then_posts() -> None: @@ -214,7 +219,7 @@ def test_v1_upload_renews_session_on_401() -> None: def test_v1_upload_raises_on_5xx_after_retries() -> None: - """5xx upload responses surface as ``HTTPError`` after retries exhaust.""" + """5xx upload responses are retried 3x and surface as ``GlpiServerError``.""" http = _FakeV1Http( responses={ @@ -224,10 +229,14 @@ def test_v1_upload_raises_on_5xx_after_retries() -> None: } ) session = _make(http) - with pytest.raises(tenacity.RetryError) as excinfo: + with pytest.raises(GlpiServerError) as excinfo: session.upload_document("a.txt", b"x", "text/plain") - inner = excinfo.value.last_attempt.exception() - assert isinstance(inner, requests.HTTPError) + assert excinfo.value.status_code == 500 + # The retry predicate must retry GlpiServerError, not just RequestException: + # pin the attempt count so a predicate regression fails loudly instead of + # silently dropping to 1 attempt. + upload_calls = [c for c in http.calls if c["url"].endswith("/Document")] + assert len(upload_calls) == 3 def test_v1_upload_raises_on_4xx_without_retry() -> None: @@ -249,7 +258,11 @@ def test_v1_upload_raises_on_4xx_without_retry() -> None: def test_v1_upload_raises_on_unexpected_payload() -> None: - """A non-mapping JSON payload raises ``ValueError`` without retry.""" + """A non-mapping JSON payload raises ``GlpiProtocolError`` without retry. + + ``GlpiProtocolError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ http = _FakeV1Http( responses={ @@ -259,12 +272,13 @@ def test_v1_upload_raises_on_unexpected_payload() -> None: } ) session = _make(http) - with pytest.raises(ValueError, match="unexpected payload"): + with pytest.raises(GlpiProtocolError, match="unexpected payload") as excinfo: session.upload_document("a.txt", b"x", "text/plain") + assert isinstance(excinfo.value, ValueError) def test_v1_init_raises_on_5xx_after_retries() -> None: - """5xx ``initSession`` responses surface as ``HTTPError`` after retries.""" + """5xx ``initSession`` responses are retried 3x and raise ``GlpiServerError``.""" http = _FakeV1Http( responses={ @@ -272,10 +286,11 @@ def test_v1_init_raises_on_5xx_after_retries() -> None: } ) session = _make(http) - with pytest.raises(tenacity.RetryError) as excinfo: + with pytest.raises(GlpiServerError) as excinfo: session._init_session() - inner = excinfo.value.last_attempt.exception() - assert isinstance(inner, requests.HTTPError) + assert excinfo.value.status_code == 500 + init_calls = [c for c in http.calls if c["url"].endswith("/initSession")] + assert len(init_calls) == 3 def test_v1_init_raises_on_4xx_without_retry() -> None: @@ -292,14 +307,19 @@ def test_v1_init_raises_on_4xx_without_retry() -> None: def test_v1_init_raises_when_token_missing() -> None: - """``initSession`` returning no token raises ``ValueError`` without retry.""" + """``initSession`` returning no token raises ``GlpiProtocolError`` without retry. + + ``GlpiProtocolError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ http = _FakeV1Http( responses={"init": [FakeResponse(status_code=200, payload={})]}, ) session = _make(http) - with pytest.raises(ValueError, match="no session_token"): + with pytest.raises(GlpiProtocolError, match="no session_token") as excinfo: session._init_session() + assert isinstance(excinfo.value, ValueError) def test_v1_close_kills_session_and_closes_http() -> None: @@ -408,7 +428,7 @@ def test_request_json_raises_on_4xx_without_retry() -> None: def test_request_json_retries_on_5xx() -> None: - """5xx responses surface as ``HTTPError`` after retries exhaust.""" + """5xx responses are retried 3x and surface as ``GlpiServerError``.""" http = _FakeV1Http( responses={ @@ -417,10 +437,56 @@ def test_request_json_retries_on_5xx() -> None: } ) session = _make(http) - with pytest.raises(tenacity.RetryError) as excinfo: + with pytest.raises(GlpiServerError) as excinfo: + session.request_json("GET", "PluginFieldsContainer") + assert excinfo.value.status_code == 500 + json_calls = [c for c in http.calls if c["url"].endswith("/PluginFieldsContainer")] + assert len(json_calls) == 3 + + +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 + (``_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 + drop v1 network retries from 3 attempts to 1 while every committed test + stayed green. + """ + + class _FlakyHttp(_FakeV1Http): + def get( + self, + url: str, + headers: dict[str, str], + timeout: int, + **kwargs: Any, + ) -> FakeResponse: + if url.endswith("/PluginFieldsContainer"): + self.calls.append( + { + "method": "GET", + "url": url, + "headers": headers, + "timeout": timeout, + **kwargs, + } + ) + raise requests.ConnectionError("network down") + return super().get(url, headers, timeout, **kwargs) + + http = _FlakyHttp( + responses={ + "init": [FakeResponse(status_code=200, payload={"session_token": "tk"})], + } + ) + session = _make(http) + with pytest.raises(requests.ConnectionError): session.request_json("GET", "PluginFieldsContainer") - inner = excinfo.value.last_attempt.exception() - assert isinstance(inner, requests.HTTPError) + json_calls = [c for c in http.calls if c["url"].endswith("/PluginFieldsContainer")] + assert len(json_calls) == 3 def test_session_token_invalid_marker_triggers_renew() -> None: diff --git a/glpi_python_client/clients/_base_client.py b/glpi_python_client/clients/_base_client.py index 41e5f6c..5febafb 100644 --- a/glpi_python_client/clients/_base_client.py +++ b/glpi_python_client/clients/_base_client.py @@ -98,7 +98,7 @@ def __init__( Raises ------ - ValueError + GlpiValidationError If the supplied configuration is incomplete or invalid (e.g. missing OAuth credentials together with no v1 fallback). """ @@ -163,7 +163,7 @@ def from_env( Raises ------ - ValueError + GlpiValidationError If the resolved configuration is missing a required field. """ diff --git a/glpi_python_client/clients/api/__init__.py b/glpi_python_client/clients/api/__init__.py index 8c43571..4f7237b 100644 --- a/glpi_python_client/clients/api/__init__.py +++ b/glpi_python_client/clients/api/__init__.py @@ -25,15 +25,21 @@ ) from glpi_python_client.clients.api.dropdowns import LocationMixin from glpi_python_client.clients.api.knowledgebase import ( + AsyncKBArticleMixin, KBArticleCommentMixin, KBArticleMixin, KBArticleRevisionMixin, KBCategoryMixin, ) from glpi_python_client.clients.api.management import DocumentMixin -from glpi_python_client.clients.api.plugins import PluginFieldsMixin +from glpi_python_client.clients.api.plugins import ( + AsyncPluginFieldsMixin, + PluginFieldsMixin, +) __all__ = [ + "AsyncKBArticleMixin", + "AsyncPluginFieldsMixin", "DocumentMixin", "EntityMixin", "FollowupMixin", diff --git a/glpi_python_client/clients/api/administration/_entity.py b/glpi_python_client/clients/api/administration/_entity.py index 8c9d5b7..b0dad3b 100644 --- a/glpi_python_client/clients/api/administration/_entity.py +++ b/glpi_python_client/clients/api/administration/_entity.py @@ -113,7 +113,7 @@ def get_entity(self, entity_id: GlpiId) -> GetEntity: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -139,9 +139,10 @@ def create_entity(self, entity: PostEntity) -> int: Raises ------ - ValueError - If the create response is missing ``id`` or returns a - non-success HTTP status. + GlpiStatusError + If the GLPI server returns a non-success HTTP status. + GlpiProtocolError + If the create response is missing the ``id`` field. """ return self._resource_create( @@ -169,7 +170,7 @@ def update_entity(self, entity_id: GlpiId, entity: PatchEntity) -> None: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -197,7 +198,7 @@ def delete_entity(self, entity_id: GlpiId, *, force: bool | None = None) -> None Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/api/administration/_user.py b/glpi_python_client/clients/api/administration/_user.py index c4279b7..c81cf98 100644 --- a/glpi_python_client/clients/api/administration/_user.py +++ b/glpi_python_client/clients/api/administration/_user.py @@ -125,7 +125,7 @@ def get_user(self, user_id: GlpiId) -> GetUser: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -150,9 +150,10 @@ def create_user(self, user: PostUser) -> int: Raises ------ - ValueError - If the create response is missing ``id`` or returns a - non-success HTTP status. + GlpiStatusError + If the GLPI server returns a non-success HTTP status. + GlpiProtocolError + If the create response is missing the ``id`` field. """ return self._resource_create( @@ -179,7 +180,7 @@ def update_user(self, user_id: GlpiId, user: PatchUser) -> None: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -207,7 +208,7 @@ def delete_user(self, user_id: GlpiId, *, force: bool | None = None) -> None: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/api/assistance/_team.py b/glpi_python_client/clients/api/assistance/_team.py index 738c2b6..059e20c 100644 --- a/glpi_python_client/clients/api/assistance/_team.py +++ b/glpi_python_client/clients/api/assistance/_team.py @@ -43,7 +43,7 @@ def list_ticket_team_members(self, ticket_id: GlpiId) -> list[GetTeamMember]: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -72,7 +72,7 @@ def add_ticket_team_member(self, ticket_id: GlpiId, member: PostTeamMember) -> N Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -104,7 +104,7 @@ def remove_ticket_team_member( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/api/assistance/_ticket.py b/glpi_python_client/clients/api/assistance/_ticket.py index fc3f08f..575fca7 100644 --- a/glpi_python_client/clients/api/assistance/_ticket.py +++ b/glpi_python_client/clients/api/assistance/_ticket.py @@ -135,7 +135,7 @@ def get_ticket(self, ticket_id: GlpiId) -> GetTicket: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -161,9 +161,10 @@ def create_ticket(self, ticket: PostTicket) -> int: Raises ------ - ValueError - If the create response is missing the ``id`` field or the - HTTP status is not success. + GlpiStatusError + If the HTTP status is not success. + GlpiProtocolError + If the create response is missing the ``id`` field. """ return self._resource_create( @@ -190,7 +191,7 @@ def update_ticket(self, ticket_id: GlpiId, ticket: PatchTicket) -> None: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -219,7 +220,7 @@ def delete_ticket(self, ticket_id: GlpiId, *, force: bool | None = None) -> None Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/api/assistance/timeline/_document.py b/glpi_python_client/clients/api/assistance/timeline/_document.py index 86cf583..fe98f9b 100644 --- a/glpi_python_client/clients/api/assistance/timeline/_document.py +++ b/glpi_python_client/clients/api/assistance/timeline/_document.py @@ -79,7 +79,7 @@ def get_ticket_timeline_document( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -114,9 +114,10 @@ def link_ticket_timeline_document( Raises ------ - ValueError - If the create response is missing ``id`` or returns a - non-success HTTP status. + GlpiStatusError + If the GLPI server returns a non-success HTTP status. + GlpiProtocolError + If the create response is missing the ``id`` field. """ return self._resource_create( @@ -156,7 +157,7 @@ def update_ticket_timeline_document( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -199,7 +200,7 @@ def unlink_ticket_timeline_document( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/api/assistance/timeline/_followup.py b/glpi_python_client/clients/api/assistance/timeline/_followup.py index 9e7294d..8ad722c 100644 --- a/glpi_python_client/clients/api/assistance/timeline/_followup.py +++ b/glpi_python_client/clients/api/assistance/timeline/_followup.py @@ -75,7 +75,7 @@ def get_ticket_followup( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -104,9 +104,10 @@ def create_ticket_followup(self, ticket_id: GlpiId, followup: PostFollowup) -> i Raises ------ - ValueError - If the create response is missing ``id`` or returns a - non-success HTTP status. + GlpiStatusError + If the GLPI server returns a non-success HTTP status. + GlpiProtocolError + If the create response is missing the ``id`` field. """ return self._resource_create( @@ -145,7 +146,7 @@ def update_ticket_followup( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -183,7 +184,7 @@ def delete_ticket_followup( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/api/assistance/timeline/_solution.py b/glpi_python_client/clients/api/assistance/timeline/_solution.py index d37ca9d..b2dd766 100644 --- a/glpi_python_client/clients/api/assistance/timeline/_solution.py +++ b/glpi_python_client/clients/api/assistance/timeline/_solution.py @@ -74,7 +74,7 @@ def get_ticket_solution( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -103,9 +103,10 @@ def create_ticket_solution(self, ticket_id: GlpiId, solution: PostSolution) -> i Raises ------ - ValueError - If the create response is missing ``id`` or returns a - non-success HTTP status. + GlpiStatusError + If the GLPI server returns a non-success HTTP status. + GlpiProtocolError + If the create response is missing the ``id`` field. """ return self._resource_create( @@ -142,7 +143,7 @@ def update_ticket_solution( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -180,7 +181,7 @@ def delete_ticket_solution( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/api/assistance/timeline/_task.py b/glpi_python_client/clients/api/assistance/timeline/_task.py index 5d3f72e..004bd90 100644 --- a/glpi_python_client/clients/api/assistance/timeline/_task.py +++ b/glpi_python_client/clients/api/assistance/timeline/_task.py @@ -73,7 +73,7 @@ def get_ticket_task(self, ticket_id: GlpiId, task_id: GlpiId) -> GetTicketTask: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -100,9 +100,10 @@ def create_ticket_task(self, ticket_id: GlpiId, task: PostTicketTask) -> int: Raises ------ - ValueError - If the create response is missing ``id`` or returns a - non-success HTTP status. + GlpiStatusError + If the GLPI server returns a non-success HTTP status. + GlpiProtocolError + If the create response is missing the ``id`` field. """ return self._resource_create( @@ -139,7 +140,7 @@ def update_ticket_task( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -175,7 +176,7 @@ def delete_ticket_task( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/api/dropdowns/_location.py b/glpi_python_client/clients/api/dropdowns/_location.py index 1dac3b9..b4caa57 100644 --- a/glpi_python_client/clients/api/dropdowns/_location.py +++ b/glpi_python_client/clients/api/dropdowns/_location.py @@ -64,7 +64,7 @@ def get_location(self, location_id: GlpiId) -> GetLocation: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -89,9 +89,10 @@ def create_location(self, location: PostLocation) -> int: Raises ------ - ValueError - If the create response is missing ``id`` or returns a - non-success HTTP status. + GlpiStatusError + If the GLPI server returns a non-success HTTP status. + GlpiProtocolError + If the create response is missing the ``id`` field. """ return self._resource_create( @@ -118,7 +119,7 @@ def update_location(self, location_id: GlpiId, location: PatchLocation) -> None: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -148,7 +149,7 @@ def delete_location( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/api/knowledgebase/__init__.py b/glpi_python_client/clients/api/knowledgebase/__init__.py index 2bed9e8..89ee53a 100644 --- a/glpi_python_client/clients/api/knowledgebase/__init__.py +++ b/glpi_python_client/clients/api/knowledgebase/__init__.py @@ -1,8 +1,11 @@ -"""GLPI ``/Knowledgebase`` mixins for the Synchronous client.""" +"""GLPI ``/Knowledgebase`` mixins for the synchronous and asynchronous clients.""" from __future__ import annotations from glpi_python_client.clients.api.knowledgebase._article import KBArticleMixin +from glpi_python_client.clients.api.knowledgebase._article_async import ( + AsyncKBArticleMixin, +) from glpi_python_client.clients.api.knowledgebase._category import KBCategoryMixin from glpi_python_client.clients.api.knowledgebase._comment import ( KBArticleCommentMixin, @@ -12,6 +15,7 @@ ) __all__ = [ + "AsyncKBArticleMixin", "KBArticleCommentMixin", "KBArticleMixin", "KBArticleRevisionMixin", diff --git a/glpi_python_client/clients/api/knowledgebase/_article.py b/glpi_python_client/clients/api/knowledgebase/_article.py index 13d5091..17b0ae9 100644 --- a/glpi_python_client/clients/api/knowledgebase/_article.py +++ b/glpi_python_client/clients/api/knowledgebase/_article.py @@ -10,6 +10,7 @@ from collections.abc import Sequence +from glpi_python_client._errors import GlpiValidationError from glpi_python_client.clients.commons._constants import ( KB_ARTICLE_ENDPOINT, GlpiId, @@ -74,7 +75,7 @@ def get_kb_article(self, article_id: GlpiId) -> GetKBArticle: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -170,7 +171,7 @@ def set_kb_article_categories( ------ RuntimeError When no legacy v1 session is configured on the client. - ValueError + GlpiStatusError When the legacy API returns a non-success status. """ @@ -189,8 +190,8 @@ def _apply_category_fallback( No-op when ``categories`` is ``None``. An empty list clears every category (used by update); ``create_kb_article`` skips the empty case - before calling this helper. Raises ``ValueError`` when a category - reference lacks an ``id``. + before calling this helper. Raises ``GlpiValidationError`` when a + category reference lacks an ``id``. """ if categories is None: @@ -198,7 +199,7 @@ def _apply_category_fallback( ids: list[int] = [] for ref in categories: if ref.id is None: - raise ValueError( + raise GlpiValidationError( "KB article categories require an 'id' to be linked; got a " "category reference without an id." ) diff --git a/glpi_python_client/clients/api/knowledgebase/_article_async.py b/glpi_python_client/clients/api/knowledgebase/_article_async.py new file mode 100644 index 0000000..74dd642 --- /dev/null +++ b/glpi_python_client/clients/api/knowledgebase/_article_async.py @@ -0,0 +1,152 @@ +"""Asynchronous overrides for KB article category assignment. + +The v2 API exposes ``KBArticle.categories[].id`` as ``readOnly``, so +:meth:`create_kb_article` and :meth:`update_kb_article` apply categories +through the legacy v1 ``_categories`` fallback. That fallback calls the +public :meth:`set_kb_article_categories` through ``self``, which the async +bridge has wrapped into a coroutine — so the synchronous bodies drop the +call and the article silently keeps no categories. + +These overrides strip ``categories`` from the model, run the untouched +synchronous v2 write in a worker thread (its own fallback then no-ops), +and apply the categories with an awaited call. Keeping the sync module +untouched makes the fix purely additive. + +The mixin must sit **before** :class:`KBArticleMixin` in the +:class:`~glpi_python_client.clients.AsyncGlpiClient` base list. +""" + +from __future__ import annotations + +import asyncio + +from glpi_python_client._errors import GlpiValidationError +from glpi_python_client.clients.api.knowledgebase._article import KBArticleMixin +from glpi_python_client.clients.commons._constants import GlpiId +from glpi_python_client.models.api_schema._common import IdNameRef +from glpi_python_client.models.api_schema.knowledgebase._article import ( + PatchKBArticle, + PostKBArticle, +) + + +class AsyncKBArticleMixin(KBArticleMixin): + """Async overrides for the two KB article writes that set categories.""" + + async def _apply_category_fallback_async( + self, article_id: GlpiId, categories: list[IdNameRef] | None + ) -> None: + """Apply ``categories`` through the awaited legacy fallback. + + Mirrors :meth:`KBArticleMixin._apply_category_fallback` but awaits + the bridge-wrapped :meth:`set_kb_article_categories`. + + Parameters + ---------- + article_id : GlpiId + Identifier of the article to re-categorise. + categories : list[IdNameRef] | None + Category references to link. ``None`` is a no-op; an empty + list clears every category. + + Returns + ------- + None + + Raises + ------ + GlpiValidationError + When a category reference lacks an ``id``. + """ + + if categories is None: + return + ids: list[int] = [] + for ref in categories: + if ref.id is None: + raise GlpiValidationError( + "KB article categories require an 'id' to be linked; got a " + "category reference without an id." + ) + ids.append(ref.id) + # ``set_kb_article_categories`` is declared on ``KBArticleMixin``, the + # very class this mixin subclasses, so mypy resolves it statically as + # the synchronous ``-> None`` method rather than the bridge-generated + # coroutine it becomes at runtime on ``AsyncGlpiClient``. That mismatch + # is exactly what makes the await necessary here. + await self.set_kb_article_categories( # type: ignore[misc, func-returns-value] + article_id, ids + ) + + async def create_kb_article( # type: ignore[override] + self, article: PostKBArticle + ) -> int: + """Create one knowledge base article and return its new identifier. + + Async override of :meth:`KBArticleMixin.create_kb_article`. The v2 + create runs in a worker thread with ``categories`` stripped, then + the categories are applied through the awaited legacy fallback. + Error semantics match the synchronous version: the create is not + undone when the category assignment fails. + + Parameters + ---------- + article : PostKBArticle + Body of the article to create. + + Returns + ------- + int + Identifier assigned by GLPI. + + Raises + ------ + RuntimeError + When the article was created but assigning its categories + failed. The message names the new article id. + """ + + stripped = article.model_copy(update={"categories": None}) + new_id: int = await asyncio.to_thread( + KBArticleMixin.create_kb_article, self, stripped + ) + if article.categories: + try: + await self._apply_category_fallback_async(new_id, article.categories) + except Exception as exc: + raise RuntimeError( + f"KB article {new_id} was created but assigning its " + f"categories failed: {exc}" + ) from exc + return new_id + + async def update_kb_article( # type: ignore[override] + self, article_id: GlpiId, article: PatchKBArticle + ) -> None: + """Update one knowledge base article with a partial body. + + Async override of :meth:`KBArticleMixin.update_kb_article`. The v2 + patch runs in a worker thread with ``categories`` stripped, then + the categories are applied through the awaited legacy fallback. + ``None`` leaves categories untouched; an empty list clears them. + + Parameters + ---------- + article_id : GlpiId + Identifier of the article to update. + article : PatchKBArticle + Partial body to apply. + + Returns + ------- + None + """ + + stripped = article.model_copy(update={"categories": None}) + await asyncio.to_thread( + KBArticleMixin.update_kb_article, self, article_id, stripped + ) + await self._apply_category_fallback_async(article_id, article.categories) + + +__all__ = ["AsyncKBArticleMixin"] diff --git a/glpi_python_client/clients/api/knowledgebase/_category.py b/glpi_python_client/clients/api/knowledgebase/_category.py index 1f4557c..5479f33 100644 --- a/glpi_python_client/clients/api/knowledgebase/_category.py +++ b/glpi_python_client/clients/api/knowledgebase/_category.py @@ -68,7 +68,7 @@ def get_kb_category(self, category_id: GlpiId) -> GetKBCategory: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py b/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py new file mode 100644 index 0000000..86d7f24 --- /dev/null +++ b/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py @@ -0,0 +1,236 @@ +"""Async-client tests for KB article category assignment. + +The v2 API cannot write KB categories, so create/update apply them through +the legacy v1 ``_categories`` fallback. That fallback runs through the +public ``set_kb_article_categories``, which the async bridge wraps into a +coroutine — so without an override the category write is silently dropped +and the article is created with no category at all. +""" + +from __future__ import annotations + +from typing import Any + +import pytest + +from glpi_python_client import ( + AsyncGlpiClient, + GlpiValidationError, + PatchKBArticle, + PostKBArticle, +) +from glpi_python_client.models.api_schema._common import IdNameRef +from glpi_python_client.testing.utils import FakeResponse, make_async_client + + +class _FakeV1: + """Stand-in for ``GLPIV1Session`` recording ``request_json`` calls.""" + + def __init__(self) -> None: + self.calls: list[dict[str, Any]] = [] + + 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: + self.calls.append({"method": method, "path": path, "json_body": json_body}) + return [{"1": True, "message": ""}] + + +@pytest.fixture +def client() -> AsyncGlpiClient: + """Return an async client with the v2 transport stubbed out. + + Both stubs record every ``json_body`` they receive (on the ``post_bodies`` + / ``patch_bodies`` attributes attached to the client) so tests can assert + that stripping ``categories`` for the legacy fallback left the rest of + the v2 request body untouched. + """ + + c = make_async_client() + post_bodies: list[dict[str, object] | None] = [] + patch_bodies: list[dict[str, object] | None] = [] + + def _post( + endpoint: str, + json_body: dict[str, object] | None = None, + skip_entity: bool = False, + ) -> FakeResponse: + post_bodies.append(json_body) + return FakeResponse(status_code=201, payload={"id": 42}) + + def _patch( + endpoint: str, json_body: dict[str, object] | None = None + ) -> FakeResponse: + patch_bodies.append(json_body) + return FakeResponse(status_code=200, payload={"id": 42}) + + c._post_request = _post # type: ignore[assignment] + c._update_request = _patch # type: ignore[assignment] + c.post_bodies = post_bodies # type: ignore[attr-defined] + c.patch_bodies = patch_bodies # type: ignore[attr-defined] + return c + + +async def test_create_kb_article_links_categories(client: AsyncGlpiClient) -> None: + """Creating with categories must actually issue the v1 category write.""" + + fake = _FakeV1() + client._v1 = fake # type: ignore[assignment] + + new_id = await client.create_kb_article( + PostKBArticle(name="t", answer="a", categories=[IdNameRef(id=7, name="cat")]) + ) + + assert new_id == 42 + assert fake.calls == [ + { + "method": "PUT", + "path": "KnowbaseItem/42", + "json_body": {"input": {"_categories": [7]}}, + } + ] + # The stripped v2 body must still carry every other field: only + # ``categories`` was removed before the worker-thread create call runs. + # A ``model_copy(update=...)`` that nuked more than ``categories`` would + # otherwise pass every other assertion in this file undetected. + assert client.post_bodies == [{"name": "t", "answer": "a"}] # type: ignore[attr-defined] + + +async def test_create_kb_article_without_categories_needs_no_v1( + client: AsyncGlpiClient, +) -> None: + """Omitting categories must not require a v1 session.""" + + client._v1 = None + assert await client.create_kb_article(PostKBArticle(name="t", answer="a")) == 42 + + +async def test_create_kb_article_wraps_category_failure( + client: AsyncGlpiClient, +) -> None: + """A category failure after create raises RuntimeError naming the id.""" + + client._v1 = None # no v1 session -> the fallback raises RuntimeError + + with pytest.raises(RuntimeError, match="KB article 42 was created but"): + await client.create_kb_article( + PostKBArticle( + name="t", answer="a", categories=[IdNameRef(id=7, name="cat")] + ) + ) + + +async def test_create_kb_article_wraps_missing_id_category( + client: AsyncGlpiClient, +) -> None: + """A category ref without an id is wrapped in the same ``RuntimeError``. + + ``_apply_category_fallback_async`` raises ``ValueError`` before ever + touching a v1 session when a category reference has no ``id`` (see + ``_article_async.py``). ``create_kb_article`` wraps every fallback + failure — including this one — into ``RuntimeError``. This is the + async copy of a branch already covered on the sync client; the two + copies can drift independently, so this branch needs its own test + rather than relying on sync coverage. + """ + + with pytest.raises(RuntimeError, match="KB article 42 was created but"): + await client.create_kb_article( + PostKBArticle(name="t", answer="a", categories=[IdNameRef(name="cat")]) + ) + + +async def test_update_kb_article_links_categories(client: AsyncGlpiClient) -> None: + """Updating with a non-empty list must issue the v1 category write. + + This is the update-path counterpart of + ``test_create_kb_article_links_categories``. Without it, the + non-empty-list case on ``update_kb_article`` is only covered by + composition: the ``[]`` test below proves the update path fires the + legacy fallback at all, and the create test proves the id-collection + loop works, but neither proves a non-empty list on *update*, the + headline regression this branch fixed, actually reaches the v1 + ``PUT``. + """ + + fake = _FakeV1() + client._v1 = fake # type: ignore[assignment] + + await client.update_kb_article( + 42, PatchKBArticle(name="t3", categories=[IdNameRef(id=7, name="cat")]) + ) + + assert fake.calls == [ + { + "method": "PUT", + "path": "KnowbaseItem/42", + "json_body": {"input": {"_categories": [7]}}, + } + ] + # The stripped v2 patch body must still carry every other field: only + # ``categories`` was removed before the worker-thread update call runs. + assert client.patch_bodies == [{"name": "t3"}] # type: ignore[attr-defined] + + +async def test_update_kb_article_clears_categories(client: AsyncGlpiClient) -> None: + """An empty list clears every category through the v1 fallback.""" + + fake = _FakeV1() + client._v1 = fake # type: ignore[assignment] + + await client.update_kb_article(42, PatchKBArticle(name="t2", categories=[])) + + assert fake.calls == [ + { + "method": "PUT", + "path": "KnowbaseItem/42", + "json_body": {"input": {"_categories": []}}, + } + ] + # The stripped v2 patch body must still carry every other field: only + # ``categories`` was removed before the worker-thread update call runs. + # A ``model_copy(update=...)`` that nuked more than ``categories`` would + # otherwise pass every other assertion in this file undetected. + assert client.patch_bodies == [{"name": "t2"}] # type: ignore[attr-defined] + + +async def test_update_kb_article_raises_on_missing_id_category( + client: AsyncGlpiClient, +) -> None: + """A category ref without an id raises the raw ``GlpiValidationError`` on update. + + Unlike ``create_kb_article``, ``update_kb_article`` does not wrap the + fallback call in a ``try``/``except``: the v2 patch has already been + applied by the time categories are assigned, so there is nothing to + roll back and no article-was-created message to build around. The raw + ``GlpiValidationError`` from ``_apply_category_fallback_async`` must + therefore propagate unchanged. ``GlpiValidationError`` inherits + ``ValueError`` so existing callers that catch the broader type keep + working. + """ + + with pytest.raises(GlpiValidationError, match="require an 'id'") as excinfo: + await client.update_kb_article( + 42, PatchKBArticle(name="t2", categories=[IdNameRef(name="cat")]) + ) + assert isinstance(excinfo.value, ValueError) + + +async def test_update_kb_article_without_categories_skips_v1( + client: AsyncGlpiClient, +) -> None: + """``categories=None`` leaves categories untouched and calls no v1.""" + + fake = _FakeV1() + client._v1 = fake # type: ignore[assignment] + + await client.update_kb_article(42, PatchKBArticle(name="t2")) + + assert fake.calls == [] diff --git a/glpi_python_client/clients/api/knowledgebase/tests/test_article_mixin.py b/glpi_python_client/clients/api/knowledgebase/tests/test_article_mixin.py index 8593c90..c902acc 100644 --- a/glpi_python_client/clients/api/knowledgebase/tests/test_article_mixin.py +++ b/glpi_python_client/clients/api/knowledgebase/tests/test_article_mixin.py @@ -6,7 +6,13 @@ import pytest -from glpi_python_client import GlpiClient, IdNameRef, PatchKBArticle, PostKBArticle +from glpi_python_client import ( + GlpiClient, + GlpiValidationError, + IdNameRef, + PatchKBArticle, + PostKBArticle, +) from glpi_python_client.testing.utils import FakeResponse, make_client @@ -223,14 +229,20 @@ def test_create_kb_article_category_failure_raises_without_rollback( def test_create_kb_article_ref_without_id_raises(client: GlpiClient) -> None: + """``create_kb_article`` wraps the failure in ``RuntimeError`` (kept bare + by design), chaining the underlying ``GlpiValidationError`` as its cause. + """ + rec = _Recorder() rec.install(client) client._v1 = _FakeV1() # type: ignore[assignment] - with pytest.raises(RuntimeError, match="require an 'id'"): + with pytest.raises(RuntimeError, match="require an 'id'") as excinfo: client.create_kb_article( PostKBArticle(name="P", content="c", categories=[IdNameRef(name="Parrots")]) ) assert not any(c["method"] == "DELETE" for c in rec.calls) + assert isinstance(excinfo.value.__cause__, GlpiValidationError) + assert isinstance(excinfo.value.__cause__, ValueError) def test_create_kb_article_empty_categories_skips_v1(client: GlpiClient) -> None: diff --git a/glpi_python_client/clients/api/management/_document.py b/glpi_python_client/clients/api/management/_document.py index fdd02f1..b9cee40 100644 --- a/glpi_python_client/clients/api/management/_document.py +++ b/glpi_python_client/clients/api/management/_document.py @@ -9,6 +9,7 @@ import logging +from glpi_python_client._errors import GlpiValidationError from glpi_python_client.clients.commons._constants import ( DOCUMENT_ENDPOINT, GlpiId, @@ -77,7 +78,7 @@ def get_document(self, document_id: GlpiId) -> GetDocument: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -106,9 +107,10 @@ def create_document(self, document: PostDocument) -> int: Raises ------ - ValueError - If the create response is missing ``id`` or returns a - non-success HTTP status. + GlpiStatusError + If the GLPI server returns a non-success HTTP status. + GlpiProtocolError + If the create response is missing the ``id`` field. """ return self._resource_create( @@ -136,7 +138,7 @@ def update_document(self, document_id: GlpiId, document: PatchDocument) -> None: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -166,7 +168,7 @@ def delete_document( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -195,7 +197,7 @@ def download_document_content(self, document_id: GlpiId) -> bytes: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -255,14 +257,14 @@ def upload_document( Raises ------ - ValueError + GlpiValidationError If ``filename`` is empty. RuntimeError If the v1 session is not configured on the client. """ if not filename: - raise ValueError("GLPI document upload requires a filename") + raise GlpiValidationError("GLPI document upload requires a filename") v1 = self._require_v1_session("document uploads") return v1.upload_document( filename, diff --git a/glpi_python_client/clients/api/plugins/__init__.py b/glpi_python_client/clients/api/plugins/__init__.py index aa74d24..f064e45 100644 --- a/glpi_python_client/clients/api/plugins/__init__.py +++ b/glpi_python_client/clients/api/plugins/__init__.py @@ -6,5 +6,6 @@ """ from glpi_python_client.clients.api.plugins._fields import PluginFieldsMixin +from glpi_python_client.clients.api.plugins._fields_async import AsyncPluginFieldsMixin -__all__ = ["PluginFieldsMixin"] +__all__ = ["AsyncPluginFieldsMixin", "PluginFieldsMixin"] diff --git a/glpi_python_client/clients/api/plugins/_fields.py b/glpi_python_client/clients/api/plugins/_fields.py index e29dccc..6993725 100644 --- a/glpi_python_client/clients/api/plugins/_fields.py +++ b/glpi_python_client/clients/api/plugins/_fields.py @@ -33,6 +33,7 @@ import json from typing import Any +from glpi_python_client._errors import GlpiProtocolError, GlpiValidationError from glpi_python_client.clients.commons._transport import TransportMixin from glpi_python_client.models.api_schema.plugins import ( GetPluginFieldsContainer, @@ -81,15 +82,25 @@ def _extract_row_id(payload: object) -> int: """Return the row id reported by the v1 API for a CRUD response. The v1 plugin endpoints return ``[{"": true, "message": ""}]`` - where ```` is the affected row identifier. ``ValueError`` is - raised when the payload does not match this shape. + where ```` is the affected row identifier. ``GlpiProtocolError`` + is raised when the payload does not match this shape. + + Raises + ------ + GlpiProtocolError + When ``payload`` is not a non-empty list of mappings, or no + mapping key parses as a numeric row id. """ if not isinstance(payload, list) or not payload: - raise ValueError(f"GLPI Fields plugin response missing row id: {payload!r}") + raise GlpiProtocolError( + f"GLPI Fields plugin response missing row id: {payload!r}" + ) first = payload[0] if not isinstance(first, dict): - raise ValueError(f"GLPI Fields plugin response not a mapping: {payload!r}") + raise GlpiProtocolError( + f"GLPI Fields plugin response not a mapping: {payload!r}" + ) for key in first: if key == "message": continue @@ -97,7 +108,7 @@ def _extract_row_id(payload: object) -> int: return int(key) except (TypeError, ValueError): continue - raise ValueError( + raise GlpiProtocolError( f"GLPI Fields plugin response did not include a numeric id: {payload!r}" ) @@ -342,8 +353,8 @@ def set_ticket_custom_fields( Existing value rows are updated in place; missing rows are created with the supplied payload. Containers/fields that the - server does not know about raise ``ValueError`` *before* any - write to keep the call atomic from the caller's perspective. + server does not know about raise ``GlpiValidationError`` *before* + any write to keep the call atomic from the caller's perspective. Parameters ---------- @@ -365,14 +376,14 @@ def set_ticket_custom_fields( } unknown = sorted(set(values) - set(by_name)) if unknown: - raise ValueError( + raise GlpiValidationError( "Unknown plugin-fields container(s) for Ticket: " + ", ".join(unknown) ) for container_name, column_values in values.items(): container = by_name[container_name] if container.id is None: - raise ValueError( + raise GlpiProtocolError( f"Container {container_name!r} has no id; cannot write values" ) @@ -383,7 +394,7 @@ def set_ticket_custom_fields( } unknown_fields = sorted(set(column_values) - declared) if unknown_fields: - raise ValueError( + raise GlpiValidationError( f"Unknown field(s) for container {container_name!r}: " + ", ".join(unknown_fields) ) diff --git a/glpi_python_client/clients/api/plugins/_fields_async.py b/glpi_python_client/clients/api/plugins/_fields_async.py new file mode 100644 index 0000000..a84d1f3 --- /dev/null +++ b/glpi_python_client/clients/api/plugins/_fields_async.py @@ -0,0 +1,157 @@ +"""Asynchronous overrides for the Fields plugin aggregation helpers. + +:meth:`get_ticket_custom_fields` and :meth:`set_ticket_custom_fields` call +sibling public methods through ``self``. Under the async bridge those +resolve to coroutine functions, so the synchronous bodies would receive +coroutine objects instead of data. These overrides await the +bridge-wrapped calls on the event loop instead. + +The mixin must sit **before** :class:`PluginFieldsMixin` in the +:class:`~glpi_python_client.clients.AsyncGlpiClient` base list so the +bridge's ``__init_subclass__`` hook finds the coroutine via ``getattr`` +and leaves it alone. + +Every awaited call below carries ``# type: ignore[misc]``: the awaited +method (e.g. :meth:`PluginFieldsMixin.list_plugin_fields_containers`) is +declared on this mixin's own parent, so mypy resolves it statically as +the synchronous ``-> T`` method rather than the bridge-generated +coroutine it becomes at runtime on +:class:`~glpi_python_client.clients.AsyncGlpiClient`. See the matching +note in +:mod:`glpi_python_client.clients.api.knowledgebase._article_async` for +the same vocabulary, and contrast with the ``[attr-defined]`` codes in +:mod:`glpi_python_client.clients.custom._statistics_async`, where the +awaited methods are declared on a *different* mixin and mypy cannot +resolve them statically at all. +""" + +from __future__ import annotations + +from typing import Any + +from glpi_python_client._errors import GlpiProtocolError, GlpiValidationError +from glpi_python_client.clients.api.plugins._fields import ( + _TICKET_ITEMTYPE, + PluginFieldsMixin, +) +from glpi_python_client.models.api_schema.plugins import GetPluginFieldsContainer + + +class AsyncPluginFieldsMixin(PluginFieldsMixin): + """Async overrides for the two Fields plugin aggregation helpers.""" + + async def get_ticket_custom_fields( # type: ignore[override] + self, ticket_id: int + ) -> dict[str, dict[str, Any]]: + """Return the custom-field values defined for one ticket. + + Async override of + :meth:`PluginFieldsMixin.get_ticket_custom_fields`; the awaited + calls are the bridge-wrapped public helpers. + + Parameters + ---------- + ticket_id : int + Identifier of the ticket whose custom values are requested. + + Returns + ------- + dict[str, dict[str, Any]] + Per-container value mappings. Empty when the ticket has no + stored custom values across any container. + """ + + containers = await self.list_plugin_fields_containers( # type: ignore[misc] + itemtype=_TICKET_ITEMTYPE + ) + result: dict[str, dict[str, Any]] = {} + for container in containers: + name = container.name + if not name: + continue + rows = await self.list_item_plugin_field_rows( # type: ignore[misc] + _TICKET_ITEMTYPE, ticket_id, name + ) + if not rows: + continue + result[name] = dict(rows[0].extra_payload) + return result + + async def set_ticket_custom_fields( # type: ignore[override] + self, + ticket_id: int, + values: dict[str, dict[str, Any]], + ) -> None: + """Persist custom-field values on one ticket. + + Async override of + :meth:`PluginFieldsMixin.set_ticket_custom_fields`. Validation + order is identical to the synchronous version: unknown containers + and fields raise before any write. + + Parameters + ---------- + ticket_id : int + Identifier of the ticket whose custom values must be set. + values : dict[str, dict[str, Any]] + Nested mapping ``{container_name: {field_name: value}}``. + + Returns + ------- + None + """ + + if not values: + return + + containers = await self.list_plugin_fields_containers( # type: ignore[misc] + itemtype=_TICKET_ITEMTYPE + ) + by_name: dict[str, GetPluginFieldsContainer] = { + c.name: c for c in containers if c.name is not None + } + unknown = sorted(set(values) - set(by_name)) + if unknown: + raise GlpiValidationError( + "Unknown plugin-fields container(s) for Ticket: " + ", ".join(unknown) + ) + + for container_name, column_values in values.items(): + container = by_name[container_name] + if container.id is None: + raise GlpiProtocolError( + f"Container {container_name!r} has no id; cannot write values" + ) + + declared_fields = await self.list_plugin_fields_fields( # type: ignore[misc] + container_id=container.id + ) + declared = {f.name for f in declared_fields if f.name is not None} + unknown_fields = sorted(set(column_values) - declared) + if unknown_fields: + raise GlpiValidationError( + f"Unknown field(s) for container {container_name!r}: " + + ", ".join(unknown_fields) + ) + + existing_rows = await self.list_item_plugin_field_rows( # type: ignore[misc] + _TICKET_ITEMTYPE, ticket_id, container_name + ) + if existing_rows and existing_rows[0].id is not None: + await self.update_item_plugin_field_row( # type: ignore[misc, func-returns-value] + itemtype=_TICKET_ITEMTYPE, + container_name=container_name, + row_id=existing_rows[0].id, + values=column_values, + ) + else: + await self.create_item_plugin_field_row( # type: ignore[misc] + itemtype=_TICKET_ITEMTYPE, + items_id=ticket_id, + container_id=container.id, + container_name=container_name, + values=column_values, + ) + + +__all__ = ["AsyncPluginFieldsMixin"] diff --git a/glpi_python_client/clients/api/plugins/tests/test_fields_async.py b/glpi_python_client/clients/api/plugins/tests/test_fields_async.py new file mode 100644 index 0000000..77fab60 --- /dev/null +++ b/glpi_python_client/clients/api/plugins/tests/test_fields_async.py @@ -0,0 +1,147 @@ +"""Async-client tests for the Fields plugin aggregation helpers. + +These helpers call sibling *public* methods through ``self``. On +``AsyncGlpiClient`` those resolve to bridge-wrapped coroutines, so without +a hand-written async override they raise ``TypeError: 'coroutine' object +is not iterable``. See clients/tests/test_async_selfcall_guard.py. +""" + +from __future__ import annotations + +from typing import Any + +import pytest + +from glpi_python_client import AsyncGlpiClient, GlpiProtocolError, GlpiValidationError +from glpi_python_client.testing.utils import make_async_client + + +class _FakeV1: + """Stand-in for ``GLPIV1Session`` returning queued payloads.""" + + def __init__(self, responses: list[object]) -> None: + self.responses = list(responses) + self.calls: list[dict[str, Any]] = [] + + 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: + self.calls.append({"method": method, "path": path, "json_body": json_body}) + if not self.responses: + raise AssertionError(f"Unexpected v1 call: {method} {path}") + return self.responses.pop(0) + + +@pytest.fixture +def client() -> AsyncGlpiClient: + """Return an async client with no HTTP plumbing wired up.""" + + return make_async_client() + + +async def test_get_ticket_custom_fields_returns_values( + client: AsyncGlpiClient, +) -> None: + """The async helper must return data, not a dropped coroutine.""" + + client._v1 = _FakeV1( # type: ignore[assignment] + [ + [{"id": 1, "name": "custom", "itemtypes": '["Ticket"]'}], + [{"id": 5, "customfield": "hello"}], + ] + ) + + result = await client.get_ticket_custom_fields(1) + + assert result == {"custom": {"customfield": "hello"}} + + +async def test_set_ticket_custom_fields_updates_existing_row( + client: AsyncGlpiClient, +) -> None: + """An existing value row is updated in place through the v1 session.""" + + fake = _FakeV1( + [ + [{"id": 1, "name": "custom", "itemtypes": '["Ticket"]'}], + [{"id": 9, "plugin_fields_containers_id": 1, "name": "customfield"}], + [{"id": 5, "customfield": "old"}], + [{"5": True, "message": ""}], + ] + ) + client._v1 = fake # type: ignore[assignment] + + await client.set_ticket_custom_fields(1, {"custom": {"customfield": "new"}}) + + update = fake.calls[-1] + assert update["method"] == "PUT" + assert update["json_body"] == {"input": {"id": 5, "customfield": "new"}} + + +async def test_set_ticket_custom_fields_rejects_unknown_container( + client: AsyncGlpiClient, +) -> None: + """Unknown containers raise before any write. + + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ + + client._v1 = _FakeV1( # type: ignore[assignment] + [[{"id": 1, "name": "custom", "itemtypes": '["Ticket"]'}]] + ) + + with pytest.raises( + GlpiValidationError, match="Unknown plugin-fields container" + ) as excinfo: + await client.set_ticket_custom_fields(1, {"nope": {"x": 1}}) + assert isinstance(excinfo.value, ValueError) + + +async def test_set_ticket_custom_fields_rejects_container_without_id( + client: AsyncGlpiClient, +) -> None: + """A matched container with no ``id`` raises before any write. + + The container came from the server's own + ``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. + """ + + client._v1 = _FakeV1( # type: ignore[assignment] + [[{"name": "custom", "itemtypes": '["Ticket"]'}]] + ) + + with pytest.raises(GlpiProtocolError, match="has no id") as excinfo: + await client.set_ticket_custom_fields(1, {"custom": {"customfield": "x"}}) + assert isinstance(excinfo.value, ValueError) + + +async def test_set_ticket_custom_fields_rejects_unknown_field( + client: AsyncGlpiClient, +) -> None: + """A typo in the field name raises before any write. + + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ + + client._v1 = _FakeV1( # type: ignore[assignment] + [ + [{"id": 1, "name": "custom", "itemtypes": '["Ticket"]'}], + [{"id": 9, "plugin_fields_containers_id": 1, "name": "customfield"}], + ] + ) + + with pytest.raises(GlpiValidationError, match="Unknown field") as excinfo: + await client.set_ticket_custom_fields(1, {"custom": {"typo": "value"}}) + assert isinstance(excinfo.value, ValueError) diff --git a/glpi_python_client/clients/api/plugins/tests/test_fields_mixin.py b/glpi_python_client/clients/api/plugins/tests/test_fields_mixin.py index d249f08..fce847a 100644 --- a/glpi_python_client/clients/api/plugins/tests/test_fields_mixin.py +++ b/glpi_python_client/clients/api/plugins/tests/test_fields_mixin.py @@ -6,7 +6,7 @@ import pytest -from glpi_python_client import GlpiClient +from glpi_python_client import GlpiClient, GlpiProtocolError, GlpiValidationError from glpi_python_client.clients.api.plugins._fields import ( _container_targets_itemtype, _extract_row_id, @@ -103,14 +103,19 @@ def test_extract_row_id_parses_plugin_response() -> None: def test_extract_row_id_rejects_unexpected_payload() -> None: - """Unexpected payloads raise ``ValueError``.""" + """Unexpected payloads raise ``GlpiProtocolError``, also a ``ValueError``.""" - with pytest.raises(ValueError): + with pytest.raises(GlpiProtocolError) as excinfo: _extract_row_id([]) - with pytest.raises(ValueError): + assert isinstance(excinfo.value, ValueError) + + with pytest.raises(GlpiProtocolError) as excinfo: _extract_row_id([{"message": ""}]) - with pytest.raises(ValueError): + assert isinstance(excinfo.value, ValueError) + + with pytest.raises(GlpiProtocolError) as excinfo: _extract_row_id([42]) + assert isinstance(excinfo.value, ValueError) def test_require_v1_raises_without_session(client: GlpiClient) -> None: @@ -336,18 +341,52 @@ def test_set_ticket_custom_fields_creates_when_missing(client: GlpiClient) -> No def test_set_ticket_custom_fields_rejects_unknown_container(client: GlpiClient) -> None: - """A typo in the container name raises before any write.""" + """A typo in the container name raises before any write. + + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ fake = _FakeV1(responses=[[{"id": 10, "name": "real", "itemtypes": '["Ticket"]'}]]) client._v1 = fake # type: ignore[assignment] - with pytest.raises(ValueError, match="Unknown plugin-fields container"): + with pytest.raises( + GlpiValidationError, match="Unknown plugin-fields container" + ) as excinfo: client.set_ticket_custom_fields(62571, {"typo": {"any": "value"}}) # No mutation was sent. assert all(c["method"] == "GET" for c in fake.calls) + assert isinstance(excinfo.value, ValueError) + + +def test_set_ticket_custom_fields_rejects_container_without_id( + client: GlpiClient, +) -> None: + """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` + 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"]'}]]) + 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"}} + ) + assert all(c["method"] == "GET" for c in fake.calls) + assert isinstance(excinfo.value, ValueError) def test_set_ticket_custom_fields_rejects_unknown_field(client: GlpiClient) -> None: - """A typo in the field name raises before any write.""" + """A typo in the field name raises before any write. + + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ fake = _FakeV1( responses=[ @@ -362,8 +401,9 @@ def test_set_ticket_custom_fields_rejects_unknown_field(client: GlpiClient) -> N ] ) client._v1 = fake # type: ignore[assignment] - with pytest.raises(ValueError, match="Unknown field"): + with pytest.raises(GlpiValidationError, match="Unknown field") as excinfo: client.set_ticket_custom_fields(62571, {"aidelarsolution": {"typo": "value"}}) + assert isinstance(excinfo.value, ValueError) def test_set_ticket_custom_fields_with_empty_mapping_is_noop( diff --git a/glpi_python_client/clients/async_client.py b/glpi_python_client/clients/async_client.py index 4cc34fd..e8d4d30 100644 --- a/glpi_python_client/clients/async_client.py +++ b/glpi_python_client/clients/async_client.py @@ -4,10 +4,12 @@ into :class:`~glpi_python_client.clients.sync_client.GlpiClient` and wraps each public method into a coroutine through :class:`~glpi_python_client.clients.commons._async_bridge.AsyncBridge`. -Helpers that benefit from concurrent fan-out -(:meth:`get_ticket_context`, :meth:`get_task_statistics`) are replaced -by their dedicated async overrides under -:mod:`glpi_python_client.clients.custom`. +Some methods ship hand-written async overrides instead — for concurrent +``asyncio.gather`` fan-out (:mod:`glpi_python_client.clients.custom`) or +to stop the bridge from silently dropping an internal call to a sibling +public method through ``self`` +(:mod:`glpi_python_client.clients.api.knowledgebase`, +:mod:`glpi_python_client.clients.api.plugins`). The async client owns the same HTTP session and token manager as the synchronous client but its lifecycle is driven through ``async with`` / @@ -32,15 +34,15 @@ from glpi_python_client.clients._base_client import _BaseGlpiClient from glpi_python_client.clients.api import ( + AsyncKBArticleMixin, + AsyncPluginFieldsMixin, DocumentMixin, EntityMixin, FollowupMixin, KBArticleCommentMixin, - KBArticleMixin, KBArticleRevisionMixin, KBCategoryMixin, LocationMixin, - PluginFieldsMixin, SolutionMixin, TeamMemberMixin, TicketMixin, @@ -73,10 +75,10 @@ class AsyncGlpiClient( # type: ignore[misc] EntityMixin, LocationMixin, KBCategoryMixin, - KBArticleMixin, + AsyncKBArticleMixin, KBArticleCommentMixin, KBArticleRevisionMixin, - PluginFieldsMixin, + AsyncPluginFieldsMixin, AsyncTicketContextMixin, AsyncStatisticsMixin, _BaseGlpiClient, @@ -86,9 +88,11 @@ class AsyncGlpiClient( # type: ignore[misc] Every public sync method exposed by the inherited mixins is automatically wrapped into a coroutine that defers the blocking call - to a worker thread. The custom helpers that benefit from concurrent - fan-out provide hand-written async overrides which are preserved as - coroutine functions by the bridge. + to a worker thread. A handful of methods ship hand-written async + overrides instead — for concurrent fan-out or to stop the bridge + from silently dropping an internal call to a sibling public method + through ``self`` — which are preserved as coroutine functions by the + bridge. Construction parameters and :meth:`from_env` are documented on :class:`~glpi_python_client.clients._base_client._BaseGlpiClient`; @@ -101,20 +105,27 @@ def __init__(self, *, executor: Executor | None = None, **kwargs: Any) -> None: Parameters ---------- executor : concurrent.futures.Executor | None, optional - Optional executor every wrapped call is routed through. When - ``None`` (the default) the bridge falls back to - :func:`asyncio.to_thread`, which uses the loop's default - thread pool executor. Supply a dedicated + Optional executor that every bridge-generated wrapped call + (see :class:`~glpi_python_client.clients.commons._async_bridge.AsyncBridge`) + is routed through. When ``None`` (the default) the bridge + falls back to :func:`asyncio.to_thread`, which uses the + loop's default thread pool executor. Supply a dedicated :class:`concurrent.futures.ThreadPoolExecutor` when the application performs aggressive fan-outs that would - otherwise saturate the default pool. + otherwise saturate the default pool. This does **not** + cover the hand-written + :class:`~glpi_python_client.clients.api.knowledgebase._article_async.AsyncKBArticleMixin` + overrides (``create_kb_article``/``update_kb_article``): they + dispatch their worker-thread call through a plain + :func:`asyncio.to_thread` and always use the loop's default + thread pool regardless of this argument. **kwargs : Any Remaining keyword arguments forwarded to :class:`~glpi_python_client.clients._base_client._BaseGlpiClient`. Raises ------ - ValueError + GlpiValidationError If the supplied configuration is incomplete or invalid (e.g. missing OAuth credentials together with no v1 fallback). """ diff --git a/glpi_python_client/clients/commons/__init__.py b/glpi_python_client/clients/commons/__init__.py index ed0e19e..b2130c6 100644 --- a/glpi_python_client/clients/commons/__init__.py +++ b/glpi_python_client/clients/commons/__init__.py @@ -1,7 +1,7 @@ """Reusable client-layer building blocks shared across the API mixins. The commons package centralises constants, HTTP helpers, RSQL filter -builders, error formatting, transport, and the client configuration helpers +builders, transport, and the client configuration helpers used by the per-endpoint mixins under :mod:`glpi_python_client.clients.api` and the higher-level helpers under :mod:`glpi_python_client.clients.custom`. """ diff --git a/glpi_python_client/clients/commons/_config.py b/glpi_python_client/clients/commons/_config.py index 707d22f..3b417bd 100644 --- a/glpi_python_client/clients/commons/_config.py +++ b/glpi_python_client/clients/commons/_config.py @@ -15,6 +15,8 @@ import requests import urllib3 +from glpi_python_client._errors import GlpiValidationError + if TYPE_CHECKING: from glpi_python_client.auth._v1_session import GLPIV1Session from glpi_python_client.auth.auth import GLPITokenManager @@ -120,6 +122,14 @@ def parse_optional_env_int(value: object) -> int | None: ``None`` is preserved, native integers are accepted as-is, and strings are converted through ``int()`` so explicit overrides and environment values follow the same normalisation path. + + Raises + ------ + GlpiValidationError + If a string value cannot be parsed as an integer (e.g. + ``GLPI_TIMEOUT=abc``). + TypeError + If ``value`` is neither ``None``, ``int``, nor ``str``. """ if value is None: @@ -127,7 +137,12 @@ def parse_optional_env_int(value: object) -> int | None: if isinstance(value, int): return value if isinstance(value, str): - return int(value) + try: + return int(value) + except ValueError as exc: + raise GlpiValidationError( + f"Invalid integer environment value: {value!r}" + ) from exc raise TypeError("Integer environment values must be strings or integers") @@ -149,7 +164,7 @@ def parse_optional_env_bool(value: object, *, default: bool) -> bool: return True if value.casefold() in {"0", "false", "no", "off"}: return False - raise ValueError(f"Invalid boolean environment value: {value!r}") + raise GlpiValidationError(f"Invalid boolean environment value: {value!r}") def build_client_env_config( @@ -201,7 +216,7 @@ def normalize_client_api_url(glpi_api_url: object, *, client_name: str) -> str: """ if not isinstance(glpi_api_url, str) or not glpi_api_url: - raise ValueError(f"{client_name} requires glpi_api_url") + raise GlpiValidationError(f"{client_name} requires glpi_api_url") return glpi_api_url.rstrip("/") @@ -217,7 +232,7 @@ def validate_v1_document_config( """ if bool(v1_base_url) != bool(v1_user_token): - raise ValueError( + raise GlpiValidationError( "GLPI v1 document support requires both v1_base_url and v1_user_token." ) diff --git a/glpi_python_client/clients/commons/_errors.py b/glpi_python_client/clients/commons/_errors.py deleted file mode 100644 index e6625ce..0000000 --- a/glpi_python_client/clients/commons/_errors.py +++ /dev/null @@ -1,27 +0,0 @@ -"""Error formatting helpers for client-side exception handling. - -The helpers here normalise messages produced by the third-party libraries -the transport layer relies on so higher-level mixins can log readable error -strings without depending on those libraries directly. -""" - -from __future__ import annotations - -from tenacity import RetryError - - -def remote_error_message(exc: Exception) -> str: - """Return a readable message for one remote-call exception. - - ``tenacity.RetryError`` instances are unwrapped to expose the underlying - failure message instead of the retry wrapper representation. - """ - - if isinstance(exc, RetryError): - inner_exception = exc.last_attempt.exception() - if isinstance(inner_exception, Exception): - return str(inner_exception) - return str(exc) - - -__all__ = ["remote_error_message"] diff --git a/glpi_python_client/clients/commons/_http.py b/glpi_python_client/clients/commons/_http.py index 4573982..a662565 100644 --- a/glpi_python_client/clients/commons/_http.py +++ b/glpi_python_client/clients/commons/_http.py @@ -12,6 +12,11 @@ import requests +from glpi_python_client._errors import ( + GlpiProtocolError, + GlpiServerError, + status_error_class, +) from glpi_python_client.clients.commons._constants import RequestParamValue @@ -48,10 +53,15 @@ def require_access_token(access_token: str | None) -> str: Transport helpers call this right before request dispatch so missing token state turns into a clear local error instead of a malformed API call. + + Raises + ------ + GlpiProtocolError + When ``access_token`` is empty or ``None``. """ if not access_token: - raise ValueError("Failed to acquire access token for API request") + raise GlpiProtocolError("Failed to acquire access token for API request") return access_token @@ -112,6 +122,11 @@ def finalize_request_response( Server errors are raised immediately while non-success statuses outside the accepted set are logged for higher-level mutation and lookup helpers to interpret consistently. + + Raises + ------ + GlpiServerError + When ``response.status_code`` is a 5xx server error. """ method_name = method.upper() @@ -121,14 +136,19 @@ def finalize_request_response( f"{response.status_code} {response.reason}" ) logger.warning(message) - raise requests.HTTPError(message) + raise GlpiServerError( + message, + status_code=response.status_code, + url=url, + response_text=response.text, + ) if response.status_code not in success_statuses: logger.warning( "GLPI %s %s returned %s: %s", method_name, url, response.status_code, - response.text[:200], + response.text, ) return response @@ -139,15 +159,26 @@ def ensure_response_status( success_statuses: tuple[int, ...], failure_message: str, ) -> None: - """Raise a consistent ``ValueError`` for an unexpected response status. + """Raise a typed :class:`GlpiStatusError` for an unexpected response status. Higher-level client methods use this helper to keep their mutation and - fetch failure messages aligned across the per-endpoint mixins. + fetch failure messages aligned across the per-endpoint mixins. The + raised class narrows to :class:`GlpiAuthError`, :class:`GlpiNotFoundError` + or :class:`GlpiServerError` where the status allows it. + + Raises + ------ + GlpiStatusError + When ``response.status_code`` is outside ``success_statuses``. """ if response.status_code not in success_statuses: - raise ValueError( - f"{failure_message}: {response.status_code} {response.text[:200]}" + error_class = status_error_class(response.status_code) + raise error_class( + f"{failure_message}: {response.status_code} {response.text}", + status_code=response.status_code, + url=str(response.url), + response_text=response.text, ) @@ -187,6 +218,11 @@ def require_response_int( GLPI v2 create responses document numeric identifiers under a small set of keys. Callers list the candidate keys explicitly so the behaviour stays predictable. + + Raises + ------ + GlpiProtocolError + When none of ``keys`` maps to an integer value in the response. """ result = response_json_mapping(response) @@ -194,7 +230,7 @@ def require_response_int( value = result.get(key) if isinstance(value, int) and not isinstance(value, bool): return value - raise ValueError(missing_message) + raise GlpiProtocolError(missing_message) def list_payload_items(payload: object) -> list[dict[str, object]]: diff --git a/glpi_python_client/clients/commons/_transport.py b/glpi_python_client/clients/commons/_transport.py index 7041b64..feb6ed0 100644 --- a/glpi_python_client/clients/commons/_transport.py +++ b/glpi_python_client/clients/commons/_transport.py @@ -38,6 +38,7 @@ import requests from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed +from glpi_python_client._errors import GlpiServerError from glpi_python_client.clients.commons._http import ( build_request_headers, build_request_url, @@ -210,9 +211,10 @@ def _execute_request( ) @retry( - retry=retry_if_exception_type(requests.RequestException), + retry=retry_if_exception_type((requests.RequestException, GlpiServerError)), stop=stop_after_attempt(3), wait=wait_fixed(3), + reraise=True, ) def _get_request( self, @@ -222,9 +224,11 @@ def _get_request( ) -> requests.Response: """Execute one authenticated GLPI ``GET`` request. - Network-level request exceptions are retried according to the - transport retry policy before the response is returned to the - caller. + Network errors (:class:`requests.RequestException`) and 5xx + responses (:class:`~glpi_python_client.GlpiServerError`) are + retried up to 3 times, with ``reraise=True`` so the real error + propagates once retries are exhausted; 4xx responses are + returned as-is without a retry. """ return self._execute_request( @@ -236,9 +240,10 @@ def _get_request( ) @retry( - retry=retry_if_exception_type(requests.RequestException), + retry=retry_if_exception_type((requests.RequestException, GlpiServerError)), stop=stop_after_attempt(3), wait=wait_fixed(3), + reraise=True, ) def _post_request( self, @@ -262,9 +267,10 @@ def _post_request( ) @retry( - retry=retry_if_exception_type(requests.RequestException), + retry=retry_if_exception_type((requests.RequestException, GlpiServerError)), stop=stop_after_attempt(3), wait=wait_fixed(3), + reraise=True, ) def _update_request( self, @@ -287,9 +293,10 @@ def _update_request( ) @retry( - retry=retry_if_exception_type(requests.RequestException), + retry=retry_if_exception_type((requests.RequestException, GlpiServerError)), stop=stop_after_attempt(3), wait=wait_fixed(3), + reraise=True, ) def _delete_request( self, @@ -384,8 +391,8 @@ def _resource_get( model : type[ModelT] Pydantic class used to validate the response payload. failure_message : str - Message embedded in the ``ValueError`` raised on a non-success - HTTP status. + Message embedded in the ``GlpiStatusError`` raised on a + non-success HTTP status. skip_entity : bool, optional When ``True`` the ``GLPI-Entity`` header is omitted. @@ -423,10 +430,10 @@ def _resource_create( body_model : GlpiModel Pydantic body serialised through :func:`model_to_payload`. failure_message : str - Message embedded in the ``ValueError`` raised on a non-success - HTTP status. + Message embedded in the ``GlpiStatusError`` raised on a + non-success HTTP status. missing_message : str - Message embedded in the ``ValueError`` raised when the + Message embedded in the ``GlpiProtocolError`` raised when the response payload does not contain any of the expected identifier keys. log_message_factory : Callable[[int], str] @@ -477,8 +484,8 @@ def _resource_update( Partial Pydantic body serialised through :func:`model_to_payload`. failure_message : str - Message embedded in the ``ValueError`` raised on a non-success - HTTP status. + Message embedded in the ``GlpiStatusError`` raised on a + non-success HTTP status. log_message : str Pre-formatted message logged at ``INFO`` level on success. @@ -513,8 +520,8 @@ def _resource_delete( endpoint : str Resource path forwarded to the transport ``DELETE`` helper. failure_message : str - Message embedded in the ``ValueError`` raised on a non-success - HTTP status. + Message embedded in the ``GlpiStatusError`` raised on a + non-success HTTP status. log_message : str Pre-formatted message logged at ``INFO`` level on success. force : bool | None, optional diff --git a/glpi_python_client/clients/commons/tests/test_errors.py b/glpi_python_client/clients/commons/tests/test_errors.py deleted file mode 100644 index ad7559c..0000000 --- a/glpi_python_client/clients/commons/tests/test_errors.py +++ /dev/null @@ -1,41 +0,0 @@ -"""Unit tests for :func:`remote_error_message`.""" - -from __future__ import annotations - -from concurrent.futures import Future - -from tenacity import RetryError - -from glpi_python_client.clients.commons._errors import remote_error_message - - -def _make_retry_error(inner: Exception) -> RetryError: - """Wrap ``inner`` in a ``tenacity.RetryError`` for testing.""" - - future: Future[object] = Future() - future.set_exception(inner) - return RetryError(future) - - -def test_remote_error_message_returns_plain_str_for_regular_exception() -> None: - """Non-retry exceptions are stringified directly.""" - - assert remote_error_message(ValueError("boom")) == "boom" - - -def test_remote_error_message_unwraps_retry_error() -> None: - """``RetryError`` exposes the underlying failure message.""" - - inner = RuntimeError("network down") - wrapped = _make_retry_error(inner) - assert remote_error_message(wrapped) == "network down" - - -def test_remote_error_message_falls_back_when_inner_is_missing() -> None: - """A ``RetryError`` without a real inner exception falls back to ``str``.""" - - future: Future[object] = Future() - future.set_result("ok") - wrapped = RetryError(future) - # The inner attempt did not raise, so the helper falls back to str(exc). - assert remote_error_message(wrapped) == str(wrapped) diff --git a/glpi_python_client/clients/commons/tests/test_http.py b/glpi_python_client/clients/commons/tests/test_http.py index 4b25c11..337f95f 100644 --- a/glpi_python_client/clients/commons/tests/test_http.py +++ b/glpi_python_client/clients/commons/tests/test_http.py @@ -10,6 +10,7 @@ import pytest +from glpi_python_client import GlpiProtocolError, GlpiValidationError from glpi_python_client.clients.commons._http import ( build_request_headers, build_request_url, @@ -129,3 +130,32 @@ def test_fake_response_round_trip() -> None: response = FakeResponse(payload={"hello": "world"}) assert json.loads(response.text.replace("'", '"')) == {"hello": "world"} + + +def test_require_access_token_raises_protocol_error_when_missing() -> None: + """A missing token means the OAuth response was unusable, not a caller error.""" + + with pytest.raises(GlpiProtocolError) as excinfo: + require_access_token(None) + + assert isinstance(excinfo.value, ValueError) + assert str(excinfo.value) == "Failed to acquire access token for API request" + + +def test_require_response_int_raises_protocol_error_when_id_missing() -> None: + """A 2xx create response without a numeric id is a protocol failure.""" + + response = FakeResponse(status_code=201, payload={"nope": "x"}) + with pytest.raises(GlpiProtocolError) as excinfo: + require_response_int( + response, keys=("id",), missing_message="GLPI create returned no id" + ) + + assert isinstance(excinfo.value, ValueError) + assert str(excinfo.value) == "GLPI create returned no id" + + +def test_protocol_error_is_not_a_validation_error() -> None: + """A server-shape fault must not masquerade as a caller mistake.""" + + assert not issubclass(GlpiProtocolError, GlpiValidationError) diff --git a/glpi_python_client/clients/commons/tests/test_retry_semantics.py b/glpi_python_client/clients/commons/tests/test_retry_semantics.py new file mode 100644 index 0000000..d628f0b --- /dev/null +++ b/glpi_python_client/clients/commons/tests/test_retry_semantics.py @@ -0,0 +1,163 @@ +"""Retry semantics for the v2 transport: 5xx is retried, 4xx is not. + +These tests are the regression net for the retry predicate. Getting the +predicate wrong disables retries silently — nothing raises, nothing fails, +requests simply stop being retried. See the 0.4.0 plan-1 notes. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from typing import Any + +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.testing.utils import FakeResponse, make_client + +_RETRIED_METHODS = ( + "_get_request", + "_post_request", + "_update_request", + "_delete_request", +) + + +@pytest.fixture(autouse=True) +def _no_retry_sleep(monkeypatch: pytest.MonkeyPatch) -> None: + """Drop the 3s fixed wait so retry tests stay instant. + + The decorator's ``Retrying`` object is patched directly. Patching + ``tenacity.nap.time.sleep`` would work today but silently stops working + on the async path, so it is deliberately not used here. + """ + + for name in _RETRIED_METHODS: + monkeypatch.setattr(getattr(GlpiClient, name).retry, "wait", wait_fixed(0)) + + +@pytest.fixture +def client() -> Iterator[Any]: + """Return a client with auth stubbed so no token call is made.""" + + c = make_client() + c._auth.access_token = "test-token" + c._auth.ensure_token = lambda: None + yield c + c.close() + + +@pytest.mark.parametrize("method_name", _RETRIED_METHODS) +def test_5xx_is_retried_three_times_and_reraises_server_error( + client: Any, method_name: str +) -> None: + """A persistent 5xx costs 3 attempts and surfaces as ``GlpiServerError``. + + Parametrized across all four retried verbs (``_get_request``, + ``_post_request``, ``_update_request``, ``_delete_request``): they share + the same decorator, but before this test only ``_get_request``'s attempt + count was pinned. + """ + + attempts: list[int] = [] + + def _send(method: str, url: str, **kw: Any) -> FakeResponse: + attempts.append(1) + return FakeResponse( + status_code=500, payload={}, text="boom", reason="Server Error" + ) + + client._send_request = _send + with pytest.raises(GlpiServerError) as excinfo: + getattr(client, method_name)("Assistance/Ticket") + + assert len(attempts) == 3 + assert excinfo.value.status_code == 500 + assert excinfo.value.url == "https://glpi.example.test/api.php/Assistance/Ticket" + + +def test_persistent_5xx_does_not_surface_as_retry_error(client: Any) -> None: + """``reraise=True``: callers see the real error, never ``tenacity.RetryError``.""" + + import tenacity + + client._send_request = lambda method, url, **kw: FakeResponse( + status_code=503, payload={}, text="down", reason="Service Unavailable" + ) + with pytest.raises(GlpiServerError) as excinfo: + client._get_request("Assistance/Ticket") + assert not isinstance(excinfo.value, tenacity.RetryError) + + +@pytest.mark.parametrize("method_name", _RETRIED_METHODS) +def test_4xx_is_not_retried_by_the_transport(client: Any, method_name: str) -> None: + """A 4xx is logged and returned by ``finalize_request_response``, not retried. + + Parametrized across all four retried verbs so a predicate regression + that starts retrying 4xx on any single verb fails loudly. + """ + + attempts: list[int] = [] + + def _send(method: str, url: str, **kw: Any) -> FakeResponse: + attempts.append(1) + return FakeResponse(status_code=404, payload={}, text="nope") + + client._send_request = _send + response = getattr(client, method_name)("Assistance/Ticket/1") + + assert len(attempts) == 1 + assert response.status_code == 404 + + +def test_4xx_raises_a_typed_status_error_from_ensure_response_status() -> None: + """The 4xx raise stays in ``ensure_response_status`` and is typed.""" + + response = FakeResponse(status_code=404, payload={}, text="nope") + with pytest.raises(GlpiNotFoundError) as excinfo: + ensure_response_status( + response, + success_statuses=(200, 206), + failure_message="Failed to fetch ticket 1", + ) + + assert excinfo.value.status_code == 404 + assert isinstance(excinfo.value, ValueError) + assert str(excinfo.value) == "Failed to fetch ticket 1: 404 nope" + + +def test_tolerant_search_still_returns_empty_on_4xx(client: Any) -> None: + """Search endpoints that pass no ``failure_message`` still swallow a 4xx. + + Guards the 7 tolerant ``_resource_list`` call sites against the 4xx raise + being moved into ``finalize_request_response``. + """ + + client._send_request = lambda method, url, **kw: FakeResponse( + status_code=400, payload=[], text="[]" + ) + assert client.search_tickets() == [] + + +@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. + + Parametrized across all four retried verbs so the network-fault attempt + count is pinned for each, not just ``_get_request``. + """ + + attempts: list[int] = [] + + def _send(method: str, url: str, **kw: Any) -> FakeResponse: + attempts.append(1) + raise requests.ConnectionError("network down") + + client._send_request = _send + with pytest.raises(requests.ConnectionError): + getattr(client, method_name)("Assistance/Ticket") + + assert len(attempts) == 3 diff --git a/glpi_python_client/clients/custom/_statistics.py b/glpi_python_client/clients/custom/_statistics.py index 03149a5..ccb2329 100644 --- a/glpi_python_client/clients/custom/_statistics.py +++ b/glpi_python_client/clients/custom/_statistics.py @@ -14,6 +14,7 @@ from datetime import date, timedelta from typing import TypedDict +from glpi_python_client._errors import GlpiValidationError from glpi_python_client.clients.commons._filters import ( rsql_all_filter, rsql_any_filter, @@ -125,8 +126,9 @@ def get_ticket_statistics( Raises ------ - ValueError - If ``default_days < 1`` or ``start_date > end_date``. + GlpiValidationError + If ``default_days < 1``, ``start_date`` / ``end_date`` is not a + valid ISO date, or ``start_date`` is after ``end_date``. """ start, end = _resolve_window( @@ -265,8 +267,9 @@ def get_task_durations( Raises ------ - ValueError - If ``default_days < 1`` or ``start_date > end_date``. + GlpiValidationError + If ``default_days < 1``, ``start_date`` / ``end_date`` is not a + valid ISO date, or ``start_date`` is after ``end_date``. """ start, end = _resolve_window( @@ -421,14 +424,14 @@ def get_user_activity( Raises ------ - ValueError + GlpiValidationError If none of ``user_id``, ``username``, ``realname``, or ``firstname`` are supplied, or if the supplied criteria match no GLPI users. """ if all(v is None for v in (user_id, username, realname, firstname)): - raise ValueError( + raise GlpiValidationError( "At least one of user_id, username, realname, or " "firstname must be supplied" ) @@ -454,7 +457,7 @@ def get_user_activity( limit=200, ) if not matched_users: - raise ValueError("No users matched the supplied criteria") + raise GlpiValidationError("No users matched the supplied criteria") resolved_user_ids = [u.id for u in matched_users if u.id is not None] user_display_map = { u.id: ( @@ -561,18 +564,31 @@ def _resolve_window( Validation matches the legacy analytics helper: positive default span, parsed ISO dates, and ``start <= end``. + + Raises + ------ + GlpiValidationError + If ``default_days < 1``, ``start_date`` / ``end_date`` is not a + valid ISO ``YYYY-MM-DD`` string, or ``start_date`` is after + ``end_date``. """ if default_days < 1: - raise ValueError("default_days must be a positive integer") - parsed_end = date.fromisoformat(end_date) if end_date else date.today() - parsed_start = ( - date.fromisoformat(start_date) - if start_date - else parsed_end - timedelta(days=default_days - 1) - ) + raise GlpiValidationError("default_days must be a positive integer") + try: + parsed_end = date.fromisoformat(end_date) if end_date else date.today() + except ValueError as exc: + raise GlpiValidationError(f"Invalid end_date: {end_date!r}") from exc + try: + parsed_start = ( + date.fromisoformat(start_date) + if start_date + else parsed_end - timedelta(days=default_days - 1) + ) + except ValueError as exc: + raise GlpiValidationError(f"Invalid start_date: {start_date!r}") from exc if parsed_start > parsed_end: - raise ValueError("start_date must be less than or equal to end_date") + raise GlpiValidationError("start_date must be less than or equal to end_date") return parsed_start, parsed_end diff --git a/glpi_python_client/clients/custom/_statistics_async.py b/glpi_python_client/clients/custom/_statistics_async.py index a9692aa..fc4d00b 100644 --- a/glpi_python_client/clients/custom/_statistics_async.py +++ b/glpi_python_client/clients/custom/_statistics_async.py @@ -18,6 +18,7 @@ import asyncio +from glpi_python_client._errors import GlpiValidationError from glpi_python_client.clients.custom._statistics import ( StatisticsMixin, TaskDurationsResult, @@ -295,8 +296,9 @@ async def get_ticket_statistics( # type: ignore[override] Raises ------ - ValueError - If ``default_days < 1`` or ``start_date > end_date``. + GlpiValidationError + If ``default_days < 1``, ``start_date`` / ``end_date`` is not a + valid ISO date, or ``start_date`` is after ``end_date``. """ from glpi_python_client.clients.commons._filters import ( @@ -388,7 +390,7 @@ async def get_user_activity( # type: ignore[override] Raises ------ - ValueError + GlpiValidationError If none of ``user_id``, ``username``, ``realname``, or ``firstname`` are supplied, or if the supplied criteria match no GLPI users. @@ -406,7 +408,7 @@ async def get_user_activity( # type: ignore[override] ) if all(v is None for v in (user_id, username, realname, firstname)): - raise ValueError( + raise GlpiValidationError( "At least one of user_id, username, realname, or " "firstname must be supplied" ) @@ -432,7 +434,7 @@ async def get_user_activity( # type: ignore[override] limit=200, ) if not matched_users: - raise ValueError("No users matched the supplied criteria") + raise GlpiValidationError("No users matched the supplied criteria") resolved_user_ids = [u.id for u in matched_users if u.id is not None] user_display_map = { u.id: ( diff --git a/glpi_python_client/clients/custom/_ticket_context.py b/glpi_python_client/clients/custom/_ticket_context.py index 673b0ee..7bcd32d 100644 --- a/glpi_python_client/clients/custom/_ticket_context.py +++ b/glpi_python_client/clients/custom/_ticket_context.py @@ -43,7 +43,7 @@ def get_ticket_context(self, ticket_id: GlpiId) -> GlpiTicketContext: Raises ------ - ValueError + GlpiStatusError If any of the underlying GLPI calls returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/custom/_ticket_context_async.py b/glpi_python_client/clients/custom/_ticket_context_async.py index c1ea55e..b1f386b 100644 --- a/glpi_python_client/clients/custom/_ticket_context_async.py +++ b/glpi_python_client/clients/custom/_ticket_context_async.py @@ -47,7 +47,7 @@ async def get_ticket_context( # type: ignore[override] Raises ------ - ValueError + GlpiStatusError If any of the underlying GLPI calls returns a non-success HTTP status. """ diff --git a/glpi_python_client/clients/custom/tests/test_statistics.py b/glpi_python_client/clients/custom/tests/test_statistics.py index ee3733b..519b111 100644 --- a/glpi_python_client/clients/custom/tests/test_statistics.py +++ b/glpi_python_client/clients/custom/tests/test_statistics.py @@ -17,6 +17,7 @@ GlpiPriority, GlpiTicketStatus, GlpiTicketType, + GlpiValidationError, ) from glpi_python_client.models.api_schema._common import ( IdNameCompletenameRef, @@ -134,12 +135,36 @@ def fake_search( def test_get_ticket_statistics_rejects_invalid_window(client: GlpiClient) -> None: - """Invalid date inputs raise locally before any HTTP request.""" + """Invalid date inputs raise locally before any HTTP request. - with pytest.raises(ValueError, match="default_days"): + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ + + with pytest.raises(GlpiValidationError, match="default_days") as exc1: client.get_ticket_statistics(default_days=0) - with pytest.raises(ValueError, match="start_date"): + assert isinstance(exc1.value, ValueError) + with pytest.raises(GlpiValidationError, match="start_date") as exc2: client.get_ticket_statistics(start_date="2026-02-01", end_date="2026-01-01") + assert isinstance(exc2.value, ValueError) + + +def test_get_ticket_statistics_rejects_malformed_iso_date( + client: GlpiClient, +) -> None: + """A malformed ISO date string raises ``GlpiValidationError``, not a bare + ``date.fromisoformat`` ``ValueError``. + + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working, and the original ``ValueError`` + from ``date.fromisoformat`` is chained via ``from`` rather than + swallowed. + """ + + with pytest.raises(GlpiValidationError, match="start_date") as excinfo: + client.get_ticket_statistics(start_date="2026-13-45", end_date="2026-01-31") + assert isinstance(excinfo.value, ValueError) + assert isinstance(excinfo.value.__cause__, ValueError) def test_get_task_statistics_zero_for_empty_input(client: GlpiClient) -> None: @@ -421,10 +446,15 @@ def fake_iter(rsql_filter: str = "", *, batch_size: int = 200): def test_get_user_activity_raises_without_identifier(client: GlpiClient) -> None: - """Calling without any identifier raises ValueError.""" + """Calling without any identifier raises ``GlpiValidationError``. - with pytest.raises(ValueError, match="user_id"): + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ + + with pytest.raises(GlpiValidationError, match="user_id") as excinfo: client.get_user_activity() + assert isinstance(excinfo.value, ValueError) def test_get_user_activity_single_user_happy_path(client: GlpiClient) -> None: @@ -487,7 +517,11 @@ def fake_task_durations( def test_get_user_activity_raises_when_no_users_matched(client: GlpiClient) -> None: - """When search_users returns empty a ValueError is raised.""" + """When search_users returns empty a ``GlpiValidationError`` is raised. + + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ from glpi_python_client.models.api_schema.administration._user import GetUser @@ -501,8 +535,9 @@ def fake_search_users( return [] client.search_users = fake_search_users # type: ignore[method-assign] - with pytest.raises(ValueError, match="No users matched"): + with pytest.raises(GlpiValidationError, match="No users matched") as excinfo: client.get_user_activity(username="ghost") + assert isinstance(excinfo.value, ValueError) def test_get_user_activity_multi_user_merge(client: GlpiClient) -> None: diff --git a/glpi_python_client/clients/tests/test_api_coverage.py b/glpi_python_client/clients/tests/test_api_coverage.py index 77c41f1..3819b8a 100644 --- a/glpi_python_client/clients/tests/test_api_coverage.py +++ b/glpi_python_client/clients/tests/test_api_coverage.py @@ -15,6 +15,7 @@ from glpi_python_client import ( GlpiClient, + GlpiValidationError, PatchDocument, PatchEntity, PatchFollowup, @@ -411,10 +412,15 @@ def test_download_document_raises_on_failure(client: GlpiClient) -> None: def test_upload_document_requires_filename(client: GlpiClient) -> None: - """``upload_document`` rejects an empty filename before any HTTP call.""" + """``upload_document`` rejects an empty filename before any HTTP call. - with pytest.raises(ValueError, match="filename"): + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ + + with pytest.raises(GlpiValidationError, match="filename") as excinfo: client.upload_document(filename="", content=b"x") + assert isinstance(excinfo.value, ValueError) def test_upload_document_dispatches_to_v1(client: GlpiClient) -> None: diff --git a/glpi_python_client/clients/tests/test_async_branches.py b/glpi_python_client/clients/tests/test_async_branches.py index b3cf646..7a8decd 100644 --- a/glpi_python_client/clients/tests/test_async_branches.py +++ b/glpi_python_client/clients/tests/test_async_branches.py @@ -7,7 +7,7 @@ import pytest -from glpi_python_client import AsyncGlpiClient +from glpi_python_client import AsyncGlpiClient, GlpiValidationError from glpi_python_client.testing.utils import FakeResponse, make_async_client @@ -515,11 +515,16 @@ async def fake_search_tickets(rsql_filter: str = "", **kwargs: Any) -> list[Any] async def test_async_get_user_activity_raises_without_criteria() -> None: - """ValueError is raised when no user criteria are supplied.""" + """``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(ValueError, match="At least one of"): + with pytest.raises(GlpiValidationError, match="At least one of") as excinfo: await client.get_user_activity() + assert isinstance(excinfo.value, ValueError) await client.close() @@ -554,7 +559,11 @@ async def fake_task_durations(**kwargs: Any) -> Any: async def test_async_get_user_activity_by_username_no_match_raises() -> None: - """ValueError is raised when no users match the supplied criteria.""" + """``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() @@ -563,8 +572,9 @@ async def fake_search_users(rsql_filter: str = "", **kwargs: Any) -> list[Any]: client.search_users = fake_search_users # type: ignore[method-assign] - with pytest.raises(ValueError, match="No users matched"): + 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() diff --git a/glpi_python_client/clients/tests/test_async_selfcall_guard.py b/glpi_python_client/clients/tests/test_async_selfcall_guard.py new file mode 100644 index 0000000..42fa6a2 --- /dev/null +++ b/glpi_python_client/clients/tests/test_async_selfcall_guard.py @@ -0,0 +1,226 @@ +"""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_glpi_client.py b/glpi_python_client/clients/tests/test_glpi_client.py index 51bfe6a..f1114ca 100644 --- a/glpi_python_client/clients/tests/test_glpi_client.py +++ b/glpi_python_client/clients/tests/test_glpi_client.py @@ -7,7 +7,7 @@ import pytest -from glpi_python_client import GlpiClient +from glpi_python_client import GlpiClient, GlpiValidationError from glpi_python_client.clients.commons._config import ( build_client_env_config, normalize_client_api_url, @@ -27,23 +27,40 @@ def test_normalize_client_api_url_strips_trailing_slash() -> None: def test_normalize_client_api_url_rejects_missing_value() -> None: - """Missing or empty URL raises ``ValueError`` with the client name.""" + """Missing or empty URL raises ``GlpiValidationError`` with the client name. - with pytest.raises(ValueError, match="X requires glpi_api_url"): + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ + + with pytest.raises(GlpiValidationError, match="X requires glpi_api_url") as exc1: normalize_client_api_url(None, client_name="X") - with pytest.raises(ValueError, match="X requires glpi_api_url"): + assert isinstance(exc1.value, ValueError) + with pytest.raises(GlpiValidationError, match="X requires glpi_api_url") as exc2: normalize_client_api_url("", client_name="X") - with pytest.raises(ValueError, match="X requires glpi_api_url"): + assert isinstance(exc2.value, ValueError) + with pytest.raises(GlpiValidationError, match="X requires glpi_api_url") as exc3: normalize_client_api_url(123, client_name="X") # type: ignore[arg-type] + assert isinstance(exc3.value, ValueError) def test_validate_v1_document_config_rejects_partial_pair() -> None: - """Either both v1 values are present or both are absent.""" + """Either both v1 values are present or both are absent. - with pytest.raises(ValueError, match="v1_base_url and v1_user_token"): + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ + + with pytest.raises( + GlpiValidationError, match="v1_base_url and v1_user_token" + ) as exc1: validate_v1_document_config(v1_base_url="https://x", v1_user_token=None) - with pytest.raises(ValueError, match="v1_base_url and v1_user_token"): + assert isinstance(exc1.value, ValueError) + with pytest.raises( + GlpiValidationError, match="v1_base_url and v1_user_token" + ) as exc2: validate_v1_document_config(v1_base_url=None, v1_user_token="t") + assert isinstance(exc2.value, ValueError) def test_validate_v1_document_config_allows_complete_pair() -> None: @@ -74,6 +91,20 @@ def test_parse_optional_env_int_rejects_other_types() -> None: parse_optional_env_int(1.5) +def test_parse_optional_env_int_rejects_unparseable_string() -> None: + """A non-numeric string (e.g. ``GLPI_TIMEOUT=abc``) raises ``GlpiValidationError``. + + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working, and the original ``int()`` + ``ValueError`` is chained via ``from`` rather than swallowed. + """ + + with pytest.raises(GlpiValidationError, match="abc") as excinfo: + parse_optional_env_int("abc") + assert isinstance(excinfo.value, ValueError) + assert isinstance(excinfo.value.__cause__, ValueError) + + @pytest.mark.parametrize( ("value", "default", "expected"), [ @@ -97,10 +128,15 @@ def test_parse_optional_env_bool_truthy_and_falsy( def test_parse_optional_env_bool_rejects_unknown_string() -> None: - """Unknown strings raise ``ValueError``.""" + """Unknown strings raise ``GlpiValidationError``. - with pytest.raises(ValueError): + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ + + with pytest.raises(GlpiValidationError) as excinfo: parse_optional_env_bool("maybe", default=False) + assert isinstance(excinfo.value, ValueError) def test_parse_optional_env_bool_rejects_other_types() -> None: diff --git a/glpi_python_client/clients/tests/test_raise_site_audit.py b/glpi_python_client/clients/tests/test_raise_site_audit.py new file mode 100644 index 0000000..b599a9f --- /dev/null +++ b/glpi_python_client/clients/tests/test_raise_site_audit.py @@ -0,0 +1,112 @@ +"""Audit every raise statement in library code against the error contract. + +This is a structural guard, not a behavioural one. It exists because the +0.4.0 error migration is a mechanical sweep across raise sites in +non-test library code -- as of this writing 24 ``GlpiValidationError``, +9 ``GlpiProtocolError``, 4 ``RuntimeError``, 2 ``TypeError``, +1 ``GlpiServerError``, plus 2 ``status_error_class(...)`` dispatch sites +(42 total) -- and a missed one is invisible: a bare ValueError still +passes every existing ``pytest.raises(ValueError)`` test. + +The RuntimeError and TypeError sites are deliberately exempt. Converting +them to GlpiValidationError -- which inherits ValueError, not TypeError -- +would silently break ``except TypeError`` / ``except RuntimeError`` in user +code and the 12 tests in this repo that assert on those two types. See +plan-1 decision D3. +""" + +from __future__ import annotations + +import ast +import pathlib + +_PACKAGE_ROOT = pathlib.Path(__file__).resolve().parents[2] + +_ALLOWED = { + "GlpiValidationError", + "GlpiProtocolError", + "GlpiServerError", + "GlpiStatusError", + "error_class", # status_error_class(...) dispatch result + "RuntimeError", # exempt by design -- see module docstring + "TypeError", # exempt by design -- see module docstring +} + + +def _library_modules() -> list[pathlib.Path]: + """Return every non-test, non-testing module in the package.""" + + return [ + path + for path in sorted(_PACKAGE_ROOT.rglob("*.py")) + if "tests" not in path.parts + and "testing" not in path.parts + and not path.name.startswith("test_") + ] + + +def _raise_sites() -> list[tuple[str, int, str]]: + """Return ``(module, lineno, exception_name)`` for every raise statement.""" + + sites: list[tuple[str, int, str]] = [] + for path in _library_modules(): + tree = ast.parse(path.read_text(encoding="utf-8")) + for node in ast.walk(tree): + if not isinstance(node, ast.Raise) or node.exc is None: + continue + exc = node.exc + name = ( + ast.unparse(exc.func) if isinstance(exc, ast.Call) else ast.unparse(exc) + ) + sites.append( + (path.relative_to(_PACKAGE_ROOT).as_posix(), node.lineno, name) + ) + return sites + + +def test_the_ast_walk_finds_a_known_raise_site() -> None: + """Positive control: prove the walk actually finds raise statements. + + If ``_raise_sites`` ever silently returned ``[]`` (e.g. because + ``_library_modules`` globbed the wrong root, or the AST walk predicate + stopped matching ``ast.Raise`` nodes), every other test in this module + would pass vacuously. Pin that at least one known-good, never-removed + raise site is found so a broken walk fails loudly instead. + """ + + sites = _raise_sites() + assert sites, "the raise-site walk found nothing -- it is broken" + # clients/commons/_transport.py:106 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" + ] + assert transport_sites, ( + "the raise-site walk did not find the known RuntimeError raise in " + "clients/commons/_transport.py -- it is not actually walking the " + "package" + ) + + +def test_no_bare_value_error_is_raised_by_library_code() -> None: + """Every caller-facing error is typed; bare ``ValueError`` is gone.""" + + offenders = [site for site in _raise_sites() if site[2] == "ValueError"] + 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.""" + + offenders = [site for site in _raise_sites() if site[2].startswith("requests.")] + assert offenders == [], f"requests exception raise sites remain: {offenders}" + + +def test_every_raise_site_uses_an_allowed_exception() -> None: + """No raise site drifts outside the documented error contract.""" + + offenders = [site for site in _raise_sites() if site[2] not in _ALLOWED] + assert offenders == [], f"unexpected raise sites: {offenders}" diff --git a/glpi_python_client/testing/utils.py b/glpi_python_client/testing/utils.py index 76d6ff1..a939225 100644 --- a/glpi_python_client/testing/utils.py +++ b/glpi_python_client/testing/utils.py @@ -35,6 +35,7 @@ def __init__( text: str | None = None, content: bytes | None = None, reason: str = "", + url: str = "https://glpi.example.test/api.php/fake", ) -> None: self.status_code = status_code self._payload = {"id": 321} if payload is None else payload @@ -42,6 +43,7 @@ def __init__( self.text = str(self._payload) if text is None else text self.content = self.text.encode() if content is None else content self.reason = reason + self.url = url def json(self) -> Any: return self._payload diff --git a/glpi_python_client/tests/__init__.py b/glpi_python_client/tests/__init__.py new file mode 100644 index 0000000..4461fb3 --- /dev/null +++ b/glpi_python_client/tests/__init__.py @@ -0,0 +1 @@ +"""Tests for the package-root modules.""" diff --git a/glpi_python_client/tests/test_errors.py b/glpi_python_client/tests/test_errors.py new file mode 100644 index 0000000..8b5d699 --- /dev/null +++ b/glpi_python_client/tests/test_errors.py @@ -0,0 +1,134 @@ +"""Unit tests for the public exception hierarchy.""" + +from __future__ import annotations + +import copy +import pickle + +import pytest + +from glpi_python_client import ( + GlpiAuthError, + GlpiError, + GlpiNotFoundError, + GlpiProtocolError, + GlpiServerError, + GlpiStatusError, + GlpiTimeoutError, + GlpiTransportError, + GlpiValidationError, +) +from glpi_python_client._errors import status_error_class + + +def test_every_public_error_derives_from_glpi_error() -> None: + """One ``except GlpiError`` catches the whole library surface.""" + + for cls in ( + GlpiTransportError, + GlpiTimeoutError, + GlpiStatusError, + GlpiAuthError, + GlpiNotFoundError, + GlpiServerError, + GlpiValidationError, + GlpiProtocolError, + ): + assert issubclass(cls, GlpiError) + + +def test_timeout_is_a_transport_error() -> None: + """``GlpiTimeoutError`` narrows ``GlpiTransportError``.""" + + assert issubclass(GlpiTimeoutError, GlpiTransportError) + + +def test_transport_errors_are_not_value_errors() -> None: + """A transport fault is not a caller mistake, so it is not a ``ValueError``.""" + + assert not issubclass(GlpiTransportError, ValueError) + + +@pytest.mark.parametrize( + "cls", [GlpiStatusError, GlpiValidationError, GlpiProtocolError] +) +def test_value_error_back_compat(cls: type[Exception]) -> None: + """Callers written against the old bare-``ValueError`` contract keep working.""" + + assert issubclass(cls, ValueError) + + +def test_status_error_carries_diagnostics_and_preserves_message() -> None: + """``GlpiStatusError`` exposes status/url/body and keeps ``str(e)`` intact.""" + + error = GlpiStatusError( + "Failed to fetch ticket 1: 404 nope", + status_code=404, + url="https://glpi.example.test/api.php/Assistance/Ticket/1", + response_text="nope", + ) + assert error.status_code == 404 + assert error.url == "https://glpi.example.test/api.php/Assistance/Ticket/1" + assert error.response_text == "nope" + assert str(error) == "Failed to fetch ticket 1: 404 nope" + + +def test_status_error_response_text_defaults_to_empty() -> None: + """``response_text`` is optional.""" + + error = GlpiStatusError("boom", status_code=500, url="https://x") + assert error.response_text == "" + + +def test_status_error_is_catchable_as_value_error_with_match() -> None: + """Legacy ``pytest.raises(ValueError, match=...)`` assertions still fire.""" + + with pytest.raises(ValueError, match="404 nope"): + raise GlpiNotFoundError( + "Failed to fetch ticket 1: 404 nope", + status_code=404, + url="https://x", + response_text="nope", + ) + + +def test_status_error_survives_pickle_round_trip() -> None: + """Keyword-only arguments do not break ``pickle`` (used across processes).""" + + error = GlpiServerError( + "boom", status_code=503, url="https://x", response_text="down" + ) + restored = pickle.loads(pickle.dumps(error)) + assert isinstance(restored, GlpiServerError) + assert restored.status_code == 503 + assert restored.url == "https://x" + assert restored.response_text == "down" + assert str(restored) == "boom" + + +def test_status_error_survives_copy() -> None: + """``copy.copy`` uses the same reduce protocol as pickle.""" + + error = GlpiAuthError("nope", status_code=401, url="https://x") + assert copy.copy(error).status_code == 401 + + +@pytest.mark.parametrize( + ("status_code", "expected"), + [ + (401, GlpiAuthError), + (403, GlpiAuthError), + (404, GlpiNotFoundError), + (500, GlpiServerError), + (503, GlpiServerError), + (599, GlpiServerError), + (418, GlpiStatusError), + (400, GlpiStatusError), + ], +) +def test_status_error_class_dispatch( + status_code: int, expected: type[GlpiStatusError] +) -> None: + """``status_error_class`` maps each status band to its narrowest class.""" + + assert status_error_class(status_code) is expected diff --git a/integration_tests/test_integration.py b/integration_tests/test_integration.py index d6101bf..ea0de46 100644 --- a/integration_tests/test_integration.py +++ b/integration_tests/test_integration.py @@ -17,6 +17,7 @@ from glpi_python_client import ( GlpiClient, + GlpiStatusError, GlpiTicketContext, PostFollowup, PostLocation, @@ -26,6 +27,7 @@ PostTicketTask, PostUser, ) +from glpi_python_client.models.api_schema.plugins import GetPluginFieldsContainer pytestmark = pytest.mark.integration @@ -467,21 +469,39 @@ def test_iter_search_entities_yields_batches(client: GlpiClient) -> None: def test_iter_search_tickets_multi_page(client: GlpiClient) -> None: - """iter_search_tickets paginates correctly when batch_size forces multiple pages. + """iter_search_tickets advances ``start`` across successive pages. - Uses a small batch size (3) so that any instance with more than - three tickets exercises the multi-page code path. + Uses a small batch size (3) so any instance with more than three + tickets exercises the multi-page code path, and stops after a few + pages: a real instance holds tens of thousands of tickets, so an + unbounded walk would issue ~20k requests and run for hours. + + ``status.id==1`` is the filter GLPI actually honours -- the v2 contract + types ``status`` as an object, so a bare ``status==1`` is silently + dropped and the search degrades to "every ticket". """ from glpi_python_client.models.api_schema.assistance._ticket import GetTicket + max_pages = 3 collected: list[GetTicket] = [] - for batch in client.iter_search_tickets("status==1", batch_size=3): + pages = 0 + for batch in client.iter_search_tickets("status.id==1", batch_size=3): assert isinstance(batch, list) assert len(batch) <= 3 collected.extend(batch) + pages += 1 + if pages >= max_pages: + break + + if pages < 2: + pytest.skip("instance has fewer than two pages of tickets to paginate") - assert isinstance(collected, list) + # Distinct ids across pages prove the offset advanced; a stuck ``start`` + # would re-yield the first page forever, which the old unbounded loop + # could not have detected. + ids = [t.id for t in collected] + assert len(set(ids)) == len(ids) # --------------------------------------------------------------------------- @@ -652,25 +672,52 @@ def test_get_user_activity_raises_without_identifier(client: GlpiClient) -> None # # 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 (no container attached to ``Ticket``) -# the tests skip cleanly so the suite stays portable across instances. +# When the plugin is not installed the tests skip cleanly so the suite stays +# portable across instances. # --------------------------------------------------------------------------- _FIELDS_TEST_TICKET_ID = 62571 +# 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 +# like from the outside. An installed-but-empty plugin returns 200 and []. +_FIELDS_ITEMTYPE_UNKNOWN = "ERROR_RESOURCE_NOT_FOUND_NOR_COMMONDBTM" + def _skip_when_no_v1(live_config: _LiveGlpiConfig) -> None: if not live_config.v1_base_url or not live_config.v1_user_token: pytest.skip("live GLPI v1 credentials not configured") -def test_plugin_fields_containers_discovery( +@pytest.fixture +def fields_containers( client: GlpiClient, live_config: _LiveGlpiConfig +) -> list[GetPluginFieldsContainer]: + """Return Ticket containers, skipping when the Fields plugin is absent. + + Credentials being configured says nothing about the plugin being + installed: without it GLPI rejects the ``PluginFieldsContainer`` + itemtype outright rather than returning an empty list. Only that exact + signature skips -- any other status error is a real failure. + """ + + _skip_when_no_v1(live_config) + try: + return client.list_plugin_fields_containers(itemtype="Ticket") + except GlpiStatusError as exc: + if exc.status_code == 400 and _FIELDS_ITEMTYPE_UNKNOWN in ( + exc.response_text or "" + ): + pytest.skip("GLPI Fields plugin is not installed on this instance") + raise + + +def test_plugin_fields_containers_discovery( + client: GlpiClient, fields_containers: list[GetPluginFieldsContainer] ) -> None: """Discover Ticket-attached Fields plugin containers on the live instance.""" - _skip_when_no_v1(live_config) - containers = client.list_plugin_fields_containers(itemtype="Ticket") + containers = fields_containers if not containers: pytest.skip("no PluginFieldsContainer attached to Ticket on this instance") for container in containers: @@ -683,7 +730,7 @@ def test_plugin_fields_containers_discovery( def test_get_ticket_custom_fields_round_trip_on_known_ticket( - client: GlpiClient, live_config: _LiveGlpiConfig + client: GlpiClient, fields_containers: list[GetPluginFieldsContainer] ) -> None: """Round-trip the ``aidelarsolution`` custom field on ticket 62571. @@ -691,8 +738,7 @@ def test_get_ticket_custom_fields_round_trip_on_known_ticket( read back, and finally restored so the test is net-zero. """ - _skip_when_no_v1(live_config) - containers = client.list_plugin_fields_containers(itemtype="Ticket") + containers = fields_containers container = next((c for c in containers if c.name == "aidelarsolution"), None) if container is None: pytest.skip("'aidelarsolution' container missing on this instance") @@ -721,12 +767,12 @@ def test_get_ticket_custom_fields_round_trip_on_known_ticket( ) +@pytest.mark.usefixtures("fields_containers") def test_set_ticket_custom_fields_rejects_unknown_container( - client: GlpiClient, live_config: _LiveGlpiConfig + client: GlpiClient, ) -> None: """Writing to a non-existent container raises before any HTTP call.""" - _skip_when_no_v1(live_config) with pytest.raises(ValueError, match="Unknown plugin-fields container"): client.set_ticket_custom_fields( _FIELDS_TEST_TICKET_ID, diff --git a/integration_tests/test_integration_async.py b/integration_tests/test_integration_async.py index 6e25a6c..39a9d75 100644 --- a/integration_tests/test_integration_async.py +++ b/integration_tests/test_integration_async.py @@ -25,7 +25,6 @@ import pytest import pytest_asyncio -import requests from test_integration import ( # noqa: F401 _LiveGlpiConfig, _suffix, @@ -34,6 +33,7 @@ from glpi_python_client import ( AsyncGlpiClient, + GlpiNotFoundError, GlpiTicketContext, PostFollowup, PostLocation, @@ -267,16 +267,15 @@ async def test_cancellation_releases_awaiter( async def test_exception_propagates_from_worker_thread( async_client: AsyncGlpiClient, ) -> None: - """Errors raised on the worker thread propagate to the awaiter unchanged. + """A missing ticket surfaces as a typed GLPI error. We trigger one by reading a ticket id that almost certainly does not exist. The async client must surface the same - :class:`requests.HTTPError` (or any - :class:`requests.RequestException` subclass after retry) that the - sync client would raise. + :class:`~glpi_python_client.GlpiNotFoundError` the sync client + would raise — users never import the HTTP library to catch it. """ - with pytest.raises((requests.RequestException, ValueError)): + with pytest.raises(GlpiNotFoundError): await async_client.get_ticket(2**31 - 1)