Fix unable to start on windows with h2 (#39739)

src/metabase/db/data_source.clj

@@ -17,7 +17,10 @@
 
 (set! *warn-on-reflection* true)
 
-(p/deftype+ DataSource [^String url ^Properties properties]
+;; NOTE: Never instantiate a DataSource directly
+;; Use one of our helper functions below to ensure [[update-h2/update-if-needed!]] is called
+;; You can use [[raw-connection-string->DataSource]] or [[broken-out-details->DataSource]]
+(p/deftype+ ^:private DataSource [^String url ^Properties properties]
   pretty/PrettyPrintable
   (pretty [_]
     ;; in dev we can actually print out the details, it's useful in debugging. Everywhere else we should obscure them
@@ -28,10 +31,9 @@
 
   javax.sql.DataSource
   (getConnection [_]
-    (update-h2/update-if-needed! url)
-    (if properties
-      (DriverManager/getConnection url properties)
-      (DriverManager/getConnection url)))
+   (if properties
+     (DriverManager/getConnection url properties)
+     (DriverManager/getConnection url)))
 
   ;; we don't use (.getConnection this url user password) so we don't need to implement it.
   (getConnection [_ _user _password]
@@ -85,6 +87,7 @@
          m     (cond-> m
                  (seq username) (assoc :user username)
                  (seq password) (assoc :password password))]
+     (update-h2/update-if-needed! s)
      (->DataSource s (some-> (not-empty m) connection-pool/map->properties)))))
 
 (defn broken-out-details->DataSource
@@ -97,4 +100,6 @@
         url                                     (format "jdbc:%s:%s" subprotocol subname)
         properties                              (some-> (not-empty (dissoc spec :classname :subprotocol :subname))
                                                         connection-pool/map->properties)]
+
+    (update-h2/update-if-needed! url)
     (->DataSource url properties)))

src/metabase/db/update_h2.clj

@@ -35,7 +35,7 @@
   [jdbc-url]
   (second (re-matches #"jdbc:h2:file:(.*)$" jdbc-url)))
 
-(defn db-version
+(defn- db-version!
   "Returns the H2 major version number of H2 MV database file at path, or nil if no file exists"
   [jdbc-url]
   ;; The H2 database version is indicated in the "format:" key of the MV file header, which is 4096 bytes
@@ -87,15 +87,21 @@
 
 (def ^:private h2-lock (Object.))
 
+(defn- update-needed?
+  [jdbc-url]
+  (= 1 (db-version! jdbc-url)))
+
 (defn update-if-needed!
   "Updates H2 database at db-path from version 1.x to 2.x if jdbc-url points
-   to version 1 H2 database."
+  to version 1 H2 database."
   [jdbc-url]
-  (locking h2-lock
-    (when (= 1 (db-version jdbc-url))
-      (log/info "H2 v1 database detected, updating...")
-      (try
-        (update! jdbc-url)
-        (catch Exception e
-          (log/error "Failed to update H2 database:" e)
-          (throw e))))))
+  (when (and (h2-base-path jdbc-url) (update-needed? jdbc-url))
+    (locking h2-lock
+      ;; the database may have been upgraded while we waited for the lock
+      (when (update-needed? jdbc-url)
+        (log/info "H2 v1 database detected, updating...")
+        (try
+         (update! jdbc-url)
+         (catch Exception e
+           (log/error "Failed to update H2 database:" e)
+           (throw e)))))))

test/metabase/cmd/dump_to_h2_test.clj

@@ -26,26 +26,20 @@
 (deftest dump-deletes-target-db-files-tests
   ;; test fails when the application db is anything but H2 presently
   ;; TODO: make this test work with postgres / mysql / mariadb
-  (mt/with-temp-file [tmp-h2-db "mbtest_dump.h2"]
-    (let [tmp-h2-db-mv  (str tmp-h2-db ".mv.db")
-          file-contents {tmp-h2-db    "Not really an H2 DB"
-                         tmp-h2-db-mv "Not really another H2 DB"}]
+  (mt/with-temp-file [tmp-h2-db "mbtest_dump.h2"
+                      tmp-h2-db-mv "mbtest_dump.h2.mv.db"]
+    (let [h2-file-dump-content "H:2,block:61,blockSize:1000,chunk:7,clean:1,created:18e17379d42,format:2,version:7"
+          file-contents        {tmp-h2-db    h2-file-dump-content
+                                tmp-h2-db-mv h2-file-dump-content}]
       ;; 1. Don't actually run the copy steps themselves
-      (with-redefs [copy/copy! (constantly nil)]
-        (try
-          (doseq [[filename contents] file-contents]
-            (spit filename contents))
-          (dump-to-h2/dump-to-h2! tmp-h2-db)
+      (mt/with-dynamic-redefs [copy/copy! (constantly nil)]
+        (doseq [[filename contents] file-contents]
+          (spit filename contents))
+        (dump-to-h2/dump-to-h2! tmp-h2-db)
 
-          (doseq [filename (keys file-contents)]
-            (testing (str filename " was deleted")
-              (is (false? (.exists (io/file filename))))))
-
-          (finally
-            (doseq [filename (keys file-contents)
-                    :let     [file (io/file filename)]]
-              (when (.exists file)
-                (io/delete-file file)))))))))
+        (doseq [filename (keys file-contents)]
+          (testing (str filename " was deleted")
+            (is (false? (.exists (io/file filename))))))))))
 
 (deftest cmd-dump-to-h2-returns-code-from-dump-test
   (with-redefs [dump-to-h2/dump-to-h2! #(throw (Exception. "err"))