Opened 8 years ago

Closed 8 years ago

#15011 closed (invalid)

An improvement of GenericForeignKey which behaves badly in some a bit uncommon but possible situations

Reported by: pinkeen@… Owned by: nobody
Component: Contrib apps Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


The fk_field associated with GenericForeignKey field gets updated upon assigning an object to GFK field and initialization of the parent model (via kwargs). In my opinion it should also get updated when the parent model is saved.

Suppose we have a model (A) with GFK field. We create an instance (b) of some model (B) and assign it to the GFK field of an instance of model A (a). If a wasn't saved prior to the assigning then the fk_field will be None when saving this instance of A. If we don't have NOT NULL (null=True) constraints on the fk_field then the a will be saved with fk_field=NULL thus failing silently (I don't consider this to be a proper nor logical behaviour.

My proposition is to add a hook to pre_save signal of the parent model in which we update the fk_field if not set. If the the GFK field has an object assigned already but it isn't saved yet then we throw an exception.

I am attaching my improved version of the GFK class (via subclassing) - it should be trivial to merge if you deem it worthy.

Filip Sobalski

Attachments (1) (1.5 KB) - added by pinkeen@… 8 years ago.

Download all attachments as: .zip

Change History (3)

Changed 8 years ago by pinkeen@…

Attachment: added

comment:1 Changed 8 years ago by pinkeen@…

Of course it should be 'b':
If b wasn't saved prior to the assigning then the fk_field will be None when saving this instance of A.

comment:2 Changed 8 years ago by Russell Keith-Magee

Resolution: invalid
Status: newclosed

Firstly - please upload changes as diffs, not blocks of code that we have to guess where to integrate.

Secondly - test cases aren't optional. If you want use to take you seriously, you need to provide programatic test cases integrated into Django's own test suite.

Lastly, the change you propose would make GFKs inconsistent with FKs.

Note: See TracTickets for help on using tickets.
Back to Top