Improve `defsetting` validation and other misc small tweaks (#17896)

* Move email->domain and email-in-domain? util functions to metabase.util

* Improve the error handling for with-temp a little more.

* Extra error context for with-temporary-setting-values

* Improve Setting docstring forms validation: handle aliased forms like `i18n/deferred-tru`

* Fix missing :require

src/metabase/models/setting.clj

@@ -615,15 +615,30 @@
      ;; :refer-clojure :exclude doesn't seem to work in this case
      (metabase.models.setting/set! setting new-value))))
 
-(defn- is-expression? [symbols expression]
-  (when (list? expression)
-    ((set symbols) (first expression))))
+;; The next few functions are for validating the Setting description (i.e., docstring) at macroexpansion time. They
+;; check that the docstring is a valid deferred i18n form (e.g. [[metabase.util.i18n/deferred-tru]]) so the Setting
+;; description will be localized properly when it shows up in the FE admin interface.
+
+(def ^:private allowed-deferred-i18n-forms
+  #{`deferred-trs `deferred-tru})
+
+(defn- is-form?
+  "Whether `form` is a function call/macro call form starting with a symbol in `symbols`.
+
+    (is-form? #{`deferred-tru} `(deferred-tru \"wow\")) ; -> true"
+  [symbols form]
+  (when (and (list? form)
+             (symbol? (first form)))
+    ;; resolve the symbol to a var and convert back to a symbol so we can get the actual name rather than whatever
+    ;; alias the current namespace happens to be using
+    (let [symb (symbol (resolve (first form)))]
+      ((set symbols) symb))))
 
 (defn- valid-trs-or-tru? [desc]
-  (is-expression? #{'deferred-trs 'deferred-tru `deferred-trs `deferred-tru} desc))
+  (is-form? allowed-deferred-i18n-forms desc))
 
 (defn- valid-str-of-trs-or-tru? [maybe-str-expr]
-  (when (is-expression? #{'str `str} maybe-str-expr)
+  (when (is-form? #{`str} maybe-str-expr)
     ;; When there are several i18n'd sentences, there will probably be a surrounding `str` invocation and a space in
     ;; between the sentences, remove those to validate the i18n clauses
     (let [exprs-without-strs (remove (every-pred string? str/blank?) (rest maybe-str-expr))]
@@ -632,15 +647,19 @@
            (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"
-  [desc]
-  (when-not (or (valid-trs-or-tru? desc)
-                (valid-str-of-trs-or-tru? desc))
-    (throw (IllegalArgumentException.
-            (trs "defsetting descriptions strings must have `:visibilty` `:internal`, `:setter` `:none`, or internationalized, found: `{0}`"
-                 (pr-str desc)))))
-  desc)
+  "Check that `description-form` is a i18n form (e.g. [[metabase.util.i18n/deferred-tru]]), or a [[str]] form consisting
+  of one or more deferred i18n forms. Returns `description-form` as-is."
+  [description-form]
+  (when-not (or (valid-trs-or-tru? description-form)
+                (valid-str-of-trs-or-tru? description-form))
+    ;; this doesn't need to be i18n'ed because it's a compile-time error.
+    (throw (ex-info (str "defsetting docstrings must be an *deferred* i18n form unless the Setting has"
+                         " `:visibilty` `:internal` or `:setter` `:none`."
+                         (format " Got: ^%s %s"
+                                 (some-> description-form class (.getCanonicalName))
+                                 (pr-str description-form)))
+                    {:description-form description-form})))
+  description-form)
 
 (defmacro defsetting
   "Defines a new Setting that will be added to the DB at some point in the future.
@@ -659,9 +678,8 @@
    *  `:default`    - The default value of the setting. This must be of the same type as the Setting type, e.g. the
                       default for an `:integer` setting must be some sort of integer. (default: `nil`)
 
-   *  `: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.
+   *  `:type`       - `:string` (default) or one of the other types listed in [[Type]]. Non-`:string` Settings have
+                       special default getters and setters that automatically coerce values to the correct types.
 
    *  `:visibility` - `:public`, `:authenticated`, `:admin` (default), or `:internal`. Controls where this setting is
                       visible

test/metabase/test/redefs.clj

@@ -3,13 +3,12 @@
   needed when running)."
   (:require [metabase.plugins.classloader :as classloader]
             [metabase.test-runner.parallel :as test-runner.parallel]
+            [toucan.db :as db]
             [toucan.util.test :as tt]))
 
-;; wrap `do-with-temp` so it initializes the DB before doing the other stuff it usually does
+;; replace [[toucan.util.test/do-with-temp]] so it initializes the DB before doing the other stuff it usually does
 
-(defonce orig-do-with-temp tt/do-with-temp)
-
-(defn do-with-temp [& args]
+(defn do-with-temp [model attributes f]
   (classloader/require 'metabase.test.initialize)
   ((resolve 'metabase.test.initialize/initialize-if-needed!) :db)
   ;; so with-temp-defaults are loaded
@@ -19,19 +18,19 @@
   ;; TODO -- there's not really a reason that we can't use with-temp in parallel tests -- it depends on the test -- so
   ;; once we're a little more comfortable with the current parallel stuff we should remove this restriction.
   (test-runner.parallel/assert-test-is-not-parallel "with-temp")
-  ;; catch any Exceptions thrown by the original call and rethrow them with some extra context to make them a little
-  ;; easier to debug.
-  (try
-    (apply orig-do-with-temp args)
-    (catch Throwable e
-      ;; only wrap the Exception with additional context if it's not already wrapped. We don't need a bunch of nested
-      ;; `do-with-temp` calls that didn't fail wrapping the Exception from the one that did and adding a bunch of
-      ;; noise
-      (if (::with-temp-error (ex-data e))
-        (throw e)
-        (throw (ex-info (str "with-temp error: " (ex-message e))
-                        {:args args, ::with-temp-error true}
-                        e))))))
+  ;; catch any Exceptions thrown when creating the object and rethrow them with some extra context to make them a
+  ;; little easier to debug.
+  (let [temp-object (try
+                      (db/insert! model (merge (tt/with-temp-defaults model)
+                                               attributes))
+                      (catch Throwable e
+                        (throw (ex-info (str "with-temp error: " (ex-message e))
+                                        {:model (name model), :attributes attributes}
+                                        e))))]
+    (try
+      (f temp-object)
+      (finally
+        (db/delete! model :id (:id temp-object))))))
 
 (alter-var-root #'tt/do-with-temp (constantly do-with-temp))
 

test/metabase/test/util.clj

@@ -349,6 +349,11 @@
         (setting/set! setting-k value)
         (testing (colorize/blue (format "\nSetting %s = %s\n" (keyword setting-k) (pr-str value)))
           (thunk))
+        (catch Throwable e
+          (throw (ex-info (str "Error in with-temporary-setting-values: " (ex-message e))
+                          {:setting setting-k
+                           :value   value}
+                          e)))
         (finally
           (setting/set! setting-k original-db-or-cache-value))))))