Fixed #19580 -- Unified behavior of reverse foreign key and many-to-many relations for unsaved instances.

django/db/models/fields/related_descriptors.py

@@ -627,6 +627,15 @@ def create_reverse_many_to_one_manager(superclass, rel):
 
             self.core_filters = {self.field.name: instance}
 
+            # Even if this relation is not to pk, we require still pk value.
+            # The wish is that the instance has been already saved to DB,
+            # although having a pk value isn't a guarantee of that.
+            if self.instance.pk is None:
+                raise ValueError(
+                    f"{instance.__class__.__name__!r} instance needs to have a primary "
+                    f"key value before this relationship can be used."
+                )
+
         def __call__(self, *, manager):
             manager = getattr(self.model, manager)
             manager_class = create_reverse_many_to_one_manager(manager.__class__, rel)
@@ -634,6 +643,14 @@ def create_reverse_many_to_one_manager(superclass, rel):
 
         do_not_call_in_templates = True
 
+        def _check_fk_val(self):
+            for field in self.field.foreign_related_fields:
+                if getattr(self.instance, field.attname) is None:
+                    raise ValueError(
+                        f'"{self.instance!r}" needs to have a value for field '
+                        f'"{field.attname}" before this relationship can be used.'
+                    )
+
         def _apply_rel_filters(self, queryset):
             """
             Filter the queryset for the instance this manager is bound to.
@@ -714,6 +731,7 @@ def create_reverse_many_to_one_manager(superclass, rel):
             return queryset, rel_obj_attr, instance_attr, False, cache_name, False
 
         def add(self, *objs, bulk=True):
+            self._check_fk_val()
             self._remove_prefetched_objects()
             db = router.db_for_write(self.model, instance=self.instance)
 
@@ -752,6 +770,7 @@ def create_reverse_many_to_one_manager(superclass, rel):
         add.alters_data = True
 
         def create(self, **kwargs):
+            self._check_fk_val()
             kwargs[self.field.name] = self.instance
             db = router.db_for_write(self.model, instance=self.instance)
             return super(RelatedManager, self.db_manager(db)).create(**kwargs)
@@ -759,6 +778,7 @@ def create_reverse_many_to_one_manager(superclass, rel):
         create.alters_data = True
 
         def get_or_create(self, **kwargs):
+            self._check_fk_val()
             kwargs[self.field.name] = self.instance
             db = router.db_for_write(self.model, instance=self.instance)
             return super(RelatedManager, self.db_manager(db)).get_or_create(**kwargs)
@@ -766,6 +786,7 @@ def create_reverse_many_to_one_manager(superclass, rel):
         get_or_create.alters_data = True
 
         def update_or_create(self, **kwargs):
+            self._check_fk_val()
             kwargs[self.field.name] = self.instance
             db = router.db_for_write(self.model, instance=self.instance)
             return super(RelatedManager, self.db_manager(db)).update_or_create(**kwargs)
@@ -779,6 +800,7 @@ def create_reverse_many_to_one_manager(superclass, rel):
             def remove(self, *objs, bulk=True):
                 if not objs:
                     return
+                self._check_fk_val()
                 val = self.field.get_foreign_related_value(self.instance)
                 old_ids = set()
                 for obj in objs:
@@ -802,6 +824,7 @@ def create_reverse_many_to_one_manager(superclass, rel):
             remove.alters_data = True
 
             def clear(self, *, bulk=True):
+                self._check_fk_val()
                 self._clear(self, bulk)
 
             clear.alters_data = True
@@ -822,6 +845,7 @@ def create_reverse_many_to_one_manager(superclass, rel):
             _clear.alters_data = True
 
         def set(self, objs, *, bulk=True, clear=False):
+            self._check_fk_val()
             # Force evaluation of `objs` in case it's a queryset whose value
             # could be affected by `manager.clear()`. Refs #19816.
             objs = tuple(objs)

docs/releases/4.1.txt

@@ -375,6 +375,14 @@ this difference. In Django 4.0 and earlier,
 second example query, but this undocumented behavior led to queries with
 excessive joins.
 
+Reverse foreign key changes for unsaved model instances
+-------------------------------------------------------
+
+In order to unify the behavior with many-to-many relations for unsaved model
+instances, a reverse foreign key now raises ``ValueError`` when calling
+:class:`related managers <django.db.models.fields.related.RelatedManager>` for
+unsaved objects.
+
 Miscellaneous
 -------------
 

tests/many_to_one/tests.py

@@ -738,14 +738,16 @@ class ManyToOneTests(TestCase):
         self.assertEqual("id", cat.remote_field.get_related_field().name)
 
     def test_relation_unsaved(self):
-        # The <field>_set manager does not join on Null value fields (#17541)
         Third.objects.create(name="Third 1")
         Third.objects.create(name="Third 2")
         th = Third(name="testing")
-        # The object isn't saved and thus the relation field is null - we won't even
-        # execute a query in this case.
-        with self.assertNumQueries(0):
-            self.assertEqual(th.child_set.count(), 0)
+        # The object isn't saved and the relation cannot be used.
+        msg = (
+            "'Third' instance needs to have a primary key value before this "
+            "relationship can be used."
+        )
+        with self.assertRaisesMessage(ValueError, msg):
+            th.child_set.count()
         th.save()
         # Now the model is saved, so we will need to execute a query.
         with self.assertNumQueries(1):

tests/many_to_one_null/tests.py

@@ -146,3 +146,36 @@ class ManyToOneNullTests(TestCase):
         self.assertIs(d1.car, None)
         with self.assertNumQueries(0):
             self.assertEqual(list(c1.drivers.all()), [])
+
+    def test_unsaved(self):
+        msg = (
+            "'Car' instance needs to have a primary key value before this relationship "
+            "can be used."
+        )
+        with self.assertRaisesMessage(ValueError, msg):
+            Car(make="Ford").drivers.all()
+
+    def test_related_null_to_field_related_managers(self):
+        car = Car.objects.create(make=None)
+        driver = Driver.objects.create()
+        msg = (
+            f'"{car!r}" needs to have a value for field "make" before this '
+            f"relationship can be used."
+        )
+        with self.assertRaisesMessage(ValueError, msg):
+            car.drivers.add(driver)
+        with self.assertRaisesMessage(ValueError, msg):
+            car.drivers.create()
+        with self.assertRaisesMessage(ValueError, msg):
+            car.drivers.get_or_create()
+        with self.assertRaisesMessage(ValueError, msg):
+            car.drivers.update_or_create()
+        with self.assertRaisesMessage(ValueError, msg):
+            car.drivers.remove(driver)
+        with self.assertRaisesMessage(ValueError, msg):
+            car.drivers.clear()
+        with self.assertRaisesMessage(ValueError, msg):
+            car.drivers.set([driver])
+
+        with self.assertNumQueries(0):
+            self.assertEqual(car.drivers.count(), 0)

tests/null_queries/tests.py

@@ -44,9 +44,14 @@ class NullQueriesTests(TestCase):
         with self.assertRaisesMessage(ValueError, "Cannot use None as a query value"):
             Choice.objects.filter(id__gt=None)
 
-        # Related managers use __exact=None implicitly if the object hasn't been saved.
-        p2 = Poll(question="How?")
-        self.assertEqual(repr(p2.choice_set.all()), "<QuerySet []>")
+    def test_unsaved(self):
+        poll = Poll(question="How?")
+        msg = (
+            "'Poll' instance needs to have a primary key value before this "
+            "relationship can be used."
+        )
+        with self.assertRaisesMessage(ValueError, msg):
+            poll.choice_set.all()
 
     def test_reverse_relations(self):
         """