Re: decorator_from_middleware changes

[Tobias McNulty]

On Tue, Jun 21, 2016 at 7:00 PM, Carl Meyer  wrote:

> Still halfway tempted to go with the original "make middleware authors
> responsible for this themselves, if they want to access
> response.content" approach.
>

I am curious to hear what others think, but I'm leaning this way too.
Having a permanent helper (perhaps MiddewareMixin as you suggested) for
dealing with TemplateResponses certainly sounds like a win, too.

Tobias
-- 


*Tobias McNulty*Chief Executive Officer

tob...@caktusgroup.com
www.caktusgroup.com

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAMGFDKTOoMmA-VdRUMcvgbJJr_q_xWHNoH7Su%2BTiHivsW%2B%2BVxw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: decorator_from_middleware changes

[Carl Meyer]

Hi Shai,

Apologies, I didn't fully process your suggestion or the reason for the
flag the first time. I think it would work, though I'd be tempted to set
the flag to False on the transition middleware by default, to provide
perfect back-compat for existing uses of decorator_from_middleware until
someone opts in by setting the flag to True or writing their own
new-style middleware that doesn't inherit the mixin.

I'm still not entirely happy with this approach for the reason Tobias
highlighted, though - it doesn't behave like someone might expect a view
decorator to behave, and I think probably it should have an API that
clarifies that what it's really doing is applying per-view middleware.

Still halfway tempted to go with the original "make middleware authors
responsible for this themselves, if they want to access
response.content" approach.

Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/5769C70E.1010308%40oddbird.net.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature

Re: decorator_from_middleware changes

[Shai Berger]

Hi Carl,

On Tuesday 21 June 2016 19:32:15 Carl Meyer wrote:
> Hi Shai,
> 
> On 06/20/2016 11:07 PM, Shai Berger wrote:
> > Well, we can change decorator_from_middleware to introspect the
> > middleware itself and decide accordingly:
> > 
> > -- if it does not have process_request or process_response as callable
> > attributes, and it is callable itself, it is a new-style middleware
> 
> "Does not have process_request and process_response as callable
> attributes" isn't reliable -- there's nothing preventing a new-style
> middleware from having such methods and calling them from `__call__` 

That was an "if", not "if and only if". 

> (in fact any middleware converted using MiddlewareMixin will fit this
> description).
> 

And I suggested a way to deal with them in the next two paragraphs:

> > -- if it does, we can define a new attribute '_dep5' (or something) whose
> > presence will indicate a dep5-compatible middleware. This attribute can
> > be defined on the transition mixin, so that people who use it for
> > transition don't have to care
> > 
> > -- if it has callable process_request/process_response and doesn't have
> > _dep5 then it is an old-style middleware
> > 

...except that on second thought, rather than just looking for its presence, 
we could look for it to be truthy -- so a middleware could explicitly mark 
itself as "old style" for the decorator, by setting it to False.

> 
> We can indeed introspect to try to auto-detect. It's a bit more complex
> due to the different `__init__` signature as well, and it would fail in
> case someone defined __call__ on an old-style middleware for some
> reason, but I've already written the code for this in
> https://github.com/django/django/pull/6765
> 

I missed the difference in __init__, but if you already took care of it...

> The main reason I abandoned auto-detection for this second attempt is
> that the behavior of these new-style view decorators is different enough
> that I'd like to provide better flexibility for third-party projects.
> Imagine a third-party project that currently provides both a middleware
> and a view decorator, created by decorator_from_middleware. Now they are
> adding Django 1.10 compatibility. If we auto-detect, and they inherit
> their middleware from the transition mixin so it's cross-compatible,
> decorator_from_middleware will suddenly detect it as new-style, leaving
> them no way to continue to provide their old decorator with fully
> backwards-compatible behavior.
> 

So, for middlewares where it matters, inherit the mixin and specify the 
attribute as False.

> Basically, I think middleware_decorator does something that's
> fundamentally different enough from decorator_from_middleware that it is
> clearer to make it a separate function and let people make explicit
> choices about which one they are using.
> 

And yet the plan is to deprecate decorator_from_middleware -- that is, "rob" 
people of that choice later on. As long as we intend to do that, I think it 
better to minimize churn.

Shai.

Re: decorator_from_middleware changes

[Carl Meyer]

On 06/21/2016 10:53 AM, Carl Meyer wrote:
> If we're OK in the long term with making middleware-turned-decorator
> authors responsible for correctly handling TemplateResponse themselves,
> we should just go there right away via
> https://github.com/django/django/pull/6765 -- we can take care of
> backwards-compatibility quite nicely there in the transition mixin. In
> fact I'd say the overall backward-compatibility of this approach (#6765)
> is better than that of #6805 (the extra-middleware-annotation approach),
> due to converting all our builtin decorators-from-middleware to the new
> style in the latter.
> 
> The downside of #6765 is that the boilerplate for correctly handling
> TemplateResponse is pretty ugly [1], and any middleware that accesses
> response.content would pretty much need to do that, in case it might
> sometime be used in decorator_from_middleware.
> 
> To be honest though, I'm still not sure that isn't our best option. Most
> middleware I've written don't access response.content, they set headers,
> so they'd be unaffected. Some of Django's built-in ones do touch
> content, but we can deal with the ugly TemplateResponse boilerplate in
> our own middleware.

We could also leave MiddlewareMixin in place indefinitely, and people
who don't want to do advanced things in __call__ (like using context
managers) can keep inheriting from the mixin and writing process_request
and process_response methods until eternity. Then the mixin takes care
of TemplateResponse deferral and they don't need to worry about it.

Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/57697232.7010501%40oddbird.net.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature

Re: decorator_from_middleware changes

[Carl Meyer]

Hi Tobias,

On 06/21/2016 06:22 AM, Tobias McNulty wrote:
> On Mon, Jun 20, 2016 at 10:51 AM, Carl Meyer  > wrote
>> Possible disadvantages of this approach:
> 
>  
> 
>> 3. The new behavior may surprise some people accustomed to the old
>> behavior, since it means the effect of the middleware decorator won't
>> occur immediately where the decorator is applied. E.g. if you apply a
>> middleware decorator to a view, and then another custom view decorator
>> outside that, the custom view decorator won't see the effects of the
>> middleware decorator in the actual response (it would only see a new
>> entry in view_func._extra_middleware).
> 
> This one seems like the gotcha to me.

Yeah, I agree.

> What if, for example, I have a
> view decorator whose effects I specifically don't want to cache, but I
> do want to cache the underlying view; is it fair to require that the
> person write a middleware and then turn it into a decorator to be able
> to do that?

Caching happens to be a bad example, because AFAICT nobody should ever
use the cache_page decorator (or cache full responses right after the
view function, as you describe here) due to
https://code.djangoproject.com/ticket/15855 -- unless I suppose they are
pre-adding all the correct Vary headers themselves.

But I think the general point is a good one nonetheless.

> Are we effectively saying, "for view decorators to behave
> the way you might expect, implement them as middleware"? It seems odd,
> to me at least, that I should care what the underlying implementation of
> a decorator is before I use it. It also violates the 'strict in/out
> layering' goal, albeit for decorators instead of middleware. (A similar
> argument could be said of exceptions -- what if I want to trap an
> exception raised by a middleware-turned-decorator?)
> 
> It might be okay if the decorators themselves were explicit about what
> they were doing, for example @cache_page(3600) might become:
> 
> @add_middleware(CacheMiddleware, cache_timeout=3600)

Yes, I did think about this. It makes it more difficult to use, but it
does give a clearer picture of what's actually happening. We aren't
directly decorating the view function, we're just annotating it with an
extra per-view middleware.

> However, that's probably a bigger and unnecessary change.
> 
> Would it be possible to implement a combination of the approaches, i.e.,
> make the delay in applying the middleware-turned-decorator the exception
> rather than the rule, perhaps only for TemplateResponses and
> specifically for the purpose of supporting a deprecation plan? And then,
> long-term, leave it up to middleware/decorator authors & users to decide
> how best to implement/layer them, being explicit about the implications
> of rendering the response or perhaps more appropriately, "not rendering
> if you can avoid it" (i.e., your first strategy)?

I don't think it's worth doing this just for a deprecation path. If
we're OK in the long term with making middleware-turned-decorator
authors responsible for correctly handling TemplateResponse themselves,
we should just go there right away via
https://github.com/django/django/pull/6765 -- we can take care of
backwards-compatibility quite nicely there in the transition mixin. In
fact I'd say the overall backward-compatibility of this approach (#6765)
is better than that of #6805 (the extra-middleware-annotation approach),
due to converting all our builtin decorators-from-middleware to the new
style in the latter.

The downside of #6765 is that the boilerplate for correctly handling
TemplateResponse is pretty ugly [1], and any middleware that accesses
response.content would pretty much need to do that, in case it might
sometime be used in decorator_from_middleware.

To be honest though, I'm still not sure that isn't our best option. Most
middleware I've written don't access response.content, they set headers,
so they'd be unaffected. Some of Django's built-in ones do touch
content, but we can deal with the ugly TemplateResponse boilerplate in
our own middleware.

Carl


 [1]
https://github.com/carljm/django/blob/7d999844bd4b43b82a472634074156c24a4fc7cb/docs/topics/http/middleware.txt#L304

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/576970EC.50704%40oddbird.net.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature

Re: decorator_from_middleware changes

[Carl Meyer]

Hi Shai,

On 06/20/2016 11:07 PM, Shai Berger wrote:
> Well, we can change decorator_from_middleware to introspect the middleware 
> itself and decide accordingly:
> 
> -- if it does not have process_request or process_response as callable 
> attributes, and it is callable itself, it is a new-style middleware 

"Does not have process_request and process_response as callable
attributes" isn't reliable -- there's nothing preventing a new-style
middleware from having such methods and calling them from `__call__` (in
fact any middleware converted using MiddlewareMixin will fit this
description).

> -- if it does, we can define a new attribute '_dep5' (or something) whose 
> presence will indicate a dep5-compatible middleware. This attribute can be 
> defined on the transition mixin, so that people who use it for transition 
> don't 
> have to care
> 
> -- if it has callable process_request/process_response and doesn't have _dep5 
> then it is an old-style middleware
> 
> Note that decorator_from_middleware already used to instantiate the 
> middleware 
> class anyway, so the check can be applied to a middleware object, not to a 
> middleware factory.
> 
> The whole introspection behavior, including the _dep5 attribute and the code 
> that checks for it, can be dropped when old-style middleware is finally 
> dropped. If someone uses _dep5 manually, it will do them no harm later.

We can indeed introspect to try to auto-detect. It's a bit more complex
due to the different `__init__` signature as well, and it would fail in
case someone defined __call__ on an old-style middleware for some
reason, but I've already written the code for this in
https://github.com/django/django/pull/6765

The main reason I abandoned auto-detection for this second attempt is
that the behavior of these new-style view decorators is different enough
that I'd like to provide better flexibility for third-party projects.
Imagine a third-party project that currently provides both a middleware
and a view decorator, created by decorator_from_middleware. Now they are
adding Django 1.10 compatibility. If we auto-detect, and they inherit
their middleware from the transition mixin so it's cross-compatible,
decorator_from_middleware will suddenly detect it as new-style, leaving
them no way to continue to provide their old decorator with fully
backwards-compatible behavior.

Basically, I think middleware_decorator does something that's
fundamentally different enough from decorator_from_middleware that it is
clearer to make it a separate function and let people make explicit
choices about which one they are using.

Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/57696C0F.5050804%40oddbird.net.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature

Re: decorator_from_middleware changes

[Tobias McNulty]

On Mon, Jun 20, 2016 at 10:51 AM, Carl Meyer  wrote
> Possible disadvantages of this approach:



> 3. The new behavior may surprise some people accustomed to the old
> behavior, since it means the effect of the middleware decorator won't
> occur immediately where the decorator is applied. E.g. if you apply a
> middleware decorator to a view, and then another custom view decorator
> outside that, the custom view decorator won't see the effects of the
> middleware decorator in the actual response (it would only see a new
> entry in view_func._extra_middleware).

This one seems like the gotcha to me. What if, for example, I have a view
decorator whose effects I specifically don't want to cache, but I do want
to cache the underlying view; is it fair to require that the person write a
middleware and then turn it into a decorator to be able to do that? Are we
effectively saying, "for view decorators to behave the way you might
expect, implement them as middleware"? It seems odd, to me at least, that I
should care what the underlying implementation of a decorator is before I
use it. It also violates the 'strict in/out layering' goal, albeit for
decorators instead of middleware. (A similar argument could be said of
exceptions -- what if I want to trap an exception raised by a
middleware-turned-decorator?)

It might be okay if the decorators themselves were explicit about what they
were doing, for example @cache_page(3600) might become:

@add_middleware(CacheMiddleware, cache_timeout=3600)

However, that's probably a bigger and unnecessary change.

Would it be possible to implement a combination of the approaches, i.e.,
make the delay in applying the middleware-turned-decorator the exception
rather than the rule, perhaps only for TemplateResponses and specifically
for the purpose of supporting a deprecation plan? And then, long-term,
leave it up to middleware/decorator authors & users to decide how best to
implement/layer them, being explicit about the implications of rendering
the response or perhaps more appropriately, "not rendering if you can avoid
it" (i.e., your first strategy)?

Tobias
-- 

Tobias McNulty
Chief Executive Officer

tob...@caktusgroup.com
www.caktusgroup.com

Sent from my mobile.

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CAMGFDKT7CuRLaS2Z%2BKA-_pSzBOXXGBhj_J1yD9ORe8stwX%3DBGQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Re: decorator_from_middleware changes

[Shai Berger]

Well, we can change decorator_from_middleware to introspect the middleware 
itself and decide accordingly:

-- if it does not have process_request or process_response as callable 
attributes, and it is callable itself, it is a new-style middleware 

-- if it does, we can define a new attribute '_dep5' (or something) whose 
presence will indicate a dep5-compatible middleware. This attribute can be 
defined on the transition mixin, so that people who use it for transition don't 
have to care

-- if it has callable process_request/process_response and doesn't have _dep5 
then it is an old-style middleware

Note that decorator_from_middleware already used to instantiate the middleware 
class anyway, so the check can be applied to a middleware object, not to a 
middleware factory.

The whole introspection behavior, including the _dep5 attribute and the code 
that checks for it, can be dropped when old-style middleware is finally 
dropped. If someone uses _dep5 manually, it will do them no harm later.

What am I missing?

Shai.

Re: decorator_from_middleware changes

[Carl Meyer]

On 06/20/2016 06:57 PM, Tim Graham wrote:
> I may have seen this idea in IRC but instead of a new
> middleware_decorator function did you consider varying
> decorator_from_middleware's behavior based on "if settings.MIDDLEWARE is
> None" similar to what BaseHandler does? We could add a check in
> decorator_from_middleware to raise an error if MIDDLEWARE is None and
> the middleware doesn't seem to be the new-style.

I did think about that, but I think having the output of a function like
this (which is usually called at import time) be non-deterministic
depending on settings is problematic on several levels.

At the most basic level, we usually try to avoid import-time settings
access, to avoid an import causing a "DJANGO_SETTINGS_MODULE is not
defined" error.

But it also seems wrong for a decorator-generator to generate a
completely different decorator based on settings. It's not uncommon that
a third-party library might provide both a middleware and a decorator
generated via decorator_from_middleware. Should the nature and behavior
of that third-party decorator change fundamentally depending on whether
it happens to be imported in a project that has (or hasn't) converted
from MIDDLEWARE_CLASSES to MIDDLEWARE? What about a third-party
old-style decorator from on old-style middleware, imported in a project
using MIDDLEWARE? There's no reason that decorator can't work the same
as it always has (the decorator itself doesn't have any dependency on
which type of middleware your project uses), but under this proposal it
would instead suddenly raise an error if you even try to import it.

I think this approach is more problematic and less backwards-compatible
(especially for third-party projects) than just introducing a new
function and deprecating the old one.

Carl

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/57689D2E.3060600%40oddbird.net.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: OpenPGP digital signature

Re: decorator_from_middleware changes

[Tim Graham]

I may have seen this idea in IRC but instead of a new middleware_decorator 
function did you consider varying decorator_from_middleware's behavior 
based on "if settings.MIDDLEWARE is None" similar to what BaseHandler does? 
We could add a check in decorator_from_middleware to raise an error if 
MIDDLEWARE is None and the middleware doesn't seem to be the new-style.

On Monday, June 20, 2016 at 10:52:06 AM UTC-4, Carl Meyer wrote:
>
> Hi all, 
>
> In porting decorator_from_middleware to support DEP 5 middleware, I ran 
> into one stumbling block: TemplateResponse. Normally with old-style 
> middleware, process_response runs after TemplateResponses have been 
> rendered. But a view decorator runs immediately wrapping the view, when 
> TemplateResponses will not have been rendered yet. So 
> decorator_from_middleware has special support for TemplateResponse -- if 
> it detects one, rather than calling process_response immediately, it 
> sets it as an on_render_callback. 
>
> But in DEP 5 middleware, request and response processing are not 
> separable -- they are part of the same function. This is the source of 
> the benefits of DEP 5: strict in/out layering, the ability to use 
> context managers in middleware, etc. But it also means that there is no 
> way to delay just the response-processing portion of the middleware 
> until after render. 
>
> My initial strategy [1] was to simply make middleware authors 
> responsible for this: a middleware that needed to access 
> response.content and wanted to be compatible with 
> decorator_from_middleware would be responsible to detect and handle 
> unrendered TemplateResponse appropriately. 
>
> In discussion in IRC, Florian Apolloner suggested an alternative: change 
> the implementation of decorator_from_middleware so that instead of 
> immediately applying all middleware methods as an actual view decorator, 
> make the decorator simply annotate the view callable with an 
> `_extra_middleware` list, and then add support for this annotation to 
> the base handler (effectively making it support per-view middleware). 
> The handler runs per-view middleware as if it were listed last in 
> MIDDLEWARE (that is, "closest" to the view). I've implemented this 
> suggestion in [2]. It has several advantages: 
>
> 1. It consolidates middleware-handling logic in one place, the handler. 
> Currently this logic is effectively duplicated in the handler and in 
> decorator_from_middleware, and I've noticed several bugs in the current 
> decorator_from_middleware resulting from inconsistencies between the 
> duplicate implementations (e.g. if process_view or process_exception 
> returns a response, normally that response passes through 
> process_response, but in decorator_from_middleware it does not.) 
> Consolidation of this logic will improve consistency of middleware 
> behavior when listed in settings vs when applied via decorator. 
>
> 2. It allows us to apply middleware-decorators in closer to the same way 
> that normal middleware is applied, e.g. response-handling can happen 
> after render() is called on TemplateResponses, solving the initial 
> problem. 
>
> Possible disadvantages of this approach: 
>
> 1. In order to avoid immediate breakage of decorator_from_middleware 
> when used with third-party middleware that don't yet support DEP 5, I've 
> implemented the new approach as a new function, `middleware_decorator`, 
> that will replace and deprecate `decorator_from_middleware`. This is 
> unfortunate churn, but I don't see another good way to be 
> backwards-compatible. 
>
> 2. The decorators internal to Django that rely on 
> decorator_from_middleware (the csrf decorators, cache_page) will be 
> immediately converted to middleware_decorator, resulting in subtle 
> behavior differences from the current ones. 
>
> 3. The new behavior may surprise some people accustomed to the old 
> behavior, since it means the effect of the middleware decorator won't 
> occur immediately where the decorator is applied. E.g. if you apply a 
> middleware decorator to a view, and then another custom view decorator 
> outside that, the custom view decorator won't see the effects of the 
> middleware decorator in the actual response (it would only see a new 
> entry in view_func._extra_middleware). 
>
> Thoughts welcome. 
>
> Carl 
>
>  [1] https://github.com/django/django/pull/6765 
>  [2] https://github.com/django/django/pull/6805 
>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers  (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit