#22447 closed Bug (fixed)
Migrations have bases=(NewBase,) for models with custom metaclass
Reported by: | Craig de Stigter | Owned by: | Simon Charette |
---|---|---|---|
Component: | Migrations | Version: | 1.7-beta-1 |
Severity: | Release blocker | Keywords: | |
Cc: | Sebastian Bauer | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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
Attachments (1)
Change History (14)
by , 11 years ago
Attachment: | testproject.zip added |
---|
comment:1 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 11 years ago
Severity: | Normal → Release blocker |
---|
comment:3 by , 11 years ago
Cc: | added |
---|
comment:4 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 11 years ago
Has patch: | set |
---|
Can you make sure the following patch works for you https://github.com/django/django/pull/2567.
comment:6 by , 11 years ago
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.
comment:7 by , 11 years ago
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 base.__dict__))):
comment:8 by , 11 years ago
It doesn't work in the mttp
case because they skip ModelBase.__new__
here, it would work if they called super(MPTTModelBase, cls).__new__
instead.
Unsure how to deal correctly with this. I've proposed six
to replace their with_metaclass
implementation with the one Flask uses in order to prevent those NewBase
artifacts. I ran the full Django test suite with this modified version and the NewBase
hacks removed 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.
comment:9 by , 11 years ago
I went ahead and created a pull request on six
for this issue. Hopefully if it's merged in we can quickly merge a fix for Django as well.
comment:10 by , 11 years ago
Your pull request seems to have been merged into six
so I went forward and updated 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.
comment:12 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Test project that shows the problem