From 53dd1ec6b5c93dcf9fc2c1aa2297980403a208c0 Mon Sep 17 00:00:00 2001 From: martin bendsoe Date: Fri, 7 Feb 2020 13:21:14 +0100 Subject: [PATCH 1/4] Updated API docs, removed Transaction.success attribute --- docs/source/transactions.rst | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/docs/source/transactions.rst b/docs/source/transactions.rst index 957f80a07..4e98216e8 100644 --- a/docs/source/transactions.rst +++ b/docs/source/transactions.rst @@ -81,24 +81,6 @@ 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 @@ -107,7 +89,7 @@ It also gives applications the ability to directly control `commit` and `rollbac .. 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. From 78469bd8ad15f9cb457e7de8dd92bb51ef12cb4a Mon Sep 17 00:00:00 2001 From: martin bendsoe Date: Fri, 7 Feb 2020 14:30:41 +0100 Subject: [PATCH 2/4] Changed transaction.success to be an internal flag Using the tx.commit() and tx.rollback() where the success flag was being used before. --- neo4j/work/simple.py | 37 ++++++++++++--------------- tests/integration/conftest.py | 2 +- tests/integration/test_bookmarking.py | 5 +++- tests/integration/test_explicit_tx.py | 5 ++-- 4 files changed, 24 insertions(+), 25 deletions(-) diff --git a/neo4j/work/simple.py b/neo4j/work/simple.py index b8c5ad17d..bac180e5b 100644 --- a/neo4j/work/simple.py +++ b/neo4j/work/simple.py @@ -376,11 +376,11 @@ 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() except (ServiceUnavailable, SessionExpired) as error: @@ -414,8 +414,8 @@ def write_transaction(self, 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 +426,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,8 +444,8 @@ 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) + if self._success is None: + self._success = not bool(exc_type) self.close() def run(self, statement, parameters=None, **kwparameters): @@ -489,29 +493,22 @@ 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._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._success = False self.close() def close(self): - """ Close this transaction, triggering either a COMMIT or a ROLLBACK, - depending on the value of :attr:`.success`. + """ Close this transaction, triggering either a COMMIT or a ROLLBACK. :raise TransactionError: if already closed """ @@ -519,11 +516,11 @@ def close(self): 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() diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index f618cbbe9..35adddc73 100644 --- a/tests/integration/conftest.py +++ b/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.rollback() return value def f(cypher, **parameters): diff --git a/tests/integration/test_bookmarking.py b/tests/integration/test_bookmarking.py index 27285f856..eabbed66f 100644 --- a/tests/integration/test_bookmarking.py +++ b/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 diff --git a/tests/integration/test_explicit_tx.py b/tests/integration/test_explicit_tx.py index 03cce1677..0626b0126 100644 --- a/tests/integration/test_explicit_tx.py +++ b/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 " From b2499502a0b666e4b591a518ab4235715cbf53db Mon Sep 17 00:00:00 2001 From: martin bendsoe Date: Fri, 7 Feb 2020 16:22:03 +0100 Subject: [PATCH 3/4] Changed transaction.close method to be an internal method. The user should only use tx.commit() or tx.rollback() for explicit transactions. --- docs/source/transactions.rst | 2 -- neo4j/work/simple.py | 10 +++++----- tests/integration/test_explicit_tx.py | 15 ++++++++------- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/docs/source/transactions.rst b/docs/source/transactions.rst index 4e98216e8..489cba8b2 100644 --- a/docs/source/transactions.rst +++ b/docs/source/transactions.rst @@ -81,8 +81,6 @@ It also gives applications the ability to directly control `commit` and `rollbac .. automethod:: sync - .. automethod:: close - .. automethod:: closed .. automethod:: commit diff --git a/neo4j/work/simple.py b/neo4j/work/simple.py index bac180e5b..be4500174 100644 --- a/neo4j/work/simple.py +++ b/neo4j/work/simple.py @@ -382,7 +382,7 @@ def _run_transaction(self, access_mode, unit_of_work, *args, **kwargs): 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: @@ -446,7 +446,7 @@ def __exit__(self, exc_type, exc_value, traceback): return if self._success is None: self._success = not bool(exc_type) - self.close() + self._close() def run(self, statement, parameters=None, **kwparameters): """ Run a Cypher statement within the context of this transaction. @@ -496,7 +496,7 @@ def commit(self): :raise TransactionError: if already closed """ self._success = True - self.close() + self._close() def rollback(self): """ Mark this transaction as unsuccessful and close in order to @@ -505,9 +505,9 @@ def rollback(self): :raise TransactionError: if already closed """ self._success = False - self.close() + self._close() - def close(self): + def _close(self): """ Close this transaction, triggering either a COMMIT or a ROLLBACK. :raise TransactionError: if already closed diff --git a/tests/integration/test_explicit_tx.py b/tests/integration/test_explicit_tx.py index 0626b0126..5516e1560 100644 --- a/tests/integration/test_explicit_tx.py +++ b/tests/integration/test_explicit_tx.py @@ -155,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): From 855bc039522685a8266ec08103b6e1c70b626ed4 Mon Sep 17 00:00:00 2001 From: martin bendsoe Date: Mon, 10 Feb 2020 12:20:23 +0100 Subject: [PATCH 4/4] 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. --- neo4j/work/simple.py | 40 +++++++++++++++++++++++++++++++++++ tests/integration/conftest.py | 2 +- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/neo4j/work/simple.py b/neo4j/work/simple.py index be4500174..69e4ec433 100644 --- a/neo4j/work/simple.py +++ b/neo4j/work/simple.py @@ -405,9 +405,49 @@ 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) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 35adddc73..5ba095a44 100644 --- a/tests/integration/conftest.py +++ b/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.rollback() + tx._success = False # This is not a recommended pattern return value def f(cypher, **parameters):