From 34488adb3b9ce4cc94a1a0b9e19b191e1455db86 Mon Sep 17 00:00:00 2001
From: metamben <103100869+metamben@users.noreply.github.com>
Date: Thu, 29 Dec 2022 23:50:33 +0300
Subject: [PATCH] Fix flaky metabase.api.notify-test/post-db-id-test (#27465)
It seems the expectation was that the binding *execute-asynchronously* to false
in the test would affect the binding seen by the server. Instead of this a new
parameter is added to the request that makes the server return only when the
asynchronous operation has completed.
---
src/metabase/api/notify.clj | 23 ++++-----
test/metabase/api/notify_test.clj | 83 +++++++++++++++----------------
2 files changed, 50 insertions(+), 56 deletions(-)
diff --git a/src/metabase/api/notify.clj b/src/metabase/api/notify.clj
index 82755ebd3d6..484c83f0412 100644
--- a/src/metabase/api/notify.clj
+++ b/src/metabase/api/notify.clj
@@ -11,8 +11,6 @@
[schema.core :as s]
[toucan.db :as db]))
-(def ^:private ^:dynamic *execute-asynchronously* true)
-
(api/defendpoint POST "/db/:id"
"Notification about a potential schema change to one of our `Databases`.
Caller can optionally specify a `:table_id` or `:table_name` in the body to limit updates to a single
@@ -20,24 +18,21 @@
regardless if a `:table_id` or `:table_name` is passed.
This endpoint is secured by an API key that needs to be passed as a `X-METABASE-APIKEY` header which needs to be defined in
the `MB_API_KEY` [environment variable](https://www.metabase.com/docs/latest/configuring-metabase/environment-variables.html#mb_api_key)"
- [id :as {{:keys [table_id table_name scan]} :body}]
+ [id :as {{:keys [table_id table_name scan synchronous?]} :body}]
{table_id (s/maybe su/IntGreaterThanZero)
table_name (s/maybe su/NonBlankString)
scan (s/maybe (s/enum "full" "schema"))}
(let [schema? (when scan (#{"schema" :schema} scan))
table-sync-fn (if schema? sync-metadata/sync-table-metadata! sync/sync-table!)
- db-sync-fn (if schema? sync-metadata/sync-db-metadata! sync/sync-database!)
- execute! (fn [thunk]
- (if *execute-asynchronously*
- (future (thunk))
- (thunk)))]
+ db-sync-fn (if schema? sync-metadata/sync-db-metadata! sync/sync-database!)]
(api/let-404 [database (db/select-one Database :id id)]
- (cond
- table_id (api/let-404 [table (db/select-one Table :db_id id, :id (int table_id))]
- (execute! #(table-sync-fn table)))
- table_name (api/let-404 [table (db/select-one Table :db_id id, :name table_name)]
- (execute! #(table-sync-fn table)))
- :else (execute! #(db-sync-fn database)))))
+ (cond-> (cond
+ table_id (api/let-404 [table (db/select-one Table :db_id id, :id (int table_id))]
+ (future (table-sync-fn table)))
+ table_name (api/let-404 [table (db/select-one Table :db_id id, :name table_name)]
+ (future (table-sync-fn table)))
+ :else (future (db-sync-fn database)))
+ synchronous? deref)))
{:success true})
diff --git a/test/metabase/api/notify_test.clj b/test/metabase/api/notify_test.clj
index ee4d74927a8..6ec608da650 100644
--- a/test/metabase/api/notify_test.clj
+++ b/test/metabase/api/notify_test.clj
@@ -2,7 +2,6 @@
(:require
[clj-http.client :as http]
[clojure.test :refer :all]
- [metabase.api.notify :as api.notify]
[metabase.http-client :as client]
[metabase.models.database :as database]
[metabase.server.middleware.util :as mw.util]
@@ -55,44 +54,44 @@
(select-keys (ex-data e) [:status :body])))))))))
(deftest post-db-id-test
- (binding [api.notify/*execute-asynchronously* false]
- (mt/test-drivers (mt/normal-drivers)
- (let [table-name (->> (mt/db) database/tables first :name)
- post (fn post-api
- ([payload] (post-api payload 200))
- ([payload expected-code]
- (mt/with-temporary-setting-values [api-key "test-api-key"]
- (mt/client :post expected-code (format "notify/db/%d" (u/the-id (mt/db)))
- {:request-options api-headers}
- payload))))]
- (testing "sync just table when table is provided"
- (let [long-sync-called? (atom false), short-sync-called? (atom false)]
- (with-redefs [metabase.sync/sync-table! (fn [_table] (reset! long-sync-called? true))
- metabase.sync.sync-metadata/sync-table-metadata! (fn [_table] (reset! short-sync-called? true))]
- (post {:scan :full, :table_name table-name})
- (is @long-sync-called?)
- (is (not @short-sync-called?)))))
- (testing "only a quick sync when quick parameter is provided"
- (let [long-sync-called? (atom false), short-sync-called? (atom false)]
- (with-redefs [metabase.sync/sync-table! (fn [_table] (reset! long-sync-called? true))
- metabase.sync.sync-metadata/sync-table-metadata! (fn [_table] (reset! short-sync-called? true))]
- (post {:scan :schema, :table_name table-name})
- (is (not @long-sync-called?))
- (is @short-sync-called?))))
- (testing "full db sync by default"
- (let [full-sync? (atom false)]
- (with-redefs [metabase.sync/sync-database! (fn [_db] (reset! full-sync? true))]
- (post {})
- (is @full-sync?))))
- (testing "simple sync with params"
- (let [full-sync? (atom false)
- smaller-sync (atom true)]
- (with-redefs [metabase.sync/sync-database! (fn [_db] (reset! full-sync? true))
- metabase.sync.sync-metadata/sync-db-metadata! (fn [_db] (reset! smaller-sync true))]
- (post {:scan :schema})
- (is (not @full-sync?))
- (is @smaller-sync))))
- (testing "errors on unrecognized scan options"
- (is (= {:errors
- {:scan "value may be nil, or if non-nil, value must be one of: `full`, `schema`."}}
- (post {:scan :unrecognized} 400))))))))
+ (mt/test-drivers (mt/normal-drivers)
+ (let [table-name (->> (mt/db) database/tables first :name)
+ post (fn post-api
+ ([payload] (post-api payload 200))
+ ([payload expected-code]
+ (mt/with-temporary-setting-values [api-key "test-api-key"]
+ (mt/client :post expected-code (format "notify/db/%d" (u/the-id (mt/db)))
+ {:request-options api-headers}
+ (merge {:synchronous? true}
+ payload)))))]
+ (testing "sync just table when table is provided"
+ (let [long-sync-called? (promise), short-sync-called? (promise)]
+ (with-redefs [metabase.sync/sync-table! (fn [_table] (deliver long-sync-called? true))
+ metabase.sync.sync-metadata/sync-table-metadata! (fn [_table] (deliver short-sync-called? true))]
+ (post {:scan :full, :table_name table-name})
+ (is @long-sync-called?)
+ (is (not (realized? short-sync-called?))))))
+ (testing "only a quick sync when quick parameter is provided"
+ (let [long-sync-called? (promise), short-sync-called? (promise)]
+ (with-redefs [metabase.sync/sync-table! (fn [_table] (deliver long-sync-called? true))
+ metabase.sync.sync-metadata/sync-table-metadata! (fn [_table] (deliver short-sync-called? true))]
+ (post {:scan :schema, :table_name table-name})
+ (is (not (realized? long-sync-called?)))
+ (is @short-sync-called?))))
+ (testing "full db sync by default"
+ (let [full-sync? (promise)]
+ (with-redefs [metabase.sync/sync-database! (fn [_db] (deliver full-sync? true))]
+ (post {})
+ (is @full-sync?))))
+ (testing "simple sync with params"
+ (let [full-sync? (promise)
+ smaller-sync (promise)]
+ (with-redefs [metabase.sync/sync-database! (fn [_db] (deliver full-sync? true))
+ metabase.sync.sync-metadata/sync-db-metadata! (fn [_db] (deliver smaller-sync true))]
+ (post {:scan :schema})
+ (is (not (realized? full-sync?)))
+ (is @smaller-sync))))
+ (testing "errors on unrecognized scan options"
+ (is (= {:errors
+ {:scan "value may be nil, or if non-nil, value must be one of: `full`, `schema`."}}
+ (post {:scan :unrecognized} 400)))))))
--
GitLab