Extra tests & minor tweaks for settings

src/metabase/models/setting.clj

@@ -55,13 +55,13 @@
   (s/enum :string :boolean :json))
 
 (def ^:private SettingDefinition
-  {:setting-name s/Keyword
-   :description  s/Str            ; used for docstring and is user-facing in the admin panel
-   :default      s/Any            ; this is a string because in the DB all settings are stored as strings; different getters can handle type conversion *from* string
-   :setting-type Type
-   :getter       clojure.lang.IFn
-   :setter       clojure.lang.IFn
-   :internal?    s/Bool})         ; should the API never return this setting? (default: false)
+  {:name        s/Keyword
+   :description s/Str            ; used for docstring and is user-facing in the admin panel
+   :default     s/Any            ; this is a string because in the DB all settings are stored as strings; different getters can handle type conversion *from* string
+   :type        Type
+   :getter      clojure.lang.IFn
+   :setter      clojure.lang.IFn
+   :internal?   s/Bool})         ; should the API never return this setting? (default: false)
 
 
 (defonce ^:private registered-settings
@@ -92,7 +92,7 @@
 ;;; ------------------------------------------------------------ get ------------------------------------------------------------
 
 (defn- setting-name ^String [setting-or-name]
-  (name (:setting-name (resolve-setting setting-or-name))))
+  (name (:name (resolve-setting setting-or-name))))
 
 (defn- env-var-name
   "Get the env var corresponding to SETTING-OR-NAME.
@@ -234,13 +234,13 @@
    This is used internally be `defsetting`; you shouldn't need to use it yourself."
   [{setting-name :name, setting-type :type, default :default, :as setting}]
   (u/prog1 (let [setting-type (s/validate Type (or setting-type :string))]
-             (merge {:setting-name setting-name
-                     :description  nil
-                     :setting-type setting-type
-                     :default      default
-                     :getter       (partial (default-getter-for-type setting-type) setting-name)
-                     :setter       (partial (default-setter-for-type setting-type) setting-name)
-                     :internal?    false}
+             (merge {:name        setting-name
+                     :description nil
+                     :type        setting-type
+                     :default     default
+                     :getter      (partial (default-getter-for-type setting-type) setting-name)
+                     :setter      (partial (default-setter-for-type setting-type) setting-name)
+                     :internal?   false}
                     (dissoc setting :name :type :default)))
     (s/validate SettingDefinition <>)
     (swap! registered-settings assoc setting-name <>)))
@@ -251,7 +251,7 @@
 
 (defn metadata-for-setting-fn
   "Create metadata for the function automatically generated by `defsetting`."
-  [{:keys [default description setting-type], :as setting}]
+  [{:keys [default description], setting-type :type, :as setting}]
   {:arglists '([] [new-value])
    ;; 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!
    :doc (str/join "\n" [        description
@@ -331,16 +331,22 @@
   ;; irrelevant changes (to other settings) are made.
   (events/publish-event :settings-update settings))
 
-(defn all
-  "Return a sequence of Settings maps, including value and description.
-   (For security purposes, this doesn't return the value of a setting if it was set via env var)."
-  []
-  (for [[k setting] (sort-by first @registered-settings)
-        :let        [v (get k)]]
+
+(defn- user-facing-info [setting]
+  (let [k (:name setting)
+        v (get k)]
     {:key         k
-     :value       (when (not= v (env-var-value setting))
+     :value       (when (and (not= v (env-var-value setting))
+                             (not= v (:default setting)))
                     v)
      :description (:description setting)
      :default     (or (when (env-var-value setting)
                         (format "Using $%s" (env-var-name setting)))
                       (:default setting))}))
+
+(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 [setting (sort-by :name (vals @registered-settings))]
+    (user-facing-info setting)))

test/metabase/models/setting_test.clj

@@ -18,6 +18,14 @@
   "Test setting - this only shows up in dev (2)"
   :default "[Default Value]")
 
+(defsetting test-boolean-setting
+  "Test setting - this only shows up in dev (3)"
+  :type :boolean)
+
+(defsetting test-json-setting
+  "Test setting - this only shows up in dev (4)"
+  :type :json)
+
 
 ;; ## HELPER FUNCTIONS
 
@@ -104,7 +112,58 @@
    (setting-exists-in-db? :test-setting-2)])
 
 
-;; ## ALL SETTINGS FUNCTIONS
+;;; ------------------------------------------------------------ all & user-facing-info ------------------------------------------------------------
+
+(resolve-private-fns metabase.models.setting resolve-setting user-facing-info)
+
+;; these tests are to check that settings get returned with the correct information; these functions are what feed into the API
+
+(defn- user-facing-info-with-db-and-env-var-values [setting db-value env-var-value]
+  (do-with-temporary-setting-value setting db-value
+    (fn []
+      (with-redefs [environ.core/env {(keyword (str "mb-" (name setting))) env-var-value}]
+        (dissoc (user-facing-info (resolve-setting setting))
+                :key :description)))))
+
+;; user-facing-info w/ no db value, no env var value, no default value
+(expect
+  {:value nil, :default nil}
+  (user-facing-info-with-db-and-env-var-values :test-setting-1 nil nil))
+
+;; user-facing-info w/ no db value, no env var value, default value
+(expect
+  {:value nil, :default "[Default Value]"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-2 nil nil))
+
+;; user-facing-info w/ no db value, env var value, no default value -- shouldn't leak env var value
+(expect
+  {:value nil, :default "Using $MB_TEST_SETTING_1"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-1 nil "TOUCANS"))
+
+;; user-facing-info w/ no db value, env var value, default value
+(expect
+  {:value nil, :default "Using $MB_TEST_SETTING_2"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-2 nil "TOUCANS"))
+
+;; user-facing-info w/ db value, no env var value, no default value
+(expect
+  {:value "WOW", :default nil}
+  (user-facing-info-with-db-and-env-var-values :test-setting-1 "WOW" nil))
+
+;; user-facing-info w/ db value, no env var value, default value
+(expect
+  {:value "WOW", :default "[Default Value]"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-2 "WOW" nil))
+
+;; user-facing-info w/ db value, env var value, no default value -- the DB value should take precedence over the env var
+(expect
+  {:value "WOW", :default "Using $MB_TEST_SETTING_1"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-1 "WOW" "ENV VAR"))
+
+;; user-facing-info w/ db value, env var value, default value -- env var should take precedence over default value
+(expect
+  {:value "WOW", :default "Using $MB_TEST_SETTING_2"}
+  (user-facing-info-with-db-and-env-var-values :test-setting-2 "WOW" "ENV VAR"))
 
 ;; all
 (expect
@@ -123,3 +182,59 @@
       (for [setting (setting/all)
             :when   (re-find #"^test-setting-\d$" (name (:key setting)))]
         setting)))
+
+
+;;; ------------------------------------------------------------ BOOLEAN SETTINGS ------------------------------------------------------------
+
+(expect
+  {:value nil, :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
+(expect
+  {:value true, :default "Using $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
+(expect
+  (user-facing-info-with-db-and-env-var-values :test-boolean-setting nil "TRUE"))
+
+;; should throw exception if value isn't true / false
+(expect
+  Exception
+  (test-boolean-setting "X"))
+
+(expect
+  Exception
+  (user-facing-info-with-db-and-env-var-values :test-boolean-setting nil "X"))
+
+;; should be able to set value with a string...
+(expect
+  "false"
+  (test-boolean-setting "FALSE"))
+
+(expect
+  false
+  (do (test-boolean-setting "FALSE")
+      (test-boolean-setting)))
+
+;; ... or a boolean
+(expect
+  "false"
+  (test-boolean-setting false))
+
+(expect
+  false
+  (do (test-boolean-setting false)
+      (test-boolean-setting)))
+
+;;; ------------------------------------------------------------ JSON SETTINGS ------------------------------------------------------------
+
+(expect
+  "{\"a\":100,\"b\":200}"
+  (test-json-setting {:a 100, :b 200}))
+
+(expect
+  {:a 100, :b 200}
+  (do (test-json-setting {:a 100, :b 200})
+      (test-json-setting)))

test/metabase/test/util.clj

@@ -329,6 +329,7 @@
    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)]
     (try