id	summary	reporter	owner	description	type	status	component	version	severity	resolution	keywords	cc	stage	has_patch	needs_docs	needs_tests	needs_better_patch	easy	ui_ux
33004	Inconsistent / Unexpected handling of assigning unsaved model to Generic Foreign Key	Finn Andersen	Sarah Boyce	"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)


"	Bug	closed	Database layer (models, ORM)	dev	Normal	fixed	fk, gfk, generic foreign key, validation	Tim Graham	Ready for checkin	1	0	0	0	0	0