Opened 3 years ago

Closed 3 years ago

Last modified 3 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 3 years ago.
Add deconstruct method to django.core.files.storage.FileSystemStorage
FileSystemStorage_deconstruct_tests.patch (1018 bytes) - added by nliberg 3 years ago.
Tests for FileSystemStorage decode method

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by Baptiste Mispelon

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:2 Changed 3 years ago by Baptiste Mispelon

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.

Changed 3 years ago by nliberg

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

Changed 3 years ago by nliberg

Tests for FileSystemStorage decode method

comment:3 Changed 3 years ago by 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.

comment:4 Changed 3 years ago by Tim Graham

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

comment:5 Changed 3 years ago by 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.

comment:6 Changed 3 years ago by Tim Graham

#22373 was a duplicate.

comment:7 Changed 3 years ago by 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.

comment:8 Changed 3 years ago by 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.

comment:9 Changed 3 years ago by 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.

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

Resolution: fixed
Status: newclosed

In 694441827714a3e08f0d02c4650dc3388a867baa:

Fixed #22337: FileSystemStorage marked as deconstructible and tested.

comment:11 Changed 3 years ago by Andrew Godwin <andrew@…>

In f53d1576caab1594b29f6215604ef23ab6e5745e:

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

comment:12 Changed 3 years ago by Andrew Godwin <andrew@…>

In 827d5dc189fba33ec69d8916be310ff1e76361b1:

Improve docs around deconstruction/serialisation (refs #22337)

comment:13 Changed 3 years ago by Andrew Godwin <andrew@…>

In 1ed876ee5bd71092faf07559a25091bc27f96489:

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

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