Opened 11 years ago

Closed 3 years ago

#3055 closed defect (fixed)

GenericRelation should squawk if pointed to a model that doesn't have a GenericForeignKey on it

Reported by: panta@… Owned by: Marcos Moyano
Component: Database layer (models, ORM) Version: master
Severity: major Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no


Originally discussed on this django-users thread.

I'm restating here the problem for future reference and for more comfort.

The problem happens when using the following structure:

class Tag(models.Model):

class TaggedObject(models.Model):
   content_type = ...
   object_id = models.PositiveIntegerField("Object ID")
   tag = models.ForeignKey(Tag)

   content_object = models.GenericForeignKey()

class UserTaggedObject(models.Model):
   user = ...
   object_tag = models.ForeignKey(TaggedObject)

class ArticleAttachment(models.Model):
   tags = models.GenericRelation(TaggedObject)
   user_tags = models.GenericRelation(UserTaggedObject)

When deleting an ArticleAttachment (with delete()), I get an OperationalError exception (1054, "Unknown column 'object_id' in 'where clause'"). Drilling down through the backtrace, the offending SQL code is in:

django_src/django/db/backends/ in execute

  12. return self.cursor.execute(sql, params) ...

Local vars
Variable        Value

params          (5L,)
self            <django.db.backends.util.CursorDebugWrapper object at 0xb6cdda0c>
sql             'DELETE FROM `tags_usertaggedobject` WHERE `object_id` IN (%s)'

which seems wrong to me, because object_id is in tags_taggedobject
and not tags_usertaggedobject.


Attachments (2)

ticket_30500.patch (2.9 KB) - added by Marcos Moyano 7 years ago.
patch proposal
ticket_30500_2.patch (3.4 KB) - added by Marcos Moyano 7 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 10 years ago by Jason Yan

I believe this is invalid. GenericRelation assumes the model has content_type and object_id fields and will not follow the foreign key for content_type/object_id. The error is because it is trying to remove UserTaggedObjects since there is a GenericRelation in ArticleAttachment to it.

comment:2 Changed 9 years ago by Jacob

Triage Stage: UnreviewedAccepted

comment:3 Changed 7 years ago by Carl Meyer

Summary: Deleting an object with two generic relations to different but related models (via a ForeignKey) apparently generates incorrect SQLGenericRelation should squawk if pointed to a model that doesn't have a GenericForeignKey on it

Jason's comment is correct. The SQL is wrong because the models are wrong. It's not legit to point a GenericRelation at a model which doesn't have a GenericForeignKey.

However, the result should not be bad SQL when you try to delete; GenericRelations should fail fast if they are invalid. Updating the summary accordingly.

comment:4 Changed 7 years ago by Marcos Moyano

Owner: changed from nobody to Marcos Moyano
Status: newassigned

Changed 7 years ago by Marcos Moyano

Attachment: ticket_30500.patch added

patch proposal

Changed 7 years ago by Marcos Moyano

Attachment: ticket_30500_2.patch added

comment:5 Changed 7 years ago by Marcos Moyano

Triage Stage: AcceptedReady for checkin

comment:6 Changed 7 years ago by Ramiro Morales

Has patch: set

Nit: consider changing slightly the validation message to '"model '%s' must have a GenericForeignKey in order to create a GenericRelation pointing to it."' or similar.

Other that that, this is RFC.

comment:7 Changed 7 years ago by Ramiro Morales

Resolution: fixed
Status: assignedclosed

Fixed in r14563.

comment:8 Changed 7 years ago by Ramiro Morales

(In [14565]) [1.2.X] Fixed #3055 -- Validate that models target of a GenericRelation have a GenericForeignKey field.

Thanks jason for diagnosing the problem and Marcos Moyano for the patch.

Backport of [14563] from trunk

comment:9 Changed 6 years ago by Ramiro Morales

Easy pickings: unset
UI/UX: unset

This change has introduced a dependancy of core on contrib apps reported by tickets #16283 and #16368.

Long term solution for validate core management command extensibility is what #8579 asks for.

comment:10 Changed 6 years ago by Ramiro Morales

In [16493]:

(The changeset message doesn't reference this ticket)

comment:11 Changed 6 years ago by Ramiro Morales

Resolution: fixed
Status: closedreopened

comment:12 Changed 6 years ago by Ramiro Morales

In [16541]:

[1.3.X] Reverted [14563] because it introduced a dependency from core on a contrib app (contenttypes). Fixes #16283, Refs #3055. Thanks TheRoSS for the report and Aymeric Augustin for finding the problem.

This caused models shipped with some contrib apps to pollute the namespace when user's apps had the same name (e.g. auth, sites), even when these contrib apps weren't installed.

This undesired loading of contrib apps happened when model validation was executed, for example when running management commands that set or inherit requires_model_validation=True:
cleanup, dumpdata, flush, loaddata, reset, runfcgi, sql, sqlall, sqlclear, sqlcustom, sqlflush, sqlindexes, sqlinitialdata, sqlreset, sqlsequencereset, syncdb, createsuperusers, ping_google, collectstatic, findstatic.

This could also cause hard to diagnose problems e.g. when performing reverse URL resolving.

Backport of [16493] from trunk.

comment:13 Changed 6 years ago by Malcolm Tredinnick

Triage Stage: Ready for checkinAccepted

Moving out of "ready for checkin" status, since the patch requires rethinking.

comment:14 Changed 6 years ago by Carl Meyer

This blocks on #16905 (extensible validation).

comment:15 Changed 5 years ago by Łukasz Rekucki

Patch needs improvement: set

comment:16 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:17 Changed 3 years ago by Russell Keith-Magee <russell@…>

Resolution: fixed
Status: newclosed

In d818e0c9b2b88276cc499974f9eee893170bf0a8:

Fixed #16905 -- Added extensible checks (nee validation) framework

This is the result of Christopher Medrela's 2013 Summer of Code project.

Thanks also to Preston Holmes, Tim Graham, Anssi Kääriäinen, Florian
Apolloner, and Alex Gaynor for review notes along the way.

Also: Fixes #8579, fixes #3055, fixes #19844.

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