Opened 7 years ago

Closed 4 years ago

#7800 closed Bug (fixed)

GenericRelation doesn't respect changes from the default GenericForeignKey field names in the contenttypes framework

Reported by: devinj Owned by: charmless
Component: contrib.contenttypes Version: master
Severity: Normal Keywords:
Cc: miracle2k Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I have a GenericForeignKey with custom field names ( http://pastebin.ca/1073849 ). I get an error when I use one of the GenericRelations, because it doesn't try to use this custom field name.
FieldError: Cannot resolve keyword 'content_type' into field. Choices are: author, channel, content, creation_time, id, object_id, parent_type, post, replies, title, update_time

Instead, it tries to use the default names. I think that's these lines in this paste: http://pastebin.ca/1074704 . It should be noted that at one point I did use the default names, but never really tested it, and ended up changing their names and deleting the db, recreating the db using syncdb. I am not aware of any other steps I should have taken.

I've tried asking around (in #django on irc.freenode.net) to see if I've missed anything, but nobody else knows, so this is my best guess for an error I can't otherwise explain.

Attachments (2)

models.py (2.9 KB) - added by devinj 7 years ago.
The models.py file
gfk-from-option.diff (2.3 KB) - added by charmless 7 years ago.
Initial sketch of 'from' option for GenericRelation

Download all attachments as: .zip

Change History (12)

Changed 7 years ago by devinj

The models.py file

comment:1 Changed 7 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The lines in the source you point to are not the problem. I think you are misunderstanding what the pop() does on both those lines.

Could you construct a smaller example that demonstrates the problem? 60 lines of models to have to wade through is probably more than necessary. And what steps have to be taken to actually trigger the issue. You're doing something to see a particular error, but you haven't said exactly what that is. So we can't be sure how to construct a test case that is testing the same code paths.

It's very possible that you've found a bug, but at the moment there isn't enough information here to repeat the problem or confirm it, so if you could construct a small pair of models and a particular filter or query that shows how to trigger the problem you're seeing, that would be greatly appreciated.

comment:2 Changed 7 years ago by devinj

Yeah, I figured I wouldn't get the right section, since I don't know the django internals at all. All I did was look for a string literal where there should presumably be a variable. Let me try to make a smaller example.

Here's a pastebin link: http://pastebin.ca/1074745 and here's a session with it: http://pastebin.ca/1074748

comment:3 Changed 7 years ago by mk

  • Component changed from Uncategorized to Contrib apps
  • Triage Stage changed from Unreviewed to Accepted

I can confirm this bug. I've tried fixing it but got lost somewhere in the query construction.

comment:4 Changed 7 years ago by charmless

  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Owner changed from nobody to charmless
  • Patch needs improvement set
  • Status changed from new to assigned

I've run into this as well. The nasty part is that it's not caught by model validation, so everything seems fine until you use it.

The central issue is these lines in django/contrib/contenttypes/generic.py (from line 114 in SVN 8279):

        # Override content-type/object-id field names on the related class
        self.object_id_field_name = kwargs.pop("object_id_field", "object_id")
        self.content_type_field_name = kwargs.pop("content_type_field", "content_type")

These are two optional arguments not mentioned in the docs (that I can find), which default to 'object_id' and 'content_type' for the two fields.

At the least, there should be some sort of validation error if a GenericRelation tries to associate itself to a model without the object_id/pk and content_type/ct fields it expects. These two fields would also need to be documented.

Alternatively, I've got a bit of a nicer mechanism for this. I'll attach a patch which lets you do such things as:

   ...
   relname1 = GenericRelation(Blog) # connect to the only existing GenericForeignKey on Blog, error if there isn't exactly one
   relname2 = GenericRelation(Lifestream, from='item') # connects to the GenericForeignKey named 'item' on the Lifestream model
   ...

I've added checks for any case where the connection is broken or ambiguous. Comments appreciated!

Changed 7 years ago by charmless

Initial sketch of 'from' option for GenericRelation

comment:5 Changed 7 years ago by charmless

This obviously needs some massaging, including tests and docs. I'll supply both if there isn't immediate hate of the idea.

comment:6 Changed 7 years ago by miracle2k

  • Cc miracle2k added

comment:7 Changed 6 years ago by ramiro

  • Triage Stage changed from Accepted to Design decision needed

This ticket has grown to encompass three things:

  1. It asks for need for documentation of the object_id_field and content_type_field options of GenericRelation.
  2. It ask for validation of this stuff at model validation time.
  3. It includes a patch for a revamped implementation of the feature.

Item 1 was dealt with in #10273 and #10303. I don't know which if any of the remaining two items are valid and so worth keeping as an open ticket.

comment:8 Changed 4 years ago by gabrielhurley

  • Component changed from Contrib apps to contrib.contenttypes

comment:9 Changed 4 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:10 Changed 4 years ago by Alex

  • Easy pickings unset
  • Resolution set to fixed
  • Status changed from assigned to closed
  • UI/UX unset

Discussion with Jacob: the original issue, lack of documentation for object_id_field and content_type_field has been fixed, so this is being closed. Jacob has opened a new ticket, #16867, to track the validation of these fields.

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