Opened 2 months ago

Closed 4 weeks ago

Last modified 4 weeks ago

#28613 closed Cleanup/optimization (fixed)

Document that GenericForeignKey returns None when referring to nonexistent pk

Reported by: Sjoerd Job Postmus Owned by: Harshit Jain
Component: Documentation Version: master
Severity: Normal Keywords: genericforeignkey
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

We have a model as follows.

class RemoteIdentifierToLocal(models.Model):
    remote_id = models.UUIDField(primary_key=True)

    local_object_content_type = models.ForeignKey(ContentType)
    local_object_object_id = models.PositiveIntegerField()
    local_object = GenericForeignKey(ct_field='local_object_content_type', fk_field='local_object_object_id')

which is used as follows:

if guid is not None:
    local_object = RemoteIdentifierToLocal.objects.get(remote_id=guid).local_object

To my surprise, we found a case where guid was not-None, and local_object was None.

This was traced back to the implementation of GenericForeignKey containing a try/except-pass:

https://github.com/django/django/blob/master/django/contrib/contenttypes/fields.py#L241 (now)
https://github.com/django/django/commit/bca5327b21eb2e3ee18292cbe532d6d0071201d8#diff-cc83843e623c1ab07a24317073330058R62 (initial commit, 11 years ago)

Now, apparently this has been this way for 11 years already, so must be the "correct" behaviour. However I have not found any documentation of this fact, and would (myself) expect the attribute-access to *do* raise an exception or warning of some sort.

Change History (6)

comment:1 Changed 8 weeks ago by Tim Graham

Component: contrib.contenttypesDocumentation
Summary: GenericForeignKey silently returns None when referring to non-existent pk.Document that GenericForeignKey returns None when referring to nonexistent pk
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

There's one assertion in Django's test suite that fails with the exception catching removed. I doubt it's appropriate to raise a warning for properly working code (assuming the test represents a valid use case -- I didn't study it in much detail). Accepting at least as a documentation enhancement.

comment:2 Changed 7 weeks ago by Harshit Jain

Owner: changed from nobody to Harshit Jain
Status: newassigned

I would like to document this. Can you tell me where do I need to add this?

comment:3 Changed 7 weeks ago by Harshit Jain

Has patch: set

The Documentation has been added. Here is the link to the pull request - https://github.com/django/django/pull/9181.

comment:4 Changed 7 weeks ago by Tim Graham

Patch needs improvement: set

comment:5 Changed 4 weeks ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In f9a01d40:

[2.0.x] Fixed #28613 -- Doc'd the return value for GenericForeignKey when the related object is deleted.

Backport of 1a82fc245eb8ac4b131ec02a6ac3e112deb8d5a6 from master

comment:6 Changed 4 weeks ago by Tim Graham <timograham@…>

In 1a82fc2:

Fixed #28613 -- Doc'd the return value for GenericForeignKey when the related object is deleted.

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