#21157 closed Bug (fixed)

Problems with ResolverMatch

Reported by: marfire Owned by: marfire
Component: Core (URLs) Version: master
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 Changed 18 months ago by marfire

  • Cc k@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Type changed from Uncategorized to Bug

comment:2 Changed 18 months ago by marfire

  • Has patch set
  • Owner changed from nobody to marfire
  • Status changed from new to assigned

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 Changed 16 months ago by marfire

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

All tests passed with SQLite.

Last edited 16 months ago by marfire (previous) (diff)

comment:4 Changed 16 months ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 16 months ago by aaugustin

  • Component changed from Core (Other) to Core (URLs)

comment:6 Changed 11 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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