Merge pull request #9896 from metabase/add-csv-setting-type

Add CSV setting type; Settings framework improvements

src/metabase/models/setting.clj

@@ -33,8 +33,10 @@
             [clojure
              [core :as core]
              [string :as str]]
+            [clojure.data.csv :as csv]
             [clojure.tools.logging :as log]
             [environ.core :as env]
+            [medley.core :as m]
             [metabase
              [events :as events]
              [util :as u]]
@@ -45,7 +47,8 @@
             [schema.core :as s]
             [toucan
              [db :as db]
-             [models :as models]]))
+             [models :as models]])
+  (:import java.io.StringWriter))
 
 (models/defmodel Setting
   "The model that underlies `defsetting`."
@@ -58,7 +61,7 @@
 
 
 (def ^:private Type
-  (s/enum :string :boolean :json :integer :double :timestamp))
+  (s/enum :string :boolean :json :integer :double :timestamp :csv))
 
 (def ^:private default-tag-for-type
   "Type tag that will be included in the Setting's metadata, so that the getter function will not cause reflection
@@ -121,7 +124,7 @@
 
 (def ^:private ^:dynamic *disable-cache* false)
 
-(defn- db-value
+(defn- db-or-cache-value
   "Get the value, if any, of `setting-or-name` from the DB (using / restoring the cache as needed)."
   ^String [setting-or-name]
   (if *disable-cache*
@@ -141,7 +144,7 @@
    If the fetched value is an empty string it is considered to be unset and this function returns `nil`."
   ^String [setting-or-name]
   (let [setting (resolve-setting setting-or-name)
-        v       (or (db-value setting)
+        v       (or (db-or-cache-value setting)
                     (env-var-value setting)
                     (str (:default setting)))]
     (when (seq v)
@@ -168,14 +171,12 @@
 (defn get-integer
   "Get integer value of (presumably `:integer`) `setting-or-name`. This is the default getter for `:integer` settings."
   ^Integer [setting-or-name]
-  (when-let [s (get-string setting-or-name)]
-    (Integer/parseInt s)))
+  (some-> (get-string setting-or-name) Integer/parseInt))
 
 (defn get-double
   "Get double value of (presumably `:double`) `setting-or-name`. This is the default getter for `:double` settings."
   ^Double [setting-or-name]
-  (when-let [s (get-string setting-or-name)]
-    (Double/parseDouble s)))
+  (some-> (get-string setting-or-name) Double/parseDouble))
 
 (defn get-json
   "Get the string value of `setting-or-name` and parse it as JSON."
@@ -187,13 +188,19 @@
   [setting-or-name]
   (du/->Timestamp (get-string setting-or-name) :no-timezone))
 
+(defn get-csv
+  "Get the string value of `setting-or-name` and parse it as CSV, returning a sequence of exploded strings."
+  [setting-or-name]
+  (some-> (get-string setting-or-name) csv/read-csv first))
+
 (def ^:private default-getter-for-type
   {:string    get-string
    :boolean   get-boolean
    :integer   get-integer
    :json      get-json
    :timestamp get-timestamp
-   :double    get-double})
+   :double    get-double
+   :csv       get-csv})
 
 (defn get
   "Fetch the value of `setting-or-name`. What this means depends on the Setting's `:getter`; by default, this looks for
@@ -258,16 +265,21 @@
         (cache/restore-cache-if-needed!)
         ;; write to DB
         (cond
-          (nil? new-value)                       (db/simple-delete! Setting :key setting-name)
+          (nil? new-value)
+          (db/simple-delete! Setting :key setting-name)
+
           ;; if there's a value in the cache then the row already exists in the DB; update that
-          (contains? (cache/cache) setting-name) (update-setting! setting-name new-value)
+          (contains? (cache/cache) setting-name)
+          (update-setting! setting-name new-value)
+
           ;; if there's nothing in the cache then the row doesn't exist, insert a new one
-          :else                                  (set-new-setting! setting-name new-value))
+          :else
+          (set-new-setting! setting-name new-value))
         ;; update cached value
         (cache/update-cache! setting-name new-value)
-        ;; Record the fact that a Setting has been updated so eventaully other instances (if applicable) find out about it
-        ;; (For Settings that don't use the Cache, don't update the `last-updated` value, because it will cause other
-        ;; instances to do needless reloading of the cache from the DB)
+        ;; Record the fact that a Setting has been updated so eventaully other instances (if applicable) find out
+        ;; about it (For Settings that don't use the Cache, don't update the `last-updated` value, because it will
+        ;; cause other instances to do needless reloading of the cache from the DB)
         (when-not *disable-cache*
           (cache/update-settings-last-updated!))
         ;; Now return the `new-value`.
@@ -308,17 +320,38 @@
   (set-string! setting-or-name (some-> new-value json/generate-string)))
 
 (defn set-timestamp!
-  "Serialize `new-value` for `setting-or-name` as a ISO 8601-encoded timestamp strign and save it."
+  "Serialize `new-value` for `setting-or-name` as a ISO 8601-encoded timestamp string and save it."
   [setting-or-name new-value]
   (set-string! setting-or-name (some-> new-value du/date->iso-8601)))
 
+(defn- serialize-csv [value]
+  (cond
+    ;; if we're passed as string, assume it's already CSV-encoded
+    (string? value)
+    value
+
+    (sequential? value)
+    (let [s (with-open [writer (StringWriter.)]
+              (csv/write-csv writer [value])
+              (str writer))]
+      (first (str/split-lines s)))
+
+    :else
+    value))
+
+(defn set-csv!
+  "Serialize `new-value` for `setting-or-name` as a CSV-encoded string and save it."
+  [setting-or-name new-value]
+  (set-string! setting-or-name (serialize-csv new-value)))
+
 (def ^:private default-setter-for-type
   {:string    set-string!
    :boolean   set-boolean!
    :integer   set-integer!
    :json      set-json!
    :timestamp set-timestamp!
-   :double    set-double!})
+   :double    set-double!
+   :csv       set-csv!})
 
 (defn set!
   "Set the value of `setting-or-name`. What this means depends on the Setting's `:setter`; by default, this just updates
@@ -371,23 +404,25 @@
    ;; indentation below is intentional to make it clearer what shape the generated documentation is going to take.
    ;; Turn on auto-complete-mode in Emacs and see for yourself!
    :tag tag
-   :doc (str/join "\n" [        description
-                                ""
-                        (format "`%s` is a %s Setting. You can get its value by calling:" (setting-name setting) (name setting-type))
-                                ""
-                        (format "    (%s)"                                                (setting-name setting))
-                                ""
-                                "and set its value by calling:"
-                                ""
-                        (format "    (%s <new-value>)"                                    (setting-name setting))
-                                ""
-                        (format "You can also set its value with the env var `%s`."       (env-var-name setting))
-                                ""
-                                "Clear its value by calling:"
-                                ""
-                        (format "    (%s nil)"                                            (setting-name setting))
-                                ""
-                        (format "Its default value is `%s`."                              (if (nil? default) "nil" default))])})
+   :doc (str/join
+         "\n"
+         [        description
+          ""
+          (format "`%s` is a %s Setting. You can get its value by calling:" (setting-name setting) (name setting-type))
+          ""
+          (format "    (%s)"                                                (setting-name setting))
+          ""
+          "and set its value by calling:"
+          ""
+          (format "    (%s <new-value>)"                                    (setting-name setting))
+          ""
+          (format "You can also set its value with the env var `%s`."       (env-var-name setting))
+          ""
+          "Clear its value by calling:"
+          ""
+          (format "    (%s nil)"                                            (setting-name setting))
+          ""
+          (format "Its default value is `%s`."                              (if (nil? default) "nil" default))])})
 
 
 
@@ -397,6 +432,7 @@
   (fn
     ([]
      (get setting))
+
     ([new-value]
      ;; need to qualify this or otherwise the reader gets this confused with the set! used for things like
      ;; (set! *warn-on-reflection* true)
@@ -420,7 +456,8 @@
            (every? valid-trs-or-tru? exprs-without-strs)))))
 
 (defn- validate-description
-  "Validates the description expression `desc-expr`, ensuring it contains an i18n form, or a string consisting of 1 or more i18n forms"
+  "Validates the description expression `desc-expr`, ensuring it contains an i18n form, or a string consisting of 1 or
+  more i18n forms"
   [desc]
   (when-not (or (valid-trs-or-tru? desc)
                 (valid-str-of-trs-or-tru? desc))
@@ -444,19 +481,26 @@
    You may optionally pass any of the OPTIONS below:
 
    *  `:default`    - The default value of the setting. (default: `nil`)
-   *  `:type`       - `:string` (default), `:boolean`, `:integer`, or `:json`. Non-`:string` settings have special
-                      default getters and setters that automatically coerce values to the correct types.
+
+   *  `:type`       - `:string` (default), `:boolean`, `:integer`, `:json`, `:double`, or `:timestamp`. Non-`:string`
+                      Settings have special default getters and setters that automatically coerce values to the correct
+                      types.
+
    *  `:internal?`  - This Setting is for internal use and shouldn't be exposed in the UI (i.e., not returned by the
-                       corresponding endpoints). Default: `false`
+                      corresponding endpoints). Default: `false`
+
    *  `:getter`     - A custom getter fn, which takes no arguments. Overrides the default implementation. (This can in
                       turn call functions in this namespace like `get-string` or `get-boolean` to invoke the default
                       getter behavior.)
+
    *  `:setter`     - A custom setter fn, which takes a single argument. Overrides the default implementation. (This
                       can in turn call functions in this namespace like `set-string!` or `set-boolean!` to invoke the
                       default setter behavior. Keep in mind that the custom setter may be passed `nil`, which should
                       clear the values of the Setting.)
+
    *  `:cache?`     - Should this Setting be cached? (default `true`)? Be careful when disabling this, because it could
                       have a very negative performance impact.
+
    *  `:sensitive?` - Is this a sensitive setting, such as a password, that we should never return in plaintext?
                       (Default: `false`). Obfuscation is not done by getter functions, but instead by functions that
                       ultimately return these values via the API, such as `all` below. (In other words, code in the
@@ -505,12 +549,20 @@
 (defn user-facing-value
   "Get the value of a Setting that should be displayed to a User (i.e. via `/api/setting/` endpoints): for Settings set
   via env vars, or Settings whose value has not been set (i.e., Settings whose value is the same as the default value)
-  no value is displayed; for sensitive Settings, the value is obfuscated."
-  [setting-or-name]
+  no value is displayed; for sensitive Settings, the value is obfuscated.
+
+  Accepts options:
+
+  * `:getter` -- the getter function to use to fetch the Setting value. By default, uses `setting/get`, which will
+    convert the setting to the appropriate type; you can use `get-string` to get all string values of Settings, for
+    example."
+  [setting-or-name & {:keys [getter], :or {getter get}}]
   (let [{:keys [sensitive? default], k :name, :as setting} (resolve-setting setting-or-name)
-        v                                                  (get k)
-        value-is-default?                                  (= v default)
-        value-is-from-env-var?                             (= v (env-var-value setting))]
+        unparsed-value                                     (get-string k)
+        parsed-value                                       (getter k)
+        ;; `default` and `env-var-value` are probably still in serialized form so compare
+        value-is-default?                                  (= unparsed-value default)
+        value-is-from-env-var?                             (= unparsed-value (env-var-value setting))]
     (cond
       ;; TODO - Settings set via an env var aren't returned for security purposes. It is an open question whether we
       ;; should obfuscate them and still show the last two characters like we do for sensitive values that are set via
@@ -519,15 +571,16 @@
       nil
 
       sensitive?
-      (obfuscate-value v)
+      (obfuscate-value parsed-value)
 
       :else
-      v)))
+      parsed-value)))
 
-(defn- user-facing-info [{:keys [sensitive? default description], k :name, :as setting}]
+(defn- user-facing-info
+  [{:keys [sensitive? default description], k :name, :as setting} & {:as options}]
   (let [set-via-env-var? (boolean (env-var-value setting))]
     {:key            k
-     :value          (user-facing-value setting)
+     :value          (m/mapply user-facing-value setting options)
      :is_env_setting set-via-env-var?
      :env_name       (env-var-name setting)
      :description    (str description)
@@ -537,7 +590,9 @@
 
 (defn all
   "Return a sequence of Settings maps in a format suitable for consumption by the frontend.
-   (For security purposes, this doesn't return the value of a setting if it was set via env var)."
-  []
+   (For security purposes, this doesn't return the value of a Setting if it was set via env var).
+
+   `options` are passed to `user-facing-value`."
+  [& {:as options}]
   (for [setting (sort-by :name (vals @registered-settings))]
-    (user-facing-info setting)))
+    (m/mapply user-facing-info setting options)))

test/metabase/api/embed_test.clj

@@ -33,10 +33,13 @@
 
 (defn sign [claims] (jwt/sign claims *secret-key*))
 
+(defn do-with-new-secret-key [f]
+  (binding [*secret-key* (random-embedding-secret-key)]
+    (tu/with-temporary-setting-values [embedding-secret-key *secret-key*]
+      (f))))
+
 (defmacro with-new-secret-key {:style/indent 0} [& body]
-  `(binding [*secret-key* (random-embedding-secret-key)]
-     (tu/with-temporary-setting-values [~'embedding-secret-key *secret-key*]
-       ~@body)))
+  `(do-with-new-secret-key (fn [] ~@body)))
 
 (defn card-token {:style/indent 1} [card-or-id & [additional-token-params]]
   (sign (merge {:resource {:question (u/get-id card-or-id)}

test/metabase/api/setting_test.clj

 (ns metabase.api.setting-test
   (:require [expectations :refer [expect]]
-            [metabase.models.setting-test :refer [set-settings! test-sensitive-setting test-setting-1 test-setting-2]]
+            [metabase.models.setting-test :refer [test-sensitive-setting test-setting-1 test-setting-2]]
             [metabase.test.data.users :refer [user->client]]))
 
 ;; ## Helper Fns
@@ -27,8 +27,10 @@
     :env_name       "MB_TEST_SETTING_2"
     :description    "Test setting - this only shows up in dev (2)"
     :default        "[Default Value]"}]
- (do (set-settings! nil "FANCY")
-     (fetch-test-settings)))
+  (do
+    (test-setting-1 nil)
+    (test-setting-2 "FANCY")
+    (fetch-test-settings)))
 
 ;; Check that non-superusers are denied access
 (expect "You don't have permissions to do that."

test/metabase/models/setting_test.clj

@@ -35,6 +35,17 @@
   :internal? true
   :type :json)
 
+(defsetting ^:private test-csv-setting
+  "Test setting - this only shows up in dev (5)"
+  :internal? true
+  :type :csv)
+
+(defsetting ^:private test-csv-setting-with-default
+  "Test setting - this only shows up in dev (6)"
+  :internal? true
+  :type :csv
+  :default "A,B,C")
+
 ;; ## HELPER FUNCTIONS
 
 (defn db-fetch-setting
@@ -45,25 +56,31 @@
 (defn setting-exists-in-db? [setting-name]
   (boolean (Setting :key (name setting-name))))
 
-(defn set-settings! [setting-1-value setting-2-value]
-  (test-setting-1 setting-1-value)
-  (test-setting-2 setting-2-value))
-
-
 (expect
   String
   (:tag (meta #'test-setting-1)))
 
 ;; ## GETTERS
 ;; Test defsetting getter fn. Should return the value from env var MB_TEST_SETTING_1
-(expect "ABCDEFG"
-  (do (set-settings! nil nil)
-      (test-setting-1)))
+(expect
+  "ABCDEFG"
+  (do
+    (test-setting-1 nil)
+    (test-setting-1)))
 
-;; Test getting a default value
-(expect "[Default Value]"
-  (do (set-settings! nil nil)
-      (test-setting-2)))
+;; Test getting a default value -- if you clear the value of a Setting it should revert to returning the default value
+(expect
+  "[Default Value]"
+  (do
+    (test-setting-2 nil)
+    (test-setting-2)))
+
+;; `user-facing-value` should return `nil` for a Setting that is using the default value
+(expect
+  nil
+  (do
+    (test-setting-2 nil)
+    (setting/user-facing-value :test-setting-2)))
 
 
 ;; ## SETTERS
@@ -186,12 +203,28 @@
    :is_env_setting false
    :env_name       "MB_TEST_SETTING_2"
    :default        "[Default Value]"}
-  (do (set-settings! nil "TOUCANS")
+  (do (test-setting-1 nil)
+      (test-setting-2 "TOUCANS")
       (some (fn [setting]
               (when (re-find #"^test-setting-2$" (name (:key setting)))
                 setting))
             (setting/all))))
 
+;; all with custom getter
+(expect
+  {:key            :test-setting-2
+   :value          7
+   :description    "Test setting - this only shows up in dev (2)"
+   :is_env_setting false
+   :env_name       "MB_TEST_SETTING_2"
+   :default        "[Default Value]"}
+  (do (test-setting-1 nil)
+      (test-setting-2 "TOUCANS")
+      (some (fn [setting]
+              (when (re-find #"^test-setting-2$" (name (:key setting)))
+                setting))
+            (setting/all :getter (comp count setting/get-string)))))
+
 ;; all
 (expect
   [{:key            :test-setting-1
@@ -206,7 +239,8 @@
     :env_name       "MB_TEST_SETTING_2"
     :description    "Test setting - this only shows up in dev (2)"
     :default        "[Default Value]"}]
-  (do (set-settings! nil "S2")
+  (do (test-setting-1 nil)
+      (test-setting-2 "S2")
       (for [setting (setting/all)
             :when   (re-find #"^test-setting-\d$" (name (:key setting)))]
         setting)))
@@ -235,9 +269,12 @@
   {:value nil, :is_env_setting false, :env_name "MB_TEST_BOOLEAN_SETTING", :default nil}
   (user-facing-info-with-db-and-env-var-values :test-boolean-setting nil nil))
 
-;; boolean settings shouldn't be obfuscated when set by env var
+;; values set by env vars should never be shown to the User
 (expect
-  {:value true, :is_env_setting true, :env_name "MB_TEST_BOOLEAN_SETTING", :default "Using value of env var $MB_TEST_BOOLEAN_SETTING"}
+  {:value          nil
+   :is_env_setting true
+   :env_name       "MB_TEST_BOOLEAN_SETTING"
+   :default        "Using value of env var $MB_TEST_BOOLEAN_SETTING"}
   (user-facing-info-with-db-and-env-var-values :test-boolean-setting nil "true"))
 
 ;; env var values should be case-insensitive
@@ -311,6 +348,67 @@
   (:tag (meta #'toucan-name)))
 
 
+;;; -------------------------------------------------- CSV Settings --------------------------------------------------
+
+(defn- fetch-csv-setting-value [v]
+  (with-redefs [setting/get-string (constantly v)]
+    (test-csv-setting)))
+
+;; should be able to fetch a simple CSV setting
+(expect
+  ["A" "B" "C"]
+  (fetch-csv-setting-value "A,B,C"))
+
+;; should also work if there are quoted values that include commas in them
+(expect
+  ["A" "B" "C1,C2" "ddd"]
+  (fetch-csv-setting-value "A,B,\"C1,C2\",ddd"))
+
+(defn- set-and-fetch-csv-setting-value! [v]
+  (test-csv-setting v)
+  {:db-value     (db/select-one-field :value setting/Setting :key "test-csv-setting")
+   :parsed-value (test-csv-setting)})
+
+;; should be able to correctly set a simple CSV setting
+(expect
+  {:db-value "A,B,C", :parsed-value ["A" "B" "C"]}
+  (set-and-fetch-csv-setting-value! ["A" "B" "C"]))
+
+;; should be a able to set a CSV setting with a value that includes commas
+(expect
+  {:db-value "A,B,C,\"D1,D2\"", :parsed-value ["A" "B" "C" "D1,D2"]}
+  (set-and-fetch-csv-setting-value! ["A" "B" "C" "D1,D2"]))
+
+;; should be able to set a CSV setting with a value that includes spaces
+(expect
+  {:db-value "A,B,C, D ", :parsed-value ["A" "B" "C" " D "]}
+  (set-and-fetch-csv-setting-value! ["A" "B" "C" " D "]))
+
+;; should be a able to set a CSV setting when the string is already CSV-encoded
+(expect
+  {:db-value "A,B,C", :parsed-value ["A" "B" "C"]}
+  (set-and-fetch-csv-setting-value! "A,B,C"))
+
+;; should be able to set nil CSV setting
+(expect
+  {:db-value nil, :parsed-value nil}
+  (set-and-fetch-csv-setting-value! nil))
+
+;; default values for CSV settings should work
+(expect
+  ["A" "B" "C"]
+  (do
+    (test-csv-setting-with-default nil)
+    (test-csv-setting-with-default)))
+
+;; `user-facing-value` should be `nil` for CSV Settings with default values
+(expect
+  nil
+  (do
+    (test-csv-setting-with-default nil)
+    (setting/user-facing-value :test-csv-setting-with-default)))
+
+
 ;;; ----------------------------------------------- Encrypted Settings -----------------------------------------------
 
 (defn- actual-value-in-db [setting-key]

test/metabase/test/util.clj

@@ -299,18 +299,23 @@
 
 
 (defn do-with-temporary-setting-value
-  "Temporarily set the value of the `Setting` named by keyword SETTING-K to VALUE and execute F, then re-establish the
-  original value. This works much the same way as `binding`.
+  "Temporarily set the value of the Setting named by keyword `setting-k` to `value` and execute `f`, then re-establish
+  the original value. This works much the same way as `binding`.
 
    Prefer the macro `with-temporary-setting-values` over using this function directly."
   {:style/indent 2}
   [setting-k value f]
-  (let [original-value (setting/get setting-k)]
+  ;; for saving & restoring the original values we're using `get-string` and `set-string!` to bypass the magic getters
+  ;; & setters which might have some restrictions or other unexpected behavior. We're using these functions rather
+  ;; than manipulating values in the DB directly so we don't have to worry about invalidating the Settings cache
+  ;; ourselves
+  (let [original-value     (setting/get-string setting-k)
+        value-was-default? (not (db/select-one setting/Setting :key (u/keyword->qualified-name setting-k)))]
     (try
       (setting/set! setting-k value)
       (f)
       (finally
-        (setting/set! setting-k original-value)))))
+        (setting/set-string! setting-k (when-not value-was-default? original-value))))))
 
 (defmacro with-temporary-setting-values
   "Temporarily bind the values of one or more `Settings`, execute body, and re-establish the original values. This
@@ -350,9 +355,9 @@
   in the DB for 'permanent' rows (rows that live for the life of the test suite, rather than just a single test). For
   example, Database/Table/Field rows related to the test DBs can be temporarily tweaked in this way.
 
-      ;; temporarily make Field 100 a FK to Field 200 and call (do-something)
-      (with-temp-vals-in-db Field 100 {:fk_target_field_id 200, :special_type \"type/FK\"}
-        (do-something))"
+    ;; temporarily make Field 100 a FK to Field 200 and call (do-something)
+    (with-temp-vals-in-db Field 100 {:fk_target_field_id 200, :special_type \"type/FK\"}
+      (do-something))"
   {:style/indent 3}
   [model object-or-id column->temp-value & body]
   `(do-with-temp-vals-in-db ~model ~object-or-id ~column->temp-value (fn [] ~@body)))

test/metabase/test/util_test.clj

 (ns metabase.test.util-test
   "Tests for the test utils!"
-  (:require [expectations :refer :all]
-            [metabase.models.field :refer [Field]]
+  (:require [expectations :refer [expect]]
+            [metabase.models
+             [field :refer [Field]]
+             [setting :as setting]]
             [metabase.test
              [data :as data]
              [util :as tu]]
@@ -23,3 +25,24 @@
       (tu/with-temp-vals-in-db Field (data/id :venues :price) {:position -1}
         (throw (Exception.))))
     (db/select-one-field :position Field :id (data/id :venues :price))))
+
+
+(setting/defsetting test-util-test-setting
+  "Another internal test setting"
+  :internal? true
+  :default "A,B,C"
+  :type :csv)
+
+;; `with-temporary-setting-values` should do its thing
+(expect
+  ["D" "E" "F"]
+  (tu/with-temporary-setting-values [test-util-test-setting ["D" "E" "F"]]
+    (test-util-test-setting)))
+
+;; `with-temporary-setting-values` shouldn't stomp over default values
+(expect
+  ["A" "B" "C"]
+  (do
+    (tu/with-temporary-setting-values [test-util-test-setting ["D" "E" "F"]]
+      (test-util-test-setting))
+    (test-util-test-setting)))