[Django] #26038: FileSystemStorage size() method does not use current MEDIA_ROOT setting value
Django
value
--------------------------------------+-------------------------------
Reporter: voutilad | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: 1.8
Severity: Normal | Keywords: testing fieldfile
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+-------------------------------
While writing some functional tests and using `@override_settings` to
point to a different `MEDIA_ROOT` location on my filesystem containing
test media files, getting a`FieldFile`'s `size` property resulted in
`OSError` exceptions because of an attempt to read the file in the app's
default (i.e. not overridden) `MEDIA_ROOT` location.
For instance, if I decorate my test class with like:
{{{#!python
@override_settings(
MEDIA_ROOT = os.path.join(settings.INSTALL_ROOT,
'cl/assets/media/test/')
)
class FeedsFunctionalTest(StaticLiveServerTestCase):
...
}}}
My test fails during `setUp()` due to logic in the test class that
attempts to read `size` on test model's `FieldFile` instance. (It's
persisting the value in an index for downline use.) It appears the
underlying `FileSystemStorage` class is getting and keeping the original
`MEDIA_ROOT` before the `@override_settings` decorator gets to change my
setting. It then uses that old value to build the path to call
`os.path.getsize(self.path(name))`.
Right now the workaround I'm using is to override the `size()` method (not
`path()` at the moment) like so:
{{{#!python
class MyDifferentFileSystemStorage(FileSystemStorage):
def size(self, name):
"""
Override the size method of FileSystemStorage to work around
bug in
Django 1.8 where MEDIA_ROOT is not used dynamically when
building the
underlying absolute path.
"""
return os.path.getsize(os.path.join(settings.MEDIA_ROOT,
name))
}}}
This looks like a bug to me as this negatively impacts using some test
utils (namely `override_settings`) and functional testing in general.
I'm experiencing this in v1.8, but looking at the code it appears to be
the same logic in Django's ''master'' branch as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/26038>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
value
Reporter: voutilad | Owner: nobody
Type: Bug | Status: new
uploads/storage |
Severity: Normal | Resolution:
Keywords: testing fieldfile | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Could you provide a failing test that demonstrates the problem? There is
some usage of `@override_settings(MEDIA_ROOT=...)` in
`tests/file_uploads`.
--
Ticket URL: <https://code.djangoproject.com/ticket/26038#comment:1>
Django
value
Reporter: voutilad | Owner: nobody
Type: Bug | Status: new
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Easy pickings: 0 | UI/UX: 0
Comment (by voutilad):
Replying to [comment:1 timgraham]:
> Could you provide a failing test that demonstrates the problem? There is
some usage of `@override_settings(MEDIA_ROOT=...)` in
`tests/file_uploads`.
Will do, hopefully today.
--
Ticket URL: <https://code.djangoproject.com/ticket/26038#comment:2>
Django
value
Reporter: voutilad | Owner: nobody
Type: Bug | Status: new
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Easy pickings: 0 | UI/UX: 0
Comment (by voutilad):
I've created a simple Django project that recreates the issue and I think
have identified some more information related to the problem. When using
the built-in `FileSystemStorage` class, the `@override_settings()` works
fine against `MEDIA_ROOT`. When creating a new class based on
`FileSystemStorage`, even when doing nothing (see below), it causes the
failure.
'''storage.py'''
{{{#!python
from django.core.files.storage import FileSystemStorage
class SubclassedFileSystemStorage(FileSystemStorage):
pass
}}}
'''models.py:'''
{{{#!python
from django.db import models
from issue26038.storage import SubclassedFileSystemStorage
class Media(models.Model):
binary_file = models.FileField(upload_to='prod/')
diff_storage_binary_file = models.FileField(
upload_to='prod/',
storage=SubclassedFileSystemStorage()
)
}}}
I've got a project located in GitHub that quickly recreates the issue:
[https://github.com/voutilad/django-issue26038]
--
Ticket URL: <https://code.djangoproject.com/ticket/26038#comment:3>
Django
value
Reporter: voutilad | Owner: nobody
Type: Bug | Status: new
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I suspect you'll need to implement a signal listener similar to what's
done in
[https://github.com/django/django/blob/62e83c71d2086b91d58c313e46933ef7aa8b6db1/django/test/signals.py#L121-L133
django.test.signals].
If that works, maybe we can adapt this ticket into a documentation
addition unless there's an idea of how Django could take care of this
automatically.
--
Ticket URL: <https://code.djangoproject.com/ticket/26038#comment:4>
Django
value
Reporter: voutilad | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: testing fieldfile | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* version: 1.8 => master
* stage: Unreviewed => Accepted
Comment:
I think `django.core.files.storage.FileSystemStorage` should take care of
recomputing its settings based properties (`MEDIAL_URL`, `MEDIA_ROOT`,
`FILE_UPLOAD_PERMISSIONS` and `FILE_UPLOAD_DIRECTORY_PERMISSIONS`) by
itself by registering a weak `setting_changed` receiver during its
initialization phase.
The `default_storage` should only be reset if `DEFAULT_FILE_STORAGE` is
changed.
--
Ticket URL: <https://code.djangoproject.com/ticket/26038#comment:5>
Django
value
Reporter: voutilad | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: testing fieldfile | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/5943 Here's what I had in mind].
--
Ticket URL: <https://code.djangoproject.com/ticket/26038#comment:6>
Django
value
Reporter: voutilad | Owner: nobody
Type: Bug | Status: new
uploads/storage |
Severity: Normal | Resolution:
Keywords: testing fieldfile | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26038#comment:7>
Django
value
Reporter: voutilad | Owner: nobody
Type: Bug | Status: closed
Component: File | Version: master
uploads/storage |
Keywords: testing fieldfile | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette <charette.s@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"56c461a0d7c1cbb9d5709bda817579c7ee18d474" 56c461a0]:
{{{
#!CommitTicketReference repository=""
revision="56c461a0d7c1cbb9d5709bda817579c7ee18d474"
Fixed #26038 -- Changed FileSystemStorage defaults on setting change.
Thanks to Dave Voutila for the report and Tim for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26038#comment:8>