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...
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/regressiont....
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

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.

On 15 September 2011 22:44, Jacob Kaplan-Moss <ja...@jacobian.org> wrote:

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 :)

--
Łukasz Rekucki

--
Łukasz Rekucki

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...

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

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?

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)

Cheers, Roald

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

--
Łukasz Rekucki

--
Łukasz Rekucki

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.

Hi Łukasz,

On 09/16/2011 03:42 AM, Łukasz Rekucki wrote:

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-----

     >>> 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

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-----

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

«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 :)

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/
rein...@vanrees.org             http://www.nelen-schuurmans.nl/
"If you're not sure what to do, make something. -- Paul Graham"

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.

On Fri, Sep 16, 2011 at 9:34 AM, Reinout van Rees <rein...@vanrees.org>wrote:

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

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

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

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.  

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."