Handling django exceptions more transparently

[clbarnes]

The problem

If python raises an exception, django/DRF doesn't handle it unless it's an error it understands. Instead, it just bundles up the exception info (error message, type name, traceback) into a JSON object and returns that with a 200 status. This leads to opaque errors for clients: they check the status (OK), deserialise the JSON (OK), and then only fail when they try to index into it, or check the length or whatever. I suspect things get weird if you passed a parameter requesting a msgpack return value, an error is raised and you get JSON back.

The correct(?) solution

Returning the error information is a great idea, if django also returned an appropriate HTTP error code and the clients used that. However, this is a lot of code to change - try/excepts around every errorable line on the server, and adjusting every client to deal with the responses. It's probably too late to start doing this now, unless we had a "tech debt hackathon" which included some of the downstream developers.

The minimal solution

Some clients (the CATMAID frontend, catpy for sure) already parse the returned JSON, try to recognise if it's an error response, and raise an exception for it (this is a potential performance issue in catpy, which deserialises the JSON multiple times). As a minimum, it would be nice to add this sort of error handling to the django.test.client.Client class - then at least our unit tests would be transparent about where the error happens and return the traceback.

[tomka]

The fact that CATMAID returns a 200 status upon an error isn't really the fault of Django or DRF. Django would normally return status 500 on an exception, which I feel is correct as the general case. However, in CATMAID we have added a middleware that catches all exceptions and returns them in the common (but not very helpful) format with status 200. This is defined in AjaxExceptionMiddleware in middleware.py.

I suspect things get weird if you passed a parameter requesting a msgpack return value, an error is raised and you get JSON back.

Yes, unfortunately.

Returning the error information is a great idea, if django also returned an appropriate HTTP error code and the clients used that.

I agree.

However, this is a lot of code to change - try/excepts around every errorable line on the server, and adjusting every client to deal with the responses. It's probably too late to start doing this now, unless we had a "tech debt hackathon" which included some of the downstream developer

I don't think we would need to try/except every errorable code and feel this would make a lot of code harder to read. I think I would favor a solution were our views and other logic would just continue using exceptions as they do now.

Instead maybe the following would improve the situation already and is minimal work: we modify the AjaxExceptionMiddleware so that we return status 500 on errors, keeping the JSON format of the current response. Since there are quite a few places where we sanitize and check input values and raise a ValueError in case of problematic input, it might be semantically more correct in this case to return a 4xx code (client error), maybe just 400. Practically it would mean to replace the return JsonResponse(response) of the AjaxExceptionMiddleware with this:

status = 400 if type(exception) == ValueError else 500
return JsonResponse(response, status=status, safe=False)

This way clients would only need to make sure they now test for a non-200 status and read out the error information as they do already. The regular web-front end does this already, which is why the above code works with the current front-end without further modifications. I suppose most clients check the status already, but it might require slight modification so that error information is also parsed in this case. This is probably an acceptable change for most clients.

I am not aware of any endpoints that would generate an error response on their own (bypassing the middleware), so this should work for most if not all cases.

[clbarnes]

Thanks Tom, I didn't think about middlewares! That does look like a simple solution which would cover most cases (at least if we used isinstance so we caught subclasses of ValueError too). The downstream clients would need to adjust, but it would make their code quite a lot simpler so it probably won't upset them too much, and if they have a well-factored request-making layer it should only be a change in one place.

[tomka]

Good point about isinstance. I'll test a bit more and if everything seems to work, I'll push this change and add a note to the release/update nodes and post a short explanation to the mailing list.

[schlegelp]

What will be returned as textual reason for the 500 server errors? Anything meaningful? E.g. the same as error in the response json (e.g. {'error': "Skeleton #1 doesn't exist", 'details': "....}?

[tomka]

Yes, the response body of all the errors is the same as before, i.e. a JSON object with at least `error`, `type` and `details` fields.

…

On September 27, 2019 2:29:49 PM UTC, Philipp Schlegel ***@***.***> wrote: What will be returned as textual reason for the 500 server errors? Anything meaningful? E.g. the same as `error` in the response json (e.g. `{'error': "Skeleton #1 doesn't exist", 'details': "....}`? -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #1921 (comment)