Make sure to report server side error message #926
Conversation
WalkthroughThe changes involve updates to the error handling and output messaging within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SubsetCommand
participant HttpClient
User->>SubsetCommand: Execute command
SubsetCommand->>HttpClient: Send request
HttpClient-->>SubsetCommand: Response (status code)
alt status code 422
SubsetCommand-->>User: Display error message (simplified)
else conflicting options
SubsetCommand-->>User: Display conflict message
else status code 400 or higher
HttpClient-->>SubsetCommand: Extract reason from JSON
SubsetCommand-->>User: Display error message (from JSON)
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
tests/utils/test_http_client.py (2)
47-49: Enhance test method documentation.While the docstring provides a basic description, it would be more helpful to include:
- The specific scenario being tested (500 error with custom reason)
- Expected behavior (error propagation)
- The relationship with the HTTP client changes
- def test_reason(self): - '''make sure we correctly propagate error message from the server''' + def test_reason(self): + ''' + Verify that server error messages are correctly propagated to the client. + + Tests that when the server returns a 500 error with a custom reason, + the error message is: + 1. Accessible via response.reason + 2. Included in the exception message when raise_for_status() is called + '''
54-58: Make exception testing more specific.The current test could be more robust by:
- Asserting the specific exception type
- Verifying the complete error message format
- try: - res.raise_for_status() - self.fail("should have raised") - except Exception as e: - self.assertIn("Welp", str(e)) + with self.assertRaises(requests.exceptions.HTTPError) as cm: + res.raise_for_status() + + error_msg = str(cm.exception) + self.assertIn("500 Server Error", error_msg) + self.assertIn("Welp", error_msg)launchable/utils/http_client.py (2)
107-118: LGTM! Consider these minor improvements.The error handling implementation looks good and aligns well with the PR objective. A few suggestions for improvement:
- Consider using
contextlib.suppressfor cleaner exception handling:+from contextlib import suppress if response.status_code >= 400 and response.headers.get("Content-Type", "").startswith("application/json"): - try: - response.reason = response.json().get("reason", response.reason) - except json.JSONDecodeError: - pass + with suppress(json.JSONDecodeError): + response.reason = response.json().get("reason", response.reason)
- Consider adding type hints for the response object to improve code maintainability:
from requests import Response def request(self, ...) -> Response:🧰 Tools
🪛 Ruff
114-117: Use
contextlib.suppress(json.JSONDecodeError)instead oftry-except-passReplace with
contextlib.suppress(json.JSONDecodeError)(SIM105)
108-112: Consider making the comment more concise.While the comment provides valuable context, it could be more concise:
-# because (I believe, though I could be wrong) HTTP/2 got rid of status message, our server side HTTP stack -# doesn't let us forward the status message (=response.reason), which would have been otherwise a very handy -# mechanism to reliably forward error messages. So instead, we forward JSON error response of the form -# {"reason": ...}. Backfill response.reason with this JSON error message if it exists, so that the exceptions -# thrown from response.raise_for_status() will have a meaningful message. +# HTTP/2 doesn't support status messages, so we use JSON responses with {"reason": ...} to provide detailed +# error messages. This backfills response.reason to ensure raise_for_status() throws meaningful exceptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
launchable/commands/subset.py(1 hunks)launchable/utils/http_client.py(1 hunks)tests/utils/test_http_client.py(1 hunks)
🧰 Additional context used
🪛 Ruff
launchable/utils/http_client.py
114-117: Use contextlib.suppress(json.JSONDecodeError) instead of try-except-pass
Replace with contextlib.suppress(json.JSONDecodeError)
(SIM105)
🔇 Additional comments (1)
launchable/commands/subset.py (1)
488-488: Verify if res.reason provides sufficient error details.
While using res.reason simplifies the error handling, please verify that it provides the same level of detail as the previous implementation that used res.json().get("reason"). Some APIs might include more detailed error information in the JSON response body.
Let's verify the error details from both sources:
tests/utils/test_http_client.py
Outdated
| def test_reason(self): | ||
| '''make sure we correctly propagate error message from the server''' | ||
| cli = _HttpClient() | ||
| res = cli.request("GET", "error") | ||
| self.assertEqual(res.status_code, 500) | ||
| self.assertEqual(res.reason, "Welp") | ||
|
|
||
| try: | ||
| res.raise_for_status() | ||
| self.fail("should have raised") | ||
| except Exception as e: | ||
| self.assertIn("Welp", str(e)) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the code inspection, I can now provide the final response:
Additional test coverage needed for HTTP error handling
The HTTP client handles various error scenarios that should be tested:
- Client errors (400-level status codes) with JSON error responses
- Server errors in the retry list (429, 500, 502, 503, 504)
- Error responses with missing or malformed JSON in the response body
Key areas to test:
- Error handling in
response.json().get("reason", response.reason)for both valid and invalid JSON - Retry behavior for status codes in
status_forcelist=[429, 500, 502, 503, 504] - Different Content-Type header scenarios when status code >= 400
🔗 Analysis chain
Consider adding more test cases for comprehensive coverage.
The current test only verifies the 500 error case. Consider adding tests for:
- Different error status codes (400, 401, 403, etc.)
- Different response formats (missing reason, malformed JSON)
- Edge cases in error message handling
Let me check the HTTP client implementation to understand what status codes it handles.
Let me check if there are any other test files that might cover different status codes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if we have test coverage for different status codes
rg -A 5 "status_code.*[45][0-9]{2}" tests/utils/test_http_client.py
Length of output: 254
Script:
#!/bin/bash
# Check the HTTP client implementation for status code handling
ast-grep --pattern 'class _HttpClient:
$$$
'
Length of output: 7659
Script:
#!/bin/bash
# Check for other test files and their HTTP status code coverage
rg -l "status_code.*[45][0-9]{2}" tests/
Length of output: 74
kohsuke
force-pushed
the
error-message-capture
branch
from
fe216f1 to
1a0211c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
launchable/utils/http_client.py (1)
113-118: Consider using contextlib.suppress for cleaner exception handling.The current try-except-pass block can be simplified using contextlib.suppress.
+from contextlib import suppress + if response.status_code >= 400 and response.headers.get("Content-Type", "").startswith("application/json"): - try: - response.reason = response.json().get("reason", response.reason) - except json.JSONDecodeError: - pass + with suppress(json.JSONDecodeError): + response.reason = response.json().get("reason", response.reason)🧰 Tools
🪛 Ruff
114-117: Use
contextlib.suppress(json.JSONDecodeError)instead oftry-except-passReplace with
contextlib.suppress(json.JSONDecodeError)(SIM105)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
launchable/commands/subset.py(1 hunks)launchable/utils/http_client.py(1 hunks)tests/utils/test_http_client.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- launchable/commands/subset.py
- tests/utils/test_http_client.py
🧰 Additional context used
🪛 Ruff
launchable/utils/http_client.py
114-117: Use contextlib.suppress(json.JSONDecodeError) instead of try-except-pass
Replace with contextlib.suppress(json.JSONDecodeError)
(SIM105)
🔇 Additional comments (2)
launchable/utils/http_client.py (2)
108-112: LGTM! Well-documented error handling approach.
The comment clearly explains the HTTP/2 limitation and the chosen solution using JSON error responses. This is a robust approach to ensure meaningful error messages are propagated.
113-116: Verify error handling coverage.
The implementation looks solid, but let's verify the error handling coverage across different scenarios.
✅ Verification successful
Let me analyze the current HTTP client implementation more thoroughly.
Error handling implementation is comprehensive and correctly implemented
The error handling implementation in http_client.py is well-designed and properly integrated:
- It correctly handles HTTP error responses (status >= 400) by attempting to extract detailed error messages from JSON responses
- The implementation is the single source of HTTP client logic in the codebase
- Error handling is consistently used across all API interactions
- The fallback to original
response.reasonis maintained when JSON parsing fails - The implementation properly handles content type verification before JSON parsing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent error handling patterns and potential edge cases
# Test 1: Check if there are other places in the codebase that might need similar error handling
echo "Checking for other HTTP client implementations or response handling..."
rg -l "response.*status_code" --type py
# Test 2: Verify error message handling consistency
echo "Checking for other error message handling patterns..."
rg "\.reason\s*=" --type py
# Test 3: Look for potential JSON response handling
echo "Checking for other JSON response processing..."
rg "\.json\(\)" --type py
Length of output: 3599
Script:
#!/bin/bash
# Let's examine the complete HTTP client implementation to understand the error handling context
echo "Checking the complete HTTP client implementation..."
cat launchable/utils/http_client.py
echo -e "\nChecking for response status code handling patterns..."
rg "status_code" -B 2 -A 2 --type py
Length of output: 15089
kohsuke
force-pushed
the
error-message-capture
branch
from
1a0211c to
19726ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
launchable/utils/http_client.py (2)
107-118: LGTM! Consider adding debug logging for JSON parsing failures.The error handling enhancement effectively addresses the HTTP/2 limitation by extracting error messages from JSON responses. The implementation is robust with proper content type checking and fallback handling.
Consider adding debug logging when JSON parsing fails to help with troubleshooting:
if response.status_code >= 400 and response.headers.get("Content-Type", "").startswith("application/json"): try: response.reason = response.json().get("reason", response.reason) except json.JSONDecodeError: + Logger().debug(f"Failed to parse error response JSON: {response.text}") pass🧰 Tools
🪛 Ruff
114-117: Use
contextlib.suppress(json.JSONDecodeError)instead oftry-except-passReplace with
contextlib.suppress(json.JSONDecodeError)(SIM105)
114-117: Consider usingcontextlib.suppressfor cleaner exception handling.Replace the try-except-pass block with
contextlib.suppressfor more idiomatic Python code.+from contextlib import suppress + if response.status_code >= 400 and response.headers.get("Content-Type", "").startswith("application/json"): - try: + with suppress(json.JSONDecodeError): response.reason = response.json().get("reason", response.reason) - except json.JSONDecodeError: - pass🧰 Tools
🪛 Ruff
114-117: Use
contextlib.suppress(json.JSONDecodeError)instead oftry-except-passReplace with
contextlib.suppress(json.JSONDecodeError)(SIM105)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
launchable/commands/subset.py(1 hunks)launchable/utils/http_client.py(1 hunks)tests/utils/test_http_client.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- launchable/commands/subset.py
- tests/utils/test_http_client.py
🧰 Additional context used
🪛 Ruff
launchable/utils/http_client.py
114-117: Use contextlib.suppress(json.JSONDecodeError) instead of try-except-pass
Replace with contextlib.suppress(json.JSONDecodeError)
(SIM105)
kohsuke
force-pushed
the
error-message-capture
branch
from
19726ca to
d9ee0ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
launchable/utils/http_client.py (3)
113-117: Consider these improvements to the error handling block.
- Use
contextlib.suppressfor more idiomatic Python:- try: - response.reason = response.json().get("reason", response.reason) - except json.JSONDecodeError: - pass + from contextlib import suppress + with suppress(json.JSONDecodeError): + response.reason = response.json().get("reason", response.reason)
- Make the content type check more robust:
- if response.status_code >= 400 and response.headers.get("Content-Type", "").startswith("application/json"): + if (response.status_code >= 400 and + "application/json" in response.headers.get("Content-Type", "").lower()):🧰 Tools
🪛 Ruff
114-117: Use
contextlib.suppress(json.JSONDecodeError)instead oftry-except-passReplace with
contextlib.suppress(json.JSONDecodeError)(SIM105)
113-117: Add debug logging for JSON parsing failures.Consider adding debug logging when JSON parsing fails to help troubleshoot server response issues:
try: response.reason = response.json().get("reason", response.reason) except json.JSONDecodeError: + Logger().debug( + f"Failed to parse error response as JSON: {response.text[:200]}" + ) pass🧰 Tools
🪛 Ruff
114-117: Use
contextlib.suppress(json.JSONDecodeError)instead oftry-except-passReplace with
contextlib.suppress(json.JSONDecodeError)(SIM105)
113-117: Consider adding type hints for better code maintainability.Add type hints for the response object to improve code maintainability and IDE support:
+from requests import Response + def request(self, ...) -> Response: # ... response = self.session.request(...)🧰 Tools
🪛 Ruff
114-117: Use
contextlib.suppress(json.JSONDecodeError)instead oftry-except-passReplace with
contextlib.suppress(json.JSONDecodeError)(SIM105)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
launchable/commands/subset.py(1 hunks)launchable/utils/http_client.py(1 hunks)tests/utils/test_http_client.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- launchable/commands/subset.py
🧰 Additional context used
🪛 Ruff
launchable/utils/http_client.py
114-117: Use contextlib.suppress(json.JSONDecodeError) instead of try-except-pass
Replace with contextlib.suppress(json.JSONDecodeError)
(SIM105)
🔇 Additional comments (5)
tests/utils/test_http_client.py (3)
5-6: LGTM!
The Session import is appropriately placed and necessary for the new test implementation.
49-63: LGTM! Well-structured test implementation.
The test effectively verifies the error message propagation from server to client, with good coverage of both response properties and exception handling.
49-63: Consider expanding test coverage for comprehensive error handling.
While this test effectively covers the 500 error case, consider adding tests for:
- Different error status codes (400, 401, 403)
- Various response formats (missing reason field, malformed JSON)
- Retry behavior for status codes in retry list (429, 502, 503, 504)
Would you like me to help draft additional test cases?
launchable/utils/http_client.py (2)
108-112: LGTM! Well-documented workaround for HTTP/2 limitations.
The comment clearly explains the rationale behind using JSON for error messages due to HTTP/2's limitations with status messages.
113-117: Verify error message propagation across different scenarios.
Let's verify the error handling behavior across different scenarios.
✅ Verification successful
Let me gather more context about the error handling implementation and tests to ensure proper verification.
Let me check the test cases specifically to verify the error handling behavior.
Error handling implementation is properly tested and propagated
The error handling implementation has been verified through the following findings:
- The
test_reasonmethod inHttpClientTestspecifically tests the error message propagation against a real endpoint (/intake/error) and verifies both the status code and custom reason. - The error handling is properly propagated through the client hierarchy:
_HttpClientimplements the base error handlingLaunchableClientandTrackingClientextend this with additional error tracking capabilities
- The implementation correctly handles JSON parsing failures with a fallback to the original reason.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent error handling patterns across the codebase
# Look for other places where HTTP response errors are handled
echo "Checking for other HTTP error handling patterns:"
rg -l "response\.(raise_for_status|reason)" --type py
# Look for similar JSON error response handling
echo "Checking for similar JSON error response handling:"
rg "json\(\).get\(['\"]reason" --type py
# Look for potential places where this error handling might be needed
echo "Checking for other HTTP client implementations:"
rg "class.*Client.*:|def.*request.*\(" --type py
Length of output: 1031
Script:
#!/bin/bash
# Check the test implementation for error handling scenarios
echo "Checking test implementation for error handling:"
rg -A 10 "def test.*error" tests/utils/test_http_client.py
# Look at the actual implementation of the HTTP client error handling
echo "Checking HTTP client error handling implementation:"
rg -A 10 "if response.status_code >= 400" launchable/utils/http_client.py
# Check if other clients extend or use this error handling
echo "Checking client relationships:"
rg "from.*http_client import|_HttpClient" --type py
Length of output: 3523
Script:
#!/bin/bash
# Get the content of the HTTP client test file
echo "Content of test_http_client.py:"
cat tests/utils/test_http_client.py
# Check implementation of the clients that use _HttpClient
echo "Checking TrackingClient implementation:"
cat launchable/utils/tracking.py
echo "Checking LaunchableClient implementation:"
cat launchable/utils/launchable_client.py
Length of output: 9272
🧰 Tools
🪛 Ruff
114-117: Use contextlib.suppress(json.JSONDecodeError) instead of try-except-pass
Replace with contextlib.suppress(json.JSONDecodeError)
(SIM105)
kohsuke
force-pushed
the
error-message-capture
branch
from
d9ee0ab to
0da7d94
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
launchable/utils/http_client.py (1)
114-117: Consider usingcontextlib.suppressfor cleaner error handling.The try-except-pass pattern can be simplified.
+from contextlib import suppress ... - try: - response.reason = response.json().get("reason", response.reason) - except json.JSONDecodeError: - pass + with suppress(json.JSONDecodeError): + response.reason = response.json().get("reason", response.reason)🧰 Tools
🪛 Ruff
114-117: Use
contextlib.suppress(json.JSONDecodeError)instead oftry-except-passReplace with
contextlib.suppress(json.JSONDecodeError)(SIM105)
launchable/commands/subset.py (2)
Line range hint
12-24: Consider improving warning message readabilityThe warning messages could be more readable by using consistent formatting and clearer separation between different options.
Consider this format for the warning message:
- msg = ( - "WARNING: --observation and --output-exclusion-rules are set. " - "No output will be generated." - ) + msg = ( + "WARNING: The following options are in conflict:\n" + " - --observation\n" + " - --output-exclusion-rules\n" + "\nNo output will be generated." + )
Line range hint
12-582: Consider refactoring for better code organizationThe current implementation could benefit from several structural improvements:
- The
subsetfunction has too many parameters which makes it hard to maintain- Error handling is scattered throughout the code
- The
Optimizeclass should be in its own fileConsider these improvements:
- Create a configuration class to encapsulate command options:
@dataclass class SubsetConfig: target: Optional[PercentageType] session: Optional[str] base_path: Optional[str] # ... other options @classmethod def from_click_context(cls, ctx: click.Context) -> 'SubsetConfig': return cls(**ctx.params)
- Move the
Optimizeclass to a separate file:# launchable/commands/subset_optimizer.py class Optimize(TestPathWriter): def __init__(self, app: Application, config: SubsetConfig): self.config = config # ... rest of the implementation
- Centralize error handling:
# launchable/utils/error_handler.py class ErrorHandler: @staticmethod def handle_api_error(res: Response, tracking_client: TrackingClient) -> None: if res.status_code == 422: msg = f"Error: {res.reason}" tracking_client.send_error_event( event_name=Tracking.ErrorEvent.USER_ERROR, stack_trace=msg, ) click.echo(click.style(msg, fg="red"), err=True) sys.exit(1)This refactoring would improve:
- Code organization and maintainability
- Error handling consistency
- Testing capabilities
- Type safety
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
launchable/commands/subset.py(1 hunks)launchable/utils/http_client.py(1 hunks)tests/utils/test_http_client.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/utils/test_http_client.py
🧰 Additional context used
🪛 Ruff
launchable/utils/http_client.py
114-117: Use contextlib.suppress(json.JSONDecodeError) instead of try-except-pass
Replace with contextlib.suppress(json.JSONDecodeError)
(SIM105)
🔇 Additional comments (3)
launchable/utils/http_client.py (2)
107-118: LGTM! Error handling enhancement looks secure and robust.
The implementation safely extracts error messages from JSON responses while:
- Only using the predefined "reason" field
- Properly validating Content-Type
- Maintaining fallback to default reason
🧰 Tools
🪛 Ruff
114-117: Use contextlib.suppress(json.JSONDecodeError) instead of try-except-pass
Replace with contextlib.suppress(json.JSONDecodeError)
(SIM105)
107-118: Verify error message propagation with retry mechanism.
The error handling changes look good, but let's verify how error messages propagate when requests are retried.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Error handling with retry mechanism is properly implemented
The changes to error message propagation work well with the retry mechanism because:
- The retry configuration is properly set up with specific status codes (429, 500, 502, 503, 504) and allowed methods
- The error message enhancement happens after all retry attempts, just before returning the final response
- The error handling is consistent across the codebase with proper exception propagation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if error handling is consistent across retry attempts
# and verify no unintended interactions with retry logic
# Test 1: Check retry configuration
rg -A 10 'Retry\(' .
# Test 2: Look for other error handling in the retry path
rg -A 5 'raise_for_status|response\.reason'
# Test 3: Check for any custom exception handling around retries
ast-grep --pattern 'try {
$$$
raise_for_status()
$$$
}'
Length of output: 9315
🧰 Tools
🪛 Ruff
114-117: Use contextlib.suppress(json.JSONDecodeError) instead of try-except-pass
Replace with contextlib.suppress(json.JSONDecodeError)
(SIM105)
launchable/commands/subset.py (1)
Line range hint 488-496: LGTM: Improved error handling for validation errors
The simplified error handling now uses the HTTP reason directly and properly tracks the error event while providing clear user feedback.
Summary by CodeRabbit
New Features
subsetcommand, providing clearer feedback on command conflicts and API response issues.Bug Fixes
Tests