[Django] #30995: Feature/docs: how should url converters decline to match for a named route?
Django
-------------------------------------+-------------------------------------
Reporter: jcushman | Owner: nobody
Type: New | Status: new
feature |
Component: Core | Version: 2.2
(URLs) | Keywords: reverse, urls,
Severity: Normal | converter
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30995>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: jcushman | Owner: nobody
Component: Core (URLs) | Version: 2.2
Severity: Normal | Resolution:
Keywords: reverse, urls, | Triage Stage:
converter | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "converter_test.py" added.
Single-file Django project demonstrating type-matching based url
resolution
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Core (URLs) | Version: 2.2
Keywords: reverse, urls, | Triage Stage:
converter | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30995#comment:1>
Django
-------------------------------------+-------------------------------------
Component: Core (URLs) | Version: 2.2
Severity: Normal | Resolution:
converter | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: invalid =>
Comment:
''Having multiple URLs with the same name is not supported.''
Hmm, I don't think that's right.
[https://docs.djangoproject.com/en/dev/topics/http/urls/#naming-url-
patterns 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30995#comment:2>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: reverse, urls, | Triage Stage: Accepted
converter |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* version: 2.2 => master
* easy: 1 => 0
* stage: Unreviewed => Accepted
Comment:
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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/30995#comment:3>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: reverse, urls, | Triage Stage: Accepted
converter |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Jack Cushman):
''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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30995#comment:4>
Django
-------------------------------------+-------------------------------------
| Cushman
Type: New feature | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: reverse, urls, | Triage Stage: Accepted
converter |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* owner: nobody => Jack Cushman
* status: new => assigned
Comment:
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. 🙂
--
Ticket URL: <https://code.djangoproject.com/ticket/30995#comment:5>
Django
-------------------------------------+-------------------------------------
| Cushman
Type: New feature | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: reverse, urls, | Triage Stage: Accepted
converter |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
FWIW [https://groups.google.com/forum/#!searchin/django-
developers/ContinueResolving%7Csort:date 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30995#comment:6>
Django
-------------------------------------+-------------------------------------
| Cushman
Type: New feature | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: reverse, urls, | Triage Stage: Accepted
converter |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Here's a PR for review: [https://github.com/django/django/pull/12121 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30995#comment:7>
Django
-------------------------------------+-------------------------------------
| Cushman
Type: New feature | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
Keywords: reverse, urls, | Triage Stage: Accepted
converter |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30995#comment:8>
Django
-------------------------------------+-------------------------------------
| Cushman
Type: New feature | Status: assigned
Component: Core (URLs) | Version: master
Severity: Normal | Resolution:
converter | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by felixxm):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/30995#comment:9>
Django
-------------------------------------+-------------------------------------
| Cushman
Component: Core (URLs) | Version: master
Keywords: reverse, urls, | Triage Stage: Ready for
converter | checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"eb629f4c028ae220084904db84d633d7b3f0af20" eb629f4]:
{{{
#!CommitTicketReference repository=""
revision="eb629f4c028ae220084904db84d633d7b3f0af20"
Fixed #30995 -- Allowed converter.to_url() to raise ValueError to indicate
no match.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30995#comment:10>