Allow parameters to target template tags by ID (#23119)

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))

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.")

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)
 

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