Ticket #2977: new_reverse_urlresolver.3.patch

django/core/urlresolvers.py

@@ -11 +11 @@
 from django.core.exceptions import ImproperlyConfigured, ViewDoesNotExist
 import re
 
+re_bracket = re.compile(r'(?<!\\)((?:\\\\)*)([()])')  # Open or close bracket not preceeded by a single slash
+re_type = type(re_bracket)
+
+character_set_pattern = (r'(?<!\\)((?:\\\\)*)\[((?:(?!\\)(?:\\\\)*\]|[^\]])+)\]')
+re_unescape = re.compile(r'\\(.)|[$?*+^()]|%s' % character_set_pattern)
+def unescape(value):
+    """
+    Unescape a regex string.
+    
+    Removes any re characters used for start/end/repetition, unescapes any
+    escaped characters, and replaces character sets with the first matching
+    character in that set.
+    """
+    def repl(m):
+        escaped, slashes, contents = m.groups()
+        if contents:
+            # We're in a character set.
+            if contents[0] == '\\':
+                contents = contents[:2]
+            else:
+                contents = contents[:1]
+            return '%s%s' % (slashes, contents)
+        elif escaped and re.match(r'[\ddDsSwW]', escaped):
+            # These cases shouldn't ever come up - no match possible if they do.
+            raise ValueError(r"Regular expression notation '\%s' was outside of a group so this pattern is not reversable" % escaped)
+        elif escaped and escaped in 'AZbB':
+            # These cases should just return nothing.
+            return ''
+        # For every other case: if it's the escaped version then return it without
+        # a slash, otherwise return nothing.
+        return escaped or ''
+    return re_unescape.sub(repl, value)
+
 class Resolver404(Http404):
     pass
 
@@ -42 +75 @@
 
     Raises NoReverseMatch if the args/kwargs aren't valid for the regex.
     """
-    # TODO: Handle nested parenthesis in the following regex.
-    result = re.sub(r'\(([^)]+)\)', MatchChecker(args, kwargs), regex.pattern)
-    return result.replace('^', '').replace('$', '')
+    # Regex can either be a string or a regular epression.
+    if isinstance(regex, re_type):
+        regex = regex.pattern
+    return ReverseRegexLookup(regex).check(args, kwargs)
 
-class MatchChecker(object):
-    "Class used in reverse RegexURLPattern lookup."
-    def __init__(self, args, kwargs):
-        self.args, self.kwargs = args, kwargs
-        self.current_arg = 0
+def tokenize(text):
+    """
+    Recursive tokenizer for regular expression parenthesis.
+    """
+    def parse(text, top=True, named_group=False):
+        bits = []
+        m = re_bracket.search(text)
+        while m:
+            before, text = text[:m.start()+len(m.group(1))], text[m.end():]
+            if before:
+                bits.append(before)
+            if m.group(2) != '(':
+                break
+            inner_bits, text, named_group = parse(text, top=False, named_group=not top and named_group)
+            if inner_bits:
+                inline = named_group
+                first_bit = inner_bits[0]
+                if isinstance(first_bit, str):
+                    if first_bit.startswith('?'):
+                        # Regex extension notation.
+                        if first_bit.startswith('?:'):
+                            # No need to parse this non-grouping parenthesis.
+                            inline = True
+                            inner_bits[0] = first_bit[2:]
+                        elif first_bit.startswith('?P'):
+                            # Named group, set variable so higher levels will flatten.
+                            named_group = True
+                        else:
+                            # Skip all other extension notation.
+                            inner_bits = None
+            if inner_bits:
+                if inline:
+                    bits.extend(inner_bits)
+                else:
+                    bits.append(inner_bits)
+            m = re_bracket.search(text)
+        return bits, text, named_group
 
-    def __call__(self, match_obj):
-        # match_obj.group(1) is the contents of the parenthesis.
-        # First we need to figure out whether it's a named or unnamed group.
-        #
-        grouped = match_obj.group(1)
-        m = re.search(r'^\?P<(\w+)>(.*?)$', grouped)
-        if m: # If this was a named group...
-            # m.group(1) is the name of the group
-            # m.group(2) is the regex.
+    def build_re(bits):
+        output = []
+        for bit in bits:
+            if isinstance(bit, list):
+                bit = build_re(bit)
+            output.append(bit)
+        return '(%s)' % ''.join(output)
+
+    bits, text, named_group = parse(text)
+    if text:
+        bits.append(text)
+    # Now tokenize the bits. Each token will either be a string or a regex.
+    tokens = []
+    count = 0
+    error = None
+    for bit in bits:
+        if isinstance(bit, list):
+            # Build the regex here so it only has to be compiled once.
+            bit = re.compile('%s$' % build_re(bit))
+            count += 1
+        else:
+            # Unescape special characters which could possibly be used in a URL
+            # and strip unused regular expression syntax.
             try:
-                value = self.kwargs[m.group(1)]
-            except KeyError:
-                # It was a named group, but the arg was passed in as a
-                # positional arg or not at all.
-                try:
-                    value = self.args[self.current_arg]
-                    self.current_arg += 1
-                except IndexError:
-                    # The arg wasn't passed in.
-                    raise NoReverseMatch('Not enough positional arguments passed in')
-            test_regex = m.group(2)
-        else: # Otherwise, this was a positional (unnamed) group.
-            try:
-                value = self.args[self.current_arg]
-                self.current_arg += 1
-            except IndexError:
-                # The arg wasn't passed in.
-                raise NoReverseMatch('Not enough positional arguments passed in')
-            test_regex = grouped
-        # Note we're using re.match here on purpose because the start of
-        # to string needs to match.
-        if not re.match(test_regex + '$', str(value)): # TODO: Unicode?
-            raise NoReverseMatch("Value %r didn't match regular expression %r" % (value, test_regex))
-        return str(value) # TODO: Unicode?
+                bit = unescape(bit)
+            except ValueError, err:
+                error = err
+                break
+        tokens.append(bit)
+    return tokens, count, error
 
+class ReverseRegexLookup(object):
+    def __init__(self, text):
+        self.tokens, self.minimum_arguments, self.error = tokenize(text)
+
+    def check(self, args=[], kwargs={}):
+        # Note: args and kwargs will be destroyed (using .pop()) so if you need
+        # to keep using them, pass copies.
+        if self.error:
+            raise NoReverseMatch(self.error)
+        if self.minimum_arguments > len(args) + len(kwargs):
+            raise NoReverseMatch('Not enough arguments passed in')
+        match = []
+        args = list(args)
+        kwargs = kwargs.copy()
+        for token in self.tokens:
+            if isinstance(token, re_type):   # A regex token.
+                value = None
+                # Is it a named argument?
+                if token.groupindex:
+                    try:
+                        value = kwargs.pop(token.groupindex.keys()[0])
+                    except KeyError:
+                        # It was a named group, but the arg was passed in as a
+                        # positional arg or not at all.
+                        pass
+                if value is None:
+                    try:
+                        value = args.pop(0)
+                    except IndexError:
+                        # The arg wasn't passed in.
+                        raise NoReverseMatch('Not enough positional arguments passed in')
+                value = str(value)   # TODO: Unicode?
+                if not token.match(value):
+                    raise NoReverseMatch("Value %r didn't match regular expression %r" % (value, token.pattern))
+                match.append(value)
+            else:    # A string token.
+                match.append(token)
+        match = ''.join(match)
+        return match
+
 class RegexURLPattern(object):
     def __init__(self, regex, callback, default_args=None, name=None):
         # regex is a string representing a regular expression.
@@ -94 +196 @@
         # which represents the path to a module and a view function name, or a
         # callable object (view).
         self.regex = re.compile(regex)
+        self.reverse_regex_lookup = ReverseRegexLookup(regex)
         if callable(callback):
             self._callback = callback
         else:
@@ -150 +253 @@
         return self.reverse_helper(*args, **kwargs)
 
     def reverse_helper(self, *args, **kwargs):
-        return reverse_helper(self.regex, *args, **kwargs)
+        return self.reverse_regex_lookup.check(args, kwargs)
 
 class RegexURLResolver(object):
     def __init__(self, regex, urlconf_name, default_kwargs=None):
         # regex is a string representing a regular expression.
         # urlconf_name is a string representing the module containing urlconfs.
         self.regex = re.compile(regex)
+        self.reverse_regex_lookup = ReverseRegexLookup(regex)
         self.urlconf_name = urlconf_name
         self.callback = None
         self.default_kwargs = default_kwargs or {}
@@ -230 +334 @@
         raise NoReverseMatch
 
     def reverse_helper(self, lookup_view, *args, **kwargs):
+        result = self.reverse_regex_lookup.check(args, kwargs)
+        # .check() swallows used args, so the resolver is checking both itself
+        # and its children using the one set of arguments.
         sub_match = self.reverse(lookup_view, *args, **kwargs)
-        result = reverse_helper(self.regex, *args, **kwargs)
         return result + sub_match
 
 def resolve(path, urlconf=None):

tests/regressiontests/urlpatterns_reverse/tests.py

@@ -23 +23 @@
     ('^people/(?P<state>\w\w)/(?P<name>\w+)/$', NoReverseMatch, [], {'name': 'adrian'}),
     ('^people/(?P<state>\w\w)/(\w+)/$', NoReverseMatch, ['il'], {'name': 'adrian'}),
     ('^people/(?P<state>\w\w)/(\w+)/$', 'people/il/adrian/', ['adrian'], {'state': 'il'}),
+
+    ('^places?/$', 'places/', [], {}),
+    ('^places+/$', 'places/', [], {}),
+    ('^places*/$', 'places/', [], {}),
+    ('^people/(?:name/)', 'people/name/', [], {}),
+    ('^people/(?:name/)?', 'people/name/', [], {}),
+    ('^character_set/[abcdef0-9]/$', 'character_set/a/', [], {}),
+    (r'^people/((?P<state>\w\w)/test)?/(\w+)/$', 'people/il/test/adrian/', ['adrian'], {'state': 'il'}),
+    (r'^people/((?P<state>\w\w)/test)?/(\w+)/$', NoReverseMatch, ['adrian'], {}),
+
+    (r'^places/(\d+|[a-z_]+)/', 'places/4/', [4], {}),
+    (r'^places/(\d+|[a-z_]+)/', 'places/harlem/', ['harlem'], {}),
+    (r'^places/(\d+|[a-z_]+)/', NoReverseMatch, ['harlem64'], {}),
+
+    (r'^price/\$(\d+)/$', 'price/$10/', ['10'], {}),
+    (r'^price/[$](\d+)/$', 'price/$10/', ['10'], {}),
+    (r'^price/[\$](\d+)/$', 'price/$10/', ['10'], {}),
+    (r'^product/(?P<product>\w+)\+\(\$(?P<price>\d+(\.\d+)?)\)/$', 'product/chocolate+($2.00)/', ['2.00'], {'product': 'chocolate'}),
+    (r'^headlines/(?P<year>\d+)\.(?P<month>\d+)\.(?P<day>\d+)/$', 'headlines/2007.5.21/', [], dict(year=2007, month=5, day=21)),
+    (r'^windows_path/(?P<drive_name>[A-Z]):\\(?P<path>.+)/$', r'windows_path/C:\Documents and Settings\spam/', [], dict(drive_name='C', path=r'Documents and Settings\spam')),
+    (r'^special_chars/(.+)/$', r'special_chars/+\$*/', [r'+\$*'], {}),
+    (r'^(?P<name>.+)/\d+/$', NoReverseMatch, [], dict(name='john')),
 )
 
 class URLPatternReverse(unittest.TestCase):
     def test_urlpattern_reverse(self):
         for regex, expected, args, kwargs in test_data:
             try:
-                got = reverse_helper(re.compile(regex), *args, **kwargs)
+                got = reverse_helper(regex, *args, **kwargs)
             except NoReverseMatch, e:
                 self.assertEqual(expected, NoReverseMatch)
             else:
                 self.assertEquals(got, expected)
 
 if __name__ == "__main__":
-    run_tests(1)
+    unittest.main()