Opened 13 years ago

Closed 6 weeks ago

#16281 closed Bug (fixed)

ContentType.get_object_for_this_type using wrong database for creating object

Reported by: tfrydrychewicz@… Owned by: bcail
Component: contrib.contenttypes Version: dev
Severity: Normal Keywords: contenttype, object get_object_for_this_type, database, multiple
Cc: gklein@…, bcail Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There is a subtle error in ContentType.get_object_for_this_type method.

def get_object_for_this_type(self, **kwargs):
    return self.model_class()._default_manager.using(self._state.db).get(**kwargs)

Database used to get model_class object is taken from self._state.db, which provides an error when contenttype model is hold in one database and model, of which object we're going to create, in another one.

Database should be provided using self.model_class().objects.db not self._state.db.

Attachments (1)

16281.diff (656 bytes ) - added by tfrydrychewicz@… 13 years ago.
Patch

Download all attachments as: .zip

Change History (38)

by tfrydrychewicz@…, 13 years ago

Attachment: 16281.diff added

Patch

comment:1 by anonymous, 13 years ago

Needs tests: set

comment:2 by Stephen Burrows, 13 years ago

Triage Stage: UnreviewedAccepted
Version: 1.3SVN

comment:3 by Ramiro Morales, 13 years ago

See also #16088.

comment:4 by Dan Poirier, 12 years ago

Owner: changed from nobody to Dan Poirier
Status: newassigned

comment:5 by Dan Poirier, 12 years ago

Cc: Dan Poirier added
Resolution: invalid
Status: assignedclosed

After studying this, I'm not sure the current code is wrong. I'd want to see a use case in which Django clearly does the wrong thing to be convinced otherwise. Please re-open the ticket if there is one.

The proposed fix:

 -        return self.model_class()._base_manager.using(self._state.db).get(**kwargs)
 +        return self.model_class()._base_manager.using(self.model_class().objects.db).get(**kwargs)

seems to assume that there is exactly one database in which it is correct to look for instances of a given model, but in Django, it's not required that all instances of a model be stored in one database.

Applying the fix causes one of the multiple database tests, test_generic_key_separation(), to fail. This test puts pairs of related instances in each of the two test databases and ensures that querying for the related instances works, but after this change, is unable to find the related instance in the 2nd database correctly, probably because now get_object_for_this_type() is only looking in the default database.

The test does assume that ContentType objects exist in every database in which a model has instances, but that seems reasonable, given the requirement in Django that related objects be stored in the same database. ContentType objects don't actually have ForeignKeys, but logically they're still related to the models they represent.

The current code looks for the instance of the requested model in the same database in which the ContentType instance being used was found, which fits in with the "related instances in same database" requirement.

However, in working through all this, I found nothing in the Django documentation discussing how the content type framework and multiple databases interact, and perhaps this ticket demonstrates the need for more documentation on how to correctly use content types when multiple databases are involved.

comment:6 by Gertjan Klein <gklein@…>, 11 years ago

Resolution: invalid
Status: closedreopened

I don't understand the patch or it's implications enough to comment on it. However, I can confirm this is a valid bug in Django 1.4. I have an application where some tables reside in a mysql database; these are pre-existing tables, not managed by Django, for which I created models using inspectdb. Everything else (auth, contenttypes, ...) resides in a sqlite database. A database router manages which model is mapped to which database. This works fine everywhere.

Recently, I decided to add a get_absolute_url method to one of the models residing in the mysql database. I now have a "show on site" link in the admin for that model. Without the patch above, clicking this link causes a DatabaseError exception, because Django attempts to access the model's table in the sqlite database.

It seems to me that this should just work. Django knows the model's table resides in the mysql database, because it is editing an instance of it.

The proposed fix above does indeed fix this problem for me: the link now properly links to my site. (I haven't tried trunk, but the relevant code appears to be unchanged from 1.4, so I expect it to behave the same.)

comment:7 by Gertjan Klein <gklein@…>, 11 years ago

Cc: gklein@… added
Severity: Release blockerNormal

comment:8 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:9 by anonymous, 11 years ago

Based on poirier's earlier comment, would it not work to simply remove the .using(self._state.db) clause from the original code altogether? Then the DB router could do its usual job of deciding where to point the query.

in reply to:  9 ; comment:10 by Gertjan Klein <gklein@…>, 11 years ago

Replying to anonymous:

would it not work to simply remove the .using(self._state.db) clause from the original code altogether?

I can confirm that works in my situation (described above in [comment 6 comment:6]).

comment:11 by Dan Poirier, 11 years ago

Cc: Dan Poirier removed

in reply to:  10 comment:12 by anonymous, 11 years ago

Replying to Gertjan Klein <gklein@…>:

Replying to anonymous:

would it not work to simply remove the .using(self._state.db) clause from the original code altogether?

I can confirm that works in my situation (described above in [comment 6 comment:6]).

This also fixes the problem in for me. However, it also causes test_generic_key_separation (regressiontests.multiple_database.tests.QueryTestCase) to fail (just like the proposed fix by poirier above). This isn't a problem in my application, but it clearly is a regression elsewhere.

This change allows get_object_for_this_type to continue using the instance._state.db for the regression test case, but the router-provided database for cases that don't specify a database. I don't think this change is complete, as there are almost certainly more locations that need to pass using= in order to not regress.

diff --git a/django/contrib/contenttypes/generic.py b/django/contrib/contenttypes/generic.py
index e83c83a..ee04262 100644
--- a/django/contrib/contenttypes/generic.py
+++ b/django/contrib/contenttypes/generic.py
@@ -123,7 +123,7 @@ class GenericForeignKey(object):
             if ct_id:
                 ct = self.get_content_type(id=ct_id, using=instance._state.db)
                 try:
-                    rel_obj = ct.get_object_for_this_type(pk=getattr(instance, self.fk_field))
+                    rel_obj = ct.get_object_for_this_type(using=instance._state.db, pk=getattr(instance, self.fk_field))
                 except ObjectDoesNotExist:
                     pass
             setattr(instance, self.cache_attr, rel_obj)
diff --git a/django/contrib/contenttypes/models.py b/django/contrib/contenttypes/models.py
index b658655..4c4d92b 100644
--- a/django/contrib/contenttypes/models.py
+++ b/django/contrib/contenttypes/models.py
@@ -157,20 +157,23 @@ class ContentType(models.Model):
         return models.get_model(self.app_label, self.model,
                                 only_installed=False)
 
-    def get_object_for_this_type(self, **kwargs):
+    def get_object_for_this_type(self, using=None, **kwargs):
         """
         Returns an object of this type for the keyword arguments given.
         Basically, this is a proxy around this object_type's get_object() model
         method. The ObjectNotExist exception, if thrown, will not be caught,
         so code that calls this method should catch it.
         """
-        return self.model_class()._base_manager.using(self._state.db).get(**kwargs)
+        if using:
+            return self.model_class()._base_manager.using(using).get(**kwargs)
+        else:
+            return self.model_class()._base_manager.get(**kwargs)
 
     def get_all_objects_for_this_type(self, **kwargs):
         """
         Returns all objects of this type for the keyword arguments given.
         """
-        return self.model_class()._base_manager.using(self._state.db).filter(**kwargs)
+        return self.model_class()._base_manager.filter(**kwargs)
 
     def natural_key(self):
         return (self.app_label, self.model)

comment:13 by Greg Brown, 9 years ago

I've just been bitten by this bug (django 1.7) - I have a particular app in a separate database, and now the admin's "view on site" link doesn't work because it's looking for the object in the default database. What are the chances of it being addressed?

I don't really think this is an edge case; as it stands the "view on site" link will fail for *any* object kept in a separate database, and before 1.7 the only way to remove said link is to remove the get_absolute_url method from the model.

Last edited 9 years ago by Greg Brown (previous) (diff)

comment:14 by Tim Graham, 9 years ago

It looks to me like writing a patch with tests is the next step to address this.

comment:15 by Tim Graham, 8 years ago

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

comment:16 by Tim Graham, 8 years ago

#25564 is a duplicate.

comment:17 by Tim Graham, 3 years ago

#30760 is another duplicate.

comment:18 by Mariusz Felisiak, 20 months ago

Owner: Dan Poirier removed
Status: newassigned

comment:19 by Mariusz Felisiak, 20 months ago

Status: assignednew

comment:20 by bcail, 4 months ago

Here's a similar patch that does the current query, and if it fails, falls back to the using the default router. This passes the tests if I fix the number of expected queries in one test. If we go this direction, we'd probably want to update the get_all_objects_for_this_type method as well.

diff --git a/django/contrib/contenttypes/models.py b/django/contrib/contenttypes/models.py
index 0d98ed3a4d..2cf08bc85e 100644
--- a/django/contrib/contenttypes/models.py
+++ b/django/contrib/contenttypes/models.py
@@ -2,7 +2,7 @@ from collections import defaultdict
 
 from django.apps import apps
 from django.db import models
-from django.db.models import Q
+from django.db.models import ObjectDoesNotExist, Q
 from django.utils.translation import gettext_lazy as _
 
 
@@ -181,7 +181,10 @@ class ContentType(models.Model):
         method. The ObjectNotExist exception, if thrown, will not be caught,
         so code that calls this method should catch it.
         """
-        return self.model_class()._base_manager.using(self._state.db).get(**kwargs)
+        try:
+            return self.model_class()._base_manager.using(self._state.db).get(**kwargs)
+        except ObjectDoesNotExist:
+            return self.model_class()._base_manager.get(**kwargs)
 
     def get_all_objects_for_this_type(self, **kwargs):
         """
diff --git a/tests/contenttypes_tests/test_fields.py b/tests/contenttypes_tests/test_fields.py
index 5510f34cd0..dd1edae834 100644
--- a/tests/contenttypes_tests/test_fields.py
+++ b/tests/contenttypes_tests/test_fields.py
@@ -32,7 +32,7 @@ class GenericForeignKeyTests(TestCase):
         Question.objects.all().delete()
 
         post = Post.objects.get(pk=post.pk)
-        with self.assertNumQueries(1):
+        with self.assertNumQueries(2):
             self.assertEqual(post.object_id, question_pk)
             self.assertIsNone(post.parent)
             self.assertIsNone(post.parent)

comment:21 by bcail, 4 months ago

Cc: bcail added
Needs tests: unset

Actually, the patch in comment number 12, by anonymous, seems to work quite well. I added a test and opened a PR with that change.

comment:22 by Mariusz Felisiak, 4 months ago

Owner: set to bcail
Status: newassigned

comment:23 by Mariusz Felisiak, 3 months ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:24 by bcail, 2 months ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Thanks, Mariusz - I updated docs and commented on the PR.

comment:25 by Mariusz Felisiak, 2 months ago

Needs tests: set
Patch needs improvement: set

comment:26 by bcail, 2 months ago

Needs tests: unset
Patch needs improvement: unset

comment:27 by Mariusz Felisiak, 2 months ago

Needs tests: set
Patch needs improvement: set

comment:28 by bcail, 2 months ago

Needs tests: unset
Patch needs improvement: unset

comment:29 by Mariusz Felisiak, 2 months ago

Needs tests: set

comment:30 by bcail, 2 months ago

Needs tests: unset

Hi Mariusz, I posted another comment on the PR for you to look at.

comment:31 by Mariusz Felisiak, 2 months ago

Needs tests: set

This still requires further tests, check out comments.

comment:32 by bcail, 2 months ago

Yup - could I add the additional tests after the questions are resolved?

comment:33 by bcail, 8 weeks ago

Needs tests: unset

I added a test for View on Site.

For ticket 15610, I added a comment with a question on that issue.

comment:34 by Mariusz Felisiak, 7 weeks ago

Needs tests: set

comment:35 by bcail, 6 weeks ago

Needs tests: unset

I updated the tests.

comment:36 by Mariusz Felisiak, 6 weeks ago

Triage Stage: AcceptedReady for checkin

comment:37 by Mariusz Felisiak <felisiak.mariusz@…>, 6 weeks ago

Resolution: fixed
Status: assignedclosed

In 02a600f:

Fixed #16281 -- Fixed ContentType.get_object_for_this_type() in a multiple database setup.

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