Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#22337 closed Bug (fixed)

makemigrations not working when Field takes FileSystemStorage parameter

Reported by: nliberg Owned by: nobody
Component: Migrations Version: 1.7-beta-1
Severity: Release blocker Keywords: FileSystemStorage, migrations
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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'))

Attachments (2)

FileSystemStorage_deconstruct.patch (1.1 KB ) - added by nliberg 10 years ago.
Add deconstruct method to django.core.files.storage.FileSystemStorage
FileSystemStorage_deconstruct_tests.patch (1018 bytes ) - added by nliberg 10 years ago.
Tests for FileSystemStorage decode method

Download all attachments as: .zip

Change History (15)

comment:1 by Baptiste Mispelon, 10 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:2 by Baptiste Mispelon, 10 years ago

Needs tests: set
Patch needs improvement: set

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

Thanks.

by nliberg, 10 years ago

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

by nliberg, 10 years ago

Tests for FileSystemStorage decode method

comment:3 by nliberg, 10 years ago

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.

comment:4 by Tim Graham, 10 years ago

Easy pickings: unset
Needs tests: unset
Patch needs improvement: unset

comment:5 by chris cauley, 10 years ago

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.

comment:6 by Tim Graham, 10 years ago

#22373 was a duplicate.

comment:7 by rajiv@…, 10 years ago

+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.

comment:8 by loic84, 10 years ago

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.

comment:9 by Andrew Godwin, 10 years ago

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.

comment:10 by Andrew Godwin <andrew@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 694441827714a3e08f0d02c4650dc3388a867baa:

Fixed #22337: FileSystemStorage marked as deconstructible and tested.

comment:11 by Andrew Godwin <andrew@…>, 10 years ago

In f53d1576caab1594b29f6215604ef23ab6e5745e:

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

comment:12 by Andrew Godwin <andrew@…>, 10 years ago

In 827d5dc189fba33ec69d8916be310ff1e76361b1:

Improve docs around deconstruction/serialisation (refs #22337)

comment:13 by Andrew Godwin <andrew@…>, 10 years ago

In 1ed876ee5bd71092faf07559a25091bc27f96489:

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

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