Fixed #23813 -- Added checks for common URL pattern errors

Thanks jwa and lamby for the suggestions, and timgraham and jarshwah
for their reviews.

django/core/checks/__init__.py

@@ -15,6 +15,8 @@ import django.core.checks.security.base  # NOQA isort:skip
 import django.core.checks.security.csrf  # NOQA isort:skip
 import django.core.checks.security.sessions  # NOQA isort:skip
 import django.core.checks.templates  # NOQA isort:skip
+import django.core.checks.urls  # NOQA isort:skip
+
 
 __all__ = [
     'CheckMessage',

django/core/checks/registry.py

@@ -17,6 +17,7 @@ class Tags(object):
     security = 'security'
     signals = 'signals'
     templates = 'templates'
+    urls = 'urls'
 
 
 class CheckRegistry(object):

django/core/checks/urls.py

@@ -0,0 +1,87 @@
+from __future__ import unicode_literals
+
+from . import Tags, Warning, register
+
+
+@register(Tags.urls)
+def check_url_config(app_configs, **kwargs):
+    from django.core.urlresolvers import get_resolver
+    resolver = get_resolver()
+    return check_resolver(resolver)
+
+
+def check_resolver(resolver):
+    """
+    Recursively check the resolver
+    """
+    from django.core.urlresolvers import RegexURLPattern, RegexURLResolver
+    warnings = []
+    for pattern in resolver.url_patterns:
+        if isinstance(pattern, RegexURLResolver):
+            warnings.extend(check_include_trailing_dollar(pattern))
+            # Check resolver recursively
+            warnings.extend(check_resolver(pattern))
+        elif isinstance(pattern, RegexURLPattern):
+            warnings.extend(check_pattern_name(pattern))
+
+        warnings.extend(check_pattern_startswith_slash(pattern))
+
+    return warnings
+
+
+def describe_pattern(pattern):
+    """
+    Formats the URL pattern for display in warning messages
+    """
+    description = "'{}'".format(pattern.regex.pattern)
+    if getattr(pattern, 'name', False):
+        description += " [name='{}']".format(pattern.name)
+    return description
+
+
+def check_include_trailing_dollar(pattern):
+    """
+    Checks that include is not used with a regex ending with a dollar.
+    """
+    regex_pattern = pattern.regex.pattern
+    if regex_pattern.endswith('$') and not regex_pattern.endswith('\$'):
+        warning = Warning(
+            "Your URL pattern {} uses include with a regex ending with a '$'. "
+            "Remove the dollar from the regex to avoid problems including "
+            "URLs.".format(describe_pattern(pattern)),
+            id="urls.W001",
+        )
+        return [warning]
+    else:
+        return []
+
+
+def check_pattern_startswith_slash(pattern):
+    """
+    Checks that the pattern does not begin with a forward slash
+    """
+    regex_pattern = pattern.regex.pattern
+    if regex_pattern.startswith('/') or regex_pattern.startswith('^/'):
+        warning = Warning(
+            "Your URL pattern {} has a regex beginning with a '/'. "
+            "Remove this slash as it is unnecessary.".format(describe_pattern(pattern)),
+            id="urls.W002",
+        )
+        return [warning]
+    else:
+        return []
+
+
+def check_pattern_name(pattern):
+    """
+    Checks that the pattern name does not contain a colon
+    """
+    if pattern.name is not None and ":" in pattern.name:
+        warning = Warning(
+            "Your URL pattern {} has a name including a ':'. Remove the colon, to "
+            "avoid ambiguous namespace references.".format(describe_pattern(pattern)),
+            id="urls.W003",
+        )
+        return [warning]
+    else:
+        return []

django/core/urlresolvers.py

@@ -146,7 +146,7 @@ def get_callable(lookup_view, can_fail=False):
 
 
 @lru_cache.lru_cache(maxsize=None)
-def get_resolver(urlconf):
+def get_resolver(urlconf=None):
     if urlconf is None:
         from django.conf import settings
         urlconf = settings.ROOT_URLCONF

docs/ref/checks.txt

@@ -81,6 +81,7 @@ Django's system checks are organized using the following tags:
 * ``security``: Checks security related configuration.
 * ``templates``: Checks template related configuration.
 * ``caches``: Checks cache related configuration.
+* ``urls``: Checks URL configuration.
 
 Some checks may be registered with multiple tags.
 
@@ -580,3 +581,18 @@ configured:
 
 * **caches.E001**: You must define a ``'default'`` cache in your
   :setting:`CACHES` setting.
+
+URLs
+----
+
+The following checks are performed on your URL configuration:
+
+* **urls.W001**: Your URL pattern ``<pattern>`` uses
+  :func:`~django.conf.urls.include` with a ``regex`` ending with a
+  ``$``. Remove the dollar from the ``regex`` to avoid problems
+  including URLs.
+* **urls.W002**: Your URL pattern ``<pattern>`` has a ``regex``
+  beginning with a ``/``. Remove this slash as it is unnecessary.
+* **urls.W003**: Your URL pattern ``<pattern>`` has a ``name``
+  including a ``:``. Remove the colon, to avoid ambiguous namespace
+  references.

docs/releases/1.9.txt

@@ -641,6 +641,8 @@ URLs
   of (<list of patterns>, <application namespace>) as the first argument to
   :func:`~django.conf.urls.include`.
 
+* System checks have been added for common URL pattern mistakes.
+
 Validators
 ^^^^^^^^^^
 

tests/check_framework/test_urls.py

@@ -0,0 +1,37 @@
+from django.core.checks.urls import check_url_config
+from django.test import SimpleTestCase
+from django.test.utils import override_settings
+
+
+class CheckUrlsTest(SimpleTestCase):
+    @override_settings(ROOT_URLCONF='check_framework.urls_no_warnings')
+    def test_include_no_warnings(self):
+        result = check_url_config(None)
+        self.assertEqual(result, [])
+
+    @override_settings(ROOT_URLCONF='check_framework.urls_include')
+    def test_include_with_dollar(self):
+        result = check_url_config(None)
+        self.assertEqual(len(result), 1)
+        warning = result[0]
+        self.assertEqual(warning.id, 'urls.W001')
+        expected_msg = "Your URL pattern '^include-with-dollar$' uses include with a regex ending with a '$'."
+        self.assertIn(expected_msg, warning.msg)
+
+    @override_settings(ROOT_URLCONF='check_framework.urls_slash')
+    def test_url_beginning_with_slash(self):
+        result = check_url_config(None)
+        self.assertEqual(len(result), 1)
+        warning = result[0]
+        self.assertEqual(warning.id, 'urls.W002')
+        expected_msg = "Your URL pattern '/starting-with-slash/$' has a regex beginning with a '/'"
+        self.assertIn(expected_msg, warning.msg)
+
+    @override_settings(ROOT_URLCONF='check_framework.urls_name')
+    def test_url_pattern_name_with_colon(self):
+        result = check_url_config(None)
+        self.assertEqual(len(result), 1)
+        warning = result[0]
+        self.assertEqual(warning.id, 'urls.W003')
+        expected_msg = "Your URL pattern '^$' [name='name_with:colon'] has a name including a ':'."
+        self.assertIn(expected_msg, warning.msg)

tests/check_framework/urls_include.py

@@ -0,0 +1,7 @@
+from django.conf.urls import include, url
+
+urlpatterns = [
+    url(r'^', include([
+        url(r'^include-with-dollar$', include([])),
+    ])),
+]

tests/check_framework/urls_name.py

@@ -0,0 +1,13 @@
+from django.conf.urls import include, url
+from django.http import HttpResponse
+
+
+def view(request):
+    return HttpResponse('')
+
+
+urlpatterns = [
+    url('^', include([
+        url(r'^$', view, name='name_with:colon'),
+    ])),
+]

tests/check_framework/urls_no_warnings.py

@@ -0,0 +1,15 @@
+from django.conf.urls import include, url
+from django.http import HttpResponse
+
+
+def view(request):
+    return HttpResponse('')
+
+
+urlpatterns = [
+    url(r'^foo/', view, name='foo'),
+    # This dollar is ok as it is escaped
+    url(r'^\$', include([
+        url(r'^bar/$', view, name='bar'),
+    ])),
+]

tests/check_framework/urls_slash.py

@@ -0,0 +1,13 @@
+from django.conf.urls import include, url
+from django.http import HttpResponse
+
+
+def view(request):
+    return HttpResponse('')
+
+
+urlpatterns = [
+    url('^', include([
+        url(r'/starting-with-slash/$', view),
+    ])),
+]