From 0dff0e205fed65db1157e22551f5c382378b3e51 Mon Sep 17 00:00:00 2001
From: Ngoc Khuat <qn.khuat@gmail.com>
Date: Thu, 20 Jul 2023 17:04:40 +0700
Subject: [PATCH] Fetch-values for implicit actions save QueryExecution in
action context (#32465)
---
resources/migrations/000_migrations.yaml | 25 +++++++++++++++++++
shared/src/metabase/mbql/schema.cljc | 6 +++--
src/metabase/actions/execution.clj | 20 ++++++---------
src/metabase/api/dashboard.clj | 14 ++++++-----
src/metabase/api/public.clj | 14 ++++++-----
src/metabase/models/action.clj | 3 +--
src/metabase/models/dashboard_card.clj | 7 ++++++
.../middleware/process_userland_query.clj | 11 ++++----
test/metabase/actions/execution_test.clj | 20 +++++++++++++++
test/metabase/api/dataset_test.clj | 1 +
test/metabase/api/public_test.clj | 10 ++++----
.../process_userland_query_test.clj | 2 ++
12 files changed, 95 insertions(+), 38 deletions(-)
create mode 100644 test/metabase/actions/execution_test.clj
diff --git a/resources/migrations/000_migrations.yaml b/resources/migrations/000_migrations.yaml
index ec6a440b741..6af0e535811 100644
--- a/resources/migrations/000_migrations.yaml
+++ b/resources/migrations/000_migrations.yaml
@@ -15948,6 +15948,31 @@ databaseChangeLog:
tableName: computation_job
rollback: # no rollback needed since this table has been unused since 2018
+ - changeSet:
+ id: v48.00-005
+ author: qnkhuat
+ comment: Added 0.48.0 - Add query_execution.action_id
+ changes:
+ - addColumn:
+ tableName: query_execution
+ columns:
+ - column:
+ name: action_id
+ type: integer
+ remarks: 'The ID of the action associated with this query execution, if any.'
+
+ - changeSet:
+ id: v48.00-006
+ author: qnkhuat
+ comment: Added 0.48.0 - Index query_execution.action_id
+ changes:
+ - createIndex:
+ tableName: query_execution
+ indexName: idx_query_execution_action_id
+ columns:
+ column:
+ name: action_id
+
# >>>>>>>>>> DO NOT ADD NEW MIGRATIONS BELOW THIS LINE! ADD THEM ABOVE <<<<<<<<<<
########################################################################################################################
diff --git a/shared/src/metabase/mbql/schema.cljc b/shared/src/metabase/mbql/schema.cljc
index c70614f6455..b924ddb660b 100644
--- a/shared/src/metabase/mbql/schema.cljc
+++ b/shared/src/metabase/mbql/schema.cljc
@@ -1684,7 +1684,8 @@
(def Context
"Schema for `info.context`; used for informational purposes to record how a query was executed."
- (s/enum :ad-hoc
+ (s/enum :action
+ :ad-hoc
:collection
:csv-download
:dashboard
@@ -1708,6 +1709,7 @@
;; used for permissions checking or anything like that so don't try to be sneaky
(s/optional-key :context) (s/maybe Context)
(s/optional-key :executed-by) (s/maybe helpers/IntGreaterThanZero)
+ (s/optional-key :action-id) (s/maybe helpers/IntGreaterThanZero)
(s/optional-key :card-id) (s/maybe helpers/IntGreaterThanZero)
(s/optional-key :card-name) (s/maybe helpers/NonBlankString)
(s/optional-key :dashboard-id) (s/maybe helpers/IntGreaterThanZero)
@@ -1720,7 +1722,7 @@
;; these in yourself. In fact, I would like this a lot better if we could take these keys out of `:info` entirely
;; and have the code that saves QueryExceutions figure out their values when it goes to save them
(s/optional-key :query-hash) (s/maybe #?(:clj (Class/forName "[B")
- :cljs s/Any))})
+ :cljs s/Any))})
;;; --------------------------------------------- Metabase [Outer] Query ---------------------------------------------
diff --git a/src/metabase/actions/execution.clj b/src/metabase/actions/execution.clj
index f9b2eb826fd..219ec637d66 100644
--- a/src/metabase/actions/execution.clj
+++ b/src/metabase/actions/execution.clj
@@ -206,15 +206,15 @@
(execute-action! action request-parameters)))
(defn- fetch-implicit-action-values
- [dashboard-id action request-parameters]
+ [action request-parameters]
(api/check (contains? #{"row/update" "row/delete"} (:kind action))
400
(tru "Values can only be fetched for actions that require a Primary Key."))
(let [implicit-action (keyword (:kind action))
{:keys [prefetch-parameters]} (build-implicit-query action implicit-action request-parameters)
info {:executed-by api/*current-user-id*
- :context :question
- :dashboard-id dashboard-id}
+ :context :action
+ :action-id (:id action)}
card (t2/select-one Card :id (:model_id action))
;; prefilling a form with day old data would be bad
result (binding [persisted-info/*allow-persisted-substitution* false]
@@ -234,12 +234,8 @@
(defn fetch-values
"Fetch values to pre-fill implicit action execution - custom actions will return no values.
- Must pass in parameters of shape `{<parameter-id> <value>}` for primary keys."
- [dashboard-id dashcard-id request-parameters]
- (let [dashcard (api/check-404 (t2/select-one DashboardCard
- :id dashcard-id
- :dashboard_id dashboard-id))
- action (api/check-404 (action/select-action :id (:action_id dashcard)))]
- (if (= :implicit (:type action))
- (fetch-implicit-action-values dashboard-id action request-parameters)
- {})))
+ Must pass in parameters of shape `{<parameter-id> <value>}` for primary keys."
+ [action request-parameters]
+ (if (= :implicit (:type action))
+ (fetch-implicit-action-values action request-parameters)
+ {}))
diff --git a/src/metabase/api/dashboard.clj b/src/metabase/api/dashboard.clj
index a0d67589e7e..7989da0690d 100644
--- a/src/metabase/api/dashboard.clj
+++ b/src/metabase/api/dashboard.clj
@@ -955,15 +955,17 @@
;;; ---------------------------------- Executing the action associated with a Dashcard -------------------------------
-#_{:clj-kondo/ignore [:deprecated-var]}
-(api/defendpoint-schema GET "/:dashboard-id/dashcard/:dashcard-id/execute"
+
+(api/defendpoint GET "/:dashboard-id/dashcard/:dashcard-id/execute"
"Fetches the values for filling in execution parameters. Pass PK parameters and values to select."
[dashboard-id dashcard-id parameters]
- {dashboard-id su/IntGreaterThanZero
- dashcard-id su/IntGreaterThanZero
- parameters su/JSONString}
+ {dashboard-id ms/PositiveInt
+ dashcard-id ms/PositiveInt
+ parameters ms/JSONString}
(api/read-check :model/Dashboard dashboard-id)
- (actions.execution/fetch-values dashboard-id dashcard-id (json/parse-string parameters)))
+ (actions.execution/fetch-values
+ (api/check-404 (dashboard-card/dashcard->action dashcard-id))
+ (json/parse-string parameters)))
#_{:clj-kondo/ignore [:deprecated-var]}
(api/defendpoint-schema POST "/:dashboard-id/dashcard/:dashcard-id/execute"
diff --git a/src/metabase/api/public.clj b/src/metabase/api/public.clj
index 130fcd11954..6ce13d3fba7 100644
--- a/src/metabase/api/public.clj
+++ b/src/metabase/api/public.clj
@@ -20,6 +20,7 @@
[metabase.models.action :as action]
[metabase.models.card :as card :refer [Card]]
[metabase.models.dashboard :refer [Dashboard]]
+ [metabase.models.dashboard-card :as dashboard-card]
[metabase.models.dimension :refer [Dimension]]
[metabase.models.field :refer [Field]]
[metabase.models.interface :as mi]
@@ -298,15 +299,16 @@
:export-format :api
:parameters parameters)))
-#_{:clj-kondo/ignore [:deprecated-var]}
-(api/defendpoint-schema GET "/dashboard/:uuid/dashcard/:dashcard-id/execute"
+(api/defendpoint GET "/dashboard/:uuid/dashcard/:dashcard-id/execute"
"Fetches the values for filling in execution parameters. Pass PK parameters and values to select."
[uuid dashcard-id parameters]
- {dashcard-id su/IntGreaterThanZero
- parameters su/JSONString}
+ {dashcard-id ms/PositiveInt
+ parameters ms/JSONString}
(validation/check-public-sharing-enabled)
- (let [dashboard-id (api/check-404 (t2/select-one-pk Dashboard :public_uuid uuid, :archived false))]
- (actions.execution/fetch-values dashboard-id dashcard-id (json/parse-string parameters))))
+ (api/check-404 (t2/select-one-pk Dashboard :public_uuid uuid :archived false))
+ (actions.execution/fetch-values
+ (api/check-404 (dashboard-card/dashcard->action dashcard-id))
+ (json/parse-string parameters)))
(def ^:private dashcard-execution-throttle (throttle/make-throttler :dashcard-id :attempts-threshold 5000))
diff --git a/src/metabase/models/action.clj b/src/metabase/models/action.clj
index 19db88f16d3..e6be410491e 100644
--- a/src/metabase/models/action.clj
+++ b/src/metabase/models/action.clj
@@ -3,7 +3,6 @@
[cheshire.core :as json]
[medley.core :as m]
[metabase.models.card :refer [Card]]
- [metabase.models.dashboard-card :refer [DashboardCard]]
[metabase.models.interface :as mi]
[metabase.models.query :as query]
[metabase.models.serialization :as serdes]
@@ -94,7 +93,7 @@
[{archived? :archived, id :id, model-id :model_id, :as changes}]
(u/prog1 changes
(if archived?
- (t2/delete! DashboardCard :action_id id)
+ (t2/delete! :model/DashboardCard :action_id id)
(check-model-is-not-a-saved-question model-id))))
(defmethod mi/perms-objects-set :model/Action
diff --git a/src/metabase/models/dashboard_card.clj b/src/metabase/models/dashboard_card.clj
index 6bcee0ebd59..728a3ba2b4e 100644
--- a/src/metabase/models/dashboard_card.clj
+++ b/src/metabase/models/dashboard_card.clj
@@ -5,6 +5,7 @@
[metabase.db :as mdb]
[metabase.db.query :as mdb.query]
[metabase.db.util :as mdb.u]
+ [metabase.models.action :as action]
[metabase.models.card :refer [Card]]
[metabase.models.dashboard-card-series :refer [DashboardCardSeries]]
[metabase.models.interface :as mi]
@@ -104,6 +105,12 @@
;;; ---------------------------------------------------- CRUD FNS ----------------------------------------------------
+(defn dashcard->action
+ "Get the action associated with a dashcard if exists, return `nil` otherwise."
+ [dashcard-or-dashcard-id]
+ (some->> (t2/select-one-fn :action_id :model/DashboardCard :id (u/the-id dashcard-or-dashcard-id))
+ (action/select-action :id)))
+
(s/defn retrieve-dashboard-card
"Fetch a single DashboardCard by its ID value."
[id :- su/IntGreaterThanZero]
diff --git a/src/metabase/query_processor/middleware/process_userland_query.clj b/src/metabase/query_processor/middleware/process_userland_query.clj
index 9bca73db5b4..e0ff5e04d0e 100644
--- a/src/metabase/query_processor/middleware/process_userland_query.clj
+++ b/src/metabase/query_processor/middleware/process_userland_query.clj
@@ -74,7 +74,7 @@
(merge
(-> query-execution
add-running-time
- (dissoc :error :hash :executor_id :card_id :dashboard_id :pulse_id :result_rows :native))
+ (dissoc :error :hash :executor_id :action_id :card_id :dashboard_id :pulse_id :result_rows :native))
result
{:status :completed
:average_execution_time (when cached?
@@ -108,13 +108,14 @@
(defn- query-execution-info
"Return the info for the QueryExecution entry for this `query`."
{:arglists '([query])}
- [{{:keys [executed-by query-hash context card-id dashboard-id pulse-id]} :info
- database-id :database
- query-type :type
- :as query}]
+ [{{:keys [executed-by query-hash context action-id card-id dashboard-id pulse-id]} :info
+ database-id :database
+ query-type :type
+ :as query}]
{:pre [(instance? (Class/forName "[B") query-hash)]}
{:database_id database-id
:executor_id executed-by
+ :action_id action-id
:card_id card-id
:dashboard_id dashboard-id
:pulse_id pulse-id
diff --git a/test/metabase/actions/execution_test.clj b/test/metabase/actions/execution_test.clj
new file mode 100644
index 00000000000..491140969e9
--- /dev/null
+++ b/test/metabase/actions/execution_test.clj
@@ -0,0 +1,20 @@
+(ns metabase.actions.execution-test
+ (:require
+ [clojure.test :refer :all]
+ [metabase.actions.execution :as actions.execution]
+ [metabase.models.action :as action]
+ [metabase.test :as mt]
+ [toucan2.core :as t2]))
+
+(set! *warn-on-reflection* true)
+
+(deftest fetch-values-save-execution-info-test
+ (testing "fetch values for implicit action will save an execution info"
+ (mt/with-actions-enabled
+ (mt/with-actions [_ {:dataset true :dataset_query (mt/mbql-query venues {:fields [$id $name]})}
+ {:keys [action-id]} {:type :implicit :kind "row/update"}]
+ (is (= {"id" 1 "name" "Red Medicine"}
+ (actions.execution/fetch-values (action/select-action :id action-id) {"id" 1})))
+ ;; the query execution is saved async, so we need to sleep a bit
+ (Thread/sleep 200)
+ (is (true? (t2/exists? :model/QueryExecution :action_id action-id :context :action)))))))
diff --git a/test/metabase/api/dataset_test.clj b/test/metabase/api/dataset_test.clj
index 61871d55727..8d35d95b6a6 100644
--- a/test/metabase/api/dataset_test.clj
+++ b/test/metabase/api/dataset_test.clj
@@ -107,6 +107,7 @@
:dashboard_id nil
:error nil
:id true
+ :action_id nil
:cache_hit false
:database_id (mt/id)
:started_at true
diff --git a/test/metabase/api/public_test.clj b/test/metabase/api/public_test.clj
index 8c257b66815..613263bc184 100644
--- a/test/metabase/api/public_test.clj
+++ b/test/metabase/api/public_test.clj
@@ -397,11 +397,11 @@
(t2/update! Card (u/the-id card) {:archived true})
(is (= {:name true, :ordered_cards 0, :ordered_tabs 0}
(fetch-public-dashboard dash)))))
- (testing "dashboard with tabs should return ordered_tabs"
- (api.dashboard-test/with-simple-dashboard-with-tabs [{:keys [dashboard-id]}]
- (t2/update! :model/Dashboard :id dashboard-id (shared-obj))
- (is (= {:name true, :ordered_cards 2, :ordered_tabs 2}
- (fetch-public-dashboard (t2/select-one :model/Dashboard :id dashboard-id)))))))))
+ (testing "dashboard with tabs should return ordered_tabs"
+ (api.dashboard-test/with-simple-dashboard-with-tabs [{:keys [dashboard-id]}]
+ (t2/update! :model/Dashboard :id dashboard-id (shared-obj))
+ (is (= {:name true, :ordered_cards 2, :ordered_tabs 2}
+ (fetch-public-dashboard (t2/select-one :model/Dashboard :id dashboard-id)))))))))
(deftest public-dashboard-with-implicit-action-only-expose-unhidden-fields
(mt/with-temporary-setting-values [enable-public-sharing true]
diff --git a/test/metabase/query_processor/middleware/process_userland_query_test.clj b/test/metabase/query_processor/middleware/process_userland_query_test.clj
index 82eec6e5495..60fefc3a1bf 100644
--- a/test/metabase/query_processor/middleware/process_userland_query_test.clj
+++ b/test/metabase/query_processor/middleware/process_userland_query_test.clj
@@ -69,6 +69,7 @@
:native false
:pulse_id nil
:card_id nil
+ :action_id nil
:context nil
:running_time true
:cache_hit false
@@ -94,6 +95,7 @@
:json_query query
:native false
:pulse_id nil
+ :action_id nil
:card_id nil
:context nil
:running_time true
--
GitLab