Opened 3 years ago

Closed 2 years ago

Last modified 2 years 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.

Change History (12)

comment:1 Changed 3 years ago by akaariai

  • Triage Stage changed from Unreviewed to Accepted

Marking as accepted, the reporter seems trustworthy...

comment:2 Changed 3 years 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 3 years 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 3 years 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 2 years 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 2 years 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 2 years 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 2 years ago by aaugustin (next)

comment:8 Changed 2 years 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 2 years ago by aaugustin

#16444 was a duplicate.

comment:11 Changed 2 years 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 2 years 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...

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