Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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.
  • Loading branch information...
commit 9595183d03cfd0d94ae2dd506a3d2b86cf5c74a7 1 parent 12ca312
@freakboy3742 freakboy3742 authored
View
40 django/db/models/fields/related.py
@@ -159,9 +159,8 @@ def __init__(self, related):
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 @@ def __init__(self, field_with_rel):
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 get_queryset(self):
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 get_prefetch_queryset(self, instances):
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 get_queryset(self):
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
View
10 django/db/models/manager.py
@@ -68,6 +68,7 @@ def __init__(self):
self.model = None
self._inherited = False
self._db = None
+ self._hints = {}
@classmethod
def _get_queryset_methods(cls, queryset_class):
@@ -144,21 +145,22 @@ def _copy_to_model(self, model):
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
View
22 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 @@ def ordered(self):
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 @@ def _clone(self, klass=None, setup=False, **kwargs):
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 @@ def _as_sql(self, connection):
# 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 @@ def __getitem__(self, k):
@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):
"""
View
14 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
~~~~~~~~~~~~~~~~~~~~
View
241 tests/multiple_database/tests.py
@@ -1902,3 +1902,244 @@ def test_migrate_to_other_database_with_router(self):
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})
Please sign in to comment.
Something went wrong with that request. Please try again.