Return HTTP 500 errors as JSON instead of HTML

billglover commented 4 years ago

Requests that result in an HTTP 500 Internal Server Error status code return HTML even if the client is expecting to receive a JSON response. A standard JSON error response format should be adopted to be sympathetic to client error handling code.

Why? When the server returns an error, the client (React front end, mobile app, CLI, etc.) will want to parse that error and take appropriate action. In some cases this will be displaying a message to the user, in other it may be retrying the request or logging the error. If the client is expecting to parse a JSON response and it receives HTML it will fail to parse details of the error.

Why now? If we introduce inconsistencies in the way the server responds with errors, we make it difficult to write clients. Clients become increasingly complex to handle these inconsistencies and it gets harder to retrofit consistency.

Cost of doing nothing? In the case of a HTTP 500 Internal Server Error clients could look at the HTTP status code in the response and decide not to parse the response body. This workaround would avoid the need for complex error handling logic, but would reduce the information that clients could determine about the cause of the error.

chris48s commented 4 years ago

Bit of a drive-by comment, but if you're just having trouble with unhandled exceptions that don't extend DRF's base APIException class, have a look at the DRF Generic Error Views: https://www.django-rest-framework.org/api-guide/exceptions/#generic-error-views That probably solves your problem with a couple of lines of config?

tgrrr commented 4 years ago

Why don't we log the error with a service like Sentry, and keep the UX clean?

billglover commented 4 years ago

Server side errors could still be logged. Whether this requires an external service is probably worth a separate issue for discussion.

This issue was raised because clients typically expect a consistent message format from requests to the back-end. Our clients get JSON responses for with most status codes but we are currently sending HTML for the 500 status code. This inconsistency is often a source of friction for clients of the back-end service.

BethanyG commented 4 years ago

I believe this is a settings / URL config issue. Currently, in cbv3_django_prototype/cbv3_django_prototype/config/settings/local.pywe have:

DEBUG = True
MIDDLEWARE += ["debug_toolbar.middleware.DebugToolbarMiddleware"]
INSTALLED_APPS += ["django_extensions"]
we are using Django Debug Toolbar
we have some basic HTML templates and routes set for developers running the project for the first time.
- Templates base, home and about are in in cbv3_django_prototype/cbv3_django_prototype/templates/base.html, cbv3_django_prototype/cbv3_django_prototype/templates/home.html, and cbv3_django_prototype/cbv3_django_prototype/templates/about.html.
- The routes for these can be found in cbv3_django_prototype/cbv3_django_prototype/config/urls.py

What this means is that when a 500 is thrown in debug, the debug output is formatted for the debug toolbar, so that it can be navigated in a web page, or can hook into tools such as debugger-runserver or django-rundbg. You can see this in the notes in cbv3_django_prototype/cbv3_django_prototype/config/urls.py file, where they've defined some additional routes for different HTTP status codes.

When a 500 is thrown with DEBUG=False, the output is being formatted for those default home and about pages we have thrown up there for placeholders.

Of course, this is not what we want for the resources api endpoint, so we'll need to add something like path("500/", 'rest_framework.exceptions.server_error') for DRF routes for when DEBUG = False,

or otherwise configure the DRF routes in cbv3_django_prototype/cbv3_django_prototype/resources/urls.py to serve up the appropriate JSON when a 500 happens on the resource/ endpoint rather than taking the HTML templates from the home & about routes.

Don't have all the details off the top of my head, but this DRF documentation is where I'm looking: https://www.django-rest-framework.org/api-guide/exceptions/#generic-error-views.

DRF is currently set to return JSON for all other requests.

BethanyG commented 4 years ago

I'd like to add that I think for DEBUG=True, we need to keep error messages in HTML format so that they can be displayed properly in the debug toolbar, etc. But for any sort of production deploy we of course would turn DEBUG=False.

BethanyG commented 4 years ago

For the logging configuration, see cbv3_django_prototype/cbv3_django_prototype/config/settings/base.py under LOGGING.

The logging configuration docs for Django can be found here Customization for Django logging can be found here.

billglover commented 4 years ago

This is some really useful background and some good links to continue exploring. I'm really surprised that the Accept header is ignored for the error path so that's what I'm going to dig into. I suspect this comes from the Django origins targeting browser based requests.

The behaviour I expected was:

browser based requests get all the debug extras when running in debug mode
browser based requests get nice error pages when running in normal mode
json client requests get json responses in all modes
debug information is always sent to the logs in debug mode

The challenge with changing the error responses between environments is that it means I don't get to test client error handling until I deploy in production mode.

chris48s commented 4 years ago

If you want to change the error handling behaviour in debug mode based on the Accept header, you could write a custom error handling middleware that implements process_exception()

https://docs.djangoproject.com/en/3.0/topics/http/middleware/#process-exception