deserialization should happen inside a transaction

metabase · Feb 5, 2024 · 4e482b0 · 4e482b0
1 parent b2b9e63
commit 4e482b0
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 16 deletions.
diff --git a/enterprise/backend/src/metabase_enterprise/serialization/api.clj b/enterprise/backend/src/metabase_enterprise/serialization/api.clj
@@ -18,9 +18,10 @@
    [metabase.util.log :as log]
    [metabase.util.malli.schema :as ms]
    [metabase.util.random :as u.random]
-   [ring.core.protocols :as ring.protocols])
+   [ring.core.protocols :as ring.protocols]
+   [toucan2.core :as t2])
   (:import
-   (java.io File ByteArrayOutputStream)))
+   (java.io ByteArrayOutputStream File)))
 
 (set! *warn-on-reflection* true)
 
@@ -160,11 +161,12 @@
   [:as {raw-params :params}]
   (api/check-superuser)
   (try
-    (let [{:keys [log-file callback]} (unpack&import (get-in raw-params ["file" :tempfile])
-                                                     (get-in raw-params ["file" :size]))]
-      {:status  200
-       :headers {"Content-Type" "text/plain"}
-       :body    (on-response! log-file callback)})
+    (t2/with-transaction [_tx]
+      (let [{:keys [log-file callback]} (unpack&import (get-in raw-params ["file" :tempfile])
+                                                       (get-in raw-params ["file" :size]))]
+        {:status  200
+         :headers {"Content-Type" "text/plain"}
+         :body    (on-response! log-file callback)}))
     (finally
       (io/delete-file (get-in raw-params ["file" :tempfile])))))
 

diff --git a/enterprise/backend/src/metabase_enterprise/serialization/cmd.clj b/enterprise/backend/src/metabase_enterprise/serialization/cmd.clj
@@ -96,8 +96,9 @@
   ;(when-not (load/compatible? path)
   ;  (log/warn (trs "Dump was produced using a different version of Metabase. Things may break!")))
   (log/info (trs "Loading serialized Metabase files from {0}" path))
-  (serdes/with-cache
-    (v2.load/load-metabase! (v2.ingest/ingest-yaml path) opts)))
+  (t2/with-transaction [_tx]
+    (serdes/with-cache
+      (v2.load/load-metabase! (v2.ingest/ingest-yaml path) opts))))
 
 (mu/defn v2-load!
   "SerDes v2 load entry point.

diff --git a/enterprise/backend/test/metabase_enterprise/serialization/api_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/api_test.clj
@@ -4,6 +4,7 @@
    [clojure.string :as str]
    [clojure.test :refer :all]
    [metabase-enterprise.serialization.api :as api.serialization]
+   [metabase-enterprise.serialization.v2.ingest :as v2.ingest]
    [metabase.models :refer [Card Collection Dashboard]]
    [metabase.test :as mt]
    [metabase.util :as u]
@@ -106,7 +107,7 @@
       (mt/with-premium-features #{:serialization}
         (testing "POST /api/ee/serialization/export"
           (mt/with-temp [Collection coll  {}
-                         Dashboard  _dash {:collection_id (:id coll)}
+                         Dashboard  dash {:collection_id (:id coll)}
                          Card       card  {:collection_id (:id coll)}]
             (let [res    (mt/user-http-request :crowberto :post 200 "ee/serialization/export"
                                                :collection (:id coll) :data_model false :settings false)
@@ -130,14 +131,35 @@
 
                 (let [res (mt/user-http-request :crowberto :post 200 "ee/serialization/import"
                                                 {:request-options {:headers {"content-type" "multipart/form-data"}}}
-                                                {:file ba})]
+                                                {:file ba})
+                      log (io/reader (io/input-stream res))]
                   (testing "We get our data items back"
                     (is (= #{:collection :dashboard :card :database}
-                           (->> (line-seq (io/reader (io/input-stream res)))
-                                log-types))))
-                  (testing "And they hit the db"
-                    (is (= (:name card)
-                           (t2/select-one-fn :name :model/Card :id (:id card))))))))
+                           (->> (line-seq log)
+                                log-types)))))
+                (testing "And they hit the db"
+                  (is (= (:name card)
+                         (t2/select-one-fn :name :model/Card :id (:id card))))))
+
+              (testing "ERROR /api/ee/serialization/import"
+                (t2/update! :model/Card {:id (:id card)} {:name (str "qwe_" (:name card))})
+
+                (let [ingest-file @#'v2.ingest/ingest-file]
+                  (with-redefs [v2.ingest/ingest-file (fn [f]
+                                                        (let [data (ingest-file f)]
+                                                          (= (:entity_id dash) (:entity_id data)
+                                                             (throw (ex-info "Error loading dashboard" {:data data}))
+                                                             data)))]
+                    (let [res (mt/user-http-request :crowberto :post 200 "ee/serialization/import"
+                                                    {:request-options {:headers {"content-type" "multipart/form-data"}}}
+                                                    {:file ba})
+                          log (io/reader (io/input-stream res))]
+                      (is (=? #".*Error loading dashboard.*"
+                             (->> (line-seq log) (drop 2) first))))))
+
+                (testing "Transaction rolled back and no changes are visible"
+                  (is (= (str "qwe_" (:name card))
+                         (t2/select-one-fn :name :model/Card :id (:id card)))))))
 
             (testing "Only admins can export/import"
               (is (= "You don't have permissions to do that."