Opened 12 years ago
Closed 12 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 , 12 years ago
| Cc: | added |
|---|---|
| Type: | Uncategorized → Bug |
comment:2 by , 12 years ago
| Has patch: | set |
|---|---|
| Owner: | changed from to |
| Status: | new → assigned |
comment:3 by , 12 years ago
Updated the patch to accommodate the flake8 fixes and ticket 21351.
All tests passed with SQLite.
comment:4 by , 12 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:5 by , 12 years ago
| Component: | Core (Other) → Core (URLs) |
|---|
comment:6 by , 12 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()/urlis 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.)