Opened 4 months ago

Last modified 5 weeks ago

#33004 assigned Bug

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

Reported by: Finn Andersen Owned by: Jonny Park
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: fk, gfk, generic foreign key, validation
Cc: Tim Graham Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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)

Change History (10)

comment:1 Changed 4 months ago by Mariusz Felisiak

Cc: Tim Graham added
Triage Stage: UnreviewedAccepted

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.

comment:2 Changed 4 months ago by Mariusz Felisiak

Type: Cleanup/optimizationBug

comment:3 Changed 4 months ago by Finn Andersen

Needs documentation: set
Needs tests: set

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)

comment:4 Changed 4 months ago by Mariusz Felisiak

Needs documentation: unset
Needs tests: unset

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

comment:5 Changed 3 months ago by Jonny Park

Owner: changed from nobody to Jonny Park
Status: newassigned

comment:6 Changed 3 months ago by Jonny Park

comment:7 Changed 3 months ago by Jacob Walls

Has patch: set

comment:8 Changed 3 months ago by 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()
Last edited 3 months ago by Tim Graham (previous) (diff)

comment:9 in reply to:  8 Changed 7 weeks ago by 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.

comment:10 Changed 5 weeks ago by Mariusz Felisiak

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top