#22058 closed New feature (wontfix)

Consider a more general method to replace handlerXXX

Reported by: debanshuk Owned by: anubhav9042
Component: Core (URLs) Version: master
Severity: Normal Keywords:
Cc: anubhav9042@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

With class based views it's not straight forward to to raise a Http404 error if the view doesn't support HTTP method (i.e. a 405 error), with function based views it was. So, in such cases clients (users) see a blank page with 405 response code, but with no message to tell them where to go from there. I guess Django should raise and handle Http405 exception and use a default handler405 view (which of course can be overridden) to return response to user in such a case, as it does in case of 404, 403 and 500 errors.

Change History (13)

comment:1 Changed 17 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

405 errors are an order of magnitude less common than 404 and 403.

I don't think it's a good idea to keep adding exception classes and hardcoding handlerXXX in URLconfs. We should start thinking about a more generic mechanism.

For instance we could have a view to deal with client errors (4XX), a view for server errors (5XX), and pass adequate arguments to these views. But that would create lots of code churn in existing projects :/

comment:2 Changed 16 months ago by anubhav9042

I am interested in this especially I want this to incorporate in GSoC proposal.
As suggested we can just have two views which can be matched with regex 4XX and 5XX, and accordingly lot of code will be reduced.
Also as this ticket suggests, we can add some more cases including 405 like 401, etc

Any suggestions??

comment:3 Changed 16 months ago by anubhav9042

  • Cc anubhav9042@… added
  • Owner changed from nobody to anubhav9042
  • Status changed from new to assigned

comment:4 Changed 16 months ago by timo

  • Summary changed from Add `Http405` exception class and `handler405` view (simillar to 404, 403 and 500) to Consider a more general method to replace handlerXXX
  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 13 months ago by anubhav9042

I think the best solution is to refactor the code creating only two handlers handler4XX & handler5XX and two views. And in order to prevent any backwards incompatibility, we can let the existing exceptions as it is and create two generic ones. This way in future people will be able to deal with any kind of errors along with the previous ones provided for 403,404,500. I mean this way whatever Aymeric suggests can be done without any "code churn in existing projects".

comment:6 Changed 13 months ago by timo

I'm not sure about that suggestion. Having two handlers to catch all errors could encourage gigantic functions. It could be useful to look into how other frameworks deal with this. Flask, for example, allows you to decorate functions with the error code which seems nice:

@app.errorhandler(404)
def page_not_found(e):
    ...

comment:7 Changed 13 months ago by aaugustin

If memory serves, that's also how Flask's url dispatcher works in general. One decorates view functions to declare the URL they're available at. So this mechanism is consistent with Flask's design but not with Django's.

For better or worse, Django's contract is to provide view information under conventional names in the ROOT_URLCONF module: urlpatterns, handler403/404/500. I think we should keep this declaration mechanism because there's no strong reason to change it.

The open question is — what conventional name(s) do we add to ROOT_URLCONF and with what semantics?

Ideas:

  • handler view function for all errors.
  • handler4xx / handler5xx view functions — it's a smaller change and it decreases the chances of having a dysfunctional 5xx view. That's why I prefer to deal with 4xx and 5xx separately.
  • handlers dict mapping error codes to view functions, possibly a defaultdict to provide a fallback for unknown errors
  • handlers4xx / handlers5xx dicts, which allows providing two defaultdicts with different defaults, for the same reason as above
  • etc.

comment:8 Changed 13 months ago by anubhav9042

I have done some work on handler4xx and handler5xx and I think it works just fine.
Have a look: https://github.com/coder9042/django/compare/gsoc_%2322058

As far as the functionality is concerned, I think it works fine, but there might some design decisions involved for which some things might have to be changed.

Last edited 13 months ago by anubhav9042 (previous) (diff)

comment:9 Changed 13 months ago by anubhav9042

I have updated the above work with fix for #21668 as commented there.

comment:10 Changed 13 months ago by timo

Consolidating the views looks good (perhaps name them something like 4xx_error and 5xx_error). Adding 'handler4xx' and 'handler5xx' with the ability to define a specific version like 'handler404' also looks good. Could you try to put together a minimal patch with just these concepts and not all the other refactorings like in the resolver and the new exception classes (we'll do those separately as it's best to separate new features from internal cleanups)?

Also, we need to keep the old django.views'django.views.defaults.* views and deprecate them as they are documented.

comment:12 Changed 13 months ago by timo

From IRC: "apollo13 doesn't see why we'd need a generic handler for every http status code out there."

Absent some additional arguments, I guess I'd agree. I'd still like to do some of the code cleanup in resolvers that the existing PR implements, but I'm okay if we "won't fix" this ticket in general.

In the case of the problem described by the ticket, you can override the View.http_method_not_allowed() method if you want to customize the behavior of class-based views.

comment:13 Changed 13 months ago by timo

  • Resolution set to wontfix
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.
Back to Top