Code

Opened 6 years ago

Last modified 15 months ago

#5797 assigned Bug

decorator_from_middleware can cause middleware hooks to run out of correct order.

Reported by: jdunck Owned by: hirokiky
Component: HTTP handling Version: master
Severity: Normal Keywords: cache middleware decorator gzip conditional http
Cc: hirokiky@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

cache_page, gzip_page, and conditional_page all use decorator_from_middleware.

decorator_from_middleware wraps the view function to simulate middleware being run on a per-view basis.

Here's the interesting bit:

        def _wrapped_view(request, *args, **kwargs):
            if hasattr(middleware, 'process_request'):
                result = middleware.process_request(request)
                if result is not None:
                    return result
            if hasattr(middleware, 'process_view'):
                result = middleware.process_view(request, view_func, *args, **kwargs)
                if result is not None:
                    return result
            try:
                response = view_func(request, *args, **kwargs)
            except Exception, e:
                if hasattr(middleware, 'process_exception'):
                    result = middleware.process_exception(request, e)
                    if result is not None:
                        return result
                raise
            if hasattr(middleware, 'process_response'):
                result = middleware.process_response(request, response)
                if result is not None:
                    return result
            return response

The problem is that middleware's order is important. Suppose two middleware decorators, A and B, are applied to the same view.

The normal flow of middleware processing would be: A.process_request, B.process_request, A.process_view, B.process_view, view, B.process_response, A.process_response.

The flow of the *decorated execution* is this: A.process_request, A.process_view, B.process_request, B.process_view, view, B.process_response, A.process_response.

This is not yet a real-world issue as far as I know, but seems to be a lurking problem. I think the only reason it hasn't surfaced is that view decorators are fairly uncommon. I don't see a simple way to fix this.

Attachments (0)

Change History (9)

comment:1 Changed 6 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 6 years ago by gwilson

One solution would be to have a decorator that takes in middleware classes, something like:

@middleware(CacheMiddleware, TransactionMiddleware)
def myview(request)
   ...

The middleware decorator would combine each type of middleware method and run each type at the appropriate time.

But next you'll be telling me that you want to order the middleware differently for process_request and process_response :)

comment:3 Changed 5 years ago by thejaswi_puthraya

  • Component changed from Uncategorized to HTTP handling

comment:4 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to Bug

comment:5 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:6 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:7 Changed 15 months ago by hirokiky

  • Cc hirokiky@… added
  • Has patch set
  • Needs documentation set
  • Needs tests set

I fixed decorator_from_middleware to support multiple arguments caused by gwilson's idea,
here this (https://github.com/hirokiky/django/commit/2db135897f7dcc86861d4e3d6328e8d86a3ecf4c) .

It passed the existing tests by Python2.7.1 and Python 3.3.
But, It needs more testes, and documentation.

comment:8 Changed 15 months ago by hirokiky

  • Owner changed from nobody to hirokiky
  • Status changed from new to assigned

comment:9 Changed 15 months ago by hirokiky

  • Needs documentation unset
  • Needs tests unset

I send the pull-request here (https://github.com/django/django/pull/659).

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from hirokiky to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.