Refactor/httpx transport and codegen - #19
Merged
Merged
Conversation
Swaps the HTTP transport to httpx across the v2 transport, the legacy v1 session and the OAuth token manager in one change. They share _http.py, and the reason-phrase read there is covered only by the v1 tests, so porting them separately would have left the shared code validated exclusively by tests exercising the old transport. Behaviour is preserved. Three places where the libraries disagree silently needed explicit correction, all found by probing rather than by reading docs: * Query params with a None value are dropped again. httpx encodes them as a valueless `key=`, and GLPI reads an empty filter value as "match everything" -- the swap would have quietly widened queries. A test already named `test_request_params_drops_none_values` asserted the opposite of its own name; requests had been dropping them at the transport layer, so nothing noticed. * bytes and bool params keep their previous rendering. httpx emits the Python repr `b'x'` and a lowercase `true`. * Redirects are still followed; httpx does not follow them by default. Network faults now raise GlpiTransportError / GlpiTimeoutError with the transport exception attached as __cause__, delivering the contract the previous release documented but could not yet honour: catching GlpiError covers the whole failure surface and users never import the HTTP library. The retry predicates move onto that library-owned type in the same commit. This was the migration's sharpest edge: the two libraries' exception trees are disjoint, so a predicate still naming the old base matches nothing and every retry vanishes -- silently, with a green suite. Naming a type the library raises itself makes it unreproducible. Verified by mutation: reverting the predicate fails 7 tests spanning all three transports. Also drops requests/urllib3/types-requests, adds unasync as a dev dep, and untracks .coverage, which the venv coverage hook rewrote on every run and dirtied the tree. 567 passed, mypy strict clean (83 files), coverage 96.96%, sphinx -W clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Lands the machinery plan 4 needs before any of the 8,370 LOC under clients/ and auth/ moves: the build script, the two hand-written concurrency twins, the CI staleness gate, and the guard that covers what that gate structurally cannot. unasync_build.py generates glpi_python_client/_sync/ from _async/ and supports --check for CI. The substitution map is kept deliberately short -- httpx's names (unasync's built-in Async*->Sync* convention produces SyncClient, which does not exist) plus this package's own client class. Every other name is spelled identically in both trees, because each entry in that map is a chance for a silent collision. _concurrency.py is hand-written on both sides and excluded from generation. It is where the trees differ in kind rather than syntax: * gather is asyncio.gather on one side and plain sequential evaluation on the other. unasync leaves asyncio.gather intact, so a generated twin would call it from sync code. The call shape is identical because stripping await makes each argument evaluate eagerly -- which is what sequential execution is. * The auth lock cannot be substituted in either direction. A threading.Lock on the async side deadlocks: it is held across an await, so a second task blocks the loop and the holder can never resume. An asyncio.Lock on the sync side binds to the first loop that contends it and breaks cross-thread sharing -- latently, since acquire() only resolves the loop on the contended path, so uncontended use passes. The CI diff gate catches a stale _sync/ and nothing else. A token collision is deterministic, so regenerating reproduces it byte for byte, the diff stays clean and CI passes while the sync client is wrong. test_unasync_codegen.py scans for that directly. Running it over the existing 78 modules found zero genuine collisions -- the only hits are the intended renames -- which settles the dossier's one open risk that had no way of being checked before the tree existed. Codegen verified end to end on a representative sample first: async def, await, async with, async for, async generators, __aenter__/__aexit__, httpx.AsyncClient, aclose, AsyncIterator and __all__ all generate correctly, and mypy --strict accepts both trees including the duplicate module basenames. 572 passed, mypy strict clean (87 files), coverage 96.94%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GLPI's priority scale runs 1-6, where 6 is "Major"; the published contract advertises 1-5 and GlpiPriority followed the contract. GetTicket.priority is typed with that enum and pydantic validates every record in a result set, so one escalated ticket raised ValidationError and failed the entire search -- not just that row. A reporting query filtering on high priority is the most likely place to meet one. Adds GlpiPriority.MAJOR = 6, leaving the five existing members on their current identifiers so stored filters keep their meaning. urgency and impact really do stop at 5 and now accept a value GLPI will never send; accepting an impossible value is harmless, rejecting a real one was not. 574 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the async facade with genuine non-blocking I/O and collapses the two client surfaces into one codebase. AsyncGlpiClient used to wrap every synchronous method in asyncio.to_thread, so "async" meant "blocking call on a worker thread". It now runs on httpx.AsyncClient with no thread pool and no executor. glpi_python_client/_async/ is hand-written; _sync/ is generated from it by unasync_build.py, committed, and diffed in CI. Endpoint logic exists exactly once, which removes the failure mode the bridge kept producing: a sync body calling a sibling public method through self got back an un-awaited coroutine and silently did nothing. Three shipped bugs of that shape were fixed by hand this year; the shape is now unreachable. Deletes 1,235 LOC of hand-written async overrides the bridge forced into existence -- including _statistics_async.py, 500 lines duplicating the most intricate logic in the package with no test asserting the two copies agreed. Fan-out survives as a `gather` helper written once at the call site: asyncio.gather on the async surface, sequential evaluation on the generated one, because stripping `await` already makes each argument evaluate eagerly. Two findings worth recording, both from running rather than reading: * The dossier says not to substitute asyncio.Lock for threading.Lock but not what to do instead. A threading.Lock on the async surface deadlocks outright -- it is held across an await, so a second task blocks the loop and the holder can never resume to release it. _concurrency.py is hand-written per surface for that reason, and test_async_surface.py drives ten contending tasks through the token path to prove it. * build_client_resources built the session and unwound it in an except clause. That is inexpressible on the async surface: httpx.AsyncClient has no synchronous close and __init__ cannot await one. Credentials are now validated before anything is constructed, so there is nothing to unwind -- and the test asserts no session is built at all, which is the stronger property. Tests move to glpi_python_client/tests/ and keep exercising the shipped sync client. test_async_surface.py covers what the codegen cannot: that the async tree actually awaits, contends its lock without deadlocking, overlaps its fan-out, and translates faults inside an awaited try block. Coverage is measured on the generated tree only -- both trees contain the same statements, so counting both would hide real gaps behind a doubled denominator; the correspondence is enforced by --check and mypy. Public imports are unchanged. Private module paths gain a tree segment. 523 passed, mypy strict clean (119 files, both trees), coverage 96.95%, sphinx -W clean, _sync/ verified identical to regeneration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by running the async client against live preprod GLPI: three statistics integration tests failed with "object async_generator can't be used in 'await' expression". The async-ification pass had put `await` in front of `self.iter_search_tickets(...)` inside a `for` loop at two sites in _statistics.py. Worth recording why nothing else caught it. `await gen()` and `async for x in gen()` strip to the *same* correct synchronous loop, so the generated sync client was right, the diff gate was clean, mypy passed on both trees, and all 523 unit tests passed. The bug existed only on the async surface, only on paths the unit suite reaches through the sync client. A green suite proved nothing here. Adds a static guard: test_no_async_generator_is_awaited walks the async tree, collects every `async def` that yields, and fails on any `await` of one. Mutation-checked -- reintroducing either site fails the test. Also updates the async integration tests for the removed bridge. The executor-routing test is replaced rather than deleted: it asserted that a caller-supplied pool received the work, and the property that replaced it is that there is no pool at all, so the calls run on the loop thread. That is now checked directly against the live server. Live suite: 42 passed, 4 skipped (Fields plugin absent from preprod). This also settles the open question of whether preprod still accepts the v1 initSession flow after the transport swap -- it does. 524 passed, mypy strict clean (119 files), coverage 96.95%. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rewrite renamed one client module and deleted four others; the documentation did not follow. Nothing caught it. Sphinx runs with `nitpicky` off, so an unresolvable target renders as plain text rather than failing the build, and the private modules holding most of these references are never autodoc'd in the first place. The user-facing half was the worse half. The README, the user guide, the API reference, the package docstring and two skills all still described the deleted bridge: an async facade wrapping each synchronous method into a coroutine dispatched to a worker thread. The user guide's "Custom thread pools" section and step 8 of `glpi-client-setup` went further and documented an `executor=` constructor argument that no longer exists, so both examples raise `TypeError` as written. That section is now "Bounding concurrency" and shows an `asyncio.Semaphore`. Separately, the generated tree documented itself in terms of the hand-written one. unasync repoints imports, where `_async` is its own NAME token, but a dotted path inside a docstring is a single string token and survives untouched, so 47 cross-references in the shipped sync client pointed into `_async/`. The diff gate cannot see this for the same reason it cannot see a token collision: the omission is deterministic, so regeneration reproduces it and the diff stays clean. `unasync_build` now repoints the qualified prefix. Two guards keep it from recurring, both mutation-checked: * `test_docstring_references.py` resolves every qualified reference in the package against the live modules. Writing it turned up two more dead targets that reading had missed. * `test_the_generated_tree_never_names_the_async_one` asserts the sync tree never mentions `_async`. Disabling the repointing makes it report 28 offenders. Also corrects `TicketContextMixin`, which claimed its five calls ran sequentially and that an async override fanned them out, contradicting both the code and its own method docstring; and drops the `requests` intersphinx mapping, which survived the transport swap and made every docs build fetch an inventory nothing referenced. 527 unit tests pass, coverage 96.95%, mypy strict clean over 119 files, sphinx -W clean, `_sync/` verified identical to regeneration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19 +/- ##
==========================================
+ Coverage 96.89% 96.95% +0.05%
==========================================
Files 79 74 -5
Lines 2644 2330 -314
==========================================
- Hits 2562 2259 -303
+ Misses 82 71 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…d package The plugin-fields docs, examples and fixtures were written against a live GLPI instance, so its custom container names and a real ticket id shipped in both the sdist and the wheel. None of it is needed to explain the API. - rename the worked-example container/field to `extrainfo`/`extrainfofield` and the second fixture container to `secondary` - replace the real ticket id with 1234 in the shipped tests - read the integration test's ticket id and container/field names from local secrets (`glpi_fields_ticket_id`, `glpi_fields_container`, `glpi_fields_field`) instead of hardcoding them; the test skips cleanly when they are unset - use a neutral entity fragment in the user-location skill example - exclude docs/superpowers/, the API contract dump and .coverage from the build so gitignore is no longer the only thing keeping them out The container docstring lives in _async/ and was regenerated into _sync/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Enable correct async operations with httpx as transport, and using code generation to generate sync client based on the async code.
The old async client had two hard ceilings, both from defaults:
concurrent calls : min(32, cpu_count+4), asyncio's default executor -> unbounded
keep-alive connections : 10 — urllib3 pool_maxsize, with pool_block=False -> unbounded
With pool_block=False, urllib3 doesn't queue past the 10th connection, it opens a fresh socket and discards it after the response. So beyond 10-way fan-out the old client silently paid a new TCP (and, against real GLPI, TLS) handshake on most requests, forever.
Cancellation. Saturate the client with 24 slow calls, cancel them all, then issue one ordinary request: