Opened 17 years ago

Closed 10 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: dev
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

Description

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/util.py 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.

Marco

Attachments (2)

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

Download all attachments as: .zip

Change History (19)

comment:1 by Jason Yan, 17 years ago

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 by Jacob, 16 years ago

Triage Stage: UnreviewedAccepted

comment:3 by Carl Meyer, 13 years ago

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 by Marcos Moyano, 13 years ago

Owner: changed from nobody to Marcos Moyano
Status: newassigned

by Marcos Moyano, 13 years ago

Attachment: ticket_30500.patch added

patch proposal

by Marcos Moyano, 13 years ago

Attachment: ticket_30500_2.patch added

comment:5 by Marcos Moyano, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:6 by Ramiro Morales, 13 years ago

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 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: assignedclosed

Fixed in r14563.

comment:8 by Ramiro Morales, 13 years ago

(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 by Ramiro Morales, 13 years ago

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 by Ramiro Morales, 13 years ago

In [16493]:

(The changeset message doesn't reference this ticket)

comment:11 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: closedreopened

comment:12 by Ramiro Morales, 13 years ago

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 by Malcolm Tredinnick, 13 years ago

Triage Stage: Ready for checkinAccepted

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

comment:14 by Carl Meyer, 13 years ago

This blocks on #16905 (extensible validation).

comment:15 by Łukasz Rekucki, 12 years ago

Patch needs improvement: set

comment:16 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:17 by Russell Keith-Magee <russell@…>, 10 years ago

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