[Django] #22373: Django Migrations keeps detecting changes to FileField using custom storage
Django
-------------------------------------------+------------------------
Reporter: Matthias Pronk <django@…> | Owner:
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------------+------------------------
I've a FileField that uses a custom storage backend, which is a subclass
of FileSystemStorage. After adding a deconstruct function to my custom
storage backend, Django's migration framework keeps detecting that the
FileField has changed ad infinitum.
{{{
from django.core.files.storage import FileSystemStorage
class MediaStorage(FileSystemStorage):
def deconstruct(self):
return ('foo.media.storage.MediaStorage', [], {})
}}}
The migration file shows:
{{{
migrations.AlterField(
model_name='file',
name='file',
field=models.FileField(storage=foo.media.storage.MediaStorage(),
upload_to='foo_media/', max_length=255, verbose_name=u'file'),
),
}}}
Note: This bug is related to #22337, but not the same.
--
Ticket URL: <https://code.djangoproject.com/ticket/22373>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Matthias Pronk | Owner:
<django@…> | Status: new
Type: Bug | Version: master
Component: Migrations | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by bmispelon):
* severity: Normal => Release blocker
* needs_better_patch: => 0
* needs_tests: => 0
* version: 1.7-beta-1 => master
* needs_docs: => 0
* stage: Unreviewed => Accepted
Comment:
Hi,
It seems that the autodetector is comparing the instances of the storage
class instead of their deconstructed versions.
Since the storage class doesn't implement `__eq__`, this means that the
autodetector wrongly assumes you're using a new object and therefore
create a new migration.
Not sure what the best course of action is, but this is definitely a bug
(I'll also mark it as a release blocker).
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22373#comment:1>
Django
Reporter: Matthias Pronk | Owner:
<django@…> | Status: new
Type: Bug | Version: master
Component: Migrations | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by chriscauley):
Are the details of the storage actually used in the database? If not then
I would argue that they have no place in the migration. I brought this up
on several other related tickets (#22351 and #22436). If you make
`my_module.MyCustomStorage` part of the migration and then change the name
or implementation details of it then the past migrations will be broken.
My solution in #22426 is to have `Field.deconstruct()` replace functions
for `upload_to` and other similar kwargs with a function that raises
NotImplementedError when called. It seems to work (I'm very new at this,
so please be brutal in your criticism), and I think a similar approach
would work here. There's a link to my git commit in the last comment on
#22426. Any thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/22373#comment:2>
Django
Reporter: Matthias Pronk | Owner:
<django@…> | Status: new
Type: Bug | Version: master
Component: Migrations | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Matthias Pronk <django@…>):
@chriscauley: No, the details of the storage are not related to the
database in any way. So if I can keep this out of the migration i'll be
fine. I'll try your "workaround" of raising a NotImplementedException.
--
Ticket URL: <https://code.djangoproject.com/ticket/22373#comment:3>
Django
Reporter: Matthias Pronk | Owner:
<django@…> | Status: new
Type: Bug | Version: master
Component: Migrations | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by chriscauley):
You should also consider just removing the storage kwarg in
`FileField.deconstruct()`. Since it's an optional argument this may make
more sense. `upload_to` is required this is not an option in the other
tickets. Then again, if a migration somehow touches a storage it may be
better to raise `NotImplementedError`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22373#comment:4>
Django
Reporter: Matthias Pronk | Owner:
Type: Bug | Version: master
Component: Migrations | Resolution: duplicate
Severity: Release blocker | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => duplicate
Comment:
Duplicate of #22337
--
Ticket URL: <https://code.djangoproject.com/ticket/22373#comment:5>