Opened 2 months ago

Last modified 2 months ago

#28438 new Bug

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

Reported by: Michal Dabski Owned by: nobody
Component: Migrations Version: 1.11
Severity: Normal Keywords: migration, models, mixin, postgresql
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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

Change History (9)

comment:1 Changed 2 months ago by Michal Dabski

Description: modified (diff)

comment:2 Changed 2 months ago by Michal Dabski

Keywords: migration models mixin sql added

comment:3 Changed 2 months ago by Michal Dabski

Description: modified (diff)

comment:4 Changed 2 months ago by Paul Sajna

Component: UncategorizedMigrations
Type: UncategorizedBug

comment:5 Changed 2 months ago by Andrei Petre

Keywords: postgresql added; sql removed

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.

comment:6 Changed 2 months ago 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.

comment:7 Changed 2 months ago by 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.
Last edited 2 months ago by Andrei Petre (previous) (diff)

comment:8 Changed 2 months ago by Tim Graham

Triage Stage: UnreviewedAccepted

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

comment:9 Changed 2 months ago 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 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.

Note: See TracTickets for help on using tickets.
Back to Top