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 soto_url
works the same asto_python
, and aValueError
indicates no match (which I think just means wrappingtry: ... 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)
Change History (11)
by , 5 years ago
Attachment: | converter_test.py added |
---|
comment:1 by , 5 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
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 , 5 years ago
Resolution: | invalid |
---|---|
Status: | closed → new |
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 , 5 years ago
Easy pickings: | unset |
---|---|
Triage Stage: | Unreviewed → Accepted |
Version: | 2.2 → master |
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 , 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 , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
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 , 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 , 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 , 5 years ago
Patch needs improvement: | set |
---|
comment:9 by , 5 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Single-file Django project demonstrating type-matching based url resolution