[Django] #33414: Diamond inheritance causes duplicated PK error when creating an object, if the primary key field has a default.
Django
the primary key field has a default.
-----------------------------------------+------------------------
Reporter: Yu Li | Owner: nobody
Type: Bug | Status: new
Component: Uncategorized | Version: 4.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Hi, I'm not sure if this is a bug or an unsupported feature. But I looked
into the django/db/models/base.py source code and now have a pretty good
understanding of what is happening.
My business code uses a diamond shape inheritance to model different types
of user posts: UserPost, ImagePost, VideoPost, and MixedContentPost. The
inheritance goes like this: both ImagePost and VideoPost extend from
UserPost, and the MixedContentPost inherits from ImagePost and VideoPost.
All of them are concrete models
I read the doc and expected it to work, similar to the example
{{{
class Piece(models.Model):
pass
class Article(Piece):
article_piece = models.OneToOneField(Piece, on_delete=models.CASCADE,
parent_link=True)
...
class Book(Piece):
book_piece = models.OneToOneField(Piece, on_delete=models.CASCADE,
parent_link=True)
...
class BookReview(Book, Article):
pass
}}}
However, I found out that the doc's example only works when these models
use a primary key field that does not have a default. In my case, we are
using a UUIDField as the primary key with a default of uuid4. Trying to
create a BookReview in our case, causes a django.db.utils.IntegrityError:
UNIQUE constraint failed error, because django tries to create the Piece
object twice, with the same uuid.
The same behavior is found if I used a AutoField on Piece, with a custom
default function, such as
{{{
id = 99
def increment():
global id
id += 1
return id
}}}
This default function makes no sense in practice, but I just use it here
to illustrate the root cause of the problem:
The _save_table method in db/models/base.py has a like this:
{{{
# Skip an UPDATE when adding an instance and primary key has a
default.
if (
not raw
and not force_insert
and self._state.adding
and meta.pk.default
and meta.pk.default is not NOT_PROVIDED
):
force_insert = True
}}}
When a default is not present, which is the case of the documentation
example, Django will first insert the first instance of the Piece object,
and then for the second one, since force_insert is False, _save_table
tries an update and goes through. Therefore there is not duplicate.
However, if a default is present, then the second Piece becomes an
insertion as well (because meta.pk.default and meta.pk.default is not
NOT_PROVIDED, force_insert is True). This causes a duplication on the
primary key.
On the other hand, _insert_parent does an in-order traversal calling
_save_table on each node, so even in the no-default pk case, it is calling
a redundant update on the root node after the insertion..
So which function is at fault?
The source code _save_table assumes that if you are calling it with a
default pk then you can skip an update. This assumption looks weird to me:
why only when there IS a default pk you can skip update? Why not just skip
update as long as we know we are inserting? (via self._state.adding) Is it
just to make it special so that AutoField works? If _save_table's
responsibility is to try updating before inserting, except when the params
force it to do an update or insert, then it shouldn't override that
behavior by this self-assumeption within it.
I think the solution is to simply move the check to save_base. And don't
do this check in _save_parents.
Like this:
{{{
def save_base(
self,
raw=False,
force_insert=False,
force_update=False,
using=None,
update_fields=None,
):
"""
Handle the parts of saving which should be done only once per
save,
yet need to be done in raw saves, too. This includes some sanity
checks and signal sending.
The 'raw' argument is telling save_base not to save any parent
models and not to do any changes to the values before save. This
is used by fixture loading.
"""
using = using or router.db_for_write(self.__class__,
instance=self)
assert not (force_insert and (force_update or update_fields))
assert update_fields is None or update_fields
cls = origin = self.__class__
# Skip proxies, but keep the origin as the proxy model.
if cls._meta.proxy:
cls = cls._meta.concrete_model
meta = cls._meta
if not meta.auto_created:
pre_save.send(
sender=origin,
instance=self,
raw=raw,
using=using,
update_fields=update_fields,
)
# A transaction isn't needed if one query is issued.
if meta.parents:
context_manager = transaction.atomic(using=using,
savepoint=False)
else:
context_manager =
transaction.mark_for_rollback_on_error(using=using)
with context_manager:
parent_inserted = False
if not raw:
parent_inserted = self._save_parents(cls, using,
update_fields)
# Skip an UPDATE when adding an instance and primary key has a
default.
if (
not raw
and not force_insert
and self._state.adding
and meta.pk.default
and meta.pk.default is not NOT_PROVIDED
):
force_insert = True
updated = self._save_table(
raw,
cls,
force_insert or parent_inserted,
force_update,
using,
update_fields,
)
# Store the database on which the object was saved
self._state.db = using
# Once saved, this is no longer a to-be-added instance.
self._state.adding = False
# Signal that the save is complete
if not meta.auto_created:
post_save.send(
sender=origin,
instance=self,
created=(not updated),
update_fields=update_fields,
raw=raw,
using=using,
)
save_base.alters_data = True
}}}
I have never contributed to Django before. If you think I'm right on this
one I'll look into creating a PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/33414>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
the primary key field has a default.
Reporter: Yu Li | Owner: Yu Li
Type: Bug | Status: assigned
Component: Uncategorized | Version: 4.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Yu Li):
* owner: nobody => Yu Li
* status: new => assigned
Comment:
Is this project completely community supported? Is there a project lead I
can discuss with to confirm my suspicion? Thank you.
See this patch for my changes:
https://github.com/kaleido-
public/django/commit/70c00ccff35f039705eb0a748939d4c3d6dbe93a
--
Ticket URL: <https://code.djangoproject.com/ticket/33414#comment:1>
Django
the primary key field has a default.
Reporter: Yu Li | Owner: Yu Li
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* component: Uncategorized => Database layer (models, ORM)
Comment:
You can create a pull request and see if any tests break.
--
Ticket URL: <https://code.djangoproject.com/ticket/33414#comment:2>
Django
the primary key field has a default.
Reporter: Yu Li | Owner: Yu Li
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Simon Charette (added)
* stage: Unreviewed => Accepted
Comment:
Thanks for the report. I was able to reproduce this issue.
> This assumption looks weird to me: why only when there IS a default pk
you can skip update?
This is an optimization to skip `UPDATE` when inherited primary key has a
default (see babd4126853e48594b61e8db71a83d7bdd929b9c and #29129).
> Why not just skip update as long as we know we are inserting? (via
self._state.adding)
As far as I'm aware, this would be contrary to the currently
[https://docs.djangoproject.com/en/4.0/ref/models/instances/#how-django-
knows-to-update-vs-insert documented] process.
> I think the solution is to simply move the check to save_base. And don't
do this check in _save_parents.
This breaks optimization added in
babd4126853e48594b61e8db71a83d7bdd929b9c, see
`basic.tests.ModelInstanceCreationTests.test_save_parent_primary_with_default`.
> Is this project completely community supported?
Yes.
> Is there a project lead I can discuss with to confirm my suspicion?
Thank you.
No, we don't have a project lead. You can join the DevelopersMailingList
and share your ideas. You can also interact on the
[https://forum.djangoproject.com/ Django forum] and the #django-dev IRC
channel.
--
Ticket URL: <https://code.djangoproject.com/ticket/33414#comment:3>
Django
the primary key field has a default.
Reporter: Yu Li | Owner: Yu Li
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Abhijeet Viswa (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33414#comment:4>
Django
the primary key field has a default.
Reporter: Yu Li | Owner: Yu Li
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
Thank you for your detailed report.
I haven't looked into the details of how this can be addressed but one
thing is certain. When an instance of model class at the ''head'' of a
diamond inheritance graph is saved the ORM knows that there isn't a reason
to update involved tables/nodes more than once. In other words, creating a
`BookReview` should only require 4 queries and not 6 and this issue
predates babd4126853e48594b61e8db71a83d7bdd929b9c. I believe that's what
we should focus on solving here as that will address this unfortunate
collision while reducing the number of queries to a minimum.
Test coverage for model diamond inheritance is limited given it's a rarely
used feature and ''good'' core support for fields with Python generated
defaults is also relatively recent so I'm not surprised we missed this
relatively obscure edge case in #29129.
--
Ticket URL: <https://code.djangoproject.com/ticket/33414#comment:5>
Django
the primary key field has a default.
Reporter: Yu Li | Owner: Yu Li
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
After a quick investigation it seems like we simply don't have any tests
covering diamond inheritance model saves.
The only instance of diamond inheritance I could find in the test suite
was
[https://github.com/django/django/blob/652c68ffeebd510a6f59e1b56b3e007d07683ad8/tests/model_meta/models.py#L144-L158
in model_meta tests] but this hierarchy of models exists solely for
`Option` testing purposes.
Here's what a tentative patch could look like for this issue, it seems to
be passing the full suite on SQLite at least.
{{{#!diff
diff --git a/django/db/models/base.py b/django/db/models/base.py
index 37f6a3dd58..d363a1ddeb 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -823,21 +823,29 @@ def save_base(self, raw=False, force_insert=False,
save_base.alters_data = True
- def _save_parents(self, cls, using, update_fields):
+ def _save_parents(self, cls, using, update_fields,
saved_parents=None):
"""Save all the parents of cls using values from self."""
meta = cls._meta
inserted = False
+ if saved_parents is None:
+ saved_parents = {}
for parent, field in meta.parents.items():
# Make sure the link fields are synced between parent and
self.
if (field and getattr(self, parent._meta.pk.attname) is None
and
getattr(self, field.attname) is not None):
setattr(self, parent._meta.pk.attname, getattr(self,
field.attname))
- parent_inserted = self._save_parents(cls=parent, using=using,
update_fields=update_fields)
- updated = self._save_table(
- cls=parent, using=using, update_fields=update_fields,
- force_insert=parent_inserted,
- )
- if not updated:
+ if (parent_updated := saved_parents.get(parent)) is None:
+ parent_inserted = self._save_parents(
+ cls=parent, using=using, update_fields=update_fields,
saved_parents=saved_parents,
+ )
+ updated = self._save_table(
+ cls=parent, using=using, update_fields=update_fields,
+ force_insert=parent_inserted,
+ )
+ if not updated:
+ inserted = True
+ saved_parents[parent] = updated
+ elif not parent_updated:
inserted = True
# Set the parent's PK value to self.
if field:
diff --git a/tests/model_inheritance/models.py
b/tests/model_inheritance/models.py
index fa37e121ea..99ce78a0f0 100644
--- a/tests/model_inheritance/models.py
+++ b/tests/model_inheritance/models.py
@@ -175,3 +175,20 @@ class Child(Parent):
class GrandChild(Child):
pass
+
+
+# Diamond inheritance
+class CommonAncestor(models.Model):
+ pass
+
+
+class FirstParent(CommonAncestor):
+ first_ancestor = models.OneToOneField(CommonAncestor, models.CASCADE,
primary_key=True, parent_link=True)
+
+
+class SecondParent(CommonAncestor):
+ second_ancestor = models.OneToOneField(CommonAncestor,
models.CASCADE, primary_key=True, parent_link=True)
+
+
+class CommonChild(FirstParent, SecondParent):
+ pass
diff --git a/tests/model_inheritance/tests.py
b/tests/model_inheritance/tests.py
index 550a297fb3..97fb3e4a78 100644
--- a/tests/model_inheritance/tests.py
+++ b/tests/model_inheritance/tests.py
@@ -7,7 +7,7 @@
from django.test.utils import CaptureQueriesContext, isolate_apps
from .models import (
- Base, Chef, CommonInfo, GrandChild, GrandParent, ItalianRestaurant,
+ Base, Chef, CommonInfo, CommonChild, GrandChild, GrandParent,
ItalianRestaurant,
MixinModel, Parent, ParkingLot, Place, Post, Restaurant, Student,
SubBase,
Supplier, Title, Worker,
)
@@ -150,6 +150,12 @@ def b():
sql = query['sql']
self.assertIn('INSERT INTO', sql, sql)
+ def test_create_diamond_mti(self):
+ with self.assertNumQueries(4):
+ common_child = CommonChild.objects.create()
+ with self.assertNumQueries(4):
+ common_child.save()
+
def test_eq(self):
# Equality doesn't transfer in multitable inheritance.
self.assertNotEqual(Place(id=1), Restaurant(id=1))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33414#comment:6>
Django
the primary key field has a default.
Reporter: Yu Li | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Yu Li => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/33414#comment:7>
Django
the primary key field has a default.
Reporter: Yu Li | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Akash Kumar Sen):
* cc: Akash Kumar Sen (added)
* owner: (none) => Akash Kumar Sen
* has_patch: 0 => 1
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33414#comment:8>
Django
the primary key field has a default.
Reporter: Yu Li | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Bhuvnesh):
[https://github.com/django/django/pull/16903 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33414#comment:9>
Django
the primary key field has a default.
Reporter: Yu Li | Owner: Akash
| Kumar Sen
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33414#comment:10>
Django
the primary key field has a default.
Reporter: Yu Li | Owner: Akash
| Kumar Sen
Component: Database layer | Version: 4.0
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"5d20e0207867615f6989714261ea3c0a3eb85d88" 5d20e02]:
{{{
#!CommitTicketReference repository=""
revision="5d20e0207867615f6989714261ea3c0a3eb85d88"
Fixed #33414 -- Fixed creating diamond-shaped MTI objects for common
ancestor with primary key that has a default.
Co-authored-by: Simon Charette <chare...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33414#comment:11>