re-thinking middleware
711 views
Skip to first unread message
Carl Meyer
Jan 7, 2016, 9:51:08 PM1/7/16
to django-d...@googlegroups.com
Hi all,
Back at the Django Under the Hood sprints in November, Florian and I
started batting around some ideas for fixing some of the problems with
the existing middleware abstraction. Florian put together an initial
pull request with a POC for some of those ideas. I've now updated the
proposal (incorporating some implementation ideas from Pyramid's
middleware equivalent) and written it up in the form of a DEP. You can
find the DEP at
https://github.com/django/deps/blob/master/draft/0005-rethinking-middleware.rst
and I'm also pasting the full text below, to ease in-line commenting via
email.
Comments welcome!
================================
DEP 0005: Re-thinking middleware
================================
:DEP: 0005
:Author: Carl Meyer
:Implementation Team: Florian Apolloner
:Shepherd: Carl Meyer
:Status: Draft
:Type: Feature
:Created: 2016-01-07
:Last-Modified: 2016-01-07
.. contents:: Table of Contents
:depth: 3
:local:
Abstract
========
The existing Django "middleware" abstraction suffers from an absence of
strict layering and balanced in/out calls to a given middleware. This
DEP proposes an improved abstraction for wrapping the request cycle in
strictly layered pre-view and post-view actions.
Motivation
==========
In theory, and per `the documentation`_, ``process_request`` will be
called for each incoming request, ``process_response`` will be called
for each outgoing response, and ``process_exception`` will be called in
case of an uncaught exception.
This description seems to imply the invariant that if
``process_request`` is called, either ``process_response`` or
``process_exception`` will later be called on that same middleware in
that same request cycle. Django itself has in the past included
middleware (the now-defunct ``TransactionMiddleware``) that implicitly
relied on this invariant.
In fact, due to the short-circuiting and exception-handling behavior of
various middleware methods, this invariant does not hold. It is possible
for a middleware to have its ``process_request`` method called, but then
never see its ``process_response`` or ``process_exception`` called for
that request (e.g. in case of an uncaught exception in a "later"
middleware method).
It is also possible for a middleware to never see its
``process_request`` method called for a given request (because an
earlier middleware's ``process_request`` returned a response), but still
have its ``process_response`` or ``process_exception`` method called on
that response.
This lack of strict in/out layering makes it impossible to safely
implement some types of middleware (such as ``TransactionMiddleware``),
and requires verbose defensive programming: e.g. even if
``process_request`` sets a certain attribute on the request,
``process_response`` on that same middleware can't assume that that
attribute will be present on the request it receives.
This is the primary problem that this DEP intends to solve.
.. _the documentation:
https://docs.djangoproject.com/en/stable/topics/http/middleware/
Acknowledgment
==============
The proposed API in this DEP is modelled on Pyramid's `Tween`_ concept
(the author and implementor of this DEP developed a very similar idea
independently at a Django sprint before reading about Tweens).
.. _Tween:
http://docs.pylonsproject.org/projects/pyramid/en/latest/narr/hooks.html#registering-tweens
Specification
=============
This DEP introduces a new setting, ``MIDDLEWARE``, which contains an
ordered list of dotted paths to middleware factories.
A middleware factory can be written as a function that looks like this::
def simple_middleware(get_response):
# one-time configuration and initialization
def middleware(request):
# code to be executed for each request before
# the view is called
response = get_response(request)
# code to be executed for each request/response after
# the view is called
return response
return middleware
Or it can be written as a class with a ``__call__`` method, like this::
class SimpleMiddleware(object):
def __init__(self, get_response):
self.get_response = get_response
# one-time configuration and initialization
def __call__(self, request):
# code to be executed for each request before
# the view is called
response = self.get_response(request)
# code to be executed for each request/response after
# the view is called
return response
In prose instead of examples: a middleware factory is a callable that
takes a ``get_response`` callable and returns a middleware. A middleware
is a callable that takes a ``request`` and returns a ``response``. (Just
like a view! Turtles all the way down!)
The ``get_response`` callable provided by Django might be the actual
view (if this is the last listed middleware), or it might be the next
middleware in the chain. The current middleware doesn't need to know or
care what exactly it is -- just that it represents "upstream", and that
it also takes a request and returns a response.
(The above is a slight simplification -- the ``get_response`` callable
for the last middleware in the chain won't be the actual view, it'll be
a wrapper method from the handler which takes care of view middleware,
calling the view with appropriate url args, and template-response
middleware; see below.)
Disabling middleware
--------------------
A middleware can be disabled at setup time, if it's not needed or not
supported under the current settings.
For a class-based middleware, this is achieved the same way as in
current Django: by raising ``MiddlewareNotUsed`` from the ``__init__``
method.
A function middleware factory can either raise ``MiddlewareNotUsed``, or
can simply return ``None`` instead of a middleware callable.
View and template-response middleware
-------------------------------------
The above examples already encompass the full functionality of
``process_request`` (this is the code that goes before the call to
``get_response``), ``process_response`` (the code that goes after), and
``process_exception`` (just place the call to ``get_response`` within a
``try/except`` block). It also allows more powerful idioms, like
wrapping the call to ``get_response`` in a context manager
(e.g. ``transaction.atomic``).
This DEP does not propose to change the implementation of view
middleware or template-response middleware (since these are really
single-point hooks, not wrappers, and don't suffer from the same in/out
balancing issue). A middleware that wishes to implement one or both of
these hooks should be implemented in the class style, and should
implement ``process_view`` and/or ``process_template_response`` methods,
exactly as it would today.
Changes in short-circuiting semantics
-------------------------------------
Under the new scheme, middleware really will behave more like an
"onion", as described in the documentation. That is, when a middleware
short-circuits the upstream middleware and view by returning a response,
that response will only pass through previous middleware in the list,
rather than passing through the ``process_response`` methods of *all*
middleware (including some who never got a crack at
``process_request``), as occurs today.
Similarly, a middleware that modifies the request on the way in and does
pass it on upstream can be guaranteed that it will always see the
response on the way back out. (If it also wants to see any uncaught
exception on the way out, it can just wrap its call to ``get_response``
in a ``try/except``).
Backwards Compatibility
=======================
"New-style" middleware factories cannot inter-operate
backwards-compatibly in a single mixed list with old-style middlewares,
because it is not possible to maintain both the "in/out balanced"
invariant of the new and the existing short-circuiting behaviors of the
old. This is why a new ``MIDDLEWARE`` setting is introduced to contain
the new-style middleware factories. If the ``MIDDLEWARE`` setting is
provided (it will initially be set to ``None`` in the global default
settings), the old ``MIDDLEWARE_CLASSES`` setting will be ignored. If
``MIDDLEWARE`` is not set, ``MIDDLEWARE_CLASSES`` will behave exactly as
it does today.
The implementation of this DEP will include new-style implementations of
all middlewares included in Django; the current implementations will not
be removed. The ``startproject`` template will include a ``MIDDLEWARE``
setting referencing the new-style middleware.
Transition assistance mixin
---------------------------
In order to ease providing the existing built-in middleware in both
new-style and old-style forms, and to ease similar conversions of
third-party middleware, a converter mix-in will be provided, with an
implementation similar to the following::
class MiddlewareConversionMixin(object):
def __init__(self, get_response):
self.get_response = get_response
super(MiddlewareMixin, self).__init__()
def __call__(self, request):
response = None
if hasattr(self, 'process_request'):
response = self.process_request(request)
if not response:
try:
response = self.get_response(request)
except Exception as e:
if hasattr(self, 'process_exception'):
return self.process_exception(request, e)
else:
raise
if hasattr(self, 'process_response'):
response = self.process_response(request, response)
return response
In most cases, this mixin will be sufficient to convert a middleware
with sufficient backwards-compatibility; the new short-circuiting
semantics will be harmless or even beneficial to the existing
middleware. In a few unusual cases, a middleware class may need more
invasive changes to adjust to the new semantics.
Deprecation
-----------
The fallback from a missing ``MIDDLEWARE`` setting to
``MIDDLEWARE_CLASSES`` will be subject to a normal deprecation path. At
the conclusion of that deprecation path, support for the fallback, the
old-style middleware implementations in Django, and the conversion
mixin, will be removed.
Rationale
=========
The above specification has the advantage that a very similar scheme is
already in use and battle-tested in another widely-used Python web
framework, Pyramid.
Alternatives considered and rejected:
Simple functions
----------------
Earlier drafts of this proposal suggested that a middleware could be
implemented as a simple function that took both ``request`` and
``get_response`` directly, rather than as a factory::
def simple_middleware(request, get_response):
# request-munging
response = get_response(request)
# response-munging
return response
This approach turned out to have two disadvantages: it was less
backwards-compatible, because it's not compatible with class-based
middleware (when would a class be instantiated?), and it would be
slower, since it requires Django to construct a new chain of closures
for every request, whereas the factory approach allows the closure chain
to be constructed just once and reused for each request.
Using generators
----------------
It would be possible to eliminate the need to pass in a ``get_response``
callable by turning middleware into generators which would ``yield`` the
request, and then Django would call ``.send(response)`` on the generator
object to pass back in the response. In that case a middleware body
might look like this::
def simple_middleware(request):
# request-munging
response = yield request
# response-munging
return response
This is clever; probably too clever. In the end it doesn't provide any
useful benefits over the approach proposed above and takes advantage of
Python features that are unfamiliar to many developers (generators that
receive values).
Reference Implementation
========================
The reference implementation work-in-progress (which as of this writing
doesn't yet implement the proposal described here, but rather an earlier
iteration) can be found at
https://github.com/django/django/pull/5591/files
Copyright
=========
This document has been placed in the public domain per the Creative
Commons CC0 1.0 Universal license
(http://creativecommons.org/publicdomain/zero/1.0/deed).
Back at the Django Under the Hood sprints in November, Florian and I
started batting around some ideas for fixing some of the problems with
the existing middleware abstraction. Florian put together an initial
pull request with a POC for some of those ideas. I've now updated the
proposal (incorporating some implementation ideas from Pyramid's
middleware equivalent) and written it up in the form of a DEP. You can
find the DEP at
https://github.com/django/deps/blob/master/draft/0005-rethinking-middleware.rst
and I'm also pasting the full text below, to ease in-line commenting via
email.
Comments welcome!
================================
DEP 0005: Re-thinking middleware
================================
:DEP: 0005
:Author: Carl Meyer
:Implementation Team: Florian Apolloner
:Shepherd: Carl Meyer
:Status: Draft
:Type: Feature
:Created: 2016-01-07
:Last-Modified: 2016-01-07
.. contents:: Table of Contents
:depth: 3
:local:
Abstract
========
The existing Django "middleware" abstraction suffers from an absence of
strict layering and balanced in/out calls to a given middleware. This
DEP proposes an improved abstraction for wrapping the request cycle in
strictly layered pre-view and post-view actions.
Motivation
==========
In theory, and per `the documentation`_, ``process_request`` will be
called for each incoming request, ``process_response`` will be called
for each outgoing response, and ``process_exception`` will be called in
case of an uncaught exception.
This description seems to imply the invariant that if
``process_request`` is called, either ``process_response`` or
``process_exception`` will later be called on that same middleware in
that same request cycle. Django itself has in the past included
middleware (the now-defunct ``TransactionMiddleware``) that implicitly
relied on this invariant.
In fact, due to the short-circuiting and exception-handling behavior of
various middleware methods, this invariant does not hold. It is possible
for a middleware to have its ``process_request`` method called, but then
never see its ``process_response`` or ``process_exception`` called for
that request (e.g. in case of an uncaught exception in a "later"
middleware method).
It is also possible for a middleware to never see its
``process_request`` method called for a given request (because an
earlier middleware's ``process_request`` returned a response), but still
have its ``process_response`` or ``process_exception`` method called on
that response.
This lack of strict in/out layering makes it impossible to safely
implement some types of middleware (such as ``TransactionMiddleware``),
and requires verbose defensive programming: e.g. even if
``process_request`` sets a certain attribute on the request,
``process_response`` on that same middleware can't assume that that
attribute will be present on the request it receives.
This is the primary problem that this DEP intends to solve.
.. _the documentation:
https://docs.djangoproject.com/en/stable/topics/http/middleware/
Acknowledgment
==============
The proposed API in this DEP is modelled on Pyramid's `Tween`_ concept
(the author and implementor of this DEP developed a very similar idea
independently at a Django sprint before reading about Tweens).
.. _Tween:
http://docs.pylonsproject.org/projects/pyramid/en/latest/narr/hooks.html#registering-tweens
Specification
=============
This DEP introduces a new setting, ``MIDDLEWARE``, which contains an
ordered list of dotted paths to middleware factories.
A middleware factory can be written as a function that looks like this::
def simple_middleware(get_response):
# one-time configuration and initialization
def middleware(request):
# code to be executed for each request before
# the view is called
response = get_response(request)
# code to be executed for each request/response after
# the view is called
return response
return middleware
Or it can be written as a class with a ``__call__`` method, like this::
class SimpleMiddleware(object):
def __init__(self, get_response):
self.get_response = get_response
# one-time configuration and initialization
def __call__(self, request):
# code to be executed for each request before
# the view is called
response = self.get_response(request)
# code to be executed for each request/response after
# the view is called
return response
In prose instead of examples: a middleware factory is a callable that
takes a ``get_response`` callable and returns a middleware. A middleware
is a callable that takes a ``request`` and returns a ``response``. (Just
like a view! Turtles all the way down!)
The ``get_response`` callable provided by Django might be the actual
view (if this is the last listed middleware), or it might be the next
middleware in the chain. The current middleware doesn't need to know or
care what exactly it is -- just that it represents "upstream", and that
it also takes a request and returns a response.
(The above is a slight simplification -- the ``get_response`` callable
for the last middleware in the chain won't be the actual view, it'll be
a wrapper method from the handler which takes care of view middleware,
calling the view with appropriate url args, and template-response
middleware; see below.)
Disabling middleware
--------------------
A middleware can be disabled at setup time, if it's not needed or not
supported under the current settings.
For a class-based middleware, this is achieved the same way as in
current Django: by raising ``MiddlewareNotUsed`` from the ``__init__``
method.
A function middleware factory can either raise ``MiddlewareNotUsed``, or
can simply return ``None`` instead of a middleware callable.
View and template-response middleware
-------------------------------------
The above examples already encompass the full functionality of
``process_request`` (this is the code that goes before the call to
``get_response``), ``process_response`` (the code that goes after), and
``process_exception`` (just place the call to ``get_response`` within a
``try/except`` block). It also allows more powerful idioms, like
wrapping the call to ``get_response`` in a context manager
(e.g. ``transaction.atomic``).
This DEP does not propose to change the implementation of view
middleware or template-response middleware (since these are really
single-point hooks, not wrappers, and don't suffer from the same in/out
balancing issue). A middleware that wishes to implement one or both of
these hooks should be implemented in the class style, and should
implement ``process_view`` and/or ``process_template_response`` methods,
exactly as it would today.
Changes in short-circuiting semantics
-------------------------------------
Under the new scheme, middleware really will behave more like an
"onion", as described in the documentation. That is, when a middleware
short-circuits the upstream middleware and view by returning a response,
that response will only pass through previous middleware in the list,
rather than passing through the ``process_response`` methods of *all*
middleware (including some who never got a crack at
``process_request``), as occurs today.
Similarly, a middleware that modifies the request on the way in and does
pass it on upstream can be guaranteed that it will always see the
response on the way back out. (If it also wants to see any uncaught
exception on the way out, it can just wrap its call to ``get_response``
in a ``try/except``).
Backwards Compatibility
=======================
"New-style" middleware factories cannot inter-operate
backwards-compatibly in a single mixed list with old-style middlewares,
because it is not possible to maintain both the "in/out balanced"
invariant of the new and the existing short-circuiting behaviors of the
old. This is why a new ``MIDDLEWARE`` setting is introduced to contain
the new-style middleware factories. If the ``MIDDLEWARE`` setting is
provided (it will initially be set to ``None`` in the global default
settings), the old ``MIDDLEWARE_CLASSES`` setting will be ignored. If
``MIDDLEWARE`` is not set, ``MIDDLEWARE_CLASSES`` will behave exactly as
it does today.
The implementation of this DEP will include new-style implementations of
all middlewares included in Django; the current implementations will not
be removed. The ``startproject`` template will include a ``MIDDLEWARE``
setting referencing the new-style middleware.
Transition assistance mixin
---------------------------
In order to ease providing the existing built-in middleware in both
new-style and old-style forms, and to ease similar conversions of
third-party middleware, a converter mix-in will be provided, with an
implementation similar to the following::
class MiddlewareConversionMixin(object):
def __init__(self, get_response):
self.get_response = get_response
super(MiddlewareMixin, self).__init__()
def __call__(self, request):
response = None
if hasattr(self, 'process_request'):
response = self.process_request(request)
if not response:
try:
response = self.get_response(request)
except Exception as e:
if hasattr(self, 'process_exception'):
return self.process_exception(request, e)
else:
raise
if hasattr(self, 'process_response'):
response = self.process_response(request, response)
return response
In most cases, this mixin will be sufficient to convert a middleware
with sufficient backwards-compatibility; the new short-circuiting
semantics will be harmless or even beneficial to the existing
middleware. In a few unusual cases, a middleware class may need more
invasive changes to adjust to the new semantics.
Deprecation
-----------
The fallback from a missing ``MIDDLEWARE`` setting to
``MIDDLEWARE_CLASSES`` will be subject to a normal deprecation path. At
the conclusion of that deprecation path, support for the fallback, the
old-style middleware implementations in Django, and the conversion
mixin, will be removed.
Rationale
=========
The above specification has the advantage that a very similar scheme is
already in use and battle-tested in another widely-used Python web
framework, Pyramid.
Alternatives considered and rejected:
Simple functions
----------------
Earlier drafts of this proposal suggested that a middleware could be
implemented as a simple function that took both ``request`` and
``get_response`` directly, rather than as a factory::
def simple_middleware(request, get_response):
# request-munging
response = get_response(request)
# response-munging
return response
This approach turned out to have two disadvantages: it was less
backwards-compatible, because it's not compatible with class-based
middleware (when would a class be instantiated?), and it would be
slower, since it requires Django to construct a new chain of closures
for every request, whereas the factory approach allows the closure chain
to be constructed just once and reused for each request.
Using generators
----------------
It would be possible to eliminate the need to pass in a ``get_response``
callable by turning middleware into generators which would ``yield`` the
request, and then Django would call ``.send(response)`` on the generator
object to pass back in the response. In that case a middleware body
might look like this::
def simple_middleware(request):
# request-munging
response = yield request
# response-munging
return response
This is clever; probably too clever. In the end it doesn't provide any
useful benefits over the approach proposed above and takes advantage of
Python features that are unfamiliar to many developers (generators that
receive values).
Reference Implementation
========================
The reference implementation work-in-progress (which as of this writing
doesn't yet implement the proposal described here, but rather an earlier
iteration) can be found at
https://github.com/django/django/pull/5591/files
Copyright
=========
This document has been placed in the public domain per the Creative
Commons CC0 1.0 Universal license
(http://creativecommons.org/publicdomain/zero/1.0/deed).
Ryan Hiebert
Jan 8, 2016, 12:07:16 AM1/8/16
to django-d...@googlegroups.com
Sent from my iPhone
> On Jan 7, 2016, at 20:50, Carl Meyer <ca...@oddbird.net> wrote:
>
> Hi all,
>
> Back at the Django Under the Hood sprints in November, Florian and I
> started batting around some ideas for fixing some of the problems with
> the existing middleware abstraction. Florian put together an initial
> pull request with a POC for some of those ideas. I've now updated the
> proposal (incorporating some implementation ideas from Pyramid's
> middleware equivalent) and written it up in the form of a DEP. You can
> find the DEP at
>
> https://github.com/django/deps/blob/master/draft/0005-rethinking-middleware.rst
>
> and I'm also pasting the full text below, to ease in-line commenting via
> email.
>
> Comments welcome!
Curtis Maloney
Jan 8, 2016, 6:32:07 AM1/8/16
to django-d...@googlegroups.com
In general, I love it.
It's MUCH simpler for people to write and comprehend... requires no
classes [but IMHO the callable class is "cleaner"] and allows for
configurable middlewares easily...
I do wonder, though... how the anti-import-strings factions will
react... I'm sure it can, at least, support direct callables being in
the MIDDLEWARE list, not just strings?
--
Curtis
It's MUCH simpler for people to write and comprehend... requires no
classes [but IMHO the callable class is "cleaner"] and allows for
configurable middlewares easily...
I do wonder, though... how the anti-import-strings factions will
react... I'm sure it can, at least, support direct callables being in
the MIDDLEWARE list, not just strings?
--
Curtis
Markus Holtermann
Jan 8, 2016, 6:51:25 AM1/8/16
to django-d...@googlegroups.com, Curtis Maloney
Thank you Florian and Carl for continuing the work on that topic. I like both the DEP as well as the example.
I would, however, include the exception handling in the examples provided in section "Specification" as that is an integral part of middlewares, too.
Nitpicking, I would also name the settings variable MIDDLEWARES (i.e. plural) as it is a list of middlewares, not just one.
/Markus
I would, however, include the exception handling in the examples provided in section "Specification" as that is an integral part of middlewares, too.
Nitpicking, I would also name the settings variable MIDDLEWARES (i.e. plural) as it is a list of middlewares, not just one.
/Markus
Florian Apolloner
Jan 8, 2016, 8:50:47 AM1/8/16
to Django developers (Contributions to Django itself), cur...@tinbrain.net
On Friday, January 8, 2016 at 12:51:25 PM UTC+1, Markus Holtermann wrote:
Nitpicking, I would also name the settings variable MIDDLEWARES (i.e. plural) as it is a list of middlewares, not just one.
Yes, my thought too.
I do wonder, though... how the anti-import-strings factions will
react... I'm sure it can, at least, support direct callables being in
the MIDDLEWARE list, not just strings?
We can support it (we actually do that in one place already, take a wild guess), would I wouldn't call it a feature and also wouldn't really document it; after all if a middleware imports models or so you are tosed.
Florian Apolloner
Jan 8, 2016, 9:29:01 AM1/8/16
to Django developers (Contributions to Django itself)
Mhm,
I currently see no easy way to give get_response access to the list of middlewares like done in https://github.com/django/django/pull/5591/files#diff-dbd7d9159676b15fc9a096b0adb919e9R122 and following lines. The reason is that providing get_response at setup/init time means it has to be one "static" method which is the same for all requests. Aside from using a thread local or stashing the middleware list on the request, this method has no way of getting access to the current middleware chain. Any ideas?
Cheers,
Florian
I currently see no easy way to give get_response access to the list of middlewares like done in https://github.com/django/django/pull/5591/files#diff-dbd7d9159676b15fc9a096b0adb919e9R122 and following lines. The reason is that providing get_response at setup/init time means it has to be one "static" method which is the same for all requests. Aside from using a thread local or stashing the middleware list on the request, this method has no way of getting access to the current middleware chain. Any ideas?
Cheers,
Florian
Florian Apolloner
Jan 8, 2016, 10:27:07 AM1/8/16
to Django developers (Contributions to Django itself)
I've gone ahead and pushed a new commit to match the spec (not tested yet, but hey :D).
Florian Apolloner
Jan 8, 2016, 10:55:08 AM1/8/16
to Django developers (Contributions to Django itself)
Haha, I was so focused on my previous approach that I forgot, that I could just chain them during setup (thanks knbk). So the question remaining now: Do we want to be able to change middlewares per request or not :D I guess I'll submit a second PR with the non-request usage.
On Friday, January 8, 2016 at 3:29:01 PM UTC+1, Florian Apolloner wrote:
Florian Apolloner
Jan 8, 2016, 11:01:15 AM1/8/16
to Django developers (Contributions to Django itself)
PR1 (allows changing the middlewares during the middleware run): https://github.com/django/django/pull/5591
PR2 (static middleware chain, seems simpler and probably the way to go): https://github.com/django/django/pull/5949/
PR2 (static middleware chain, seems simpler and probably the way to go): https://github.com/django/django/pull/5949/
Carl Meyer
Jan 8, 2016, 11:53:03 AM1/8/16
to django-d...@googlegroups.com
Hi Curtis,
On 01/08/2016 04:31 AM, Curtis Maloney wrote:
> In general, I love it.
Great!
> It's MUCH simpler for people to write and comprehend... requires no
> classes [but IMHO the callable class is "cleaner"] and allows for
> configurable middlewares easily...
>
> I do wonder, though... how the anti-import-strings factions will
> react... I'm sure it can, at least, support direct callables being in
> the MIDDLEWARE list, not just strings?
Well, I guess I'm part of that faction, and I wrote the proposal :-)
The settings file is one place where import-strings are pretty much
unavoidable in Django, because you can't safely import anything into
your settings model that indirectly imports models, or certain other
parts of Django that require settings to be loaded. I don't _like_ this,
but it's the reality, at least until we get rid of settings as a global
singleton (if that ever happens).
We could support directly including middleware factories instead of just
import strings in your MIDDLEWARE setting, but I don't think that's a
good idea, as encouraging people to import stuff from their project into
their settings file is almost certain to end with it blowing up in their
face at some point.
Carl
On 01/08/2016 04:31 AM, Curtis Maloney wrote:
> In general, I love it.
> It's MUCH simpler for people to write and comprehend... requires no
> classes [but IMHO the callable class is "cleaner"] and allows for
> configurable middlewares easily...
>
> I do wonder, though... how the anti-import-strings factions will
> react... I'm sure it can, at least, support direct callables being in
> the MIDDLEWARE list, not just strings?
The settings file is one place where import-strings are pretty much
unavoidable in Django, because you can't safely import anything into
your settings model that indirectly imports models, or certain other
parts of Django that require settings to be loaded. I don't _like_ this,
but it's the reality, at least until we get rid of settings as a global
singleton (if that ever happens).
We could support directly including middleware factories instead of just
import strings in your MIDDLEWARE setting, but I don't think that's a
good idea, as encouraging people to import stuff from their project into
their settings file is almost certain to end with it blowing up in their
face at some point.
Carl
Aymeric Augustin
Jan 8, 2016, 12:35:28 PM1/8/16
to django-d...@googlegroups.com
+1
Great work.
The only (and minor) concern I have is about allowing function-based middleware factories to return None.
In the spirit of “there should be only one way to do it”, I would require raising MiddlewareNotUsed explicitly to disable a middleware. A middleware factory that returns None would cause an ImproperlyConfigured exception. Otherwise middleware could be skipped by mistake, if the developer forgets to return the middleware from the factory. This is especially a concern for production-only middleware that developers can’t run locally.
It’s easy to imagine scenarios where this would escalate into a security issue. For instance, consider a middleware that hooks into a centralized corporate authentication system and rejects unauthorized users. It’s common not to have a testing version of that kind of infrastructure and for developers not to have keys for the production version. Add an incomplete testing strategy that only check that authorized users have access…
--
Aymeric.
> --
> 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-develop...@googlegroups.com.
> To post to this group, send email to django-d...@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/568F2405.4030806%40oddbird.net.
> For more options, visit https://groups.google.com/d/optout.
Great work.
The only (and minor) concern I have is about allowing function-based middleware factories to return None.
In the spirit of “there should be only one way to do it”, I would require raising MiddlewareNotUsed explicitly to disable a middleware. A middleware factory that returns None would cause an ImproperlyConfigured exception. Otherwise middleware could be skipped by mistake, if the developer forgets to return the middleware from the factory. This is especially a concern for production-only middleware that developers can’t run locally.
It’s easy to imagine scenarios where this would escalate into a security issue. For instance, consider a middleware that hooks into a centralized corporate authentication system and rejects unauthorized users. It’s common not to have a testing version of that kind of infrastructure and for developers not to have keys for the production version. Add an incomplete testing strategy that only check that authorized users have access…
--
Aymeric.
> 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-develop...@googlegroups.com.
> To post to this group, send email to django-d...@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/568F2405.4030806%40oddbird.net.
> For more options, visit https://groups.google.com/d/optout.
Ryan Hiebert
Jan 8, 2016, 12:57:25 PM1/8/16
to django-d...@googlegroups.com
> On Jan 8, 2016, at 11:35 AM, Aymeric Augustin <aymeric....@polytechnique.org> wrote:
>
> +1
>
> Great work.
>
> The only (and minor) concern I have is about allowing function-based middleware factories to return None.
>
> In the spirit of “there should be only one way to do it”, I would require raising MiddlewareNotUsed explicitly to disable a middleware. A middleware factory that returns None would cause an ImproperlyConfigured exception. Otherwise middleware could be skipped by mistake, if the developer forgets to return the middleware from the factory. This is especially a concern for production-only middleware that developers can’t run locally.
>
> It’s easy to imagine scenarios where this would escalate into a security issue. For instance, consider a middleware that hooks into a centralized corporate authentication system and rejects unauthorized users. It’s common not to have a testing version of that kind of infrastructure and for developers not to have keys for the production version. Add an incomplete testing strategy that only check that authorized users have access…
Ryan Hiebert
Jan 8, 2016, 1:01:59 PM1/8/16
to django-d...@googlegroups.com
> On Jan 8, 2016, at 11:57 AM, Ryan Hiebert <ry...@ryanhiebert.com> wrote:
>
>> On Jan 8, 2016, at 11:35 AM, Aymeric Augustin <aymeric....@polytechnique.org> wrote:
>>
>> In the spirit of “there should be only one way to do it”, I would require raising MiddlewareNotUsed explicitly to disable a middleware.
>
> I definitely agree with this critique. I regularly forget to return the wrapper function when writing decorators, and this seems likely to have the same issue. Having it warn me early and loud that I screwed up would be very helpful.
And perhaps instead of raising, it could just return MiddlewareNotUsed, in a similar way as you would return NotImplemented on implementations of dunder methods. This case doesn't require doing things that way, as there's no equivalent to the right-side fallback of operators, but I think it may still be an option worth considering.Carl Meyer
Jan 8, 2016, 1:38:25 PM1/8/16
to django-d...@googlegroups.com
Thanks everyone for the reviews. I've pushed a new commit addressing
most of the issues raised:
https://github.com/django/deps/commit/62b0ee351727cb0e7ef41ba6dd2f3f7280a219de
More comments and replies (to various people in the thread) below:
On 01/08/2016 09:01 AM, Florian Apolloner wrote:
> PR1 (allows changing the middlewares during the middleware run):
> https://github.com/django/django/pull/5591
> PR2 (static middleware chain, seems simpler and probably the way to
> go): https://github.com/django/django/pull/5949/
Yes, the latter is what Pyramid does and what I had in mind; in fact,
its performance benefits are one of the justifications given in the DEP
for why to use the factory approach rather than simple functions. I
don't think "changing middlewares per request" is a use case that's
worth the performance implications (and it's not a use case we currently
support anyway).
Thanks for the super-quick turnaround on implementation!
On 01/08/2016 10:35 AM, Aymeric Augustin wrote:
> The only (and minor) concern I have is about allowing function-based
> middleware factories to return None.
>
> In the spirit of “there should be only one way to do it”, I would
> require raising MiddlewareNotUsed explicitly to disable a middleware.
> A middleware factory that returns None would cause an
> ImproperlyConfigured exception. Otherwise middleware could be skipped
> by mistake, if the developer forgets to return the middleware from
> the factory. This is especially a concern for production-only
> middleware that developers can’t run locally.
[snip]
Yes, you and Ryan are absolutely right, allowing `None` was a mistake
and I've removed it from the spec. For function-based middleware,
there's also the option to simply return the ``get_response`` callable
you were given, which has the same effect of "skipping yourself." That
falls out naturally from the concept, so I don't see any reason to
disallow it, but we don't need to mention it in the docs if we want to
stick to "one way to do it."
On 01/08/2016 04:51 AM, Markus Holtermann wrote:
> I would, however, include the exception handling in the examples
> provided in section "Specification" as that is an integral part of
> middlewares, too.
Good idea, I've made that change in the commit linked above.
> Nitpicking, I would also name the settings variable MIDDLEWARES
> (i.e.plural) as it is a list of middlewares, not just one.
Well, this is a matter of some debate :-) See e.g.
https://github.com/rack/rack/issues/332, where a number of people
fervently argue that "middleware" is a "mass noun" along the lines of
e.g. "furniture", and that it is even incorrect to say "a middleware"
(much like you would never say "a furniture"); instead we should always
say "a middleware component."
I think those people are fighting a lost battle; the usage of
"middleware" as singular is already well established in the Python
world, even outside Django; people frequently talk about "a WSGI
middleware."
That said, my ear still prefers "middleware" as both the singular and
the plural (there are such words in English) and dislikes "middlewares."
So I'd prefer to stick with MIDDLEWARE and haven't changed it in the
spec. But if most people think MIDDLEWARES is better, I won't stand in
the way.
We could also go with something like MIDDLEWARE_FACTORIES or
MIDDLEWARE_PIPELINE and avoid the issue altogether :-)
Carl
most of the issues raised:
https://github.com/django/deps/commit/62b0ee351727cb0e7ef41ba6dd2f3f7280a219de
More comments and replies (to various people in the thread) below:
On 01/08/2016 09:01 AM, Florian Apolloner wrote:
> PR1 (allows changing the middlewares during the middleware run):
> https://github.com/django/django/pull/5591
> PR2 (static middleware chain, seems simpler and probably the way to
> go): https://github.com/django/django/pull/5949/
its performance benefits are one of the justifications given in the DEP
for why to use the factory approach rather than simple functions. I
don't think "changing middlewares per request" is a use case that's
worth the performance implications (and it's not a use case we currently
support anyway).
Thanks for the super-quick turnaround on implementation!
On 01/08/2016 10:35 AM, Aymeric Augustin wrote:
> The only (and minor) concern I have is about allowing function-based
> middleware factories to return None.
>
> In the spirit of “there should be only one way to do it”, I would
> require raising MiddlewareNotUsed explicitly to disable a middleware.
> A middleware factory that returns None would cause an
> ImproperlyConfigured exception. Otherwise middleware could be skipped
> by mistake, if the developer forgets to return the middleware from
> the factory. This is especially a concern for production-only
> middleware that developers can’t run locally.
Yes, you and Ryan are absolutely right, allowing `None` was a mistake
and I've removed it from the spec. For function-based middleware,
there's also the option to simply return the ``get_response`` callable
you were given, which has the same effect of "skipping yourself." That
falls out naturally from the concept, so I don't see any reason to
disallow it, but we don't need to mention it in the docs if we want to
stick to "one way to do it."
On 01/08/2016 04:51 AM, Markus Holtermann wrote:
> I would, however, include the exception handling in the examples
> provided in section "Specification" as that is an integral part of
> middlewares, too.
> Nitpicking, I would also name the settings variable MIDDLEWARES
Well, this is a matter of some debate :-) See e.g.
https://github.com/rack/rack/issues/332, where a number of people
fervently argue that "middleware" is a "mass noun" along the lines of
e.g. "furniture", and that it is even incorrect to say "a middleware"
(much like you would never say "a furniture"); instead we should always
say "a middleware component."
I think those people are fighting a lost battle; the usage of
"middleware" as singular is already well established in the Python
world, even outside Django; people frequently talk about "a WSGI
middleware."
That said, my ear still prefers "middleware" as both the singular and
the plural (there are such words in English) and dislikes "middlewares."
So I'd prefer to stick with MIDDLEWARE and haven't changed it in the
spec. But if most people think MIDDLEWARES is better, I won't stand in
the way.
We could also go with something like MIDDLEWARE_FACTORIES or
MIDDLEWARE_PIPELINE and avoid the issue altogether :-)
Carl
Ryan Hiebert
Jan 8, 2016, 1:48:39 PM1/8/16
to django-d...@googlegroups.com
> On Jan 8, 2016, at 12:38 PM, Carl Meyer <ca...@oddbird.net> wrote:
>
> Yes, you and Ryan are absolutely right, allowing `None` was a mistake
> and I've removed it from the spec. For function-based middleware,
> there's also the option to simply return the ``get_response`` callable
> you were given, which has the same effect of "skipping yourself." That
> falls out naturally from the concept, so I don't see any reason to
> disallow it, but we don't need to mention it in the docs if we want to
> stick to "one way to do it."
I really like it, because it avoids both raising an exception and
importing a common symbol.
While class-based middleware factories couldn't just do the same thing
in the __init__ method, they could do that in a __new__ method instead,
and then we could make returning the ``get_response`` function the
Carl Meyer
Jan 8, 2016, 1:53:38 PM1/8/16
to django-d...@googlegroups.com
Hi Ryan,
On 01/08/2016 11:48 AM, Ryan Hiebert wrote:
> I hadn't considered the option of just returning ``get_response``.
> I really like it, because it avoids both raising an exception and
> importing a common symbol.
>
> While class-based middleware factories couldn't just do the same thing
> in the __init__ method, they could do that in a __new__ method instead,
> and then we could make returning the ``get_response`` function the
> "one way to do it."
I'd be more sympathetic to this viewpoint if Django hadn't already had
the `MiddlewareNotUsed` exception for many years in this very role.
There's no reason to break it if we don't absolutely have to. And since
we've got it, and it works in any scenario, it may as well be the
preferred method.
Implementing `__new__` is something many Python developers aren't
familiar with and don't do regularly, so I think it's not a good choice
as the primary recommendation.
Also, disabling middleware at setup time isn't all that common, so an
import isn't a big deal.
That said, I expect plenty of people (probably myself included) to use
the "just return ``get_response``" trick, whether it's documented or
not, and I think that's totally fine.
Carl
On 01/08/2016 11:48 AM, Ryan Hiebert wrote:
> I hadn't considered the option of just returning ``get_response``.
> I really like it, because it avoids both raising an exception and
> importing a common symbol.
>
> While class-based middleware factories couldn't just do the same thing
> in the __init__ method, they could do that in a __new__ method instead,
> and then we could make returning the ``get_response`` function the
> "one way to do it."
the `MiddlewareNotUsed` exception for many years in this very role.
There's no reason to break it if we don't absolutely have to. And since
we've got it, and it works in any scenario, it may as well be the
preferred method.
Implementing `__new__` is something many Python developers aren't
familiar with and don't do regularly, so I think it's not a good choice
as the primary recommendation.
Also, disabling middleware at setup time isn't all that common, so an
import isn't a big deal.
That said, I expect plenty of people (probably myself included) to use
the "just return ``get_response``" trick, whether it's documented or
not, and I think that's totally fine.
Carl
Ryan Hiebert
Jan 8, 2016, 1:57:40 PM1/8/16
to django-d...@googlegroups.com
> On Jan 8, 2016, at 12:53 PM, Carl Meyer <ca...@oddbird.net> wrote:
>
> On 01/08/2016 11:48 AM, Ryan Hiebert wrote:
>>
>> While class-based middleware factories couldn't just do the same thing
>> in the __init__ method, they could do that in a __new__ method instead,
>> and then we could make returning the ``get_response`` function the
>> "one way to do it."
>
> I'd be more sympathetic to this viewpoint if Django hadn't already had
> the `MiddlewareNotUsed` exception for many years in this very role.
> There's no reason to break it if we don't absolutely have to. And since
> we've got it, and it works in any scenario, it may as well be the
> preferred method.
>
>
> That said, I expect plenty of people (probably myself included) to use
> the "just return ``get_response``" trick, whether it's documented or
> not, and I think that's totally fine.
>
Your reasoning makes sense. I'll probably be one of those that will prefer
> That said, I expect plenty of people (probably myself included) to use
> the "just return ``get_response``" trick, whether it's documented or
> not, and I think that's totally fine.
>
the return ``get_response`` trick, even in classed based middlewares.
Carl Meyer
Jan 8, 2016, 2:10:55 PM1/8/16
to django-d...@googlegroups.com
On 01/08/2016 11:38 AM, Carl Meyer wrote:
> On 01/08/2016 04:51 AM, Markus Holtermann wrote:
>> Nitpicking, I would also name the settings variable MIDDLEWARES
>> (i.e.plural) as it is a list of middlewares, not just one.
>
> Well, this is a matter of some debate :-) See e.g.
> https://github.com/rack/rack/issues/332, where a number of people
> fervently argue that "middleware" is a "mass noun" along the lines of
> e.g. "furniture", and that it is even incorrect to say "a middleware"
> (much like you would never say "a furniture"); instead we should always
> say "a middleware component."
>
> I think those people are fighting a lost battle; the usage of
> "middleware" as singular is already well established in the Python
> world, even outside Django; people frequently talk about "a WSGI
> middleware."
>
> That said, my ear still prefers "middleware" as both the singular and
> the plural (there are such words in English) and dislikes "middlewares."
> So I'd prefer to stick with MIDDLEWARE and haven't changed it in the
> spec. But if most people think MIDDLEWARES is better, I won't stand in
> the way.
>
> We could also go with something like MIDDLEWARE_FACTORIES or
> MIDDLEWARE_PIPELINE and avoid the issue altogether :-)
To take it one level up, historically speaking, using the term
> On 01/08/2016 04:51 AM, Markus Holtermann wrote:
>> Nitpicking, I would also name the settings variable MIDDLEWARES
>> (i.e.plural) as it is a list of middlewares, not just one.
>
> Well, this is a matter of some debate :-) See e.g.
> https://github.com/rack/rack/issues/332, where a number of people
> fervently argue that "middleware" is a "mass noun" along the lines of
> e.g. "furniture", and that it is even incorrect to say "a middleware"
> (much like you would never say "a furniture"); instead we should always
> say "a middleware component."
>
> I think those people are fighting a lost battle; the usage of
> "middleware" as singular is already well established in the Python
> world, even outside Django; people frequently talk about "a WSGI
> middleware."
>
> That said, my ear still prefers "middleware" as both the singular and
> the plural (there are such words in English) and dislikes "middlewares."
> So I'd prefer to stick with MIDDLEWARE and haven't changed it in the
> spec. But if most people think MIDDLEWARES is better, I won't stand in
> the way.
>
> We could also go with something like MIDDLEWARE_FACTORIES or
> MIDDLEWARE_PIPELINE and avoid the issue altogether :-)
"middleware" to mean "something between the web server and a web app's
controller/view" is questionable to begin with; note the wikipedia page
for "middleware" doesn't even mention this use of the term once. So
another option is to find a different term entirely and let the word
"middleware" die out from the Django lexicon. One advantage of this
would be that we wouldn't have to keep talking about "new-style" vs
"old-style" middleware :-)
But on the whole I think this is fighting a lost cause too - there are
WSGI middleware, Flask has middleware, Rack has middleware. So wikipedia
and historical usage aside, our use of the term is by now already well
established in the web-frameworks world. I think changing to a different
word will probably create more confusion than anything.
Carl
Raphaël Barrois
Jan 10, 2016, 6:57:09 PM1/10/16
to django-d...@googlegroups.com
I've got only one minor suggestion to the "backwards compatibility"
section of the DEP.
> It currently states that "If the ``MIDDLEWARE`` setting is provided
> [...], the old ``MIDDLEWARE_CLASSES`` setting will be ignored.
I suggest that, instead, this fails loudly if both ``MIDDLEWARE`` and
``MIDDLEWARE_CLASSES`` are set.
This would prevent projects from keeping the two versions around and
having to remember which one is currently in use.
Beyond that, I love the overall design of this upgrade :)
--
Raphaël
Tim Graham
Jan 11, 2016, 6:24:07 AM1/11/16
to Django developers (Contributions to Django itself)
That warning sounds like a good idea. We have a similar warning if both the old and new style template settings are defined:
https://github.com/django/django/commit/24620d71f2116da31abe6c9391f7bc807ac23c0b
https://github.com/django/django/commit/24620d71f2116da31abe6c9391f7bc807ac23c0b
Carl Meyer
Jan 11, 2016, 1:02:35 PM1/11/16
to django-d...@googlegroups.com
On 01/10/2016 04:54 PM, Raphaël Barrois wrote:
> I've got only one minor suggestion to the "backwards compatibility"
> section of the DEP.
>
>> It currently states that "If the ``MIDDLEWARE`` setting is provided
>> [...], the old ``MIDDLEWARE_CLASSES`` setting will be ignored.
>
> I suggest that, instead, this fails loudly if both ``MIDDLEWARE`` and
> ``MIDDLEWARE_CLASSES`` are set.
> This would prevent projects from keeping the two versions around and
> having to remember which one is currently in use.
Yes, that's a good idea. I think I had vaguely in mind that this wasn't
> I've got only one minor suggestion to the "backwards compatibility"
> section of the DEP.
>
>> It currently states that "If the ``MIDDLEWARE`` setting is provided
>> [...], the old ``MIDDLEWARE_CLASSES`` setting will be ignored.
>
> I suggest that, instead, this fails loudly if both ``MIDDLEWARE`` and
> ``MIDDLEWARE_CLASSES`` are set.
> This would prevent projects from keeping the two versions around and
> having to remember which one is currently in use.
possible because the global default for MIDDLEWARE_CLASSES is non-empty,
but I guess we can still check to see whether the actual value != the
global default.
> Beyond that, I love the overall design of this upgrade :)
Carl
Shai Berger
Jan 11, 2016, 6:53:12 PM1/11/16
to django-d...@googlegroups.com
On Monday 11 January 2016 01:54:58 Raphaël Barrois wrote:
>
> Hi,
>
> I've got only one minor suggestion to the "backwards compatibility"
> section of the DEP.
>
> > It currently states that "If the ``MIDDLEWARE`` setting is provided
> > [...], the old ``MIDDLEWARE_CLASSES`` setting will be ignored.
>
> I suggest that, instead, this fails loudly if both ``MIDDLEWARE`` and
> ``MIDDLEWARE_CLASSES`` are set.
> This would prevent projects from keeping the two versions around and
> having to remember which one is currently in use.
>
If the failure is too loud, it makes it hard to use the same settings file for
>
> Hi,
>
> I've got only one minor suggestion to the "backwards compatibility"
> section of the DEP.
>
> > It currently states that "If the ``MIDDLEWARE`` setting is provided
> > [...], the old ``MIDDLEWARE_CLASSES`` setting will be ignored.
>
> I suggest that, instead, this fails loudly if both ``MIDDLEWARE`` and
> ``MIDDLEWARE_CLASSES`` are set.
> This would prevent projects from keeping the two versions around and
> having to remember which one is currently in use.
>
testing with multiple versions of Django -- a practice used by Django itself,
and I think also by some reusable apps. We've run into this issue with the
change of the database test settings.
Shai.
Mat Clayton
Jan 12, 2016, 6:44:25 PM1/12/16
to Django developers (Contributions to Django itself)
Hey guys,
+1 as well, this is really great work, on both patches!
Modifying the middleware stack mid request would be very useful for us.
We currently run 3 copies of the same code, with essentially different urls.py and middlewares loaded, each one is used to run a variation of the site, for example www/mobile web and api, and have slightly different requirements, which are handled in middlewares, for example api calls are authenticated by access tokens and www/mobile use cookies.
Ideally we'd love to consolidate our deployment into a single set of python web workers, but to date its been a little too complex to be worth it. We could however use the url dispatching patch and PR1 to write a middleware which could load in the necessary middleware stack based on the requirements of each request.
So my preference therefore would be for PR1 as I can see a few edge cases it would enable solutions for.
Mat
Carl Meyer
Jan 18, 2016, 6:48:37 PM1/18/16
to django-d...@googlegroups.com
Hi Mat,
On 01/12/2016 04:23 PM, Mat Clayton wrote:
> +1 as well, this is really great work, on both patches!
>
> Modifying the middleware stack mid request would be very useful for us.
>
> We currently run 3 copies of the same code, with essentially different
> urls.py and middlewares loaded, each one is used to run a variation of
> the site, for example www/mobile web and api, and have slightly
> different requirements, which are handled in middlewares, for example
> api calls are authenticated by access tokens and www/mobile use cookies.
>
> Ideally we'd love to consolidate our deployment into a single set of
> python web workers, but to date its been a little too complex to be
> worth it. We could however use the url dispatching patch and PR1 to
> write a middleware which could load in the necessary middleware stack
> based on the requirements of each request.
>
> So my preference therefore would be for PR1 as I can see a few edge
> cases it would enable solutions for.
I don't think it's worth penalizing the performance of the common case
in order to enable these edge cases. I think it should be possible to
add a "per-request middleware" feature atop PR #2 in a fully
backwards-compatible way without compromising performance in the common
case (essentially by compiling multiple alternative request-paths in the
handler instead of just one), but this should be handled as a separate
feature, built in a separate pull request; it's out of scope for DEP 5.
It solves a completely different problem from the one DEP 5 is aiming to
solve.
I also think there are fairly easy ways to solve your problem in the
meantime without the per-request middleware feature, for instance with
hybrid middleware that conditionally does something-vs-nothing (or
one-thing-vs-another) depending on some attribute attached to the request.
Carl
On 01/12/2016 04:23 PM, Mat Clayton wrote:
> +1 as well, this is really great work, on both patches!
>
> Modifying the middleware stack mid request would be very useful for us.
>
> We currently run 3 copies of the same code, with essentially different
> urls.py and middlewares loaded, each one is used to run a variation of
> the site, for example www/mobile web and api, and have slightly
> different requirements, which are handled in middlewares, for example
> api calls are authenticated by access tokens and www/mobile use cookies.
>
> Ideally we'd love to consolidate our deployment into a single set of
> python web workers, but to date its been a little too complex to be
> worth it. We could however use the url dispatching patch and PR1 to
> write a middleware which could load in the necessary middleware stack
> based on the requirements of each request.
>
> So my preference therefore would be for PR1 as I can see a few edge
> cases it would enable solutions for.
in order to enable these edge cases. I think it should be possible to
add a "per-request middleware" feature atop PR #2 in a fully
backwards-compatible way without compromising performance in the common
case (essentially by compiling multiple alternative request-paths in the
handler instead of just one), but this should be handled as a separate
feature, built in a separate pull request; it's out of scope for DEP 5.
It solves a completely different problem from the one DEP 5 is aiming to
solve.
I also think there are fairly easy ways to solve your problem in the
meantime without the per-request middleware feature, for instance with
hybrid middleware that conditionally does something-vs-nothing (or
one-thing-vs-another) depending on some attribute attached to the request.
Carl
Carl Meyer
Jan 18, 2016, 6:54:41 PM1/18/16
to django-d...@googlegroups.com
`TEMPLATE_*` vs `TEMPLATES`) is the appropriate level; likely to catch
an unintentional usage, but silence-able if the duplication is intentional.
Carl
Carl Meyer
Jan 18, 2016, 6:57:14 PM1/18/16
to django-d...@googlegroups.com
On 01/10/2016 04:54 PM, Raphaël Barrois wrote:
> I've got only one minor suggestion to the "backwards compatibility"
> section of the DEP.
>
>> It currently states that "If the ``MIDDLEWARE`` setting is provided
>> [...], the old ``MIDDLEWARE_CLASSES`` setting will be ignored.
>
> I suggest that, instead, this fails loudly if both ``MIDDLEWARE`` and
> ``MIDDLEWARE_CLASSES`` are set.
> This would prevent projects from keeping the two versions around and
> having to remember which one is currently in use.
Great suggestion, thanks. I've added it to the todo checklist on the PR:
> section of the DEP.
>
>> It currently states that "If the ``MIDDLEWARE`` setting is provided
>> [...], the old ``MIDDLEWARE_CLASSES`` setting will be ignored.
>
> I suggest that, instead, this fails loudly if both ``MIDDLEWARE`` and
> ``MIDDLEWARE_CLASSES`` are set.
> This would prevent projects from keeping the two versions around and
> having to remember which one is currently in use.
https://github.com/django/django/pull/5949
Carl
Carl Meyer
Jan 18, 2016, 9:20:03 PM1/18/16
to Django developers (Contributions to Django itself)
I've updated DEP 5 with a new round of clarifications and tweaks based on the most recent feedback: https://github.com/django/deps/compare/62b0...master
Carl
Carl
Tim Graham
May 4, 2016, 7:59:05 PM5/4/16
to Django developers (Contributions to Django itself)
I've been working on this and wanted to raise a couple points for discussion.
How should we treat error messages in place like system checks where we have phrases like "Edit your MIDDLEWARE_CLASSES" ... of course the check can easily check both MIDDLEWARE and MIDDLEWARE_CLASSES without much effort but should we make the error message "smart" and display the version of the setting that the user is using? Alternatively, we could always reference MIDDLEWARE (the non-deprecated version) or use some variation like "MIDDLEWARE(_CLASSES)" or "MIDDLEWARE/MIDDLEWARE_CLASSES" until the deprecation period ends.
Another point for discussion is whether we need to duplicate a lot of tests so we test that the middleware continue to work with both the old-style MIDDLEWARE_CLASSES and the new style MIDDLEWARE response handling. I guess a subclass of anything that uses @override_settings(MIDDLEWARE=...) that uses @override_settings(MIDDLEWARE_CLASSES=...) might work. Just putting it out there in case anyone has a better idea.
How should we treat error messages in place like system checks where we have phrases like "Edit your MIDDLEWARE_CLASSES" ... of course the check can easily check both MIDDLEWARE and MIDDLEWARE_CLASSES without much effort but should we make the error message "smart" and display the version of the setting that the user is using? Alternatively, we could always reference MIDDLEWARE (the non-deprecated version) or use some variation like "MIDDLEWARE(_CLASSES)" or "MIDDLEWARE/MIDDLEWARE_CLASSES" until the deprecation period ends.
Another point for discussion is whether we need to duplicate a lot of tests so we test that the middleware continue to work with both the old-style MIDDLEWARE_CLASSES and the new style MIDDLEWARE response handling. I guess a subclass of anything that uses @override_settings(MIDDLEWARE=...) that uses @override_settings(MIDDLEWARE_CLASSES=...) might work. Just putting it out there in case anyone has a better idea.
charettes
May 4, 2016, 10:57:36 PM5/4/16
to Django developers (Contributions to Django itself)
Hi Tim,
I think we should favor displaying a message in accordance with the setting the user is using as it will make the transition less confusing. In the case of the documented check message I think using the form "MIDDLEWARE/MIDDLEWARE_CLASSES" would make it easier to read then mentioning the two possible variants. We already alter the document messages anyway to account for their dynamic nature.
In the case of the tests I believe both code path should continue to be tested. From the top of my head I can't think of an alternative to subclasses using @override_settings. I suggest we make the *legacy* tests class extend the MIDDLEWARE using test class and not the other way around as it will make the MIDDLEWARE_CLASSES code removal clearer.
Simon
I think we should favor displaying a message in accordance with the setting the user is using as it will make the transition less confusing. In the case of the documented check message I think using the form "MIDDLEWARE/MIDDLEWARE_CLASSES" would make it easier to read then mentioning the two possible variants. We already alter the document messages anyway to account for their dynamic nature.
In the case of the tests I believe both code path should continue to be tested. From the top of my head I can't think of an alternative to subclasses using @override_settings. I suggest we make the *legacy* tests class extend the MIDDLEWARE using test class and not the other way around as it will make the MIDDLEWARE_CLASSES code removal clearer.
Simon
Carl Meyer
May 6, 2016, 7:59:38 PM5/6/16
to django-d...@googlegroups.com
I agree with Simon on both counts. We do usually continue to test
deprecated code paths until they are removed, but I also think the
duplication in cases of tests overriding MIDDLEWARE_CLASSES might not be
necessary in _all_ cases; I think some discretion could be used
depending on to what extent the middleware is incidental to the tests vs
the direct subject of the test. But it might be simpler to just do them
all than to make that determination.
Carl
> <https://github.com/django/deps/compare/62b0...master>
>
> Carl
> <https://groups.google.com/d/msgid/django-developers/eb1cf3f4-c021-40f6-be65-35427b2bf5c5%40googlegroups.com?utm_medium=email&utm_source=footer>.
deprecated code paths until they are removed, but I also think the
duplication in cases of tests overriding MIDDLEWARE_CLASSES might not be
necessary in _all_ cases; I think some discretion could be used
depending on to what extent the middleware is incidental to the tests vs
the direct subject of the test. But it might be simpler to just do them
all than to make that determination.
Carl
>
> 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-develop...@googlegroups.com
> <mailto:django-develop...@googlegroups.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-develop...@googlegroups.com
> To post to this group, send email to django-d...@googlegroups.com
> <mailto:django-d...@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/eb1cf3f4-c021-40f6-be65-35427b2bf5c5%40googlegroups.com
> To view this discussion on the web visit
> <https://groups.google.com/d/msgid/django-developers/eb1cf3f4-c021-40f6-be65-35427b2bf5c5%40googlegroups.com?utm_medium=email&utm_source=footer>.
Tim Graham
May 7, 2016, 10:27:27 AM5/7/16
to Django developers (Contributions to Django itself)
Thanks, I have what I hope is a minimally mergable patch here: https://github.com/django/django/pull/6501
As noted on the PR, there a more things that should be before the 1.10 feature freeze but I'm hoping I can ticket them out and get some help from the community after merging this first step so that I can continue spending most of my time reviewing patches.
As noted on the PR, there a more things that should be before the 1.10 feature freeze but I'm hoping I can ticket them out and get some help from the community after merging this first step so that I can continue spending most of my time reviewing patches.
> <mailto:django-developers+unsub...@googlegroups.com>.
Carl Meyer
May 10, 2016, 5:37:26 PM5/10/16
to django-d...@googlegroups.com
On 05/07/2016 08:27 AM, Tim Graham wrote:
> Thanks, I have what I hope is a minimally mergable patch here:
> https://github.com/django/django/pull/6501
Thanks Tim (and Florian for all the previous work on this patch). I've
> Thanks, I have what I hope is a minimally mergable patch here:
> https://github.com/django/django/pull/6501
updated the DEP with a couple minor changes to reflect the latest
learnings from the implementation; you can see the latest changes at
https://github.com/django/deps/compare/763530e1a9...master
I reviewed this thread and I think the response was pretty much
positive; I didn't see any outstanding unaddressed concerns. If you have
any, please let me know! I plan to ask the technical board to approve
DEP 5 shortly.
> As noted on the PR, there a more things that should be before the 1.10
> feature freeze but I'm hoping I can ticket them out and get some help
> from the community after merging this first step so that I can continue
> spending most of my time reviewing patches.
the decorator_from_middleware update tomorrow.
I'm not entirely sure that a graphic is still needed with the new
system; its behavior is simpler than the old. But if a motivated and
appropriately skilled party provides a useful graphic, that'd be great
of course!
Carl
> > <mailto:django-develop...@googlegroups.com
> <javascript:>>.
> > To post to this group, send email to django-d...@googlegroups.com
> <javascript:>
> > <mailto:django-d...@googlegroups.com <javascript:>>.
> <https://groups.google.com/group/django-developers>.
> > To view this discussion on the web visit
> >
> https://groups.google.com/d/msgid/django-developers/eb1cf3f4-c021-40f6-be65-35427b2bf5c5%40googlegroups.com
> <https://groups.google.com/d/msgid/django-developers/eb1cf3f4-c021-40f6-be65-35427b2bf5c5%40googlegroups.com>
>
> >
> <https://groups.google.com/d/msgid/django-developers/eb1cf3f4-c021-40f6-be65-35427b2bf5c5%40googlegroups.com?utm_medium=email&utm_source=footer
> >
> https://groups.google.com/d/msgid/django-developers/eb1cf3f4-c021-40f6-be65-35427b2bf5c5%40googlegroups.com
> <https://groups.google.com/d/msgid/django-developers/eb1cf3f4-c021-40f6-be65-35427b2bf5c5%40googlegroups.com>
>
> >
> <https://groups.google.com/d/msgid/django-developers/eb1cf3f4-c021-40f6-be65-35427b2bf5c5%40googlegroups.com?utm_medium=email&utm_source=footer>>.
>
> > For more options, visit https://groups.google.com/d/optout
> <https://groups.google.com/d/optout>.
>
> > For more options, visit https://groups.google.com/d/optout
>
> --
> 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-develop...@googlegroups.com
> <mailto:django-develop...@googlegroups.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-develop...@googlegroups.com
> To post to this group, send email to django-d...@googlegroups.com
> <mailto:django-d...@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/04f2e0f0-aa41-4a24-abea-85d8c292bcc5%40googlegroups.com
> <mailto:django-d...@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/04f2e0f0-aa41-4a24-abea-85d8c292bcc5%40googlegroups.com?utm_medium=email&utm_source=footer>.
Carl Meyer
May 11, 2016, 1:19:11 PM5/11/16
to django-d...@googlegroups.com
On 05/10/2016 03:37 PM, Carl Meyer wrote:
> I've
> I've
> updated the DEP with a couple minor changes to reflect the latest
> learnings from the implementation; you can see the latest changes at
> https://github.com/django/deps/compare/763530e1a9...master
Better version of this link (to exclude more recent changes to other
> learnings from the implementation; you can see the latest changes at
> https://github.com/django/deps/compare/763530e1a9...master
DEPs in master):
https://github.com/django/deps/compare/763530e1a9...77c93d46baf
Carl
Carl Meyer
May 17, 2016, 12:14:00 AM5/17/16
to Django developers (Contributions to Django itself)
The technical board has approved DEP 5, and since the patch [1] is also ready to go, I moved it straight to Final state. Thanks to everyone who contributed to the discussion, and especially to Florian and Tim for the patch.
Carl
Reply all
Reply to author
Forward
0 new messages