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 , 17 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 16 years ago
comment:3 by , 16 years ago
Component: | Uncategorized → HTTP handling |
---|
comment:4 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:7 by , 12 years ago
Cc: | 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 , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:9 by , 12 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
I send the pull-request here (https://github.com/django/django/pull/659).
follow-up: 12 comment:10 by , 11 years ago
Has patch: | unset |
---|---|
Owner: | removed |
Status: | assigned → new |
The pull-requelt doesn't work current master brunch. (https://github.com/django/django/pull/659)
comment:11 by , 11 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
comment:12 by , 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 , 7 years ago
Cc: | added |
---|
One solution would be to have a decorator that takes in middleware classes, something like:
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
andprocess_response
:)