Opened 14 years ago

Last modified 13 months 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
Pull Requests:How to create a pull request

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.

According to the ticket's flags, the next step(s) to move this issue forward are:

  • To add tests to the patch, then uncheck the "Needs tests" flag on the ticket.
  • If creating a new pull request, include a link to the pull request in the ticket comment when making that update. The usual format is: [https://github.com/django/django/pull/#### PR].

Change History (22)

comment:1 by Russell Keith-Magee, 14 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, 14 years ago

Component: Contrib appscontrib.contenttypes

comment:3 by Luke Plant, 14 years ago

Type: Bug

comment:4 by Luke Plant, 14 years ago

Severity: Normal

comment:5 by django@…, 14 years ago

Cc: django@… added
Easy pickings: unset

by natalie.stepina@…, 14 years ago

Attachment: django_contenttypes.patch added

Django 1.2.1 patch for ContentType multi-db support

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

Has patch: set

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

comment:7 by Ramiro Morales, 14 years ago

Needs tests: set

comment:8 by Aymeric Augustin, 13 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, 12 years ago

see similar ticket #19068

comment:10 by Andi Albrecht, 12 years ago

Cc: albrecht.andi@… added

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

Cc: 4glitch@… added

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

Cc: django@… added

by 4glitch@…, 12 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@…, 11 years ago

Attachment: django-1.6.5-15610.patch added

The same approach for 1.6.5

comment:14 by Claude Paroz, 11 years ago

Thanks, but tests are still missing...

comment:15 by socketubs, 11 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, 11 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@…>, 11 years ago

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

comment:18 by Tim Graham, 9 years ago

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

comment:19 by bcail, 13 months 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