request.urlconf does not get used in URL template tag

[Trey]

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.

[Trey <trey@…>]

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

[Trey <trey@…>]

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

[Chris Beaven]

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.

[James Bennett]

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.

[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.

[James Bennett]

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".

[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?

[Chris Beaven]

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.

[Chris Beaven]

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.

[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?

[Chris Beaven]

forgot to add some test files

[Chris Beaven]

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.

[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.

[Justin]

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) 

[Jannis Leidel]

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

[anonymous]

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.

[Chris Beaven]

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

[Sean Brant]

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

[Brian Rosner]

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.

[Brian Rosner]

(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.