Make 404 & 403 responses consistent with exceptions.APIException output by fengsi · Pull Request #5763 · encode/django-rest-framework (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation4 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

fengsi

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

@rpkilby

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

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.

rpkilby

@fengsi

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

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

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request

Nov 17, 2020