app1.MyModel: (models.E005) The field 'id' from parent model 'app2.othermodel' clashes with the field 'id' from parent model 'app3.othermodel'

[benjaoming]

The system check framework borks on multiple inheritance due to the 'id' field being the same on two different models, even though that field is IMO not relevant to the child class.

See this issue on django-wiki:

​https://github.com/benjaoming/django-wiki/issues/255

ERRORS:
wiki.ArticleSubscription: (models.E005) The field 'id' from parent model 'wiki.articleplugin' clashes with the field 'id' from parent model 'django_notify.subscription'.

Basically, we have app1 creating a model with multiple inheritance from models in app2 and app3. Thus, model fields cannot be renamed from app1 since it has nothing to do with the code base of app2 and app3.

I think it's a simple case of leaving the 'id' field out of the check since it's not inherited to the child model, anyways.

The above model looks like this:

class ArticleSubscription(ArticlePlugin, Subscription):
    pass

The SQL for creating this (django 1.6 and 1.7)

CREATE TABLE "wiki_articlesubscription" (
    "subscription_ptr_id" integer NOT NULL UNIQUE REFERENCES "notify_subscription" ("id"),
    "articleplugin_ptr_id" integer NOT NULL PRIMARY KEY REFERENCES "wiki_articleplugin" ("id")
)

The new system check was introduced through #17673 and commit ee9fcb1672ddf5910ed8c45c37a00f32ebbe2bb1:

I would suggest skipping the check on 'id' in order to reflect what can be interpreted in the docs and the SQL. In the docs, we can read:

Just as with Python’s subclassing, it’s possible for a Django model to inherit from multiple parent models. Keep in mind that normal Python name resolution rules apply. The first base class that a particular name (e.g. Meta) appears in will be the one that is used; for example, this means that if multiple parents contain a Meta class, only the first one is going to be used, and all others will be ignored.

...and in the SQL, we see that the PK of the first model is chosen as primary key. Thus, if the child model retains an id field proxy that references the first models PK (in this case, articleplugin_ptr_id), everything is fine! And in the docs, we can add that inheriting from a non-abstract model will always reference the id field of the child to the first parent models PK (as with the Meta class!).

[Baptiste Mispelon]

Hi,

I can indeed reproduce this issue with the following models:

from django.db import models

class Foo(models.Model):
    pass


class Bar(models.Model):
    pass


class Baz(Foo, Bar):
    pass

These models pass the validation in 1.6 but do fail in 1.7 so I think the release blocker flag is warranted.

Not sure what the best fix for this would be, but it's interesting to note that (at least in 1.6), there does appear to be a duplicated id field:

>>> Baz._meta.fields
[<django.db.models.fields.AutoField: id>, <django.db.models.fields.AutoField: id>, <django.db.models.fields.related.OneToOneField: bar_ptr>, <django.db.models.fields.related.OneToOneField: foo_ptr>]

Thanks!

[Tim Graham]

If you try something like Baz.objects.create(bar_ptr_id=N), you get a object with bar_ptr_id=X and foo_ptr_id=X (where X is the next primary key value of Foo) regardless of what you specify as "N", so I guess maybe akaaria's comment from #17673, "The biggest problem is this: assume a StudentWorker has Student with id = 1 and Worker with id = 2 as parents.", cannot happen. Seems like the solution to maintain backwards compatibility would be to have the ID of the child model use the id of the first model. Loic is investing this further.

[loic84]

Ideally we would special case autocreated AutoField, both at save and at query time, since these aren't technically needed for a MTI subclass to operate, but that's far from trivial and definitely not something we can do this late in the 1.7 cycle.

I suggest we make this a backward incompatible change (documented as such), there was a fair risk of data corruption with the previous situation.

[Tim Graham]

I think if you can make a convincing case there is a risk of data corruption, documenting this could be sufficient.

One size may not fit all, but if we can also recommend a way to migrate away from doing this, that would also be great.

Here's the ​ArticleSubscription mentioned above.

[benjaoming]

loic84, that would also mean to suggest a suitable workaround. For instance: Apps with multiple inheritance of non-abstract classes have to switch to single-inheritance + OneToOneFields?

Also, it would mean that the Release Notes have to state "Django has temporarily dropped support for multiple inheritance of non-abstract classes".. a bit of a draw-back.

Can the temporary fix be to alter the "check" framework to ignore this kind of case and thus keep things running like before? (I'm not familiar with the implementation and the exact consequence of what you mentioned about the AutoField).

[loic84]

Example:

class Article(models.Model):
    headline = models.CharField(max_length=50)
    body = models.TextField()

class Book(models.Model):
    title = models.CharField(max_length=50)

class BookReview(Book, Article):
    pass

>>> article = Article.objects.create(headline='Some piece of news.')
>>> 
>>> assert Article.objects.get(pk=article.pk).headline == article.headline
>>> 
>>> review = BookReview.objects.create(headline='Review of Little Red Riding Hood.', title='Little Red Riding Hood')
>>> 
>>> assert Article.objects.get(pk=article.pk).headline == article.headline
Traceback (most recent call last):
  File "<console>", line 1, in <module>
AssertionError
>>> assert Article.objects.get(pk=article.pk).headline == review.headline
>>> 

One workaround is to use explicit AutoField in the base models:

class Article(models.Model):
    article_id = models.AutoField(primary_key=True)
    # ...

class Book(models.Model):
    book_id = models.AutoField(primary_key=True)
    # ...

class BookReview(Book, Article):
    pass

Or use a common ancestor to hold the AutoField:

class Piece(models.Model):
    pass

class Article(Piece):
    # ...

class Book(Piece):
    # ...

class BookReview(Book, Article):
    pass

[loic84]

@benjaoming there is still support for "multiple inheritance of non-abstract classes", just not with clashing fields. The lack of warning didn't imply support; the code makes it obvious that this was never an intended behavior and while it's definitely possible to rework it to support this, it's really not easy.

[Tim Graham]

I will convert the info that Loic has provided into a documentation patch.

[Tim Graham]

​PR

[Tim Graham <timograham@…>]

In 8ec388a69da13a8a5cf82604a26abe13be7dd1cb:

Fixed #22442 -- Provided additional documentation regarding id fields clashing.

Thanks benjaoming for raising the issue and Loic for the examples.

[Tim Graham <timograham@…>]

In 6d4df45e2927c45344b9f014f739466d61f808c8:

[1.7.x] Fixed #22442 -- Provided additional documentation regarding id fields clashing.

Thanks benjaoming for raising the issue and Loic for the examples.

Backport of 8ec388a69d from master

[benjaoming]

I just want to add that I think it's a good solution because it forces more explicitness about what the primary keys of a multiple inheritance model and its ancestors are. Well-thought and nicely written, Loic!

If you don't mind, I'll just post my comments in here, once I've completed a changeover in my current set of models, because it's a changeover that needs data to be migrated.