Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fixed #22337: FileSystemStorage marked as deconstructible and tested.

  • Loading branch information...
commit 694441827714a3e08f0d02c4650dc3388a867baa 1 parent 61da5f3
@andrewgodwin andrewgodwin authored
View
2  django/core/files/storage.py
@@ -13,6 +13,7 @@
from django.utils.six.moves.urllib.parse import urljoin
from django.utils.text import get_valid_filename
from django.utils._os import safe_join, abspathu
+from django.utils.deconstruct import deconstructible
__all__ = ('Storage', 'FileSystemStorage', 'DefaultStorage', 'default_storage')
@@ -144,6 +145,7 @@ def modified_time(self, name):
raise NotImplementedError('subclasses of Storage must provide a modified_time() method')
+@deconstructible
class FileSystemStorage(Storage):
@loic Collaborator
loic added a note

Not saying it should, but did you consider adding it to Storage?

@andrewgodwin Owner

I did, but I felt that might be overstretching, especially with backwards-compat (what if an argument to a custom storage is itself not deconstructible?)

I'll add a note to the custom storage docs that you should deal with this, and a mention in the release notes.

@loic Collaborator
loic added a note

I bring this up because I faced the same dilemma for validators (objects that are not strictly ORM related, that yet need to be reconstructible). That's what pushed me to turn some of your internal code into the @decontructible decorator, to make it as little invasive as possible on those objects.

I'm not too sure there is a backwards compat issue since the mechanism would only kick in if the storage is used in a FileField, at which point it's already broken since the storage itself is non-deconstructible.

For this case though, I think a docs solution is a good compromise.

@andrewgodwin Owner

Yeah, and this is a more general problem too with any custom class instance arguments. I've beefed up the release notes and the custom storage docs in 827d5dc; we might want to call it out in more places too, but I'm happy with that as a start.

@loic Collaborator
loic added a note

The referenced commit looks good. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
"""
Standard filesystem storage
View
2  tests/field_deconstruction/tests.py
@@ -3,6 +3,7 @@
from django.db import models
from django.test import TestCase, override_settings
from django.utils import six
+from django.core.files.storage import FileSystemStorage
class FieldDeconstructionTests(TestCase):
@@ -141,6 +142,7 @@ def test_file_field(self):
self.assertEqual(path, "django.db.models.FileField")
self.assertEqual(args, [])
self.assertEqual(kwargs, {"upload_to": "foo/bar"})
+ # Test max_length
field = models.FileField(upload_to="foo/bar", max_length=200)
name, path, args, kwargs = field.deconstruct()
self.assertEqual(path, "django.db.models.FileField")
View
17 tests/file_storage/tests.py
@@ -64,6 +64,23 @@ def test_get_nonexisting_storage_module(self):
'django.core.files.non_existing_storage.NonExistingStorage')
+class FileStorageDeconstructionTests(unittest.TestCase):
+
+ def test_deconstruction(self):
+ path, args, kwargs = temp_storage.deconstruct()
+ self.assertEqual(path, "django.core.files.storage.FileSystemStorage")
+ self.assertEqual(args, tuple())
+ self.assertEqual(kwargs, {'location': temp_storage_location})
+
+ kwargs_orig = {
+ 'location': temp_storage_location,
+ 'base_url': 'http://myfiles.example.com/'
+ }
+ storage = FileSystemStorage(**kwargs_orig)
+ path, args, kwargs = storage.deconstruct()
+ self.assertEqual(kwargs, kwargs_orig)
+
+
class FileStorageTests(unittest.TestCase):
storage_class = FileSystemStorage
@loic

Not saying it should, but did you consider adding it to Storage?

@andrewgodwin

I did, but I felt that might be overstretching, especially with backwards-compat (what if an argument to a custom storage is itself not deconstructible?)

I'll add a note to the custom storage docs that you should deal with this, and a mention in the release notes.

@loic

I bring this up because I faced the same dilemma for validators (objects that are not strictly ORM related, that yet need to be reconstructible). That's what pushed me to turn some of your internal code into the @decontructible decorator, to make it as little invasive as possible on those objects.

I'm not too sure there is a backwards compat issue since the mechanism would only kick in if the storage is used in a FileField, at which point it's already broken since the storage itself is non-deconstructible.

For this case though, I think a docs solution is a good compromise.

@andrewgodwin

Yeah, and this is a more general problem too with any custom class instance arguments. I've beefed up the release notes and the custom storage docs in 827d5dc; we might want to call it out in more places too, but I'm happy with that as a start.

@loic

The referenced commit looks good. Thanks.

Please sign in to comment.
Something went wrong with that request. Please try again.