Opened 15 months ago

Closed 14 months ago

Last modified 14 months ago

#22447 closed Bug (fixed)

Migrations have bases=(NewBase,) for models with custom metaclass

Reported by: cdestigter Owned by: charettes
Component: Migrations Version: 1.7-beta-1
Severity: Release blocker Keywords:
Cc: anih 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 cdestigter 15 months ago.
Test project that shows the problem

Download all attachments as: .zip

Change History (14)

Changed 15 months ago by cdestigter

Test project that shows the problem

comment:1 Changed 15 months ago by charettes

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 15 months ago by charettes

  • Severity changed from Normal to Release blocker

comment:3 Changed 15 months ago by anih

  • Cc anih added

comment:4 Changed 15 months ago by charettes

  • Owner changed from nobody to charettes
  • Status changed from new to assigned

comment:5 Changed 15 months ago by charettes

  • Has patch set

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

comment:6 Changed 15 months ago 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.

comment:7 Changed 15 months ago 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 base.__dict__))):
Last edited 15 months ago by cdestigter (previous) (diff)

comment:8 Changed 15 months ago by charettes

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 15 months ago by charettes (previous) (diff)

comment:9 Changed 15 months ago by bendavis78

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 15 months ago by bendavis78 (previous) (diff)

comment:10 Changed 14 months ago by charettes

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 14 months ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

Ship it!

comment:12 Changed 14 months ago by Simon Charette <charette.s@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 390f888745803a2f6c75c5a22402daf1221f8e29:

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

Thanks to cdestigter for the report.

comment:13 Changed 14 months 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