Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#19541 closed Bug (fixed)

Reversing URLs in response middleware or streamed responses won't use per-request urlconf

Reported by: Aymeric Augustin Owned by: Aymeric Augustin
Component: HTTP handling Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Theoretically, after the URLconf is reset, reversing URLs isn't possible any longer.

This code was introduced in order to fix #5034.

Change History (12)

comment:1 by Anssi Kääriäinen, 11 years ago

Triage Stage: UnreviewedAccepted

Marking as accepted, the reporter seems trustworthy...

comment:2 by Aymeric Augustin, 11 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

I haven't actually tested it, I filed this ticket while I was focusing on another problem in the area.

comment:3 by Anssi Kääriäinen, 11 years ago

Not directly related, but...

While I was looking at this issue I noticed that we construct a *new* instance of the resolver instead of using get_urlconf() which would give us an existing instance. Seems like we are doing some extra work because of this (likely not much). This might be intentional, and it might be that we aren't actually doing that much extra work. I find the urlresolver code somewhat hard to follow, so I am not sure if there is a problem here or not.

As for this ticket: yes, it looks like if you will do reverse/resolve in the generator of a streaming response it will use the default urlconf instead of the request's urlconf. I wonder if that will be an issue in practice. We could likely defer the reset of urlconf into the .close() of the request.

comment:4 by Carl Meyer, 11 years ago

Summary: Reversing URLs in streamed responses (probably) doesn't workReversing URLs in response middleware or streamed responses won't use per-request urlconf

#19784 reports the same problem wrt response middleware.

I don't think it's quite accurate to say that "reversing URLs isn't possible any longer"; it just won't use a per-request custom URLconf.

comment:5 by Florian Apolloner, 11 years ago

So what do we wanna do? Just remove the reset or do it in the close of the Response?

comment:6 by Carl Meyer, 11 years ago

Removing the reset is not an option, wouldn't the following request in that thread then use the previous request's custom URLconf, which might be incorrect?

comment:7 by Aymeric Augustin, 11 years ago

The URLconf is set to settings.ROOT_URLCONF at the beginning of every request:
https://github.com/django/django/blob/1e4a27d08790c96f657d2e960c8142d1ca69aede/django/core/handlers/base.py#L80

And it's set to None at the end of every request:
https://github.com/django/django/blob/1e4a27d08790c96f657d2e960c8142d1ca69aede/django/core/handlers/base.py#L179

(The try and the finally clauses highlighted in these two links go together.)


I think Django should just stop resetting the URLconf at the end of every request:

  • it'll be reset at the very beginning of the next request anyway (even before request middleware runs),
  • this code was part of the original commit (#5034, [6c61ca3d]) rather than introduced to fix a bug,
  • it does cause bugs for streaming responses,
  • the rationale given in the comment is rather weak.

So, I'd like to remove this try/finally block — and deindent the 100 lines inside. I'm not attaching a patch because the deindent would cause too much noise.

Last edited 11 years ago by Aymeric Augustin (previous) (diff)

comment:8 by Aymeric Augustin, 11 years ago

Related quote from #django-dev:

28-21:32:28      <subleq> I have had problems with that set_urlconf(None)
28-21:32:42      <subleq> I needed to reverse urls in a response middleware

comment:10 by Aymeric Augustin, 11 years ago

#16444 was a duplicate.

comment:11 by Loic Bistuer <loic.bistuer@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 521765f63d724f0a1becb614530170f5d00fc6a9:

Fixed #19541 -- Fixed BaseHandler to enable reversing URLs in response middlewares
and streamed responses with respect to per-request urlconf.

comment:12 by Jannis Leidel <jannis@…>, 11 years ago

In 24234555a01f3ebff2e969210024c86842896671:

Merge pull request #885 from loic/ticket19541

Fixed #19541 -- Fixed BaseHandler to enable reversing URLs in response middlewares...

Note: See TracTickets for help on using tickets.
Back to Top