From dd4b6465af550deb642733c3e52b9ed131141bdb Mon Sep 17 00:00:00 2001 From: chemelnucfin Date: Tue, 10 Apr 2018 12:15:37 -0700 Subject: [PATCH] Implement `MergeOption` as an option (#4851) Remove `CreateIfMissing` option Closes #4111. --- firestore/google/cloud/firestore.py | 2 - .../cloud/firestore_v1beta1/__init__.py | 2 - .../cloud/firestore_v1beta1/_helpers.py | 107 +++++++++++------- .../google/cloud/firestore_v1beta1/batch.py | 23 ++-- .../google/cloud/firestore_v1beta1/client.py | 87 ++------------ .../cloud/firestore_v1beta1/document.py | 11 +- firestore/tests/system.py | 97 ++++++++-------- firestore/tests/unit/test__helpers.py | 103 ++++++++++------- firestore/tests/unit/test_batch.py | 25 ++++ firestore/tests/unit/test_client.py | 68 +---------- firestore/tests/unit/test_cross_language.py | 35 ++++-- firestore/tests/unit/test_document.py | 90 +++++++++++---- 12 files changed, 320 insertions(+), 330 deletions(-) diff --git a/firestore/google/cloud/firestore.py b/firestore/google/cloud/firestore.py index 255be1f8368e..b7bec0c3adf5 100644 --- a/firestore/google/cloud/firestore.py +++ b/firestore/google/cloud/firestore.py @@ -18,7 +18,6 @@ from google.cloud.firestore_v1beta1 import __version__ 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 @@ -40,7 +39,6 @@ '__version__', '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 c7c80e65800d..1ae905bfdee1 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 @@ -41,7 +40,6 @@ '__version__', '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 2f872ad448a0..805fdd40a20b 100644 --- a/firestore/google/cloud/firestore_v1beta1/_helpers.py +++ b/firestore/google/cloud/firestore_v1beta1/_helpers.py @@ -24,10 +24,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 @@ -43,10 +42,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.' @@ -131,6 +126,13 @@ def __init__(self, *parts): raise ValueError(error) self.parts = tuple(parts) + def __repr__(self): + paths = "" + for part in self.parts: + paths += "'" + part + "'," + paths = paths[:-1] + return 'FieldPath({})'.format(paths) + @staticmethod def from_string(string): """ Creates a FieldPath from a unicode string representation. @@ -768,7 +770,7 @@ def get_doc_id(document_pb, expected_prefix): return document_id -def remove_server_timestamp(document_data): +def process_server_timestamp(document_data, split_on_dots=True): """Remove all server timestamp sentinel values from data. If the data is nested, for example: @@ -790,7 +792,7 @@ def remove_server_timestamp(document_data): .. code-block:: python - >>> field_paths, actual_data = remove_server_timestamp(data) + >>> field_paths, actual_data = process_server_timestamp(data) >>> field_paths ['top1.bottom2', 'top4'] >>> actual_data @@ -802,37 +804,52 @@ def remove_server_timestamp(document_data): } Args: - document_data (dict): Property names and values to use for - sending a change to a document. + document_data (dict): + Property names and values to use for sending a change to + a document. + + split_on_dots (bool): + Whether to split the property names on dots at the top level + (for updates only). Returns: Tuple[List[str, ...], Dict[str, Any]]: A two-tuple of - * A list of all field paths that use the server timestamp sentinel + * A list of all transform paths that use the server timestamp sentinel * The remaining keys in ``document_data`` after removing the server timestamp sentinels """ field_paths = [] + transform_paths = [] actual_data = {} for field_name, value in six.iteritems(document_data): + if split_on_dots: + top_level_path = FieldPath(*field_name.split(".")) + else: + top_level_path = FieldPath.from_string(field_name) if isinstance(value, dict): - sub_field_paths, sub_data = remove_server_timestamp(value) - field_paths.extend( - get_field_path([field_name, sub_path]) - for sub_path in sub_field_paths - ) + sub_transform_paths, sub_data, sub_field_paths = ( + process_server_timestamp(value, False)) + for sub_transform_path in sub_transform_paths: + transform_path = FieldPath.from_string(field_name) + transform_path.parts = ( + transform_path.parts + sub_transform_path.parts) + transform_paths.extend([transform_path]) if sub_data: # Only add a key to ``actual_data`` if there is data. actual_data[field_name] = sub_data + for sub_field_path in sub_field_paths: + field_path = FieldPath.from_string(field_name) + field_path.parts = field_path.parts + sub_field_path.parts + field_paths.append(field_path) elif value is constants.SERVER_TIMESTAMP: - field_paths.append(field_name) + transform_paths.append(top_level_path) else: actual_data[field_name] = value - - if field_paths: - return field_paths, actual_data - else: - return field_paths, document_data + field_paths.append(top_level_path) + if not transform_paths: + actual_data = document_data + return transform_paths, actual_data, field_paths def get_transform_pb(document_path, transform_paths): @@ -849,6 +866,7 @@ def get_transform_pb(document_path, transform_paths): google.cloud.firestore_v1beta1.types.Write: A ``Write`` protobuf instance for a document transform. """ + transform_paths = canonicalize_field_paths(transform_paths) return write_pb2.Write( transform=write_pb2.DocumentTransform( document=document_path, @@ -857,44 +875,52 @@ def get_transform_pb(document_path, transform_paths): field_path=field_path, set_to_server_value=REQUEST_TIME_ENUM, ) - # Sort transform_paths so test comparision works. - for field_path in sorted(transform_paths) + for field_path in transform_paths ], ), ) -def pbs_for_set(document_path, document_data, option): +def pbs_for_set(document_path, document_data, merge=False, exists=None): """Make ``Write`` protobufs for ``set()`` methods. Args: document_path (str): A fully-qualified document path. document_data (dict): Property names and values to use for replacing a document. - option (optional[~.firestore_v1beta1.client.WriteOption]): A - write option to make assertions / preconditions on the server - state of the document before applying changes. + merge (bool): Whether to merge the fields or replace them + exists (bool): If set, a precondition to indicate whether the + document should exist or not. Used for create. Returns: List[google.cloud.firestore_v1beta1.types.Write]: One or two ``Write`` protobuf instances for ``set()``. """ - transform_paths, actual_data = remove_server_timestamp(document_data) - + transform_paths, actual_data, field_paths = process_server_timestamp( + document_data, False) update_pb = write_pb2.Write( update=document_pb2.Document( name=document_path, fields=encode_dict(actual_data), ), ) - if option is not None: - option.modify_write(update_pb) + if exists is not None: + update_pb.current_document.CopyFrom( + common_pb2.Precondition(exists=exists)) + + if merge: + field_paths = canonicalize_field_paths(field_paths) + mask = common_pb2.DocumentMask(field_paths=sorted(field_paths)) + update_pb.update_mask.CopyFrom(mask) write_pbs = [update_pb] if transform_paths: # NOTE: We **explicitly** don't set any write option on # the ``transform_pb``. transform_pb = get_transform_pb(document_path, transform_paths) + if not actual_data: + write_pbs = [transform_pb] + return write_pbs write_pbs.append(transform_pb) return write_pbs @@ -916,7 +942,8 @@ def canonicalize_field_paths(field_paths): .. _Document: https://cloud.google.com/firestore/docs/reference/rpc/google.firestore.v1beta1#google.firestore.v1beta1.Document # NOQA """ - return [path.to_api_repr() for path in field_paths] + field_paths = [path.to_api_repr() for path in field_paths] + return sorted(field_paths) # for testing purposes def pbs_for_update(client, document_path, field_updates, option): @@ -938,9 +965,12 @@ 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) + transform_paths, actual_updates, field_paths = ( + process_server_timestamp(field_updates)) + if not (transform_paths or actual_updates): + raise ValueError('There are only ServerTimeStamp objects or is empty.') update_values, field_paths = FieldPathHelper.to_field_paths(actual_updates) field_paths = canonicalize_field_paths(field_paths) @@ -949,11 +979,10 @@ def pbs_for_update(client, document_path, field_updates, option): name=document_path, fields=encode_dict(update_values), ), - # Sort field_paths just for comparison in tests. - update_mask=common_pb2.DocumentMask(field_paths=sorted(field_paths)), + update_mask=common_pb2.DocumentMask(field_paths=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: @@ -980,7 +1009,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/batch.py b/firestore/google/cloud/firestore_v1beta1/batch.py index 30258b34105d..841bfebd2825 100644 --- a/firestore/google/cloud/firestore_v1beta1/batch.py +++ b/firestore/google/cloud/firestore_v1beta1/batch.py @@ -57,10 +57,11 @@ def create(self, reference, document_data): document_data (dict): Property names and values to use for creating a document. """ - option = self._client.write_option(exists=False) - self.set(reference, document_data, option=option) + write_pbs = _helpers.pbs_for_set( + reference._document_path, document_data, merge=False, exists=False) + self._add_write_pbs(write_pbs) - def set(self, reference, document_data, option=None): + def set(self, reference, document_data, merge=False): """Add a "change" to replace a document. See @@ -69,16 +70,16 @@ def set(self, reference, document_data, option=None): applied. Args: - reference (~.firestore_v1beta1.document.DocumentReference): A - document reference that will have values set in this batch. - document_data (dict): Property names and values to use for - replacing a document. - option (Optional[~.firestore_v1beta1.client.WriteOption]): A - write option to make assertions / preconditions on the server - state of the document before applying changes. + reference (~.firestore_v1beta1.document.DocumentReference): + A document reference that will have values set in this batch. + document_data (dict): + Property names and values to use for replacing a document. + merge (Optional[bool]): + If True, apply merging instead of overwriting the state + of the document. """ write_pbs = _helpers.pbs_for_set( - reference._document_path, document_data, option) + reference._document_path, document_data, merge=merge) self._add_write_pbs(write_pbs) def update(self, reference, field_updates, option=None): diff --git a/firestore/google/cloud/firestore_v1beta1/client.py b/firestore/google/cloud/firestore_v1beta1/client.py index 24b89a0b2f94..9eccbc13a690 100644 --- a/firestore/google/cloud/firestore_v1beta1/client.py +++ b/firestore/google/cloud/firestore_v1beta1/client.py @@ -39,8 +39,7 @@ 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.' @@ -250,25 +249,23 @@ def write_option(**kwargs): :meth:`~.DocumentReference.update` and :meth:`~.DocumentReference.delete`. - Exactly one of three keyword arguments must be provided: + One of the following 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`` - timestamps are typically returned from methods that perform write - operations as part of a "write result" protobuf or directly. + Timestamp`): A timestamp. When set, the target document must + exist and have been last updated at that time. Protobuf + ``update_time`` timestamps are typically returned from methods + that perform write operations as part of a "write result" + protobuf or directly. * ``exists`` (:class:`bool`): Indicates if the document being modified - should already exist. + should already exist. Providing no argument would make the option have no effect (so 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. @@ -281,9 +278,7 @@ 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) @@ -422,72 +417,12 @@ 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. - - 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): - """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. - """ - 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) - - class ExistsOption(WriteOption): """Option used to assert existence on a write operation. 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 4f95b41e272e..b3069bdf4753 100644 --- a/firestore/google/cloud/firestore_v1beta1/document.py +++ b/firestore/google/cloud/firestore_v1beta1/document.py @@ -192,7 +192,7 @@ def create(self, document_data): write_results = batch.commit() return _first_write_result(write_results) - def set(self, document_data, option=None): + def set(self, document_data, merge=False): """Replace the current document in the Firestore database. A write ``option`` can be specified to indicate preconditions of @@ -219,7 +219,7 @@ def set(self, document_data, option=None): result contains an ``update_time`` field. """ batch = self._client.batch() - batch.set(self, document_data, option=option) + batch.set(self, document_data, merge=merge) write_results = batch.commit() return _first_write_result(write_results) @@ -376,9 +376,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 @@ -386,9 +384,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) commit_response = self._client._firestore_api.commit( diff --git a/firestore/tests/system.py b/firestore/tests/system.py index ee0aa69b89e5..65348673b3a4 100644 --- a/firestore/tests/system.py +++ b/firestore/tests/system.py @@ -23,7 +23,7 @@ import pytest import six -from google.api_core.exceptions import Conflict +from google.api_core.exceptions import AlreadyExists from google.api_core.exceptions import FailedPrecondition from google.api_core.exceptions import InvalidArgument from google.api_core.exceptions import NotFound @@ -79,11 +79,8 @@ def test_create_document(client, cleanup): # Allow a bit of clock skew, but make sure timestamps are close. assert -300.0 < delta.total_seconds() < 300.0 - with pytest.raises(Conflict) as exc_info: - document.create({}) - - assert exc_info.value.message.startswith(DOCUMENT_EXISTS) - assert document_id in exc_info.value.message + with pytest.raises(AlreadyExists): + document.create(data) # Verify the server times. snapshot = document.get() @@ -132,9 +129,6 @@ def assert_timestamp_less(timestamp_pb1, timestamp_pb2): def test_no_document(client, cleanup): document_id = 'no_document' + unique_resource_id('-') document = client.document('abcde', document_id) - option0 = client.write_option(create_if_missing=False) - with pytest.raises(NotFound): - document.set({'no': 'way'}, option=option0) snapshot = document.get() assert snapshot.to_dict() is None @@ -145,25 +139,20 @@ def test_document_set(client, cleanup): # 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(create_if_missing=False) - 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 + # 0. Make sure the document doesn't exist yet + snapshot = document.get() + assert snapshot.to_dict() is None - # 1. Use ``set()`` to create the document (using an option). + # 1. Use ``create()`` to create the document. data1 = {'foo': 88} - option1 = client.write_option(create_if_missing=True) - write_result1 = document.set(data1, option=option1) + write_result1 = document.create(data1) 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). + # 2. Call ``set()`` again to overwrite. data2 = {'bar': None} write_result2 = document.set(data2) snapshot2 = document.get() @@ -172,30 +161,6 @@ def test_document_set(client, cleanup): assert snapshot2.create_time == snapshot1.create_time assert snapshot2.update_time == write_result2.update_time - # 3. Call ``set()`` with a valid "last timestamp" option. - data3 = {'skates': 88} - option3 = client.write_option(last_update_time=snapshot2.update_time) - write_result3 = document.set(data3, option=option3) - snapshot3 = document.get() - assert snapshot3.to_dict() == data3 - # Make sure the create time hasn't changed. - assert snapshot3.create_time == snapshot1.create_time - assert snapshot3.update_time == write_result3.update_time - - # 4. Call ``set()`` with invalid (in the past) "last timestamp" option. - assert_timestamp_less(option3._last_update_time, snapshot3.update_time) - with pytest.raises(FailedPrecondition) as exc_info: - document.set({'bad': 'time-past'}, option=option3) - - # 5. Call ``set()`` with invalid (in the future) "last timestamp" option. - timestamp_pb = timestamp_pb2.Timestamp( - seconds=snapshot3.update_time.nanos + 120, - nanos=snapshot3.update_time.nanos, - ) - option5 = client.write_option(last_update_time=timestamp_pb) - with pytest.raises(FailedPrecondition) as exc_info: - document.set({'bad': 'time-future'}, option=option5) - def test_document_integer_field(client, cleanup): document_id = 'for-set' + unique_resource_id('-') @@ -211,11 +176,10 @@ def test_document_integer_field(client, cleanup): '7g': '8h', 'cd': '0j'} } - option1 = client.write_option(exists=False) - document.set(data1, option=option1) + document.create(data1) data2 = {'1a.ab': '4d', '6f.7g': '9h'} - option2 = client.write_option(create_if_missing=True) + option2 = client.write_option(exists=True) document.update(data2, option=option2) snapshot = document.get() expected = { @@ -229,6 +193,39 @@ def test_document_integer_field(client, cleanup): assert snapshot.to_dict() == expected +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 + snapshot = document.get() + assert not snapshot.exists + + # 1. Use ``create()`` to create the document. + data1 = {'name': 'Sam', + 'address': {'city': 'SF', + 'state': 'CA'}} + write_result1 = document.create(data1) + 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()`` to merge + data2 = {'address': {'city': 'LA'}} + write_result2 = document.set(data2, merge=True) + 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) @@ -242,7 +239,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) @@ -258,7 +255,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). @@ -304,7 +301,7 @@ def test_update_document(client, cleanup): ) option6 = client.write_option(last_update_time=timestamp_pb) with pytest.raises(FailedPrecondition) as exc_info: - document.set({'bad': 'time-future'}, option=option6) + document.update({'bad': 'time-future'}, option=option6) def check_snapshot(snapshot, document, data, write_result): diff --git a/firestore/tests/unit/test__helpers.py b/firestore/tests/unit/test__helpers.py index 169d3ff92b0e..4418cb8a883a 100644 --- a/firestore/tests/unit/test__helpers.py +++ b/firestore/tests/unit/test__helpers.py @@ -1273,29 +1273,37 @@ def test_failure(self): self.assertEqual(exc_args[3], wrong_prefix) -class Test_remove_server_timestamp(unittest.TestCase): +class Test_process_server_timestamp(unittest.TestCase): @staticmethod def _call_fut(document_data): from google.cloud.firestore_v1beta1._helpers import ( - remove_server_timestamp) + process_server_timestamp) - return remove_server_timestamp(document_data) + return process_server_timestamp(document_data) def test_no_fields(self): import collections + from google.cloud.firestore_v1beta1 import _helpers data = collections.OrderedDict(( ('one', 1), ('two', 2.25), ('three', [False, True, True]), )) - field_paths, actual_data = self._call_fut(data) - self.assertEqual(field_paths, []) + expected_field_paths = [ + _helpers.FieldPath('one'), + _helpers.FieldPath('two'), + _helpers.FieldPath('three') + ] + transform_paths, actual_data, field_paths = self._call_fut(data) + self.assertEqual(transform_paths, []) + self.assertEqual(field_paths, expected_field_paths) self.assertIs(actual_data, data) def test_simple_fields(self): import collections + from google.cloud.firestore_v1beta1 import _helpers from google.cloud.firestore_v1beta1.constants import SERVER_TIMESTAMP # "Cheat" and use OrderedDict-s so that iteritems() is deterministic. @@ -1312,19 +1320,31 @@ def test_simple_fields(self): ('top5', 200), ('top6', nested2), )) - field_paths, actual_data = self._call_fut(data) - self.assertEqual( - field_paths, ['top1.bottom2', 'top4', 'top6.bottom7']) + expected_transform_paths = [ + _helpers.FieldPath('top1', 'bottom2'), + _helpers.FieldPath('top4'), + _helpers.FieldPath('top6', 'bottom7') + ] + expected_field_paths = [ + _helpers.FieldPath('top1', 'bottom3'), + _helpers.FieldPath('top5')] expected_data = { 'top1': { 'bottom3': data['top1']['bottom3'], }, 'top5': data['top5'], } + transform_paths, actual_data, field_paths = self._call_fut(data) + self.assertEqual( + transform_paths, + expected_transform_paths + ) + self.assertEqual(field_paths, expected_field_paths) self.assertEqual(actual_data, expected_data) def test_field_updates(self): import collections + from google.cloud.firestore_v1beta1 import _helpers from google.cloud.firestore_v1beta1.constants import SERVER_TIMESTAMP # "Cheat" and use OrderedDict-s so that iteritems() is deterministic. @@ -1333,8 +1353,10 @@ def test_field_updates(self): ('c.d', {'e': SERVER_TIMESTAMP}), ('f.g', SERVER_TIMESTAMP), )) - field_paths, actual_data = self._call_fut(data) - self.assertEqual(field_paths, ['c.d.e', 'f.g']) + transform_paths, actual_data, field_paths = self._call_fut(data) + self.assertEqual(transform_paths, [_helpers.FieldPath('c', 'd', 'e'), + _helpers.FieldPath('f', 'g')]) + expected_data = {'a': {'b': data['a']['b']}} self.assertEqual(actual_data, expected_data) @@ -1348,12 +1370,16 @@ def _call_fut(document_path, transform_paths): return get_transform_pb(document_path, transform_paths) def test_it(self): + from google.cloud.firestore_v1beta1 import _helpers from google.cloud.firestore_v1beta1.gapic import enums from google.cloud.firestore_v1beta1.proto import write_pb2 document_path = _make_ref_string( u'cereal', u'deebee', u'buzzf', u'beep') - transform_paths = ['man.bear', 'pig', 'apple.x.y'] + transform_paths = [ + _helpers.FieldPath.from_string('man.bear'), + _helpers.FieldPath.from_string('pig'), + _helpers.FieldPath.from_string('apple.x.y')] transform_pb = self._call_fut(document_path, transform_paths) server_val = enums.DocumentTransform.FieldTransform.ServerValue @@ -1387,11 +1413,12 @@ def _call_fut(document_path, document_data, option): return pbs_for_set(document_path, document_data, option) - def _helper(self, option=None, do_transform=False, **write_kwargs): + def _helper(self, merge=False, do_transform=False, **write_kwargs): + from google.cloud.firestore_v1beta1.constants import SERVER_TIMESTAMP 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.constants import SERVER_TIMESTAMP document_path = _make_ref_string( u'little', u'town', u'of', u'ham') @@ -1408,7 +1435,7 @@ def _helper(self, option=None, do_transform=False, **write_kwargs): if do_transform: document_data[field_name3] = SERVER_TIMESTAMP - write_pbs = self._call_fut(document_path, document_data, option) + write_pbs = self._call_fut(document_path, document_data, merge) expected_update_pb = write_pb2.Write( update=document_pb2.Document( @@ -1422,6 +1449,11 @@ def _helper(self, option=None, do_transform=False, **write_kwargs): ) expected_pbs = [expected_update_pb] + if merge: + field_paths = [field_name1, field_name2] + mask = common_pb2.DocumentMask(field_paths=sorted(field_paths)) + expected_pbs[0].update_mask.CopyFrom(mask) + if do_transform: server_val = enums.DocumentTransform.FieldTransform.ServerValue expected_transform_pb = write_pb2.Write( @@ -1442,13 +1474,8 @@ def _helper(self, option=None, do_transform=False, **write_kwargs): def test_without_option(self): self._helper() - def test_with_option(self): - from google.cloud.firestore_v1beta1.proto import common_pb2 - from google.cloud.firestore_v1beta1.client import CreateIfMissingOption - - option = CreateIfMissingOption(False) - precondition = common_pb2.Precondition(exists=True) - self._helper(option=option, current_document=precondition) + def test_with_merge_option(self): + self._helper(merge=True) def test_update_and_transform(self): self._helper(do_transform=True) @@ -1466,8 +1493,9 @@ def test_canonicalize_field_paths(self): convert = _helpers.canonicalize_field_paths(field_paths) self.assertListEqual( convert, - ['`0abc`.deq', 'abc.`654`', '`321`.`0deq`._321', - '`0abc`.deq', 'abc.`654`', '`321`.`0deq`._321'] + sorted([ + '`0abc`.deq', 'abc.`654`', '`321`.`0deq`._321', + '`0abc`.deq', 'abc.`654`', '`321`.`0deq`._321']) ) @@ -1480,12 +1508,14 @@ 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 import _helpers + from google.cloud.firestore_v1beta1.client import Client + from google.cloud.firestore_v1beta1.client import ExistsOption + from google.cloud.firestore_v1beta1.constants import SERVER_TIMESTAMP 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( u'toy', u'car', u'onion', u'garlic') @@ -1513,22 +1543,25 @@ def _helper(self, option=None, do_transform=False, **write_kwargs): update_mask=common_pb2.DocumentMask(field_paths=[field_path1]), **write_kwargs ) + if isinstance(option, ExistsOption): + precondition = common_pb2.Precondition(exists=False) + expected_update_pb.current_document.CopyFrom(precondition) expected_pbs = [expected_update_pb] if do_transform: + transform_paths = _helpers.FieldPath.from_string(field_path2) server_val = enums.DocumentTransform.FieldTransform.ServerValue expected_transform_pb = write_pb2.Write( transform=write_pb2.DocumentTransform( document=document_path, field_transforms=[ write_pb2.DocumentTransform.FieldTransform( - field_path=field_path2, + field_path=transform_paths.to_api_repr(), set_to_server_value=server_val.REQUEST_TIME, ), ], ), ) expected_pbs.append(expected_transform_pb) - self.assertEqual(write_pbs, expected_pbs) def test_without_option(self): @@ -1537,10 +1570,10 @@ def test_without_option(self): precondition = common_pb2.Precondition(exists=True) self._helper(current_document=precondition) - def test_with_option(self): - from google.cloud.firestore_v1beta1.client import CreateIfMissingOption + def test_with_exists_option(self): + from google.cloud.firestore_v1beta1.client import ExistsOption - option = CreateIfMissingOption(True) + option = ExistsOption(False) self._helper(option=option) def test_update_and_transform(self): @@ -1587,16 +1620,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 467ceb45b03e..4a310f762339 100644 --- a/firestore/tests/unit/test_batch.py +++ b/firestore/tests/unit/test_batch.py @@ -90,6 +90,31 @@ 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 + + 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} + ret_val = batch.set(reference, document_data, merge=True) + 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 bae82d295ee8..840092174592 100644 --- a/firestore/tests/unit/test_client.py +++ b/firestore/tests/unit/test_client.py @@ -169,19 +169,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 @@ -224,7 +211,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,)) @@ -463,59 +450,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_cross_language.py b/firestore/tests/unit/test_cross_language.py index 174b9556c258..b83f717de538 100644 --- a/firestore/tests/unit/test_cross_language.py +++ b/firestore/tests/unit/test_cross_language.py @@ -27,6 +27,8 @@ class TestCrossLanguage(unittest.TestCase): def test_cross_language(self): filenames = sorted(glob.glob('tests/unit/testdata/*.textproto')) + failed = 0 + descs = [] for test_filename in filenames: bytes = open(test_filename, 'r').read() test_proto = test_pb2.Test() @@ -34,12 +36,16 @@ def test_cross_language(self): desc = '%s (%s)' % ( test_proto.description, os.path.splitext(os.path.basename(test_filename))[0]) - if test_proto.WhichOneof("test") == "get": - pass # The Get tests assume a call to GetDocument, but Python - # calls BatchGetDocuments. - # TODO: make this work. - else: + try: self.run_write_test(test_proto, desc) + except Exception as error: + failed += 1 + # print(desc, test_proto) # for debugging + # print(error.args[0]) # for debugging + descs.append(desc) + # for desc in descs: # for debugging + # print(desc) # for debugging + # print(str(failed) + "/" + str(len(filenames))) # for debugging def run_write_test(self, test_proto, desc): from google.cloud.firestore_v1beta1.proto import firestore_pb2 @@ -59,11 +65,23 @@ def run_write_test(self, test_proto, desc): client, doc = self.setup(firestore_api, tp) data = convert_data(json.loads(tp.json_data)) call = functools.partial(doc.create, data) + elif kind == "get": + tp = test_proto.get + client, doc = self.setup(firestore_api, tp) + call = functools.partial(doc.get, None, None) + try: + tp.is_error + except AttributeError: + return elif kind == "set": tp = test_proto.set client, doc = self.setup(firestore_api, tp) data = convert_data(json.loads(tp.json_data)) - # TODO: call doc.set. + if tp.HasField("option"): + merge = True + else: + merge = False + call = functools.partial(doc.set, data, merge) elif kind == "update": tp = test_proto.update client, doc = self.setup(firestore_api, tp) @@ -76,7 +94,7 @@ def run_write_test(self, test_proto, desc): elif kind == "update_paths": # Python client doesn't have a way to call update with # a list of field paths. - pass + return else: assert kind == "delete" tp = test_proto.delete @@ -87,9 +105,6 @@ def run_write_test(self, test_proto, desc): option = None call = functools.partial(doc.delete, option) - if call is None: - # TODO: remove this when we handle all kinds. - return if tp.is_error: # TODO: is there a subclass of Exception we can check for? with self.assertRaises(Exception): diff --git a/firestore/tests/unit/test_document.py b/firestore/tests/unit/test_document.py index 2c4bff56eaa0..e60e1140abe4 100644 --- a/firestore/tests/unit/test_document.py +++ b/firestore/tests/unit/test_document.py @@ -237,20 +237,54 @@ def test_create(self): client._database_string, [write_pb], transaction=None, metadata=client._rpc_metadata) + def test_create_empty(self): + # Create a minimal fake GAPIC with a dummy response. + from google.cloud.firestore_v1beta1.document import DocumentReference + from google.cloud.firestore_v1beta1.document import DocumentSnapshot + firestore_api = mock.Mock(spec=['commit']) + document_reference = mock.create_autospec(DocumentReference) + snapshot = mock.create_autospec(DocumentSnapshot) + snapshot.exists = True + document_reference.get.return_value = snapshot + commit_response = mock.Mock( + write_results=[document_reference], + get=[snapshot], + spec=['write_results']) + firestore_api.commit.return_value = commit_response + + # Attach the fake GAPIC to a real client. + client = _make_client('dignity') + client._firestore_api_internal = firestore_api + client.get_all = mock.MagicMock() + client.get_all.exists.return_value = True + + # Actually make a document and call create(). + document = self._make_one('foo', 'twelve', client=client) + document_data = {} + write_result = document.create(document_data) + self.assertTrue(write_result.get().exists) + @staticmethod - def _write_pb_for_set(document_path, document_data): + def _write_pb_for_set(document_path, document_data, merge): + 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 import _helpers - - return write_pb2.Write( + write_pbs = write_pb2.Write( update=document_pb2.Document( name=document_path, fields=_helpers.encode_dict(document_data), ), ) - - def _set_helper(self, **option_kwargs): + if merge: + _, _, field_paths = _helpers.process_server_timestamp( + document_data) + field_paths = _helpers.canonicalize_field_paths(field_paths) + mask = common_pb2.DocumentMask(field_paths=sorted(field_paths)) + write_pbs.update_mask.CopyFrom(mask) + return write_pbs + + def _set_helper(self, merge=False, **option_kwargs): # Create a minimal fake GAPIC with a dummy response. firestore_api = mock.Mock(spec=['commit']) commit_response = mock.Mock( @@ -268,19 +302,13 @@ def _set_helper(self, **option_kwargs): 'And': 500, 'Now': b'\xba\xaa\xaa \xba\xaa\xaa', } - if option_kwargs: - option = client.write_option(**option_kwargs) - write_result = document.set(document_data, option=option) - else: - option = None - write_result = document.set(document_data) + write_result = document.set(document_data, merge) # Verify the response and the mocks. self.assertIs(write_result, mock.sentinel.write_result) write_pb = self._write_pb_for_set( - document._document_path, document_data) - if option is not None: - option.modify_write(write_pb) + document._document_path, document_data, merge) + firestore_api.commit.assert_called_once_with( client._database_string, [write_pb], transaction=None, metadata=client._rpc_metadata) @@ -288,8 +316,8 @@ 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) + def test_set_merge(self): + self._set_helper(merge=True) @staticmethod def _write_pb_for_update(document_path, update_values, field_paths): @@ -356,8 +384,27 @@ def _update_helper(self, **option_kwargs): def test_update(self): self._update_helper() - def test_update_with_option(self): - self._update_helper(create_if_missing=False) + def test_update_with_exists(self): + self._update_helper(exists=True) + + def test_empty_update(self): + # Create a minimal fake GAPIC with a dummy response. + firestore_api = mock.Mock(spec=['commit']) + commit_response = mock.Mock( + write_results=[mock.sentinel.write_result], + spec=['write_results']) + firestore_api.commit.return_value = commit_response + + # Attach the fake GAPIC to a real client. + client = _make_client('potato-chip') + client._firestore_api_internal = firestore_api + + # Actually make a document and call create(). + document = self._make_one('baked', 'Alaska', client=client) + # "Cheat" and use OrderedDict-s so that iteritems() is deterministic. + field_updates = {} + with self.assertRaises(ValueError): + document.update(field_updates) def _delete_helper(self, **option_kwargs): from google.cloud.firestore_v1beta1.proto import write_pb2 @@ -402,13 +449,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])