Opened 17 years ago

Closed 14 years ago

#5034 closed (fixed)

request.urlconf does not get used in URL template tag

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

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@…> 17 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 Chris Beaven 16 years ago.
request_aware_reversing.2.diff (11.1 KB ) - added by Chris Beaven 16 years ago.
forgot to add some test files
set_urlconf.diff (9.3 KB ) - added by Chris Beaven 16 years ago.
set_urlconf.2.diff (8.1 KB ) - added by Sean Brant 14 years ago.
Updated to work with 1.2 also clears _urlconfs dict on each request

Download all attachments as: .zip

Change History (28)

by Trey <trey@…>, 17 years ago

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

comment:1 by Trey <trey@…>, 17 years ago

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 by Chris Beaven, 17 years ago

Needs documentation: set
Triage Stage: UnreviewedDesign 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 by James Bennett, 17 years ago

Resolution: duplicate
Status: newclosed

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 by Trey <trey@…>, 17 years ago

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 by James Bennett, 17 years ago

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 by Trey <trey@…>, 17 years ago

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?

by Chris Beaven, 16 years ago

comment:7 by Chris Beaven, 16 years ago

Needs documentation: unset
Resolution: duplicate
Status: closedreopened

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 by Chris Beaven, 16 years ago

Owner: changed from nobody to Chris Beaven
Status: reopenednew

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 by Trey, 16 years ago

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?

by Chris Beaven, 16 years ago

forgot to add some test files

in reply to:  9 comment:10 by Chris Beaven, 16 years ago

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 by Trey, 16 years ago

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 by Justin, 16 years ago

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 by anonymous, 16 years ago

Cc: jannis@… added

comment:14 by Jannis Leidel, 16 years ago

Cc: jannis@… removed

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

comment:15 by anonymous, 16 years ago

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 by Chris Beaven, 16 years ago

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

by Chris Beaven, 16 years ago

Attachment: set_urlconf.diff added

comment:17 by anonymous, 15 years ago

Cc: eallik@… added

comment:18 by jos3ph, 15 years ago

Cc: joesaccount@… added

comment:19 by Florian Apolloner, 15 years ago

Cc: Florian Apolloner added

comment:20 by Gonzalo Saavedra, 15 years ago

Cc: Gonzalo Saavedra added

comment:21 by chachra, 15 years ago

Cc: chachra added

by Sean Brant, 14 years ago

Attachment: set_urlconf.2.diff added

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

comment:22 by Brian Rosner, 14 years ago

Owner: changed from Chris Beaven to Brian Rosner
Status: newassigned

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 by Brian Rosner, 14 years ago

Resolution: fixed
Status: assignedclosed

(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