makemigrations not working when Field takes FileSystemStorage parameter

[nliberg]

FileSystemStorage is not serializable. Because of this running ./manage.py makemigrations for an app that contains fields that use such storage results in an exception:
ValueError: Cannot serialize: <django.core.files.storage.FileSystemStorage object at 0x109c02310>

Background:
When manage.py makemigrations is run on a new app the deconstruct method is invoked for all fields. According to the django documentation every value in the deconstructed kwargs dictionary "should itself be serializable". However, this is not the case when one passes a FileSystemStorage object as storage parameter to the FileField constructor. The deconstruct method of the FileFIeld class adds the FileSystemStorage storage object to kwargs without it being serializable.

Fix:
I attach a patch that adds a deconstruct method to the django.core.files.storage.FileSystemStorage class.

Reproducing
The problem can be reproduced by starting a new app using the following models.py file and running ./manage.py makemigrations <appname>

from django.db import models
from django.core.files.storage import FileSystemStorage

class MyModel(models.Model):
    myfile = models.FileField('File', upload_to='myfiles', storage=FileSystemStorage(location='/tmp'))

[Baptiste Mispelon]

The patch looks sensible and it does seem to fix the issue but it needs some tests too.

Thanks.

[nliberg]

Add deconstruct method to django.core.files.storage.FileSystemStorage

[nliberg]

Tests for FileSystemStorage decode method

[nliberg]

I modified the original patch slightly. The decode method now avoids adding items to kwargs where the value is the default value. Furthermore I added some test code to tests/file_storage/tests.py in the second patch.

[chris cauley]

I propose that the storage should be omitted from FileField.deconstruct(). It's an optional kwarg and it has no impact on the database. Changes in custom storages could potentially break past migrations. In south the various states of each model were stored as dicts of strings so that changes in python code could not break schema. I cover this in more detail in several related tickets: #22373, #22351, and #22436.

[Tim Graham]

#22373 was a duplicate.

[rajiv@…]

+1 on the excluding storage from FileField.deconstruct as chriscauley proposes. This affects migrations with custom storages such as S3BotoStorage from django-storages. Unless there is an expectation that all storage backends will implement deconstruct (to no benefit from what I can tell, as it does not actually affect the migration), it seems like an unnecessary burden on updating projects.

[loic84]

Why not using ®deconstructible from django.utils.deconstruct? That should work with most subclasses of storage without extra efforts from their authors'.

Regarding skipping storage in deconstruct(), that means FileField wouldn't work in RunPython operation, I'd rather keep the fake ORM as capable as possible.

[Andrew Godwin]

Yes, we have to keep all parameters on fields, even if they're not database-affecting; the RunPython method still sees those fields and uses them from Python.

I'll fix this now using @deconstructible, as Loic is correct that it's safe to use here.

[Andrew Godwin <andrew@…>]

In 694441827714a3e08f0d02c4650dc3388a867baa:

Fixed #22337: FileSystemStorage marked as deconstructible and tested.

[Andrew Godwin <andrew@…>]

In f53d1576caab1594b29f6215604ef23ab6e5745e:

[1.7.x] Fixed #22337: FileSystemStorage marked as deconstructible and tested.

[Andrew Godwin <andrew@…>]

In 827d5dc189fba33ec69d8916be310ff1e76361b1:

Improve docs around deconstruction/serialisation (refs #22337)

[Andrew Godwin <andrew@…>]

In 1ed876ee5bd71092faf07559a25091bc27f96489:

[1.7.x] Improve docs around deconstruction/serialisation (refs #22337)