diff --git a/firestore/google/cloud/firestore.py b/firestore/google/cloud/firestore.py index 9e0efdcb576e..4e7fd4768f13 100644 --- a/firestore/google/cloud/firestore.py +++ b/firestore/google/cloud/firestore.py @@ -19,7 +19,6 @@ from google.cloud.firestore_v1beta1 import AdminClient from google.cloud.firestore_v1beta1 import Client from google.cloud.firestore_v1beta1 import CollectionReference -from google.cloud.firestore_v1beta1 import CreateIfMissingOption from google.cloud.firestore_v1beta1 import DELETE_FIELD from google.cloud.firestore_v1beta1 import DocumentReference from google.cloud.firestore_v1beta1 import DocumentSnapshot @@ -42,7 +41,6 @@ 'AdminClient', 'Client', 'CollectionReference', - 'CreateIfMissingOption', 'DELETE_FIELD', 'DocumentReference', 'DocumentSnapshot', diff --git a/firestore/google/cloud/firestore_v1beta1/__init__.py b/firestore/google/cloud/firestore_v1beta1/__init__.py index e0069d68dc0d..d8b992b1d661 100644 --- a/firestore/google/cloud/firestore_v1beta1/__init__.py +++ b/firestore/google/cloud/firestore_v1beta1/__init__.py @@ -22,7 +22,6 @@ from google.cloud.firestore_v1beta1._helpers import ReadAfterWriteError from google.cloud.firestore_v1beta1.batch import WriteBatch from google.cloud.firestore_v1beta1.client import Client -from google.cloud.firestore_v1beta1.client import CreateIfMissingOption from google.cloud.firestore_v1beta1.client import ExistsOption from google.cloud.firestore_v1beta1.client import LastUpdateOption from google.cloud.firestore_v1beta1.client import WriteOption @@ -46,7 +45,6 @@ 'AdminClient', 'Client', 'CollectionReference', - 'CreateIfMissingOption', 'DELETE_FIELD', 'DocumentReference', 'DocumentSnapshot', diff --git a/firestore/google/cloud/firestore_v1beta1/_helpers.py b/firestore/google/cloud/firestore_v1beta1/_helpers.py index 3acda674b7dc..9d0cc324f3bb 100644 --- a/firestore/google/cloud/firestore_v1beta1/_helpers.py +++ b/firestore/google/cloud/firestore_v1beta1/_helpers.py @@ -28,10 +28,9 @@ import grpc import six +from google.cloud import exceptions from google.cloud._helpers import _datetime_to_pb_timestamp from google.cloud._helpers import _pb_timestamp_to_datetime -from google.cloud import exceptions - from google.cloud.firestore_v1beta1 import constants from google.cloud.firestore_v1beta1.gapic import enums from google.cloud.firestore_v1beta1.proto import common_pb2 @@ -47,10 +46,6 @@ 'The data at {!r} is not a dictionary, so it cannot contain the key {!r}') FIELD_PATH_DELIMITER = '.' DOCUMENT_PATH_DELIMITER = '/' -_NO_CREATE_TEMPLATE = ( - 'The ``create_if_missing`` option cannot be used ' - 'on ``{}()`` requests.') -NO_CREATE_ON_DELETE = _NO_CREATE_TEMPLATE.format('delete') INACTIVE_TXN = ( 'Transaction not in progress, cannot be used in API requests.') READ_AFTER_WRITE_ERROR = 'Attempted read after write in a transaction.' @@ -817,7 +812,6 @@ def pbs_for_set(document_path, document_data, option): or two ``Write`` protobuf instances for ``set()``. """ transform_paths, actual_data = remove_server_timestamp(document_data) - update_pb = write_pb2.Write( update=document_pb2.Document( name=document_path, @@ -825,7 +819,7 @@ def pbs_for_set(document_path, document_data, option): ), ) if option is not None: - option.modify_write(update_pb) + option.modify_write(update_pb, actual_data=actual_data, path=document_path) write_pbs = [update_pb] if transform_paths: @@ -856,7 +850,7 @@ def pbs_for_update(client, document_path, field_updates, option): """ if option is None: # Default uses ``exists=True``. - option = client.write_option(create_if_missing=False) + option = client.write_option(exists=True) transform_paths, actual_updates = remove_server_timestamp(field_updates) update_values, field_paths = FieldPathHelper.to_field_paths(actual_updates) @@ -870,7 +864,7 @@ def pbs_for_update(client, document_path, field_updates, option): update_mask=common_pb2.DocumentMask(field_paths=sorted(field_paths)), ) # Due to the default, we don't have to check if ``None``. - option.modify_write(update_pb) + option.modify_write(update_pb, field_paths=field_paths) write_pbs = [update_pb] if transform_paths: @@ -897,7 +891,7 @@ def pb_for_delete(document_path, option): """ write_pb = write_pb2.Write(delete=document_path) if option is not None: - option.modify_write(write_pb, no_create_msg=NO_CREATE_ON_DELETE) + option.modify_write(write_pb) return write_pb diff --git a/firestore/google/cloud/firestore_v1beta1/client.py b/firestore/google/cloud/firestore_v1beta1/client.py index d19898bbb652..7555928af7aa 100644 --- a/firestore/google/cloud/firestore_v1beta1/client.py +++ b/firestore/google/cloud/firestore_v1beta1/client.py @@ -37,13 +37,15 @@ from google.cloud.firestore_v1beta1.document import DocumentSnapshot from google.cloud.firestore_v1beta1.gapic import firestore_client from google.cloud.firestore_v1beta1.transaction import Transaction +from google.cloud.firestore_v1beta1.proto import common_pb2 +from google.cloud.firestore_v1beta1.proto import document_pb2 +from google.cloud.firestore_v1beta1.proto import write_pb2 DEFAULT_DATABASE = '(default)' """str: The default database used in a :class:`~.firestore.client.Client`.""" _BAD_OPTION_ERR = ( - 'Exactly one of ``create_if_missing``, ``last_update_time`` ' - 'and ``exists`` must be provided.') + 'Exactly one of ``last_update_time`` or ``exists`` must be provided.') _BAD_DOC_TEMPLATE = ( 'Document {!r} appeared in response but was not present among references') _ACTIVE_TXN = 'There is already an active transaction.' @@ -252,10 +254,8 @@ def write_option(**kwargs): :meth:`~.DocumentReference.update` and :meth:`~.DocumentReference.delete`. - Exactly one of three keyword arguments must be provided: + One of the following two keyword arguments must be provided: - * ``create_if_missing`` (:class:`bool`): Indicates if the document - should be created if it doesn't already exist. * ``last_update_time`` (:class:`google.protobuf.timestamp_pb2.\ Timestamp`): A timestamp. When set, the target document must exist and have been last updated at that time. Protobuf ``update_time`` @@ -268,9 +268,8 @@ def write_option(**kwargs): it is not allowed). Providing multiple would be an apparent contradiction, since ``last_update_time`` assumes that the document **was** updated (it can't have been updated if it - doesn't exist) and both ``create_if_missing`` and ``exists`` indicate - that it is unknown if the document exists or not (but in different - ways). + doesn't exist) and ``exists`` indicate that it is unknown if the + document exists or not. Args: kwargs (Dict[str, Any]): The keyword arguments described above. @@ -283,12 +282,12 @@ def write_option(**kwargs): raise TypeError(_BAD_OPTION_ERR) name, value = kwargs.popitem() - if name == 'create_if_missing': - return CreateIfMissingOption(value) - elif name == 'last_update_time': + if name == 'last_update_time': return LastUpdateOption(value) elif name == 'exists': return ExistsOption(value) + elif name == 'merge': + return MergeOption() else: extra = '{!r} was provided'.format(name) raise TypeError(_BAD_OPTION_ERR, extra) @@ -424,47 +423,37 @@ def modify_write(self, write_pb, **unused_kwargs): write_pb.current_document.CopyFrom(current_doc) -class CreateIfMissingOption(WriteOption): - """Option used to assert "create if missing" on a write operation. +class MergeOption(WriteOption): + """Option used to merge on a write operation. This will typically be created by :meth:`~.firestore_v1beta1.client.Client.write_option`. - - Args: - create_if_missing (bool): Indicates if the document should be created - if it doesn't already exist. """ - - def __init__(self, create_if_missing): - self._create_if_missing = create_if_missing - - def modify_write(self, write_pb, no_create_msg=None): + def modify_write(self, write_pb, actual_data=None, path=None, **unused_kwargs): """Modify a ``Write`` protobuf based on the state of this write option. - If: - - * ``create_if_missing=False``, adds a precondition that requires - existence - * ``create_if_missing=True``, does not add any precondition - * ``no_create_msg`` is passed, raises an exception. For example, in a - :meth:`~.DocumentReference.delete`, no "create" can occur, so it - wouldn't make sense to "create if missing". - Args: write_pb (google.cloud.firestore_v1beta1.types.Write): A ``Write`` protobuf instance to be modified with a precondition determined by the state of this option. - no_create_msg (Optional[str]): A message to use to indicate that - a create operation is not allowed. - - Raises: - ValueError: If ``no_create_msg`` is passed. + actual_data (dict): + The actual field names and values to use for replacing a + document. + path (str): A fully-qualified document_path + unused_kwargs (Dict[str, Any]): Keyword arguments accepted by + other subclasses that are unused here. """ - if no_create_msg is not None: - raise ValueError(no_create_msg) - elif not self._create_if_missing: - current_doc = types.Precondition(exists=True) - write_pb.current_document.CopyFrom(current_doc) + actual_data, field_paths = _helpers.FieldPathHelper.to_field_paths(actual_data) + doc = document_pb2.Document( + name=path, + fields=_helpers.encode_dict(actual_data) + ) + write = write_pb2.Write( + update=doc, + ) + write_pb.CopyFrom(write) + mask = common_pb2.DocumentMask(field_paths=sorted(field_paths)) + write_pb.update_mask.CopyFrom(mask) class ExistsOption(WriteOption): @@ -473,24 +462,6 @@ class ExistsOption(WriteOption): This will typically be created by :meth:`~.firestore_v1beta1.client.Client.write_option`. - This option is closely related to - :meth:`~.firestore_v1beta1.client.CreateIfMissingOption`, - but a "create if missing". In fact, - - .. code-block:: python - - >>> ExistsOption(exists=True) - - is (mostly) equivalent to - - .. code-block:: python - - >>> CreateIfMissingOption(create_if_missing=False) - - The only difference being that "create if missing" cannot be used - on some operations (e.g. :meth:`~.DocumentReference.delete`) - while "exists" can. - Args: exists (bool): Indicates if the document being modified should already exist. diff --git a/firestore/google/cloud/firestore_v1beta1/document.py b/firestore/google/cloud/firestore_v1beta1/document.py index 3ba6a4a82ca3..5bc4d332642d 100644 --- a/firestore/google/cloud/firestore_v1beta1/document.py +++ b/firestore/google/cloud/firestore_v1beta1/document.py @@ -377,9 +377,7 @@ def delete(self, option=None): Args: option (Optional[~.firestore_v1beta1.client.WriteOption]): A write option to make assertions / preconditions on the server - state of the document before applying changes. Note that - ``create_if_missing`` can't be used here since it does not - apply (i.e. a "delete" cannot "create"). + state of the document before applying changes. Returns: google.protobuf.timestamp_pb2.Timestamp: The time that the delete @@ -387,9 +385,6 @@ def delete(self, option=None): when the delete was sent (i.e. nothing was deleted), this method will still succeed and will still return the time that the request was received by the server. - - Raises: - ValueError: If the ``create_if_missing`` write option is used. """ write_pb = _helpers.pb_for_delete(self._document_path, option) with _helpers.remap_gax_error_on_commit(): diff --git a/firestore/tests/system.py b/firestore/tests/system.py index 082e42262b43..2c32400d8879 100644 --- a/firestore/tests/system.py +++ b/firestore/tests/system.py @@ -139,7 +139,7 @@ def test_document_set(client, cleanup): cleanup(document) # 0. Make sure the document doesn't exist yet using an option. - option0 = client.write_option(create_if_missing=False) + option0 = client.write_option(exists=True) with pytest.raises(NotFound) as exc_info: document.set({'no': 'way'}, option=option0) @@ -148,7 +148,7 @@ def test_document_set(client, cleanup): # 1. Use ``set()`` to create the document (using an option). data1 = {'foo': 88} - option1 = client.write_option(create_if_missing=True) + option1 = client.write_option(exists=False) write_result1 = document.set(data1, option=option1) snapshot1 = document.get() assert snapshot1.to_dict() == data1 @@ -194,6 +194,47 @@ def test_document_set(client, cleanup): assert exc_to_code(exc_info.value.cause) == StatusCode.FAILED_PRECONDITION +def test_document_set_merge(client, cleanup): + document_id = 'for-set' + unique_resource_id('-') + document = client.document('i-did-it', document_id) + # Add to clean-up before API request (in case ``set()`` fails). + cleanup(document) + + # 0. Make sure the document doesn't exist yet using an option. + option0 = client.write_option(exists=True) + with pytest.raises(NotFound) as exc_info: + document.set({'no': 'way'}, option=option0) + + assert exc_info.value.message.startswith(MISSING_DOCUMENT) + assert document_id in exc_info.value.message + + # 1. Use ``set()`` to create the document (using an option). + data1 = {'name': 'Sam', + 'address': {'city': 'SF', + 'state': 'CA'} + } + option1 = client.write_option(exists=False) + write_result1 = document.set(data1, option=option1) + snapshot1 = document.get() + assert snapshot1.to_dict() == data1 + # Make sure the update is what created the document. + assert snapshot1.create_time == snapshot1.update_time + assert snapshot1.update_time == write_result1.update_time + + # 2. Call ``set()`` again to overwrite (no option). + data2 = {'address.city': 'LA'} + option2 = client.write_option(merge=True) + write_result2 = document.set(data2, option=option2) + snapshot2 = document.get() + assert snapshot2.to_dict() == {'name': 'Sam', + 'address': {'city': 'LA', + 'state': 'CA'} + } + # Make sure the create time hasn't changed. + assert snapshot2.create_time == snapshot1.create_time + assert snapshot2.update_time == write_result2.update_time + + def test_update_document(client, cleanup): document_id = 'for-update' + unique_resource_id('-') document = client.document('made', document_id) @@ -207,7 +248,7 @@ def test_update_document(client, cleanup): assert document_id in exc_info.value.message # 1. Try to update before the document exists (now with an option). - option1 = client.write_option(create_if_missing=False) + option1 = client.write_option(exists=True) with pytest.raises(NotFound) as exc_info: document.update({'still': 'not-there'}, option=option1) assert exc_info.value.message.startswith(MISSING_DOCUMENT) @@ -223,7 +264,7 @@ def test_update_document(client, cleanup): }, 'other': True, } - option2 = client.write_option(create_if_missing=True) + option2 = client.write_option(exists=False) write_result2 = document.update(data, option=option2) # 3. Send an update without a field path (no option). diff --git a/firestore/tests/unit/test__helpers.py b/firestore/tests/unit/test__helpers.py index 5669ba1da71f..e7e7a4689e0b 100644 --- a/firestore/tests/unit/test__helpers.py +++ b/firestore/tests/unit/test__helpers.py @@ -1172,9 +1172,9 @@ def test_without_option(self): def test_with_option(self): from google.cloud.firestore_v1beta1.proto import common_pb2 - from google.cloud.firestore_v1beta1.client import CreateIfMissingOption + from google.cloud.firestore_v1beta1.client import ExistsOption - option = CreateIfMissingOption(False) + option = ExistsOption(True) precondition = common_pb2.Precondition(exists=True) self._helper(option=option, current_document=precondition) @@ -1191,11 +1191,12 @@ def _call_fut(client, document_path, field_updates, option): return pbs_for_update(client, document_path, field_updates, option) def _helper(self, option=None, do_transform=False, **write_kwargs): + from google.cloud.firestore_v1beta1.client import Client + from google.cloud.firestore_v1beta1.client import ExistsOption from google.cloud.firestore_v1beta1.gapic import enums from google.cloud.firestore_v1beta1.proto import common_pb2 from google.cloud.firestore_v1beta1.proto import document_pb2 from google.cloud.firestore_v1beta1.proto import write_pb2 - from google.cloud.firestore_v1beta1.client import Client from google.cloud.firestore_v1beta1.constants import SERVER_TIMESTAMP document_path = _make_ref_string( @@ -1216,6 +1217,9 @@ def _helper(self, option=None, do_transform=False, **write_kwargs): map_pb = document_pb2.MapValue(fields={ 'yum': _value_pb(bytes_value=value), }) + if isinstance(option, ExistsOption): + write_kwargs = {'current_document' : {'exists': option._exists}} + expected_update_pb = write_pb2.Write( update=document_pb2.Document( name=document_path, @@ -1250,9 +1254,9 @@ def test_without_option(self): def test_with_option(self): from google.cloud.firestore_v1beta1.proto import common_pb2 - from google.cloud.firestore_v1beta1.client import CreateIfMissingOption + from google.cloud.firestore_v1beta1.client import ExistsOption - option = CreateIfMissingOption(True) + option = ExistsOption(False) self._helper(option=option) def test_update_and_transform(self): @@ -1299,16 +1303,6 @@ def test_with_option(self): precondition = common_pb2.Precondition(update_time=update_time) self._helper(option=option, current_document=precondition) - def test_bad_option(self): - from google.cloud.firestore_v1beta1._helpers import NO_CREATE_ON_DELETE - from google.cloud.firestore_v1beta1.client import CreateIfMissingOption - - option = CreateIfMissingOption(True) - with self.assertRaises(ValueError) as exc_info: - self._helper(option=option) - - self.assertEqual(exc_info.exception.args, (NO_CREATE_ON_DELETE,)) - class Test_get_transaction_id(unittest.TestCase): diff --git a/firestore/tests/unit/test_batch.py b/firestore/tests/unit/test_batch.py index 8f66b5211d17..59775a5f87c5 100644 --- a/firestore/tests/unit/test_batch.py +++ b/firestore/tests/unit/test_batch.py @@ -93,6 +93,33 @@ def test_set(self): ) self.assertEqual(batch._write_pbs, [new_write_pb]) + def test_set_merge(self): + from google.cloud.firestore_v1beta1.proto import document_pb2 + from google.cloud.firestore_v1beta1.proto import write_pb2 + from google.cloud.firestore_v1beta1.client import MergeOption + + client = _make_client() + batch = self._make_one(client) + self.assertEqual(batch._write_pbs, []) + + reference = client.document('another', 'one') + field = 'zapzap' + value = u'meadows and flowers' + document_data = {field: value} + option = MergeOption() + ret_val = batch.set(reference, document_data, option) + self.assertIsNone(ret_val) + new_write_pb = write_pb2.Write( + update=document_pb2.Document( + name=reference._document_path, + fields={ + field: _value_pb(string_value=value), + }, + ), + update_mask={'field_paths':[field]} + ) + self.assertEqual(batch._write_pbs, [new_write_pb]) + def test_update(self): from google.cloud.firestore_v1beta1.proto import common_pb2 from google.cloud.firestore_v1beta1.proto import document_pb2 diff --git a/firestore/tests/unit/test_client.py b/firestore/tests/unit/test_client.py index edb0bdfcdccf..314fc505ea6e 100644 --- a/firestore/tests/unit/test_client.py +++ b/firestore/tests/unit/test_client.py @@ -179,19 +179,6 @@ def test_field_path(self): klass = self._get_target_class() self.assertEqual(klass.field_path('a', 'b', 'c'), 'a.b.c') - def test_write_option_create(self): - from google.cloud.firestore_v1beta1.client import CreateIfMissingOption - - klass = self._get_target_class() - - option1 = klass.write_option(create_if_missing=False) - self.assertIsInstance(option1, CreateIfMissingOption) - self.assertFalse(option1._create_if_missing) - - option2 = klass.write_option(create_if_missing=True) - self.assertIsInstance(option2, CreateIfMissingOption) - self.assertTrue(option2._create_if_missing) - def test_write_option_last_update(self): from google.protobuf import timestamp_pb2 from google.cloud.firestore_v1beta1.client import LastUpdateOption @@ -234,7 +221,7 @@ def test_write_multiple_args(self): klass = self._get_target_class() with self.assertRaises(TypeError) as exc_info: klass.write_option( - create_if_missing=False, + exists=False, last_update_time=mock.sentinel.timestamp) self.assertEqual(exc_info.exception.args, (_BAD_OPTION_ERR,)) @@ -473,59 +460,6 @@ def test_modify_write_update_time(self): self.assertEqual(write_pb.current_document, expected_doc) -class TestCreateIfMissingOption(unittest.TestCase): - - @staticmethod - def _get_target_class(): - from google.cloud.firestore_v1beta1.client import CreateIfMissingOption - - return CreateIfMissingOption - - def _make_one(self, *args, **kwargs): - klass = self._get_target_class() - return klass(*args, **kwargs) - - def test_constructor(self): - option = self._make_one(mock.sentinel.totes_bool) - self.assertIs(option._create_if_missing, mock.sentinel.totes_bool) - - def test_modify_write_dont_create(self): - from google.cloud.firestore_v1beta1.proto import common_pb2 - from google.cloud.firestore_v1beta1.proto import write_pb2 - - option = self._make_one(False) - write_pb = write_pb2.Write() - ret_val = option.modify_write(write_pb) - - self.assertIsNone(ret_val) - expected_doc = common_pb2.Precondition(exists=True) - self.assertEqual(write_pb.current_document, expected_doc) - - def test_modify_write_do_create(self): - from google.cloud.firestore_v1beta1.proto import write_pb2 - - option = self._make_one(True) - write_pb = write_pb2.Write() - ret_val = option.modify_write(write_pb) - - self.assertIsNone(ret_val) - # No precondition is set here. - self.assertFalse(write_pb.HasField('current_document')) - - def test_modify_write_create_not_allowed(self): - no_create_msg = mock.sentinel.message - option1 = self._make_one(True) - option2 = self._make_one(False) - - with self.assertRaises(ValueError) as exc_info: - option1.modify_write(None, no_create_msg=no_create_msg) - self.assertEqual(exc_info.exception.args, (no_create_msg,)) - - with self.assertRaises(ValueError) as exc_info: - option2.modify_write(None, no_create_msg=no_create_msg) - self.assertEqual(exc_info.exception.args, (no_create_msg,)) - - class TestExistsOption(unittest.TestCase): @staticmethod diff --git a/firestore/tests/unit/test_document.py b/firestore/tests/unit/test_document.py index 8607c6fb6a6e..5a3eb9af1bca 100644 --- a/firestore/tests/unit/test_document.py +++ b/firestore/tests/unit/test_document.py @@ -249,6 +249,7 @@ def _write_pb_for_set(document_path, document_data): ) def _set_helper(self, **option_kwargs): + from google.cloud.firestore_v1beta1._helpers import FieldPathHelper # Create a minimal fake GAPIC with a dummy response. firestore_api = mock.Mock(spec=['commit']) commit_response = mock.Mock( @@ -277,8 +278,10 @@ def _set_helper(self, **option_kwargs): self.assertIs(write_result, mock.sentinel.write_result) write_pb = self._write_pb_for_set( document._document_path, document_data) + update_values, field_paths = FieldPathHelper.to_field_paths(document_data) + if option is not None: - option.modify_write(write_pb) + option.modify_write(write_pb, field_paths=field_paths) firestore_api.commit.assert_called_once_with( client._database_string, [write_pb], transaction=None, options=client._call_options) @@ -286,9 +289,6 @@ def _set_helper(self, **option_kwargs): def test_set(self): self._set_helper() - def test_set_with_option(self): - self._set_helper(create_if_missing=False) - @staticmethod def _write_pb_for_update(document_path, update_values, field_paths): from google.cloud.firestore_v1beta1.proto import common_pb2 @@ -355,7 +355,7 @@ def test_update(self): self._update_helper() def test_update_with_option(self): - self._update_helper(create_if_missing=False) + self._update_helper(exists=False) def _delete_helper(self, **option_kwargs): from google.cloud.firestore_v1beta1.proto import write_pb2 @@ -400,13 +400,6 @@ def test_delete_with_option(self): ) self._delete_helper(last_update_time=timestamp_pb) - def test_delete_with_bad_option(self): - from google.cloud.firestore_v1beta1._helpers import NO_CREATE_ON_DELETE - - with self.assertRaises(ValueError) as exc_info: - self._delete_helper(create_if_missing=True) - self.assertEqual(exc_info.exception.args, (NO_CREATE_ON_DELETE,)) - def test_get_success(self): # Create a minimal fake client with a dummy response. response_iterator = iter([mock.sentinel.snapshot])