From c6925a889053f619ccb15ddd612731d8bdd398eb Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Fri, 23 Dec 2022 16:54:15 +0700
Subject: [PATCH] Dropdown sources using card can select column (#27343)
Co-authored-by: Alexander Polyankin <alexander.polyankin@metabase.com>
---
src/metabase/api/dashboard.clj | 28 +++-
src/metabase/models/parameter_card.clj | 43 ------
src/metabase/models/params/card_values.clj | 54 ++++++++
src/metabase/util/schema.clj | 19 ++-
test/metabase/api/dashboard_test.clj | 124 +++++++++---------
.../models/params/card_values_test.clj | 118 +++++++++++++++++
test/metabase/test.clj | 1 +
7 files changed, 278 insertions(+), 109 deletions(-)
create mode 100644 src/metabase/models/params/card_values.clj
create mode 100644 test/metabase/models/params/card_values_test.clj
diff --git a/src/metabase/api/dashboard.clj b/src/metabase/api/dashboard.clj
index 935f75638f9..1a563cd7d66 100644
--- a/src/metabase/api/dashboard.clj
+++ b/src/metabase/api/dashboard.clj
@@ -3,6 +3,7 @@
(:require
[cheshire.core :as json]
[clojure.set :as set]
+ [clojure.string :as str]
[clojure.tools.logging :as log]
[compojure.core :refer [DELETE GET POST PUT]]
[medley.core :as m]
@@ -22,8 +23,8 @@
[metabase.models.dashboard-card :as dashboard-card :refer [DashboardCard]]
[metabase.models.field :refer [Field]]
[metabase.models.interface :as mi]
- [metabase.models.parameter-card :as parameter-card]
[metabase.models.params :as params]
+ [metabase.models.params.card-values :as params.card-values]
[metabase.models.params.chain-filter :as chain-filter]
[metabase.models.query :as query :refer [Query]]
[metabase.models.query.permissions :as query-perms]
@@ -36,6 +37,7 @@
[metabase.query-processor.pivot :as qp.pivot]
[metabase.query-processor.util :as qp.util]
[metabase.related :as related]
+ [metabase.search.util :as search]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[metabase.util.schema :as su]
@@ -734,14 +736,32 @@
(api/throw-403 e)
(throw e)))))))
+(defn- query-matches
+ "Filter the values according to the `search-term`.
+
+ Values could have 2 shapes
+ - [value1, value2]
+ - [[value1, label1], [value2, label2]] - we search using label in this case"
+ [query values]
+ (let [normalized-query (search/normalize query)]
+ (filter #(str/includes? (search/normalize (if (string? %)
+ %
+ ;; search by label
+ (second %)))
+ normalized-query) values)))
+
(defn- static-parameter-values
[{values-source-options :values_source_config :as _param} query]
(when-let [values (:values values-source-options)]
{:values (if query
- (parameter-card/query-matches query values)
+ (query-matches query values)
values)
:has_more_values false}))
+(defn- card-parameter-values
+ [{config :values_source_config :as _param} query]
+ (params.card-values/values-from-card (:card_id config) (:value_field config) query))
+
(s/defn param-values
"Fetch values for a parameter.
@@ -764,8 +784,8 @@
:status-code 400})))
(case (:values_source_type param)
"static-list" (static-parameter-values param query)
- "card" (parameter-card/values-for-dashboard dashboard param-key query)
- (chain-filter dashboard param-key constraint-param-key->value query)))))
+ "card" (card-parameter-values param query)
+ nil (chain-filter dashboard param-key constraint-param-key->value query)))))
(api/defendpoint GET "/:id/params/:param-key/values"
"Fetch possible values of the parameter whose ID is `:param-key`. If the values come directly from a query, optionally
diff --git a/src/metabase/models/parameter_card.clj b/src/metabase/models/parameter_card.clj
index 8527b9d7842..23161947171 100644
--- a/src/metabase/models/parameter_card.clj
+++ b/src/metabase/models/parameter_card.clj
@@ -1,8 +1,5 @@
(ns metabase.models.parameter-card
(:require
- [clojure.string :as str]
- [metabase.query-processor :as qp]
- [metabase.search.util :as search]
[metabase.util :as u]
[metabase.util.i18n :refer [tru]]
[toucan.db :as db]
@@ -72,43 +69,3 @@
upsertable-parameters (filter upsertable? parameters)]
(upsert-for-dashboard! dashboard-id upsertable-parameters)
(delete-all-for-parameterized-object! "dashboard" dashboard-id (map :id upsertable-parameters))))
-
-;;; ----------------------------------------------- param values -----------------------------------------------
-
-(defn query-matches
- "Filter the values according to the `search-term`.
-
- Values could have 2 shapes
- - [value1, value2]
- - [[value1, label1], [value2, label2]] - we search using label in this case"
- [search-term values]
- (if (str/blank? search-term)
- values
- (let [normalized-search-term (search/normalize search-term)]
- (filter #(str/includes? (search/normalize (if (string? %)
- %
- ;; search by label
- (second %)))
- normalized-search-term) values))))
-
-(defn- query-for-dashboard
- [{dashboard-id :id} param-key]
- (->> (db/query {:select [:dataset_query]
- :from [:report_card]
- :join [:parameter_card [:= :report_card.id :parameter_card.card_id]]
- :where [:and
- [:= :parameterized_object_id dashboard-id]
- [:= :parameterized_object_type "dashboard"]
- [:= :parameter_id param-key]]})
- (db/do-post-select 'Card)
- first
- :dataset_query))
-
-(defn values-for-dashboard
- "Returns a map with the filter `:values` associated with the dashboard."
- [dashboard param-key search-term]
- {:values
- (->> (qp/process-query (query-for-dashboard dashboard param-key) {:rff (constantly conj)})
- (map first)
- (query-matches search-term))
- :has_more_values false}) ;; TODO: this should be more clever
diff --git a/src/metabase/models/params/card_values.clj b/src/metabase/models/params/card_values.clj
new file mode 100644
index 00000000000..487643a12a1
--- /dev/null
+++ b/src/metabase/models/params/card_values.clj
@@ -0,0 +1,54 @@
+(ns metabase.models.params.card-values
+ "Code related to getting values for a parameter where its source values is a card."
+ (:require
+ [clojure.string :as str]
+ [metabase.models :refer [Card]]
+ [metabase.query-processor :as qp]
+ [metabase.util.schema :as su]
+ [schema.core :as s]
+ [toucan.db :as db]))
+
+(def ^:dynamic *max-rows*
+ "Maximum number of rows returned when running a card.
+ It's 2000 because this is the limit we use when viewing a question.
+ Maybe we should lower it for the sake of display a parameter dropdown."
+ 2000)
+
+(defn- values-from-card-query
+ [card-id value-field query]
+ (let [card (db/select-one Card :id card-id)]
+ {:database (:database_id card)
+ :type :query
+ :query (merge
+ {:source-table (format "card__%d" card-id)
+ :fields [value-field]
+ :limit *max-rows*}
+ (when query
+ {:filter [:contains [:lower value-field] (str/lower-case query)]}))
+ :middleware {:disable-remaps? true}}))
+
+(s/defn values-from-card
+ "Get values a field from a card.
+
+ (values-from-card 1 [:field \"name\" nil] \"red\")
+ ;; will execute a mbql that looks like
+ ;; {:source-table (format \"card__%d\" card-id)
+ ;; :fields [value-field]
+ ;; :filter [:contains [:lower value-field] \"red\"]
+ ;; :limit *max-rows*}
+ =>
+ {:values [\"Red Medicine\"]
+ :has_more_values false}
+ "
+ ([card-id value-field-ref]
+ (values-from-card card-id value-field-ref nil))
+
+ ([card-id :- su/IntGreaterThanZero
+ value-field :- su/Field
+ query :- (s/maybe su/NonBlankString)]
+ (let [mbql-query (values-from-card-query card-id value-field query)
+ query-limit (get-in mbql-query [:query :limit])
+ result (qp/process-query mbql-query)]
+ {:values (map first (get-in result [:data :rows]))
+ ;; if the row_count returned = the limit we specified, then it's probably has more than that
+ :has_more_values (= (:row_count result) query-limit)})))
diff --git a/src/metabase/util/schema.clj b/src/metabase/util/schema.clj
index 912fda514b8..0624e9e03c6 100644
--- a/src/metabase/util/schema.clj
+++ b/src/metabase/util/schema.clj
@@ -5,6 +5,8 @@
[clojure.string :as str]
[clojure.walk :as walk]
[medley.core :as m]
+ [metabase.mbql.normalize :as mbql.normalize]
+ [metabase.mbql.schema :as mbql.s]
[metabase.types :as types]
[metabase.util :as u]
[metabase.util.date-2 :as u.date]
@@ -288,6 +290,13 @@
(deferred-tru "Valid field semantic or relation type (keyword or string)"))
(deferred-tru "value must be a valid field semantic or relation type (keyword or string).")))
+(def Field
+ "Schema for a valid Field for API usage."
+ (with-api-error-message (s/pred
+ (comp (complement (s/checker mbql.s/Field))
+ mbql.normalize/normalize-tokens))
+ (deferred-tru "value must an array with :field id-or-name and an options map")))
+
(def CoercionStrategyKeywordOrString
"Like `CoercionStrategy` but accepts either a keyword or string."
(with-api-error-message (s/pred #(isa? (keyword %) :Coercion/*) (deferred-tru "Valid coercion strategy"))
@@ -364,8 +373,14 @@
(def ValuesSourceConfig
"Schema for valid source_options within a Parameter"
;; TODO: This should be tighter
- {(s/optional-key :values) (s/cond-pre [s/Any])
- (s/optional-key :card_id) IntGreaterThanZero})
+ {;; for source_type = 'static-list'
+ (s/optional-key :values) (s/cond-pre [s/Any])
+
+ ;; for source_type = 'card'
+ (s/optional-key :card_id) IntGreaterThanZero
+ (s/optional-key :value_field) Field
+ ;; label_field is optional
+ (s/optional-key :label_field) Field})
(def Parameter
"Schema for a valid Parameter.
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 8e24bdf606a..0f7fc4542bd 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -2061,31 +2061,35 @@
(deftest parameter-values-from-card-test
;; TODO add permissions tests
- (mt/with-temp* [Card [{card-id :id} {:database_id (mt/id)
- :table_id (mt/id :venues)
- :dataset_query (mt/native-query {:query "SELECT name FROM venues WHERE name LIKE '%Bar%'"})}]
- Dashboard [{dashboard-id :id} {:parameters [{:id "abc"
- :type "category"
- :name "CATEGORY"
- :values_source_type "card"
- :values_source_config {:card_id card-id}}]}]]
+ (mt/with-temp*
+ [Card [{card-id :id}
+ (merge (mt/card-with-source-metadata-for-query (mt/mbql-query venues {:limit 5}))
+ {:database_id (mt/id)
+ :table_id (mt/id :venues)})]
+ Dashboard [{dashboard-id :id}
+ {:parameters [{:id "abc"
+ :type "category"
+ :name "CATEGORY"
+ :values_source_type "card"
+ :values_source_config {:card_id card-id
+ :value_field (mt/$ids $venues.name)}}]}]]
+
(testing "It uses the results of the card's query execution"
(let-url [url (chain-filter-values-url dashboard-id "abc")]
- (is (= {:values ["The Misfit Restaurant + Bar"
- "My Brother's Bar-B-Q"
- "Two Sisters Bar & Books"
- "Tanoshi Sushi & Sake Bar"
- "Barney's Beanery"]
+ (is (= {:values ["Red Medicine"
+ "Stout Burgers & Beers"
+ "The Apple Pan"
+ "Wurstküche"
+ "Brite Spot Family Restaurant"]
:has_more_values false}
(mt/user-http-request :rasta :get 200 url)))))
(testing "it only returns search matches"
- (let-url [url (chain-filter-search-url dashboard-id "abc" "sushi")]
- (is (= {:values ["Tanoshi Sushi & Sake Bar"]
+ (let-url [url (chain-filter-search-url dashboard-id "abc" "red")]
+ (is (= {:values ["Red Medicine"]
:has_more_values false}
(mt/user-http-request :rasta :get 200 url)))))))
-
(deftest valid-filter-fields-test
(testing "GET /api/dashboard/params/valid-filter-fields"
(letfn [(url [filtered filtering]
@@ -2188,55 +2192,55 @@
:type :native
:native {:query "delete from users;"}}}]]
(is (= "Write queries are only executable via the Actions API."
- (:message (mt/user-http-request :rasta :post 405 (url :card-id (:id card))))))))))))
+ (:message (mt/user-http-request :rasta :post 405 (url :card-id (:id card)))))))))))))
- ;; see also [[metabase.query-processor.dashboard-test]]
- (deftest dashboard-card-query-parameters-test
- (testing "POST /api/dashboard/:dashboard-id/card/:card-id/query"
- (with-chain-filter-fixtures [{{dashboard-id :id} :dashboard, {card-id :id} :card, {dashcard-id :id} :dashcard}]
- (let [url (dashboard-card-query-url dashboard-id card-id dashcard-id)]
- (testing "parameters"
- (testing "Should respect valid parameters"
- (is (schema= (dashboard-card-query-expected-results-schema :row-count 6)
- (mt/user-http-request :rasta :post 202 url
- {:parameters [{:id "_PRICE_"
- :value 4}]})))
- (testing "New parameter types"
- (testing :number/=
- (is (schema= (dashboard-card-query-expected-results-schema :row-count 94)
- (mt/user-http-request :rasta :post 202 url
- {:parameters [{:id "_PRICE_"
- :type :number/=
- :value [1 2 3]}]}))))))
- (testing "Should return error if parameter doesn't exist"
- (is (= "Dashboard does not have a parameter with ID \"_THIS_PARAMETER_DOES_NOT_EXIST_\"."
- (mt/user-http-request :rasta :post 400 url
- {:parameters [{:id "_THIS_PARAMETER_DOES_NOT_EXIST_"
- :value 3}]}))))
- (testing "Should return sensible error message for invalid parameter input"
- (is (= {:errors {:parameters (str "value may be nil, or if non-nil, value must be an array. "
- "Each value must be a parameter map with an 'id' key")}}
- (mt/user-http-request :rasta :post 400 url
- {:parameters {"_PRICE_" 3}}))))
- (testing "Should ignore parameters that are valid for the Dashboard but not part of this Card (no mapping)"
- (testing "Sanity check"
- (is (schema= (dashboard-card-query-expected-results-schema :row-count 6)
- (mt/user-http-request :rasta :post 202 url
- {:parameters [{:id "_PRICE_"
- :value 4}]}))))
- (mt/with-temp-vals-in-db DashboardCard dashcard-id {:parameter_mappings []}
- (is (schema= (dashboard-card-query-expected-results-schema :row-count 100)
+;; see also [[metabase.query-processor.dashboard-test]]
+(deftest dashboard-card-query-parameters-test
+ (testing "POST /api/dashboard/:dashboard-id/card/:card-id/query"
+ (with-chain-filter-fixtures [{{dashboard-id :id} :dashboard, {card-id :id} :card, {dashcard-id :id} :dashcard}]
+ (let [url (dashboard-card-query-url dashboard-id card-id dashcard-id)]
+ (testing "parameters"
+ (testing "Should respect valid parameters"
+ (is (schema= (dashboard-card-query-expected-results-schema :row-count 6)
+ (mt/user-http-request :rasta :post 202 url
+ {:parameters [{:id "_PRICE_"
+ :value 4}]})))
+ (testing "New parameter types"
+ (testing :number/=
+ (is (schema= (dashboard-card-query-expected-results-schema :row-count 94)
(mt/user-http-request :rasta :post 202 url
{:parameters [{:id "_PRICE_"
- :value 4}]})))))
-
- ;; don't let people try to be sneaky and get around our validation by passing in a different `:target`
- (testing "Should ignore incorrect `:target` passed in to API endpoint"
+ :type :number/=
+ :value [1 2 3]}]}))))))
+ (testing "Should return error if parameter doesn't exist"
+ (is (= "Dashboard does not have a parameter with ID \"_THIS_PARAMETER_DOES_NOT_EXIST_\"."
+ (mt/user-http-request :rasta :post 400 url
+ {:parameters [{:id "_THIS_PARAMETER_DOES_NOT_EXIST_"
+ :value 3}]}))))
+ (testing "Should return sensible error message for invalid parameter input"
+ (is (= {:errors {:parameters (str "value may be nil, or if non-nil, value must be an array. "
+ "Each value must be a parameter map with an 'id' key")}}
+ (mt/user-http-request :rasta :post 400 url
+ {:parameters {"_PRICE_" 3}}))))
+ (testing "Should ignore parameters that are valid for the Dashboard but not part of this Card (no mapping)"
+ (testing "Sanity check"
(is (schema= (dashboard-card-query-expected-results-schema :row-count 6)
(mt/user-http-request :rasta :post 202 url
- {:parameters [{:id "_PRICE_"
- :target [:dimension [:field (mt/id :venues :id) nil]]
- :value 4}]}))))))))))
+ {:parameters [{:id "_PRICE_"
+ :value 4}]}))))
+ (mt/with-temp-vals-in-db DashboardCard dashcard-id {:parameter_mappings []}
+ (is (schema= (dashboard-card-query-expected-results-schema :row-count 100)
+ (mt/user-http-request :rasta :post 202 url
+ {:parameters [{:id "_PRICE_"
+ :value 4}]})))))
+
+ ;; don't let people try to be sneaky and get around our validation by passing in a different `:target`
+ (testing "Should ignore incorrect `:target` passed in to API endpoint"
+ (is (schema= (dashboard-card-query-expected-results-schema :row-count 6)
+ (mt/user-http-request :rasta :post 202 url
+ {:parameters [{:id "_PRICE_"
+ :target [:dimension [:field (mt/id :venues :id) nil]]
+ :value 4}]})))))))))
(defn- parse-export-format-results [^bytes results export-format]
(with-open [is (java.io.ByteArrayInputStream. results)]
diff --git a/test/metabase/models/params/card_values_test.clj b/test/metabase/models/params/card_values_test.clj
new file mode 100644
index 00000000000..3d721d64a5f
--- /dev/null
+++ b/test/metabase/models/params/card_values_test.clj
@@ -0,0 +1,118 @@
+(ns metabase.models.params.card-values-test
+ (:require
+ [clojure.test :refer :all]
+ [metabase.models :refer [Card]]
+ [metabase.models.params.card-values :as params.card-values]
+ [metabase.test :as mt]))
+
+(deftest with-mbql-card-test
+ (doseq [dataset? [true false]]
+ (testing (format "source card is a %s" (if dataset? "model" "question"))
+ (binding [params.card-values/*max-rows* 3]
+ (testing "with simple mbql"
+ (mt/with-temp* [Card [{card-id :id}
+ (merge (mt/card-with-source-metadata-for-query (mt/mbql-query venues))
+ {:database_id (mt/id)
+ :dataset dataset?
+ :table_id (mt/id :venues)})]]
+ (testing "get values"
+ (is (=? {:has_more_values true,
+ :values ["Red Medicine"
+ "Stout Burgers & Beers"
+ "The Apple Pan"]}
+ (params.card-values/values-from-card
+ card-id
+ (mt/$ids $venues.name)))))
+
+ (testing "case in-sensitve search test"
+ (is (=? {:has_more_values false
+ :values ["Liguria Bakery" "Noe Valley Bakery"]}
+ (params.card-values/values-from-card
+ card-id
+ (mt/$ids $venues.name)
+ "bakery"))))))
+
+ (testing "has aggregation column"
+ (mt/with-temp* [Card [{card-id :id}
+ (merge (mt/card-with-source-metadata-for-query
+ (mt/mbql-query venues
+ {:aggregation [[:sum $venues.price]]
+ :breakout [[:field %categories.name {:source-field %venues.category_id}]]}))
+ {:database_id (mt/id)
+ :dataset dataset?
+ :table_id (mt/id :venues)})]]
+
+ (testing "get values from breakout columns"
+ (is (=? {:has_more_values true,
+ :values ["American" "Artisan" "Asian"]}
+ (params.card-values/values-from-card
+ card-id
+ (mt/$ids $categories.name)))))
+
+ (testing "get values from aggregation column"
+ (is (=? {:has_more_values true,
+ :values [20 4 4]}
+ (params.card-values/values-from-card
+ card-id
+ [:field "sum" {:base-type :type/Float}]))))
+
+ (testing "doing case in-sensitve search on breakout columns"
+ (is (=? {:has_more_values false
+ :values ["Bakery"]}
+ (params.card-values/values-from-card
+ card-id
+ (mt/$ids $categories.name)
+ "bakery"))))))
+
+ (testing "should disable remapping when getting fk columns"
+ (mt/with-column-remappings [venues.name {"Red Medicine" "RM",
+ "Stout Burgers & Beers" "SBB",
+ "The Apple Pan" "TAP"}]
+ (mt/with-temp* [Card [{card-id :id}
+ (merge (mt/card-with-source-metadata-for-query
+ (mt/mbql-query venues))
+
+ {:database_id (mt/id)
+ :dataset dataset?
+ :table_id (mt/id :venues)})]]
+
+ (testing "get values returns the value, not remapped values"
+ (is (=? {:has_more_values true,
+ :values ["Red Medicine" "Stout Burgers & Beers" "The Apple Pan"]}
+ (params.card-values/values-from-card
+ card-id
+ (mt/$ids $categories.name)))))
+
+ (testing "search with the value, not remapped values"
+ (is (=? {:has_more_values false,
+ :values ["Red Medicine"]}
+ (params.card-values/values-from-card
+ card-id
+ (mt/$ids $categories.name)
+ "medicine")))))))))))
+
+(deftest with-native-card-test
+ (doseq [dataset? [true false]]
+ (testing (format "source card is a %s with native question" (if dataset? "model" "question"))
+ (binding [params.card-values/*max-rows* 3]
+ (mt/with-temp* [Card [{card-id :id}
+ (merge (mt/card-with-source-metadata-for-query
+ (mt/native-query {:query "select * from venues where lower(name) like '%red%'"}))
+ {:database_id (mt/id)
+ :dataset dataset?
+ :table_id (mt/id :venues)})]]
+ ;; HACK: run the card so the card's result_metadata is available
+ (testing "get values from breakout columns"
+ (is (=? {:has_more_values false,
+ :values ["Red Medicine" "Fred 62"]}
+ (params.card-values/values-from-card
+ card-id
+ [:field "NAME" {:base-type :type/Float}]))))
+
+ (testing "doing case in-sensitve search on breakout columns"
+ (is (=? {:has_more_values false
+ :values ["Red Medicine"]}
+ (params.card-values/values-from-card
+ card-id
+ [:field "NAME" {:base-type :type/Float}]
+ "medicine")))))))))
diff --git a/test/metabase/test.clj b/test/metabase/test.clj
index 2992b6f9676..23a3db99f3b 100644
--- a/test/metabase/test.clj
+++ b/test/metabase/test.clj
@@ -154,6 +154,7 @@
with-bigquery-fks]
[qp.test-util
+ card-with-source-metadata-for-query
store-contents
with-database-timezone-id
with-everything-store
--
GitLab