fix: log S3 response body on cache upload failure - #117
Conversation
raise_for_status() only gives "403 Forbidden" — the S3 error body (AccessDenied, Request has expired, SignatureDoesNotMatch, etc.) is the only place that says why. Read it off exc.response.text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018cayDdcs1iuui8Pgh5npKm
|
📦 Python package built successfully!
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
==========================================
+ Coverage 74.42% 74.46% +0.04%
==========================================
Files 95 95
Lines 5704 5707 +3
Branches 850 851 +1
==========================================
+ Hits 4245 4250 +5
+ Misses 1182 1180 -2
Partials 277 277
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
🚀 Review App Deployment Started
|
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018cayDdcs1iuui8Pgh5npKm
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughSQL cache upload failures now format Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/unit/test_sql_caching.py (2)
401-401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required docstrings and type hints.
The two new test methods have no docstrings, parameter annotations, or
-> Nonereturn annotations. Add short docstrings and explicit annotations formock_put,mock_logger, and the return value.As per coding guidelines: “Use docstrings for all functions/classes” and “Use explicit type hints for function parameters and return values.”
Also applies to: 421-421
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_sql_caching.py` at line 401, Update the new test methods test_http_error_logs_response_body and the method at the additional referenced location to include short docstrings, explicit type annotations for mock_put and mock_logger, and a -> None return annotation, following the annotations already used by neighboring tests.Source: Coding guidelines
401-417: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest the 500-character response-body limit.
The HTTP test uses a short body. Add a response body longer than 500 characters and assert that only the first 500 characters are logged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_sql_caching.py` around lines 401 - 417, Update test_http_error_logs_response_body to use an HTTP error response body exceeding 500 characters, then assert the logged body contains exactly its first 500 characters and excludes content beyond that limit while preserving the existing status-code and URL-redaction assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/test_sql_caching.py`:
- Around line 402-410: Update the mocked HTTP response in the relevant SQL cache
upload test to set response.url to upload_url, reuse upload_url for
upload_sql_cache, and inspect repr(mock_logger.error.call_args) to assert
upload_url is absent, ensuring the complete logger call is redacted.
---
Nitpick comments:
In `@tests/unit/test_sql_caching.py`:
- Line 401: Update the new test methods test_http_error_logs_response_body and
the method at the additional referenced location to include short docstrings,
explicit type annotations for mock_put and mock_logger, and a -> None return
annotation, following the annotations already used by neighboring tests.
- Around line 401-417: Update test_http_error_logs_response_body to use an HTTP
error response body exceeding 500 characters, then assert the logged body
contains exactly its first 500 characters and excludes content beyond that limit
while preserving the existing status-code and URL-redaction assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa53193b-e4f1-4bb4-aac8-ef84ff026c06
📒 Files selected for processing (2)
deepnote_toolkit/sql/sql_caching.pytests/unit/test_sql_caching.py
The test asserted the presigned upload URL stays out of the log, but never set response.url on the mock response. raise_for_status() interpolates response.url into str(exc), so with it unset the exception string read "for url: None" and the assertion passed against the pre-fix code too. Setting response.url makes the assertion bite: verified it now fails against the old str(exc) logging and passes with the status+body format. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018cayDdcs1iuui8Pgh5npKm
3 participants
raise_for_status()only surfaces403 Forbidden— the S3 XML error body is the only place that distinguishesAccessDenied/Request has expired/SignatureDoesNotMatch/ expired presigned URL. This readsexc.response.textso the reason reaches the log.Before
Status code only, no reason. Presigned URL with signing params leaked into the log.
After
S3 error body with the actual reason. Presigned URL no longer logged for HTTP errors.
Non-HTTP exceptions (ConnectionError, Timeout) still fall through to
str(exc)— unchanged from main.🤖 Generated with Claude Code
https://claude.ai/code/session_018cayDdcs1iuui8Pgh5npKm
Summary by CodeRabbit
Bug Fixes
Tests