Opened 17 years ago

Last modified 7 years ago

#5797 new Bug

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

Reported by: Jeremy Dunck Owned by:
Component: HTTP handling Version: dev
Severity: Normal Keywords: cache middleware decorator gzip conditional http
Cc: hirokiky@…, Herbert Fortes Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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.

Change History (13)

comment:1 by Jacob, 17 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Gary Wilson, 16 years ago

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 by Thejaswi Puthraya, 16 years ago

Component: UncategorizedHTTP handling

comment:4 by Gabriel Hurley, 14 years ago

Severity: Normal
Type: Bug

comment:5 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:6 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:7 by Hiroki Kiyohara, 12 years ago

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 by Hiroki Kiyohara, 12 years ago

Owner: changed from nobody to Hiroki Kiyohara
Status: newassigned

comment:9 by Hiroki Kiyohara, 12 years ago

Needs documentation: unset
Needs tests: unset

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

comment:10 by Hiroki Kiyohara, 11 years ago

Has patch: unset
Owner: Hiroki Kiyohara removed
Status: assignednew

The pull-requelt doesn't work current master brunch. (https://github.com/django/django/pull/659)

comment:11 by Hiroki Kiyohara, 11 years ago

Has patch: set
Patch needs improvement: set

in reply to:  10 comment:12 by Asif Saifuddin Auvi, 9 years ago

Replying to hirokiky:

The pull-requelt doesn't work current master brunch. (https://github.com/django/django/pull/659)

so to make it properly need to rework based on this https://github.com/hirokiky/django/commit/2db135897f7dcc86861d4e3d6328e8d86a3ecf4c against master right?

comment:13 by Herbert Fortes, 7 years ago

Cc: Herbert Fortes added
Note: See TracTickets for help on using tickets.
Back to Top