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

Jump to bottom

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

Closed

dominicghizzoni wants to merge 1 commit into middlewarehq:main from dominicghizzoni:issue561

dominicghizzoni commented Oct 3, 2024

Linked Issue(s) #561

Acceptance Criteria fulfillment

Changed app.py to change such that flash returns errors as JSON instead.

Further comments

May want to check to see if it works, I was having trouble running some of the program.


          I modified app.py to make changes mentioned in issue middlewarehq#561

4d119b0

CLAassistant commented Oct 3, 2024 •

edited

Loading

All committers have signed the CLA.

Contributor

jayantbh commented Oct 3, 2024

Hey @dominicghizzoni, thank you so much for your PR.
We will however need you to go ahead and sign the CLA shared by the bot in the comment above.
Thanks! :)

Contributor

jayantbh commented Oct 3, 2024

Please also update the PR title to reflect the nature of the change.

dominicghizzoni changed the title ~~I modified app.py to make changes mentioned in issue #561~~ I modified app.py to make changes mentioned in issue #561 (Return Flask errors as JSON instead of HTML)

dominicghizzoni changed the title ~~I modified app.py to make changes mentioned in issue #561 (Return Flask errors as JSON instead of HTML)~~ Return Flask errors as JSON instead of HTML (mentioned in issue #561)

Author

dominicghizzoni commented Oct 3, 2024

what must i change to pass all the checks

Contributor

VipinDevelops commented Oct 4, 2024 •

edited

Loading

what must i change to pass all the checks

Hey @dominicghizzoni Please read Contributions Guide It will help you.

thesujai reviewed

View reviewed changes

backend/analytics_server/app.py

@@ @@ -36,5 +37,19 @@ @@
               configure_db_with_app(app)
               initialize_database(app)
+              def handle_exception(e):

Contributor

thesujai Oct 7, 2024 •

edited

Loading

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)

backend/analytics_server/app.py

@@ @@ -36,5 +37,19 @@ @@
               configure_db_with_app(app)
               initialize_database(app)
+              def handle_exception(e):
+                  if isinstance(e, HTTPException):
+                      return jsonify({

Contributor

thesujai Oct 7, 2024 •

edited

Loading

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)

Contributor

samad-yar-khan Oct 10, 2024

@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.

Contributor

samad-yar-khan Oct 10, 2024

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

samad-yar-khan requested changes

View reviewed changes

backend/analytics_server/app.py

@@ @@ -36,5 +37,19 @@ @@
               configure_db_with_app(app)
               initialize_database(app)
+              def handle_exception(e):

Contributor

samad-yar-khan Oct 10, 2024

@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

backend/analytics_server/app.py

@@ @@ -36,5 +37,19 @@ @@
               configure_db_with_app(app)
               initialize_database(app)
+              def handle_exception(e):
+                  if isinstance(e, HTTPException):
+                      return jsonify({

Contributor

samad-yar-khan Oct 10, 2024

@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.

backend/analytics_server/app.py

@@ @@ -36,5 +37,19 @@ @@
               configure_db_with_app(app)
               initialize_database(app)
+              def handle_exception(e):
+                  if isinstance(e, HTTPException):
+                      return jsonify({

Contributor

samad-yar-khan Oct 10, 2024

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

Contributor

thesujai commented Oct 10, 2024

@samad-yar-khan the point I was trying to convey was errors like 404(all errors less than 500) should be handled in the business logic itself right? There won't more than 2-3 error possibility for any endpoint. I can observe that most of the api view are already checking for errors and returning the json error as well.

For the 404 page not found related to false routes or redirections, should be handled with the frontend routing.

So imo there will be some endpoints where something unexpected will occur, and for that flask returns with a html error file. So can't show that file in front end where we are expecting a simple object/string.
To avoid this we want to respond with a json. For debugging concerns these error can be logged to db or something like we can leverage certains tools like Sentry. But the error that is return as json should be hardcoded.

Please let me know if I am misunderstanding it, my knowledge is very limited after all.

jayantbh mentioned this pull request

Improve error handling on the flask backend #561

Open

Contributor

jayantbh commented Oct 23, 2024

Hey @dominicghizzoni, are you considering continuing this PR?

Author

dominicghizzoni commented Oct 23, 2024

Hey @dominicghizzoni, are you considering continuing this PR?

Someone else in the issue thread said they would try to complete it

Contributor

jayantbh commented Oct 30, 2024

Closing this PR then. :)

jayantbh closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet