Hi Tobias, On 06/21/2016 06:22 AM, Tobias McNulty wrote: > On Mon, Jun 20, 2016 at 10:51 AM, Carl Meyer <c...@oddbird.net > <mailto:c...@oddbird.net>> wrote >> Possible disadvantages of this approach: > > <snip> > >> 3. The new behavior may surprise some people accustomed to the old >> behavior, since it means the effect of the middleware decorator won't >> occur immediately where the decorator is applied. E.g. if you apply a >> middleware decorator to a view, and then another custom view decorator >> outside that, the custom view decorator won't see the effects of the >> middleware decorator in the actual response (it would only see a new >> entry in view_func._extra_middleware). > > This one seems like the gotcha to me.
Yeah, I agree. > What if, for example, I have a > view decorator whose effects I specifically don't want to cache, but I > do want to cache the underlying view; is it fair to require that the > person write a middleware and then turn it into a decorator to be able > to do that? Caching happens to be a bad example, because AFAICT nobody should ever use the cache_page decorator (or cache full responses right after the view function, as you describe here) due to https://code.djangoproject.com/ticket/15855 -- unless I suppose they are pre-adding all the correct Vary headers themselves. But I think the general point is a good one nonetheless. > Are we effectively saying, "for view decorators to behave > the way you might expect, implement them as middleware"? It seems odd, > to me at least, that I should care what the underlying implementation of > a decorator is before I use it. It also violates the 'strict in/out > layering' goal, albeit for decorators instead of middleware. (A similar > argument could be said of exceptions -- what if I want to trap an > exception raised by a middleware-turned-decorator?) > > It might be okay if the decorators themselves were explicit about what > they were doing, for example @cache_page(3600) might become: > > @add_middleware(CacheMiddleware, cache_timeout=3600) Yes, I did think about this. It makes it more difficult to use, but it does give a clearer picture of what's actually happening. We aren't directly decorating the view function, we're just annotating it with an extra per-view middleware. > However, that's probably a bigger and unnecessary change. > > Would it be possible to implement a combination of the approaches, i.e., > make the delay in applying the middleware-turned-decorator the exception > rather than the rule, perhaps only for TemplateResponses and > specifically for the purpose of supporting a deprecation plan? And then, > long-term, leave it up to middleware/decorator authors & users to decide > how best to implement/layer them, being explicit about the implications > of rendering the response or perhaps more appropriately, "not rendering > if you can avoid it" (i.e., your first strategy)? I don't think it's worth doing this just for a deprecation path. If we're OK in the long term with making middleware-turned-decorator authors responsible for correctly handling TemplateResponse themselves, we should just go there right away via https://github.com/django/django/pull/6765 -- we can take care of backwards-compatibility quite nicely there in the transition mixin. In fact I'd say the overall backward-compatibility of this approach (#6765) is better than that of #6805 (the extra-middleware-annotation approach), due to converting all our builtin decorators-from-middleware to the new style in the latter. The downside of #6765 is that the boilerplate for correctly handling TemplateResponse is pretty ugly [1], and any middleware that accesses response.content would pretty much need to do that, in case it might sometime be used in decorator_from_middleware. To be honest though, I'm still not sure that isn't our best option. Most middleware I've written don't access response.content, they set headers, so they'd be unaffected. Some of Django's built-in ones do touch content, but we can deal with the ugly TemplateResponse boilerplate in our own middleware. Carl [1] https://github.com/carljm/django/blob/7d999844bd4b43b82a472634074156c24a4fc7cb/docs/topics/http/middleware.txt#L304 -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@googlegroups.com. Visit this group at https://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/576970EC.50704%40oddbird.net. For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: OpenPGP digital signature
