From d7fe72d257e0c13967f0ee5165d9c47bb447c5fa Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Mon, 27 Nov 2023 16:30:10 -0800 Subject: [PATCH 01/39] added begin_later flag --- google/cloud/datastore/client.py | 28 ++++++++++++----- google/cloud/datastore/helpers.py | 43 ++++++++++++--------------- google/cloud/datastore/transaction.py | 26 ++++++++++++++-- 3 files changed, 63 insertions(+), 34 deletions(-) diff --git a/google/cloud/datastore/client.py b/google/cloud/datastore/client.py index 3f5041d6..ce019c8f 100644 --- a/google/cloud/datastore/client.py +++ b/google/cloud/datastore/client.py @@ -122,7 +122,7 @@ def _extended_lookup( missing=None, deferred=None, eventual=False, - transaction_id=None, + transaction=None, retry=None, timeout=None, read_time=None, @@ -158,10 +158,10 @@ def _extended_lookup( consistency. If True, request ``EVENTUAL`` read consistency. - :type transaction_id: str - :param transaction_id: If passed, make the request in the scope of - the given transaction. Incompatible with - ``eventual==True`` or ``read_time``. + :type transaction: Transaction + :param transaction: If passed, make the request in the scope of + the given transaction. Incompatible with + ``eventual==True`` or ``read_time``. :type retry: :class:`google.api_core.retry.Retry` :param retry: @@ -177,7 +177,7 @@ def _extended_lookup( :type read_time: datetime :param read_time: (Optional) Read time to use for read consistency. Incompatible with - ``eventual==True`` or ``transaction_id``. + ``eventual==True`` or ``transaction``. This feature is in private preview. :type database: str @@ -199,8 +199,15 @@ def _extended_lookup( results = [] + transaction_id = None + new_transaction_options = None + if transaction is not None: + transaction_id = transaction.id + if transaction._begin_later and transaction_id is None and transaction._status == transaction._INITIAL: + new_transaction_options = transaction._options + loop_num = 0 - read_options = helpers.get_read_options(eventual, transaction_id, read_time) + read_options = helpers.get_read_options(eventual, transaction_id, read_time, new_transaction_options) while loop_num < _MAX_LOOPS: # loop against possible deferred. loop_num += 1 request = { @@ -214,6 +221,11 @@ def _extended_lookup( **kwargs, ) + # set new transaction id + if new_transaction_options is not None: + transaction._id = lookup_response.transaction + transaction._status = transaction._IN_PROGRESS + # Accumulate the new results. results.extend(result.entity for result in lookup_response.found) @@ -570,7 +582,7 @@ def get_multi( eventual=eventual, missing=missing, deferred=deferred, - transaction_id=transaction and transaction.id, + transaction=transaction, retry=retry, timeout=timeout, read_time=read_time, diff --git a/google/cloud/datastore/helpers.py b/google/cloud/datastore/helpers.py index e8894883..4b87c5ba 100644 --- a/google/cloud/datastore/helpers.py +++ b/google/cloud/datastore/helpers.py @@ -230,7 +230,7 @@ def entity_to_protobuf(entity): return entity_pb -def get_read_options(eventual, transaction_id, read_time=None): +def get_read_options(eventual, transaction_id, read_time=None, new_transaction_options=None): """Validate rules for read options, and assign to the request. Helper method for ``lookup()`` and ``run_query``. @@ -245,33 +245,28 @@ def get_read_options(eventual, transaction_id, read_time=None): :type read_time: datetime :param read_time: Read data from the specified time (may be null). This feature is in private preview. + :type new_transaction_options: :class:`google.cloud.datastore_v1.types.TransactionOptions` + :param new_transaction_options: Options for a new transaction. + :rtype: :class:`.datastore_pb2.ReadOptions` :returns: The read options corresponding to the inputs. :raises: :class:`ValueError` if more than one of ``eventual==True``, - ``transaction``, and ``read_time`` is specified. + ``transaction_id``, ``read_time``, and ``new_transaction_options`` is specified. """ - if transaction_id is None: - if eventual: - if read_time is not None: - raise ValueError("eventual must be False when read_time is specified") - else: - return datastore_pb2.ReadOptions( - read_consistency=datastore_pb2.ReadOptions.ReadConsistency.EVENTUAL - ) - else: - if read_time is None: - return datastore_pb2.ReadOptions() - else: - read_time_pb = timestamp_pb2.Timestamp() - read_time_pb.FromDatetime(read_time) - return datastore_pb2.ReadOptions(read_time=read_time_pb) - else: - if eventual: - raise ValueError("eventual must be False when in a transaction") - elif read_time is not None: - raise ValueError("transaction and read_time are mutual exclusive") - else: - return datastore_pb2.ReadOptions(transaction=transaction_id) + if sum(bool(x) for x in (eventual, transaction_id, read_time, new_transaction_options)) > 1: + raise ValueError("At most one of eventual, transaction, or read_time is allowed.") + new_options = datastore_pb2.ReadOptions() + if transaction_id is not None: + new_options.transaction = transaction_id + if read_time is not None: + read_time_pb = timestamp_pb2.Timestamp() + read_time_pb.FromDatetime(read_time) + new_options.read_time = read_time_pb + if new_transaction_options is not None: + new_options.new_transaction = new_transaction_options + if eventual: + new_options.read_consistency = datastore_pb2.ReadOptions.ReadConsistency.EVENTUAL + return new_options def key_from_protobuf(pb): diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 3e71ae26..29d3bb10 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -149,15 +149,22 @@ class Transaction(Batch): :param read_time: (Optional) Time at which the transaction reads entities. Only allowed when ``read_only=True``. This feature is in private preview. + :type begin_later: bool + :param begin_later: (Optional) If True, the transaction will be started + when the first RPC is made. If False, the transaction + will be started immediately. Default is True. + :raises: :class:`ValueError` if read_time is specified when ``read_only=False``. """ _status = None - def __init__(self, client, read_only=False, read_time=None): + def __init__(self, client, read_only=False, read_time=None, begin_later=False): + # TODO: begin_later defaults to True super(Transaction, self).__init__(client) self._id = None + self._begin_later = begin_later if read_only: if read_time is not None: @@ -180,7 +187,7 @@ def __init__(self, client, read_only=False, read_time=None): def id(self): """Getter for the transaction ID. - :rtype: str + :rtype: str or None :returns: The ID of the current transaction. """ return self._id @@ -320,4 +327,19 @@ def put(self, entity): if "read_only" in self._options: raise RuntimeError("Transaction is read only") else: + if self._begin_later and self._status == self._INITIAL: + # If we haven't begun yet, we need to do so now. + self.begin() super(Transaction, self).put(entity) + + def delete(self, key): + if self._begin_later and self._status == self._INITIAL: + # If we haven't begun yet, we need to do so now. + self.begin() + super(Transaction, self).delete(key) + + def __enter__(self): + if not self._begin_later: + self.begin() + self._client._push_batch(self) + return self From 14942e007c619f821bb1e1a9cd9115c17343d0f3 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Mon, 27 Nov 2023 16:45:43 -0800 Subject: [PATCH 02/39] refactoring --- google/cloud/datastore/client.py | 10 +++++----- google/cloud/datastore/transaction.py | 17 ++++++++++++++++- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/google/cloud/datastore/client.py b/google/cloud/datastore/client.py index ce019c8f..aaac9b07 100644 --- a/google/cloud/datastore/client.py +++ b/google/cloud/datastore/client.py @@ -203,7 +203,8 @@ def _extended_lookup( new_transaction_options = None if transaction is not None: transaction_id = transaction.id - if transaction._begin_later and transaction_id is None and transaction._status == transaction._INITIAL: + if transaction_id is None and transaction._begin_later and transaction._status == transaction._INITIAL: + # if transaction hasn't been initialized, initialize it as part of this request new_transaction_options = transaction._options loop_num = 0 @@ -221,10 +222,9 @@ def _extended_lookup( **kwargs, ) - # set new transaction id - if new_transaction_options is not None: - transaction._id = lookup_response.transaction - transaction._status = transaction._IN_PROGRESS + # set new transaction id if we just started a transaction + if transaction and lookup_response.transaction: + transaction._begin_with_id(lookup_response.transaction) # Accumulate the new results. results.extend(result.entity for result in lookup_response.found) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 29d3bb10..9e85a1be 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -188,7 +188,7 @@ def id(self): """Getter for the transaction ID. :rtype: str or None - :returns: The ID of the current transaction. + :returns: The ID of the current transaction, or None if not started. """ return self._id @@ -247,6 +247,21 @@ def begin(self, retry=None, timeout=None): self._status = self._ABORTED raise + def _begin_with_id(self, transaction_id): + """ + Attach newly created transaction to an existing transaction ID. + + This is used when begin_later is True, when the first lookup request + associated with this transaction creates a new transaction ID. + + :type transaction_id: str + :param transaction_id: ID of the transaction to attach to. + """ + if self._status is not self._INITIAL: + raise ValueError("Transaction already begun.") + self._id = transaction_id + self._status = self._IN_PROGRESS + def rollback(self, retry=None, timeout=None): """Rolls back the current transaction. From 8ce4654495f5f39b116ecb83927c9e60462713d3 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Mon, 27 Nov 2023 16:54:05 -0800 Subject: [PATCH 03/39] got tests passing --- google/cloud/datastore/transaction.py | 3 +-- tests/unit/test_client.py | 5 +++-- tests/unit/test_transaction.py | 3 +++ 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 9e85a1be..2039fd6c 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -160,8 +160,7 @@ class Transaction(Batch): _status = None - def __init__(self, client, read_only=False, read_time=None, begin_later=False): - # TODO: begin_later defaults to True + def __init__(self, client, read_only=False, read_time=None, begin_later=True): super(Transaction, self).__init__(client) self._id = None self._begin_later = begin_later diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 412f3923..8b02d59a 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -1847,7 +1847,7 @@ def _make_commit_response(*keys): return datastore_pb2.CommitResponse(mutation_results=mutation_results) -def _make_lookup_response(results=(), missing=(), deferred=()): +def _make_lookup_response(results=(), missing=(), deferred=(), transaction=None): entity_results_found = [ mock.Mock(entity=result, spec=["entity"]) for result in results ] @@ -1858,7 +1858,8 @@ def _make_lookup_response(results=(), missing=(), deferred=()): found=entity_results_found, missing=entity_results_missing, deferred=deferred, - spec=["found", "missing", "deferred"], + transaction=transaction, + spec=["found", "missing", "deferred", "transaction"], ) diff --git a/tests/unit/test_transaction.py b/tests/unit/test_transaction.py index 23574ef4..796f6b5d 100644 --- a/tests/unit/test_transaction.py +++ b/tests/unit/test_transaction.py @@ -518,6 +518,9 @@ def _make_options(read_only=False, previous_transaction=None, read_time=None): def _make_transaction(client, **kw): from google.cloud.datastore.transaction import Transaction + # default to begin_later=False + kw.setdefault("begin_later", False) + return Transaction(client, **kw) From 18d6a3478c56c26ebf0ad9ca659ac6c2979b5b42 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Mon, 27 Nov 2023 16:59:47 -0800 Subject: [PATCH 04/39] improved docstring --- google/cloud/datastore/transaction.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 2039fd6c..612cb294 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -151,8 +151,10 @@ class Transaction(Batch): :type begin_later: bool :param begin_later: (Optional) If True, the transaction will be started - when the first RPC is made. If False, the transaction - will be started immediately. Default is True. + lazily (i.e. when the first RPC is made). If False, + the transaction will be started as soon as the context manager + is entered. `self.begin()` can also be called manually to begin + the transaction at any time. Default is True. :raises: :class:`ValueError` if read_time is specified when ``read_only=False``. From af92f68c4496fd1e8ab0f05958e652586694b9e8 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 14:03:17 -0800 Subject: [PATCH 05/39] added helper tests --- tests/unit/test_helpers.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index 467a2df1..13c54b76 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -586,6 +586,31 @@ def test__get_read_options_w_default_wo_txn_w_read_time(): assert read_options == expected +def test__get_read_options_w_new_transaction(): + from google.cloud.datastore.helpers import get_read_options + from google.cloud.datastore_v1.types import datastore as datastore_pb2 + input_options = datastore_pb2.TransactionOptions() + read_options = get_read_options(False, None, new_transaction_options=input_options) + expected = datastore_pb2.ReadOptions(new_transaction=input_options) + assert read_options == expected + + +@pytest.mark.parametrize("args", [ + (True, "id"), (True, "id", None), (True, None, "read_time"), (True, None, None, "new"), + (False, "id", "read_time"), (False, "id", None, "new"), + (False, None, "read_time", "new"), +]) +def test__get_read_options_w_multiple_args(args): + """ + arguments are mutually exclusive. + Should raise ValueError if multiple are set + """ + from google.cloud.datastore.helpers import get_read_options + + with pytest.raises(ValueError): + get_read_options(*args) + + def test__pb_attr_value_w_datetime_naive(): import calendar import datetime From 11cedaca3e3c4a50e85858975253e4061702d1a6 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 14:18:22 -0800 Subject: [PATCH 06/39] fixed type in docstring --- google/cloud/datastore/transaction.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 612cb294..d9c39e02 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -188,7 +188,7 @@ def __init__(self, client, read_only=False, read_time=None, begin_later=True): def id(self): """Getter for the transaction ID. - :rtype: str or None + :rtype: bytes or None :returns: The ID of the current transaction, or None if not started. """ return self._id @@ -255,7 +255,7 @@ def _begin_with_id(self, transaction_id): This is used when begin_later is True, when the first lookup request associated with this transaction creates a new transaction ID. - :type transaction_id: str + :type transaction_id: bytes :param transaction_id: ID of the transaction to attach to. """ if self._status is not self._INITIAL: From 27454d54ee4be71788c92e4ad4ac5613712b768f Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 14:19:38 -0800 Subject: [PATCH 07/39] added client test --- tests/unit/test_client.py | 45 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 8b02d59a..1bf55866 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -705,6 +705,51 @@ def test_client_get_multi_hit_w_transaction(database_id): ds_api.lookup.assert_called_once_with(request=expected_request) +@pytest.mark.parametrize("database_id", [None, "somedb"]) +def test_client_get_multi_hit_w_transaction_begin_later(database_id): + """ + Transactions with begin_later set should begin on first read + """ + from google.cloud.datastore_v1.types import datastore as datastore_pb2 + from google.cloud.datastore.key import Key + + kind = "Kind" + id_ = 1234 + path = [{"kind": kind, "id": id_}] + expected_server_id = b"123" + + # Make a found entity pb to be returned from mock backend. + entity_pb = _make_entity_pb(PROJECT, kind, id_, "foo", "Foo", database=database_id) + + # Make a connection to return the entity pb. + creds = _make_credentials() + client = _make_client(credentials=creds, database=database_id) + lookup_response = _make_lookup_response(results=[entity_pb], transaction=expected_server_id) + ds_api = _make_datastore_api(lookup_response=lookup_response) + client._datastore_api_internal = ds_api + + key = Key(kind, id_, project=PROJECT, database=database_id) + txn = client.transaction(begin_later=True) + assert txn._id is None + assert txn._status == txn._INITIAL + client.get_multi([key], transaction=txn) + + # transaction should now be started + assert txn._id == expected_server_id + assert txn._id is not None + assert txn._status == txn._IN_PROGRESS + + # check rpc args + expected_read_options = datastore_pb2.ReadOptions(new_transaction=txn._options) + expected_request = { + "project_id": PROJECT, + "keys": [key.to_protobuf()], + "read_options": expected_read_options, + } + set_database_id_to_request(expected_request, database_id) + ds_api.lookup.assert_called_once_with(request=expected_request) + + @pytest.mark.parametrize("database_id", [None, "somedb"]) def test_client_get_multi_hit_w_read_time(database_id): from datetime import datetime From b952b8dfaab3424b0725fdae9b282c839296767f Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 14:47:46 -0800 Subject: [PATCH 08/39] handle context manager close --- google/cloud/datastore/transaction.py | 59 +++++++++++++++++---------- tests/unit/test_transaction.py | 25 ++++++++++++ 2 files changed, 63 insertions(+), 21 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index d9c39e02..b4115731 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -263,6 +263,15 @@ def _begin_with_id(self, transaction_id): self._id = transaction_id self._status = self._IN_PROGRESS + def _end_empty_transaction(self): + """ + End a transaction that was never used. + """ + if self._status != self._INITIAL: + raise ValueError("Transaction already begun.") + self._status = self._ABORTED + self._id = None + def rollback(self, retry=None, timeout=None): """Rolls back the current transaction. @@ -281,21 +290,25 @@ def rollback(self, retry=None, timeout=None): Note that if ``retry`` is specified, the timeout applies to each individual attempt. """ - kwargs = _make_retry_timeout_kwargs(retry, timeout) - - try: - # No need to use the response it contains nothing. - request = { - "project_id": self.project, - "transaction": self._id, - } - - set_database_id_to_request(request, self._client.database) - self._client._datastore_api.rollback(request=request, **kwargs) - finally: - super(Transaction, self).rollback() - # Clear our own ID in case this gets accidentally reused. - self._id = None + if self._status == self._INITIAL: + # If we haven't begun yet, set to closed state + self._end_empty_transaction() + else: + kwargs = _make_retry_timeout_kwargs(retry, timeout) + + try: + # No need to use the response it contains nothing. + request = { + "project_id": self.project, + "transaction": self._id, + } + + set_database_id_to_request(request, self._client.database) + self._client._datastore_api.rollback(request=request, **kwargs) + finally: + super(Transaction, self).rollback() + # Clear our own ID in case this gets accidentally reused. + self._id = None def commit(self, retry=None, timeout=None): """Commits the transaction. @@ -319,13 +332,17 @@ def commit(self, retry=None, timeout=None): Note that if ``retry`` is specified, the timeout applies to each individual attempt. """ - kwargs = _make_retry_timeout_kwargs(retry, timeout) + if self._status == self._INITIAL: + # If we haven't begun yet, set to closed state + self._end_empty_transaction() + else: + kwargs = _make_retry_timeout_kwargs(retry, timeout) - try: - super(Transaction, self).commit(**kwargs) - finally: - # Clear our own ID in case this gets accidentally reused. - self._id = None + try: + super(Transaction, self).commit(**kwargs) + finally: + # Clear our own ID in case this gets accidentally reused. + self._id = None def put(self, entity): """Adds an entity to be committed. diff --git a/tests/unit/test_transaction.py b/tests/unit/test_transaction.py index 796f6b5d..ee7ed900 100644 --- a/tests/unit/test_transaction.py +++ b/tests/unit/test_transaction.py @@ -375,6 +375,7 @@ def test_transaction_context_manager_no_raise(database_id): xact = _make_transaction(client) with xact: + assert xact._status == xact._IN_PROGRESS # only set between begin / commit assert xact.id == id_ @@ -427,6 +428,30 @@ class Foo(Exception): client._datastore_api.rollback.assert_called_once_with(request=expected_request) +@pytest.mark.parametrize("database_id", [None, "somedb"]) +def test_transaction_context_manager_w_begin_later(database_id): + """ + If begin_later is set, don't begin transaction when entering context manager + """ + from google.cloud.datastore_v1.types import datastore as datastore_pb2 + + project = "PROJECT" + id_ = 912830 + ds_api = _make_datastore_api(xact_id=id_) + client = _Client(project, datastore_api=ds_api, database=database_id) + xact = _make_transaction(client, begin_later=True) + + with xact: + assert xact._status == xact._INITIAL + assert xact.id is None + # should be finalized after context manager block + assert xact._status == xact._ABORTED + assert xact.id is None + # no need to call commit or rollback + assert ds_api.commit.call_count == 0 + assert ds_api.rollback.call_count == 0 + + @pytest.mark.parametrize("database_id", [None, "somedb"]) def test_transaction_put_read_only(database_id): project = "PROJECT" From 81108dfce164418904bff57ab512fe06f1294db6 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 14:49:47 -0800 Subject: [PATCH 09/39] removed extra indents --- google/cloud/datastore/transaction.py | 48 ++++++++++++++------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index b4115731..44f96dc7 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -293,22 +293,23 @@ def rollback(self, retry=None, timeout=None): if self._status == self._INITIAL: # If we haven't begun yet, set to closed state self._end_empty_transaction() - else: - kwargs = _make_retry_timeout_kwargs(retry, timeout) - - try: - # No need to use the response it contains nothing. - request = { - "project_id": self.project, - "transaction": self._id, - } - - set_database_id_to_request(request, self._client.database) - self._client._datastore_api.rollback(request=request, **kwargs) - finally: - super(Transaction, self).rollback() - # Clear our own ID in case this gets accidentally reused. - self._id = None + return + + kwargs = _make_retry_timeout_kwargs(retry, timeout) + + try: + # No need to use the response it contains nothing. + request = { + "project_id": self.project, + "transaction": self._id, + } + + set_database_id_to_request(request, self._client.database) + self._client._datastore_api.rollback(request=request, **kwargs) + finally: + super(Transaction, self).rollback() + # Clear our own ID in case this gets accidentally reused. + self._id = None def commit(self, retry=None, timeout=None): """Commits the transaction. @@ -335,14 +336,15 @@ def commit(self, retry=None, timeout=None): if self._status == self._INITIAL: # If we haven't begun yet, set to closed state self._end_empty_transaction() - else: - kwargs = _make_retry_timeout_kwargs(retry, timeout) + return + + kwargs = _make_retry_timeout_kwargs(retry, timeout) - try: - super(Transaction, self).commit(**kwargs) - finally: - # Clear our own ID in case this gets accidentally reused. - self._id = None + try: + super(Transaction, self).commit(**kwargs) + finally: + # Clear our own ID in case this gets accidentally reused. + self._id = None def put(self, entity): """Adds an entity to be committed. From defd08024d09060ac410aff2ed42a8473098706e Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 15:14:03 -0800 Subject: [PATCH 10/39] added transaction tests --- tests/unit/test_transaction.py | 81 +++++++++++++++++++++++++++++++--- 1 file changed, 75 insertions(+), 6 deletions(-) diff --git a/tests/unit/test_transaction.py b/tests/unit/test_transaction.py index ee7ed900..e6d0f559 100644 --- a/tests/unit/test_transaction.py +++ b/tests/unit/test_transaction.py @@ -428,22 +428,26 @@ class Foo(Exception): client._datastore_api.rollback.assert_called_once_with(request=expected_request) +@pytest.mark.parametrize("with_exception", [False, True]) @pytest.mark.parametrize("database_id", [None, "somedb"]) -def test_transaction_context_manager_w_begin_later(database_id): +def test_transaction_context_manager_w_begin_later(database_id, with_exception): """ If begin_later is set, don't begin transaction when entering context manager """ - from google.cloud.datastore_v1.types import datastore as datastore_pb2 - project = "PROJECT" id_ = 912830 ds_api = _make_datastore_api(xact_id=id_) client = _Client(project, datastore_api=ds_api, database=database_id) xact = _make_transaction(client, begin_later=True) - with xact: - assert xact._status == xact._INITIAL - assert xact.id is None + try: + with xact: + assert xact._status == xact._INITIAL + assert xact.id is None + if with_exception: + raise RuntimeError("expected") + except RuntimeError: + pass # should be finalized after context manager block assert xact._status == xact._ABORTED assert xact.id is None @@ -466,6 +470,71 @@ def test_transaction_put_read_only(database_id): xact.put(entity) + +@pytest.mark.parametrize("database_id", [None, "somedb"]) +def test_transaction_begin_later_with_delete(database_id): + """ + begin_later transactions should begin on delete + """ + project = "PROJECT" + id_ = 912830 + ds_api = _make_datastore_api(xact_id=id_) + client = _Client(project, datastore_api=ds_api, database=database_id) + xact = _make_transaction(client, begin_later=True) + + fake_key = mock.Mock() + + with mock.patch("google.cloud.datastore.batch.Batch.delete") as delete: + with xact: + assert xact._status == xact._INITIAL + assert xact.id is None + xact.delete(fake_key) + # call should have started transaction + assert xact._status == xact._IN_PROGRESS + assert xact.id == id_ + # super class delete should have been called + assert delete.call_count == 1 + assert delete.call_args == mock.call(fake_key) + # should be finalized after context manager block + assert xact._status == xact._FINISHED + assert xact.id is None + # should have committed + assert ds_api.commit.call_count == 1 + assert ds_api.rollback.call_count == 0 + + +@pytest.mark.parametrize("database_id", [None, "somedb"]) +def test_transaction_begin_later_with_put(database_id): + """ + begin_later transactions should begin on put + """ + project = "PROJECT" + id_ = 912830 + ds_api = _make_datastore_api(xact_id=id_) + client = _Client(project, datastore_api=ds_api, database=database_id) + xact = _make_transaction(client, begin_later=True) + + fake_entity = mock.Mock() + + with mock.patch("google.cloud.datastore.batch.Batch.put") as put: + with xact: + assert xact._status == xact._INITIAL + assert xact.id is None + xact.put(fake_entity) + # call should have started transaction + assert xact._status == xact._IN_PROGRESS + assert xact.id == id_ + # super class put should have been called + assert put.call_count == 1 + assert put.call_args == mock.call(fake_entity) + # should be finalized after context manager block + assert xact._status == xact._FINISHED + assert xact.id is None + # should have committed + assert ds_api.commit.call_count == 1 + assert ds_api.rollback.call_count == 0 + + def _make_key(kind, id_, project, database=None): from google.cloud.datastore_v1.types import entity as entity_pb2 From 2063f71797ede3f3c7d3808f0a090b008736bbe9 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 15:33:19 -0800 Subject: [PATCH 11/39] added system tests --- tests/system/test_transaction.py | 46 ++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/tests/system/test_transaction.py b/tests/system/test_transaction.py index a93538fb..d06f6d01 100644 --- a/tests/system/test_transaction.py +++ b/tests/system/test_transaction.py @@ -40,6 +40,52 @@ def test_transaction_via_with_statement( entities_to_delete.append(retrieved_entity) assert retrieved_entity == entity +@pytest.mark.parametrize("database_id", [None, _helpers.TEST_DATABASE], indirect=True) +@pytest.mark.parametrize("first_call", ["get", "put", "delete"]) +def test_transaction_begin_later(datastore_client, entities_to_delete, database_id, first_call): + """ + transactions with begin_later should call begin on first rpc + """ + key = datastore_client.key("Company", "Google") + entity = datastore.Entity(key=key) + entity["url"] = "www.google.com" + + datastore_client.put(entity) + result_entity = datastore_client.get(key) + + with datastore_client.transaction(begin_later=True) as xact: + assert xact._id is None + assert xact._status == xact._INITIAL + if first_call == "get": + datastore_client.get(entity.key) + elif first_call == "put": + xact.put(entity) + elif first_call == "delete": + xact.delete(result_entity.key) + assert xact._id is not None + assert xact._status == xact._IN_PROGRESS + assert xact._status == xact._FINISHED + + entities_to_delete.append(result_entity) + + +@pytest.mark.parametrize("database_id", [None, _helpers.TEST_DATABASE], indirect=True) +@pytest.mark.parametrize("raise_exception", [True, False]) +def test_transaction_begin_later_noop(datastore_client, database_id, raise_exception): + """ + empty begin later transactions should terminate quietly + """ + try: + with datastore_client.transaction(begin_later=True) as xact: + assert xact._id is None + assert xact._status == xact._INITIAL + if raise_exception: + raise RuntimeError("test") + except RuntimeError: + pass + assert xact._status == xact._ABORTED + assert xact._id is None + @pytest.mark.parametrize("database_id", [None, _helpers.TEST_DATABASE], indirect=True) def test_transaction_via_explicit_begin_get_commit( From 648204d31fd1b113e9afcde3700184401d575a33 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 15:43:19 -0800 Subject: [PATCH 12/39] fixed lint --- google/cloud/datastore/client.py | 10 ++++++++-- google/cloud/datastore/helpers.py | 17 +++++++++++++---- tests/system/test_transaction.py | 5 ++++- tests/unit/test_client.py | 5 +++-- tests/unit/test_helpers.py | 18 +++++++++++++----- tests/unit/test_transaction.py | 1 - 6 files changed, 41 insertions(+), 15 deletions(-) diff --git a/google/cloud/datastore/client.py b/google/cloud/datastore/client.py index aaac9b07..4e6df298 100644 --- a/google/cloud/datastore/client.py +++ b/google/cloud/datastore/client.py @@ -203,12 +203,18 @@ def _extended_lookup( new_transaction_options = None if transaction is not None: transaction_id = transaction.id - if transaction_id is None and transaction._begin_later and transaction._status == transaction._INITIAL: + if ( + transaction_id is None + and transaction._begin_later + and transaction._status == transaction._INITIAL + ): # if transaction hasn't been initialized, initialize it as part of this request new_transaction_options = transaction._options loop_num = 0 - read_options = helpers.get_read_options(eventual, transaction_id, read_time, new_transaction_options) + read_options = helpers.get_read_options( + eventual, transaction_id, read_time, new_transaction_options + ) while loop_num < _MAX_LOOPS: # loop against possible deferred. loop_num += 1 request = { diff --git a/google/cloud/datastore/helpers.py b/google/cloud/datastore/helpers.py index 4b87c5ba..647bf89a 100644 --- a/google/cloud/datastore/helpers.py +++ b/google/cloud/datastore/helpers.py @@ -230,7 +230,9 @@ def entity_to_protobuf(entity): return entity_pb -def get_read_options(eventual, transaction_id, read_time=None, new_transaction_options=None): +def get_read_options( + eventual, transaction_id, read_time=None, new_transaction_options=None +): """Validate rules for read options, and assign to the request. Helper method for ``lookup()`` and ``run_query``. @@ -253,8 +255,13 @@ def get_read_options(eventual, transaction_id, read_time=None, new_transaction_o :raises: :class:`ValueError` if more than one of ``eventual==True``, ``transaction_id``, ``read_time``, and ``new_transaction_options`` is specified. """ - if sum(bool(x) for x in (eventual, transaction_id, read_time, new_transaction_options)) > 1: - raise ValueError("At most one of eventual, transaction, or read_time is allowed.") + is_set = [ + bool(x) for x in (eventual, transaction_id, read_time, new_transaction_options) + ] + if sum(is_set) > 1: + raise ValueError( + "At most one of eventual, transaction, or read_time is allowed." + ) new_options = datastore_pb2.ReadOptions() if transaction_id is not None: new_options.transaction = transaction_id @@ -265,7 +272,9 @@ def get_read_options(eventual, transaction_id, read_time=None, new_transaction_o if new_transaction_options is not None: new_options.new_transaction = new_transaction_options if eventual: - new_options.read_consistency = datastore_pb2.ReadOptions.ReadConsistency.EVENTUAL + new_options.read_consistency = ( + datastore_pb2.ReadOptions.ReadConsistency.EVENTUAL + ) return new_options diff --git a/tests/system/test_transaction.py b/tests/system/test_transaction.py index d06f6d01..755d957f 100644 --- a/tests/system/test_transaction.py +++ b/tests/system/test_transaction.py @@ -40,9 +40,12 @@ def test_transaction_via_with_statement( entities_to_delete.append(retrieved_entity) assert retrieved_entity == entity + @pytest.mark.parametrize("database_id", [None, _helpers.TEST_DATABASE], indirect=True) @pytest.mark.parametrize("first_call", ["get", "put", "delete"]) -def test_transaction_begin_later(datastore_client, entities_to_delete, database_id, first_call): +def test_transaction_begin_later( + datastore_client, entities_to_delete, database_id, first_call +): """ transactions with begin_later should call begin on first rpc """ diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 1bf55866..2b5c01f4 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -715,7 +715,6 @@ def test_client_get_multi_hit_w_transaction_begin_later(database_id): kind = "Kind" id_ = 1234 - path = [{"kind": kind, "id": id_}] expected_server_id = b"123" # Make a found entity pb to be returned from mock backend. @@ -724,7 +723,9 @@ def test_client_get_multi_hit_w_transaction_begin_later(database_id): # Make a connection to return the entity pb. creds = _make_credentials() client = _make_client(credentials=creds, database=database_id) - lookup_response = _make_lookup_response(results=[entity_pb], transaction=expected_server_id) + lookup_response = _make_lookup_response( + results=[entity_pb], transaction=expected_server_id + ) ds_api = _make_datastore_api(lookup_response=lookup_response) client._datastore_api_internal = ds_api diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index 13c54b76..c3255e4f 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -589,17 +589,25 @@ def test__get_read_options_w_default_wo_txn_w_read_time(): def test__get_read_options_w_new_transaction(): from google.cloud.datastore.helpers import get_read_options from google.cloud.datastore_v1.types import datastore as datastore_pb2 + input_options = datastore_pb2.TransactionOptions() read_options = get_read_options(False, None, new_transaction_options=input_options) expected = datastore_pb2.ReadOptions(new_transaction=input_options) assert read_options == expected -@pytest.mark.parametrize("args", [ - (True, "id"), (True, "id", None), (True, None, "read_time"), (True, None, None, "new"), - (False, "id", "read_time"), (False, "id", None, "new"), - (False, None, "read_time", "new"), -]) +@pytest.mark.parametrize( + "args", + [ + (True, "id"), + (True, "id", None), + (True, None, "read_time"), + (True, None, None, "new"), + (False, "id", "read_time"), + (False, "id", None, "new"), + (False, None, "read_time", "new"), + ], +) def test__get_read_options_w_multiple_args(args): """ arguments are mutually exclusive. diff --git a/tests/unit/test_transaction.py b/tests/unit/test_transaction.py index e6d0f559..ca1b97f7 100644 --- a/tests/unit/test_transaction.py +++ b/tests/unit/test_transaction.py @@ -470,7 +470,6 @@ def test_transaction_put_read_only(database_id): xact.put(entity) - @pytest.mark.parametrize("database_id", [None, "somedb"]) def test_transaction_begin_later_with_delete(database_id): """ From 087eaafdc844199e1c69f3395c8442793b1ab5e3 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 15:57:55 -0800 Subject: [PATCH 13/39] refactor using wrappers --- google/cloud/datastore/transaction.py | 52 +++++++++++++++------------ 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 44f96dc7..bfa3d236 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -263,15 +263,36 @@ def _begin_with_id(self, transaction_id): self._id = transaction_id self._status = self._IN_PROGRESS - def _end_empty_transaction(self): + def _abort_if_not_began(fn): """ - End a transaction that was never used. + Function wrapper to abort transaction if it hasn't started when + the wrapped function is called. + + Used by commit and rollback. """ - if self._status != self._INITIAL: - raise ValueError("Transaction already begun.") - self._status = self._ABORTED - self._id = None + def wrapped(self, *args, **kwargs): + if self._status == self._INITIAL: + self._status = self._ABORTED + self._id = None + return None + else: + return fn(self, *args, **kwargs) + return wrapped + def _begin_if_not_began(fn): + """ + Function wrapper to begin transaction if it hasn't started when + the wrapped function is called. + + Used by put and delete. + """ + def wrapped(self, *args, **kwargs): + if self._begin_later and self._status == self._INITIAL: + self.begin() + return fn(self, *args, **kwargs) + return wrapped + + @_abort_if_not_began def rollback(self, retry=None, timeout=None): """Rolls back the current transaction. @@ -290,11 +311,6 @@ def rollback(self, retry=None, timeout=None): Note that if ``retry`` is specified, the timeout applies to each individual attempt. """ - if self._status == self._INITIAL: - # If we haven't begun yet, set to closed state - self._end_empty_transaction() - return - kwargs = _make_retry_timeout_kwargs(retry, timeout) try: @@ -311,6 +327,7 @@ def rollback(self, retry=None, timeout=None): # Clear our own ID in case this gets accidentally reused. self._id = None + @_abort_if_not_began def commit(self, retry=None, timeout=None): """Commits the transaction. @@ -333,11 +350,6 @@ def commit(self, retry=None, timeout=None): Note that if ``retry`` is specified, the timeout applies to each individual attempt. """ - if self._status == self._INITIAL: - # If we haven't begun yet, set to closed state - self._end_empty_transaction() - return - kwargs = _make_retry_timeout_kwargs(retry, timeout) try: @@ -346,6 +358,7 @@ def commit(self, retry=None, timeout=None): # Clear our own ID in case this gets accidentally reused. self._id = None + @_begin_if_not_began def put(self, entity): """Adds an entity to be committed. @@ -362,15 +375,10 @@ def put(self, entity): if "read_only" in self._options: raise RuntimeError("Transaction is read only") else: - if self._begin_later and self._status == self._INITIAL: - # If we haven't begun yet, we need to do so now. - self.begin() super(Transaction, self).put(entity) + @_begin_if_not_began def delete(self, key): - if self._begin_later and self._status == self._INITIAL: - # If we haven't begun yet, we need to do so now. - self.begin() super(Transaction, self).delete(key) def __enter__(self): From cbb2c0a4e62c87ef264bfb5e8a2db2fb8e404e55 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 16:04:05 -0800 Subject: [PATCH 14/39] added functools.wraps --- google/cloud/datastore/transaction.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index bfa3d236..21ee411d 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -13,6 +13,7 @@ # limitations under the License. """Create / interact with Google Cloud Datastore transactions.""" +import functools from google.cloud.datastore.batch import Batch from google.cloud.datastore_v1.types import TransactionOptions @@ -270,6 +271,8 @@ def _abort_if_not_began(fn): Used by commit and rollback. """ + + @functools.wraps(fn) def wrapped(self, *args, **kwargs): if self._status == self._INITIAL: self._status = self._ABORTED @@ -277,6 +280,7 @@ def wrapped(self, *args, **kwargs): return None else: return fn(self, *args, **kwargs) + return wrapped def _begin_if_not_began(fn): @@ -286,10 +290,13 @@ def _begin_if_not_began(fn): Used by put and delete. """ + + @functools.wraps(fn) def wrapped(self, *args, **kwargs): if self._begin_later and self._status == self._INITIAL: self.begin() return fn(self, *args, **kwargs) + return wrapped @_abort_if_not_began From 471ead2f889a8fa4dc89b658217f6a302825562b Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 16:06:00 -0800 Subject: [PATCH 15/39] fixed mypy --- google/cloud/datastore/transaction.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 21ee411d..5f7e5320 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -264,6 +264,7 @@ def _begin_with_id(self, transaction_id): self._id = transaction_id self._status = self._IN_PROGRESS + @staticmethod def _abort_if_not_began(fn): """ Function wrapper to abort transaction if it hasn't started when @@ -283,6 +284,7 @@ def wrapped(self, *args, **kwargs): return wrapped + @staticmethod def _begin_if_not_began(fn): """ Function wrapper to begin transaction if it hasn't started when From 8da9e00057864c3b89c7feaf84780bdcc5b4e04e Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 16:41:48 -0800 Subject: [PATCH 16/39] fixed mypy --- google/cloud/datastore/transaction.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 5f7e5320..61421708 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -14,6 +14,7 @@ """Create / interact with Google Cloud Datastore transactions.""" import functools +from typing import Callable from google.cloud.datastore.batch import Batch from google.cloud.datastore_v1.types import TransactionOptions @@ -264,8 +265,7 @@ def _begin_with_id(self, transaction_id): self._id = transaction_id self._status = self._IN_PROGRESS - @staticmethod - def _abort_if_not_began(fn): + def _abort_if_not_began(fn: Callable) -> Callable: # type: ignore """ Function wrapper to abort transaction if it hasn't started when the wrapped function is called. @@ -284,8 +284,7 @@ def wrapped(self, *args, **kwargs): return wrapped - @staticmethod - def _begin_if_not_began(fn): + def _begin_if_not_began(fn: Callable) -> Callable: # type: ignore """ Function wrapper to begin transaction if it hasn't started when the wrapped function is called. From 95797e928a34ec2da353d386da1af93864c005f0 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Tue, 28 Nov 2023 16:59:59 -0800 Subject: [PATCH 17/39] added test --- tests/unit/test_transaction.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/unit/test_transaction.py b/tests/unit/test_transaction.py index ca1b97f7..ff9aa213 100644 --- a/tests/unit/test_transaction.py +++ b/tests/unit/test_transaction.py @@ -81,6 +81,27 @@ def test_transaction_constructor_read_write_w_read_time(database_id): _make_transaction(client, read_only=False, read_time=read_time) +@pytest.mark.parametrize("database_id", [None, "somedb"]) +def test_transaction_constructor_begin_later(database_id): + from google.cloud.datastore.transaction import Transaction + + project = "PROJECT" + client = _Client(project, database=database_id) + expected_id = b"1234" + + xact = _make_transaction(client, begin_later=True) + assert xact._status == Transaction._INITIAL + assert xact.id is None + + xact._begin_with_id(expected_id) + assert xact._status == Transaction._IN_PROGRESS + assert xact.id == expected_id + + # calling a second time should raise exeception + with pytest.raises(ValueError): + xact._begin_with_id(expected_id) + + @pytest.mark.parametrize("database_id", [None, "somedb"]) def test_transaction_current(database_id): from google.cloud.datastore_v1.types import datastore as datastore_pb2 From 09d945f0add2a0e6c0de843655da676562ebe50b Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 30 Nov 2023 10:49:46 -0800 Subject: [PATCH 18/39] default begin_later to False --- google/cloud/datastore/transaction.py | 4 ++-- tests/unit/test_transaction.py | 3 --- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 61421708..043356cd 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -156,7 +156,7 @@ class Transaction(Batch): lazily (i.e. when the first RPC is made). If False, the transaction will be started as soon as the context manager is entered. `self.begin()` can also be called manually to begin - the transaction at any time. Default is True. + the transaction at any time. Default is False. :raises: :class:`ValueError` if read_time is specified when ``read_only=False``. @@ -164,7 +164,7 @@ class Transaction(Batch): _status = None - def __init__(self, client, read_only=False, read_time=None, begin_later=True): + def __init__(self, client, read_only=False, read_time=None, begin_later=False): super(Transaction, self).__init__(client) self._id = None self._begin_later = begin_later diff --git a/tests/unit/test_transaction.py b/tests/unit/test_transaction.py index ff9aa213..6ba13223 100644 --- a/tests/unit/test_transaction.py +++ b/tests/unit/test_transaction.py @@ -632,9 +632,6 @@ def _make_options(read_only=False, previous_transaction=None, read_time=None): def _make_transaction(client, **kw): from google.cloud.datastore.transaction import Transaction - # default to begin_later=False - kw.setdefault("begin_later", False) - return Transaction(client, **kw) From 982b998bd9287fda85936cc3c04c084a2d6cdd80 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 15 Dec 2023 15:18:54 -0800 Subject: [PATCH 19/39] added new_transaction to query and aggregation classes --- google/cloud/datastore/aggregation.py | 6 +++- google/cloud/datastore/client.py | 6 +--- google/cloud/datastore/query.py | 5 ++- tests/unit/test_aggregation.py | 48 +++++++++++++++++++++++++++ tests/unit/test_query.py | 42 +++++++++++++++++++++++ 5 files changed, 100 insertions(+), 7 deletions(-) diff --git a/google/cloud/datastore/aggregation.py b/google/cloud/datastore/aggregation.py index 47ebfebd..bff9861e 100644 --- a/google/cloud/datastore/aggregation.py +++ b/google/cloud/datastore/aggregation.py @@ -442,13 +442,17 @@ def _next_page(self): return None query_pb = self._build_protobuf() + new_transaction_options = None transaction = self.client.current_transaction if transaction is None: transaction_id = None else: transaction_id = transaction.id + if transation._begin_later and transaction._status == transaction._INITIAL: + # if transaction hasn't been initialized, initialize it as part of this request + new_transaction_options = transaction._options read_options = helpers.get_read_options( - self._eventual, transaction_id, self._read_time + self._eventual, transaction_id, self._read_time, new_transaction_options ) partition_id = entity_pb2.PartitionId( diff --git a/google/cloud/datastore/client.py b/google/cloud/datastore/client.py index 4e6df298..008ceee7 100644 --- a/google/cloud/datastore/client.py +++ b/google/cloud/datastore/client.py @@ -203,11 +203,7 @@ def _extended_lookup( new_transaction_options = None if transaction is not None: transaction_id = transaction.id - if ( - transaction_id is None - and transaction._begin_later - and transaction._status == transaction._INITIAL - ): + if and transaction._begin_later and transaction._status == transaction._INITIAL: # if transaction hasn't been initialized, initialize it as part of this request new_transaction_options = transaction._options diff --git a/google/cloud/datastore/query.py b/google/cloud/datastore/query.py index 57c0702c..85cc97a9 100644 --- a/google/cloud/datastore/query.py +++ b/google/cloud/datastore/query.py @@ -778,13 +778,16 @@ def _next_page(self): return None query_pb = self._build_protobuf() + new_transaction_options = None transaction = self.client.current_transaction if transaction is None: transaction_id = None else: transaction_id = transaction.id + # if transaction hasn't been initialized, initialize it as part of this request + new_transaction_options = transaction._options read_options = helpers.get_read_options( - self._eventual, transaction_id, self._read_time + self._eventual, transaction_id, self._read_time, new_transaction_options ) partition_id = entity_pb2.PartitionId( diff --git a/tests/unit/test_aggregation.py b/tests/unit/test_aggregation.py index 15d11aca..a493c779 100644 --- a/tests/unit/test_aggregation.py +++ b/tests/unit/test_aggregation.py @@ -612,6 +612,54 @@ def test_transaction_id_populated(database_id, aggregation_type, aggregation_arg assert read_options.transaction == client.current_transaction.id +@pytest.mark.parametrize("database_id", [None, "somedb"], indirect=True) +@pytest.mark.parametrize( + "aggregation_type,aggregation_args", + [ + ("count", ()), + ( + "sum", + ("appearances",), + ), + ("avg", ("appearances",)), + ], +) +def test_transaction_begin_later(database_id, aggregation_type, aggregation_args): + """ + When an aggregation is run in the context of a transaction with begin_later=True, + the new_transaction field should be populated in the request read_options. + """ + import mock + + # make a fake begin_later transaction + transaction = mock.Mock() + transaction.id = None + transaction._begin_later = True + transaction._state = transaction._INITIAL + mock_datastore_api = mock.Mock() + mock_gapic = mock_datastore_api.run_aggregation_query + mock_gapic.return_value = _make_aggregation_query_response([]) + client = _Client( + None, + datastore_api=mock_datastore_api, + database=database_id, + transaction=transaction, + ) + + query = _make_query(client) + aggregation_query = _make_aggregation_query(client=client, query=query) + + # initiate requested aggregation (ex count, sum, avg) + getattr(aggregation_query, aggregation_type)(*aggregation_args) + # run mock query + list(aggregation_query.fetch()) + assert mock_gapic.call_count == 1 + request = mock_gapic.call_args[1]["request"] + read_options = request["read_options"] + # ensure new_transaction is populated + assert read_options.transaction is None + assert read_options.new_transaction == transaction._options + class _Client(object): def __init__( self, diff --git a/tests/unit/test_query.py b/tests/unit/test_query.py index 84c0bedf..1f9648c9 100644 --- a/tests/unit/test_query.py +++ b/tests/unit/test_query.py @@ -698,6 +698,48 @@ def test_transaction_id_populated(database_id): assert read_options.transaction == client.current_transaction.id +@pytest.mark.parametrize("database_id", [None, "somedb"]) +def test_transaction_begin_later(database_id): + """ + When an aggregation is run in the context of a transaction with begin_later=True, + the new_transaction field should be populated in the request read_options. + """ + import mock + + + # make a fake begin_later transaction + transaction = mock.Mock() + transaction.id = None + transaction._begin_later = True + transaction._state = transaction._INITIAL + + mock_datastore_api = mock.Mock() + mock_gapic = mock_datastore_api.run_query + + more_results_enum = 3 # NO_MORE_RESULTS + response_pb = _make_query_response([], b"", more_results_enum, 0) + mock_gapic.return_value = response_pb + + client = _Client( + None, + datastore_api=mock_datastore_api, + database=database_id, + transaction=transaction, + ) + + query = _make_query(client) + # run mock query + list(query.fetch()) + assert mock_gapic.call_count == 1 + request = mock_gapic.call_args[1]["request"] + read_options = request["read_options"] + # ensure new_transaction is populated + assert read_options.transaction is None + assert read_options.new_transaction == transaction._options + + + + def test_iterator_constructor_defaults(): query = object() client = object() From e7b8b9a23e6265ed25114651da32f938819cf7ed Mon Sep 17 00:00:00 2001 From: Mend Renovate Date: Thu, 7 Dec 2023 20:33:05 +0100 Subject: [PATCH 20/39] chore(deps): update all dependencies (#505) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(deps): update all dependencies * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --------- Co-authored-by: Owl Bot --- .github/workflows/mypy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/mypy.yml b/.github/workflows/mypy.yml index 14459186..3915cddd 100644 --- a/.github/workflows/mypy.yml +++ b/.github/workflows/mypy.yml @@ -10,7 +10,7 @@ jobs: - name: Checkout uses: actions/checkout@v4 - name: Setup Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: "3.8" - name: Install nox From 026c9342c8402b39687be6b824033683d2cd958b Mon Sep 17 00:00:00 2001 From: "gcf-owl-bot[bot]" <78513119+gcf-owl-bot[bot]@users.noreply.github.com> Date: Sun, 10 Dec 2023 09:01:28 -0500 Subject: [PATCH 21/39] build: update actions/checkout and actions/setup-python (#507) Source-Link: https://github.com/googleapis/synthtool/commit/3551acd1261fd8f616cbfd054cda9bd6d6ac75f4 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:230f7fe8a0d2ed81a519cfc15c6bb11c5b46b9fb449b8b1219b3771bcb520ad2 Co-authored-by: Owl Bot --- .github/.OwlBot.lock.yaml | 4 ++-- .github/workflows/docs.yml | 8 ++++---- .github/workflows/lint.yml | 4 ++-- .github/workflows/unittest.yml | 8 ++++---- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/.github/.OwlBot.lock.yaml b/.github/.OwlBot.lock.yaml index 773c1dfd..40bf9973 100644 --- a/.github/.OwlBot.lock.yaml +++ b/.github/.OwlBot.lock.yaml @@ -13,5 +13,5 @@ # limitations under the License. docker: image: gcr.io/cloud-devrel-public-resources/owlbot-python:latest - digest: sha256:2f155882785883336b4468d5218db737bb1d10c9cea7cb62219ad16fe248c03c -# created: 2023-11-29T14:54:29.548172703Z + digest: sha256:230f7fe8a0d2ed81a519cfc15c6bb11c5b46b9fb449b8b1219b3771bcb520ad2 +# created: 2023-12-09T15:16:25.430769578Z diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index 221806ce..698fbc5c 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -8,9 +8,9 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Setup Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: "3.9" - name: Install nox @@ -24,9 +24,9 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Setup Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: "3.10" - name: Install nox diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 16d5a9e9..4866193a 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -8,9 +8,9 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Setup Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: "3.8" - name: Install nox diff --git a/.github/workflows/unittest.yml b/.github/workflows/unittest.yml index a32027b4..d6ca6562 100644 --- a/.github/workflows/unittest.yml +++ b/.github/workflows/unittest.yml @@ -11,9 +11,9 @@ jobs: python: ['3.7', '3.8', '3.9', '3.10', '3.11', '3.12'] steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Setup Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: ${{ matrix.python }} - name: Install nox @@ -37,9 +37,9 @@ jobs: - unit steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Setup Python - uses: actions/setup-python@v4 + uses: actions/setup-python@v5 with: python-version: "3.8" - name: Install coverage From a001d5ff4a9593898ce3bb343cdcb5c50dd64444 Mon Sep 17 00:00:00 2001 From: "release-please[bot]" <55107282+release-please[bot]@users.noreply.github.com> Date: Tue, 12 Dec 2023 12:34:21 -0800 Subject: [PATCH 22/39] chore(main): release 2.19.0 (#481) Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> --- .release-please-manifest.json | 2 +- CHANGELOG.md | 18 ++++++++++++++++++ google/cloud/datastore/gapic_version.py | 2 +- google/cloud/datastore/version.py | 2 +- google/cloud/datastore_admin/gapic_version.py | 2 +- .../cloud/datastore_admin_v1/gapic_version.py | 2 +- google/cloud/datastore_v1/gapic_version.py | 2 +- 7 files changed, 24 insertions(+), 6 deletions(-) diff --git a/.release-please-manifest.json b/.release-please-manifest.json index a627e662..b7f666a6 100644 --- a/.release-please-manifest.json +++ b/.release-please-manifest.json @@ -1,3 +1,3 @@ { - ".": "2.18.0" + ".": "2.19.0" } \ No newline at end of file diff --git a/CHANGELOG.md b/CHANGELOG.md index 499af3b6..52d6dfc7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,24 @@ [1]: https://pypi.org/project/google-cloud-datastore/#history +## [2.19.0](https://github.com/googleapis/python-datastore/compare/v2.18.0...v2.19.0) (2023-12-10) + + +### Features + +* Add support for Python 3.12 ([#498](https://github.com/googleapis/python-datastore/issues/498)) ([d1d60fa](https://github.com/googleapis/python-datastore/commit/d1d60fa602eca2062a505a0750b0ce6dccc771cd)) +* Introduce compatibility with native namespace packages ([#497](https://github.com/googleapis/python-datastore/issues/497)) ([87b3392](https://github.com/googleapis/python-datastore/commit/87b339228896da197b0ee77e2b00994431ae8d2e)) + + +### Bug Fixes + +* Use `retry_async` instead of `retry` in async client ([4e15ce6](https://github.com/googleapis/python-datastore/commit/4e15ce640580f14fb1ee5d8ad49ea48e860ff1da)) + + +### Documentation + +* Minor formatting ([#476](https://github.com/googleapis/python-datastore/issues/476)) ([b13b15c](https://github.com/googleapis/python-datastore/commit/b13b15cd95c02c923f9991b088bb71eda777cf46)) + ## [2.18.0](https://github.com/googleapis/python-datastore/compare/v2.17.0...v2.18.0) (2023-09-05) diff --git a/google/cloud/datastore/gapic_version.py b/google/cloud/datastore/gapic_version.py index 38172a8e..28762874 100644 --- a/google/cloud/datastore/gapic_version.py +++ b/google/cloud/datastore/gapic_version.py @@ -12,4 +12,4 @@ # See the License for the specific language governing permissions and # limitations under the License. -__version__ = "2.18.0" # {x-release-please-version} +__version__ = "2.19.0" # {x-release-please-version} diff --git a/google/cloud/datastore/version.py b/google/cloud/datastore/version.py index a613e5ea..2605c08a 100644 --- a/google/cloud/datastore/version.py +++ b/google/cloud/datastore/version.py @@ -12,4 +12,4 @@ # See the License for the specific language governing permissions and # limitations under the License. -__version__ = "2.18.0" +__version__ = "2.19.0" diff --git a/google/cloud/datastore_admin/gapic_version.py b/google/cloud/datastore_admin/gapic_version.py index f09943f6..0f1a446f 100644 --- a/google/cloud/datastore_admin/gapic_version.py +++ b/google/cloud/datastore_admin/gapic_version.py @@ -13,4 +13,4 @@ # See the License for the specific language governing permissions and # limitations under the License. # -__version__ = "2.18.0" # {x-release-please-version} +__version__ = "2.19.0" # {x-release-please-version} diff --git a/google/cloud/datastore_admin_v1/gapic_version.py b/google/cloud/datastore_admin_v1/gapic_version.py index 552e8442..8dc121fd 100644 --- a/google/cloud/datastore_admin_v1/gapic_version.py +++ b/google/cloud/datastore_admin_v1/gapic_version.py @@ -12,4 +12,4 @@ # See the License for the specific language governing permissions and # limitations under the License. -__version__ = "2.18.0" # {x-release-please-version} +__version__ = "2.19.0" # {x-release-please-version} diff --git a/google/cloud/datastore_v1/gapic_version.py b/google/cloud/datastore_v1/gapic_version.py index 552e8442..8dc121fd 100644 --- a/google/cloud/datastore_v1/gapic_version.py +++ b/google/cloud/datastore_v1/gapic_version.py @@ -12,4 +12,4 @@ # See the License for the specific language governing permissions and # limitations under the License. -__version__ = "2.18.0" # {x-release-please-version} +__version__ = "2.19.0" # {x-release-please-version} From aef3befcdb25ef3ee41f5d656945ee4bfa23cdaf Mon Sep 17 00:00:00 2001 From: Mend Renovate Date: Tue, 12 Dec 2023 23:18:29 +0100 Subject: [PATCH 23/39] chore(deps): update dependency google-cloud-datastore to v2.19.0 (#508) --- samples/snippets/requirements.txt | 2 +- samples/snippets/schedule-export/requirements.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/samples/snippets/requirements.txt b/samples/snippets/requirements.txt index d4e90e37..5bccacc5 100644 --- a/samples/snippets/requirements.txt +++ b/samples/snippets/requirements.txt @@ -1 +1 @@ -google-cloud-datastore==2.18.0 \ No newline at end of file +google-cloud-datastore==2.19.0 \ No newline at end of file diff --git a/samples/snippets/schedule-export/requirements.txt b/samples/snippets/schedule-export/requirements.txt index a84b83a1..b748abdc 100644 --- a/samples/snippets/schedule-export/requirements.txt +++ b/samples/snippets/schedule-export/requirements.txt @@ -1 +1 @@ -google-cloud-datastore==2.18.0 +google-cloud-datastore==2.19.0 From 3c2e4a74f154535b8dc373b370fc872dfe732de7 Mon Sep 17 00:00:00 2001 From: "gcf-owl-bot[bot]" <78513119+gcf-owl-bot[bot]@users.noreply.github.com> Date: Thu, 14 Dec 2023 19:27:11 -0500 Subject: [PATCH 24/39] build: update actions/upload-artifact and actions/download-artifact (#510) Source-Link: https://github.com/googleapis/synthtool/commit/280ddaed417057dfe5b1395731de07b7d09f5058 Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:346ab2efb51649c5dde7756cbbdc60dd394852ba83b9bbffc292a63549f33c17 Co-authored-by: Owl Bot --- .github/.OwlBot.lock.yaml | 4 ++-- .github/workflows/unittest.yml | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/.OwlBot.lock.yaml b/.github/.OwlBot.lock.yaml index 40bf9973..9bee2409 100644 --- a/.github/.OwlBot.lock.yaml +++ b/.github/.OwlBot.lock.yaml @@ -13,5 +13,5 @@ # limitations under the License. docker: image: gcr.io/cloud-devrel-public-resources/owlbot-python:latest - digest: sha256:230f7fe8a0d2ed81a519cfc15c6bb11c5b46b9fb449b8b1219b3771bcb520ad2 -# created: 2023-12-09T15:16:25.430769578Z + digest: sha256:346ab2efb51649c5dde7756cbbdc60dd394852ba83b9bbffc292a63549f33c17 +# created: 2023-12-14T22:17:57.611773021Z diff --git a/.github/workflows/unittest.yml b/.github/workflows/unittest.yml index d6ca6562..f4a337c4 100644 --- a/.github/workflows/unittest.yml +++ b/.github/workflows/unittest.yml @@ -26,9 +26,9 @@ jobs: run: | nox -s unit-${{ matrix.python }} - name: Upload coverage results - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: - name: coverage-artifacts + name: coverage-artifact-${{ matrix.python }} path: .coverage-${{ matrix.python }} cover: @@ -47,11 +47,11 @@ jobs: python -m pip install --upgrade setuptools pip wheel python -m pip install coverage - name: Download coverage results - uses: actions/download-artifact@v3 + uses: actions/download-artifact@v4 with: - name: coverage-artifacts path: .coverage-results/ - name: Report coverage results run: | - coverage combine .coverage-results/.coverage* + find .coverage-results -type f -name '*.zip' -exec unzip {} \; + coverage combine .coverage-results/**/.coverage* coverage report --show-missing --fail-under=100 From 3ac41ea05b67feb31218889379924ec0d5efb3ac Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 15 Dec 2023 15:29:36 -0800 Subject: [PATCH 25/39] got tests passing --- google/cloud/datastore/aggregation.py | 2 +- google/cloud/datastore/client.py | 2 +- tests/unit/test_aggregation.py | 6 ++++-- tests/unit/test_query.py | 4 +++- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/google/cloud/datastore/aggregation.py b/google/cloud/datastore/aggregation.py index bff9861e..1a78309c 100644 --- a/google/cloud/datastore/aggregation.py +++ b/google/cloud/datastore/aggregation.py @@ -448,7 +448,7 @@ def _next_page(self): transaction_id = None else: transaction_id = transaction.id - if transation._begin_later and transaction._status == transaction._INITIAL: + if transaction._begin_later and transaction._status == transaction._INITIAL: # if transaction hasn't been initialized, initialize it as part of this request new_transaction_options = transaction._options read_options = helpers.get_read_options( diff --git a/google/cloud/datastore/client.py b/google/cloud/datastore/client.py index 008ceee7..fa484499 100644 --- a/google/cloud/datastore/client.py +++ b/google/cloud/datastore/client.py @@ -203,7 +203,7 @@ def _extended_lookup( new_transaction_options = None if transaction is not None: transaction_id = transaction.id - if and transaction._begin_later and transaction._status == transaction._INITIAL: + if transaction._begin_later and transaction._status == transaction._INITIAL: # if transaction hasn't been initialized, initialize it as part of this request new_transaction_options = transaction._options diff --git a/tests/unit/test_aggregation.py b/tests/unit/test_aggregation.py index a493c779..b149a628 100644 --- a/tests/unit/test_aggregation.py +++ b/tests/unit/test_aggregation.py @@ -630,12 +630,14 @@ def test_transaction_begin_later(database_id, aggregation_type, aggregation_args the new_transaction field should be populated in the request read_options. """ import mock + from google.cloud.datastore_v1.types import TransactionOptions # make a fake begin_later transaction transaction = mock.Mock() transaction.id = None transaction._begin_later = True - transaction._state = transaction._INITIAL + transaction._status = transaction._INITIAL + transaction._options = TransactionOptions(read_only=TransactionOptions.ReadOnly()) mock_datastore_api = mock.Mock() mock_gapic = mock_datastore_api.run_aggregation_query mock_gapic.return_value = _make_aggregation_query_response([]) @@ -657,7 +659,7 @@ def test_transaction_begin_later(database_id, aggregation_type, aggregation_args request = mock_gapic.call_args[1]["request"] read_options = request["read_options"] # ensure new_transaction is populated - assert read_options.transaction is None + assert not read_options.transaction assert read_options.new_transaction == transaction._options class _Client(object): diff --git a/tests/unit/test_query.py b/tests/unit/test_query.py index 1f9648c9..5e330259 100644 --- a/tests/unit/test_query.py +++ b/tests/unit/test_query.py @@ -705,6 +705,7 @@ def test_transaction_begin_later(database_id): the new_transaction field should be populated in the request read_options. """ import mock + from google.cloud.datastore_v1.types import TransactionOptions # make a fake begin_later transaction @@ -712,6 +713,7 @@ def test_transaction_begin_later(database_id): transaction.id = None transaction._begin_later = True transaction._state = transaction._INITIAL + transaction._options = TransactionOptions(read_only=TransactionOptions.ReadOnly()) mock_datastore_api = mock.Mock() mock_gapic = mock_datastore_api.run_query @@ -734,7 +736,7 @@ def test_transaction_begin_later(database_id): request = mock_gapic.call_args[1]["request"] read_options = request["read_options"] # ensure new_transaction is populated - assert read_options.transaction is None + assert not read_options.transaction assert read_options.new_transaction == transaction._options From 7e8bb6efdcc54ede26e367d9bbe09c6453c99a9b Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Fri, 15 Dec 2023 23:31:59 +0000 Subject: [PATCH 26/39] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot=20?= =?UTF-8?q?post-processor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- tests/unit/test_aggregation.py | 1 + tests/unit/test_query.py | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/unit/test_aggregation.py b/tests/unit/test_aggregation.py index b149a628..04733a7d 100644 --- a/tests/unit/test_aggregation.py +++ b/tests/unit/test_aggregation.py @@ -662,6 +662,7 @@ def test_transaction_begin_later(database_id, aggregation_type, aggregation_args assert not read_options.transaction assert read_options.new_transaction == transaction._options + class _Client(object): def __init__( self, diff --git a/tests/unit/test_query.py b/tests/unit/test_query.py index 5e330259..8a3ed603 100644 --- a/tests/unit/test_query.py +++ b/tests/unit/test_query.py @@ -707,7 +707,6 @@ def test_transaction_begin_later(database_id): import mock from google.cloud.datastore_v1.types import TransactionOptions - # make a fake begin_later transaction transaction = mock.Mock() transaction.id = None @@ -740,8 +739,6 @@ def test_transaction_begin_later(database_id): assert read_options.new_transaction == transaction._options - - def test_iterator_constructor_defaults(): query = object() client = object() From 4ad10710bdb8651647b9a2ae7043e1bfaede2f9a Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Fri, 9 Feb 2024 22:01:42 +0000 Subject: [PATCH 27/39] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot=20?= =?UTF-8?q?post-processor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- .kokoro/requirements.txt | 6 +++--- noxfile.py | 20 +------------------- 2 files changed, 4 insertions(+), 22 deletions(-) diff --git a/.kokoro/requirements.txt b/.kokoro/requirements.txt index bb3d6ca3..e5c1ffca 100644 --- a/.kokoro/requirements.txt +++ b/.kokoro/requirements.txt @@ -263,9 +263,9 @@ jeepney==0.8.0 \ # via # keyring # secretstorage -jinja2==3.1.3 \ - --hash=sha256:7d6d50dd97d52cbc355597bd845fabfbac3f551e1f99619e39a35ce8c370b5fa \ - --hash=sha256:ac8bd6544d4bb2c9792bf3a159e80bba8fda7f07e81bc3aed565432d5925ba90 +jinja2==3.1.2 \ + --hash=sha256:31351a702a408a9e7595a8fc6150fc3f43bb6bf7e319770cbc0db9df9437e852 \ + --hash=sha256:6088930bfe239f0e6710546ab9c19c9ef35e29792895fed6e6e31a023a182a61 # via gcp-releasetool keyring==24.2.0 \ --hash=sha256:4901caaf597bfd3bbd78c9a0c7c4c29fcd8310dab2cffefe749e916b6527acd6 \ diff --git a/noxfile.py b/noxfile.py index e4e112d5..a7c7fd3e 100644 --- a/noxfile.py +++ b/noxfile.py @@ -310,16 +310,7 @@ def docs(session): session.install("-e", ".") session.install( - # We need to pin to specific versions of the `sphinxcontrib-*` packages - # which still support sphinx 4.x. - # See https://github.com/googleapis/sphinx-docfx-yaml/issues/344 - # and https://github.com/googleapis/sphinx-docfx-yaml/issues/345. - "sphinxcontrib-applehelp==1.0.4", - "sphinxcontrib-devhelp==1.0.2", - "sphinxcontrib-htmlhelp==2.0.1", - "sphinxcontrib-qthelp==1.0.3", - "sphinxcontrib-serializinghtml==1.1.5", - "sphinx==4.5.0", + "sphinx==4.0.1", "alabaster", "recommonmark", ) @@ -356,15 +347,6 @@ def docfx(session): session.install("-e", ".") session.install( - # We need to pin to specific versions of the `sphinxcontrib-*` packages - # which still support sphinx 4.x. - # See https://github.com/googleapis/sphinx-docfx-yaml/issues/344 - # and https://github.com/googleapis/sphinx-docfx-yaml/issues/345. - "sphinxcontrib-applehelp==1.0.4", - "sphinxcontrib-devhelp==1.0.2", - "sphinxcontrib-htmlhelp==2.0.1", - "sphinxcontrib-qthelp==1.0.3", - "sphinxcontrib-serializinghtml==1.1.5", "gcp-sphinx-docfx-yaml", "alabaster", "recommonmark", From ce9e335ac77f5eb707adca12689f45c945cf7917 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 9 Feb 2024 14:02:26 -0800 Subject: [PATCH 28/39] remove begin from put and delete --- google/cloud/datastore/transaction.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 043356cd..2a1b44e9 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -366,7 +366,6 @@ def commit(self, retry=None, timeout=None): # Clear our own ID in case this gets accidentally reused. self._id = None - @_begin_if_not_began def put(self, entity): """Adds an entity to be committed. @@ -385,7 +384,6 @@ def put(self, entity): else: super(Transaction, self).put(entity) - @_begin_if_not_began def delete(self, key): super(Transaction, self).delete(key) From 28f6d10bba96dd1232eface549caacdbc254bb06 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 9 Feb 2024 14:05:18 -0800 Subject: [PATCH 29/39] fixed jinja version --- .github/.OwlBot.lock.yaml | 4 ++-- .kokoro/requirements.txt | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/.OwlBot.lock.yaml b/.github/.OwlBot.lock.yaml index 35591e16..d8a1bbca 100644 --- a/.github/.OwlBot.lock.yaml +++ b/.github/.OwlBot.lock.yaml @@ -13,5 +13,5 @@ # limitations under the License. docker: image: gcr.io/cloud-devrel-public-resources/owlbot-python:latest - digest: sha256:346ab2efb51649c5dde7756cbbdc60dd394852ba83b9bbffc292a63549f33c17 -# created: 2023-12-14T22:17:57.611773021Z + digest: sha256:5ea6d0ab82c956b50962f91d94e206d3921537ae5fe1549ec5326381d8905cfa +# created: 2024-01-15T16:32:08.142785673Z diff --git a/.kokoro/requirements.txt b/.kokoro/requirements.txt index e5c1ffca..bb3d6ca3 100644 --- a/.kokoro/requirements.txt +++ b/.kokoro/requirements.txt @@ -263,9 +263,9 @@ jeepney==0.8.0 \ # via # keyring # secretstorage -jinja2==3.1.2 \ - --hash=sha256:31351a702a408a9e7595a8fc6150fc3f43bb6bf7e319770cbc0db9df9437e852 \ - --hash=sha256:6088930bfe239f0e6710546ab9c19c9ef35e29792895fed6e6e31a023a182a61 +jinja2==3.1.3 \ + --hash=sha256:7d6d50dd97d52cbc355597bd845fabfbac3f551e1f99619e39a35ce8c370b5fa \ + --hash=sha256:ac8bd6544d4bb2c9792bf3a159e80bba8fda7f07e81bc3aed565432d5925ba90 # via gcp-releasetool keyring==24.2.0 \ --hash=sha256:4901caaf597bfd3bbd78c9a0c7c4c29fcd8310dab2cffefe749e916b6527acd6 \ From 61237b3e9a82ccae89db17e28250fe793366320a Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 9 Feb 2024 14:38:47 -0800 Subject: [PATCH 30/39] fixed tests --- google/cloud/datastore/query.py | 5 +-- tests/unit/test_aggregation.py | 4 ++- tests/unit/test_query.py | 10 +++--- tests/unit/test_transaction.py | 64 --------------------------------- 4 files changed, 12 insertions(+), 71 deletions(-) diff --git a/google/cloud/datastore/query.py b/google/cloud/datastore/query.py index 85cc97a9..0775e47c 100644 --- a/google/cloud/datastore/query.py +++ b/google/cloud/datastore/query.py @@ -784,8 +784,9 @@ def _next_page(self): transaction_id = None else: transaction_id = transaction.id - # if transaction hasn't been initialized, initialize it as part of this request - new_transaction_options = transaction._options + if transaction._begin_later and transaction._status == transaction._INITIAL: + # if transaction hasn't been initialized, initialize it as part of this request + new_transaction_options = transaction._options read_options = helpers.get_read_options( self._eventual, transaction_id, self._read_time, new_transaction_options ) diff --git a/tests/unit/test_aggregation.py b/tests/unit/test_aggregation.py index 04733a7d..8284b808 100644 --- a/tests/unit/test_aggregation.py +++ b/tests/unit/test_aggregation.py @@ -471,7 +471,9 @@ def _next_page_helper(txn_id=None, retry=None, timeout=None, database_id=None): if txn_id is None: client = _Client(project, datastore_api=ds_api, database=database_id) else: - transaction = mock.Mock(id=txn_id, spec=["id"]) + transaction = mock.Mock( + id=txn_id, _begin_later=False, spec=["id", "_begin_later"] + ) client = _Client( project, datastore_api=ds_api, transaction=transaction, database=database_id ) diff --git a/tests/unit/test_query.py b/tests/unit/test_query.py index 8a3ed603..6c2063bb 100644 --- a/tests/unit/test_query.py +++ b/tests/unit/test_query.py @@ -667,7 +667,7 @@ def test_eventual_transaction_fails(database_id): @pytest.mark.parametrize("database_id", [None, "somedb"]) def test_transaction_id_populated(database_id): """ - When an aggregation is run in the context of a transaction, the transaction + When an query is run in the context of a transaction, the transaction ID should be populated in the request. """ import mock @@ -699,7 +699,7 @@ def test_transaction_id_populated(database_id): @pytest.mark.parametrize("database_id", [None, "somedb"]) -def test_transaction_begin_later(database_id): +def test_query_transaction_begin_later(database_id): """ When an aggregation is run in the context of a transaction with begin_later=True, the new_transaction field should be populated in the request read_options. @@ -711,7 +711,7 @@ def test_transaction_begin_later(database_id): transaction = mock.Mock() transaction.id = None transaction._begin_later = True - transaction._state = transaction._INITIAL + transaction._status = transaction._INITIAL transaction._options = TransactionOptions(read_only=TransactionOptions.ReadOnly()) mock_datastore_api = mock.Mock() @@ -926,7 +926,9 @@ def _next_page_helper( if txn_id is None: client = _Client(project, database=database, datastore_api=ds_api) else: - transaction = mock.Mock(id=txn_id, spec=["id"]) + transaction = mock.Mock( + id=txn_id, _begin_later=False, spec=["id", "_begin_later"] + ) client = _Client( project, database=database, datastore_api=ds_api, transaction=transaction ) diff --git a/tests/unit/test_transaction.py b/tests/unit/test_transaction.py index 6ba13223..b934e4f4 100644 --- a/tests/unit/test_transaction.py +++ b/tests/unit/test_transaction.py @@ -491,70 +491,6 @@ def test_transaction_put_read_only(database_id): xact.put(entity) -@pytest.mark.parametrize("database_id", [None, "somedb"]) -def test_transaction_begin_later_with_delete(database_id): - """ - begin_later transactions should begin on delete - """ - project = "PROJECT" - id_ = 912830 - ds_api = _make_datastore_api(xact_id=id_) - client = _Client(project, datastore_api=ds_api, database=database_id) - xact = _make_transaction(client, begin_later=True) - - fake_key = mock.Mock() - - with mock.patch("google.cloud.datastore.batch.Batch.delete") as delete: - with xact: - assert xact._status == xact._INITIAL - assert xact.id is None - xact.delete(fake_key) - # call should have started transaction - assert xact._status == xact._IN_PROGRESS - assert xact.id == id_ - # super class delete should have been called - assert delete.call_count == 1 - assert delete.call_args == mock.call(fake_key) - # should be finalized after context manager block - assert xact._status == xact._FINISHED - assert xact.id is None - # should have committed - assert ds_api.commit.call_count == 1 - assert ds_api.rollback.call_count == 0 - - -@pytest.mark.parametrize("database_id", [None, "somedb"]) -def test_transaction_begin_later_with_put(database_id): - """ - begin_later transactions should begin on put - """ - project = "PROJECT" - id_ = 912830 - ds_api = _make_datastore_api(xact_id=id_) - client = _Client(project, datastore_api=ds_api, database=database_id) - xact = _make_transaction(client, begin_later=True) - - fake_entity = mock.Mock() - - with mock.patch("google.cloud.datastore.batch.Batch.put") as put: - with xact: - assert xact._status == xact._INITIAL - assert xact.id is None - xact.put(fake_entity) - # call should have started transaction - assert xact._status == xact._IN_PROGRESS - assert xact.id == id_ - # super class put should have been called - assert put.call_count == 1 - assert put.call_args == mock.call(fake_entity) - # should be finalized after context manager block - assert xact._status == xact._FINISHED - assert xact.id is None - # should have committed - assert ds_api.commit.call_count == 1 - assert ds_api.rollback.call_count == 0 - - def _make_key(kind, id_, project, database=None): from google.cloud.datastore_v1.types import entity as entity_pb2 From 77e410a98bccdba445579b76d0b5d23cc7f39b73 Mon Sep 17 00:00:00 2001 From: Owl Bot Date: Fri, 9 Feb 2024 22:40:50 +0000 Subject: [PATCH 31/39] =?UTF-8?q?=F0=9F=A6=89=20Updates=20from=20OwlBot=20?= =?UTF-8?q?post-processor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md --- noxfile.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/noxfile.py b/noxfile.py index a7c7fd3e..e4e112d5 100644 --- a/noxfile.py +++ b/noxfile.py @@ -310,7 +310,16 @@ def docs(session): session.install("-e", ".") session.install( - "sphinx==4.0.1", + # We need to pin to specific versions of the `sphinxcontrib-*` packages + # which still support sphinx 4.x. + # See https://github.com/googleapis/sphinx-docfx-yaml/issues/344 + # and https://github.com/googleapis/sphinx-docfx-yaml/issues/345. + "sphinxcontrib-applehelp==1.0.4", + "sphinxcontrib-devhelp==1.0.2", + "sphinxcontrib-htmlhelp==2.0.1", + "sphinxcontrib-qthelp==1.0.3", + "sphinxcontrib-serializinghtml==1.1.5", + "sphinx==4.5.0", "alabaster", "recommonmark", ) @@ -347,6 +356,15 @@ def docfx(session): session.install("-e", ".") session.install( + # We need to pin to specific versions of the `sphinxcontrib-*` packages + # which still support sphinx 4.x. + # See https://github.com/googleapis/sphinx-docfx-yaml/issues/344 + # and https://github.com/googleapis/sphinx-docfx-yaml/issues/345. + "sphinxcontrib-applehelp==1.0.4", + "sphinxcontrib-devhelp==1.0.2", + "sphinxcontrib-htmlhelp==2.0.1", + "sphinxcontrib-qthelp==1.0.3", + "sphinxcontrib-serializinghtml==1.1.5", "gcp-sphinx-docfx-yaml", "alabaster", "recommonmark", From f4512baf8569c86e969ce02e738c4dd08b7de872 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 9 Feb 2024 14:53:39 -0800 Subject: [PATCH 32/39] remove unused decorator --- google/cloud/datastore/transaction.py | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 2a1b44e9..cf92cbf0 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -284,22 +284,6 @@ def wrapped(self, *args, **kwargs): return wrapped - def _begin_if_not_began(fn: Callable) -> Callable: # type: ignore - """ - Function wrapper to begin transaction if it hasn't started when - the wrapped function is called. - - Used by put and delete. - """ - - @functools.wraps(fn) - def wrapped(self, *args, **kwargs): - if self._begin_later and self._status == self._INITIAL: - self.begin() - return fn(self, *args, **kwargs) - - return wrapped - @_abort_if_not_began def rollback(self, retry=None, timeout=None): """Rolls back the current transaction. From b2012a898d6804b635822be837c1e40d7ad91cc1 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Fri, 9 Feb 2024 14:58:12 -0800 Subject: [PATCH 33/39] removed unneeded delete override --- google/cloud/datastore/transaction.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index cf92cbf0..090f76c6 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -368,9 +368,6 @@ def put(self, entity): else: super(Transaction, self).put(entity) - def delete(self, key): - super(Transaction, self).delete(key) - def __enter__(self): if not self._begin_later: self.begin() From cfa11db305efbb112b369ac9edec2083c35a4878 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 22 Feb 2024 16:11:02 -0800 Subject: [PATCH 34/39] fixed commit without begin call --- google/cloud/datastore/batch.py | 17 +++++++++-- google/cloud/datastore/transaction.py | 43 ++++++++++++++------------- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/google/cloud/datastore/batch.py b/google/cloud/datastore/batch.py index e0dbf26d..32a320ee 100644 --- a/google/cloud/datastore/batch.py +++ b/google/cloud/datastore/batch.py @@ -192,6 +192,19 @@ def mutations(self): """ return self._mutations + def _allow_mutations(self) -> bool: + """ + This method is called to see if the batch is in a proper state to allow + `put` and `delete` operations. + + the Transaction subclass overrides this method to support + the `begin_later` flag. + + :rtype: bool + :returns: True if the batch is in a state to allow mutations. + """ + return self._status == self._IN_PROGRESS + def put(self, entity): """Remember an entity's state to be saved during :meth:`commit`. @@ -218,7 +231,7 @@ def put(self, entity): progress, if entity has no key assigned, or if the key's ``project`` does not match ours. """ - if self._status != self._IN_PROGRESS: + if not self._allow_mutations(): raise ValueError("Batch must be in progress to put()") if entity.key is None: @@ -248,7 +261,7 @@ def delete(self, key): progress, if key is not complete, or if the key's ``project`` does not match ours. """ - if self._status != self._IN_PROGRESS: + if not self._allow_mutations(): raise ValueError("Batch must be in progress to delete()") if key.is_partial: diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index 090f76c6..ec6e5927 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -265,26 +265,6 @@ def _begin_with_id(self, transaction_id): self._id = transaction_id self._status = self._IN_PROGRESS - def _abort_if_not_began(fn: Callable) -> Callable: # type: ignore - """ - Function wrapper to abort transaction if it hasn't started when - the wrapped function is called. - - Used by commit and rollback. - """ - - @functools.wraps(fn) - def wrapped(self, *args, **kwargs): - if self._status == self._INITIAL: - self._status = self._ABORTED - self._id = None - return None - else: - return fn(self, *args, **kwargs) - - return wrapped - - @_abort_if_not_began def rollback(self, retry=None, timeout=None): """Rolls back the current transaction. @@ -303,6 +283,12 @@ def rollback(self, retry=None, timeout=None): Note that if ``retry`` is specified, the timeout applies to each individual attempt. """ + # if transaction has not started, abort it + if self._status == self._INITIAL: + self._status = self._ABORTED + self._id = None + return None + kwargs = _make_retry_timeout_kwargs(retry, timeout) try: @@ -319,7 +305,6 @@ def rollback(self, retry=None, timeout=None): # Clear our own ID in case this gets accidentally reused. self._id = None - @_abort_if_not_began def commit(self, retry=None, timeout=None): """Commits the transaction. @@ -342,6 +327,15 @@ def commit(self, retry=None, timeout=None): Note that if ``retry`` is specified, the timeout applies to each individual attempt. """ + # if transaction has not begun, either begin now, or abort if empty + if self._status == self._INITIAL: + if not self._mutations: + self._status = self._ABORTED + self._id = None + return None + else: + self.begin() + kwargs = _make_retry_timeout_kwargs(retry, timeout) try: @@ -373,3 +367,10 @@ def __enter__(self): self.begin() self._client._push_batch(self) return self + + def _allow_mutations(self): + """ + Mutations can be added to a transaction if it is in IN_PROGRESS state, + or if it is in INITIAL state and the begin_later flag is set. + """ + return self._status == self._IN_PROGRESS or (self._begin_later and self._status == self._INITIAL) From efc8e8db05ee4ce3590dc1201e70831d5b826961 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 22 Feb 2024 16:46:06 -0800 Subject: [PATCH 35/39] improved tests --- google/cloud/datastore/batch.py | 9 ++-- tests/system/test_transaction.py | 8 +-- tests/unit/test_transaction.py | 93 ++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 7 deletions(-) diff --git a/google/cloud/datastore/batch.py b/google/cloud/datastore/batch.py index 32a320ee..58dd2174 100644 --- a/google/cloud/datastore/batch.py +++ b/google/cloud/datastore/batch.py @@ -383,10 +383,11 @@ def __enter__(self): def __exit__(self, exc_type, exc_val, exc_tb): try: - if exc_type is None: - self.commit() - else: - self.rollback() + if self._status not in (self._ABORTED, self._FINISHED): + if exc_type is None: + self.commit() + else: + self.rollback() finally: self._client._pop_batch() diff --git a/tests/system/test_transaction.py b/tests/system/test_transaction.py index 755d957f..f9daf129 100644 --- a/tests/system/test_transaction.py +++ b/tests/system/test_transaction.py @@ -47,7 +47,7 @@ def test_transaction_begin_later( datastore_client, entities_to_delete, database_id, first_call ): """ - transactions with begin_later should call begin on first rpc + transactions with begin_later should call begin on first get rpc, or on commit """ key = datastore_client.key("Company", "Google") entity = datastore.Entity(key=key) @@ -61,12 +61,14 @@ def test_transaction_begin_later( assert xact._status == xact._INITIAL if first_call == "get": datastore_client.get(entity.key) + assert xact._status == xact._IN_PROGRESS + assert xact._id is not None elif first_call == "put": xact.put(entity) + assert xact._status == xact._INITIAL elif first_call == "delete": xact.delete(result_entity.key) - assert xact._id is not None - assert xact._status == xact._IN_PROGRESS + assert xact._status == xact._INITIAL assert xact._status == xact._FINISHED entities_to_delete.append(result_entity) diff --git a/tests/unit/test_transaction.py b/tests/unit/test_transaction.py index b934e4f4..92b455e0 100644 --- a/tests/unit/test_transaction.py +++ b/tests/unit/test_transaction.py @@ -491,6 +491,99 @@ def test_transaction_put_read_only(database_id): xact.put(entity) +@pytest.mark.parametrize("database_id", [None, "somedb"]) +def test_transaction_put_w_begin_later(database_id): + """ + If begin_later is set, should be able to call put without begin first + """ + project = "PROJECT" + id_ = 943243 + ds_api = _make_datastore_api(xact_id=id_) + client = _Client(project, datastore_api=ds_api, database=database_id) + entity = _Entity(database=database_id) + with _make_transaction(client, begin_later=True) as xact: + assert xact._status == xact._INITIAL + assert len(xact.mutations) == 0 + xact.put(entity) + assert len(xact.mutations) == 1 + # should still be in initial state + assert xact._status == xact._INITIAL + +@pytest.mark.parametrize("database_id", [None, "somedb"]) +def test_transaction_delete_w_begin_later(database_id): + """ + If begin_later is set, should be able to call delete without begin first + """ + project = "PROJECT" + id_ = 943243 + ds_api = _make_datastore_api(xact_id=id_) + client = _Client(project, datastore_api=ds_api, database=database_id) + entity = _Entity(database=database_id) + with _make_transaction(client, begin_later=True) as xact: + assert xact._status == xact._INITIAL + assert len(xact.mutations) == 0 + xact.delete(entity.key.completed_key("name")) + assert len(xact.mutations) == 1 + # should still be in initial state + assert xact._status == xact._INITIAL + + +@pytest.mark.parametrize("database_id", [None, "somedb"]) +def test_transaction_rollback_no_begin(database_id): + """ + If rollback is called without begin, transaciton should abort + """ + project = "PROJECT" + id_ = 943243 + ds_api = _make_datastore_api(xact_id=id_) + client = _Client(project, datastore_api=ds_api, database=database_id) + with _make_transaction(client, begin_later=True) as xact: + assert xact._status == xact._INITIAL + with mock.patch.object(xact, "begin") as begin: + xact.rollback() + begin.assert_not_called() + assert xact._status == xact._ABORTED + + +@pytest.mark.parametrize("database_id", [None, "somedb"]) +def test_transaction_commit_no_begin(database_id): + """ + If commit is called without begin, and it has mutations staged, + should call begin before commit + """ + project = "PROJECT" + id_ = 943243 + ds_api = _make_datastore_api(xact_id=id_) + client = _Client(project, datastore_api=ds_api, database=database_id) + entity = _Entity(database=database_id) + with _make_transaction(client, begin_later=True) as xact: + assert xact._status == xact._INITIAL + xact.put(entity) + assert xact._status == xact._INITIAL + with mock.patch.object(xact, "begin") as begin: + begin.side_effect = lambda: setattr(xact, "_status", xact._IN_PROGRESS) + xact.commit() + begin.assert_called_once_with() + + +@pytest.mark.parametrize("database_id", [None, "somedb"]) +def test_empty_transaction_commit(database_id): + """ + If commit is called without begin, and it has no mutations staged, + should abort + """ + project = "PROJECT" + id_ = 943243 + ds_api = _make_datastore_api(xact_id=id_) + client = _Client(project, datastore_api=ds_api, database=database_id) + with _make_transaction(client, begin_later=True) as xact: + assert xact._status == xact._INITIAL + with mock.patch.object(xact, "begin") as begin: + xact.commit() + begin.assert_not_called() + assert xact._status == xact._ABORTED + + def _make_key(kind, id_, project, database=None): from google.cloud.datastore_v1.types import entity as entity_pb2 From 6872fcf8cb05db26882601d68b8073d11ac720fe Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 22 Feb 2024 17:03:37 -0800 Subject: [PATCH 36/39] refactored out helper function --- google/cloud/datastore/aggregation.py | 12 +++--------- google/cloud/datastore/client.py | 12 ++++-------- google/cloud/datastore/helpers.py | 20 ++++++++++++++++++++ google/cloud/datastore/query.py | 11 +++-------- google/cloud/datastore/transaction.py | 4 +++- tests/unit/test_transaction.py | 1 + 6 files changed, 34 insertions(+), 26 deletions(-) diff --git a/google/cloud/datastore/aggregation.py b/google/cloud/datastore/aggregation.py index 1a78309c..1384f332 100644 --- a/google/cloud/datastore/aggregation.py +++ b/google/cloud/datastore/aggregation.py @@ -442,15 +442,9 @@ def _next_page(self): return None query_pb = self._build_protobuf() - new_transaction_options = None - transaction = self.client.current_transaction - if transaction is None: - transaction_id = None - else: - transaction_id = transaction.id - if transaction._begin_later and transaction._status == transaction._INITIAL: - # if transaction hasn't been initialized, initialize it as part of this request - new_transaction_options = transaction._options + transaction_id, new_transaction_options = helpers.get_transaction_options( + self.client.current_transaction + ) read_options = helpers.get_read_options( self._eventual, transaction_id, self._read_time, new_transaction_options ) diff --git a/google/cloud/datastore/client.py b/google/cloud/datastore/client.py index fa484499..b1e79d91 100644 --- a/google/cloud/datastore/client.py +++ b/google/cloud/datastore/client.py @@ -200,17 +200,13 @@ def _extended_lookup( results = [] transaction_id = None - new_transaction_options = None - if transaction is not None: - transaction_id = transaction.id - if transaction._begin_later and transaction._status == transaction._INITIAL: - # if transaction hasn't been initialized, initialize it as part of this request - new_transaction_options = transaction._options - - loop_num = 0 + transaction_id, new_transaction_options = helpers.get_transaction_options( + transaction + ) read_options = helpers.get_read_options( eventual, transaction_id, read_time, new_transaction_options ) + loop_num = 0 while loop_num < _MAX_LOOPS: # loop against possible deferred. loop_num += 1 request = { diff --git a/google/cloud/datastore/helpers.py b/google/cloud/datastore/helpers.py index 647bf89a..6eaa3b89 100644 --- a/google/cloud/datastore/helpers.py +++ b/google/cloud/datastore/helpers.py @@ -278,6 +278,26 @@ def get_read_options( return new_options +def get_transaction_options(transaction): + """ + Get the transaction_id or new_transaction_options field from an active transaction object, + for use in get_read_options + + These are mutually-exclusive fields, so one or both will be None. + + :rtype: Tuple[Optional[bytes], Optional[google.cloud.datastore_v1.types.TransactionOptions]] + :returns: The transaction_id and new_transaction_options fields from the transaction object. + """ + transaction_id, new_transaction_options = None, None + if transaction is not None: + if transaction.id is not None: + transaction_id = transaction.id + elif transaction._begin_later and transaction._status == transaction._INITIAL: + # If the transaction has not yet been begun, we can use the new_transaction_options field. + new_transaction_options = transaction._options + return transaction_id, new_transaction_options + + def key_from_protobuf(pb): """Factory method for creating a key based on a protobuf. diff --git a/google/cloud/datastore/query.py b/google/cloud/datastore/query.py index 0775e47c..72d6fe51 100644 --- a/google/cloud/datastore/query.py +++ b/google/cloud/datastore/query.py @@ -779,14 +779,9 @@ def _next_page(self): query_pb = self._build_protobuf() new_transaction_options = None - transaction = self.client.current_transaction - if transaction is None: - transaction_id = None - else: - transaction_id = transaction.id - if transaction._begin_later and transaction._status == transaction._INITIAL: - # if transaction hasn't been initialized, initialize it as part of this request - new_transaction_options = transaction._options + transaction_id, new_transaction_options = helpers.get_transaction_options( + self.client.current_transaction + ) read_options = helpers.get_read_options( self._eventual, transaction_id, self._read_time, new_transaction_options ) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index ec6e5927..afbe3f4b 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -373,4 +373,6 @@ def _allow_mutations(self): Mutations can be added to a transaction if it is in IN_PROGRESS state, or if it is in INITIAL state and the begin_later flag is set. """ - return self._status == self._IN_PROGRESS or (self._begin_later and self._status == self._INITIAL) + return self._status == self._IN_PROGRESS or ( + self._begin_later and self._status == self._INITIAL + ) diff --git a/tests/unit/test_transaction.py b/tests/unit/test_transaction.py index 92b455e0..cee384bb 100644 --- a/tests/unit/test_transaction.py +++ b/tests/unit/test_transaction.py @@ -509,6 +509,7 @@ def test_transaction_put_w_begin_later(database_id): # should still be in initial state assert xact._status == xact._INITIAL + @pytest.mark.parametrize("database_id", [None, "somedb"]) def test_transaction_delete_w_begin_later(database_id): """ From 19ed6e953f1dac1f5807a10e2a775314836ea460 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 22 Feb 2024 17:03:54 -0800 Subject: [PATCH 37/39] fixed lint --- google/cloud/datastore/transaction.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/google/cloud/datastore/transaction.py b/google/cloud/datastore/transaction.py index afbe3f4b..52c17ce2 100644 --- a/google/cloud/datastore/transaction.py +++ b/google/cloud/datastore/transaction.py @@ -13,9 +13,6 @@ # limitations under the License. """Create / interact with Google Cloud Datastore transactions.""" -import functools -from typing import Callable - from google.cloud.datastore.batch import Batch from google.cloud.datastore_v1.types import TransactionOptions from google.protobuf import timestamp_pb2 From 4682186d2afe8380d570000358b0188368462314 Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 22 Feb 2024 17:05:13 -0800 Subject: [PATCH 38/39] added comment --- google/cloud/datastore/batch.py | 1 + 1 file changed, 1 insertion(+) diff --git a/google/cloud/datastore/batch.py b/google/cloud/datastore/batch.py index 58dd2174..69100bc6 100644 --- a/google/cloud/datastore/batch.py +++ b/google/cloud/datastore/batch.py @@ -383,6 +383,7 @@ def __enter__(self): def __exit__(self, exc_type, exc_val, exc_tb): try: + # commit or rollback if not in terminal state if self._status not in (self._ABORTED, self._FINISHED): if exc_type is None: self.commit() From abf29e3abea06b22a30a1d8847ed76dd1ea101eb Mon Sep 17 00:00:00 2001 From: Daniel Sanche Date: Thu, 22 Feb 2024 17:24:46 -0800 Subject: [PATCH 39/39] added tests for helper --- tests/unit/test_helpers.py | 52 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/unit/test_helpers.py b/tests/unit/test_helpers.py index c3255e4f..7ba75d52 100644 --- a/tests/unit/test_helpers.py +++ b/tests/unit/test_helpers.py @@ -619,6 +619,58 @@ def test__get_read_options_w_multiple_args(args): get_read_options(*args) +def test__get_transaction_options_none(): + """ + test with empty transaction input + """ + from google.cloud.datastore.helpers import get_transaction_options + + t_id, new_t = get_transaction_options(None) + assert t_id is None + assert new_t is None + + +def test__get_transaction_options_w_id(): + """ + test with transaction with id set + """ + from google.cloud.datastore.helpers import get_transaction_options + from google.cloud.datastore import Transaction + + expected_id = b"123abc" + txn = Transaction(None, begin_later=True) + txn._id = expected_id + t_id, new_t = get_transaction_options(txn) + assert t_id == expected_id + assert new_t is None + + +def test__get_transaction_options_w_begin_later(): + """ + if begin later is set and it hasn't begun, should return new_transaction_options + """ + from google.cloud.datastore.helpers import get_transaction_options + from google.cloud.datastore import Transaction + + txn = Transaction(None, begin_later=True) + t_id, new_t = get_transaction_options(txn) + assert t_id is None + assert new_t is txn._options + + +def test__get_transaction_options_not_started(): + """ + If the transaction is noet set as begin_later, but it hasn't begun, return None for both + """ + from google.cloud.datastore.helpers import get_transaction_options + from google.cloud.datastore import Transaction + + txn = Transaction(None, begin_later=False) + t_id, new_t = get_transaction_options(txn) + assert t_id is None + assert new_t is None + + def test__pb_attr_value_w_datetime_naive(): import calendar import datetime