Code

Opened 16 months ago

Closed 13 months ago

Last modified 13 months ago

#19541 closed Bug (fixed)

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

Reported by: aaugustin Owned by: aaugustin
Component: HTTP handling Version: master
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.

Attachments (0)

Change History (12)

comment:1 Changed 15 months ago by akaariai

  • Triage Stage changed from Unreviewed to Accepted

Marking as accepted, the reporter seems trustworthy...

comment:2 Changed 15 months ago by aaugustin

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

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

comment:3 Changed 15 months ago by akaariai

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 Changed 14 months ago by carljm

  • Summary changed from Reversing URLs in streamed responses (probably) doesn't work to Reversing 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 Changed 14 months ago by apollo13

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

comment:6 Changed 14 months ago by carljm

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 Changed 14 months ago by aaugustin

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 (6c61ca3d) and not 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.

Version 0, edited 14 months ago by aaugustin (next)

comment:8 Changed 14 months ago by aaugustin

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 Changed 13 months ago by aaugustin

#16444 was a duplicate.

comment:11 Changed 13 months ago by Loic Bistuer <loic.bistuer@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 521765f63d724f0a1becb614530170f5d00fc6a9:

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

comment:12 Changed 13 months ago by Jannis Leidel <jannis@…>

In 24234555a01f3ebff2e969210024c86842896671:

Merge pull request #885 from loic/ticket19541

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.