Opened 8 years ago

Closed 3 years ago

#7551 closed Bug (fixed)

GenericForeignKey fails if initialized to None during model init

Reported by: SamBull Owned by: bouke
Component: contrib.contenttypes Version: master
Severity: Normal Keywords: contenttypes genericforeignkey
Cc: sbull@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by ramiro)

Supposing you have a model with a GenericForeignKey...

class Example(models.Model):
    name = models.CharField()
    content_type = models.ForeignKey(ContentType, null=True, blank=True)
    object_id = models.PositiveIntegerField(null=True, blank=True)
    obj = GenericForeignKey()

If you initialize like this, everything is fine:

ex = Example(name='foobar')

But if you do this...

ex = Example(name='foobar', obj=None)

You get an exciting error:
Impossible arguments to GFK.get_content_type!

It looks like the GFK tries to initialize the content_type and object_id fields if its attribute is one of the ones passed to the init. In fact, it should follow the example of get and only initialize those fields if the argument passed to it is non-None.

Attachments (2)

fix_for_content_type_bug.diff (681 bytes) - added by SamBull 8 years ago.
GFK-init patch
fix_for_content_type_bug_improved.diff (639 bytes) - added by SamBull 8 years ago.
revised patch

Download all attachments as: .zip

Change History (17)

Changed 8 years ago by SamBull

GFK-init patch

comment:1 Changed 8 years ago by SamBull

  • Cc sbull@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Gah, I realized my ct and obj_id fields in the example won't work with the GFK field unless I tell it what they're called. This is a bug in my example, my original code properly identifies the content type and object id fields to the GFK.

comment:2 Changed 8 years ago by ericholscher

  • milestone set to 1.0
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 8 years ago by edrex

  • Owner changed from nobody to edrex

comment:4 Changed 8 years ago by edrex

  • Owner edrex deleted

comment:5 Changed 8 years ago by ubernostrum

  • milestone changed from 1.0 to post-1.0

This feels like enough of an edge case (knowingly passing None when you could just leave it out of the argument list to the constructor) that it can wait a bit.

Changed 8 years ago by SamBull

revised patch

comment:6 Changed 8 years ago by SamBull

I've tried to provide a better patch.

I agree that this is an unusual situation, but it can cause hard to debug errors. The problem is that you might be passing in an object via the model's constructor and not know if it's None or not.

Null GFK fields are allowed, so passing in a value that might be None should be allowed as well.

I do agree that this may not be needed for 1.0, but I wanted to highlight the problems this bug causes.

comment:7 Changed 7 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:8 Changed 5 years ago by gabrielhurley

  • Component changed from Contrib apps to contrib.contenttypes

comment:9 Changed 5 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:10 Changed 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:11 Changed 4 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:12 Changed 3 years ago by ramiro

  • Description modified (diff)

comment:13 Changed 3 years ago by ramiro

See also #18599.

comment:14 Changed 3 years ago by bouke

  • Owner set to bouke
  • Status changed from new to assigned

I've improved on this patch by also checking if None is allowed as the foreign key. Previous patches just ignored None, but that doesn't seem right as sometimes None is really invalid, for example when the GFK is non-optional. See my PR:

comment:15 Changed 3 years ago by Tim Graham <timograham@…>

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

In 3918eeb9fde03a2ad1941bce022615fff8eae34d:

Fixed #7551 -- Made GFK allow None init argument.

Thanks SamBull for the report.

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