Fix a few url generation bugs and add complexity failsafe.

closes #9
closes #10

Author: bckohan

doc/source/configuration.rst

@@ -151,3 +151,19 @@ parameter must contain the full path where the template will be rendered includi
 
 Provide additional parameters for each template in the ``context`` dictionary. Any context variables
 specified here that clash with global context variables will override them.
+
+
+``RENDER_STATIC_REVERSAL_LIMIT``
+--------------------------------
+
+The guess and check reversal mechanism used to ensure that `urls_to_js` produces the same reversals
+as Django's `reverse` is an **O(n^p)** operation where **n** is the number of placeholder candidates
+to try and **p** is the number of arguments in the url. Its possible for this to produce a
+complexity explosion for rare cases where the URL has a large number of arguments with unregistered
+placeholders. A limit on the number of tries is enforced to guard against this. User's may adjust
+the limit via the ``RENDER_STATIC_REVERSAL_LIMIT`` settings parameter. By default it is 2**14 tries
+which runs in ~seconds per URL.
+
+The solution if this limit is hit, is to provide more specific placeholders as placeholders are
+attempted in order of specificity where specificity is defined by url name, variable name,
+app name and/or converter type.

doc/source/reference.rst

@@ -65,6 +65,7 @@ exceptions
 .. automodule:: render_static.exceptions
 
    .. autoclass:: URLGenerationFailed
+   .. autoclass:: ReversalLimitHit
 
 
 .. _placeholders:

render_static/exceptions.py

@@ -10,3 +10,10 @@ class URLGenerationFailed(Exception):
     Thrown by `urls_to_js` under any circumstance where URL generation fails for a specific
     fully qualified URL name.
     """
+
+
+class ReversalLimitHit(Exception):
+    """
+    Thrown by `urls_to_js` under any circumstance where the configured maximum number of tries has
+    been hit when attempting to reverse a URL.
+    """

render_static/placeholders.py

@@ -101,7 +101,7 @@ def register_unnamed_placeholders(
 ) -> None:
     """
     Register a list of placeholders for a url_name and optionally an app_name that takes unnamed
-    arguments.
+    arguments. The list indices should correspond to the argument order as passed to reverse.
 
     :param url_name: The name of the url path to register the placeholders for
     :param placeholders: The list of placeholders to use
@@ -147,22 +147,35 @@ def resolve_placeholders(
 
 def resolve_unnamed_placeholders(
         url_name: str,
-        app_name: Optional[str] = None
+        nargs: int,
+        app_name: Optional[str] = None,
 ) -> Iterable:
     """
     Resolve placeholders to use for a url with unnamed parameters based on the url name and
     optionally the app_name.
 
     :param url_name: The name of the URL to search for
+    :param nargs: The number of arguments for this url
     :param app_name: The optional app_name to search for
-    :return: A list of lists of placeholders to try
+    :return: A list of lists of placeholders to try, where the outer list is indexed by argument
+        index
     """
 
-    placeholders = []
+    placeholders: List[List[object]] = [[] for arg in range(0, nargs)]
+
+    def add_candidates(candidates: List[List[object]]) -> None:
+        for candidate in candidates:
+            if len(candidate) == nargs:
+                for idx, arg in enumerate(candidate):
+                    placeholders[idx].append(arg)
     if app_name:
-        placeholders.extend(app_unnamed_placeholders.get(app_name, {}).get(url_name, []))
-    placeholders.extend(unnamed_placeholders.get(url_name, []))
-    return placeholders + [always for always in ALWAYS_TRY_THESE if always not in placeholders]
+        add_candidates(app_unnamed_placeholders.get(app_name, {}).get(url_name, []))
+    add_candidates(unnamed_placeholders.get(url_name, []))
+
+    for arg in placeholders:
+        arg.extend([always for always in ALWAYS_TRY_THESE if always not in arg])
+
+    return placeholders
 
 
 register_variable_placeholder('app_label', 'site', app_name='admin')

render_static/tests/tests.py

@@ -4,8 +4,10 @@
 import os
 import shutil
 import subprocess
+import traceback
 import uuid
 from pathlib import Path
+from time import perf_counter
 
 import js2py
 from deepdiff import DeepDiff
@@ -17,6 +19,7 @@
 from django.template.utils import InvalidTemplateEngineError
 from django.test import TestCase, override_settings
 from django.urls import reverse
+from django.urls.exceptions import NoReverseMatch
 from django.utils.module_loading import import_string
 from render_static import placeholders
 from render_static.backends import StaticDjangoTemplates, StaticJinja2Templates
@@ -1538,7 +1541,7 @@ def tearDown(self):
 
 
 @override_settings(ROOT_URLCONF='render_static.tests.urls2')
-class UnregisteredURLTest(URLJavascriptMixin, BaseTestCase):
+class CornerCaseTest(URLJavascriptMixin, BaseTestCase):
 
     def setUp(self):
         self.clear_placeholder_registries()
@@ -1574,7 +1577,38 @@ def test_no_default_registered(self):
         self.compare('default', args=['unnamed'])
 
     @override_settings(STATIC_TEMPLATES={
-        'context': {'include': ['default']},
+        'ENGINES': [{
+            'BACKEND': 'render_static.backends.StaticDjangoTemplates',
+            'OPTIONS': {
+                'loaders': [
+                    ('render_static.loaders.StaticLocMemLoader', {
+                        'urls.js': ('{% urls_to_js '
+                                    'visitor="render_static.ClassURLWriter" '
+                                    'include=include '
+                                    '%}')
+                    })
+                ],
+                'builtins': ['render_static.templatetags.render_static']
+            },
+        }],
+        'templates': {'urls.js': {'context': {'include': ['no_capture']}}}
+    })
+    def test_non_capturing_unnamed(self):
+        """
+        Tests that unnamed arguments can still work when the users also include non-capturing groups
+        for whatever reason. Hard to imagine an actual use case for these - but reverse still seems
+        to work, so javascript reverse should too
+        :return:
+        """
+        self.es6_mode = True
+        self.url_js = None
+        self.class_mode = ClassURLWriter.class_name_
+
+        placeholders.register_unnamed_placeholders('no_capture', ['0000'])
+        call_command('render_static', 'urls.js')
+        self.compare('no_capture', args=['5555'])
+
+    @override_settings(STATIC_TEMPLATES={
         'ENGINES': [{
             'BACKEND': 'render_static.backends.StaticDjangoTemplates',
             'OPTIONS': {
@@ -1599,22 +1633,20 @@ def test_named_unnamed_conflation1(self):
         self.url_js = None
         self.class_mode = ClassURLWriter.class_name_
 
-        print(reverse('special', kwargs={'choice': 'first'}))
-        print(reverse('special', args=['first']))
-
         self.assertRaises(CommandError, lambda: call_command('render_static', 'urls.js'))
 
         placeholders.register_variable_placeholder('choice', 'first')
+        placeholders.register_variable_placeholder('choice1', 'second')
         self.assertRaises(CommandError, lambda: call_command('render_static', 'urls.js'))
-        placeholders.register_unnamed_placeholders('special', ['first'])
+        placeholders.register_unnamed_placeholders('special', ['third'])
 
         call_command('render_static', 'urls.js')
-        self.compare('special', {'choice': 'first'})
-        self.compare('special', ['first'])
-
+        self.compare('special', {'choice': 'second'})
+        self.compare('special', {'choice': 'second', 'choice1': 'first'})
+        self.compare('special', args=['third'])
 
     @override_settings(
-        ROOT_URLCONF='render_static.tests.urls2',
+        ROOT_URLCONF='render_static.tests.urls3',
         STATIC_TEMPLATES={
             'context': {'include': ['default']},
             'ENGINES': [{
@@ -1642,23 +1674,130 @@ def test_named_unnamed_conflation2(self):
         self.url_js = None
         self.class_mode = ClassURLWriter.class_name_
 
-        print(reverse('special', kwargs={'choice': 'first'}))
-        print(reverse('special', args=['first']))
-
         self.assertRaises(CommandError, lambda: call_command('render_static', 'urls.js'))
 
         placeholders.register_variable_placeholder('choice', 'first')
+        placeholders.register_variable_placeholder('choice1', 'second')
         self.assertRaises(CommandError, lambda: call_command('render_static', 'urls.js'))
+        placeholders.register_unnamed_placeholders('special', ['third'])
+
+        call_command('render_static', 'urls.js')
+        self.compare('special', {'choice': 'second'})
+        self.compare('special', {'choice': 'second', 'choice1': 'first'})
+        self.compare('special', args=['third'])
+
+    @override_settings(
+        ROOT_URLCONF='render_static.tests.urls4',
+        STATIC_TEMPLATES={
+            'context': {'include': ['default']},
+            'ENGINES': [{
+                'BACKEND': 'render_static.backends.StaticDjangoTemplates',
+                'OPTIONS': {
+                    'loaders': [
+                        ('render_static.loaders.StaticLocMemLoader', {
+                            'urls.js': '{% urls_to_js visitor="render_static.ClassURLWriter" %}'
+                        })
+                    ],
+                    'builtins': ['render_static.templatetags.render_static']
+                },
+            }]
+        }
+    )
+    def test_named_unnamed_conflation3(self):
+        """
+        This tests surfaces what appears to be a Django bug in reverse(). urls_to_js should not
+        fail in this circumstance, but should leave a comment breadcrumb in the generated JS that
+        indicates why no reversal was produced - alternatively if bug is fixed it should also pass
+
+        https://github.com/bckohan/django-render-static/issues/9
+        """
+        self.es6_mode = True
+        self.url_js = None
+        self.class_mode = ClassURLWriter.class_name_
+
+        placeholders.register_variable_placeholder('choice', 'first')
         placeholders.register_unnamed_placeholders('special', ['first'])
+        call_command('render_static', 'urls.js')
+
+        with open(GLOBAL_STATIC_DIR / 'urls.js', 'r') as urls:
+            if 'reverse matched unexpected pattern' not in urls.read():
+                self.compare('special', kwargs={'choice': 'first'})  # pragma: no cover
+                self.compare('special', args=['first'])  # pragma: no cover
+
+        self.assertTrue(True)
 
+    @override_settings(
+        STATIC_TEMPLATES={
+            'context': {'include': ['bad_mix']},
+            'ENGINES': [{
+                'BACKEND': 'render_static.backends.StaticDjangoTemplates',
+                'OPTIONS': {
+                    'loaders': [
+                        ('render_static.loaders.StaticLocMemLoader', {
+                            'urls.js': '{% urls_to_js '
+                                       'visitor="render_static.ClassURLWriter" '
+                                       'include=include %}'
+                        })
+                    ],
+                    'builtins': ['render_static.templatetags.render_static']
+                },
+            }]
+        }
+    )
+    def test_named_unnamed_bad_mix(self):
+        """
+        Mix of named and unnamed arguments should not be reversible!
+        """
+        self.es6_mode = True
+        self.url_js = None
+        self.class_mode = ClassURLWriter.class_name_
+
+        placeholders.register_variable_placeholder('named', '1111')
+        placeholders.register_unnamed_placeholders('bad_mix', ['unnamed'])
         call_command('render_static', 'urls.js')
-        self.compare('special', {'choice': 'first'})
-        self.compare('special', ['first'])
 
+        with open(GLOBAL_STATIC_DIR / 'urls.js', 'r') as urls:
+            self.assertTrue('this path may not be reversible' in urls.read())
+
+        self.assertRaises(
+            ValueError,
+            lambda: reverse(
+                'bad_mix',
+                kwargs={'named': '1111'}, args=['unnamed'])
+        )
+        self.assertRaises(NoReverseMatch, lambda: reverse('bad_mix', kwargs={'named': '1111'}))
+
+    @override_settings(
+        STATIC_TEMPLATES={
+            'context': {'include': ['bad_mix2']},
+            'ENGINES': [{
+                'BACKEND': 'render_static.backends.StaticDjangoTemplates',
+                'OPTIONS': {
+                    'loaders': [
+                        ('render_static.loaders.StaticLocMemLoader', {
+                            'urls.js': '{% urls_to_js '
+                                       'visitor="render_static.ClassURLWriter" '
+                                       'include=include %}'
+                        })
+                    ],
+                    'builtins': ['render_static.templatetags.render_static']
+                },
+            }]
+        }
+    )
+    def test_named_unnamed_bad_mix2(self):
+        """
+        Mix of named and unnamed arguments should not be reversible!
+        """
+        self.es6_mode = True
+        self.url_js = None
+        self.class_mode = ClassURLWriter.class_name_
+
+        placeholders.register_variable_placeholder('named', '1111')
+        placeholders.register_unnamed_placeholders('bad_mix2', ['unnamed'])
+        self.assertRaises(CommandError, lambda: call_command('render_static', 'urls.js'))
 
-    """
     @override_settings(STATIC_TEMPLATES={
-        'context': {'include': ['default']},
         'ENGINES': [{
             'BACKEND': 'render_static.backends.StaticDjangoTemplates',
             'OPTIONS': {
@@ -1673,20 +1812,58 @@ def test_named_unnamed_conflation2(self):
                 'builtins': ['render_static.templatetags.render_static']
             },
         }],
-        'templates': {'urls.js': {'context': {'include': ['special']}}}
+        'templates': {'urls.js': {'context': {'include': ['complex']}}}
     })
     def test_complexity_boundary(self):
+        """
         https://github.com/bckohan/django-render-static/issues/10
+
         For URLs with lots of unregistered arguments, the reversal attempts may produce an explosion
-        of complexity. If there are
+        of complexity. Check that the failsafe is working.
         :return:
+        """
         self.es6_mode = True
+        self.class_mode = ClassURLWriter.class_name_
         self.url_js = None
 
-        self.assertRaises(CommandError, call_command('render_static', 'urls.js'))
+        t1 = perf_counter()
+        tb_str = ''
+        try:
+            call_command('render_static', 'urls.js')
+        except Exception as complexity_error:
+            tb_str = traceback.format_exc()
+            t2 = perf_counter()
+
+        self.assertTrue('ReversalLimitHit' in tb_str)
+
+        # very generous reversal timing threshold of 20 seconds - anecdotally the default limit of
+        # 2**15 should be hit in about 3 seconds.
+        self.assertTrue(t2-t1 < 20)
+
+        placeholders.register_variable_placeholder('one', '666')
+        placeholders.register_variable_placeholder('two', '666')
+        placeholders.register_variable_placeholder('three', '666')
+        placeholders.register_variable_placeholder('four', '666')
+        placeholders.register_variable_placeholder('five', '666')
+        placeholders.register_variable_placeholder('six', '666')
+        placeholders.register_variable_placeholder('seven', '666')
+        placeholders.register_variable_placeholder('eight', '666')
 
-        self.compare('default')
-    """
+        call_command('render_static', 'urls.js')
+
+        self.compare(
+            'complex',
+            {
+                'one': 666,
+                'two': 666,
+                'three': 666,
+                'four': 666,
+                'five': 666,
+                'six': 666,
+                'seven': 666,
+                'eight': 666,
+            }
+        )
 
     # uncomment to not delete generated js
     def tearDown(self):
@@ -1854,7 +2031,6 @@ def test_unknown_pattern(self):
         self.assertRaises(CommandError, lambda: call_command('render_static', 'urls.js'))
 
     def test_register_bogus_converter(self):
-        from render_static import placeholders as gen
         self.assertRaises(
             ValueError,
             lambda: placeholders.register_converter_placeholder('Not a converter type!', 1234)