Opened 4 years ago

Closed 4 years ago

#31239 closed New feature (wontfix)

Add a new exception type for request.GET and request.POST

Reported by: Victor Porton Owned by:
Component: Error reporting Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I believe we should throw a derived exception of MultiValueDictKeyError rather than MultiValueDictKeyError when a value is missing in request.GET or request.POST.

This would add extra convenience when catching errors caused by a wrong request, easily distinguishing them with other errors (e.g. programmer's errors).

I have in code of my custom View:

    def handle_exception(self, exc):
        if isinstance(exc, MultiValueDictKeyError) or isinstance(exc, KeyError):
            # It is an unwise assumption that this is necessarily missing HTTP param,
            # but rewriting it in other way would be time consuming (and maybe even more error prone).
            return MyErrorResponse({"code": "PAR_1",
                                    "message": "Missing param.",
                                    "field": exc.args[0]})

You see, I did it wrong because of this missing feature. It would be too much work to do it right without relying on the suggested feature.

Any code following good programming practices (namely Liskov substitution principle) won't be broken by this change.

Change History (7)

comment:1 by Mariusz Felisiak, 4 years ago

I'm not sure what you're proposing. Do you want to raise a different exception when key is missing in a QueryDict?

comment:2 by Victor Porton, 4 years ago

@felixxm Yes. But this exception is to be derived from MultiValueDictKeyError to preserve backward compatibility.

comment:3 by Mariusz Felisiak, 4 years ago

Can you describe a real use case? I don't see where you will have to catch this manually 🤔, and it's even more niche to have to distinguish between errors raised by QueryDict and MultiValueDict, IMO.

comment:4 by Victor Porton, 4 years ago

@felixxm Be more cateful. I did already provide a real (from a production code) use case in the text of the bug report. OK, I am repeating it here:

I have in code of my custom View:

    def handle_exception(self, exc):
        if isinstance(exc, MultiValueDictKeyError) or isinstance(exc, KeyError):
            # It is an unwise assumption that this is necessarily missing HTTP param,
            # but rewriting it in other way would be time consuming (and maybe even more error prone).
            return MyErrorResponse({"code": "PAR_1",
                                    "message": "Missing param.",
                                    "field": exc.args[0]})

Here MultiValueDictKeyError should be instead a custom exception for catching namely errors in GET/POST.

This would make catching such errors and returning the appropriate error code easy. Now it is too cumbersome: need to add a try...except in every check of request.GET/request.POST. Is it convenient?

comment:5 by Mariusz Felisiak, 4 years ago

I saw that example, but it's not a use case scenario. My question is why/where you need to use this handle_exception() instead of checking that expected value is in request.GET or request.POST, e.g.

if expected_key is not in request.GET:
    return MyErrorResponse(...)

comment:6 by Victor Porton, 4 years ago

@felixxm This your question is also already answered by me:

Writing try...except around every request.GET and request.POST is very cumbersome. So cumbersome that some programmers prefer not to check but just get a 500 Internal Server Error in this case, knowing it is an erroneous behavior.

Programming is about automation. Need to automate this checking for the entire program easily. In my code example I show how this automation could be done. Unfortunately if we catch MultiValueDictKeyError as in my example, we also "catch" some programming errors, file reading errors, database errors, etc. producing a wrong HTTP status. It is just bad.

comment:7 by Mariusz Felisiak, 4 years ago

Resolution: wontfix
Status: newclosed

I wasn't proposing writing try...except and IMO it's not a good practice not to handle an unexpected data gracefully. Creating a single method to catch and handle all kind of exceptions is error prone.

You can start a discussion on DevelopersMailingList if you don't agree.

Note: See TracTickets for help on using tickets.
Back to Top