From 431a2ab33056796cd72b72549c2023f75b101cb0 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Mon, 6 Jun 2022 13:23:49 -0400
Subject: [PATCH] Allow parameters to target template tags by ID (#23119)
---
shared/src/metabase/mbql/schema.cljc | 8 +++-
.../driver/common/parameters/values.clj | 44 +++++++++++++------
src/metabase/driver/sql.clj | 2 +-
.../driver/common/parameters/values_test.clj | 35 ++++++++++++++-
4 files changed, 70 insertions(+), 19 deletions(-)
diff --git a/shared/src/metabase/mbql/schema.cljc b/shared/src/metabase/mbql/schema.cljc
index 056db4acccb..22191be2382 100644
--- a/shared/src/metabase/mbql/schema.cljc
+++ b/shared/src/metabase/mbql/schema.cljc
@@ -1370,7 +1370,9 @@
;; examples:
;;
;; {:target [:dimension [:template-tag "my_tag"]]}
+;; {:target [:dimension [:template-tag {:id "my_tag_id"}]]}
;; {:target [:variable [:template-tag "another_tag"]]}
+;; {:target [:variable [:template-tag {:id "another_tag_id"}]]}
;; {:target [:dimension [:field 100 nil]]}
;; {:target [:field 100 nil]}
;;
@@ -1384,10 +1386,12 @@
;; supposed to work, but we have test #18747 that attempts to set it. I'm not convinced this should actually be
;; allowed.
-;; this is the reference like [:template-tag "whatever"], not the [[TemplateTag]] schema for when it's declared in
+;; this is the reference like [:template-tag <whatever>], not the [[TemplateTag]] schema for when it's declared in
;; `:template-tags`
(defclause template-tag
- tag-name helpers/NonBlankString)
+ tag-name
+ (s/cond-pre helpers/NonBlankString
+ {:id helpers/NonBlankString}))
(defclause dimension
target (s/cond-pre Field template-tag))
diff --git a/src/metabase/driver/common/parameters/values.clj b/src/metabase/driver/common/parameters/values.clj
index d18171d5dd8..633a1848032 100644
--- a/src/metabase/driver/common/parameters/values.clj
+++ b/src/metabase/driver/common/parameters/values.clj
@@ -53,16 +53,32 @@
(s/named (s/maybe (s/cond-pre params/SingleValue MultipleValues su/Map))
"Valid param value(s)"))
-(s/defn ^:private params-with-target
- "Return `params` with a matching `target`. `target` is something like:
-
- [:dimension [:template-tag <param-name>]] ; for FieldFilters (Field Filters)
- [:variable [:template-tag <param-name>]] ; for other types of params"
- [params :- (s/maybe [mbql.s/Parameter]) target :- mbql.s/ParameterTarget]
- (seq (for [param params
- :when (= (:target param) target)]
- param)))
-
+(s/defn ^:private tag-targets
+ "Given a template tag, returns a set of `target` structures that can be used to target the tag.
+ Potential targets look something like:
+
+ [:dimension [:template-tag {:id <param-id>}]
+ [:dimension [:template-tag <param-name>]] ; for Field Filters
+
+ [:variable [:template-tag {:id <param-id>}]]
+ [:variable [:template-tag <param-name>]] ; for other types of params
+
+ Targeting template tags by ID is preferable (as of version 44) but targeting by name is supported for backwards
+ compatibility."
+ [tag :- mbql.s/TemplateTag]
+ (let [target-type (case (:type tag)
+ :dimension :dimension
+ :variable)]
+ #{[target-type [:template-tag (:name tag)]]
+ [target-type [:template-tag {:id (:id tag)}]]}))
+
+(s/defn ^:private tag-params
+ "Return params from the provided `params` list targeting the provided `tag`."
+ [tag :- mbql.s/TemplateTag params :- (s/maybe [mbql.s/Parameter])]
+ (let [targets (tag-targets tag)]
+ (seq (for [param params
+ :when (contains? targets (:target param))]
+ param))))
;;; FieldFilter Params (Field Filters) (e.g. WHERE {{x}})
@@ -79,7 +95,7 @@
"Get parameter value(s) for a Field filter. Returns map if there is a normal single value, or a vector of maps for
multiple values."
[tag :- mbql.s/TemplateTag params :- (s/maybe [mbql.s/Parameter])]
- (let [matching-params (params-with-target params [:dimension [:template-tag (:name tag)]])
+ (let [matching-params (tag-params tag params)
normalize-params (fn [params]
;; remove `:target` which is no longer needed after this point.
(let [params (map #(dissoc % :target) params)]
@@ -162,9 +178,9 @@
(s/defn ^:private param-value-for-raw-value-tag
"Get the value that should be used for a raw value (i.e., non-Field filter) template tag from `params`."
- [{tag-name :name, :as tag} :- mbql.s/TemplateTag
- params :- (s/maybe [mbql.s/Parameter])]
- (let [matching-param (when-let [matching-params (not-empty (params-with-target params [:variable [:template-tag tag-name]]))]
+ [tag :- mbql.s/TemplateTag
+ params :- (s/maybe [mbql.s/Parameter])]
+ (let [matching-param (when-let [matching-params (not-empty (tag-params tag params))]
;; double-check and make sure we didn't end up with multiple mappings or something crazy like that.
(when (> (count matching-params) 1)
(throw (ex-info (tru "Error: multiple values specified for parameter; non-Field Filter parameters can only have one value.")
diff --git a/src/metabase/driver/sql.clj b/src/metabase/driver/sql.clj
index 6510069cf07..19ae3665670 100644
--- a/src/metabase/driver/sql.clj
+++ b/src/metabase/driver/sql.clj
@@ -11,7 +11,7 @@
[potemkin :as p]
[schema.core :as s]))
-(comment sql.params.substitution/keep-me) ; this is so `cljr-clean-ns` and the liner don't remove the `:require`
+(comment sql.params.substitution/keep-me) ; this is so `cljr-clean-ns` and the linter don't remove the `:require`
(driver/register! :sql, :abstract? true)
diff --git a/test/metabase/driver/common/parameters/values_test.clj b/test/metabase/driver/common/parameters/values_test.clj
index 901e7ba5e69..a7bfb3957c4 100644
--- a/test/metabase/driver/common/parameters/values_test.clj
+++ b/test/metabase/driver/common/parameters/values_test.clj
@@ -14,15 +14,25 @@
[metabase.util.schema :as su]
[schema.core :as s])
(:import clojure.lang.ExceptionInfo
+ java.util.UUID
metabase.driver.common.parameters.ReferencedCardQuery))
+(def ^:private test-uuid (str (UUID/randomUUID)))
+
(deftest variable-value-test
(mt/with-everything-store
- (testing "Specified value"
+ (testing "Specified value, targeted by name"
(is (= "2"
(#'params.values/value-for-tag
{:name "id", :display-name "ID", :type :text, :required true, :default "100"}
[{:type :category, :target [:variable [:template-tag "id"]], :value "2"}]))))
+
+ (testing "Specified value, targeted by ID"
+ (is (= "2"
+ (#'params.values/value-for-tag
+ {:name "id", :id test-uuid, :display-name "ID", :type :text, :required true, :default "100"}
+ [{:type :category, :target [:variable [:template-tag {:id test-uuid}]], :value "2"}]))))
+
(testing "Multiple values with new operators"
(is (= 20
(#'params.values/value-for-tag
@@ -62,7 +72,7 @@
(deftest field-filter-test
(testing "specified"
- (testing "date range for a normal :type/Temporal field"
+ (testing "date range for a normal :type/Temporal field, targeted by name"
(is (= {:field (extra-field-info
{:id (mt/id :checkins :date)
:name "DATE"
@@ -82,6 +92,27 @@
:target [:dimension [:template-tag "checkin_date"]]
:value "2015-04-01~2015-05-01"}]))))
+ (testing "date range for a normal :type/Temporal field, targeted by id"
+ (is (= {:field (extra-field-info
+ {:id (mt/id :checkins :date)
+ :name "DATE"
+ :parent_id nil
+ :table_id (mt/id :checkins)
+ :base_type :type/Date
+ :semantic_type nil})
+ :value {:type :date/range
+ :value "2015-04-01~2015-05-01"}}
+ (value-for-tag
+ {:name "checkin_date"
+ :id test-uuid
+ :display-name "Checkin Date"
+ :type :dimension
+ :dimension [:field (mt/id :checkins :date) nil]
+ :widget-type :date/all-options}
+ [{:type :date/range
+ :target [:dimension [:template-tag {:id test-uuid}]]
+ :value "2015-04-01~2015-05-01"}]))))
+
(testing "date range for a UNIX timestamp field should work just like a :type/Temporal field (#11934)"
(mt/dataset tupac-sightings
(mt/$ids sightings
--
GitLab