From eb5086943a8646dbf429fc0054f41071c0adddf3 Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Thu, 8 Dec 2016 15:11:15 -0800 Subject: [PATCH 1/8] Add Query class and tests. --- .../google/cloud/firestore/collection.py | 92 ++++++++++++++++++- firestore/unit_tests/test_collection.py | 46 +++++++++- firestore/unit_tests/test_document.py | 4 +- 3 files changed, 135 insertions(+), 7 deletions(-) diff --git a/firestore/google/cloud/firestore/collection.py b/firestore/google/cloud/firestore/collection.py index 7cfbb2e994a8..8dd4da6c1ae3 100644 --- a/firestore/google/cloud/firestore/collection.py +++ b/firestore/google/cloud/firestore/collection.py @@ -14,8 +14,92 @@ """Firestore Collection and Query classes.""" +from google.cloud.gapic.firestore.v1alpha1 import enums -class CollectionRef(object): +from google.firestore.v1alpha1.query_pb2 import Filter +from google.firestore.v1alpha1.query_pb2 import KindExpression +from google.firestore.v1alpha1.query_pb2 import PropertyFilter +from google.firestore.v1alpha1.query_pb2 import PropertyReference +from google.firestore.v1alpha1.query_pb2 import Query as PbQuery + +from google.protobuf.wrappers_pb2 import Int32Value + + +class Query(object): + """Firestore Query. + + :type client: :class:`~google.cloud.firestore.Client` + :param client: Firestore client context. + + :type ref_path: :class: `~google.cloud.firestore.Path` + :param ref_path: Path to a collection to be queried. + + :type limit: int + :param limit: (optional) Maximum number of results to return. + """ + + def __init__(self, + client, + ref_path, + limit=None): + if not ref_path.is_collection: + raise ValueError('Invalid collection path: %s' % (ref_path,)) + + self._client = client + self._path = ref_path + self._limit = limit + + def limit(self, count): + """Limit a query to return a fixed number of results. + + :type count: int + :param count: Maximum number of documents to return when ``.get()`` + is called. + + :rtype: :class:`~google.cloud.firestore.collection.Query` + :return: A limited ``Query``. + """ + return Query(self._client, self._path, limit=count) + + def get(self): + """Read the documents in the collection which are indicated + by this query. + + :rtype: list of :class:`~google.cloud.firestore.Document` + :returns: A sequence of ``Documents`` that fulfill the query + conditions from a collection. + """ + import google.cloud.firestore._values as values + from google.cloud.firestore.document import Document + from google.cloud.firestore.path import Path + + complete_filter = Filter(property_filter=PropertyFilter( + property=PropertyReference(name='__key__'), + op=enums.Operator.HAS_PARENT, + value=values.encode_value(self._path.parent))) + query = PbQuery( + kind=[KindExpression(name=self._path.kind)], + filter=complete_filter, + limit=Int32Value(value=self._limit) if self._limit else None) + response = self._client._api.run_query( + self._client.project, + self._client.database_id, + partition_id=None, + read_options=None, + query=query, + gql_query=None, + property_mask=None) + result = [ + Document( + self._client, + Path.from_key_proto(entity_result.entity.key), + values.decode_dict(entity_result.entity.properties)) + for entity_result in response.batch.entity_results + ] + return result + + +class CollectionRef(Query): """Reference to a collection location in a Firestore database. :type client: :class:`~google.cloud.firestore.Client` @@ -26,6 +110,7 @@ class CollectionRef(object): """ def __init__(self, client, path): + super(CollectionRef, self).__init__(client, path) self._client = client self._path = path @@ -38,10 +123,9 @@ def doc(self, doc_id): :rtype: :class:`~google.cloud.firestore.Document` :returns: A reference to the child document. """ - import google.cloud.firestore.document as document + from google.cloud.firestore.document import DocumentRef - return document.DocumentRef(self._client, - self._path.child(doc_id)) + return DocumentRef(self._client, self._path.child(doc_id)) def add(self, value): """Create a new document and save it in this colleciton. diff --git a/firestore/unit_tests/test_collection.py b/firestore/unit_tests/test_collection.py index 1945b0a1b1ef..92331ea8bbfc 100644 --- a/firestore/unit_tests/test_collection.py +++ b/firestore/unit_tests/test_collection.py @@ -15,7 +15,7 @@ import unittest -class TestColletionRef(unittest.TestCase): +class TestCollectionRef(unittest.TestCase): def setUp(self): from google.cloud.firestore.path import Path @@ -60,3 +60,47 @@ def test_new_doc(self): doc_ref = col_ref.new_doc() self.assertIsInstance(doc_ref, DocumentRef) self.assertTrue(len(doc_ref.id) >= 20) + + +class TestQuery(unittest.TestCase): + + def setUp(self): + import mock + from google.cloud.firestore.path import Path + from google.cloud.firestore.client import Client + from google.cloud.gapic.firestore.v1alpha1.datastore_api import ( + DatastoreApi) + + self.collection_path = Path('my-collection') + self.doc_path = Path('my-collection', 'my-document') + self.client = Client() + self.client._api = mock.MagicMock(spec=DatastoreApi) + + @staticmethod + def _get_target_class(): + from google.cloud.firestore.collection import Query + + return Query + + def _make_one(self, *args, **kwargs): + return self._get_target_class()(self.client, *args, **kwargs) + + def test_constructor(self): + query = self._make_one(self.collection_path) + self.assertIsInstance(query, self._get_target_class()) + self.assertIsNone(query._limit) + + def test_constructor_illegal_path(self): + with self.assertRaisesRegexp(ValueError, r'Invalid.*path'): + self._make_one(self.doc_path) + + def test_limit(self): + query = self._make_one(self.collection_path).limit(123) + + self.assertEqual(query._limit, 123) + + def test_get(self): + query = self._make_one(self.collection_path) + docs = query.get() + self.assertEqual(self.client._api.run_query.call_count, 1) + self.assertIsInstance(docs, list) diff --git a/firestore/unit_tests/test_document.py b/firestore/unit_tests/test_document.py index de5a3f9221b9..061b613499f2 100644 --- a/firestore/unit_tests/test_document.py +++ b/firestore/unit_tests/test_document.py @@ -37,7 +37,7 @@ def _make_one(self, path, properties, *args, **kwargs): def test_constructor(self): doc = self._make_one(self.doc_path, {}) - self.assertIsNotNone(doc) + self.assertIsInstance(doc, self._get_target_class()) def test_id(self): doc = self._make_one(self.doc_path, {}) @@ -92,7 +92,7 @@ def _make_one(self, *args, **kwargs): def test_constructor(self): doc_ref = self._make_one(self.client, self.doc_path) - self.assertIsNotNone(doc_ref) + self.assertIsInstance(doc_ref, self._get_target_class()) def test_invalid_path(self): with self.assertRaisesRegexp(ValueError, 'Invalid'): From e4ae1ea7cdfd83c28ebf751114c93c03b794d182 Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Wed, 21 Dec 2016 09:24:23 -0800 Subject: [PATCH 2/8] Review: import module instead of classes. --- .../google/cloud/firestore/collection.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/firestore/google/cloud/firestore/collection.py b/firestore/google/cloud/firestore/collection.py index eedffec4af7d..691d1ccd448a 100644 --- a/firestore/google/cloud/firestore/collection.py +++ b/firestore/google/cloud/firestore/collection.py @@ -16,11 +16,7 @@ from google.cloud.gapic.firestore.v1alpha1 import enums -from google.firestore.v1alpha1.query_pb2 import Filter -from google.firestore.v1alpha1.query_pb2 import KindExpression -from google.firestore.v1alpha1.query_pb2 import PropertyFilter -from google.firestore.v1alpha1.query_pb2 import PropertyReference -from google.firestore.v1alpha1.query_pb2 import Query as PbQuery +from google.firestore.v1alpha1 import query_pb2 from google.protobuf.wrappers_pb2 import Int32Value @@ -73,12 +69,13 @@ def get(self): from google.cloud.firestore.document import Document from google.cloud.firestore._path import Path - complete_filter = Filter(property_filter=PropertyFilter( - property=PropertyReference(name='__key__'), - op=enums.Operator.HAS_PARENT, - value=values.encode_value(self._path.parent))) - query = PbQuery( - kind=[KindExpression(name=self._path.kind)], + complete_filter = query_pb2.Filter( + property_filter=query_pb2.PropertyFilter( + property=query_pb2.PropertyReference(name='__key__'), + op=enums.Operator.HAS_PARENT, + value=values.encode_value(self._path.parent))) + query = query_pb2.Query( + kind=[query_pb2.KindExpression(name=self._path.kind)], filter=complete_filter, limit=Int32Value(value=self._limit) if self._limit else None) response = self._client._api.run_query( From 7d470dff7cff466086d7c78cd1a4f8848b78b933 Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Wed, 21 Dec 2016 09:35:05 -0800 Subject: [PATCH 3/8] Review: comment and formatting fixes. --- firestore/google/cloud/firestore/collection.py | 8 ++++++-- firestore/unit_tests/test_collection.py | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/firestore/google/cloud/firestore/collection.py b/firestore/google/cloud/firestore/collection.py index 691d1ccd448a..d2358b887ee8 100644 --- a/firestore/google/cloud/firestore/collection.py +++ b/firestore/google/cloud/firestore/collection.py @@ -49,11 +49,11 @@ def limit(self, count): """Limit a query to return a fixed number of results. :type count: int - :param count: Maximum number of documents to return when ``.get()`` + :param count: Maximum number of documents to return when :meth:`get` is called. :rtype: :class:`~google.cloud.firestore.collection.Query` - :return: A limited ``Query``. + :return: A limited :class:`Query`. """ return Query(self._client, self._path, limit=count) @@ -74,10 +74,12 @@ def get(self): property=query_pb2.PropertyReference(name='__key__'), op=enums.Operator.HAS_PARENT, value=values.encode_value(self._path.parent))) + query = query_pb2.Query( kind=[query_pb2.KindExpression(name=self._path.kind)], filter=complete_filter, limit=Int32Value(value=self._limit) if self._limit else None) + response = self._client._api.run_query( self._client.project, self._client.database_id, @@ -86,6 +88,7 @@ def get(self): query=query, gql_query=None, property_mask=None) + result = [ Document( self._client, @@ -93,6 +96,7 @@ def get(self): values.decode_dict(entity_result.entity.properties)) for entity_result in response.batch.entity_results ] + return result diff --git a/firestore/unit_tests/test_collection.py b/firestore/unit_tests/test_collection.py index c1f318f19ef1..df58e0b4600b 100644 --- a/firestore/unit_tests/test_collection.py +++ b/firestore/unit_tests/test_collection.py @@ -69,7 +69,7 @@ def test_new_document(self): DatastoreApi) client = Client() - client._api = mock.MagicMock(spec=DatastoreApi) + client._api = mock.Mock(spec=DatastoreApi) col_ref = self._make_one(client, Path('my-collection')) doc_ref = col_ref.new_document() self.assertIsInstance(doc_ref, DocumentRef) From 8b49e3b88b3675b2db78467664e2a5dd287cf41d Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Tue, 24 Jan 2017 10:56:57 -0800 Subject: [PATCH 4/8] Fix merge. --- firestore/google/cloud/firestore/collection.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firestore/google/cloud/firestore/collection.py b/firestore/google/cloud/firestore/collection.py index d2358b887ee8..0f1127e80232 100644 --- a/firestore/google/cloud/firestore/collection.py +++ b/firestore/google/cloud/firestore/collection.py @@ -66,7 +66,7 @@ def get(self): conditions from a collection. """ import google.cloud.firestore._values as values - from google.cloud.firestore.document import Document + from google.cloud.firestore.document import DocumentResult from google.cloud.firestore._path import Path complete_filter = query_pb2.Filter( @@ -90,7 +90,7 @@ def get(self): property_mask=None) result = [ - Document( + DocumentResult( self._client, Path.from_key_proto(entity_result.entity.key), values.decode_dict(entity_result.entity.properties)) From a13e739195d1f8173976342dfeb5f8257e84f121 Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Tue, 24 Jan 2017 16:36:56 -0800 Subject: [PATCH 5/8] Remove setUp from test. --- firestore/unit_tests/test_collection.py | 39 +++++++++++++------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/firestore/unit_tests/test_collection.py b/firestore/unit_tests/test_collection.py index 5a53c294b6a8..dd4d16d13200 100644 --- a/firestore/unit_tests/test_collection.py +++ b/firestore/unit_tests/test_collection.py @@ -78,18 +78,6 @@ def test_new_document(self): class TestQuery(unittest.TestCase): - def setUp(self): - import mock - from google.cloud.firestore._path import Path - from google.cloud.firestore.client import Client - from google.cloud.gapic.firestore.v1alpha1.datastore_api import ( - DatastoreApi) - - self.collection_path = Path('my-collection') - self.doc_path = Path('my-collection', 'my-document') - self.client = Client() - self.client._api = mock.MagicMock(spec=DatastoreApi) - @staticmethod def _get_target_class(): from google.cloud.firestore.collection import Query @@ -97,24 +85,39 @@ def _get_target_class(): return Query def _make_one(self, *args, **kwargs): - return self._get_target_class()(self.client, *args, **kwargs) + import mock + from google.cloud.firestore.client import Client + from google.cloud.gapic.firestore.v1alpha1.datastore_api import ( + DatastoreApi) + + client = Client() + client._api = mock.MagicMock(spec=DatastoreApi) + return self._get_target_class()(client, *args, **kwargs) def test_constructor(self): - query = self._make_one(self.collection_path) + from google.cloud.firestore._path import Path + + query = self._make_one(Path('my-collection')) self.assertIsInstance(query, self._get_target_class()) self.assertIsNone(query._limit) def test_constructor_illegal_path(self): + from google.cloud.firestore._path import Path + with self.assertRaisesRegexp(ValueError, r'Invalid.*path'): - self._make_one(self.doc_path) + self._make_one(Path('my-collection', 'my-document')) def test_limit(self): - query = self._make_one(self.collection_path).limit(123) + from google.cloud.firestore._path import Path + + query = self._make_one(Path('my-collection')).limit(123) self.assertEqual(query._limit, 123) def test_get(self): - query = self._make_one(self.collection_path) + from google.cloud.firestore._path import Path + + query = self._make_one(Path('my-collection')) docs = query.get() - self.assertEqual(self.client._api.run_query.call_count, 1) + self.assertEqual(query._client._api.run_query.call_count, 1) self.assertIsInstance(docs, list) From 37c4312756ef2902498e8faa0020ed0a6f4ba3ba Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Tue, 24 Jan 2017 16:37:27 -0800 Subject: [PATCH 6/8] Review comments addressed. --- .../google/cloud/firestore/collection.py | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/firestore/google/cloud/firestore/collection.py b/firestore/google/cloud/firestore/collection.py index 0f1127e80232..df2e3e0f3836 100644 --- a/firestore/google/cloud/firestore/collection.py +++ b/firestore/google/cloud/firestore/collection.py @@ -32,6 +32,9 @@ class Query(object): :type limit: int :param limit: (optional) Maximum number of results to return. + + :raises ValueError: Path must be a valid Collection path (not a + Document path). """ def __init__(self, @@ -48,6 +51,9 @@ def __init__(self, def limit(self, count): """Limit a query to return a fixed number of results. + This function returns a new (immutable) instance of the ``Query`` + (rather than modify the existing ``Query``) to impose the limit. + :type count: int :param count: Maximum number of documents to return when :meth:`get` is called. @@ -58,11 +64,10 @@ def limit(self, count): return Query(self._client, self._path, limit=count) def get(self): - """Read the documents in the collection which are indicated - by this query. + """Read the documents in the collection indicated by this query. - :rtype: list of :class:`~google.cloud.firestore.Document` - :returns: A sequence of ``Documents`` that fulfill the query + :rtype: list of :class:`~google.cloud.firestore.DocumentResult` + :returns: A sequence of Documents that fulfill the query conditions from a collection. """ import google.cloud.firestore._values as values @@ -110,18 +115,13 @@ class CollectionRef(Query): :param path: Path location of this collection. """ - def __init__(self, client, path): - super(CollectionRef, self).__init__(client, path) - self._client = client - self._path = path - def document(self, doc_id): """Reference to a child document in the collection. :type doc_id: int or str :param doc_id: The (unique) document identifier. - :rtype: :class:`~google.cloud.firestore.Document` + :rtype: :class:`~google.cloud.firestore.DocumentResult` :returns: A reference to the child document. """ from google.cloud.firestore.document import DocumentRef @@ -134,13 +134,13 @@ def add(self, value): :type value: dict :param value: Dictionary of values to save to the document. - :rtype: :class:`~google.cloud.firestore.Document` - :returns: A ``Document`` that was saved to the database. + :rtype: :class:`~google.cloud.firestore.DocumentResult` + :returns: A ``DocumentResult`` that was saved to the database. """ return self.new_document().set(value) def new_document(self): - """Create a reference to a new ``Document`` in this collection. + """Create a reference to a new Document in this collection. New documents are created with uniquely created client-side identifiers. From 25f996d356135b1fcc1d13b717d5d0e3ae37943e Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Tue, 24 Jan 2017 17:03:14 -0800 Subject: [PATCH 7/8] Add path check to test. --- firestore/unit_tests/test_document.py | 1 + 1 file changed, 1 insertion(+) diff --git a/firestore/unit_tests/test_document.py b/firestore/unit_tests/test_document.py index 594b1e619414..327e62b6f2e9 100644 --- a/firestore/unit_tests/test_document.py +++ b/firestore/unit_tests/test_document.py @@ -108,6 +108,7 @@ def _make_one(self, *args, **kwargs): def test_constructor(self): doc_ref = self._make_one(self.client, self.doc_path) self.assertIsInstance(doc_ref, self._get_target_class()) + self.assertEqual(doc_ref._path, self.doc_path) def test_invalid_path(self): with self.assertRaisesRegexp(ValueError, 'Invalid'): From b7662c7338f510c75cb187ef13ecd97f5bc11705 Mon Sep 17 00:00:00 2001 From: Mike Koss Date: Thu, 26 Jan 2017 08:57:59 -0800 Subject: [PATCH 8/8] Remove un-needed ternary operator. --- firestore/google/cloud/firestore/collection.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firestore/google/cloud/firestore/collection.py b/firestore/google/cloud/firestore/collection.py index df2e3e0f3836..4632017db750 100644 --- a/firestore/google/cloud/firestore/collection.py +++ b/firestore/google/cloud/firestore/collection.py @@ -83,7 +83,7 @@ def get(self): query = query_pb2.Query( kind=[query_pb2.KindExpression(name=self._path.kind)], filter=complete_filter, - limit=Int32Value(value=self._limit) if self._limit else None) + limit=Int32Value(value=self._limit)) response = self._client._api.run_query( self._client.project,