Re: #12804 - decorating admin views marked as invalid

2010-02-09 Thread Russell Keith-Magee 
On Tue, Feb 9, 2010 at 10:51 PM, Luke Plant  wrote:
> On Tuesday 09 February 2010 02:40:03 Russell Keith-Magee wrote:
>
>> >> What do people think?
>> >
>> > Ugh. Assuming no one can come up with a brilliant fix so that the
>> > auto_adapt stuff can work when combined with arbitrary other
>> > decorators (I can't), I favor switching to a system that requires
>> > different decorators for methods vs. functions and documenting
>> > the backwards-incompatibility for 1.2. What you propose there
>> > reads pretty well to me.
>>
>> I concur. Ugh. :-)
>>
>> It's not nice but given the alternatives, I think the backwards
>> incompatibility is the best option. +1 to introducing method_dec
>> (bikeshedding - I actually prefer method_decorator), removing
>> @auto_adapt_to_method, and documenting the problem with
>> user_passes_test.
>
> OK, good, that's what I was thinking (complete with 'ugh' reaction!).
>
> Now, should we backport to 1.1.X or not?  We could justify *not*
> backporting on the grounds that this is a fix for #12804, which does
> not exist in 1.1.X.  Given that no-one filed a bug about this for the
> lifetime of 1.0 and 1.1, it's probably an obscure enough corner case
> in the context of 1.1.X that backporting this fix will cause more harm
> than good.  And if someone does have a problem on 1.1.X, we can
> provide a work-around (which is to use method_decorator).
>
> Unless anyone shouts, I'll just commit to trunk.

I'm happy to call this a "fix for @auto_adapt_to_method in 1.2",
rather than a fix for the problem in 1.1.

Russ %-)

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

Re: #12804 - decorating admin views marked as invalid

2010-02-09 Thread Luke Plant 
On Tuesday 09 February 2010 02:40:03 Russell Keith-Magee wrote:

> >> What do people think?
> >
> > Ugh. Assuming no one can come up with a brilliant fix so that the
> > auto_adapt stuff can work when combined with arbitrary other
> > decorators (I can't), I favor switching to a system that requires
> > different decorators for methods vs. functions and documenting
> > the backwards-incompatibility for 1.2. What you propose there
> > reads pretty well to me.
> 
> I concur. Ugh. :-)
> 
> It's not nice but given the alternatives, I think the backwards
> incompatibility is the best option. +1 to introducing method_dec
> (bikeshedding - I actually prefer method_decorator), removing
> @auto_adapt_to_method, and documenting the problem with
> user_passes_test.

OK, good, that's what I was thinking (complete with 'ugh' reaction!). 

Now, should we backport to 1.1.X or not?  We could justify *not* 
backporting on the grounds that this is a fix for #12804, which does 
not exist in 1.1.X.  Given that no-one filed a bug about this for the 
lifetime of 1.0 and 1.1, it's probably an obscure enough corner case 
in the context of 1.1.X that backporting this fix will cause more harm 
than good.  And if someone does have a problem on 1.1.X, we can 
provide a work-around (which is to use method_decorator).

Unless anyone shouts, I'll just commit to trunk.

Luke

-- 
"You'll be glad to know, I'm going to donate all the snot I sneeze 
to hospitals for mucus transfusions." (Calvin and Hobbes)

Luke Plant || http://lukeplant.me.uk/

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

Re: #12804 - decorating admin views marked as invalid

2010-02-09 Thread Florian Apolloner 
On Feb 8, 2:44 pm, Luke Plant  wrote:
> What do people think?
+1 for everything which removes the magic from auto_adapt_to_methods.
Can't say much about the backwards incompatiblity issue, cause I don't
care ;) Of course it would be nice, but given the options I would just
document it and live with it…

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

Re: #12804 - decorating admin views marked as invalid

2010-02-08 Thread Russell Keith-Magee 
On Tue, Feb 9, 2010 at 7:19 AM, Karen Tracey  wrote:
> On Mon, Feb 8, 2010 at 8:44 AM, Luke Plant  wrote:
>>
>> What do people think?
>
> Ugh. Assuming no one can come up with a brilliant fix so that the auto_adapt
> stuff can work when combined with arbitrary other decorators (I can't), I
> favor switching to a system that requires different decorators for methods
> vs. functions and documenting the backwards-incompatibility for 1.2. What
> you propose there reads pretty well to me.

I concur. Ugh. :-)

It's not nice but given the alternatives, I think the backwards
incompatibility is the best option. +1 to introducing method_dec
(bikeshedding - I actually prefer method_decorator), removing
@auto_adapt_to_method, and documenting the problem with
user_passes_test.

Russ %-)

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

Re: #12804 - decorating admin views marked as invalid

2010-02-08 Thread Karen Tracey 
On Mon, Feb 8, 2010 at 8:44 AM, Luke Plant  wrote:

> What do people think?
>

Ugh. Assuming no one can come up with a brilliant fix so that the auto_adapt
stuff can work when combined with arbitrary other decorators (I can't), I
favor switching to a system that requires different decorators for methods
vs. functions and documenting the backwards-incompatibility for 1.2. What
you propose there reads pretty well to me.

Karen

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

Re: #12804 - decorating admin views marked as invalid

2010-02-08 Thread Luke Plant 
On Monday 08 February 2010 08:21:27 Florian Apolloner wrote:

> thx for your quick response, I'll update the ticket and reopen it
>  (I hope that's okay in this case…)

That's fine, apologies for the premature closing.  Here is my 
analysis, sorry in advance for the length.  Unfortunately, we have 
backwards incompatibilities which ever way we go, as far as I can see.

== flaw in @auto_adapt_to_methods ==

@auto_adapt_to_methods is flawed.  It is supposed to allow the same 
decorator to work with functions:

  @does_something_with_arg
  def foo(arg):
 # ...

...and methods:

  class Foo(object):
  @does_something_with_arg
  def meth(self, arg):
  # pass

Effectively it tries to cope with two different function signatures.  
However, when a function is defined, you have no idea whether you are 
'inside' a class or not, so the decision is delegated to runtime.

This is done by wrapping functions in a MethodDecoratorAdaptor object, 
which:

1) implements __call__, to handle the case of a simple function

2) implements __get__, to handle the case of a method.  This works by 
delaying the application of the decorator until the method is 
retrieved, at which point we have the 'self' parameter (passed in to 
__get__) and can create a 'bound' callable which does not have the 
self parameter in the function signature, and therefore matches the 
expected function signature.

The second of these fails if you use an additional decorator that is 
not implemented using @auto_adapt_to_methods.  Such a decorator will 
treat the MethodDecoratorAdaptor object as a simple callable, and the 
descriptor magic never kicks in.

The simplest example is as follows:

   class Foo(object):
   @my_decorator
   @does_something_with_arg
   def meth(self, arg):
   pass

In this case, we might be able to fix @my_decorator simply by 
decorating it with @auto_adapt_to_methods.  But that isn't always 
possible - in Florian's case, his my_decorator needs access to 'self'.

Example code for all of this is attached in 
'auto_adapt_to_methods.py'.  I don't think this approach is fixable, 
but I'd be happy to be proved wrong!

This problem applies to everything we use @auto_adapt_to_methods on, 
which includes csrf_protect, login_required, user_passes_test, 
never_cache and anything created using decorator_from_middleware.

== a fix ==

Instead of trying an automagic 'adapt to methods', we can have a 
manual 'adapt to methods' decorator, called 'method_dec' (better names 
welcome, 'method_decorator' seemed a bit long).  So we would have:

  @csrf_protect
  def my_view(request):
  # 

  class MyClass(object):
  @method_dec(csrf_protect)
  def my_view(self, request):
  # ...

or

  csrf_protect_m = method_dec(csrf_protect)

  class MyClass(object):
  @csrf_protect_m
  def my_view(self, request):
  # ...

I've implemented this in the attached 'method_dec.py'.  It doesn't 
rely on the descriptor protocol, so it should be robust.

== the problem ==

Unfortunately, we have problems and backwards incompatibilities which 
ever way we go.  Although @auto_adapt_to_methods and 
MethodDecoratorAdaptor were introduced after 1.1 (so we are allowed to 
remove them), they replace the _CheckLogin class, which served the 
same purpose, but was specific to the user_passes_test decorator.  It 
used the same methodology (__call__ and __get__ implementation), and 
so it will suffer from exactly the same flaw.

So, if we do nothing, we have the general bug described above, and 
Florian's bug.  In his case, it appears as a backwards 
incompatibility, as he can no longer do this:

  class MyModelAdmin(ModelAdmin):

  change_view = my_decorator(ModelAdmin.change_view)

This is because ModelAdmin.change_view is now decorated with 
csrf_protect, which has the flawed 'auto adapt' behaviour.

If we replace the automatic auto_adapt_to_methods with the manual 
method_dec, then we face:

1) backwards incompatibilities for those following trunk, which is not 
ideal but allowed.

2) backwards incompatibilities for anyone who has used login_required 
or user_passes_test on *methods* since 1.0.  The release notes would 
have something like this:

<<<
user_passes_test and login_required
---
Previously, it was possible to use 'login_required' (and other 
decorators generated using 'user_passes_test') both on functions 
(where the first argument was 'request') and on methods (where the 
first argument was 'self', and the second argument was 'request').  
However, we have found that the trick which enabled this is flawed. It 
only works in limited circumstances, and produces errors that are very 
difficult to debug when it does not work.

For this reason, the 'auto adapt' behaviour has been removed, and if 
you are using these decorators on methods, you will need to manually 
apply `django.utils.decorators.method_dec` to convert the decorator to 
one that works with

Re: #12804 - decorating admin views marked as invalid

2010-02-08 Thread Florian Apolloner 
Hi,

thx for your quick response, I'll update the ticket and reopen it (I
hope that's okay in this case…)

Cheers, Florian

On Feb 8, 3:05 am, Luke Plant  wrote:
> Ignore my last post - my 'fix' only fixes one corner case, and not a
> very useful one either.  I've got it fairly sorted out now, I'll post
> tomorrow about this.
>
> Luke
>
> On Sunday 07 February 2010 21:11:42 Luke Plant wrote:
>
>
>
> > On Sunday 07 February 2010 10:45:29 Florian Apolloner wrote:
> > > Hi,
>
> > > first of all, I agree with Luke that it's hard to fix this if
> > > it's possible at all [1]. The only real problem I have with this
> > > ticket beeing closed is the backwards compatibility issue.
> > > Decorating admin views worked just fine in 1.1 and r11660 broke
> > > it (At least I hope it worked in 1.1, but as the admin view
> > > wasn't decorated at all there, it should have worked). Imho we
> > > should at least consider documenting it, and maybe mention it in
> > > the release notes, as custom admin code might break by an upgrade
> > > from 1.1 to 1.2.
>
> > I've thought some more about it, and I think you are right.
> > Descriptors hurt my head, and descriptors plus decorators and
> > decorator decorators (like auto_adapt_to_methods) are particularly
> > bad!
>
> > Python 3.0 is actually more helpful here, as it does away with
> >  unbound methods - they are just functions. That helps to clarify
> >  that your pattern (as on the ticket) is not actually strange, and
> >  ought to work - SomeClass.method is just a function, and it is the
> >  function as originally defined, so it ought to work as an
> >  assignment in that context.
>
> > Anyway, could you try the attached patch?  My experiments suggest
> >  it should fix the issue, but coming up with it hurt my brain too
> >  much for me to do any more!
>
> > Luke
>
> --
> "Yes, wearily I sit here, pain and misery my only companions. And
> vast intelligence of course. And infinite sorrow. And..." (Marvin
> the paranoid android)
>
> Luke Plant ||http://lukeplant.me.uk/

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

Re: #12804 - decorating admin views marked as invalid

2010-02-07 Thread Luke Plant 
Ignore my last post - my 'fix' only fixes one corner case, and not a 
very useful one either.  I've got it fairly sorted out now, I'll post 
tomorrow about this.

Luke

On Sunday 07 February 2010 21:11:42 Luke Plant wrote:
> On Sunday 07 February 2010 10:45:29 Florian Apolloner wrote:
> > Hi,
> >
> > first of all, I agree with Luke that it's hard to fix this if
> > it's possible at all [1]. The only real problem I have with this
> > ticket beeing closed is the backwards compatibility issue.
> > Decorating admin views worked just fine in 1.1 and r11660 broke
> > it (At least I hope it worked in 1.1, but as the admin view
> > wasn't decorated at all there, it should have worked). Imho we
> > should at least consider documenting it, and maybe mention it in
> > the release notes, as custom admin code might break by an upgrade
> > from 1.1 to 1.2.
> 
> I've thought some more about it, and I think you are right.
> Descriptors hurt my head, and descriptors plus decorators and
> decorator decorators (like auto_adapt_to_methods) are particularly
> bad!
> 
> Python 3.0 is actually more helpful here, as it does away with
>  unbound methods - they are just functions. That helps to clarify
>  that your pattern (as on the ticket) is not actually strange, and
>  ought to work - SomeClass.method is just a function, and it is the
>  function as originally defined, so it ought to work as an
>  assignment in that context.
> 
> Anyway, could you try the attached patch?  My experiments suggest
>  it should fix the issue, but coming up with it hurt my brain too
>  much for me to do any more!
> 
> Luke
> 

-- 
"Yes, wearily I sit here, pain and misery my only companions. And 
vast intelligence of course. And infinite sorrow. And..." (Marvin 
the paranoid android)

Luke Plant || http://lukeplant.me.uk/

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

Re: #12804 - decorating admin views marked as invalid

2010-02-07 Thread Luke Plant 
On Sunday 07 February 2010 10:45:29 Florian Apolloner wrote:
> Hi,
> 
> first of all, I agree with Luke that it's hard to fix this if it's
> possible at all [1]. The only real problem I have with this ticket
> beeing closed is the backwards compatibility issue. Decorating
>  admin views worked just fine in 1.1 and r11660 broke it (At least
>  I hope it worked in 1.1, but as the admin view wasn't decorated at
>  all there, it should have worked). Imho we should at least
>  consider documenting it, and maybe mention it in the release
>  notes, as custom admin code might break by an upgrade from 1.1 to
>  1.2.

I've thought some more about it, and I think you are right.  
Descriptors hurt my head, and descriptors plus decorators and 
decorator decorators (like auto_adapt_to_methods) are particularly 
bad!

Python 3.0 is actually more helpful here, as it does away with unbound 
methods - they are just functions. That helps to clarify that your 
pattern (as on the ticket) is not actually strange, and ought to work 
- SomeClass.method is just a function, and it is the function as 
originally defined, so it ought to work as an assignment in that 
context.

Anyway, could you try the attached patch?  My experiments suggest it 
should fix the issue, but coming up with it hurt my brain too much for 
me to do any more!

Luke

-- 
"Yes, wearily I sit here, pain and misery my only companions. And 
vast intelligence of course. And infinite sorrow. And..." (Marvin 
the paranoid android)

Luke Plant || http://lukeplant.me.uk/

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

Index: django/utils/decorators.py
===
--- django/utils/decorators.py	(revision 12380)
+++ django/utils/decorators.py	(working copy)
@@ -35,6 +35,8 @@
 def __call__(self, *args, **kwargs):
 return self.decorator(self.func)(*args, **kwargs)
 def __get__(self, instance, owner):
+if instance is None:
+return self
 return self.decorator(self.func.__get__(instance, owner))
 
 def auto_adapt_to_methods(decorator):