Opened 8 years ago

Closed 6 years ago

#5034 closed (fixed)

request.urlconf does not get used in URL template tag

Reported by: Trey Owned by: brosner
Component: Core (Other) Version: master
Severity: Keywords: url, urlconf, override
Cc: justin2@…, eallik@…, joesaccount@…, apollo13, gonz, chachra Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

When you override the ROOT_URLCONF in a middleware by setting request.urlconf the URL template tag fails to resolve urls according to the new URL module. I have done quite a bit of searching on this topic and haven't found any definitive answers.

Based on #3187 this seems to be the correct thing to do. The request dict appears to be a poor way to transmit the data but without some sort of integrated threadlocals system and per the previously mentioned tag this patch seems appropriate.

Attachments (5)

url_templatetag_urlconf.patch (1.1 KB) - added by Trey <trey@…> 8 years ago.
This patch will modify the URLNode class in template/defaulttags.py to use the urlconf in the request context if available.
request_aware_reversing.diff (10.1 KB) - added by SmileyChris 8 years ago.
request_aware_reversing.2.diff (11.1 KB) - added by SmileyChris 8 years ago.
forgot to add some test files
set_urlconf.diff (9.3 KB) - added by SmileyChris 7 years ago.
set_urlconf.2.diff (8.1 KB) - added by seanbrant 6 years ago.
Updated to work with 1.2 also clears _urlconfs dict on each request

Download all attachments as: .zip

Change History (28)

Changed 8 years ago by Trey <trey@…>

This patch will modify the URLNode class in template/defaulttags.py to use the urlconf in the request context if available.

comment:1 Changed 8 years ago by Trey <trey@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I forgot to mention, for this to work you will need to use the REQUEST context.
http://www.djangoproject.com/documentation/templates_python/#django-core-context-processors-request

comment:2 Changed 8 years ago by SmileyChris

  • Needs documentation set
  • Triage Stage changed from Unreviewed to Design decision needed

Thanks Trey, very timely since we have been discussing the future need to couple reverse with request somehow recently in Django devs.

As you have stated, currently this looks like the best solution (introducing the prerequisite of the request context processor of you require changing the URLconf). Perhaps some documentation could be provided to explain this.

For now, I'll pass to Design Decision for another set of eyes.

comment:3 Changed 8 years ago by ubernostrum

  • Resolution set to duplicate
  • Status changed from new to closed

I'd be inclined to accept it on its own, but it really ought to be rolled in with #3530 which does the same to permalink, and so kill two birds with one stone by fixing the underlying problem with reverse. Closing this in favor of #3530 somewhat arbitrarily, since that's the older ticket.

comment:4 Changed 8 years ago by Trey <trey@…>

I would tend to agree with you normally. However, I see no reason to close the ticket since there doesn't seem to be an actual solution to this problem in the code or currently proposed. I don't think ignoring the problem in hopes that a better way will be proposed in the future is an answer.

From what I can tell there isn't a solution proposed or even considered in #3530 that would blanket these problems. I would be very disappointed for this patch to be ignored simply because it branches from a deeper rooted problem.

comment:5 Changed 8 years ago by ubernostrum

Trey, I'm not saying "give up on your patch, it's rejected". I'm saying "this other ticket is working on very similar stuff, let's merge into one ticket and get all the work done there".

comment:6 Changed 8 years ago by Trey <trey@…>

I didn't think you were saying that, at least not like that. I just don't see the relationship between this ticket and #3530 that's all. I can see how they are similar, but the real problem is that request isn't portable to all pieces of the app. Unless some sort of threadlocal or, if I were in Java, a singleton style of class were introduced the problem will remain.

Anyway, I guess my point is that the over arching issue is much larger than either of these two tickets originally and seems like it would be a major change to the core of django which would require a lot of design decisions. I would like to see a ticket or topic discussing what the long term plan is if you know of one. Otherwise, should you or I move my patch over to #3530 so that I don't have to patch every instance of django?

Changed 8 years ago by SmileyChris

comment:7 Changed 8 years ago by SmileyChris

  • Needs documentation unset
  • Resolution duplicate deleted
  • Status changed from closed to reopened

I agree with Trey that this ticket isn't that closely related to the other that they need a merge.

My patch comes with tests and documentation.

comment:8 Changed 8 years ago by SmileyChris

  • Owner changed from nobody to SmileyChris
  • Status changed from reopened to new

Hope you don't mind Trey, but I'll take ownership of the ticket. If you could give some feedback on my patch it'd be appreciated.

comment:9 follow-up: Changed 8 years ago by trey

I think that this patch covers a lot of the instances where you would want to override the URLconf instead of my original patch which just updated the {% url %} tag. This is how J2EE stuff solves the problem and it looks very clean to me. Short of using threadlocals or some other black magic this is as good as it's gonna get :)

However, weighing in on the URL block tag specifically. It seems that you assume request will always be available in the context if it's a Request Context. Since there is a built-in django.core.context_processors.request template context processor is request always available or does that context need to be present?

Changed 8 years ago by SmileyChris

forgot to add some test files

comment:10 in reply to: ↑ 9 Changed 8 years ago by SmileyChris

Replying to trey:

I think that this patch covers a lot of the instances where you would want to override the URLconf instead of my original patch which just updated the {% url %} tag. This is how J2EE stuff solves the problem and it looks very clean to me. Short of using threadlocals or some other black magic this is as good as it's gonna get :)

Yep, I think this is a good interim. Even if the other patch gets in, it's up to the individual to implement where as this patch will at least cover most cases for people who need it (at least it clearly documents where it won't).

However, weighing in on the URL block tag specifically. It seems that you assume request will always be available in the context if it's a Request Context. Since there is a built-in django.core.context_processors.request template context processor is request always available or does that context need to be present?

It's not actually getting it from the context itself (because, as you noted, it would depend on the request context processor). I've made a small change to RequestContext -- it now stores request as an attribute. So rather than looking for a context variable, we can just look directly at the context.request attribute.

comment:11 Changed 8 years ago by trey

Ah, very nice. I missed that small line.

I will test this patch out on my installs and see if I can root out any other issues.

comment:12 Changed 7 years ago by justinf

I think there may be a little too much use of isinstance() in this patch.

Specifically on lines 362-365 of defaulttags.py

        if isinstance(context, RequestContext): 
            urlconf = context.request 
        else: 
            urlconf = None 

should probably be replaced with something like

        urlconf = getattr(context, 'request', None) or context.get('request', None)

I ran into a case where I couldn't pass a RequestContext to a template, but I could set contextrequest?, and this change allowed {% url %} to still work. Of course, there could be issues with assuming that contextrequest? is a HttpRequest, but it's probably very rare that it isn't.

I'm not sure how the other uses of isinstance() might effect things, but line 305 of urlresolver.py seems like it could be removed:

if urlconf is not None and isinstance(urlconf, HttpRequest): 
        urlconf = getattr(urlconf, 'urlconf', None) 

to leave

urlconf = getattr(urlconf, 'urlconf', urlconf) 

comment:13 Changed 7 years ago by anonymous

  • Cc jannis@… added

comment:14 Changed 7 years ago by jezdez

  • Cc jannis@… removed

Just in case you need help during the coming sprintsfor docs and tests, I'm here to help :)

comment:15 Changed 7 years ago by anonymous

  • Cc justin2@… added

I'm sure most of us who pay attention to this ticket are aware of this, but the patch here no longer works with trunk.

It looks like Malcolm might be working on this, but if you need to have request aware reversing and want to use trunk, a simple addition to get_resolver() in urlresolvers.py and some middleware will make it work. This uses threading.local rather than explicitly passing the request.

First, there's this middleware that I put in django/middleware/request.py:

try:
    from threading import local
except ImportError:
    from django.utils._threading_local import local

_thread_locals = local()
def get_request():
    return getattr(_thread_locals, 'request', None)

class RequestMiddleware(object):
    """Middleware that saves the current request in thread local storage."""
    def process_request(self, request):
        _thread_locals.request = request

Then the change to get_resolver():

from django.middleware.request import get_request

def get_resolver(urlconf):
    if urlconf is None:
        request = get_request()
        if request:
            urlconf = request.urlconf
    if urlconf is None:
        from django.conf import settings
        urlconf = settings.ROOT_URLCONF
    return RegexURLResolver(r'^/', urlconf)

I've only done minimal testing, but I figured someone looking here might want to try this.

comment:16 Changed 7 years ago by SmileyChris

After talking with Malcolm today on IRC, here's a new take on this. Less leaky.

Changed 7 years ago by SmileyChris

comment:17 Changed 7 years ago by anonymous

  • Cc eallik@… added

comment:18 Changed 7 years ago by jos3ph

  • Cc joesaccount@… added

comment:19 Changed 7 years ago by apollo13

  • Cc apollo13 added

comment:20 Changed 6 years ago by gonz

  • Cc gonz added

comment:21 Changed 6 years ago by chachra

  • Cc chachra added

Changed 6 years ago by seanbrant

Updated to work with 1.2 also clears _urlconfs dict on each request

comment:22 Changed 6 years ago by brosner

  • Owner changed from SmileyChris to brosner
  • Status changed from new to assigned

I'm not particularly convinced moving away from request.urlconf is the best move. Conceptually it is a good idea as it makes it clear what the operation is being performed. Exposing a function that modifies a thread local store feels wrong to me. I've modified the patch slightly to simply map request.urlconf to the underlying set_urlconf and get_urlconf. See http://github.com/brosner/django/commit/3fab27f43dec665d32d7008013272984df3b482f. I am keen on getting this in and will stick by discussion as best as I can.

comment:23 Changed 6 years ago by brosner

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

(In [11740]) Fixed #5034 -- honor request.urlconf in reverse and resolve.

This enables {% url %} to honor request.urlconf set from process_request
middleware methods.

Thanks SmileyChris for the initial patch work.

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