fix: Added Json error handling instead of HTML #631
Conversation
|
|
WalkthroughThe changes introduce two new error handling functions within the Flask application. The Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as Analytics Server (Flask App)
C->>A: Send Request
A->>A: Process Request
alt HTTPException Occurs
A->>A: handle_http_exception(e)
A->>C: Return JSON (error message, status code, path)
else Other Exception Occurs
A->>A: handle_exception(e)
alt Debug Mode Enabled
A->>A: Append traceback to response
end
A->>C: Return JSON (error message, status code 500, path, exception type)
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
harshit078
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/analytics_server/app.py (1)
73-74: Check error handler registration order.The order of error handler registration might lead to unexpected behavior. Since
Exceptionis registered first andHTTPExceptionis a subclass ofException, the generic handler might catch HTTP exceptions before the specific handler.Consider reversing the registration order:
-app.register_error_handler(Exception, handle_exception) -app.register_error_handler(HTTPException, handle_http_exception) +app.register_error_handler(HTTPException, handle_http_exception) +app.register_error_handler(Exception, handle_exception)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/analytics_server/app.py(2 hunks)
🔇 Additional comments (4)
backend/analytics_server/app.py (4)
3-5: Appropriate import additions for error handling.The new imports are necessary for the error handling implementation:
jsonifyandrequestfrom Flask for JSON responses,HTTPExceptionfrom werkzeug for specific HTTP error handling, andtracebackfor detailed error reporting.
41-53: Well-implemented HTTP exception handler.This handler appropriately converts HTTP exceptions into structured JSON responses, which aligns with the PR's objective of improving error handling. The implementation includes all relevant information (error flag, message, status code, and path) and correctly sets the response status code to match the exception.
54-72: Comprehensive generic exception handler with proper security considerations.The implementation provides detailed error information while being security-conscious by only including traceback details in debug mode. The structured JSON response maintains consistency with the HTTP exception handler format.
77-77: Good practice to explicitly convert port number to integer.Converting
ANALYTICS_SERVER_PORTto an integer usingint()ensures that the port is properly interpreted by Flask, preventing potential type-related issues.
backend/analytics_server/app.py
Outdated
| def handle_http_exception(e): | ||
| """Handle HTTP exceptions by returning JSON""" | ||
| response = jsonify({ | ||
| "error": True, |
There was a problem hiding this comment.
Thanks for the submission!
Quick Q: Why error: true?
Why not just - not use the error key?
The response has a status code, so that'll be sufficient for a client to know it's an error.
There was a problem hiding this comment.
@jayantbh Thanks for pointing out the mistake. You are correct, we should just pass error as a metric and remove message as error and error code would be sufficient
There was a problem hiding this comment.
@jayantbh Should I add sentry error tracing inside of app.py or it should be displayed as it is ?
There was a problem hiding this comment.
Let's keep this PR limited and simple.
|
@harshit078 Thanks for the PR. It would be great if you can include some tests for your changes |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/analytics_server/tests/tests_error_handling.py (4)
9-23: Error handler looks solid with a minor improvement possible.The general exception handler correctly formats errors as JSON with appropriate details. However, it always uses a fixed 500 status code regardless of the exception type. Consider checking if the exception has a
codeattribute and using that when available.@flask_app.errorhandler(Exception) def handle_exception(e): """Handle non-HTTP exceptions by returning JSON""" + status_code = getattr(e, 'code', 500) error_details = { "error": str(e) or "Internal Server Error", - "status_code": 500, + "status_code": status_code, "path": request.path, "exception_type": e.__class__.__name__, } if flask_app.debug: import traceback error_details["traceback"] = traceback.format_exc() response = jsonify(error_details) - response.status_code = 500 + response.status_code = status_code return response
54-58: Consider adding a tearDown method to reset Flask app configuration.The setUp method correctly configures the test client, but since some tests modify the app configuration (like setting DEBUG to True), it would be good practice to reset these configurations in a tearDown method.
def setUp(self): self.app = flask_app.test_client() flask_app.config["TESTING"] = True flask_app.config["DEBUG"] = False + def tearDown(self): + # Reset app configuration to default test state + flask_app.config["TESTING"] = True + flask_app.config["DEBUG"] = False
92-103: Good test for debug mode behavior, but consider improving state reset.The test correctly verifies that traceback information is included in debug mode. However, it would be better to use a try/finally block to ensure the debug mode is reset even if the test fails.
def test_debug_mode_traceback(self): """Test that traceback is included in debug mode""" flask_app.config["DEBUG"] = True + try: response = self.app.get("/test-debug-exception") data = json.loads(response.data) self.assertIn("traceback", data) self.assertTrue(len(data["traceback"]) > 0) + finally: flask_app.config["DEBUG"] = False - flask_app.config["DEBUG"] = False
1-113: Consider adding tests for additional HTTP exception types.The current tests cover NotFound (404) and BadRequest (400), but it would be beneficial to test other common HTTP exceptions like Forbidden (403), Unauthorized (401), or MethodNotAllowed (405) to ensure they're handled correctly.
def test_http_exception_handler_403(self): """Test handling of 403 Forbidden exception""" # You'll need to create a route that raises a Forbidden exception @flask_app.route("/test-forbidden") def forbidden_endpoint(): from werkzeug.exceptions import Forbidden raise Forbidden("Access denied") response = self.app.get("/test-forbidden") self.assertEqual(response.status_code, 403) data = json.loads(response.data) self.assertIn("error", data) self.assertEqual(data["status_code"], 403) self.assertEqual(data["path"], "/test-forbidden") self.assertEqual(data["error"], "Access denied")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/analytics_server/tests/tests_error_handling.py(1 hunks)
🔇 Additional comments (7)
backend/analytics_server/tests/tests_error_handling.py (7)
25-33: Well-implemented HTTP exception handler.The handler correctly extracts the necessary information from HTTP exceptions and returns it in a consistent JSON format. The code properly sets both the response content and status code.
35-51: Good test route coverage for different scenarios.The test routes effectively cover different error cases needed for testing. Each route triggers a specific type of exception to validate different aspects of the error handlers.
59-68: Good 404 error handling test.The test correctly validates that non-existent endpoints return 404 status codes with the expected JSON structure.
69-79: Well-structured test for BadRequest exception.The test properly validates that a Bad Request exception is handled correctly and returns the expected JSON response with the right status code and error message.
80-91: Comprehensive test for general exception handling.This test thoroughly validates the general exception handler, checking the status code, error message, path, and exception type in the response.
104-110: Good test for production mode behavior.This test correctly verifies that traceback information is excluded when not in debug mode.
1-113:✅ Verification successful
Ensure these error handlers are properly registered in the main application.
The PR objective mentioned adding JSON error handling to the middleware, but this file only contains tests. Make sure the same error handlers are properly registered in the main Flask application.
🏁 Script executed:
#!/bin/bash # Check if the error handlers are registered in the main Flask application # Look for the error handlers in the main app.py file echo "Checking for error handlers in app.py:" grep -n "errorhandler" backend/analytics_server/app.py # Also check for imports of these handlers if they're defined elsewhere echo -e "\nChecking for imports of error handlers:" grep -n "handle_exception\|handle_http_exception" backend/analytics_server/app.py # Look for Flask app instance in app.py echo -e "\nChecking for Flask app instance:" grep -n "Flask(__name__)" backend/analytics_server/app.pyLength of output: 742
Verified: Error handlers are properly registered in the main application.
The registration in
backend/analytics_server/app.pyconfirms that bothhandle_exceptionandhandle_http_exceptionare correctly attached to the Flask app. The code:
- Registers
HTTPExceptionwithhandle_http_exception(line 72)- Registers general
Exceptionwithhandle_exception(line 71)This satisfies the original comment's request to ensure JSON error handling is active in the middleware.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/analytics_server/tests/tests_error_handling.py (3)
14-15: Consider adding a more descriptive fallback error messageUsing
str(e) or "Internal Server Error"is good for handling empty error messages, but you might want to consider more descriptive fallback messages based on exception types.- "error": str(e) or "Internal Server Error", + "error": str(e) or f"Internal Server Error: {e.__class__.__name__}",
28-30: Consider handling more HTTP exception typesCurrently, only NotFound (404) and BadRequest (400) exceptions are explicitly handled. Consider using the base
HTTPExceptionclass to handle all HTTP errors consistently.- @flask_app.errorhandler(NotFound) - @flask_app.errorhandler(BadRequest) + @flask_app.errorhandler(werkzeug.exceptions.HTTPException)
1-122: Consider adding a test for error response content typeThe tests validate the structure of the JSON response but don't explicitly check that the response content type is set to
application/json. This would ensure the API is returning the proper content type headers.def test_response_content_type(self): """Test that error responses have the correct content type""" response = self.app.get("/non-existent-endpoint") self.assertEqual(response.content_type, "application/json")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/analytics_server/tests/tests_error_handling.py(1 hunks)
🔇 Additional comments (9)
backend/analytics_server/tests/tests_error_handling.py (9)
10-26: Well-structured error handler for non-HTTP exceptionsThe implementation properly converts exceptions to JSON responses with useful context data. The conditional inclusion of traceback information in debug mode is a good security practice.
28-37: Good HTTP exception handler implementationThe handler correctly processes HTTP exceptions and returns properly formatted JSON responses with appropriate status codes.
39-58: Well-defined test routesThe test routes are clear and concise, each targeting a specific error scenario which makes the tests more readable and maintainable.
60-66: Good test class setupThe test setup properly configures the Flask test client and sets appropriate testing flags.
67-76: Thorough 404 error testThe test validates all aspects of the 404 error response, including status code, JSON structure, and path information.
77-87: Comprehensive 400 error testThe test properly validates the handling of 400 errors, including checking the custom error message.
88-99: Detailed general exception testThe test effectively validates all fields in the error response for general exceptions, including exception type information.
100-111: Good debug mode testingThe test correctly verifies that traceback information is included when the app is in debug mode, and properly resets the debug flag afterward.
112-118: Good production mode testingThe test correctly verifies that traceback information is excluded in production mode, which is important for security.
|
@harshit078 Thanks for the changes. I noticed that you haven't added the error handling for the sync_api. It still returns html errors. check |
…ved error structure layout
|
@adnanhashmi09, Added error handling in |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/analytics_server/sync_app.py (1)
56-56: Convert port to integer when running the app.The port value from environment variables is typically returned as a string, which could cause issues when passed to
app.run().- app.run(port=SYNC_SERVER_PORT) + app.run(port=int(SYNC_SERVER_PORT))This ensures the port is properly converted to an integer, preventing potential runtime errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/analytics_server/sync_app.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/analytics_server/sync_app.py (1)
backend/analytics_server/app.py (2)
handle_http_exception(44-50)handle_exception(55-69)
🔇 Additional comments (4)
backend/analytics_server/sync_app.py (4)
3-5: Good addition of necessary imports for JSON error handling.The addition of
jsonifyandrequestfrom Flask,HTTPExceptionfrom werkzeug, and thetracebackmodule are appropriate for implementing JSON-based error handling.
24-33: Well-structured HTTP exception handler.The HTTP exception handler follows best practices by:
- Returning a JSON response with useful error information
- Including the original HTTP status code
- Providing the request path for debugging
- Setting the appropriate response status code
This will improve the developer experience when handling API errors.
34-50: Comprehensive general exception handler with appropriate debug controls.The implementation properly:
- Handles all non-HTTP exceptions
- Provides detailed error information in a structured JSON format
- Includes exception type information for easier debugging
- Conditionally includes traceback information only in debug mode, which is a good security practice
This approach will prevent exposing sensitive stack trace information in production while still providing useful debugging details when needed.
24-53: Add test coverage for the new error handlers.As mentioned in the PR comments by adnanhashmi09, it would be beneficial to add tests for these error handlers to ensure they function correctly in different scenarios.
Consider creating tests that verify:
- HTTP exceptions return proper JSON responses with correct status codes
- Non-HTTP exceptions return 500 status code with appropriate details
- Debug mode properly includes/excludes traceback information
You can reference the tests added in
backend/analytics_server/tests/tests_error_handling.pyas mentioned in the AI summary.
backend/analytics_server/app.py
Outdated
| app.register_error_handler(Exception, handle_exception) | ||
| app.register_error_handler(HTTPException, handle_http_exception) | ||
|
|
There was a problem hiding this comment.
These also required to be removed, don't they? @harshit078 ?
Proposed changes
Linked Issue(s)
Further comments
Summary by CodeRabbit