From cd4da34fc1f1df08f593e461b2f670bfd61d0d2f Mon Sep 17 00:00:00 2001 From: sarahboyce Date: Tue, 19 Apr 2022 13:21:23 +0200 Subject: [PATCH] Fixed #33004 -- Made saving objects with unsaved GenericForeignKey raise ValueError. This aligns to the behaviour of OneToOneField and ForeignKey fields. Thanks Jonny Park for the initial patch. --- django/db/models/base.py | 20 ++++++++++++++-- tests/generic_relations/tests.py | 29 ++++++++++++++++------- tests/generic_relations_regress/models.py | 5 ---- tests/generic_relations_regress/tests.py | 10 -------- 4 files changed, 38 insertions(+), 26 deletions(-) diff --git a/django/db/models/base.py b/django/db/models/base.py index 556e25510c..9aa6d7596a 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -1072,8 +1072,9 @@ class Model(metaclass=ModelBase): def _prepare_related_fields_for_save(self, operation_name, fields=None): # Ensure that a model instance without a PK hasn't been assigned to - # a ForeignKey or OneToOneField on this model. If the field is - # nullable, allowing the save would result in silent data loss. + # a ForeignKey, GenericForeignKey or OneToOneField on this model. If + # the field is nullable, allowing the save would result in silent data + # loss. for field in self._meta.concrete_fields: if fields and field not in fields: continue @@ -1107,6 +1108,21 @@ class Model(metaclass=ModelBase): self, field.attname ): field.delete_cached_value(self) + # GenericForeignKeys are private. + for field in self._meta.private_fields: + if fields and field not in fields: + continue + if ( + field.is_relation + and field.is_cached(self) + and hasattr(field, "fk_field") + ): + obj = field.get_cached_value(self, default=None) + if obj and obj.pk is None: + raise ValueError( + f"{operation_name}() prohibited to prevent data loss due to " + f"unsaved related object '{field.name}'." + ) def delete(self, using=None, keep_parents=False): if self.pk is None: diff --git a/tests/generic_relations/tests.py b/tests/generic_relations/tests.py index 7c49e218dd..1ee14f5381 100644 --- a/tests/generic_relations/tests.py +++ b/tests/generic_relations/tests.py @@ -1,8 +1,7 @@ from django.contrib.contenttypes.models import ContentType from django.core.exceptions import FieldError -from django.db import IntegrityError from django.db.models import Q -from django.test import SimpleTestCase, TestCase +from django.test import SimpleTestCase, TestCase, skipUnlessDBFeature from .models import ( AllowsNullGFK, @@ -501,14 +500,26 @@ class GenericRelationsTests(TestCase): with self.assertRaisesMessage(FieldError, msg): TaggedItem.objects.get(content_object="") - def test_unsaved_instance_on_generic_foreign_key(self): - """ - Assigning an unsaved object to GenericForeignKey should raise an - exception on model.save(). - """ + def test_unsaved_generic_foreign_key_parent_save(self): quartz = Mineral(name="Quartz", hardness=7) - with self.assertRaises(IntegrityError): - TaggedItem.objects.create(tag="shiny", content_object=quartz) + tagged_item = TaggedItem(tag="shiny", content_object=quartz) + msg = ( + "save() prohibited to prevent data loss due to unsaved related object " + "'content_object'." + ) + with self.assertRaisesMessage(ValueError, msg): + tagged_item.save() + + @skipUnlessDBFeature("has_bulk_insert") + def test_unsaved_generic_foreign_key_parent_bulk_create(self): + quartz = Mineral(name="Quartz", hardness=7) + tagged_item = TaggedItem(tag="shiny", content_object=quartz) + msg = ( + "bulk_create() prohibited to prevent data loss due to unsaved related " + "object 'content_object'." + ) + with self.assertRaisesMessage(ValueError, msg): + TaggedItem.objects.bulk_create([tagged_item]) def test_cache_invalidation_for_content_type_id(self): # Create a Vegetable and Mineral with the same id. diff --git a/tests/generic_relations_regress/models.py b/tests/generic_relations_regress/models.py index a028ff01d8..dc55b2a83b 100644 --- a/tests/generic_relations_regress/models.py +++ b/tests/generic_relations_regress/models.py @@ -104,11 +104,6 @@ class Company(models.Model): links = GenericRelation(Link) -# For testing #13085 fix, we also use Note model defined above -class Developer(models.Model): - name = models.CharField(max_length=15) - - class Team(models.Model): name = models.CharField(max_length=15) diff --git a/tests/generic_relations_regress/tests.py b/tests/generic_relations_regress/tests.py index 16705131a7..6c708fefbb 100644 --- a/tests/generic_relations_regress/tests.py +++ b/tests/generic_relations_regress/tests.py @@ -1,4 +1,3 @@ -from django.db import IntegrityError from django.db.models import ProtectedError, Q, Sum from django.forms.models import modelform_factory from django.test import TestCase, skipIfDBFeature @@ -15,7 +14,6 @@ from .models import ( Contact, Content, D, - Developer, Guild, HasLinkThing, Link, @@ -140,14 +138,6 @@ class GenericRelationTests(TestCase): self.assertEqual(count_places(p1), 1) self.assertEqual(count_places(p2), 1) - def test_target_model_is_unsaved(self): - """Test related to #13085""" - # Fails with another, ORM-level error - dev1 = Developer(name="Joe") - note = Note(note="Deserves promotion", content_object=dev1) - with self.assertRaises(IntegrityError): - note.save() - def test_target_model_len_zero(self): """ Saving a model with a GenericForeignKey to a model instance whose