Opened 5 years ago

Closed 2 years ago

#16733 closed Bug (duplicate)

pk/id bug in multiple inheritance of concrete classes

Reported by: Leo Shklovskii Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: anssi.kaariainen@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (8)

comment:1 Changed 5 years ago by Julien Phalip

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: needsinfo
Status: newclosed

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

comment:2 Changed 5 years ago by Leo Shklovskii

Resolution: needsinfo
Status: closedreopened

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__
{'_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.

Last edited 5 years ago by Leo Shklovskii (previous) (diff)

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

comment:4 Changed 5 years ago by Anssi Kääriäinen

Cc: anssi.kaariainen@… added
Triage Stage: UnreviewedAccepted

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?

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

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

comment:7 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:8 Changed 2 years ago by Tim Graham

Resolution: duplicate
Status: newclosed

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

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