Merge pull request #16 from baraline/feat/real-async-io

Feat/real async io

Author: baraline

CHANGELOG.md

@@ -0,0 +1,136 @@
+# Changelog
+
+All notable changes to this project are documented in this file.
+
+The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
+
+## Unreleased
+
+### Fixed
+
+- `GLPITokenManager._refresh_access_token`'s retry decorator no longer
+  retries a `GlpiServerError` from its fall-through to the nested
+  `_acquire_token()` call. That nested call already carries its own
+  independent 3-attempt retry decorator for `GlpiServerError`, so the
+  outer decorator retrying it too meant a persistent 5xx during token
+  refresh cost 3 (outer attempts) × (1 refresh POST + 3 nested acquire
+  POSTs) = 12 POST requests and ~33s of `wait_fixed(3)` sleep, instead of
+  the 3 attempts the retry configuration alone would suggest. The outer
+  decorator now only retries `requests.RequestException` (a genuine
+  network fault on the refresh POST itself), which is not covered by the
+  nested call at all. A persistent 5xx now costs exactly 1 refresh POST +
+  3 nested acquire POSTs = 4 POST requests. A persistent 401 (2 POSTs) and
+  a network error on the refresh POST (3 POSTs) are unaffected.
+- `AsyncGlpiClient.create_kb_article` / `update_kb_article` no longer
+  silently drop `categories`. Both methods called the public
+  `set_kb_article_categories` through `self` from inside a synchronous
+  method body; `AsyncBridge.__init_subclass__` wraps every public sync
+  method into a coroutine, so that call returned an un-awaited coroutine
+  instead of performing the write. The article was created (or updated)
+  successfully, a valid id was returned, and no exception was raised —
+  the category assignment simply never happened. Fixed with hand-written
+  async overrides in `_article_async.py` that strip `categories` from the
+  v2 body, run the v2 write in a worker thread, and apply the category
+  fallback through an awaited call.
+- `AsyncGlpiClient.get_ticket_custom_fields` / `set_ticket_custom_fields`
+  raised `TypeError: 'coroutine' object is not iterable` and were
+  unusable. Same root cause as above: a sync method reaching a sibling
+  public method through `self` received a coroutine instead of a result.
+  Fixed with hand-written async overrides in `_fields_async.py`.
+- The integration suite is runnable end-to-end again. Two defects, both in
+  `integration_tests/` only (no library code involved):
+  - `test_iter_search_tickets_multi_page` walked *every* matching ticket in
+    batches of 3 with no upper bound — it was the only one of the suite's
+    seven `iter_search` loops missing a `break`. Against a real instance
+    (59,879 matching tickets) that is ~19,960 requests and several hours,
+    which stalled the whole suite. It now stops after 3 pages and asserts
+    that ids do not repeat across pages, which actually verifies that the
+    `start` offset advances — the old unbounded loop asserted only
+    `isinstance(collected, list)` and so could not have detected a stuck
+    offset.
+  - The three GLPI Fields plugin tests failed rather than skipped when the
+    plugin is not installed. `_skip_when_no_v1` only checked that v1
+    *credentials were configured*, never that the *plugin existed*; an
+    absent plugin makes GLPI reject the `PluginFieldsContainer` itemtype
+    with a 400 rather than return an empty list. A new `fields_containers`
+    fixture skips on exactly that signature (400 +
+    `ERROR_RESOURCE_NOT_FOUND_NOR_COMMONDBTM`) and re-raises anything else.
+- `parse_optional_env_int` (environment/config parsing) and
+  `StatisticsMixin._resolve_window` (the date-window helper behind
+  `get_ticket_statistics` / `get_task_durations` / `get_user_activity`)
+  no longer let a malformed value escape as a bare stdlib `ValueError`
+  from `int()` / `date.fromisoformat()` (e.g. `GLPI_TIMEOUT=abc` or
+  `get_ticket_statistics(start_date="2026-13-45")`). Both now raise
+  `GlpiValidationError`, chaining the original error via `from` rather
+  than swallowing it. Non-breaking: `GlpiValidationError` inherits
+  `ValueError`, so `except ValueError` still catches it.
+
+### Added
+
+- `glpi_python_client/clients/tests/test_async_selfcall_guard.py`: a
+  structural AST guard that fails the suite if any public method on
+  `GlpiClient` transitively reaches another public method through a
+  literal `self.name(...)` call (directly, or via a private helper)
+  without a corresponding hand-written async override on
+  `AsyncGlpiClient`. This prevents the same bug class — silent data loss
+  or a `TypeError` at call time, depending on how the dropped coroutine is
+  used — from being reintroduced by a future endpoint.
+- A public exception hierarchy, exported from the package root:
+  `GlpiError`, `GlpiTransportError`, `GlpiTimeoutError`, `GlpiStatusError`,
+  `GlpiAuthError`, `GlpiNotFoundError`, `GlpiServerError`,
+  `GlpiValidationError` and `GlpiProtocolError`. `GlpiStatusError` and its
+  subclasses carry `.status_code`, `.url` and `.response_text`. A GLPI 404
+  and a bad argument were previously both a bare `ValueError` and could not
+  be told apart.
+- `FakeResponse` (in the public `glpi_python_client.testing` module) gained
+  a `url` attribute.
+- A user-guide "Error handling" section documenting the exception
+  hierarchy and the retry behaviour for both the transport layer and OAuth
+  token acquisition/refresh.
+
+### Changed
+
+- **Breaking:** a persistent 5xx now raises `GlpiServerError` instead of
+  `tenacity.RetryError`. The retry decorators gained `reraise=True`. Code
+  doing `except tenacity.RetryError` and digging out
+  `.last_attempt.exception()` should now catch `GlpiServerError` directly.
+- **Breaking:** unexpected HTTP statuses raise a `GlpiStatusError` subclass;
+  rejected arguments and configuration raise `GlpiValidationError`; 2xx
+  responses with an unusable body raise `GlpiProtocolError`. All three
+  inherit `ValueError`, so existing `except ValueError` handlers keep
+  working.
+- **Breaking:** a non-2xx OAuth token response raises `GlpiAuthError` (401/403)
+  or `GlpiServerError` (5xx). The token retry decorators had no `retry=`
+  predicate and therefore retried every failure, including a rejected
+  credential; a wrong `client_secret` cost 3 attempts and 6 seconds. OAuth
+  4xx is now final, matching the rest of the library. OAuth 5xx is still
+  retried.
+- **Breaking:** the private `glpi_python_client.clients.commons._errors`
+  module and its `remote_error_message` helper are removed. It had no
+  library call sites, and `reraise=True` leaves it nothing to unwrap.
+
+### Unchanged (deliberately)
+
+- Retry semantics: 5xx retried 3 times with a 3-second fixed wait, 4xx never
+  retried.
+- Tolerant search endpoints still return `[]` rather than raising on a 4xx.
+- The `TypeError` sites in environment parsing and the `RuntimeError` sites
+  for closed clients, missing v1 sessions and partial KB failures still
+  raise those types. `GlpiValidationError` inherits `ValueError`, not
+  `TypeError`, so converting them would break `except TypeError` callers.
+- The transport is still `requests`. Network faults (connection reset, DNS,
+  timeout) still surface as `requests` exceptions; they become
+  `GlpiTransportError` / `GlpiTimeoutError` when the transport moves to
+  httpx, with no change to the class names above.
+
+### Notes
+
+- Both fixed bugs shared one root cause: `AsyncBridge` wraps every public
+  sync method into a coroutine, so a sync method body calling a sibling
+  public method through `self` (rather than through a hand-written async
+  override) silently receives a coroutine instead of the real return
+  value.
+- This is a documentation-only release note; **no version was released**
+  from this branch. The next release is planned as 0.4.0, an httpx +
+  unasync rewrite that removes `AsyncBridge` entirely, making this class
+  of bug structurally impossible rather than merely guarded against.

docs/api_reference.rst

@@ -28,6 +28,61 @@ the asynchronous one wraps each synchronous method into a coroutine.
    :members:
    :show-inheritance:
 
+Exceptions
+----------
+
+Exceptions raised for a bad argument, an unexpected HTTP status, or an
+unusable response body derive from :class:`GlpiError`.
+:class:`GlpiStatusError`, :class:`GlpiValidationError` and
+:class:`GlpiProtocolError` also inherit :class:`ValueError` for backwards
+compatibility with releases that raised bare ``ValueError``.
+
+This is not the library's entire failure surface. Network-level faults
+(connection failures, DNS errors, timeouts) still propagate as
+``requests`` exceptions today: :class:`GlpiTransportError` and
+:class:`GlpiTimeoutError` are reserved for that case but are not raised
+until a future httpx transport swap. A handful of sites also
+deliberately still raise bare ``RuntimeError`` or ``TypeError`` instead
+of a library type, so existing ``except RuntimeError`` / ``except
+TypeError`` code keeps working. See :ref:`error-handling` in the user
+guide for the full picture, including which methods raise which type.
+
+.. autoexception:: GlpiError
+   :members:
+   :show-inheritance:
+
+.. autoexception:: GlpiTransportError
+   :members:
+   :show-inheritance:
+
+.. autoexception:: GlpiTimeoutError
+   :members:
+   :show-inheritance:
+
+.. autoexception:: GlpiStatusError
+   :members:
+   :show-inheritance:
+
+.. autoexception:: GlpiAuthError
+   :members:
+   :show-inheritance:
+
+.. autoexception:: GlpiNotFoundError
+   :members:
+   :show-inheritance:
+
+.. autoexception:: GlpiServerError
+   :members:
+   :show-inheritance:
+
+.. autoexception:: GlpiValidationError
+   :members:
+   :show-inheritance:
+
+.. autoexception:: GlpiProtocolError
+   :members:
+   :show-inheritance:
+
 Aggregated Models
 -----------------
 

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,15 +102,22 @@ 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
    responsibility.
 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.

docs/user_guide.rst

@@ -42,6 +42,8 @@ The guide is split into the following sections:
     context view, and the reporting helpers.
 6. **End-to-end examples** — full workflows that combine the previous
    building blocks.
+7. **Error handling** — the public exception hierarchy, what each
+   branch means, and how retries behave.
 
 The sample snippets in sections 3 to 6 use the synchronous
 :class:`GlpiClient`. Every snippet works on the asynchronous client by
@@ -1396,4 +1398,113 @@ Example output::
            'duration_by_user': {'22': 4500, '21': 1800},
            'duration_by_ticket': {120: 1800, 121: 900, 122: 1800, 123: 1800},
        },
-   }
+   }
+
+.. _error-handling:
+
+7. Error handling
+-----------------
+
+Exceptions the client raises for a bad argument, an unexpected HTTP
+status, or an unusable response body derive from
+:class:`~glpi_python_client.GlpiError`, so one handler covers that part
+of the library surface:
+
+.. code-block:: python
+
+   from glpi_python_client import GlpiClient, GlpiError
+
+   client = GlpiClient.from_env()
+   try:
+       ticket = client.get_ticket(42)
+   except GlpiError as exc:
+       print(f"GLPI call failed: {exc}")
+
+This is not yet the client's entire failure surface. The client is still
+built on ``requests``, and network-level faults -- connection failures,
+DNS errors, timeouts -- still propagate as ``requests`` exceptions today
+rather than a :class:`~glpi_python_client.GlpiError` subclass. Catch
+``requests.RequestException`` alongside :class:`~glpi_python_client.GlpiError`
+if you need to handle those too:
+
+.. code-block:: python
+
+   import requests
+   from glpi_python_client import GlpiClient, GlpiError
+
+   client = GlpiClient.from_env()
+   try:
+       ticket = client.get_ticket(42)
+   except (GlpiError, requests.RequestException) as exc:
+       print(f"GLPI call failed: {exc}")
+
+A handful of sites also deliberately still raise bare ``RuntimeError``
+(using a closed client, a missing v1 document session, a partially
+failed knowledge-base write) or ``TypeError`` (a malformed environment
+value) instead of a library type, so ``except RuntimeError`` / ``except
+TypeError`` code written against earlier releases keeps working.
+
+The hierarchy lets you narrow as far as you need:
+
+.. code-block:: text
+
+   GlpiError
+   ├── GlpiTransportError      reserved for the httpx transport swap;
+   │   └── GlpiTimeoutError    not raised yet -- see the note above
+   ├── GlpiStatusError         GLPI answered with an unexpected status
+   │   ├── GlpiAuthError       401 / 403
+   │   ├── GlpiNotFoundError   404
+   │   └── GlpiServerError     5xx (retried up to 3 attempts before it
+   │                           reaches you)
+   ├── GlpiValidationError     the client rejected your argument
+   └── GlpiProtocolError       GLPI answered 2xx with an unusable body
+
+:class:`~glpi_python_client.GlpiStatusError` carries the diagnostics you
+usually want:
+
+.. code-block:: python
+
+   from glpi_python_client import GlpiNotFoundError
+
+   try:
+       ticket = client.get_ticket(999999)
+   except GlpiNotFoundError as exc:
+       print(exc.status_code)    # 404
+       print(exc.url)            # the absolute URL that was requested
+       print(exc.response_text)  # the response body
+
+.. note::
+
+   :class:`~glpi_python_client.GlpiStatusError`,
+   :class:`~glpi_python_client.GlpiValidationError` and
+   :class:`~glpi_python_client.GlpiProtocolError` also inherit
+   :class:`ValueError`. Code written against earlier releases, which
+   raised bare ``ValueError``, keeps working unchanged.
+
+Retry behaviour
+~~~~~~~~~~~~~~~
+
+Each transport and v1-session retry decorator retries a server error
+(5xx) up to 3 attempts with a 3-second fixed wait before
+:class:`~glpi_python_client.GlpiServerError` reaches you. Client errors
+(4xx) are never retried — they cannot succeed on a second attempt.
+
+OAuth token acquisition follows the same 3-attempt policy, with one
+exception: refreshing an already-issued token does not raise directly on
+a failed response. It logs a warning and falls through to a fresh token
+acquisition, which carries its own independent 3-attempt retry
+decorator. The refresh method's own retry decorator only retries a
+network-level fault on the refresh request itself (a
+``requests.RequestException`` raised before any response is received) —
+it does **not** retry a :class:`~glpi_python_client.GlpiServerError`
+from the fall-through, since that failure is already being retried by
+the nested acquisition call. A persistent 5xx encountered while
+refreshing therefore costs exactly 1 refresh POST + up to 3 nested
+acquisition POSTs = 4 POST requests before
+:class:`~glpi_python_client.GlpiServerError` reaches you. A rejected
+credential (401/403) is not retried at either layer and fails after at
+most 2 POST requests.
+
+Search methods are deliberately tolerant: ``search_tickets`` and its
+siblings return an empty list rather than raising when GLPI rejects the
+query. Methods that fetch or mutate one specific record always raise.