From d6fd0ce0999e94a5d361910082d82b7db44416b8 Mon Sep 17 00:00:00 2001 From: Chris McDonough Date: Tue, 8 Jan 2019 23:21:12 -0500 Subject: [PATCH 1/7] closes #6826: respect transform values passed into collection.add --- .../cloud/firestore_v1beta1/collection.py | 6 ++-- firestore/tests/unit/test_collection.py | 30 ++++++++++++++----- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/firestore/google/cloud/firestore_v1beta1/collection.py b/firestore/google/cloud/firestore_v1beta1/collection.py index 9d616fe4d20c..32cf2b7bdd64 100644 --- a/firestore/google/cloud/firestore_v1beta1/collection.py +++ b/firestore/google/cloud/firestore_v1beta1/collection.py @@ -159,9 +159,8 @@ def add(self, document_data, document_id=None): """ if document_id is None: parent_path, expected_prefix = self._parent_info() - document_pb = document_pb2.Document( - fields=_helpers.encode_dict(document_data) - ) + + document_pb = document_pb2.Document() created_document_pb = self._client._firestore_api.create_document( parent_path, @@ -174,6 +173,7 @@ def add(self, document_data, document_id=None): new_document_id = _helpers.get_doc_id(created_document_pb, expected_prefix) document_ref = self.document(new_document_id) + document_ref.set(document_data) return created_document_pb.update_time, document_ref else: document_ref = self.document(document_id) diff --git a/firestore/tests/unit/test_collection.py b/firestore/tests/unit/test_collection.py index 6d555526e1d0..b77fa5b7cf57 100644 --- a/firestore/tests/unit/test_collection.py +++ b/firestore/tests/unit/test_collection.py @@ -191,11 +191,21 @@ def test__parent_info_nested(self): def test_add_auto_assigned(self): from google.cloud.firestore_v1beta1.proto import document_pb2 - from google.cloud.firestore_v1beta1 import _helpers from google.cloud.firestore_v1beta1.document import DocumentReference + from google.cloud.firestore_v1beta1 import SERVER_TIMESTAMP + from google.cloud.firestore_v1beta1._helpers import pbs_for_set_no_merge # Create a minimal fake GAPIC add attach it to a real client. - firestore_api = mock.Mock(spec=["create_document"]) + firestore_api = mock.Mock(spec=["create_document", "commit"]) + write_result = mock.Mock( + update_time=mock.sentinel.update_time, spec=["update_time"] + ) + commit_response = mock.Mock( + write_results=[write_result], + spec=["write_results", "commit_time"], + commit_time=mock.sentinel.commit_time, + ) + firestore_api.commit.return_value = commit_response create_doc_response = document_pb2.Document() firestore_api.create_document.return_value = create_doc_response client = _make_client() @@ -212,8 +222,9 @@ def test_add_auto_assigned(self): create_doc_response.update_time.FromDatetime(datetime.datetime.utcnow()) firestore_api.create_document.return_value = create_doc_response - # Actually call add() on our collection. - document_data = {"been": "here"} + # Actually call add() on our collection; include a transform to make + # sure transforms during adds work. + document_data = {"been": "here", "now": SERVER_TIMESTAMP} update_time, document_ref = collection.add(document_data) # Verify the response and the mocks. @@ -223,9 +234,7 @@ def test_add_auto_assigned(self): expected_path = collection._path + (auto_assigned_id,) self.assertEqual(document_ref._path, expected_path) - expected_document_pb = document_pb2.Document( - fields=_helpers.encode_dict(document_data) - ) + expected_document_pb = document_pb2.Document() firestore_api.create_document.assert_called_once_with( parent_path, collection_id=collection.id, @@ -234,6 +243,13 @@ def test_add_auto_assigned(self): mask=None, metadata=client._rpc_metadata, ) + write_pbs = pbs_for_set_no_merge(document_ref._document_path, document_data) + firestore_api.commit.assert_called_once_with( + client._database_string, + write_pbs, + transaction=None, + metadata=client._rpc_metadata, + ) @staticmethod def _write_pb_for_create(document_path, document_data): From 7d365a137e76fd8c3f35eac8bcc3e0877be1044b Mon Sep 17 00:00:00 2001 From: Chris McDonough Date: Wed, 9 Jan 2019 09:34:35 -0500 Subject: [PATCH 2/7] use set_result.update_time as update time response value instead of created response update time; hail mary to see if system tests take it --- firestore/google/cloud/firestore_v1beta1/collection.py | 4 ++-- firestore/tests/unit/test_collection.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/firestore/google/cloud/firestore_v1beta1/collection.py b/firestore/google/cloud/firestore_v1beta1/collection.py index 32cf2b7bdd64..9c0f98ac7860 100644 --- a/firestore/google/cloud/firestore_v1beta1/collection.py +++ b/firestore/google/cloud/firestore_v1beta1/collection.py @@ -173,8 +173,8 @@ def add(self, document_data, document_id=None): new_document_id = _helpers.get_doc_id(created_document_pb, expected_prefix) document_ref = self.document(new_document_id) - document_ref.set(document_data) - return created_document_pb.update_time, document_ref + set_result = document_ref.set(document_data) + return set_result.update_time, document_ref else: document_ref = self.document(document_id) write_result = document_ref.create(document_data) diff --git a/firestore/tests/unit/test_collection.py b/firestore/tests/unit/test_collection.py index b77fa5b7cf57..09fa1ffe22d0 100644 --- a/firestore/tests/unit/test_collection.py +++ b/firestore/tests/unit/test_collection.py @@ -228,7 +228,7 @@ def test_add_auto_assigned(self): update_time, document_ref = collection.add(document_data) # Verify the response and the mocks. - self.assertIs(update_time, create_doc_response.update_time) + self.assertIs(update_time, mock.sentinel.update_time) self.assertIsInstance(document_ref, DocumentReference) self.assertIs(document_ref._client, client) expected_path = collection._path + (auto_assigned_id,) From 84a45aaaec2087b1a722013f554d79ad6915a6c9 Mon Sep 17 00:00:00 2001 From: Chris McDonough Date: Wed, 9 Jan 2019 10:10:46 -0500 Subject: [PATCH 3/7] hail mary for system test --- firestore/tests/system.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firestore/tests/system.py b/firestore/tests/system.py index 226b1bd9bfbb..5fc04efc2c1a 100644 --- a/firestore/tests/system.py +++ b/firestore/tests/system.py @@ -409,7 +409,7 @@ def test_collection_add(client, cleanup): cleanup(document_ref1) snapshot1 = document_ref1.get() assert snapshot1.to_dict() == data1 - assert snapshot1.create_time == update_time1 + #assert snapshot1.create_time == update_time1 assert snapshot1.update_time == update_time1 assert RANDOM_ID_REGEX.match(document_ref1.id) From b8078c5bcb5b7eaf712ec518d3825f5f675df830 Mon Sep 17 00:00:00 2001 From: Chris McDonough Date: Wed, 9 Jan 2019 10:22:59 -0500 Subject: [PATCH 4/7] offs --- firestore/tests/system.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/firestore/tests/system.py b/firestore/tests/system.py index 5fc04efc2c1a..7cefe3d52662 100644 --- a/firestore/tests/system.py +++ b/firestore/tests/system.py @@ -409,7 +409,13 @@ def test_collection_add(client, cleanup): cleanup(document_ref1) snapshot1 = document_ref1.get() assert snapshot1.to_dict() == data1 - #assert snapshot1.create_time == update_time1 + # NB: We used to assert here that the snapshot create_time was the document + # update time as returned by add(), but now under the hood, to allow for + # transform values passed into add, the document is created, then a set() + # is issued against the document later; the value returned by add is the + # result of the latter. We don't have access to the create time, + # so we don't have a value to make this assertion with. + # assert snapshot1.create_time == update_time1 assert snapshot1.update_time == update_time1 assert RANDOM_ID_REGEX.match(document_ref1.id) @@ -429,7 +435,8 @@ def test_collection_add(client, cleanup): cleanup(document_ref3) snapshot3 = document_ref3.get() assert snapshot3.to_dict() == data3 - assert snapshot3.create_time == update_time3 + # see NB above + #assert snapshot3.create_time == update_time3 assert snapshot3.update_time == update_time3 assert RANDOM_ID_REGEX.match(document_ref3.id) From fff88f3f31cec71578441dab8e9a245beb21c8f3 Mon Sep 17 00:00:00 2001 From: Chris McDonough Date: Thu, 10 Jan 2019 01:10:29 -0500 Subject: [PATCH 5/7] resurrect assertions --- firestore/tests/system.py | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/firestore/tests/system.py b/firestore/tests/system.py index 7cefe3d52662..330a426057a1 100644 --- a/firestore/tests/system.py +++ b/firestore/tests/system.py @@ -409,13 +409,7 @@ def test_collection_add(client, cleanup): cleanup(document_ref1) snapshot1 = document_ref1.get() assert snapshot1.to_dict() == data1 - # NB: We used to assert here that the snapshot create_time was the document - # update time as returned by add(), but now under the hood, to allow for - # transform values passed into add, the document is created, then a set() - # is issued against the document later; the value returned by add is the - # result of the latter. We don't have access to the create time, - # so we don't have a value to make this assertion with. - # assert snapshot1.create_time == update_time1 + assert snapshot1.create_time <= update_time1 assert snapshot1.update_time == update_time1 assert RANDOM_ID_REGEX.match(document_ref1.id) @@ -435,8 +429,7 @@ def test_collection_add(client, cleanup): cleanup(document_ref3) snapshot3 = document_ref3.get() assert snapshot3.to_dict() == data3 - # see NB above - #assert snapshot3.create_time == update_time3 + assert snapshot3.create_time <= update_time3 assert snapshot3.update_time == update_time3 assert RANDOM_ID_REGEX.match(document_ref3.id) From aae4420795fe091ba18ad54d8703b789badf6a7a Mon Sep 17 00:00:00 2001 From: Chris McDonough Date: Thu, 10 Jan 2019 15:13:36 -0500 Subject: [PATCH 6/7] timestamps dont support lteq --- firestore/tests/system.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firestore/tests/system.py b/firestore/tests/system.py index 330a426057a1..fa806829a37f 100644 --- a/firestore/tests/system.py +++ b/firestore/tests/system.py @@ -409,7 +409,7 @@ def test_collection_add(client, cleanup): cleanup(document_ref1) snapshot1 = document_ref1.get() assert snapshot1.to_dict() == data1 - assert snapshot1.create_time <= update_time1 + assert snapshot1.create_time < update_time1 assert snapshot1.update_time == update_time1 assert RANDOM_ID_REGEX.match(document_ref1.id) @@ -429,7 +429,7 @@ def test_collection_add(client, cleanup): cleanup(document_ref3) snapshot3 = document_ref3.get() assert snapshot3.to_dict() == data3 - assert snapshot3.create_time <= update_time3 + assert snapshot3.create_time < update_time3 assert snapshot3.update_time == update_time3 assert RANDOM_ID_REGEX.match(document_ref3.id) From cbd2a78808d0afa562a461777c3e747113e615b6 Mon Sep 17 00:00:00 2001 From: Chris McDonough Date: Thu, 10 Jan 2019 15:32:38 -0500 Subject: [PATCH 7/7] lt unsupported; bail --- firestore/tests/system.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/firestore/tests/system.py b/firestore/tests/system.py index fa806829a37f..670b3dcdfa16 100644 --- a/firestore/tests/system.py +++ b/firestore/tests/system.py @@ -409,7 +409,6 @@ def test_collection_add(client, cleanup): cleanup(document_ref1) snapshot1 = document_ref1.get() assert snapshot1.to_dict() == data1 - assert snapshot1.create_time < update_time1 assert snapshot1.update_time == update_time1 assert RANDOM_ID_REGEX.match(document_ref1.id) @@ -429,7 +428,6 @@ def test_collection_add(client, cleanup): cleanup(document_ref3) snapshot3 = document_ref3.get() assert snapshot3.to_dict() == data3 - assert snapshot3.create_time < update_time3 assert snapshot3.update_time == update_time3 assert RANDOM_ID_REGEX.match(document_ref3.id)