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

Already on GitHub? Sign in to your account

Refactored common code in handlers/base.py and urlresolvers.py #2828

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

coder9042 commented Jun 18, 2014

What is done:

  • Refactored handlers into generic ones.
  • Deprecated original default views.
  • Added generic views.

Improvements Required:

  • Docs build have several warnings due to removing original handlers.
  • A writeup for new views.

Suggestions:

  • We could add two generic exceptions also: HttpClientException and HttpServerException to add onto the functionality.

@timgraham timgraham and 1 other commented on an outdated diff Jun 18, 2014

django/core/handlers/base.py
if settings.DEBUG:
+ logger.warning('%s: %s', MSG.get(404, None), request.path,
@timgraham

timgraham Jun 18, 2014

Owner

why is this logic moved inside DEBUG?

@coder9042

coder9042 Jun 18, 2014

Contributor

The logging was for whole but since I had shifted the logging to separate function, so in order to prevent it from being logged twice, I moved this inside.

@timgraham

timgraham Jun 20, 2014

Owner

.get() -> []

@timgraham timgraham commented on an outdated diff Jun 18, 2014

django/core/handlers/base.py
@@ -70,6 +71,31 @@ def make_view_atomic(self, view):
view = transaction.atomic(using=db.alias)(view)
return view
+ def call_handler(self, request, resolver, view, client=True):
@timgraham

timgraham Jun 18, 2014

Owner

I think it's make more sense to rename "view" to status_code and pass it as an integer instead of a string.

@timgraham timgraham commented on an outdated diff Jun 18, 2014

django/views/defaults.py
@@ -73,8 +70,52 @@ def permission_denied(request, template_name='403.html'):
If the template does not exist, an Http403 response containing the text
"403 Forbidden" (as per RFC 2616) will be returned.
"""
+ warnings.warn(
+ "This will no longer be supported in Django 2.0.",
+ RemovedInDjango19Warning, stacklevel=2
+ )
+ return error4xx(request, '403', template_name)
+
+@requires_csrf_token
+def error5xx(request, view_type='500', template_name=None):
@timgraham

timgraham Jun 18, 2014

Owner

same as above: view_type -> status_code and pass it as an integer.

@timgraham timgraham commented on an outdated diff Jun 18, 2014

django/views/defaults.py
try:
template = loader.get_template(template_name)
except TemplateDoesNotExist:
- return http.HttpResponseForbidden('<h1>403 Forbidden</h1>', content_type='text/html')
- return http.HttpResponseForbidden(template.render(RequestContext(request)))
+ return http.HttpResponse('<h1>%s (%s)</h1>' % (MSG[status], view_type), status=status, content_type='text/html')
+ return http.HttpResponse(template.render(Context({})), status=status)
+
+
+@requires_csrf_token
+def error4xx(request, view_type='400', template_name=None):
+ """
+ 4xx error handler.
+ """
+ if view_type == '404':
+ return error404(request, template_name)
@timgraham

timgraham Jun 18, 2014

Owner

As discussed on IRC, just keep page_not_found and don't add error404.

@timgraham timgraham commented on an outdated diff Jun 18, 2014

docs/ref/views.txt
@@ -85,6 +85,11 @@ Three things to note about 404 views:
your 404 view will never be used, and your URLconf will be displayed
instead, with some debug information.
+.. deprecated:: 2.0
+
+This function will no longer be supported in Django 2.0.
@timgraham

timgraham Jun 18, 2014

Owner

no longer be supported -> will be removed

@coder9042 coder9042 commented on an outdated diff Jun 18, 2014

docs/ref/views.txt
@@ -105,7 +105,12 @@ If :setting:`DEBUG` is set to ``True`` (in your settings module), then
your 500 view will never be used, and the traceback will be displayed
instead, with some debug information.
-.. _http_forbidden_view:
+.. deprecated:: 2.0
+
+This function will be removed in Django 2.0.
+:ref:`http_error_5xx_view` will be used instead.
+
+.. http_forbidden_view:
@coder9042

coder9042 Jun 18, 2014

Contributor

missed an _. Will fix that on next push.

@timgraham timgraham and 1 other commented on an outdated diff Jun 19, 2014

django/views/defaults.py
@@ -73,8 +77,37 @@ def permission_denied(request, template_name='403.html'):
If the template does not exist, an Http403 response containing the text
"403 Forbidden" (as per RFC 2616) will be returned.
"""
+ warnings.warn(
+ "This will no longer be supported in Django 2.0.",
+ RemovedInDjango20Warning, stacklevel=2
+ )
+ return error4xx(request, 403, template_name)
@timgraham

timgraham Jun 19, 2014

Owner

403 currently uses RequestContext but now uses Context.

@coder9042

coder9042 Jun 19, 2014

Contributor

Yes but I don't think that would create a problem, seeing from the way its was used here.
If it does then do tell me.
Before: http.HttpResponseForbidden(template.render(RequestContext(request)))
Now: http.HttpResponse(template.render(Context({})), status=status_code)

Edits:
Also we could do return http.HttpResponse(template.render(RequestContext(request)), status=status_code) in the generic views. That way it will be same for 403 and also if some extra Context data is passed to others it doesn't matter.
As far as I think this would be a good thing to do.
Thoughts?

@timgraham

timgraham Jun 19, 2014

Owner

I think RequestContext was omitted from at least server_error because if there's a problem with one of the context processors, you'll be unable to render to server_error view. You can probably read about the decisions in tickets.

@coder9042

coder9042 Jun 19, 2014

Contributor

@timgraham
So if we pass Context instead of RequestContext, then is there a problem?
If yes, I could add a check if status_code == 403 and send RequestContext.

@timgraham

timgraham Jun 19, 2014

Owner

Yes, I think we will need to do some special casing unfortunately. If we could find some discussion in tickets for the reasons for the difference, that would help us decide what to do. For example, we may want to use RequestContext by default and special case Context, since we are now generalizing this to other 4XX codes.

@timgraham timgraham and 1 other commented on an outdated diff Jun 19, 2014

docs/ref/urls.txt
@@ -144,61 +144,32 @@ include()
See :ref:`including-other-urlconfs` and :ref:`namespaces-and-include`.
-handler400
@timgraham

timgraham Jun 19, 2014

Owner

need some versionchanged / versionadded annotations

@coder9042

coder9042 Jun 19, 2014

Contributor

@timgraham
Will do that.
Btw because we are no longer having handler400/404/403/500, so there are several problems in the docs especially in deprecation.txt and release_notes where these are linked.
Any suggestions on how to avoid that.

@timgraham

timgraham Jun 19, 2014

Owner

You'll have to remove the links.

@timgraham timgraham commented on an outdated diff Jun 19, 2014

docs/ref/views.txt
@@ -131,6 +136,11 @@ view you can use code like this::
raise PermissionDenied
# ...
+.. deprecated:: 2.0
+
+This function will be removed in Django 2.0.
+:ref:`http_error_4xx_view` will be used instead.
@timgraham

timgraham Jun 19, 2014

Owner

link to defaults.error5xx instead

@coder9042 coder9042 changed the title from Fixed #22058 -- Refactored handlers and added a general mechanism to deal with client and server errors. to Fixed #22058 -- Refactored handlers and added a generic mechanism to deal with client and server errors. Jun 19, 2014

Contributor

coder9042 commented Jun 19, 2014

@timgraham
Docs updated and default views use RequestContext as discussed on IRC.

@timgraham timgraham commented on an outdated diff Jun 20, 2014

django/conf/urls/__init__.py
-handler400 = 'django.views.defaults.bad_request'
-handler403 = 'django.views.defaults.permission_denied'
-handler404 = 'django.views.defaults.page_not_found'
-handler500 = 'django.views.defaults.server_error'
+handler5xx = 'django.views.defaults.error5xx'
@timgraham

timgraham Jun 20, 2014

Owner

please switch the order of these: 4xx before 5xx

@timgraham timgraham commented on an outdated diff Jun 20, 2014

django/core/handlers/base.py
@@ -5,6 +5,7 @@
import types
from django import http
+from django.http.response import REASON_PHRASES as MSG
@timgraham

timgraham Jun 20, 2014

Owner

a better alias would be HTTP_REASON_PHRASES -- MSG is very vague.

@timgraham timgraham and 1 other commented on an outdated diff Jun 20, 2014

django/core/handlers/base.py
@@ -70,6 +71,31 @@ def make_view_atomic(self, view):
view = transaction.atomic(using=db.alias)(view)
return view
+ def call_handler(self, request, resolver, status_code, client=True):
+ logger.warning(
+ '%s: %s', MSG.get(status_code, None), request.path,
@timgraham

timgraham Jun 20, 2014

Owner

I think we can use MSG[status_code] here -- are there any examples where the status code wouldn't be in the dictionary? If not, a better fallback than None (which there is no need to specify as it's the default if .get() doesn't return a value) would be something like 'HTTP %d' % status_code

@coder9042

coder9042 Jun 20, 2014

Contributor

Some status code are not in the dictionary.
I looked at all HTTP codes here: http://en.wikipedia.org/wiki/List_of_HTTP_status_codes

@timgraham timgraham commented on an outdated diff Jun 20, 2014

django/core/handlers/base.py
@@ -70,6 +71,31 @@ def make_view_atomic(self, view):
view = transaction.atomic(using=db.alias)(view)
return view
+ def call_handler(self, request, resolver, status_code, client=True):
+ logger.warning(
+ '%s: %s', MSG.get(status_code, None), request.path,
+ extra={
+ 'status_code': status_code,
+ 'request': request
@timgraham

timgraham Jun 20, 2014

Owner

I know it was already like this, but I prefer including the trailing comma in dictionaries so that if more items are added later, we don't have to modify the line again (keeps diffs and git blame cleaner)

@timgraham timgraham commented on an outdated diff Jun 20, 2014

django/views/defaults.py
@@ -37,11 +41,11 @@ def server_error(request, template_name='500.html'):
Templates: :template:`500.html`
Context: None
"""
- try:
- template = loader.get_template(template_name)
- except TemplateDoesNotExist:
- return http.HttpResponseServerError('<h1>Server Error (500)</h1>', content_type='text/html')
- return http.HttpResponseServerError(template.render(Context({})))
+ warnings.warn(
+ "This will no longer be supported in Django 2.0.",
@timgraham

timgraham Jun 20, 2014

Owner

"django.view.defaults.X will be removed in Django 2.0"

@timgraham timgraham commented on an outdated diff Jun 20, 2014

django/views/defaults.py
@@ -37,11 +41,11 @@ def server_error(request, template_name='500.html'):
Templates: :template:`500.html`
Context: None
"""
- try:
@timgraham

timgraham Jun 20, 2014

Owner

I would prefer to leave these views untouched rather than modify them to use errorXXX.

@timgraham timgraham commented on an outdated diff Jun 20, 2014

django/views/defaults.py
try:
template = loader.get_template(template_name)
except TemplateDoesNotExist:
- return http.HttpResponseForbidden('<h1>403 Forbidden</h1>', content_type='text/html')
- return http.HttpResponseForbidden(template.render(RequestContext(request)))
+ return http.HttpResponse('<h1>%s (%d)</h1>' % (MSG[status_code], status_code), status=status_code, content_type='text/html')
+ if status_code == 400:
@timgraham

timgraham Jun 20, 2014

Owner

context_class = Context if status_code == 400 else RequestContext

@timgraham timgraham commented on an outdated diff Jun 20, 2014

docs/internals/deprecation.txt
@@ -373,9 +373,8 @@ details on these changes.
* ``django.conf.urls.defaults`` will be removed. The functions
:func:`~django.conf.urls.include`, :func:`~django.conf.urls.patterns` and
- :func:`~django.conf.urls.url` plus :data:`~django.conf.urls.handler404`,
- :data:`~django.conf.urls.handler500`, are now available through
- :mod:`django.conf.urls` .
+ :func:`~django.conf.urls.url` `handler404`, `handler500`, are now available
@timgraham

timgraham Jun 20, 2014

Owner

"plus" was removed
` need to be doubled on handlerXXX

@timgraham timgraham commented on an outdated diff Jun 20, 2014

docs/ref/urls.txt
@@ -144,61 +144,36 @@ include()
See :ref:`including-other-urlconfs` and :ref:`namespaces-and-include`.
-handler400
-----------
-
-.. data:: handler400
-
-A callable, or a string representing the full Python import path to the view
-that should be called if the HTTP client has sent a request that caused an error
-condition and a response with a status code of 400.
-
-By default, this is ``'django.views.defaults.bad_request'``. That default
-value should suffice.
-
-See the documentation about :ref:`the 400 (bad request) view
-<http_bad_request_view>` for more information.
+.. versionadded:: 1.8
@timgraham

timgraham Jun 20, 2014

Owner

should go below .. data:: handler4xx

@timgraham timgraham commented on an outdated diff Jun 20, 2014

docs/ref/views.txt
@@ -105,6 +106,11 @@ If :setting:`DEBUG` is set to ``True`` (in your settings module), then
your 500 view will never be used, and the traceback will be displayed
instead, with some debug information.
+.. deprecated:: 2.0
+
+This function will be removed in Django 2.0.
@timgraham

timgraham Jun 20, 2014

Owner

as its functionality has been consolidated into error5xx.

@timgraham timgraham commented on an outdated diff Jun 20, 2014

django/core/urlresolvers.py
@@ -393,26 +393,23 @@ def url_patterns(self):
raise ImproperlyConfigured(msg.format(name=self.urlconf_name))
return patterns
- def _resolve_special(self, view_type):
- callback = getattr(self.urlconf_module, 'handler%s' % view_type, None)
+ def _resolve_special(self, status_code, client=True):
@timgraham

timgraham Jun 20, 2014

Owner

don't seem like we need client=True -- how about getattr(urls, 'handler%dxx' str(status_code)[0])

@timgraham timgraham commented on an outdated diff Jun 20, 2014

django/core/handlers/base.py
@@ -70,6 +71,31 @@ def make_view_atomic(self, view):
view = transaction.atomic(using=db.alias)(view)
return view
+ def call_handler(self, request, resolver, status_code, client=True):
+ logger.warning(
+ '%s: %s', MSG.get(status_code, None), request.path,
+ extra={
+ 'status_code': status_code,
+ 'request': request
+ })
+ try:
+ # Although we currently are not catching any exception
@timgraham

timgraham Jun 20, 2014

Owner

I would prefer to omit it for now.

@timgraham timgraham and 1 other commented on an outdated diff Jun 20, 2014

django/core/handlers/base.py
@@ -70,6 +71,31 @@ def make_view_atomic(self, view):
view = transaction.atomic(using=db.alias)(view)
return view
+ def call_handler(self, request, resolver, status_code, client=True):
+ logger.warning(
+ '%s: %s', MSG.get(status_code, None), request.path,
+ extra={
+ 'status_code': status_code,
+ 'request': request
+ })
+ try:
+ # Although we currently are not catching any exception
+ # related to server error, but 'client' is used so that
+ # when we create generic exceptions it will be helpful.
+ if client:
+ callback, param_dict = resolver.resolve4xx(status_code)
@timgraham

timgraham Jun 20, 2014

Owner

Just use _resolve_special(status_code) as noted below, I don't think we need different resolveXXX variants.

@coder9042

coder9042 Jun 20, 2014

Contributor

I have created resolve4xx and resolve5xx so as to keep the behaviour as earlier as their was resolve404/403/400/500 all of which called _resolve_special. Basically I think it was for better usage by user as you may see in the tests.

@timgraham

timgraham Jun 20, 2014

Owner

how about _resolve_special -> resolve_error_handler(). I don't see a need for separate methods like resolve4XX and resolve5XX, especially when you pass any status code to either and achieve the same result.

@timgraham timgraham commented on an outdated diff Jun 20, 2014

django/core/handlers/base.py
@@ -70,6 +71,31 @@ def make_view_atomic(self, view):
view = transaction.atomic(using=db.alias)(view)
return view
+ def call_handler(self, request, resolver, status_code, client=True):
@timgraham

timgraham Jun 20, 2014

Owner

I think get_exception_response would be a better name for the method.

@timgraham timgraham commented on an outdated diff Jun 20, 2014

django/views/defaults.py
+ 5xx error handler.
+ """
+ if not template_name:
+ template_name = "%d.html" % status_code
+ try:
+ template = loader.get_template(template_name)
+ except TemplateDoesNotExist:
+ return http.HttpResponse('<h1>%s (%d)</h1>' % (MSG[status_code], status_code), status=status_code, content_type='text/html')
+ if status_code == 500:
+ return http.HttpResponse(template.render(Context({})), status=status_code)
+ else:
+ return http.HttpResponse(template.render(RequestContext(request)), status=status_code)
+
+
+@requires_csrf_token
+def error4xx(request, status_code=400, template_name=None):
@timgraham

timgraham Jun 20, 2014

Owner

Please reorder to put error4xx before error5xx.

Owner

timgraham commented Jun 20, 2014

A lot of my comments are applicable in multiple places but I haven't commented every time, so please take care to review the entire diff carefully and apply them as applicable Thanks!

@timgraham timgraham commented on an outdated diff Jun 20, 2014

django/views/defaults.py
+dep_msg = 'django.view.defaults."%s" will be removed in Django 2.0.'
@timgraham

timgraham Jun 20, 2014

Owner

module level constants should be upper case -- also try to use more descriptive names like DEPRECATED_VIEW_MSG

@coder9042 coder9042 changed the title from Fixed #22058 -- Refactored handlers and added a generic mechanism to deal with client and server errors. to Fixed #22058 -- Refactored handler and created a generic mechanism to deal with server/client errors. Jun 20, 2014

Contributor

coder9042 commented Jun 21, 2014

@timgraham
I have adjusted the code of params['status_code'] in urlresolvers.py. This way the user now can define his handlers the way he likes. Added a test to demonstrate the complete functioning of custom handlers with custom views.

Contributor

coder9042 commented Jun 21, 2014

@timgraham
Also regarding checking url_conf for any definition of custom handlers, its already doing. From the very beginning, I had ensured that.

@coder9042 coder9042 and 1 other commented on an outdated diff Jun 21, 2014

docs/releases/1.8.txt
@@ -332,6 +332,16 @@ The check also applies to the columns generated in an implicit
and then specify :attr:`~django.db.models.Field.db_column` on its column(s)
as needed.
+Handlers refactored into generic ones
@coder9042

coder9042 Jun 21, 2014

Contributor

@timgraham
I think we could mention why we are putting this here, i.e. if anyone tries to import them else it would seem strange to put something like this under backwards incompatible and might create confusion.
Or we could put it under a different section, that way users will be informed of the new way of working.

IMO, second option is better..what do you say as mostly people do not import this rather define their custom handers in url_conf and that is backwards compatible and this above note might confuse them.

@timgraham

timgraham Jun 21, 2014

Owner

Yes, of course we should say why it's backwards incompatible (and that you can still define your own handlers).

Contributor

coder9042 commented Jun 21, 2014

buildbot, test this please.

Owner

timgraham commented Jun 21, 2014

Re: checking url_conf for any definition of custom handlers -- unless I'm missing something, currently the user cannot define a custom catch-all handler4xx or handler5xx, only a specific one like handler404.

@timgraham timgraham commented on an outdated diff Jun 21, 2014

tests/view_tests/views.py
@@ -60,6 +61,18 @@ def raises404(request):
resolver.resolve('/not-in-urls')
+def error400(request):
+ return HttpResponseBadRequest('Custom 400', content_type="text/html")
@timgraham

timgraham Jun 21, 2014

Owner

content_type seems like an extra detail that we don't really need for these tests (along with the corresponding assertions)

@timgraham timgraham commented on an outdated diff Jun 21, 2014

django/views/defaults.py
try:
template = loader.get_template(template_name)
except TemplateDoesNotExist:
return http.HttpResponseForbidden('<h1>403 Forbidden</h1>', content_type='text/html')
return http.HttpResponseForbidden(template.render(RequestContext(request)))
+
+@requires_csrf_token
+def error4xx(request, status_code=400, template_name=None):
+ """
+ 4xx error handler.
+ """
+ if not template_name:
+ template_name = "%d.html" % status_code
+ if status_code == 404:
+ return page_not_found(request, template_name)
+ try:
+ template = loader.get_template(template_name)
+ except TemplateDoesNotExist:
+ return http.HttpResponse('<h1>%s (%d)</h1>' % (HTTP_REASON_PHRASES.get(status_code, 'HTTP'), status_code), status=status_code, content_type='text/html')
@timgraham

timgraham Jun 21, 2014

Owner

we could multi-line this and the similar line in error5xx.

Contributor

coder9042 commented Jun 22, 2014

@timgraham
Sorry misinterpreted on handler 4|5xx. Incorporated it now. Other changes made as well.

Contributor

coder9042 commented Jun 22, 2014

@timgraham
Is anything else required or its good to go ?

@coder9042 coder9042 changed the title from Fixed #22058 -- Refactored handler and created a generic mechanism to deal with server/client errors. to Fixed #22058 -- Refactored code in handlers/base.py and urlresolvers.py Jun 23, 2014

Contributor

coder9042 commented Jun 23, 2014

PR changed to refactor code only as per discussion with @timgraham and @apollo13 .

@timgraham timgraham and 1 other commented on an outdated diff Jun 23, 2014

django/core/handlers/base.py
@@ -70,6 +71,22 @@ def make_view_atomic(self, view):
view = transaction.atomic(using=db.alias)(view)
return view
+ def get_exception_response(self, request, resolver, status_code):
+ logger.warning(
@timgraham

timgraham Jun 23, 2014

Owner

I think we should revert the logging changes as it appears we're adding additional logging calls where they didn't exist before.

@coder9042

coder9042 Jun 23, 2014

Contributor

Except in case of 403, we are not.
In that also warnings are being logged at different loggers

@coder9042

coder9042 Jun 23, 2014

Contributor

403 uses security logger but does not logs at default logger, I think we could do that..
edits: I meant logging at both places..

@timgraham

timgraham Jun 23, 2014

Owner

And we are now logging 500s twice... and the names like "Not Found" are now different. It's not worth all these differences and inconsistencies.

@coder9042

coder9042 Jun 23, 2014

Contributor

How come we are logging 500 twice?

And logging messages are those in http/response.py, so they are same I think.

@timgraham

timgraham Jun 23, 2014

Owner

My mistake on 500, but "NOT FOUND" is different from "Not Found"

@coder9042

coder9042 Jun 23, 2014

Contributor

Case should not be a problem.
I think if just because of the case, we are writing a code thrice instead of once, then its not good.
But still if you say, I'll revert back.

@timgraham

timgraham Jun 23, 2014

Owner

If we were starting from scratch, we'd probably go with this, but I'd rather not change it now.

Use a local status_code=40X variable in the except clauses if you don't want to respond the int.

@coder9042

coder9042 Jun 23, 2014

Contributor

Didn't get what you meant by last line.

@timgraham

timgraham Jun 23, 2014

Owner

e.g.

 except http.Http404 as e:
    status_code = 404
    logger.warning('Not Found: %s', request.path,
        extra={
          'status_code': status_code,
...
@coder9042

coder9042 Jun 23, 2014

Contributor

What purpose will it serve?
How its diff from 'status_code':404 ?
I don't understand it yet.

@timgraham

timgraham Jun 23, 2014

Owner

It's my suggestion to address your concern: ""we are writing a code thrice instead of once"

@coder9042 coder9042 commented on an outdated diff Jun 23, 2014

django/core/handlers/base.py
@@ -186,14 +185,7 @@ def get_response(self, request):
'request': request
})
- try:
- callback, param_dict = resolver.resolve400()
- response = callback(request, **param_dict)
- except:
- signals.got_request_exception.send(
- sender=self.__class__, request=request)
- response = self.handle_uncaught_exception(request,
- resolver, sys.exc_info())
+ response = self.get_exception_response(request, resolver, 404)
@coder9042

coder9042 Jun 23, 2014

Contributor

Should be 400, i'll change it when I squash and push.

@coder9042 coder9042 changed the title from Fixed #22058 -- Refactored code in handlers/base.py and urlresolvers.py to Refactored common code in handlers/base.py and urlresolvers.py Jun 23, 2014

Owner

timgraham commented Jun 23, 2014

merged in 7f76251.

@timgraham timgraham closed this Jun 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment