[Django] #22601: Migrations don't recognize fields added by mixins
Django
----------------------------+------------------------
Reporter: semenov | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+------------------------
Consider the following `app/models.py`:
{{{#!python
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:
{{{#!python
# 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22601>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
What is preventing you from using an `abstract` model here? That's exactly
what they're meant for.
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:1>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:2>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
> 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.
{{{#!python
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>]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:3>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* stage: Unreviewed => Accepted
Comment:
Accepting, at least on the documentation front.
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:4>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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 [https://docs.djangoproject.com/en/dev/topics/db/models/#multi-
table-inheritance 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:5>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* has_patch: 0 => 1
* component: Migrations => Documentation
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:6>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: closed
Component: Documentation | Version: 1.7-beta-2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"2ea1e70b85fee2ee1229c62b82ae283bea68cb73"]:
{{{
#!CommitTicketReference repository=""
revision="2ea1e70b85fee2ee1229c62b82ae283bea68cb73"
Fixed #22601 -- Added a note about model inheritance.
Thanks semenov for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:7>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: closed
Component: Documentation | Version: 1.7-beta-2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"5d7ad16a1b8c0efb29ab0d18370a4392308263f3"]:
{{{
#!CommitTicketReference repository=""
revision="5d7ad16a1b8c0efb29ab0d18370a4392308263f3"
[1.7.x] Fixed #22601 -- Added a note about model inheritance.
Thanks semenov for the report.
Backport of 2ea1e70b85 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:9>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: closed
Component: Documentation | Version: 1.7-beta-2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"082920d8277719b799056b92f63420f051304ae7"]:
{{{
#!CommitTicketReference repository=""
revision="082920d8277719b799056b92f63420f051304ae7"
[1.6.x] Fixed #22601 -- Added a note about model inheritance.
Thanks semenov for the report.
Backport of 2ea1e70b85 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:8>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: closed
Component: Documentation | Version: 1.7-beta-2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:10>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: closed
Component: Documentation | Version: 1.7-beta-2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by EvilDMP):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:11>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: closed
Component: Documentation | Version: 1.7-beta-2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:12>
Django
Reporter: semenov | Owner: nobody
Type: Bug | Status: closed
Component: Documentation | Version: 1.7-beta-2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felipeochoa):
Woops I think the problem was the app wasn't getting tracked by
makemigrations
Replying to [comment:12 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
>
> }}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22601#comment:13>