Initial migration creates fields not listed in the migration if mixin class changes

[Michal Dabski]

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

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

[Andrei Petre]

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 ​in inheritance documentation, so I'd probably just use abstract models ​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 ​this github package, but I'd stay away from it unless I understood what it did, which I don't haha.

[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.

[Andrei Petre]

Ah, I see, that PermissionMixin 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.

[Tim Graham]

I guess it's related and/or duplicate of #23521.

[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 ​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.

[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.

[Giannis Terzopoulos]

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(null=True)),
        ],
        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)