fix: structure api error messages #214
Conversation
There was a problem hiding this comment.
good start, but some wrong turns that can be fixed.
you should add unit tests to test_utils.py and rename the test_response to test_error_handling or so.
here are some resources for testing Django with DjangoTest plus and DRF:
https://www.django-rest-framework.org/api-guide/testing/
https://www.revsys.com/tidbits/django-test-plus/
wazimap_ng/utils.py
Outdated
|
|
||
| def error_handler(request, **kwargs): | ||
|
|
||
| if request.path.startswith('/api/v1'): |
There was a problem hiding this comment.
instead of using the path you should rather look for the content_type and if it's json return json otherwise leave as is.
There was a problem hiding this comment.
Since this fix is to structure all error messages in json, I'll remove the if statement and render all 500 error responses in json.
There was a problem hiding this comment.
no, the issue was that all API requests should respond with JSON.
Not all requests. Otherwise the Admin will show JSON errors and the DRF preview potentially as well.
That's why we need the check for content type here.
There was a problem hiding this comment.
Fixed. During test , I had to assign content_type to underlying request object so function could be tested without errors.
tests/test_response.py
Outdated
| factory = RequestFactory() | ||
| request = factory.get('/api/v1/jkhj') | ||
|
|
||
| response = error_handler(request) |
There was a problem hiding this comment.
you're headed in the wrong direction here.
you want to test the result of the exception raised by DRF and django and not call your own error_handler
tests/test_utils.py
Outdated
|
|
||
| def test_custom_exception(self): | ||
| response = self.get('/api/v1/datasets/3452/') |
There was a problem hiding this comment.
this is the integration test and should go to test_error_handler
tests/test_error_handling.py
Outdated
| def test_error_json(self): | ||
| request = self.get('/foo/bar') | ||
|
|
||
| response = error_handler(request) |
There was a problem hiding this comment.
this is a unit test since it's calling the error_handler method and should go in test_utils.py
| } | ||
| } | ||
|
|
||
| response = self.get('dataset-detail', pk=234) |
There was a problem hiding this comment.
this is the correct test for the 404.
thanks. we need a similar one for the 500
There was a problem hiding this comment.
During testing, django raises the exception inplace of calling the handler500 function which returns the json response. I wrote the unit test for the handler500 function which is the test_error_handler_func, should I write the live server test case for 500 error ?
tests/test_error_handling.py
Outdated
|
|
||
| response = self.get('dataset-detail', pk=234) | ||
|
|
||
| data = json.loads(response.content) |
There was a problem hiding this comment.
take a look at the docs from DRF: https://www.django-rest-framework.org/api-guide/testing/#checking-the-response-data
you can use .data instead of .response
There was a problem hiding this comment.
@Geeker1 I am not able to see error message on custom api functions like all_detail api end point if I pass invalid porfile id
the solution will work only DRF views but we want to implement it all api endpoints including cutsom api_views
|
What is the status with this PR - I see it's still in draft - is it still being worked on? |
Yes, it is. Will soon push updates |
Description
Structuring error messages from api to users using django-rest custom exception handler.
Related Issue
#214
How to test it locally
docker-compose run --rm -e DJANGO_CONFIGURATION=Test web pytest /app/tests/test_utils.py /app/tests/test_error_handlingChangelog
Added test for custom exception handler function
Replaced Custom Renderer with Exception Handler
Added
Updated
Removed
Checklist
Pull Request
Commits
Code Quality
Testing