From 50555ec73e8fdd978ca755eadef948321d2ef73c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20W=C3=B3jcik?= Date: Wed, 3 Jul 2019 11:07:55 +0200 Subject: [PATCH] Better management of the automatic "_cls" field (#2112) * Rename BaseQuerySet._initial_query to BaseQuerySet._cls_query This new name more accurately reflects the purpose of the dict. It is either empty for documents that don't use inheritance or it contains a `{"_cls": ...}` shape query. There was nothing "initial" about it per se. * Drop read_preference as a kwarg on BaseQuerySet.__call__/filter It was a poor design choice to offer two different ways to do the same thing: 1. `SomeDoc.objects(foo=bar, bar=baz).read_preference(...)` 2. `SomeDoc.objects(foo=bar, bar=baz, read_preference=...)` Option 1 is good because it's immediately obvious which part defines the query to be used and which part defines the read preference. Option 2 is bad because you don't immediately know whether `read_preference` is a query kwarg or a reserved keyword with some special behavior. If you wanted to be particularly cruel, you could even write `SomeDoc.objects(foo=bar, read_preference=..., bar=baz)`. THIS IS A BREAKING CHANGE. From now on you have to use the `BaseQuerySet.read_preference(...)` method. * Add a BaseQuerySet.clear_cls_query method + get rid of the class_check kwarg This is similar to what the previous commit did to read preference except that in this case we were still missing a `BaseQuerySet` method for clearing the `_cls` query. Now, instead of the undocumented, untested, and confusing interface: `ParentDoc.objects(foo=bar, bar=baz, class_check=False)` We do: `ParentDoc.objects(foo=bar, bar=baz).clear_cls_query()` --- docs/changelog.rst | 4 +++ mongoengine/queryset/base.py | 57 ++++++++++++++++---------------- tests/queryset/queryset.py | 63 +++++++++++++++++++++++++++--------- 3 files changed, 80 insertions(+), 44 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index a78c9e13..6512c1d3 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -6,6 +6,10 @@ Changelog Development =========== - (Fill this out as you fix issues and develop your features). +- BREAKING CHANGE: `class_check` and `read_preference` keyword arguments are no longer available when filtering a `QuerySet`. #2112 + - Instead of `Doc.objects(foo=bar, read_preference=...)` use `Doc.objects(foo=bar).read_preference(...)`. + - Instead of `Doc.objects(foo=bar, class_check=False)` use `Doc.objects(foo=bar).clear_cls_query(...)`. + - This change also renames the private `QuerySet._initial_query` attribute to `_cls_query`. - BREAKING CHANGE: Removed the deprecated `format` param from `QuerySet.explain` #2113 - BREAKING CHANGE: Renamed `MongoEngineConnectionError` to `ConnectionFailure` #2111 - If you catch/use `MongoEngineConnectionError` in your code, you'll have to rename it. diff --git a/mongoengine/queryset/base.py b/mongoengine/queryset/base.py index 42c4b927..ba3ac95a 100644 --- a/mongoengine/queryset/base.py +++ b/mongoengine/queryset/base.py @@ -53,13 +53,12 @@ class BaseQuerySet(object): self._collection_obj = collection self._mongo_query = None self._query_obj = Q() - self._initial_query = {} + self._cls_query = {} self._where_clause = None self._loaded_fields = QueryFieldList() self._ordering = None self._snapshot = False self._timeout = True - self._class_check = True self._slave_okay = False self._read_preference = None self._iter = False @@ -72,9 +71,9 @@ class BaseQuerySet(object): # subclasses of the class being used if document._meta.get("allow_inheritance") is True: if len(self._document._subclasses) == 1: - self._initial_query = {"_cls": self._document._subclasses[0]} + self._cls_query = {"_cls": self._document._subclasses[0]} else: - self._initial_query = {"_cls": {"$in": self._document._subclasses}} + self._cls_query = {"_cls": {"$in": self._document._subclasses}} self._loaded_fields = QueryFieldList(always_include=["_cls"]) self._cursor_obj = None @@ -86,23 +85,19 @@ class BaseQuerySet(object): self._max_time_ms = None self._comment = None - def __call__(self, q_obj=None, class_check=True, read_preference=None, **query): + def __call__(self, q_obj=None, **query): """Filter the selected documents by calling the :class:`~mongoengine.queryset.QuerySet` with a query. :param q_obj: a :class:`~mongoengine.queryset.Q` object to be used in the query; the :class:`~mongoengine.queryset.QuerySet` is filtered multiple times with different :class:`~mongoengine.queryset.Q` - objects, only the last one will be used - :param class_check: If set to False bypass class name check when - querying collection - :param read_preference: if set, overrides connection-level - read_preference from `ReplicaSetConnection`. - :param query: Django-style query keyword arguments + objects, only the last one will be used. + :param query: Django-style query keyword arguments. """ query = Q(**query) if q_obj: - # make sure proper query object is passed + # Make sure proper query object is passed. if not isinstance(q_obj, QNode): msg = ( "Not a query object: %s. " @@ -111,16 +106,10 @@ class BaseQuerySet(object): raise InvalidQueryError(msg) query &= q_obj - if read_preference is None: - queryset = self.clone() - else: - # Use the clone provided when setting read_preference - queryset = self.read_preference(read_preference) - + queryset = self.clone() queryset._query_obj &= query queryset._mongo_query = None queryset._cursor_obj = None - queryset._class_check = class_check return queryset @@ -222,8 +211,7 @@ class BaseQuerySet(object): return self.__call__() def filter(self, *q_objs, **query): - """An alias of :meth:`~mongoengine.queryset.QuerySet.__call__` - """ + """An alias of :meth:`~mongoengine.queryset.QuerySet.__call__`""" return self.__call__(*q_objs, **query) def search_text(self, text, language=None): @@ -743,7 +731,7 @@ class BaseQuerySet(object): Do NOT return any inherited documents. """ if self._document._meta.get("allow_inheritance") is True: - self._initial_query = {"_cls": self._document._class_name} + self._cls_query = {"_cls": self._document._class_name} return self @@ -777,7 +765,7 @@ class BaseQuerySet(object): copy_props = ( "_mongo_query", - "_initial_query", + "_cls_query", "_none", "_query_obj", "_where_clause", @@ -785,7 +773,6 @@ class BaseQuerySet(object): "_ordering", "_snapshot", "_timeout", - "_class_check", "_slave_okay", "_read_preference", "_iter", @@ -1100,6 +1087,20 @@ class BaseQuerySet(object): return queryset + def clear_cls_query(self): + """Clear the default "_cls" query. + + By default, all queries generated for documents that allow inheritance + include an extra "_cls" clause. In most cases this is desirable, but + sometimes you might achieve better performance if you clear that + default query. + + Scan the code for `_cls_query` to get more details. + """ + queryset = self.clone() + queryset._cls_query = {} + return queryset + def comment(self, text): """Add a comment to the query. @@ -1651,13 +1652,11 @@ class BaseQuerySet(object): def _query(self): if self._mongo_query is None: self._mongo_query = self._query_obj.to_query(self._document) - if self._class_check and self._initial_query: + if self._cls_query: if "_cls" in self._mongo_query: - self._mongo_query = { - "$and": [self._initial_query, self._mongo_query] - } + self._mongo_query = {"$and": [self._cls_query, self._mongo_query]} else: - self._mongo_query.update(self._initial_query) + self._mongo_query.update(self._cls_query) return self._mongo_query @property diff --git a/tests/queryset/queryset.py b/tests/queryset/queryset.py index 21f35012..9dc68f2e 100644 --- a/tests/queryset/queryset.py +++ b/tests/queryset/queryset.py @@ -4588,6 +4588,44 @@ class QuerySetTest(unittest.TestCase): doc.save() self.assertEqual(MyDoc.objects.only("test__47").get().test["47"], 1) + def test_clear_cls_query(self): + class Parent(Document): + name = StringField() + meta = {"allow_inheritance": True} + + class Child(Parent): + age = IntField() + + Parent.drop_collection() + + # Default query includes the "_cls" check. + self.assertEqual( + Parent.objects._query, {"_cls": {"$in": ("Parent", "Parent.Child")}} + ) + + # Clearing the "_cls" query should work. + self.assertEqual(Parent.objects.clear_cls_query()._query, {}) + + # Clearing the "_cls" query should not persist across queryset instances. + self.assertEqual( + Parent.objects._query, {"_cls": {"$in": ("Parent", "Parent.Child")}} + ) + + # The rest of the query should not be cleared. + self.assertEqual( + Parent.objects.filter(name="xyz").clear_cls_query()._query, {"name": "xyz"} + ) + + Parent.objects.create(name="foo") + Child.objects.create(name="bar", age=1) + self.assertEqual(Parent.objects.clear_cls_query().count(), 2) + self.assertEqual(Parent.objects.count(), 2) + self.assertEqual(Child.objects().count(), 1) + + # XXX This isn't really how you'd want to use `clear_cls_query()`, but + # it's a decent test to validate its behavior nonetheless. + self.assertEqual(Child.objects.clear_cls_query().count(), 2) + def test_read_preference(self): class Bar(Document): txt = StringField() @@ -4595,40 +4633,35 @@ class QuerySetTest(unittest.TestCase): meta = {"indexes": ["txt"]} Bar.drop_collection() - bars = list(Bar.objects(read_preference=ReadPreference.PRIMARY)) - self.assertEqual([], bars) + bar = Bar.objects.create(txt="xyz") - self.assertRaises(TypeError, Bar.objects, read_preference="Primary") + bars = list(Bar.objects.read_preference(ReadPreference.PRIMARY)) + self.assertEqual(bars, [bar]) - # read_preference as a kwarg - bars = Bar.objects(read_preference=ReadPreference.SECONDARY_PREFERRED) - self.assertEqual(bars._read_preference, ReadPreference.SECONDARY_PREFERRED) - self.assertEqual( - bars._cursor._Cursor__read_preference, ReadPreference.SECONDARY_PREFERRED - ) - - # read_preference as a query set method bars = Bar.objects.read_preference(ReadPreference.SECONDARY_PREFERRED) self.assertEqual(bars._read_preference, ReadPreference.SECONDARY_PREFERRED) self.assertEqual( bars._cursor._Cursor__read_preference, ReadPreference.SECONDARY_PREFERRED ) - # read_preference after skip + # Make sure that `.read_preference(...)` does accept string values. + self.assertRaises(TypeError, Bar.objects.read_preference, "Primary") + + # Make sure read preference is respected after a `.skip(...)`. bars = Bar.objects.skip(1).read_preference(ReadPreference.SECONDARY_PREFERRED) self.assertEqual(bars._read_preference, ReadPreference.SECONDARY_PREFERRED) self.assertEqual( bars._cursor._Cursor__read_preference, ReadPreference.SECONDARY_PREFERRED ) - # read_preference after limit + # Make sure read preference is respected after a `.limit(...)`. bars = Bar.objects.limit(1).read_preference(ReadPreference.SECONDARY_PREFERRED) self.assertEqual(bars._read_preference, ReadPreference.SECONDARY_PREFERRED) self.assertEqual( bars._cursor._Cursor__read_preference, ReadPreference.SECONDARY_PREFERRED ) - # read_preference after order_by + # Make sure read preference is respected after an `.order_by(...)`. bars = Bar.objects.order_by("txt").read_preference( ReadPreference.SECONDARY_PREFERRED ) @@ -4637,7 +4670,7 @@ class QuerySetTest(unittest.TestCase): bars._cursor._Cursor__read_preference, ReadPreference.SECONDARY_PREFERRED ) - # read_preference after hint + # Make sure read preference is respected after a `.hint(...)`. bars = Bar.objects.hint([("txt", 1)]).read_preference( ReadPreference.SECONDARY_PREFERRED )