RFC: kondo lint to enforce naming convention for thread not safe defn/defmacro in tests

.clj-kondo/config.edn

@@ -595,11 +595,11 @@
    metabase.related-test/with-world                                             macros.metabase.related-test/with-world
    metabase.shared.util.namespaces/import-fn                                    macros.metabase.shared.util.namespaces/import-fn
    metabase.test.data.users/with-group-for-user                                 macros.metabase.test.data.users/with-group-for-user
-   metabase.test.util/with-temp-env-var-value                                   macros.metabase.test.util/with-temp-env-var-value
+   metabase.test.util/with-temp-env-var-value!                                  macros.metabase.test.util/with-temp-env-var-value!
    metabase.test.util/with-temporary-raw-setting-values                         macros.metabase.test.util/with-temporary-raw-setting-values
    metabase.test/with-group-for-user                                            macros.metabase.test.data.users/with-group-for-user
    metabase.test/with-persistence-enabled                                       macros.metabase.test.persistence/with-persistence-enabled
-   metabase.test/with-temp-env-var-value                                        macros.metabase.test.util/with-temp-env-var-value
+   metabase.test/with-temp-env-var-value!                                       macros.metabase.test.util/with-temp-env-var-value!
    metabase.test/with-temporary-raw-setting-values                              macros.metabase.test.util/with-temporary-raw-setting-values}}
 
  :config-in-comment
@@ -635,10 +635,16 @@
  :config-in-ns
  {test-namespaces
   {:linters
-   {:inline-def                                    {:level :off}
-    :missing-docstring                             {:level :off}
-    :private-call                                  {:level :off}
-    :hooks.metabase.test.data/mbql-query-first-arg {:level :error}}}
+   {:inline-def                                                 {:level :off}
+    :missing-docstring                                          {:level :off}
+    :private-call                                               {:level :off}
+    :hooks.metabase.test.data/mbql-query-first-arg              {:level :error}
+    :metabase/defmacro-in-test-contains-unthread-safe-functions {:level :warning}}
+   :hooks
+   {:analyze-call
+    {clojure.core/defmacro hooks.clojure.core/unsafe-forms-should-end-with-exclamation
+     clojure.core/defn     hooks.clojure.core/unsafe-forms-should-end-with-exclamation
+     clojure.core/defn-    hooks.clojure.core/unsafe-forms-should-end-with-exclamation}}}
 
   source-namespaces
   {:linters

.clj-kondo/hooks/clojure/core.clj

@@ -0,0 +1,72 @@
+(ns hooks.clojure.core
+  (:require
+   [clj-kondo.hooks-api :as hooks]
+   [clojure.string :as str]))
+
+(defn- node->qualified-symbol [node]
+  (try
+    (when (hooks/token-node? node)
+      (let [sexpr (hooks/sexpr node)]
+        (when (symbol? sexpr)
+          (when-let [resolved (hooks/resolve {:name sexpr})]
+            (symbol (name (:ns resolved)) (name (:name resolved)))))))
+    ;; some symbols like `*count/Integer` aren't resolvable.
+    (catch Exception _
+      nil)))
+
+(def ^:private white-card-symbols
+  '#{;; these toucan methods might actually set global values if it's used outside of a transaction,
+     ;; but since mt/with-temp runs in a transaction, so we'll ignore them in this case.
+     toucan2.core/delete!
+     toucan2.core/update!
+     toucan2.core/insert!})
+
+(defn- end-with-exclamation?
+  [s]
+  (str/ends-with? s "!"))
+
+(defn- unsafe-forms-should-end-with-exclamation*
+  [{[defn-or-defmacro form-name] :children, :as node}]
+  (when-not (end-with-exclamation? (:string-value form-name))
+    (letfn [(walk [f form]
+              (f form)
+              (doseq [child (:children form)]
+                (walk f child)))]
+      (walk (fn [form]
+              (when-let [qualified-symbol (node->qualified-symbol form)]
+                (when (and (not (contains? white-card-symbols qualified-symbol))
+                           (end-with-exclamation? qualified-symbol))
+                  (hooks/reg-finding! (assoc (meta form-name)
+                                             :message (format "The name of this %s should end with `!` because it contains calls to not thread safe form `%s`."
+                                                              (:string-value defn-or-defmacro) qualified-symbol)
+                                             :type :metabase/defmacro-in-test-contains-unthread-safe-functions)))))
+            node))
+    node))
+
+(defn unsafe-forms-should-end-with-exclamation
+  "Used to ensure defn and defmacro in test namespace to have name ending with `!` if it's not thread-safe.
+
+  A function or a macro can be defined as 'not thread safe' when their funciton name ends with a `!`."
+  [{:keys [node cljc lang]}]
+  (when (or (not cljc)
+            (= lang :clj))
+    (unsafe-forms-should-end-with-exclamation* node))
+  {:node node})
+
+(comment
+ (require '[clj-kondo.core :as clj-kondo])
+ (def form (str '(defmacro a
+                   [x]
+                   `(fun-call x))))
+
+ (def form "(defmacro a
+           [x]
+           `(some! ~x))")
+ (str (hooks/parse-string form))
+ (hooks/sexpr (hooks/parse-string form))
+ (binding [hooks/*reload* true]
+    (-> form
+        (with-in-str (clj-kondo/run! {:lint ["-"]}))
+        :findings))
+ (do (unsafe-forms-should-end-with-exclamation* (hooks/parse-string form))
+     nil))

.clj-kondo/macros/metabase/test/util.clj

@@ -1,11 +1,11 @@
 (ns macros.metabase.test.util)
 
-(defmacro with-temp-env-var-value [bindings & body]
+(defmacro with-temp-env-var-value! [bindings & body]
   `((constantly nil)
     ~@(map second (partition-all 2 bindings))
     ~@body))
 
 (defmacro with-temporary-raw-setting-values [bindings & body]
   `((constantly nil)
-     ~@(map second (partition-all 2 bindings))
+    ~@(map second (partition-all 2 bindings))
      ~@body))

enterprise/backend/src/metabase_enterprise/advanced_config/file.clj

@@ -150,7 +150,7 @@
   "Environment variables and system properties used in this namespace. This is a dynamic version
   of [[environ.core/env]]; it is dynamic for test mocking purposes.
 
-  Yes, [[metabase.test/with-temp-env-var-value]] exists, but it is not allowed inside parallel tests. This is an
+  Yes, [[metabase.test/with-temp-env-var-value!]] exists, but it is not allowed inside parallel tests. This is an
   experiment that I may adapt into a new pattern in the future to allow further test parallelization."
   env/env)
 

enterprise/backend/test/metabase_enterprise/audit_db_test.clj

@@ -21,7 +21,7 @@
   "Calls `ensure-audit-db-installed!` before and after `body` to ensure that the audit DB is installed and then
   restored if necessary. Also disables audit content loading if it is already loaded."
   `(let [audit-collection-exists?# (t2/exists? :model/Collection :type "instance-analytics")]
-     (mt/with-temp-env-var-value [mb-load-analytics-content (not audit-collection-exists?#)]
+     (mt/with-temp-env-var-value! [mb-load-analytics-content (not audit-collection-exists?#)]
        (mbc/ensure-audit-db-installed!)
        (try
          ~@body
@@ -69,7 +69,7 @@
         (t2/update! Database :is_audit true {:engine "h2"})))))
 
 (deftest instance-analytics-content-is-copied-to-mb-plugins-dir-test
-  (mt/with-temp-env-var-value [mb-plugins-dir "card_catalogue_dir"]
+  (mt/with-temp-env-var-value! [mb-plugins-dir "card_catalogue_dir"]
     (try
      (let [plugins-dir (plugins/plugins-dir)]
        (fs/create-dirs plugins-dir)
@@ -82,7 +82,7 @@
        (fs/delete-tree (plugins/plugins-dir))))))
 
 (deftest all-instance-analytics-content-is-copied-from-mb-plugins-dir-test
-  (mt/with-temp-env-var-value [mb-plugins-dir "card_catalogue_dir"]
+  (mt/with-temp-env-var-value! [mb-plugins-dir "card_catalogue_dir"]
     (try
       (#'audit-db/ia-content->plugins (plugins/plugins-dir))
       (is (= (count (file-seq (io/file (str (fs/path (plugins/plugins-dir) "instance_analytics")))))

enterprise/backend/test/metabase_enterprise/public_settings_test.clj

@@ -39,7 +39,7 @@
              (embed.settings/embedding-app-origin! "https://metabase.com")))
 
         (testing "even if env is set, return the default value"
-          (mt/with-temp-env-var-value [mb-embedding-app-origin "https://metabase.com"]
+          (mt/with-temp-env-var-value! [mb-embedding-app-origin "https://metabase.com"]
             (is (nil? (embed.settings/embedding-app-origin)))))))
 
     (testing "can change embedding-app-origin if :embedding is enabled"
@@ -49,6 +49,6 @@
                (embed.settings/embedding-app-origin)))
 
         (testing "it works with env too"
-          (mt/with-temp-env-var-value [mb-embedding-app-origin "ssh://metabase.com"]
+          (mt/with-temp-env-var-value! [mb-embedding-app-origin "ssh://metabase.com"]
             (is (= "ssh://metabase.com"
                    (embed.settings/embedding-app-origin)))))))))

enterprise/backend/test/metabase_enterprise/task/truncate_audit_tables_test.clj

@@ -38,7 +38,7 @@
         ;; Mock a cloud environment so that we can change the setting value via env var
         (with-redefs [premium-features/is-hosted? (constantly true)]
           (testing "When the threshold is 0 (representing infinity), no rows are deleted"
-            (mt/with-temp-env-var-value [mb-audit-max-retention-days 0]
+            (mt/with-temp-env-var-value! [mb-audit-max-retention-days 0]
               (#'task.truncate-audit-tables/truncate-audit-tables!)
               (is (= #{qe1-id qe2-id qe3-id}
                      (t2/select-fn-set :id :model/QueryExecution {:where [:in :id [qe1-id qe2-id qe3-id]]}))))))))))
@@ -60,7 +60,7 @@
         ;; Mock a cloud environment so that we can change the setting value via env var
         (with-redefs [premium-features/is-hosted? (constantly true)]
           (testing "When the threshold is 30 days, two rows are deleted"
-            (mt/with-temp-env-var-value [mb-audit-max-retention-days 30]
+            (mt/with-temp-env-var-value! [mb-audit-max-retention-days 30]
               (#'task.truncate-audit-tables/truncate-audit-tables!)
               (is (= #{al1-id}
                      (t2/select-fn-set :id :model/AuditLog {:where [:in :id [al1-id al2-id al3-id]]}))))))))))
@@ -83,7 +83,7 @@
         ;; Mock a cloud environment so that we can change the setting value via env var
         (with-redefs [premium-features/is-hosted? (constantly true)]
           (testing "When the threshold is 30 days, two rows are deleted"
-            (mt/with-temp-env-var-value [mb-audit-max-retention-days 30]
+            (mt/with-temp-env-var-value! [mb-audit-max-retention-days 30]
               (#'task.truncate-audit-tables/truncate-audit-tables!)
               (is (= #{vl1-id}
                      (t2/select-fn-set :id :model/ViewLog {:where [:in :id [vl1-id vl2-id vl3-id]]}))))))))))

test/metabase/api/dashboard_test.clj

@@ -1961,7 +1961,7 @@
       (mt/with-actions-test-data
         (doseq [enable-actions? [true false]
                 encrypt-db?     [true false]]
-          (mt/with-temp-env-var-value [mb-encryption-secret-key encrypt-db?]
+          (mt/with-temp-env-var-value! [mb-encryption-secret-key encrypt-db?]
             (mt/with-temp-vals-in-db Database (mt/id) {:settings {:database-enable-actions enable-actions?}}
               (mt/with-actions [{:keys [action-id]} {:type :query :visualization_settings {:hello true}}]
                 (mt/with-temp [Dashboard     {dashboard-id :id} {}

test/metabase/api/email_test.clj

@@ -112,12 +112,12 @@
                            :email-from-address)]
           (tu/discard-setting-changes [email-smtp-host email-smtp-port email-smtp-security email-smtp-username
                                        email-smtp-password email-from-address email-from-name email-reply-to]
-            (mt/with-temp-env-var-value [mb-email-smtp-port     (:email-smtp-port default-email-settings)
-                                         mb-email-smtp-host     (:email-smtp-host default-email-settings)
-                                         mb-email-smtp-security (name (:email-smtp-security default-email-settings))
-                                         mb-email-smtp-username (:email-smtp-username default-email-settings)
-                                         mb-email-smtp-password (:email-smtp-password default-email-settings)
-                                         mb-email-from-address  (:email-from-address default-email-settings)]
+            (mt/with-temp-env-var-value! [mb-email-smtp-port     (:email-smtp-port default-email-settings)
+                                          mb-email-smtp-host     (:email-smtp-host default-email-settings)
+                                          mb-email-smtp-security (name (:email-smtp-security default-email-settings))
+                                          mb-email-smtp-username (:email-smtp-username default-email-settings)
+                                          mb-email-smtp-password (:email-smtp-password default-email-settings)
+                                          mb-email-from-address  (:email-from-address default-email-settings)]
               (with-redefs [email/test-smtp-settings (constantly {::email/error nil})]
                 (testing "API request"
                   (is (= (-> default-email-settings

test/metabase/api/geojson_test.clj

@@ -215,7 +215,7 @@
                              :region_name "NAME"}}
             expected-value (merge @#'api.geojson/builtin-geojson custom-geojson)]
         (mt/with-temporary-setting-values [custom-geojson nil]
-          (mt/with-temp-env-var-value [mb-custom-geojson (json/generate-string custom-geojson)]
+          (mt/with-temp-env-var-value! [mb-custom-geojson (json/generate-string custom-geojson)]
             (binding [setting/*disable-cache* true]
               (testing "Should parse env var custom GeoJSON and merge in"
                 (is (= expected-value
@@ -240,7 +240,7 @@
 (deftest disable-custom-geojson-test
   (testing "Should be able to disable GeoJSON proxying endpoints by env var"
     (mt/with-temporary-setting-values [custom-geojson test-custom-geojson]
-      (mt/with-temp-env-var-value [mb-custom-geojson-enabled false]
+      (mt/with-temp-env-var-value! [mb-custom-geojson-enabled false]
         (testing "Should not be able to fetch GeoJSON via URL proxy endpoint"
           (is (= "Custom GeoJSON is not enabled"
                  (mt/user-real-request :crowberto :get 400 "geojson" :url test-geojson-url))))

test/metabase/api/metabot_test.clj

@@ -13,7 +13,7 @@
 
 (deftest metabot-only-works-on-models-test
   (testing "POST /api/metabot/model/:model-id won't work for a table endpoint"
-    (mt/with-temp-env-var-value [mb-is-metabot-enabled true]
+    (mt/with-temp-env-var-value! [mb-is-metabot-enabled true]
       (mt/dataset test-data
         (let [q        "At what time was the status closed for each user?"
               response (mt/user-http-request :rasta :post 404
@@ -23,7 +23,7 @@
 
 (deftest metabot-model-happy-path-test
   (testing "POST /api/metabot/model/:model-id happy path"
-    (mt/with-temp-env-var-value [mb-is-metabot-enabled true]
+    (mt/with-temp-env-var-value! [mb-is-metabot-enabled true]
       (mt/dataset test-data
         (t2.with-temp/with-temp
           [Card orders-model {:name    "Orders Model"
@@ -52,7 +52,7 @@
 
 (deftest metabot-model-sad-path-test
   (testing "POST /api/metabot/model/:model-id produces a message when no SQL is found"
-    (mt/with-temp-env-var-value [mb-is-metabot-enabled true]
+    (mt/with-temp-env-var-value! [mb-is-metabot-enabled true]
       (mt/dataset test-data
         (t2.with-temp/with-temp
           [Card orders-model {:name    "Orders Model"
@@ -73,7 +73,7 @@
 
 (deftest metabot-database-happy-path-test
   (testing "POST /api/metabot/database/:database-id happy path"
-    (mt/with-temp-env-var-value [mb-is-metabot-enabled true]
+    (mt/with-temp-env-var-value! [mb-is-metabot-enabled true]
       (mt/dataset test-data
         (t2.with-temp/with-temp
           [Card orders-model {:name    "Orders Model"
@@ -103,7 +103,7 @@
 
 (deftest metabot-database-no-model-found-test
   (testing "With embeddings, you'll always get _some_ model, unless there aren't any at all."
-    (mt/with-temp-env-var-value [mb-is-metabot-enabled true]
+    (mt/with-temp-env-var-value! [mb-is-metabot-enabled true]
       (mt/dataset test-data
         (t2.with-temp/with-temp
           [Card _orders-model {:name    "Not a model"
@@ -124,7 +124,7 @@
 
 (deftest metabot-database-no-sql-found-test
   (testing "When we can't find sql from the selected model, we return a message"
-    (mt/with-temp-env-var-value [mb-is-metabot-enabled true]
+    (mt/with-temp-env-var-value! [mb-is-metabot-enabled true]
       (mt/dataset test-data
         (t2.with-temp/with-temp
           [Card orders-model {:name    "Orders Model"
@@ -147,7 +147,7 @@
 
 (deftest openai-40X-test
   ;; We can use the metabot-client/bot-endpoint redefs to simulate various failure modes in the bot server
-  (mt/with-temp-env-var-value [mb-is-metabot-enabled true]
+  (mt/with-temp-env-var-value! [mb-is-metabot-enabled true]
     (testing "Too many requests returns a useful message"
       (mt/dataset test-data
         (t2.with-temp/with-temp
@@ -217,7 +217,7 @@
 
 (deftest metabot-infer-native-sql-test
   (testing "POST /database/:database-id/query"
-    (mt/with-temp-env-var-value [mb-is-metabot-enabled true]
+    (mt/with-temp-env-var-value! [mb-is-metabot-enabled true]
       (mt/dataset test-data
         (mt/with-temp [Card _orders-model {:name    "Orders Model"
                                            :dataset_query

test/metabase/api/session_test.clj

@@ -389,23 +389,23 @@
 (deftest reset-token-ttl-hours-test
   (testing "Test reset-token-ttl-hours-test"
     (testing "reset-token-ttl-hours-test is reset to default when not set"
-      (mt/with-temp-env-var-value [mb-reset-token-ttl-hours nil]
+      (mt/with-temp-env-var-value! [mb-reset-token-ttl-hours nil]
         (is (= 48 (setting/get-value-of-type :integer :reset-token-ttl-hours)))))
 
     (testing "reset-token-ttl-hours-test is set to positive value"
-      (mt/with-temp-env-var-value [mb-reset-token-ttl-hours 36]
+      (mt/with-temp-env-var-value! [mb-reset-token-ttl-hours 36]
         (is (= 36 (setting/get-value-of-type :integer :reset-token-ttl-hours)))))
 
     (testing "reset-token-ttl-hours-test is set to large positive value"
-      (mt/with-temp-env-var-value [mb-reset-token-ttl-hours (+ Integer/MAX_VALUE 1)]
+      (mt/with-temp-env-var-value! [mb-reset-token-ttl-hours (+ Integer/MAX_VALUE 1)]
         (is (= (+ Integer/MAX_VALUE 1) (setting/get-value-of-type :integer :reset-token-ttl-hours)))))
 
     (testing "reset-token-ttl-hours-test is set to zero"
-      (mt/with-temp-env-var-value [mb-reset-token-ttl-hours 0]
+      (mt/with-temp-env-var-value! [mb-reset-token-ttl-hours 0]
         (is (= 0 (setting/get-value-of-type :integer :reset-token-ttl-hours)))))
 
     (testing "reset-token-ttl-hours-test is set to negative value"
-      (mt/with-temp-env-var-value [mb-reset-token-ttl-hours -1]
+      (mt/with-temp-env-var-value! [mb-reset-token-ttl-hours -1]
         (is (= -1 (setting/get-value-of-type :integer :reset-token-ttl-hours)))))))
 
 (deftest properties-test

test/metabase/api/setup_test.clj

@@ -603,15 +603,15 @@
 
 (deftest user-defaults-test
   (testing "with no user defaults configured"
-    (mt/with-temp-env-var-value [mb-user-defaults nil]
+    (mt/with-temp-env-var-value! [mb-user-defaults nil]
       (is (= "Not found." (client/client :get "setup/user_defaults")))))
 
   (testing "with defaults containing no token"
-    (mt/with-temp-env-var-value [mb-user-defaults "{}"]
+    (mt/with-temp-env-var-value! [mb-user-defaults "{}"]
       (is (= "Not found." (client/client :get "setup/user_defaults")))))
 
   (testing "with valid configuration"
-    (mt/with-temp-env-var-value [mb-user-defaults "{\"token\":\"123456\",\"email\":\"john.doe@example.com\"}"]
+    (mt/with-temp-env-var-value! [mb-user-defaults "{\"token\":\"123456\",\"email\":\"john.doe@example.com\"}"]
       (testing "with mismatched token"
         (is (= "You don't have permissions to do that." (client/client :get "setup/user_defaults?token=987654"))))
       (testing "with valid token"