#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 , 11 years ago
comment:2 by , 11 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.
comment:3 by , 11 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 , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Accepting, at least on the documentation front.
comment:5 by , 11 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 , 11 years ago
Component: | Migrations → Documentation |
---|---|
Has patch: | set |
comment:7 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:10 by , 11 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 , 11 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.
follow-up: 13 comment:12 by , 9 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
comment:13 by , 9 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 thePermissionsMixin
.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
What is preventing you from using an
abstract
model here? That's exactly what they're meant for.