Merge pull request #151 from jgallagher/transaction-lifetimes

Fix transaction lifetimes and nested savepoints

Changelog.md

@@ -1,5 +1,10 @@
 # Version UPCOMING (...)
 
+* BREAKING CHANGE: Creating transactions from a `Connection` or savepoints from a `Transaction`
+  now take `&mut self` instead of `&self` to correctly represent that transactions within a
+  connection are inherently nested. While a transaction is alive, the parent connection or
+  transaction is unusable, so `Transaction` now implements `Deref<Target=Connection>`, giving
+  access to `Connection`'s methods via the `Transaction` itself.
 * Adds `insert` convenience method to `Statement` which returns the row ID of an inserted row.
 * Adds `exists` convenience method returning whether a query finds one or more rows.
 * Adds support for serializing types from the `serde_json` crate. Requires the `serde_json` feature.

src/lib.rs

@@ -237,7 +237,7 @@ impl Connection {
     /// # Failure
     ///
     /// Will return `Err` if the underlying SQLite call fails.
-    pub fn transaction(&self) -> Result<Transaction> {
+    pub fn transaction(&mut self) -> Result<Transaction> {
         Transaction::new(self, TransactionBehavior::Deferred)
     }
 
@@ -248,7 +248,7 @@ impl Connection {
     /// # Failure
     ///
     /// Will return `Err` if the underlying SQLite call fails.
-    pub fn transaction_with_behavior(&self, behavior: TransactionBehavior) -> Result<Transaction> {
+    pub fn transaction_with_behavior(&mut self, behavior: TransactionBehavior) -> Result<Transaction> {
         Transaction::new(self, behavior)
     }
 

src/transaction.rs

@@ -1,3 +1,5 @@
+use std::borrow::Cow;
+use std::ops::Deref;
 use {Result, Connection};
 
 /// Old name for `TransactionBehavior`. `SqliteTransactionBehavior` is deprecated.
@@ -47,13 +49,13 @@ pub struct Transaction<'conn> {
 
 impl<'conn> Transaction<'conn> {
     /// Begin a new transaction. Cannot be nested; see `savepoint` for nested transactions.
-    pub fn new(conn: &Connection, behavior: TransactionBehavior) -> Result<Transaction> {
+    pub fn new(conn: &mut Connection, behavior: TransactionBehavior) -> Result<Transaction> {
         let query = match behavior {
             TransactionBehavior::Deferred => "BEGIN DEFERRED",
             TransactionBehavior::Immediate => "BEGIN IMMEDIATE",
             TransactionBehavior::Exclusive => "BEGIN EXCLUSIVE",
         };
-        conn.execute_batch(query).map(|_| {
+        conn.execute_batch(query).map(move |_| {
             Transaction {
                 conn: conn,
                 depth: 0,
@@ -89,11 +91,12 @@ impl<'conn> Transaction<'conn> {
     ///     tx.commit()
     /// }
     /// ```
-    pub fn savepoint(&self) -> Result<Transaction> {
+    pub fn savepoint(&mut self) -> Result<Transaction> {
-        self.conn.execute_batch("SAVEPOINT sp").map(|_| {
+        let new_depth = self.depth + 1;
+        self.conn.execute_batch(&format!("SAVEPOINT sp{}", new_depth)).map(|_| {
             Transaction {
                 conn: self.conn,
-                depth: self.depth + 1,
+                depth: new_depth,
                 commit: false,
                 finished: false,
             }
@@ -127,11 +130,12 @@ impl<'conn> Transaction<'conn> {
 
     fn commit_(&mut self) -> Result<()> {
         self.finished = true;
-        self.conn.execute_batch(if self.depth == 0 {
+        let sql = if self.depth == 0 {
-            "COMMIT"
+            Cow::Borrowed("COMMIT")
         } else {
-            "RELEASE sp"
+            Cow::Owned(format!("RELEASE sp{}", self.depth))
-        })
+        };
+        self.conn.execute_batch(&sql)
     }
 
     /// A convenience method which consumes and rolls back a transaction.
@@ -141,11 +145,12 @@ impl<'conn> Transaction<'conn> {
 
     fn rollback_(&mut self) -> Result<()> {
         self.finished = true;
-        self.conn.execute_batch(if self.depth == 0 {
+        let sql = if self.depth == 0 {
-            "ROLLBACK"
+            Cow::Borrowed("ROLLBACK")
         } else {
-            "ROLLBACK TO sp"
+            Cow::Owned(format!("ROLLBACK TO sp{}", self.depth))
-        })
+        };
+        self.conn.execute_batch(&sql)
     }
 
     /// Consumes the transaction, committing or rolling back according to the current setting
@@ -166,6 +171,14 @@ impl<'conn> Transaction<'conn> {
     }
 }
 
+impl<'conn> Deref for Transaction<'conn> {
+    type Target = Connection;
+
+    fn deref(&self) -> &Connection {
+        self.conn
+    }
+}
+
 #[allow(unused_must_use)]
 impl<'conn> Drop for Transaction<'conn> {
     fn drop(&mut self) {
@@ -186,69 +199,80 @@ mod test {
 
     #[test]
     fn test_drop() {
-        let db = checked_memory_handle();
+        let mut db = checked_memory_handle();
         {
-            let _tx = db.transaction().unwrap();
+            let tx = db.transaction().unwrap();
-            db.execute_batch("INSERT INTO foo VALUES(1)").unwrap();
+            tx.execute_batch("INSERT INTO foo VALUES(1)").unwrap();
             // default: rollback
         }
         {
             let mut tx = db.transaction().unwrap();
-            db.execute_batch("INSERT INTO foo VALUES(2)").unwrap();
+            tx.execute_batch("INSERT INTO foo VALUES(2)").unwrap();
             tx.set_commit()
         }
         {
-            let _tx = db.transaction().unwrap();
+            let tx = db.transaction().unwrap();
             assert_eq!(2i32,
-                       db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)).unwrap());
+                       tx.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)).unwrap());
         }
     }
 
     #[test]
     fn test_explicit_rollback_commit() {
-        let db = checked_memory_handle();
+        let mut db = checked_memory_handle();
         {
             let tx = db.transaction().unwrap();
-            db.execute_batch("INSERT INTO foo VALUES(1)").unwrap();
+            tx.execute_batch("INSERT INTO foo VALUES(1)").unwrap();
             tx.rollback().unwrap();
         }
         {
             let tx = db.transaction().unwrap();
-            db.execute_batch("INSERT INTO foo VALUES(2)").unwrap();
+            tx.execute_batch("INSERT INTO foo VALUES(2)").unwrap();
             tx.commit().unwrap();
         }
         {
-            let _tx = db.transaction().unwrap();
+            let tx = db.transaction().unwrap();
             assert_eq!(2i32,
-                       db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)).unwrap());
+                       tx.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)).unwrap());
         }
     }
 
     #[test]
     fn test_savepoint() {
-        let db = checked_memory_handle();
+        fn assert_current_sum(x: i32, conn: &Connection) {
+            let i = conn.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)).unwrap();
+            assert_eq!(x, i);
+        }
+
+        let mut db = checked_memory_handle();
         {
             let mut tx = db.transaction().unwrap();
-            db.execute_batch("INSERT INTO foo VALUES(1)").unwrap();
+            tx.execute_batch("INSERT INTO foo VALUES(1)").unwrap();
+            assert_current_sum(1, &tx);
             tx.set_commit();
             {
                 let mut sp1 = tx.savepoint().unwrap();
-                db.execute_batch("INSERT INTO foo VALUES(2)").unwrap();
+                sp1.execute_batch("INSERT INTO foo VALUES(2)").unwrap();
-                sp1.set_commit();
+                assert_current_sum(3, &sp1);
+                // will rollback sp1
                 {
-                    let sp2 = sp1.savepoint().unwrap();
+                    let mut sp2 = sp1.savepoint().unwrap();
-                    db.execute_batch("INSERT INTO foo VALUES(4)").unwrap();
+                    sp2.execute_batch("INSERT INTO foo VALUES(4)").unwrap();
+                    assert_current_sum(7, &sp2);
                     // will rollback sp2
                     {
                         let sp3 = sp2.savepoint().unwrap();
-                        db.execute_batch("INSERT INTO foo VALUES(8)").unwrap();
+                        sp3.execute_batch("INSERT INTO foo VALUES(8)").unwrap();
+                        assert_current_sum(15, &sp3);
                         sp3.commit().unwrap();
                         // committed sp3, but will be erased by sp2 rollback
                     }
-                }
+                    assert_current_sum(15, &sp2);
-            }
+                }
-        }
+                assert_current_sum(3, &sp1);
-        assert_eq!(3i32,
+            }
-                   db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)).unwrap());
+            assert_current_sum(1, &tx);
+        }
+        assert_current_sum(1, &db);
     }
 }