From e044858323297a95bcc903c7bc983b9093422fc7 Mon Sep 17 00:00:00 2001
From: Mark Drobnak <mdrobnak@mozilla.com>
Date: Thu, 13 Aug 2020 15:10:00 -0400
Subject: [PATCH] fix: Avoid implicit transactions in DbTransactionPool (#777)

* Avoid implicit transactions in DbTransactionPool

* Always determine if a transaction is read/write by checking HTTP method

Also do some refactoring to simplify the code.

* Remove the begin transaction call from delete_all

Now that it's handled in TransactionDbPool.

Closes #768
---
 src/db/transaction.rs | 35 ++++++++++++++++-------------------
 src/web/handlers.rs   |  5 -----
 2 files changed, 16 insertions(+), 24 deletions(-)

diff --git a/src/db/transaction.rs b/src/db/transaction.rs
index f4b2154da1..e5f9e9eb48 100644
--- a/src/db/transaction.rs
+++ b/src/db/transaction.rs
@@ -21,7 +21,6 @@ use std::future::Future;
 #[derive(Clone)]
 pub struct DbTransactionPool {
     pool: Box<dyn DbPool>,
-    lock_collection: Option<params::LockCollection>,
     is_read: bool,
     tags: Tags,
     user_id: HawkIdentifier,
@@ -48,10 +47,10 @@ impl DbTransactionPool {
         let db2 = db.clone();
 
         // Lock for transaction
-        let result = match (self.lock_collection.clone(), self.is_read) {
+        let result = match (self.get_lock_collection(), self.is_read) {
             (Some(lc), true) => db.lock_for_read(lc).await,
             (Some(lc), false) => db.lock_for_write(lc).await,
-            _ => Ok(()),
+            (None, is_read) => db.begin(!is_read).await,
         };
 
         // Handle lock error
@@ -156,6 +155,16 @@ impl DbTransactionPool {
         };
         Ok(resp)
     }
+
+    /// Create a lock collection if there is a collection to lock
+    fn get_lock_collection(&self) -> Option<params::LockCollection> {
+        self.collection
+            .clone()
+            .map(|collection| params::LockCollection {
+                collection,
+                user_id: self.user_id.clone(),
+            })
+    }
 }
 
 impl FromRequest for DbTransactionPool {
@@ -193,7 +202,7 @@ impl FromRequest for DbTransactionPool {
                 }
             };
             let collection = match col_result {
-                Ok(v) => v,
+                Ok(v) => v.map(|collection| collection.collection),
                 Err(e) => {
                     // Semi-example to show how to use metrics inside of middleware.
                     Metrics::from(state.as_ref()).incr("sync.error.collectionParam");
@@ -212,25 +221,13 @@ impl FromRequest for DbTransactionPool {
             let bso = BsoParam::extrude(req.head(), &mut req.extensions_mut()).ok();
             let bso_opt = bso.map(|b| b.bso);
 
-            let (lc, is_read) = if let Some(collection) = collection {
-                let lc = params::LockCollection {
-                    user_id: user_id.clone(),
-                    collection: collection.collection,
-                };
-                let is_read = match method {
-                    Method::GET | Method::HEAD => true,
-                    _ => false,
-                };
-
-                (Some(lc), is_read)
-            } else {
-                (None, true)
+            let is_read = match method {
+                Method::GET | Method::HEAD => true,
+                _ => false,
             };
-            let collection = lc.as_ref().map(|c| c.collection.clone());
             let precondition = PreConditionHeaderOpt::extrude(&req.headers(), Some(tags.clone()))?;
             let pool = Self {
                 pool: state.db_pool.clone(),
-                lock_collection: lc,
                 is_read,
                 tags,
                 user_id,
diff --git a/src/web/handlers.rs b/src/web/handlers.rs
index 9ea3e87883..6b8194bd3e 100644
--- a/src/web/handlers.rs
+++ b/src/web/handlers.rs
@@ -96,11 +96,6 @@ pub async fn delete_all(
     db_pool
         .transaction_http(|db| async move {
             meta.metrics.incr("request.delete_all");
-            // transaction_http won't implicitly begin a write transaction
-            // for DELETE /storage because it lacks a collection. So it's done
-            // manually here, partly to not further complicate the unit test's
-            // transactions
-            db.begin(true).await?;
             Ok(HttpResponse::Ok().json(db.delete_storage(meta.user_id).await?))
         })
         .await