From 9595183d03cfd0d94ae2dd506a3d2b86cf5c74a7 Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Mon, 30 Sep 2013 13:05:43 +0800 Subject: [PATCH] Fixed #13724: Corrected routing of write queries involving managers. Previously, if a database request spanned a related object manager, the first manager encountered would cause a request to the router, and this would bind all subsequent queries to the same database returned by the router. Unfortunately, the first router query would be performed using a read request to the router, resulting in bad routing information being used if the subsequent query was actually a write. This change defers the call to the router until the final query is acutally made. It includes a small *BACKWARDS INCOMPATIBILITY* on an edge case - see the release notes for details. Thanks to Paul Collins (@paulcollinsiii) for the excellent debugging work and patch. --- django/db/models/fields/related.py | 40 +++-- django/db/models/manager.py | 10 +- django/db/models/query.py | 22 ++- docs/releases/1.7.txt | 14 ++ tests/multiple_database/tests.py | 241 +++++++++++++++++++++++++++++ 5 files changed, 303 insertions(+), 24 deletions(-) diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 23959f6b18..6c1e96ad8c 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -159,9 +159,8 @@ class SingleRelatedObjectDescriptor(six.with_metaclass(RenameRelatedObjectDescri def is_cached(self, instance): return hasattr(instance, self.cache_name) - def get_queryset(self, **db_hints): - db = router.db_for_read(self.related.model, **db_hints) - return self.related.model._base_manager.using(db) + def get_queryset(self, **hints): + return self.related.model._base_manager.db_manager(hints=hints) def get_prefetch_queryset(self, instances): rel_obj_attr = attrgetter(self.related.field.attname) @@ -256,15 +255,14 @@ class ReverseSingleRelatedObjectDescriptor(six.with_metaclass(RenameRelatedObjec def is_cached(self, instance): return hasattr(instance, self.cache_name) - def get_queryset(self, **db_hints): - db = router.db_for_read(self.field.rel.to, **db_hints) - rel_mgr = self.field.rel.to._default_manager + def get_queryset(self, **hints): + rel_mgr = self.field.rel.to._default_manager.db_manager(hints=hints) # If the related manager indicates that it should be used for # related fields, respect that. if getattr(rel_mgr, 'use_for_related_fields', False): - return rel_mgr.using(db) + return rel_mgr else: - return QuerySet(self.field.rel.to).using(db) + return QuerySet(self.field.rel.to, hints=hints) def get_prefetch_queryset(self, instances): rel_obj_attr = self.field.get_foreign_related_value @@ -385,8 +383,12 @@ def create_foreign_related_manager(superclass, rel_field, rel_model): return self.instance._prefetched_objects_cache[rel_field.related_query_name()] except (AttributeError, KeyError): db = self._db or router.db_for_read(self.model, instance=self.instance) - qs = super(RelatedManager, self).get_queryset().using(db).filter(**self.core_filters) empty_strings_as_null = connections[db].features.interprets_empty_strings_as_nulls + qs = super(RelatedManager, self).get_queryset() + qs._add_hints(instance=self.instance) + if self._db: + qs = qs.using(self._db) + qs = qs.filter(**self.core_filters) for field in rel_field.foreign_related_fields: val = getattr(self.instance, field.attname) if val is None or (val == '' and empty_strings_as_null): @@ -398,9 +400,12 @@ def create_foreign_related_manager(superclass, rel_field, rel_model): rel_obj_attr = rel_field.get_local_related_value instance_attr = rel_field.get_foreign_related_value instances_dict = dict((instance_attr(inst), inst) for inst in instances) - db = self._db or router.db_for_read(self.model, instance=instances[0]) query = {'%s__in' % rel_field.name: instances} - qs = super(RelatedManager, self).get_queryset().using(db).filter(**query) + qs = super(RelatedManager, self).get_queryset() + qs._add_hints(instance=instances[0]) + if self._db: + qs = qs.using(self._db) + qs = qs.filter(**query) # Since we just bypassed this class' get_queryset(), we must manage # the reverse relation manually. for rel_obj in qs: @@ -545,14 +550,21 @@ def create_many_related_manager(superclass, rel): try: return self.instance._prefetched_objects_cache[self.prefetch_cache_name] except (AttributeError, KeyError): - db = self._db or router.db_for_read(self.instance.__class__, instance=self.instance) - return super(ManyRelatedManager, self).get_queryset().using(db)._next_is_sticky().filter(**self.core_filters) + qs = super(ManyRelatedManager, self).get_queryset() + qs._add_hints(instance=self.instance) + if self._db: + qs = qs.using(self._db) + return qs._next_is_sticky().filter(**self.core_filters) def get_prefetch_queryset(self, instances): instance = instances[0] db = self._db or router.db_for_read(instance.__class__, instance=instance) query = {'%s__in' % self.query_field_name: instances} - qs = super(ManyRelatedManager, self).get_queryset().using(db)._next_is_sticky().filter(**query) + qs = super(ManyRelatedManager, self).get_queryset() + qs._add_hints(instance=instance) + if self._db: + qs = qs.using(db) + qs = qs._next_is_sticky().filter(**query) # M2M: need to annotate the query in order to get the primary model # that the secondary model was actually related to. We know that diff --git a/django/db/models/manager.py b/django/db/models/manager.py index 4f16b5ebfe..48ad3db9cc 100644 --- a/django/db/models/manager.py +++ b/django/db/models/manager.py @@ -68,6 +68,7 @@ class BaseManager(six.with_metaclass(RenameManagerMethods)): self.model = None self._inherited = False self._db = None + self._hints = {} @classmethod def _get_queryset_methods(cls, queryset_class): @@ -144,21 +145,22 @@ class BaseManager(six.with_metaclass(RenameManagerMethods)): mgr._inherited = True return mgr - def db_manager(self, using): + def db_manager(self, using=None, hints=None): obj = copy.copy(self) - obj._db = using + obj._db = using or self._db + obj._hints = hints or self._hints return obj @property def db(self): - return self._db or router.db_for_read(self.model) + return self._db or router.db_for_read(self.model, **self._hints) def get_queryset(self): """ Returns a new QuerySet object. Subclasses can override this method to easily customize the behavior of the Manager. """ - return self._queryset_class(self.model, using=self._db) + return self._queryset_class(self.model, using=self._db, hints=self._hints) def all(self): # We can't proxy this method through the `QuerySet` like we do for the diff --git a/django/db/models/query.py b/django/db/models/query.py index 2da4b6595f..0aff4d89ad 100644 --- a/django/db/models/query.py +++ b/django/db/models/query.py @@ -47,9 +47,10 @@ class QuerySet(object): Represents a lazy database lookup for a set of objects. """ - def __init__(self, model=None, query=None, using=None): + def __init__(self, model=None, query=None, using=None, hints=None): self.model = model self._db = using + self._hints = hints or {} self.query = query or sql.Query(self.model) self._result_cache = None self._sticky_filter = False @@ -904,8 +905,8 @@ class QuerySet(object): def db(self): "Return the database that will be used if this query is executed now" if self._for_write: - return self._db or router.db_for_write(self.model) - return self._db or router.db_for_read(self.model) + return self._db or router.db_for_write(self.model, **self._hints) + return self._db or router.db_for_read(self.model, **self._hints) ################### # PRIVATE METHODS # @@ -955,7 +956,7 @@ class QuerySet(object): query = self.query.clone() if self._sticky_filter: query.filter_is_sticky = True - c = klass(model=self.model, query=query, using=self._db) + c = klass(model=self.model, query=query, using=self._db, hints=self._hints) c._for_write = self._for_write c._prefetch_related_lookups = self._prefetch_related_lookups[:] c._known_related_objects = self._known_related_objects @@ -1025,6 +1026,14 @@ class QuerySet(object): # empty" result. value_annotation = True + def _add_hints(self, **hints): + """ + Update hinting information for later use by Routers + """ + # If there is any hinting information, add it to what we already know. + # If we have a new hint for an existing key, overwrite with the new value. + self._hints.update(hints) + class InstanceCheckMeta(type): def __instancecheck__(self, instance): @@ -1485,10 +1494,11 @@ class RawQuerySet(object): annotated model instances. """ def __init__(self, raw_query, model=None, query=None, params=None, - translations=None, using=None): + translations=None, using=None, hints=None): self.raw_query = raw_query self.model = model self._db = using + self._hints = hints or {} self.query = query or sql.RawQuery(sql=raw_query, using=self.db, params=params) self.params = params or () self.translations = translations or {} @@ -1572,7 +1582,7 @@ class RawQuerySet(object): @property def db(self): "Return the database that will be used if this query is executed now" - return self._db or router.db_for_read(self.model) + return self._db or router.db_for_read(self.model, **self._hints) def using(self, alias): """ diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index 573ad49dc2..74fc74947c 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -368,6 +368,20 @@ For apps with migrations, ``allow_migrate`` will now get passed without custom attributes, methods or managers. Make sure your ``allow_migrate`` methods are only referring to fields or other items in ``model._meta``. +Passing ``None`` to ``Manager.db_manager()`` +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In previous versions of Django, it was possible to use +``db_manager(using=None)`` on a model manager instance to obtain a manager +instance using default routing behavior, overriding any manually specified +database routing. In Django 1.7, a value of ``None`` passed to db_manager will +produce a router that *retains* any manually assigned database routing -- the +manager will *not* be reset. This was necessary to resolve an inconsistency in +the way routing information cascaded over joins. See `Ticket #13724`_ for more +details. + +.. _Ticket #13724: https://code.djangoproject.com/ticket/13724 + pytz may be required ~~~~~~~~~~~~~~~~~~~~ diff --git a/tests/multiple_database/tests.py b/tests/multiple_database/tests.py index 8a31f5f530..859330c40d 100644 --- a/tests/multiple_database/tests.py +++ b/tests/multiple_database/tests.py @@ -1902,3 +1902,244 @@ class MigrateTestCase(TestCase): router.routers = old_routers self.assertEqual(cts.count(), 0) + + + + +class RouterUsed(Exception): + WRITE = 'write' + + def __init__(self, mode, model, hints): + self.mode = mode + self.model = model + self.hints = hints + + +class RouteForWriteTestCase(TestCase): + multi_db = True + RAISE_MSG = 'Db for write called' + + class WriteCheckRouter(object): + def db_for_write(self, model, **hints): + raise RouterUsed(mode=RouterUsed.WRITE, model=model, hints=hints) + + def setUp(self): + self._old_rtrs = router.routers + + def tearDown(self): + router.routers = self._old_rtrs + + def enable_router(self): + router.routers = [RouteForWriteTestCase.WriteCheckRouter()] + + def test_fk_delete(self): + owner = Person.objects.create(name='Someone') + pet = Pet.objects.create(name='fido', owner=owner) + self.enable_router() + try: + pet.owner.delete() + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Person) + self.assertEqual(e.hints, {'instance': owner}) + + def test_reverse_fk_delete(self): + owner = Person.objects.create(name='Someone') + to_del_qs = owner.pet_set.all() + self.enable_router() + try: + to_del_qs.delete() + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Pet) + self.assertEqual(e.hints, {'instance': owner}) + + def test_reverse_fk_get_or_create(self): + owner = Person.objects.create(name='Someone') + self.enable_router() + try: + owner.pet_set.get_or_create(name='fido') + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Pet) + self.assertEqual(e.hints, {'instance': owner}) + + def test_reverse_fk_update(self): + owner = Person.objects.create(name='Someone') + pet = Pet.objects.create(name='fido', owner=owner) + self.enable_router() + try: + owner.pet_set.update(name='max') + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Pet) + self.assertEqual(e.hints, {'instance': owner}) + + def test_m2m_add(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + self.enable_router() + try: + book.authors.add(auth) + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Book.authors.through) + self.assertEqual(e.hints, {'instance': book}) + + def test_m2m_clear(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + book.authors.add(auth) + self.enable_router() + try: + book.authors.clear() + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Book.authors.through) + self.assertEqual(e.hints, {'instance': book}) + + def test_m2m_delete(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + book.authors.add(auth) + self.enable_router() + try: + book.authors.all().delete() + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Person) + self.assertEqual(e.hints, {'instance': book}) + + def test_m2m_get_or_create(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + self.enable_router() + try: + book.authors.get_or_create(name='Someone else') + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Book) + self.assertEqual(e.hints, {'instance': book}) + + def test_m2m_remove(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + book.authors.add(auth) + self.enable_router() + self.assertRaisesMessage(AttributeError, self.RAISE_MSG, ) + try: + book.authors.remove(auth) + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Book.authors.through) + self.assertEqual(e.hints, {'instance': book}) + + def test_m2m_update(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + book.authors.add(auth) + self.enable_router() + try: + book.authors.all().update(name='Different') + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Person) + self.assertEqual(e.hints, {'instance': book}) + + def test_reverse_m2m_add(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + self.enable_router() + try: + auth.book_set.add(book) + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Book.authors.through) + self.assertEqual(e.hints, {'instance': auth}) + + def test_reverse_m2m_clear(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + book.authors.add(auth) + self.enable_router() + try: + auth.book_set.clear() + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Book.authors.through) + self.assertEqual(e.hints, {'instance': auth}) + + def test_reverse_m2m_delete(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + book.authors.add(auth) + self.enable_router() + try: + auth.book_set.all().delete() + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Book) + self.assertEqual(e.hints, {'instance': auth}) + + def test_reverse_m2m_get_or_create(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + self.enable_router() + try: + auth.book_set.get_or_create(title="New Book", published=datetime.datetime.now()) + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Person) + self.assertEqual(e.hints, {'instance': auth}) + + def test_reverse_m2m_remove(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + book.authors.add(auth) + self.enable_router() + try: + auth.book_set.remove(book) + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Book.authors.through) + self.assertEqual(e.hints, {'instance': auth}) + + def test_reverse_m2m_update(self): + auth = Person.objects.create(name='Someone') + book = Book.objects.create(title="Pro Django", + published=datetime.date(2008, 12, 16)) + book.authors.add(auth) + self.enable_router() + try: + auth.book_set.all().update(title='Different') + self.fail('db_for_write() not invoked on router') + except RouterUsed, e: + self.assertEqual(e.mode, RouterUsed.WRITE) + self.assertEqual(e.model, Book) + self.assertEqual(e.hints, {'instance': auth})