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

Reported by: Clifford Gama Owned by: Clifford Gama
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Csirmaz Bendegúz, Simon Charette Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no
Pull Requests:19275 build:success

Description (last modified by Clifford Gama)

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):  
    170170        with self.assertRaises(Restaurant.DoesNotExist):
    171171            place.restaurant
    172172
     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
    173183    def test_reverse_relationship_cache_cascade(self):
    174184        """
    175185        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 Clifford Gama, 3 days ago

Description: modified (diff)

comment:2 by Clifford Gama, 3 days ago

Has patch: set

comment:3 by NandiniPahuja, 3 days ago

Hi, I’d like to work on this issue. Is it still available?

comment:4 by Jacob Walls, 2 days ago

Take a look at the last comment: notice the ticket owner already submitted a pull request.

comment:5 by Sarah Boyce, 39 hours ago

Cc: Csirmaz Bendegúz Simon Charette added
Keywords: _is_pk_set removed
Severity: Release blockerNormal
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: UnreviewedAccepted

Hi Clifford, thank you for the report!
This is also failing on 5.1, so this is not a regression introduced by 5.2.

Last edited 39 hours ago by Sarah Boyce (previous) (diff)

comment:6 by Clifford Gama, 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:

  1. 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:  
      180180
      181181    def test_unsaved_error(self):
      182182        m = self.base_model(a=1, b=2)
       183        manual_pk_obj = GeneratedModelNonAutoPk(id=101, a="a")
      183184        msg = "Cannot read a generated field from an unsaved model."
      184185        with self.assertRaisesMessage(AttributeError, msg):
      185186            m.field
       187        with self.assertRaisesMessage(AttributeError, msg):  # Raises integrity error instead
       188            manual_pk_obj.b
      186189
      187190    def test_full_clean(self):
      188191        m = self.base_model(a=1, b=2)
  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  
      11from django.test import TestCase
      22
      3 from .models import DumbCategory, NamedCategory, ProxyCategory
       3from .models import DateTimePK, DumbCategory, NamedCategory, ProxyCategory
      44
      55
      66class ContainsTests(TestCase):
      class ContainsTests(TestCase):  
      1313        msg = "QuerySet.contains() cannot be used on unsaved objects."
      1414        with self.assertRaisesMessage(ValueError, msg):
      1515            DumbCategory.objects.contains(DumbCategory())
       16        with self.assertRaisesMessage(ValueError, msg):
       17            DateTimePK.objects.contains(DateTimePK())
      1618
      1719    def test_obj_type(self):
      1820        msg = "'obj' must be a model instance."
  3. 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):  
      19821982            Employment.objects.filter(employer__in=[company, Company(name="unsaved")])
      19831983        with self.assertRaisesMessage(ValueError, msg):
      19841984            StaffUser.objects.filter(staff=Staff(name="unsaved"))
       1985        with self.assertRaisesMessage(ValueError, msg):
       1986            ExtraInfo.objects.filter(date=DateTimePK())
      19851987
      19861988
      19871989class SelectRelatedTests(TestCase):
      class ExcludeTests(TestCase):  
      33603362            Employment.objects.exclude(employer__in=[company, Company(name="unsaved")])
      33613363        with self.assertRaisesMessage(ValueError, msg):
      33623364            StaffUser.objects.exclude(staff=Staff(name="unsaved"))
       3365        with self.assertRaisesMessage(ValueError, msg):
       3366            ExtraInfo.objects.exclude(date=DateTimePK())
      33633367
      33643368
      33653369class ExcludeTest17600(TestCase):

comment:7 by Sarah Boyce, 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 Jacob Walls, 34 hours ago

Summary: Unsaved related object with primary_key=True field does not raise usaved object errorUnsaved related object with primary_key=True field does not raise unsaved object error
Note: See TracTickets for help on using tickets.
Back to Top