RFC: kondo lint to enforce naming convention for thread not safe defn…

…/defmacro in tests (#37126)

.clj-kondo/config.edn

@@ -597,11 +597,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
@@ -637,11 +637,17 @@
  :config-in-ns
  {test-namespaces
   {:linters
-   {:inline-def                                    {:level :off}
-    :missing-docstring                             {:level :off}
-    :private-call                                  {:level :off}
-    :metabase/defsetting-must-specify-export       {:level :off}
-    :hooks.metabase.test.data/mbql-query-first-arg {:level :error}}}
+   {:inline-def                                          {:level :off}
+    :missing-docstring                                   {:level :off}
+    :private-call                                        {:level :off}
+    :metabase/defsetting-must-specify-export             {:level :off}
+    :hooks.metabase.test.data/mbql-query-first-arg       {:level :error}
+    :metabase/test-helpers-use-non-thread-safe-functions {:level :warning}}
+   :hooks
+   {:analyze-call
+    {clojure.core/defmacro hooks.clojure.core/non-thread-safe-form-should-end-with-exclamation
+     clojure.core/defn     hooks.clojure.core/non-thread-safe-form-should-end-with-exclamation
+     clojure.core/defn-    hooks.clojure.core/non-thread-safe-form-should-end-with-exclamation}}}
 
   source-namespaces
   {:linters

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

@@ -0,0 +1,198 @@
+(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)
+         (let [resolved (hooks/resolve {:name sexpr})]
+           (when-not (= :clj-kondo/unknown-namespace (:ns resolved))
+             (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!
+     toucan2.core/insert-returning-instances!
+     toucan2.core/insert-returning-pks!
+     clojure.core.async/<!!
+     clojure.core.async/>!!
+     clojure.core.async/alts!!
+     clojure.core.async/close!
+     clojure.core.async/poll!
+     clojure.core.memoize/memo-clear!
+     clojure.core/conj!
+     clojure.core/persistent!
+     clojure.core/reset!
+     clojure.core/swap!
+     clojure.core/volatile!
+     clojure.core/vreset!
+     clojure.core/vswap!
+     clojure.java.jdbc/execute!
+     methodical.core/add-aux-method-with-unique-key!
+     methodical.core/remove-aux-method-with-unique-key!
+     next.jdbc/execute!
+
+     ;; TODO: most of these symbols shouldn't be here, we should go through them and
+     ;; find the functions/macros that use them and make sure their names end with !
+     ;; best way to do this is try remove each of these and rely on kondo output to find places where it's used
+     clojure.test/grant-collection-perms!
+     clojure.test/grant-collection-perms-fn!
+     clojure.test/grant-perms-fn!
+     clojure.test/purge-old-entries!
+     clojure.test/revoke-collection-perms!
+     clojure.test/save-results!
+     metabase-enterprise.advanced-permissions.models.permissions/update-db-download-permissions!
+     metabase-enterprise.internal-user/install-internal-user!
+     metabase-enterprise.sso.integrations.saml-test/call-with-login-attributes-cleared!
+     metabase.actions/perform-action!
+     metabase.analytics.snowplow-test/fake-track-event-impl!
+     metabase.analytics.snowplow/track-event-impl!
+     metabase.api.public-test/add-card-to-dashboard!
+     metabase.cmd.dump-to-h2/dump-to-h2!
+     metabase.cmd.load-from-h2/load-from-h2!
+     metabase.core/ensure-audit-db-installed!
+     metabase.db.schema-migrations-test.impl/run-migrations-in-range!
+     metabase.db.setup/migrate!
+     metabase.db.setup/setup-db!
+     metabase.db/setup-db!
+     metabase.driver.mongo-test/create-database-from-row-maps!
+     metabase.driver.postgres-test/create-enums-db!
+     metabase.driver.postgres-test/drop-if-exists-and-create-db!
+     metabase.driver.sql-jdbc.execute/execute-statement!
+     metabase.email-test/reset-inbox!
+     metabase.email/send-email!
+     metabase.models.action/insert!
+     metabase.models.collection.graph-test/clear-graph-revisions!
+     metabase.models.collection.graph-test/do-with-n-temp-users-with-personal-collections!
+     metabase.models.field-values/create-or-update-full-field-values!
+     metabase.models.model-index/add-values!
+     metabase.models.moderation-review/create-review!
+     metabase.models.on-demand-test/add-dashcard-with-parameter-mapping!
+     metabase.models.permissions/grant-application-permissions!
+     metabase.models.permissions/grant-collection-read-permissions!
+     metabase.models.permissions/grant-collection-readwrite-permissions!
+     metabase.models.permissions/grant-full-data-permissions!
+     metabase.models.permissions/grant-native-readwrite-permissions!
+     metabase.models.permissions/grant-permissions!
+     metabase.models.permissions/revoke-application-permissions!
+     metabase.models.permissions/revoke-data-perms!
+     metabase.models.permissions/update-data-perms-graph!
+     metabase.models.permissions/update-group-permissions!
+     metabase.models.persisted-info/ready-database!
+     metabase.models.revision/revert!
+     metabase.models.setting-test/test-user-local-allowed-setting!
+     metabase.models.setting-test/test-user-local-only-setting!
+     metabase.models.setting.cache/restore-cache!
+     metabase.models.setting/set!
+     metabase.models.setting/validate-settings-formatting!
+     metabase.permissions.test-util/with-restored-perms!
+     metabase.pulse/send-notifications!
+     metabase.pulse/send-pulse!
+     metabase.query-processor.streaming.interface/begin!
+     metabase.query-processor.streaming.interface/finish!
+     metabase.query-processor.streaming.interface/write-row!
+     metabase.sample-data/try-to-extract-sample-database!
+     metabase.setup/create-token!
+     metabase.sync.sync-metadata.fields.sync-metadata/update-field-metadata-if-needed!
+     metabase.sync.sync-metadata/sync-db-metadata!
+     metabase.sync.util-test/sync-database!
+     metabase.sync.util/store-sync-summary!
+     metabase.sync/sync-database!
+     metabase.task.index-values/job-init!
+     metabase.task.persist-refresh/job-init!
+     metabase.task.persist-refresh/refresh-tables!
+     metabase.task.persist-refresh/schedule-persistence-for-database!
+     metabase.task/delete-task!
+     metabase.test.data.bigquery-cloud-sdk/execute!
+     metabase.test.data.impl/copy-db-tables-and-fields!
+     metabase.test.data.impl/get-or-create-database!
+     metabase.test.data.impl/get-or-create-test-data-db!
+     metabase.test.data.interface/create-db!
+     metabase.test.data.interface/destroy-db!
+     metabase.test.data.oracle/create-user!
+     metabase.test.data.oracle/drop-user!
+     metabase.test.data.sql-jdbc.load-data/make-insert!
+     metabase.test.data.users/clear-cached-session-tokens!
+     metabase.test.initialize/do-initialization!
+     metabase.test.initialize/initialize-if-needed!
+     metabase.test.integrations.ldap/start-ldap-server!
+     metabase.test.util.log/ensure-unique-logger!
+     metabase.test.util.log/set-ns-log-level!
+     metabase.test.util/do-with-temp-env-var-value!
+     metabase.test.util/restore-raw-setting!
+     metabase.test.util/upsert-raw-setting!
+     metabase.test/initialize-if-needed!
+     metabase.test/test-helpers-set-global-values!
+     metabase.test/with-temp-env-var-value!
+     metabase.upload-test/set-local-infile!
+     metabase.util.files/create-dir-if-not-exists!
+     metabase.util.ssh-test/start-mock-servers!
+     metabase.util.ssh-test/stop-mock-servers!})
+
+(defn- end-with-exclamation?
+  [s]
+  (str/ends-with? s "!"))
+
+(defn- non-thread-safe-form-should-end-with-exclamation*
+  [{[defn-or-defmacro form-name] :children, :as node}]
+  (when-not (and (:string-value form-name)
+                 (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 non thread safe form `%s`."
+                                                              (:string-value defn-or-defmacro) qualified-symbol)
+                                             :type :metabase/test-helpers-use-non-thread-safe-functions)))))
+            node))
+    node))
+
+(defn non-thread-safe-form-should-end-with-exclamation
+  "Used to ensure defn and defmacro in test namespace to have name ending with `!` if it's non-thread-safe.
+  A function or a macro can be defined as 'not thread safe' when their funciton name ends with a `!`.
+
+  Only used in tests to identify thread-safe/non-thread-safe test helpers. See #37126"
+  [{:keys [node cljc lang]}]
+  (when (or (not cljc)
+            (= lang :clj))
+    (non-thread-safe-form-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))")
+
+ (def form "(defun f
+           [x]
+           (let [g! (fn [] 1)]
+           (g!)))")
+
+ (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 (non-thread-safe-form-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

@@ -24,7 +24,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
@@ -72,7 +72,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)
@@ -85,7 +85,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

@@ -37,7 +37,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"
@@ -47,6 +47,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

@@ -36,7 +36,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]]}))))))))))
@@ -58,7 +58,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]]}))))))))))
@@ -81,7 +81,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

@@ -2027,7 +2027,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))))