Opened 11 years ago

Closed 10 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 Kevin Christopher Henry, 11 years ago

Cc: k@… added
Type: UncategorizedBug

comment:2 by Kevin Christopher Henry, 11 years ago

Has patch: set
Owner: changed from nobody to Kevin Christopher Henry
Status: newassigned

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 when reverse() / 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.)

comment:3 by Kevin Christopher Henry, 10 years ago

Updated the patch to accommodate the flake8 fixes and ticket 21351.

All tests passed with SQLite.

Version 0, edited 10 years ago by Kevin Christopher Henry (next)

comment:4 by Aymeric Augustin, 10 years ago

Triage Stage: UnreviewedAccepted

comment:5 by Aymeric Augustin, 10 years ago

Component: Core (Other)Core (URLs)

comment:6 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 91afc00513bd2fa6302ea2be35e1f842cbd5fd38:

Fixed #21157 -- Fixed problems with ResolverMatch

  • Fixed bug in get_callable() that caused resolve() to put a string in ResolverMatch.func.
  • Made ResolverMatch.url_name match the actual url name (or None).
  • Updated tests that used the string value in ResolverMatch.func, and added regression tests for this bug.
  • Corrected test urls whose dummy view paths caused failures (behavior that was previously masked by this bug).
Note: See TracTickets for help on using tickets.
Back to Top