Code

Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#4915 closed (fixed)

A smarter resolver: try to disambiguate URL patterns

Reported by: dusk@… Owned by: mtredinnick
Component: Core (Other) Version: master
Severity: Keywords:
Cc: martin@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

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.

Attachments (7)

django-smarter-resolver.patch (2.1 KB) - added by dusk@… 7 years ago.
Preliminary resolver patch
django-smarter-resolver-2.patch (4.7 KB) - added by dusk@… 7 years ago.
django-smarter-resolver-3.patch (5.1 KB) - added by dusk@… 7 years ago.
reverse-with-args.patch (2.2 KB) - added by Ilya Semenov <semenov@…> 7 years ago.
alternative approach
reverse-with-args-2.patch (2.1 KB) - added by Ilya Semenov <semenov@…> 7 years ago.
alternative approach, removed debug output
smarter-urlresolvers.8461.diff (2.2 KB) - added by semenov 6 years ago.
smarter-urlresolvers.8695.diff (2.2 KB) - added by semenov 6 years ago.
revision bump

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by dusk@…

Preliminary resolver patch

comment:1 Changed 7 years ago by mtredinnick

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

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.

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

comment:3 Changed 7 years ago by mtredinnick

  • Has patch set
  • Needs tests set
  • Owner changed from adrian to mtredinnick
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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.

comment:4 Changed 7 years ago by 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.

Changed 7 years ago by dusk@…

comment:5 follow-up: Changed 7 years ago by 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.

Changed 7 years ago by dusk@…

comment:6 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by SmileyChris

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

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by 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.

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

Changed 7 years ago by Ilya Semenov <semenov@…>

alternative approach

Changed 7 years ago by Ilya Semenov <semenov@…>

alternative approach, removed debug output

comment:9 Changed 6 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick

comment:10 in reply to: ↑ 7 Changed 6 years ago by 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.

comment:11 Changed 6 years ago by 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).

Changed 6 years ago by semenov

Changed 6 years ago by semenov

revision bump

comment:12 Changed 6 years ago by mtredinnick

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

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

comment:13 Changed 5 years ago by anonymous

  • Cc martin@… added

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.