From ee0e8d77e207a7cd0bfcb17babf206b26e3cb175 Mon Sep 17 00:00:00 2001
From: Robsdedude <rouven.bauer@neo4j.com>
Date: Fri, 19 Jan 2024 21:52:48 +0100
Subject: [PATCH] Consider transaction rolled back after failure

---
 CHANGELOG.md                                     |  1 +
 neo4j/src/driver/transaction.rs                  | 16 ++++++++++++++--
 testkit_backend/src/testkit_backend/responses.rs |  9 +++------
 .../src/testkit_backend/session_holder.rs        | 13 ++++---------
 4 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 916f309..1d8b2df 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@
  - Add support for Bolt 5.2, which adds notification filtering.
  - Add `Driver::is_encrypted()`.
  - Reduce the number of lifetime generic parameters in `TransactionQueryBuilder` and `TransactionRecordStream`.
+ - Fix `Transaction::rolblack()` failing if a result stream failed before.
 
 ## 0.0.2
  - Update dependencies.  
diff --git a/neo4j/src/driver/transaction.rs b/neo4j/src/driver/transaction.rs
index 7d9f826..d3f23dc 100644
--- a/neo4j/src/driver/transaction.rs
+++ b/neo4j/src/driver/transaction.rs
@@ -86,14 +86,26 @@ impl<'driver, 'tx> Transaction<'driver, 'tx> {
     /// This is the default behavior when the transaction is dropped.
     /// However, when dropping the transaction, potential errors will be swallowed.
     pub fn rollback(self) -> Result<()> {
-        self.drop_result.into_inner()?;
-        self.inner_tx.rollback()
+        match self.drop_result.into_inner() {
+            Ok(_) => self.inner_tx.rollback(),
+            Err(_) => {
+                // Nothing to do here.
+                // The transaction already failed and doesn't need to be rolled back.
+                Ok(())
+            }
+        }
     }
 }
 
 /// A result cursor as returned by [`TransactionQueryBuilder::run()`].
 ///
 /// It implements [`Iterator`] and can be used to iterate over the [`Record`]s.
+///
+/// Before ending the transaction ([`Transaction::commit()`] or [`Transaction::rollback()`]), all
+/// record streams spawned from it must be dropped.
+/// While calling [`drop(stream)`] works fine for this purpose, it will swallow any outstanding
+/// errors.
+/// Therefore, it is recommended to use [`TransactionRecordStream::consume()`] instead.
 #[derive(Debug)]
 pub struct TransactionRecordStream<'driver, 'tx>(
     RecordStream<'driver>,
diff --git a/testkit_backend/src/testkit_backend/responses.rs b/testkit_backend/src/testkit_backend/responses.rs
index e8403d7..eb40fd1 100644
--- a/testkit_backend/src/testkit_backend/responses.rs
+++ b/testkit_backend/src/testkit_backend/responses.rs
@@ -112,12 +112,9 @@ static REGEX_SKIPPED_TESTS: OnceLock<Vec<(&'static Regex, &'static str)>> = Once
 
 fn get_plain_skipped_tests() -> &'static HashMap<&'static str, &'static str> {
     PLAIN_SKIPPED_TESTS.get_or_init(|| {
-        let mut map = HashMap::new();
-        map.insert(
-            "neo4j.test_tx_run.TestTxRun.test_tx_res_fails_whole_tx",
-            "backend (SessionHolder) doesn't keep result buffers around after error",
-        );
-        map
+        HashMap::from([
+            // ("path.to.skipped_test", "reason"),
+        ])
     })
 }
 
diff --git a/testkit_backend/src/testkit_backend/session_holder.rs b/testkit_backend/src/testkit_backend/session_holder.rs
index 417291c..42df3da 100644
--- a/testkit_backend/src/testkit_backend/session_holder.rs
+++ b/testkit_backend/src/testkit_backend/session_holder.rs
@@ -470,15 +470,10 @@ impl SessionHolderRunner {
                                             }
                                             Err(err) => {
                                                 known_transactions.insert(id, TxFailState::Failed);
-                                                // outside the receiver, we'll just reply with an
-                                                // error to the command. So query and parameters
-                                                // don't matter.
-                                                _ = buffered_command.insert(TransactionRun {
-                                                    transaction_id: command.transaction_id,
-                                                    query: String::from(""),
-                                                    params: None,
-                                                }.into());
-                                                break Err(err);
+                                                let msg = TransactionRunResult {
+                                                    result: Err(err.into()),
+                                                };
+                                                tx_res.send(msg.into()).unwrap();
                                             }
                                         }
                                     }