Metaclass conflict when doing createmigrations in ModelState.render

[kosz85]

I'm migrating my project from Django 1.6.x to 1.8.2, I found this bug and fix for it.
In my project I'm using few ModelMixins with custom __metadata__, problem is that render is trying do a class using type but this code forgot about python class metadata multi inheritance.

Traceback looks like this, for each model with more complex metadata:

$ /manage.py makemigrations
(...)
Traceback (most recent call last):
  File "/home/vagrant/***/manage.py", line 10, in <module>
    execute_from_command_line(sys.argv)
  File "/home/vagrant/env/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 338, in execute_from_command_line
    utility.execute()
  File "/home/vagrant/env/local/lib/python2.7/site-packages/django/core/management/__init__.py", line 330, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/home/vagrant/env/local/lib/python2.7/site-packages/django/core/management/base.py", line 390, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/home/vagrant/env/local/lib/python2.7/site-packages/django/core/management/base.py", line 441, in execute
    output = self.handle(*args, **options)
  File "/home/vagrant/env/local/lib/python2.7/site-packages/django/core/management/commands/makemigrations.py", line 125, in handle
    migration_name=self.migration_name,
  File "/home/vagrant/env/local/lib/python2.7/site-packages/django/db/migrations/autodetector.py", line 43, in changes
    changes = self._detect_changes(convert_apps, graph)
  File "/home/vagrant/env/local/lib/python2.7/site-packages/django/db/migrations/autodetector.py", line 110, in _detect_changes
    self.old_apps = self.from_state.concrete_apps
  File "/home/vagrant/env/local/lib/python2.7/site-packages/django/db/migrations/state.py", line 170, in concrete_apps
    self.apps = StateApps(self.real_apps, self.models, ignore_swappable=True)
  File "/home/vagrant/env/local/lib/python2.7/site-packages/django/db/migrations/state.py", line 232, in __init__
    self.render_multiple(list(models.values()) + self.real_models)
  File "/home/vagrant/env/local/lib/python2.7/site-packages/django/db/migrations/state.py", line 262, in render_multiple
    model.render(self)
  File "/home/vagrant/env/local/lib/python2.7/site-packages/django/db/migrations/state.py", line 548, in render
    body,
TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases

Fix is simple. I used classmaker from here ​http://code.activestate.com/recipes/204197-solving-the-metaclass-conflict/
And replaced line 543 of db/migrations/state.py from this:

        # Then, make a Model object (apps.register_model is called in __new__)
        return type(
            str(self.name),
            bases,
            body,
        )

to this:

        # Then, make a Model object (apps.register_model is called in __new__)
        from lib.utils.meta_maker import classmaker
        return classmaker()(str(self.name), bases, body)

My lib.utils.meta_maker is code from link I provided, with little changes. This way it works without a problems as metaclases are handled properly.
Classmaker in most cases will return type, but for my more complex models will do the magic.

[Tim Graham]

Thanks for the report. We'll need a regression test so we can reproduce the issue.

[kosz85]

I attached isolated case from my project. There is also version of metaclassmaker that I use, it's same as in the link. I know that playing with multiple inheritance and metaclasses is not so simple, but with metaclassmaker it's also not a problem, and I get quite nice features at model level like all subclasses (and subsubsubsub classes ) aware class registry, or some class aware lists, and other magic tools.
With this one you can create all things like Notifications, LogTypes, IndexableMixins, SyncMixins, ..., with automatic population of all ever created classes of objects in nice and neat way.

[Tim Graham]

It looks like there's no changes needed to make this work on Python 3. I'm in favor of "won't fix" given the complexity of bundling metaclassmaker in Django and the timetable for dropping Python 2 support. Thoughts?

[kosz85]

The same problem will occur at Python 3 probably, as metaclasses mechanics didn't change too much. If you want I can work on version for Python 3 also, I'm thinking about migrating my code anyway.

[kosz85]

Metaconflicts isolated example - with six (tested on 2.7 & 3.4)

[kosz85]

I updated example, I used six to make it more generic. The problem exists in both python 2.7 and 3.4, I added new method six_with_metaclassmaker it's similar to six.with_metaclass but more specific for this case, as six.with_metaclass and six.add_metaclass aren't made for such cases:

• The first (correct) approach is assuming that meta is already a class, and not function like in 2.7, but metaclassmaker is just a factory of proper type and not type class itself.
• Second approach (incorrect) can't deal with class that rise error itself because of metaclass inheritance we want to deal with.
• So third approach is copy of first approach but using type as base for proxy class for metaclassmaker

[Tim Graham]

Could you put together a patch so we can more easily see what's involved in fixing it?

[kosz85]

Hah, I wanted to avoid it, as I have some nasty deadlines, but ok I will try to do it this or next week, be patient.
Just help me decide where I should put the code with metaclassmaker? As db/migrations/metaclassmaker.py or you have some particular place for such internal tools? I can also try do it a part of render method or state module, though I think encapsulation is better way of doing it.

[Tim Graham]

I guess somewhere in django.utils would probably make sense. I'll tentatively accept the ticket for now.

[kosz85]

Added pull request

[kosz85]

​https://github.com/django/django/pull/4968

[Markus Holtermann]

I'm not really happy with bundling metaclassmaker either. Furthermore, I think there are a few problems with your current implementation:

1. What happens if a model's metaclass changes from MyModelBase to Django's ModelBase or vice versa?
2. I don't see an explicit tracking of the class in generated migration files, do I?
3. As the metaclass has to be tracked inside migrations (only which metaclass, what the metaclass does) but is only referenced from the regular code base, can we find a solution like for the model managers?
4. Keep in mind, that any fields a metaclass adds to a model are automatically serialized (see e.g. abstract models).

[kosz85]

Yeah, It's quite hard topic, but your current solution just doesn't work with code that work on South, and you have to take into account, that there may be more then 1 metaclass. Let's discuss and think on some solution but first one thing:
All your points are occurring also in current version. My fix is only dealing with multiple inheritance of metaclasses. Currently you also can have custom metaclass and you can change it without any note at migrations, the only thing is that current migrations support only 1 such metaclass now, and break on multiple inheritance.

1. I don't understand your question :) It would be changed, and derived class would be changed. It's rather unlikely that you will be changing ModelBase itself, as it's more like you want to just add some metaclass functionality. The new metaclass will have both functionalities from ModelBase and your custom metaclass. If I understand correctly what you would like to do, so to change the custom MyModelBase with ModelBase it probably would work in most case, but you will need to modify base classes to be able to create class object. In my opinion this solution would be even scarier.

2. No I didn't do anything like that. But do we really need it? Currently BaseModel is also not listed, and if I will add normal metaclass with single not multiple inheritance it will work the same way as it's now without any notes that there is different metaclass. But of course it's probably possible to list metaclasses, but I don't really think it's needed. The change in fileds will be noticed, options are still part of BaseModel, custom metacalss doesn't change it. In most cases custom metaclasses are for some tricks, like global registry, singletons, class definitions validators, and they are not used for dealing with models itself. That in my case, maybe I'm wrong, but I don't see much more danger here comparing to current solution.

3. It's possible to list metaclasses, the code that is constructing metaclass is doing it in fact it's just map(type, bases) and you can go with it like you will do with normal bases, as metaclass is also a class, but with base of type. So it's possible to reuse same code for construction bases list. Thought managers are quite different being.

4. Yes, and I don't see any problem with it. Thought as I said before, I don't think it's this kind of metaclass :) Normally I would use 2 different mixins for custom metaclass and abstract Model with fields, as it would be cleaner and easier to maintain. Also that way I can reuse metaclass in many models. But of course you can also try to inherit your metaclass from ModelBase and made it custom, but you can do it even now and you have to deal with same problems you listed, but without error I reported. My fix solves only multiple inheritance problem, with witch ModelState couldn't deal with.

[Tim Graham]

Marking as "Patch needs improvement" since the current pull request ​results in a performance regression.

[kosz85]

I upgraded it, now it shouldn't be much slower for normal cases, but will be a bit slower for Models that use metaclasses. Please check again, I wait for this to be merged, I don't like idea to support own django fork :)

[Markus Holtermann]

Patch looks good to me.

[Tim Graham]

Patch still scares me and could be motivated with documentation about the use cases I think.

[kosz85]

That's not smth to be scared of. I can explain what is happening there:

Each class has metaclass, which default is type. It's class of class. You can change it and made your own type declaring metaclass. What new migrations did, is bad assumption that class would be created only using type, and that there would be only simple conflicts.
First take a note that Model metaclass is already set to BaseModel.

Assume such situation:
class cA with metaclass mA
class cB with metaclass mB
(Finaly your model)
class cC(cA, cB) with metaclass mC

Python has no problem with standard metaclasses.
When mA and mB eguals default type then python knows that mC is also type.
Also when mA or mB equals default type and the other not (for example BaseModel) then he can easily solve conflict because every metaclass is derived from type so Python is using the other one so mC would be BaseModel.
But when mA is custom and mB is custom, he don't know what to do, and there are even worse scenarios, there may be 3 or more bases to inherit from. Then standard response is to manually build mC that inherit from mA and mB (like in normal class inheritance).

So what is doing this patch is reacting to that TypeError, and constructing (it's normal factory) such custom metaclass from metaclasses that are used in bases of this class. That new metaclass is used then to create object, instead of standard type. So no magic ;) The only magic is simple factory that populates metaclasses from bases.

[Mike Amy]

Also encountered the same issues.

I've created a lighter pull request that fixes it for me and makes the proposed test pass. It doesn't require metaclassmaker.

(Django 1.10) ​https://github.com/django/django/pull/5907
(Django 1.9) ​https://github.com/django/django/pull/5908

[kosz85]

Thats the same what metaclassmaker was doing ;) you just did it inline without naming function, great that it's fixed. Thx for your effort :)

[Loic Bistuer]

I don't think this is the right approach.

The current status of inheritance in migrations:

• Inheritance is only partially supported (abstract models are ignored) in CreateModel() with the bases kwarg.
• Inheritance changes aren't tracked over time.
• Metaclasses aren't supported.

With the exception of abstract models (ignoring them was a deliberate design decision by Andrew), we could cleanly fix all the other shortcomings by:

• Adding a metaclass=ModelBase kwarg to CreateModel().
• Adding a AlterModelClass(bases, metaclass) operation.

Doing so would bring us closer to standard Python inheritance, support changes over time, and replace the proposed runtime magic with a declarative syntax.

[Yingtong Li]

It looks like it's been a while since there was any movement in this ticket, but I've just encountered this issue myself (Django 1.10.4). kosz85's patches were not sufficient for my use case, as my metaclass influenced the database structure of the Model, and so needed to be used rather than merely emulated.

I have put together some quick changes along the lines of Loic Bistuer's suggestions, which are sufficient for my needs:

​https://github.com/django/django/compare/master...RunasSudo:migrations-with-metaclasses?expand=1

However, more work would be required to implement the rest of the functionality and fully complete the integration of the changes with the rest of Django, and I do not have the confidence in my own ability to do so. Hopefully these changes may form the foundation for further changes if anyone plans further work on this ticket.

[kosz85]

It would be nice to have it integrated in django. I'm using custom builds for latest LTS adding this bugfix.

[Ethan J. Howell]

Building off of @Yingtong Li's PR I submitted a new one that should solve everything: ​https://github.com/django/django/pull/13384.

Although it would be nice to see if there's a short-term solution anyone else has come up with.

[Mariusz Felisiak]

The current solution doesn't track metaclass changes, see ​comment.

[Jacob Walls]

This is probably worth another look. Even though changing metaclass bases don't generate migrations, that seems like another flavor of #23521, no? (That ticket seems to be a catch-all for "can't change my parent classes").

[Jacob Walls]

Of course, feel free to set the flags back if this really shouldn't move forward without tracking metaclass changes.

[Mariusz Felisiak]

IMO, we shouldn't move forward without tracking metaclass changes, see comment.

[Jacob Walls]

Yeah, that makes sense safety-wise. I guess it's different enough from #23521 in that there's more of an expectation here that metaclasses can/will change.