Code

Opened 4 years ago

Closed 12 months ago

#13137 closed Bug (fixed)

GenericForeignKey does not allow content type PK 0

Reported by: devesine Owned by: woodlee
Component: contrib.contenttypes Version: master
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 devesine 4 years ago.
Code and instructions to reproduce
allow_contenttype_id_0.patch (1.1 KB) - added by devesine 4 years ago.
Initial proposed patch
allow_contenttype_pk_0_with_tests.diff (2.6 KB) - added by devesine 4 years ago.
Proposed patch (with tests)

Download all attachments as: .zip

Change History (23)

Changed 4 years ago by devesine

Code and instructions to reproduce

Changed 4 years ago by devesine

Initial proposed patch

comment:1 Changed 4 years ago by russellm

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

Changed 4 years ago by devesine

Proposed patch (with tests)

comment:2 Changed 4 years ago by devesine

  • Needs tests unset

comment:3 Changed 3 years ago by gabrielhurley

  • Component changed from Contrib apps to contrib.contenttypes

comment:4 Changed 3 years ago by lukeplant

  • Type set to Bug

comment:5 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:6 Changed 3 years ago by julien

  • Patch needs improvement set

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

comment:7 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:8 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:9 Changed 20 months ago by woodlee

  • Owner changed from nobody to woodlee
  • Status changed from new to assigned

comment:11 Changed 20 months ago by woodlee

  • Patch needs improvement unset

comment:12 Changed 20 months ago by woodlee

All contenttypes.ContentTypesTests pass under sqlite.

comment:13 Changed 18 months ago by ramiro

See also #13085.

comment:14 Changed 17 months ago by ramiro

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 Changed 17 months ago by anonymous

  • Triage Stage changed from Accepted to Design decision needed

comment:16 Changed 17 months ago by ramiro

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

comment:17 Changed 16 months ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

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

comment:18 Changed 16 months ago by aaugustin

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 Changed 12 months ago by timo

  • Resolution set to fixed
  • Status changed from assigned to closed

Fixed in [6bdc47f75ca].

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.