[Django] #22447: Migrations have bases=(NewBase, ) for models with custom metaclass
Django
----------------------------+------------------------
Reporter: cdestigter | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+------------------------
With this model:
{{{
from django.db import models
from django.db.models.base import ModelBase
from django.utils import six
class MetaZork(ModelBase):
pass
class BlibbityBlorg(six.with_metaclass(MetaZork, models.Model)):
ZerbyBlugg = models.CharField(max_length=1)
}}}
`django-admin makemigrations` produces the following:
{{{
operations = [
migrations.CreateModel(
name=b'BlibbityBlorg',
fields=[
('id', models.AutoField(verbose_name='ID',
serialize=False, auto_created=True, primary_key=True)),
(b'ZerbyBlugg', models.CharField(max_length=1)),
],
options={
},
bases=(django.db.models.base.NewBase,),
),
]
}}}
NewBase is a `six` artifact; the correct base would be models.Model.
Originally reported by petkostas on django-mptt: https://github.com
/django-mptt/django-mptt/issues/302
--
Ticket URL: <https://code.djangoproject.com/ticket/22447>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: cdestigter | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-1
Keywords: | Triage Stage: Accepted
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_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:1>
Django
Reporter: cdestigter | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* severity: Normal => Release blocker
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:2>
Django
Reporter: cdestigter | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anih):
* cc: anih (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:3>
Django
Reporter: cdestigter | Owner: charettes
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* status: new => assigned
* owner: nobody => charettes
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:4>
Django
Reporter: cdestigter | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* has_patch: 0 => 1
Comment:
Can you make sure the following patch works for you
https://github.com/django/django/pull/2567.
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:5>
Django
Reporter: cdestigter | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cdestigter):
This works for the testproject I submitted, but doesn't work for mptt
because `__module__` is different:
{{{
(Pdb) base
<class 'mptt.models.NewBase'>
(Pdb) base.__module__
'mptt.models'
}}}
I'm not sure why six is setting this module differently; it's nothing
special that mptt is doing AFAIK.
Doing some quick research into the differences, I'll post back shortly.
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:6>
Django
Reporter: cdestigter | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cdestigter):
Well, I still don't understand why this happens, but it seems that the
`NewBase` doesn't always have the `__module__ you expect. So maybe the
code shouldn't check for that.
{{{
ipdb> MuppettZarkity.mro()
[<class 'testproject.testapp.models.MuppettZarkity'>, <class
'mptt.models.MPTTModel'>, <class 'mptt.models.NewBase'>, <class
'django.db.models.base.Model'>, <class 'django.db.models.base.NewBase'>,
<type 'object'>]
ipdb> BlibbityBlorg.mro()
[<class 'testproject.testapp.models.BlibbityBlorg'>, <class
'testproject.testapp.models.ZerbyJarb'>, <class
'django.db.models.base.NewBase'>, <class 'django.db.models.base.Model'>,
<class 'django.db.models.base.NewBase'>, <type 'object'>]
}}}
However they do consistenly have nothing much in their `.__dict__` so
perhaps that's a better check than `.__module__`:
{{{
if (base.__name__ == 'NewBase' and
any((not attr.startswith('__') for attr in
dir(newbase)))):
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:7>
Django
Reporter: cdestigter | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
It doesn't work in the `mttp` case because they skip `ModelBase.__new__`
[https://github.com/django-mptt/django-
mptt/blob/master/mptt/models.py#L217-L218 here], it would work if they
called `super(MPTTModelBase, cls).__new__` instead.
Unsure how to deal correctly with this.
[https://bitbucket.org/gutworth/six/issue/66/replace-the-implementation-of
I've proposed] `six` to replace their `with_metaclass` implementation with
[https://github.com/mitsuhiko/flask/blob/6ec83e18dca497a8fbfca6caca5999984bd32f2e/flask/_compat.py#L56-L73
the one Flask uses] in order to prevent those `NewBase` artifacts. I ran
the full test suite with this modified version and it fully passed.
Fixing the issue at this level makes more sense me than trying to work
around it by `__mro__` and `__dict__` checks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:8>
Django
Reporter: cdestigter | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bendavis78):
I went ahead and created a [https://bitbucket.org/gutworth/six/pull-
request/35/fixed-66-replace-the-implementation-of/diff:pull request] on
``six`` for this issue. Hopefully if it's merged in we can quickly merge a
fix for Django as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:9>
Django
Reporter: cdestigter | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
Your pull request seems to have been merged into `six` so I went forward
and updated [https://github.com/django/django/pull/2567 my PR] to break
changes into two commits: one that backport the new version of
`with_metaclass` to our vendored version of `six` and a second one that
adds a regression test case for the issue reported here.
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:10>
Django
Reporter: cdestigter | Owner: charettes
Type: Bug | Status: assigned
Component: Migrations | Version:
Keywords: | Resolution:
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* stage: Accepted => Ready for checkin
Comment:
Ship it!
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:11>
Django
Reporter: cdestigter | Owner: charettes
Component: Migrations | Version:
Severity: Release blocker | 1.7-beta-1
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"390f888745803a2f6c75c5a22402daf1221f8e29"]:
{{{
#!CommitTicketReference repository=""
revision="390f888745803a2f6c75c5a22402daf1221f8e29"
Fixed #22447 -- Make sure custom model bases can be migrated.
Thanks to cdestigter for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:12>
Django
Reporter: cdestigter | Owner: charettes
Type: Bug | Status: closed
Component: Migrations | Version:
Severity: Release blocker | 1.7-beta-1
Keywords: | Resolution: fixed
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette <charette.s@…>):
In [changeset:"cda5745df0e9301c65e13f552ee19a4bf0490997"]:
{{{
#!CommitTicketReference repository=""
revision="cda5745df0e9301c65e13f552ee19a4bf0490997"
[1.7.x] Fixed #22447 -- Make sure custom model bases can be migrated.
Thanks to cdestigter for the report.
Backport of 390f888745 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22447#comment:13>