Opened 11 years ago
Closed 11 years ago
#21157 closed Bug (fixed)
Problems with ResolverMatch
Reported by: | Kevin Christopher Henry | Owned by: | Kevin Christopher Henry |
---|---|---|---|
Component: | Core (URLs) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | k@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
One problem is that ResolverMatch.func
can sometimes be a string rather than a callable.
The ultimate source of this is that resolve()
is using the same get_callable()
algorithm as reverse()
. reverse()
has to guess whether it is being passed a string import path or a string pattern name, and it does this by looking for a .
in the string. When resolve()
uses this same function and passes in an unqualified path name, it's treated as a pattern name and returned as is. But since resolve()
knows that it's dealing with a view, it would be better in this case to always return a callable (or raise an import Exception).
I would say this is definitely a bug in the implementation (as opposed to the documentation) except that the author of urlpatterns_reverse.tests.ResolverMatchTests.test_urlpattern_resolve()
clearly anticipated this behavior.
Another problem is that the documentation says that url_name
is "The name of the URL pattern that matches the URL", whereas ResolverMatch
will insert the string path to the view here if no name was defined. I think the documented behavior makes more sense - why conflate the view path with the pattern name here?
Change History (6)
comment:1 by , 11 years ago
Cc: | added |
---|---|
Type: | Uncategorized → Bug |
comment:2 by , 11 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:3 by , 11 years ago
Updated the patch to accommodate the flake8 fixes and ticket 21351.
All tests passed with SQLite.
comment:4 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:5 by , 11 years ago
Component: | Core (Other) → Core (URLs) |
---|
comment:6 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Patch here: https://github.com/django/django/pull/1704
Although this brings the implementation into compliance with the documentation, there's one situation I can think of where the user could see a change: if they have an unused url entry that points to an invalid view that happens to not have a
'.'
in it, an application that was previously working will now produce an exception whenreverse()
/url
is used. As described in Malcolm's comment to #6568, this exception raising is purposeful behavior; it just happened to be masked by this bug. This is an obscure (but possible) scenario, so I'm not sure if a warning in the documentation is called for. (I came across it because some parts of the existing test suite have this problem.)