#14512: Decorating class based views (was Re: new class based views)
Łukasz Rekucki
> On Wed, 2010-10-20 at 16:05 +0200, Łukasz Rekucki wrote:
>
>> OTOH, it's annoying to have to write an dispatch() method with a super
>> inside (which effectively does nothing), just to apply a decorator.
>
> That's a good point I hadn't thought of, as are Russell's other points.
>
> Just to put it on the table, another option we thought of at one point
> was to have a 'decorators' attribute on the class, which is a list of
> decorators that is applied by the as_view() method. It would in theory
> allow you to add decorators to either end of the set of decorators that
> are applied, something like this:
>
> class MyView(SomeOtherView):
>
> decorators = SomeOtherView.decorators + [my_dec_1, my_dec_2]
I'm addicted to decorators so I would probably end up with a decorator
that adds items
to that list :)
>
> It gets a bit trickier with multiple inheritance. I don't know if that
> would be too much of a big deal - I would imagine that most of the mixin
> classes would not be providing decorators. But then my imagination is
> probably limited.
I didn't find a use case for decorating a mixin yet, but i'm only
starting to explore (I didn't have this problem in any of my own
applications that use CBV). There is a simple pattern to change a
decorator into a mixin that should play well with multiple
inheritance.
On a related note, while writing a class_decorator() I noticed that
method_decorator() doesn't work with decorators that set attributes on
the view (like csrf_exempt). This is because the decorator is invoked
on a closure inside _wrapper() during method call (i.e. during method
call, not class creation), while as_view() only sees _wrapper - which
has no attributes.
--
Łukasz Rekucki
Luke Plant
> On a related note, while writing a class_decorator() I noticed that
> method_decorator() doesn't work with decorators that set attributes on
> the view (like csrf_exempt). This is because the decorator is invoked
> on a closure inside _wrapper() during method call (i.e. during method
> call, not class creation), while as_view() only sees _wrapper - which
> has no attributes.
Good catch! The attached patch should fix it, but it feels a bit icky,
due to the double application of the decorator. Let me know if it solves
this problem for you, or if you can think of a better way to fix it.
Luke
--
If you can't answer a man's arguments, all is not lost; you can
still call him vile names. (Elbert Hubbard)
Luke Plant || http://lukeplant.me.uk/
Łukasz Rekucki
> On Thu, 2010-10-21 at 01:40 +0200, Łukasz Rekucki wrote:
>
>> On a related note, while writing a class_decorator() I noticed that
>> method_decorator() doesn't work with decorators that set attributes on
>> the view (like csrf_exempt). This is because the decorator is invoked
>> on a closure inside _wrapper() during method call (i.e. during method
>> call, not class creation), while as_view() only sees _wrapper - which
>> has no attributes.
>
> Good catch! The attached patch should fix it, but it feels a bit icky,
> due to the double application of the decorator. Let me know if it solves
> this problem for you, or if you can think of a better way to fix it.
I does fell icky. I haven't found a better way to solve this in
general case, so I started thinking about a solution designed for CBV
specifically. Both are a variation on what you proposed earlier:
1) Add get_decorators() class method, that as is called by as_view(),
to get a list of decorators
to be applied on the closure.
class MyView(View):
@classmethod
def get_decorators(cls):
return (login_required,)
This can be folded to a class decorator:
def decorate_class(fdec):
@wraps(fdec)
def _decorator(cls):
original_method = cls.get_decorators
@wraps(original_method)
def method_replacement(cls):
return (fdec,) + original_method()
cls.get_decorators = method_replacement
return cls
return _decorator
@decorate_class(login_required):
class MyView(View):
pass
2) decorators class property:
class MyView(View):
decorators = (login_required,)
# in View:
def as_view(cls, **initkwargs):
# all the stuff now
# now the hack part that follows MRO
all_decorators = sum( (c.decorators for c in cls.mro() if
"decorators" in c.__dict__), [])
for dec in decorators:
view = dec(view)
return view
This also can be turned into a class decorator.
--
Łukasz Rekucki
Luke Plant
> > Good catch! The attached patch should fix it, but it feels a bit icky,
> > due to the double application of the decorator. Let me know if it solves
> > this problem for you, or if you can think of a better way to fix it.
>
> I does fell icky. I haven't found a better way to solve this in
> general case, so I started thinking about a solution designed for CBV
> specifically.
method_decorator does need fixing anyway, and the fix isn't as bad as I
first thought. The existing implementation already suffered from the
following issue: when you decorate a top-level function, the decorator
runs at the time the module is imported. But when you use the same
decorator with method_decorator on a method in a top-level class, the
decorator actually runs every time that method is called - *not* when
the module is imported and the class is defined. With my change, the
decorator also runs once when the class is defined, in addition to every
time the method is called.
> Both are a variation on what you proposed earlier:
>
> 1) Add get_decorators() class method, that as is called by as_view(),
> to get a list of decorators
> to be applied on the closure.
>
> class MyView(View):
> @classmethod
> def get_decorators(cls):
> return (login_required,)
>
> This can be folded to a class decorator:
>
> def decorate_class(fdec):
> @wraps(fdec)
> def _decorator(cls):
> original_method = cls.get_decorators
> @wraps(original_method)
> def method_replacement(cls):
> return (fdec,) + original_method()
> cls.get_decorators = method_replacement
> return cls
> return _decorator
>
> @decorate_class(login_required):
> class MyView(View):
> pass
This one seems much better than the decorators class property I
suggested - no hacking with MRO. Encouraging the use of the class
decorator seems like the way to go. However, there are some points with
your implementation:
1) It mutates the original class which is not a good idea really. I
should be able to do this:
MyView1 = decorate_class(login_required)(MyView)
MyView2 = decorate_class(something_else)(MyView)
and not have login_required applied to MyView to MyView2
That means subclassing inside decorate_class.
2) We need to be careful about the order in which decorators are
applied. I would expect the following 3 chunks of code to be the
equivalent:
@decorate_class(dec1)
@decorate_class(dec2)
class MyView(View):
# ....
class MyView(View):
def get_decorators(self):
return [dec1, dec2]
class MyView(View):
@method_decorator(dec1)
@method_decorator(dec2)
def dispatch(self, request, *args, **kwargs):
#...
All three should effectively be doing something like:
view = dec1(dec2(view))
3) And there is a general design issue. With this get_decorators()
method, there is both flexibility and some surprising behaviour: we
could write something like this:
@decorate_class(dec1)
@decorate_class(dec2)
class MyView(View):
# ....
and someone else could write:
class MySubClass(MyView):
@classmethod
def get_decorators(self):
return []
and dec1 and dec2 would not be applied. I don't know if this is a good
idea or not. If you consider decorators to be part of the
configurability that class based views are designed to support, then it
does make sense. If you consider them as a shortcut to defining the
intrinsic functionality of a view, it doesn't.
Łukasz Rekucki
> On Thu, 2010-10-21 at 15:02 +0200, Łukasz Rekucki wrote:
>
>> > Good catch! The attached patch should fix it, but it feels a bit icky,
>> > due to the double application of the decorator. Let me know if it solves
>> > this problem for you, or if you can think of a better way to fix it.
>>
>> I does fell icky. I haven't found a better way to solve this in
>> general case, so I started thinking about a solution designed for CBV
>> specifically.
>
> method_decorator does need fixing anyway, and the fix isn't as bad as I
> first thought. The existing implementation already suffered from the
> following issue: when you decorate a top-level function, the decorator
> runs at the time the module is imported. But when you use the same
> decorator with method_decorator on a method in a top-level class, the
> decorator actually runs every time that method is called - *not* when
> the module is imported and the class is defined. With my change, the
> decorator also runs once when the class is defined, in addition to every
> time the method is called.
To me, both versions aren't obvious and definitly not something I
would except from a decorator - to run only once. But I don't have any
better ideas myself, so I won't complain.
I thought for a moment there, that modifing the original is inline
with how function decorators work, but you're right. Do you consider
subclassing inside a decorated bad, or is it ok? With subclassing,
this is pretty much
the same as converting a decorator to a Mixin:
class MyView(Decorator(login_required), View):
pass
Of course there's an issuse with MRO and class identity.
>
> 2) We need to be careful about the order in which decorators are
> applied. I would expect the following 3 chunks of code to be the
> equivalent:
>
> @decorate_class(dec1)
> @decorate_class(dec2)
> class MyView(View):
> # ....
>
>
> class MyView(View):
> def get_decorators(self):
> return [dec1, dec2]
>
>
> class MyView(View):
> @method_decorator(dec1)
> @method_decorator(dec2)
> def dispatch(self, request, *args, **kwargs):
> #...
>
>
> All three should effectively be doing something like:
>
> view = dec1(dec2(view))
Agreed. 1 and 3 should be the same. 2 is actually broken. My original
example should be:
class MyView(View):
@classmethod
def get_decorators(cls):
return (login_required,) + super(cls).get_decorators()
That's a big -1 for this approach, 'cause it's error prone. If someone
forgets the super() call it just resets the whole decorator stack (at
least in my naive implementation).
>
> 3) And there is a general design issue. With this get_decorators()
> method, there is both flexibility and some surprising behaviour: we
> could write something like this:
>
> @decorate_class(dec1)
> @decorate_class(dec2)
> class MyView(View):
> # ....
>
>
> and someone else could write:
>
> class MySubClass(MyView):
> @classmethod
> def get_decorators(self):
> return []
>
> and dec1 and dec2 would not be applied. I don't know if this is a good
> idea or not. If you consider decorators to be part of the
> configurability that class based views are designed to support, then it
> does make sense. If you consider them as a shortcut to defining the
> intrinsic functionality of a view, it doesn't.
Consider:
@class_decorator(dec_x)
class A(View):
def dispatch():
#some code A
@class_decorator(dec_y)
class B(A)
def dispatch():
#some code B
What is the order of code execution:
* (dec_y, codeB, dec_x, codeA) or
* (dec_y, dec_x, codeB, codeA).
I think it's the first option - so the anwser would be "shortcut to
defining the intrinsic functionality". But we already have one
mechanism to create this kind of shortcuts - Mixins.
Another question is, how to handle duplicate decorators. If I subclass
from 2 views that already have login_required(), should the decorator
be applied twice or should decorators be unique and be ordered just
like the classes they are defined in? First can be impractical, second
can be very non-obvious. It's hard to choose.
--
Łukasz Rekucki
Łukasz Rekucki
I just added a patch to this ticket. It includes 2 solutions:
First one is an implementation of Luke's idea with a class attribute.
You just define an sequence:
class MyView(View):
decorators = [login_required, never_cache]
# MyView.as_view === login_required(never_cache(View.as_view))
The `as_view` method then applies the decorators to the closure before
returning it. This works nice with
subclassing and isn't very magic. The important thing to note, is that
all decorators are applied before the login enters dispatch(), so this
is different then applying decorators to dispatch().
Second one, introduces a class decorator that decorates the as_view()
method. This can be altered to use method_decorator() to decorate
dispatch() instead, but during the whole process I decided that
decorating as_view() works better (or maybe my brain turned into a
cloud of steam from all this decoration).
This approach has some serious pitfall, I wasn't aware of before:
Subclassing in a decorator + super() in Python 2.x = trouble. See [1]
and [2] for example of how to shot yourself in the foot with it. Thus,
the decorator modifies the class it was given. I added a "subclass"
argument, so you can write:
MyView = view_decorator(login_required, subclass=True)(TemplateView)
but it looks kinda clumsy.
Comments here or directly on my github branch[3] very appreciated.
Sorry, for another long email and thanks for reading it.
[1]: http://github.com/lqc/django/commit/9b32817e00cdfa82568c45506e4c5b17cec68748#L0R53
[2]: http://gist.github.com/643536
[3]: http://github.com/lqc/django/commits/cbvdecoration_ticket14512
Best regards,
Łukasz Rekucki
Łukasz Rekucki
patch and uploaded a new one[1]. Would be good to know if there is
something obviously broken about my patch.
[1]: http://code.djangoproject.com/ticket/14512
Cheers,
Łukasz Rekucki