#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: | dev |
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 by , 7 years ago
Component: | contrib.contenttypes → Documentation |
---|---|
Summary: | GenericForeignKey silently returns None when referring to non-existent pk. → Document that GenericForeignKey returns None when referring to nonexistent pk |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Cleanup/optimization |
comment:2 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I would like to document this. Can you tell me where do I need to add this?
comment:3 by , 7 years ago
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 by , 7 years ago
Patch needs improvement: | set |
---|
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.