A smarter resolver: try to disambiguate URL patterns

[dusk@…]

The current development version of Django has added support for "named URL patterns" to try to handle reverse matching of views which can be accessed in multiple ways. I think this is a messy solution, given that the machinery exists to do this more cleanly. So I've written a simple patch to the reverse resolver that makes this possible.

[dusk@…]

Preliminary resolver patch

[Malcolm Tredinnick]

You'll need to include some explanation of how your resolver fixes the existing problems, I think.

For example, if I have two different URL patterns both taking an integer as a pattern and both calling a particular generic view (with, say, some extra default args in each case), I don't see how this differentiates between them. Since the parameter lists and view functions are identical in the two cases, there is no way to differentiate sans some extra information like a name. So you cannot eliminate the need for named patterns entirely (which was precisely why they were introduced).

If you are also trying to fix some other problem, perhaps including some tests to show what you are fixing would help with review. At the moment, the patch isn't too clear.

[dusk@…]

I guess an explanation of the situation I'm trying to solve here would probably be apropos.

I've got a view function 'account.views.profile' with the signature

def profile(request, user=None):

which uses None as a placeholder for "the current user". To go along with this, I've also got the URL patterns

    (r'^/profile/$', 'account.views.profile'),
    (r'^/profile/(?P<user>\d+)/', 'account.views.profile'),

Using the current reverse resolver, however, this ends up not working correctly. Depending on which pattern is first in the URL configuration, either

django.core.urlresolvers.reverse('account.views.profile', kwargs={'user': 12345})

will (incorrectly) return '/profile/', or

django.core.urlresolvers.reverse('account.views.profile')

will throw NoReverseMatch. This is primarily because the current reverse resolver can only store one URL pattern per view function.

What I'm after with this patch is rectifying that situation at least partially. I haven't done away with named patterns entirely - what I've done is a little more subtle:

• RegexURLResolver._get_reverse_dict now creates a dictionary mapping view functions to lists of URL patterns which use those functions, rather than simply "crushing" duplicates.
• Using this new information, RegexURLResolver.reverse tests its results by resolving them (this could probably be optimized!) and making sure that the view function, and arguments match the view function and args/kwargs which were passed into RegexURLResolver.reverse().

The critical difference here is that this makes RegexURLResolver.reverse() an inverse function of RegexURLResolver.resolve(). This has a number of desirable properties; for example, generating a URL using the {{url}} template function will always generate a URL which will resolve to the requested view function with the specified parameters.

If there are genuinely two different patterns which map to the exact same view function and parameters, I will agree that there is no solution to that except named patterns. However, I don't think that it should be necessary to use named functions when there is only one reverse mapping which preserves all the parameters.

[Malcolm Tredinnick]

Cool, thanks for the explanation. So you're actually solving a slightly different problem, which is a known bug, so the patch is most welcome. Reverse lookups when the parameter list is the only difference doesn't work well at the moment,you're right, and it's a bug in my book. If you can fix up a couple of problems with the patch, we'll take this in a heartbeat.

A few small changes if you've got the motivation (perfect patches get committed much faster):

1. Add some tests to tests/regressiontests/templates/ . Basically this will mean adding some URLS to urls.py, possibly create a new view function in views.py, and then adding some examples showing the disambiguation to the url tag tests in tests.py -- search for "url01" in tests.py to find where the existing tests are. This doesn't have to be anything too extreme: a pair of URL patterns as you describe above and then a url template tag test for each pattern where the only difference is the parameters (so two url tag tests).
2. There's a print statement in your patch. Often a big clue it's not ready to be committed. :-)
3. While your at it, add your name to the AUTHORS file and include that in the patch.

[anonymous]

I've attached a patch that tweaks an existing template test to cover the new reverse resolver, as well as relaxing the equivalence check a bit (by converting all parameters to strings) and fixing a few lurking Unicode bugs that popped up when I was debugging the regression tests.

There's a catch, though: one of the tests ('url02') fails. However, this appears to not be an issue with my modifications, but rather some sort of latent bug in the resolver that I've just happened to expose! Specifically, patterns containing keyword arguments are losing their positional arguments. As the expanded reverse resolver actually checks to make sure that the URL it constructs has the same parameters as it got passed in, it (appropriately) rejects the URL that it constructs for patterns containing both types of arguments. I've opened #4925 to address this issue.

[dusk@…]

Given the resolution of #4925, looks like the url02 test was flawed all along. I'm attaching a patch with a slightly modified test that ought to sew this one up neatly.

[Chris Beaven]

Replying to dusk@woofle.net:

Given the resolution of #4925, looks like the url02 test was flawed all along. I'm attaching a patch with a slightly modified test that ought to sew this one up neatly.

Current behaviour is that url02 isn't flawed. Forward resolving ignores positional args if any kwargs are given, but the reverse resolver accepts both (falling back to using positional arguments if a keyword arg is not present but required).

[dusk@…]

Replying to SmileyChris:

Current behaviour is that url02 isn't flawed. Forward resolving ignores positional args if any kwargs are given, but the reverse resolver accepts both (falling back to using positional arguments if a keyword arg is not present but required).

Hmm, that honestly seems kind of broken. The purpose of the reverse resolver is to generate URLs which will map to the supplied view and parameters - hence, generating a URL which doesn't resolve back to the parameters that you specified is incorrect behavior. It's probably better to refuse to generate a URL at all than to generate one which won't behave as desired.

[Ilya Semenov <semenov@…>]

I had the same problem and did a quick fix about that.

Now as I discovered this ticket I'm attaching my patch as well. I found my approach much simplier, though I might be missing something of course.

[Ilya Semenov <semenov@…>]

alternative approach

[Ilya Semenov <semenov@…>]

alternative approach, removed debug output

[anonymous]

Replying to dusk@woofle.net:

Replying to SmileyChris:

Current behaviour is that url02 isn't flawed. Forward resolving ignores positional args if any kwargs are given, but the reverse resolver accepts both (falling back to using positional arguments if a keyword arg is not present but required).

Hmm, that honestly seems kind of broken. The purpose of the reverse resolver is to generate URLs which will map to the supplied view and parameters - hence, generating a URL which doesn't resolve back to the parameters that you specified is incorrect behavior. It's probably better to refuse to generate a URL at all than to generate one which won't behave as desired.

I actually agree with you. But I've had this discussion with Malcolm before and he prefers letting the reverse resolver be more flexible.

[Ilya Semenov]

If anybodes cares, I'm attaching the updated patch against [8461].

See below the test cases.

First, what happens with the trunk code:

    (r'^/profile/$', 'account.views.profile'),
    (r'^/profile/(?P<user>\d+)/', 'account.views.profile'),

# ...

django.core.urlresolvers.reverse('account.views.profile') # "/profile/"
django.core.urlresolvers.reverse('account.views.profile', kwargs={'user': 12345}) # "/profile/" - BUG!

and again with the trunk:

    (r'^/profile/(?P<user>\d+)/', 'account.views.profile'),
    (r'^/profile/$', 'account.views.profile'),

# ...

django.core.urlresolvers.reverse('account.views.profile', kwargs={'user': 12345}) # "/profile/12345/"
django.core.urlresolvers.reverse('account.views.profile') # throws NoReverseMatch - BUG!

With the patch, in both cases:

django.core.urlresolvers.reverse('account.views.profile', kwargs={'user': 12345}) # "/profile/12345/"
django.core.urlresolvers.reverse('account.views.profile') # "/profile/"
# and even:
django.core.urlresolvers.reverse('account.views.profile', args=(12345,)) # "/profile/12345/"

The lookup algo complexity is still constant time.

The patch is not perfect though:

    (r'^/list/(?P<user>\d+)/(?P<page>\d+)/', 'list.users'),

# ...

django.core.urlresolvers.reverse('list.users', kwargs={'user':123, 'page':456}) # "/list/123/456/"
django.core.urlresolvers.reverse('list.users', args=(123, 456,)) # "/list/123/456/"
django.core.urlresolvers.reverse('list.users', args=(123,), kwargs={'page': 456}) # throws NoReverseMatch - BUG!

since it handles only two situations: when all named args are passed in kwargs (as expected), or when all named args are passed with args (trivial "fallback" behaviour).

[Ilya Semenov]

revision bump

[Malcolm Tredinnick]

(In [8760]) A rewrite of the reverse URL parsing: the reverse() call and the "url" template tag.

This is fully backwards compatible, but it fixes a bunch of little bugs. Thanks
to SmileyChris and Ilya Semenov for some early patches in this area that were
incorporated into this change.

Fixed #2977, #4915, #6934, #7206.