Opened 9 years ago

Last modified 23 hours ago

#25068 assigned Bug

Metaclass conflict when doing createmigrations in ModelState.render

Reported by: kosz85 Owned by: Tom L.
Component: Migrations Version: dev
Severity: Normal Keywords: metaclass conflict createmigrations
Cc: Loic Bistuer, Markus Holtermann Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

metaconflicts.zip (7.2 KB ) - added by kosz85 9 years ago.
Metaconflicts isolated example - with six (tested on 2.7 & 3.4)

Download all attachments as: .zip

Change History (48)

comment:1 by Tim Graham, 9 years ago

Type: UncategorizedBug

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

comment:2 by kosz85, 9 years ago

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.

comment:3 by Tim Graham, 9 years ago

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?

comment:4 by kosz85, 9 years ago

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.

by kosz85, 9 years ago

Attachment: metaconflicts.zip added

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

comment:5 by kosz85, 9 years ago

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

comment:6 by Tim Graham, 9 years ago

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

comment:7 by kosz85, 9 years ago

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.

comment:8 by Tim Graham, 9 years ago

Severity: Release blockerNormal
Triage Stage: UnreviewedAccepted

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

comment:9 by kosz85, 9 years ago

Added pull request

comment:11 by Tim Graham, 9 years ago

Has patch: set

comment:12 by Markus Holtermann, 9 years ago

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

comment:13 by Tim Graham, 9 years ago

Patch needs improvement: set

comment:14 by kosz85, 9 years ago

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.
  1. 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.
  1. 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.
  1. 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.

comment:15 by kosz85, 9 years ago

Patch needs improvement: unset

comment:16 by Tim Graham, 9 years ago

Patch needs improvement: set

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

comment:17 by kosz85, 9 years ago

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

comment:18 by Tim Graham, 9 years ago

Patch needs improvement: unset

comment:19 by Markus Holtermann, 8 years ago

Triage Stage: AcceptedReady for checkin

Patch looks good to me.

comment:20 by Tim Graham, 8 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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

comment:21 by kosz85, 8 years ago

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.

Last edited 8 years ago by kosz85 (previous) (diff)

comment:22 by Mike Amy, 8 years ago

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

Last edited 8 years ago by Mike Amy (previous) (diff)

comment:23 by Tim Graham, 8 years ago

Patch needs improvement: unset

comment:24 by kosz85, 8 years ago

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

comment:25 by Loic Bistuer, 8 years ago

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.

comment:26 by Loic Bistuer, 8 years ago

Cc: Loic Bistuer added

comment:27 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:28 by Yingtong Li, 7 years ago

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.

comment:29 by kosz85, 7 years ago

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

comment:30 by Ethan J. Howell, 4 years ago

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.

comment:31 by Ethan J. Howell, 4 years ago

Owner: changed from nobody to Ethan J. Howell
Status: newassigned

comment:32 by Ethan J. Howell, 4 years ago

Patch needs improvement: unset
Version: 1.8master

comment:33 by Mariusz Felisiak, 4 years ago

Cc: Markus Holtermann added

comment:34 by Mariusz Felisiak, 4 years ago

Needs tests: set

comment:35 by Markus Holtermann, 4 years ago

Needs tests: unset
Triage Stage: AcceptedReady for checkin

comment:36 by Mariusz Felisiak, 4 years ago

Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

comment:37 by Ethan J. Howell, 3 years ago

Needs tests: unset
Patch needs improvement: unset

comment:38 by Mariusz Felisiak, 3 years ago

Patch needs improvement: set

comment:39 by Ethan J. Howell, 3 years ago

Patch needs improvement: unset

comment:40 by Mariusz Felisiak, 3 years ago

Needs tests: set
Patch needs improvement: set

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

comment:41 by Jacob Walls, 2 years ago

Patch needs improvement: unset

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").

comment:42 by Jacob Walls, 2 years ago

Needs tests: unset

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

comment:43 by Mariusz Felisiak, 2 years ago

IMO, we shouldn't move forward without tracking metaclass changes, see comment. We cannot assume that metaclass will never change.

Last edited 2 years ago by Mariusz Felisiak (previous) (diff)

comment:44 by Jacob Walls, 2 years ago

Needs tests: set
Patch needs improvement: set

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.

comment:45 by Tom L., 3 days ago

Owner: changed from Ethan J. Howell to Tom L.

comment:46 by Tom L., 3 days ago

Needs tests: unset
Patch needs improvement: unset

comment:47 by Mariusz Felisiak, 23 hours ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top