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).

10 files changed, 116 insertions, 88 deletions

diff --git a/django/core/urlresolvers.py b/django/core/urlresolvers.py
index 8ca602c241..2f05571687 100644
--- a/django/core/urlresolvers.py
+++ b/django/core/urlresolvers.py
@@ -39,34 +39,31 @@ class ResolverMatch(object):
         self.func = func
         self.args = args
         self.kwargs = kwargs
+        self.url_name = url_name
         self.app_name = app_name
+
         if namespaces:
             self.namespaces = [x for x in namespaces if x]
         else:
             self.namespaces = []
-        if not url_name:
-            if not hasattr(func, '__name__'):
-                # An instance of a callable class
-                url_name = '.'.join([func.__class__.__module__, func.__class__.__name__])
-            else:
-                # A function
-                url_name = '.'.join([func.__module__, func.__name__])
-        self.url_name = url_name
+        self.namespace = ':'.join(self.namespaces)
 
-    @property
-    def namespace(self):
-        return ':'.join(self.namespaces)
+        if not hasattr(func, '__name__'):
+            # A class-based view
+            self._func_path = '.'.join([func.__class__.__module__, func.__class__.__name__])
+        else:
+            # A function-based view
+            self._func_path = '.'.join([func.__module__, func.__name__])
 
-    @property
-    def view_name(self):
-        return ':'.join(filter(bool, (self.namespace, self.url_name)))
+        view_path = url_name or self._func_path
+        self.view_name = ':'.join(self.namespaces + [view_path])
 
     def __getitem__(self, index):
         return (self.func, self.args, self.kwargs)[index]
 
     def __repr__(self):
-        return "ResolverMatch(func=%s, args=%s, kwargs=%s, url_name='%s', app_name='%s', namespace='%s')" % (
-            self.func, self.args, self.kwargs, self.url_name, self.app_name, self.namespace)
+        return "ResolverMatch(func=%s, args=%s, kwargs=%s, url_name=%s, app_name=%s, namespaces=%s)" % (
+            self._func_path, self.args, self.kwargs, self.url_name, self.app_name, self.namespaces)
 
 
 class Resolver404(Http404):
@@ -80,43 +77,61 @@ class NoReverseMatch(Exception):
 @lru_cache.lru_cache(maxsize=None)
 def get_callable(lookup_view, can_fail=False):
     """
-    Convert a string version of a function name to the callable object.
-
-    If the lookup_view is not an import path, it is assumed to be a URL pattern
-    label and the original string is returned.
+    Return a callable corresponding to lookup_view. This function is used
+    by both resolve() and reverse(), so can_fail allows the caller to choose
+    between returning the input as is and raising an exception when the input
+    string can't be interpreted as an import path.
 
-    If can_fail is True, lookup_view might be a URL pattern label, so errors
-    during the import fail and the string is returned.
+    If lookup_view is already a callable, return it.
+    If lookup_view is a string import path that can be resolved to a callable,
+      import that callable and return it.
+    If lookup_view is some other kind of string and can_fail is True, the string
+      is returned as is. If can_fail is False, an exception is raised (either
+      ImportError or ViewDoesNotExist).
     """
-    if not callable(lookup_view):
-        mod_name, func_name = get_mod_func(lookup_view)
-        if func_name == '':
+    if callable(lookup_view):
+        return lookup_view
+
+    mod_name, func_name = get_mod_func(lookup_view)
+    if not func_name:  # No '.' in lookup_view
+        if can_fail:
             return lookup_view
+        else:
+            raise ImportError(
+                "Could not import '%s'. The path must be fully qualified." %
+                lookup_view)
 
-        try:
-            mod = import_module(mod_name)
-        except ImportError:
+    try:
+        mod = import_module(mod_name)
+    except ImportError:
+        if can_fail:
+            return lookup_view
+        else:
             parentmod, submod = get_mod_func(mod_name)
-            if (not can_fail and submod != '' and
-                    not module_has_submodule(import_module(parentmod), submod)):
+            if submod and not module_has_submodule(import_module(parentmod), submod):
                 raise ViewDoesNotExist(
-                    "Could not import %s. Parent module %s does not exist." %
+                    "Could not import '%s'. Parent module %s does not exist." %
                     (lookup_view, mod_name))
-            if not can_fail:
+            else:
                 raise
+    else:
+        try:
+            view_func = getattr(mod, func_name)
+        except AttributeError:
+            if can_fail:
+                return lookup_view
+            else:
+                raise ViewDoesNotExist(
+                    "Could not import '%s'. View does not exist in module %s." %
+                    (lookup_view, mod_name))
         else:
-            try:
-                lookup_view = getattr(mod, func_name)
-                if not callable(lookup_view):
-                    raise ViewDoesNotExist(
-                        "Could not import %s.%s. View is not callable." %
-                        (mod_name, func_name))
-            except AttributeError:
-                if not can_fail:
-                    raise ViewDoesNotExist(
-                        "Could not import %s. View does not exist in module %s." %
-                        (lookup_view, mod_name))
-    return lookup_view
+            if not callable(view_func):
+                # For backwards compatibility this is raised regardless of can_fail
+                raise ViewDoesNotExist(
+                    "Could not import '%s.%s'. View is not callable." %
+                    (mod_name, func_name))
+
+            return view_func
 
 
 @lru_cache.lru_cache(maxsize=None)
diff --git a/tests/middleware/extra_urls.py b/tests/middleware/extra_urls.py
index 3b1fb85604..80f07c5074 100644
--- a/tests/middleware/extra_urls.py
+++ b/tests/middleware/extra_urls.py
@@ -1,7 +1,7 @@
 from django.conf.urls import url
 
 urlpatterns = [
-    url(r'^customurlconf/noslash$', 'view'),
-    url(r'^customurlconf/slash/$', 'view'),
-    url(r'^customurlconf/needsquoting#/$', 'view'),
+    url(r'^customurlconf/noslash$', 'middleware.views.empty_view'),
+    url(r'^customurlconf/slash/$', 'middleware.views.empty_view'),
+    url(r'^customurlconf/needsquoting#/$', 'middleware.views.empty_view'),
 ]
diff --git a/tests/middleware/urls.py b/tests/middleware/urls.py
index c429bb90fe..7d6689590c 100644
--- a/tests/middleware/urls.py
+++ b/tests/middleware/urls.py
@@ -1,7 +1,7 @@
 from django.conf.urls import url
 
 urlpatterns = [
-    url(r'^noslash$', 'view'),
-    url(r'^slash/$', 'view'),
-    url(r'^needsquoting#/$', 'view'),
+    url(r'^noslash$', 'middleware.views.empty_view'),
+    url(r'^slash/$', 'middleware.views.empty_view'),
+    url(r'^needsquoting#/$', 'middleware.views.empty_view'),
 ]
diff --git a/tests/middleware/views.py b/tests/middleware/views.py
new file mode 100755
index 0000000000..c1ee23e893
--- /dev/null
+++ b/tests/middleware/views.py
@@ -0,0 +1,5 @@
+from django.http import HttpResponse

+

+

+def empty_view(request, *args, **kwargs):

+    return HttpResponse('')

diff --git a/tests/model_permalink/urls.py b/tests/model_permalink/urls.py
index cbe50288b8..8ad8a16d05 100644
--- a/tests/model_permalink/urls.py
+++ b/tests/model_permalink/urls.py
@@ -1,5 +1,5 @@
 from django.conf.urls import url
 
 urlpatterns = [
-    url(r'^guitarists/(\w{1,50})/$', 'unimplemented_view_placeholder', name='guitarist_detail'),
+    url(r'^guitarists/(\w{1,50})/$', 'model_permalink.views.empty_view', name='guitarist_detail'),
 ]
diff --git a/tests/model_permalink/views.py b/tests/model_permalink/views.py
new file mode 100755
index 0000000000..c1ee23e893
--- /dev/null
+++ b/tests/model_permalink/views.py
@@ -0,0 +1,5 @@
+from django.http import HttpResponse

+

+

+def empty_view(request, *args, **kwargs):

+    return HttpResponse('')

diff --git a/tests/urlpatterns_reverse/erroneous_urls.py b/tests/urlpatterns_reverse/erroneous_urls.py
index 213642008d..966b219e21 100644
--- a/tests/urlpatterns_reverse/erroneous_urls.py
+++ b/tests/urlpatterns_reverse/erroneous_urls.py
@@ -5,6 +5,8 @@ urlpatterns = [
     url(r'erroneous_inner/$', 'urlpatterns_reverse.views.erroneous_view'),
     # Module has erroneous import
     url(r'erroneous_outer/$', 'urlpatterns_reverse.erroneous_views_module.erroneous_view'),
+    # Module is an unqualified string
+    url(r'erroneous_unqualified/$', 'unqualified_view'),
     # View does not exist
     url(r'missing_inner/$', 'urlpatterns_reverse.views.missing_view'),
     # View is not callable
diff --git a/tests/urlpatterns_reverse/namespace_urls.py b/tests/urlpatterns_reverse/namespace_urls.py
index 19b1b178b7..166189d286 100644
--- a/tests/urlpatterns_reverse/namespace_urls.py
+++ b/tests/urlpatterns_reverse/namespace_urls.py
@@ -10,9 +10,9 @@ class URLObject(object):
 
     def urls(self):
         return ([
-            url(r'^inner/$', 'empty_view', name='urlobject-view'),
-            url(r'^inner/(?P<arg1>[0-9]+)/(?P<arg2>[0-9]+)/$', 'empty_view', name='urlobject-view'),
-            url(r'^inner/\+\\\$\*/$', 'empty_view', name='urlobject-special-view'),
+            url(r'^inner/$', 'urlpatterns_reverse.views.empty_view', name='urlobject-view'),
+            url(r'^inner/(?P<arg1>[0-9]+)/(?P<arg2>[0-9]+)/$', 'urlpatterns_reverse.views.empty_view', name='urlobject-view'),
+            url(r'^inner/\+\\\$\*/$', 'urlpatterns_reverse.views.empty_view', name='urlobject-special-view'),
         ], self.app_name, self.namespace)
     urls = property(urls)
 
diff --git a/tests/urlpatterns_reverse/tests.py b/tests/urlpatterns_reverse/tests.py
index 7c1501a7d3..ade0a7a68d 100644
--- a/tests/urlpatterns_reverse/tests.py
+++ b/tests/urlpatterns_reverse/tests.py
@@ -25,41 +25,41 @@ from .views import empty_view
 
 
 resolve_test_data = (
-    # These entries are in the format: (path, url_name, app_name, namespace, view_func, args, kwargs)
+    # These entries are in the format: (path, url_name, app_name, namespace, view_name, func, args, kwargs)
     # Simple case
-    ('/normal/42/37/', 'normal-view', None, '', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
-    ('/view_class/42/37/', 'view-class', None, '', views.view_class_instance, tuple(), {'arg1': '42', 'arg2': '37'}),
-    ('/included/normal/42/37/', 'inc-normal-view', None, '', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
-    ('/included/view_class/42/37/', 'inc-view-class', None, '', views.view_class_instance, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/normal/42/37/', 'normal-view', None, '', 'normal-view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/view_class/42/37/', 'view-class', None, '', 'view-class', views.view_class_instance, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/included/normal/42/37/', 'inc-normal-view', None, '', 'inc-normal-view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/included/view_class/42/37/', 'inc-view-class', None, '', 'inc-view-class', views.view_class_instance, tuple(), {'arg1': '42', 'arg2': '37'}),
 
     # Unnamed args are dropped if you have *any* kwargs in a pattern
-    ('/mixed_args/42/37/', 'mixed-args', None, '', views.empty_view, tuple(), {'arg2': '37'}),
-    ('/included/mixed_args/42/37/', 'inc-mixed-args', None, '', views.empty_view, tuple(), {'arg2': '37'}),
+    ('/mixed_args/42/37/', 'mixed-args', None, '', 'mixed-args', views.empty_view, tuple(), {'arg2': '37'}),
+    ('/included/mixed_args/42/37/', 'inc-mixed-args', None, '', 'inc-mixed-args', views.empty_view, tuple(), {'arg2': '37'}),
 
-    # Unnamed views will be resolved to the function/class name
-    ('/unnamed/normal/42/37/', 'urlpatterns_reverse.views.empty_view', None, '', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
-    ('/unnamed/view_class/42/37/', 'urlpatterns_reverse.views.ViewClass', None, '', views.view_class_instance, tuple(), {'arg1': '42', 'arg2': '37'}),
+    # Unnamed views should have None as the url_name. Regression data for #21157.
+    ('/unnamed/normal/42/37/', None, None, '', 'urlpatterns_reverse.views.empty_view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/unnamed/view_class/42/37/', None, None, '', 'urlpatterns_reverse.views.ViewClass', views.view_class_instance, tuple(), {'arg1': '42', 'arg2': '37'}),
 
     # If you have no kwargs, you get an args list.
-    ('/no_kwargs/42/37/', 'no-kwargs', None, '', views.empty_view, ('42', '37'), {}),
-    ('/included/no_kwargs/42/37/', 'inc-no-kwargs', None, '', views.empty_view, ('42', '37'), {}),
+    ('/no_kwargs/42/37/', 'no-kwargs', None, '', 'no-kwargs', views.empty_view, ('42', '37'), {}),
+    ('/included/no_kwargs/42/37/', 'inc-no-kwargs', None, '', 'inc-no-kwargs', views.empty_view, ('42', '37'), {}),
 
     # Namespaces
-    ('/test1/inner/42/37/', 'urlobject-view', 'testapp', 'test-ns1', 'empty_view', tuple(), {'arg1': '42', 'arg2': '37'}),
-    ('/included/test3/inner/42/37/', 'urlobject-view', 'testapp', 'test-ns3', 'empty_view', tuple(), {'arg1': '42', 'arg2': '37'}),
-    ('/ns-included1/normal/42/37/', 'inc-normal-view', None, 'inc-ns1', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
-    ('/included/test3/inner/42/37/', 'urlobject-view', 'testapp', 'test-ns3', 'empty_view', tuple(), {'arg1': '42', 'arg2': '37'}),
-    ('/default/inner/42/37/', 'urlobject-view', 'testapp', 'testapp', 'empty_view', tuple(), {'arg1': '42', 'arg2': '37'}),
-    ('/other2/inner/42/37/', 'urlobject-view', 'nodefault', 'other-ns2', 'empty_view', tuple(), {'arg1': '42', 'arg2': '37'}),
-    ('/other1/inner/42/37/', 'urlobject-view', 'nodefault', 'other-ns1', 'empty_view', tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/test1/inner/42/37/', 'urlobject-view', 'testapp', 'test-ns1', 'test-ns1:urlobject-view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/included/test3/inner/42/37/', 'urlobject-view', 'testapp', 'test-ns3', 'test-ns3:urlobject-view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/ns-included1/normal/42/37/', 'inc-normal-view', None, 'inc-ns1', 'inc-ns1:inc-normal-view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/included/test3/inner/42/37/', 'urlobject-view', 'testapp', 'test-ns3', 'test-ns3:urlobject-view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/default/inner/42/37/', 'urlobject-view', 'testapp', 'testapp', 'testapp:urlobject-view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/other2/inner/42/37/', 'urlobject-view', 'nodefault', 'other-ns2', 'other-ns2:urlobject-view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/other1/inner/42/37/', 'urlobject-view', 'nodefault', 'other-ns1', 'other-ns1:urlobject-view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
 
     # Nested namespaces
-    ('/ns-included1/test3/inner/42/37/', 'urlobject-view', 'testapp', 'inc-ns1:test-ns3', 'empty_view', tuple(), {'arg1': '42', 'arg2': '37'}),
-    ('/ns-included1/ns-included4/ns-included2/test3/inner/42/37/', 'urlobject-view', 'testapp', 'inc-ns1:inc-ns4:inc-ns2:test-ns3', 'empty_view', tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/ns-included1/test3/inner/42/37/', 'urlobject-view', 'testapp', 'inc-ns1:test-ns3', 'inc-ns1:test-ns3:urlobject-view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
+    ('/ns-included1/ns-included4/ns-included2/test3/inner/42/37/', 'urlobject-view', 'testapp', 'inc-ns1:inc-ns4:inc-ns2:test-ns3', 'inc-ns1:inc-ns4:inc-ns2:test-ns3:urlobject-view', views.empty_view, tuple(), {'arg1': '42', 'arg2': '37'}),
 
     # Namespaces capturing variables
-    ('/inc70/', 'inner-nothing', None, 'inc-ns5', views.empty_view, tuple(), {'outer': '70'}),
-    ('/inc78/extra/foobar/', 'inner-extra', None, 'inc-ns5', views.empty_view, tuple(), {'outer': '78', 'extra': 'foobar'}),
+    ('/inc70/', 'inner-nothing', None, 'inc-ns5', 'inc-ns5:inner-nothing', views.empty_view, tuple(), {'outer': '70'}),
+    ('/inc78/extra/foobar/', 'inner-extra', None, 'inc-ns5', 'inc-ns5:inner-extra', views.empty_view, tuple(), {'outer': '78', 'extra': 'foobar'}),
 )
 
 test_data = (
@@ -141,8 +141,6 @@ test_data = (
     # once with an explicit argument, and once using the default value on
     # the method. This is potentially ambiguous, as you have to pick the
     # correct view for the arguments provided.
-    ('kwargs_view', '/arg_view/', [], {}),
-    ('kwargs_view', '/arg_view/10/', [], {'arg1': 10}),
     ('urlpatterns_reverse.views.absolute_kwargs_view', '/absolute_arg_view/', [], {}),
     ('urlpatterns_reverse.views.absolute_kwargs_view', '/absolute_arg_view/10/', [], {'arg1': 10}),
     ('non_path_include', '/includes/non_path_include/', [], {}),
@@ -603,7 +601,6 @@ class ErrorHandlerResolutionTests(TestCase):
     """Tests for handler400, handler404 and handler500"""
 
     def setUp(self):
-        from django.core.urlresolvers import RegexURLResolver
         urlconf = 'urlpatterns_reverse.urls_error_handlers'
         urlconf_callables = 'urlpatterns_reverse.urls_error_handlers_callables'
         self.resolver = RegexURLResolver(r'^$', urlconf)
@@ -651,7 +648,7 @@ class NoRootUrlConfTests(TestCase):
 class ResolverMatchTests(TestCase):
 
     def test_urlpattern_resolve(self):
-        for path, name, app_name, namespace, func, args, kwargs in resolve_test_data:
+        for path, url_name, app_name, namespace, view_name, func, args, kwargs in resolve_test_data:
             # Test legacy support for extracting "function, args, kwargs"
             match_func, match_args, match_kwargs = resolve(path)
             self.assertEqual(match_func, func)
@@ -661,12 +658,13 @@ class ResolverMatchTests(TestCase):
             # Test ResolverMatch capabilities.
             match = resolve(path)
             self.assertEqual(match.__class__, ResolverMatch)
-            self.assertEqual(match.url_name, name)
-            self.assertEqual(match.args, args)
-            self.assertEqual(match.kwargs, kwargs)
+            self.assertEqual(match.url_name, url_name)
             self.assertEqual(match.app_name, app_name)
             self.assertEqual(match.namespace, namespace)
+            self.assertEqual(match.view_name, view_name)
             self.assertEqual(match.func, func)
+            self.assertEqual(match.args, args)
+            self.assertEqual(match.kwargs, kwargs)
 
             # ... and for legacy purposes:
             self.assertEqual(match[0], func)
@@ -693,6 +691,9 @@ class ErroneousViewTests(TestCase):
         self.assertRaises(ViewDoesNotExist, self.client.get, '/missing_outer/')
         self.assertRaises(ViewDoesNotExist, self.client.get, '/uncallable/')
 
+        # Regression test for #21157
+        self.assertRaises(ImportError, self.client.get, '/erroneous_unqualified/')
+
     def test_erroneous_reverse(self):
         """
         Ensure that a useful exception is raised when a regex is invalid in the
diff --git a/tests/urlpatterns_reverse/urls.py b/tests/urlpatterns_reverse/urls.py
index 05b925d1d5..bc30280da0 100644
--- a/tests/urlpatterns_reverse/urls.py
+++ b/tests/urlpatterns_reverse/urls.py
@@ -62,14 +62,14 @@ with warnings.catch_warnings(record=True):
         url(r'^(?:foo|bar)(\w+)/$', empty_view, name="disjunction"),
 
         # Regression views for #9038. See tests for more details
-        url(r'arg_view/$', 'kwargs_view'),
-        url(r'arg_view/(?P<arg1>[0-9]+)/$', 'kwargs_view'),
+        url(r'arg_view/$', 'urlpatterns_reverse.views.kwargs_view'),
+        url(r'arg_view/(?P<arg1>[0-9]+)/$', 'urlpatterns_reverse.views.kwargs_view'),
         url(r'absolute_arg_view/(?P<arg1>[0-9]+)/$', absolute_kwargs_view),
         url(r'absolute_arg_view/$', absolute_kwargs_view),
 
         # Tests for #13154. Mixed syntax to test both ways of defining URLs.
-        url(r'defaults_view1/(?P<arg1>[0-9]+)/', 'defaults_view', {'arg2': 1}, name='defaults'),
-        (r'defaults_view2/(?P<arg1>[0-9]+)/', 'defaults_view', {'arg2': 2}, 'defaults'),
+        url(r'defaults_view1/(?P<arg1>[0-9]+)/', 'urlpatterns_reverse.views.defaults_view', {'arg2': 1}, name='defaults'),
+        (r'defaults_view2/(?P<arg1>[0-9]+)/', 'urlpatterns_reverse.views.defaults_view', {'arg2': 2}, 'defaults'),
 
         url('^includes/', include(other_patterns)),
     )