4.0 remove success flag in transaction usage pattern (#355)

* Updated API docs, removed Transaction.success attribute

* Changed transaction.success to be an internal flag

Using the tx.commit() and tx.rollback() where the success flag was being
used before.

* Changed transaction.close method to be an internal method.

The user should only use tx.commit() or tx.rollback() for explicit
transactions.

* Fixed integration test and added comment about usage.

The integration test uses old pattern for doing a roll back for now.

Added pydocs for the

session.read_transaction

and

session.write_transaction

how the unit of work patterns should be used with these methods.

Author: martin-neotech

docs/source/transactions.rst

@@ -81,33 +81,13 @@ It also gives applications the ability to directly control `commit` and `rollbac
 
     .. automethod:: sync
 
-    .. attribute:: success
-
-        This attribute can be used to determine the outcome of a transaction on closure.
-        Specifically, this will be either a COMMIT or a ROLLBACK.
-        A value can be set for this attribute multiple times in user code before a transaction completes, with only the final value taking effect.
-
-        On closure, the outcome is evaluated according to the following rules:
-
-        ================  ====================  ===========================  ==============  ===============  =================
-        :attr:`.success`  ``__exit__`` cleanly  ``__exit__`` with exception  ``tx.close()``  ``tx.commit()``  ``tx.rollback()``
-        ================  ====================  ===========================  ==============  ===============  =================
-        :const:`None`     COMMIT                ROLLBACK                     ROLLBACK        COMMIT           ROLLBACK
-        :const:`True`     COMMIT                COMMIT [1]_                  COMMIT          COMMIT           ROLLBACK
-        :const:`False`    ROLLBACK              ROLLBACK                     ROLLBACK        COMMIT           ROLLBACK
-        ================  ====================  ===========================  ==============  ===============  =================
-
-        .. [1] While a COMMIT will be attempted in this scenario, it will likely fail if the exception originated from Cypher execution within that transaction.
-
-    .. automethod:: close
-
     .. automethod:: closed
 
     .. automethod:: commit
 
     .. automethod:: rollback
 
-Closing an explicit transaction can either happen automatically at the end of a ``with`` block, using the :attr:`.Transaction.success` attribute to determine success,
+Closing an explicit transaction can either happen automatically at the end of a ``with`` block,
 or can be explicitly controlled through the :meth:`.Transaction.commit` and :meth:`.Transaction.rollback` methods.
 Explicit transactions are most useful for applications that need to distribute Cypher execution across multiple functions for the same transaction.
 

neo4j/work/simple.py

@@ -376,13 +376,13 @@ def _run_transaction(self, access_mode, unit_of_work, *args, **kwargs):
                 try:
                     result = unit_of_work(tx, *args, **kwargs)
                 except Exception:
-                    tx.success = False
+                    tx._success = False
                     raise
                 else:
-                    if tx.success is None:
-                        tx.success = True
+                    if tx._success is None:
+                        tx._success = True
                 finally:
-                    tx.close()
+                    tx._close()
             except (ServiceUnavailable, SessionExpired) as error:
                 errors.append(error)
             except TransientError as error:
@@ -405,17 +405,57 @@ def _run_transaction(self, access_mode, unit_of_work, *args, **kwargs):
             raise ServiceUnavailable("Transaction failed")
 
     def read_transaction(self, unit_of_work, *args, **kwargs):
+        """
+        Execute a unit of work in a managed read transaction.
+        This transaction will automatically be committed unless an exception is thrown during query execution or by the user code.
+
+        Managed transactions should not generally be explicitly committed (via tx.commit()).
+
+        Example:
+
+        def do_cypher(tx, cypher):
+            result = tx.run(cypher)
+            # consume result
+            return 1
+
+        session.read_transaction(do_cypher, "RETURN 1")
+
+        :param unit_of_work: A function that takes a transaction as an argument and do work with the transaction. unit_of_work(tx, *args, **kwargs)
+        :param args: arguments for the unit_of_work function
+        :param kwargs: key word arguments for the unit_of_work function
+        :return: a result as returned by the given unit of work
+        """
         return self._run_transaction(READ_ACCESS, unit_of_work, *args, **kwargs)
 
     def write_transaction(self, unit_of_work, *args, **kwargs):
+        """
+        Execute a unit of work in a managed write transaction.
+        This transaction will automatically be committed unless an exception is thrown during query execution or by the user code.
+
+        Managed transactions should not generally be explicitly committed (via tx.commit()).
+
+        Example:
+
+        def do_cypher(tx, cypher):
+            result = tx.run(cypher)
+            # consume result
+            return 1
+
+        session.write_transaction(do_cypher, "RETURN 1")
+
+        :param unit_of_work: A function that takes a transaction as an argument and do work with the transaction. unit_of_work(tx, *args, **kwargs)
+        :param args: key word arguments for the unit_of_work function
+        :param kwargs: key word arguments for the unit_of_work function
+        :return: a result as returned by the given unit of work
+        """
         return self._run_transaction(WRITE_ACCESS, unit_of_work, *args, **kwargs)
 
 
 class Transaction:
     """ Container for multiple Cypher queries to be executed within
     a single context. Transactions can be used within a :py:const:`with`
-    block where the value of :attr:`.success` will determine whether
-    the transaction is committed or rolled back on :meth:`.Transaction.close`::
+    block where the transaction is committed or rolled back on based on
+    whether or not an exception is raised::
 
         with session.begin_transaction() as tx:
             pass
@@ -426,7 +466,11 @@ class Transaction:
     #: will be rolled back. This attribute can be set in user code
     #: multiple times before a transaction completes, with only the final
     #: value taking effect.
-    success = None
+    #
+    # This became internal with Neo4j 4.0, when the server-side
+    # transaction semantics changed.
+    #
+    _success = None
 
     _closed = False
 
@@ -440,9 +484,9 @@ def __enter__(self):
     def __exit__(self, exc_type, exc_value, traceback):
         if self._closed:
             return
-        if self.success is None:
-            self.success = not bool(exc_type)
-        self.close()
+        if self._success is None:
+            self._success = not bool(exc_type)
+        self._close()
 
     def run(self, statement, parameters=None, **kwparameters):
         """ Run a Cypher statement within the context of this transaction.
@@ -489,41 +533,34 @@ def commit(self):
         """ Mark this transaction as successful and close in order to
         trigger a COMMIT. This is functionally equivalent to::
 
-            tx.success = True
-            tx.close()
-
         :raise TransactionError: if already closed
         """
-        self.success = True
-        self.close()
+        self._success = True
+        self._close()
 
     def rollback(self):
         """ Mark this transaction as unsuccessful and close in order to
         trigger a ROLLBACK. This is functionally equivalent to::
 
-            tx.success = False
-            tx.close()
-
         :raise TransactionError: if already closed
         """
-        self.success = False
-        self.close()
+        self._success = False
+        self._close()
 
-    def close(self):
-        """ Close this transaction, triggering either a COMMIT or a ROLLBACK,
-        depending on the value of :attr:`.success`.
+    def _close(self):
+        """ Close this transaction, triggering either a COMMIT or a ROLLBACK.
 
         :raise TransactionError: if already closed
         """
         self._assert_open()
         try:
             self.sync()
         except Neo4jError:
-            self.success = False
+            self._success = False
             raise
         finally:
             if self.session.has_transaction():
-                if self.success:
+                if self._success:
                     self.session.commit_transaction()
                 else:
                     self.session.rollback_transaction()

tests/integration/conftest.py

@@ -350,7 +350,7 @@ def cypher_eval(bolt_driver):
     def run_and_rollback(tx, cypher, **parameters):
         result = tx.run(cypher, **parameters)
         value = result.single().value()
-        tx.success = False
+        tx._success = False  # This is not a recommended pattern
         return value
 
     def f(cypher, **parameters):

tests/integration/test_bookmarking.py

@@ -58,9 +58,12 @@ def test_bookmark_should_be_none_after_rollback(driver):
     with driver.session(default_access_mode=WRITE_ACCESS) as session:
         with session.begin_transaction() as tx:
             tx.run("CREATE (a)")
+
     assert session.last_bookmark() is not None
+
     with driver.session(default_access_mode=WRITE_ACCESS) as session:
         with session.begin_transaction() as tx:
             tx.run("CREATE (a)")
-            tx.success = False
+            tx.rollback()
+
     assert session.last_bookmark() is None

tests/integration/test_explicit_tx.py

@@ -85,7 +85,7 @@ def test_can_commit_transaction_using_with_block(session):
         tx.run("MATCH (a) WHERE id(a) = $n "
                "SET a.foo = $foo", {"n": node_id, "foo": "bar"})
 
-        tx.success = True
+        tx.commit()
 
     # Check the property value
     result = session.run("MATCH (a) WHERE id(a) = $n "
@@ -106,8 +106,7 @@ def test_can_rollback_transaction_using_with_block(session):
         # Update a property
         tx.run("MATCH (a) WHERE id(a) = $n "
                "SET a.foo = $foo", {"n": node_id, "foo": "bar"})
-
-        tx.success = False
+        tx.rollback()
 
     # Check the property value
     result = session.run("MATCH (a) WHERE id(a) = $n "
@@ -156,13 +155,14 @@ def test_transaction_timeout(driver):
                 tx2.run("MATCH (a:Node) SET a.property = 2").consume()
 
 
-def test_exit_after_explicit_close_should_be_silent(bolt_driver):
-    with bolt_driver.session() as s:
-        with s.begin_transaction() as tx:
-            assert not tx.closed()
-            tx.close()
-            assert tx.closed()
-        assert tx.closed()
+# TODO: Re-enable and test when TC is available again
+# def test_exit_after_explicit_close_should_be_silent(bolt_driver):
+#     with bolt_driver.session() as s:
+#         with s.begin_transaction() as tx:
+#             assert not tx.closed()
+#             tx.close()
+#             assert tx.closed()
+#         assert tx.closed()
 
 
 def test_should_sync_after_commit(session):