Opened 14 months ago

Closed 12 months ago

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

Download all attachments as: .zip

Change History (15)

comment:1 Changed 14 months ago by bmispelon

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 14 months ago by bmispelon

  • 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 14 months ago by nliberg

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

Changed 14 months ago by nliberg

Tests for FileSystemStorage decode method

comment:3 Changed 14 months 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 13 months ago by timo

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

comment:5 Changed 13 months ago by chriscauley

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 13 months ago by timo

#22373 was a duplicate.

comment:7 Changed 12 months 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 12 months 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 12 months ago by andrewgodwin

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 12 months ago by Andrew Godwin <andrew@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 694441827714a3e08f0d02c4650dc3388a867baa:

Fixed #22337: FileSystemStorage marked as deconstructible and tested.

comment:11 Changed 12 months ago by Andrew Godwin <andrew@…>

In f53d1576caab1594b29f6215604ef23ab6e5745e:

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

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

In 827d5dc189fba33ec69d8916be310ff1e76361b1:

Improve docs around deconstruction/serialisation (refs #22337)

comment:13 Changed 12 months 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