From 8c0d9ab1d07ce460efba57d4ca5c37315209c59e Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Mon, 1 Jul 2024 08:21:13 -0600
Subject: [PATCH] Memoize driver.u/supports? only in prod (#44938)
---
src/metabase/driver/util.clj | 30 +++++++++++++++++--------
test/metabase/driver/util_test.clj | 35 +++++++++++++++---------------
2 files changed, 39 insertions(+), 26 deletions(-)
diff --git a/src/metabase/driver/util.clj b/src/metabase/driver/util.clj
index 23e4b70bbb4..bbfa7261b86 100644
--- a/src/metabase/driver/util.clj
+++ b/src/metabase/driver/util.clj
@@ -215,19 +215,31 @@
critical metabase features that use this check."
5000)
-(def ^{:arglists '([driver feature database])} supports?
+(def ^:dynamic *memoize-supports?*
+ "If true, [[supports?]] is memoized for the application DB. Memoization is disabled in dev and test mode by default to avoid
+ accidental coupling between tests."
+ (not (or config/is-test? config/is-dev?)))
+
+(def ^:private supports?*
+ (fn [driver feature database]
+ (try
+ (u/with-timeout supports?-timeout-ms
+ (driver/database-supports? driver feature database))
+ (catch Throwable e
+ (log/error e (u/format-color 'red "Failed to check feature '%s' for database '%s'" (name feature) (:name database)))
+ false))))
+
+(def ^:private memoized-supports?*
+ (mdb/memoize-for-application-db supports?*))
+
+(defn supports?
"A defensive wrapper around [[database-supports?]]. It adds logging, caching, and error handling to avoid crashing the app
if this method takes a long time to execute or throws an exception. This is useful because `supports?` is used in so many
critical places in the app, and we don't want a single driver to crash the app if it throws an exception, or delay the user
if it takes a long time to execute."
- (mdb/memoize-for-application-db ; memoize because this can be called in a tight loop, and will only change if the database changes versions
- (fn [driver feature database]
- (try
- (u/with-timeout supports?-timeout-ms
- (driver/database-supports? driver feature database))
- (catch Throwable e
- (log/error e (u/format-color 'red "Failed to check feature '%s' for database '%s'" (name feature) (:name database)))
- false)))))
+ [driver feature database]
+ (let [f (if *memoize-supports?* memoized-supports?* supports?*)]
+ (f driver feature database)))
(defn features
"Return a set of all features supported by `driver` with respect to `database`."
diff --git a/test/metabase/driver/util_test.clj b/test/metabase/driver/util_test.clj
index 2d5bb4c49be..41a4faa0d67 100644
--- a/test/metabase/driver/util_test.clj
+++ b/test/metabase/driver/util_test.clj
@@ -290,24 +290,25 @@
(with-redefs [driver/database-supports? (fn [_ _ _] (throw (Exception. "test exception message")))]
(let [db (assoc fake-test-db :name (mt/random-name))
log-messages (mt/with-log-messages-for-level [metabase.driver.util :error]
- (is (false? (driver.u/supports? :test-driver :test-feature db))))]
+ (is (false? (driver.u/supports? :test-driver :expressions db))))]
(is (some (fn [[level ^Throwable exception message]]
(and (= level :error)
(= (.getMessage exception) "test exception message")
- (= message (u/format-color 'red "Failed to check feature 'test-feature' for database '%s'" (:name db)))))
+ (= message (u/format-color 'red "Failed to check feature 'expressions' for database '%s'" (:name db)))))
log-messages)))))
- (testing "supports? returns false when `driver/database-supports?` takes longer than the timeout"
- (let [db (assoc fake-test-db :name (mt/random-name))]
- (with-redefs [driver.u/supports?-timeout-ms 100
- driver/database-supports? (fn [_ _ _] (Thread/sleep 200) true)]
- (let [log-messages (mt/with-log-messages-for-level [metabase.driver.util :error]
- (is (false? (driver.u/supports? :test-driver :test-feature db))))]
- (is (some (fn [[level ^Throwable exception message]]
- (and (= level :error)
- (= (.getMessage exception) "Timed out after 100.0 ms")
- (= message (u/format-color 'red "Failed to check feature 'test-feature' for database '%s'" (:name db)))))
- log-messages))))
- (testing "we memoize the results for the same database, so we don't log the error again"
- (let [log-messages (mt/with-log-messages-for-level [metabase.driver.util :error]
- (is (false? (driver.u/supports? :test-driver :test-feature db))))]
- (is (nil? log-messages))))))))
+ (binding [driver.u/*memoize-supports?* true]
+ (testing "supports? returns false when `driver/database-supports?` takes longer than the timeout"
+ (let [db (assoc fake-test-db :name (mt/random-name))]
+ (with-redefs [driver.u/supports?-timeout-ms 100
+ driver/database-supports? (fn [_ _ _] (Thread/sleep 200) true)]
+ (let [log-messages (mt/with-log-messages-for-level [metabase.driver.util :error]
+ (is (false? (driver.u/supports? :test-driver :expressions db))))]
+ (is (some (fn [[level ^Throwable exception message]]
+ (and (= level :error)
+ (= (.getMessage exception) "Timed out after 100.0 ms")
+ (= message (u/format-color 'red "Failed to check feature 'expressions' for database '%s'" (:name db)))))
+ log-messages))))
+ (testing "we memoize the results for the same database, so we don't log the error again"
+ (let [log-messages (mt/with-log-messages-for-level [metabase.driver.util :error]
+ (is (false? (driver.u/supports? :test-driver :expressions db))))]
+ (is (nil? log-messages)))))))))
--
GitLab