From 91afc00513bd2fa6302ea2be35e1f842cbd5fd38 Mon Sep 17 00:00:00 2001 From: Kevin Christopher Henry Date: Thu, 3 Oct 2013 03:20:04 -0400 Subject: [PATCH] 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). --- django/core/urlresolvers.py | 113 +++++++++++--------- tests/middleware/extra_urls.py | 6 +- tests/middleware/urls.py | 6 +- tests/middleware/views.py | 5 + tests/model_permalink/urls.py | 2 +- tests/model_permalink/views.py | 5 + tests/urlpatterns_reverse/erroneous_urls.py | 2 + tests/urlpatterns_reverse/namespace_urls.py | 6 +- tests/urlpatterns_reverse/tests.py | 61 +++++------ tests/urlpatterns_reverse/urls.py | 8 +- 10 files changed, 121 insertions(+), 93 deletions(-) create mode 100755 tests/middleware/views.py create mode 100755 tests/model_permalink/views.py 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. + 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 the lookup_view is not an import path, it is assumed to be a URL pattern - label and the original string is returned. - - 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 == '': - return lookup_view + if callable(lookup_view): + return lookup_view - try: - mod = import_module(mod_name) - except ImportError: - parentmod, submod = get_mod_func(mod_name) - if (not can_fail and submod != '' and - not module_has_submodule(import_module(parentmod), submod)): - raise ViewDoesNotExist( - "Could not import %s. Parent module %s does not exist." % - (lookup_view, mod_name)) - if not can_fail: - raise + mod_name, func_name = get_mod_func(lookup_view) + if not func_name: # No '.' in lookup_view + if can_fail: + return lookup_view 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 + raise ImportError( + "Could not import '%s'. The path must be fully qualified." % + lookup_view) + + try: + mod = import_module(mod_name) + except ImportError: + if can_fail: + return lookup_view + else: + parentmod, submod = get_mod_func(mod_name) + if submod and not module_has_submodule(import_module(parentmod), submod): + raise ViewDoesNotExist( + "Could not import '%s'. Parent module %s does not exist." % + (lookup_view, mod_name)) + 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: + 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[0-9]+)/(?P[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[0-9]+)/(?P[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[0-9]+)/$', 'kwargs_view'), + url(r'arg_view/$', 'urlpatterns_reverse.views.kwargs_view'), + url(r'arg_view/(?P[0-9]+)/$', 'urlpatterns_reverse.views.kwargs_view'), url(r'absolute_arg_view/(?P[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[0-9]+)/', 'defaults_view', {'arg2': 1}, name='defaults'), - (r'defaults_view2/(?P[0-9]+)/', 'defaults_view', {'arg2': 2}, 'defaults'), + url(r'defaults_view1/(?P[0-9]+)/', 'urlpatterns_reverse.views.defaults_view', {'arg2': 1}, name='defaults'), + (r'defaults_view2/(?P[0-9]+)/', 'urlpatterns_reverse.views.defaults_view', {'arg2': 2}, 'defaults'), url('^includes/', include(other_patterns)), )