Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#22442 closed Bug (fixed)

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

Reported by: benjaoming Owned by: Tim Graham
Component: Documentation Version: 1.7-beta-1
Severity: Release blocker Keywords: multiple model inheritance
Cc: Tim Graham, Anssi Kääriäinen Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


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:

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

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

Change History (13)

comment:1 Changed 8 years ago by Baptiste Mispelon

Triage Stage: UnreviewedAccepted
Type: UncategorizedBug


I can indeed reproduce this issue with the following models:

from django.db import models

class Foo(models.Model):

class Bar(models.Model):

class Baz(Foo, Bar):

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>]


comment:2 Changed 8 years ago by 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.

comment:3 Changed 8 years ago by 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.

comment:4 Changed 8 years ago by 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.

comment:5 Changed 8 years ago by 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).

comment:6 Changed 8 years ago by loic84


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

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

Or use a common ancestor to hold the AutoField:

class Piece(models.Model):

class Article(Piece):
    # ...

class Book(Piece):
    # ...

class BookReview(Book, Article):

comment:7 Changed 8 years ago by 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.

comment:8 Changed 8 years ago by Tim Graham

Component: Core (System checks)Documentation

comment:9 Changed 8 years ago by Tim Graham

Owner: changed from nobody to Tim Graham
Status: newassigned

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

comment:10 Changed 8 years ago by Tim Graham

Has patch: set

comment:11 Changed 8 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 8ec388a69da13a8a5cf82604a26abe13be7dd1cb:

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

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

comment:12 Changed 8 years ago by 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

comment:13 Changed 8 years ago by 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.

Note: See TracTickets for help on using tickets.
Back to Top