Opened 13 years ago
Closed 10 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: | dev |
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 by , 13 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
comment:2 by , 13 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → 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.
comment:3 by , 13 years ago
comment:4 by , 13 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → 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 by , 13 years ago
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 by , 13 years ago
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 by , 12 years ago
Status: | reopened → new |
---|
comment:8 by , 10 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
Validation logic to prevent shadowing model fields was added in #17673.
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: