diff --git a/google/cloud/firestore_v1/collection.py b/google/cloud/firestore_v1/collection.py index 27c3eeaa31..f6e5ffa146 100644 --- a/google/cloud/firestore_v1/collection.py +++ b/google/cloud/firestore_v1/collection.py @@ -14,7 +14,6 @@ """Classes for representing collections for the Google Cloud Firestore API.""" import random -import warnings import six @@ -257,6 +256,11 @@ def order_by(self, field_path, **kwargs): def limit(self, count): """Create a limited query with this collection as parent. + .. note:: + + `limit` and `limit_to_last` are mutually exclusive. + Setting `limit` will drop previously set `limit_to_last`. + See :meth:`~google.cloud.firestore_v1.query.Query.limit` for more information on this method. @@ -272,6 +276,29 @@ def limit(self, count): query = query_mod.Query(self) return query.limit(count) + def limit_to_last(self, count): + """Create a limited to last query with this collection as parent. + + .. note:: + + `limit` and `limit_to_last` are mutually exclusive. + Setting `limit_to_last` will drop previously set `limit`. + + See + :meth:`~google.cloud.firestore_v1.query.Query.limit_to_last` + for more information on this method. + + Args: + count (int): Maximum number of documents to return that + match the query. + + Returns: + :class:`~google.cloud.firestore_v1.query.Query`: + A limited to last query. + """ + query = query_mod.Query(self) + return query.limit_to_last(count) + def offset(self, num_to_skip): """Skip to an offset in a query with this collection as parent. @@ -375,13 +402,25 @@ def end_at(self, document_fields): return query.end_at(document_fields) def get(self, transaction=None): - """Deprecated alias for :meth:`stream`.""" - warnings.warn( - "'Collection.get' is deprecated: please use 'Collection.stream' instead.", - DeprecationWarning, - stacklevel=2, - ) - return self.stream(transaction=transaction) + """Read the documents in this collection. + + This sends a ``RunQuery`` RPC and returns a list of documents + returned in the stream of ``RunQueryResponse`` messages. + + Args: + transaction + (Optional[:class:`~google.cloud.firestore_v1.transaction.Transaction`]): + An existing transaction that this query will run in. + + If a ``transaction`` is used and it already has write operations + added, this method cannot be used (i.e. read-after-write is not + allowed). + + Returns: + list: The documents in this collection that match the query. + """ + query = query_mod.Query(self) + return query.get(transaction=transaction) def stream(self, transaction=None): """Read the documents in this collection. diff --git a/google/cloud/firestore_v1/query.py b/google/cloud/firestore_v1/query.py index 6a6326c903..1b729a2081 100644 --- a/google/cloud/firestore_v1/query.py +++ b/google/cloud/firestore_v1/query.py @@ -20,7 +20,6 @@ """ import copy import math -import warnings from google.protobuf import wrappers_pb2 import six @@ -86,6 +85,8 @@ class Query(object): The "order by" entries to use in the query. limit (Optional[int]): The maximum number of documents the query is allowed to return. + limit_to_last (Optional[bool]): + Denotes whether a provided limit is applied to the end of the result set. offset (Optional[int]): The number of results to skip. start_at (Optional[Tuple[dict, bool]]): @@ -134,6 +135,7 @@ def __init__( field_filters=(), orders=(), limit=None, + limit_to_last=False, offset=None, start_at=None, end_at=None, @@ -144,6 +146,7 @@ def __init__( self._field_filters = field_filters self._orders = orders self._limit = limit + self._limit_to_last = limit_to_last self._offset = offset self._start_at = start_at self._end_at = end_at @@ -158,6 +161,7 @@ def __eq__(self, other): and self._field_filters == other._field_filters and self._orders == other._orders and self._limit == other._limit + and self._limit_to_last == other._limit_to_last and self._offset == other._offset and self._start_at == other._start_at and self._end_at == other._end_at @@ -212,6 +216,7 @@ def select(self, field_paths): field_filters=self._field_filters, orders=self._orders, limit=self._limit, + limit_to_last=self._limit_to_last, offset=self._offset, start_at=self._start_at, end_at=self._end_at, @@ -281,6 +286,7 @@ def where(self, field_path, op_string, value): field_filters=new_filters, orders=self._orders, limit=self._limit, + limit_to_last=self._limit_to_last, offset=self._offset, start_at=self._start_at, end_at=self._end_at, @@ -333,6 +339,7 @@ def order_by(self, field_path, direction=ASCENDING): field_filters=self._field_filters, orders=new_orders, limit=self._limit, + limit_to_last=self._limit_to_last, offset=self._offset, start_at=self._start_at, end_at=self._end_at, @@ -340,9 +347,14 @@ def order_by(self, field_path, direction=ASCENDING): ) def limit(self, count): - """Limit a query to return a fixed number of results. + """Limit a query to return at most `count` matching results. - If the current query already has a limit set, this will overwrite it. + If the current query already has a `limit` set, this will override it. + + .. note:: + + `limit` and `limit_to_last` are mutually exclusive. + Setting `limit` will drop previously set `limit_to_last`. Args: count (int): Maximum number of documents to return that match @@ -359,6 +371,40 @@ def limit(self, count): field_filters=self._field_filters, orders=self._orders, limit=count, + limit_to_last=False, + offset=self._offset, + start_at=self._start_at, + end_at=self._end_at, + all_descendants=self._all_descendants, + ) + + def limit_to_last(self, count): + """Limit a query to return the last `count` matching results. + + If the current query already has a `limit_to_last` + set, this will override it. + + .. note:: + + `limit` and `limit_to_last` are mutually exclusive. + Setting `limit_to_last` will drop previously set `limit`. + + Args: + count (int): Maximum number of documents to return that match + the query. + + Returns: + :class:`~google.cloud.firestore_v1.query.Query`: + A limited query. Acts as a copy of the current query, modified + with the newly added "limit" filter. + """ + return self.__class__( + self._parent, + projection=self._projection, + field_filters=self._field_filters, + orders=self._orders, + limit=count, + limit_to_last=True, offset=self._offset, start_at=self._start_at, end_at=self._end_at, @@ -386,13 +432,14 @@ def offset(self, num_to_skip): field_filters=self._field_filters, orders=self._orders, limit=self._limit, + limit_to_last=self._limit_to_last, offset=num_to_skip, start_at=self._start_at, end_at=self._end_at, all_descendants=self._all_descendants, ) - def _check_snapshot(self, document_fields): + def _check_snapshot(self, document_snapshot): """Validate local snapshots for non-collection-group queries. Raises: @@ -402,26 +449,26 @@ def _check_snapshot(self, document_fields): if self._all_descendants: return - if document_fields.reference._path[:-1] != self._parent._path: + if document_snapshot.reference._path[:-1] != self._parent._path: raise ValueError("Cannot use snapshot from another collection as a cursor.") - def _cursor_helper(self, document_fields, before, start): + def _cursor_helper(self, document_fields_or_snapshot, before, start): """Set values to be used for a ``start_at`` or ``end_at`` cursor. The values will later be used in a query protobuf. - When the query is sent to the server, the ``document_fields`` will + When the query is sent to the server, the ``document_fields_or_snapshot`` will be used in the order given by fields set by :meth:`~google.cloud.firestore_v1.query.Query.order_by`. Args: - document_fields + document_fields_or_snapshot (Union[:class:`~google.cloud.firestore_v1.document.DocumentSnapshot`, dict, list, tuple]): a document snapshot or a dictionary/list/tuple of fields representing a query results cursor. A cursor is a collection of values that represent a position in a query result set. before (bool): Flag indicating if the document in - ``document_fields`` should (:data:`False`) or + ``document_fields_or_snapshot`` should (:data:`False`) or shouldn't (:data:`True`) be included in the result set. start (Optional[bool]): determines if the cursor is a ``start_at`` cursor (:data:`True`) or an ``end_at`` cursor (:data:`False`). @@ -431,15 +478,15 @@ def _cursor_helper(self, document_fields, before, start): A query with cursor. Acts as a copy of the current query, modified with the newly added "start at" cursor. """ - if isinstance(document_fields, tuple): - document_fields = list(document_fields) - elif isinstance(document_fields, document.DocumentSnapshot): - self._check_snapshot(document_fields) + if isinstance(document_fields_or_snapshot, tuple): + document_fields_or_snapshot = list(document_fields_or_snapshot) + elif isinstance(document_fields_or_snapshot, document.DocumentSnapshot): + self._check_snapshot(document_fields_or_snapshot) else: # NOTE: We copy so that the caller can't modify after calling. - document_fields = copy.deepcopy(document_fields) + document_fields_or_snapshot = copy.deepcopy(document_fields_or_snapshot) - cursor_pair = document_fields, before + cursor_pair = document_fields_or_snapshot, before query_kwargs = { "projection": self._projection, "field_filters": self._field_filters, @@ -457,23 +504,23 @@ def _cursor_helper(self, document_fields, before, start): return self.__class__(self._parent, **query_kwargs) - def start_at(self, document_fields): + def start_at(self, document_fields_or_snapshot): """Start query results at a particular document value. The result set will **include** the document specified by - ``document_fields``. + ``document_fields_or_snapshot``. If the current query already has specified a start cursor -- either via this method or :meth:`~google.cloud.firestore_v1.query.Query.start_after` -- this will overwrite it. - When the query is sent to the server, the ``document_fields`` will + When the query is sent to the server, the ``document_fields_or_snapshot`` will be used in the order given by fields set by :meth:`~google.cloud.firestore_v1.query.Query.order_by`. Args: - document_fields + document_fields_or_snapshot (Union[:class:`~google.cloud.firestore_v1.document.DocumentSnapshot`, dict, list, tuple]): a document snapshot or a dictionary/list/tuple of fields representing a query results cursor. A cursor is a collection @@ -485,25 +532,25 @@ def start_at(self, document_fields): a copy of the current query, modified with the newly added "start at" cursor. """ - return self._cursor_helper(document_fields, before=True, start=True) + return self._cursor_helper(document_fields_or_snapshot, before=True, start=True) - def start_after(self, document_fields): + def start_after(self, document_fields_or_snapshot): """Start query results after a particular document value. The result set will **exclude** the document specified by - ``document_fields``. + ``document_fields_or_snapshot``. If the current query already has specified a start cursor -- either via this method or :meth:`~google.cloud.firestore_v1.query.Query.start_at` -- this will overwrite it. - When the query is sent to the server, the ``document_fields`` will + When the query is sent to the server, the ``document_fields_or_snapshot`` will be used in the order given by fields set by :meth:`~google.cloud.firestore_v1.query.Query.order_by`. Args: - document_fields + document_fields_or_snapshot (Union[:class:`~google.cloud.firestore_v1.document.DocumentSnapshot`, dict, list, tuple]): a document snapshot or a dictionary/list/tuple of fields representing a query results cursor. A cursor is a collection @@ -514,25 +561,27 @@ def start_after(self, document_fields): A query with cursor. Acts as a copy of the current query, modified with the newly added "start after" cursor. """ - return self._cursor_helper(document_fields, before=False, start=True) + return self._cursor_helper( + document_fields_or_snapshot, before=False, start=True + ) - def end_before(self, document_fields): + def end_before(self, document_fields_or_snapshot): """End query results before a particular document value. The result set will **exclude** the document specified by - ``document_fields``. + ``document_fields_or_snapshot``. If the current query already has specified an end cursor -- either via this method or :meth:`~google.cloud.firestore_v1.query.Query.end_at` -- this will overwrite it. - When the query is sent to the server, the ``document_fields`` will + When the query is sent to the server, the ``document_fields_or_snapshot`` will be used in the order given by fields set by :meth:`~google.cloud.firestore_v1.query.Query.order_by`. Args: - document_fields + document_fields_or_snapshot (Union[:class:`~google.cloud.firestore_v1.document.DocumentSnapshot`, dict, list, tuple]): a document snapshot or a dictionary/list/tuple of fields representing a query results cursor. A cursor is a collection @@ -543,25 +592,27 @@ def end_before(self, document_fields): A query with cursor. Acts as a copy of the current query, modified with the newly added "end before" cursor. """ - return self._cursor_helper(document_fields, before=True, start=False) + return self._cursor_helper( + document_fields_or_snapshot, before=True, start=False + ) - def end_at(self, document_fields): + def end_at(self, document_fields_or_snapshot): """End query results at a particular document value. The result set will **include** the document specified by - ``document_fields``. + ``document_fields_or_snapshot``. If the current query already has specified an end cursor -- either via this method or :meth:`~google.cloud.firestore_v1.query.Query.end_before` -- this will overwrite it. - When the query is sent to the server, the ``document_fields`` will + When the query is sent to the server, the ``document_fields_or_snapshot`` will be used in the order given by fields set by :meth:`~google.cloud.firestore_v1.query.Query.order_by`. Args: - document_fields + document_fields_or_snapshot (Union[:class:`~google.cloud.firestore_v1.document.DocumentSnapshot`, dict, list, tuple]): a document snapshot or a dictionary/list/tuple of fields representing a query results cursor. A cursor is a collection @@ -572,7 +623,9 @@ def end_at(self, document_fields): A query with cursor. Acts as a copy of the current query, modified with the newly added "end at" cursor. """ - return self._cursor_helper(document_fields, before=False, start=False) + return self._cursor_helper( + document_fields_or_snapshot, before=False, start=False + ) def _filters_pb(self): """Convert all the filters into a single generic Filter protobuf. @@ -729,13 +782,42 @@ def _to_protobuf(self): return query_pb2.StructuredQuery(**query_kwargs) def get(self, transaction=None): - """Deprecated alias for :meth:`stream`.""" - warnings.warn( - "'Query.get' is deprecated: please use 'Query.stream' instead.", - DeprecationWarning, - stacklevel=2, - ) - return self.stream(transaction=transaction) + """Read the documents in the collection that match this query. + + This sends a ``RunQuery`` RPC and returns a list of documents + returned in the stream of ``RunQueryResponse`` messages. + + Args: + transaction + (Optional[:class:`~google.cloud.firestore_v1.transaction.Transaction`]): + An existing transaction that this query will run in. + + If a ``transaction`` is used and it already has write operations + added, this method cannot be used (i.e. read-after-write is not + allowed). + + Returns: + list: The documents in the collection that match this query. + """ + is_limited_to_last = self._limit_to_last + + if self._limit_to_last: + # In order to fetch up to `self._limit` results from the end of the + # query flip the defined ordering on the query to start from the + # end, retrieving up to `self._limit` results from the backend. + for order in self._orders: + order.direction = _enum_from_direction( + self.DESCENDING + if order.direction == self.ASCENDING + else self.ASCENDING + ) + self._limit_to_last = False + + result = self.stream(transaction=transaction) + if is_limited_to_last: + result = reversed(list(result)) + + return list(result) def stream(self, transaction=None): """Read the documents in the collection that match this query. @@ -764,6 +846,11 @@ def stream(self, transaction=None): :class:`~google.cloud.firestore_v1.document.DocumentSnapshot`: The next document that fulfills the query. """ + if self._limit_to_last: + raise ValueError( + "Query results for queries that include limit_to_last() " + "constraints cannot be streamed. Use Query.get() instead." + ) parent_path, expected_prefix = self._parent._parent_info() response_iterator = self._client._firestore_api.run_query( parent_path, diff --git a/tests/system/test_system.py b/tests/system/test_system.py index 71ac07fcee..5a5184aca5 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -30,7 +30,9 @@ from google.cloud._helpers import _pb_timestamp_to_datetime from google.cloud._helpers import UTC from google.cloud import firestore_v1 as firestore +from google.cloud.firestore_v1.client import _FIRESTORE_EMULATOR_HOST from test_utils.system import unique_resource_id +from test_utils.system import EmulatorCreds from time import sleep @@ -41,11 +43,19 @@ DOCUMENT_EXISTS = "Document already exists: " UNIQUE_RESOURCE_ID = unique_resource_id("-") +FIRESTORE_EMULATOR = os.getenv(_FIRESTORE_EMULATOR_HOST) is not None + @pytest.fixture(scope=u"module") def client(): - credentials = service_account.Credentials.from_service_account_file(FIRESTORE_CREDS) - project = FIRESTORE_PROJECT or credentials.project_id + if FIRESTORE_EMULATOR: + credentials = EmulatorCreds() + project = FIRESTORE_PROJECT + else: + credentials = service_account.Credentials.from_service_account_file( + FIRESTORE_CREDS + ) + project = FIRESTORE_PROJECT or credentials.project_id yield firestore.Client(project=project, credentials=credentials) @@ -126,6 +136,7 @@ def test_create_document_w_subcollection(client, cleanup): assert sorted(child.id for child in children) == sorted(child_ids) +@pytest.mark.skipif(FIRESTORE_EMULATOR, reason="Internal Issue b/137866686") def test_cannot_use_foreign_key(client, cleanup): document_id = "cannot" + UNIQUE_RESOURCE_ID document = client.document("foreign-key", document_id) @@ -280,6 +291,7 @@ def test_document_update_w_int_field(client, cleanup): assert snapshot1.to_dict() == expected +@pytest.mark.skipif(FIRESTORE_EMULATOR, reason="Internal Issue b/137867104") def test_update_document(client, cleanup): document_id = "for-update" + UNIQUE_RESOURCE_ID document = client.document("made", document_id) @@ -861,6 +873,7 @@ def test_collection_group_queries_filters(client, cleanup): assert found == set(["cg-doc2"]) +@pytest.mark.skipif(FIRESTORE_EMULATOR, reason="Internal Issue b/137865992") def test_get_all(client, cleanup): collection_name = "get-all" + UNIQUE_RESOURCE_ID diff --git a/tests/unit/v1/test_collection.py b/tests/unit/v1/test_collection.py index fde538b9db..aaa84cbfc2 100644 --- a/tests/unit/v1/test_collection.py +++ b/tests/unit/v1/test_collection.py @@ -371,6 +371,18 @@ def test_limit(self): self.assertIs(query._parent, collection) self.assertEqual(query._limit, limit) + def test_limit_to_last(self): + from google.cloud.firestore_v1.query import Query + + LIMIT = 15 + collection = self._make_one("collection") + query = collection.limit_to_last(LIMIT) + + self.assertIsInstance(query, Query) + self.assertIs(query._parent, collection) + self.assertEqual(query._limit, LIMIT) + self.assertTrue(query._limit_to_last) + def test_offset(self): from google.cloud.firestore_v1.query import Query @@ -484,38 +496,26 @@ def test_list_documents_w_page_size(self): @mock.patch("google.cloud.firestore_v1.query.Query", autospec=True) def test_get(self, query_class): - import warnings - collection = self._make_one("collection") - with warnings.catch_warnings(record=True) as warned: - get_response = collection.get() + get_response = collection.get() query_class.assert_called_once_with(collection) query_instance = query_class.return_value - self.assertIs(get_response, query_instance.stream.return_value) - query_instance.stream.assert_called_once_with(transaction=None) - # Verify the deprecation - self.assertEqual(len(warned), 1) - self.assertIs(warned[0].category, DeprecationWarning) + self.assertIs(get_response, query_instance.get.return_value) + query_instance.get.assert_called_once_with(transaction=None) @mock.patch("google.cloud.firestore_v1.query.Query", autospec=True) def test_get_with_transaction(self, query_class): - import warnings - collection = self._make_one("collection") transaction = mock.sentinel.txn - with warnings.catch_warnings(record=True) as warned: - get_response = collection.get(transaction=transaction) + get_response = collection.get(transaction=transaction) query_class.assert_called_once_with(collection) query_instance = query_class.return_value - self.assertIs(get_response, query_instance.stream.return_value) - query_instance.stream.assert_called_once_with(transaction=transaction) - # Verify the deprecation - self.assertEqual(len(warned), 1) - self.assertIs(warned[0].category, DeprecationWarning) + self.assertIs(get_response, query_instance.get.return_value) + query_instance.get.assert_called_once_with(transaction=transaction) @mock.patch("google.cloud.firestore_v1.query.Query", autospec=True) def test_stream(self, query_class): diff --git a/tests/unit/v1/test_query.py b/tests/unit/v1/test_query.py index bdb0e922d0..7e9557f6d5 100644 --- a/tests/unit/v1/test_query.py +++ b/tests/unit/v1/test_query.py @@ -1063,8 +1063,6 @@ def test__to_protobuf_limit_only(self): self.assertEqual(structured_query_pb, expected_pb) def test_get_simple(self): - import warnings - # Create a minimal fake GAPIC. firestore_api = mock.Mock(spec=["run_query"]) @@ -1079,18 +1077,18 @@ def test_get_simple(self): _, expected_prefix = parent._parent_info() name = "{}/sleep".format(expected_prefix) data = {"snooze": 10} + response_pb = _make_query_response(name=name, data=data) + firestore_api.run_query.return_value = iter([response_pb]) # Execute the query and check the response. query = self._make_one(parent) + returned = query.get() - with warnings.catch_warnings(record=True) as warned: - get_response = query.get() - - self.assertIsInstance(get_response, types.GeneratorType) - returned = list(get_response) + self.assertIsInstance(returned, list) self.assertEqual(len(returned), 1) + snapshot = returned[0] self.assertEqual(snapshot.reference._path, ("dee", "sleep")) self.assertEqual(snapshot.to_dict(), data) @@ -1104,9 +1102,60 @@ def test_get_simple(self): metadata=client._rpc_metadata, ) - # Verify the deprecation - self.assertEqual(len(warned), 1) - self.assertIs(warned[0].category, DeprecationWarning) + def test_get_limit_to_last(self): + from google.cloud import firestore + from google.cloud.firestore_v1.query import _enum_from_direction + + # Create a minimal fake GAPIC. + firestore_api = mock.Mock(spec=["run_query"]) + + # Attach the fake GAPIC to a real client. + client = _make_client() + client._firestore_api_internal = firestore_api + + # Make a **real** collection reference as parent. + parent = client.collection("dee") + + # Add a dummy response to the minimal fake GAPIC. + _, expected_prefix = parent._parent_info() + name = "{}/sleep".format(expected_prefix) + data = {"snooze": 10} + data2 = {"snooze": 20} + + response_pb = _make_query_response(name=name, data=data) + response_pb2 = _make_query_response(name=name, data=data2) + + firestore_api.run_query.return_value = iter([response_pb2, response_pb]) + + # Execute the query and check the response. + query = self._make_one(parent) + query = query.order_by( + u"snooze", direction=firestore.Query.DESCENDING + ).limit_to_last(2) + returned = query.get() + + self.assertIsInstance(returned, list) + self.assertEqual( + query._orders[0].direction, _enum_from_direction(firestore.Query.ASCENDING) + ) + self.assertEqual(len(returned), 2) + + snapshot = returned[0] + self.assertEqual(snapshot.reference._path, ("dee", "sleep")) + self.assertEqual(snapshot.to_dict(), data) + + snapshot2 = returned[1] + self.assertEqual(snapshot2.reference._path, ("dee", "sleep")) + self.assertEqual(snapshot2.to_dict(), data2) + + # Verify the mock call. + parent_path, _ = parent._parent_info() + firestore_api.run_query.assert_called_once_with( + parent_path, + query._to_protobuf(), + transaction=None, + metadata=client._rpc_metadata, + ) def test_stream_simple(self): # Create a minimal fake GAPIC. @@ -1145,6 +1194,20 @@ def test_stream_simple(self): metadata=client._rpc_metadata, ) + def test_stream_with_limit_to_last(self): + # Attach the fake GAPIC to a real client. + client = _make_client() + # Make a **real** collection reference as parent. + parent = client.collection("dee") + # Execute the query and check the response. + query = self._make_one(parent) + query = query.limit_to_last(2) + + stream_response = query.stream() + + with self.assertRaises(ValueError): + list(stream_response) + def test_stream_with_transaction(self): # Create a minimal fake GAPIC. firestore_api = mock.Mock(spec=["run_query"])