Opened 3 days ago
Last modified 34 hours ago
#36259 assigned Bug
Unsaved related object with primary_key=True field does not raise unsaved object error
Description (last modified by ) ¶
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:
-
TabularUnified tests/one_to_one/tests.py
diff --git a/tests/one_to_one/tests.py b/tests/one_to_one/tests.py index 451e97c274..67b1c32e90 100644
a b class OneToOneTests(TestCase): 170 170 with self.assertRaises(Restaurant.DoesNotExist): 171 171 place.restaurant 172 172 173 def test_unsaved_object_with_supplied_primary_key(self): 174 link1 = Place.objects.create(name="Ushaka", address="Ethekwini") 175 link2 = ManualPrimaryKey(primary_key="YTREWQ", name="moonwalk") 176 msg = ( 177 "save() prohibited to prevent data loss due to unsaved related object " 178 "'link2'." 179 ) 180 with self.assertRaisesMessage(ValueError, msg): 181 MultiModel.objects.create(link1=link1, link2=link2, name="Unsavable") 182 173 183 def test_reverse_relationship_cache_cascade(self): 174 184 """ 175 185 Regression test for #9023: accessing the reverse relationship shouldn't
which results in
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 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.
According to the ticket's flags, the next step(s) to move this issue forward are:
- For anyone except the patch author to review the patch using the patch review checklist and either mark the ticket as "Ready for checkin" if everything looks good, or leave comments for improvement and mark the ticket as "Patch needs improvement".
Change History (8)
comment:1 by , 3 days ago
Description: | modified (diff) |
---|
comment:2 by , 3 days ago
Has patch: | set |
---|
comment:4 by , 2 days ago
Take a look at the last comment: notice the ticket owner already submitted a pull request.
comment:5 by , 39 hours ago
Cc: | added |
---|---|
Keywords: | _is_pk_set removed |
Severity: | Release blocker → Normal |
Summary: | `_is_pk_set()` is incorrectly used to check for unsaved models. → Unsaved related object with primary_key=True field does not raise usaved object error |
Triage Stage: | Unreviewed → Accepted |
Hi Clifford, thank you for the report!
This is also failing on 5.1, so this is not a regression introduced by 5.2.
comment:6 by , 38 hours ago
Thank you, Sarah!
I should have mentioned in the report that this incorrect use of _is_pk_set()
affects also three other cases:
- Access of a generated field on an unsaved instance with a specified primary key.
-
TabularUnified tests/model_fields/test_generatedfield.py
diff --git a/tests/model_fields/test_generatedfield.py b/tests/model_fields/test_generatedfield.py index e00a665ec8..ab80536e38 100644
a b class GeneratedFieldTestMixin: 180 180 181 181 def test_unsaved_error(self): 182 182 m = self.base_model(a=1, b=2) 183 manual_pk_obj = GeneratedModelNonAutoPk(id=101, a="a") 183 184 msg = "Cannot read a generated field from an unsaved model." 184 185 with self.assertRaisesMessage(AttributeError, msg): 185 186 m.field 187 with self.assertRaisesMessage(AttributeError, msg): # Raises integrity error instead 188 manual_pk_obj.b 186 189 187 190 def test_full_clean(self): 188 191 m = self.base_model(a=1, b=2)
-
QuerySet.contains(unsaved_with_manual_pk)
raises integrity error.-
TabularUnified tests/queries/test_contains.py
diff --git a/tests/queries/test_contains.py b/tests/queries/test_contains.py index 2aa4badc72..aef3b475f9 100644
a b 1 1 from django.test import TestCase 2 2 3 from .models import D umbCategory, NamedCategory, ProxyCategory3 from .models import DateTimePK, DumbCategory, NamedCategory, ProxyCategory 4 4 5 5 6 6 class ContainsTests(TestCase): … … class ContainsTests(TestCase): 13 13 msg = "QuerySet.contains() cannot be used on unsaved objects." 14 14 with self.assertRaisesMessage(ValueError, msg): 15 15 DumbCategory.objects.contains(DumbCategory()) 16 with self.assertRaisesMessage(ValueError, msg): 17 DateTimePK.objects.contains(DateTimePK()) 16 18 17 19 def test_obj_type(self): 18 20 msg = "'obj' must be a model instance."
-
filter|exclude(obj=unsaved_obj_with_manual_pk)
-
TabularUnified tests/queries/tests.py
diff --git a/tests/queries/tests.py b/tests/queries/tests.py index c429a93af3..f30dbb66ff 100644
a b class Queries5Tests(TestCase): 1982 1982 Employment.objects.filter(employer__in=[company, Company(name="unsaved")]) 1983 1983 with self.assertRaisesMessage(ValueError, msg): 1984 1984 StaffUser.objects.filter(staff=Staff(name="unsaved")) 1985 with self.assertRaisesMessage(ValueError, msg): 1986 ExtraInfo.objects.filter(date=DateTimePK()) 1985 1987 1986 1988 1987 1989 class SelectRelatedTests(TestCase): … … class ExcludeTests(TestCase): 3360 3362 Employment.objects.exclude(employer__in=[company, Company(name="unsaved")]) 3361 3363 with self.assertRaisesMessage(ValueError, msg): 3362 3364 StaffUser.objects.exclude(staff=Staff(name="unsaved")) 3365 with self.assertRaisesMessage(ValueError, msg): 3366 ExtraInfo.objects.exclude(date=DateTimePK()) 3363 3367 3364 3368 3365 3369 class ExcludeTest17600(TestCase):
-
comment:7 by , 37 hours ago
Thank you for the extra details. I've checked and all of these are still failing on 5.1 so are not regressions introduced by 5.2
I also think each case is roughly covered by the title, with the GeneratedField being a special case
comment:8 by , 34 hours ago
Summary: | Unsaved related object with primary_key=True field does not raise usaved object error → Unsaved related object with primary_key=True field does not raise unsaved object error |
---|
PR