Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#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)

testproject.zip (10.1 KB ) - added by Craig de Stigter 11 years ago.
Test project that shows the problem

Download all attachments as: .zip

Change History (14)

by Craig de Stigter, 11 years ago

Attachment: testproject.zip added

Test project that shows the problem

comment:1 by Simon Charette, 11 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Simon Charette, 11 years ago

Severity: NormalRelease blocker

comment:3 by Sebastian Bauer, 11 years ago

Cc: Sebastian Bauer added

comment:4 by Simon Charette, 11 years ago

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:5 by Simon Charette, 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 Craig de Stigter, 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 Craig de Stigter, 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__))):
Last edited 11 years ago by Craig de Stigter (previous) (diff)

comment:8 by Simon Charette, 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.

Last edited 11 years ago by Simon Charette (previous) (diff)

comment:9 by Ben Davis, 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.

Last edited 11 years ago by Ben Davis (previous) (diff)

comment:10 by Simon Charette, 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:11 by Aymeric Augustin, 11 years ago

Triage Stage: AcceptedReady for checkin

Ship it!

comment:12 by Simon Charette <charette.s@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 390f888745803a2f6c75c5a22402daf1221f8e29:

Fixed #22447 -- Make sure custom model bases can be migrated.

Thanks to cdestigter for the report.

comment:13 by Simon Charette <charette.s@…>, 11 years ago

In 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

Note: See TracTickets for help on using tickets.
Back to Top