From 726b659f8b0f080faf98bedcd9e2b627d17a7f6a Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Thu, 1 Dec 2022 13:19:13 -0500
Subject: [PATCH] Extract sample DB to plugins directory on startup (#26828)
* initial implementation
* some refactor and improved error handling
* another refactor
* fix reflection warning
* add test and more refactor
* another test
* misc comment improvements and formatting
* fix tests
* small refactor
* fix function call
* address tamas's comments
---
.clj-kondo/config.edn | 2 +-
src/metabase/plugins.clj | 78 ++++++++++---------
src/metabase/sample_data.clj | 78 +++++++++++++++----
src/metabase/util/files.clj | 6 +-
...database_test.clj => sample_data_test.clj} | 36 ++++++++-
5 files changed, 142 insertions(+), 58 deletions(-)
rename test/metabase/{sample_database_test.clj => sample_data_test.clj} (64%)
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index 853a8d409d6..3edf6e9faac 100644
--- a/.clj-kondo/config.edn
+++ b/.clj-kondo/config.edn
@@ -572,7 +572,7 @@
metabase.query-processor-test.expressions-test/calculate-bird-scarcity hooks.metabase.query-processor-test.expressions-test/calculate-bird-scarcity
metabase.query-processor-test.filter-test/count-with-filter-clause hooks.metabase.test.data/$ids
metabase.query-processor.middleware.cache-test/with-mock-cache hooks.common/with-two-bindings
- metabase.sample-database-test/with-temp-sample-database-db hooks.common/with-one-binding
+ metabase.sample-data-test/with-temp-sample-database-db hooks.common/with-one-binding
metabase.test.data.datasets/test-drivers hooks.common/do*
metabase.test.data.users/with-group hooks.common/let-one-with-optional-value
metabase.test.data/$ids hooks.metabase.test.data/$ids
diff --git a/src/metabase/plugins.clj b/src/metabase/plugins.clj
index 12edbf5404c..abce9123b1d 100644
--- a/src/metabase/plugins.clj
+++ b/src/metabase/plugins.clj
@@ -1,5 +1,6 @@
(ns metabase.plugins
- (:require [clojure.java.classpath :as classpath]
+ (:require [clojure.core.memoize :as memoize]
+ [clojure.java.classpath :as classpath]
[clojure.java.io :as io]
[clojure.string :as str]
[clojure.tools.logging :as log]
@@ -17,41 +18,48 @@
(or (env/env :mb-plugins-dir)
(.getAbsolutePath (io/file "plugins"))))
-;; logic for determining plugins dir -- see below
-(defonce ^:private plugins-dir*
- (delay
- (let [filename (plugins-dir-filename)]
- (try
- ;; attempt to create <current-dir>/plugins if it doesn't already exist. Check that the directory is readable.
- (let [path (u.files/get-path filename)]
- (u.files/create-dir-if-not-exists! path)
- (assert (Files/isWritable path)
- (trs "Metabase does not have permissions to write to plugins directory {0}" filename))
- path)
- ;; If we couldn't create the directory, or the directory is not writable, fall back to a temporary directory
- ;; rather than failing to launch entirely. Log instructions for what should be done to fix the problem.
- (catch Throwable e
- (log/warn
- e
- (trs "Metabase cannot use the plugins directory {0}" filename)
- "\n"
- (trs "Please make sure the directory exists and that Metabase has permission to write to it.")
- (trs "You can change the directory Metabase uses for modules by setting the environment variable MB_PLUGINS_DIR.")
- (trs "Falling back to a temporary directory for now."))
- ;; Check whether the fallback temporary directory is writable. If it's not, there's no way for us to
- ;; gracefully proceed here. Throw an Exception detailing the critical issues.
- (let [path (u.files/get-path (System/getProperty "java.io.tmpdir"))]
- (assert (Files/isWritable path)
- (trs "Metabase cannot write to temporary directory. Please set MB_PLUGINS_DIR to a writable directory and restart Metabase."))
- path))))))
-
-;; Actual logic is wrapped in a delay rather than a normal function so we don't log the error messages more than once
-;; in cases where we have to fall back to the system temporary directory
-(defn- plugins-dir
- "Get a `Path` to the Metabase plugins directory, creating it if needed. If it cannot be created for one reason or
- another, or if we do not have write permissions for it, use a temporary directory instead."
+(def ^:private plugins-dir*
+ ;; Memoized so we don't log the error messages multiple times if the plugins directory doesn't change
+ (memoize/memo
+ (fn [filename]
+ (try
+ ;; attempt to create <current-dir>/plugins if it doesn't already exist. Check that the directory is readable.
+ (let [path (u.files/get-path filename)]
+ (u.files/create-dir-if-not-exists! path)
+ (assert (Files/isWritable path)
+ (trs "Metabase does not have permissions to write to plugins directory {0}" filename))
+ {:path path, :temp false})
+ ;; If we couldn't create the directory, or the directory is not writable, fall back to a temporary directory
+ ;; rather than failing to launch entirely. Log instructions for what should be done to fix the problem.
+ (catch Throwable e
+ (log/warn
+ e
+ (trs "Metabase cannot use the plugins directory {0}" filename)
+ "\n"
+ (trs "Please make sure the directory exists and that Metabase has permission to write to it.")
+ (trs "You can change the directory Metabase uses for modules by setting the environment variable MB_PLUGINS_DIR.")
+ (trs "Falling back to a temporary directory for now."))
+ ;; Check whether the fallback temporary directory is writable. If it's not, there's no way for us to
+ ;; gracefully proceed here. Throw an Exception detailing the critical issues.
+ (let [path (u.files/get-path (System/getProperty "java.io.tmpdir"))]
+ (assert (Files/isWritable path)
+ (trs "Metabase cannot write to temporary directory. Please set MB_PLUGINS_DIR to a writable directory and restart Metabase."))
+ {:path path, :temp true}))))))
+
+(defn plugins-dir-info
+ "Map with a :path key containing the `Path` to the Metabase plugins directory, and a :temp key indicating whether a
+ temporary directory was used."
^Path []
- @plugins-dir*)
+ (plugins-dir* (plugins-dir-filename)))
+
+(defn plugins-dir
+ "Get a `Path` to the Metabase plugins directory, creating it if needed. If it cannot be created for one reason or
+ another, or if we do not have write permissions for it, use a temporary directory instead.
+
+ This is a wrapper around `plugins-dir-info` which also contains a :temp key indicating whether a temporary directory
+ was used."
+ []
+ (:path (plugins-dir-info)))
(defn- extract-system-modules! []
(when (io/resource "modules")
diff --git a/src/metabase/sample_data.clj b/src/metabase/sample_data.clj
index d6d1ebece48..810be1f44e4 100644
--- a/src/metabase/sample_data.clj
+++ b/src/metabase/sample_data.clj
@@ -3,23 +3,63 @@
[clojure.string :as str]
[clojure.tools.logging :as log]
[metabase.models.database :refer [Database]]
+ [metabase.plugins :as plugins]
[metabase.sync :as sync]
+ [metabase.util.files :as u.files]
[metabase.util.i18n :refer [trs]]
- [toucan.db :as db]))
+ [ring.util.codec :as codec]
+ [toucan.db :as db])
+ (:import java.net.URL))
(def ^:private ^String sample-database-name "Sample Database")
(def ^:private ^String sample-database-filename "sample-database.db.mv.db")
-(defn- db-details []
+;; Reuse the plugins directory for the destination to extract the sample database because it's pretty much guaranteed
+;; to exist and be writable.
+(defn- target-path
+ []
+ (u.files/append-to-path (plugins/plugins-dir) sample-database-filename))
+
+(defn- process-sample-db-path
+ [base-path]
+ (-> base-path
+ (str/replace #"\.mv\.db$" "") ; strip the .mv.db suffix from the path
+ codec/url-decode ; for some reason the path can get URL-encoded so we decode it here
+ (str ";USER=GUEST;PASSWORD=guest"))) ; specify the GUEST user account created for the DB
+
+(defn- jar-db-details
+ [^URL resource]
+ (-> (.getPath resource)
+ (str/replace #"^file:" "zip:") ; to connect to an H2 DB inside a JAR just replace file: with zip: (this doesn't
+ ; do anything when running from the Clojure CLI, which has no `file:` prefix)
+ process-sample-db-path))
+
+(defn- extract-sample-database!
+ []
+ (u.files/with-open-path-to-resource [sample-db-path sample-database-filename]
+ (let [dest-path (target-path)]
+ (u.files/copy-file! sample-db-path dest-path)
+ (-> (str "file:" dest-path)
+ process-sample-db-path))))
+
+(defn- try-to-extract-sample-database!
+ "Tries to extract the sample database out of the JAR (for performance) and then returns a db-details map
+ containing a path to the copied database."
+ []
(let [resource (io/resource sample-database-filename)]
(when-not resource
(throw (Exception. (trs "Sample database DB file ''{0}'' cannot be found."
sample-database-filename))))
- {:db (-> (.getPath resource)
- (str/replace #"^file:" "zip:") ; to connect to an H2 DB inside a JAR just replace file: with zip: (this doesn't do anything when running from the Clojure CLI, which has no `file:` prefix)
- (str/replace #"\.mv\.db$" "") ; strip the .mv.db suffix from the path
- (str/replace #"%20" " ") ; for some reason the path can get URL-encoded and replace spaces with `%20`; this breaks things so switch them back to spaces
- (str ";USER=GUEST;PASSWORD=guest"))})) ; specify the GUEST user account created for the DB
+ {:db
+ (if-not (:temp (plugins/plugins-dir-info))
+ (extract-sample-database!)
+ (do
+ ;; If the plugins directory is a temp directory, fall back to reading the DB directly from the JAR until a
+ ;; working plugins directory is available. (We want to ensure the sample DB is in a stable location.)
+ (log/warn (trs (str "Sample database could not be extracted to the plugins directory,"
+ "which may result in slow startup times. "
+ "Please set MB_PLUGINS_DIR to a writable directory and restart Metabase.")))
+ (jar-db-details resource)))}))
(defn add-sample-database!
"Add the sample database as a Metabase DB if it doesn't already exist."
@@ -27,18 +67,22 @@
(when-not (db/exists? Database :is_sample true)
(try
(log/info (trs "Loading sample database"))
- (sync/sync-database! (db/insert! Database
- :name sample-database-name
- :details (db-details)
- :engine :h2
- :is_sample true))
+ (let [details (try-to-extract-sample-database!)]
+ (sync/sync-database! (db/insert! Database
+ :name sample-database-name
+ :details details
+ :engine :h2
+ :is_sample true)))
(catch Throwable e
(log/error e (trs "Failed to load sample database"))))))
(defn update-sample-database-if-needed!
"Update the path to the sample database DB if it exists in case the JAR has moved."
- []
- (when-let [sample-db (db/select-one Database :is_sample true)]
- (let [intended (db-details)]
- (when (not= (:details sample-db) intended)
- (db/update! Database (:id sample-db) :details intended)))))
+ ([]
+ (update-sample-database-if-needed! (db/select-one Database :is_sample true)))
+
+ ([sample-db]
+ (when sample-db
+ (let [intended (try-to-extract-sample-database!)]
+ (when (not= (:details sample-db) intended)
+ (db/update! Database (:id sample-db) :details intended))))))
diff --git a/src/metabase/util/files.clj b/src/metabase/util/files.clj
index 6e1a522e7cf..575ad9b83ca 100644
--- a/src/metabase/util/files.clj
+++ b/src/metabase/util/files.clj
@@ -29,7 +29,9 @@
^Path [& path-components]
(apply get-path-in-filesystem (FileSystems/getDefault) path-components))
-(defn- append-to-path ^Path [^Path path & components]
+(defn append-to-path
+ "Appends string `components` to the end of a Path, returning a new Path."
+ ^Path [^Path path & components]
(loop [^Path path path, [^String component & more] components]
(let [path (.resolve path component)]
(if-not (seq more)
@@ -106,7 +108,7 @@
(defn do-with-open-path-to-resource
"Impl for `with-open-path-to-resource`."
- [^String resource, f]
+ [^String resource f]
(let [url (io/resource resource)]
(when-not url
(throw (FileNotFoundException. (trs "Resource does not exist."))))
diff --git a/test/metabase/sample_database_test.clj b/test/metabase/sample_data_test.clj
similarity index 64%
rename from test/metabase/sample_database_test.clj
rename to test/metabase/sample_data_test.clj
index 573c7735c36..c670333acde 100644
--- a/test/metabase/sample_database_test.clj
+++ b/test/metabase/sample_data_test.clj
@@ -1,11 +1,15 @@
-(ns metabase.sample-database-test
+(ns metabase.sample-data-test
"Tests to make sure the Sample Database syncs the way we would expect."
- (:require [clojure.test :refer :all]
+ (:require [clojure.core.memoize :as memoize]
+ [clojure.string :as str]
+ [clojure.test :refer :all]
[metabase.models :refer [Database Field Table]]
+ [metabase.plugins :as plugins]
[metabase.sample-data :as sample-data]
[metabase.sync :as sync]
[metabase.test :as mt]
[metabase.util :as u]
+ [metabase.util.files :as u.files]
[toucan.db :as db]
[toucan.hydrate :refer [hydrate]]))
@@ -14,7 +18,7 @@
;; These tools are pretty sophisticated for the amount of tests we have!
(defn- sample-database-db []
- {:details (#'sample-data/db-details)
+ {:details (#'sample-data/try-to-extract-sample-database!)
:engine :h2
:name "Sample Database"})
@@ -40,6 +44,32 @@
;;; ----------------------------------------------------- Tests ------------------------------------------------------
+(def ^:private extracted-db-path-regex #"^file:.*plugins/sample-database.db;USER=GUEST;PASSWORD=guest$")
+
+(deftest extract-sample-database-test
+ (testing "The Sample Database is copied out of the JAR into the plugins directory before the DB details are saved."
+ (with-redefs [sync/sync-database! (constantly nil)]
+ (with-temp-sample-database-db [db]
+ (let [db-path (get-in db [:details :db])]
+ (is (re-matches extracted-db-path-regex db-path))))))
+
+ (testing "If the plugins directory is not creatable or writable, we fall back to reading from the DB in the JAR"
+ (memoize/memo-clear! @#'plugins/plugins-dir*)
+ (let [original-var u.files/create-dir-if-not-exists!]
+ (with-redefs [u.files/create-dir-if-not-exists! (fn [_] (throw (Exception.)))]
+ (with-temp-sample-database-db [db]
+ (let [db-path (get-in db [:details :db])]
+ (is (not (str/includes? db-path "plugins"))))
+
+ (testing "If the plugins directory is writable on a subsequent startup, the sample DB is copied"
+ (with-redefs [u.files/create-dir-if-not-exists! original-var]
+ (memoize/memo-clear! @#'plugins/plugins-dir*)
+ (sample-data/update-sample-database-if-needed! db)
+ (let [db-path (get-in (db/select-one Database :id (:id db)) [:details :db])]
+ (is (re-matches extracted-db-path-regex db-path)))))))))
+
+ (memoize/memo-clear! @#'plugins/plugins-dir*))
+
(deftest sync-sample-database-test
(testing (str "Make sure the Sample Database is getting synced correctly. For example PEOPLE.NAME should be "
"has_field_values = search instead of `list`.")
--
GitLab