Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make 404 & 403 responses consistent with exceptions.APIException output #5763

Merged
merged 1 commit into from
Jan 30, 2018
Merged

Conversation

fengsi
Copy link
Contributor

@fengsi fengsi commented Jan 23, 2018

Map Django's 404 & 403 to DRF's exceptions, making responses consistent (otherwise default_code / code is not honored).

@rpkilby
Copy link
Member

rpkilby commented Jan 24, 2018

Going through the git blame, it looks like the current implementation is just an artifact of history. I don't know if there's a reason to distinguish Django's Http404 from DRF's NotFound. cc @tomchristie?

@tomchristie
Copy link
Member

making responses consistent

I didn't immediately see any behavioral change here, but on a second pass I think I understand.
I assume you're globally setting the class attribute on one or both of NotFound/PermissionDenied someplace, eg...

NotFound. default_detail = '...'

I've got slightly mixed feelings about the change. I don't especially like changing exc rather than treating extra cases explicitly. However it is a little shorter, so probably okay with it.

@fengsi
Copy link
Contributor Author

fengsi commented Jan 25, 2018

I had a custom exception handler which renames detail to error, and adds an error_code field as well:

def custom_exception_handler(exc, context):
    if isinstance(exc, Http404):
        exc = NotFound()
    elif isinstance(exc, Http403):
        exc = PermissionDenied()
    response = exception_handler(exc=exc, context=context)
    if response and isinstance(response.data, dict):
        detail = response.data.pop('detail', None)
        if detail:
            response.data['error'] = detail
        if hasattr(exc, 'get_codes'):
            response.data['error_code'] = exc.get_codes()
    return response

404 & 403 were hard-coded responses w/o the error_code part. This way they are both consistent with other APIException instances. :)

@blueyed
Copy link
Contributor

blueyed commented Jan 29, 2018

Nice.
Reminded me of #5379 - this approach seems to be better.

@rpkilby rpkilby added this to the 3.8 Release milestone Jan 29, 2018
@tomchristie tomchristie merged commit df77f7b into encode:master Jan 30, 2018
@fengsi fengsi deleted the 404-403 branch January 30, 2018 22:11
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants