Skip to content

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
wants to merge 8 commits into
base: main
Choose a base branch
from
41 changes: 39 additions & 2 deletions backend/analytics_server/app.py
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

Expand Down Expand Up @@ -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,
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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 ?

Copy link
Member

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.

"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)

Copy link
Contributor

Choose a reason for hiding this comment

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

These also required to be removed, don't they? @harshit078 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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))
Loading