From 0ceeda2e7427ddb1267686ad15a08b4787fd5b19 Mon Sep 17 00:00:00 2001
From: Jonathan Johnson <jon@khonsulabs.com>
Date: Mon, 9 May 2022 07:53:08 -0700
Subject: [PATCH] Removing TransactionLog::current_transaction_id

In reviewing other code that was calling current_transaction_id, I
realized there was a naming issue with this function. Since it was a
simple pass-through function, rather than just fixing its code, I opted
to keep that code centralized on State.

This makes it a true breaking change for anyone who was using it, but it
forces them to decide whether they want the last written id or the next
id as part of the upgrade, rather than me silently causing
incompatibility issues.
---
 CHANGELOG.md                      |  5 +++++
 nebari/src/transaction/log.rs     | 20 +++++++++++---------
 nebari/src/transaction/manager.rs |  2 +-
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 812007614a..51e037d892 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -33,6 +33,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `StdFile::flush()` is now implemented using
 `std::File::flush()`. To synchronize all data and metadata, use
 `File::synchronize()` instead.
+- `TransactionLog::current_transaction_id` has been removed, and
+  `TransactionLog::state` now returns a reference to the current state. This
+  function previously was returning `State::next_transaction_id()`, which made
+  the name a little weird. To preserve existing behavior, use
+  `log.state().next_transaction_id()`.
 
 ### Fixed
 
diff --git a/nebari/src/transaction/log.rs b/nebari/src/transaction/log.rs
index ecdf3e5732..8780be0041 100644
--- a/nebari/src/transaction/log.rs
+++ b/nebari/src/transaction/log.rs
@@ -150,11 +150,6 @@ impl<File: ManagedFile> TransactionLog<File> {
         self.log.close()
     }
 
-    /// Returns the current transaction id.
-    pub fn current_transaction_id(&self) -> TransactionId {
-        self.state.next_transaction_id()
-    }
-
     /// Begins a new transaction, exclusively locking `trees`.
     pub fn new_transaction<
         'a,
@@ -168,8 +163,8 @@ impl<File: ManagedFile> TransactionLog<File> {
     }
 
     /// Returns the current state of the log.
-    pub fn state(&self) -> State {
-        self.state.clone()
+    pub fn state(&self) -> &State {
+        &self.state
     }
 }
 
@@ -728,6 +723,7 @@ mod tests {
         log_file_tests("any_memory_log_file", AnyFileManager::memory(), None, None);
     }
 
+    #[allow(clippy::too_many_lines)]
     fn log_file_tests<Manager: FileManager>(
         file_name: &str,
         file_manager: Manager,
@@ -756,7 +752,10 @@ mod tests {
             TransactionLog::<Manager::File>::initialize_state(&state, &context).unwrap();
             let mut transactions =
                 TransactionLog::<Manager::File>::open(&log_path, state, context.clone()).unwrap();
-            assert_eq!(transactions.current_transaction_id(), TransactionId(id));
+            assert_eq!(
+                transactions.state().next_transaction_id(),
+                TransactionId(id)
+            );
             let mut tx = transactions.new_transaction([&b"hello"[..]]);
 
             tx.transaction.data = Some(ArcBytes::from(id.to_be_bytes()));
@@ -870,7 +869,10 @@ mod tests {
 
         let mut valid_ids = HashSet::new();
         for id in 1..=10_000 {
-            assert_eq!(transactions.current_transaction_id(), TransactionId(id));
+            assert_eq!(
+                transactions.state().next_transaction_id(),
+                TransactionId(id)
+            );
             let tx = transactions.new_transaction([&b"hello"[..]]);
             if rng.generate::<u8>() < 8 {
                 // skip a few ids.
diff --git a/nebari/src/transaction/manager.rs b/nebari/src/transaction/manager.rs
index 797026eb35..7c46daabe6 100644
--- a/nebari/src/transaction/manager.rs
+++ b/nebari/src/transaction/manager.rs
@@ -207,7 +207,7 @@ impl<Manager: FileManager> ManagerThread<Manager> {
                 return;
             }
         };
-        let transaction_id = log.current_transaction_id();
+        let transaction_id = log.state().next_transaction_id();
         drop(state_sender.send(Ok(state)));
 
         Self {