[Django] #28438: Initial migration creates fields not listed in the migration if mixin class changes
Django
changes
-----------------------------------------+------------------------
Reporter: Michal Dabski | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.11
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Consider the sample model with a mixin class:
{{{
from django.db import models
class TestMixin(object):
pass
class TestModel(TestMixin, models.Model):
sample_field = models.CharField(max_length=20)
}}}
When running makemigrations, django creates a perfectly good and valid
migration:
{{{
# -*- coding: utf-8 -*-
# Generated by Django 1.11.3 on 2017-07-26 17:03
from __future__ import unicode_literals
from django.db import migrations, models
import migration_test.models
class Migration(migrations.Migration):
initial = True
dependencies = [
]
operations = [
migrations.CreateModel(
name='TestModel',
fields=[
('id', models.AutoField(auto_created=True,
primary_key=True, serialize=False, verbose_name='ID')),
('sample_field', models.CharField(max_length=20)),
],
bases=(migration_test.models.TestMixin, models.Model),
),
]
}}}
SQL:
{{{
BEGIN;
--
-- Create model TestModel
--
CREATE TABLE "migration_test_testmodel" ("id" serial NOT NULL PRIMARY KEY,
"sample_field" varchar(20) NOT NULL);
COMMIT;
}}}
Next, refactor mixin class to add a field to it - need to change mixin's
base class to {{{models.Model}}}, otherwise field will not be correctly
inherited by models:
{{{
class TestMixin(models.Model):
mixin_field = models.CharField(max_length=20, default='test')
class Meta:
abstract = True
}}}
Thie creates a new migration which adds {{{mixin_field}}} to it - nothing
special. However, when applying both migrations after the model changes,
second migration fails with the following error
{{{django.db.utils.ProgrammingError: column "mixin_field" of relation
"migration_test_testmodel" already exists}}}. as it turns out, the first
migration's SQL has now changed to include {{{mixin_field}}}:
{{{
BEGIN;
--
-- Create model TestModel
--
CREATE TABLE "migration_test_testmodel" ("id" serial NOT NULL PRIMARY KEY,
"mixin_field" varchar(20) NOT NULL, "sample_field" varchar(20) NOT NULL);
COMMIT;
}}}
The python code of the migration obviously has not changed, but the
resulting SQL did, and it includes a field not explicitly listed in the
migration's {{{fields}}}.
Note:
- this does not happen if the mixin class extends {{{models.Model}}} to
begin with.
- if model extends a mixin that extends {{{object}}}, it ends up in
model's {{{bases}}}, however if mixin extends {{{model.Model}}} it does
not.
Tested with Django 1.11.3, Python 2.7
--
Ticket URL: <https://code.djangoproject.com/ticket/28438>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
changes
Reporter: Michal Dabski | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.11
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Michal Dabski:
Old description:
New description:
class TestMixin(object):
pass
class Migration(migrations.Migration):
initial = True
dependencies = [
]
}}}
SQL:
This creates a new migration which adds {{{mixin_field}}} to it - nothing
special. However, when applying both migrations after the model changes,
second migration fails with the following error
{{{django.db.utils.ProgrammingError: column "mixin_field" of relation
"migration_test_testmodel" already exists}}}. as it turns out, the first
migration's SQL has now changed to include {{{mixin_field}}}:
{{{
BEGIN;
--
-- Create model TestModel
--
CREATE TABLE "migration_test_testmodel" ("id" serial NOT NULL PRIMARY KEY,
"mixin_field" varchar(20) NOT NULL, "sample_field" varchar(20) NOT NULL);
COMMIT;
}}}
The python code of the migration obviously has not changed, but the
resulting SQL did, and it includes a field not explicitly listed in the
migration's {{{fields}}}.
Note:
- this does not happen if the mixin class extends {{{models.Model}}} to
begin with.
- if model extends a mixin that extends {{{object}}}, it ends up in
model's {{{bases}}}, however if mixin extends {{{model.Model}}} it does
not.
Tested with Django 1.11.3, Python 2.7
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28438#comment:1>
Django
changes
Reporter: Michal Dabski | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.11
Keywords: | Triage Stage:
migration,models,mixin,sql | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Michal Dabski):
* keywords: => migration,models,mixin,sql
--
Ticket URL: <https://code.djangoproject.com/ticket/28438#comment:2>
Django
changes
Reporter: Michal Dabski | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.11
Keywords: | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Michal Dabski:
Old description:
> Consider the sample model with a mixin class:
> This creates a new migration which adds {{{mixin_field}}} to it - nothing
> special. However, when applying both migrations after the model changes,
> second migration fails with the following error
> {{{django.db.utils.ProgrammingError: column "mixin_field" of relation
> "migration_test_testmodel" already exists}}}. as it turns out, the first
> migration's SQL has now changed to include {{{mixin_field}}}:
> {{{
> BEGIN;
> --
> -- Create model TestModel
> --
> CREATE TABLE "migration_test_testmodel" ("id" serial NOT NULL PRIMARY
> KEY, "mixin_field" varchar(20) NOT NULL, "sample_field" varchar(20) NOT
> NULL);
> COMMIT;
> }}}
> The python code of the migration obviously has not changed, but the
> resulting SQL did, and it includes a field not explicitly listed in the
> migration's {{{fields}}}.
>
> Note:
> - this does not happen if the mixin class extends {{{models.Model}}} to
> begin with.
> - if model extends a mixin that extends {{{object}}}, it ends up in
> model's {{{bases}}}, however if mixin extends {{{model.Model}}} it does
> not.
>
> Tested with Django 1.11.3, Python 2.7
New description:
class TestMixin(object):
pass
class Migration(migrations.Migration):
initial = True
dependencies = [
]
}}}
SQL:
This creates a new migration which adds {{{mixin_field}}} to it - nothing
special. However, when applying both migrations after the model changes,
second migration fails with the following error
{{{django.db.utils.ProgrammingError: column "mixin_field" of relation
"migration_test_testmodel" already exists}}}. as it turns out, the first
migration's SQL has now changed to include {{{mixin_field}}}:
{{{
BEGIN;
--
-- Create model TestModel
--
CREATE TABLE "migration_test_testmodel" ("id" serial NOT NULL PRIMARY KEY,
"mixin_field" varchar(20) NOT NULL, "sample_field" varchar(20) NOT NULL);
COMMIT;
}}}
The python code of the migration obviously has not changed, but the
resulting SQL did, and it includes a field not explicitly listed in the
migration's {{{fields}}}.
Note:
- this does not happen if the mixin class extends {{{models.Model}}} to
begin with.
- if model extends a mixin that extends {{{object}}}, it ends up in
model's {{{bases}}}, however if mixin extends {{{model.Model}}} it does
not.
- when mixin's base class changes, schema migration does not reflect this
change in model's {{{bases}}}
Tested with Django 1.11.3, Python 2.7
Proposed solution:
migration should not create database fields for model fields not
explicitly listed in {{{fields}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28438#comment:3>
Django
changes
Reporter: Michal Dabski | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Paul Sajna):
* type: Uncategorized => Bug
* component: Uncategorized => Migrations
--
Ticket URL: <https://code.djangoproject.com/ticket/28438#comment:4>
Django
changes
Reporter: Michal Dabski | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andrei Petre):
* keywords: migration,models,mixin,sql =>
migration,models,mixin,postgresql
Comment:
Hi Michal,
I'm not sure why the `bases=...` is added when you have a non-abstract
base class that doesn't extend from `models.Model`. Maybe someone more
knowledgable could answer that. I can't find any mention of mixin
[https://docs.djangoproject.com/en/1.11/topics/db/models/#model-
inheritance in inheritance documentation], so I'd probably just use
abstract models [https://stackoverflow.com/questions/3254436/django-model-
mixins-inherit-from-models-model-or-from-object#25817237 as suggested in
this SO post] to avoid going in undocumented territory.
Maybe documentation not to use `object` base class with models could be
added? Or trigger an error if you tried doing that to not shoot yourself
in the foot?
I could reproduce this for `postgres` (worked for `sqlite` since it
generates a new table in 2nd migration and copies every row, instead of
adding a new column; probably inefficient, not the way to go anyway), so
assuming that's what you used.
Also found [https://github.com/tbartelmess/django-model-mixins this github
package], but I'd stay away from it unless I understood what it did, which
I don't haha.
--
Ticket URL: <https://code.djangoproject.com/ticket/28438#comment:5>
Django
changes
Reporter: Michal Dabski | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michal Dabski):
At this point my code has been deployed to production with mixin
inheriting from object. I use mixin to reuse code across two models that
have separete hierarchy trees, so common base class was not an option.
At the moment my best workaround is to keep the mixin as-is, and add a
field to each subclass as needed, but that violates separation of concerns
principle and DRY.
I have seen mixin pattern being used in models in Django itself, so I
think it should be OK to use mixins. For example, {{{PermissionMixin}}}
class. The only difference is that my mixin used to inherit from
{{{object}}} originally, which was recorded by the initial migration.
--
Ticket URL: <https://code.djangoproject.com/ticket/28438#comment:6>
Django
changes
Reporter: Michal Dabski | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andrei Petre):
Ah, I see, that mixin is made using an abstract model as I thought, cool,
thanks.
Ok, in that case maybe you could ask in `django-users` or ask in IRC for
help? Some ideas of how to overcome that:
- fix code as you suggested by using abstract and fake that 2nd migration
to run, since that won't happen again for that same model (but other
django code might rerun migrations, not sure? e.g. tests? but can be
turned off) ; so next time you add another field to that mixin it won't
change migration 0001 since you already ran it, so the newly generated
migration to `AddField` will run fine. Might also help to squash all
migrations so it'll generate proper new tables?
- or, if possible, create a new model and extend from proper mixin and
copy all records over? But this seems more complicated in an environment
with lots of data, probably there's a better workaround like the one above
if that works.
--
Ticket URL: <https://code.djangoproject.com/ticket/28438#comment:7>
Django
changes
Reporter: Michal Dabski | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
migration,models,mixin,postgresql |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
I guess it's related and/or duplicate of #23521.
--
Ticket URL: <https://code.djangoproject.com/ticket/28438#comment:8>
Django
changes
Reporter: Michal Dabski | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
migration,models,mixin,postgresql |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Shai Berger):
My first reaction, when I saw this ticket, was to think that it's invalid:
Once you have a migration that uses some code construct -- a function or a
base-class -- that code should not change, as changing it in any
significant way undermines the idea that migrations recreate the models as
they were at the time the migration was generated.
However, that's not what our docs say -- the
[https://docs.djangoproject.com/en/1.11/topics/migrations/#historical-
models docs] only say that the function or class needs "to be kept
around", not that it needs to be kept unchanged.
That said, I still don't think Django should support such changes, other
than to flag them as errors.
**That** said, I'm not sure if there's any reasonable way to detect these
situations automatically.
Either way, I think we should fix the docs.
#23521 may be related, but I don't think it's a duplicate, as the use-case
described there is an intended one.
--
Ticket URL: <https://code.djangoproject.com/ticket/28438#comment:9>
Django
changes
Reporter: Michal Dabski | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
migration,models,mixin,postgresql |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Matthew Schinckel):
I hit this one today: without testing (I'll try to get there), it seems
likely that adding fields to an abstract model will have a similar result.
--
Ticket URL: <https://code.djangoproject.com/ticket/28438#comment:10>
Django
changes
Reporter: Michal Dabski | Owner: nobody
Component: Migrations | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
migration,models,mixin,postgresql |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by giannisterzopoulos):
This seems to be still an issue. Here's couple of things that I am
noticing:
- There is, of course, no problem if you run `makemigrations` once in the
end, after converting the Mixin into an abstract Model.
- There is also no problem if you run `migrate` before changing the Mixin,
and then `makemigrations` and `migrate` again.\\
That actually results to the exact same migration as in the problematic
case.
Here is a test I wrote that can reproduce the issue, in case it can be
helpful:
{{{
# <In tests.migrations.test_operations.OperationTests>
def test_field_can_be_added_to_mixin_bases(self):
# Replicating the scenario where a Mixin class has initially no fields
# and does not extend Model; and then gets refactored to an abstract
Model
# with fields specified.
class SomeMixin(models.Model):
cuteness = models.IntegerField(null=True)
class Meta:
abstract = True
operation = migrations.CreateModel(
name='CutePony',
fields=[
('id', models.AutoField(primary_key=True)),
('pink', models.IntegerField(default=3)),
],
bases=(SomeMixin, models.Model),
)
app_label = 'test_28438'
project_state, new_state = self.make_test_state(app_label, operation)
with connection.schema_editor() as editor:
operation.database_forwards(app_label, editor, project_state,
new_state)
project_state = new_state
new_state = project_state.clone()
operation = migrations.AddField(
model_name='CutePony',
name='cuteness',
field=models.IntegerField(null=True)
)
operation.state_forwards(app_label, new_state)
with connection.schema_editor() as editor:
operation.database_forwards(app_label, editor, project_state,
new_state)
self.assertTableExists(f'{app_label}_cutepony')
}}}
(This raises: `django.db.utils.OperationalError: duplicate column name:
cuteness`)
--
Ticket URL: <https://code.djangoproject.com/ticket/28438#comment:11>