decorator_from_middleware changes
207 views
Skip to first unread message
Carl Meyer
Jun 20, 2016, 10:52:06 AM6/20/16
to django-d...@googlegroups.com
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
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
Tim Graham
Jun 20, 2016, 8:57:18 PM6/20/16
to Django developers (Contributions to Django itself)
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.
Carl Meyer
Jun 20, 2016, 9:49:55 PM6/20/16
to django-d...@googlegroups.com
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
> 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.
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
Shai Berger
Jun 21, 2016, 1:08:08 AM6/21/16
to django-d...@googlegroups.com
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.
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.
Tobias McNulty
Jun 21, 2016, 8:22:28 AM6/21/16
to django-developers
On Mon, Jun 20, 2016 at 10:51 AM, Carl Meyer <ca...@oddbird.net> wrote
> Possible disadvantages of this approach:
<snip>
> 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.
Carl Meyer
Jun 21, 2016, 12:32:34 PM6/21/16
to django-d...@googlegroups.com
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
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
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.
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
Carl Meyer
Jun 21, 2016, 12:53:17 PM6/21/16
to django-d...@googlegroups.com
Hi Tobias,
On 06/21/2016 06:22 AM, Tobias McNulty wrote:
> On Mon, Jun 20, 2016 at 10:51 AM, Carl Meyer <ca...@oddbird.net
> <mailto:ca...@oddbird.net>> wrote
> 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
On 06/21/2016 06:22 AM, Tobias McNulty wrote:
> On Mon, Jun 20, 2016 at 10:51 AM, Carl Meyer <ca...@oddbird.net
>> Possible disadvantages of this approach:
>
> <snip>
>
>> 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.
>
> <snip>
>
>> 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?
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)
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)?
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
Carl Meyer
Jun 21, 2016, 12:58:43 PM6/21/16
to django-d...@googlegroups.com
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
> 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.
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
Shai Berger
Jun 21, 2016, 4:39:06 PM6/21/16
to django-d...@googlegroups.com
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.
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__`
> (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
> >
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
>
> 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.
>
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.
>
people of that choice later on. As long as we intend to do that, I think it
better to minimize churn.
Shai.
Carl Meyer
Jun 21, 2016, 7:00:47 PM6/21/16
to django-d...@googlegroups.com
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
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
Tobias McNulty
Jun 21, 2016, 7:47:33 PM6/21/16
to django-developers
On Tue, Jun 21, 2016 at 7:00 PM, Carl Meyer <ca...@oddbird.net> 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.
Reply all
Reply to author
Forward
0 new messages