Skip to content

Return Flask errors as JSON instead of HTML (mentioned in issue #561) #567

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Return Flask errors as JSON instead of HTML (mentioned in issue #561) #567

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion backend/analytics_server/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from os import getenv

from flask import Flask
from flask import Flask, jsonify
from werkzeug.exceptions import HTTPException

from env import load_app_env

Expand Down Expand Up @@ -36,5 +37,19 @@
configure_db_with_app(app)
initialize_database(app)

def handle_exception(e):
Copy link
Contributor

@thesujai thesujai Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The handler is implemented well(needs a little tweak), but we need to let the flask know that this is the Error Handler it should be using for unhandled errors(Because flask returns with HTML Content when an unexpected/unhandled error occurs)
Check this: https://flask.palletsprojects.com/en/2.3.x/errorhandling/#registering

so adding something like below to app should suffice:
app.register_error_handler(Exception, handle_exception)

Copy link
Contributor

Choose a reason for hiding this comment

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

@dominicghizzoni I agree with @thesujai here. The code is 50% there. I would suggest going through this doc:
https://flask.palletsprojects.com/en/2.3.x/errorhandling/#registering, https://flask.palletsprojects.com/en/2.3.x/errorhandling/#returning-api-errors-as-json.

After going through these and the example you would have an idea of what kind of a code.
I think Generic Exception handlers can be applied in our case

if isinstance(e, HTTPException):
return jsonify({
Copy link
Contributor

@thesujai thesujai Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The problem with error JSON is that it’s not compatible (or generalized) for JavaScript, which is the client for the backend. Instead, we should only return:

return jsonify({
  "error":"Internal Server Error"
}), 500

We don’t want to expose the user to details about the server-side error. Providing a detailed description could be a security risk, as it may reveal internal information about our code.(feels weird talking about code revealing to user, when the whole code is OS 😅)

I am just saying again this hander is not supposed to handle errors < 500(status code) because they should be dealt with assertion, if-else, try-catches, or middleware(Its more of a developer's responsibility when shipping features)

Copy link
Contributor

Choose a reason for hiding this comment

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

@thesujai I think there is value in telling the client "the id you requested data for does not exist, hence a 404". It might be hidden from the user via the next js bff handling.

This backend should also be usable as a standalone analytics/data service if needed. Also, a generic 500 makes the API hard to debug.

Please let me know if I misunderstood this comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@thesujai thanks alot for reviewing this code beforehand and linking the documentation. Really appreciate it 🙌

"error": e.name,
"description": e.description,
"status_code": e.code
}), e.code

return jsonify({
"error": "Internal Server Error",
"description": str(e),
"status_code": 500
}), 500

if __name__ == "__main__":
app.run(port=ANALYTICS_SERVER_PORT)
Loading