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

[panta@…]

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

[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.

[Carl Meyer]

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.

[Marcos Moyano]

patch proposal

[Ramiro Morales]

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.

[Ramiro Morales]

Fixed in r14563.

[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

[Ramiro Morales]

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.

[Ramiro Morales]

In [16493]:

(The changeset message doesn't reference this ticket)

[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.

[Malcolm Tredinnick]

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

[Carl Meyer]

This blocks on #16905 (extensible validation).

[Russell Keith-Magee <russell@…>]

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.