diff --git a/frontend/test/metabase/scenarios/admin/settings/email.cy.spec.js b/frontend/test/metabase/scenarios/admin/settings/email.cy.spec.js
index 7cea381a1c419f93ea61d1bc70f082202b059fb3..a863aa696611712c6e2613d4efdb8cd73a90b5b4 100644
--- a/frontend/test/metabase/scenarios/admin/settings/email.cy.spec.js
+++ b/frontend/test/metabase/scenarios/admin/settings/email.cy.spec.js
@@ -38,7 +38,7 @@ describe("scenarios > admin > settings > email settings", () => {
});
cy.visit("/admin/settings/email");
cy.findByText("Send test email").click();
- cy.findByText("Sorry, something went wrong. Please try again.");
+ cy.findAllByText("Wrong host or port").should("have.length", 2);
});
it("should send a test email for a valid SMTP configuration", () => {
diff --git a/src/metabase/api/email.clj b/src/metabase/api/email.clj
index 085b1beac53b726510a123207bd5815d3afe72b4..ee6eea735dcf6d22e711b0eb53c83d49e78ed42f 100644
--- a/src/metabase/api/email.clj
+++ b/src/metabase/api/email.clj
@@ -8,6 +8,7 @@
[metabase.api.common :as api]
[metabase.email :as email]
[metabase.models.setting :as setting]
+ [metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.schema :as su]))
@@ -21,20 +22,21 @@
(defn- humanize-error-messages
"Convert raw error message responses from our email functions into our normal api error response structure."
- [{:keys [error message]}]
- (when (not= :SUCCESS error)
- (log/warn "Problem connecting to mail server:" message)
+ [{::email/keys [error]}]
+ (when error
(let [conn-error {:errors {:email-smtp-host "Wrong host or port"
:email-smtp-port "Wrong host or port"}}
creds-error {:errors {:email-smtp-username "Wrong username or password"
- :email-smtp-password "Wrong username or password"}}]
- (condp re-matches message
+ :email-smtp-password "Wrong username or password"}}
+ message (str/join ": " (map ex-message (u/full-exception-chain error)))]
+ (log/warn "Problem connecting to mail server:" message)
+ (condp re-find message
;; bad host = "Unknown SMTP host: foobar"
#"^Unknown SMTP host:.*$"
conn-error
;; host seems valid, but host/port failed connection = "Could not connect to SMTP host: localhost, port: 123"
- #"^Could not connect to SMTP host:.*$"
+ #".*Could(?: not)|(?:n't) connect to (?:SMTP )?host.*"
conn-error
;; seen this show up on mandrill
@@ -51,7 +53,7 @@
;; everything else :(
#".*"
- {:message "Sorry, something went wrong. Please try again."}))))
+ {:message (str "Sorry, something went wrong. Please try again. Error: " message)}))))
(defn- humanize-email-corrections
"Formats warnings when security settings are autocorrected."
@@ -68,20 +70,22 @@
[:as {settings :body}]
{settings su/Map}
(api/check-superuser)
- (let [email-settings (select-keys settings (keys mb-to-smtp-settings))
- smtp-settings (-> (set/rename-keys email-settings mb-to-smtp-settings)
- (assoc :port (some-> (:email-smtp-port settings) Integer/parseInt)))
- response (email/test-smtp-connection smtp-settings)
- tested-settings (merge settings (select-keys response [:port :security]))
- [_ corrections _] (data/diff settings tested-settings)
- properly-named-corrections (set/rename-keys corrections (set/map-invert mb-to-smtp-settings))
- corrected-settings (merge email-settings properly-named-corrections)]
- (if (= :SUCCESS (:error response))
+ (let [settings (-> settings
+ (select-keys (keys mb-to-smtp-settings))
+ (set/rename-keys mb-to-smtp-settings))
+ settings (cond-> settings
+ (string? (:port settings)) (update :port #(Long/parseLong ^String %))
+ (string? (:security settings)) (update :security keyword))
+ response (email/test-smtp-connection settings)]
+ (if-not (::email/error response)
;; test was good, save our settings
- (assoc (setting/set-many! corrected-settings)
- :with-corrections (humanize-email-corrections properly-named-corrections))
+ (assoc (setting/set-many! (set/rename-keys response (set/map-invert mb-to-smtp-settings)))
+ :with-corrections (let [[_ corrections] (data/diff settings response)]
+ (-> corrections
+ (set/rename-keys (set/map-invert mb-to-smtp-settings))
+ humanize-email-corrections)))
;; test failed, return response message
- {:status 500
+ {:status 400
:body (humanize-error-messages response)})))
(api/defendpoint DELETE "/"
@@ -92,7 +96,8 @@
api/generic-204-no-content)
(api/defendpoint POST "/test"
- "Send a test email. You must be a superuser to do this."
+ "Send a test email using the SMTP Settings. You must be a superuser to do this. Returns `{:ok true}` if we were able
+ to send the message successfully, otherwise a standard 400 error response."
[]
(api/check-superuser)
(let [response (email/send-message!
@@ -100,9 +105,9 @@
:recipients [(:email @api/*current-user*)]
:message-type :text
:message "Your Metabase emails are working — hooray!")]
- (if (= :SUCCESS (:error response))
+ (if-not (::email/error response)
{:ok true}
- {:status 500
+ {:status 400
:body (humanize-error-messages response)})))
(api/define-routes)
diff --git a/src/metabase/api/ldap.clj b/src/metabase/api/ldap.clj
index ae45645b85724687072e57a3cf2096b5ec78b1fb..e26175a04d6ea9552278f76e100c4e083d167919 100644
--- a/src/metabase/api/ldap.clj
+++ b/src/metabase/api/ldap.clj
@@ -92,9 +92,8 @@
(api/check-superuser)
(let [ldap-settings (select-keys settings (keys mb-settings->ldap-details))
ldap-details (-> (set/rename-keys ldap-settings mb-settings->ldap-details)
- (assoc :port
- (when (seq (:ldap-port settings))
- (Integer/parseInt (:ldap-port settings)))))
+ (assoc :port (when-let [^String ldap-port (not-empty (:ldap-port settings))]
+ (Long/parseLong ldap-port))))
results (if-not (:ldap-enabled settings)
;; when disabled just respond with a success message
{:status :SUCCESS}
diff --git a/src/metabase/email.clj b/src/metabase/email.clj
index 94c0a043e53f133e63ffc1b12ccead268797d3d1..f60b23a4b7401779ffb68717ab7f1139492e8c5c 100644
--- a/src/metabase/email.clj
+++ b/src/metabase/email.clj
@@ -29,21 +29,22 @@
(deferred-tru "SMTP password.")
:sensitive? true)
-;; TODO - smtp-port should be switched to type :integer
(defsetting email-smtp-port
- (deferred-tru "The port your SMTP server uses for outgoing emails."))
+ (deferred-tru "The port your SMTP server uses for outgoing emails.")
+ :type :integer)
(defsetting email-smtp-security
(deferred-tru "SMTP secure connection protocol. (tls, ssl, starttls, or none)")
- :default (deferred-tru "none")
+ :type :keyword
+ :default :none
:setter (fn [new-value]
(when (some? new-value)
- (assert (contains? #{"tls" "ssl" "none" "starttls"} new-value)))
- (setting/set-string! :email-smtp-security new-value)))
+ (assert (#{:tls :ssl :none :starttls} (keyword new-value))))
+ (setting/set-keyword! :email-smtp-security new-value)))
;; ## PUBLIC INTERFACE
-(def ^{:arglists '([smtp-credentials email-details]), :style/indent 1} send-email!
+(def ^{:arglists '([smtp-credentials email-details])} send-email!
"Internal function used to send messages. Should take 2 args - a map of SMTP credentials, and a map of email details.
Provided so you can swap this out with an \"inbox\" for test purposes."
postal/send-message)
@@ -69,7 +70,7 @@
(-> {:host (email-smtp-host)
:user (email-smtp-username)
:pass (email-smtp-password)
- :port (Integer/parseInt (email-smtp-port))}
+ :port (email-smtp-port)}
(add-ssl-settings (email-smtp-security))))
(def ^:private EmailMessage
@@ -91,8 +92,7 @@
{:style/indent 0}
[{:keys [subject recipients message-type message]} :- EmailMessage]
(when-not (email-smtp-host)
- (let [^String msg (tru "SMTP host is not set.")]
- (throw (Exception. msg))))
+ (throw (Exception. (tru "SMTP host is not set."))))
;; Now send the email
(send-email! (smtp-settings)
{:from (email-from-address)
@@ -104,36 +104,49 @@
:html [{:type "text/html; charset=utf-8"
:content message}])}))
-(defn send-message!
- "Send an email to one or more `recipients`.
- `recipients` is a sequence of email addresses; `message-type` must be either `:text` or `:html` or `:attachments`.
-
- (email/send-message!
- :subject \"[Metabase] Password Reset Request\"
- :recipients [\"cam@metabase.com\"]
- :message-type :text
- :message \"How are you today?\")
+(def ^:private SMTPStatus
+ "Schema for the response returned by various functions in [[metabase.email]]. Response will be a map with the key
+ `:metabase.email/error`, which will either be `nil` (indicating no error) or an instance of [[java.lang.Throwable]]
+ with the error."
+ {::error (s/maybe Throwable)})
- Upon success, this returns the `message` that was just sent. This function will catch and log any exception,
- returning a map with a description of the error"
- {:style/indent 0}
- [& {:keys [subject recipients message-type message] :as msg-args}]
+(defn send-message!
+ "Send an email to one or more `:recipients`. `:recipients` is a sequence of email addresses; `:message-type` must be
+ either `:text` or `:html` or `:attachments`.
+
+ (email/send-message!
+ :subject \"[Metabase] Password Reset Request\"
+ :recipients [\"cam@metabase.com\"]
+ :message-type :text
+ :message \"How are you today?\")
+
+ Upon success, this returns the `:message` that was just sent. (TODO -- confirm this.) This function will catch and
+ log any exception, returning a [[SMTPStatus]]."
+ [& {:as msg-args}]
(try
(send-message-or-throw! msg-args)
(catch Throwable e
(log/warn e (trs "Failed to send email"))
- {:error :ERROR ; Huh?
- :message (.getMessage e)})))
-
-(defn- run-smtp-test
- "tests an SMTP configuration by attempting to connect and authenticate
- if an authenticated method is passed in :security."
- [{:keys [host port user pass sender security] :as details}]
+ {::error e})))
+
+(def ^:private SMTPSettings
+ {:host su/NonBlankString
+ :port su/IntGreaterThanZero
+ ;; TODO -- not sure which of these other ones are actually required or not, and which are optional.
+ (s/optional-key :user) (s/maybe s/Str)
+ (s/optional-key :security) (s/maybe (s/enum :tls :ssl :none :starttls))
+ (s/optional-key :pass) (s/maybe s/Str)
+ (s/optional-key :sender) (s/maybe s/Str)})
+
+(s/defn ^:private test-smtp-settings :- SMTPStatus
+ "Tests an SMTP configuration by attempting to connect and authenticate if an authenticated method is passed
+ in `:security`."
+ [{:keys [host port user pass sender security], :as details} :- SMTPSettings]
{:pre [(string? host)
(integer? port)]}
(try
- (let [ssl? (= security "ssl")
- proto (if ssl? "smtps" "smtp")
+ (let [ssl? (= (keyword security) :ssl)
+ proto (if ssl? "smtps" "smtp")
details (-> details
(assoc :proto proto
:connectiontimeout "1000"
@@ -143,46 +156,55 @@
(.setDebug false))]
(with-open [transport (.getTransport session proto)]
(.connect transport host port user pass)))
- {:error :SUCCESS
- :message nil}
+ {::error nil}
(catch Throwable e
(log/error e (trs "Error testing SMTP connection"))
- {:error :ERROR
- :message (.getMessage e)})))
+ {::error e})))
+
+(def ^:private email-security-order [:tls :starttls :ssl])
-(def ^:private email-security-order ["tls" "starttls" "ssl"])
+(def ^:private retry-delay-ms
+ "Amount of time to wait between retrying SMTP connections with different security options. This delay exists to keep
+ us from getting banned on Outlook.com."
+ 500)
-(defn- guess-smtp-security
+(s/defn ^:private guess-smtp-security :- (s/maybe (s/enum :tls :starttls :ssl))
"Attempts to use each of the security methods in security order with the same set of credentials. This is used only
when the initial connection attempt fails, so it won't overwrite a functioning configuration. If this uses something
- other than the provided method, a warning gets printed on the config page"
- [details]
- (loop [[security-type & more-to-try] email-security-order] ;; make sure this is not lazy, or chunking
- (when security-type ;; can cause some servers to block requests
- (let [test-result (run-smtp-test (assoc details :security security-type))]
- (if (not= :ERROR (:error test-result))
- (assoc test-result :security security-type)
- (do
- (Thread/sleep 500) ;; try not to get banned from outlook.com
- (recur more-to-try)))))))
-
-(defn test-smtp-connection
- "Test the connection to an SMTP server to determine if we can send emails.
-
- Takes in a dictionary of properties such as:
- {:host \"localhost\"
- :port 587
- :user \"bigbird\"
- :pass \"luckyme\"
- :sender \"foo@mycompany.com\"
- :security \"tls\"}"
- [details]
- (let [inital-attempt (run-smtp-test details)
- it-worked? (= :SUCCESS (:error inital-attempt))
- attempted-fix (when-not it-worked?
- (guess-smtp-security details))
- we-fixed-it? (= :SUCCESS (:error attempted-fix))]
- (cond
- it-worked? inital-attempt
- we-fixed-it? attempted-fix
- :else inital-attempt)))
+ other than the provided method, a warning gets printed on the config page.
+
+ If unable to connect with any security method, returns `nil`. Otherwise returns the security method that we were
+ able to connect successfully with."
+ [details :- SMTPSettings]
+ ;; make sure this is not lazy, or chunking can cause some servers to block requests
+ (some
+ (fn [security-type]
+ (if-not (::error (test-smtp-settings (assoc details :security security-type)))
+ security-type
+ (do
+ (Thread/sleep retry-delay-ms) ; Try not to get banned from outlook.com
+ nil)))
+ email-security-order))
+
+(s/defn test-smtp-connection :- (s/conditional
+ ::error SMTPStatus
+ :else SMTPSettings)
+ "Test the connection to an SMTP server to determine if we can send emails. Takes in a dictionary of properties such
+ as:
+
+ {:host \"localhost\"
+ :port 587
+ :user \"bigbird\"
+ :pass \"luckyme\"
+ :sender \"foo@mycompany.com\"
+ :security :tls}
+
+ Attempts to connect with different `:security` options. If able to connect successfully, returns working
+ [[SMTPSettings]]. If unable to connect with any `:security` options, returns an [[SMTPStatus]] with the `::error`."
+ [{:keys [port security], :as details} :- SMTPSettings]
+ (let [initial-attempt (test-smtp-settings details)]
+ (if-not (::error initial-attempt)
+ details
+ (if-let [working-security-type (guess-smtp-security details)]
+ (assoc details :security working-security-type)
+ initial-attempt))))
diff --git a/src/metabase/integrations/ldap.clj b/src/metabase/integrations/ldap.clj
index 332269788c6426233a64f688d09254c132bb341d..897ebd1fb1e96e5d3171369520e964b93dccc616 100644
--- a/src/metabase/integrations/ldap.clj
+++ b/src/metabase/integrations/ldap.clj
@@ -23,15 +23,17 @@
(defsetting ldap-port
(deferred-tru "Server port, usually 389 or 636 if SSL is used.")
- :default "389")
+ :type :integer
+ :default 389)
(defsetting ldap-security
(deferred-tru "Use SSL, TLS or plain text.")
- :default "none"
+ :type :keyword
+ :default :none
:setter (fn [new-value]
- (when-not (nil? new-value)
- (assert (contains? #{"none" "ssl" "starttls"} new-value)))
- (setting/set-string! :ldap-security new-value)))
+ (when (some? new-value)
+ (assert (#{:none :ssl :starttls} (keyword new-value))))
+ (setting/set-keyword! :ldap-security new-value)))
(defsetting ldap-bind-dn
(deferred-tru "The Distinguished Name to bind as (if any), this user will be used to lookup information about other users."))
@@ -99,16 +101,18 @@
(ldap-user-base)))))
(defn- details->ldap-options [{:keys [host port bind-dn password security]}]
- ;; Connecting via IPv6 requires us to use this form for :host, otherwise
- ;; clj-ldap will find the first : and treat it as an IPv4 and port number
- {:host {:address host
- :port (if (string? port)
- (Integer/parseInt port)
- port)}
- :bind-dn bind-dn
- :password password
- :ssl? (= security "ssl")
- :startTLS? (= security "starttls")})
+ (let [security (keyword security)
+ port (if (string? port)
+ (Integer/parseInt port)
+ port)]
+ ;; Connecting via IPv6 requires us to use this form for :host, otherwise
+ ;; clj-ldap will find the first : and treat it as an IPv4 and port number
+ {:host {:address host
+ :port port}
+ :bind-dn bind-dn
+ :password password
+ :ssl? (= security :ssl)
+ :startTLS? (= security :starttls)}))
(defn- settings->ldap-options []
(details->ldap-options {:host (ldap-host)
@@ -134,6 +138,8 @@
`(do-with-ldap-connection (fn [~(vary-meta connection-binding assoc :tag `LDAPConnectionPool)]
~@body)))
+;; TODO -- the usage of `:ERROR` and `:STATUS` like this is weird. Just do something like {::error nil} for success and
+;; {::error exception} for an error
(def ^:private user-base-error {:status :ERROR, :message "User search base does not exist or is unreadable"})
(def ^:private group-base-error {:status :ERROR, :message "Group search base does not exist or is unreadable"})
diff --git a/src/metabase/models/humanization.clj b/src/metabase/models/humanization.clj
index 9a8912ce2af2a33833be399f30679a7254c71c54..836d9829a2435fd7aae488229372cb622295db5c 100644
--- a/src/metabase/models/humanization.clj
+++ b/src/metabase/models/humanization.clj
@@ -13,6 +13,7 @@
[clojure.tools.logging :as log]
[metabase.models.setting :as setting :refer [defsetting]]
[metabase.util.i18n :refer [deferred-tru trs tru]]
+ [schema.core :as s]
[toucan.db :as db]))
(declare humanization-strategy)
@@ -47,7 +48,7 @@
;; simple replaces hyphens and underscores with spaces and capitalizes
(defmethod name->human-readable-name :simple
([s] (name->human-readable-name :simple s))
- ([_, ^String s]
+ ([_ ^String s]
;; explode on hyphens, underscores, and spaces
(when (seq s)
(str/join " " (for [part (str/split s #"[-_\s]+")
@@ -57,14 +58,13 @@
;; actual advanced method has been excised. this one just calls out to simple
(defmethod name->human-readable-name :advanced
([s] (name->human-readable-name :simple s))
- ([_, ^String s] (name->human-readable-name :simple s)))
+ ([_ ^String s] (name->human-readable-name :simple s)))
;; :none is just an identity implementation
(defmethod name->human-readable-name :none
([s] s)
([_ s] s))
-
(defn- re-humanize-names!
"Update all non-custom display names of all instances of `model` (e.g. Table or Field)."
[old-strategy model]
@@ -80,32 +80,34 @@
:display_name new-strategy-display-name))))
(db/select-reducible [model :id :name :display_name])))
-(defn- re-humanize-table-and-field-names!
+(s/defn ^:private re-humanize-table-and-field-names!
"Update the non-custom display names of all Tables & Fields in the database using new values obtained from
the (obstensibly swapped implementation of) `name->human-readable-name`."
- [old-strategy]
+ [old-strategy :- s/Keyword]
(doseq [model ['Table 'Field]]
(re-humanize-names! old-strategy model)))
(defn- set-humanization-strategy! [new-value]
- (let [new-strategy (or new-value "simple")]
+ (let [new-strategy (keyword (or new-value :simple))]
;; check to make sure `new-strategy` is a valid strategy, or throw an Exception it is it not.
- (when-not (get-method name->human-readable-name (keyword new-strategy))
+ (when-not (get-method name->human-readable-name new-strategy)
(throw (IllegalArgumentException.
(tru "Invalid humanization strategy ''{0}''. Valid strategies are: {1}"
new-strategy (keys (methods name->human-readable-name))))))
- (let [old-strategy (setting/get-string :humanization-strategy)]
+ (let [old-strategy (setting/get-keyword :humanization-strategy)]
;; ok, now set the new value
- (setting/set-string! :humanization-strategy (some-> new-value name))
+ (setting/set-keyword! :humanization-strategy new-value)
;; now rehumanize all the Tables and Fields using the new strategy.
;; TODO: Should we do this in a background thread because it is potentially slow?
- (log/info (trs "Changing Table & Field names humanization strategy from ''{0}'' to ''{1}''" old-strategy new-strategy))
+ (log/info (trs "Changing Table & Field names humanization strategy from ''{0}'' to ''{1}''"
+ (name old-strategy) (name new-strategy)))
(re-humanize-table-and-field-names! old-strategy))))
(defsetting ^{:added "0.28.0"} humanization-strategy
(str (deferred-tru "To make table and field names more human-friendly, Metabase will replace dashes and underscores in them with spaces.")
" "
(deferred-tru "We’ll capitalize each word while at it, so ‘last_visited_at’ will become ‘Last Visited At’."))
- :default "simple"
+ :type :keyword
+ :default :simple
:setter set-humanization-strategy!)
diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj
index 49470298104696c2c30f58b8692cdf27a4755e84..a5778664adf54912b32b6b858e84cb721b019f56 100644
--- a/src/metabase/models/setting.clj
+++ b/src/metabase/models/setting.clj
@@ -45,8 +45,9 @@
[schema.core :as s]
[toucan.db :as db]
[toucan.models :as models])
- (:import clojure.lang.Symbol
- java.io.StringWriter))
+ (:import [clojure.lang Keyword Symbol]
+ java.io.StringWriter
+ java.time.temporal.Temporal))
(models/defmodel Setting
"The model that underlies `defsetting`."
@@ -72,8 +73,36 @@
:boolean `Boolean
:integer `Long
:double `Double
- :timestamp 'java.time.temporal.Temporal
- :keyword 'Keyword})
+ :timestamp `Temporal
+ :keyword `Keyword})
+
+(defn- validate-default-value-for-type
+ "Check whether the `:default` value of a Setting (if provided) agrees with the Setting's `:type` and its `:tag` (which
+ usually comes from [[default-tag-for-type]])."
+ [{setting-type :type, setting-name :name, :keys [tag default], :as setting-definition}]
+ ;; the errors below don't need to be i18n'ed since they're definition-time errors rather than user-facing
+ (when (some? tag)
+ (assert ((some-fn symbol? string?) tag) (format "Setting :tag should be a symbol or string, got: ^%s %s"
+ (.getCanonicalName (class tag))
+ (pr-str tag))))
+ (when (and (some? default)
+ (some? tag))
+ (let [klass (if (string? tag)
+ (try
+ (Class/forName tag)
+ (catch Throwable e
+ e))
+ (resolve tag))]
+ (when-not (class? klass)
+ (throw (ex-info (format "Cannot resolve :tag %s to a class. Is it fully qualified?" (pr-str tag))
+ {:tag klass}
+ (when (instance? Throwable klass) klass))))
+ (when-not (instance? klass default)
+ (throw (ex-info (format "Wrong :default type: got ^%s %s, but expected a %s"
+ (.getCanonicalName (class default))
+ (pr-str default)
+ (.getCanonicalName ^Class klass))
+ {:tag klass}))))))
(def ^:private SettingDefinition
{:name s/Keyword
@@ -155,8 +184,7 @@
(str/replace (name setting-nm) #"[^a-zA-Z0-9_-]*" ""))
(defn- env-var-name
- "Get the env var corresponding to `setting-definition-or-name`.
- (This is used primarily for documentation purposes)."
+ "Get the env var corresponding to `setting-definition-or-name`. (This is used primarily for documentation purposes)."
^String [setting-definition-or-name]
(str "MB_" (-> (setting-name setting-definition-or-name)
munge-setting-name
@@ -185,25 +213,37 @@
(cache/restore-cache-if-needed!)
(clojure.core/get (cache/cache) (setting-name setting-definition-or-name)))))
+(defn default-value
+ "Get the `:default` value of `setting-definition-or-name` if one was specified. With optional arg `pred`, only returns
+ default value if `(some-> default pred)` is truthy."
+ ([setting-definition-or-name]
+ (default-value setting-definition-or-name nil))
+ ([setting-definition-or-name pred]
+ (let [{:keys [default]} (resolve-setting setting-definition-or-name)]
+ (if pred
+ (when (some-> default pred)
+ default)
+ default))))
+
(defn get-string
- "Get string value of `setting-definition-or-name`. This is the default getter for `String` settings; value is fetched
+ "Get string value of `setting-definition-or-name`. This is the default getter for `:string` settings. Value is fetched
as follows:
- 1. From corresponding env var, if any;
- 2. From the database (i.e., set via the admin panel), if a value is present;
- 3. The default value, if one was specified.
+ 1. From corresponding env var, if any;
+ 2. From the database (i.e., set via the admin panel), if a value is present;
+ 3. The default value, if one was specified, *if* it is a string.
- If the fetched value is an empty string it is considered to be unset and this function returns `nil`."
+ If the fetched value is an empty string it is considered to be unset and this function returns `nil`."
^String [setting-definition-or-name]
(let [v (or (env-var-value setting-definition-or-name)
(db-or-cache-value setting-definition-or-name)
- (str (:default (resolve-setting setting-definition-or-name))))]
+ (default-value setting-definition-or-name string?))]
(when (seq v)
v)))
-(defn string->boolean
+(s/defn string->boolean :- (s/maybe s/Bool)
"Interpret a `string-value` of a Setting as a boolean."
- [string-value]
+ [string-value :- (s/maybe s/Str)]
(when (seq string-value)
(case (str/lower-case string-value)
"true" true
@@ -212,38 +252,64 @@
(tru "Invalid value for string: must be either \"true\" or \"false\" (case-insensitive)."))))))
(defn get-boolean
- "Get boolean value of (presumably `:boolean`) `setting-definition-or-name`. This is the default getter for `:boolean`
- settings. Returns one of the following values:
+ "Get the value of `setting-definition-or-name` as a boolean. Default getter for `:boolean` Settings.
+
+ Calls [[get-string]] to get the underlying string value from the DB or env var. If a string value is found, parses
+ it as a boolean using the rules below; if not, returns the [[default-value]] directly if it it is a boolean.
+
+ Strings are parsed as follows:
- * `nil` if string value of `setting-definition-or-name` is unset (or empty)
- * `true` if *lowercased* string value of `setting-definition-or-name` is `true`
- * `false` if *lowercased* string value of `setting-definition-or-name` is `false`."
+ * `true` if *lowercased* string value is `true`
+ * `false` if *lowercased* string value is `false`.
+ * Otherwise, throw an Exception."
^Boolean [setting-definition-or-name]
- (string->boolean (get-string setting-definition-or-name)))
+ (if-let [s (get-string setting-definition-or-name)]
+ (string->boolean s)
+ (default-value setting-definition-or-name boolean?)))
(defn get-integer
- "Get integer value of (presumably `:integer`) `setting-definition-or-name`. This is the default getter for `:integer`
- settings."
- ^Integer [setting-definition-or-name]
- (some-> (get-string setting-definition-or-name) Integer/parseInt))
+ "Get the value of `setting-definition-or-name` as a long. Default getter for `:integer` Settings.
+
+ Calls [[get-string]] to get the underlying string value from the DB or env var. If a string value is found, converts
+ it to a keyword with [[java.lang.Long/parseLong]]; if not, returns the [[default-value]] directly if it it is an
+ integer."
+ ^Long [setting-definition-or-name]
+ (if-let [s (get-string setting-definition-or-name)]
+ (Long/parseLong s)
+ ;; if default is actually `Integer` or something make sure we return an instance of `Long`.
+ (some-> (default-value setting-definition-or-name integer?) long)))
(defn get-double
- "Get double value of (presumably `:double`) `setting-definition-or-name`. This is the default getter for `:double`
- settings."
+ "Get the value of `setting-definition-or-name` as a double. Default getter for `:double` Settings.
+
+ Calls [[get-string]] to get the underlying string value from the DB or env var. If a string value is found, converts
+ it to a keyword with [[java.lang.Double/parseDouble]]; if not, returns the [[default-value]] directly if it it is a
+ double."
^Double [setting-definition-or-name]
- (some-> (get-string setting-definition-or-name) Double/parseDouble))
+ (if-let [s (get-string setting-definition-or-name)]
+ (Double/parseDouble s)
+ (default-value setting-definition-or-name double?)))
(defn get-keyword
- "Get value of (presumably `:string`) `setting-definition-or-name` as keyword. This is the default getter for
- `:keyword` settings."
+ "Get the value of `setting-definition-or-name` as a keyword. Default getter for `:keyword` Settings.
+
+ Calls [[get-string]] to get the underlying string value from the DB or env var. If a string value is found, converts
+ it to a keyword with [[keyword]]; if not, returns the [[default-value]] directly if it it is a keyword."
^clojure.lang.Keyword [setting-definition-or-name]
- (some-> setting-definition-or-name get-string keyword))
+ (if-let [s (get-string setting-definition-or-name)]
+ (keyword s)
+ (default-value setting-definition-or-name keyword?)))
(defn get-json
- "Get the string value of `setting-definition-or-name` and parse it as JSON."
+ "Get the value of `setting-definition-or-name` as parsed JSON. Default getter for `:json` Settings.
+
+ Calls [[get-string]] to get the underlying string value from the DB or env var. If a string value is found, parses
+ it with [[cheshire.core/parse-string]]; if not, returns the [[default-value]] directly if it it is a collection."
[setting-definition-or-name]
(try
- (json/parse-string (get-string setting-definition-or-name) keyword)
+ (if-let [s (get-string setting-definition-or-name)]
+ (json/parse-string s keyword)
+ (default-value setting-definition-or-name coll?))
(catch Throwable e
(let [{setting-name :name} (resolve-setting setting-definition-or-name)]
(throw (ex-info (tru "Error parsing JSON setting {0}: {1}" setting-name (ex-message e))
@@ -251,15 +317,27 @@
e))))))
(defn get-timestamp
- "Get the string value of `setting-definition-or-name` and parse it as an ISO-8601-formatted string, returning a
- Timestamp."
- [setting-definition-or-name]
- (u.date/parse (get-string setting-definition-or-name)))
+ "Get the value of `setting-definition-or-name` as a [[java.time.temporal.Temporal]] of some sort (e.g.
+ a [[java.time.OffsetDateTime]]. Default getter for `:timestamp` Settings.
+
+ Calls [[get-string]] to get the underlying string value from the DB or env var. If a string value is found, parses
+ it with [[metabase.util.date-2/parse]]; if not, returns the [[default-value]] directly if it it is an instance
+ of [[java.time.temporal.Temporal]]."
+ ^Temporal [setting-definition-or-name]
+ (if-let [s (get-string setting-definition-or-name)]
+ (u.date/parse s)
+ (default-value setting-definition-or-name (partial instance? Temporal))))
(defn get-csv
- "Get the string value of `setting-definition-or-name` and parse it as CSV, returning a sequence of exploded strings."
+ "Get the value of `setting-definition-or-name` as a sequence of exploded strings. Default getter for `:csv`
+ Settings.
+
+ Calls [[get-string]] to get the underlying string value from the DB or env var. If a string value is found, parses
+ it with [[clojure.data.csv/read-csv]]; if not, returns the [[default-value]] directly if it it is sequential."
[setting-definition-or-name]
- (some-> (get-string setting-definition-or-name) csv/read-csv first))
+ (if-let [s (get-string setting-definition-or-name)]
+ (some-> s csv/read-csv first)
+ (default-value setting-definition-or-name sequential?)))
(def ^:private default-getter-for-type
{:string get-string
@@ -478,6 +556,7 @@
:cache? true}
(dissoc setting :name :type :default)))
(s/validate SettingDefinition <>)
+ (validate-default-value-for-type <>)
;; eastwood complains about (setting-name @registered-settings) for shadowing the function `setting-name`
(when-let [registered-setting (clojure.core/get @registered-settings setting-name)]
(when (not= setting-ns (:namespace registered-setting))
@@ -521,9 +600,7 @@
""
(format " (%s nil)" (setting-name setting))
""
- (format "Its default value is `%s`." (if (nil? default) "nil" default))])})
-
-
+ (format "Its default value is `%s`." (pr-str default))])})
(defn setting-fn
"Create the automatically defined getter/setter function for settings defined by `defsetting`."
@@ -579,7 +656,8 @@
You may optionally pass any of the OPTIONS below:
- * `:default` - The default value of the setting. (default: `nil`)
+ * `: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
@@ -669,8 +747,8 @@
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))]
+ value-is-default? (= parsed-value default)
+ value-is-from-env-var? (some-> (env-var-value setting) (= unparsed-value))]
(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
diff --git a/src/metabase/public_settings.clj b/src/metabase/public_settings.clj
index c960606c5b6d06ae05b9970fcf46f719eb04093c..11a9371638b68bf78d2ca1cb0aa0c5a6be5891ff 100644
--- a/src/metabase/public_settings.clj
+++ b/src/metabase/public_settings.clj
@@ -3,6 +3,7 @@
[clojure.tools.logging :as log]
[java-time :as t]
[metabase.config :as config]
+ [metabase.driver :as driver]
[metabase.driver.util :as driver.u]
[metabase.models.setting :as setting :refer [defsetting]]
[metabase.plugins.classloader :as classloader]
@@ -197,13 +198,13 @@
(defsetting query-caching-max-ttl
(deferred-tru "The absolute maximum time to keep any cached query results, in seconds.")
:type :double
- :default (* 60 60 24 100)) ; 100 days
+ :default (* 60.0 60.0 24.0 100.0)) ; 100 days
;; TODO -- this isn't really a TTL at all. Consider renaming to something like `-min-duration`
(defsetting query-caching-min-ttl
(deferred-tru "Metabase will cache all saved questions with an average query execution time longer than this many seconds:")
:type :double
- :default 60)
+ :default 60.0)
(defsetting query-caching-ttl-ratio
(str (deferred-tru "To determine how long each saved question''s cached result should stick around, we take the query''s average execution time and multiply that by whatever you input here.")
@@ -252,8 +253,13 @@
:type :boolean
:default true
:getter (fn []
- (or (setting/get-boolean :enable-password-login)
- (not (sso-configured?)))))
+ ;; if `:enable-password-login` has an *explict* (non-default) value, and SSO is configured, use that;
+ ;; otherwise this always returns true.
+ (let [v (setting/get-boolean :enable-password-login)]
+ (if (and (some? v)
+ (sso-configured?))
+ v
+ true))))
(defsetting breakout-bins-num
(deferred-tru "When using the default binning strategy and a number of bins is not provided, this number will be used as the default.")
@@ -353,7 +359,7 @@
"Current report timezone abbreviation"
:visibility :public
:setter :none
- :getter (fn [] (short-timezone-name (setting/get :report-timezone))))
+ :getter (fn [] (short-timezone-name (driver/report-timezone))))
(defsetting version
"Metabase's version info"
@@ -390,7 +396,7 @@
It won''t affect SQL queries.")
:visibility :public
:type :keyword
- :default "sunday")
+ :default :sunday)
(defsetting ssh-heartbeat-interval-sec
(deferred-tru "Controls how often the heartbeats are sent when an SSH tunnel is established (in seconds).")
diff --git a/src/metabase/query_processor/middleware/catch_exceptions.clj b/src/metabase/query_processor/middleware/catch_exceptions.clj
index eaa059147641bf209b17bf9d3dd9e2b2d5ca8eed..eba11d852d28cf21359659b40c7b61988952bb71 100644
--- a/src/metabase/query_processor/middleware/catch_exceptions.clj
+++ b/src/metabase/query_processor/middleware/catch_exceptions.clj
@@ -85,15 +85,10 @@
;; TODO -- some of this logic duplicates the functionality of `clojure.core/Throwable->map`, we should consider
;; whether we can use that more extensively and remove some of this logic
-(defn- cause [^Throwable e]
- (let [cause (.getCause e)]
- (when-not (= cause e)
- cause)))
-
-(defn- exception-chain [^Throwable e]
- (->> (iterate cause e)
- (take-while some?)
- reverse))
+(defn- exception-chain
+ "Exception chain in reverse order, e.g. inner-most cause first."
+ [e]
+ (reverse (u/full-exception-chain e)))
(defn- best-top-level-error
"In cases where the top-level Exception doesn't have the best error message, return a better one to use instead. We
diff --git a/src/metabase/task/send_pulses.clj b/src/metabase/task/send_pulses.clj
index b4bfd10cd6ddd7f6d896185d4159017e52fe0e76..932df1ff6f6cccdaa0ff4cc7bd9b672421acfeb3 100644
--- a/src/metabase/task/send_pulses.clj
+++ b/src/metabase/task/send_pulses.clj
@@ -6,9 +6,9 @@
[clojurewerkz.quartzite.jobs :as jobs]
[clojurewerkz.quartzite.schedule.cron :as cron]
[clojurewerkz.quartzite.triggers :as triggers]
+ [metabase.driver :as driver]
[metabase.models.pulse :as pulse]
[metabase.models.pulse-channel :as pulse-channel]
- [metabase.models.setting :as setting]
[metabase.models.task-history :as task-history]
[metabase.pulse :as p]
[metabase.task :as task]
@@ -81,7 +81,7 @@
(try
(task-history/with-task-history {:task "send-pulses"}
;; determine what time it is right now (hour-of-day & day-of-week) in reporting timezone
- (let [reporting-timezone (setting/get :report-timezone)
+ (let [reporting-timezone (driver/report-timezone)
now (if (empty? reporting-timezone)
(time/now)
(time/to-time-zone (time/now) (time/time-zone-for-id reporting-timezone)))
diff --git a/src/metabase/troubleshooting.clj b/src/metabase/troubleshooting.clj
index ebd18bec504cc475110a6ec2a030afaf933a5612..0bcb7ba7a7ddc1a42f33fa82ed591b932d8ffb6d 100644
--- a/src/metabase/troubleshooting.clj
+++ b/src/metabase/troubleshooting.clj
@@ -3,7 +3,7 @@
[clojure.java.jmx :as jmx]
[metabase.config :as mc]
[metabase.db :as mdb]
- [metabase.models.setting :as setting]
+ [metabase.driver :as driver]
[metabase.util.stats :as mus]
[toucan.db :as db])
(:import javax.management.ObjectName))
@@ -38,7 +38,7 @@
:version (.getDriverVersion metadata)}})
:run-mode (mc/config-kw :mb-run-mode)
:version mc/mb-version-info
- :settings {:report-timezone (setting/get :report-timezone)}})
+ :settings {:report-timezone (driver/report-timezone)}})
(defn- conn-pool-bean-diag-info [acc ^ObjectName jmx-bean]
(let [bean-id (.getCanonicalName jmx-bean)
diff --git a/src/metabase/util.clj b/src/metabase/util.clj
index 97aa24506f11a587b732899b5e8fd8de68e8063b..ad4daa88a5b50b382f7d383a4d9d446c84e44c3d 100644
--- a/src/metabase/util.clj
+++ b/src/metabase/util.clj
@@ -431,9 +431,10 @@
(str/join (take max-length (slugify s)))))
(defn full-exception-chain
- "Gather the full exception chain into a single vector."
+ "Gather the full exception chain into a sequence."
[e]
- (take-while some? (iterate #(.getCause ^Throwable %) e)))
+ (when (instance? Throwable e)
+ (take-while some? (iterate ex-cause e))))
(defn all-ex-data
"Like `ex-data`, but merges `ex-data` from causes. If duplicate keys exist, the keys from the highest level are
diff --git a/test/metabase/api/email_test.clj b/test/metabase/api/email_test.clj
index 127c9e0211325245d5f748f5729c94f539e0c337..369eb4dce47b05dcf56e24c302cfe8b5638886f0 100644
--- a/test/metabase/api/email_test.clj
+++ b/test/metabase/api/email_test.clj
@@ -1,9 +1,19 @@
(ns metabase.api.email-test
(:require [clojure.test :refer :all]
+ [metabase.api.email :as api.email]
[metabase.email :as email]
[metabase.models.setting :as setting]
[metabase.test :as mt]
- [metabase.test.util :as tu]))
+ [metabase.test.util :as tu]
+ [metabase.util :as u]))
+
+(deftest humanize-error-messages-test
+ (is (= {:errors {:email-smtp-host "Wrong host or port", :email-smtp-port "Wrong host or port"}}
+ (#'api.email/humanize-error-messages
+ {::email/error (Exception. "Couldn't connect to host, port: foobar, 789; timeout 1000: foobar")})))
+ (is (= {:message "Sorry, something went wrong. Please try again. Error: Some unexpected message"}
+ (#'api.email/humanize-error-messages
+ {::email/error (Exception. "Some unexpected message")}))))
(defn- email-settings
[]
@@ -16,31 +26,73 @@
(def ^:private default-email-settings
{:email-smtp-host "foobar"
- :email-smtp-port "789"
- :email-smtp-security "tls"
+ :email-smtp-port 789
+ :email-smtp-security :tls
:email-smtp-username "munchkin"
:email-smtp-password "gobble gobble"
:email-from-address "eating@hungry.com"})
-;; PUT /api/email - check updating email settings
+(deftest test-email-settings-test
+ (testing "POST /api/email/test -- send a test email"
+ (mt/with-temporary-setting-values [email-from-address "notifications@metabase.com"]
+ (mt/with-fake-inbox
+ (testing "Non-admin -- request should fail"
+ (is (= "You don't have permissions to do that."
+ (mt/user-http-request :rasta :post 403 "email/test")))
+ (is (= {}
+ @mt/inbox)))
+ (is (= {:ok true}
+ (mt/user-http-request :crowberto :post 200 "email/test")))
+ (is (= {"crowberto@metabase.com"
+ [{:from "notifications@metabase.com",
+ :to ["crowberto@metabase.com"],
+ :subject "Metabase Test Email",
+ :body "Your Metabase emails are working — hooray!"}]}
+ @mt/inbox))))))
+
(deftest update-email-settings-test
- (testing "PUT /api/email"
- (tu/discard-setting-changes [email-smtp-host email-smtp-port email-smtp-security email-smtp-username
- email-smtp-password email-from-address]
- (with-redefs [email/test-smtp-connection (constantly {:error :SUCCESS})]
- (is (= (assoc default-email-settings
- :with-corrections {})
- (mt/user-http-request :crowberto :put 200 "email" default-email-settings)))
- (is (= default-email-settings
- (email-settings)))))))
+ (testing "PUT /api/email - check updating email settings"
+ ;; [[metabase.email/email-smtp-port]] was originally a string Setting (it predated our introduction of different
+ ;; Settings types) -- make sure our API endpoints still work if you pass in the value as a String rather than an
+ ;; integer.
+ (doseq [:let [original-values (email-settings)]
+ body [default-email-settings
+ (update default-email-settings :email-smtp-port str)]
+ ;; test what happens on both a successful and an unsuccessful connection.
+ [success? f] {true (fn [thunk]
+ (with-redefs [email/test-smtp-settings (constantly {::email/error nil})]
+ (thunk)))
+ false (fn [thunk]
+ (with-redefs [email/retry-delay-ms 0]
+ (thunk)))}]
+ (tu/discard-setting-changes [email-smtp-host email-smtp-port email-smtp-security email-smtp-username
+ email-smtp-password email-from-address]
+ (testing (format "SMTP connection is valid? %b\n" success?)
+ (f (fn []
+ (testing "API request"
+ (testing (format "\nRequest body =\n%s" (u/pprint-to-str body))
+ (if success?
+ (is (= (-> default-email-settings
+ (assoc :with-corrections {})
+ (update :email-smtp-security name))
+ (mt/user-http-request :crowberto :put 200 "email" body)))
+ (is (= {:errors {:email-smtp-host "Wrong host or port"
+ :email-smtp-port "Wrong host or port"}}
+ (mt/user-http-request :crowberto :put 400 "email" body))))))
+ (testing "Settings after API request is finished"
+ (is (= (if success?
+ default-email-settings
+ original-values)
+ (email-settings)))))))))))
(deftest clear-email-settings-test
(testing "DELETE /api/email"
(tu/discard-setting-changes [email-smtp-host email-smtp-port email-smtp-security email-smtp-username
email-smtp-password email-from-address]
- (with-redefs [email/test-smtp-connection (constantly {:error :SUCCESS})]
- (is (= (assoc default-email-settings
- :with-corrections {})
+ (with-redefs [email/test-smtp-settings (constantly {::email/error nil})]
+ (is (= (-> default-email-settings
+ (assoc :with-corrections {})
+ (update :email-smtp-security name))
(mt/user-http-request :crowberto :put 200 "email" default-email-settings)))
(let [new-email-settings (email-settings)]
(is (nil? (mt/user-http-request :crowberto :delete 204 "email")))
@@ -48,7 +100,7 @@
new-email-settings))
(is (= {:email-smtp-host nil
:email-smtp-port nil
- :email-smtp-security "none"
+ :email-smtp-security :none
:email-smtp-username nil
:email-smtp-password nil
:email-from-address "notifications@metabase.com"}
diff --git a/test/metabase/api/session_test.clj b/test/metabase/api/session_test.clj
index 385a1cf00fcabe141f65377bcad47ce5eba27166..5c6195691ed00390816749e750d6dcccddb1bb88 100644
--- a/test/metabase/api/session_test.clj
+++ b/test/metabase/api/session_test.clj
@@ -396,8 +396,10 @@
(deftest ldap-login-test
(ldap.test/with-ldap-server
(testing "Test that we can login with LDAP"
- (let [user-id (test-users/user->id :rasta)]
+ (let [user-id (mt/user->id :rasta)]
(try
+ ;; TODO -- it's not so nice to go around permanently deleting stuff like Sessions like this in tests. We
+ ;; should just create a temp User instead for this test
(db/simple-delete! Session :user_id user-id)
(is (schema= SessionResponse
(mt/client :post 200 "session" (mt/user->credentials :rasta))))
@@ -419,7 +421,7 @@
(mt/client :post 401 "session" (mt/user->credentials :lucky)))))
(testing "Test that a deactivated user cannot login with LDAP"
- (let [user-id (test-users/user->id :rasta)]
+ (let [user-id (mt/user->id :rasta)]
(try
(db/update! User user-id :is_active false)
(is (= {:errors {:_error "Your account is disabled."}}
@@ -430,7 +432,7 @@
(testing "Test that login will fallback to local for broken LDAP settings"
(mt/with-temporary-setting-values [ldap-user-base "cn=wrong,cn=com"]
;; delete all other sessions for the bird first, otherwise test doesn't seem to work (TODO - why?)
- (let [user-id (test-users/user->id :rasta)]
+ (let [user-id (mt/user->id :rasta)]
(try
(db/simple-delete! Session :user_id user-id)
(is (schema= SessionResponse
diff --git a/test/metabase/api/setting_test.clj b/test/metabase/api/setting_test.clj
index c4e541fe97932d4f7c0e349501401cb1dd7cceaa..8ecd6803c816045ca7e238925338e10010749192 100644
--- a/test/metabase/api/setting_test.clj
+++ b/test/metabase/api/setting_test.clj
@@ -1,8 +1,10 @@
(ns metabase.api.setting-test
(:require [clojure.test :refer :all]
[metabase.models.setting-test :refer [test-sensitive-setting test-setting-1 test-setting-2 test-setting-3]]
+ [metabase.public-settings.metastore-test :as metastore-test]
[metabase.test :as mt]
- [metabase.test.fixtures :as fixtures]))
+ [metabase.test.fixtures :as fixtures]
+ [schema.core :as s]))
(use-fixtures :once (fixtures/initialize :db))
@@ -45,12 +47,27 @@
(is (= "OK!"
(fetch-setting :test-setting-2 200))))))
+(deftest fetch-calculated-settings-test
+ (testing "GET /api/setting"
+ (testing "Should return the correct `:value` for Settings with no underlying DB/env var value"
+ (metastore-test/with-metastore-token-features #{:embedding}
+ (is (schema= {:key (s/eq "hide-embed-branding?")
+ :value (s/eq true)
+ :is_env_setting (s/eq false)
+ :env_name (s/eq "MB_HIDE_EMBED_BRANDING")
+ :default (s/eq nil)
+ s/Keyword s/Any}
+ (some
+ (fn [{setting-name :key, :as setting}]
+ (when (= setting-name "hide-embed-branding?")
+ setting))
+ (mt/user-http-request :crowberto :get 200 "setting"))))))))
+
(deftest fetch-internal-settings-test
(testing "Test that we can't fetch internal settings"
(test-setting-3 "NOPE!")
(is (= "Setting :test-setting-3 is internal"
- (mt/suppress-output
- (:message (fetch-setting :test-setting-3 500)))))))
+ (:message (fetch-setting :test-setting-3 500))))))
(deftest update-settings-test
(testing "PUT /api/setting/:key"
diff --git a/test/metabase/email/messages_test.clj b/test/metabase/email/messages_test.clj
index 76075072ed5b2621779d626ad9c3d406e73f1105..3d7f49fc6f001ccccabf55888e1d962a29dbc90e 100644
--- a/test/metabase/email/messages_test.clj
+++ b/test/metabase/email/messages_test.clj
@@ -21,31 +21,28 @@
(deftest password-reset-email
(testing "password reset email can be sent successfully"
- (email-test/do-with-fake-inbox
- (fn []
- (messages/send-password-reset-email! "test@test.com" false "test.domain.com" "http://localhost/some/url" true)
- (is (= [{:from "notifications@metabase.com",
- :to ["test@test.com"],
- :subject "[Metabase] Password Reset Request",
- :body [{:type "text/html; charset=utf-8"}]}]
- (-> (@email-test/inbox "test@test.com")
- (update-in [0 :body 0] dissoc :content)))))))
+ (email-test/with-fake-inbox
+ (messages/send-password-reset-email! "test@test.com" false "test.domain.com" "http://localhost/some/url" true)
+ (is (= [{:from "notifications@metabase.com",
+ :to ["test@test.com"],
+ :subject "[Metabase] Password Reset Request",
+ :body [{:type "text/html; charset=utf-8"}]}]
+ (-> (@email-test/inbox "test@test.com")
+ (update-in [0 :body 0] dissoc :content))))))
;; Email contents contain randomized elements, so we only check for the inclusion of a single word to verify
;; that the contents changed in the tests below.
(testing "password reset email tells user if they should log in with Google Sign-In"
- (email-test/do-with-fake-inbox
- (fn []
- (messages/send-password-reset-email! "test@test.com" true "test.domain.com" "http://localhost/some/url" true)
- (is (-> (@email-test/inbox "test@test.com")
- (get-in [0 :body 0 :content])
- (str/includes? "Google"))))))
+ (email-test/with-fake-inbox
+ (messages/send-password-reset-email! "test@test.com" true "test.domain.com" "http://localhost/some/url" true)
+ (is (-> (@email-test/inbox "test@test.com")
+ (get-in [0 :body 0 :content])
+ (str/includes? "Google")))))
(testing "password reset email tells user if their account is inactive"
- (email-test/do-with-fake-inbox
- (fn []
- (messages/send-password-reset-email! "test@test.com" false "test.domain.com" "http://localhost/some/url" false)
- (is (-> (@email-test/inbox "test@test.com")
- (get-in [0 :body 0 :content])
- (str/includes? "deactivated")))))))
+ (email-test/with-fake-inbox
+ (messages/send-password-reset-email! "test@test.com" false "test.domain.com" "http://localhost/some/url" false)
+ (is (-> (@email-test/inbox "test@test.com")
+ (get-in [0 :body 0 :content])
+ (str/includes? "deactivated"))))))
(defmacro ^:private with-create-temp-failure [& body]
`(with-redefs [messages/create-temp-file (fn [~'_]
diff --git a/test/metabase/email_test.clj b/test/metabase/email_test.clj
index 431362bafe04935979cee240ff9a523e1ab16fc2..15f9d0e01a151dd74154c50579e9130a8a28493a 100644
--- a/test/metabase/email_test.clj
+++ b/test/metabase/email_test.clj
@@ -64,10 +64,10 @@
(defn do-with-fake-inbox
"Impl for `with-fake-inbox` macro; prefer using that rather than calling this directly."
[f]
- (with-redefs [metabase.email/send-email! fake-inbox-email-fn]
+ (with-redefs [email/send-email! fake-inbox-email-fn]
(reset-inbox!)
(tu/with-temporary-setting-values [email-smtp-host "fake_smtp_host"
- email-smtp-port "587"]
+ email-smtp-port 587]
(f))))
(defmacro with-fake-inbox
@@ -159,8 +159,8 @@
email-smtp-host "smtp.metabase.com"
email-smtp-username "lucky"
email-smtp-password "d1nner3scapee!"
- email-smtp-port "1025"
- email-smtp-security "none"]
+ email-smtp-port 1025
+ email-smtp-security :none]
(testing "basic sending"
(is (=
[{:from (email/email-from-address)
diff --git a/test/metabase/integrations/ldap_test.clj b/test/metabase/integrations/ldap_test.clj
index ff560260951f49790e2fc1e633f410aa972ac418..5f4b87048fc31196c89ce0585bc47d6d986d6971 100644
--- a/test/metabase/integrations/ldap_test.clj
+++ b/test/metabase/integrations/ldap_test.clj
@@ -14,7 +14,7 @@
:port (ldap.test/get-ldap-port)
:bind-dn "cn=Directory Manager"
:password "password"
- :security "none"
+ :security :none
:user-base "dc=metabase,dc=com"
:group-base "dc=metabase,dc=com"})
diff --git a/test/metabase/models/setting_test.clj b/test/metabase/models/setting_test.clj
index 5abf870d859ec7588bd4f40030934c1a4c15e83b..ec972e83bacd7a26654bb57e07cc5871dd08f470 100644
--- a/test/metabase/models/setting_test.clj
+++ b/test/metabase/models/setting_test.clj
@@ -45,7 +45,7 @@
"Test setting - this only shows up in dev (6)"
:visibility :internal
:type :csv
- :default "A,B,C")
+ :default ["A" "B" "C"])
(defsetting test-env-setting
"Test setting - this only shows up in dev (7)"
@@ -55,6 +55,13 @@
"Name for the Metabase Toucan mascot."
:visibility :internal)
+(setting/defsetting test-setting-calculated-getter
+ "Test setting - this only shows up in dev (8)"
+ :type :boolean
+ :setter :none
+ :getter (fn []
+ true))
+
;; ## HELPER FUNCTIONS
(defn db-fetch-setting
@@ -81,12 +88,18 @@
(testing "Test getting a default value -- if you clear the value of a Setting it should revert to returning the default value"
(test-setting-2 nil)
(is (= "[Default Value]"
- (test-setting-2))))
+ (test-setting-2)))))
+(deftest user-facing-value-test
(testing "`user-facing-value` should return `nil` for a Setting that is using the default value"
(test-setting-2 nil)
(is (= nil
- (setting/user-facing-value :test-setting-2)))))
+ (setting/user-facing-value :test-setting-2))))
+ (testing "`user-facing-value` should work correctly for calculated Settings (no underlying value)"
+ (is (= true
+ (test-setting-calculated-getter)))
+ (is (= true
+ (setting/user-facing-value :test-setting-calculated-getter)))))
(deftest defsetting-setter-fn-test
(test-setting-2 "FANCY NEW VALUE <3")
@@ -302,8 +315,9 @@
(testing "if value isn't true / false"
(testing "getter should throw exception"
- (is (thrown?
+ (is (thrown-with-msg?
Exception
+ #"Invalid value for string: must be either \"true\" or \"false\" \(case-insensitive\)"
(test-boolean-setting "X"))))
(testing "user-facing info should just return `nil` instead of failing entirely"
@@ -599,3 +613,41 @@
(testing "Removes characters not-compliant with shells"
(is (= "aa1aa-b2b_cc3c"
(#'setting/munge-setting-name "aa1'aa@#?-b2@b_cc'3?c?")))))
+
+(deftest validate-default-value-for-type-test
+ (letfn [(validate [tag default]
+ (@#'setting/validate-default-value-for-type
+ {:tag tag, :default default, :name :a-setting, :type :fake-type}))]
+ (testing "No default value"
+ (is (nil? (validate `String nil))))
+ (testing "No tag"
+ (is (nil? (validate nil "abc"))))
+ (testing "tag is not a symbol or string"
+ (is (thrown-with-msg?
+ AssertionError
+ #"Setting :tag should be a symbol or string, got: \^clojure\.lang\.Keyword :string"
+ (validate :string "Green Friend"))))
+ (doseq [[tag valid-tag?] {"String" false
+ "java.lang.String" true
+ 'STRING false
+ `str false
+ `String true}
+ [value valid-value?] {"Green Friend" true
+ :green-friend false}]
+ (testing (format "Tag = %s (valid = %b)" (pr-str tag) valid-tag?)
+ (testing (format "Value = %s (valid = %b)" (pr-str value) valid-value?)
+ (cond
+ (and valid-tag? valid-value?)
+ (is (nil? (validate tag value)))
+
+ (not valid-tag?)
+ (is (thrown-with-msg?
+ Exception
+ #"Cannot resolve :tag .+ to a class"
+ (validate tag value)))
+
+ (not valid-value?)
+ (is (thrown-with-msg?
+ Exception
+ #"Wrong :default type: got \^clojure\.lang\.Keyword :green-friend, but expected a java\.lang\.String"
+ (validate tag value)))))))))
diff --git a/test/metabase/test/integrations/ldap.clj b/test/metabase/test/integrations/ldap.clj
index 848f4bc8dcf8f27e926914b86e260a74cfdf5680..b361dc8c0b2c55e1d3dac2c531188fb16c80ba63 100644
--- a/test/metabase/test/integrations/ldap.clj
+++ b/test/metabase/test/integrations/ldap.clj
@@ -47,7 +47,7 @@
(try
(tu/with-temporary-setting-values [ldap-enabled true
ldap-host "localhost"
- ldap-port (str (get-ldap-port))
+ ldap-port (get-ldap-port)
ldap-bind-dn "cn=Directory Manager"
ldap-password "password"
ldap-user-base "dc=metabase,dc=com"
diff --git a/test/metabase/test/util_test.clj b/test/metabase/test/util_test.clj
index c37ba25d63f5c4bb14d8c9ef682b1e5ea4aa6935..3ab7d0381b33c0f9f597fdbe426d8ca176d24b85 100644
--- a/test/metabase/test/util_test.clj
+++ b/test/metabase/test/util_test.clj
@@ -27,8 +27,8 @@
(setting/defsetting test-util-test-setting
"Another internal test setting"
:visibility :internal
- :default "A,B,C"
- :type :csv)
+ :default ["A" "B" "C"]
+ :type :csv)
(deftest with-temporary-setting-values-test
(testing "`with-temporary-setting-values` should do its thing"
diff --git a/test/metabase/util_test.clj b/test/metabase/util_test.clj
index 24de651fc7cc06a5dc597b844ecfe8c5d7ad3577..dffcb3a913c2e25c47dd9b2dfa42c65784331fb6 100644
--- a/test/metabase/util_test.clj
+++ b/test/metabase/util_test.clj
@@ -127,9 +127,28 @@
(is (= expected
(u/slugify s))))))))
+(deftest full-exception-chain-test
+ (testing "Not an Exception"
+ (is (= nil
+ (u/full-exception-chain nil)))
+ (is (= nil
+ (u/full-exception-chain 100))))
+ (testing "No causes"
+ (let [e (ex-info "A" {:a 1})]
+ (is (= ["A"]
+ (map ex-message (u/full-exception-chain e))))
+ (is (= [{:a 1}]
+ (map ex-data (u/full-exception-chain e))))))
+ (testing "w/ causes"
+ (let [e (ex-info "A" {:a 1} (ex-info "B" {:b 2} (ex-info "C" {:c 3})))]
+ (is (= ["A" "B" "C"]
+ (map ex-message (u/full-exception-chain e))))
+ (is (= [{:a 1} {:b 2} {:c 3}]
+ (map ex-data (u/full-exception-chain e)))))))
+
(deftest select-nested-keys-test
(mt/are+ [m keyseq expected] (= expected
- (u/select-nested-keys m keyseq))
+ (u/select-nested-keys m keyseq))
{:a 100, :b {:c 200, :d 300}} [:a [:b :d] :c] {:a 100, :b {:d 300}}
{:a 100, :b {:c 200, :d 300}} [:b] {:b {:c 200, :d 300}}
{:a 100, :b {:c 200, :d 300}} [[:b :c :d]] {:b {:c 200, :d 300}}