Unsaved related object with primary_key=True field does not raise unsaved object error

[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:

tests/one_to_one/tests.py

@@ -170 +170 @@
         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

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.

[Clifford Gama]

​PR

[NandiniPahuja]

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

[Jacob Walls]

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

[Sarah Boyce]

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

[Clifford Gama]

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.

   tests/model_fields/test_generatedfield.py

   @@ -180 +180 @@
    
        def test_unsaved_error(self):
            m = self.base_model(a=1, b=2)
   +        manual_pk_obj = GeneratedModelNonAutoPk(id=101, a="a")
            msg = "Cannot read a generated field from an unsaved model."
            with self.assertRaisesMessage(AttributeError, msg):
                m.field
   +        with self.assertRaisesMessage(AttributeError, msg):  # Raises integrity error instead
   +            manual_pk_obj.b
    
        def test_full_clean(self):
            m = self.base_model(a=1, b=2)

2. QuerySet.contains(unsaved_with_manual_pk) raises integrity error.

   tests/queries/test_contains.py

   @@ -1 +1 @@
    from django.test import TestCase
    
   -from .models import DumbCategory, NamedCategory, ProxyCategory
   +from .models import DateTimePK, DumbCategory, NamedCategory, ProxyCategory
    
    
    class ContainsTests(TestCase):
   @@ -13 +13 @@
            msg = "QuerySet.contains() cannot be used on unsaved objects."
            with self.assertRaisesMessage(ValueError, msg):
                DumbCategory.objects.contains(DumbCategory())
   +        with self.assertRaisesMessage(ValueError, msg):
   +            DateTimePK.objects.contains(DateTimePK())
    
        def test_obj_type(self):
            msg = "'obj' must be a model instance."

3. filter|exclude(obj=unsaved_obj_with_manual_pk)

   tests/queries/tests.py

   @@ -1982 +1982 @@
                Employment.objects.filter(employer__in=[company, Company(name="unsaved")])
            with self.assertRaisesMessage(ValueError, msg):
                StaffUser.objects.filter(staff=Staff(name="unsaved"))
   +        with self.assertRaisesMessage(ValueError, msg):
   +            ExtraInfo.objects.filter(date=DateTimePK())
    
    
    class SelectRelatedTests(TestCase):
   @@ -3360 +3362 @@
                Employment.objects.exclude(employer__in=[company, Company(name="unsaved")])
            with self.assertRaisesMessage(ValueError, msg):
                StaffUser.objects.exclude(staff=Staff(name="unsaved"))
   +        with self.assertRaisesMessage(ValueError, msg):
   +            ExtraInfo.objects.exclude(date=DateTimePK())
    
    
    class ExcludeTest17600(TestCase):

[Sarah Boyce]

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

[Tim Schilling]

Assigning to Django Sprints for to reserve for an upcoming sprint. DjangoCon US ends August 28, this is available for someone else to pick up on the 29th.

Clifford Gama, if you'd like to continue to work on this please re-assign this to yourself.

[Alin Morosanu]

The ticket is assigned to me. My next step is to read the existing review and confirm the first case to investigate

[Alin Morosanu]

I’m investigating #36259, which reports that "Django’s unsaved-object checks can be bypassed when an unsaved instance already has a primary key".

The original proposed solution was to use _state.adding to detect unsaved instances. However, in ​PR #19275, Jacob Walls identified existing workflows that stricter checks would reject:

• Querying related objects through a new UUID-backed instance, such as pizza.toppings.all().
• Saving interdependent objects within a transaction where foreign-key constraints are checked after both objects have been saved.

I’ve investigated the four cases described in this ticket. On the tested checkout, with SQLite:

• Saving with a missing referenced row raises IntegrityError when the constraint is checked.
• contains() returns False for an assigned PK with no matching row.
• Related filter() and exclude() queries execute without an unsaved-object error.
• Reading an unloaded generated field with an assigned PK but no matching row raises DoesNotExist.

I have an uncommitted prototype addressing only the generated-field case. It allows the normal lookup, preserving successful retrieval when the PK identifies an existing row. If that lookup raises the model’s DoesNotExist and the instance is marked as adding, it raises the existing unsaved-field AttributeError instead. It does not change saves, membership checks, or related filters.

Seven regression cases fail against the unchanged runtime and pass with this change.

Could a maintainer clarify whether the existing save and query behaviors should be preserved? Would the generated-field exception change be suitable for a separate, narrowly scoped PR, or should a broader behaviour change and possible deprecation be discussed first?

[Jacob Walls]

Replying to Alin Morosanu:

Could a maintainer clarify whether the existing save and query behaviors should be preserved? Would the generated-field exception change be suitable for a separate, narrowly scoped PR, or should a broader behaviour change and possible deprecation be discussed first?

Do you have any new information about it?

[Alin Morosanu]

I found no new reason to change the save/query behavior discussed in ​PR #19275.

I tested a smaller change: reading a generated field on a new instance raises AttributeError when its PK has no matching row, instead of DoesNotExist. Successful lookups and saved-instance errors stay unchanged.

Seven tests fail before the change and pass after it. I found no data loss here; the benefit is a more consistent error. Is that enough reason to change it?

[Alin Morosanu]

I updated ​PR #22182 to cover all four cases in the description. It is still a draft and a proposal, so I'd like a decision on the approach before I polish it.

What the PR does

• Case 2 (generated fields): reading an unloaded generated field on a new instance raises AttributeError instead of DoesNotExist when no row matches. Lookups that find a row don't change.
• Cases 1, 3 and 4: a new instance with a primary key set (_state.adding is True) now warns with RemovedInDjango2029Warning when used in a related filter, in QuerySet.contains(), or saved as a cached related object. Results stay the same while it only warns. The docs propose raising ValueError in Django 2029.
• Reverse related managers (reads), prefetching, inline formsets and deletion use the target values, so they keep working without warnings. This follows ​Jacob's review on PR #19275: querying related objects should stay allowed, and any change needs a deprecation path.

Questions

1. Is _state.adding the right check? It also warns for valid code, such as a pre_save handler or a custom clean() that filters by a new instance with a default UUID primary key, a filter with a deserialized instance, and saving the related row later in the same transaction.
2. Should case 2 and the warnings be decided together, or split? Case 2 still loads values from an existing row, while the warnings treat the same kind of instance as not usable in queries.
3. Is the 2029 date OK, or should this only warn for now?

Known limit: for acontains() and asave(), the warning points to asgiref, not the caller.

[Farhan Ali]

I went through the four cases again, along with the review on ​PR 19275. All examples use an instance built in Python with its primary key set, which was never saved or loaded, e.g. x = Author(pk=1).

Case 1: saving a relation to it

Book.objects.create(author=x)

There's no early ValueError; the database raises IntegrityError if the row doesn't exist. This looks intentional. The comment in Model._prepare_related_fields_for_save() says:

# A pk may have been assigned manually to a model instance not
# saved to the database (or auto-generated in a case like
# UUIDField), but we allow the save to proceed and rely on the
# database to raise an IntegrityError if applicable.

Jacob's review of ​PR 19275 also showed valid code that depends on this, e.g. creating two related rows inside one transaction.

Case 3: QuerySet.contains()

Author.objects.contains(x)

Returns False if row 1 doesn't exist and True if it does. That's the correct answer to "is this row in the queryset?". The ValueError is there for instances with no primary key, where there's nothing to compare.

Case 4: filter() / exclude()

Book.objects.filter(author=x)

Generates WHERE author_id = 1, the same as filter(author=1). The unsaved-object check came from #31486, where an instance with pk=None was silently treated like author=None. On that ticket, checking obj.pk is None was preferred over obj._state.adding as "less controversial". With a primary key set, the query does what was asked. Jacob's review of ​PR 19275 also showed that rejecting these breaks valid code such as pizza.toppings.all() inside save().

Case 2: reading an unloaded generated field
This one is inconsistent. For a new instance, the error depends only on whether the primary key happens to be set:

GeneratedModelNonAutoPk(a=5).b
# AttributeError: Cannot retrieve deferred field 'b' from an unsaved model.

GeneratedModelNonAutoPk(id=99, a=5).b   # no row with id=99
# DoesNotExist: GeneratedModelNonAutoPk matching query does not exist.

Both instances are new and unsaved, but the second one runs a query and fails with DoesNotExist.

Suggestion
Cases 1, 3, and 4 look like intended behavior, and changing them would break valid code, so I'd leave them as they are. Case 2 is the only real inconsistency, and I'd like to scope this ticket to it. Simon, Jacob, does that sound reasonable?
As suggested by Alin here https://code.djangoproject.com/ticket/36259?cnum_edit=18#comment:16

[Jacob Walls]

Thanks Alin and Farhan for the research here -- it's valuable to record all these findings.

I agree on Cases 1, 3, and 4 being intended behavior based on the current information.

Looking more closely at the GeneratedField case, I think it's also intended behavior. If there's a PK set, and you request the generated field, a query should happen, and you should get DoesNotExist if the row is missing. If there's no PK, no query can be done, so you get an error that says so.

---

Stepping back for a minute, I think Clifford may have been led down the path of investigating the use of _state.adding due to Django's existing wording in error messages about "unsaved objects". But in reality, the checks are about whether the PK is set. That's understandable. For many Django users without custom primary_key=True fields, and no use case for in-memory "unfetched" instances, the two are equivalent.

I think the most we should be doing here is considering a tweak to the wording of the existing error to mention "unset PK" instead of "unsaved object". If you'd like to propose that separately, we can consider it, but I would suggest a new ticket for it just to keep the discussion a little easier to follow.