#36259: `_is_pk_set()` is incorrectly used to check for unsaved models.
-------------------------------------+-------------------------------------
Reporter: Clifford Gama | Owner: Clifford
| Gama
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: _is_pk_set | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Clifford Gama:
Old description:
> Since models defined with custom primary keys can have `pk` set before
> being saved to the database, this means they bypass this as if they were
> saved. Here is an example:
> {{{#!diff
> diff --git a/tests/one_to_one/tests.py b/tests/one_to_one/tests.py
> index 451e97c274..67b1c32e90 100644
> --- a/tests/one_to_one/tests.py
> +++ b/tests/one_to_one/tests.py
> @@ -170,6 +170,16 @@ class OneToOneTests(TestCase):
> with self.assertRaises(Restaurant.DoesNotExist):
> place.restaurant
>
> + def test_unsaved_object_with_supplied_primary_key(self):
> + link1 = Place.objects.create(name="Ushaka", address="Ethekwini")
> + link2 = ManualPrimaryKey(primary_key="YTREWQ", name="moonwalk")
> + msg = (
> + "save() prohibited to prevent data loss due to unsaved
> related object "
> + "'link2'."
> + )
> + with self.assertRaisesMessage(ValueError, msg):
> + MultiModel.objects.create(link1=link1, link2=link2,
> name="Unsavable")
> +
> def test_reverse_relationship_cache_cascade(self):
> """
> Regression test for #9023: accessing the reverse relationship
> shouldn't
> }}}
>
> which results in
> {{{#!python
> FE
> ======================================================================
> ERROR: test_unsaved_object_with_supplied_primary_key
> (one_to_one.tests.OneToOneTests.test_unsaved_object_with_supplied_primary_key)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/home/clifford/code/django/django/db/backends/sqlite3/base.py",
> line 304, in check_constraints
> raise IntegrityError(
> django.db.utils.IntegrityError: The row in table 'one_to_one_multimodel'
> with primary key '1' has an invalid foreign key:
> one_to_one_multimodel.link2_id contains a value 'YTREWQ' that does not
> have a corresponding value in one_to_one_manualprimarykey.primary_key.
>
> ======================================================================
> FAIL: test_unsaved_object_with_supplied_primary_key
> (one_to_one.tests.OneToOneTests.test_unsaved_object_with_supplied_primary_key)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/home/clifford/code/django/tests/one_to_one/tests.py", line 180,
> in test_unsaved_object_with_supplied_primary_key
> with self.assertRaisesMessage(ValueError, msg):
> File "/usr/lib/python3.12/contextlib.py", line 144, in __exit__
> next(self.gen)
> AssertionError: ValueError not raised
>
> ----------------------------------------------------------------------
> Ran 1 test in 0.004s
>
> FAILED (failures=1, errors=1)
> }}}
> And here is the culprit
> [https://github.com/django/django/blob/ef6a83789b310a441237a190a493c9586a4cb260/django/db/models/base.py#L1220C17-L1224C22
> code]:
> This is done in a few other places. My proposal is to replace such uses
> cases of `_is_pk_set` with `instance._state.added` checks.
New description:
Since models defined with custom primary keys can have `pk` set before
being saved to the database, this means they bypass this as if they were
saved. Here is an example:
{{{#!diff
diff --git a/tests/one_to_one/tests.py b/tests/one_to_one/tests.py
index 451e97c274..67b1c32e90 100644
--- a/tests/one_to_one/tests.py
+++ b/tests/one_to_one/tests.py
@@ -170,6 +170,16 @@ class OneToOneTests(TestCase):
with self.assertRaises(Restaurant.DoesNotExist):
place.restaurant
+ def test_unsaved_object_with_supplied_primary_key(self):
+ link1 = Place.objects.create(name="Ushaka", address="Ethekwini")
+ link2 = ManualPrimaryKey(primary_key="YTREWQ", name="moonwalk")
+ msg = (
+ "save() prohibited to prevent data loss due to unsaved
related object "
+ "'link2'."
+ )
+ with self.assertRaisesMessage(ValueError, msg):
+ MultiModel.objects.create(link1=link1, link2=link2,
name="Unsavable")
+
def test_reverse_relationship_cache_cascade(self):
"""
Regression test for #9023: accessing the reverse relationship
shouldn't
}}}
which results in
{{{#!python
FE
======================================================================
ERROR: test_unsaved_object_with_supplied_primary_key
(one_to_one.tests.OneToOneTests.test_unsaved_object_with_supplied_primary_key)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/clifford/code/django/django/db/backends/sqlite3/base.py",
line 304, in check_constraints
raise IntegrityError(
django.db.utils.IntegrityError: The row in table 'one_to_one_multimodel'
with primary key '1' has an invalid foreign key:
one_to_one_multimodel.link2_id contains a value 'YTREWQ' that does not
have a corresponding value in one_to_one_manualprimarykey.primary_key.
======================================================================
FAIL: test_unsaved_object_with_supplied_primary_key
(one_to_one.tests.OneToOneTests.test_unsaved_object_with_supplied_primary_key)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/clifford/code/django/tests/one_to_one/tests.py", line 180,
in test_unsaved_object_with_supplied_primary_key
with self.assertRaisesMessage(ValueError, msg):
File "/usr/lib/python3.12/contextlib.py", line 144, in __exit__
next(self.gen)
AssertionError: ValueError not raised
----------------------------------------------------------------------
Ran 1 test in 0.004s
FAILED (failures=1, errors=1)
}}}
And here is the culprit
[https://github.com/django/django/blob/ef6a83789b310a441237a190a493c9586a4cb260/django/db/models/base.py#L1220C17-L1224C22
code]:
This is done in a few other places. My proposal is to replace such uses
cases of `_is_pk_set` with `instance._state.adding` checks.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/36259#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/010701959dd4c079-b65b2359-2480-4fd2-967e-076d42feb558-000000%40eu-central-1.amazonses.com.
