Fixed #30179 -- Fixed form Media merging when pairwise merging is insufficient.

Thanks gasman for the tests, and codingjoe and timgraham for the review.

django/forms/widgets.py

@@ -6,16 +6,21 @@ import copy
 import datetime
 import re
 import warnings
+from collections import defaultdict
 from itertools import chain
 
 from django.conf import settings
 from django.forms.utils import to_current_timezone
 from django.templatetags.static import static
 from django.utils import datetime_safe, formats
+from django.utils.datastructures import OrderedSet
 from django.utils.dates import MONTHS
 from django.utils.formats import get_format
 from django.utils.html import format_html, html_safe
 from django.utils.safestring import mark_safe
+from django.utils.topological_sort import (
+    CyclicDependencyError, stable_topological_sort,
+)
 from django.utils.translation import gettext_lazy as _
 
 from .renderers import get_default_renderer
@@ -59,22 +64,15 @@ class Media:
 
     @property
     def _css(self):
-        css = self._css_lists[0]
-        # filter(None, ...) avoids calling merge with empty dicts.
-        for obj in filter(None, self._css_lists[1:]):
-            css = {
-                medium: self.merge(css.get(medium, []), obj.get(medium, []))
-                for medium in css.keys() | obj.keys()
-            }
-        return css
+        css = defaultdict(list)
+        for css_list in self._css_lists:
+            for medium, sublist in css_list.items():
+                css[medium].append(sublist)
+        return {medium: self.merge(*lists) for medium, lists in css.items()}
 
     @property
     def _js(self):
-        js = self._js_lists[0]
-        # filter(None, ...) avoids calling merge() with empty lists.
-        for obj in filter(None, self._js_lists[1:]):
-            js = self.merge(js, obj)
-        return js
+        return self.merge(*self._js_lists)
 
     def render(self):
         return mark_safe('\n'.join(chain.from_iterable(getattr(self, 'render_' + name)() for name in MEDIA_TYPES)))
@@ -115,39 +113,37 @@ class Media:
         raise KeyError('Unknown media type "%s"' % name)
 
     @staticmethod
-    def merge(list_1, list_2):
+    def merge(*lists):
         """
-        Merge two lists while trying to keep the relative order of the elements.
-        Warn if the lists have the same two elements in a different relative
-        order.
+        Merge lists while trying to keep the relative order of the elements.
+        Warn if the lists have the same elements in a different relative order.
 
         For static assets it can be important to have them included in the DOM
         in a certain order. In JavaScript you may not be able to reference a
         global or in CSS you might want to override a style.
         """
-        # Start with a copy of list_1.
-        combined_list = list(list_1)
-        last_insert_index = len(list_1)
-        # Walk list_2 in reverse, inserting each element into combined_list if
-        # it doesn't already exist.
-        for path in reversed(list_2):
-            try:
-                # Does path already exist in the list?
-                index = combined_list.index(path)
-            except ValueError:
-                # Add path to combined_list since it doesn't exist.
-                combined_list.insert(last_insert_index, path)
-            else:
-                if index > last_insert_index:
-                    warnings.warn(
-                        'Detected duplicate Media files in an opposite order:\n'
-                        '%s\n%s' % (combined_list[last_insert_index], combined_list[index]),
-                        MediaOrderConflictWarning,
-                    )
-                # path already exists in the list. Update last_insert_index so
-                # that the following elements are inserted in front of this one.
-                last_insert_index = index
-        return combined_list
+        dependency_graph = defaultdict(set)
+        all_items = OrderedSet()
+        for list_ in filter(None, lists):
+            head = list_[0]
+            # The first items depend on nothing but have to be part of the
+            # dependency graph to be included in the result.
+            dependency_graph.setdefault(head, set())
+            for item in list_:
+                all_items.add(item)
+                # No self dependencies
+                if head != item:
+                    dependency_graph[item].add(head)
+                head = item
+        try:
+            return stable_topological_sort(all_items, dependency_graph)
+        except CyclicDependencyError:
+            warnings.warn(
+                'Detected duplicate Media files in an opposite order: {}'.format(
+                    ', '.join(repr(l) for l in lists)
+                ), MediaOrderConflictWarning,
+            )
+            return list(all_items)
 
     def __add__(self, other):
         combined = Media()

tests/admin_inlines/tests.py

@@ -497,10 +497,10 @@ class TestInlineMedia(TestDataMixin, TestCase):
             response.context['inline_admin_formsets'][0].media._js,
             [
                 'admin/js/vendor/jquery/jquery.min.js',
-                'admin/js/jquery.init.js',
-                'admin/js/inlines.min.js',
                 'my_awesome_inline_scripts.js',
                 'custom_number.js',
+                'admin/js/jquery.init.js',
+                'admin/js/inlines.min.js',
             ]
         )
         self.assertContains(response, 'my_awesome_inline_scripts.js')

tests/admin_widgets/test_autocomplete_widget.py

@@ -139,4 +139,4 @@ class AutocompleteMixinTests(TestCase):
                 else:
                     expected_files = base_files
                 with translation.override(lang):
-                    self.assertEqual(AutocompleteSelect(rel, admin.site).media._js, expected_files)
+                    self.assertEqual(AutocompleteSelect(rel, admin.site).media._js, list(expected_files))

tests/forms_tests/tests/test_media.py

@@ -25,8 +25,8 @@ class FormsMediaTestCase(SimpleTestCase):
         )
         self.assertEqual(
             repr(m),
-            "Media(css={'all': ('path/to/css1', '/path/to/css2')}, "
-            "js=('/path/to/js1', 'http://media.other.com/path/to/js2', 'https://secure.other.com/path/to/js3'))"
+            "Media(css={'all': ['path/to/css1', '/path/to/css2']}, "
+            "js=['/path/to/js1', 'http://media.other.com/path/to/js2', 'https://secure.other.com/path/to/js3'])"
         )
 
         class Foo:
@@ -125,8 +125,8 @@ class FormsMediaTestCase(SimpleTestCase):
 <link href="/path/to/css3" type="text/css" media="all" rel="stylesheet">
 <script type="text/javascript" src="/path/to/js1"></script>
 <script type="text/javascript" src="http://media.other.com/path/to/js2"></script>
-<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>
-<script type="text/javascript" src="/path/to/js4"></script>"""
+<script type="text/javascript" src="/path/to/js4"></script>
+<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>"""
         )
 
         # media addition hasn't affected the original objects
@@ -151,6 +151,17 @@ class FormsMediaTestCase(SimpleTestCase):
         self.assertEqual(str(w4.media), """<link href="/path/to/css1" type="text/css" media="all" rel="stylesheet">
 <script type="text/javascript" src="/path/to/js1"></script>""")
 
+    def test_media_deduplication(self):
+        # A deduplication test applied directly to a Media object, to confirm
+        # that the deduplication doesn't only happen at the point of merging
+        # two or more media objects.
+        media = Media(
+            css={'all': ('/path/to/css1', '/path/to/css1')},
+            js=('/path/to/js1', '/path/to/js1'),
+        )
+        self.assertEqual(str(media), """<link href="/path/to/css1" type="text/css" media="all" rel="stylesheet">
+<script type="text/javascript" src="/path/to/js1"></script>""")
+
     def test_media_property(self):
         ###############################################################
         # Property-based media definitions
@@ -197,12 +208,12 @@ class FormsMediaTestCase(SimpleTestCase):
         self.assertEqual(
             str(w6.media),
             """<link href="http://media.example.com/static/path/to/css1" type="text/css" media="all" rel="stylesheet">
-<link href="/path/to/css2" type="text/css" media="all" rel="stylesheet">
 <link href="/other/path" type="text/css" media="all" rel="stylesheet">
+<link href="/path/to/css2" type="text/css" media="all" rel="stylesheet">
 <script type="text/javascript" src="/path/to/js1"></script>
+<script type="text/javascript" src="/other/js"></script>
 <script type="text/javascript" src="http://media.other.com/path/to/js2"></script>
-<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>
-<script type="text/javascript" src="/other/js"></script>"""
+<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>"""
         )
 
     def test_media_inheritance(self):
@@ -247,8 +258,8 @@ class FormsMediaTestCase(SimpleTestCase):
 <link href="/path/to/css2" type="text/css" media="all" rel="stylesheet">
 <script type="text/javascript" src="/path/to/js1"></script>
 <script type="text/javascript" src="http://media.other.com/path/to/js2"></script>
-<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>
-<script type="text/javascript" src="/path/to/js4"></script>"""
+<script type="text/javascript" src="/path/to/js4"></script>
+<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>"""
         )
 
     def test_media_inheritance_from_property(self):
@@ -322,8 +333,8 @@ class FormsMediaTestCase(SimpleTestCase):
 <link href="/path/to/css2" type="text/css" media="all" rel="stylesheet">
 <script type="text/javascript" src="/path/to/js1"></script>
 <script type="text/javascript" src="http://media.other.com/path/to/js2"></script>
-<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>
-<script type="text/javascript" src="/path/to/js4"></script>"""
+<script type="text/javascript" src="/path/to/js4"></script>
+<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>"""
         )
 
     def test_media_inheritance_single_type(self):
@@ -420,8 +431,8 @@ class FormsMediaTestCase(SimpleTestCase):
 <link href="/path/to/css3" type="text/css" media="all" rel="stylesheet">
 <script type="text/javascript" src="/path/to/js1"></script>
 <script type="text/javascript" src="http://media.other.com/path/to/js2"></script>
-<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>
-<script type="text/javascript" src="/path/to/js4"></script>"""
+<script type="text/javascript" src="/path/to/js4"></script>
+<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>"""
         )
 
     def test_form_media(self):
@@ -462,8 +473,8 @@ class FormsMediaTestCase(SimpleTestCase):
 <link href="/path/to/css3" type="text/css" media="all" rel="stylesheet">
 <script type="text/javascript" src="/path/to/js1"></script>
 <script type="text/javascript" src="http://media.other.com/path/to/js2"></script>
-<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>
-<script type="text/javascript" src="/path/to/js4"></script>"""
+<script type="text/javascript" src="/path/to/js4"></script>
+<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>"""
         )
 
         # Form media can be combined to produce a single media definition.
@@ -477,8 +488,8 @@ class FormsMediaTestCase(SimpleTestCase):
 <link href="/path/to/css3" type="text/css" media="all" rel="stylesheet">
 <script type="text/javascript" src="/path/to/js1"></script>
 <script type="text/javascript" src="http://media.other.com/path/to/js2"></script>
-<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>
-<script type="text/javascript" src="/path/to/js4"></script>"""
+<script type="text/javascript" src="/path/to/js4"></script>
+<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>"""
         )
 
         # Forms can also define media, following the same rules as widgets.
@@ -495,28 +506,28 @@ class FormsMediaTestCase(SimpleTestCase):
         self.assertEqual(
             str(f3.media),
             """<link href="http://media.example.com/static/path/to/css1" type="text/css" media="all" rel="stylesheet">
+<link href="/some/form/css" type="text/css" media="all" rel="stylesheet">
 <link href="/path/to/css2" type="text/css" media="all" rel="stylesheet">
 <link href="/path/to/css3" type="text/css" media="all" rel="stylesheet">
-<link href="/some/form/css" type="text/css" media="all" rel="stylesheet">
 <script type="text/javascript" src="/path/to/js1"></script>
+<script type="text/javascript" src="/some/form/javascript"></script>
 <script type="text/javascript" src="http://media.other.com/path/to/js2"></script>
-<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>
 <script type="text/javascript" src="/path/to/js4"></script>
-<script type="text/javascript" src="/some/form/javascript"></script>"""
+<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>"""
         )
 
         # Media works in templates
         self.assertEqual(
             Template("{{ form.media.js }}{{ form.media.css }}").render(Context({'form': f3})),
             """<script type="text/javascript" src="/path/to/js1"></script>
+<script type="text/javascript" src="/some/form/javascript"></script>
 <script type="text/javascript" src="http://media.other.com/path/to/js2"></script>
-<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>
 <script type="text/javascript" src="/path/to/js4"></script>
-<script type="text/javascript" src="/some/form/javascript"></script>"""
+<script type="text/javascript" src="https://secure.other.com/path/to/js3"></script>"""
             """<link href="http://media.example.com/static/path/to/css1" type="text/css" media="all" rel="stylesheet">
+<link href="/some/form/css" type="text/css" media="all" rel="stylesheet">
 <link href="/path/to/css2" type="text/css" media="all" rel="stylesheet">
-<link href="/path/to/css3" type="text/css" media="all" rel="stylesheet">
-<link href="/some/form/css" type="text/css" media="all" rel="stylesheet">"""
+<link href="/path/to/css3" type="text/css" media="all" rel="stylesheet">"""
         )
 
     def test_html_safe(self):
@@ -526,19 +537,23 @@ class FormsMediaTestCase(SimpleTestCase):
 
     def test_merge(self):
         test_values = (
-            (([1, 2], [3, 4]), [1, 2, 3, 4]),
+            (([1, 2], [3, 4]), [1, 3, 2, 4]),
             (([1, 2], [2, 3]), [1, 2, 3]),
             (([2, 3], [1, 2]), [1, 2, 3]),
             (([1, 3], [2, 3]), [1, 2, 3]),
             (([1, 2], [1, 3]), [1, 2, 3]),
             (([1, 2], [3, 2]), [1, 3, 2]),
+            (([1, 2], [1, 2]), [1, 2]),
+            ([[1, 2], [1, 3], [2, 3], [5, 7], [5, 6], [6, 7, 9], [8, 9]], [1, 5, 8, 2, 6, 3, 7, 9]),
+            ((), []),
+            (([1, 2],), [1, 2]),
         )
-        for (list1, list2), expected in test_values:
-            with self.subTest(list1=list1, list2=list2):
-                self.assertEqual(Media.merge(list1, list2), expected)
+        for lists, expected in test_values:
+            with self.subTest(lists=lists):
+                self.assertEqual(Media.merge(*lists), expected)
 
     def test_merge_warning(self):
-        msg = 'Detected duplicate Media files in an opposite order:\n1\n2'
+        msg = 'Detected duplicate Media files in an opposite order: [1, 2], [2, 1]'
         with self.assertWarnsMessage(RuntimeWarning, msg):
             self.assertEqual(Media.merge([1, 2], [2, 1]), [1, 2])
 
@@ -546,28 +561,30 @@ class FormsMediaTestCase(SimpleTestCase):
         """
         The relative order of scripts is preserved in a three-way merge.
         """
-        # custom_widget.js doesn't depend on jquery.js.
-        widget1 = Media(js=['custom_widget.js'])
-        widget2 = Media(js=['jquery.js', 'uses_jquery.js'])
-        form_media = widget1 + widget2
-        # The relative ordering of custom_widget.js and jquery.js has been
-        # established (but without a real need to).
-        self.assertEqual(form_media._js, ['custom_widget.js', 'jquery.js', 'uses_jquery.js'])
-        # The inline also uses custom_widget.js. This time, it's at the end.
-        inline_media = Media(js=['jquery.js', 'also_jquery.js']) + Media(js=['custom_widget.js'])
-        merged = form_media + inline_media
-        self.assertEqual(merged._js, ['custom_widget.js', 'jquery.js', 'uses_jquery.js', 'also_jquery.js'])
+        widget1 = Media(js=['color-picker.js'])
+        widget2 = Media(js=['text-editor.js'])
+        widget3 = Media(js=['text-editor.js', 'text-editor-extras.js', 'color-picker.js'])
+        merged = widget1 + widget2 + widget3
+        self.assertEqual(merged._js, ['text-editor.js', 'text-editor-extras.js', 'color-picker.js'])
+
+    def test_merge_js_three_way2(self):
+        # The merge prefers to place 'c' before 'b' and 'g' before 'h' to
+        # preserve the original order. The preference 'c'->'b' is overridden by
+        # widget3's media, but 'g'->'h' survives in the final ordering.
+        widget1 = Media(js=['a', 'c', 'f', 'g', 'k'])
+        widget2 = Media(js=['a', 'b', 'f', 'h', 'k'])
+        widget3 = Media(js=['b', 'c', 'f', 'k'])
+        merged = widget1 + widget2 + widget3
+        self.assertEqual(merged._js, ['a', 'b', 'c', 'f', 'g', 'h', 'k'])
 
     def test_merge_css_three_way(self):
-        widget1 = Media(css={'screen': ['a.css']})
-        widget2 = Media(css={'screen': ['b.css']})
-        widget3 = Media(css={'all': ['c.css']})
-        form1 = widget1 + widget2
-        form2 = widget2 + widget1
-        # form1 and form2 have a.css and b.css in different order...
-        self.assertEqual(form1._css, {'screen': ['a.css', 'b.css']})
-        self.assertEqual(form2._css, {'screen': ['b.css', 'a.css']})
-        # ...but merging succeeds as the relative ordering of a.css and b.css
-        # was never specified.
-        merged = widget3 + form1 + form2
-        self.assertEqual(merged._css, {'screen': ['a.css', 'b.css'], 'all': ['c.css']})
+        widget1 = Media(css={'screen': ['c.css'], 'all': ['d.css', 'e.css']})
+        widget2 = Media(css={'screen': ['a.css']})
+        widget3 = Media(css={'screen': ['a.css', 'b.css', 'c.css'], 'all': ['e.css']})
+        merged = widget1 + widget2
+        # c.css comes before a.css because widget1 + widget2 establishes this
+        # order.
+        self.assertEqual(merged._css, {'screen': ['c.css', 'a.css'], 'all': ['d.css', 'e.css']})
+        merged = merged + widget3
+        # widget3 contains an explicit ordering of c.css and a.css.
+        self.assertEqual(merged._css, {'screen': ['a.css', 'b.css', 'c.css'], 'all': ['d.css', 'e.css']})