#31517: ManifestFilesMixin.file_hash() returning None get's included in hashed
filename as 'None'.
-------------------------------------+-------------------------------------
Reporter: Richard Campen | Owner: Richard
Type: | Campen
Cleanup/optimization | Status: assigned
Component: contrib.staticfiles | Version: master
Severity: Normal | Resolution:
Keywords: HashedFilesMixin, | Triage Stage: Accepted
ManifestFilesMixin, file_hash, |
hashed_name |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by felixxm):
* status: new => assigned
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* owner: nobody => Richard Campen
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Old description:
> When returning a string from a custom` ManifestFilesMixin.file_hash()`
> implementation, the resulting file name is
> `<file_path>.<custom_hash>.<ext>` as expected, whereas returning `None`
> results in `<file_path>None.<ext>`.
>
> [https://groups.google.com/forum/#!searchin/django-
> developers/file_hash%7Csort:date/django-
> developers/GXNDGBb3tlM/VE61CAgoAgAJ Discussion on django-developers]
> supports this behaviour being unintended.
>
> Behavior appears to have been introduced with [#17896] which split the
> file hashing into a separate method.
>
> The following test, when included in the
> `test_storage.TestCollectionManifestStorage` test class demonstrates the
> bug:
>
> {{{
> def test_hashed_name_unchanged_when_file_hash_is_None(self):
> with
> mock.patch('django.contrib.staticfiles.storage.ManifestStaticFilesStorage.file_hash',
> return_value=None):
> self.assertEqual(storage.staticfiles_storage.hashed_name('test/file.txt'),
> 'test/file.txt')
> }}}
>
> As suggested by the name of my test, my opinion is that the correct
> behaviour should be that if `file_hash` returns None, then no hash is
> inserted into the filename and it therefore remains unchanged.
>
> With that in mind, a possible solution is to change the following lines
> in the `hashed_name()` method (~line 100 in `staticfiles.storage`):
>
> {{{
> if file_hash is not None:
> file_hash = ".%s" % file_hash
> hashed_name = os.path.join(path, "%s%s%s" % (root, file_hash, ext))
> }}}
>
> to
>
> {{{
> if file_hash is None:
> file_hash = ""
> else:
> file_hash = ".%s" % file_hash
> hashed_name = os.path.join(path, "%s%s%s" % (root, file_hash, ext))
> }}}
New description:
When returning a string from a custom` ManifestFilesMixin.file_hash()`
implementation, the resulting file name is
`<file_path>.<custom_hash>.<ext>` as expected, whereas returning `None`
results in `<file_path>None.<ext>`.
[https://groups.google.com/forum/#!searchin/django-
developers/file_hash%7Csort:date/django-
developers/GXNDGBb3tlM/VE61CAgoAgAJ Discussion on django-developers]
supports this behaviour being unintended.
Behavior appears to have been introduced with #17896 which split the file
hashing into a separate method.
The following test, when included in the
`test_storage.TestCollectionManifestStorage` test class demonstrates the
bug:
{{{
def test_hashed_name_unchanged_when_file_hash_is_None(self):
with
mock.patch('django.contrib.staticfiles.storage.ManifestStaticFilesStorage.file_hash',
return_value=None):
self.assertEqual(storage.staticfiles_storage.hashed_name('test/file.txt'),
'test/file.txt')
}}}
As suggested by the name of my test, my opinion is that the correct
behaviour should be that if `file_hash` returns None, then no hash is
inserted into the filename and it therefore remains unchanged.
With that in mind, a possible solution is to change the following lines
in the `hashed_name()` method (~line 100 in `staticfiles.storage`):
{{{
if file_hash is not None:
file_hash = ".%s" % file_hash
hashed_name = os.path.join(path, "%s%s%s" % (root, file_hash, ext))
}}}
to
{{{
if file_hash is None:
file_hash = ""
else:
file_hash = ".%s" % file_hash
hashed_name = os.path.join(path, "%s%s%s" % (root, file_hash, ext))
}}}
--
Comment:
Thanks for this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/31517#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/065.cb5dfa106b89eb4836ebf126192c5f25%40djangoproject.com.
