Opened 5 years ago

Closed 5 years ago

#30995 closed New feature (fixed)

Feature/docs: how should url converters decline to match for a named route?

Reported by: Jack Cushman Owned by: Jack Cushman
Component: Core (URLs) Version: dev
Severity: Normal Keywords: reverse, urls, converter
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It is sometimes convenient to have multiple instances of a named route, where the correct one is chosen based on whether the URL converters to_url call succeeds. For example, the attached file has routes like this:

    path('export/foo/<foo:obj>', index, name='export'),
    path('export/bar/<bar:obj>', index, name='export'),

I then wanted to put {% url "export" some_foo_or_bar %} in a generic export template and have the correct URL inserted.

My first attempt to do this was to raise ValueError in to_url for non-matching values, hoping that it would work the same as to_python where the docs specify that "A ValueError is interpreted as no match."

That didn't work -- nothing catches the ValueError in to_url -- so I found the workaround demonstrated in the attachment: if to_url returns an empty string (or some string that doesn't match the converter's regex), then the later regex check in _reverse_with_prefix will detect the non-match and everything seems to work out.

So this is either a feature request or a documentation check. I'm thinking either:

  • _reverse_with_prefix could be updated so to_url works the same as to_python, and a ValueError indicates no match (which I think just means wrapping try: ... except ValueError: continue around the appropriate line), or
  • the docs could be updated to indicate that, in to_url, a converter should return a non-regex-matching string such as the empty string in order to decline a match.

Attachments (1)

converter_test.py (1.8 KB ) - added by Jack Cushman 5 years ago.
Single-file Django project demonstrating type-matching based url resolution

Download all attachments as: .zip

Change History (11)

by Jack Cushman, 5 years ago

Attachment: converter_test.py added

Single-file Django project demonstrating type-matching based url resolution

comment:1 by Carlton Gibson, 5 years ago

Resolution: invalid
Status: newclosed

Having multiple URLs with the same name is not supported. (You'd need to have a converter that knew how to handle both types, or else two URLs with distinct names.)

I think this kind of question is better aimed at support channels. See TicketClosingReasons/UseSupportChannels.

comment:2 by Jack Cushman, 5 years ago

Resolution: invalid
Status: closednew

Having multiple URLs with the same name is not supported.

Hmm, I don't think that's right. According to the docs: "You may also use the same name for multiple URL patterns if they differ in their arguments. In addition to the URL name, reverse() matches the number of arguments and the names of the keyword arguments."

There's code in django.urls.resolvers specifically to support polymorphic name resolution, which is why examples like this will work:

Resolution based on keywords:

    path('export/baz/<int:baz>', index, name='export'),  # {% url "export" baz=1 %}
    path('export/boo/<int:boo>', index, name='export'),  # {% url "export" boo=1 %} 

Resolution based on values:

    re_path(r'^import/foo/(\d+)$', index, name='import'),  # {% url "import" 123 %}
    re_path(r'^import/bar/([a-z]+)$', index, name='import'),  # {% url "import" "abc" %}

It is strange for polymorphic name resolution to be supported in both of those cases, but not in the case where one converter can successfully convert the value and the other cannot. I filed this bug to address that edge case.

comment:3 by Carlton Gibson, 5 years ago

Easy pickings: unset
Triage Stage: UnreviewedAccepted
Version: 2.2master

OK, good. You're right about the number of args and keyword arguments case. Super. Sorry for missing that.
(Is your second "Resolution based on values" example right? It should be number of positional args, rather then type, no? Doesn't matter.)

The issue here is that in your case the kwarg is obj both times. So reverse can't currently pick it up. Fine.

Then, yes, your suggestion to raise a ValueError seems plausible enough. Want to take that on?
(I've unchecked "Easy pickings" as I think there's probably more in this than that entails.)

comment:4 by Jack Cushman, 5 years ago

Is your second "Resolution based on values" example right? It should be number of positional args, rather then type, no?

Yeah, just for the sake of documenting this, the actual logic of reverse() is a little more sophisticated than the docs suggest. Reversing by name returns the last-defined url with the given name, where (a) the arg count or kwarg keywords match, and (b) the generated url matches the path or re_path's regex. The second test means that we match based on argument type as well as count in practice. While matching on type isn't documented (that I know of), I think it's implicit to the user expectation that reverse() should return URLs that resolve back to the matched path -- otherwise in basic <int:id> cases you get a weird situation where passing the wrong value to reverse() appears to work but then returns a non-resolvable URL.

I hadn't noticed, but that also means that resolving based on url converter type already works in some situations!

    path('import/foo/<str:obj>', index, name='import'),  # {% url "import" "abc" %}
    path('import/bar/<int:obj>', index, name='import'),  # {% url "import" 123 %}

The above works because IntConverter happens to follow the undocumented protocol that converters can decline a match by returning a string that doesn't match their regex, so reversing on "abc" falls through to the previous route.

Then, yes, your suggestion to raise a ValueError seems plausible enough. Want to take that on?

Sure! Haven't contributed before, but the contributor docs look great -- I'll take a pass at it and see if I can figure it out.

comment:5 by Carlton Gibson, 5 years ago

Owner: changed from nobody to Jack Cushman
Status: newassigned

Hey Jack, super follow-up.

Sure! Haven't contributed before, but the contributor docs look great -- I'll take a pass at it and see if I can figure it out.

Great! Welcome aboard! 🚀

If you need an input open a PR early and we'll do that.

A docs clarification may be in order whilst you're in here. 🙂

comment:6 by Simon Charette, 5 years ago

FWIW this was discussed before and accepted in #16774

The idea was to introduce new exceptions dedicated to this purpose, e.g. ContinueResolving. I guess this ticket should be closed as a duplicate.

comment:7 by Jack Cushman, 5 years ago

Has patch: set

Here's a PR for review: PR

Simon, thanks for finding those related links! This ticket is distinct from #16774, though. That one is about allowing views to raise ContinueResolving internally once control is handed off to them, which would be a much larger change to the internal resolution logic.

This one involves adding a single try: ... except ValueError: continue to the reverse() logic to give url converters an explicit way to deny a match in to_url(), which they can already do by returning empty string or other non-regex-matching response, and which is consistent with the handling of ValueError in to_python(). As you can see from the new tests in the PR, Django already supports type-based reverse resolution in a variety of cases, and this PR just fills in one edge case.

I do see how the two are related, though! Depending why someone wants to raise ContinueResolving from a view, they might be able to solve the same problem by raising ValueError from converter.to_python since converters landed in Django 2.0. That's not affected one way or another by this patch to reverse(), though.

comment:8 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:9 by Mariusz Felisiak, 5 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In eb629f4:

Fixed #30995 -- Allowed converter.to_url() to raise ValueError to indicate no match.

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