Opened 14 years ago

Closed 11 years ago

#13137 closed Bug (fixed)

GenericForeignKey does not allow content type PK 0

Reported by: Justin de Vesine Owned by: woodlee
Component: contrib.contenttypes Version: dev
Severity: Normal Keywords: contenttype genericforeignkey
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The GenericForeignKey system does not properly recognize when a ContentType ID 0 is linked to. In django/contrib/contenttypes/generic.py, two specific instances are checking for "if primarykey:" rather than "if primarykey is not None:".

This specifically came up in our environment because we are matching the ContentType ids to our legacy database content type ids, which start at 0 rather than 1.

To reproduce, rename attached foobar_models.py to models.py and place in an app named 'foobar', then follow the directions in comments at the bottom of the models file.

The attached proposed patch changes these two if statements to "is not None", which I believe is more correctly in the spirit of the code as well as allowing this functional case.

Attachments (3)

foobar_models.py (1.7 KB ) - added by Justin de Vesine 14 years ago.
Code and instructions to reproduce
allow_contenttype_id_0.patch (1.1 KB ) - added by Justin de Vesine 14 years ago.
Initial proposed patch
allow_contenttype_pk_0_with_tests.diff (2.6 KB ) - added by Justin de Vesine 14 years ago.
Proposed patch (with tests)

Download all attachments as: .zip

Change History (23)

by Justin de Vesine, 14 years ago

Attachment: foobar_models.py added

Code and instructions to reproduce

by Justin de Vesine, 14 years ago

Initial proposed patch

comment:1 by Russell Keith-Magee, 14 years ago

Needs tests: set
Triage Stage: UnreviewedAccepted

by Justin de Vesine, 14 years ago

Proposed patch (with tests)

comment:2 by Justin de Vesine, 14 years ago

Needs tests: unset

comment:3 by Gabriel Hurley, 13 years ago

Component: Contrib appscontrib.contenttypes

comment:4 by Luke Plant, 13 years ago

Type: Bug

comment:5 by Luke Plant, 13 years ago

Severity: Normal

comment:6 by Julien Phalip, 13 years ago

Patch needs improvement: set

Could you rewrite the tests using unittests since this is now Django's preferred way?

comment:7 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:8 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:9 by woodlee, 11 years ago

Owner: changed from nobody to woodlee
Status: newassigned

comment:11 by woodlee, 11 years ago

Patch needs improvement: unset

comment:12 by woodlee, 11 years ago

All contenttypes.ContentTypesTests pass under sqlite.

comment:13 by Ramiro Morales, 11 years ago

See also #13085.

comment:14 by Ramiro Morales, 11 years ago

I'd say close this ticket as wontfix because ContentType is an 'internal' model over which the user has no control so its IntegerField id PK follows the usual semantics of a Django model regarding it having a value of 0 meaning the model has no corresponding DB record.
So the only scenario in which CT could have a 0 PK is an edge-case one and IMHO it's not worth to special-case PK semantics of CT for it.

comment:15 by anonymous, 11 years ago

Triage Stage: AcceptedDesign decision needed

comment:16 by Ramiro Morales, 11 years ago

The one that changed triage state to DDN was me. Sorry

comment:17 by Aymeric Augustin, 11 years ago

Triage Stage: Design decision neededAccepted

I'm in favor of strict checking against None when determining is something is "missing" (which is the case here).

comment:18 by Aymeric Augustin, 11 years ago

Similar cleanup already happened in this file:
https://github.com/django/django/commit/04d9730b127c689b8eda01cbc913efa6e2eb230b#L0L55

The patch fixes the two last instances of if foo:.

comment:20 by Tim Graham, 11 years ago

Resolution: fixed
Status: assignedclosed

Fixed in [6bdc47f75ca].

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