fix(feedback): parse JSON after checking status code #103167
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ) | ||
| return None | ||
|
|
||
| response_data = response.json() |
There was a problem hiding this comment.
Bug: Unhandled JSON Errors Violate Contract
Moving response.json() outside the try/except block means JSON parsing errors will now raise an unhandled exception instead of returning None as documented. The function's contract states it "Returns None if the request fails", but a JSONDecodeError will now propagate to callers instead of being gracefully handled.
There was a problem hiding this comment.
Yeah... Should I wrap all the response.json() in their own try/catch??
There was a problem hiding this comment.
Imo we shouldnt do more try catches. We've been trying to move away from the blanket exc handlers when making seer requests, endpoints have their own handlers that should work fine
| {"detail": "Failed to generate user feedback label groups"}, status=500 | ||
| ) | ||
| label_groups = response_data["data"] | ||
| label_groups = response.json()["data"] |
There was a problem hiding this comment.
Bug: JSON data errors now unhandled.
Moving response.json()["data"] outside the try/except block means JSON parsing or key access errors will raise an unhandled exception instead of being caught and returning the intended 500 error response with proper logging. Previously these errors were caught by the exception handler at lines 222-225.
|
|
||
| # Guaranteed to be a list of strings (validated in Seer) | ||
| return labels | ||
| return response.json()["data"]["labels"] |
There was a problem hiding this comment.
Bug: Error Handling Removes Vital Debugging Context
Moving response.json()["data"]["labels"] outside the try/except block means JSON parsing or key access errors will now raise without the contextual logging that previously occurred. Before, such failures were logged with "Seer failed to generate user feedback labels" before re-raising, providing important debugging context that is now lost.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #103167 +/- ##
===========================================
- Coverage 80.70% 80.67% -0.03%
===========================================
Files 9226 9207 -19
Lines 394041 393528 -513
Branches 25104 24999 -105
===========================================
- Hits 317995 317483 -512
- Misses 75598 75620 +22
+ Partials 448 425 -23 |
fixes a bunch of noisy `JSONDecodeErrors` in title gen, label gen, and spam detection that were happening because we tried to JSON decode 500 responses: https://sentry.sentry.io/issues/6977994202/?query=%22jsondecodeerror%22&referrer=issue-stream https://sentry.sentry.io/issues/6978011530/?query=%22jsondecodeerror%22&referrer=issue-stream https://sentry.sentry.io/issues/6978011522/?query=%22jsondecodeerror%22&referrer=issue-stream https://sentry.sentry.io/issues/6978011504/?query=%22jsondecodeerror%22&referrer=issue-stream `response.json()` calls are now outside try/catch which SHOULD be safe because by the time we get here, the status code is 200. This pattern matches what's currently in [`organization_feedback_summary.py`](https://github.com/getsentry/sentry/blob/b91b6e10dd3812b081ca3b317b8923bca709e796/src/sentry/feedback/endpoints/organization_feedback_summary.py#L64).
fixes a bunch of noisy
JSONDecodeErrorsin title gen, label gen, and spam detection that were happening because we tried to JSON decode 500 responses:https://sentry.sentry.io/issues/6977994202/?query=%22jsondecodeerror%22&referrer=issue-stream
https://sentry.sentry.io/issues/6978011530/?query=%22jsondecodeerror%22&referrer=issue-stream
https://sentry.sentry.io/issues/6978011522/?query=%22jsondecodeerror%22&referrer=issue-stream
https://sentry.sentry.io/issues/6978011504/?query=%22jsondecodeerror%22&referrer=issue-stream
response.json()calls are now outside try/catch which SHOULD be safe because by the time we get here, the status code is 200. This pattern matches what's currently inorganization_feedback_summary.py.