Simplify middlewares (again) and get rid of process_view

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Simplify middlewares (again) and get rid of process_view

Florian Apolloner
Hi there,

After refactoring the middlewares to new-style middlewares as we have them now, I am left with two pain points:

 * atomic requests are still special cased and not in a middleware
 * process_view is as useless as always (it can neither alter nor convert args/kwargs or the view)

To change this I am proposing the following changes:

 * Deprecate request.urlconf and provide a way to set the urlconf __before__ the middleware chain is entered
 * Resolve view before the middleware chain is entered

This will imo improve existing code and allow for many new possibilities:

1.) Replace or transform kwargs/view. If the view is resolved before the middleware chain is entered, we can set resolver_match immediatelly effectively making process_view useless:

class MyMiddleware:
    def __init__(self, get_response):
        self.get_response = get_response

    def __call__(self, request):
        view, args, kwargs = request.resolver_match
        
        # If you call view() directly here you can return a response and do what process_view could do:
        return view(request, args, kwargs)
        
        # ... or you could add a profiling decorator to the view without affecting the rest of the middlewares
        # process_view did not allow for this feature in the past (you couldn't alter the view, just call and return a response):
        request.resolver_match.func = profiler(request.resolver_match.func)
        return self.get_response(request)

2.) Rewrite atomic requests to a middleware again:

class TransactionMiddleware:
    def __init__(self, get_response):
        self.get_response = get_response

    def __call__(self, request):
        # Either as it is now by simply covering the view
        request.resolver_match.func = make_view_atomic(request.resolver_match.func)
        
        # ... or by actually covering the following middleware layers too:
        # This was not possible before because middlewares wouldn't have access to the view
        # Excemption would still be controlled by the view itself
        view, args, kwargs = request.resolver_match
        get_response = self.get_response
        non_atomic_requests = getattr(view, '_non_atomic_requests', set())
        for db in connections.all():
            if db.settings_dict['ATOMIC_REQUESTS'] and db.alias not in non_atomic_requests:
                get_response = transaction.atomic(using=db.alias)(view)
                
        return get_response(request)
        
        
The only thing left is to provide a way to dynamically alter the URL conf; which we can do by adding a setting (yes a setting) which points to a callable with a default implementation of:

def urlconf_factory(request):
    return settings.ROOT_URLCONF
    
Before entering the middleware chain we'd then do

    urlconf = urlconf_factory(request)
    set_urlconf(urlconf)
    resolver = get_resolver(urlconf)
    request.resolver_match = resolver.resolve(request.path_info)
    request.urlconf = urlconf # if needed    
    
 -- this would have the added benefit of allowing proper reversal of urls inside middlewares if needed (the correct urlconf is already set in the thread local)


What do you think?

Cheers,
Florian

--
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 [hidden email].
To post to this group, send email to [hidden email].
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/a4d843d8-adce-4c61-aa52-c722c5d94f39%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: Simplify middlewares (again) and get rid of process_view

Carl Meyer-4
Hi Florian,

On 5/12/18 10:22 AM, Florian Apolloner wrote:

> After refactoring the middlewares to new-style middlewares as we have them now, I am left with two pain points:
>
>  * atomic requests are still special cased and not in a middleware
>  * process_view is as useless as always (it can neither alter nor convert args/kwargs or the view)
>
> To change this I am proposing the following changes:
>
>  * Deprecate request.urlconf and provide a way to set the urlconf __before__ the middleware chain is entered
>  * Resolve view before the middleware chain is entered

I'm not sure this part is feasible. It's an intentional part of
middleware design AFAIK (and useful) that middleware can modify
request.path and have this modification respected in view resolution.

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 [hidden email].
To post to this group, send email to [hidden email].
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/0c8e1164-931e-72a9-438c-c099fd7447ad%40oddbird.net.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: Simplify middlewares (again) and get rid of process_view

Florian Apolloner
Hi Carl,

On Wednesday, May 16, 2018 at 5:58:02 AM UTC+2, Carl Meyer wrote:
I'm not sure this part is feasible. It's an intentional part of
middleware design AFAIK (and useful) that middleware can modify
request.path and have this modification respected in view resolution.

my proposed urlresolver_factory takes the request as argument and should therefore allow for anything that the middleware would allow too -- are there any specific usecases you have in mind that would require a middleware?

--
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 [hidden email].
To post to this group, send email to [hidden email].
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/3f216c46-c48d-4ff1-bd6a-dcc2fb090c43%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: Simplify middlewares (again) and get rid of process_view

Adam Johnson-2
I'm also in favour of simplifying away process_view . And having worked with a multi-domain site that altered request.urlconf in a middleware, I know the problems it can bring.

Carl's point about modifying request.path is interesting - if we're looking to allow modifying it in the resolver step but not the middleware, would we "seal" the request to error if it gets changed later?

On Wed, 16 May 2018 at 18:49, Florian Apolloner <[hidden email]> wrote:
Hi Carl,

On Wednesday, May 16, 2018 at 5:58:02 AM UTC+2, Carl Meyer wrote:
I'm not sure this part is feasible. It's an intentional part of
middleware design AFAIK (and useful) that middleware can modify
request.path and have this modification respected in view resolution.

my proposed urlresolver_factory takes the request as argument and should therefore allow for anything that the middleware would allow too -- are there any specific usecases you have in mind that would require a middleware?

--
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 [hidden email].
To post to this group, send email to [hidden email].
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/3f216c46-c48d-4ff1-bd6a-dcc2fb090c43%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


--
Adam

--
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 [hidden email].
To post to this group, send email to [hidden email].
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/CAMyDDM0AvzAsyDO9Wzz7aGe-b2SEd6jjxQFot54GDXCnD-hohg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: Simplify middlewares (again) and get rid of process_view

Carl Meyer-4
In reply to this post by Florian Apolloner
On 5/16/18 10:49 AM, Florian Apolloner wrote:

> Hi Carl,
>
> On Wednesday, May 16, 2018 at 5:58:02 AM UTC+2, Carl Meyer wrote:
>
>     I'm not sure this part is feasible. It's an intentional part of
>     middleware design AFAIK (and useful) that middleware can modify
>     request.path and have this modification respected in view resolution.
>
>
> my proposed urlresolver_factory takes the request as argument and should
> therefore allow for anything that the middleware would allow too -- are
> there any specific usecases you have in mind that would require a
> middleware?

Composing two different middleware that both alter request.path for
different purposes? This will require the site owner to manually compose
in their urlconf_factory.

I think your core proposal that the main middleware call layering should
run after view resolution instead of before it is probably a better
design for middleware in principle, but it needs work in a couple areas:

1. Motivating the change in a way that clarifies how it will benefit
typical Django users, sufficiently to justify the churn. What new
possibilities are unlocked by the change, and has there been demand for
those possibilities?
2. Fleshing out how a reasonable transition path for existing middleware
(including third-party middleware) would look.

It's too bad we didn't think about this possibility during the original
DEP5, to avoid multiple middleware changes within a few Django versions.

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 [hidden email].
To post to this group, send email to [hidden email].
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/679c0352-d374-a979-90e3-732ea619ac63%40oddbird.net.
For more options, visit https://groups.google.com/d/optout.