#1015 closed defect (fixed)
django.utils.decorators.decorator_from_middleware doesn't work with parameters
| Reported by: | Owned by: | Adrian Holovaty | |
|---|---|---|---|
| Component: | Tools | Version: | |
| Severity: | normal | Keywords: | cache_page |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | yes | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
django.utils.decorators.decorator_from_middleware doesn't work for decorators with parameters:
- Any parameter of decorator causes a call to
_decorator_from_middleware(). - Instead of actual wrapping
_wrapped_view()is called, which fails because at that pointrequestis actually a view function.
Basically this works:
@cache_page def my_cool_view(request): # cool processing
This doesn't:
@cache_page(60) def my_cool_view(request): # cool processing
Nor this:
@cache_page(cache_timeout=60) def my_cool_view(request): # cool processing
Just try it.
If this is an intended behavior, we need to update documentation for @cache_page. (I didn't check if there is a documentation for @gzip_page and @conditional_page).
It makes sense to document that per-page caching still uses CACHE_MIDDLEWARE_SECONDS and CACHE_MIDDLEWARE_KEY_PREFIX, if no decorator arguments were given. And currently it is impossible to specify them.
Attachments (2)
Change History (15)
comment:1 by , 19 years ago
comment:2 by , 18 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:3 by , 18 years ago
Possible solution, keeping backward compatibility:
--- django/utils/decorators.py (revision 4490)
+++ django/utils/decorators.py (working copy)
@@ -30,4 +30,12 @@
return result
return response
return _wrapped_view
+
+ def _true_24_decorator(*args, **kwargs):
+ def _decorator(f):
+ return _decorator_from_middleware(f, *args, **kwargs)
+ return _decorator
+
+ _decorator_from_middleware.decorator = _true_24_decorator
+
return _decorator_from_middleware
Usage:
@cache_page.decorator(60)
def my_cool_view(request):
# cool processing
If it's ok, then feel free to rename the "decorator" attribute to something nice and short enough to type.
comment:4 by , 18 years ago
| Component: | Documentation → Uncategorized |
|---|---|
| Has patch: | set |
| Needs tests: | set |
| Patch needs improvement: | set |
comment:6 by , 18 years ago
| Component: | Uncategorized → Tools |
|---|---|
| Keywords: | cache_page added |
| Owner: | changed from to |
comment:7 by , 18 years ago
Decorators that take arguments need to be functions that return a function that returns a decorator. See these docs for info:
- http://www.python.org/doc/2.4.4/whatsnew/node6.html
- http://www.python.org/dev/peps/pep-0318/
- http://mail.python.org/pipermail/python-dev/2004-September/048874.html
From PEP 318:
The current syntax also allows decorator declarations to call a function that returns a decorator:
@decomaker(argA, argB, ...) def func(arg1, arg2, ...): pass
This is equivalent to:
func = decomaker(argA, argB, ...)(func)
The cache_page decorator appears to be the only one using decorator_from_middleware that also takes arguments, but decorator_from_middleware does not handle decorators that take arguments.
Also the cache documentation is incorrect, because instead of:
slashdot_this = cache_page(slashdot_this, 60 * 15)
it should be
slashdot_this = cache_page(60 * 15)(slashdot_this)
by , 18 years ago
comment:8 by , 18 years ago
Description of attached patch:
- Added a
decorator_with_arguments_from_middlewarefunction for generating functions that generate decorators from middleware classes whose constructors take arguments. - Factored out the inner decorator function from in
decorator_from_middlewareinto amake_decorator_from_middlewarefunction that generates decorators from middleware classes. - Changed the
cache_pagedecorator to usedecorator_with_arguments_from_middlewaresince it takes parameters. - Added
CACHE_MIDDLEWARE_SECONDSto the global settings. You will get an error when usingcache_pagewithout this setting since it is using theCacheMiddleware, which usesCACHE_MIDDLEWARE_SECONDS. - Documented the fact that
cache_pagecan take more than one argument.
comment:9 by , 18 years ago
| Patch needs improvement: | unset |
|---|
I should note that the patch is backwards incompatible with people using the cache_page decorator as documented:
slashdot_this = cache_page(slashdot_this, 60 * 15)
comment:10 by , 18 years ago
Would be nice to avoid the backwards incompatibility if we could. A new name for cache_page would be one way (then cache_page() would work as it does now and the new named function would be the new decorator).
Most of the rest looks great. Will probably give decorator_with_arguments_from_middleware a shorter name to avoid inflicting RSI on the world's population, but that's only cosmetic.
comment:11 by , 17 years ago
comment:12 by , 17 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
Duplicate of #1047.