#37088 new Bug

path based MediaAsset equality

Reported by: Johannes Maron Owned by:
Component: Forms Version: 6.0
Severity: Normal Keywords:
Cc: Johannes Maron Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

This is a spin-off from here: https://github.com/django/django/pull/21239#issuecomment-4396168812

Currently MediaAsset.eq compares only the path.

This approach was deliberately chosen to benefit performance.

However, in the case of link-elements and even script this approach can break.
For a link, rel=stylesheet and rel=refretch can share the same path. Prefetching is also not too crazy a use case; someone will probably try to do it.

A compromise could be to define an explicit list of attributes to compare, instead of doing a full dictionary comparison via attributres.
Something like:

diff --git a/django/forms/widgets.py b/django/forms/widgets.py
index 6972155402..70561d2c8c 100644
--- a/django/forms/widgets.py
+++ b/django/forms/widgets.py
@@ -66,14 +66,20 @@ class MediaOrderConflictWarning(RuntimeWarning):
 class MediaAsset:
     element_template = "{path}"
 
+    # Only compare selected attributes to ensure performant comparison in Media.merge.
+    equality_attributes = ("path",)
+
     def __init__(self, path, **attributes):
         self._path = path
         self.attributes = attributes
 
     def __eq__(self, other):
-        # Compare the path only, to ensure performant comparison in
-        # Media.merge.
-        return (self.__class__ is other.__class__ and self.path == other.path) or (
+        return (
+            self.__class__ is other.__class__ and self.path == other.path and all(
+                self.attributes.get(attr, None) == other.attributes.get(attr, None)
+                for attr in self.equality_attributes
+            )
+        ) or (
             isinstance(other, str) and self._path == other
         )
 

Change History (0)

Note: See TracTickets for help on using tickets.
Back to Top