Opened 10 years ago

Closed 10 years ago

Last modified 10 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 10 years ago.
Test project that shows the problem

Download all attachments as: .zip

Change History (14)

by Craig de Stigter, 10 years ago

Attachment: testproject.zip added

Test project that shows the problem

comment:1 by Simon Charette, 10 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Simon Charette, 10 years ago

Severity: NormalRelease blocker

comment:3 by Sebastian Bauer, 10 years ago

Cc: Sebastian Bauer added

comment:4 by Simon Charette, 10 years ago

Owner: changed from nobody to Simon Charette
Status: newassigned

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

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

Version 0, edited 10 years ago by Simon Charette (next)

comment:9 by Ben Davis, 10 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 10 years ago by Ben Davis (previous) (diff)

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

Triage Stage: AcceptedReady for checkin

Ship it!

comment:12 by Simon Charette <charette.s@…>, 10 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@…>, 10 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