[Django] #22337: makemigrations not working when Field takes FileSystemStorage parameter
Django
----------------------------+-------------------------------------------
Reporter: nliberg | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-1
Severity: Normal | Keywords: FileSystemStorage, migrations
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 1 | UI/UX: 0
----------------------------+-------------------------------------------
`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>`
{{{#!python
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'))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22337>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version:
Keywords: FileSystemStorage, | Resolution:
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by bmispelon):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* severity: Normal => Release blocker
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:1>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version:
Keywords: FileSystemStorage, | Resolution:
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by bmispelon):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
The patch looks sensible and it does seem to fix the issue but it needs
some tests too.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:2>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version:
Keywords: FileSystemStorage, | Resolution:
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:3>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version:
Keywords: FileSystemStorage, | Resolution:
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
-------------------------------------+-------------------------------------
Changes (by timo):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* easy: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:4>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version:
Keywords: FileSystemStorage, | Resolution:
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:5>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version:
Keywords: FileSystemStorage, | Resolution:
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timo):
#22373 was a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:6>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version:
Keywords: FileSystemStorage, | Resolution:
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:7>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version:
Keywords: FileSystemStorage, | Resolution:
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:8>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version:
Keywords: FileSystemStorage, | Resolution:
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:9>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version:
Severity: Release blocker | 1.7-beta-1
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"694441827714a3e08f0d02c4650dc3388a867baa"]:
{{{
#!CommitTicketReference repository=""
revision="694441827714a3e08f0d02c4650dc3388a867baa"
Fixed #22337: FileSystemStorage marked as deconstructible and tested.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:10>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version:
Severity: Release blocker | 1.7-beta-1
Keywords: FileSystemStorage, | Resolution: fixed
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Andrew Godwin <andrew@…>):
In [changeset:"f53d1576caab1594b29f6215604ef23ab6e5745e"]:
{{{
#!CommitTicketReference repository=""
revision="f53d1576caab1594b29f6215604ef23ab6e5745e"
[1.7.x] Fixed #22337: FileSystemStorage marked as deconstructible and
tested.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:11>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version:
Severity: Release blocker | 1.7-beta-1
Keywords: FileSystemStorage, | Resolution: fixed
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Andrew Godwin <andrew@…>):
In [changeset:"827d5dc189fba33ec69d8916be310ff1e76361b1"]:
{{{
#!CommitTicketReference repository=""
revision="827d5dc189fba33ec69d8916be310ff1e76361b1"
Improve docs around deconstruction/serialisation (refs #22337)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:12>
Django
Reporter: nliberg | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version:
Severity: Release blocker | 1.7-beta-1
Keywords: FileSystemStorage, | Resolution: fixed
migrations | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Andrew Godwin <andrew@…>):
In [changeset:"1ed876ee5bd71092faf07559a25091bc27f96489"]:
{{{
#!CommitTicketReference repository=""
revision="1ed876ee5bd71092faf07559a25091bc27f96489"
[1.7.x] Improve docs around deconstruction/serialisation (refs #22337)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22337#comment:13>