On 05/23/2011 11:31 AM, Luke Plant wrote: > 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: > > http://code.djangoproject.com/attachment/ticket/16004/16004.fix.alternative.diff > > 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: > > http://docs.djangoproject.com/en/dev/ref/contrib/csrf/#view-needs-protection-for-one-path 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 ;-) Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
