FileSystemStorage size() method does not use current MEDIA_ROOT setting value

[Dave Voutila]

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 aFieldFile'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:

@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:

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.

[Tim Graham]

Could you provide a failing test that demonstrates the problem? There is some usage of @override_settings(MEDIA_ROOT=...) in tests/file_uploads.

[Dave Voutila]

Replying to 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.

[Dave Voutila]

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

from django.core.files.storage import FileSystemStorage

class SubclassedFileSystemStorage(FileSystemStorage):

    pass

models.py:

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

(Side note: I was testing this with Django 1.8.7, but a quick test shows it also fails the same way in Django 1.9.1)

[Tim Graham]

I suspect you'll need to implement a signal listener similar to what's done in ​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.

[Simon Charette]

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.

[Simon Charette]

​Here's what I had in mind.

[Simon Charette <charette.s@…>]

In 56c461a0:

Fixed #26038 -- Changed FileSystemStorage defaults on setting change.

Thanks to Dave Voutila for the report and Tim for the review.