Feat/real async io#16
Open
baraline wants to merge 21 commits into
Open
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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.
Fix two live AsyncGlpiClient bugs sharing one root cause: the async bridge wraps public sync methods into coroutines, so a sync body calling a sibling public method through self silently dropped the call. - create_kb_article/update_kb_article silently discarded categories (article created, id returned, no exception, category never linked) - get_ticket_custom_fields/set_ticket_custom_fields raised TypeError Adds a structural AST guard so the bug class cannot be reintroduced. Unreleased: version stays 0.3.4. The next release is 0.4.0 (httpx + unasync), which removes the bridge and makes this class of bug impossible. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
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.
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.
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.
…r 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 <noreply@anthropic.com>
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.
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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.
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.
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 <noreply@anthropic.com>
_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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16 +/- ##
==========================================
+ Coverage 97.18% 97.39% +0.20%
==========================================
Files 77 79 +2
Lines 2419 2536 +117
==========================================
+ Hits 2351 2470 +119
+ Misses 68 66 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add new error contract, async-bridge fixes, integration-suite repairs