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

[Actions] Add emitter and action tests (#24199)

* wip

* Update kondo hook for with-temp*

`with-temp*` allows for unpaired bindings if you just want the default
model. This would break the `let` that the hook was binding if you had
an unpaired temp model followed by others.

* Adding tests for emitters and actions

Test hydration of :emitters

Test hydration of :action

Test http emitter execution

Test emitter crud

Test failure cases around emitter execution

Small consistency changes made to non-test code that shouldn't affect happy path FE code, largely 404 checking and keyword/string consistency.

* Fix unused requires

* Fix macro resolution
parent 01e1d722
No related branches found
No related tags found
No related merge requests found
......@@ -350,6 +350,10 @@
:lint-as
{metabase-enterprise.serialization.test-util/with-random-dump-dir clojure.core/let
metabase.actions.test-util/with-query-action clojure.core/fn
metabase.actions.test-util/with-http-action clojure.core/fn
metabase.actions.test-util/with-card-emitter clojure.core/let
metabase.actions.test-util/with-dashboard-emitter clojure.core/let
metabase.api.common/let-404 clojure.core/let
metabase.db.data-migrations/defmigration clojure.core/def
metabase.db.liquibase/with-liquibase clojure.core/let
......
......@@ -5,7 +5,9 @@
(let [pairs (partition 2 bindings)
db-refs (map first pairs)
let-stream (for [[_ binding+opts] pairs
part (:children binding+opts)]
;; if children is size 1 then ensure nil is on the right side of the let
:let [[binding-part opts] (:children binding+opts)]
part [binding-part opts]]
part)]
(api/vector-node [(api/vector-node db-refs)
(api/list-node (list* (api/token-node `let)
......
......@@ -2,7 +2,7 @@
:show-docs-arity-on-same-line? true
:project-specs [{:project-path "deps.edn"
:classpath-cmd ["clojure" "-A:dev:ee:ee-dev:drivers:drivers-dev" "-Spath"]}]
:clean {:ns-inner-blocks-indentation :keep}
:linters {:clojure-lsp/unused-public-var {:level :warning
:exclude-when-defined-by
#{metabase.api.common/defendpoint}}}}
......@@ -113,32 +113,40 @@
(defn execute-http-action!
"Calls an http endpoint based on action and params"
[action params->value]
(let [{:keys [method url body headers]} (:template action)
request {:method (keyword method)
:url (parse-and-substitute url params->value)
:accept :json
:content-type :json
:throw-exceptions false
:headers (merge
;; TODO maybe we want to default Agent here? Maybe Origin/Referer?
{"X-Metabase-Action" (:name action)}
(-> headers
(parse-and-substitute params->value)
(json/decode)))
:body (parse-and-substitute body params->value)}
response (update (select-keys (http/request request) [:body :headers :status]) :body json/decode)
;; TODO this is pretty ineficient. We parse with `:as :json`, then reencode within a response
;; I couldn't find a way to get JSONNode out of cheshire, so we fall back to jackson.
;; Should jackson be added explicitly to deps.edn?
response-node (.readTree ^ObjectMapper @object-mapper (json/generate-string response))
error (json/parse-string (apply-json-query response-node (or (:error_handle action) ".status >= 400")))]
(if error
{:status 400
:headers {"Content-Type" "application/json"}
:body (when-not (boolean? error) error)}
(if-some [response (some->> action :response_handle (apply-json-query response-node))]
{:status 200
(try
(let [{:keys [method url body headers]} (:template action)
request {:method (keyword method)
:url (parse-and-substitute url params->value)
:accept :json
:content-type :json
:throw-exceptions false
:headers (merge
;; TODO maybe we want to default Agent here? Maybe Origin/Referer?
{"X-Metabase-Action" (:name action)}
(-> headers
(parse-and-substitute params->value)
(json/decode)))
:body (parse-and-substitute body params->value)}
response (update (select-keys (http/request request) [:body :headers :status]) :body json/decode)
;; TODO this is pretty ineficient. We parse with `:as :json`, then reencode within a response
;; I couldn't find a way to get JSONNode out of cheshire, so we fall back to jackson.
;; Should jackson be added explicitly to deps.edn?
response-node (.readTree ^ObjectMapper @object-mapper (json/generate-string response))
error (json/parse-string (apply-json-query response-node (or (:error_handle action) ".status >= 400")))]
(log/trace "Response before handle:" response)
(if error
{:status 400
:headers {"Content-Type" "application/json"}
:body response}
{:status 204
:body nil}))))
:body (if (boolean? error)
{:remote-status (:status response)}
error)}
(if-some [response (some->> action :response_handle (apply-json-query response-node))]
{:status 200
:headers {"Content-Type" "application/json"}
:body response}
{:status 204
:body nil})))
(catch Exception e
(throw (ex-info (str "Problem building request: " (ex-message e))
{:template (:template action)}
e)))))
......@@ -59,15 +59,13 @@
(db/delete! HTTPAction :action_id action-id)
api/generic-204-no-content)
;; TODO -- 99% sure these are busted. See https://github.com/metabase/metabase/issues/23935
(api/defendpoint POST "/"
"Create a new HTTP action."
[:as {action :body}]
(when (not= "http" (:type action))
(throw (ex-info (trs "Action type is not supported") {:status-code 400 :action action})))
(let [http-action (db/insert! HTTPAction action)]
(if-let [action-id (:action_id http-action)]
(let [action-id (action/insert! action)]
(if action-id
(first (action/select-actions :id action-id))
;; db/insert! does not return a value when used with h2
;; so we return the most recently updated http action.
......
......@@ -60,12 +60,14 @@
(api/defendpoint PUT "/:emitter-id"
"Endpoint to update an emitter."
[emitter-id :as {emitter :body}]
(api/check-404 (Emitter emitter-id))
(db/update! Emitter emitter-id emitter)
api/generic-204-no-content)
(api/defendpoint DELETE "/:emitter-id"
"Endpoint to delete an emitter."
[emitter-id]
(api/check-404 (Emitter emitter-id))
(db/delete! Emitter :id emitter-id)
api/generic-204-no-content)
......@@ -75,52 +77,6 @@
s/Keyword s/Any}}
(su/with-api-error-message "map of parameter name or ID -> map of parameter `:value` and `:type` of the value")))
(comment
;; Assuming things look like this
;; GET native query action
:parameters
;;[
;; {
;; "id": "2fddf797-838e-81eb-4828-53f947932486",
;; "type": "number/=",
;; "target": [
;; "variable",
;; [
;; "template-tag",
;; "product_id"
;; ]
;; ],
;; "name": "Product",
;; "slug": "product_id"
;; }
;;]
;; PUT dashboards/:id
:emitters
:parameter_mappings
;;{
;; "2fddf797-838e-81eb-4828-53f947932486": [
;; "variable",
;; [
;; "template-tag",
;; "product_id"
;; ]
;; ]
;;}
;; on execution
:parameters
;;{
;; "2fddf797-838e-81eb-4828-53f947932486": {
;; "value": 1,
;; "type": "number/="
;; }
;;}
)
(defn- execute-http-emitter!
[emitter parameters]
;; TODO check the types match
......
......@@ -42,4 +42,12 @@
(restore-snapshot! name)
nil)
(api/defendpoint POST "/echo"
[fail :as {:keys [body]}]
(if fail
{:status 400
:body {:error-code "oops"}}
{:status 200
:body body}))
(api/define-routes)
(ns metabase.models.action
(:require [medley.core :as m]
(:require [cheshire.core :as json]
[medley.core :as m]
[metabase.models.interface :as mi]
[metabase.util :as u]
[metabase.util.encryption :as encryption]
......@@ -10,19 +11,20 @@
(models/defmodel HTTPAction :http_action)
(models/defmodel Action :action)
(models/add-type! ::json-with-nested-parameters
:in (comp mi/json-in
(fn [template]
(u/update-if-exists template :parameters mi/normalize-parameters-list)))
:out (comp (fn [template]
(u/update-if-exists template :parameters (mi/catch-normalization-exceptions mi/normalize-parameters-list)))
mi/json-out-with-keywordization))
(u/strict-extend (class Action)
models/IModel
(merge models/IModelDefaults
{:types (constantly {:type :keyword})
:properties (constantly {:timestamped? true})}))
(defn- pre-insert
[action]
(let [base-action (db/insert! Action (select-keys action [:type]))]
(-> action
(dissoc :type)
(assoc :action_id (u/the-id base-action)))))
(defn- pre-update
[action]
;; All possible sub-type columns
......@@ -39,8 +41,7 @@
(merge models/IModelDefaults
{:primary-key (constantly :action_id) ; This is ok as long as we're 1:1
:pre-delete pre-delete
:pre-update pre-update
:pre-insert pre-insert}))
:pre-update pre-update}))
(u/strict-extend (class QueryAction)
models/IModel
......@@ -49,7 +50,22 @@
(u/strict-extend (class HTTPAction)
models/IModel
(merge Action-subtype-IModel-impl
{:types (constantly {:template :json})}))
{:types (constantly {:template ::json-with-nested-parameters})}))
(defn insert!
"Inserts an Action and related HTTPAction or QueryAction. Returns the action id."
[action-data]
(db/transaction
(let [action (db/insert! Action {:type (:type action-data)})
model (case (keyword (:type action))
:http HTTPAction
:query QueryAction)]
(db/execute! {:insert-into model
:values [(-> action-data
(dissoc :type)
(u/update-if-exists :template json/encode)
(assoc :action_id (:id action)))]})
(:id action))))
(def ^:private encrypted-json-out (comp mi/json-out-with-keywordization encryption/maybe-decrypt))
......
......@@ -217,8 +217,7 @@
(defn- create-actions-when-is-writable! [{is-write? :is_write card-id :id}]
(when is-write?
(when-not (db/select-one action/QueryAction :card_id card-id)
(db/insert! action/QueryAction {:card_id card-id
:type :query}))))
(action/insert! {:card_id card-id :type :query}))))
(defn- delete-actions-when-not-writable! [{is-write? :is_write card-id :id}]
(when (not is-write?)
......
......@@ -3,11 +3,14 @@
[clojure.java.jdbc :as jdbc]
[clojure.test :refer :all]
[metabase.driver.sql-jdbc.connection :as sql-jdbc.conn]
[metabase.models
:refer [Card CardEmitter Database Emitter EmitterAction QueryAction]]
[metabase.http-client :as client]
[metabase.models :refer [Action Card CardEmitter Dashboard DashboardEmitter
Database Emitter EmitterAction QueryAction]]
[metabase.models.action :as action]
[metabase.test :as mt]
[metabase.test.data.dataset-definitions :as defs]
[metabase.test.data.interface :as tx]
[metabase.test.initialize :as initialize]
[toucan.db :as db]))
(def ^:dynamic ^:private *actions-test-data-tables*
......@@ -120,6 +123,8 @@
:display-name "ID"
:type :number
:required true}}}}
:name "Query Example"
:parameters [{:id "id" :type "number"}]
:is_write true}]]
(let [action-id (db/select-one-field :action_id QueryAction :card_id card-id)]
(f {:query-action-card-id card-id
......@@ -135,37 +140,84 @@
[[bindings] & body]
`(do-with-query-action (fn [~bindings] ~@body)))
(defn do-with-card-emitter
"Impl for [[with-card-emitter]]."
[{:keys [action-id], :as context} f]
(mt/with-temp* [Card [{emitter-card-id :id}]
Emitter [{emitter-id :id} {:parameter_mappings {"my_id" [:variable [:template-tag "id"]]}}]]
(testing "Sanity check: emitter-id should be non-nil"
(is (integer? emitter-id)))
(testing "Sanity check: make sure parameter mappings were defined the way we'd expect"
(is (= {:my_id [:variable [:template-tag "id"]]}
(db/select-one-field :parameter_mappings Emitter :id emitter-id))))
;; these are tied to the Card and Emitter above and will get cascade deleted. We can't use `with-temp*` for them
;; because it doesn't seem to work with tables with compound PKs
(db/insert! EmitterAction {:emitter_id emitter-id
:action_id action-id})
(db/insert! CardEmitter {:card_id emitter-card-id
:action_id action-id})
(f (assoc context
:emitter-id emitter-id
:emitter-card-id emitter-card-id))))
(defn do-with-http-action
"Impl for [[with-http-action]]."
[f]
(initialize/initialize-if-needed! :web-server)
(mt/with-model-cleanup [Action]
(let [action-id (action/insert! {:type :http
:name "Echo Example"
:template {:url (client/build-url "testing/echo[[?fail={{fail}}]]" {})
:method :post
:body "{\"the_parameter\": {{id}}}"
:headers "{\"x-test\": \"{{id}}\"}"
:parameters [{:id "id" :type "number"}
{:id "fail" :type "text"}]}
:response_handle ".body"})]
(f {:action-id action-id}))))
(defmacro with-http-action
"Execute `body` with a newly created QueryAction. `bindings` is a map with key `:action-id`.
(with-http-action [{:keys [action-id], :as context}]
(do-something))"
{:style/indent 1}
[[bindings] & body]
`(do-with-http-action (fn [~bindings] ~@body)))
(defn do-with-emitter
"Impl for [[with-emitter]]."
[card-or-dashboard-model {:keys [action-id], :as context} f]
(let [parent-model (db/resolve-model card-or-dashboard-model)]
(mt/with-temp* [parent-model [{emitter-parent-id :id}]
Emitter [{emitter-id :id} {:parameter_mappings {"my_id" [:variable [:template-tag "id"]]
"my_fail" [:variable [:template-tag "fail"]]}}]]
(testing "Sanity check: emitter-id should be non-nil"
(is (integer? emitter-id)))
(testing "Sanity check: make sure parameter mappings were defined the way we'd expect"
(is (= {:my_id [:variable [:template-tag "id"]]
:my_fail [:variable [:template-tag "fail"]]}
(db/select-one-field :parameter_mappings Emitter :id emitter-id))))
;; these are tied to the Card or Dashboad and Emitter above and will get cascade deleted. We can't use `with-temp*` for them
;; because it doesn't seem to work with tables with compound PKs
(db/insert! EmitterAction {:emitter_id emitter-id
:action_id action-id})
(condp = (type parent-model)
(type Card)
(db/insert! CardEmitter {:card_id emitter-parent-id
:emitter_id emitter-id})
(type Dashboard)
(db/insert! DashboardEmitter {:dashboard_id emitter-parent-id
:emitter_id emitter-id}))
(f (assoc context
:emitter-id emitter-id
:emitter-parent-id emitter-parent-id)))))
(defmacro with-dashboard-emitter
"Execute `body` with a newly created DashboardEmitter created for an Action with `:action-id`. Intended for use with the
`context` returned by with [[with-query-action]]. `bindings` is bound to a map with the keys `:emitter-id` and
`:emitter-parent-id` pointing to the dashboard-id.
(with-query-action [{:keys [action-id query-action-card-id], :as context}]
(with-dashboard-emitter [{:keys [emitter-id emitter-parent-id]} context]
(do-something)))"
{:style/indent 1
:arglists '([bindings {:keys [action-id], :as _action}] & body)}
[[bindings action] & body]
`(do-with-emitter Dashboard ~action (fn [~bindings] ~@body)))
(defmacro with-card-emitter
"Execute `body` with a newly created CardEmitter created for an Action with `:action-id`. Intended for use with the
`context` returned by with [[with-query-action]]. `bindings` is bound to a map with the keys `:emitter-id` and
`:emitter-card-id`.
`:emitter-parent-id` pointing to the card-id.
(with-query-action [{:keys [action-id query-action-card-id], :as context}]
(with-card-emitter [{:keys [emitter-id emitter-card-id]} context]
(with-card-emitter [{:keys [emitter-id emitter-parent-id]} context]
(do-something)))"
{:style/indent 1, :arglists '([bindings {:keys [action-id], :as _action}] & body)}
{:style/indent 1
:arglists '([bindings {:keys [action-id], :as _action}] & body)}
[[bindings action] & body]
`(do-with-card-emitter ~action (fn [~bindings] ~@body)))
`(do-with-emitter Card ~action (fn [~bindings] ~@body)))
(defn do-with-actions-enabled
"Impl for [[with-actions-enabled]]."
......
(ns metabase.api.emitter-test
(:require
[clojure.string :as str]
[clojure.test :refer :all]
[metabase.actions.test-util :as actions.test-util]
[metabase.models :refer [Card Dashboard]]
[metabase.models :refer [Card Dashboard Emitter]]
[metabase.test :as mt]
[metabase.util.schema :as su]
[schema.core :as s]))
......@@ -31,16 +32,87 @@
(mt/user-http-request :crowberto :post 200 "emitter" {:dashboard_id dashboard-id
:action_id action-id})))))))))))
(deftest execute-custom-action-test
(deftest update-emitter-test
(testing "PUT /api/emitter/:id"
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(actions.test-util/with-actions-test-data-and-actions-enabled
(actions.test-util/with-query-action [action]
(actions.test-util/with-card-emitter [{:keys [emitter-id]} action]
(testing "Should be able to update an emitter"
(mt/user-http-request :crowberto :put 204 (format "emitter/%d" emitter-id)
{:options {:a 1}})
(is (partial= {:options {:a 1}} (Emitter emitter-id))))
(testing "Should 404 if bad emitter-id"
(is (= "Not found."
(mt/user-http-request :crowberto :put 404 (format "emitter/%d" Integer/MAX_VALUE)
{}))))))))))
(deftest delete-emitter-test
(testing "DELETE /api/emitter/:id"
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(actions.test-util/with-actions-test-data-and-actions-enabled
(actions.test-util/with-query-action [action]
(actions.test-util/with-card-emitter [{:keys [emitter-id]} action]
(testing "Should be able to delete an emitter"
(is (nil? (mt/user-http-request :crowberto :delete 204 (format "emitter/%d" emitter-id)))))
(testing "Should 404 if bad emitter-id"
(is (= "Not found."
(mt/user-http-request :crowberto :delete 404 (format "emitter/%d" Integer/MAX_VALUE)
{}))))))))))
(deftest execute-query-action-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(actions.test-util/with-actions-test-data-and-actions-enabled
(actions.test-util/with-query-action [action]
(actions.test-util/with-card-emitter [{:keys [emitter-id]} action]
(let [emitter-path (format "emitter/%d/execute" emitter-id)]
(testing "Should be able to execute an emitter"
(is (= {:rows-affected 1}
(mt/user-http-request :crowberto :post 200 emitter-path
{:parameters {"my_id" {:type :number/=
:value 1}}}))))
(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"
(is (= {:rows-affected 0}
(mt/user-http-request :crowberto :post 200 emitter-path
{:parameters {"my_id" {:type :number/=
:value Integer/MAX_VALUE}}}))))
(testing "Should 404 if bad emitter-id"
(is (= "Not found."
(mt/user-http-request :crowberto :post 404 (format "emitter/%d/execute" Integer/MAX_VALUE)
{}))))
(testing "Missing parameter should fail gracefully"
(is (partial= {:message "Error executing QueryEmitter: 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."}
(mt/user-http-request :crowberto :post 500 emitter-path
{:parameters {}}))))
(testing "Sending an invalid number should fail gracefully"
(is (partial= {:message "Error executing QueryEmitter: Error building query parameter map: Error determining value for parameter \"id\": Unparseable number: \"BAD\""}
(mt/user-http-request :crowberto :post 500 emitter-path
{:parameters {"my_id" {:type :number/= :value "BAD"}}}))))))))))
(deftest execute-http-action-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(actions.test-util/with-actions-test-data-and-actions-enabled
(actions.test-util/with-query-action [context]
(actions.test-util/with-http-action [context]
(actions.test-util/with-card-emitter [{:keys [emitter-id]} context]
(testing "Should be able to execute an emitter"
(is (= {:rows-affected 1}
(mt/user-http-request :crowberto :post 200 (format "emitter/%d/execute" emitter-id)
{:parameters {"my_id" {:type :number/=
:value 1}}}))))
(is (= [1 "Bird Shop"]
(mt/first-row
(mt/run-mbql-query categories {:filter [:= $id 1]})))))))))
(let [emitter-path (format "emitter/%d/execute" emitter-id)]
(testing "Should be able to execute an emitter"
(is (= {:the_parameter 1}
(mt/user-http-request :crowberto :post 200 emitter-path
{:parameters {"my_id" {:type :number/= :value 1}}}))))
(testing "Should handle errors"
(is (= {:remote-status 400}
(mt/user-http-request :crowberto :post 400 emitter-path
{:parameters {"my_id" {:type :number/= :value 1}
"my_fail" {:type :text :value "true"}}}))))
(testing "Missing parameter should fail gracefully"
(is (partial= {:message "Problem building request: Cannot call the service: missing required parameters: #{\"id\"}"}
(mt/user-http-request :crowberto :post 500 emitter-path
{:parameters {}}))))
(testing "Sending an invalid number should fail gracefully"
(is (str/starts-with? (:message (mt/user-http-request :crowberto :post 500 emitter-path
{:parameters {"my_id" {:type :number/= :value "BAD"}}}))
"Problem building request:")))))))))
(ns metabase.models.action-test
(:require [clojure.test :refer :all]
[metabase.actions.test-util :as actions.test-util]
[metabase.models :refer [Emitter]]
[metabase.test :as mt]
[toucan.hydrate :refer [hydrate]]))
(deftest hydrate-query-action-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(actions.test-util/with-actions-test-data-and-actions-enabled
(actions.test-util/with-query-action [{:keys [query-action-card-id action-id] :as context}]
(actions.test-util/with-card-emitter [{:keys [emitter-id]} context]
(let [emitter (Emitter emitter-id)
hydrated-emitter (hydrate emitter :action)]
(is (partial=
{:id action-id
:name "Query Example"
:card {:id query-action-card-id}
:parameters [{:id "id" :type :number}]}
(:action hydrated-emitter)))))))))
(deftest hydrate-http-action-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(actions.test-util/with-actions-test-data-and-actions-enabled
(actions.test-util/with-http-action [{:keys [action-id] :as context}]
(actions.test-util/with-card-emitter [{:keys [emitter-id]} context]
(let [emitter (Emitter emitter-id)
hydrated-emitter (hydrate emitter :action)]
(is (partial=
{:id action-id
:name "Echo Example"
:parameters [{:id "id" :type :number}
{:id "fail" :type :text}]}
(:action hydrated-emitter)))))))))
(ns metabase.models.emitter-test
(:require [clojure.test :refer :all]
[metabase.actions.test-util :as actions.test-util]
[metabase.models :refer [Card Dashboard]]
[metabase.test :as mt]
[toucan.hydrate :refer [hydrate]]))
(deftest test-hydration
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(actions.test-util/with-actions-test-data-and-actions-enabled
(actions.test-util/with-query-action [context]
(actions.test-util/with-card-emitter [{:keys [emitter-id emitter-parent-id]} context]
(let [card (Card emitter-parent-id)
hydrated-card (hydrate card :emitters)]
(is (partial=
[{:id emitter-id}]
(:emitters hydrated-card)))))))))
(deftest dashboard-emitter-hydration-test
(mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
(actions.test-util/with-actions-test-data-and-actions-enabled
(actions.test-util/with-query-action [context]
(actions.test-util/with-dashboard-emitter [{:keys [emitter-id emitter-parent-id]} context]
(let [dashboard (Dashboard emitter-parent-id)
hydrated-card (hydrate dashboard [:emitters :action])]
(is (partial=
[{:id emitter-id}]
(:emitters hydrated-card)))))))))
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment