Skip to content
Snippets Groups Projects
Unverified Commit e1232397 authored by Alexander Polyankin's avatar Alexander Polyankin Committed by GitHub
Browse files

Pass parameter values as JSON (#46250)

parent cbbcf4c8
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
Showing
with 111 additions and 72 deletions
e2e/test/scenarios/dashboard/tabs.cy.spec.js
+ 2
− 2
View file @ e1232397
......@@ -614,12 +614,12 @@ describe("scenarios > dashboard > tabs", () => {
cy.intercept(
"GET",
`/api/embed/dashboard/*/dashcard/*/card/${ORDERS_QUESTION_ID}`,
`/api/embed/dashboard/*/dashcard/*/card/${ORDERS_QUESTION_ID}*`,
cy.spy().as("firstTabQuerySpy"),
).as("firstTabQuery");
cy.intercept(
"GET",
`/api/embed/dashboard/*/dashcard/*/card/${ORDERS_COUNT_QUESTION_ID}`,
`/api/embed/dashboard/*/dashcard/*/card/${ORDERS_COUNT_QUESTION_ID}*`,
cy.spy().as("secondTabQuerySpy"),
).as("secondTabQuery");
......
frontend/src/metabase/dashboard/actions/data-fetching.js
+ 2
− 3
View file @ e1232397
......@@ -236,9 +236,8 @@ export const fetchCardData = createThunkAction(
token: dashcard.dashboard_id,
dashcardId: dashcard.id,
cardId: card.id,
...getParameterValuesBySlug(
dashboard.parameters,
parameterValues,
parameters: JSON.stringify(
getParameterValuesBySlug(dashboard.parameters, parameterValues),
),
ignore_cache: ignoreCache,
},
......
frontend/src/metabase/public/containers/PublicOrEmbeddedQuestion/PublicOrEmbeddedQuestion.tsx
+ 3
− 1
View file @ e1232397
......@@ -126,7 +126,9 @@ export const PublicOrEmbeddedQuestion = ({
card,
)({
token,
...getParameterValuesBySlug(parameters, parameterValues),
parameters: JSON.stringify(
getParameterValuesBySlug(parameters, parameterValues),
),
});
} else if (uuid) {
// public links currently apply parameters client-side
......
frontend/src/metabase/redux/downloads.ts
+ 8
− 5
View file @ e1232397
......@@ -96,7 +96,10 @@ const getDatasetParams = ({
return {
method: "GET",
url: `/api/embed/dashboard/${token}/dashcard/${dashcardId}/card/${cardId}/${type}`,
params: Urls.getEncodedUrlSearchParams({ ...params, format_rows }),
params: new URLSearchParams({
parameters: JSON.stringify(params),
format_rows,
}),
};
}
......@@ -140,14 +143,14 @@ const getDatasetParams = ({
const isEmbeddedQuestion = token != null;
if (isEmbeddedQuestion) {
// For whatever wacky reason the /api/embed endpoint expect params like ?key=value instead
// of like ?params=<json-encoded-params-array> like the other endpoints do.
const params = new URLSearchParams(window.location.search);
params.set("format_rows", format_rows);
return {
method: "GET",
url: Urls.embedCard(token, type),
params,
params: new URLSearchParams({
parameters: JSON.stringify(Object.fromEntries(params)),
format_rows,
}),
};
}
......
src/metabase/api/embed.clj
+ 18
− 11
View file @ e1232397
......@@ -62,7 +62,7 @@
:card-id card-id
:token-params (embed/get-in-unsigned-token-or-throw unsigned-token [:params])
:embedding-params (t2/select-one-fn :embedding_params Card :id card-id)
:query-params query-params
:query-params (api.embed.common/parse-query-params query-params)
:qp qp
:constraints constraints
:options options)))
......@@ -75,7 +75,7 @@
{:resource {:question <card-id>}
:params <parameters>}"
[token & query-params]
(run-query-for-unsigned-token-async (embed/unsign token) :api query-params))
(run-query-for-unsigned-token-async (embed/unsign token) :api (api.embed.common/parse-query-params query-params)))
(api/defendpoint GET ["/card/:token/query/:export-format", :export-format api.dataset/export-format-regex]
"Like `GET /api/embed/card/query`, but returns the results as a file in the specified format."
......@@ -85,7 +85,7 @@
(run-query-for-unsigned-token-async
(embed/unsign token)
export-format
(dissoc (m/map-keys keyword query-params) :format_rows)
(api.embed.common/parse-query-params (dissoc (m/map-keys keyword query-params) :format_rows))
:constraints nil
:middleware {:process-viz-settings? true
:js-int-to-string? false
......@@ -132,7 +132,7 @@
:card-id card-id
:embedding-params (t2/select-one-fn :embedding_params Dashboard :id dashboard-id)
:token-params (embed/get-in-unsigned-token-or-throw unsigned-token [:params])
:query-params (dissoc query-params :format_rows)
:query-params (api.embed.common/parse-query-params (dissoc query-params :format_rows))
:constraints constraints
:qp qp
:middleware middleware)))
......@@ -143,7 +143,8 @@
[token dashcard-id card-id & query-params]
{dashcard-id ms/PositiveInt
card-id ms/PositiveInt}
(u/prog1 (process-query-for-dashcard-with-signed-token token dashcard-id card-id :api query-params)
(u/prog1 (process-query-for-dashcard-with-signed-token token dashcard-id card-id :api
(api.embed.common/parse-query-params query-params))
(events/publish-event! :event/card-read {:object-id card-id, :user-id api/*current-user-id*, :context :dashboard})))
......@@ -230,7 +231,7 @@
:export-format api.dataset/export-format-regex]
"Fetch the results of running a Card belonging to a Dashboard using a JSON Web Token signed with the
`embedding-secret-key` return the data in one of the export formats"
[token export-format dashcard-id card-id format_rows, :as {:keys [query-params]}]
[token export-format dashcard-id card-id format_rows :as {:keys [query-params]}]
{dashcard-id ms/PositiveInt
card-id ms/PositiveInt
format_rows [:maybe :boolean]
......@@ -239,7 +240,7 @@
dashcard-id
card-id
export-format
(m/map-keys keyword query-params)
(api.embed.common/parse-query-params (dissoc (m/map-keys keyword query-params) :format_rows))
:constraints nil
:middleware {:process-viz-settings? true
:js-int-to-string? false
......@@ -258,12 +259,14 @@
(api/defendpoint GET "/dashboard/:token/params/:param-key/values"
"Embedded version of chain filter values endpoint."
[token param-key :as {:keys [query-params]}]
(api.embed.common/dashboard-param-values token param-key nil query-params))
(api.embed.common/dashboard-param-values token param-key nil
(api.embed.common/parse-query-params query-params)))
(api/defendpoint GET "/dashboard/:token/params/:param-key/search/:prefix"
"Embedded version of chain filter search endpoint."
[token param-key prefix :as {:keys [query-params]}]
(api.embed.common/dashboard-param-values token param-key prefix query-params))
(api.embed.common/dashboard-param-values token param-key prefix
(api.embed.common/parse-query-params query-params)))
(api/defendpoint GET "/card/:token/params/:param-key/values"
"Embedded version of api.card filter values endpoint."
......@@ -296,7 +299,9 @@
{:resource {:question <card-id>}
:params <parameters>}"
[token & query-params]
(run-query-for-unsigned-token-async (embed/unsign token) :api query-params :qp qp.pivot/run-pivot-query))
(run-query-for-unsigned-token-async (embed/unsign token)
:api (api.embed.common/parse-query-params query-params)
:qp qp.pivot/run-pivot-query))
(api/defendpoint GET "/pivot/dashboard/:token/dashcard/:dashcard-id/card/:card-id"
"Fetch the results of running a Card belonging to a Dashboard using a JSON Web Token signed with the
......@@ -304,7 +309,9 @@
[token dashcard-id card-id & query-params]
{dashcard-id ms/PositiveInt
card-id ms/PositiveInt}
(u/prog1 (process-query-for-dashcard-with-signed-token token dashcard-id card-id :api query-params :qp qp.pivot/run-pivot-query)
(u/prog1 (process-query-for-dashcard-with-signed-token token dashcard-id card-id
:api (api.embed.common/parse-query-params query-params)
:qp qp.pivot/run-pivot-query)
(events/publish-event! :event/card-read {:object-id card-id, :user-id api/*current-user-id*, :context :dashboard})))
(api/define-routes)
src/metabase/api/embed/common.clj
+ 18
− 27
View file @ e1232397
(ns metabase.api.embed.common
(:require
[cheshire.core :as json]
[clojure.set :as set]
[clojure.string :as str]
[medley.core :as m]
......@@ -118,18 +119,21 @@
: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.
(defn parse-query-params
"Parses parameter values from the query string in a backward compatible way.
;; 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.
Before (v50 and below) we passed parameter values as separate query string parameters \"?param1=A&param2=B\". The
problem with this approach is that we cannot reliably distinguish between numbers and numeric strings, as well as
booleans and boolean strings. To fix this issue we introduced another query string parameter `:parameters` which
contains serialized JSON with parameter values. If this object cannot be found or parsed, we fallback to plain query
string parameters."
[query-params]
(or (try
(when-let [parameters (:parameters query-params)]
(json/parse-string parameters keyword))
(catch Throwable _
nil))
query-params))
(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
......@@ -138,22 +142,9 @@
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]
(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)))))))
(-> query-params
(update-keys keyword)
(update-vals (fn [v] (if (= v "") nil 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
......
src/metabase/api/preview_embed.clj
+ 9
− 5
View file @ e1232397
......@@ -46,7 +46,7 @@
:token-params (embed/get-in-unsigned-token-or-throw unsigned-token [:params])
:embedding-params (embed/get-in-unsigned-token-or-throw unsigned-token [:_embedding_params])
:constraints {:max-results max-results}
:query-params query-params)))
:query-params (api.embed.common/parse-query-params query-params))))
(api/defendpoint GET "/dashboard/:token"
"Fetch a Dashboard you're considering embedding by passing a JWT `token`. "
......@@ -59,7 +59,11 @@
(api/defendpoint GET "/dashboard/:token/params/:param-key/values"
"Embedded version of chain filter values endpoint."
[token param-key :as {:keys [query-params]}]
(api.embed.common/dashboard-param-values token param-key nil query-params {:preview true}))
(api.embed.common/dashboard-param-values token
param-key
nil
(api.embed.common/parse-query-params query-params)
{:preview true}))
(api/defendpoint GET "/dashboard/:token/dashcard/:dashcard-id/card/:card-id"
"Fetch the results of running a Card belonging to a Dashboard you're considering embedding with JWT `token`."
......@@ -78,7 +82,7 @@
:card-id card-id
:embedding-params embedding-params
:token-params token-params
:query-params query-params)))
:query-params (api.embed.common/parse-query-params query-params))))
(api/defendpoint GET "/pivot/card/:token/query"
"Fetch the query results for a Card you're considering embedding by passing a JWT `token`."
......@@ -91,7 +95,7 @@
:card-id card-id
:token-params (embed/get-in-unsigned-token-or-throw unsigned-token [:params])
:embedding-params (embed/get-in-unsigned-token-or-throw unsigned-token [:_embedding_params])
:query-params query-params
:query-params (api.embed.common/parse-query-params query-params)
:qp qp.pivot/run-pivot-query)))
(api/defendpoint GET "/pivot/dashboard/:token/dashcard/:dashcard-id/card/:card-id"
......@@ -111,7 +115,7 @@
:card-id card-id
:embedding-params embedding-params
:token-params token-params
:query-params query-params
:query-params (api.embed.common/parse-query-params query-params)
:qp qp.pivot/run-pivot-query)))
(api/define-routes)
test/metabase/api/preview_embed_test.clj
+ 51
− 18
View file @ e1232397
(ns metabase.api.preview-embed-test
(:require
[buddy.sign.jwt :as jwt]
[cheshire.core :as json]
[clojure.test :refer :all]
[crypto.random :as crypto-random]
[metabase.api.dashboard-test :as api.dashboard-test]
......@@ -578,8 +579,8 @@
: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)"
(deftest boolean-parameter-values-test
(testing "embedding endpoint supports boolean parameter values (#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
......@@ -587,20 +588,20 @@
:type :native
:native
{:template-tags
{"ASDF"
{"LIKED"
{:widget-type :string/=
:default [true]
:name "ASDF"
:name "LIKED"
:type :dimension
:id "ASDF"
:id "LIKED"
:dimension [:field (mt/id :places :liked) nil]
:display-name "Asdf"
:display-name "Liked"
:options nil
:required true}}
:query "SELECT * FROM PLACES WHERE {{ASDF}}"}}}
:query "SELECT * FROM PLACES WHERE {{LIKED}}"}}}
:model/Dashboard {dashboard-id :id} {:parameters
[{:name "ASDF"
:slug "ASDF"
[{:name "LIKED"
:slug "LIKED"
:id "ccb91bc"
:type :string/=
:sectionId "string"
......@@ -610,29 +611,61 @@
:parameter_mappings
[{:parameter_id "ccb91bc"
:card_id card-id
:target [:dimension [:template-tag "ASDF"]]}]
:target [:dimension [:template-tag "LIKED"]]}]
: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"}}))]
(let [url (card-query-url card {:_embedding_params {:LIKED "enabled"}})]
(is (= [[3 "The Dentist" false]]
(-> (mt/user-http-request :crowberto :get 202 false-url)
(-> (mt/user-http-request :crowberto :get 202 url
:parameters (json/generate-string {:LIKED false}))
:data
:rows)))
(is (= [[1 "Tempest" true]
[2 "Bullit" true]]
(-> (mt/user-http-request :crowberto :get 202 true-url)
(-> (mt/user-http-request :crowberto :get 202 url
:parameters (json/generate-string {:LIKED true}))
: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"}}))]
(let [url (dashcard-url dashcard {:_embedding_params {:LIKED "enabled"}})]
(is (= [[3 "The Dentist" false]]
(-> (mt/user-http-request :crowberto :get 202 false-url)
(-> (mt/user-http-request :crowberto :get 202 url
:parameters (json/generate-string {:LIKED false}))
:data
:rows)))
(is (= [[1 "Tempest" true]
[2 "Bullit" true]]
(-> (mt/user-http-request :crowberto :get 202 true-url)
(-> (mt/user-http-request :crowberto :get 202 url
:parameters (json/generate-string {:LIKED true}))
:data
:rows))))))))))
(deftest string-parameter-values-test
(testing "embedding endpoint should not parse string values into numbers (#46240)"
(embed-test/with-embedding-enabled-and-new-secret-key
(mt/dataset airports
(mt/with-temp [:model/Card {card-id :id} {:dataset_query
{:database (mt/id)
:type :query
:query
{:source-table (mt/id :airport)
:fields [[:field (mt/id :airport :name) nil]]}}}
:model/Dashboard {dashboard-id :id} {:parameters
[{:name "NAME"
:slug "NAME"
:id "ccb91bc"
:type :string/contains
:sectionId "number"}]}
:model/DashboardCard dashcard {:dashboard_id dashboard-id
:parameter_mappings
[{:parameter_id "ccb91bc"
:card_id card-id
:target [:dimension [:field (mt/id :airport :name) nil]]}]
:card_id card-id}]
(testing "for dashboard embeds"
(let [url (dashcard-url dashcard {:_embedding_params {:NAME "enabled"}})]
(is (= [["Cheongju International Airport/Cheongju Air Base (K-59/G-513)"]]
(-> (mt/user-http-request :crowberto :get 202 url
:parameters (json/generate-string {:NAME "513"}))
:data
:rows))))))))))
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment