Opened 14 years ago

Closed 11 years ago

Last modified 2 years ago

#13085 closed Bug (fixed)

GenericForeignKey.get_content_type fails if `object` or `id` evaluate to False. Should use 'is not None' test.

Reported by: Ben Owned by: Ramiro Morales <cramm0@…>
Component: contrib.contenttypes Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

contenttypes.GenericForeignKey.get_content_type is used to find the content type of an object. If the object evaluates to False, then an error is raised. This is because 'if obj:' is used, rather than 'if obj is not None:'. The same problem exists for the 'id' argument, and I believe also in the __get__ method.

Attachments (2)

generic.py.diff (1.2 KB ) - added by Ben 14 years ago.
13085.diff (3.4 KB ) - added by Ben 14 years ago.
Patch with tests included

Download all attachments as: .zip

Change History (14)

by Ben, 14 years ago

Attachment: generic.py.diff added

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

Needs tests: set
Triage Stage: UnreviewedAccepted

Two quick points:

1) Please generate patches relative to the root of the source tree
2) Please provide a test case, not just a patch. Under what conditions will the object be false? Ideally, this should be in the form of a regression test integrated into Django's test suite, but if you can't work out how to do that, a code sample will do.

Accepting because I think I can spot the problem case (getting the content type for an unsaved object)

comment:2 by Ben, 14 years ago

Owner: changed from nobody to Ben

by Ben, 14 years ago

Attachment: 13085.diff added

Patch with tests included

comment:3 by Ben, 14 years ago

Needs tests: unset

I've included a new patch with a regression test included. The object will be false if it has nonzero is defined and returns False, or if len is defined and returns 0. Either of these should be possible without generic objects failing. The bug would also cause problems if a generic object was used with content_type_id=0.

comment:4 by Gabriel Hurley, 13 years ago

Component: Contrib appscontrib.contenttypes

comment:5 by Luke Plant, 13 years ago

Type: Bug

comment:6 by Luke Plant, 13 years ago

Severity: Normal

comment:7 by Łukasz Rekucki, 13 years ago

Easy pickings: unset
Patch needs improvement: set
UI/UX: unset

Patch no longer cleanly applies to trunk.

comment:8 by Ben, 13 years ago

Owner: Ben removed

comment:9 by Ramiro Morales, 11 years ago

#14325 and #19494 were duplicates.

comment:10 by Ramiro Morales, 11 years ago

See also #13137.

comment:11 by Ramiro Morales <cramm0@…>, 11 years ago

Owner: set to Ramiro Morales <cramm0@…>
Resolution: fixed
Status: newclosed

In 04d9730b127c689b8eda01cbc913efa6e2eb230b:

Fixed #13085 -- Don't fail on creation of model with GFK to a model with len() returning zero.

Also, according to the comments on the ticket and its duplicates, added
tests execising saving an instance of a model with a GFK to:

  • An unsaved object -- This actually doesn't generate the same failure but another ORM-level exception. The test verifies it's the case.
  • An instance of a model with a nonzero() method thant returns False for it. This doesn't fail because that code path isn't executed.
  • An instance of a model with a CharField PK and an empty value for it. This doesn't fail.

comment:12 by GitHub <noreply@…>, 2 years ago

In 1ed8ca43:

Refs #13085 -- Removed unnecessary ManyToManyFields from generic_relations_regress test models.

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