Opened 15 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)
Change History (23)
by , 15 years ago
| Attachment: | foobar_models.py added |
|---|
comment:1 by , 15 years ago
| Needs tests: | set |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
by , 15 years ago
| Attachment: | allow_contenttype_pk_0_with_tests.diff added |
|---|
Proposed patch (with tests)
comment:2 by , 15 years ago
| Needs tests: | unset |
|---|
comment:3 by , 14 years ago
| Component: | Contrib apps → contrib.contenttypes |
|---|
comment:4 by , 14 years ago
| Type: | → Bug |
|---|
comment:5 by , 14 years ago
| Severity: | → Normal |
|---|
comment:6 by , 14 years ago
| Patch needs improvement: | set |
|---|
Could you rewrite the tests using unittests since this is now Django's preferred way?
comment:9 by , 12 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:10 by , 12 years ago
comment:11 by , 12 years ago
| Patch needs improvement: | unset |
|---|
comment:14 by , 12 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 , 12 years ago
| Triage Stage: | Accepted → Design decision needed |
|---|
comment:17 by , 12 years ago
| Triage Stage: | Design decision needed → Accepted |
|---|
I'm in favor of strict checking against None when determining is something is "missing" (which is the case here).
comment:18 by , 12 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:.
Code and instructions to reproduce