pk/id bug in multiple inheritance of concrete classes

[Leo Shklovskii]

If I have two concrete classes A, B and then have a class C(A,B) a C object's pk and id don't line up.

The c.pk is a.pk and c.id is b.id. This can create all sorts of problems and confusion since you can conventionally use pk and id interchangeably.

[Julien Phalip]

It's unclear to me what you mean by "c.pk is a.pk and c.id is b.id". Please reopen this ticket if you're able to provide a simple test case illustrating the issue you're facing.

FWIW, here's some sample code I've quickly tried and it worked for me:

from django.db import models

class A(models.Model):
    pass

class B(models.Model):
    pass

class C(A, B):
    pass

>>> c = C.objects.create()
>>> c.pk
1
>>> c.id
1

[Leo Shklovskii]

Sorry, I should have provided code. There's a couple of ways to demo this problem.

Here's what I actually ran into. If you have a c that starts out like this:

>>> c.__dict__
{'_state': <django.db.models.base.ModelState object at 0x05D1F550>,
 'a_ptr_id': 1,
 'b_ptr_id': 9,
 'id': 9}

You get this behavior:

>>> c.id
9
>>> c.pk
1

Doing a C.objects.get(pk=9) fails as does a C.objects.get(id=1) while their complements do work.

Another way to demo this issue from scratch is this data loss bug:

>>> b = B.objects.create()
>>> b.id
1L
>>> c = C.objects.create()
>>> c.__dict__
Out[6]:
{'_state': <django.db.models.base.ModelState object at 0x05A48490>,
 'a_ptr_id': 1L,
 'b_ptr_id': 1L,
 'id': 1L}

c should have a b_ptr_id of 2L since there's already a B with id of 1L. The B with id of 1L just got clobbered by the data in c.

[Leo Shklovskii]

This is similar to #15993 and #12002. Not only is this demonstrated data loss, it can also wreak very subtle havoc that may be very hard to detect.

Even if it's too significant of a fix to do in 1.3, this should be a glaring warning in the documentation.

[Anssi Kääriäinen]

If I am not mistaken, your models should not validate. C has two fields with the name of id. So, this is a model validation bug.

In general, if you are multi-inheriting then fields will be overwritten

class A(models.Model):
    f1 = models.TextField()
    class Meta:
        abstract = True

class B(models.Model):
    f1 = models.TextField()
    class Meta:
        abstract = True
class C(A, B):
    pass

And now C will have B's f1 field. I don't know if the above is a feature or a bug, but for concrete inheritance it is a bug (you can't access A's f1, even though it has a column in the DB). If I had to make the choice, I would prevent field overwriting when inheriting from abstract classes, too.

The real problem is overwrite of existing fields when multi-inheriting. Marking accepted.

Would it be a good idea to create another ticket about the real problem, and mark this and #12002 duplicates of the new ticket?

[Leo Shklovskii]

Thanks for looking at this Anssi. You bring up a good point but it's unclear that Django shouldn't support this. Python supports multiple inheritance and the underlying problem that we're hitting is that Django doesn't resolve .id properly. As per Python's MRO (and the way Meta works) C.id should be A.id, not B.id.

[Anssi Kääriäinen]

My point is that in multitable, multi-inheritance situations having two fields with the same name should not be allowed at all, no matter if C.id should be A.id or B.id.

The situation is different from normal multi-inheritance because at database level you have multiple tables, and because of that, you will have both A.id and B.id in C. At Python level you can't have that. That is the problem. And because of that, in multi-table multi inheritance field names should not clash.

You technically could access the fields through c.a_ptr.id and c.b_ptr.id but I still believe field overwriting in multi-table multi-inheritance should result in an error.

[Tim Graham]

Validation logic to prevent shadowing model fields was added in #17673.