fix(http-client): make get_last_response report the actual last response - #1655
fix(http-client): make get_last_response report the actual last response#1655LioriE wants to merge 3 commits into
Conversation
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersThe review assignment covers a broad spectrum of the code changes, including core logic, test coverage, and specific contributions, to ensure comprehensive review from multiple angles.
Suggested by Shuni based on git history and PR context. Names are not @-mentioned to avoid notifying anyone — request a review from whoever fits best. |
put() was the only verb not storing the verbose last-response, making any PUT invisible to get_last_response() in both the sync and async clients. DescopeResponse's derived accessors are now functools.cached_property, which also drops a re-parse when the JSON body is literally null.
361eb09 to
5fa683c
Compare
![shuni-bot[bot]](https://avatars.githubusercontent.com/in/2536750?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni's Review
Adds the missing verbose last-response capture to put() (sync + async) and converts DescopeResponse accessors to cached_property.
The put() fix is exactly right — same placement as get/post/patch/delete (before _raise_from_response, so error responses are still captured) and covered by tests on both clients. The _json_data → cached_property change is a genuine fix for the null-body re-parse. Good bones!
Sniffed out 1 issue:
- 1 🟢 LOW:
cached_propertyon the HTTP metadata accessors duplicates httpx's own caching
See inline comment for details. Woof!
Declared coverage: FULL — 5/5 changed files reviewed.
|
|
||
| # HTTP metadata properties | ||
| @property | ||
| @cached_property |
There was a problem hiding this comment.
🟢 The seven metadata accessors don't benefit from cached_property the way _json_data does — httpx already caches text, content and cookies internally (_text/_content/_cookies), and status_code/headers/url are plain attributes while is_success is a two-comparison property.
On Python 3.9–3.11 (requires-python = ">=3.9,<4.0") cached_property.__get__ takes a descriptor-wide RLock on first access per instance, so this is net slower and serializes threads across all DescopeResponse instances — on a class whose whole point is per-thread get_last_response(). It also silently turns these from read-only into assignable/shadowable attributes.
Keeping @property here (and cached_property only on _json_data) preserves the actual fix without the tradeoff.
There was a problem hiding this comment.
Still open after the rebase — the seven metadata accessors are unchanged.
To be fair about scope, since DescopeResponse is only constructed under if self.verbose with at most one live instance per thread, the lock-contention half of this is minor in practice. The sturdier half stands: text/content/cookies are already cached inside httpx (_text/_content/_cookies), and status_code/headers/url are plain attributes, so caching them buys nothing while making them assignable.
The new is_json conversion is a different story and is a genuine win — it stops re-parsing on every access for a non-JSON body, and the docstring note about failed parses not being cached is correct (cached_property stores nothing when the getter raises, so json() keeps raising as test_non_json_body_is_inspectable expects).
Not blocking either way.
There was a problem hiding this comment.
Applied in 53382c3 — the seven metadata accessors are back to plain @property.
Verified the locking claim before acting on it: on 3.10, with self.lock is present in cached_property.__get__; Python 3.12 removed it. Repo is requires-python = ">=3.9", so that covers 3.9-3.11.
cached_property stays on _json_data and is_json, which is where it actually buys something: a null body parses once instead of on every access, and a non-JSON body costs one failed parse attempt instead of one per is_json call. Both re-verified after the revert.
There was a problem hiding this comment.
Confirmed fixed in 53382c3 — all seven are plain @property again, cached_property stays on _json_data and is_json, and the import is still used so there's no dangling-import fallout. test_json_caching (which asserts mock_response.json.call_count == 1) still holds, and nothing in the suite asserted caching on the metadata accessors, so the revert is clean.
Thanks for checking the 3.10 source rather than taking my word for the lock — and the docstring now records both halves of the reasoning, which is better than what I suggested. Resolving.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-review after the rebase onto the is_json / non-JSON-body work in main. The delta since my last pass is the is_json → cached_property conversion plus the docstring note.
No new issues. The is_json conversion is a real win — it stops re-parsing a non-JSON body on every access, and the "failed parse is not cached" claim checks out, so test_non_json_body_is_inspectable and test_descope_client.py:871 still hold. put() capture is unchanged and correct on both clients.
My earlier 🟢 note on the seven metadata accessors is still open (replied inline with scope, not blocking). Good bones — carry on! Woof!
Declared coverage: FULL — 5/5 changed files reviewed.
Coverage reportThe coverage rate went from
Diff Coverage details (click to unfold)descope/http_client_async.py
descope/_http_client_base.py
descope/descope_client.py
descope/http_client.py
descope/descope_client_async.py
|
get_last_response() picked between two independently-overwritten stores with `mgmt_resp or auth_resp`, so once both had been used a stale mgmt response always shadowed a newer auth one. Neither slot knew which was written last, so any precedence rule between them was a guess. Collapse to one store per DescopeClient, injected into every HTTPClient it builds, so "last" means last and there is nothing to arbitrate. The store is threading.local for sync and a ContextVar for async because the isolation unit differs (OS thread vs asyncio task); concurrency semantics are unchanged. Also forwards verbose and the store into OutboundApplicationByToken's no_key_client, whose responses were never captured at all.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
LioriE
changed the title
There was a problem hiding this comment.
🐕 Shuni's Review
The new commit grew the PR well past its description: a shared ThreadLocal/ContextVar last-response store now replaces the mgmt_resp or auth_resp precedence, and no_key_client finally forwards verbose.
No new issues. I chased the refactor end to end: the store is built before both clients in each __init__, every HTTPClient/HTTPClientAsync construction site forwards it, nothing references the removed _thread_local/_last_response_var, the license handshake bypasses the wrapper so it can't write a spurious "last" response, and no create_task/gather inside the SDK would break ContextVar propagation. Tests cover both orderings plus per-thread and per-task isolation. Good bones!
OutboundApplicationByToken gap merely "worth flagging" — this commit fixes both. Worth updating before merge so the ticket isn't left open by mistake.
My earlier 🟢 note on the metadata accessors is still open, still non-blocking. Woof!
Declared coverage: FULL — 10/10 changed files reviewed.
The seven HTTP metadata accessors gained nothing from caching: httpx already caches text/content/cookies internally, and status_code/headers/url are attribute reads. On Python 3.9-3.11 cached_property.__get__ takes a descriptor-wide lock on first access, so caching them cost more than it saved and made read-only properties assignable. Body parsing still caches, which is where the two real fixes are: a `null` body no longer re-parses, and is_json no longer re-attempts a failed parse on every call.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Only _http_client_base.py moved since my last pass: 53382c3 reverts the seven HTTP metadata accessors to plain @property and keeps cached_property on _json_data and is_json, where it actually pays.
No issues. Verified the revert is clean — the cached_property import is still live so nothing dangles, test_json_caching (json.call_count == 1) still holds, and no test asserted caching on the metadata accessors. The PR description now correctly says Fixes: and covers all three defects, so my earlier note about the stale scope note is settled too. Clean bill of health — good dog! 🦴
resolve_thread returned permission-denied three times, so it had to be recorded as unresolved to let this review through. Please close it manually; nothing is outstanding in the code. Woof!
Declared coverage: PARTIAL — 1/10 changed files reviewed.
1 participant
Related Issues
Fixes: https://github.com/descope/etc/issues/16377
Related PRs
Related PRs
In a Nutshell
put()now captures the verbose last-response, sync + asyncget_last_response()returns the genuinely most recent responseDescopeResponsebecomesfunctools.cached_propertynullbody and on a repeatedis_jsonDescription
Three verbose-mode debug defects, all making
get_last_response()lie.put()never captured. It was the only verb that didn't store the verbose last-response, so any PUT was silently invisible — in bothdescope/http_client.py(threading.local) anddescope/http_client_async.py(ContextVar). It now mirrorsget/post/patch/delete, placed before_raise_from_responseso error responses are still captured.get_last_response()returned a stale response. It picked between two independently-overwritten stores withmgmt_resp or auth_resp, so once both the auth and management clients had been used, a stale mgmt response always shadowed a newer auth one. The root cause is not theor— it is that there were two stores for one question, and neither slot knew which was written more recently, so any precedence rule between them was a guess. Collapsed to one store perDescopeClient, injected into everyHTTPClientit builds, so "last" means last and there is nothing to arbitrate.HTTPClientstill constructs its own store when none is passed, so standalone use and theAuth-only path are unaffected.Two implementations behind the same two-method shape, because the isolation unit genuinely differs:
threading.localfor sync (many OS threads, one client) and aContextVarfor async (many tasks sharing one event-loop thread, where a thread-local would be a single slot for all of them). Concurrency semantics are unchanged, and there are tests pinning that sharing a store does not leak a response across threads or across tasks.I deliberately did not use the
(response, seq)counter sketched in the issue. It keeps N stores and makesget_last_response()enumerate them, so every futureHTTPClienthas to be remembered and registered in that comparison or its responses go silently missing — which is exactly how the third one got missed.OutboundApplicationByToken(outbound_application.py:726,_async.py:726) builds its ownno_key_clientand never forwardedverbose, so nothing it did was captured at all; it now shares the store too.DescopeResponsere-parsed the body on every access in two cases._json_dataandis_jsonare nowfunctools.cached_property. The oldif self._json_data is Noneguard meant a literalnullbody re-parsed every time, andis_json(from #1653) probes by callingjson()in atry, so on a non-JSON body it re-attempted the full parse on every call.cached_propertydoes not cache a raising getter, sojson()still raises on a non-JSON body, which is the behavior #1653 established.functools.cacheis not usable for either: it keys onself, which is unhashable here (__eq__without__hash__) and would be pinned alive forever by a module-level cache.The seven HTTP metadata accessors (
headers,status_code,cookies,text,content,url,ok) stay plain properties, per shuni's review. httpx already cachestext/content/cookiesinternally and the rest are attribute reads, so caching bought nothing — and on Python 3.9-3.11cached_property.__get__takes a descriptor-wide lock on first access, which this repo'srequires-python = ">=3.9"still covers. Verified on 3.10 before reverting.One tradeoff worth flagging: with the store shared,
HTTPClient.get_last_response()reports the last response across every client sharing that store rather than only its own, soauth_http.get_last_response()can return a mgmt response. Both docstrings now say so. The per-client answer has no consumer, and keeping a per-client slot alongside the shared one is bloat for a debug helper.Must