From 94432ff14f0acab94c834ec0b0f8b8841f62a6c3 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Tue, 17 Aug 2021 17:03:09 -0700
Subject: [PATCH] Ensure Setting :default values match the Setting :type; fix
user-facing-value for calculated Settings. Cleanup & extra tests for email
settings (#17442)
* Change of plans. get-string (etc) only return default if it matches the type
* Better docstrings
* More tests & rework the validate-default-value logic a bit.
* Fix lint error
* Add an additional test for GET /api/setting
* Consolidate exception-chain util functions
* Email error message humanization improvements
* More tests for util ex-chain
* Clean up email error handling and add lots of extra tests
* Fix docstring indentation
* Even more email tests
* Test fixes :wrench:
* Update Cypress test again
* Code cleanup
---
.../scenarios/admin/settings/email.cy.spec.js | 2 +-
src/metabase/api/email.clj | 49 +++---
src/metabase/api/ldap.clj | 5 +-
src/metabase/email.clj | 158 ++++++++++-------
src/metabase/integrations/ldap.clj | 36 ++--
src/metabase/models/humanization.clj | 24 +--
src/metabase/models/setting.clj | 166 +++++++++++++-----
src/metabase/public_settings.clj | 18 +-
.../middleware/catch_exceptions.clj | 13 +-
src/metabase/task/send_pulses.clj | 4 +-
src/metabase/troubleshooting.clj | 4 +-
src/metabase/util.clj | 5 +-
test/metabase/api/email_test.clj | 86 +++++++--
test/metabase/api/session_test.clj | 8 +-
test/metabase/api/setting_test.clj | 23 ++-
test/metabase/email/messages_test.clj | 39 ++--
test/metabase/email_test.clj | 8 +-
test/metabase/integrations/ldap_test.clj | 2 +-
test/metabase/models/setting_test.clj | 60 ++++++-
test/metabase/test/integrations/ldap.clj | 2 +-
test/metabase/test/util_test.clj | 4 +-
test/metabase/util_test.clj | 21 ++-
22 files changed, 495 insertions(+), 242 deletions(-)
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 7cea381a1c4..a863aa69661 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 085b1beac53..ee6eea735dc 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 ae45645b857..e26175a04d6 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 94c0a043e53..f60b23a4b74 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 332269788c6..897ebd1fb1e 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 9a8912ce2af..836d9829a24 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 49470298104..a5778664adf 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 c960606c5b6..11a9371638b 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 eaa05914764..eba11d852d2 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 b4bfd10cd6d..932df1ff6f6 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 ebd18bec504..0bcb7ba7a7d 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 97aa24506f1..ad4daa88a5b 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 127c9e02113..369eb4dce47 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 385a1cf00fc..5c6195691ed 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 c4e541fe979..8ecd6803c81 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 76075072ed5..3d7f49fc6f0 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 431362bafe0..15f9d0e01a1 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 ff560260951..5f4b87048fc 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 5abf870d859..ec972e83bac 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 848f4bc8dcf..b361dc8c0b2 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 c37ba25d63f..3ab7d0381b3 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 24de651fc7c..dffcb3a913c 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}}
--
GitLab