From 8fb9f881cbf1ff5fd1e052438e7937441af6ebab Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Fri, 10 Feb 2023 12:29:49 +0100
Subject: [PATCH] Refactor functions for selecting actions (#28187)
* Create select-action
* Rename to select-actions
* Remove comments
* Remove unnecessary ()
* Missed one
* Fix GET /action/:uuid
* Use new fns more
* Fix
* Remove select-action-without-implicit-params, we don't need it
---
.../serialization/v2/e2e/yaml_test.clj | 3 +-
.../serialization/v2/extract_test.clj | 6 ++--
src/metabase/actions/execution.clj | 4 +--
src/metabase/api/action.clj | 11 ++++---
src/metabase/api/public.clj | 5 ++--
src/metabase/models/action.clj | 29 ++++++++++---------
test/metabase/models/action_test.clj | 12 ++++----
7 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/v2/e2e/yaml_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/v2/e2e/yaml_test.clj
index f14a306c2e7..b1aca7ea00a 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/v2/e2e/yaml_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/e2e/yaml_test.clj
@@ -300,7 +300,8 @@
(testing "for Actions"
(doseq [{:keys [entity_id] :as coll} (get @entities "Action")]
(is (= (clean-entity coll)
- (->> (action/select-one :entity_id entity_id)
+ (->> (db/select-one 'Action :entity_id entity_id)
+ (@#'action/hydrate-subtype)
(serdes.base/extract-one "Action" {})
clean-entity)))))
diff --git a/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj b/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj
index eeebac1d150..26a8fcdd2df 100644
--- a/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/serialization/v2/extract_test.clj
@@ -764,7 +764,7 @@
:kind "row/update"
:creator_id ann-id
:model_id card-id-1}]
- (let [action (action/select-one :id action-id)]
+ (let [action (action/select-action :id action-id)]
(testing "implicit action"
(let [ser (serdes.base/extract-one "Action" {} action)]
(is (schema= {:serdes/meta (s/eq [{:model "Action" :id (:entity_id action) :label "my_action"}])
@@ -800,7 +800,7 @@
:template {}
:creator_id ann-id
:model_id card-id-1}]
- (let [action (action/select-one :id action-id)]
+ (let [action (action/select-action :id action-id)]
(testing "action"
(let [ser (serdes.base/extract-one "Action" {} action)]
(is (schema= {:serdes/meta (s/eq [{:model "Action" :id (:entity_id action) :label "my_action"}])
@@ -836,7 +836,7 @@
:database_id db-id
:creator_id ann-id
:model_id card-id-1}]
- (let [action (action/select-one :id action-id)]
+ (let [action (action/select-action :id action-id)]
(testing "action"
(let [ser (serdes.base/extract-one "Action" {} action)]
(is (schema= {:serdes/meta (s/eq [{:model "Action"
diff --git a/src/metabase/actions/execution.clj b/src/metabase/actions/execution.clj
index a9336f180e0..caa83e0b02c 100644
--- a/src/metabase/actions/execution.clj
+++ b/src/metabase/actions/execution.clj
@@ -178,7 +178,7 @@
(let [dashcard (api/check-404 (db/select-one DashboardCard
:id dashcard-id
:dashboard_id dashboard-id))
- action (api/check-404 (first (action/actions-with-implicit-params nil :id (:action_id dashcard))))]
+ action (api/check-404 (action/select-action :id (:action_id dashcard)))]
(execute-action! action request-parameters)))
(defn- fetch-implicit-action-values
@@ -211,7 +211,7 @@
(let [dashcard (api/check-404 (db/select-one DashboardCard
:id dashcard-id
:dashboard_id dashboard-id))
- action (api/check-404 (first (action/actions-with-implicit-params nil :id (:action_id dashcard))))]
+ action (api/check-404 (action/select-action :id (:action_id dashcard)))]
(if (= :implicit (:type action))
(fetch-implicit-action-values dashboard-id action request-parameters)
{})))
diff --git a/src/metabase/api/action.clj b/src/metabase/api/action.clj
index f6e9494a8dc..897383b9a07 100644
--- a/src/metabase/api/action.clj
+++ b/src/metabase/api/action.clj
@@ -55,13 +55,12 @@
;; We don't check the permissions on the actions, we assume they are
;; readable if the model is readable.
(hydrate
- (action/actions-with-implicit-params [model] :model_id model-id)
+ (action/select-actions [model] :model_id model-id)
:creator)))
(api/defendpoint GET "/:action-id"
[action-id]
- (-> (action/actions-with-implicit-params nil :id action-id)
- first
+ (-> (action/select-action :id action-id)
(hydrate :creator)
api/read-check))
@@ -102,10 +101,10 @@
(actions/check-actions-enabled! (db/select-one Database :id database_id)))
(let [action-id (action/insert! (assoc action :creator_id api/*current-user-id*))]
(if action-id
- (first (action/actions-with-implicit-params nil :id action-id))
+ (action/select-action :id action-id)
;; db/insert! does not return a value when used with h2
;; so we return the most recently updated http action.
- (last (action/actions-with-implicit-params nil :type type)))))
+ (last (action/select-actions nil :type type)))))
(api/defendpoint PUT "/:id"
[id :as
@@ -127,7 +126,7 @@
visualization_settings [:maybe map?]}
(let [existing-action (api/write-check Action id)]
(action/update! (assoc action :id id) existing-action))
- (first (action/actions-with-implicit-params nil :id id)))
+ (action/select-action :id id))
(api/defendpoint POST "/:id/public_link"
"Generate publicly-accessible links for this Action. Returns UUID to be used in public links. (If this
diff --git a/src/metabase/api/public.clj b/src/metabase/api/public.clj
index af0309a7a21..bd60aa89c86 100644
--- a/src/metabase/api/public.clj
+++ b/src/metabase/api/public.clj
@@ -322,8 +322,7 @@
[uuid]
{uuid ms/UUIDString}
(validation/check-public-sharing-enabled)
- (-> (action/actions-with-implicit-params nil :public_uuid uuid)
- first
+ (-> (action/select-action :public_uuid uuid)
api/check-404
(select-keys action-public-keys)))
@@ -586,7 +585,7 @@
;; failing because there are no current user perms; if this Dashcard is public
;; you're by definition allowed to run it without a perms check anyway
(binding [api/*current-user-permissions-set* (delay #{"/"})]
- (let [action (api/check-404 (first (action/actions-with-implicit-params nil :public_uuid uuid)))]
+ (let [action (api/check-404 (action/select-action :public_uuid uuid))]
;; Undo middleware string->keyword coercion
(actions.execution/execute-action! action (update-keys parameters name))))))))
diff --git a/src/metabase/models/action.clj b/src/metabase/models/action.clj
index 67446092760..57b5bdba032 100644
--- a/src/metabase/models/action.clj
+++ b/src/metabase/models/action.clj
@@ -113,12 +113,6 @@
(merge (db/select-one subtype :action_id (:id action)))
(dissoc :action_id))))
-(defn select-one
- "Selects an action and fills in the subtype data.
- `options` is passed to `db/select-one` `& options` arg."
- [& options]
- (hydrate-subtype (apply db/select-one Action options)))
-
(defn- normalize-query-actions [actions]
(when (seq actions)
(let [query-actions (db/select QueryAction :action_id [:in (map :id actions)])
@@ -149,9 +143,10 @@
(select-keys implicit-action [:kind]))))
actions))))
-(defn select-actions
- "Select Actions and fill in sub type information.
- `options` is passed to `db/select` `& options` arg"
+(defn- select-actions-without-implicit-params
+ "Select Actions and fill in sub type information. Don't use this if you need implicit parameters
+ for implicit actions, use [[select-action]] instead.
+ `options` is passed to `db/select` `& options` arg."
[& options]
(let [{:keys [query http implicit]} (group-by :type (apply db/select Action options))
query-actions (normalize-query-actions query)
@@ -164,7 +159,7 @@
[fields]
(empty? (m/filter-vals #(not= % 1) (frequencies (map (comp u/slugify :name) fields)))))
-(defn implicit-action-parameters
+(defn- implicit-action-parameters
"Return a set of parameters for the given models"
[cards]
(let [card-by-table-id (into {}
@@ -192,12 +187,12 @@
::pk? (isa? (:semantic_type field) :type/PK)})))]]
[(:id card) parameters]))))
-(defn actions-with-implicit-params
+(defn select-actions
"Find actions with given options and generate implicit parameters for execution.
Pass in known-models to save a second Card lookup."
[known-models & options]
- (let [actions (apply select-actions options)
+ (let [actions (apply select-actions-without-implicit-params options)
implicit-action-model-ids (set (map :model_id (filter (comp #(= :implicit %) :type) actions)))
models-with-implicit-actions (if known-models
(->> known-models
@@ -229,12 +224,18 @@
:always seq))))))
+(defn select-action
+ "Selects an Action and fills in the subtype data and implicit parameters.
+ `options` is passed to `db/select-one` `& options` arg."
+ [& options]
+ (first (apply select-actions nil options)))
+
(mi/define-batched-hydration-method dashcard-action
:dashcard/action
"Hydrates action from DashboardCard."
[dashcards]
(let [actions-by-id (when-let [action-ids (seq (keep :action_id dashcards))]
- (m/index-by :id (actions-with-implicit-params (map :card dashcards) :id [:in action-ids])))]
+ (m/index-by :id (select-actions (map :card dashcards) :id [:in action-ids])))]
(for [dashcard dashcards]
(m/assoc-some dashcard :action (get actions-by-id (:action_id dashcard))))))
@@ -265,7 +266,7 @@
(defmethod serdes.base/load-update! "Action" [_model-name ingested local]
(log/tracef "Upserting Action %d: old %s new %s" (:id local) (pr-str local) (pr-str ingested))
(update! (assoc ingested :id (:id local)) local)
- (select-one :id (:id local)))
+ (select-action :id (:id local)))
(defmethod serdes.base/load-insert! "Action" [_model-name ingested]
(log/tracef "Inserting Action: %s" (pr-str ingested))
diff --git a/test/metabase/models/action_test.clj b/test/metabase/models/action_test.clj
index b77105c609b..bde7e63db55 100644
--- a/test/metabase/models/action_test.clj
+++ b/test/metabase/models/action_test.clj
@@ -13,12 +13,12 @@
:name "Query Example"
:model_id model-id
:parameters [{:id "id" :type :number}]}
- (first (action/select-actions :id action-id))))
+ (action/select-action :id action-id)))
(is (partial= {:id action-id
:name "Query Example"
:model_id model-id
:parameters [{:id "id" :type :number}]}
- (action/select-one :id action-id)))))))
+ (action/select-action :id action-id)))))))
(deftest hydrate-http-action-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
@@ -28,12 +28,12 @@
:name "Echo Example"
:parameters [{:id "id" :type :number}
{:id "fail" :type :text}]}
- (first (action/select-actions :id action-id))))
+ (action/select-action :id action-id)))
(is (partial= {:id action-id
:name "Echo Example"
:parameters [{:id "id" :type :number}
{:id "fail" :type :text}]}
- (action/select-one :id action-id)))))))
+ (action/select-action :id action-id)))))))
(deftest hydrate-creator-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
@@ -45,11 +45,11 @@
:creator_id (mt/user->id :crowberto)
:creator {:common_name "Crowberto Corv"}
:parameters [{:id "id" :type :number}]}
- (hydrate (first (action/select-actions :id action-id)) :creator)))
+ (hydrate (action/select-action :id action-id) :creator)))
(is (partial= {:id action-id
:name "Query Example"
:model_id model-id
:creator_id (mt/user->id :crowberto)
:creator {:common_name "Crowberto Corv"}
:parameters [{:id "id" :type :number}]}
- (hydrate (action/select-one :id action-id) :creator)))))))
+ (hydrate (action/select-action :id action-id) :creator)))))))
--
GitLab