Field Filter Value Normalization in Embedding Endpoints (#45447)

* Field Filter Value Normalization in Embedding Endpoints

Fixes 27643

Since we use query-params to pass paramter values, we have to normalize the parameter map values.

This PR is a WIP, as I'm trying to find a way to do this hollistically/properly.

First commit here is mostly as a marker for where to start working on this properly, as I think the json parsing won't
catch all cases, nor should the normalization happen without some attempt to use a schema to properly validate, maybe
even coerce things.

* read strings if boolean or numbers, replace blank str with nil

Also update the common dashboard param values fn to use embedding params from the URL blob if the app db doesn't have
embedding params. This happens upon first preview where nothing will have been written to the Appdb yet.

* Add query-param normalization test, add related embedding test

* schema didn't work in this case after all.

Worth a shot, but don't want this to block the PR, so reverting to the previous schema

* Add comment pointing out the downsides/reasons for this solution

It's nto a perfect solution, but it's probably the best we can get at the moment, without adding a new filter
type (boolean) and/or improving the design of hte embedding endpoints and how filters/parameters work

src/metabase/api/dashboard.clj

@@ -1228,10 +1228,10 @@
   "Run the query associated with a Saved Question (`Card`) in the context of a `Dashboard` that includes it."
   [dashboard-id dashcard-id card-id :as {{:keys [parameters], :as body}          :body
                                          {dashboard-load-id "dashboard_load_id"} :query-params}]
-  {dashboard-id  ms/PositiveInt
-   dashcard-id   ms/PositiveInt
-   card-id       ms/PositiveInt
-   parameters    [:maybe [:sequential ParameterWithID]]}
+  {dashboard-id ms/PositiveInt
+   dashcard-id  ms/PositiveInt
+   card-id      ms/PositiveInt
+   parameters   [:maybe [:sequential ParameterWithID]]}
   (with-dashboard-load-id dashboard-load-id
     (u/prog1 (m/mapply qp.dashboard/process-query-for-dashcard
                        (merge

src/metabase/api/embed/common.clj

@@ -118,6 +118,19 @@
                                         :dashboard-parameters parameters})))
             :value value}))))
 
+;; As of 17-07-2024, we don't have boolean filters; they're sorta possible by using a Category filter.
+;; Ideally, instead of normalizing "true" to true directly, we could use the parameter data from the dashboard
+;; to coerce values into the form we'd expect, but in the case of booleans, since we're using a category
+;; filter type, we would NOT coerce "true" to true at all. This leads to bugs, AND is actually different
+;; than what happens in a regular dashboard -> if you look at a regular dashboard with a 'Boolean' filter,
+;; the frontend sends the parameter value as a proper boolean value right away.
+
+;; This normalize is probably not the long-term fix we want, but it patches things up in the embedding
+;; endpoints to more closely match how our regular dashboard endpoint works.
+
+;; TODO: once the app has a notion of Boolean Filters, we can apply query param coercion more intelligently by
+;; using the data in the dashboard's :parameters key.
+
 (mu/defn normalize-query-params :- [:map-of :keyword :any]
   "Take a map of `query-params` and make sure they're in the right format for the rest of our code. Our
   `wrap-keyword-params` middleware normally converts all query params keys to keywords, but only if they seem like
@@ -125,9 +138,22 @@
   not automatically converted. Thus we must do it ourselves here to make sure things are done as we'd expect.
   Also, any param values that are blank strings should be parsed as nil, representing the absence of a value."
   [query-params]
-  (-> query-params
-      (update-keys keyword)
-      (update-vals (fn [v] (if (= v "") nil v)))))
+  (letfn [(maybe-read [v]
+            (if (string? v)
+              (cond
+                (#{"true" "false"} v) (parse-boolean v)
+                (str/blank? v)        nil
+                :else                 (or (parse-long v)
+                                          (parse-double v)
+                                          v))
+              v))]
+    (-> query-params
+        (update-keys keyword)
+        (update-vals (fn [v]
+                       (if (and (not (string? v))
+                                (seq v))
+                         (mapv maybe-read v)
+                         (maybe-read v)))))))
 
 (mu/defn validate-and-merge-params :- [:map-of :keyword :any]
   "Validate that the `token-params` passed in the JWT and the `user-params` (passed as part of the URL) are allowed, and
@@ -425,6 +451,11 @@
         slug-token-params                    (embed/get-in-unsigned-token-or-throw unsigned-token [:params])
         {parameters       :parameters
          embedding-params :embedding_params} (t2/select-one :model/Dashboard :id dashboard-id)
+        ;; when previewing an embed, embedding-params may initially be empty (not in Appdb yet)
+        ;; to prevent an error, use the embedding params from the token in this case
+        embedding-params                     (or embedding-params
+                                                 (when preview
+                                                   (embed/get-in-unsigned-token-or-throw unsigned-token [:_embedding_params])))
         id->slug                             (into {} (map (juxt :id :slug) parameters))
         slug->id                             (into {} (map (juxt :slug :id) parameters))
         searched-param-slug                  (get id->slug searched-param-id)]

test/metabase/api/preview_embed_test.clj

@@ -577,3 +577,62 @@
               (is (= {:values          [["African"] ["American"] ["Asian"]]
                       :has_more_values false}
                      (mt/user-http-request :rasta :get 200 url))))))))))
+
+(deftest parameter-values-are-parsed-and-query-suceeds-test
+  (testing "embedding endpoint parameter values are parsed when sensible. (#27643)"
+    (embed-test/with-embedding-enabled-and-new-secret-key
+      (mt/dataset places-cam-likes
+        (mt/with-temp [:model/Card {card-id :id :as card} {:dataset_query
+                                                           {:database (mt/id)
+                                                            :type     :native
+                                                            :native
+                                                            {:template-tags
+                                                             {"ASDF"
+                                                              {:widget-type  :string/=
+                                                               :default      [true]
+                                                               :name         "ASDF"
+                                                               :type         :dimension
+                                                               :id           "ASDF"
+                                                               :dimension    [:field (mt/id :places :liked) nil]
+                                                               :display-name "Asdf"
+                                                               :options      nil
+                                                               :required     true}}
+                                                             :query "SELECT * FROM PLACES WHERE {{ASDF}}"}}}
+                       :model/Dashboard {dashboard-id :id} {:parameters
+                                                            [{:name      "ASDF"
+                                                              :slug      "ASDF"
+                                                              :id        "ccb91bc"
+                                                              :type      :string/=
+                                                              :sectionId "string"
+                                                              :required  true
+                                                              :default   [true]}]}
+                       :model/DashboardCard dashcard {:dashboard_id dashboard-id
+                                                      :parameter_mappings
+                                                      [{:parameter_id "ccb91bc"
+                                                        :card_id      card-id
+                                                        :target       [:dimension [:template-tag "ASDF"]]}]
+                                                      :card_id      card-id}]
+          (testing "for card embeds"
+            (let [false-url (format "%s?ASDF=false" (card-query-url card {:_embedding_params {:ASDF "enabled"}}))
+                  true-url  (format "%s?ASDF=true" (card-query-url card {:_embedding_params {:ASDF "enabled"}}))]
+              (is (= [[3 "The Dentist" false]]
+                     (-> (mt/user-http-request :crowberto :get 202 false-url)
+                         :data
+                         :rows)))
+              (is (= [[1 "Tempest" true]
+                      [2 "Bullit" true]]
+                     (-> (mt/user-http-request :crowberto :get 202 true-url)
+                         :data
+                         :rows)))))
+          (testing "for dashboard embeds"
+            (let [false-url (format "%s?ASDF=false" (dashcard-url dashcard {:_embedding_params {:ASDF "enabled"}}))
+                  true-url  (format "%s?ASDF=true" (dashcard-url dashcard {:_embedding_params {:ASDF "enabled"}}))]
+              (is (= [[3 "The Dentist" false]]
+                     (-> (mt/user-http-request :crowberto :get 202 false-url)
+                         :data
+                         :rows)))
+              (is (= [[1 "Tempest" true]
+                      [2 "Bullit" true]]
+                     (-> (mt/user-http-request :crowberto :get 202 true-url)
+                         :data
+                         :rows))))))))))