Opened 13 years ago

Last modified 8 weeks ago

#15610 new Bug

Generic Foreign Keys break when used with multi-db.

Reported by: legutierr Owned by: nobody
Component: contrib.contenttypes Version: dev
Severity: Normal Keywords:
Cc: django@…, albrecht.andi@…, 4glitch@…, django@…, bcail Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Specifically, retrieving and saving objects that use the GenericForeignKey descriptor fails when the ContentType table is stored in a different database than the table of the instance being retrieved or saved.

There can only be one valid and consistent content-type table in a system. In a two-db system, the content type table will be in either db A or db B. Therefore, a portion of the system's tables will be registered in a content-type table that is stored in a different database.

This is all OK when retrieving objects based on a GenericForeignKey reference, because a user in such a situation will typically rely on the underlying routers to determine which database to use in retrieving content-type records.

However, this method in django/trunk/django/contrib/contenttypes/generic.py does not use routers in determining what database to use to fetch the content-type, but instead uses the source database of the orm object the content-type of which is being queried:

52	    def get_content_type(self, obj=None, id=None, using=None):
53	        # Convenience function using get_model avoids a circular import when
54	        # using this model
55	        ContentType = get_model("contenttypes", "contenttype")
56	        if obj:
57	             return ContentType.objects.db_manager(obj._state.db).get_for_model(obj)
58	        elif id:
59	             return ContentType.objects.db_manager(using).get_for_id(id)
60	        else:
61	            # This should never happen. I love comments like this, don't you?
62	            raise Exception("Impossible arguments to GFK.get_content_type!")

The above method is what is used at object-creation time to transform a generic object in the __init__ kwargs of a new orm object into a content-type/object-id pair for purposes of saving. The use of this method results in either: (a) an error being generated when the specified table is not to be found in the designated database; or (b) incorrect information being saved when a parallel--but incorrect (according to the router rules)--content type table from the other database is used (i.e. database corruption).

A simple fix would be to not assume that the content-types table is in the same database as the object with the GenericForeignKey field. I don't have a patch right now, but it may be as simple as replacing line 57:

57	             return ContentType.objects.db_manager(obj._state.db).get_for_model(obj)

with:

57	             return ContentType.objects.db_manager(using).get_for_model(obj)

A similar error seems to exist in the following block of code as well, however:

220	        manager = RelatedManager(
221	            model = rel_model,
222	            instance = instance,
223	            symmetrical = (self.field.rel.symmetrical and instance.__class__ == rel_model),
224	            join_table = qn(self.field.m2m_db_table()),
225	            source_col_name = qn(self.field.m2m_column_name()),
226	            target_col_name = qn(self.field.m2m_reverse_name()),
227	            content_type = ContentType.objects.db_manager(instance._state.db).get_for_model(instance),
228	            content_type_field_name = self.field.content_type_field_name,
229	            object_id_field_name = self.field.object_id_field_name
230	        )

Requiring that line 227 be replaced, perhaps:

227	            content_type = ContentType.objects.get_for_model(instance),

The problem here is that no using argument is specified for the __get__ method, nor can a using argument be specified there, making the usage in that function inconsistent with the rest of the file, should this fix be put in place.

Attachments (3)

django_contenttypes.patch (1015 bytes ) - added by natalie.stepina@… 13 years ago.
Django 1.2.1 patch for ContentType multi-db support
django_1.5.1_15610.patch (1.6 KB ) - added by 4glitch@… 11 years ago.
This works for me on 1.5.1
django-1.6.5-15610.patch (1.9 KB ) - added by 4glitch@… 10 years ago.
The same approach for 1.6.5

Download all attachments as: .zip

Change History (22)

comment:1 by Russell Keith-Magee, 13 years ago

Triage Stage: UnreviewedDesign decision needed

This isn't a minor problem to fix, but it's definitely a problem.

comment:2 by Gabriel Hurley, 13 years ago

Component: Contrib appscontrib.contenttypes

comment:3 by Luke Plant, 13 years ago

Type: Bug

comment:4 by Luke Plant, 13 years ago

Severity: Normal

comment:5 by django@…, 13 years ago

Cc: django@… added
Easy pickings: unset

by natalie.stepina@…, 13 years ago

Attachment: django_contenttypes.patch added

Django 1.2.1 patch for ContentType multi-db support

comment:6 by natalie.stepina@…, 13 years ago

Has patch: set

See attached django_contenttypes.patch. Worked for us on Django 1.2.1.

comment:7 by Ramiro Morales, 13 years ago

Needs tests: set

comment:8 by Aymeric Augustin, 12 years ago

Triage Stage: Design decision neededAccepted
UI/UX: unset

Since this is obviously a problem, I'm marking the ticket as accepted.

comment:9 by Eric Simorre, 11 years ago

see similar ticket #19068

comment:10 by Andi Albrecht, 11 years ago

Cc: albrecht.andi@… added

comment:11 by Dmitri Bogomolov <4glitch@…>, 11 years ago

Cc: 4glitch@… added

comment:12 by Edwin <django@…>, 11 years ago

Cc: django@… added

by 4glitch@…, 11 years ago

Attachment: django_1.5.1_15610.patch added

This works for me on 1.5.1

comment:13 by Joe <jac@…>, 11 years ago

4glitch, I think your patch would also need to address ContentType.get_all_objects_for_this_type(). Applying the same fix that you used for ContentType.get_object_for_this_type() seems to work for me, i.e.:

Old:

173        return self.model_class()._base_manager.using(self._state.db).filter(**kwargs)

New:

173        return self.model_class()._base_manager.using(router.db_for_read
(self.model_class()) or DEFAULT_DB_ALIAS).filter(**kwargs)

That said, I haven't tested your patch or this proposed modification thoroughly yet.

by 4glitch@…, 10 years ago

Attachment: django-1.6.5-15610.patch added

The same approach for 1.6.5

comment:14 by Claude Paroz, 10 years ago

Thanks, but tests are still missing...

comment:15 by socketubs, 10 years ago

If I understand correctly why these patches are not applied, it's just because tests are missing ?
I can help for writing tests but I have never contributed to Django...

comment:16 by Tim Graham, 10 years ago

We have a lot of documentation with tips for new contributors. You may find the patch review checklist particularly helpful.

comment:17 by Dmitri Bogomolov <4glitch@…>, 10 years ago

I cannot reproduce this issue on two sqlite databases (test_sqlite). Probably need another backend.

comment:18 by Tim Graham, 8 years ago

Might be a duplicate of #16281 as the patch proposed there also modifies these methods.

comment:19 by bcail, 8 weeks ago

Cc: bcail added

The GenericForeignKey docs (https://docs.djangoproject.com/en/5.0/ref/contrib/contenttypes/#django.contrib.contenttypes.fields.GenericForeignKey) talk about having a ForeignKey to ContentType. Would that ForeignKey work in this case, though, where ContentType is in a different database?

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