fix(http): return last response for non-JSON bodies - #1653
Conversation
get_last_response() raised JSONDecodeError instead of returning the response when the body was not JSON (e.g. an nginx 502 HTML page): `mgmt_resp or auth_resp` evaluates truthiness, which called DescopeResponse.__bool__ -> json(). __str__ and __repr__ had the same problem, so logging the response crashed too. The inspection dunders now fall back on a parse failure instead of raising, leaving JSON-body semantics unchanged. Explicit JSON access (json(), __getitem__, get, keys, items, __len__, __iter__) still raises. Adds an is_json property, and records the last response on put(), which was missing in both the sync and async clients.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersThe selection focuses on contributors with multiple commits across core client and test files, ensuring a comprehensive review of the code changes and their impacts.
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. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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
Makes DescopeResponse inspection dunders survive non-JSON bodies (nginx 502 pages), adds is_json, and records last_response on put() in both clients. The core fix is sound — ValueError correctly covers json.JSONDecodeError, JSON-body behavior is byte-for-byte unchanged, and explicit JSON access still raises as the issue requires. Good bones! 🦴
Sniffed out 3 issues:
- 1 🟡 MEDIUM:
is not Noneremoves the accidental fall-through inget_last_response(), so a stale mgmt response now wins more often - 2 🟢 LOW: unbounded
__str__on non-JSON bodies,put()verbose capture untested
See inline comments for details. Woof!
Declared coverage: FULL — 9/9 changed files reviewed.
Coverage reportThe coverage rate went from
Diff Coverage details (click to unfold)descope/_http_client_base.py
|
bool(response) answered "is the JSON body non-empty", which meant truthiness parsed the body. It is now unconditionally True: an empty JSON body no longer reads as no-response, and truthiness never parses. Kept explicit rather than deleted, since __len__ is defined and Python would otherwise fall back to it for truthiness, reintroducing both the parse and the empty-body falsiness.
|
🐕 Uh oh! I ran into an error. Please check the logs for details. |
|
🐕 Shuni hit a problem and couldn't finish. |
Revert the get_last_response() accessor and the docs to plain truthiness now that __bool__ no longer parses the body: `mgmt_resp or auth_resp` is behaviorally identical to the explicit None check. Address review: - str() echoed the whole non-JSON body while repr() capped at 200 chars. Body size is upstream-controlled, so both now share one bounded preview and point at .text for the full body. - put() verbose capture had no test in either client, which is how it was missed in the first place. Added to both.
|
🐕 Uh oh! I ran into an error. Please check the logs for details. |
|
🐕 Shuni hit a problem and couldn't finish. |
|
🐕 Uh oh! I ran into an error. Please check the logs for details. |
|
🐕 Shuni hit a problem and couldn't finish. |
The put() last-response gap belongs to descope/etc#16377, together with the get_last_response() staleness half of that issue. Keeping it here would half-close that issue and put changes outside this bug's scope into a fix for descope/etc#17556.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
There was a problem hiding this comment.
🐕 Shuni's Review
Re-review after 91f19586. The PR is now scoped to DescopeResponse alone: __bool__ is hard-coded True, str/repr/== degrade gracefully, and _text_preview() bounds the non-JSON echo at 200 chars. All three of my earlier findings are addressed — the truncation and the equality/truthiness semantics all check out, and the if response: / mgmt_resp or auth_resp call sites left on main are now safe rather than crashing. Good bones! 🦴
Sniffed out 0 code issues — no new inline findings survived verification.
One non-code note: 91f19586 reverted the put() verbose capture (impl and tests, both clients), but the description still says "put() now records the last response, in both clients" and "this PR fixes the put() verbose-capture half" of descope/etc#16377. put() on main still has no capture — worth updating the body so 16377 isn't closed early.
Heads up: my resolve_thread calls came back permission-denied, so the three prior threads stay open on GitHub despite being addressed. Replies are on each. Woof!
Declared coverage: FULL — 4/4 changed files reviewed.
|
@omercnet can u take a look? small fix |
2 participants
Related Issues
Fixes: https://github.com/descope/etc/issues/17556
In a Nutshell
get_last_response()no longer raises on non-JSON bodiesbool(response)now means "a response exists", and never parses the bodystr/repr/==degrade gracefully instead of raisingstr/reprbound the echoed non-JSON body at 200 charsis_jsonpropertyDescription
client.get_last_response()raisedJSONDecodeErrorinstead of returning the response when the body was not JSON, for example an nginx 502 HTML page.mgmt_resp or auth_respevaluates truthiness, which calledDescopeResponse.__bool__->json().__str__and__repr__had the same problem, so even logging the returned object crashed, which is exactly the production logging path the customer hit.__bool__now returnsTrueunconditionally. Truthiness answers "did I get a response", not "is the body non-empty", so it no longer touches the body at all. It stays explicitly defined rather than deleted, because__len__exists and Python would fall back to it for truthiness, reintroducing the parse.__str__,__repr__and__eq__catchValueError(json.JSONDecodeErrorsubclasses it) and fall back to a bounded text preview, astatus_code=... text=...summary, and identity. Explicit JSON access (json(),__getitem__,get,keys,items,__len__,__iter__) still raises, per the issue's expected behavior.The non-JSON preview is capped at 200 chars in both
__str__and__repr__, since the body of an upstream error page is not size-controlled by us and the motivating path is logging it once per failed request..textstill returns the full body.No caller-side change was needed: with
__bool__fixed,get_last_response()returns the response through the existingmgmt_resp or auth_resp, andif response:in the docs is a correct presence check again (a missing response isNone, which stays falsy).Behavior change worth a look in review: a response with an empty JSON body (
{}) used to be falsy and is now truthy. That was the old dict-emulation semantics. Code doingif resp:to mean "the body had content" would needif resp.json():. Reachable only viaget_last_response()under opt-inverbose=True, which shipped in 1.10.0.Must