-
Notifications
You must be signed in to change notification settings - Fork 127
fix: Added Json error handling instead of HTML #631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
harshit078
wants to merge
8
commits into
middlewarehq:main
Choose a base branch
from
harshit078:fix--error-handling-json
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+185
−3
Open
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
246208d
Added Json error handling instead of HTML and improved error fetching…
harshit078 f0fe5ed
Removed Message and improved Error format
harshit078 9677219
Added tests for error handling in app.py
harshit078 5fa6dfd
Minor fix in spacing in tests
harshit078 7a80165
Resolved black Integration test failing
harshit078 271b06c
Added JSON error handling in sync_app instead of HTML error for impro…
harshit078 43a21c2
Removed error handlers and improved code
harshit078 2037840
Removed redundant error handler in app.py
harshit078 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
from os import getenv | ||
|
||
from flask import Flask | ||
from flask import Flask, jsonify, request | ||
from werkzeug.exceptions import HTTPException | ||
import traceback | ||
|
||
from env import load_app_env | ||
|
||
|
@@ -36,5 +38,40 @@ | |
configure_db_with_app(app) | ||
initialize_database(app) | ||
|
||
# HTTP Error handler | ||
@app.errorhandler(HTTPException) | ||
def handle_http_exception(e): | ||
"""Handle HTTP exceptions by returning JSON""" | ||
response = jsonify({ | ||
"error": True, | ||
"message": e.description, | ||
"status_code": e.code, | ||
"path": request.path | ||
}) | ||
response.status_code = e.code | ||
return response | ||
|
||
# Error handler | ||
@app.errorhandler(Exception) | ||
def handle_exception(e): | ||
"""Handle non-HTTP exceptions by returning JSON""" | ||
error_details = { | ||
"error": True, | ||
"message": str(e) or "Internal Server Error", | ||
"status_code": 500, | ||
"path": request.path, | ||
"exception_type": e.__class__.__name__ | ||
} | ||
|
||
if app.debug: | ||
error_details["traceback"] = traceback.format_exc() | ||
|
||
response = jsonify(error_details) | ||
response.status_code = 500 | ||
return response | ||
|
||
app.register_error_handler(Exception, handle_exception) | ||
app.register_error_handler(HTTPException, handle_http_exception) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These also required to be removed, don't they? @harshit078 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, hence pushed a fix |
||
if __name__ == "__main__": | ||
app.run(port=ANALYTICS_SERVER_PORT) | ||
app.run(port=int(ANALYTICS_SERVER_PORT)) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this PR limited and simple.