Opened 7 years ago

Last modified 2 years ago

#15610 new Bug

Generic Foreign Keys break when used with multi-db.

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


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/ 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)


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@… 6 years ago.
Django 1.2.1 patch for ContentType multi-db support
django_1.5.1_15610.patch (1.6 KB) - added by 4glitch@… 4 years ago.
This works for me on 1.5.1
django-1.6.5-15610.patch (1.9 KB) - added by 4glitch@… 3 years ago.
The same approach for 1.6.5

Download all attachments as: .zip

Change History (21)

comment:1 Changed 7 years ago by Russell Keith-Magee

Triage Stage: UnreviewedDesign decision needed

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

comment:2 Changed 7 years ago by Gabriel Hurley

Component: Contrib appscontrib.contenttypes

comment:3 Changed 7 years ago by Luke Plant

Type: Bug

comment:4 Changed 7 years ago by Luke Plant

Severity: Normal

comment:5 Changed 6 years ago by django@…

Cc: django@… added
Easy pickings: unset

Changed 6 years ago by natalie.stepina@…

Attachment: django_contenttypes.patch added

Django 1.2.1 patch for ContentType multi-db support

comment:6 Changed 6 years ago by natalie.stepina@…

Has patch: set

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

comment:7 Changed 6 years ago by Ramiro Morales

Needs tests: set

comment:8 Changed 6 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted
UI/UX: unset

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

comment:9 Changed 5 years ago by Eric Simorre

see similar ticket #19068

comment:10 Changed 5 years ago by Andi Albrecht

Cc: albrecht.andi@… added

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

Cc: 4glitch@… added

comment:12 Changed 4 years ago by Edwin <django@…>

Cc: django@… added

Changed 4 years ago by 4glitch@…

Attachment: django_1.5.1_15610.patch added

This works for me on 1.5.1

comment:13 Changed 4 years ago by Joe <jac@…>

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


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


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.

Changed 3 years ago by 4glitch@…

Attachment: django-1.6.5-15610.patch added

The same approach for 1.6.5

comment:14 Changed 3 years ago by Claude Paroz

Thanks, but tests are still missing...

comment:15 Changed 3 years ago by socketubs

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 Changed 3 years ago by Tim Graham

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

comment:17 Changed 3 years ago by Dmitri Bogomolov <4glitch@…>

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

comment:18 Changed 2 years ago by Tim Graham

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

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