Re: [Django] #37310: MediaAsset instances with attributes should not compare equal to strings (was: MediaAsset instances with attributes violate equality/hash contract with strings)

[Django]

#37310: MediaAsset instances with attributes should not compare equal to strings
-------------------------------------+-------------------------------------
Reporter: miladkhoshdel | Owner:
| miladkhoshdel
Type: Bug | Status: assigned
Component: Forms | Version: 6.1
Severity: Release blocker | Resolution:
Keywords: MediaAsset hash | Triage Stage: Accepted
equality |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
* summary:
MediaAsset instances with attributes violate equality/hash contract
with strings
=>
MediaAsset instances with attributes should not compare equal to
strings

Comment:

Thanks, I agree this this doesn't look right. We don't seem to be checking
`attributes` when `other` is `str`, e.g.

{{{#!py
from django.forms import Script, Stylesheet
asset = Stylesheet("/static/app.js")
asset2 = "/static/app.js"
assert asset == asset2 # works (shouldn't)
asset3 = Script("/static/app.js")
assert asset == asset3 # fails only here
}}}

That would lead me to expect a solution like:

{{{#!diff
diff --git a/django/forms/widgets.py b/django/forms/widgets.py
index a52ad18b09..997c0c92b4 100644
--- a/django/forms/widgets.py
+++ b/django/forms/widgets.py
@@ -78,7 +78,7 @@ class MediaAsset:
self.__class__ is other.__class__
and self._path == other._path
and self.attributes == other.attributes
- ) or (isinstance(other, str) and self._path == other)
+ ) or (isinstance(other, str) and not self.attributes and
self._path == other)

def __hash__(self):
# Compare path and attrs to ensure performant comparison
}}}

Which gives test failures like:
{{{#!py
======================================================================
FAIL: test_add_empty
(forms_tests.tests.test_media.FormsMediaTestCase.test_add_empty)
----------------------------------------------------------------------
Traceback (most recent call last):
...
^^^^^^^^^^^^^^^
AssertionError: Lists differ: [{'screen': [Stylesheet('a.css')]}] !=
[{'screen': ['a.css']}]

First differing element 0:
{'screen': [Stylesheet('a.css')]}
{'screen': ['a.css']}

- [{'screen': [Stylesheet('a.css')]}]
? ----------- -

+ [{'screen': ['a.css']}]
}}}

... but if there is no functional impact to that change, then can we just
update the assertions?
--
Ticket URL: <https://code.djangoproject.com/ticket/37310#comment:7>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.