RFC: "universal" view decorators

[Jacob Kaplan-Moss]

Hi folks --

I'd like to convert all the view decorators built into Django to be
"universal" -- so they'll work to decorate *any* view, whether a
function, method, or class. I believe I've figured out a technique for
this, but I'd like some feedback on the approach before I dive in too
deep.

Right now view decorators (e.g. @login_required) only work on
functions. If you want to use a decorator on a method then you need to
"convert" the decorator using method_decorator(original_decorator).
You can't use view decorators on class-based views at all. This means
making a class-based view require login requires this awesomeness::

class MyView(View):
@method_decorator(login_required)
def dispatch(self, *args, **kwargs):
return super(MyView, self.dispatch(*args, **kwargs)

This makes me sad. It's really counter-intuitive and relies on a
recognizing that functions and methods are different to even know to
look for method_decorator.

#14512 proposes a adding another view-decorator-factory for decorating
class-based views, which would turn the above into::

@class_view_decorator(login_required)
class MyView(View):
...

This makes me less sad, but still sad. Factory functions. Ugh.

I want @login_required to work for anything::

@login_required
class MyView(View):
...

class Something(object):
@login_required
def my_view(self, request):
...

@login_required
def my_view(request):
...

Now, back in the day we somewhat had this: decorators tried to work
with both functions and methods. The technique turned out not to work
(see #12804) and was removed in [12399]. See
http://groups.google.com/group/django-developers/browse_thread/thread/3024b14a47f5404d
for the discussion.

I believe, however, I've figured out a different technique to make
this work: don't try to detect bound versus unbound methods, but
instead look for the HttpRequest object. It'll either be args[0] if
the view's a function, or args[1] if the view's a method. This
technique won't work for any old decorator, but it *will* work (I
think) for any decorator *designed to be applied only to views*.

I've written a proof-of-concept patch to @login_required (well,
@user_passes_test, actually):

https://gist.github.com/1220375

The test suite passes with this, with one exception:
https://code.djangoproject.com/browser/django/trunk/tests/regressiontests/decorators/tests.py#L87.
I maintain that this test is broken and should be using RequestFactory
instead.

Can I get some thoughts on this technique and some feedback on whether
it's OK to apply to every decorator built into Django?

Jacob

[Donald Stufft]

Gonna add this in here as well as ticket #14512

I think using decorators to modify the way a CBV behaves is the wrong way to go about it, my reasoning is:

1) Decorators on functions make sense, since the only way to modify a function is to wrap it, so decorators provide a shortcut to do so.

2) There's already a standard way to add functionality to a class that has existed for a long time, is well tested, and as a bonus is something that people new to python but not new to programming will understand immediately.

3) Decorating a class is different to how adding any other kind of functionality to a CBV is done.

4) Customizability. Currently your only options to change the way one of the current decorators work is to rewrite it, unless the original author of the decorator thought of your use case, thought it was valid, and added a flag/option for it.

    - This can be alleviated by using Object based decorators instead of function based, but that becomes uglier too because then you end up having to first subclass the decorator, then decorate the class.

5) Conceptually all the decorator really is doing is mixing in one method anyways, it's just hiding this away and adding more code and complexity, and reducing customizability and readability for the sole purpose of being able to use an @ symbol.

6) Forwards compatibility. The function based generic views have been deprecated. While function based views will probably always be supported, personally I think CBV's are the way forward, and it makes sense to have them be first class citizens as regards to adding in things like requiring logins, with the function based views having the helper code.

7) Backwards compatibility. With a (much simpler) bit of wrapper code, the decorators can easily still be supported (login_required etc) and can internally use the classes and present the same interface as they used to. The major difference being, now they'll be able to subclass them and customize tiny bits of it as well.

To just expand a little more in general on this

@login_required

class ProtectedView(TemplateView):

    template_name = "secret.html"

vs

class ProtectedView(LoginRequired, TemplateView):

    template_name = "secret.html"

In the first, the "things" defining how this CBV behaves are in two locations, which makes it harder to read, additionally you end up with a function that wraps a function that wraps a function (to some degree of wrapping) to actually get all of this to work. Each layer of wrapping adds another layer of indirection, and adds another layer of confusion for someone attempting to read the code.

I really dumbed down example would be https://gist.github.com/1220512 . Obviously that isn't working code, but it gives a general idea of what I mean. Obviously if that is the path that is chosen it will need to be cleaned up and made to actually work.

Hopefully this email makes sense. I just hope that since CBV's are new we can use this as an opportunity to do things in a cleaner, consistent and more generic way instead of continuing the old way (that made sense for functions) for the sake of doing it the same way.

tl;dr; Using Mixins to add in functionality to a CBV makes way more sense then using a decorator which is essentially going to be doing the same thing as a mixing would, it just makes finding what is going on harder, makes customizing the decorator harder and/or uglier, and goes against the way functionality is currently added to a CBV.

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.

[Łukasz Rekucki]

Hi,

On 15 September 2011 22:44, Jacob Kaplan-Moss <ja...@jacobian.org> wrote:
>
> #14512 proposes a adding another view-decorator-factory for decorating
> class-based views, which would turn the above into::
>
>    @class_view_decorator(login_required)
>    class MyView(View):
>        ...
>
> This makes me less sad, but still sad. Factory functions. Ugh.

As the ticket creator I feel obligated to reply :)

Thinking about it now, it does look kinda ugly. It's mainly because I
was focus on how to reuse existing decorators in CBV context. It
shouldn't be to hard to make the "view_decorator" a meta-decorator
(or a factory function as you call it) that turns login_required to
something that is both a class and function decorator. Methods are
still out-of-bounds for non-runtime introspection, but after almost 1
year of using CBV, I never needed to decorate a method.

I would like to also comment on the new approach in that ticket.
Making a shallow copy of a class is *MAGIC* to me. It breaks the basic
invariant "issubsclass(decorator(A), A) == True". This is important if
you're planing to use this as "B = decorator()(A)" (and you are,
'cause the whole point of not modifying the original class is to allow
safely doing this), as you quickly end up with weird alternate
hierarchies. So, please don't do that :)

>
> I believe, however, I've figured out a different technique to make
> this work: don't try to detect bound versus unbound methods, but
> instead look for the HttpRequest object. It'll either be args[0] if
> the view's a function, or args[1] if the view's a method. This
> technique won't work for any old decorator, but it *will* work (I
> think) for any decorator *designed to be applied only to views*.

+1 on this. I would be a bit concerned about this vs. future
implementation of generator-based-views that allow some kind of
response streaming (is someone working on that?).

>
> I've written a proof-of-concept patch to @login_required (well,
> @user_passes_test, actually):
>
>    https://gist.github.com/1220375
>

Did I already mention creating a subclass in the class decorator
breaks super ;) [https://gist.github.com/643536]

> Can I get some thoughts on this technique and some feedback on whether
> it's OK to apply to every decorator built into Django?
>

It would be great to have that meta-decorator, so everyone else could
upgrade their decorators just by decorating them.

--
Łukasz Rekucki

[Łukasz Rekucki]

2011/9/16 Łukasz Rekucki <lrek...@gmail.com>:
> Hi,

>> I believe, however, I've figured out a different technique to make
>> this work: don't try to detect bound versus unbound methods, but
>> instead look for the HttpRequest object. It'll either be args[0] if
>> the view's a function, or args[1] if the view's a method. This
>> technique won't work for any old decorator, but it *will* work (I
>> think) for any decorator *designed to be applied only to views*.
>
> +1 on this. I would be a bit concerned about this vs. future
> implementation of generator-based-views that allow some kind of
> response streaming (is someone working on that?).

Omit this part, It's late and I read HttpResponse by mistake ;), which
of course doesn't make any sense.

--
Łukasz Rekucki

[Łukasz Rekucki]

--
Łukasz Rekucki

[Jonas H.]

On 09/15/2011 11:27 PM, Donald Stufft wrote:
> tl;dr; Using Mixins to add in functionality to a CBV makes way more sense then
> using a decorator which is essentially going to be doing the same thing as a
> mixing would, it just makes finding what is going on harder, makes customizing
> the decorator harder and/or uglier, and goes against the way functionality is
> currently added to a CBV.

That sounds a lot better to me.

[Jonathan Slenders]

I agree with Donald's reasoning. Decorators belong to functional
programming, not to OO programming. Though, I still like to keep using
functions for my own views and there are decorators appropriate.

But if you go class based, you better use inheritance. However,
instead of mix-ins, I'd rather prefer a more declarative approach,
like this:

class ProtectedView(MyView):
login_required = True

Where 'MyView' checks all the view's options during a dispatch. Never
liked inheritance with multiple base classes...

[Roald de Vries]

Hi,

On Sep 16, 2011, at 12:11 AM, Łukasz Rekucki wrote:

> Hi,
>
> On 15 September 2011 22:44, Jacob Kaplan-Moss <ja...@jacobian.org>
> wrote:
>>
>> #14512 proposes a adding another view-decorator-factory for
>> decorating
>> class-based views, which would turn the above into::
>>
>> @class_view_decorator(login_required)
>> class MyView(View):
>> ...
>>
>> This makes me less sad, but still sad. Factory functions. Ugh.
>
> As the ticket creator I feel obligated to reply :)

Me (as the poster of the latest patch) too :)

> Thinking about it now, it does look kinda ugly. It's mainly because I
> was focus on how to reuse existing decorators in CBV context. It
> shouldn't be to hard to make the "view_decorator" a meta-decorator
> (or a factory function as you call it) that turns login_required to
> something that is both a class and function decorator. Methods are
> still out-of-bounds for non-runtime introspection, but after almost 1
> year of using CBV, I never needed to decorate a method.
>
> I would like to also comment on the new approach in that ticket.
> Making a shallow copy of a class is *MAGIC* to me. It breaks the basic
> invariant "issubsclass(decorator(A), A) == True". This is important if
> you're planing to use this as "B = decorator()(A)" (and you are,
> 'cause the whole point of not modifying the original class is to allow
> safely doing this), as you quickly end up with weird alternate
> hierarchies. So, please don't do that :)

I agree that in an ideal world we'd have a better solution. On the
other hand, the cbv-decorator using inheritance just didn't work, and
the one using shallow copies does (if you don't use B=decorator()(A),
indeed).

As Donald mentioned, the more elegant solution for classes is to use
base classes (mixins) instead of class decorators. I'm not sure though
if every decorator can be replaced by a mixin, and if the order of
these mixins can be arbitrary in that case (which I believe is
desirable). Can anybody comment on that?

>> I believe, however, I've figured out a different technique to make
>> this work: don't try to detect bound versus unbound methods, but
>> instead look for the HttpRequest object. It'll either be args[0] if
>> the view's a function, or args[1] if the view's a method. This
>> technique won't work for any old decorator, but it *will* work (I
>> think) for any decorator *designed to be applied only to views*.
>
> +1 on this. I would be a bit concerned about this vs. future
> implementation of generator-based-views that allow some kind of
> response streaming (is someone working on that?).
>
>> I've written a proof-of-concept patch to @login_required (well,
>> @user_passes_test, actually):
>>
>> https://gist.github.com/1220375
>
> Did I already mention creating a subclass in the class decorator
> breaks super ;) [https://gist.github.com/643536]
>
>> Can I get some thoughts on this technique and some feedback on
>> whether
>> it's OK to apply to every decorator built into Django?
>
> It would be great to have that meta-decorator, so everyone else could
> upgrade their decorators just by decorating them.

If this doesn't exist, then the view_decorator still has a right to
live.

So what would have to be created is a meta-decorator
universal_decorator, replacing or using view_decorator, such that
universal_decorator(login_required) is the actual universal decorator.
This would be applied to all django-decorators, but I think it would
be good to also allow applying universal_decorator to an already
universal decorator, such that:

# after this...
login_required = universal_decorator(login_required)
# ... this will hold
assert login_required == universal_decorator(login_required)

[Roald de Vries]

On Sep 16, 2011, at 10:08 AM, Jonathan Slenders wrote:

> class ProtectedView(MyView):
> login_required = True
>
> Where 'MyView' checks all the view's options during a dispatch. Never
> liked inheritance with multiple base classes...

How would I create my own 'decorators' in this approach?

Cheers, Roald

[Anssi Kääriäinen]

I think the syntax should be something like:

from django.contrib.auth.decorators import login_required,
permission_required

class ProtectedView(MyView):
view_decorators = [login_required,
permission_required('foo.can_do_bar')]

The difference to actually decorating the class is small, but I think
the above is a bit more explicit about what is happening.

Mixins sound like a good alternative, but does this just move the
problem in to mixin class instead of the base view class? That is, how
should the mixin class be implemented taking in account that you should
be able to decorate the function with multiple decorators? It seems you
would need something like ViewDecoratorMixinBase to take care of all the
dirty details...

- Anssi

[Łukasz Rekucki]

> I think the syntax should be something like:
>
> from django.contrib.auth.decorators import login_required,
> permission_required
>
> class ProtectedView(MyView):
>    view_decorators = [login_required, permission_required('foo.can_do_bar')]
>

There was a similar proposal on this list before introducing CBV. It
didn't gain much love.

--
Łukasz Rekucki

[Łukasz Rekucki]

Surprisingly, I agree. Using decorators to *add functionality* is a
bad idea. But `login_required` or `csrf_exempt` are not strictly
functionalities. They're preconditions. They act exactly like any
piece of middleware, but instead of applying them globally to all
view, you can do it per-view. (My view_decorator intentionally uses
as_view() instead of dispatch(), so that the decorators are called
*before*, any view login is called). If you think about it this way,
applying decorators to CBV makes a lot of sense.

--
Łukasz Rekucki

[Łukasz Rekucki]

On 16 September 2011 10:17, Roald de Vries <down...@gmail.com> wrote:
> On Sep 16, 2011, at 12:11 AM, Łukasz Rekucki wrote:
>>
>> As the ticket creator I feel obligated to reply :)
>
> Me (as the poster of the latest patch) too :)

Nice to meet you.

>
>> Thinking about it now, it does look kinda ugly. It's mainly because I
>> was focus on how to reuse existing decorators in CBV context. It
>> shouldn't be to hard  to make the "view_decorator" a meta-decorator
>> (or a factory function as you call it) that turns login_required to
>> something that is both a class and function decorator. Methods are
>> still out-of-bounds for non-runtime introspection, but after almost 1
>> year of using CBV, I never needed to decorate a method.
>>
>> I would like to also comment on the new approach in that ticket.
>> Making a shallow copy of a class is *MAGIC* to me. It breaks the basic
>> invariant "issubsclass(decorator(A), A) == True". This is important if
>> you're planing to use this as "B = decorator()(A)" (and you are,
>> 'cause the whole point of not modifying the original class is to allow
>> safely doing this), as you quickly end up with weird alternate
>> hierarchies. So, please don't do that :)
>
> I agree that in an ideal world we'd have a better solution. On the other
> hand, the cbv-decorator using inheritance just didn't work, and the one
> using shallow copies does (if you don't use B=decorator()(A), indeed).

The one that just alters the given class also works, has no magic and
has the same limitation - dont' do "B = decorator(A)". Honestly, I
never needed that, but I understand some people might. Also note, that
the "super() problem" is fixed in Python 3, where super() determines
the base class from the call frame instead of referencing a global
variable.

>
> If this doesn't exist, then the view_decorator still has a right to live.
>
> So what would have to be created is a meta-decorator universal_decorator,
> replacing or using view_decorator, such that
> universal_decorator(login_required) is the actual universal decorator. This
> would be applied to all django-decorators, but I think it would be good to
> also allow applying universal_decorator to an already universal decorator,
> such that:
>
>    # after this...
>    login_required = universal_decorator(login_required)
>    # ... this will hold
>    assert login_required == universal_decorator(login_required)
>
>

+1

--
Łukasz Rekucki

[Mikhail Korobov]

I don't agree with most points because they are assuming functions are less fancy, less customizable, less clean, more complex, etc than classes and this is not true (but let's not start FP vs OOP holywar here, FP and OOP are friends in python).

I like Jacob's proposal because there should be one way to do something and this way can't be mixins because they can't work with functions. Decorators can work with classes, this is standard python and it will be nice to have them work with class-based views.

[Carl Meyer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi Łukasz,

On 09/16/2011 03:42 AM, Łukasz Rekucki wrote:
> The one that just alters the given class also works, has no magic and
> has the same limitation - dont' do "B = decorator(A)". Honestly, I
> never needed that, but I understand some people might. Also note, that
> the "super() problem" is fixed in Python 3, where super() determines
> the base class from the call frame instead of referencing a global
> variable.

The failure mode for "just alter the given class" (i.e. monkeypatching)
is much worse if you're applying the decorator to a class defined
elsewhere (e.g. a non-subclassed generic view; and applying, say,
login_required to a generic view is a relatively common case). It
modifies the generic view itself, and suddenly every use of that view in
your entire codebase is login_required. Nasty and surprising bug, that.

Compared to this (and breaking super()), "weird alternate hierarchies"
and breaking issubclass is a minor issue. Not to say that I like it -
but we're dealing with a set of options that are all pretty bad, IMO.

Carl
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5zNRgACgkQ8W4rlRKtE2didwCghtDhwFjGgnMKySa6egPRAnnI
ka4AnigoQlKV+//MUH6ze2LWzkra1CG3
=Duo1
-----END PGP SIGNATURE-----

[Roald de Vries]

On Sep 16, 2011, at 11:42 AM, Łukasz Rekucki wrote:

> On 16 September 2011 10:17, Roald de Vries <down...@gmail.com> wrote:
>> On Sep 16, 2011, at 12:11 AM, Łukasz Rekucki wrote:
>>> I would like to also comment on the new approach in that ticket.
>>> Making a shallow copy of a class is *MAGIC* to me. It breaks the
>>> basic
>>> invariant "issubsclass(decorator(A), A) == True". This is
>>> important if
>>> you're planing to use this as "B = decorator()(A)" (and you are,
>>> 'cause the whole point of not modifying the original class is to
>>> allow
>>> safely doing this), as you quickly end up with weird alternate
>>> hierarchies. So, please don't do that :)
>>
>> I agree that in an ideal world we'd have a better solution. On the
>> other
>> hand, the cbv-decorator using inheritance just didn't work, and the
>> one
>> using shallow copies does (if you don't use B=decorator()(A),
>> indeed).
>
> The one that just alters the given class also works, has no magic and
> has the same limitation - dont' do "B = decorator(A)".

To show that this is actually a Python-thing, an analogous non-
decorator-example:

>>> class A(Base):
... def method(self, *args, **kwargs):
... super(A, self).method(*args, **kwargs)
...
... B = A
... A = SomeOtherClass
...
... B.method()
Traceback (most recent call last):
...
TypeError: unbound method method() must be called with A instance
as first argument (got nothing instead)

So IMHO it's "pythonic" to leave responsibility to use the decorator
in the right way to the programmer.

Cheers, Roald

[Carl Meyer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

My comments:

The class-decorator support isn't really universal, it's specific to
Django's built-in class-based views. This would need to be
well-documented, and the decorator should also probably check for the
existence of a "dispatch" method and say "sorry, don't know how to
decorate this class" if it's not there. (Could also check
issubclass(View) but that would prevent use with API-compatible CBVs not
descended from Django's base View).

I don't much like seeing an isinstance(HttpRequest) check introduced
(for the first time) as an implicit constraint on what can be passed to
a view. I think this is likely to break view tests in the wild using
non-HttpRequest-descended dummy requests (just like the test that fails
in Django). I don't think there's any good reason to call such tests
"broken" and make people change them, besides the desire to implement
this patch. More generally, the isinstance() requirement doesn't feel
very Pythonic.

Personally I think that the documented status quo for decorating CBVs is
adequate, if not ideal. The rundown here didn't mention the
"login_required(MyView.as_view())" approach that works in many, if not
most, cases and is simple, non-magical, and non-ugly (it also isn't
restricted to urls.py, you can just as easily do it in views.py right
under the class definition). I would be in favor of updating the docs to
recommend this approach even more strongly, showing more advanced usage
with decorator-factories (e.g. user_passes_test), and recommending the
method-decorator alternative only in the case where you want to decorate
a CBV and then subclass it (the wording in the current docs about
decorating "all instances" assumes that the as_view() approach can only
be used in urls.py).

Regardless of whether its a magical all-in-one decorator-decorator like
that proposed here, or something like class_view_decorator, IMO all of
the proposed approaches for class decoration on #14512 have serious
enough downsides that we're better off with the status quo.

Carl
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5zTk0ACgkQ8W4rlRKtE2dhtQCg4GbN/Vht7KTZFiqMQsqvmt1/
VB8AoI69o+NHNnceR5q9p8kzC9BaNJ87
=2rSD
-----END PGP SIGNATURE-----

[Daniel Moisset]

On Thu, Sep 15, 2011 at 6:27 PM, Donald Stufft <donald...@gmail.com> wrote:

Gonna add this in here as well as ticket #14512

I think using decorators to modify the way a CBV behaves is the wrong way to go about it, my reasoning is:

1) Decorators on functions make sense, since the only way to modify a function is to wrap it, so decorators provide a shortcut to do so.

2) There's already a standard way to add functionality to a class that has existed for a long time, is well tested, and as a bonus is something that people new to python but not new to programming will understand immediately.

I like the arguments here. On the other hand, I like Jacob's

«I'd like to convert all the view decorators built into Django to be "universal" -- so they'll work to decorate *any* view, whether a function, method, or class»

So what about providing something roughly like:

###
class LoginRequired():
    # magic to check for user credentials, in a CBV style
    ...
    def __call__(self, wrap_me):
        # standard magic to apply the CBV decoration to a standard function
        # This might also include Jacob's trick to work with methods too
        ...
 
login_required = LoginRequired # backwards compatibility
###

Actually that implementation of __call__ is probably the same for every decorator, so Django can provide something more like:

###
from django.something import BaseViewDecorator
class LoginRequired(BaseViewDecorator):
    # magic to check for user credentials, in a CBV style
    ...
 
login_required = LoginRequired # backwards compatibility
###

With that, user code can look like:

class SomeProtectedView(LoginRequired, TemplateView):
    ... as usual...

@login_required()
def some_other_protected_view(request, arg1, arg2):
    ....as usual...

class NotAView():
    @login_required()
    def view_method(self, request, arg1, arg2):
       ... as usual ...

Which uses the "obvious" way of extension (decorators for functions/methods and Mixins for classes) in each case (i.e., what Donald wants), but allows implementing the decorator once, and not having to have specific wrappers for each kind of view (universality, like Jacob wants).

This is a rough proposal to put another idea on the table, I know there's the gotcha of the "login_required = LoginRequired" (which is to keep backwards compatibility. Other alternative is rename the class to a non-PEP8 lowercase name). And I know there's the gotcha of having to use login_required() instead of just login_required[1].

Does this sound like a good compromise between both positions?

Regards,
   Daniel

[1] I always found a bit irritating and antipythonic to have @login_required as an alias for login_required() ; or in general, the behavior for decorators with optional args, which besides needs to be coded explicitly (DRY violation!) to behave like standard decorators. I know that changing that would break a lot of code, and it's a story for another ticket, but I couldn't miss the chance to complain :)

[Reinout van Rees]

On 15-09-11 23:27, Donald Stufft wrote:
> tl;dr; Using Mixins to add in functionality to a CBV makes way more
> sense then using a decorator which is essentially going to be doing the
> same thing as a mixing would, it just makes finding what is going on
> harder, makes customizing the decorator harder and/or uglier, and goes
> against the way functionality is currently added to a CBV.

Watch out, in general, with adding more and more mixins.

I explained just the most basic template CBV to a colleague: there are
just two mixins and a base class there, but his eyes already started to
glaze over because I had to jump from class to class to class to explain
how it all hung together.

Throw in a LoginRequiredMixin and SomeOtherMixin and you have the risk
that it turns into a big pile of un-debuggable code. Too many places to
look. Your mind's main working memory has a stack size between 6 and 8:
you just cannot juggle 3 base classes, 4 mixins and 2 method names at
the same time.

=> A mixin isn't necessarily clearer than a decorator.

Reinout

--
Reinout van Rees http://reinout.vanrees.org/
rei...@vanrees.org http://www.nelen-schuurmans.nl/
"If you're not sure what to do, make something. -- Paul Graham"

[Donald von Stufft]

In response to Reinout:

For the majority of people they won't have to care about what the LoginRequired Mixin is doing,

they'll just add it to their class definition and that will be the end of it, the same as if they were decorating it. The major

differences being now all the "things" that declare how this CBV behaves exist in one spot.If they don't need to customize

the behavior of it, they don't have to, it will just work. If they do need to customize then the option is there to do so without

having to copy/paste the code from Django into a utils.py and doing it themselves. 

Basically a decorator adds the same amount of complexity into the stack, you just can't get at it because it's essentially a black box,

so you can't tweak it.

In response to Carl Meyer:

Wrapping the view function like that is uglier (to me atleast) then just adding a mixin. There's no reason to do

it like that and if that's the only option then I would prefer a decorator. The goal is to make it simple, easy

and readable. To do so, In my opinion, we need to keep everything about a class, with the class, wrapping

it in the urls.py, or down further in the views.py just gives yet another place to look to find out why

a view is doing X.

In response to Mikhail:

Funcitions ARE less customizable then a nicely written class, tell me, with the current login_required

how would you make it return a 403 instead of a redirect to login? Or how would you make it check

that the user is_active as well as is_authorized? (The answer as far as I am aware is you would wrap

them again, adding another layer of indirection). There is 0 reason why you can't support both

@login_required and LoginRequired from the exact same code base, with just a little bit of a wrapper

(most of which already exists and is required for the non class case anyways).

In response to Łukasz:

I disagree that preconditions are not modifying the functionality of a CBV. it's changing what 

can or cannot be returned from my view, it's changing how my view behaves, and from

what I can tell, all of the current patches/implementations do it in a way that is basically

what sub classing would do, just more opaquely and more magically.

In regards to the declaring the decorators in a list:

I'm generally -1 on that. It doesn't solve

any of the problems that decorating has (well other then how to make them work with classes),

and it is different from "normal" python conventions.

--
You received this message because you are subscribed to the Google Groups "Django developers" group.

To post to this group, send email to django-developers@googlegroups.com.
To unsubscribe from this group, send email to django-developers+unsubscribe@googlegroups.com.

[Javier Guerra Giraldez]

On Fri, Sep 16, 2011 at 8:34 AM, Reinout van Rees <rei...@vanrees.org> wrote:
> Watch out, in general, with adding more and more mixins.
>
> I explained just the most basic template CBV to a colleague: there are just
> two mixins and a base class there, but his eyes already started to glaze
> over because I had to jump from class to class to class to explain how it
> all hung together.

that is my own reaction too when trying to see where in the code is
everything that i knew from the old generic view functions.

mixins are a great way to encapsulate specific, well defined behavior;
but tracing the code is a real chore, even so clean code as Django's.
I guess that when you know by heart exactly what does each one do,
they become a pleasure to use.

So, my two cents: please do the documentation first. once there's a
single place to find exactly what the purpose, mechanism and span of
each one, then adding more would be welcomed.

--
Javier

[Donald Stufft]

Documentation is being worked on, and is orthogonal to the current discussion of how

to handle things like requiring logins with the new CBVs.

[Roald de Vries]

On Sep 16, 2011, at 6:19 PM, Donald Stufft wrote:

> Documentation is being worked on, and is orthogonal to the current
> discussion of how
> to handle things like requiring logins with the new CBVs.

I just watched "Class Decorators: Radically Simple" by Jack Diederich,
who wrote the class decorators PEP, and I think it's very useful to
watch (25 min.) for this discussion. According to him it is good
practice to return the class that is decorated, which I think we
should follow, and which solves the biggest practical problems with
decorating. Moreover, the fact that class decorators exist indicate
that they are pythonic. So +1 for the decorators.

Considering the mixins: IMHO, the order of base classes shouldn't
matter. Can this be satisfied by the mixin-approach?

@Carl Meyer: I would opt for applying decorators *in* the class, so
you can still derive from it. Like::

class MyView(View):
@classonlymethod
def as_view(cls, *args, **kwargs):
return login_required(super(MyView, cls).as_view(*args,
**kwargs))

Cheers, Roald

[Donald Stufft]

Existing in python != pythonic. (not stating that class decorators aren't pythonic, but that argument is flawed)

Just watched the video my thoughts regarding it, and this discussion.

The Augment pattern is a terrible use case for class decorators when you are just overriding methods. It's nothing more then Mixins with a different syntax, for no reason that I can determine other then someone not liking the way declaring a mixin looks, functionally you are still mixing in. What benefit do you get from hiding the fact you are really just doing a Mixin? With the code Jacob posted there is… 16? (My math might be wrong) different permutations of code flow just to support universal decorators. 

The order would matter in the sense that any mixins would have to come before the base view class, so:

class ProtectedView(LoginRequired, View)

is valid but

class ProtectedView(View, LoginRequired)

is not.

But this is just python, there's nothing magical about object inheritance and in python object inheritance is left to right.

Additionally, as I just thought, there is a way to satisfy both camps. The support code I proposed to allow @login_required with Mixins to still work with function based views could include the universal decorator approach that Jacob included.

That would be a change so that the actual "is this a user, are they logged in etc" logic would live in a class that could be mixed in, then the support code for functions would use that class and wrap the test portions around the function view, and then the universal decorator code could just do the Augment pattern.

All camps == Happy? People who prefer mixins can mixin, people who prefer @decorators can decorate, everything is supported from one code base.

[James Bennett]

We have the following constraints:

1. Django supports class-based views.

2. Django supports function-based views (ultimately these are the same
thing, which is that Django supports anything as a 'view' so long as
it's callable, accepts an HttpRequest as its first positional argument
when being called and either returns an HttpResponse or raises an
exception).

3. The natural way to add processing in/around a class is subclassing
and either overriding or mixing in.

4. The natural way to add processing in/around around a function is decorating.

Any solution to this needs to address those constraints, and allow
code to look and feel natural.

Based on that, some arrangement which allows the same basic logic to
exist in a "mixin" (I dislike that word) for classes and a decorator
for functions seems most appropriate, even if it does mean having two
ways to do things -- that's a distinction I think we can live with, as
people will appreciate that it doesn't result in strange-looking code.

--
"Bureaucrat Conrad, you are technically correct -- the best kind of correct."

[Alex Gaynor]

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.

I agree with all your points, except #3, I think it's an over simplification.  There's another way to change a class: class decorators.  And there's ample precedent for their use in such a manner, unittest's skip decorators, our own decorators for swapping settings during testing, etc.

Alex

--
"I disapprove of what you say, but I will defend to the death your right to say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero

[Donald Stufft]

unittest.skip isn't a Mixin, it turns the class into an exception and raises it.

django.test.utils.override_settings is a mixin and it's terrible, it dynamically creates a new subclass, and overrides 2 methods. It's magic and more complex then need be.

[Alex Gaynor]

On Fri, Sep 16, 2011 at 10:27 PM, Donald Stufft <donald...@gmail.com> wrote:

unittest.skip isn't a Mixin, it turns the class into an exception and raises it.

It doesn't *turn* a class into anything, it simply returns a function, instead of a new class, and the function raises SkipTest, the point wasn't the implementation detail, but rather the syntax and pattern at work.

 

django.test.utils.override_settings is a mixin and it's terrible, it dynamically creates a new subclass, and overrides 2 methods. It's magic and more complex then need be.

a) it's not a mixin

b) yes, it creates subclasses, because the alternative is mutating the original class, which isn't how people generally expect decorators to work, we expect them to be syntactic sugar for:

A = wrap(*args)(A)

such that we can also do

B = wrap(*args)(A)

and have two classes (or functions).

c) I'm not sure how it's magic, but certainly if it's too complex we'd take patches to simplify the implementation.

[Justin Holmes]

James,

Your case seems to rest on what is "natural" - both in terms of how to
modify existing control structures and as a general goal that our
resulting syntax must "feel natural."

I submit that the way that will "feel natural" is to simply allow
decoration of any view, be it a class or a function, with the
decorator whose name describes precisely the functional modification.
This is simple to read, easy to remember, and, frankly, DRY.

--
Justin Holmes

Head Instructor, SlashRoot Collective
SlashRoot: Coffee House and Tech Dojo
60 Main Street
New Paltz, NY 12561
845.633.8330

[Donald Stufft]

The main reason I got into the implementation is because I think it's an important detail, decorating classes in my opinion is the right thing to do when what you are doing is something along the lines of registering the class, or doing some sort of validation and raising an exception etc.

django.test.utils.override_settings when decorating a class is nearly functionally equivalent to adding a mixin, it takes the base class, and adds in 2 methods. It's not actually a mixing, but I meant it's nearly equivalent to a mixin (in the TestCase case). I don't think class decorators are the right way to do this sort of pseudo mixin in a very opaque way. 

That being said…

Is there a problem with turning the current decorators into mixins, and support both decorating the class, and mixing in from the same code base? It's somewhat unpythonic in the 2 ways to do it camp, but it'd all be done with 1 code base, and would satisfy both camps I believe?

[Reinout van Rees]

On 16-09-11 18:08, Javier Guerra Giraldez wrote:
> I guess that when you know by heart exactly what does each one do,
> they become a pleasure to use.

They *are* a pleasure to use. A colleague switched an app over to class
based views last week and he got rid of 30% of the code. And it was more
elegant and readable.

Just mentioning that as I'm real happy with class based views :-)

[Roald de Vries]

Hi all,

Haven't seen a comment on this topic for a few days, so was wondering if a core dev can make a design decision yet. I have some spare time in the remainder of this week, so if the (universal) decorator-approach is chosen, I should be able to finish it shortly (the mixin-approach is a bit harder to estimate).

Cheers, Roald

I agree with all your points, except #3, I think it's an over simplification.  There's another way to change a class: class decorators.  And there's ample precedent for their use in such a manner, unittest's skip decorators, our own decorators for swapping settings during testing, etc.

Alex

Alex

[Donald Stufft]

I just got my wisdom teeth removed so i'm not exactly feeling the greatest, however I committed what I've done so far and pushed to my copy of django on github.

The goal is to provide a Mixin for class based views, a decorator for class based views, a decorator for methods, and a decorator for functions, all from the same codebase.

Currently I have the decorator for classes working, and the mixin working. I have the places where the other 2 will go but from a combination of not being very good at decorators, and not feeling well I haven't finished it yet.

Anyways here's what I've gotten so far: https://github.com/dstufft/django/compare/master...universal-mixin-decorators

[ptone]

On Sep 21, 8:44 am, Donald Stufft <donald.stu...@gmail.com> wrote:


>
> The goal is to provide a Mixin for class based views, a decorator for class based views, a decorator for methods, and a decorator for functions, all from the same codebase.
>
> Currently I have the decorator for classes working, and the mixin working. I have the places where the other 2 will go but from a combination of not being very good at decorators, and not feeling well I haven't finished it yet.

I like the overall direction of this approach (mixin class as the root
container of functionality) - but a number of technical issues still
need to have a resolution demonstrated (ie the decorating function
behavior)

One, perhaps pony, of a universal approach, would be to convert the
decorator kwargs into class attributes, so that they could be
overriden in subclasses of a decorated parent, something easy to do
with mixins as the starting point.

Also on a somewhat related note, a better option for middleware
process_view to interact with class based views

-Preston

[Donald Stufft]

Sorry for the delay,

Here's a possible method of handling the idea of adding the additional functionality of things like requiring logins to both FBV's and CBV's in a way that I feel is consistent with the methodologies of each of those types (decorators for FBV, Mixins for CBV).

This also keeps all the logic in one place. It allows for anyone (even people using FBV's) to easily extend the decorators and create their own versions of them, without having to rewrite the whole thing (which was one of the major reasons I like CBV's). It shouldn't break anything, the tests are passing and a functional test in a browser they all appear to be working.

https://github.com/dstufft/django/compare/master...mixin-decorators

Thoughts? Good Idea? Bad Idea?

[Andre Terra]

Definitely +1 on this idea. It seems to me like the cleanest solution yet proposed insofar as it provides the desired new functionality while requiring very little change for current code.

Cheers,

AT

[Marc Tamlyn]

I'm going to resurrect this old thread as I'd like to get it resolved in some fashion.

I used to be very in favour of the class decorators approach. I've been using an implementation of `@class_view_decorator(func)` for some time and it works pretty well. That said my implementation at least was subject to a notable flaw which is that if multiple parts of the hierarchy have the same decorator applied to them then the check gets done twice. Mixins are much cleverer than this because of MRO, so we avoid that problem.

Mixins however have their own issues - it's "harder" (for some definition) to ensure that all of your top-level permission checking happening in the correct order. That said, I am certainly veering towards implementing this using mixins (for each piece of functionality).

I'd really like to have a look at Donald's implementation, sadly it seems to no longer be on github. Do you still have then code somewhere, or can you explain the implementation idea?

Marc

On Thursday, 15 September 2011 22:44:39 UTC+2, Jacob Kaplan-Moss wrote:

Hi folks --

I'd like to convert all the view decorators built into Django to be
"universal" -- so they'll work to decorate *any* view, whether a

function, method, or class. I believe I've figured out a technique for
this, but I'd like some feedback on the approach before I dive in too
deep.

Right now view decorators (e.g. @login_required) only work on
functions. If you want to use a decorator on a method then you need to
"convert" the decorator using method_decorator(original_decorator).
You can't use view decorators on class-based views at all. This means
making a class-based view require login requires this awesomeness::

    class MyView(View):
        @method_decorator(login_required)
        def dispatch(self, *args, **kwargs):
            return super(MyView, self.dispatch(*args, **kwargs)

This makes me sad. It's really counter-intuitive and relies on a
recognizing that functions and methods are different to even know to
look for method_decorator.

#14512 proposes a adding another view-decorator-factory for decorating
class-based views, which would turn the above into::

    @class_view_decorator(login_required)
    class MyView(View):
        ...

This makes me less sad, but still sad. Factory functions. Ugh.

I want @login_required to work for anything::

    @login_required
    class MyView(View):
        ...

    class Something(object):
        @login_required
        def my_view(self, request):
            ...

    @login_required
    def my_view(request):
        ...

Now, back in the day we somewhat had this: decorators tried to work
with both functions and methods. The technique turned out not to work
(see #12804) and was removed in [12399]. See
http://groups.google.com/group/django-developers/browse_thread/thread/3024b14a47f5404d
for the discussion.

I believe, however, I've figured out a different technique to make
this work: don't try to detect bound versus unbound methods, but
instead look for the HttpRequest object. It'll either be args[0] if
the view's a function, or args[1] if the view's a method. This
technique won't work for any old decorator, but it *will* work (I
think) for any decorator *designed to be applied only to views*.

I've written a proof-of-concept patch to @login_required (well,
@user_passes_test, actually):

    https://gist.github.com/1220375

The test suite passes with this, with one exception:
https://code.djangoproject.com/browser/django/trunk/tests/regressiontests/decorators/tests.py#L87.
I maintain that this test is broken and should be using RequestFactory
instead.

Can I get some thoughts on this technique and some feedback on whether
it's OK to apply to every decorator built into Django?

Jacob

[Donald Stufft]

On May 18, 2013, at 8:57 AM, Marc Tamlyn <marc....@gmail.com> wrote:

I'm going to resurrect this old thread as I'd like to get it resolved in some fashion.

I used to be very in favour of the class decorators approach. I've been using an implementation of `@class_view_decorator(func)` for some time and it works pretty well. That said my implementation at least was subject to a notable flaw which is that if multiple parts of the hierarchy have the same decorator applied to them then the check gets done twice. Mixins are much cleverer than this because of MRO, so we avoid that problem.

Mixins however have their own issues - it's "harder" (for some definition) to ensure that all of your top-level permission checking happening in the correct order. That said, I am certainly veering towards implementing this using mixins (for each piece of functionality).

I'd really like to have a look at Donald's implementation, sadly it seems to no longer be on github. Do you still have then code somewhere, or can you explain the implementation idea?

I'm sure I have it somewhere, but knowing where is another thing all together ;)

If I recall I made a mixin superclass that added an ``as_decorator()`` class function which functioned similarly to the ``as_view()`` function. It took a common entry point and generated a non class function based on that and returned it.

As far as the actual mixins themselves I'd take a look at django-braces which has implementations of a lot of the decorators as mixins already they'd just need maybe some tweaking and then code added to allow turning them into actual decorators.

--
You received this message because you are subscribed to the Google Groups "Django developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To post to this group, send email to django-d...@googlegroups.com.

Visit this group at http://groups.google.com/group/django-developers?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

-----------------

Donald Stufft

PGP: 0x6E3CBCE93372DCFA // 7C6B 7C5D 5E2B 6356 A926 F04F 6E3C BCE9 3372 DCFA

[ptone]

On Saturday, May 18, 2013 5:57:49 AM UTC-7, Marc Tamlyn wrote:

I'm going to resurrect this old thread as I'd like to get it resolved in some fashion.

I used to be very in favour of the class decorators approach. I've been using an implementation of `@class_view_decorator(func)` for some time and it works pretty well. That said my implementation at least was subject to a notable flaw which is that if multiple parts of the hierarchy have the same decorator applied to them then the check gets done twice. Mixins are much cleverer than this because of MRO, so we avoid that problem.

Mixins however have their own issues - it's "harder" (for some definition) to ensure that all of your top-level permission checking happening in the correct order. That said, I am certainly veering towards implementing this using mixins (for each piece of functionality).

I'd really like to have a look at Donald's implementation, sadly it seems to no longer be on github. Do you still have then code somewhere, or can you explain the implementation idea?

Marc, ask and you shall receive.

I've always been convinced this is the way to go:

https://github.com/ptone/django/compare/mixin-decorators

-P

 

[Hanne Moa]

On 3 June 2013 18:09, <gavi...@gmail.com> wrote:

# `DecoratorMixin`

`DecoratorMixin` is a class factory that converts any view decorator into a class-based view mixin.

Using it is easy and intuitive:

    LoginRequiredMixin = DecoratorMixin(login_required)

Now this, this feels pythonic. Bravo!  I'm so doing this in future projects... (Though, class factories are spelled lowercase_with_underscores, no? At least in Django. For instance formset_factory().)

I've had a look at various LoginRequiredMixins out there (everybody makes their own it seems, including me). It is quite common that it doesn't cover all the things that login_required covers (like supporting the new way of doing users in 1.5) so in the case of  LoginRequiredMixin, I think the safest solution is to include a mixin in core anyway.

HM

[Donald Stufft]

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

Part of the reasoning of my original Change to make decorators classes was that it enabled much easier customization of them. Currently you basically either hope there was an option for doing what you wanted, or you copy/paste the entire thing and modify it.

Using a class lets you override it.