When TemplateResponse and process_template_response were introduced, decorator_from_middleware was not updated to match. This produces at least this problem:
The csrf_protect decorator, which is just decorator_from_middleware(CsrfViewMiddleware), does not work correctly on its own. Because the template isn't rendered until after the decorator has fully run, get_token() is not called before CsrfViewMiddleware.process_response, which means the latter does not realise it has to send the cookie, and doesn't do so.
An example view which demonstrates the problem is the login view, which now uses TemplateResponse (since April 22nd - had it been earlier, I would have found this sooner).
I imagine there are others, especially with caching decorators, but I'm surprised this hasn't been caught earlier.
However, I also imagine there will be problems if we change this, or at least we could end up removing some of the usefulness of TemplateResponse, since any decorator based on decorator_from_middleware will cause the template to be rendered.
I've written a patch (attached), and running the test suite with it produces no errors, but I'm still a bit nervous about it. Any input would be appreciated.
Regards,
Luke
-- Parenthetical remarks (however relevant) are unnecessary
OK, I've thought about this a bit more, and I think we have some nasty problems.
First, existing decorators:
1) It looks like the cache_page decorator will actually work with TemplateResponse, but only because the cache middleware special-cases responses with a 'render' method.
This is cheating really - if we had to patch this middleware to get it to handle TemplateResponse, then there will be many other 3rd party pieces of middleware that will need similar fixes, yet we gave no-one any warning of this in the 1.3 release of this fairly major backwards incompatibility.
2) crsf_protect, and almost certainly conditional_page and gzip_page, are broken for TemplateResponse objects (to a greater or lesser degree), due to decorator_from_middleware not rendering TemplateResponse objects before passing them to the process_response method.
So, we need to fix decorator_from_middleware. I think that is unavoidable.
Once we do that, and even before we do it in the case of cache_page, we have to face the fact that these decorators, and possibly others like them, cause TemplateResponse objects to be rendered. This means that the usefulness of TemplateResponse and 'template response middleware' are seriously reduced:
These features have the stated purpose of allowing a response to be altered by middleware or decorators. However, if a decorator causes the template to be rendered, then such alterations will not happen.
This is a serious issue, because decorators like these are often applied on an ad-hoc basis, without thinking that they are going to change things like the body of the response.
So, if we need to cache one page, on a site without global caching, the obvious thing to do is to add the cache_page decorator. Unfortunately, if you were actually using a template response middleware, you'll almost certainly have breakage. It is really bad that adding cache_page can change what would actually be rendered, and this could be extremely hard to debug. Similarly with the other affected decorators.
I can see 2 ways that TemplateResponses are still useful:
1) if you have 'sub views', which are private and are not going to be decorated with any of these things, and want to return responses to the main view for finalising, and you only put decorators on the main view.
2) if you provide re-usable views in a re-usable app, which have deliberately have no decorators applied, but come with instructions about the decorators that have to be applied to them.
However, the 2nd of these could make things pretty icky.
A case in point is django.contrib.auth.views.login. Once we fix csrf_protect, the TemplateResponse returned will be immediately baked, and so it is pointless using it in the first place. If we don't put csrf_protect on it, we've got a security problem - we agreed that with re-usable apps, we don't want to rely on people using CsrfViewMiddleware, but we also don't want to rely on people reading instructions like "make sure you apply csrf_protect to this view".
The only solution I can see that isn't awful is providing two views - a default and documented one that has all the right decorators applied, and one that is obviously 'use at own risk', called something like 'login_unsafe', for which we document the extra things you need.
I can see even less room for process_template_response - it just isn't useful to add a middleware for the sole purpose of messing with the body of any response, only to find that in many cases you can't mess with them after all, since the developer had to do something like add caching to a particular view etc.
So where does this leave us? I don't know. I did have some bad feelings about TemplateResponse and the idea of lazy responses, but didn't voice them at the time because I didn't have anything but a vague feeling of unease (which is not something you can either defend or argue against), and I wasn't involved in reviewing that feature much.
Luke
-- Parenthetical remarks (however relevant) are unnecessary
> The only solution I can see that isn't awful is providing two views - a > default and documented one that has all the right decorators applied, > and one that is obviously 'use at own risk', called something like > 'login_unsafe', for which we document the extra things you need.
> I can see even less room for process_template_response - it just isn't > useful to add a middleware for the sole purpose of messing with the body > of any response, only to find that in many cases you can't mess with > them after all, since the developer had to do something like add caching > to a particular view etc.
> So where does this leave us? I don't know. I did have some bad feelings > about TemplateResponse and the idea of lazy responses, but didn't voice > them at the time because I didn't have anything but a vague feeling of > unease (which is not something you can either defend or argue against), > and I wasn't involved in reviewing that feature much.
Yeah, this is bad. I don't have a lot to add at the moment, except to also point out #15855 - cache_page as a decorator breaks proper setting of Vary headers.
The reason I bring it up here is just to point out another case, unrelated to TemplateResponse, where we promote the idea that you can use a decorator as a view-specific equivalent to a middleware, but doing so breaks the semantics of the original middleware in subtle but significant ways (because it changes the order things happen in).
Not sure what that implies, except that maybe decorator_from_middleware and the pattern it encourages is ultimately at least as problematic as TemplateResponse...
> Yeah, this is bad. I don't have a lot to add at the moment, except to > also point out #15855 - cache_page as a decorator breaks proper setting > of Vary headers.
> The reason I bring it up here is just to point out another case, > unrelated to TemplateResponse, where we promote the idea that you can > use a decorator as a view-specific equivalent to a middleware, but doing > so breaks the semantics of the original middleware in subtle but > significant ways (because it changes the order things happen in).
> Not sure what that implies, except that maybe decorator_from_middleware > and the pattern it encourages is ultimately at least as problematic as > TemplateResponse...
Yeah, I guess I was seeing TemplateResponse as the culprit, on the basis of Last In First Out, but maybe it's decorators we need to be worrying about.
I've actually documented a specific instance of #15855 in the CRSF docs
And while we're on the subject, another problem with decorators vs. middleware:
A stack of decorators can behave differently from the corresponding stack of middleware, due to subtle differences in the ordering of method calls e.g. [m1.process_request, m2.process_request, m1.process_view, m2.process_view] vs [m1.process_request, m1.process_view, m2.process_request, m2.process_view]
Regards,
Luke
-- Parenthetical remarks (however relevant) are unnecessary
I actually faced a problem similar to this when I ported CBV from 1.3
to 1.2, and then tried to use the TemplateResponse with a couple of
middleware decorators that I created from some custom middleware
classes. If I recall properly, I was also having backwards
incompatibility problems with some other middleware classes that I had
installed.
The solution for me was actually very simple: instead of raising a
ContentNotRenderedError on line 110 of django/template/response.py, I
modified the code of _get_content() to call self.render(), that way
making a TemplateResponse look just like a standard response with a
static "content" field. Doing it this way still allows
TemplateResponse to be useful, it just requires you to be careful how
you order middleware classes so that the template context is updated
before request.content is accessed (i.e. at the very front). It's a
small race condition, but not inconsistent with the other ordering
issues that go along with using middleware.
That being said, I don't know if the bug I encountered was exactly the
same as what you are describing, I can't quite recall the details, but
I just wanted to throw this out there anyway. This may not be a
complete or ideal solution, but if it works, it seems like a better
option than changing the middleware API--and better than making
backwards-incompatible changes to decorator_from_middleware.
> These features have the stated purpose of allowing a response to be
> altered by middleware or decorators. However, if a decorator causes the
> template to be rendered, then such alterations will not happen.
> This is a serious issue, because decorators like these are often applied
> on an ad-hoc basis, without thinking that they are going to change
> things like the body of the response.
> So, if we need to cache one page, on a site without global caching, the
> obvious thing to do is to add the cache_page decorator. Unfortunately,
> if you were actually using a template response middleware, you'll almost
> certainly have breakage. It is really bad that adding cache_page can
> change what would actually be rendered, and this could be extremely hard
> to debug. Similarly with the other affected decorators.
I just wanted to amend my post to say that when I said that this was a
"small race condition" (is it a race condition? maybe that's the wrong
term as concurrency doesn't come into play), I hadn't thought this
through. Yeah, the usefulness of the TemplateResponse is much reduced
when you have to worry about decorators accessing the content of the
response. But that should be obvious, right, at least for caching?
Certainly if you cache a response before you modify it either the
cache will have to be wrong or the modifications have to will fail.
On May 11, 7:00 pm, Luke Plant <L.Plant...@cantab.net> wrote:
> Yeah, I guess I was seeing TemplateResponse as the culprit, on the basis
> of Last In First Out, but maybe it's decorators we need to be worrying
> about.
> I've actually documented a specific instance of #15855 in the CRSF docs
A possible fix for decorator_from_middleware just occurred to me this
morning. What if, instead of running process_response immediately, it
annotated the response with metadata saying "please run this
process_response middleware on me!" And then the running of that
middleware actually occurred later.
There'd be a couple possible variants of "later": right before your
actual middleware runs, right after, or somewhere in the middle (you
pick when). This latter option would have to be controlled by
something like a "ViewDecoratorMiddleware" which you put into your
MIDDLEWARE_CLASSES. I think this control might be necessary to get the
ordering right in some cases; there are other middlewares that should
always be first/last, so I don't think either "before all" or "after
all" would suffice. This ViewDecoratorMiddleware would also give us a
backwards-compatible way to introduce the change: if you've got this
ViewDecoratorMiddleware in your MIDDLEWARE_CLASSES,
decorator_from_middleware annotates. If you don't, it sticks with
current behavior (and raises a PendingDeprecationWarning).
I think we could even do something similar for process_view, if the
decorator annotates the actual view function with which additional
process_view middlewares should be run on it. Process_request can't be
helped; since request middleware can change the urlconf, thus
resolving to a different view, there's no way we can run request
middleware when it ought to have been run, if its applied as a
decorator. In any case, though, I think the most serious problems here
have to do with when process_response runs; process_request and
decorator_from_middleware doesn't seem as problematic.
It seems to me that this approach could potentially fix the cache_page/
Vary headers problem and the CSRF issues, make both of them work
nicely with TemplateResponse, and generally make
decorator_from_middleware less problematic.
Let me get a bit crazy now: If we want to fix the problem even more
fully, get rid of ViewDecoratorMiddleware, and allow full ordering
control of when the various middlewares-as-decorators are applied,
another option would be a way to include a specific middleware in your
MIDDLEWARE_CLASSES, but annotated as "decorator-only". So you'd go
ahead and include e.g. CsrfViewMiddleware in MIDDLEWARE_CLASSES but
marked decorator-only, and then it would only apply to requests/
responses where the resolved view has been annotated with the
csrf_protect decorator. Again, for backwards-compatibility,
decorator_from_middleware would check to see if you have the given
middleware in your MIDDLEWARE_CLASSES, marked decorator-only: if you
do, it would annotate, if you don't, it would run eagerly as it does
now.
Thoughts? Does something along these lines seem worth pursing as a
fix? Or am I off my rocker?
I think your core idea is quite interesting. I haven't had time to think through all the implications.
It does make the result of what is returned from view functions rather obscure, and this could affect things like testing.
For example, the new tests I wrote for decorator_from_middleware, which pass with my first patch on #16004, will not pass with this implementation. This means the semantics are quite difference. It also means that tests using RequestFactory could be broken, because they won't pass through the machinery of request handling.
However, with one modification, we can get something quite a bit better. Basically, if it is a TemplateResponse, we defer the process_response handling as you say. Otherwise, we do it the way we currently do it.
In fact, we *almost* have machinery in place to do this - 'TemplateResponse.add_post_render_callback' - and I've got a patch based on that which works. The only problem is that currently 'process_response' is allowed to return a completely new response object, whereas that is not supported with add_post_render_callback.
That would be easy to change, however - or we could add a similar hook which does support it. Either way, it would require changing code like this:
response.render()
to
response = response.render()
This way, the only change people have to cope with in tests is adding 'response = response.render()' to things that have 'render', rather than having to invoke some other machinery to get the expected result - TemplateResponse has all the machinery itself. Since they already have to invoke 'render' for anything that has changed to TemplateResponse, this change adds no extra burden.
(BTW, we do need to document the change introduced by [16087] in the 1.4 release notes as a breaking change - any tests against customized admin views that use RequestFactory will currently break if they don't add '.render()' calls.)
Putting all these things together, I've uploaded a new patch to #16004:
It is not nearly as invasive as the other changes you suggested. However, it does fix the failing tests in my project (i.e. fixes csrf_protect), and leaves TemplateResponse objects unrendered as long as possible, without having to change one line of my own code/tests. And the implementation doesn't make me puke either.
I agree that we ought to be doing something about middleware ordering and things like that, but perhaps that is something for Django 2000 - when we can sit down and work out how everything should fit together.
One little note while I remember - I'm really not happy about decorator functions looking at settings to see how they should work - this is fixing fragility by adding more fragility. We want to be getting rid of this kind of thing, not adding more of it. There was also the general assumption of just one view being involved, one response object. We don't want to break code that is slightly more inventive, like this:
(Sorry for vague criticisms, I don't have more time to spend on this today).
Regards,
Luke
-- "The first ten million years were the worst. And the second ten million, they were the worst too. The third ten million, I didn't enjoy at all. After that I went into a bit of a decline." (Marvin the paranoid android)
> I think your core idea is quite interesting. I haven't had time to think > through all the implications.
> It does make the result of what is returned from view functions rather > obscure, and this could affect things like testing.
I don't think it's any more obscure - an HttpResponse is still an HttpResponse - just different. It changes the semantics of decorator_from_middleware so you can't expect the middlewares to be applied immediately without further response handling (more like actual middleware).
> For example, the new tests I wrote for decorator_from_middleware, which > pass with my first patch on #16004, will not pass with this > implementation. This means the semantics are quite difference. It also > means that tests using RequestFactory could be broken, because they > won't pass through the machinery of request handling.
Yes, the semantics would be different, and this would break some current tests that assume the current semantic of immediate application as part of the view function call.
The entire issue, though, is that the current semantic of decorator_from_middleware is broken, because middlewares can't reliably be converted into something that runs immediately rather than in the middleware phase, and still work properly.
One other option, I suppose, would be to introduce this as a new function, delayed_decorator_from_middleware, and convert cache_page and the csrf stuff to use it, to un-break them, without changing the current behavior of decorator_from_middleware.
> However, with one modification, we can get something quite a bit better. > Basically, if it is a TemplateResponse, we defer the process_response > handling as you say. Otherwise, we do it the way we currently do it.
> In fact, we *almost* have machinery in place to do this - > 'TemplateResponse.add_post_render_callback' - and I've got a patch based > on that which works. The only problem is that currently > 'process_response' is allowed to return a completely new response > object, whereas that is not supported with add_post_render_callback.
> That would be easy to change, however - or we could add a similar hook > which does support it. Either way, it would require changing code like this:
> response.render()
> to
> response = response.render()
> This way, the only change people have to cope with in tests is adding > 'response = response.render()' to things that have 'render', rather than > having to invoke some other machinery to get the expected result - > TemplateResponse has all the machinery itself. Since they already have > to invoke 'render' for anything that has changed to TemplateResponse, > this change adds no extra burden.
> (BTW, we do need to document the change introduced by [16087] in the 1.4 > release notes as a breaking change - any tests against customized admin > views that use RequestFactory will currently break if they don't add > '.render()' calls.)
> Putting all these things together, I've uploaded a new patch to #16004:
> It is not nearly as invasive as the other changes you suggested. > However, it does fix the failing tests in my project (i.e. fixes > csrf_protect), and leaves TemplateResponse objects unrendered as long as > possible, without having to change one line of my own code/tests. And > the implementation doesn't make me puke either.
> I agree that we ought to be doing something about middleware ordering > and things like that, but perhaps that is something for Django 2000 - > when we can sit down and work out how everything should fit together.
This does seem like a relatively nice solution for TemplateResponse and decorator_from_middleware. Isn't the patch missing a modification to BaseHandler to do "response = response.render()" rather than just "response.render()"?
Unfortunately, this doesn't do anything for the problem with cache_page and Vary headers, which isn't related to TemplateResponse. I'm not sure we can wait for Django 2000 to find a fix for that.
> One little note while I remember - I'm really not happy about decorator > functions looking at settings to see how they should work - this is > fixing fragility by adding more fragility. We want to be getting rid of > this kind of thing, not adding more of it.
I agree, of course. I only proposed that as a temporary deprecation-path check, not a permanent behavior.
There was also the general
> assumption of just one view being involved, one response object. We > don't want to break code that is slightly more inventive, like this:
I don't like seeing that recommended in the documentation. For one thing, it's highly dependent on detailed knowledge of the implementation of csrf_protect and csrf_exempt. The response is still going to pass through both of those decorators; a naive reader would be as likely to assume "last-one-wins" as anything else. And I find it quite hard to read, compared to just having a function available that takes the request as argument and performs the CSRF check directly and immediately.
> (Sorry for vague criticisms, I don't have more time to spend on this today).
No problem, critiques appreciated. And - nor do I ;-)
>> It is not nearly as invasive as the other changes you suggested. >> However, it does fix the failing tests in my project (i.e. fixes >> csrf_protect), and leaves TemplateResponse objects unrendered as long as >> possible, without having to change one line of my own code/tests. And >> the implementation doesn't make me puke either.
Just re-read this, and realised it might come over as suggesting that your idea *did* make me puke, which isn't what I was saying at all - I was saying that this (limited) implementation of what was essentially your idea turns out quite nicely.
>> I agree that we ought to be doing something about middleware ordering >> and things like that, but perhaps that is something for Django 2000 - >> when we can sit down and work out how everything should fit together.
> This does seem like a relatively nice solution for TemplateResponse and > decorator_from_middleware. Isn't the patch missing a modification to > BaseHandler to do "response = response.render()" rather than just > "response.render()"?
Yep, good catch.
> Unfortunately, this doesn't do anything for the problem with cache_page > and Vary headers, which isn't related to TemplateResponse. I'm not sure > we can wait for Django 2000 to find a fix for that.
OK, but I think it is less pressing. #16004 is significant regression introduced in 1.3, which is now causing more bugs in trunk due to actual use of TemplateResponse. #15855 has existed since the dawn of time (0.91 at least, as far as I can tell from eye-balling the code).
I'm happy to wait around for a fuller solution to be thrashed out, but I worried that the pursuit of an ideal might make it actually more fragile than it is. I think long term we should move towards a single processing phase (e.g. decorators always just mark for action later). But to attempt that in a backwards compatible way requires making functions that are eager now lazy (in effect). That addition of laziness is very likely to add more bugs - as the addition of TemplateResponse highlighted.
decorator_from_middleware is broken, but it has been broken for a long time, and its very likely that a lot is depending on its eager behaviour.
>> There was also the general >> assumption of just one view being involved, one response object. We >> don't want to break code that is slightly more inventive, like this:
> I don't like seeing that recommended in the documentation. For one > thing, it's highly dependent on detailed knowledge of the implementation > of csrf_protect and csrf_exempt. The response is still going to pass > through both of those decorators; a naive reader would be as likely to > assume "last-one-wins" as anything else. And I find it quite hard to > read, compared to just having a function available that takes the > request as argument and performs the CSRF check directly and immediately.
It doesn't depend on anything in the implementation, as far as I can see. csrf_protect protects a view from CSRF attacks. csrf_exempt exempts a view from the checks the CSRF middleware performs. That is how they are documented, and that's all you need to know. That section of the documentation is pointing out a solution that you could deduce simply from the documentation of the parts.
I guess whether it is obvious depends on what your model is of how these work. I'd argue that by far the most obvious model is that the csrf_protect does what it says eagerly i.e. immediately, especially given that is supposed to work when the middleware is absent.
Regards,
Luke
-- "The first ten million years were the worst. And the second ten million, they were the worst too. The third ten million, I didn't enjoy at all. After that I went into a bit of a decline." (Marvin the paranoid android)
> On 23/05/11 18:33, Carl Meyer wrote: >>> It is not nearly as invasive as the other changes you suggested. >>> However, it does fix the failing tests in my project (i.e. fixes >>> csrf_protect), and leaves TemplateResponse objects unrendered as long as >>> possible, without having to change one line of my own code/tests. And >>> the implementation doesn't make me puke either.
> Just re-read this, and realised it might come over as suggesting that > your idea *did* make me puke, which isn't what I was saying at all - I > was saying that this (limited) implementation of what was essentially > your idea turns out quite nicely.
I didn't read it that way, but thanks for the clarification ;-)
>> Unfortunately, this doesn't do anything for the problem with cache_page >> and Vary headers, which isn't related to TemplateResponse. I'm not sure >> we can wait for Django 2000 to find a fix for that.
> OK, but I think it is less pressing. #16004 is significant regression > introduced in 1.3, which is now causing more bugs in trunk due to actual > use of TemplateResponse. #15855 has existed since the dawn of time (0.91 > at least, as far as I can tell from eye-balling the code).
> I'm happy to wait around for a fuller solution to be thrashed out, but I > worried that the pursuit of an ideal might make it actually more fragile > than it is. I think long term we should move towards a single processing > phase (e.g. decorators always just mark for action later). But to > attempt that in a backwards compatible way requires making functions > that are eager now lazy (in effect). That addition of laziness is very > likely to add more bugs - as the addition of TemplateResponse highlighted.
> decorator_from_middleware is broken, but it has been broken for a long > time, and its very likely that a lot is depending on its eager behaviour.
Yes, fair enough. I think your current patch is the best plan we have for now, and it wouldn't make sense to hold it; I'll keep noodling on the longer-term options.
>>> There was also the general >>> assumption of just one view being involved, one response object. We >>> don't want to break code that is slightly more inventive, like this:
>> I don't like seeing that recommended in the documentation. For one >> thing, it's highly dependent on detailed knowledge of the implementation >> of csrf_protect and csrf_exempt. The response is still going to pass >> through both of those decorators; a naive reader would be as likely to >> assume "last-one-wins" as anything else. And I find it quite hard to >> read, compared to just having a function available that takes the >> request as argument and performs the CSRF check directly and immediately.
> It doesn't depend on anything in the implementation, as far as I can > see. csrf_protect protects a view from CSRF attacks. csrf_exempt exempts > a view from the checks the CSRF middleware performs. That is how they > are documented, and that's all you need to know. That section of the > documentation is pointing out a solution that you could deduce simply > from the documentation of the parts.
> I guess whether it is obvious depends on what your model is of how these > work. I'd argue that by far the most obvious model is that the > csrf_protect does what it says eagerly i.e. immediately, especially > given that is supposed to work when the middleware is absent.
Yes, you're right here as well. I still think that pattern is ugly and hard to read, but given the documented behavior of those decorators, its function is clear.
