Repository: googleapis/python-spanner

Issue:
spanner: consider exporting Transaction._rolled_back as Transaction.rolled_back
I am currently dealing with a situation where a Transaction might have been rolled back but the exception wasn't directly passed back to me as per
```shell
======================================================================
ERROR: test_concurrent_delete_with_save (basic.tests.ConcurrentSaveTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/google/api_core/grpc_helpers.py", line 57, in error_remapped_callable
    return callable_(*args, **kwargs)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/grpc/_channel.py", line 565, in __call__
    return _end_unary_response_blocking(state, call, False, None)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/grpc/_channel.py", line 467, in _end_unary_response_blocking
    raise _Rendezvous(state, None, None, deadline)
grpc._channel._Rendezvous: <_Rendezvous of RPC that terminated with:
	status = StatusCode.FAILED_PRECONDITION
	details = "Cannot start a read or query within a transaction after Commit() or Rollback() has been called."
	debug_error_string = "{"created":"@1580864794.999511000","description":"Error received from peer ipv6:[2607:f8b0:4007:803::200a]:443","file":"src/core/lib/surface/call.cc","file_line":1046,"grpc_message":"Cannot start a read or query within a transaction after Commit() or Rollback() has been called.","grpc_status":9}"
>

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/emmanuelodeke/Library/Python/3.7/lib/python/site-packages/spanner/dbapi/cursor.py", line 89, in execute
    self.__handle_insert(self.__get_txn(), sql, args or None)
  File "/Users/emmanuelodeke/Library/Python/3.7/lib/python/site-packages/spanner/dbapi/cursor.py", line 139, in __handle_insert
    param_types=param_types,
  File "/Users/emmanuelodeke/Library/Python/3.7/lib/python/site-packages/spanner/dbapi/cursor.py", line 356, in handle_txn_exec_with_retry
    return txn_method(*args, **kwargs)
  File "/Users/emmanuelodeke/Library/Python/3.7/lib/python/site-packages/google/cloud/spanner_v1/transaction.py", line 202, in execute_update
    metadata=metadata,
  File "/Users/emmanuelodeke/Library/Python/3.7/lib/python/site-packages/google/cloud/spanner_v1/gapic/spanner_client.py", line 810, in execute_sql
    request, retry=retry, timeout=timeout, metadata=metadata
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/google/api_core/gapic_v1/method.py", line 143, in __call__
    return wrapped_func(*args, **kwargs)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/google/api_core/retry.py", line 277, in retry_wrapped_func
    on_error=on_error,
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/google/api_core/retry.py", line 182, in retry_target
    return target()
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/google/api_core/timeout.py", line 214, in func_with_timeout
    return func(*args, **kwargs)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/site-packages/google/api_core/grpc_helpers.py", line 59, in error_remapped_callable
    six.raise_from(exceptions.from_grpc_error(exc), exc)
  File "<string>", line 3, in raise_from
google.api_core.exceptions.FailedPrecondition: 400 Cannot start a read or query within a transaction after Commit() or Rollback() has been called.
```

and I see that we exported the attribute `committed` as per
https://github.com/googleapis/python-spanner/blob/386f7380b9cbbfec1d5a626ed2c1652a02c4e506/google/cloud/spanner_v1/transaction.py#L37

but unfortunately not `_rolled_back` https://github.com/googleapis/python-spanner/blob/386f7380b9cbbfec1d5a626ed2c1652a02c4e506/google/cloud/spanner_v1/transaction.py#L39
which I need in order to ensure that I can correctly run commit or rollback on my transactions without a failed precondition error.

Discussion:
HemangChothani: Just another alternative, the changes could be confined to the Transaction class:

    @property
    def rolled_back(self):
        return self._rolled_back

    @rolled_back.setter
    def rolled_back(self, value):
        self._rolled_back = value

Patch:
diff --git a/google/cloud/spanner_v1/pool.py b/google/cloud/spanner_v1/pool.py
index ce7a196..1b23575 100644
--- a/google/cloud/spanner_v1/pool.py
+++ b/google/cloud/spanner_v1/pool.py
@@ -503,7 +503,7 @@ class TransactionPingingPool(PingingPool):
             raise queue.Full
 
         txn = session._transaction
-        if txn is None or txn.committed or txn._rolled_back:
+        if txn is None or txn.committed or txn.rolled_back:
             session.transaction()
             self._pending_sessions.put(session)
         else:
diff --git a/google/cloud/spanner_v1/session.py b/google/cloud/spanner_v1/session.py
index f8e7e88..863053d 100644
--- a/google/cloud/spanner_v1/session.py
+++ b/google/cloud/spanner_v1/session.py
@@ -255,7 +255,7 @@ class Session(object):
             raise ValueError("Session has not been created.")
 
         if self._transaction is not None:
-            self._transaction._rolled_back = True
+            self._transaction.rolled_back = True
             del self._transaction
 
         txn = self._transaction = Transaction(self)
diff --git a/google/cloud/spanner_v1/transaction.py b/google/cloud/spanner_v1/transaction.py
index 29a2e5f..55e2837 100644
--- a/google/cloud/spanner_v1/transaction.py
+++ b/google/cloud/spanner_v1/transaction.py
@@ -36,7 +36,7 @@ class Transaction(_SnapshotBase, _BatchBase):
 
     committed = None
     """Timestamp at which the transaction was successfully committed."""
-    _rolled_back = False
+    rolled_back = False
     _multi_use = True
     _execute_sql_count = 0
 
@@ -58,7 +58,7 @@ class Transaction(_SnapshotBase, _BatchBase):
         if self.committed is not None:
             raise ValueError("Transaction is already committed")
 
-        if self._rolled_back:
+        if self.rolled_back:
             raise ValueError("Transaction is already rolled back")
 
     def _make_txn_selector(self):
@@ -85,7 +85,7 @@ class Transaction(_SnapshotBase, _BatchBase):
         if self.committed is not None:
             raise ValueError("Transaction already committed")
 
-        if self._rolled_back:
+        if self.rolled_back:
             raise ValueError("Transaction is already rolled back")
 
         database = self._session._database
@@ -105,7 +105,7 @@ class Transaction(_SnapshotBase, _BatchBase):
         api = database.spanner_api
         metadata = _metadata_with_prefix(database.name)
         api.rollback(self._session.name, self._transaction_id, metadata=metadata)
-        self._rolled_back = True
+        self.rolled_back = True
         del self._session._transaction
 
     def commit(self):


Test patch:
diff --git a/tests/unit/test_pool.py b/tests/unit/test_pool.py
index 2d4a9d8..b6786a7 100644
--- a/tests/unit/test_pool.py
+++ b/tests/unit/test_pool.py
@@ -837,7 +837,7 @@ def _make_transaction(*args, **kw):
 
     txn = mock.create_autospec(Transaction)(*args, **kw)
     txn.committed = None
-    txn._rolled_back = False
+    txn.rolled_back = False
     return txn
 
 
diff --git a/tests/unit/test_session.py b/tests/unit/test_session.py
index 98d98de..1eff634 100644
--- a/tests/unit/test_session.py
+++ b/tests/unit/test_session.py
@@ -463,7 +463,7 @@ class TestSession(unittest.TestCase):
         another = session.transaction()  # invalidates existing txn
 
         self.assertIs(session._transaction, another)
-        self.assertTrue(existing._rolled_back)
+        self.assertTrue(existing.rolled_back)
 
     def test_run_in_transaction_callback_raises_non_gax_error(self):
         from google.cloud.spanner_v1.proto.transaction_pb2 import (
@@ -506,7 +506,7 @@ class TestSession(unittest.TestCase):
         txn, args, kw = called_with[0]
         self.assertIsInstance(txn, Transaction)
         self.assertIsNone(txn.committed)
-        self.assertTrue(txn._rolled_back)
+        self.assertTrue(txn.rolled_back)
         self.assertEqual(args, ())
         self.assertEqual(kw, {})
 
@@ -561,7 +561,7 @@ class TestSession(unittest.TestCase):
         txn, args, kw = called_with[0]
         self.assertIsInstance(txn, Transaction)
         self.assertIsNone(txn.committed)
-        self.assertFalse(txn._rolled_back)
+        self.assertFalse(txn.rolled_back)
         self.assertEqual(args, ())
         self.assertEqual(kw, {})
 
diff --git a/tests/unit/test_transaction.py b/tests/unit/test_transaction.py
index cceff89..9ef13c2 100644
--- a/tests/unit/test_transaction.py
+++ b/tests/unit/test_transaction.py
@@ -76,7 +76,7 @@ class TestTransaction(unittest.TestCase):
         self.assertIs(transaction._session, session)
         self.assertIsNone(transaction._transaction_id)
         self.assertIsNone(transaction.committed)
-        self.assertFalse(transaction._rolled_back)
+        self.assertFalse(transaction.rolled_back)
         self.assertTrue(transaction._multi_use)
         self.assertEqual(transaction._execute_sql_count, 0)
 
@@ -98,7 +98,7 @@ class TestTransaction(unittest.TestCase):
         session = _Session()
         transaction = self._make_one(session)
         transaction._transaction_id = self.TRANSACTION_ID
-        transaction._rolled_back = True
+        transaction.rolled_back = True
         with self.assertRaises(ValueError):
             transaction._check_state()
 
@@ -125,7 +125,7 @@ class TestTransaction(unittest.TestCase):
     def test_begin_already_rolled_back(self):
         session = _Session()
         transaction = self._make_one(session)
-        transaction._rolled_back = True
+        transaction.rolled_back = True
         with self.assertRaises(ValueError):
             transaction.begin()
 
@@ -187,7 +187,7 @@ class TestTransaction(unittest.TestCase):
         session = _Session()
         transaction = self._make_one(session)
         transaction._transaction_id = self.TRANSACTION_ID
-        transaction._rolled_back = True
+        transaction.rolled_back = True
         with self.assertRaises(ValueError):
             transaction.rollback()
 
@@ -203,7 +203,7 @@ class TestTransaction(unittest.TestCase):
         with self.assertRaises(RuntimeError):
             transaction.rollback()
 
-        self.assertFalse(transaction._rolled_back)
+        self.assertFalse(transaction.rolled_back)
 
     def test_rollback_ok(self):
         from google.protobuf.empty_pb2 import Empty
@@ -218,7 +218,7 @@ class TestTransaction(unittest.TestCase):
 
         transaction.rollback()
 
-        self.assertTrue(transaction._rolled_back)
+        self.assertTrue(transaction.rolled_back)
         self.assertIsNone(session._transaction)
 
         session_id, txn_id, metadata = api._rolled_back
@@ -244,7 +244,7 @@ class TestTransaction(unittest.TestCase):
         session = _Session()
         transaction = self._make_one(session)
         transaction._transaction_id = self.TRANSACTION_ID
-        transaction._rolled_back = True
+        transaction.rolled_back = True
         with self.assertRaises(ValueError):
             transaction.commit()
 
@@ -546,7 +546,7 @@ class TestTransaction(unittest.TestCase):
                 raise Exception("bail out")
 
         self.assertEqual(transaction.committed, None)
-        self.assertTrue(transaction._rolled_back)
+        self.assertTrue(transaction.rolled_back)
         self.assertEqual(len(transaction._mutations), 1)
 
         self.assertEqual(api._committed, None)