Opened 4 years ago

Closed 6 months ago

#16733 closed Bug (duplicate)

pk/id bug in multiple inheritance of concrete classes

Reported by: Leo 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 4 years ago by julien

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to needsinfo
  • Status changed from new to closed

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 4 years ago by Leo

  • Resolution needsinfo deleted
  • Status changed from closed to reopened

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 4 years ago by Leo (previous) (diff)

comment:3 Changed 4 years ago by Leo

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 4 years ago by akaariai

  • Cc anssi.kaariainen@… added
  • Triage Stage changed from Unreviewed to Accepted

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 4 years ago by Leo

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 4 years ago by akaariai

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 2 years ago by aaugustin

  • Status changed from reopened to new

comment:8 Changed 6 months ago by timgraham

  • Resolution set to duplicate
  • Status changed from new to closed

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

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