Fix handling dimension parameters using a foreign key

[ci drivers]

src/metabase/mbql/normalize.clj

@@ -202,10 +202,11 @@
                 (cond-> tag-def
                   (:widget-type tag-def) (update :widget-type mbql.u/normalize-token)))])))
 
-(defn- normalize-query-parameter [param]
-  (-> param
-      (update :type mbql.u/normalize-token)
-      (update :target #(normalize-tokens % :ignore-path))))
+(defn- normalize-query-parameter [{:keys [type target], :as param}]
+  (cond-> param
+    ;; some things that get ran thru here, like dashcard param targets, do not have :type
+    type   (update :type mbql.u/normalize-token)
+    target (update :target #(normalize-tokens % :ignore-path))))
 
 (defn- normalize-source-query [{native? :native, :as source-query}]
   (normalize-tokens source-query [(if native? :native :query)]))
@@ -214,7 +215,7 @@
   (-> metadata
       (update :base_type    keyword)
       (update :special_type keyword)
-      (update :fingerprint  walk/keywordize-keys )))
+      (update :fingerprint  walk/keywordize-keys)))
 
 (def ^:private path->special-token-normalization-fn
   "Map of special functions that should be used to perform token normalization for a given path. For example, the

src/metabase/models/interface.clj

@@ -93,9 +93,8 @@
 
 ;; For DashCard parameter lists
 (defn- normalize-parameter-mapping-targets [parameter-mappings]
-  (for [{:keys [target], :as mapping} parameter-mappings]
-    (cond-> mapping
-      target (update :target normalize/normalize-tokens :ignore-path))))
+  (or (normalize/normalize-fragment [:parameters] parameter-mappings)
+      []))
 
 (models/add-type! :parameter-mappings
   :in  (comp json-in normalize-parameter-mapping-targets)

src/metabase/models/params.clj

@@ -5,7 +5,10 @@
              [db :as mdb]
              [util :as u]]
             [metabase.mbql.util :as mbql.u]
-            [metabase.util.i18n :as ui18n :refer [trs]]
+            [metabase.util
+             [i18n :as ui18n :refer [trs]]
+             [schema :as su]]
+            [schema.core :as s]
             [toucan
              [db :as db]
              [hydrate :refer [hydrate]]]))
@@ -14,23 +17,15 @@
 ;;; |                                                     SHARED                                                     |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn field-form->id
+(s/defn field-form->id :- su/IntGreaterThanZero
   "Expand a `field-id` or `fk->` FORM and return the ID of the Field it references. Also handles unwrapped integers.
 
-     (field-form->id [:field-id 100])  ; -> 100"
+      (field-form->id [:field-id 100])  ; -> 100"
   [field-form]
-  (cond
-    (mbql.u/is-clause? :field-id field-form)
-    (second field-form)
-
-    (mbql.u/is-clause? :fk-> field-form)
-    (last field-form)
-
-    (integer? field-form)
+  (if (integer? field-form)
     field-form
-
-    :else
-    (throw (IllegalArgumentException. (str (trs "Don't know what to do with:") " " field-form)))))
+    ;; TODO - what are we supposed to do if `field-form` is a field literal?
+    (mbql.u/field-clause->id-or-literal field-form)))
 
 (defn wrap-field-id-if-needed
   "Wrap a raw Field ID in a `:field-id` clause if needed."

src/metabase/query_processor/middleware/parameters/mbql.clj

@@ -7,14 +7,15 @@
              [field :refer [Field]]
              [params :as params]]
             [metabase.query-processor.middleware.parameters.dates :as date-params]
+            [metabase.util.schema :as su]
             [schema.core :as s]
             [toucan.db :as db]))
 
-(defn- parse-param-value-for-type
+(s/defn ^:private parse-param-value-for-type
   "Convert `param-value` to a type appropriate for `param-type`.
   The frontend always passes parameters in as strings, which is what we want in most cases; for numbers, instead
   convert the parameters to integers or floating-point numbers."
-  [param-type param-value field-id]
+  [param-type param-value, field-id :- su/IntGreaterThanZero]
   (cond
     ;; for `id` type params look up the base-type of the Field and see if it's a number or not. If it *is* a number
     ;; then recursively call this function and parse the param value as a number as appropriate.

test/metabase/http_client.clj

@@ -72,7 +72,7 @@
   (try
     (:id (client :post 200 "session" credentials))
     (catch Throwable e
-      (log/error "Failed to authenticate with username:" username "and password:" password ":" (.getMessage e)))))
+      (println "Failed to authenticate with username:" username "and password:" password ":" (.getMessage e)))))
 
 
 ;;; client
@@ -97,7 +97,7 @@
                       (json/parse-string body keyword)
                       (catch Throwable _
                         body))]
-        (log/error (u/pprint-to-str 'red body))
+        (println (u/pprint-to-str 'red body))
         (throw (ex-info message {:status-code actual-status-code}))))))
 
 (def ^:private method->request-fn

test/metabase/models/interface_test.clj

@@ -44,9 +44,8 @@
   ((type-fn :metric-segment-definition :in)
    {:filter 1000}))
 
-;; same for `:parameter-mappings`. I wasn't actually able to figure out any input that would make this fail, so cheat
-;; and override the `normalization-tokens` function to always throw an Exception so we can make sure the Toucan type
-;; fn handles the error gracefully
+;; Cheat ;; and override the `normalization-tokens` function to always throw an Exception so we can make sure the
+;; Toucan type fn handles the error gracefully
 (expect
   nil
   (tu.log/suppress-output
@@ -62,3 +61,25 @@
   (with-redefs [normalize/normalize-tokens (fn [& _] (throw (Exception. "BARF")))]
     ((type-fn :parameter-mappings :in)
      [{:target [:dimension [:field-id "ABC"]]}])))
+
+;; make sure parameter mappings correctly normalize things like fk->
+(expect
+  [{:target [:dimension [:fk-> [:field-id 23] [:field-id 30]]]}]
+  ((type-fn :parameter-mappings :out)
+   (json/generate-string
+    [{:target [:dimension [:fk-> 23 30]]}])))
+
+;; ...but parameter mappings we should not normalize things like :target
+(expect
+  [{:card-id 123, :hash "abc", :target "foo"}]
+  ((type-fn :parameter-mappings :out)
+   (json/generate-string
+    [{:card-id 123, :hash "abc", :target "foo"}])))
+
+;; we should keep empty parameter mappings as empty instead of making them nil (if `normalize` removes them because
+;; they are empty)
+;; (I think this is to prevent NPEs on the FE? Not sure why we do this)
+(expect
+  []
+  ((type-fn :parameter-mappings :out)
+   (json/generate-string [])))

test/metabase/query_processor/middleware/parameters/mbql_test.clj

@@ -3,12 +3,14 @@
   (:require [expectations :refer [expect]]
             [metabase
              [query-processor :as qp]
-             [query-processor-test :refer [first-row format-rows-by non-timeseries-engines rows]]]
+             [query-processor-test :as qp.test :refer [first-row format-rows-by non-timeseries-engines rows]]
+             [util :as u]]
             [metabase.mbql.normalize :as normalize]
             [metabase.query-processor.middleware.parameters.mbql :as mbql-params]
             [metabase.test
              [data :as data]
              [util :as tu]]
+            [metabase.driver :as driver]
             [metabase.test.data.datasets :as datasets]
             [metabase.util.date :as du]))
 
@@ -262,7 +264,7 @@
                                        :type   "date/month"
                                        :target [:field-id (data/id :checkins :date)]
                                        :value  ["2014-06" "2015-06"]}]))]
-    (-> query qp/process-query :data :native_form)))
+    (-> query qp/process-query qp.test/data :native_form)))
 
 ;; make sure that "ID" type params get converted to numbers when appropriate
 (expect
@@ -272,3 +274,25 @@
                                       :slug   "venue_id"
                                       :value  "1"
                                       :name   "Venue ID"}))
+
+;; Make sure we properly handle paramters that have `fk->` forms in `:dimension` targets (#9017)
+(datasets/expect-with-engines (filter #(driver/driver-supports? (driver/engine->driver %) :foreign-keys)
+                                      params-test-engines)
+  [[31 "Bludso's BBQ" 5 33.8894 -118.207 2]
+   [32 "Boneyard Bistro" 5 34.1477 -118.428 3]
+   [33 "My Brother's Bar-B-Q" 5 34.167 -118.595 2]
+   [35 "Smoke City Market" 5 34.1661 -118.448 1]
+   [37 "bigmista's barbecue" 5 34.118 -118.26 2]
+   [38 "Zeke's Smokehouse" 5 34.2053 -118.226 2]
+   [39 "Baby Blues BBQ" 5 34.0003 -118.465 2]]
+  (qp.test/format-rows-by [int str int (partial u/round-to-decimals 4) (partial u/round-to-decimals 4) int]
+    (qp.test/rows
+      (qp/process-query
+        (data/$ids venues
+          {:database   (data/id)
+           :type       :query
+           :query      {:source-table $$table
+                        :order-by     [[:asc $id]]}
+           :parameters [{:type   :id
+                         :target [:dimension [:fk-> $category_id $categories.name]]
+                         :value  ["BBQ"]}]})))))