Skip to content

Better 400 logging#90

Merged
akamor merged 3 commits intomainfrom
f/better-400-logging
Dec 4, 2025
Merged

Better 400 logging#90
akamor merged 3 commits intomainfrom
f/better-400-logging

Conversation

@akamor
Copy link
Copy Markdown
Contributor

@akamor akamor commented Dec 3, 2025

No description provided.

@akamor akamor requested a review from JSandler December 3, 2025 19:52
Comment on lines +208 to +218
error_message = json.dumps(err.response.json())
except: # noqa: E722
pass

try:
if error_message == '':
error_message = err.response.text
except: # noqa: E722
pass

raise TextualServerBadRequest(error_message)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: http_post raises TextualServerBadRequest, which is not caught by requests.exceptions.HTTPError in callers.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The http_post method now raises TextualServerBadRequest for 400 errors. However, calling methods like send_redact_request and send_redact_bulk_request are designed to catch requests.exceptions.HTTPError. Since TextualServerBadRequest does not inherit from requests.exceptions.HTTPError, these exceptions will not be caught by the existing try...except blocks, causing the application to crash with an unhandled exception when a 400 error occurs.

💡 Suggested Fix

Ensure TextualServerBadRequest inherits from requests.exceptions.HTTPError, or modify the calling except blocks to catch TextualServerBadRequest specifically.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: tonic_textual/classes/httpclient.py#L205-L218

Potential issue: The `http_post` method now raises `TextualServerBadRequest` for 400
errors. However, calling methods like `send_redact_request` and
`send_redact_bulk_request` are designed to catch `requests.exceptions.HTTPError`. Since
`TextualServerBadRequest` does not inherit from `requests.exceptions.HTTPError`, these
exceptions will not be caught by the existing `try...except` blocks, causing the
application to crash with an unhandled exception when a 400 error occurs.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5294566

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. It is still caught by requests.exceptions.RequestException

@akamor akamor enabled auto-merge (squash) December 4, 2025 16:10
@akamor akamor disabled auto-merge December 4, 2025 16:58
@akamor akamor merged commit a33e9c9 into main Dec 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants