From 5ed682523eec03804b5d44efd62dcf7763b179bf Mon Sep 17 00:00:00 2001 From: baraline Date: Wed, 15 Jul 2026 15:26:36 +0200 Subject: [PATCH 01/20] test(async): guard against the bridge self-call trap --- .../tests/test_async_selfcall_guard.py | 158 ++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 glpi_python_client/clients/tests/test_async_selfcall_guard.py 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..9964d24 --- /dev/null +++ b/glpi_python_client/clients/tests/test_async_selfcall_guard.py @@ -0,0 +1,158 @@ +"""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"} + +# Methods known to violate the rule, removed as each is fixed. Must reach +# empty; a non-empty set here is a shipped bug, not an accepted state. +_KNOWN_UNCOVERED: frozenset[str] = frozenset( + { + "create_kb_article", + "get_ticket_custom_fields", + "set_ticket_custom_fields", + "update_kb_article", + } +) + + +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 set(_offenders()) == set(_KNOWN_UNCOVERED), ( + "The set of bridge self-call offenders changed.\n" + f" found: {sorted(_offenders())}\n" + f" expected: {sorted(_KNOWN_UNCOVERED)}\n" + "A NEW name means a sync body calls a public method through self " + "with no async override: it will silently drop that call on " + "AsyncGlpiClient. Add an async override mixin (see " + "clients/custom/_statistics_async.py) and remove the name here." + ) + + +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" + ) From dee8c0fe4d1d62351ffc2c4df6ac9e8c48f94ee2 Mon Sep 17 00:00:00 2001 From: baraline Date: Wed, 15 Jul 2026 16:38:25 +0200 Subject: [PATCH 02/20] fix(async): make the Fields plugin helpers work on AsyncGlpiClient --- glpi_python_client/clients/api/__init__.py | 6 +- .../clients/api/plugins/__init__.py | 3 +- .../clients/api/plugins/_fields_async.py | 143 ++++++++++++++++++ .../api/plugins/tests/test_fields_async.py | 98 ++++++++++++ glpi_python_client/clients/async_client.py | 4 +- .../tests/test_async_selfcall_guard.py | 2 - 6 files changed, 250 insertions(+), 6 deletions(-) create mode 100644 glpi_python_client/clients/api/plugins/_fields_async.py create mode 100644 glpi_python_client/clients/api/plugins/tests/test_fields_async.py diff --git a/glpi_python_client/clients/api/__init__.py b/glpi_python_client/clients/api/__init__.py index 8c43571..6028a88 100644 --- a/glpi_python_client/clients/api/__init__.py +++ b/glpi_python_client/clients/api/__init__.py @@ -31,9 +31,13 @@ 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__ = [ + "AsyncPluginFieldsMixin", "DocumentMixin", "EntityMixin", "FollowupMixin", 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_async.py b/glpi_python_client/clients/api/plugins/_fields_async.py new file mode 100644 index 0000000..102271d --- /dev/null +++ b/glpi_python_client/clients/api/plugins/_fields_async.py @@ -0,0 +1,143 @@ +"""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. +""" + +from __future__ import annotations + +from typing import Any + +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 ValueError( + "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( + 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 ValueError( + 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..ce4bfec --- /dev/null +++ b/glpi_python_client/clients/api/plugins/tests/test_fields_async.py @@ -0,0 +1,98 @@ +"""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 +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.""" + + client._v1 = _FakeV1( # type: ignore[assignment] + [[{"id": 1, "name": "custom", "itemtypes": '["Ticket"]'}]] + ) + + with pytest.raises(ValueError, match="Unknown plugin-fields container"): + await client.set_ticket_custom_fields(1, {"nope": {"x": 1}}) diff --git a/glpi_python_client/clients/async_client.py b/glpi_python_client/clients/async_client.py index 4cc34fd..e0ee172 100644 --- a/glpi_python_client/clients/async_client.py +++ b/glpi_python_client/clients/async_client.py @@ -32,6 +32,7 @@ from glpi_python_client.clients._base_client import _BaseGlpiClient from glpi_python_client.clients.api import ( + AsyncPluginFieldsMixin, DocumentMixin, EntityMixin, FollowupMixin, @@ -40,7 +41,6 @@ KBArticleRevisionMixin, KBCategoryMixin, LocationMixin, - PluginFieldsMixin, SolutionMixin, TeamMemberMixin, TicketMixin, @@ -76,7 +76,7 @@ class AsyncGlpiClient( # type: ignore[misc] KBArticleMixin, KBArticleCommentMixin, KBArticleRevisionMixin, - PluginFieldsMixin, + AsyncPluginFieldsMixin, AsyncTicketContextMixin, AsyncStatisticsMixin, _BaseGlpiClient, diff --git a/glpi_python_client/clients/tests/test_async_selfcall_guard.py b/glpi_python_client/clients/tests/test_async_selfcall_guard.py index 9964d24..7927859 100644 --- a/glpi_python_client/clients/tests/test_async_selfcall_guard.py +++ b/glpi_python_client/clients/tests/test_async_selfcall_guard.py @@ -27,8 +27,6 @@ _KNOWN_UNCOVERED: frozenset[str] = frozenset( { "create_kb_article", - "get_ticket_custom_fields", - "set_ticket_custom_fields", "update_kb_article", } ) From 87c7deaec96d7cbc430d06827e34ebab0072793e Mon Sep 17 00:00:00 2001 From: baraline Date: Wed, 15 Jul 2026 17:10:00 +0200 Subject: [PATCH 03/20] fix(async): stop silently dropping KB article category assignment --- glpi_python_client/clients/api/__init__.py | 2 + .../clients/api/knowledgebase/__init__.py | 4 + .../api/knowledgebase/_article_async.py | 151 ++++++++++++++++++ .../knowledgebase/tests/test_article_async.py | 129 +++++++++++++++ glpi_python_client/clients/async_client.py | 4 +- .../tests/test_async_selfcall_guard.py | 68 ++++++-- 6 files changed, 339 insertions(+), 19 deletions(-) create mode 100644 glpi_python_client/clients/api/knowledgebase/_article_async.py create mode 100644 glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py diff --git a/glpi_python_client/clients/api/__init__.py b/glpi_python_client/clients/api/__init__.py index 6028a88..4f7237b 100644 --- a/glpi_python_client/clients/api/__init__.py +++ b/glpi_python_client/clients/api/__init__.py @@ -25,6 +25,7 @@ ) from glpi_python_client.clients.api.dropdowns import LocationMixin from glpi_python_client.clients.api.knowledgebase import ( + AsyncKBArticleMixin, KBArticleCommentMixin, KBArticleMixin, KBArticleRevisionMixin, @@ -37,6 +38,7 @@ ) __all__ = [ + "AsyncKBArticleMixin", "AsyncPluginFieldsMixin", "DocumentMixin", "EntityMixin", diff --git a/glpi_python_client/clients/api/knowledgebase/__init__.py b/glpi_python_client/clients/api/knowledgebase/__init__.py index 2bed9e8..f337d9c 100644 --- a/glpi_python_client/clients/api/knowledgebase/__init__.py +++ b/glpi_python_client/clients/api/knowledgebase/__init__.py @@ -3,6 +3,9 @@ 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_async.py b/glpi_python_client/clients/api/knowledgebase/_article_async.py new file mode 100644 index 0000000..47ce76f --- /dev/null +++ b/glpi_python_client/clients/api/knowledgebase/_article_async.py @@ -0,0 +1,151 @@ +"""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.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 + ------ + ValueError + 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 ValueError( + "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/tests/test_article_async.py b/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py new file mode 100644 index 0000000..a3a725e --- /dev/null +++ b/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py @@ -0,0 +1,129 @@ +"""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, 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.""" + + c = make_async_client() + + def _post( + endpoint: str, + json_body: dict[str, object] | None = None, + skip_entity: bool = False, + ) -> FakeResponse: + return FakeResponse(status_code=201, payload={"id": 42}) + + def _patch( + endpoint: str, json_body: dict[str, object] | None = None + ) -> FakeResponse: + return FakeResponse(status_code=200, payload={"id": 42}) + + c._post_request = _post # type: ignore[assignment] + c._update_request = _patch # type: ignore[assignment] + 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]}}, + } + ] + + +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_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[-1]["json_body"] == {"input": {"_categories": []}} + + +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/async_client.py b/glpi_python_client/clients/async_client.py index e0ee172..506f216 100644 --- a/glpi_python_client/clients/async_client.py +++ b/glpi_python_client/clients/async_client.py @@ -32,12 +32,12 @@ 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, @@ -73,7 +73,7 @@ class AsyncGlpiClient( # type: ignore[misc] EntityMixin, LocationMixin, KBCategoryMixin, - KBArticleMixin, + AsyncKBArticleMixin, KBArticleCommentMixin, KBArticleRevisionMixin, AsyncPluginFieldsMixin, diff --git a/glpi_python_client/clients/tests/test_async_selfcall_guard.py b/glpi_python_client/clients/tests/test_async_selfcall_guard.py index 7927859..b962e84 100644 --- a/glpi_python_client/clients/tests/test_async_selfcall_guard.py +++ b/glpi_python_client/clients/tests/test_async_selfcall_guard.py @@ -22,15 +22,6 @@ # Lifecycle helpers differ between the surfaces on purpose. _EXCLUDED = {"from_env", "close"} -# Methods known to violate the rule, removed as each is fixed. Must reach -# empty; a non-empty set here is a shipped bug, not an accepted state. -_KNOWN_UNCOVERED: frozenset[str] = frozenset( - { - "create_kb_article", - "update_kb_article", - } -) - def _public_names(cls: type) -> set[str]: """Return the public callable names exposed by ``cls``.""" @@ -129,14 +120,12 @@ def _offenders() -> list[str]: def test_no_new_self_call_offenders() -> None: """No public method may self-call without a hand-written async override.""" - assert set(_offenders()) == set(_KNOWN_UNCOVERED), ( - "The set of bridge self-call offenders changed.\n" - f" found: {sorted(_offenders())}\n" - f" expected: {sorted(_KNOWN_UNCOVERED)}\n" - "A NEW name means a sync body calls a public method through self " - "with no async override: it will silently drop that call on " - "AsyncGlpiClient. Add an async override mixin (see " - "clients/custom/_statistics_async.py) and remove the name here." + 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." ) @@ -154,3 +143,48 @@ def test_guard_detects_the_covered_methods() -> None: 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" + ) From b3f5d70c512e41f9ee7ed15deebd2df39944c849 Mon Sep 17 00:00:00 2001 From: baraline Date: Wed, 15 Jul 2026 17:52:30 +0200 Subject: [PATCH 04/20] fix(async): close the self-call guard's vacuity holes and doc drift Address code review findings on the KB article async-bridge fix: add an end-to-end canary pinning _public_names, _self_call_map, and _reaches_public together against the real create_kb_article shape (the synthetic pinning test alone couldn't catch either helper going silently empty); cover the untested ValueError branch for a category ref without an id on both the create (wrapped into RuntimeError) and update (raw ValueError) paths; capture and assert the v2 request body on create so a wider model_copy strip would be caught; switch the update test to whole-list call equality to catch a double-write; and correct two stale docstrings (the knowledgebase package docstring and the executor parameter, which no longer covers the hand-written KB article overrides that use asyncio.to_thread directly). Co-Authored-By: Claude Opus 4.8 --- .../clients/api/knowledgebase/__init__.py | 2 +- .../knowledgebase/tests/test_article_async.py | 66 ++++++++++++++++++- glpi_python_client/clients/async_client.py | 17 +++-- .../tests/test_async_selfcall_guard.py | 36 ++++++++++ 4 files changed, 113 insertions(+), 8 deletions(-) diff --git a/glpi_python_client/clients/api/knowledgebase/__init__.py b/glpi_python_client/clients/api/knowledgebase/__init__.py index f337d9c..89ee53a 100644 --- a/glpi_python_client/clients/api/knowledgebase/__init__.py +++ b/glpi_python_client/clients/api/knowledgebase/__init__.py @@ -1,4 +1,4 @@ -"""GLPI ``/Knowledgebase`` mixins for the Synchronous client.""" +"""GLPI ``/Knowledgebase`` mixins for the synchronous and asynchronous clients.""" from __future__ import annotations 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 index a3a725e..dc5b7a9 100644 --- a/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py +++ b/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py @@ -40,24 +40,36 @@ def request_json( @pytest.fixture def client() -> AsyncGlpiClient: - """Return an async client with the v2 transport stubbed out.""" + """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 @@ -79,6 +91,11 @@ async def test_create_kb_article_links_categories(client: AsyncGlpiClient) -> No "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( @@ -105,6 +122,26 @@ async def test_create_kb_article_wraps_category_failure( ) +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_clears_categories(client: AsyncGlpiClient) -> None: """An empty list clears every category through the v1 fallback.""" @@ -113,7 +150,32 @@ async def test_update_kb_article_clears_categories(client: AsyncGlpiClient) -> N await client.update_kb_article(42, PatchKBArticle(name="t2", categories=[])) - assert fake.calls[-1]["json_body"] == {"input": {"_categories": []}} + assert fake.calls == [ + { + "method": "PUT", + "path": "KnowbaseItem/42", + "json_body": {"input": {"_categories": []}}, + } + ] + + +async def test_update_kb_article_raises_on_missing_id_category( + client: AsyncGlpiClient, +) -> None: + """A category ref without an id raises the raw ``ValueError`` 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 + ``ValueError`` from ``_apply_category_fallback_async`` must therefore + propagate unchanged. + """ + + with pytest.raises(ValueError, match="require an 'id'"): + await client.update_kb_article( + 42, PatchKBArticle(name="t2", categories=[IdNameRef(name="cat")]) + ) async def test_update_kb_article_without_categories_skips_v1( diff --git a/glpi_python_client/clients/async_client.py b/glpi_python_client/clients/async_client.py index 506f216..4485b76 100644 --- a/glpi_python_client/clients/async_client.py +++ b/glpi_python_client/clients/async_client.py @@ -101,13 +101,20 @@ 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`. diff --git a/glpi_python_client/clients/tests/test_async_selfcall_guard.py b/glpi_python_client/clients/tests/test_async_selfcall_guard.py index b962e84..42fa6a2 100644 --- a/glpi_python_client/clients/tests/test_async_selfcall_guard.py +++ b/glpi_python_client/clients/tests/test_async_selfcall_guard.py @@ -188,3 +188,39 @@ def test_reaches_public_detects_transitive_and_direct_self_calls() -> None: 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" From c677290e54ae3536e795848d4af4fc27c99e5a6a Mon Sep 17 00:00:00 2001 From: baraline Date: Wed, 15 Jul 2026 18:16:09 +0200 Subject: [PATCH 05/20] docs: add CHANGELOG with the async bridge fixes under Unreleased Document the two async-bridge self-call bugs fixed on this branch (silent KB category loss, and set/get_ticket_custom_fields raising TypeError) and the structural AST guard added to prevent regressions, plus the shared root cause and the plan to remove AsyncBridge entirely in 0.4.0. No version bump: this branch merges to main unreleased. Also closes an assertion gap in test_update_kb_article_clears_categories: patch_bodies was captured by the fixture but never asserted, so a model_copy regression that stripped more than categories from the v2 patch body would have gone unnoticed on the update path even though the symmetric create path was already covered. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 48 +++++++++++++++++++ .../knowledgebase/tests/test_article_async.py | 5 ++ 2 files changed, 53 insertions(+) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..80dcc40 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,48 @@ +# 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 + +- `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`. + +### 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 `self` + 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. + +### 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/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py b/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py index dc5b7a9..241d2db 100644 --- a/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py +++ b/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py @@ -157,6 +157,11 @@ async def test_update_kb_article_clears_categories(client: AsyncGlpiClient) -> N "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( From defd91a063ab4abe9dea9fc7bfb9428f82c67947 Mon Sep 17 00:00:00 2001 From: baraline Date: Wed, 15 Jul 2026 19:03:07 +0200 Subject: [PATCH 06/20] docs: fix final review findings on async-bridge-self-calls Corrects five minor findings from the whole-branch review: - CLAUDE.md and docs/development.md's "single source of truth" rule said hand-crafted async overrides are warranted only for asyncio.gather fan-out; add the self-call trap as the second, independent reason (the actual cause of both bugs this branch fixed), and point at the new AST guard. - async_client.py's module/class docstrings still said async overrides exist only for fan-out and only live under clients.custom; both are now also true of clients.api.knowledgebase and clients.api.plugins, for the self-call reason. - _fields_async.py's `type: ignore[misc]` calls had no explanation; add the same rationale _article_async.py already documents, and contrast with the `[attr-defined]` codes in _statistics_async.py. - CHANGELOG.md described the guard as detecting any self-call "transitively... through self"; narrow to the literal `self.name(...)` shape it actually matches (getattr/aliasing/ partial/super()/property getters are invisible to it). - Add test_update_kb_article_links_categories, the missing direct test for update_kb_article with a non-empty category list - previously only covered by composition of the create and clear-categories tests. No behavior changes; docs, docstrings, comments, and one new test. --- CHANGELOG.md | 3 +- docs/development.md | 30 +++++++++++++---- .../knowledgebase/tests/test_article_async.py | 32 +++++++++++++++++++ .../clients/api/plugins/_fields_async.py | 13 ++++++++ glpi_python_client/clients/async_client.py | 18 +++++++---- 5 files changed, 81 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80dcc40..eaa7d89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,7 +29,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). - `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 `self` + `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 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/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py b/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py index 241d2db..889c249 100644 --- a/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py +++ b/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py @@ -142,6 +142,38 @@ async def test_create_kb_article_wraps_missing_id_category( ) +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.""" diff --git a/glpi_python_client/clients/api/plugins/_fields_async.py b/glpi_python_client/clients/api/plugins/_fields_async.py index 102271d..34f9367 100644 --- a/glpi_python_client/clients/api/plugins/_fields_async.py +++ b/glpi_python_client/clients/api/plugins/_fields_async.py @@ -10,6 +10,19 @@ :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 diff --git a/glpi_python_client/clients/async_client.py b/glpi_python_client/clients/async_client.py index 4485b76..a480751 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`` / @@ -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`; From d1274ec5b26a4a7c70171f20dc4ba297a4237bb9 Mon Sep 17 00:00:00 2001 From: baraline Date: Thu, 16 Jul 2026 11:13:09 +0200 Subject: [PATCH 07/20] feat(errors): add the public GlpiError hierarchy Declares GlpiError and its transport/status/validation/protocol branches and exports them from the package root. Nothing raises them yet. GlpiStatusError, GlpiValidationError and GlpiProtocolError also inherit ValueError so callers written against the previous bare-ValueError contract keep working. --- docs/api_reference.rst | 44 +++++++ glpi_python_client/__init__.py | 20 ++++ glpi_python_client/_errors.py | 151 ++++++++++++++++++++++++ glpi_python_client/tests/__init__.py | 1 + glpi_python_client/tests/test_errors.py | 134 +++++++++++++++++++++ 5 files changed, 350 insertions(+) create mode 100644 glpi_python_client/_errors.py create mode 100644 glpi_python_client/tests/__init__.py create mode 100644 glpi_python_client/tests/test_errors.py diff --git a/docs/api_reference.rst b/docs/api_reference.rst index 7df5bcd..ab2a984 100644 --- a/docs/api_reference.rst +++ b/docs/api_reference.rst @@ -28,6 +28,50 @@ the asynchronous one wraps each synchronous method into a coroutine. :members: :show-inheritance: +Exceptions +---------- + +Every exception raised by the client derives from :class:`GlpiError`. +:class:`GlpiStatusError`, :class:`GlpiValidationError` and +:class:`GlpiProtocolError` also inherit :class:`ValueError` for backwards +compatibility with releases that raised bare ``ValueError``. + +.. 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/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..d3aa7cf --- /dev/null +++ b/glpi_python_client/_errors.py @@ -0,0 +1,151 @@ +"""Public exception hierarchy raised by :mod:`glpi_python_client`. + +Every exception the client raises deliberately derives from :class:`GlpiError`, +so callers can catch the whole library surface with a single ``except`` clause +without importing the underlying HTTP library. + +: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. + + Raised for connection failures, DNS errors, and other network-level + faults where GLPI returned no status code at all. + """ + + +class GlpiTimeoutError(GlpiTransportError): + """The HTTP request exceeded its timeout before GLPI responded.""" + + +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/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 From 9277781093a51142dd1c3e15828f25a0496af133 Mon Sep 17 00:00:00 2001 From: baraline Date: Thu, 16 Jul 2026 12:18:47 +0200 Subject: [PATCH 08/20] feat(errors)!: raise GlpiStatusError for HTTP status, add reraise=True BREAKING CHANGE: a persistent 5xx now raises GlpiServerError instead of tenacity.RetryError. Callers no longer need to dig the real error out of RetryError.last_attempt.exception(). BREAKING CHANGE: unexpected response statuses now raise a GlpiStatusError subclass instead of a bare ValueError. GlpiStatusError inherits ValueError, so existing 'except ValueError' handlers keep working. The retry predicate must gain GlpiServerError in the same commit: 5xx no longer raises a requests exception, so the old predicate would silently stop retrying it. requests.RequestException stays in the predicate because the transport is still requests and network faults are still raised raw. Retry behaviour is unchanged: 5xx retried 3x, 4xx not retried. The 4xx raise deliberately stays in ensure_response_status; moving it into finalize_request_response would flip 7 tolerant search endpoints from returning [] to raising. --- glpi_python_client/auth/_v1_session.py | 25 ++- .../auth/tests/test_v1_session.py | 32 ++-- glpi_python_client/clients/commons/_http.py | 27 +++- .../clients/commons/_transport.py | 13 +- .../commons/tests/test_retry_semantics.py | 144 ++++++++++++++++++ glpi_python_client/testing/utils.py | 2 + 6 files changed, 213 insertions(+), 30 deletions(-) create mode 100644 glpi_python_client/clients/commons/tests/test_retry_semantics.py diff --git a/glpi_python_client/auth/_v1_session.py b/glpi_python_client/auth/_v1_session.py index f278648..2a110d0 100644 --- a/glpi_python_client/auth/_v1_session.py +++ b/glpi_python_client/auth/_v1_session.py @@ -18,8 +18,10 @@ 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). +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. :class:`ValueError` raised by status-code or payload checks does not trigger a retry — client-side or 4xx failures are surfaced immediately. """ @@ -34,6 +36,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 ( ensure_response_status, finalize_request_response, @@ -45,9 +48,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, ) @@ -214,8 +218,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`. """ @@ -315,10 +320,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('/')}" diff --git a/glpi_python_client/auth/tests/test_v1_session.py b/glpi_python_client/auth/tests/test_v1_session.py index db1e0a0..dbd14ea 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 GlpiServerError from glpi_python_client.auth._v1_session import GLPIV1Session from glpi_python_client.testing.utils import FakeResponse @@ -214,7 +214,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 +224,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: @@ -264,7 +268,7 @@ def test_v1_upload_raises_on_unexpected_payload() -> None: 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 +276,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: @@ -408,7 +413,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 +422,11 @@ 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") - inner = excinfo.value.last_attempt.exception() - assert isinstance(inner, requests.HTTPError) + 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_session_token_invalid_marker_triggers_renew() -> None: diff --git a/glpi_python_client/clients/commons/_http.py b/glpi_python_client/clients/commons/_http.py index 4573982..977bed2 100644 --- a/glpi_python_client/clients/commons/_http.py +++ b/glpi_python_client/clients/commons/_http.py @@ -12,6 +12,7 @@ import requests +from glpi_python_client._errors import GlpiServerError, status_error_class from glpi_python_client.clients.commons._constants import RequestParamValue @@ -121,7 +122,12 @@ 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[:200], + ) if response.status_code not in success_statuses: logger.warning( "GLPI %s %s returned %s: %s", @@ -139,15 +145,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[:200]}", + status_code=response.status_code, + url=str(response.url), + response_text=response.text[:200], ) diff --git a/glpi_python_client/clients/commons/_transport.py b/glpi_python_client/clients/commons/_transport.py index 7041b64..031e3ee 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, @@ -236,9 +238,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 +265,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 +291,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, 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..026424b --- /dev/null +++ b/glpi_python_client/clients/commons/tests/test_retry_semantics.py @@ -0,0 +1,144 @@ +"""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() + + +def test_5xx_is_retried_three_times_and_reraises_server_error(client: Any) -> None: + """A persistent 5xx costs 3 attempts and surfaces as ``GlpiServerError``.""" + + 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: + client._get_request("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) + + +def test_4xx_is_not_retried_by_the_transport(client: Any) -> None: + """A 4xx is logged and returned by ``finalize_request_response``, not retried.""" + + 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 = client._get_request("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() == [] + + +def test_network_errors_are_still_retried(client: Any) -> None: + """Real ``requests`` transport faults keep their retry behaviour.""" + + 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): + client._get_request("Assistance/Ticket") + + assert len(attempts) == 3 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 From fbe9126c08a891a833a954199857b87bd41a2c8d Mon Sep 17 00:00:00 2001 From: baraline Date: Thu, 16 Jul 2026 13:29:07 +0200 Subject: [PATCH 09/20] test(errors): pin v1 network retries and fix stale retry docstrings Closes review findings on plan-1 Task 2: adds a committed test that drives a requests.RequestException through the v1 session's retry predicate (previously only the GlpiServerError member was pinned, so plan 3's predicate rewrite could silently drop v1 network retries from 3 attempts to 1 with the suite staying green), corrects stale retry docstrings in _transport.py and the async integration suite that still described the pre-hierarchy requests.HTTPError behaviour, adds a Raises section to finalize_request_response, and parametrizes the v2 transport's attempt-count tests across all four retried verbs instead of GET only. No production behaviour changed. --- .../auth/tests/test_v1_session.py | 45 +++++++++++++++++++ glpi_python_client/clients/commons/_http.py | 5 +++ .../clients/commons/_transport.py | 8 ++-- .../commons/tests/test_retry_semantics.py | 37 +++++++++++---- integration_tests/test_integration_async.py | 14 ++++-- 5 files changed, 93 insertions(+), 16 deletions(-) diff --git a/glpi_python_client/auth/tests/test_v1_session.py b/glpi_python_client/auth/tests/test_v1_session.py index dbd14ea..badfbdd 100644 --- a/glpi_python_client/auth/tests/test_v1_session.py +++ b/glpi_python_client/auth/tests/test_v1_session.py @@ -429,6 +429,51 @@ def test_request_json_retries_on_5xx() -> None: 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") + 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: """An ``ERROR_SESSION_TOKEN_INVALID`` body marker counts as an auth failure.""" diff --git a/glpi_python_client/clients/commons/_http.py b/glpi_python_client/clients/commons/_http.py index 977bed2..0c904ea 100644 --- a/glpi_python_client/clients/commons/_http.py +++ b/glpi_python_client/clients/commons/_http.py @@ -113,6 +113,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() diff --git a/glpi_python_client/clients/commons/_transport.py b/glpi_python_client/clients/commons/_transport.py index 031e3ee..9ea420a 100644 --- a/glpi_python_client/clients/commons/_transport.py +++ b/glpi_python_client/clients/commons/_transport.py @@ -224,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( diff --git a/glpi_python_client/clients/commons/tests/test_retry_semantics.py b/glpi_python_client/clients/commons/tests/test_retry_semantics.py index 026424b..d628f0b 100644 --- a/glpi_python_client/clients/commons/tests/test_retry_semantics.py +++ b/glpi_python_client/clients/commons/tests/test_retry_semantics.py @@ -50,8 +50,17 @@ def client() -> Iterator[Any]: c.close() -def test_5xx_is_retried_three_times_and_reraises_server_error(client: Any) -> None: - """A persistent 5xx costs 3 attempts and surfaces as ``GlpiServerError``.""" +@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] = [] @@ -63,7 +72,7 @@ def _send(method: str, url: str, **kw: Any) -> FakeResponse: client._send_request = _send with pytest.raises(GlpiServerError) as excinfo: - client._get_request("Assistance/Ticket") + getattr(client, method_name)("Assistance/Ticket") assert len(attempts) == 3 assert excinfo.value.status_code == 500 @@ -83,8 +92,13 @@ def test_persistent_5xx_does_not_surface_as_retry_error(client: Any) -> None: assert not isinstance(excinfo.value, tenacity.RetryError) -def test_4xx_is_not_retried_by_the_transport(client: Any) -> None: - """A 4xx is logged and returned by ``finalize_request_response``, not retried.""" +@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] = [] @@ -93,7 +107,7 @@ def _send(method: str, url: str, **kw: Any) -> FakeResponse: return FakeResponse(status_code=404, payload={}, text="nope") client._send_request = _send - response = client._get_request("Assistance/Ticket/1") + response = getattr(client, method_name)("Assistance/Ticket/1") assert len(attempts) == 1 assert response.status_code == 404 @@ -128,8 +142,13 @@ def test_tolerant_search_still_returns_empty_on_4xx(client: Any) -> None: assert client.search_tickets() == [] -def test_network_errors_are_still_retried(client: Any) -> None: - """Real ``requests`` transport faults keep their retry behaviour.""" +@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] = [] @@ -139,6 +158,6 @@ def _send(method: str, url: str, **kw: Any) -> FakeResponse: client._send_request = _send with pytest.raises(requests.ConnectionError): - client._get_request("Assistance/Ticket") + getattr(client, method_name)("Assistance/Ticket") assert len(attempts) == 3 diff --git a/integration_tests/test_integration_async.py b/integration_tests/test_integration_async.py index 6e25a6c..7474555 100644 --- a/integration_tests/test_integration_async.py +++ b/integration_tests/test_integration_async.py @@ -270,10 +270,16 @@ async def test_exception_propagates_from_worker_thread( """Errors raised on the worker thread propagate to the awaiter unchanged. 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. + not exist. The async client must surface the same typed error the + sync client would raise: a 404 status raises + :class:`~glpi_python_client.GlpiNotFoundError` immediately (no + retry), while a persistent 5xx would instead raise + :class:`~glpi_python_client.GlpiServerError` after the transport's + retries are exhausted, and a network fault would raise the real + :class:`requests.RequestException` after its own retries. The + assertion below covers all three: the two GLPI-typed errors both + inherit :class:`ValueError` via + :class:`~glpi_python_client.GlpiStatusError`. """ with pytest.raises((requests.RequestException, ValueError)): From 5c483540e14454cac757aa34fb07536278a5d162 Mon Sep 17 00:00:00 2001 From: baraline Date: Thu, 16 Jul 2026 13:56:03 +0200 Subject: [PATCH 10/20] feat(auth)!: type OAuth token failures and stop retrying 4xx BREAKING CHANGE: a non-2xx OAuth token response now raises GlpiAuthError (401/403), GlpiServerError (5xx) or GlpiStatusError instead of a bare ValueError wrapped in tenacity.RetryError. BREAKING CHANGE: the token retry decorators had no retry= predicate, so they retried every exception -- including the ValueError raised for a rejected credential. A wrong client_secret cost 3 attempts and 6s of sleep. They now use the same predicate as the rest of the library: 5xx retried, 4xx final. Adds the first tests covering a non-2xx token response. --- glpi_python_client/auth/auth.py | 35 ++++++++++--- glpi_python_client/auth/tests/test_auth.py | 59 ++++++++++++++++++++++ 2 files changed, 87 insertions(+), 7 deletions(-) diff --git a/glpi_python_client/auth/auth.py b/glpi_python_client/auth/auth.py index 289207d..a8af700 100644 --- a/glpi_python_client/auth/auth.py +++ b/glpi_python_client/auth/auth.py @@ -11,7 +11,9 @@ 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, status_error_class logger = logging.getLogger(__name__) @@ -223,7 +225,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 +241,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 +259,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)[:200], ) - @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 _refresh_access_token(self) -> None: """Refresh the OAuth2 access token using the stored refresh token. diff --git a/glpi_python_client/auth/tests/test_auth.py b/glpi_python_client/auth/tests/test_auth.py index 2b9fa22..d1f8c16 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 from glpi_python_client.auth.auth import GLPITokenManager from glpi_python_client.testing.utils import FakeResponse, TokenResponse @@ -187,3 +189,60 @@ def test_token_manager_rejects_incomplete_credentials( username=kwargs.get("username"), password=kwargs.get("password"), ) + + +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 From 3352165ecf38a67f86d13da47aa05558b57d5bff Mon Sep 17 00:00:00 2001 From: baraline Date: Thu, 16 Jul 2026 14:27:45 +0200 Subject: [PATCH 11/20] test(auth): pin the refresh-path nested-retry multiplication and cover it Task 3 gave `_refresh_access_token` the same 4xx-fail-fast decorator as `_acquire_token`, but nothing drove ensure_token() through the refresh path, so the decision was only half-verified. Add tests that prime a manager (access_token + refresh_token + expired) so refresh is actually reached, and measure the real attempt counts instead of assuming them: a persistent 401 costs 2 POSTs (refresh + one nested, non-retried _acquire_token call), but a persistent 5xx costs 12 POSTs, because _refresh_access_token falls through to a nested _acquire_token() call on any non-2xx instead of raising directly, and both methods retry GlpiServerError independently (3 outer x (1 + 3 inner) = 12). This predates Task 3 and is not fixed here, only measured and pinned so a future change cannot silently alter it. Also documents the real behavior in _refresh_access_token's Raises section, and adds the missing requests.RequestException coverage for _acquire_token that Task 2's transport tests already had. Co-Authored-By: Claude Opus 4.8 --- glpi_python_client/auth/auth.py | 25 ++++ glpi_python_client/auth/tests/test_auth.py | 127 +++++++++++++++++++++ 2 files changed, 152 insertions(+) diff --git a/glpi_python_client/auth/auth.py b/glpi_python_client/auth/auth.py index a8af700..0128374 100644 --- a/glpi_python_client/auth/auth.py +++ b/glpi_python_client/auth/auth.py @@ -280,6 +280,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. Both this + method's retry decorator and the nested :meth:`_acquire_token` + call's decorator treat ``GlpiServerError`` as retryable, and each + retries independently. A persistent 5xx therefore costs up to + 3 (this method's attempts) x (1 refresh POST + 3 nested acquire + POSTs) = 12 POST requests, not the 3 attempts the retry + configuration alone would suggest. This nested-retry + multiplication predates this decorator and is a known, + measured behavior (see the auth tests), not something this + method's docstring papers over. + 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: diff --git a/glpi_python_client/auth/tests/test_auth.py b/glpi_python_client/auth/tests/test_auth.py index d1f8c16..cec8142 100644 --- a/glpi_python_client/auth/tests/test_auth.py +++ b/glpi_python_client/auth/tests/test_auth.py @@ -246,3 +246,130 @@ def test_oauth_5xx_raises_glpi_server_error_after_retries( 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_multiplies_past_three_attempts( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A persistent 5xx during refresh costs 12 POSTs, not 3. + + KNOWN DEFECT, measured and pinned here (pre-existing, not introduced by + the 4xx-fail-fast change in this task): ``_refresh_access_token`` falls + through to a *nested* ``_acquire_token()`` call on any non-2xx response + instead of raising directly (auth.py:302-303). Both methods are + independently decorated with ``stop_after_attempt(3)``, and + ``GlpiServerError`` is retryable by *both* decorators. A persistent 5xx + therefore costs 3 (this method's attempts) x (1 refresh POST + 3 nested + acquire POSTs) = 12 POST calls -- and ~24s of ``wait_fixed(3)`` sleep in + production -- not the 3 attempts / ~6s the retry configuration alone + would suggest. + + This test pins the measured reality so a future change (e.g. plan 3's + httpx swap) cannot silently alter it. Restructuring the nested-retry + topology itself is a deliberate design change and is out of scope here; + see the plan-1 task-3 report for the full write-up. + """ + + 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) == 12 + + +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 From 98e6f110ecc6611c8fbca6e83428a77eb5af62b1 Mon Sep 17 00:00:00 2001 From: baraline Date: Thu, 16 Jul 2026 14:45:28 +0200 Subject: [PATCH 12/20] feat(errors)!: raise GlpiProtocolError for unusable response bodies BREAKING CHANGE: the 7 sites where GLPI answers 2xx with a body the client cannot use now raise GlpiProtocolError instead of a bare ValueError. GlpiProtocolError inherits ValueError, so existing handlers keep working. These are deliberately not GlpiValidationError: the caller did nothing wrong, the server did. --- glpi_python_client/auth/_v1_session.py | 6 ++-- .../auth/tests/test_v1_session.py | 20 +++++++++---- .../clients/api/plugins/_fields.py | 21 +++++++++---- .../api/plugins/tests/test_fields_mixin.py | 15 ++++++---- glpi_python_client/clients/commons/_http.py | 20 +++++++++++-- .../clients/commons/tests/test_http.py | 30 +++++++++++++++++++ 6 files changed, 91 insertions(+), 21 deletions(-) diff --git a/glpi_python_client/auth/_v1_session.py b/glpi_python_client/auth/_v1_session.py index 2a110d0..21cb499 100644 --- a/glpi_python_client/auth/_v1_session.py +++ b/glpi_python_client/auth/_v1_session.py @@ -36,7 +36,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._errors import GlpiProtocolError, GlpiServerError from glpi_python_client.clients.commons._http import ( ensure_response_status, finalize_request_response, @@ -126,7 +126,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) @@ -401,7 +401,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/tests/test_v1_session.py b/glpi_python_client/auth/tests/test_v1_session.py index badfbdd..81353f2 100644 --- a/glpi_python_client/auth/tests/test_v1_session.py +++ b/glpi_python_client/auth/tests/test_v1_session.py @@ -8,7 +8,7 @@ import pytest import requests -from glpi_python_client import GlpiServerError +from glpi_python_client import GlpiProtocolError, GlpiServerError from glpi_python_client.auth._v1_session import GLPIV1Session from glpi_python_client.testing.utils import FakeResponse @@ -253,7 +253,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={ @@ -263,8 +267,9 @@ 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: @@ -297,14 +302,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: diff --git a/glpi_python_client/clients/api/plugins/_fields.py b/glpi_python_client/clients/api/plugins/_fields.py index e29dccc..4aeb4ff 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 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}" ) 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..4a07afd 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 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: diff --git a/glpi_python_client/clients/commons/_http.py b/glpi_python_client/clients/commons/_http.py index 0c904ea..e44752c 100644 --- a/glpi_python_client/clients/commons/_http.py +++ b/glpi_python_client/clients/commons/_http.py @@ -12,7 +12,11 @@ import requests -from glpi_python_client._errors import GlpiServerError, status_error_class +from glpi_python_client._errors import ( + GlpiProtocolError, + GlpiServerError, + status_error_class, +) from glpi_python_client.clients.commons._constants import RequestParamValue @@ -49,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 @@ -209,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) @@ -216,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/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) From 2807913873571dcc3110e85a734d3b855a3f902a Mon Sep 17 00:00:00 2001 From: baraline Date: Thu, 16 Jul 2026 16:34:52 +0200 Subject: [PATCH 13/20] feat(errors)!: raise GlpiValidationError for rejected arguments BREAKING CHANGE: the 23 argument- and configuration-validation sites now raise GlpiValidationError instead of a bare ValueError. GlpiValidationError inherits ValueError, so existing handlers keep working. The 2 TypeError and 4 RuntimeError sites are deliberately left alone: GlpiValidationError inherits ValueError, not TypeError, so converting them would silently break 'except TypeError' / 'except RuntimeError' callers. Adds an AST audit test pinning the whole raise-site contract, with a positive-control test proving the walk itself cannot silently return nothing (caught a real off-by-path-prefix bug in the control while writing it). Also extends test coverage for every converted site to assert both the new type and the ValueError back-compat guarantee, including two sites ("Container has no id" in the Fields plugin mixin, sync and async) that previously had no test at all. Co-Authored-By: Claude Opus 4.8 --- glpi_python_client/auth/_v1_session.py | 8 +- glpi_python_client/auth/auth.py | 16 ++- glpi_python_client/auth/tests/test_auth.py | 16 ++- .../auth/tests/test_v1_session.py | 11 +- .../clients/api/knowledgebase/_article.py | 7 +- .../api/knowledgebase/_article_async.py | 5 +- .../knowledgebase/tests/test_article_async.py | 18 ++- .../knowledgebase/tests/test_article_mixin.py | 16 ++- .../clients/api/management/_document.py | 5 +- .../clients/api/plugins/_fields.py | 12 +- .../clients/api/plugins/_fields_async.py | 7 +- .../api/plugins/tests/test_fields_async.py | 52 ++++++++- .../api/plugins/tests/test_fields_mixin.py | 41 ++++++- glpi_python_client/clients/commons/_config.py | 8 +- .../clients/custom/_statistics.py | 11 +- .../clients/custom/_statistics_async.py | 7 +- .../clients/custom/tests/test_statistics.py | 31 +++-- .../clients/tests/test_api_coverage.py | 10 +- .../clients/tests/test_async_branches.py | 20 +++- .../clients/tests/test_glpi_client.py | 42 +++++-- .../clients/tests/test_raise_site_audit.py | 108 ++++++++++++++++++ 21 files changed, 372 insertions(+), 79 deletions(-) create mode 100644 glpi_python_client/clients/tests/test_raise_site_audit.py diff --git a/glpi_python_client/auth/_v1_session.py b/glpi_python_client/auth/_v1_session.py index 21cb499..f23c531 100644 --- a/glpi_python_client/auth/_v1_session.py +++ b/glpi_python_client/auth/_v1_session.py @@ -36,7 +36,11 @@ import requests from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed -from glpi_python_client._errors import GlpiProtocolError, GlpiServerError +from glpi_python_client._errors import ( + GlpiProtocolError, + GlpiServerError, + GlpiValidationError, +) from glpi_python_client.clients.commons._http import ( ensure_response_status, finalize_request_response, @@ -78,7 +82,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( diff --git a/glpi_python_client/auth/auth.py b/glpi_python_client/auth/auth.py index 0128374..5b5a327 100644 --- a/glpi_python_client/auth/auth.py +++ b/glpi_python_client/auth/auth.py @@ -13,7 +13,11 @@ import requests from tenacity import retry, retry_if_exception_type, stop_after_attempt, wait_fixed -from glpi_python_client._errors import GlpiServerError, status_error_class +from glpi_python_client._errors import ( + GlpiServerError, + GlpiValidationError, + status_error_class, +) logger = logging.getLogger(__name__) @@ -111,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." ) @@ -352,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 cec8142..201ffee 100644 --- a/glpi_python_client/auth/tests/test_auth.py +++ b/glpi_python_client/auth/tests/test_auth.py @@ -7,7 +7,7 @@ import requests from tenacity import wait_fixed -from glpi_python_client import GlpiAuthError, GlpiServerError +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 @@ -139,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: @@ -181,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"), @@ -189,6 +198,7 @@ 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: diff --git a/glpi_python_client/auth/tests/test_v1_session.py b/glpi_python_client/auth/tests/test_v1_session.py index 81353f2..2cad8bc 100644 --- a/glpi_python_client/auth/tests/test_v1_session.py +++ b/glpi_python_client/auth/tests/test_v1_session.py @@ -8,7 +8,7 @@ import pytest import requests -from glpi_python_client import GlpiProtocolError, GlpiServerError +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: diff --git a/glpi_python_client/clients/api/knowledgebase/_article.py b/glpi_python_client/clients/api/knowledgebase/_article.py index 13d5091..ac415c6 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, @@ -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 index 47ce76f..74dd642 100644 --- a/glpi_python_client/clients/api/knowledgebase/_article_async.py +++ b/glpi_python_client/clients/api/knowledgebase/_article_async.py @@ -20,6 +20,7 @@ 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 @@ -54,7 +55,7 @@ async def _apply_category_fallback_async( Raises ------ - ValueError + GlpiValidationError When a category reference lacks an ``id``. """ @@ -63,7 +64,7 @@ async def _apply_category_fallback_async( 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/tests/test_article_async.py b/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py index 889c249..86d7f24 100644 --- a/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py +++ b/glpi_python_client/clients/api/knowledgebase/tests/test_article_async.py @@ -13,7 +13,12 @@ import pytest -from glpi_python_client import AsyncGlpiClient, PatchKBArticle, PostKBArticle +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 @@ -199,20 +204,23 @@ async def test_update_kb_article_clears_categories(client: AsyncGlpiClient) -> N async def test_update_kb_article_raises_on_missing_id_category( client: AsyncGlpiClient, ) -> None: - """A category ref without an id raises the raw ``ValueError`` on update. + """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 - ``ValueError`` from ``_apply_category_fallback_async`` must therefore - propagate unchanged. + ``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(ValueError, match="require an 'id'"): + 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( 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..3626bdb 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, @@ -255,14 +256,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/_fields.py b/glpi_python_client/clients/api/plugins/_fields.py index 4aeb4ff..e96120d 100644 --- a/glpi_python_client/clients/api/plugins/_fields.py +++ b/glpi_python_client/clients/api/plugins/_fields.py @@ -33,7 +33,7 @@ import json from typing import Any -from glpi_python_client._errors import GlpiProtocolError +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, @@ -353,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 ---------- @@ -376,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 GlpiValidationError( f"Container {container_name!r} has no id; cannot write values" ) @@ -394,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 index 34f9367..de385a9 100644 --- a/glpi_python_client/clients/api/plugins/_fields_async.py +++ b/glpi_python_client/clients/api/plugins/_fields_async.py @@ -29,6 +29,7 @@ from typing import Any +from glpi_python_client._errors import GlpiValidationError from glpi_python_client.clients.api.plugins._fields import ( _TICKET_ITEMTYPE, PluginFieldsMixin, @@ -111,14 +112,14 @@ async def set_ticket_custom_fields( # type: ignore[override] } 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 GlpiValidationError( f"Container {container_name!r} has no id; cannot write values" ) @@ -128,7 +129,7 @@ async def set_ticket_custom_fields( # type: ignore[override] 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 ValueError( + raise GlpiValidationError( f"Unknown field(s) for container {container_name!r}: " + ", ".join(unknown_fields) ) 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 index ce4bfec..9aaab10 100644 --- a/glpi_python_client/clients/api/plugins/tests/test_fields_async.py +++ b/glpi_python_client/clients/api/plugins/tests/test_fields_async.py @@ -12,7 +12,7 @@ import pytest -from glpi_python_client import AsyncGlpiClient +from glpi_python_client import AsyncGlpiClient, GlpiValidationError from glpi_python_client.testing.utils import make_async_client @@ -88,11 +88,57 @@ async def test_set_ticket_custom_fields_updates_existing_row( async def test_set_ticket_custom_fields_rejects_unknown_container( client: AsyncGlpiClient, ) -> None: - """Unknown containers raise before any write.""" + """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(ValueError, match="Unknown plugin-fields container"): + 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. + + ``GlpiValidationError`` 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(GlpiValidationError, 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 4a07afd..a4adefd 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, GlpiProtocolError +from glpi_python_client import GlpiClient, GlpiProtocolError, GlpiValidationError from glpi_python_client.clients.api.plugins._fields import ( _container_targets_itemtype, _extract_row_id, @@ -341,18 +341,48 @@ 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. + + ``GlpiValidationError`` 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(GlpiValidationError, 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=[ @@ -367,8 +397,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/commons/_config.py b/glpi_python_client/clients/commons/_config.py index 707d22f..0d2b2ea 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 @@ -149,7 +151,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 +203,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 +219,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/custom/_statistics.py b/glpi_python_client/clients/custom/_statistics.py index 03149a5..469c35a 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, @@ -421,14 +422,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 +455,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: ( @@ -564,7 +565,7 @@ def _resolve_window( """ if default_days < 1: - raise ValueError("default_days must be a positive integer") + raise GlpiValidationError("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) @@ -572,7 +573,7 @@ def _resolve_window( else parsed_end - timedelta(days=default_days - 1) ) 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..b3936b1 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, @@ -388,7 +389,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 +407,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 +433,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/tests/test_statistics.py b/glpi_python_client/clients/custom/tests/test_statistics.py index ee3733b..e165735 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,18 @@ 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_task_statistics_zero_for_empty_input(client: GlpiClient) -> None: @@ -421,10 +428,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``. + + ``GlpiValidationError`` inherits ``ValueError`` so existing callers that + catch the broader type keep working. + """ - with pytest.raises(ValueError, match="user_id"): + 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 +499,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 +517,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_glpi_client.py b/glpi_python_client/clients/tests/test_glpi_client.py index 51bfe6a..49dc7ba 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: @@ -97,10 +114,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..4a91060 --- /dev/null +++ b/glpi_python_client/clients/tests/test_raise_site_audit.py @@ -0,0 +1,108 @@ +"""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 dozens of raise sites 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 a dozen tests. 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}" From a4832712b56dd7cf50e584cb7b1174f1209af3a0 Mon Sep 17 00:00:00 2001 From: baraline Date: Thu, 16 Jul 2026 17:45:27 +0200 Subject: [PATCH 14/20] fix(errors): reclassify Fields-plugin missing-id as GlpiProtocolError The container that hits `container.id is None` in `set_ticket_custom_fields` came from the server's own `list_plugin_fields_containers` response, not from caller input -- the caller only supplied a container name that matched correctly. A missing documented `id` on an otherwise-valid server row is a protocol defect, not a validation failure, matching the already-GlpiProtocolError `_extract_row_id` sites in the same file. Also tighten the raise-site audit's docstring with the real census (39 sites: 21 GlpiValidationError, 9 GlpiProtocolError, 4 RuntimeError, 2 TypeError, 1 GlpiServerError, 2 status_error_class dispatches) instead of "dozens"/"a dozen". Investigated the analogous _statistics.py "no users matched" site and left it as GlpiValidationError: search_users legitimately returns an empty list for well-formed, caller-driven criteria, so a zero-match result is not a server contract violation. Co-Authored-By: Claude Opus 4.8 --- glpi_python_client/clients/api/plugins/_fields.py | 2 +- .../clients/api/plugins/_fields_async.py | 4 ++-- .../clients/api/plugins/tests/test_fields_async.py | 11 +++++++---- .../clients/api/plugins/tests/test_fields_mixin.py | 10 +++++++--- .../clients/tests/test_raise_site_audit.py | 10 +++++++--- 5 files changed, 24 insertions(+), 13 deletions(-) diff --git a/glpi_python_client/clients/api/plugins/_fields.py b/glpi_python_client/clients/api/plugins/_fields.py index e96120d..6993725 100644 --- a/glpi_python_client/clients/api/plugins/_fields.py +++ b/glpi_python_client/clients/api/plugins/_fields.py @@ -383,7 +383,7 @@ def set_ticket_custom_fields( for container_name, column_values in values.items(): container = by_name[container_name] if container.id is None: - raise GlpiValidationError( + raise GlpiProtocolError( f"Container {container_name!r} has no id; cannot write values" ) diff --git a/glpi_python_client/clients/api/plugins/_fields_async.py b/glpi_python_client/clients/api/plugins/_fields_async.py index de385a9..a84d1f3 100644 --- a/glpi_python_client/clients/api/plugins/_fields_async.py +++ b/glpi_python_client/clients/api/plugins/_fields_async.py @@ -29,7 +29,7 @@ from typing import Any -from glpi_python_client._errors import GlpiValidationError +from glpi_python_client._errors import GlpiProtocolError, GlpiValidationError from glpi_python_client.clients.api.plugins._fields import ( _TICKET_ITEMTYPE, PluginFieldsMixin, @@ -119,7 +119,7 @@ async def set_ticket_custom_fields( # type: ignore[override] for container_name, column_values in values.items(): container = by_name[container_name] if container.id is None: - raise GlpiValidationError( + raise GlpiProtocolError( f"Container {container_name!r} has no id; cannot write values" ) 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 index 9aaab10..77fab60 100644 --- a/glpi_python_client/clients/api/plugins/tests/test_fields_async.py +++ b/glpi_python_client/clients/api/plugins/tests/test_fields_async.py @@ -12,7 +12,7 @@ import pytest -from glpi_python_client import AsyncGlpiClient, GlpiValidationError +from glpi_python_client import AsyncGlpiClient, GlpiProtocolError, GlpiValidationError from glpi_python_client.testing.utils import make_async_client @@ -110,15 +110,18 @@ async def test_set_ticket_custom_fields_rejects_container_without_id( ) -> None: """A matched container with no ``id`` raises before any write. - ``GlpiValidationError`` inherits ``ValueError`` so existing callers that - catch the broader type keep working. + 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(GlpiValidationError, match="has no id") as excinfo: + 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) 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 a4adefd..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 @@ -363,13 +363,17 @@ def test_set_ticket_custom_fields_rejects_container_without_id( ) -> None: """A matched container with no ``id`` raises before any write. - ``GlpiValidationError`` inherits ``ValueError`` so existing callers that - catch the broader type keep working. + 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(GlpiValidationError, match="has no id") as excinfo: + with pytest.raises(GlpiProtocolError, match="has no id") as excinfo: client.set_ticket_custom_fields( 62571, {"aidelarsolution": {"aidelarsolutionfield": "value"}} ) diff --git a/glpi_python_client/clients/tests/test_raise_site_audit.py b/glpi_python_client/clients/tests/test_raise_site_audit.py index 4a91060..3ebbd4b 100644 --- a/glpi_python_client/clients/tests/test_raise_site_audit.py +++ b/glpi_python_client/clients/tests/test_raise_site_audit.py @@ -1,14 +1,18 @@ """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 dozens of raise sites and a -missed one is invisible: a bare ValueError still passes every existing +0.4.0 error migration is a mechanical sweep across 39 raise sites in +non-test library code -- 21 ``GlpiValidationError``, 9 ``GlpiProtocolError``, +4 ``RuntimeError``, 2 ``TypeError``, 1 ``GlpiServerError``, plus 2 +``status_error_class(...)`` dispatch sites -- 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 a dozen tests. See plan-1 decision D3. +code and the 12 tests in this repo that assert on those two types. See +plan-1 decision D3. """ from __future__ import annotations From f4b7829c680def8d874a9464e17e60d3e9b43746 Mon Sep 17 00:00:00 2001 From: baraline Date: Thu, 16 Jul 2026 18:08:42 +0200 Subject: [PATCH 15/20] refactor(errors)!: drop the dead remote_error_message helper BREAKING CHANGE: glpi_python_client.clients.commons._errors is removed. It was private and had zero library call sites -- only its own tests referenced it. Its sole job was unwrapping tenacity.RetryError to recover the underlying message. reraise=True means RetryError no longer reaches callers, so the helper has nothing left to unwrap. --- .../clients/commons/__init__.py | 2 +- glpi_python_client/clients/commons/_errors.py | 27 ------------ .../clients/commons/tests/test_errors.py | 41 ------------------- 3 files changed, 1 insertion(+), 69 deletions(-) delete mode 100644 glpi_python_client/clients/commons/_errors.py delete mode 100644 glpi_python_client/clients/commons/tests/test_errors.py 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/_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/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) From 4355a9ffa3f1ad908eb60233e44400f9c1280390 Mon Sep 17 00:00:00 2001 From: baraline Date: Fri, 17 Jul 2026 09:24:39 +0200 Subject: [PATCH 16/20] docs(errors): document the public error contract Adds the user-guide error-handling section, records the breaking changes in the changelog, and updates the async integration test to catch GlpiNotFoundError instead of a requests exception. --- CHANGELOG.md | 47 ++++++++++++ docs/user_guide.rst | 84 ++++++++++++++++++++- integration_tests/test_integration_async.py | 19 ++--- 3 files changed, 136 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eaa7d89..fc89b08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,53 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). `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 diff --git a/docs/user_guide.rst b/docs/user_guide.rst index 77f8ba6..56fd935 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,84 @@ 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 +----------------- + +Every exception the client raises derives from +:class:`~glpi_python_client.GlpiError`, so one handler covers the whole +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}") + +The hierarchy lets you narrow as far as you need: + +.. code-block:: text + + GlpiError + ├── GlpiTransportError the request never got a response + │ └── GlpiTimeoutError ... because it timed out + ├── 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, truncated to 200 chars + +.. 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. A persistent 5xx encountered while refreshing can therefore +cost up to 12 POST requests (3 refresh attempts, each falling through to +up to 3 nested acquisition attempts) before +:class:`~glpi_python_client.GlpiServerError` reaches you — not the 3 +attempts the retry configuration alone would suggest. 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/integration_tests/test_integration_async.py b/integration_tests/test_integration_async.py index 7474555..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,22 +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 typed error the - sync client would raise: a 404 status raises - :class:`~glpi_python_client.GlpiNotFoundError` immediately (no - retry), while a persistent 5xx would instead raise - :class:`~glpi_python_client.GlpiServerError` after the transport's - retries are exhausted, and a network fault would raise the real - :class:`requests.RequestException` after its own retries. The - assertion below covers all three: the two GLPI-typed errors both - inherit :class:`ValueError` via - :class:`~glpi_python_client.GlpiStatusError`. + not exist. The async client must surface the same + :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) From 093dcb457520053ff0db5216f9cc75fe039daf3c Mon Sep 17 00:00:00 2001 From: baraline Date: Fri, 17 Jul 2026 09:55:32 +0200 Subject: [PATCH 17/20] fix(errors): correct the public error contract's documentation Plan 1 (errors) final-review fixes. The exception hierarchy itself was already correct; the prose describing it was not. - Stop claiming every failure derives from GlpiError: network faults still raise requests exceptions until the httpx transport swap, and a handful of sites deliberately still raise RuntimeError/TypeError. - Stop documenting GlpiTransportError/GlpiTimeoutError as if they were raised today -- they are reserved for the httpx swap. - Wrap the two remaining stdlib ValueError leaks (parse_optional_env_int, StatisticsMixin._resolve_window) as chained GlpiValidationError, with tests proving both the typed error and except ValueError still catch it. - Sweep 54 stale "Raises: ValueError" docstrings (the 51 the review found plus 3 async-mirror sites) to name the actual typed exception. - Fix the 12-POST retry arithmetic explanation and the self-contradictory v1-session retry-policy docstring. - Pin _refresh_access_token's network-error retry count (3, independent of the already-pinned 12-POST persistent-5xx path) with a new test. No retry predicate, reraise=, retry policy, or 4xx/5xx raise site changed. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 9 ++++ docs/api_reference.rst | 13 +++++- docs/user_guide.rst | 45 +++++++++++++++---- glpi_python_client/_errors.py | 38 +++++++++++++--- glpi_python_client/auth/_v1_session.py | 16 ++++--- glpi_python_client/auth/tests/test_auth.py | 42 +++++++++++++++++ glpi_python_client/clients/_base_client.py | 4 +- .../clients/api/administration/_entity.py | 13 +++--- .../clients/api/administration/_user.py | 13 +++--- .../clients/api/assistance/_team.py | 6 +-- .../clients/api/assistance/_ticket.py | 13 +++--- .../api/assistance/timeline/_document.py | 13 +++--- .../api/assistance/timeline/_followup.py | 13 +++--- .../api/assistance/timeline/_solution.py | 13 +++--- .../clients/api/assistance/timeline/_task.py | 13 +++--- .../clients/api/dropdowns/_location.py | 13 +++--- .../clients/api/knowledgebase/_article.py | 4 +- .../clients/api/knowledgebase/_category.py | 2 +- .../clients/api/management/_document.py | 15 ++++--- glpi_python_client/clients/async_client.py | 2 +- glpi_python_client/clients/commons/_config.py | 15 ++++++- .../clients/commons/_transport.py | 18 ++++---- .../clients/custom/_statistics.py | 35 ++++++++++----- .../clients/custom/_statistics_async.py | 5 ++- .../clients/custom/_ticket_context.py | 2 +- .../clients/custom/_ticket_context_async.py | 2 +- .../clients/custom/tests/test_statistics.py | 18 ++++++++ .../clients/tests/test_glpi_client.py | 14 ++++++ .../clients/tests/test_raise_site_audit.py | 12 ++--- 29 files changed, 307 insertions(+), 114 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc89b08..ab01dee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). 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`. +- `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 diff --git a/docs/api_reference.rst b/docs/api_reference.rst index ab2a984..e512a4e 100644 --- a/docs/api_reference.rst +++ b/docs/api_reference.rst @@ -31,11 +31,22 @@ the asynchronous one wraps each synchronous method into a coroutine. Exceptions ---------- -Every exception raised by the client derives from :class:`GlpiError`. +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: diff --git a/docs/user_guide.rst b/docs/user_guide.rst index 56fd935..3f5cf86 100644 --- a/docs/user_guide.rst +++ b/docs/user_guide.rst @@ -1405,9 +1405,10 @@ Example output:: 7. Error handling ----------------- -Every exception the client raises derives from -:class:`~glpi_python_client.GlpiError`, so one handler covers the whole -library surface: +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 @@ -1419,13 +1420,37 @@ library surface: except GlpiError as exc: print(f"GLPI call failed: {exc}") +This is not yet the client's entire failure surface. The client is still +built on ``requests``, and network-level faults -- connection failures, +DNS errors, timeouts -- still propagate as ``requests`` exceptions today +rather than a :class:`~glpi_python_client.GlpiError` subclass. Catch +``requests.RequestException`` alongside :class:`~glpi_python_client.GlpiError` +if you need to handle those too: + +.. 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 the request never got a response - │ └── GlpiTimeoutError ... because it timed out + ├── 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 @@ -1469,10 +1494,14 @@ 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. A persistent 5xx encountered while refreshing can therefore -cost up to 12 POST requests (3 refresh attempts, each falling through to -up to 3 nested acquisition attempts) before +cost up to 12 POST requests before :class:`~glpi_python_client.GlpiServerError` reaches you — not the 3 -attempts the retry configuration alone would suggest. A rejected +attempts the retry configuration alone would suggest. The refresh +method's own retry decorator makes up to 3 refresh attempts; each +refresh attempt sends 1 refresh POST and, on failure, falls through to a +nested token-acquisition call with its own independent 3-attempt retry, +sending up to 3 more POSTs. That is 3 × (1 refresh POST + 3 nested +acquisition POSTs) = 12 POST requests in the worst case. A rejected credential (401/403) is not retried at either layer and fails after at most 2 POST requests. diff --git a/glpi_python_client/_errors.py b/glpi_python_client/_errors.py index d3aa7cf..19026c8 100644 --- a/glpi_python_client/_errors.py +++ b/glpi_python_client/_errors.py @@ -1,8 +1,23 @@ """Public exception hierarchy raised by :mod:`glpi_python_client`. -Every exception the client raises deliberately derives from :class:`GlpiError`, -so callers can catch the whole library surface with a single ``except`` clause -without importing the underlying HTTP library. +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 @@ -22,13 +37,24 @@ class GlpiError(Exception): class GlpiTransportError(GlpiError): """The HTTP request never produced a response. - Raised for connection failures, DNS errors, and other network-level - faults where GLPI returned no status code at all. + 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.""" + """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): diff --git a/glpi_python_client/auth/_v1_session.py b/glpi_python_client/auth/_v1_session.py index f23c531..96c013e 100644 --- a/glpi_python_client/auth/_v1_session.py +++ b/glpi_python_client/auth/_v1_session.py @@ -22,8 +22,14 @@ :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. -:class:`ValueError` raised by status-code or payload checks does not -trigger a retry — client-side or 4xx failures are surfaced immediately. +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 @@ -312,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 ------- diff --git a/glpi_python_client/auth/tests/test_auth.py b/glpi_python_client/auth/tests/test_auth.py index 201ffee..a11a54f 100644 --- a/glpi_python_client/auth/tests/test_auth.py +++ b/glpi_python_client/auth/tests/test_auth.py @@ -383,3 +383,45 @@ def post(self, url: str, data: dict[str, str], timeout: int) -> FakeResponse: 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. Its ``GlpiServerError`` + member is pinned by + ``test_refresh_5xx_persistent_multiplies_past_three_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/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/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/_article.py b/glpi_python_client/clients/api/knowledgebase/_article.py index ac415c6..17b0ae9 100644 --- a/glpi_python_client/clients/api/knowledgebase/_article.py +++ b/glpi_python_client/clients/api/knowledgebase/_article.py @@ -75,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. """ @@ -171,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. """ 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/management/_document.py b/glpi_python_client/clients/api/management/_document.py index 3626bdb..b9cee40 100644 --- a/glpi_python_client/clients/api/management/_document.py +++ b/glpi_python_client/clients/api/management/_document.py @@ -78,7 +78,7 @@ def get_document(self, document_id: GlpiId) -> GetDocument: Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -107,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( @@ -137,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. """ @@ -167,7 +168,7 @@ def delete_document( Raises ------ - ValueError + GlpiStatusError If the GLPI server returns a non-success HTTP status. """ @@ -196,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. """ diff --git a/glpi_python_client/clients/async_client.py b/glpi_python_client/clients/async_client.py index a480751..e8d4d30 100644 --- a/glpi_python_client/clients/async_client.py +++ b/glpi_python_client/clients/async_client.py @@ -125,7 +125,7 @@ def __init__(self, *, executor: Executor | None = None, **kwargs: Any) -> None: 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/_config.py b/glpi_python_client/clients/commons/_config.py index 0d2b2ea..3b417bd 100644 --- a/glpi_python_client/clients/commons/_config.py +++ b/glpi_python_client/clients/commons/_config.py @@ -122,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: @@ -129,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") diff --git a/glpi_python_client/clients/commons/_transport.py b/glpi_python_client/clients/commons/_transport.py index 9ea420a..feb6ed0 100644 --- a/glpi_python_client/clients/commons/_transport.py +++ b/glpi_python_client/clients/commons/_transport.py @@ -391,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. @@ -430,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] @@ -484,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. @@ -520,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/custom/_statistics.py b/glpi_python_client/clients/custom/_statistics.py index 469c35a..ccb2329 100644 --- a/glpi_python_client/clients/custom/_statistics.py +++ b/glpi_python_client/clients/custom/_statistics.py @@ -126,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( @@ -266,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( @@ -562,16 +564,29 @@ 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 GlpiValidationError("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) - ) + 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 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 b3936b1..fc4d00b 100644 --- a/glpi_python_client/clients/custom/_statistics_async.py +++ b/glpi_python_client/clients/custom/_statistics_async.py @@ -296,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 ( 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 e165735..519b111 100644 --- a/glpi_python_client/clients/custom/tests/test_statistics.py +++ b/glpi_python_client/clients/custom/tests/test_statistics.py @@ -149,6 +149,24 @@ def test_get_ticket_statistics_rejects_invalid_window(client: GlpiClient) -> Non 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: """An empty ticket list returns zeroed totals without any HTTP call.""" diff --git a/glpi_python_client/clients/tests/test_glpi_client.py b/glpi_python_client/clients/tests/test_glpi_client.py index 49dc7ba..f1114ca 100644 --- a/glpi_python_client/clients/tests/test_glpi_client.py +++ b/glpi_python_client/clients/tests/test_glpi_client.py @@ -91,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"), [ diff --git a/glpi_python_client/clients/tests/test_raise_site_audit.py b/glpi_python_client/clients/tests/test_raise_site_audit.py index 3ebbd4b..b599a9f 100644 --- a/glpi_python_client/clients/tests/test_raise_site_audit.py +++ b/glpi_python_client/clients/tests/test_raise_site_audit.py @@ -1,12 +1,12 @@ """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 39 raise sites in -non-test library code -- 21 ``GlpiValidationError``, 9 ``GlpiProtocolError``, -4 ``RuntimeError``, 2 ``TypeError``, 1 ``GlpiServerError``, plus 2 -``status_error_class(...)`` dispatch sites -- and a missed one is -invisible: a bare ValueError still passes every existing -``pytest.raises(ValueError)`` test. +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 -- From ac5fa5ecd31810b8e0de438fe01e511f3df37517 Mon Sep 17 00:00:00 2001 From: baraline Date: Fri, 17 Jul 2026 10:22:14 +0200 Subject: [PATCH 18/20] fix(auth): stop the token-refresh retry multiplication _refresh_access_token does not raise on a non-2xx refresh response: it logs and falls through to a nested _acquire_token(), which carries its own 3-attempt decorator. Because the outer decorator also retried GlpiServerError, a persistent 5xx cost 3 x (1 refresh POST + 3 nested acquire POSTs) = 12 POSTs and ~33s of waiting. Any GlpiServerError reaching the outer decorator has already been retried three times by the nested call, so retrying it again was pure duplication. Narrowing the outer predicate to requests.RequestException keeps the useful retry (a network failure of the refresh POST itself, which raises rather than falling through) and drops the duplicate one. Measured: persistent 5xx 12 -> 4 POSTs. Persistent 401 (2) and network errors on the refresh POST (3) are unchanged. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 13 ++++++ docs/user_guide.rst | 18 ++++----- glpi_python_client/auth/auth.py | 22 +++++------ glpi_python_client/auth/tests/test_auth.py | 46 +++++++++++----------- 4 files changed, 56 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab01dee..02241be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### 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 diff --git a/docs/user_guide.rst b/docs/user_guide.rst index 3f5cf86..f956093 100644 --- a/docs/user_guide.rst +++ b/docs/user_guide.rst @@ -1493,15 +1493,15 @@ 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. A persistent 5xx encountered while refreshing can therefore -cost up to 12 POST requests before -:class:`~glpi_python_client.GlpiServerError` reaches you — not the 3 -attempts the retry configuration alone would suggest. The refresh -method's own retry decorator makes up to 3 refresh attempts; each -refresh attempt sends 1 refresh POST and, on failure, falls through to a -nested token-acquisition call with its own independent 3-attempt retry, -sending up to 3 more POSTs. That is 3 × (1 refresh POST + 3 nested -acquisition POSTs) = 12 POST requests in the worst case. A rejected +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. diff --git a/glpi_python_client/auth/auth.py b/glpi_python_client/auth/auth.py index 5b5a327..5e61019 100644 --- a/glpi_python_client/auth/auth.py +++ b/glpi_python_client/auth/auth.py @@ -272,7 +272,7 @@ def _acquire_token(self) -> None: ) @retry( - retry=retry_if_exception_type((requests.RequestException, GlpiServerError)), + retry=retry_if_exception_type(requests.RequestException), stop=stop_after_attempt(3), wait=wait_fixed(3), reraise=True, @@ -295,16 +295,16 @@ def _refresh_access_token(self) -> None: 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. Both this - method's retry decorator and the nested :meth:`_acquire_token` - call's decorator treat ``GlpiServerError`` as retryable, and each - retries independently. A persistent 5xx therefore costs up to - 3 (this method's attempts) x (1 refresh POST + 3 nested acquire - POSTs) = 12 POST requests, not the 3 attempts the retry - configuration alone would suggest. This nested-retry - multiplication predates this decorator and is a known, - measured behavior (see the auth tests), not something this - method's docstring papers over. + 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. diff --git a/glpi_python_client/auth/tests/test_auth.py b/glpi_python_client/auth/tests/test_auth.py index a11a54f..99e1fb9 100644 --- a/glpi_python_client/auth/tests/test_auth.py +++ b/glpi_python_client/auth/tests/test_auth.py @@ -315,26 +315,26 @@ def test_refresh_401_stays_final_with_one_nested_acquire_call( assert len(session.calls) == 2 -def test_refresh_5xx_persistent_multiplies_past_three_attempts( +def test_refresh_5xx_persistent_costs_one_refresh_plus_nested_acquire_attempts( monkeypatch: pytest.MonkeyPatch, ) -> None: - """A persistent 5xx during refresh costs 12 POSTs, not 3. - - KNOWN DEFECT, measured and pinned here (pre-existing, not introduced by - the 4xx-fail-fast change in this task): ``_refresh_access_token`` falls - through to a *nested* ``_acquire_token()`` call on any non-2xx response - instead of raising directly (auth.py:302-303). Both methods are - independently decorated with ``stop_after_attempt(3)``, and - ``GlpiServerError`` is retryable by *both* decorators. A persistent 5xx - therefore costs 3 (this method's attempts) x (1 refresh POST + 3 nested - acquire POSTs) = 12 POST calls -- and ~24s of ``wait_fixed(3)`` sleep in - production -- not the 3 attempts / ~6s the retry configuration alone - would suggest. - - This test pins the measured reality so a future change (e.g. plan 3's - httpx swap) cannot silently alter it. Restructuring the nested-retry - topology itself is a deliberate design change and is out of scope here; - see the plan-1 task-3 report for the full write-up. + """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)) @@ -348,7 +348,7 @@ def test_refresh_5xx_persistent_multiplies_past_three_attempts( manager.ensure_token() assert excinfo.value.status_code == 503 - assert len(session.calls) == 12 + assert len(session.calls) == 4 def test_acquire_token_network_error_is_retried_three_times( @@ -391,10 +391,10 @@ def test_refresh_network_error_is_retried_three_times( """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. Its ``GlpiServerError`` - member is pinned by - ``test_refresh_5xx_persistent_multiplies_past_three_attempts`` above, and - ``_acquire_token``'s network retry is pinned by + ``_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 From 732a633bf486eb0c6e13026cfc4fdc193cbc508d Mon Sep 17 00:00:00 2001 From: baraline Date: Fri, 17 Jul 2026 16:31:10 +0200 Subject: [PATCH 19/20] test(integration): make the live suite runnable end-to-end Two defects 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 -- the only one of the suite's seven iter_search loops missing a break. Against preprod (59,879 matching tickets, ~4.9s per page) that is ~19,960 requests and several hours, so the test never finished and stalled the whole suite behind it. It now stops after 3 pages and asserts ids do not repeat across pages, which actually verifies the start offset advances. The unbounded loop asserted only isinstance(collected, list), so it could not have detected a stuck offset no matter how long it ran. Its filter also moves from `status==1` to `status.id==1`. The v2 contract types Ticket.status as object{id,name}, so a bare `status==1` is silently dropped and the search degrades to "every ticket" -- GLPI ignores filter fields it does not recognise and returns the full set with no error. The remaining `status==1` call sites are left for the dedicated filter- correctness branch. The three Fields plugin tests failed rather than skipped when the plugin is absent. _skip_when_no_v1 only checked that v1 credentials were configured, never that the plugin existed; without it GLPI rejects the PluginFieldsContainer itemtype with a 400 instead of returning an empty list, which the "no containers -> skip" guard could not catch. The new fields_containers fixture skips on exactly that signature (400 + ERROR_RESOURCE_NOT_FOUND_NOR_COMMONDBTM) and re-raises anything else, so a genuine failure still fails. Verified against live preprod: 42 passed, 4 skipped, no deselect, 3m16s. Previously the suite could not complete at all. Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 18 +++++++ integration_tests/test_integration.py | 76 +++++++++++++++++++++------ 2 files changed, 79 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 02241be..33d445e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,24 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). 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`) 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, From 90faf452d0174a51af89bbde8eea779439bf5ccb Mon Sep 17 00:00:00 2001 From: baraline Date: Fri, 17 Jul 2026 17:00:41 +0200 Subject: [PATCH 20/20] remove char limit for error txt --- docs/user_guide.rst | 2 +- glpi_python_client/auth/auth.py | 2 +- glpi_python_client/clients/commons/_http.py | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/user_guide.rst b/docs/user_guide.rst index f956093..0e0bbd2 100644 --- a/docs/user_guide.rst +++ b/docs/user_guide.rst @@ -1471,7 +1471,7 @@ usually want: 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, truncated to 200 chars + print(exc.response_text) # the response body .. note:: diff --git a/glpi_python_client/auth/auth.py b/glpi_python_client/auth/auth.py index 5e61019..3e3bf1e 100644 --- a/glpi_python_client/auth/auth.py +++ b/glpi_python_client/auth/auth.py @@ -268,7 +268,7 @@ def _acquire_token(self) -> None: f"GLPI OAuth token returned {response.status_code}: {error_detail}", status_code=response.status_code, url=self._token_url, - response_text=str(error_detail)[:200], + response_text=str(error_detail), ) @retry( diff --git a/glpi_python_client/clients/commons/_http.py b/glpi_python_client/clients/commons/_http.py index e44752c..a662565 100644 --- a/glpi_python_client/clients/commons/_http.py +++ b/glpi_python_client/clients/commons/_http.py @@ -140,7 +140,7 @@ def finalize_request_response( message, status_code=response.status_code, url=url, - response_text=response.text[:200], + response_text=response.text, ) if response.status_code not in success_statuses: logger.warning( @@ -148,7 +148,7 @@ def finalize_request_response( method_name, url, response.status_code, - response.text[:200], + response.text, ) return response @@ -175,10 +175,10 @@ def ensure_response_status( if response.status_code not in success_statuses: error_class = status_error_class(response.status_code) raise error_class( - f"{failure_message}: {response.status_code} {response.text[:200]}", + f"{failure_message}: {response.status_code} {response.text}", status_code=response.status_code, url=str(response.url), - response_text=response.text[:200], + response_text=response.text, )