diff --git a/src/metabase/driver/mongo.clj b/src/metabase/driver/mongo.clj
index 5296b2d0db7ecbe4504a4c088a461a802c6258e4..45f504ea3bc5898a6ce370e178e55c05e15f9093 100644
--- a/src/metabase/driver/mongo.clj
+++ b/src/metabase/driver/mongo.clj
@@ -66,14 +66,17 @@
(cond
;; 1. url?
(and (string? field-value)
- (u/url? field-value)) :type/URL
+ (u/url? field-value))
+ :type/URL
+
;; 2. json?
(and (string? field-value)
(or (.startsWith "{" field-value)
- (.startsWith "[" field-value))) (when-let [j (u/try-apply json/parse-string field-value)]
- (when (or (map? j)
- (sequential? j))
- :type/SerializedJSON))))
+ (.startsWith "[" field-value)))
+ (when-let [j (u/ignore-exceptions (json/parse-string field-value))]
+ (when (or (map? j)
+ (sequential? j))
+ :type/SerializedJSON))))
(defn- find-nested-fields [field-value nested-fields]
(loop [[k & more-keys] (keys field-value)
diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj
index 8c6029af06b7f52b934ed169da7a23cf64b58b2f..29a595b9e26a68ac848ae7b55fad76045e5fbce5 100644
--- a/src/metabase/models/setting.clj
+++ b/src/metabase/models/setting.clj
@@ -30,13 +30,20 @@
(setting/all)"
(:refer-clojure :exclude [get])
(:require [cheshire.core :as json]
- [clojure.string :as str]
+ [clojure
+ [core :as core]
+ [string :as str]]
+ [clojure.core.memoize :as memoize]
+ [clojure.java.jdbc :as jdbc]
[clojure.tools.logging :as log]
[environ.core :as env]
+ [honeysql.core :as hsql]
[metabase
+ [db :as mdb]
[events :as events]
[util :as u]]
- [puppetlabs.i18n.core :refer [tru]]
+ [metabase.util.honeysql-extensions :as hx]
+ [puppetlabs.i18n.core :refer [trs tru]]
[schema.core :as s]
[toucan
[db :as db]
@@ -74,7 +81,8 @@
setting-or-name
(let [k (keyword setting-or-name)]
(or (@registered-settings k)
- (throw (Exception. (str (tru "Setting {0} does not exist.\nFound: {1}" k (sort (keys @registered-settings))))))))))
+ (throw (Exception.
+ (str (tru "Setting {0} does not exist.\nFound: {1}" k (sort (keys @registered-settings))))))))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -84,12 +92,115 @@
;; Cache is a 1:1 mapping of what's in the DB
;; Cached lookup time is ~60µs, compared to ~1800µs for DB lookup
-(defonce ^:private cache
+(def ^:private cache
+ "Settings cache. Map of Setting key (string) -> Setting value (string)."
(atom nil))
-(defn- restore-cache-if-needed! []
- (when-not @cache
- (reset! cache (db/select-field->field :key :value Setting))))
+;; CACHE SYNCHRONIZATION
+;;
+;; When running multiple Metabase instances (horizontal scaling), it is of course possible for one instance to update
+;; a Setting, and, since Settings are cached (to avoid tons of DB calls), for the other instances to then have an
+;; out-of-date cache. Thus we need a way for instances to know when their caches are out of date, so they can update
+;; them accordingly. Here is our solution:
+;;
+;; We will record the last time *any* Setting was updated in a special Setting called `settings-last-updated`.
+;;
+;; Since `settings-last-updated` itself is a Setting, it will get fetched as part of each instance's local cache; we
+;; can then periodically compare the locally cached value of `settings-last-updated` with the value in the DB. If our
+;; locally cached value is older than the one in the DB, we will flush our cache. When the cache is fetched again, it
+;; will have the up-to-date value.
+;;
+;; Because different machines can have out-of-sync clocks, we'll rely entirely on the application DB for caclulating
+;; and comparing values of `settings-last-updated`. Because the Setting table itself only stores text values, we'll
+;; need to cast it between TEXT and TIMESTAMP SQL types as needed.
+
+(def ^:private ^String settings-last-updated-key "settings-last-updated")
+
+(defn- update-settings-last-updated!
+ "Update the value of `settings-last-updated` in the DB; if the row does not exist, insert one."
+ []
+ (log/debug (trs "Updating value of settings-last-updated in DB..."))
+ ;; for MySQL, cast(current_timestamp AS char); for H2 & Postgres, cast(current_timestamp AS text)
+ (let [current-timestamp-as-string-honeysql (hx/cast (if (= (mdb/db-type) :mysql) :char :text)
+ (hsql/raw "current_timestamp"))]
+ ;; attempt to UPDATE the existing row. If no row exists, `update-where!` will return false...
+ (or (db/update-where! Setting {:key settings-last-updated-key} :value current-timestamp-as-string-honeysql)
+ ;; ...at which point we will try to INSERT a new row. Note that it is entirely possible two instances can both
+ ;; try to INSERT it at the same time; one instance would fail because it would violate the PK constraint on
+ ;; `key`, and throw a SQLException. As long as one instance updates the value, we are fine, so we can go ahead
+ ;; and ignore that Exception if one is thrown.
+ (try
+ ;; Use `simple-insert!` because we do *not* want to trigger pre-insert behavior, such as encrypting `:value`
+ (db/simple-insert! Setting :key settings-last-updated-key, :value current-timestamp-as-string-honeysql)
+ (catch java.sql.SQLException e
+ ;; go ahead and log the Exception anyway on the off chance that it *wasn't* just a race condition issue
+ (log/error (tru "Error inserting new Setting:") (with-out-str (jdbc/print-sql-exception-chain e)))))))
+ ;; Now that we updated the value in the DB, go ahead and update our cached value as well, because we know about the
+ ;; changes
+ (swap! cache assoc settings-last-updated-key (db/select-one-field :value Setting :key settings-last-updated-key)))
+
+(defn- cache-out-of-date?
+ "Check whether our Settings cache is out of date. We know the cache is out of date if either of the following
+ conditions is true:
+
+ * The cache is empty (the `cache` atom is `nil`), which of course means it needs to be updated
+ * There is a value of `settings-last-updated` in the cache, and it is older than the value of in the DB. (There
+ will be no value until the first time a normal Setting is updated; thus if it is not yet set, we do not yet need
+ to invalidate our cache.)"
+ []
+ (log/debug (trs "Checking whether settings cache is out of date (requires DB call)..."))
+ (boolean
+ (or
+ ;; is the cache empty?
+ (not @cache)
+ ;; if not, get the cached value of `settings-last-updated`, and if it exists...
+ (when-let [last-known-update (core/get @cache settings-last-updated-key)]
+ ;; compare it to the value in the DB. This is done be seeing whether a row exists
+ ;; WHERE value > <local-value>
+ (db/select-one Setting
+ {:where [:and
+ [:= :key settings-last-updated-key]
+ [:> :value last-known-update]]})))))
+
+(def ^:private cache-update-check-interval-ms
+ "How often we should check whether the Settings cache is out of date (which requires a DB call)?"
+ ;; once a minute
+ (* 60 1000))
+
+(def ^:private ^{:arglists '([])} should-restore-cache?
+ "TTL-memoized version of `cache-out-of-date?`. Call this function to see whether we need to repopulate the cache with
+ values from the DB."
+ (memoize/ttl cache-out-of-date? :ttl/threshold cache-update-check-interval-ms))
+
+(def ^:private restore-cache-if-needed-lock (Object.))
+
+(defn- restore-cache-if-needed!
+ "Check whether we need to repopulate the cache with fresh values from the DB (because the cache is either empty or
+ known to be out-of-date), and do so if needed. This is intended to be called every time a Setting value is
+ retrieved, so it should be efficient; thus the calculation (`should-restore-cache?`) is itself TTL-memoized."
+ []
+ ;; There's a potential race condition here where two threads both call this at the exact same moment, and both get
+ ;; `true` when they call `should-restore-cache`, and then both simultaneously try to update the cache (or, one
+ ;; updates the cache, but the other calls `should-restore-cache?` and gets `true` before the other calls
+ ;; `memo-swap!` (see below))
+ ;;
+ ;; This is not desirable, since either situation would result in duplicate work. Better to just add a quick lock
+ ;; here so only one of them does it, since at any rate waiting for the other thread to finish the task in progress is
+ ;; certainly quicker than starting the task ourselves from scratch
+ (locking restore-cache-if-needed-lock
+ (when (should-restore-cache?)
+ (log/debug (trs "Refreshing Settings cache..."))
+ (reset! cache (db/select-field->field :key :value Setting))
+ ;; Now the cache is up-to-date. That is all good, but if we call `should-restore-cache?` again in a second it
+ ;; will still return `true`, because its result is memoized, and we would be on the hook to (again) update the
+ ;; cache. So go ahead and clear the memozied results for `should-restore-cache?`. The next time around when
+ ;; someone calls this it will cache the latest value (which should be `false`)
+ ;;
+ ;; NOTE: I tried using `memo-swap!` instead to set the cached response to `false` here, avoiding the extra DB
+ ;; call the next fn call would make, but it didn't seem to work correctly (I think it was still discarding the
+ ;; new value because of the TTL). So we will just stick with `memo-clear!` for now. (One extra DB call whenever
+ ;; the cache gets invalidated shouldn't be a huge deal)
+ (memoize/memo-clear! should-restore-cache?))))
;;; +----------------------------------------------------------------------------------------------------------------+
@@ -100,14 +211,14 @@
(name (:name (resolve-setting setting-or-name))))
(defn- env-var-name
- "Get the env var corresponding to SETTING-OR-NAME.
+ "Get the env var corresponding to `setting-or-name`.
(This is used primarily for documentation purposes)."
^String [setting-or-name]
(let [setting (resolve-setting setting-or-name)]
(str "MB_" (str/upper-case (str/replace (setting-name setting) "-" "_")))))
(defn env-var-value
- "Get the value of SETTING-OR-NAME from the corresponding env var, if any.
+ "Get the value of `setting-or-name` from the corresponding env var, if any.
The name of the Setting is converted to uppercase and dashes to underscores;
for example, a setting named `default-domain` can be set with the env var `MB_DEFAULT_DOMAIN`."
^String [setting-or-name]
@@ -117,14 +228,14 @@
v)))
(defn- db-value
- "Get the value, if any, of SETTING-OR-NAME from the DB (using / restoring the cache as needed)."
+ "Get the value, if any, of `setting-or-name` from the DB (using / restoring the cache as needed)."
^String [setting-or-name]
(restore-cache-if-needed!)
(clojure.core/get @cache (setting-name setting-or-name)))
(defn get-string
- "Get string value of SETTING-OR-NAME. This is the default getter for `String` settings; valuBis fetched as follows:
+ "Get string value of `setting-or-name`. This is the default getter for `String` settings; valuBis fetched as follows:
1. From the database (i.e., set via the admin panel), if a value is present;
2. From corresponding env var, if any;
@@ -144,32 +255,33 @@
(case (str/lower-case string-value)
"true" true
"false" false
- (throw (Exception. (str (tru "Invalid value for string: must be either \"true\" or \"false\" (case-insensitive).")))))))
+ (throw (Exception.
+ (str (tru "Invalid value for string: must be either \"true\" or \"false\" (case-insensitive).")))))))
(defn get-boolean
- "Get boolean value of (presumably `:boolean`) SETTING-OR-NAME. This is the default getter for `:boolean` settings.
+ "Get boolean value of (presumably `:boolean`) `setting-or-name`. This is the default getter for `:boolean` settings.
Returns one of the following values:
- * `nil` if string value of SETTING-OR-NAME is unset (or empty)
- * `true` if *lowercased* string value of SETTING-OR-NAME is `true`
- * `false` if *lowercased* string value of SETTING-OR-NAME is `false`."
+ * `nil` if string value of `setting-or-name` is unset (or empty)
+ * `true` if *lowercased* string value of `setting-or-name` is `true`
+ * `false` if *lowercased* string value of `setting-or-name` is `false`."
^Boolean [setting-or-name]
(string->boolean (get-string setting-or-name)))
(defn get-integer
- "Get integer value of (presumably `:integer`) SETTING-OR-NAME. This is the default getter for `:integer` settings."
+ "Get integer value of (presumably `:integer`) `setting-or-name`. This is the default getter for `:integer` settings."
^Integer [setting-or-name]
(when-let [s (get-string setting-or-name)]
(Integer/parseInt s)))
(defn get-double
- "Get double value of (presumably `:double`) SETTING-OR-NAME. This is the default getter for `:double` settings."
+ "Get double value of (presumably `:double`) `setting-or-name`. This is the default getter for `:double` settings."
^Double [setting-or-name]
(when-let [s (get-string setting-or-name)]
(Double/parseDouble s)))
(defn get-json
- "Get the string value of SETTING-OR-NAME and parse it as JSON."
+ "Get the string value of `setting-or-name` and parse it as JSON."
[setting-or-name]
(json/parse-string (get-string setting-or-name) keyword))
@@ -181,7 +293,7 @@
:double get-double})
(defn get
- "Fetch the value of SETTING-OR-NAME. What this means depends on the Setting's `:getter`; by default, this looks for
+ "Fetch the value of `setting-or-name`. What this means depends on the Setting's `:getter`; by default, this looks for
first for a corresponding env var, then checks the cache, then returns the default value of the Setting, if any."
[setting-or-name]
((:getter (resolve-setting setting-or-name))))
@@ -191,7 +303,11 @@
;;; | set! |
;;; +----------------------------------------------------------------------------------------------------------------+
-(defn- update-setting! [setting-name new-value]
+(defn- update-setting!
+ "Update an existing Setting. Used internally by `set-string!` below; do not use directly."
+ [setting-name new-value]
+ (assert (not= setting-name settings-last-updated-key)
+ (tru "You cannot update `settings-last-updated` yourself! This is done automatically."))
;; This is indeed a very annoying way of having to do things, but `update-where!` doesn't call `pre-update` (in case
;; it updates thousands of objects). So we need to manually trigger `pre-update` behavior by calling `do-pre-update`
;; so that `value` can get encrypted if `MB_ENCRYPTION_SECRET_KEY` is in use. Then take that possibly-encrypted
@@ -201,8 +317,7 @@
:value maybe-encrypted-new-value)))
(defn- set-new-setting!
- "Insert a new row for a Setting with SETTING-NAME and SETTING-VALUE.
- Takes care of resetting the cache if the insert fails for some reason."
+ "Insert a new row for a Setting. Used internally by `set-string!` below; do not use directly."
[setting-name new-value]
(try (db/insert! Setting
:key setting-name
@@ -211,13 +326,15 @@
;; and there's actually a row in the DB that's not in the cache for some reason. Go ahead and update the
;; existing value and log a warning
(catch Throwable e
- (log/warn "Error INSERTing a new Setting:" (.getMessage e)
- "\nAssuming Setting already exists in DB and updating existing value.")
+ (log/warn (tru "Error inserting a new Setting:") "\n"
+ (.getMessage e) "\n"
+ (tru "Assuming Setting already exists in DB and updating existing value."))
(update-setting! setting-name new-value))))
(s/defn set-string!
- "Set string value of SETTING-OR-NAME. A `nil` or empty NEW-VALUE can be passed to unset (i.e., delete)
- SETTING-OR-NAME."
+ "Set string value of `setting-or-name`. A `nil` or empty `new-value` can be passed to unset (i.e., delete)
+ `setting-or-name`. String-type settings use this function directly; all other types ultimately call this (e.g.
+ `set-boolean!` eventually calls `set-string!`). Returns the `new-value`."
[setting-or-name, new-value :- (s/maybe s/Str)]
(let [new-value (when (seq new-value)
new-value)
@@ -235,11 +352,14 @@
(if new-value
(swap! cache assoc setting-name new-value)
(swap! cache dissoc setting-name))
+ ;; Record the fact that a Setting has been updated so eventaully other instances (if applicable) find out about it
+ (update-settings-last-updated!)
+ ;; Now return the `new-value`.
new-value))
(defn set-boolean!
- "Set the value of boolean SETTING-OR-NAME. NEW-VALUE can be nil, a boolean, or a string representation of one, such
- as `\"true\"` or `\"false\"` (these strings are case-insensitive)."
+ "Set the value of boolean `setting-or-name`. `new-value` can be nil, a boolean, or a string representation of one,
+ such as `\"true\"` or `\"false\"` (these strings are case-insensitive)."
[setting-or-name new-value]
(set-string! setting-or-name (if (string? new-value)
(set-boolean! setting-or-name (string->boolean new-value))
@@ -249,7 +369,7 @@
nil nil))))
(defn set-integer!
- "Set the value of integer SETTING-OR-NAME."
+ "Set the value of integer `setting-or-name`."
[setting-or-name new-value]
(set-string! setting-or-name (when new-value
(assert (or (integer? new-value)
@@ -258,7 +378,7 @@
(str new-value))))
(defn set-double!
- "Set the value of double SETTING-OR-NAME."
+ "Set the value of double `setting-or-name`."
[setting-or-name new-value]
(set-string! setting-or-name (when new-value
(assert (or (float? new-value)
@@ -267,7 +387,7 @@
(str new-value))))
(defn set-json!
- "Serialize NEW-VALUE for SETTING-OR-NAME as a JSON string and save it."
+ "Serialize `new-value` for `setting-or-name` as a JSON string and save it."
[setting-or-name new-value]
(set-string! setting-or-name (when new-value
(json/generate-string new-value))))
@@ -280,7 +400,7 @@
:double set-double!})
(defn set!
- "Set the value of SETTING-OR-NAME. What this means depends on the Setting's `:setter`; by default, this just updates
+ "Set the value of `setting-or-name`. What this means depends on the Setting's `:setter`; by default, this just updates
the Settings cache and writes its value to the DB.
(set :mandrill-api-key \"xyz123\")
@@ -297,7 +417,7 @@
;;; +----------------------------------------------------------------------------------------------------------------+
(defn register-setting!
- "Register a new `Setting` with a map of `SettingDefinition` attributes.
+ "Register a new Setting with a map of `SettingDefinition` attributes.
This is used internally be `defsetting`; you shouldn't need to use it yourself."
[{setting-name :name, setting-type :type, default :default, :as setting}]
(u/prog1 (let [setting-type (s/validate Type (or setting-type :string))]
@@ -326,21 +446,21 @@
;; Turn on auto-complete-mode in Emacs and see for yourself!
:doc (str/join "\n" [ description
""
- (format "`%s` is a %s `Setting`. You can get its value by calling:" (setting-name setting) (name setting-type))
+ (format "`%s` is a %s Setting. You can get its value by calling:" (setting-name setting) (name setting-type))
""
- (format " (%s)" (setting-name setting))
+ (format " (%s)" (setting-name setting))
""
"and set its value by calling:"
""
- (format " (%s <new-value>)" (setting-name setting))
+ (format " (%s <new-value>)" (setting-name setting))
""
- (format "You can also set its value with the env var `%s`." (env-var-name setting))
+ (format "You can also set its value with the env var `%s`." (env-var-name setting))
""
"Clear its value by calling:"
""
- (format " (%s nil)" (setting-name setting))
+ (format " (%s nil)" (setting-name setting))
""
- (format "Its default value is `%s`." (if (nil? default) "nil" default))])})
+ (format "Its default value is `%s`." (if (nil? default) "nil" default))])})
@@ -357,7 +477,7 @@
(metabase.models.setting/set! setting new-value))))
(defmacro defsetting
- "Defines a new `Setting` that will be added to the DB at some point in the future.
+ "Defines a new Setting that will be added to the DB at some point in the future.
Conveniently can be used as a getter/setter as well:
(defsetting mandrill-api-key \"API key for Mandrill.\")
@@ -373,7 +493,7 @@
* `:default` - The default value of the setting. (default: `nil`)
* `:type` - `:string` (default), `:boolean`, `:integer`, or `:json`. Non-`:string` settings have special
default getters and setters that automatically coerce values to the correct types.
- * `:internal?` - This `Setting` is for internal use and shouldn't be exposed in the UI (i.e., not returned by the
+ * `:internal?` - This Setting is for internal use and shouldn't be exposed in the UI (i.e., not returned by the
corresponding endpoints). Default: `false`
* `:getter` - A custom getter fn, which takes no arguments. Overrides the default implementation. (This can in
turn call functions in this namespace like `get-string` or `get-boolean` to invoke the default
@@ -397,7 +517,7 @@
;;; +----------------------------------------------------------------------------------------------------------------+
(defn set-many!
- "Set the value of several `Settings` at once.
+ "Set the value of several Settings at once.
(set-all {:mandrill-api-key \"xyz123\", :another-setting \"ABC\"})"
[settings]
diff --git a/src/metabase/util.clj b/src/metabase/util.clj
index dca42ebe128c2bcb31233c135a71ea401c10647b..34e204693edada38097b50be687eaee91bf1e061 100644
--- a/src/metabase/util.clj
+++ b/src/metabase/util.clj
@@ -4,9 +4,7 @@
[data :as data]
[pprint :refer [pprint]]
[string :as s]]
- [clojure.java
- [classpath :as classpath]
- [jdbc :as jdbc]]
+ [clojure.java.classpath :as classpath]
[clojure.math.numeric-tower :as math]
[clojure.tools.logging :as log]
[clojure.tools.namespace.find :as ns-find]
@@ -15,7 +13,6 @@
[puppetlabs.i18n.core :as i18n :refer [trs]]
[ring.util.codec :as codec])
(:import [java.net InetAddress InetSocketAddress Socket]
- java.sql.SQLException
[java.text Normalizer Normalizer$Form]))
;; This is the very first log message that will get printed. It's here because this is one of the very first
@@ -290,45 +287,6 @@
:when (re-find #"metabase" s)]
(s/replace s #"^metabase\." ""))))})
-(defn wrap-try-catch
- "Returns a new function that wraps F in a `try-catch`. When an exception is caught, it is logged
- with `log/error` and returns `nil`."
- ([f]
- (wrap-try-catch f nil))
- ([f f-name]
- (let [exception-message (if f-name
- (format "Caught exception in %s: " f-name)
- "Caught exception: ")]
- (fn [& args]
- (try
- (apply f args)
- (catch SQLException e
- (log/error (format-color 'red "%s\n%s\n%s"
- exception-message
- (with-out-str (jdbc/print-sql-exception-chain e))
- (pprint-to-str (filtered-stacktrace e)))))
- (catch Throwable e
- (log/error (format-color 'red "%s %s\n%s"
- exception-message
- (or (.getMessage e) e)
- (pprint-to-str (filtered-stacktrace e))))))))))
-
-(defn try-apply
- "Like `apply`, but wraps F inside a `try-catch` block and logs exceptions caught.
- (This is actaully more flexible than `apply` -- the last argument doesn't have to be
- a sequence:
-
- (try-apply vector :a :b [:c :d]) -> [:a :b :c :d]
- (apply vector :a :b [:c :d]) -> [:a :b :c :d]
- (try-apply vector :a :b :c :d) -> [:a :b :c :d]
- (apply vector :a :b :c :d) -> Not ok - :d is not a sequence
-
- This allows us to use `try-apply` in more situations than we'd otherwise be able to."
- [^clojure.lang.IFn f & args]
- (apply (wrap-try-catch f) (concat (butlast args) (if (sequential? (last args))
- (last args)
- [(last args)]))))
-
(defn deref-with-timeout
"Call `deref` on a FUTURE and throw an exception if it takes more than TIMEOUT-MS."
[futur timeout-ms]
diff --git a/src/metabase/util/date.clj b/src/metabase/util/date.clj
index 23b2775a420bca234a70dd8be031577966c467dd..2ad043a84d29ea060b1314144319add26eca8644 100644
--- a/src/metabase/util/date.clj
+++ b/src/metabase/util/date.clj
@@ -1,4 +1,5 @@
(ns metabase.util.date
+ "Utility functions for working with datetimes of different types, and other related tasks."
(:require [clj-time
[coerce :as coerce]
[core :as t]
diff --git a/src/metabase/util/encryption.clj b/src/metabase/util/encryption.clj
index bcd2624ad6dfd2930e7ca4d41573945867ca68a9..7f541c43dc42fa9ab07c5fe94841c0a3818232c9 100644
--- a/src/metabase/util/encryption.clj
+++ b/src/metabase/util/encryption.clj
@@ -87,7 +87,7 @@
(log/error
(trs "Cannot decrypt encrypted credentials. Have you changed or forgot to set MB_ENCRYPTION_SECRET_KEY?")
(.getMessage e)
- (u/filtered-stacktrace e))
+ (u/pprint-to-str (u/filtered-stacktrace e)))
;; otherwise return S without decrypting. It's probably not decrypted in the first place
s)))
s)))
diff --git a/test/metabase/models/setting_test.clj b/test/metabase/models/setting_test.clj
index b4a76c5eb2b5a7e05be85d31a77c8153cb8ef4de..c7c55ef06ac6b7eb920b6d8b3b4acf98c50f3def 100644
--- a/test/metabase/models/setting_test.clj
+++ b/test/metabase/models/setting_test.clj
@@ -1,8 +1,12 @@
(ns metabase.models.setting-test
- (:require [expectations :refer :all]
+ (:require [clojure.core.memoize :as memoize]
+ [expectations :refer :all]
+ [honeysql.core :as hsql]
+ [metabase
+ [db :as mdb]
+ [util :as u]]
[metabase.models.setting :as setting :refer [defsetting Setting]]
[metabase.test.util :refer :all]
- [metabase.util :as u]
[metabase.util
[encryption :as encryption]
[encryption-test :as encryption-test]]
@@ -283,7 +287,7 @@
;; restore the cache
((resolve 'metabase.models.setting/restore-cache-if-needed!))
;; now set a value for the `toucan-name` setting the wrong way
- (db/insert! setting/Setting {:key "toucan-name", :value "Rasta"})
+ (db/insert! setting/Setting {:key "toucan-name", :value "Reggae"})
;; ok, now try to set the Setting the correct way
(toucan-name "Banana Beak")
;; ok, make sure the setting was set
@@ -317,3 +321,126 @@
(encryption-test/with-secret-key nil
(toucan-name "Sad Can")
(actual-value-in-db :toucan-name)))
+
+
+;;; --------------------------------------------- Cache Synchronization ----------------------------------------------
+
+(def ^:private settings-last-updated-key @(resolve 'metabase.models.setting/settings-last-updated-key))
+
+(defn- clear-settings-last-updated-value-in-db! []
+ (db/simple-delete! Setting {:key settings-last-updated-key}))
+
+(defn- settings-last-updated-value-in-db []
+ (db/select-one-field :value Setting :key settings-last-updated-key))
+
+(defn- clear-cache! []
+ (reset! @(resolve 'metabase.models.setting/cache) nil))
+
+(defn- settings-last-updated-value-in-cache []
+ (get @@(resolve 'metabase.models.setting/cache) settings-last-updated-key))
+
+(defn- update-settings-last-updated-value-in-db!
+ "Simulate a different instance updating the value of `settings-last-updated` in the DB by updating its value without
+ updating our locally cached value.."
+ []
+ (db/update-where! Setting {:key settings-last-updated-key}
+ :value (hsql/raw (case (mdb/db-type)
+ ;; make it one second in the future so we don't end up getting an exact match when we try to test
+ ;; to see if things update below
+ :h2 "cast(dateadd('second', 1, current_timestamp) AS text)"
+ :mysql "cast((current_timestamp + interval 1 second) AS char)"
+ :postgres "cast((current_timestamp + interval '1 second') AS text)"))))
+
+(defn- simulate-another-instance-updating-setting! [setting-name new-value]
+ (db/update-where! Setting {:key (name setting-name)} :value new-value)
+ (update-settings-last-updated-value-in-db!))
+
+(defn- flush-memoized-results-for-should-restore-cache!
+ "Remove any memoized results for `should-restore-cache?`, so we can test `restore-cache-if-needed!` works the way we'd
+ expect."
+ []
+ (memoize/memo-clear! @(resolve 'metabase.models.setting/should-restore-cache?)))
+
+;; When I update a Setting, does it set/update `settings-last-updated`?
+(expect
+ (do
+ (clear-settings-last-updated-value-in-db!)
+ (toucan-name "Bird Can")
+ (string? (settings-last-updated-value-in-db))))
+
+;; ...and is the value updated in the cache as well?
+(expect
+ (do
+ (clear-cache!)
+ (toucan-name "Bird Can")
+ (string? (settings-last-updated-value-in-cache))))
+
+;; ...and if I update it again, will the value be updated?
+(expect
+ (do
+ (clear-settings-last-updated-value-in-db!)
+ (toucan-name "Bird Can")
+ (let [first-value (settings-last-updated-value-in-db)]
+ ;; MySQL only has the resolution of one second on the timestamps here so we should wait that long to make sure
+ ;; the second-value actually ends up being greater than the first
+ (Thread/sleep 1200)
+ (toucan-name "Bird Can")
+ (let [second-value (settings-last-updated-value-in-db)]
+ ;; first & second values should be different, and first value should be "less than" the second value
+ (and (not= first-value second-value)
+ (neg? (compare first-value second-value)))))))
+
+;; If there is no cache, it should be considered out of date!`
+(expect
+ (do
+ (clear-cache!)
+ (#'setting/cache-out-of-date?)))
+
+;; But if I set a setting, it should cause the cache to be populated, and be up-to-date
+(expect
+ false
+ (do
+ (clear-cache!)
+ (toucan-name "Reggae Toucan")
+ (#'setting/cache-out-of-date?)))
+
+;; If another instance updates a Setting, `cache-out-of-date?` should return `true` based on DB comparisons...
+;; be true!
+(expect
+ (do
+ (clear-cache!)
+ (toucan-name "Reggae Toucan")
+ (simulate-another-instance-updating-setting! :toucan-name "Bird Can")
+ (#'setting/cache-out-of-date?)))
+
+;; of course, `restore-cache-if-needed!` should use TTL memoization, and the cache should not get updated right away
+;; even if another instance updates a value...
+(expect
+ "Sam"
+ (do
+ (flush-memoized-results-for-should-restore-cache!)
+ (clear-cache!)
+ (toucan-name "Sam") ; should restore cache, and put in {"toucan-name" "Sam"}
+ ;; since we cleared the memoized value of `should-restore-cache?` call it again to make sure it gets set to
+ ;; `false` as it would IRL if we were calling it again from the same instance
+ (#'setting/should-restore-cache?)
+ ;; now have another instance change the value
+ (simulate-another-instance-updating-setting! :toucan-name "Bird Can")
+ ;; our cache should not be updated yet because it's on a TTL
+ (toucan-name)))
+
+;; ...and when it comes time to check our cache for updating (when calling `restore-cache-if-needed!`, it should get
+;; the updated value. (we're not actually going to wait a minute for the memoized values of `should-restore-cache?` to
+;; be invalidated, so we will manually flush the memoization cache to simulate it happening)
+(expect
+ "Bird Can"
+ (do
+ (clear-cache!)
+ (toucan-name "Reggae Toucan")
+ (simulate-another-instance-updating-setting! :toucan-name "Bird Can")
+ (flush-memoized-results-for-should-restore-cache!)
+ ;; calling `toucan-name` will call `restore-cache-if-needed!`, which will in turn call `should-restore-cache?`.
+ ;; Since memoized value is no longer present, this should call `cache-out-of-date?`, which checks the DB; it will
+ ;; detect a cache out-of-date situation and flush the cache as appropriate, giving us the updated value when we
+ ;; call! :wow:
+ (toucan-name)))