From 92834951e36eee06dd1fc1482b113338943f0d76 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 17 May 2016 19:53:53 -0500 Subject: [PATCH 1/3] Make the creation of transactions and savepoints take &mut self. Transactions in SQLite are nested, but the previous API allowed rusqlite transaction wrappers to be created as "siblings". This resulted in unexpected (and usually wrong) behavior. --- src/lib.rs | 4 ++-- src/transaction.rs | 49 +++++++++++++++++++++++++++------------------- 2 files changed, 31 insertions(+), 22 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 09e68bb..8708869 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -237,7 +237,7 @@ impl Connection { /// # Failure /// /// Will return `Err` if the underlying SQLite call fails. - pub fn transaction(&self) -> Result { + pub fn transaction(&mut self) -> Result { 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 { + pub fn transaction_with_behavior(&mut self, behavior: TransactionBehavior) -> Result { Transaction::new(self, behavior) } diff --git a/src/transaction.rs b/src/transaction.rs index e72add4..333f7a0 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -1,3 +1,4 @@ +use std::ops::Deref; use {Result, Connection}; /// Old name for `TransactionBehavior`. `SqliteTransactionBehavior` is deprecated. @@ -47,13 +48,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 { + pub fn new(conn: &mut Connection, behavior: TransactionBehavior) -> Result { 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,7 +90,7 @@ impl<'conn> Transaction<'conn> { /// tx.commit() /// } /// ``` - pub fn savepoint(&self) -> Result { + pub fn savepoint(&mut self) -> Result { self.conn.execute_batch("SAVEPOINT sp").map(|_| { Transaction { conn: self.conn, @@ -166,6 +167,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,62 +195,62 @@ mod test { #[test] fn test_drop() { - 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(); + let tx = db.transaction().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(); + 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(); 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(); { - let sp2 = sp1.savepoint().unwrap(); - db.execute_batch("INSERT INTO foo VALUES(4)").unwrap(); + let mut sp2 = sp1.savepoint().unwrap(); + sp2.execute_batch("INSERT INTO foo VALUES(4)").unwrap(); // 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(); sp3.commit().unwrap(); // committed sp3, but will be erased by sp2 rollback } From 599bf5acfe969895f08fce05a54c96a4d7a0e187 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 17 May 2016 20:19:44 -0500 Subject: [PATCH 2/3] Add test and fix for nested savepoint rollbacks. This requires giving savepoints distinct names. --- src/transaction.rs | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/src/transaction.rs b/src/transaction.rs index 333f7a0..a30fc30 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::ops::Deref; use {Result, Connection}; @@ -91,10 +92,11 @@ impl<'conn> Transaction<'conn> { /// } /// ``` pub fn savepoint(&mut self) -> Result { - 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, } @@ -128,11 +130,12 @@ impl<'conn> Transaction<'conn> { fn commit_(&mut self) -> Result<()> { self.finished = true; - self.conn.execute_batch(if self.depth == 0 { - "COMMIT" + let sql = if self.depth == 0 { + 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. @@ -142,11 +145,12 @@ impl<'conn> Transaction<'conn> { fn rollback_(&mut self) -> Result<()> { self.finished = true; - self.conn.execute_batch(if self.depth == 0 { - "ROLLBACK" + let sql = if self.depth == 0 { + 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 @@ -235,29 +239,40 @@ mod test { #[test] fn test_savepoint() { + 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(); tx.execute_batch("INSERT INTO foo VALUES(1)").unwrap(); + assert_current_sum(1, &tx); tx.set_commit(); { let mut sp1 = tx.savepoint().unwrap(); sp1.execute_batch("INSERT INTO foo VALUES(2)").unwrap(); - sp1.set_commit(); + assert_current_sum(3, &sp1); + // will rollback sp1 { let mut sp2 = sp1.savepoint().unwrap(); sp2.execute_batch("INSERT INTO foo VALUES(4)").unwrap(); + assert_current_sum(7, &sp2); // will rollback sp2 { let sp3 = sp2.savepoint().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_current_sum(1, &tx); } - assert_eq!(3i32, - db.query_row("SELECT SUM(x) FROM foo", &[], |r| r.get(0)).unwrap()); + assert_current_sum(1, &db); } } From 7fe1848ddd829c5c8b75d907da452e7db93948f4 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 17 May 2016 20:22:34 -0500 Subject: [PATCH 3/3] Add breaking change note about Transactions to Changelog. --- Changelog.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Changelog.md b/Changelog.md index 1a95fdc..282c805 100644 --- a/Changelog.md +++ b/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`, 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.