Opened 2 years ago

Closed 2 years ago

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

Download all attachments as: .zip

Change History (14)

Changed 2 years ago by Craig de Stigter

Attachment: testproject.zip added

Test project that shows the problem

comment:1 Changed 2 years ago by Simon Charette

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 2 years ago by Simon Charette

Severity: NormalRelease blocker

comment:3 Changed 2 years ago by Sebastian Bauer

Cc: Sebastian Bauer added

comment:4 Changed 2 years ago by Simon Charette

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:5 Changed 2 years ago by Simon Charette

Has patch: set

Can you make sure the following patch works for you https://github.com/django/django/pull/2567.

comment:6 Changed 2 years ago by Craig de Stigter

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 Changed 2 years ago by Craig de Stigter

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 2 years ago by Craig de Stigter (previous) (diff)

comment:8 Changed 2 years ago by Simon Charette

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 2 years ago by Simon Charette (previous) (diff)

comment:9 Changed 2 years ago by Ben Davis

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 2 years ago by Ben Davis (previous) (diff)

comment:10 Changed 2 years ago by Simon Charette

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 Changed 2 years ago by Aymeric Augustin

Triage Stage: AcceptedReady for checkin

Ship it!

comment:12 Changed 2 years ago by Simon Charette <charette.s@…>

Resolution: fixed
Status: assignedclosed

In 390f888745803a2f6c75c5a22402daf1221f8e29:

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

Thanks to cdestigter for the report.

comment:13 Changed 2 years ago by Simon Charette <charette.s@…>

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