Return HTTP status 400 and 500 errors on back-end errors #1923
Conversation
Packing and cache busting makes the test setup only more complicated.
Ths changes CATMAID's error handling middleware so that it returns returns either status 400, 500 or custom error status code, depending on the exception. If the exception is an instance of the new `ClientError` type, it is expected to have `status_code` field, which is set to 400 by default. Permission related errors override this to return status 403. If an error is not an instance of `ClientError`, but still a `ValueError, status 400 is returned, because it is assumemd that a ValueError is the result of some bad/wrong input data. Otherwise status 500 is returned. Also fix a skeleton API test that was broken since 69c806c, but the effects where masked. Fixes #1921
This can be configured for each settings loading or per settings instance, if the back-end would ever allow anonymous user to store data. For now this fixes an error in our UI tests and gets rid of an unnecessary and failing PUT request for storing migrated settings for the anonymous user.
| status = 500 | ||
|
|
||
| # Allow easier access to the status code for clients | ||
| response['status_code'] = status |
There was a problem hiding this comment.
Is this necessary? Anyone interested in the status code will have access to it from the raw response object. It doesn't harm to throw it in, of course, I just think it's blurring the line between the transport layer and the data received.
EDIT: Ah, I see it's used by the frontend; I see it's easier to pass around the one object rather than treating the status code separately. Feasibly the client could read the code and stick it into the object as needed, but just adding it to the object might give us a bit more flexibility and reduce duplicated client code.
There was a problem hiding this comment.
Good point, thanks. After looking at it a second time I agree that it mixes different concepts. The use of the status code that motivated storing it in the first place should just not check for a status code, but for a defined error type (datastore.js checks for a 404). If it did, we wouldn't need to store in the first place and could use the status code only as optional error parsing informaiton when creating error types. I'll fix this.
There was a problem hiding this comment.
I force-pushed a fix for this. The status code is now only used as an extra option when constructing error types.
|
LGTM! Finding the tests which weren't failing properly is a nice outcome. |
tomka
force-pushed
the
features/better-http-status-codes-on-error
branch
2 times, most recently
from
5737d32
to
bce702d
Compare
The remaining uses have been converted to use CATMAID.fetch(). This is mainly done to centralize error handling.
tomka
force-pushed
the
features/better-http-status-codes-on-error
branch
from
bce702d
to
21cd4eb
Compare
This changes CATMAID's error handling middleware so that it returns returns either status 400, 500 or custom error status code, depending on the exception. Until in basically all cases status 200 was returned. If the back-end error is an instance of the new
ClientErrortype, it is expected to have astatus_codefield, which is set to 400 by default. Permission related errors override this to return status 403 (Forbidden).If an error is not an instance of
ClientError, but still aValueError, status 400 is returned, because it is assumemd that aValueErroris the result of some bad/wrong input data. DRF errors and basic Django exception are handled. Otherwise status 500 is returned. Since many errors that were only declared asExceptionwere actually the result of bad requests, I changed many of those intoValueErrorinstances.This also fixes also a handful tests for which the wrong response code masked errors before. It also fixes the attempt to write settings for the anonymous users on page load (resulting now in 401 errors, which broke the GUI tests).
This might require changes in third-party clients. Once merged, I will post a note on the mailing list.
This was talked about in a bit more detail in #1921.