Inconsistent / Unexpected handling of assigning unsaved model to Generic Foreign Key

[Finn Andersen]

https://code.djangoproject.com/ticket/10811 addresses the issue of assigned an unsaved model to a ForeignKey or OneToOneField (raises error when save() called), however the same logic / pattern does not apply to GFKs.

Given:

class ModelA(models.Model):
    name = models.CharField(max_length=20)


class ModelB(models.Model):
    gfk_ctype = models.ForeignKey(ContentType, on_delete=models.PROTECT)
    gfk_id = models.PositiveIntegerField()
    gfk = GenericForeignKey('gfk_ctype', 'gfk_id')


class ModelC(models.Model):
    fk = models.ForeignKey(ModelA, on_delete=models.CASCADE)

Foreign Key Behaviour:

In [2]: a = ModelA(name='Model A')

In [3]: c = ModelC(fk=a)

In [4]: c.fk
Out[4]: <ModelA: ModelA object (None)>

In [5]: c.save()
---------------------------------------------------------------------------
...
ValueError: save() prohibited to prevent data loss due to unsaved related object 'fk'.

In [6]: a.save()
(0.016) INSERT INTO "test_app_modela" ("name") VALUES ('Model A'); args=['Model A']

In [7]: c.fk
Out[7]: <ModelA: ModelA object (1)>

In [8]: c.save()
(0.016) INSERT INTO "test_app_modelc" ("fk_id") VALUES (1); args=[1]

GFK behaviour:

In [9]: a2 = ModelA(name='Model A2')

In [10]: b = ModelB(gfk=a2)

In [11]: b.gfk
Out[11]: <ModelA: ModelA object (None)>

In [12]: b.save()
(0.000) INSERT INTO "test_app_modelb" ("gfk_ctype_id", "gfk_id") VALUES (9, NULL); args=[9, None]
---------------------------------------------------------------------------
IntegrityError: NOT NULL constraint failed: test_app_modelb.gfk_id

In [14]: b.gfk.save()
(0.015) INSERT INTO "test_app_modela" ("name") VALUES ('Model A2'); args=['Model A2']

In [15]: b.gfk
(0.000) SELECT "test_app_modela"."id", "test_app_modela"."name" FROM "test_app_modela" WHERE "test_app_modela"."id" IS NULL LIMIT 21; args=()
None

In [17]: b.gfk_ctype
Out[17]: <ContentType: test_app | model a>

Two observations:

• No check on b.gfk and b.gfk_id value during save() which could lead to silent data loss if b.gfk_id is nullable.
• When a2 is saved, accessing b.gfk now does a redundant DB query to try and find ModelA instance with PK = None, then then returns None value (effectively un-assigning a2 model), while keeping b.gfk_ctype intact. This is because the new pk of a2 is different to the existing gfk_id (pk_val) of the GFK field (None)

What should be done:

• Modify Model.save() or Model._prepare_related_fields_for_save() to also perform verification check for GFK fields
• Modify GenericForeignKey.get() to handle case of pk_val = None (update fk_field value using PK value of GFK model if present, do not perform redundant DB query on pk=None, return previously assigned (then saved) model instead of None)

[Mariusz Felisiak]

Agreed, assigning unsaved objects should raise an error for GenericForeignKey. It was added in 5643a3b51be338196d0b292d5626ad43648448d3 but reverted later in 5980b05c1fad69eef907e0076aa2dc837edab529. It looks like an unintended change as ​release notes still claim that an error is raised in this case.

[Finn Andersen]

I think verification should be done upon save() not assignment (to align with existing behaviour for FKs and enable saving of related model at later point before calling save() of primary model)

[Mariusz Felisiak]

We don't have a patch so there is no need to set this flags.

[Jonny Park]

Made the pull request at ​https://github.com/django/django/pull/14807

[wtzhu]

The code I defined is as follows. I have verified that the data already exists in the database after I save the object of Grade, but I can not use it to save the object of Stu.

class Grade(models.Model):
    id = models.IntegerField(primary_key=True)
    grade = models.CharField(max_length=50, unique=True)


class Stu(models.Model):
    id = models.IntegerField(primary_key=True)
    name = models.CharField(max_length=50, unique=True)
    grade_of_stu = models.ForeignKey(Grade, to_field="grade", on_delete=models.CASCADE)


###########################################Django shell##############################################################################
In [17]: g = Grade(grade="3")

In [18]: g.save()

In [19]: stu = Stu(name="Fred", grade_of_stu=g)

In [20]: stu.save()
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-20-a0e3017ded06> in <module>
----> 1 stu.save()

C:\ProgramData\Anaconda3\lib\site-packages\django\db\models\base.py in save(self, force_insert, force_update, using, update_fields)
    680         non-SQL backends), respectively. Normally, they should not be set.
    681         """
--> 682         self._prepare_related_fields_for_save(operation_name='save')
    683
    684         using = using or router.db_for_write(self.__class__, instance=self)

C:\ProgramData\Anaconda3\lib\site-packages\django\db\models\base.py in _prepare_related_fields_for_save(self, operation_name)
    930                     if not field.remote_field.multiple:
    931                         field.remote_field.delete_cached_value(obj)
--> 932                     raise ValueError(
    933                         "%s() prohibited to prevent data loss due to unsaved "
    934                         "related object '%s'." % (operation_name, field.name)

ValueError: save() prohibited to prevent data loss due to unsaved related object 'grade_of_stu'.

In [21]: stu = Stu(name="Fred", grade_of_stu=Grade.objects.get(grade="3"))

In [22]: stu.save()

[Mariusz Felisiak]

Replying to wtzhu:

The code I defined is as follows. I have verified that the data already exists in the database after I save the object of Grade, but I can not use it to save the object of Stu.

This is not related with GenericForeignKey please create a separate ticket.

[Sarah Boyce]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In cd4da34f:

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.