Opened 9 years ago

Closed 20 months 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: marcosmoyano
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

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 marcosmoyano 5 years ago.
patch proposal
ticket_30500_2.patch (3.4 KB) - added by marcosmoyano 5 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 8 years ago by jason

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 8 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 5 years ago by carljm

  • Summary changed from Deleting an object with two generic relations to different but related models (via a ForeignKey) apparently generates incorrect SQL to GenericRelation 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 5 years ago by marcosmoyano

  • Owner changed from nobody to marcosmoyano
  • Status changed from new to assigned

Changed 5 years ago by marcosmoyano

patch proposal

Changed 5 years ago by marcosmoyano

comment:5 Changed 5 years ago by marcosmoyano

  • Triage Stage changed from Accepted to Ready for checkin

comment:6 Changed 5 years ago by ramiro

  • 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 5 years ago by ramiro

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

Fixed in r14563.

comment:8 Changed 5 years ago by ramiro

(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 4 years ago by ramiro

  • 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 4 years ago by ramiro

In [16493]:

(The changeset message doesn't reference this ticket)

comment:11 Changed 4 years ago by ramiro

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:12 Changed 4 years ago by ramiro

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 4 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Accepted

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

comment:14 Changed 4 years ago by carljm

This blocks on #16905 (extensible validation).

comment:15 Changed 4 years ago by lrekucki

  • Patch needs improvement set

comment:16 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:17 Changed 20 months ago by Russell Keith-Magee <russell@…>

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

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