Opened 4 years ago

Closed 4 years ago

Last modified 3 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 Changed 4 years ago by Simon Charette

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

comment:2 Changed 4 years ago by Ilya Semenov

There can only be a single abstract model base, while mixins provide more degree a 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 OO 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.

Version 0, edited 4 years ago by Ilya Semenov (next)

comment:3 Changed 4 years ago by Simon Charette

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 Changed 4 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

Accepting, at least on the documentation front.

comment:5 Changed 4 years ago by mardini

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 Changed 4 years ago by Tim Graham

Component: MigrationsDocumentation
Has patch: set

comment:7 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 2ea1e70b85fee2ee1229c62b82ae283bea68cb73:

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

Thanks semenov for the report.

comment:8 Changed 4 years ago by Tim Graham <timograham@…>

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 Changed 4 years ago by Tim Graham <timograham@…>

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 Changed 4 years ago by Ilya Semenov

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 Changed 4 years ago by Daniele Procida

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 Changed 3 years ago by Felipe

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

comment:13 in reply to:  12 Changed 3 years ago by Felipe

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