diff --git a/CHANGES.rst b/CHANGES.rst index 58fda98e0..90ca381ce 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -10,9 +10,9 @@ New ~~~~~ - ``NORMALIZE_ON_PATCH`` switches normalization on patch requests (`#1234`_) - Fixed ~~~~~ +- Document count broken with concurrent requests (`#1271`_) - If ``ignore_fields`` contains a nested field, document is mutated (`#1266`_) - Crash with Werzeug >= 0.15.3 (`#1267`_) - Fix crash when trying to ignore a nested field that doesn't exist (`#1263`_) @@ -22,6 +22,19 @@ Improved - Remove unsupported ``transparent_schema_rules`` option from docs (`#1264`_) - Bump (and pin) Wekzeug to 0.15.4 (`#1267`_) +Breaking Changes +~~~~~~~~~~~~~~~~ + +No known breaking changes for the standard framework user. However, if you are +consuming the developer API: + +- Be aware that ``io.base.DataLayer.find()`` signature has changed and an + optional ``perform_count`` argument has been added. The method return value + is now a tuple ``(cursor, count)``; ``cursor`` is the query result as + before while ``count`` is the document count, which is expected to have a + consistent value when ``perform_count = True``. + +.. _`#1271`: https://github.com/pyeve/eve/issues/1271 .. _`#1266`: https://github.com/pyeve/eve/pull/1266 .. _`#1234`: https://github.com/pyeve/eve/issues/1234 .. _`#1267`: https://github.com/pyeve/eve/issues/1267 diff --git a/eve/io/base.py b/eve/io/base.py index 26ee6077c..82bf793b4 100644 --- a/eve/io/base.py +++ b/eve/io/base.py @@ -117,7 +117,7 @@ def init_app(self, app): """ raise NotImplementedError - def find(self, resource, req, sub_resource_lookup): + def find(self, resource, req, sub_resource_lookup, perform_count=True): """ Retrieves a set of documents (rows), matching the current request. Consumed when a request hits a collection/document endpoint (`/people/`). @@ -134,6 +134,8 @@ def find(self, resource, req, sub_resource_lookup): to support with your driver. For example ``eve.io.Mongo`` supports both Python and Mongo-like query syntaxes. :param sub_resource_lookup: sub-resource lookup from the endpoint url. + :param perform_count: wether a document count should be performed and + returned to the client. .. versionchanged:: 0.3 Support for sub-resources. diff --git a/eve/io/mongo/mongo.py b/eve/io/mongo/mongo.py index 1f79eb4e6..516253528 100644 --- a/eve/io/mongo/mongo.py +++ b/eve/io/mongo/mongo.py @@ -141,7 +141,7 @@ def init_app(self, app): self.driver = PyMongos(self) self.mongo_prefix = None - def find(self, resource, req, sub_resource_lookup): + def find(self, resource, req, sub_resource_lookup, perform_count=True): """ Retrieves a set of documents matching a given request. Queries can be expressed in two different formats: the mongo query syntax, and the python syntax. The first kind of query would look like: :: @@ -258,9 +258,9 @@ def find(self, resource, req, sub_resource_lookup): if projection: args["projection"] = projection - self.__last_target = self.pymongo(resource).db[datasource], spec + target = self.pymongo(resource).db[datasource] try: - self.__last_cursor = self.pymongo(resource).db[datasource].find(**args) + result = target.find(**args) except TypeError as e: # pymongo raises ValueError when invalid query paramenters are # included. We do our best to catch them beforehand but, especially @@ -268,30 +268,27 @@ def find(self, resource, req, sub_resource_lookup): self.app.logger.exception(e) abort(400, description=debug_error_message(str(e))) - return self.__last_cursor - - @property - def last_documents_count(self): - if not self.__last_target: - return None + if perform_count: + try: + count = target.count_documents(spec) + except: + # fallback to deprecated method. this might happen when the query + # includes operators not supported by count_documents(). one + # documented use-case is when we're running on mongo 3.4 and below, + # which does not support $expr ($expr must replace $where # in + # count_documents()). + + # 1. Mongo 3.6+; $expr: pass + # 2. Mongo 3.6+; $where: pass (via fallback) + # 3. Mongo 3.4; $where: pass (via fallback) + # 4. Mongo 3.4; $expr: fail (operator not supported by db) + + # See: http://api.mongodb.com/python/current/api/pymongo/collection.html#pymongo.collection.Collection.count + count = target.count() + else: + count = None - try: - target, spec = self.__last_target - return target.count_documents(spec) - except: - # fallback to deprecated method. this might happen when the query - # includes operators not supported by count_documents(). one - # documented use-case is when we're running on mongo 3.4 and below, - # which does not support $expr ($expr must replace $where # in - # count_documents()). - - # 1. Mongo 3.6+; $expr: pass - # 2. Mongo 3.6+; $where: pass (via fallback) - # 3. Mongo 3.4; $where: pass (via fallback) - # 4. Mongo 3.4; $expr: fail (operator not supported by db) - - # See: http://api.mongodb.com/python/current/api/pymongo/collection.html#pymongo.collection.Collection.count - return self.__last_cursor.count() + return result, count def find_one( self, diff --git a/eve/methods/common.py b/eve/methods/common.py index 72f3802e2..8eb700ab7 100644 --- a/eve/methods/common.py +++ b/eve/methods/common.py @@ -849,7 +849,7 @@ def embedded_document(references, data_relation, field_name): :param data_relation: the relation schema definition. :param field_name: field name used in abort message only - .. versionadded:: 0.5 +) .. versionadded:: 0.5 """ embedded_docs = [] @@ -892,9 +892,10 @@ def embedded_document(references, data_relation, field_name): data_relation, references ) for subresource in subresources_query: - list_embedded_doc = list( - app.data.find(subresource, None, subresources_query[subresource]) + result, _ = app.data.find( + subresource, None, subresources_query[subresource] ) + list_embedded_doc = list(result) if not list_embedded_doc: embedded_docs.extend( diff --git a/eve/methods/delete.py b/eve/methods/delete.py index 3ea72bc1b..8f5c40559 100644 --- a/eve/methods/delete.py +++ b/eve/methods/delete.py @@ -217,7 +217,8 @@ def delete(resource, **lookup): # get_document should always fetch soft deleted documents from the db # callers must handle soft deleted documents default_request.show_deleted = True - originals = list(app.data.find(resource, default_request, lookup)) + result, _ = app.data.find(resource, default_request, lookup) + originals = list(result) if not originals: abort(404) # I add new callback as I want the framework to be retro-compatible diff --git a/eve/methods/get.py b/eve/methods/get.py index 83bdc1a35..96b79b1a1 100644 --- a/eve/methods/get.py +++ b/eve/methods/get.py @@ -262,7 +262,9 @@ def _perform_find(resource, lookup): # If-Modified-Since disabled on collections (#334) req.if_modified_since = None - cursor = app.data.find(resource, req, lookup) + cursor, count = app.data.find( + resource, req, lookup, perform_count=not config.OPTIMIZE_PAGINATION_FOR_SPEED + ) # If soft delete is enabled, data.find will not include items marked # deleted unless req.show_deleted is True for document in cursor: @@ -279,10 +281,7 @@ def _perform_find(resource, lookup): response[config.ITEMS] = documents - if config.OPTIMIZE_PAGINATION_FOR_SPEED: - count = None - else: - count = app.data.last_documents_count + if count: headers.append((config.HEADER_TOTAL_COUNT, count)) if config.DOMAIN[resource]["hateoas"]: @@ -454,11 +453,11 @@ def getitem_internal(resource, **lookup): # default sort for 'all', required sort for 'diffs' req.sort = '[("%s", 1)]' % config.VERSION req.if_modified_since = None # we always want the full history here - cursor = app.data.find(resource + config.VERSIONS, req, lookup) + cursor, count = app.data.find(resource + config.VERSIONS, req, lookup) # build all versions documents = [] - if app.data.last_documents_count == 0: + if count == 0: # this is the scenario when the document existed before # document versioning got turned on documents.append(latest_doc) @@ -510,7 +509,6 @@ def getitem_internal(resource, **lookup): if config.DOMAIN[resource]["hateoas"]: # use the id of the latest document for multi-document requests if cursor: - count = app.data.last_documents_count response[config.LINKS] = _pagination_links( resource, req, count, latest_doc[resource_def["id_field"]] ) diff --git a/eve/tests/methods/delete.py b/eve/tests/methods/delete.py index c301f6cc8..424858b43 100644 --- a/eve/tests/methods/delete.py +++ b/eve/tests/methods/delete.py @@ -559,24 +559,24 @@ def test_softdelete_datalayer(self): # show_deleted == True is passed or if the deleted field is part of # the lookup req.show_deleted = False - self.app.data.find(self.known_resource, req, None) - undeleted_count = self.app.data.last_documents_count + _, undeleted_count = self.app.data.find(self.known_resource, req, None) req.show_deleted = True - self.app.data.find(self.known_resource, req, None) - self.assertEqual(undeleted_count, self.app.data.last_documents_count - 1) + _, challenge = self.app.data.find(self.known_resource, req, None) + self.assertEqual(undeleted_count, challenge - 1) req.show_deleted = False - self.app.data.find(self.known_resource, req, {self.deleted_field: True}) - deleted_count = self.app.data.last_documents_count + _, deleted_count = self.app.data.find( + self.known_resource, req, {self.deleted_field: True} + ) self.assertEqual(deleted_count, 1) # find_list_of_ids will return deleted documents if given their id - self.app.data.find_list_of_ids( + ids = self.app.data.find_list_of_ids( self.known_resource, [ObjectId(self.item_id)] ) - self.assertEqual(self.app.data.last_documents_count, 1) + self.assertEqual(str(ids[0]["_id"]), self.item_id) def test_softdelete_db_fields(self): """Documents created when soft delete is enabled should include and diff --git a/eve/tests/methods/get.py b/eve/tests/methods/get.py index 4e4b2a1f0..0fa7dea16 100644 --- a/eve/tests/methods/get.py +++ b/eve/tests/methods/get.py @@ -1051,13 +1051,13 @@ def test_cursor_extra_find(self): _find = self.app.data.find hits = {"total_hits": 0} - def find(resource, req, sub_resource_lookup): + def find(resource, req, sub_resource_lookup, perform_count=True): def extra(response): response["_hits"] = hits - cursor = _find(resource, req, sub_resource_lookup) + cursor, _ = _find(resource, req, sub_resource_lookup) cursor.extra = extra - return cursor + return cursor, _ self.app.data.find = find r, status = self.get(self.known_resource)