[Django] #33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic Foreign Key

Django

unread,

Aug 8, 2021, 10:27:05 PM8/8/21

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------
Reporter: | Owner: nobody
Finndersen |
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: dev
layer (models, ORM) | Keywords: fk, gfk, generic
Severity: Normal | foreign key, validation
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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:

{{{#!python
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)

--
Ticket URL: <https://code.djangoproject.com/ticket/33004>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Django

unread,

Aug 9, 2021, 7:26:09 AM8/9/21

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* cc: Tim Graham (added)
* stage: Unreviewed => Accepted

Comment:

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 [https://docs.djangoproject.com/en/stable/releases/1.8
/#assigning-unsaved-objects-to-relations-raises-an-error release notes]
still claim that an error is raised in this case.

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:1>

Django

unread,

Aug 9, 2021, 7:26:22 AM8/9/21

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Reporter: Finn Andersen | Owner: nobody

Type: Bug | Status: new

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* type: Cleanup/optimization => Bug

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:2>

Django

unread,

Aug 9, 2021, 11:03:24 PM8/9/21

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Finn Andersen):

* needs_docs: 0 => 1
* needs_tests: 0 => 1

Comment:

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)

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:3>

Django

unread,

Aug 10, 2021, 12:55:48 AM8/10/21

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* needs_docs: 1 => 0
* needs_tests: 1 => 0

Comment:

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

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:4>

Django

unread,

Aug 22, 2021, 2:19:43 AM8/22/21

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Reporter: Finn Andersen | Owner: Jonny
| Park
Type: Bug | Status: assigned

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jonny Park):

* owner: nobody => Jonny Park
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:5>

Django

unread,

Aug 28, 2021, 2:53:55 AM8/28/21

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jonny Park):

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

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:6>

Django

unread,

Aug 28, 2021, 7:50:04 AM8/28/21

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:7>

Django

unread,

Sep 5, 2021, 10:02:19 PM9/5/21

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by wtzhu13):

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()

In [23]:

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:8>

Django

unread,

Oct 19, 2021, 1:31:49 AM10/19/21

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

Replying to [comment:8 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.

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:9>

Django

unread,

Oct 28, 2021, 2:45:12 AM10/28/21

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:10>

Django

unread,

Apr 19, 2022, 1:56:17 AM4/19/22

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Reporter: Finn Andersen | Owner: Sarah
| Boyce

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* owner: Jonny Park => Sarah Boyce

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:11>

Django

unread,

Apr 21, 2022, 4:15:27 AM4/21/22

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Keywords: fk, gfk, generic | Triage Stage: Ready for
foreign key, validation | checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/33004#comment:12>

Django

unread,

Apr 21, 2022, 5:48:45 AM4/21/22

to django-...@googlegroups.com

#33004: Inconsistent / Unexpected handling of assigning unsaved model to Generic
Foreign Key
-------------------------------------+-------------------------------------

Reporter: Finn Andersen | Owner: Sarah
| Boyce

Type: Bug | Status: closed

Component: Database layer | Version: dev
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: fk, gfk, generic | Triage Stage: Ready for
foreign key, validation | checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak <felisiak.mariusz@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"cd4da34fc1f1df08f593e461b2f670bfd61d0d2f" cd4da34f]:
{{{
#!CommitTicketReference repository=""
revision="cd4da34fc1f1df08f593e461b2f670bfd61d0d2f"
Fixed #33004 -- Made saving objects with unsaved GenericForeignKey raise
ValueError.