diff --git a/src/metabase/query_processor/card.clj b/src/metabase/query_processor/card.clj
index b1f81f7d090577aa5d8e26591e4a77eb2227f12a..3cff5cc33f4359ccc9ed4d70d29690462b7e1425 100644
--- a/src/metabase/query_processor/card.clj
+++ b/src/metabase/query_processor/card.clj
@@ -71,17 +71,38 @@
type of the parameter has to agree with the type of the template tag as well. This variable controls whether or not
this constraint is enforced.
+ In 0.44.0+ explicit parameters can be defined on MBQL cards as well. Parameters supplied in a request must correspond
+ by ID to parameters defined explicitly on the card and have an appropriate type, unless this variable is `true`.
+
Normally, when running a query in the context of a /Card/, this is `false`, and the constraint is enforced. By
binding this to a truthy value you can disable the checks. Currently this is only done
by [[metabase.query-processor.dashboard]], which does its own parameter validation before handing off to the code
here."
false)
-(defn- card-template-tag-parameters
- "Template tag parameters that have been specified for the query for Card with `card-id`, if any, returned as a map in
+(defn- card-parameters
+ "Parameters on provided Card, returned as a map with the format
+
+ {\"parameter_id\" :parameter-type ...}
+
+ This allows the expected type of a request parameter targeting a field on an MBQL query to be quickly looked up by ID.
+ Excludes parameters that do not have IDs."
+ [{parameters :parameters}]
+ (into {} (comp
+ (map (juxt :id :type))
+ (remove (comp nil? first)))
+ parameters))
+
+(defn- card-template-tags
+ "Template tags that have been specified for the query in the provided Card, if any, returned as a map in
the format
- {\"template_tag_parameter_name\" :parameter-type, ...}
+ {\"template_tag_name\" :parameter-type,
+ \"template_tag_id\" :parameter-type
+ ...}
+
+ This allows the expected type of a request parameter targeting a template tag to be quickly looked up by *either*
+ name or ID.
Template tag parameter name is the name of the parameter as it appears in the query, e.g. `{{id}}` has the `:name`
`\"id\"`.
@@ -89,23 +110,22 @@
Parameter type in this case is something like `:string` or `:number` or `:date/month-year`; parameters passed in as
parameters to the API request must be allowed for this type (i.e. `:string/=` is allowed for a `:string` parameter,
but `:number/=` is not)."
- [card-id]
- (let [query (api/check-404 (db/select-one-field :dataset_query Card :id card-id))]
- (into
- {}
- (comp
- (map (fn [[param-name {widget-type :widget-type, tag-type :type}]]
- ;; Field Filter parameters have a `:type` of `:dimension` and the widget type that should be used is
- ;; specified by `:widget-type`. Non-Field-filter parameters just have `:type`. So prefer
- ;; `:widget-type` if available but fall back to `:type` if not.
- (cond
- (= tag-type :dimension)
- [param-name widget-type]
-
- (contains? mbql.s/raw-value-template-tag-types tag-type)
- [param-name tag-type])))
- (filter some?))
- (get-in query [:native :template-tags]))))
+ [{query :dataset_query}]
+ (into
+ {}
+ (comp
+ (mapcat
+ (fn [[param-name {widget-type :widget-type, tag-type :type, id :id}]]
+ ;; Field Filter parameters have a `:type` of `:dimension` and the widget type that should be used is
+ ;; specified by `:widget-type`. Non-Field-filter parameters just have `:type`. So prefer
+ ;; `:widget-type` if available but fall back to `:type` if not.
+ (let [param-type (cond
+ (= tag-type :dimension) widget-type
+ (contains? mbql.s/raw-value-template-tag-types tag-type) tag-type)]
+ [[id param-type]
+ [param-name param-type]])))
+ (filter some?))
+ (get-in query [:native :template-tags])))
(defn- allowed-parameter-type-for-template-tag-widget-type? [parameter-type widget-type]
(when-let [allowed-template-tag-types (get-in mbql.s/parameter-types [parameter-type :allowed-for])]
@@ -150,19 +170,41 @@
(s/defn ^:private validate-card-parameters
"Unless [[*allow-arbitrary-mbql-parameters*]] is truthy, check to make all supplied `parameters` actually match up
- with template tags in the query for Card with `card-id`."
- [card-id :- su/IntGreaterThanZero parameters :- mbql.s/ParameterList]
+ with `parameters` defined on the Card with `card-id`, or template tags on the query."
+ [card-id :- su/IntGreaterThanZero request-parameters :- mbql.s/ParameterList]
(when-not *allow-arbitrary-mbql-parameters*
- (let [template-tags (card-template-tag-parameters card-id)]
- (doseq [request-parameter parameters
- :let [parameter-name (infer-parameter-name request-parameter)]]
- (let [matching-widget-type (or (get template-tags parameter-name)
- (throw (ex-info (tru "Invalid parameter: Card {0} does not have a template tag named {1}."
- card-id
- (pr-str parameter-name))
- {:type qp.error-type/invalid-parameter
- :invalid-parameter request-parameter
- :allowed-parameters (keys template-tags)})))]
+ (let [card (api/check-404 (db/select-one [Card :dataset_query :parameters] :id card-id))
+ types-on-template-tags (card-template-tags card)
+ types-on-parameters (card-parameters card)]
+ (doseq [request-parameter request-parameters
+ :let [parameter-id (:id request-parameter)
+ parameter-name (infer-parameter-name request-parameter)]]
+ (let [matching-widget-type
+ (if (seq types-on-parameters)
+ ;; If (non-template tag) parameters are defined on the card, request parameters should target them by
+ ;; ID, rather than targeting template tags directly (even if they also exist)
+ (or (get types-on-parameters parameter-id)
+ (throw (ex-info (if parameter-id
+ (tru "Invalid parameter: Card {0} does not have a parameter with the ID {1}."
+ card-id
+ (pr-str parameter-id))
+ (tru "Invalid parameter: missing id"))
+ {:type qp.error-type/invalid-parameter
+ :invalid-parameter request-parameter
+ :allowed-parameters (keys types-on-parameters)})))
+ (or
+ ;; Use ID preferentially, but fallback to name if ID is not present, as a safety net in case request
+ ;; parameters are malformed. Only need to do this for parameters targeting template tags since card
+ ;; parameters should always be targeted by ID.
+ (get types-on-template-tags parameter-id)
+ (get types-on-template-tags parameter-name)
+ (throw (ex-info (tru "Invalid parameter: Card {0} does not have a template tag with the ID {1} or name {2}."
+ card-id
+ (pr-str parameter-id)
+ (pr-str parameter-name))
+ {:type qp.error-type/invalid-parameter
+ :invalid-parameter request-parameter
+ :allowed-parameters (keys types-on-template-tags)}))))]
;; now make sure the type agrees as well
(check-allowed-parameter-value-type parameter-name matching-widget-type (:type request-parameter)))))))
diff --git a/test/metabase/query_processor/card_test.clj b/test/metabase/query_processor/card_test.clj
index 0b7ccf186581dc80b966ee81472266aaafe9c9b8..0df68c1a2d6817f42fa81e07b9f5b0ae1b37937b 100644
--- a/test/metabase/query_processor/card_test.clj
+++ b/test/metabase/query_processor/card_test.clj
@@ -74,33 +74,33 @@
:default "1"}}
:query "SELECT *\nFROM ORDERS\nWHERE id = {{id}}"}})
-(deftest card-template-tag-parameters-test
+(deftest card-template-tags-test
(testing "Card with a Field filter parameter"
- (mt/with-temp Card [{card-id :id} {:dataset_query (field-filter-query)}]
- (is (= {"date" :date/all-options}
- (#'qp.card/card-template-tag-parameters card-id)))))
+ (mt/with-temp Card [card {:dataset_query (field-filter-query)}]
+ (is (= {"date" :date/all-options, "_DATE_" :date/all-options}
+ (#'qp.card/card-template-tags card)))))
(testing "Card with a non-Field-filter parameter"
- (mt/with-temp Card [{card-id :id} {:dataset_query (non-field-filter-query)}]
- (is (= {"id" :number}
- (#'qp.card/card-template-tag-parameters card-id)))))
+ (mt/with-temp Card [card {:dataset_query (non-field-filter-query)}]
+ (is (= {"id" :number, "_ID_" :number}
+ (#'qp.card/card-template-tags card)))))
(testing "Should ignore native query snippets and source card IDs"
- (mt/with-temp Card [{card-id :id} {:dataset_query (assoc (non-field-filter-query)
- "abcdef"
- {:id "abcdef"
- :name "#1234"
- :display-name "#1234"
- :type :card
- :card-id 1234}
+ (mt/with-temp Card [card {:dataset_query (assoc (non-field-filter-query)
+ "abcdef"
+ {:id "abcdef"
+ :name "#1234"
+ :display-name "#1234"
+ :type :card
+ :card-id 1234}
- "xyz"
- {:id "xyz"
- :name "snippet: My Snippet"
- :display-name "Snippet: My Snippet"
- :type :snippet
- :snippet-name "My Snippet"
- :snippet-id 1})}]
- (is (= {"id" :number}
- (#'qp.card/card-template-tag-parameters card-id))))))
+ "xyz"
+ {:id "xyz"
+ :name "snippet: My Snippet"
+ :display-name "Snippet: My Snippet"
+ :type :snippet
+ :snippet-name "My Snippet"
+ :snippet-id 1})}]
+ (is (= {"id" :number, "_ID_" :number}
+ (#'qp.card/card-template-tags card))))))
(deftest infer-parameter-name-test
(is (= "my_param"
@@ -110,20 +110,20 @@
(is (= nil
(#'qp.card/infer-parameter-name {:target [:field 1000 nil]}))))
-(deftest validate-card-parameters-test
+(deftest validate-card-template-tag-test
(mt/with-temp Card [{card-id :id} {:dataset_query (field-filter-query)}]
(testing "Should disallow parameters that aren't actually part of the Card"
(is (thrown-with-msg?
clojure.lang.ExceptionInfo
- #"Invalid parameter: Card [\d,]+ does not have a template tag named \"fake\""
+ #"Invalid parameter: Card [\d,]+ does not have a template tag with the ID \"_FAKE_\" or name \"fake\""
(#'qp.card/validate-card-parameters card-id [{:id "_FAKE_"
:name "fake"
:type :date/single
:value "2016-01-01"}])))
(testing "As an API request"
- (is (schema= {:message #"Invalid parameter: Card [\d,]+ does not have a template tag named \"fake\".+"
+ (is (schema= {:message #"Invalid parameter: Card [\d,]+ does not have a template tag with the ID \"_FAKE_\" or name \"fake\""
:invalid-parameter (s/eq {:id "_FAKE_", :name "fake", :type "date/single", :value "2016-01-01"})
- :allowed-parameters (s/eq ["date"])
+ :allowed-parameters (s/eq ["_DATE_" "date"])
s/Keyword s/Any}
(mt/user-http-request :rasta :post (format "card/%d/query" card-id)
{:parameters [{:id "_FAKE_"
@@ -154,10 +154,50 @@
(is (= nil
(validate disallowed-type))))))))))
- (testing "Happy path -- API request should succeed if parameter is valid"
+ (testing "Happy path -- API request should succeed if parameter correlates to a template tag by ID"
+ (is (= [1000]
+ (mt/first-row (mt/user-http-request :rasta :post (format "card/%d/query" card-id)
+ {:parameters [{:id "_DATE_"
+ :type :date/single
+ :value "2016-01-01"}]})))))
+
+ (testing "Happy path -- API request should succeed if parameter correlates to a template tag by name"
+ (is (= [1000]
+ (mt/first-row (mt/user-http-request :rasta :post (format "card/%d/query" card-id)
+ {:parameters [{:name "date"
+ :type :date/single
+ :value "2016-01-01"}]})))))))
+
+(deftest validate-card-parameters-test
+ (mt/with-temp Card [{card-id :id} {:dataset_query (mt/mbql-query checkins {:aggregation [[:count]]})
+ :parameters [{:id "_DATE_"
+ :type :date/single
+ :name "Date"
+ :slug "DATE"}]}]
+ (testing "API request should fail if request parameter does not contain ID"
+ (is (schema= {:message #"Invalid parameter: missing id"
+ :invalid-parameter (s/eq {:name "date", :type "date/single", :value "2016-01-01"})
+ :allowed-parameters (s/eq ["_DATE_"])
+ s/Keyword s/Any}
+ (mt/user-http-request :rasta :post (format "card/%d/query" card-id)
+ {:parameters [{:name "date"
+ :type :date/single
+ :value "2016-01-01"}]}))))
+
+ (testing "API request should fail if request parameter ID does not exist on the card"
+ (is (schema= {:message #"Invalid parameter: Card [\d,]+ does not have a parameter with the ID \"_FAKE_\"."
+ :invalid-parameter (s/eq {:name "date", :id "_FAKE_" :type "date/single", :value "2016-01-01"})
+ :allowed-parameters (s/eq ["_DATE_"])
+ s/Keyword s/Any}
+ (mt/user-http-request :rasta :post (format "card/%d/query" card-id)
+ {:parameters [{:id "_FAKE_"
+ :name "date"
+ :type :date/single
+ :value "2016-01-01"}]}))))
+
+ (testing "Happy path -- API request should succeed if request parameter correlates to a card parameter by ID"
(is (= [1000]
(mt/first-row (mt/user-http-request :rasta :post (format "card/%d/query" card-id)
{:parameters [{:id "_DATE_"
- :name "date"
:type :date/single
:value "2016-01-01"}]})))))))