diff --git a/e2e/support/helpers/e2e-filter-helpers.js b/e2e/support/helpers/e2e-filter-helpers.js
index 4ad2b75ddc52f2b9c49ff1df44d671d707c62e08..2d34c01694865a6c3b3bbd59634369278d9465e0 100644
--- a/e2e/support/helpers/e2e-filter-helpers.js
+++ b/e2e/support/helpers/e2e-filter-helpers.js
@@ -81,3 +81,9 @@ export function setConnectedFieldSource(table, field) {
popover().findByText(table).click();
popover().findByText(field).click();
}
+
+export function changeSynchronousBatchUpdateSetting(value) {
+ cy.request("PUT", "/api/setting/synchronous-batch-updates", {
+ value: value,
+ });
+}
diff --git a/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js b/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js
index 41bce4e39417b3474045022bb277052ee26ea8c5..21d53cb88350e7a9bc1b7f102634f88f83a9247e 100644
--- a/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js
+++ b/e2e/test/scenarios/dashboard-filters/parameters.cy.spec.js
@@ -5,6 +5,7 @@ import {
ORDERS_DASHBOARD_ID,
} from "e2e/support/cypress_sample_instance_data";
import {
+ changeSynchronousBatchUpdateSetting,
disconnectDashboardFilter,
editDashboard,
filterWidget,
@@ -46,6 +47,12 @@ describe("scenarios > dashboard > parameters", () => {
beforeEach(() => {
restore();
cy.signInAsAdmin();
+ changeSynchronousBatchUpdateSetting(true);
+ });
+
+ afterEach(() => {
+ cy.signInAsAdmin();
+ changeSynchronousBatchUpdateSetting(false);
});
it("one filter should search across multiple fields", () => {
diff --git a/e2e/test/scenarios/dashboard-filters/temporal-unit-parameters.cy.spec.js b/e2e/test/scenarios/dashboard-filters/temporal-unit-parameters.cy.spec.js
index ca2ac776881e1353c295c0543e075154268ad57a..855249fba782ed6aa024ec64a679b8f6fcb83717 100644
--- a/e2e/test/scenarios/dashboard-filters/temporal-unit-parameters.cy.spec.js
+++ b/e2e/test/scenarios/dashboard-filters/temporal-unit-parameters.cy.spec.js
@@ -1,6 +1,7 @@
import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
import {
appBar,
+ changeSynchronousBatchUpdateSetting,
clearFilterWidget,
createNativeQuestion,
createQuestion,
@@ -236,9 +237,16 @@ const getParameterMapping = card => ({
describe("scenarios > dashboard > temporal unit parameters", () => {
beforeEach(() => {
restore();
+ cy.signInAsAdmin();
+ changeSynchronousBatchUpdateSetting(true);
cy.signInAsNormalUser();
});
+ afterEach(() => {
+ cy.signInAsAdmin();
+ changeSynchronousBatchUpdateSetting(false);
+ });
+
describe("mapping targets", () => {
it("should connect a parameter to a question and drill thru", () => {
createQuestion(noBreakoutQuestionDetails);
diff --git a/e2e/test/scenarios/dashboard/tabs.cy.spec.js b/e2e/test/scenarios/dashboard/tabs.cy.spec.js
index 5f208ea284d88799ede44ef84c5393f6880524ab..8ae991d740b5096fb387f3e3b300ee2a2bf48189 100644
--- a/e2e/test/scenarios/dashboard/tabs.cy.spec.js
+++ b/e2e/test/scenarios/dashboard/tabs.cy.spec.js
@@ -11,6 +11,7 @@ import {
import {
addHeadingWhileEditing,
addLinkWhileEditing,
+ changeSynchronousBatchUpdateSetting,
createDashboardWithTabs,
createNewTab,
dashboardCards,
@@ -92,12 +93,6 @@ const TAB_2 = {
name: "Tab 2",
};
-const changeSynchronousBatchUpdateSetting = value => {
- cy.request("PUT", "/api/setting/synchronous-batch-updates", {
- value: value,
- });
-};
-
describe("scenarios > dashboard > tabs", () => {
beforeEach(() => {
restore();
diff --git a/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js b/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js
index bbd8d7faf4f40b63257c129b68496c4c02e7927f..61cf2f1e766394a5c12ea47fa736165420389737 100644
--- a/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js
+++ b/e2e/test/scenarios/filters-reproductions/dashboard-filters-reproductions.cy.spec.js
@@ -11,6 +11,7 @@ import {
addOrUpdateDashboardCard,
appBar,
assertQueryBuilderRowCount,
+ changeSynchronousBatchUpdateSetting,
commandPalette,
commandPaletteSearch,
createDashboardWithTabs,
@@ -1459,6 +1460,12 @@ describe("issues 15279 and 24500", () => {
beforeEach(() => {
restore();
cy.signInAsAdmin();
+ changeSynchronousBatchUpdateSetting(true);
+ });
+
+ afterEach(() => {
+ cy.signInAsAdmin();
+ changeSynchronousBatchUpdateSetting(false);
});
it("corrupted dashboard filter should still appear in the UI without breaking other filters (metabase#15279, metabase#24500)", () => {
diff --git a/src/metabase/models/user_parameter_value.clj b/src/metabase/models/user_parameter_value.clj
index 8675503eda591f2925e70325b86cc23a2d0b8e90..f4ec8ea2da6fb6c2df3f634e447a036bbb33c521 100644
--- a/src/metabase/models/user_parameter_value.clj
+++ b/src/metabase/models/user_parameter_value.clj
@@ -1,8 +1,11 @@
(ns metabase.models.user-parameter-value
(:require
[cheshire.core :as json]
+ [medley.core :as m]
[metabase.api.common :as api]
[metabase.models.interface :as mi]
+ [metabase.util.grouper :as grouper]
+ [metabase.util.log :as log]
[metabase.util.malli :as mu]
[metabase.util.malli.schema :as ms]
[methodical.core :as methodical]
@@ -30,31 +33,49 @@
{:value {:in mi/json-in
:out json-out}})
-(mu/defn batched-upsert!
+(defn batched-upsert!
"Delete param with nil value and upsert the rest."
+ [parameters]
+ (let [to-insert (remove #(and (nil? (:value %)) (nil? (:default %))) parameters)]
+ (t2/with-transaction [_conn]
+ (doseq [batch (partition-all 1000 parameters)]
+ (t2/delete! :model/UserParameterValue
+ {:where (into [:or] (for [p batch]
+ [:and
+ [:= :user_id (:user_id p)]
+ [:= :dashboard_id (:dashboard_id p)]
+ [:= :parameter_id (:parameter_id p)]]))}))
+ (doseq [batch (partition-all 1000 to-insert)]
+ (t2/insert! :model/UserParameterValue
+ (map #(select-keys % [:user_id :dashboard_id :parameter_id :value]) batch))))))
+
+(defonce ^:private user-parameter-value-queue
+ (delay (grouper/start!
+ (fn [inputs]
+ (try
+ (batched-upsert!
+ (->> (for [input inputs
+ parameter (:parameters input)]
+ {:user_id (:user-id input)
+ :dashboard_id (:dashboard-id input)
+ :parameter_id (:id parameter)
+ :value (:value parameter)
+ :default (:default parameter)})
+ (m/index-by (juxt :user_id :dashboard_id :parameter_id))
+ vals))
+ (catch Exception e
+ (log/error e "Error saving user parameters for a dashboard"))))
+ :capacity 5000
+ :interval 5000)))
+
+(mu/defn store!
+ "Asynchronously delete params with a nil `value` and upsert the rest."
[user-id :- ms/PositiveInt
dashboard-id :- ms/PositiveInt
parameters :- [:sequential :map]]
- (let [;; delete param with nil value and no default
- to-delete-pred (fn [{:keys [value default]}]
- (and (nil? value) (nil? default)))
- to-delete (filter to-delete-pred parameters)
- to-upsert (filter (complement to-delete-pred) parameters)]
- (t2/with-transaction [_conn]
- (doseq [{:keys [id value]} to-upsert]
- (or (pos? (t2/update! :model/UserParameterValue {:user_id user-id
- :dashboard_id dashboard-id
- :parameter_id id}
- {:value value}))
- (t2/insert! :model/UserParameterValue {:user_id user-id
- :dashboard_id dashboard-id
- :parameter_id id
- :value value})))
- (when (seq to-delete)
- (t2/delete! :model/UserParameterValue
- :user_id user-id
- :dashboard_id dashboard-id
- :parameter_id [:in (map :id to-delete)])))))
+ (grouper/submit! @user-parameter-value-queue {:user-id user-id
+ :dashboard-id dashboard-id
+ :parameters parameters}))
;; hydration
diff --git a/src/metabase/query_processor/dashboard.clj b/src/metabase/query_processor/dashboard.clj
index 9250a675333a9a881311674792616bb0e361c1d6..d1c79ed5e44530998b05ef4619a0032e7b8c9f77 100644
--- a/src/metabase/query_processor/dashboard.clj
+++ b/src/metabase/query_processor/dashboard.clj
@@ -145,9 +145,7 @@
request-param-id->param))]
(when-let [user-id api/*current-user-id*]
(when (seq request-params)
- (user-parameter-value/batched-upsert!
- user-id dashboard-id
- request-params)))
+ (user-parameter-value/store! user-id dashboard-id request-params)))
(log/tracef "Dashboard parameters:\n%s\nRequest parameters:\n%s\nMerged:\n%s"
(u/pprint-to-str (update-vals dashboard-param-id->param
(fn [param]
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 0ce888982842d20a1d9e7f82252876c663cfe7b0..4c015c5f0ce49608f8f129356a217b4a253ba43d 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -391,58 +391,60 @@
(#'api.dashboard/get-dashboard dash-id))))))))
(deftest last-used-parameter-value-test
- (mt/dataset test-data
- (mt/with-column-remappings [orders.user_id people.name]
- (mt/as-admin
- (t2.with-temp/with-temp
- [Dashboard {dashboard-a-id :id} {:name "Test Dashboard"
- :creator_id (mt/user->id :crowberto)
- :parameters [{:name "Name", :slug "name", :id "a" :type :string/contains
- :default ["default_value"]}]}
- Dashboard {dashboard-b-id :id} {:name "Test Dashboard"
- :creator_id (mt/user->id :crowberto)
- :parameters [{:name "Name", :slug "name", :id "a" :type :string/contains}]}
- Card {card-id :id} {:database_id (mt/id)
- :query_type :native
- :name "test question"
- :creator_id (mt/user->id :crowberto)
- :dataset_query {:type :native
- :native {:query "SELECT COUNT(*) FROM people WHERE {{name}}"
- :template-tags
- {"name" {:name "Name"
- :display-name "name"
- :type :dimension
- :dimension [:field (mt/id :people :name) nil]
- :widget-type :string/contains}}}
- :database (mt/id)}}
- DashboardCard {dashcard-a-id :id} {:parameter_mappings [{:parameter_id "a", :card_id card-id, :target [:dimension [:template-tag "id"]]}]
- :card_id card-id
- :dashboard_id dashboard-a-id}
- DashboardCard {dashcard-b-id :id} {:parameter_mappings [{:parameter_id "a", :card_id card-id, :target [:dimension [:template-tag "id"]]}]
- :card_id card-id
- :dashboard_id dashboard-b-id}]
- (testing "User's set parameter is saved and sent back in the dashboard response, unique per dashboard."
- ;; api request mimicking a user setting a parameter value
- (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
- {:parameters [{:id "a" :value ["initial value"]}]})))
- (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-b-id dashcard-b-id card-id)
- {:parameters [{:id "a" :value ["initial value"]}]})))
- (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
- {:parameters [{:id "a" :value ["new value"]}]})))
- (is (= {:dashboard-a {:a ["new value"]}
- :dashboard-b {:a ["initial value"]}}
- {:dashboard-a (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-a-id)))
- :dashboard-b (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-b-id)))})))
- (testing "If a User unsets a parameter's value, the default is NOT used."
- ;; api request mimicking a user clearing parameter value, and no default exists
- (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
- {:parameters [{:id "a" :value nil}]})))
- (is (= {}
- (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-a-id)))))
- (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
- {:parameters [{:id "a" :value nil :default ["default value"]}]})))
- (is (= {:a nil}
- (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-a-id)))))))))))
+ (mt/test-helpers-set-global-values!
+ (mt/with-temporary-setting-values [synchronous-batch-updates true]
+ (mt/dataset test-data
+ (mt/with-column-remappings [orders.user_id people.name]
+ (mt/as-admin
+ (t2.with-temp/with-temp
+ [Dashboard {dashboard-a-id :id} {:name "Test Dashboard"
+ :creator_id (mt/user->id :crowberto)
+ :parameters [{:name "Name", :slug "name", :id "a" :type :string/contains
+ :default ["default_value"]}]}
+ Dashboard {dashboard-b-id :id} {:name "Test Dashboard"
+ :creator_id (mt/user->id :crowberto)
+ :parameters [{:name "Name", :slug "name", :id "a" :type :string/contains}]}
+ Card {card-id :id} {:database_id (mt/id)
+ :query_type :native
+ :name "test question"
+ :creator_id (mt/user->id :crowberto)
+ :dataset_query {:type :native
+ :native {:query "SELECT COUNT(*) FROM people WHERE {{name}}"
+ :template-tags
+ {"name" {:name "Name"
+ :display-name "name"
+ :type :dimension
+ :dimension [:field (mt/id :people :name) nil]
+ :widget-type :string/contains}}}
+ :database (mt/id)}}
+ DashboardCard {dashcard-a-id :id} {:parameter_mappings [{:parameter_id "a", :card_id card-id, :target [:dimension [:template-tag "id"]]}]
+ :card_id card-id
+ :dashboard_id dashboard-a-id}
+ DashboardCard {dashcard-b-id :id} {:parameter_mappings [{:parameter_id "a", :card_id card-id, :target [:dimension [:template-tag "id"]]}]
+ :card_id card-id
+ :dashboard_id dashboard-b-id}]
+ (testing "User's set parameter is saved and sent back in the dashboard response, unique per dashboard."
+ ;; api request mimicking a user setting a parameter value
+ (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
+ {:parameters [{:id "a" :value ["initial value"]}]})))
+ (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-b-id dashcard-b-id card-id)
+ {:parameters [{:id "a" :value ["initial value"]}]})))
+ (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
+ {:parameters [{:id "a" :value ["new value"]}]})))
+ (is (= {:dashboard-a {:a ["new value"]}
+ :dashboard-b {:a ["initial value"]}}
+ {:dashboard-a (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-a-id)))
+ :dashboard-b (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-b-id)))})))
+ (testing "If a User unsets a parameter's value, the default is NOT used."
+ ;; api request mimicking a user clearing parameter value, and no default exists
+ (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
+ {:parameters [{:id "a" :value nil}]})))
+ (is (= {}
+ (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-a-id)))))
+ (is (some? (mt/user-http-request :rasta :post (format "dashboard/%d/dashcard/%s/card/%s/query" dashboard-a-id dashcard-a-id card-id)
+ {:parameters [{:id "a" :value nil :default ["default value"]}]})))
+ (is (= {:a nil}
+ (:last_used_param_values (mt/user-http-request :rasta :get 200 (format "dashboard/%d" dashboard-a-id)))))))))))))
(deftest fetch-dashboard-test
(testing "GET /api/dashboard/:id"
diff --git a/test/metabase/models/user_parameter_value_test.clj b/test/metabase/models/user_parameter_value_test.clj
index 8ed7c19b9b5bcaadde7961e1a15cba53b9811dc6..226e65287726e950acd2c3268d17a5c26736f519 100644
--- a/test/metabase/models/user_parameter_value_test.clj
+++ b/test/metabase/models/user_parameter_value_test.clj
@@ -6,44 +6,53 @@
[metabase.test :as mt]
[toucan2.core :as t2]))
-(deftest user-parameter-value-batch-upsert-test
- (mt/with-temp [:model/Dashboard {dashboard-id :id} {}]
- (let [value! (fn [parameters]
- (upv/batched-upsert! (mt/user->id :rasta) dashboard-id parameters))
- value-fn (fn []
- (t2/select-fn->fn :parameter_id :value
- :model/UserParameterValue
- :user_id (mt/user->id :rasta) :dashboard_id dashboard-id))]
+(set! *warn-on-reflection* true)
- (testing "insert upv if value is non-nil"
- (value! [{:id "param1" :value 1}
- {:id "param2" :value "string"}
- {:id "param3" :value ["A" "B" "C"]}])
- (is (= {"param1" 1
- "param2" "string"
- "param3" ["A" "B" "C"]}
- (value-fn))))
+(deftest user-parameter-value-store-test
+ (mt/test-helpers-set-global-values!
+ (mt/with-temporary-setting-values [synchronous-batch-updates true]
+ (mt/with-temp [:model/Dashboard {dashboard-id :id} {}]
+ (let [store! (fn [parameters]
+ (upv/store! (mt/user->id :rasta) dashboard-id parameters))
+ retrieve-fn (fn []
+ (t2/select-fn->fn :parameter_id :value
+ :model/UserParameterValue
+ :user_id (mt/user->id :rasta) :dashboard_id dashboard-id))]
+ (testing "insert upv if value is non-nil"
+ (store! [{:id "param1" :value 1}
+ {:id "param2" :value "string"}
+ {:id "param3" :value ["A" "B" "C"]}])
+ (is (= {"param1" 1
+ "param2" "string"
+ "param3" ["A" "B" "C"]}
+ (retrieve-fn))))
- (testing "delete if value is nil"
- (value! [{:id "param1" :value "foo"} {:id "param2" :value nil}])
- (is (= {"param1" "foo"
- "param3" ["A" "B" "C"]}
- (value-fn))))
+ (testing "delete if value is nil"
+ (store! [{:id "param1" :value "foo"} {:id "param2" :value nil}])
+ (is (= {"param1" "foo"
+ "param3" ["A" "B" "C"]}
+ (retrieve-fn))))
- (testing "update existing param and insert new param"
- (value! [{:id "param1", :value "new-value"} {:id "param2", :value "new-value"}])
- (is (= {"param1" "new-value"
- "param2" "new-value"
- "param3" ["A" "B" "C"]}
- (value-fn))))
+ (testing "last value wins"
+ (store! [{:id "param1" :value "bar"} {:id "param1" :value "baz"}])
+ (is (= {"param1" "baz"
+ "param3" ["A" "B" "C"]}
+ (retrieve-fn))))
- (testing "insert nil if param has default value"
- (value! [{:id "param4" :value nil :default "default"}])
- (is (= {"param1" "new-value"
- "param2" "new-value"
- "param3" ["A" "B" "C"]
- "param4" nil}
- (value-fn)))))))
+ (testing "update existing param and insert new param"
+ (store! [{:id "param1", :value "new-value"} {:id "param2", :value "new-value"}])
+ (is (= {"param1" "new-value"
+ "param2" "new-value"
+ "param3" ["A" "B" "C"]}
+ (retrieve-fn))))
+
+ (testing "insert nil if param has default value"
+ (store! [{:id "param4" :value nil :default "default"}])
+ (is (= {"param1" "new-value"
+ "param2" "new-value"
+ "param3" ["A" "B" "C"]
+ "param4" nil}
+ (retrieve-fn)))))))))
(deftest hydrate-last-used-param-values-test
(let [rasta-id (mt/user->id :rasta)