Issue 431: return all exceptions as JSON:API error objects #437
Conversation
|
I would appreciate some suggestions on how to cleanly implement test coverage for this. |
|
(I feel a little guilty about sneaking in coverage to tox.ini as part of this PR....) |
Codecov Report
@@ Coverage Diff @@
## master #437 +/- ##
==========================================
+ Coverage 91.92% 92.02% +0.09%
==========================================
Files 57 57
Lines 2984 3021 +37
==========================================
+ Hits 2743 2780 +37
Misses 241 241
Continue to review full report at Codecov.
|
|
@sliverc Please take a look at my workaround for py27 mock not working the same as py36's. I think I'm doing it correctly. Meanwhile I've wrapped it with xfail as well. Subject to any suggestions you have, this is ready for review. |
n2ygk
changed the title
There was a problem hiding this comment.
Thanks for working on this. I like what issue you are addressing here so we can have clear json api errors.
During my review though I was thinking whether this extending exception handler is really the right approach to fix this issue.
I still left my comments but before you work on those I think we should discuss how we really wanna solve this.
First of all we should be very careful not exposing any internal information to an api caller which could have security implication.
So simply passing on the exception message can expose critical internal information. What Django does is when not in DEBUG mode it simply says server error.
But I think a simple generic error message in the right json api format should do in our case. The log file will then still show the exception trace.
Also to generate the right format it would be better to overwrite handle500 directly. This way we also don't run into the issue that Django doesn't log the error anymore.
See http://www.django-rest-framework.org/api-guide/exceptions/#generic-error-views
Let me know what you think on this approach before you continue working on this PR.
| "'unbound method get_queryset() must be called with BadBlogViewSet " | ||
| "instance as first argument (got nothing instead)' " | ||
| "rather than working properly as does python3.x mock") | ||
| class TestViewExceptions(TestBase): |
There was a problem hiding this comment.
For new tests I think we should use plain pytest and not unittest. In the long we should get rid of legacy unittest anyway.
Also question is why mocking is needed here. I think if we added BadBlogViewSet into the routing it could be simply called.
There was a problem hiding this comment.
I was frankly confused by the 3-levels of tests/, tests/unit, tests/integration. Any kind of more detail in the contributing doc you could add would be appreciated.
I can move BadBlogViewSet into the example app views. I based the mocking approach on other projects I've seen where the tests are self-contained in the test code.
|
|
||
| Even a 500 error for a jsonapi view should return a jsonapi error object, not an HTML response. | ||
| """ | ||
| if not rendered_with_json_api(context['view']): |
There was a problem hiding this comment.
I think we should move this check to exception_handler this way we can avoid duplicated code as this same code bit is already there on line 82, which would need to be moved upwards.
There was a problem hiding this comment.
Sure, will take a look.
| """ | ||
| if not rendered_with_json_api(context['view']): | ||
| return None | ||
| keymatch = re.compile(r"^Cannot resolve keyword '(?P<keyword>\w+)' into field.") |
There was a problem hiding this comment.
this should be moved into the if as it is only needed there
| errors = [ | ||
| { | ||
| "status": status_code, | ||
| "code": "field_error", |
There was a problem hiding this comment.
I do not think we should assign a code. In the
specification it states for code an application-specific error code, expressed as a string value..
rest json api framework is not an application though but a framework so I think we shoulnd't set this but only the implementing application if desired.
There was a problem hiding this comment.
I'd rather err on the side of providing more information to the client (e.g. field_error provides more useful information than just a 4xx status code).
| if not rendered_with_json_api(context['view']): | ||
| return None | ||
| keymatch = re.compile(r"^Cannot resolve keyword '(?P<keyword>\w+)' into field.") | ||
| if isinstance(exc, (FieldError, FieldDoesNotExist,)): |
There was a problem hiding this comment.
Not sure whether this is an 400 error, as it is not dependen on the request but on the actual code which is erroneousness. Specification says for 400 The request could not be understood by the server due to malformed syntax. The client SHOULD NOT repeat the request without modifications. But when looking at the code this rather looks like an internal server error to me.
| "title": "Internal Server Error", | ||
| "detail": str(exc), | ||
| "source": { | ||
| "pointer": context["request"].path, |
There was a problem hiding this comment.
A pointer should be a JSON path but we do not really know what json path as the failing request might be simply a GET request. So I think we should remove pointer altogether.
There was a problem hiding this comment.
See prior comment; When this is exception is raised as part of a viewset GET it is (maybe?) caused by an incorrect field name.
|
FYI - I'll be traveling for the next 2 weeks so will not get a chance to look at this beyond this weekend in case I appear to "go silent". I will be back! Thanks for all the help. |
|
@n2ygk However we can discuss it once you are back. Enjoy your time away! |
|
@sliverc so it's been a little more than 2 weeks(!) since I had other things backed up when I returned as we are in the midst of developing a new set of microservices based on DJA and django-oauth-toolkit, among other things. I'm going to start by digging in to your recommendation of using |
|
I'm closing the PR with the plan to open a new one once I figure out a better way to do this. |
Fixes django-json-api#326 Extracted from django-json-api#437
Fixes django-json-api#326 Extracted from django-json-api#437
Fixes #326 Extracted from #437 Co-authored-by: Alan Crosswell <[email protected]>
Fixes #431
Fixes #326
Description of the Change
Improve exception handling to return JSON:API error objects in all cases.
Checklist
CHANGELOG.mdAUTHORS