Datastore: Add support for 'TransactionOptions' by chemelnucfin · Pull Request #4357 · googleapis/google-cloud-python

chemelnucfin · 2017-11-08T06:45:30Z

Changes reflecting review and Luke's autogen (please see #4348 for issues regarding his autogen). A manual fix has been included.

Closes #4335. This includes reviews and changes from there.

chemelnucfin · 2017-11-08T17:46:34Z

hold on a second. I am still missing a couple comments.

dhermes

@chemelnucfin This PR does add partial support for TransactionOptions, but there is still much more to do to actually use the previous_transaction field "automatically" for users.

datastore/google/cloud/datastore_v1/gapic/datastore_client.py

               project_id,
-               keys,
               read_options=None,
+               keys=None,


datastore/google/cloud/datastore/transaction.py

    _status = None

-    def __init__(self, client):
+    def __init__(self, client, options=None):


datastore/tests/system/test_system.py

        self.case_entities_to_delete.append(retrieved_entity)
        self.assertEqual(retrieved_entity, entity)

+    def test_readonly_put(self):


datastore/tests/system/test_system.py

+    def test_readwrite_put(self):
+        entity = datastore.Entity(key=Config.CLIENT.key('Company', 'Google'))
+        options = types.TransactionOptions(
+                            read_write=types.TransactionOptions.ReadWrite())


datastore/google/cloud/datastore/transaction.py

    :type client: :class:`google.cloud.datastore.client.Client`
    :param client: the client used to connect to datastore.
+
+    :type options: `google.cloud.datastore.transaction.TransactionOptions`


datastore/tests/unit/test_transaction.py

+
+
+def _assert_num_mutations(test_case, mutation_pb_list, num_mutations):
+    test_case.assertEqual(len(mutation_pb_list), num_mutations)


datastore/tests/unit/test_transaction.py

+        client = _Client(project, datastore_api=ds_api)
+        options = self._make_options(read_write=
+                                     self._make_options().ReadWrite(
+                                        previous_transaction=b'321'))


datastore/tests/unit/test_transaction.py

+        ds_api = _make_datastore_api(xact_id=id_)
+        client = _Client(project, datastore_api=ds_api)
+        options = self._make_options(read_write=
+                                     self._make_options().ReadWrite(


datastore/tests/unit/test_transaction.py

+        id_ = 943243
+        ds_api = _make_datastore_api(xact_id=id_)
+        client = _Client(project, datastore_api=ds_api)
+        options = self._make_options(read_only=self._make_options().ReadOnly())


datastore/tests/unit/test_transaction.py

 def _make_datastore_api(*keys, **kwargs):
    commit_method = mock.Mock(
        return_value=_make_commit_response(*keys), spec=[])
-


chemelnucfin · 2017-11-28T22:54:18Z

I mean, wouldn't it be good to have one here to make sure it doesn't reach the network (like it is doing what it should do)?

…

On Tue, Nov 28, 2017 at 2:09 PM, Danny Hermes ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In datastore/tests/system/test_system.py <#4357 (comment)> : > @@ -442,6 +443,25 @@ def test_transaction_via_with_statement(self): self.case_entities_to_delete.append(retrieved_entity) self.assertEqual(retrieved_entity, entity) + def test_readonly_put(self): The test above it (test_transaction_via_with_statement) actually talks to the network. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4357 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADzDDArMkzdDy7R-dwVPKOnOJ5P1KfCrks5s7ISrgaJpZM4QV6hc> .

dhermes · 2017-11-28T22:56:47Z

No, we can test that code path with a unit test.

chemelnucfin · 2017-11-29T07:47:39Z

@dhermes @tseaver PTAL.

I will still need to think about how to retry the previous_transaction field though.

datastore/google/cloud/datastore/client.py

        lookup_response = datastore_api.lookup(
            project,
-            key_pbs,
+            keys=key_pbs,


datastore/google/cloud/datastore/client.py

        """Proxy to :class:`google.cloud.datastore.batch.Batch`."""
        return Batch(self)

+    def read_only_transaction(self):


datastore/google/cloud/datastore/transaction.py

+        else:
+            options = TransactionOptions(
+                read_only=TransactionOptions.ReadOnly())
+        self.options = options


datastore/google/cloud/datastore/transaction.py

+    def __init__(self, client, read_only=False):
        super(Transaction, self).__init__(client)
        self._id = None
+        if not read_only:


datastore/google/cloud/datastore/transaction.py

            self._id = None
+
+    def put(self, entity):
+        """Ensures the transaction is not marked readonly and calls Batch.put


datastore/tests/unit/test_transaction.py

+        client = _Client(project, datastore_api=ds_api)
+        read_only = self._get_options_class().ReadOnly()
+        options = self._make_options(read_only=read_only)
+        xact = self._make_one(client, read_only=True)


datastore/tests/unit/test_transaction.py

+        begin.assert_called_once()
+        self.assertEqual(begin.call_count, 1)
+
+    def test_previous_tid(self):


datastore/tests/unit/test_transaction.py

+        with self.assertRaises(RuntimeError):
+            xact.put(entity)
+
+    def test_put_read_write(self):


datastore/tests/unit/test_transaction.py

+        self.assertEqual(mutated_entity.key, entity.key.to_protobuf())
+
+
+def _mutated_pb(test_case, mutation_pb_list, mutation_type):


datastore/tests/unit/test_transaction.py

+        ds_api = _make_datastore_api(xact_id=id_)
+        client = _Client(project, datastore_api=ds_api)
+        options_klass = self._get_options_class()
+        options = self._make_options(read_only=options_klass.ReadOnly())


tseaver · 2017-12-04T17:49:10Z

@chemelnucfin Lint failure.

chemelnucfin · 2017-12-05T01:04:00Z

@tseaver PTAL, thanks.

dhermes

LGTM other than my last comment

datastore/tests/unit/test_client.py

+        client = self._make_one(credentials=creds)
+        xact = client.transaction(read_only=True)
+        self.assertIsInstance(xact._options.read_only,
+                              TransactionOptions.ReadOnly)


datastore/tests/unit/test_client.py

-        self.assertIsInstance(xact._options.read_only,
-                              TransactionOptions.ReadOnly)
+        self.assertEqual(xact._options.read_only,
+                         TransactionOptions.ReadOnly())


datastore/google/cloud/datastore/client.py

+    def transaction(self, **kwargs):
+        """Proxy to :class:`google.cloud.datastore.transaction.Transaction`.
+
+            :type kwargs: dict


dhermes

LGTM

chemelnucfin · 2017-12-07T03:38:35Z

@tseaver any comments?

chemelnucfin requested review from dhermes and lukesneeringer as code owners November 8, 2017 06:45

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 8, 2017

chemelnucfin mentioned this pull request Nov 8, 2017

Add support for 'TransactionOptions' #4335

Closed

chemelnucfin added the api: datastore Issues related to the Datastore API. label Nov 8, 2017

chemelnucfin force-pushed the issue4278-transactionoption2 branch 3 times, most recently from cee00af to 1b88972 Compare November 8, 2017 22:46

dhermes reviewed Nov 9, 2017

View reviewed changes

chemelnucfin force-pushed the issue4278-transactionoption2 branch 8 times, most recently from 0ecd530 to c92036a Compare November 29, 2017 07:31

dhermes reviewed Nov 29, 2017

View reviewed changes

chemelnucfin force-pushed the issue4278-transactionoption2 branch from c92036a to 8128847 Compare November 29, 2017 19:40

chemelnucfin force-pushed the issue4278-transactionoption2 branch 3 times, most recently from 008eea6 to 619b42c Compare December 4, 2017 19:58

dhermes reviewed Dec 5, 2017

View reviewed changes

datastore/tests/unit/test_client.py Outdated

client = self._make_one(credentials=creds)

xact = client.transaction(read_only=True)

self.assertIsInstance(xact._options.read_only,

TransactionOptions.ReadOnly)

This comment was marked as spam.

Sign in to view

dhermes reviewed Dec 6, 2017

View reviewed changes

datastore/tests/unit/test_client.py

self.assertIsInstance(xact._options.read_only,

TransactionOptions.ReadOnly)

self.assertEqual(xact._options.read_only,

TransactionOptions.ReadOnly())

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

dhermes reviewed Dec 6, 2017

View reviewed changes

datastore/google/cloud/datastore/client.py Outdated

def transaction(self, **kwargs):

"""Proxy to :class:`google.cloud.datastore.transaction.Transaction`.

:type kwargs: dict

This comment was marked as spam.

Sign in to view

dhermes approved these changes Dec 6, 2017

View reviewed changes

chemelnucfin force-pushed the issue4278-transactionoption2 branch from ee82c35 to a7e2b2b Compare December 6, 2017 19:06

Closes googleapis#4278 Datastore: Transaction Options

7f1155e

chemelnucfin force-pushed the issue4278-transactionoption2 branch from a7e2b2b to 7f1155e Compare December 6, 2017 19:11

chemelnucfin merged commit f4bdb33 into googleapis:master Dec 11, 2017

chemelnucfin deleted the issue4278-transactionoption2 branch December 11, 2017 17:51

parthea pushed a commit that referenced this pull request Nov 24, 2025

Closes #4278 Datastore: Transaction Options (#4357)

e144d1d



		def _assert_num_mutations(test_case, mutation_pb_list, num_mutations):
		test_case.assertEqual(len(mutation_pb_list), num_mutations)

		self.assertEqual(mutated_entity.key, entity.key.to_protobuf())


		def _mutated_pb(test_case, mutation_pb_list, mutation_type):

Conversation

chemelnucfin commented Nov 8, 2017 • edited by dhermes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chemelnucfin commented Nov 8, 2017

Uh oh!

dhermes left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

chemelnucfin commented Nov 28, 2017 via email

Uh oh!

dhermes commented Nov 28, 2017

Uh oh!

chemelnucfin commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

chemelnucfin commented Nov 8, 2017 •

edited by dhermes

Loading

chemelnucfin commented Nov 29, 2017 •

edited

Loading