Skip to content
Snippets Groups Projects
Unverified Commit fd998918 authored by Case Nelson's avatar Case Nelson Committed by GitHub
Browse files

[Apps] Allow extra parameters to be passed to action execution (#25198) 

* [Apps] Allow extra parameters to be passed to action execution

For actions, unlike queries, the user will be asked to fill in unmapped
parameters. This change allows the dashboard action execution to accept
incoming parameters without an "id" but with a "target". If that target
exists on the action, it will be accepted.

* Update doc string and validation message

* Review changes

Remove action-id from the execute endpoint, this will be looked up
through the dashcard.

Revert map-parameters and split parameters into an extra_parameters
value. extra_parameters are combined with the mapped parameters and
passed to execution.

* Fixing tests
parent 3d5417a4
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/actions/execution.clj
+ 22
− 7
View file @ fd998918
...@@ -5,6 +5,7 @@ ...@@ -5,6 +5,7 @@
[metabase.actions :as actions] [metabase.actions :as actions]
[metabase.actions.http-action :as http-action] [metabase.actions.http-action :as http-action]
[metabase.api.common :as api] [metabase.api.common :as api]
[metabase.mbql.normalize :as mbql.normalize]
[metabase.models :refer [Dashboard DashboardCard]] [metabase.models :refer [Dashboard DashboardCard]]
[metabase.models.action :as action] [metabase.models.action :as action]
[metabase.query-processor.error-type :as qp.error-type] [metabase.query-processor.error-type :as qp.error-type]
...@@ -50,6 +51,7 @@ ...@@ -50,6 +51,7 @@
(assoc parameter :target target))) (assoc parameter :target target)))
parameters))) parameters)))
(defn- execute-query-action! (defn- execute-query-action!
"Execute a `QueryAction` with parameters as passed in from an "Execute a `QueryAction` with parameters as passed in from an
endpoint (see [[map-parameters]] for a description of their shape). endpoint (see [[map-parameters]] for a description of their shape).
...@@ -83,22 +85,35 @@ ...@@ -83,22 +85,35 @@
(http-action/execute-http-action! action params->value))) (http-action/execute-http-action! action params->value)))
(defn execute-dashcard! (defn execute-dashcard!
"Execute the given action in the dashboard/dashcard context with the given incoming-parameters. "Execute the given action in the dashboard/dashcard context with the given unmapped-parameters and extra-parameters.
See [[map-parameters]] for a description of their expected shapes." See [[map-parameters]] for a description of their expected shapes."
[action-id dashboard-id dashcard-id incoming-parameters] [dashboard-id dashcard-id unmapped-parameters extra-parameters]
(actions/check-actions-enabled) (actions/check-actions-enabled)
(api/check-superuser) (api/check-superuser)
(api/read-check Dashboard dashboard-id) (api/read-check Dashboard dashboard-id)
(let [dashcard (api/check-404 (db/select-one DashboardCard (let [dashcard (api/check-404 (db/select-one DashboardCard
:id dashcard-id :id dashcard-id
:dashboard_id dashboard-id :dashboard_id dashboard-id))
:action_id action-id)) action (api/check-404 (first (action/select-actions :id (:action_id dashcard))))
action (api/check-404 (first (action/select-actions :id action-id)))
action-type (:type action) action-type (:type action)
_ (log/tracef "Mapping parameters\n\n%s\nwith mappings\n\n%s" _ (log/tracef "Mapping parameters\n\n%s\nwith mappings\n\n%s"
(u/pprint-to-str incoming-parameters) (u/pprint-to-str unmapped-parameters)
(u/pprint-to-str (:parameter_mappings dashcard))) (u/pprint-to-str (:parameter_mappings dashcard)))
parameters (map-parameters incoming-parameters (:parameter_mappings dashcard))] mapped-parameters (map-parameters
(mbql.normalize/normalize-fragment [:parameters] unmapped-parameters)
(:parameter_mappings dashcard))
parameters (into (mbql.normalize/normalize-fragment [:parameters] extra-parameters)
mapped-parameters)
destination-parameters-by-target (m/index-by :target (:parameters action))]
(doseq [{:keys [target]} parameters]
(when-not (contains? destination-parameters-by-target target)
(throw (ex-info (tru "No destination parameter found for target {0}. Found: {1}"
(pr-str target)
(pr-str (set (keys destination-parameters-by-target))))
{:status-code 400
:type qp.error-type/invalid-parameter
:parameters parameters
:destination-parameters (:parameters action)}))))
(case action-type (case action-type
:query :query
(execute-query-action! action parameters) (execute-query-action! action parameters)
......
src/metabase/api/dashboard.clj
+ 18
− 7
View file @ fd998918
...@@ -689,8 +689,6 @@ ...@@ -689,8 +689,6 @@
(into {} (for [field-id filtered-field-ids] (into {} (for [field-id filtered-field-ids]
[field-id (sort (chain-filter/filterable-field-ids field-id filtering-field-ids))]))))) [field-id (sort (chain-filter/filterable-field-ids field-id filtering-field-ids))])))))
;;; ---------------------------------- Executing the action associated with a Dashcard -------------------------------
(def ParameterWithID (def ParameterWithID
"Schema for a parameter map with an string `:id`." "Schema for a parameter map with an string `:id`."
(su/with-api-error-message (su/with-api-error-message
...@@ -699,11 +697,24 @@ ...@@ -699,11 +697,24 @@
"value must be a parameter map with an 'id' key")) "value must be a parameter map with an 'id' key"))
(api/defendpoint POST "/:dashboard-id/dashcard/:dashcard-id/action/:action-id/execute" (def ParameterWithTarget
"Execute the associated Action in the context of a `Dashboard` and `DashboardCard` that includes it." "Schema for a parameter map with an mbql `:target`."
[dashboard-id dashcard-id action-id :as {{:keys [parameters], :as _body} :body}] (su/with-api-error-message
{parameters (s/maybe [ParameterWithID])} {:target [s/Any]
(actions.execution/execute-dashcard! action-id dashboard-id dashcard-id parameters)) s/Keyword s/Any}
"value must be a parameter map with a 'target' key"))
;;; ---------------------------------- Executing the action associated with a Dashcard -------------------------------
(api/defendpoint POST "/:dashboard-id/dashcard/:dashcard-id/action/execute"
"Execute the associated Action in the context of a `Dashboard` and `DashboardCard` that includes it.
`parameters` should be the mapped dashboard parameters with values.
`extra_parameters` should be the extra, user entered parameter values."
[dashboard-id dashcard-id :as {{:keys [parameters extra_parameters], :as _body} :body}]
{parameters (s/maybe [ParameterWithID])
extra_parameters (s/maybe [ParameterWithTarget])}
(actions.execution/execute-dashcard! dashboard-id dashcard-id parameters extra_parameters))
;;; ---------------------------------- Running the query associated with a Dashcard ---------------------------------- ;;; ---------------------------------- Running the query associated with a Dashcard ----------------------------------
......
test/metabase/actions/test_util.clj
+ 19
− 5
View file @ fd998918
...@@ -120,14 +120,24 @@ ...@@ -120,14 +120,24 @@
:dataset_query {:database (mt/id) :dataset_query {:database (mt/id)
:type :native :type :native
:native {:query (str "UPDATE categories\n" :native {:query (str "UPDATE categories\n"
"SET name = 'Bird Shop'\n" "SET name = [[{{name}} || ' ' ||]] 'Shop'\n"
"WHERE id = {{id}}") "WHERE id = {{id}}")
:template-tags {"id" {:name "id" :template-tags {"id" {:name "id"
:display-name "ID" :display-name "ID"
:type :number :type :number
:required true}}}} :required true}
"name" {:name "name"
:display-name "Name"
:type :text
:required false}}}}
:name "Query Example" :name "Query Example"
:parameters [{:id "id" :type "number"}] :parameters [{:id "id"
:type "number"
:target [:variable [:template-tag "id"]]}
{:id "name"
:type "text"
:required false
:target [:variable [:template-tag "name"]]}]
:is_write true :is_write true
:visualization_settings {:inline true}} :visualization_settings {:inline true}}
(dissoc options-map :type))]] (dissoc options-map :type))]]
...@@ -144,8 +154,12 @@ ...@@ -144,8 +154,12 @@
:method "POST" :method "POST"
:body "{\"the_parameter\": {{id}}}" :body "{\"the_parameter\": {{id}}}"
:headers "{\"x-test\": \"{{id}}\"}" :headers "{\"x-test\": \"{{id}}\"}"
:parameters [{:id "id" :type "number"} :parameters [{:id "id"
{:id "fail" :type "text"}]} :type "number"
:target [:template-tag "id"]}
{:id "fail"
:type "text"
:target [:template-tag "fail"]}]}
:response_handle ".body"} :response_handle ".body"}
options-map))] options-map))]
(f {:action-id action-id}))))) (f {:action-id action-id})))))
......
test/metabase/api/dashboard_test.clj
+ 25
− 18
View file @ fd998918
...@@ -1880,26 +1880,35 @@ ...@@ -1880,26 +1880,35 @@
:action_id action-id :action_id action-id
:parameter_mappings [{:parameter_id "my_id" :parameter_mappings [{:parameter_id "my_id"
:target [:variable [:template-tag "id"]]}]}]] :target [:variable [:template-tag "id"]]}]}]]
(let [execute-path (format "dashboard/%s/dashcard/%s/action/%s/execute" (let [execute-path (format "dashboard/%s/dashcard/%s/action/execute"
dashboard-id dashboard-id
dashcard-id dashcard-id)]
action-id)] (testing "Dashcard parameter"
(is (partial= {:rows-affected 1} (is (partial= {:rows-affected 1}
(mt/user-http-request :crowberto :post 200 execute-path (mt/user-http-request :crowberto :post 200 execute-path
{:parameters [{:id "my_id" :type "id" :value 1}]}))) {:parameters [{:id "my_id" :type "id" :value 1}]})))
(is (= [1 "Bird Shop"] (is (= [1 "Shop"]
(mt/first-row (mt/first-row
(mt/run-mbql-query categories {:filter [:= $id 1]})))) (mt/run-mbql-query categories {:filter [:= $id 1]})))))
(testing "Extra target parameter"
(is (partial= {:rows-affected 1}
(mt/user-http-request :crowberto :post 200 execute-path
{:parameters [{:id "my_id" :type "id" :value 1}]
:extra_parameters [{:target [:variable [:template-tag "name"]]
:type "text"
:value "Bird"}]})))
(is (= [1 "Bird Shop"]
(mt/first-row
(mt/run-mbql-query categories {:filter [:= $id 1]})))))
(testing "Should affect 0 rows if id is out of range" (testing "Should affect 0 rows if id is out of range"
(is (= {:rows-affected 0} (is (= {:rows-affected 0}
(mt/user-http-request :crowberto :post 200 execute-path (mt/user-http-request :crowberto :post 200 execute-path
{:parameters [{:id "my_id" :type :number/= :value Integer/MAX_VALUE}]})))) {:parameters [{:id "my_id" :type :number/= :value Integer/MAX_VALUE}]}))))
(testing "Should 404 if bad dashcard-id" (testing "Should 404 if bad dashcard-id"
(is (= "Not found." (is (= "Not found."
(mt/user-http-request :crowberto :post 404 (format "dashboard/%d/dashcard/%s/action/%s/execute" (mt/user-http-request :crowberto :post 404 (format "dashboard/%d/dashcard/%s/action/execute"
dashboard-id dashboard-id
Integer/MAX_VALUE Integer/MAX_VALUE)
action-id)
{})))) {}))))
(testing "Missing parameter should fail gracefully" (testing "Missing parameter should fail gracefully"
(is (partial= {:message "Error executing Action: Error building query parameter map: Error determining value for parameter \"id\": You'll need to pick a value for 'ID' before this query can run."} (is (partial= {:message "Error executing Action: Error building query parameter map: Error determining value for parameter \"id\": You'll need to pick a value for 'ID' before this query can run."}
...@@ -1923,10 +1932,9 @@ ...@@ -1923,10 +1932,9 @@
:target [:template-tag "id"]} :target [:template-tag "id"]}
{:parameter_id "my_fail" {:parameter_id "my_fail"
:target [:template-tag "fail"]}]}]] :target [:template-tag "fail"]}]}]]
(let [execute-path (format "dashboard/%s/dashcard/%s/action/%s/execute" (let [execute-path (format "dashboard/%s/dashcard/%s/action/execute"
dashboard-id dashboard-id
dashcard-id dashcard-id)]
action-id)]
(testing "Should be able to execute an emitter" (testing "Should be able to execute an emitter"
(is (= {:the_parameter 1} (is (= {:the_parameter 1}
(mt/user-http-request :crowberto :post 200 execute-path (mt/user-http-request :crowberto :post 200 execute-path
...@@ -1958,10 +1966,9 @@ ...@@ -1958,10 +1966,9 @@
:action_id action-id :action_id action-id
:parameter_mappings [{:parameter_id "my_id" :parameter_mappings [{:parameter_id "my_id"
:target [:variable [:template-tag "id"]]}]}]] :target [:variable [:template-tag "id"]]}]}]]
(let [execute-path (format "dashboard/%s/dashcard/%s/action/%s/execute" (let [execute-path (format "dashboard/%s/dashcard/%s/action/execute"
dashboard-id dashboard-id
dashcard-id dashcard-id)]
action-id)]
(testing "Without actions enabled" (testing "Without actions enabled"
(is (= "Actions are not enabled." (is (= "Actions are not enabled."
(mt/user-http-request :crowberto :post 400 execute-path (mt/user-http-request :crowberto :post 400 execute-path
......
test/metabase/api/emitter_test.clj
+ 1
− 1
View file @ fd998918
...@@ -72,7 +72,7 @@ ...@@ -72,7 +72,7 @@
(mt/user-http-request :crowberto :post 200 emitter-path (mt/user-http-request :crowberto :post 200 emitter-path
{:parameters {"my_id" {:type :number/= {:parameters {"my_id" {:type :number/=
:value 1}}})))) :value 1}}}))))
(is (= [1 "Bird Shop"] (is (= [1 "Shop"]
(mt/first-row (mt/first-row
(mt/run-mbql-query categories {:filter [:= $id 1]})))) (mt/run-mbql-query categories {:filter [:= $id 1]}))))
(testing "Should affect 0 rows if id is out of range" (testing "Should affect 0 rows if id is out of range"
......
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