Opened 9 years ago

Closed 9 years ago

#26038 closed Bug (fixed)

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

Reported by: Dave Voutila Owned by: nobody
Component: File uploads/storage Version: dev
Severity: Normal Keywords: testing fieldfile
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (8)

comment:1 by Tim Graham, 9 years ago

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

in reply to:  1 comment:2 by Dave Voutila, 9 years ago

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.

comment:3 by Dave Voutila, 9 years ago

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

Version 0, edited 9 years ago by Dave Voutila (next)

comment:4 by Tim Graham, 9 years ago

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.

comment:5 by Simon Charette, 9 years ago

Triage Stage: UnreviewedAccepted
Version: 1.8master

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.

comment:6 by Simon Charette, 9 years ago

Has patch: set

comment:7 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Simon Charette <charette.s@…>, 9 years ago

Resolution: fixed
Status: newclosed

In 56c461a0:

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

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

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