Opened 10 years ago

Closed 10 years ago

Last modified 8 years ago

#22601 closed Bug (fixed)

Migrations don't recognize fields added by mixins

Reported by: Ilya Semenov Owned by: nobody
Component: Documentation Version: 1.7-beta-2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Consider the following app/models.py:

from django.db import models


class Mixin(object):
        tux = models.BooleanField(default=False)

class Foo(models.Model, Mixin):
        bar = models.BooleanField(default=False)

Running manage.py makemigrations generates a migration with no tux field:

# encoding: utf8
from __future__ import unicode_literals

from django.db import models, migrations
import app.models


class Migration(migrations.Migration):

    dependencies = [
    ]

    operations = [
        migrations.CreateModel(
            name='Foo',
            fields=[
                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
                ('bar', models.BooleanField(default=False)),
            ],
            options={
            },
            bases=(models.Model, app.models.Mixin),
        ),
    ]

It makes it hard to add reusable mixin logic to models (such as Orderable). South used to work fine with mixin fields.

Change History (13)

comment:1 by Simon Charette, 10 years ago

What is preventing you from using an abstract model here? That's exactly what they're meant for.

comment:2 by Ilya Semenov, 10 years ago

There can only be a single abstract base model, while mixins provide more degree of freedom. For instance, one could have a model which is Orderable and Approveable, and another model which is Orderable and Rateable.

Inheritance and mixins are different OOD concepts. Model inheritance is for the cases when a base model defines the base behavior, and derived classes refine or specify it. Mixins inject certain behaviour which is essentially unrelated to the actual model.

Mixins are fully supported by django models except of the new migrations module. It should either be fixed, or clearly stated that such kind of inheritance is unsupported.

Last edited 10 years ago by Ilya Semenov (previous) (diff)

comment:3 by Simon Charette, 10 years ago

There can only be a single abstract base model, while mixins provide more degree of freedom. For instance, one could have a model which is Orderable and Approveable, and another model which is Orderable and Rateable.

AFAIK Django supports multiple abstract model bases.

class Orderable(models.Model):
    order = models.PositiveIntegerField()

    class Meta:
        abstract = True

class Approveable(models.Model):
    approved = models.BooleanField(default=False)

    class Meta:
        abstract = True

class Concrete(Orderable, Approveable):
    pass

>>> print(Concrete._meta.fields)
[<django.db.models.fields.AutoField: id>, <django.db.models.fields.PositiveIntegerField: order>, <django.db.models.fields.BooleanField: approved>]

comment:4 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted

Accepting, at least on the documentation front.

comment:5 by mardini, 10 years ago

Django supports using mixins with models if you'd like to add some logic, but if the mixin adds some fields, it must inherit from models.Model, and that makes perfect sense IMO.
That's the reason why makemigrations didn't consider the field you added, since your Mixin is derived from object.
If the mixin, however, inherits models.Model, you'll be doing what's called Multi-table inheritance, and makemigrations would automatically create a OneToOneField for that.

I think there's no bug there, but since Tim asked for documentation, I added a note about that: https://github.com/django/django/pull/2724

Thanks.

comment:6 by Tim Graham, 10 years ago

Component: MigrationsDocumentation
Has patch: set

comment:7 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 2ea1e70b85fee2ee1229c62b82ae283bea68cb73:

Fixed #22601 -- Added a note about model inheritance.

Thanks semenov for the report.

comment:8 by Tim Graham <timograham@…>, 10 years ago

In 082920d8277719b799056b92f63420f051304ae7:

[1.6.x] Fixed #22601 -- Added a note about model inheritance.

Thanks semenov for the report.

Backport of 2ea1e70b85 from master

comment:9 by Tim Graham <timograham@…>, 10 years ago

In 5d7ad16a1b8c0efb29ab0d18370a4392308263f3:

[1.7.x] Fixed #22601 -- Added a note about model inheritance.

Thanks semenov for the report.

Backport of 2ea1e70b85 from master

comment:10 by Ilya Semenov, 10 years ago

I am disappointed by the fix. The inconsistency in the behaviour is still there. Non-Model-derived mixins that have Fields are still fully supported by all Django modules except the migrations. In the example above, syncdb will create the tux field in the database, QuerySets will fetch it, and object instances will have the tux property. Only the migrations module will silently ignore it.

If for some reason the migrations module can not be fixed to support non-Model-derived mixins (really? how come everything else works then?), they should be forbidden to use by the model meta class. The current implementation leaves a place for silent and hard to find mistakes, which is a bad thing. Having a clause in the documentation is not an excuse for leaving such pitfalls.

comment:11 by Daniele Procida, 10 years ago

I have had trouble in the past when trying to create mixins for models that had fields, but inherited from object rather than from Model.

I can't remember what these problems were; sorry to be vague.

On the other hand I have had no problems whatsoever creating numerous abstract model mixin classes, and building models using multiple mixins. It shouldn't be a problem.

comment:12 by Felipe, 8 years ago

Just ran into this when upgrading a custom AUTH_USER_MODEL to use the PermissionsMixin. makemigrations did not pick up any changes, but django choked on the tests:

django.db.utils.ProgrammingError: column accounts_koalauser.is_superuser does not exist

in reply to:  12 comment:13 by Felipe, 8 years ago

Woops I think the problem was the app wasn't getting tracked by makemigrations

Replying to felipeochoa:

Just ran into this when upgrading a custom AUTH_USER_MODEL to use the PermissionsMixin. makemigrations did not pick up any changes, but django choked on the tests:

django.db.utils.ProgrammingError: column accounts_koalauser.is_superuser does not exist

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