Feature request: New middleware method for "universal" decoration

[Simon Percivall]

If this has already been discussed/rejected/accepted, sorry, I did a search and found nothing.

There are several snippets and a number of methods to enable "universal" decorations of views in Django, but none of them feels really natural. Also, being able to keep this class of code in one place makes it easier to find.

Basically, what I propose is adding a middleware method, e.g. "decorate_view(...)", which returns the view function, decorated or not, or completely replaced. This new function would then be used as the "callback" before "process_view" is called in django/core/handlers/base.py.

It would enable an easier way to universally, dynamically wrap views with for instance context managers, a process which doesn't really exist today: To do this with a view middleware would mean not being able to use another view middleware, to use process_request & process_response would mean not being certain "__exit__" was called.

[gavi...@gmail.com]

This topic was also discussed during the deprecation of TransactionMiddleware and the introduction of ATOMIC_REQUESTS. The existing middleware semantics can't guarantee that  __exit__ (during process_response) will get called no matter what, necessitating the setting that invokes BaseHandler.make_view_atomic. make_view_atomic implements basically what you're suggesting, for one specific case (DB/ORM code in the front controller, ick!).

I can't find the previous discussion, does anyone have a link?

[Anssi Kääriäinen]

On Tuesday, October 15, 2013 8:23:38 AM UTC+3, gavi...@gmail.com wrote:

This topic was also discussed during the deprecation of TransactionMiddleware and the introduction of ATOMIC_REQUESTS. The existing middleware semantics can't guarantee that  __exit__ (during process_response) will get called no matter what, necessitating the setting that invokes BaseHandler.make_view_atomic. make_view_atomic implements basically what you're suggesting, for one specific case (DB/ORM code in the front controller, ick!).

I can't find the previous discussion, does anyone have a link?

Some time ago I needed to fix a TransactionMiddleware bug. A connection's transaction state wasn't reset properly between requests. I remember being very frustrated about how current middleware implementation works. The basic problem was that if a middleware's process_* method throws an exception, there is no guarantee that process_exception() methods are called. Thus TransactionMiddleware didn't get to reset the transaction state in all cases, and this led to leaked transaction state. For more details see https://code.djangoproject.com/ticket/19707.

A call-through middleware will guarantee that the semantics are easy to understand. Something like:
    def process(self, next, request):
         # The next parameter is an object that can be called. So, transaction middleware
         # would be basically this:
         with transaction.atomic():
             return next(request)

The next() call could end up calling the next middleware's process(), or if the current middleware is last in stack, then it would result in calling the resolved view method. What happens is abstracted away, so the middleware doesn't need to care. All it needs to know is that it can return next(request), any HttpResponse or raise an exception.

I think the semantics of such a method would be better than what we currently offer. For example, this is how you could implement what process_request(), process_response() and process_exception() do, but in a more robust way:

class CallProcessAll(object):
    def process(self, next, request):
        try:
             response = self.process_request(request)
             if response is None:
                 response = next(request)
        except Exception as e:
             response = self.process_exception(request, e)
             if response is None:
                 raise
        return self.process_response(request, response)

I am not suggesting using the above process() example in Django. The semantics for process_exception() in particular are backwards incompatible. It is just an example that you can achieve what is currently available with process_request(), process_response() and process_exception() methods, but with obvious semantics.

As said, the basic idea is to process the request by calling through the stack. It seems you can achieve the same with the suggested decorate_view() method. Wrap the view in decorate_view(), and to the same things you would do inside process() in the wrapper. But to me it seems that adds one step of indirection where it isn't needed. Also, a call-through middleware is conceptually easier to understand than a decorator (try-except-finally is basics, while decorators are a bit more advanced). Also, it isn't entirely clear what should happen in case decorate_view() itself raises an exception.

 - Anssi

[Shai Berger]

On Tuesday 15 October 2013 11:49:32 Anssi Kääriäinen wrote:
> On Tuesday, October 15, 2013 8:23:38 AM UTC+3, gavi...@gmail.com wrote:
> > This topic was also discussed during the deprecation of
> > TransactionMiddleware and the introduction of ATOMIC_REQUESTS. The
> > existing middleware semantics can't guarantee that __exit__ (during
> > process_response) will get called no matter what, necessitating the
> > setting that invokes BaseHandler.make_view_atomic. make_view_atomic
> > implements basically what you're suggesting, for one specific case
> > (DB/ORM code in the front controller, ick!).
> >
>

> I am not suggesting using the above process() example in Django. The
> semantics for process_exception() in particular are backwards incompatible.
> It is just an example that you can achieve what is currently available with
> process_request(), process_response() and process_exception() methods, but
> with obvious semantics.
>

I think the middlewares have grown a little complicated, and I'd try to avoid
adding functionality there. That said, the feature of universal decoration
seems like it is worthwhile. I'd try to see if it can be added through the
urls mechanism instead -- you'd get behavior that is more like a decorator, in
the sense that decoration would happen only once.

Shai.

[Marc Tamlyn]

Keyword argument to patterns() perhaps?

--
You received this message because you are subscribed to the Google Groups "Django developers" 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 http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/201310162036.05529.shai%40platonix.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Anssi Kääriäinen]

On Wednesday, October 16, 2013 8:36:05 PM UTC+3, Shai Berger wrote:

I think the middlewares have grown a little complicated, and I'd try to avoid
adding functionality there. That said, the feature of universal decoration
seems like it is worthwhile. I'd try to see if it can be added through the
urls mechanism instead -- you'd get behavior that is more like a decorator, in
the sense that decoration would happen only once.

I hacked together a branch where middlewares could define wrap_request and wrap_view methods in addition to all existing process_* methods. The wrap_request and wrap_view methods are call-through. It turns out that it will be impossible to both bubble up exceptions through wrap_* methods while also supporting current middleware semantics. Currently exceptions get converted immediately to 500 responses. Doing that and also raising the exception through the stack is obviously impossible.

Allowing view wrapping in urls.py seems like a good solution. It would allow for what ATOMIC_REQUESTS does currently. And more - it would allow wrapping just part of your views, not all of them. Having the ability to apply permission_required to a collection of views from urls.py seems like a nice feature.

That still leaves whole request wrapping into impossible to achieve state. Maybe guarantee that all process_response methods are ran even if some of the raise exceptions is enough?

Currently there is no guarantee that all process_response methods get called. It seems that support for that would be fairly small backwards incompatible change. See: https://github.com/akaariai/django/commit/cc459994bb7899e744471dcb7ec034e6e1c310af. This would add a guarantee that if process_request is called, then matching process_response gets called, too. Notable there is still no guarantee that if process_response is called, then process_request was also called. But changing that isn't easy to do.

 - Anssi

[Gavin Wahl]

I really like the idea of implementing this as a url pattern decorator. This seems similar to Alex Gaynor's talk about everything being a view. It's more flexible than a middleware because you can decorate any part of the urls tree, such as the patterns for a single reusable app.

    urls = decorate_recursively(login_required, patterns(...))

This is actually possible to implement today, see snippets like https://djangosnippets.org/snippets/2532/. The code required is kind of ugly, so I think it would benefit from an implementation in core.

--
You received this message because you are subscribed to a topic in the Google Groups "Django developers" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-developers/4TnXsONNEso/unsubscribe.
To unsubscribe from this group and all its topics, send an email to django-develop...@googlegroups.com.

To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/98bd7629-38f1-409e-b2c1-1a058ed3559e%40googlegroups.com.

[Krzysztof Jurewicz]

W dniu 24.10.2013 18:33, Gavin Wahl pisze:

> I really like the idea of implementing this as a url pattern decorator.
> This seems similar to Alex Gaynor's talk about everything being a view.
> It's more flexible than a middleware because you can decorate any part
> of the urls tree, such as the patterns for a single reusable app.
>
> urls = decorate_recursively(login_required, patterns(...))
>
> This is actually possible to implement today, see snippets like
> https://djangosnippets.org/snippets/2532/. The code required is kind of
> ugly, so I think it would benefit from an implementation in core.

There is also the django-decorator-include package:
https://github.com/jeffkistler/django-decorator-include and it contains
DecoratedPatterns class:
https://github.com/jeffkistler/django-decorator-include/blob/cc50e071e51af63e24583dc7274547796478fec3/src/decorator_include/__init__.py#L10
.