From 620767943c385e82bf234239a72e48df30a5e0b7 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Fri, 10 Jun 2022 13:31:45 -0700
Subject: [PATCH] [Actions] Execute custom actions endpoint (#23130)
* Execute custom actions endpoint [WIP] [ci skip]
* Wow! It's all working.
* Test fixes :wrench:
* Code cleanup
* More test fixes. :wrench:
* 2 final test fixes
* Remove unused namespace
---
shared/src/metabase/mbql/normalize.cljc | 5 +-
src/metabase/api/actions.clj | 4 +-
src/metabase/api/emitter.clj | 58 +++++++-
src/metabase/driver.clj | 10 +-
src/metabase/driver/h2.clj | 3 +-
src/metabase/driver/postgres.clj | 9 +-
src/metabase/driver/sql_jdbc/execute.clj | 50 ++++---
src/metabase/models/action.clj | 36 ++---
src/metabase/models/emitter.clj | 63 +++++----
src/metabase/query_processor.clj | 2 +-
src/metabase/query_processor/writeback.clj | 121 +++++++++++++++++
src/metabase/util.clj | 1 +
test/metabase/api/card_test.clj | 12 +-
test/metabase/api/dashboard_test.clj | 12 +-
test/metabase/api/emitter_test.clj | 70 ++++++++++
test/metabase/api/login_history_test.clj | 127 +++++++++---------
test/metabase/models/user_test.clj | 6 +-
test/metabase/server/middleware/auth_test.clj | 27 ++--
.../server/middleware/session_test.clj | 109 ++++++++-------
test/metabase/test/redefs.clj | 42 ++++--
test/metabase/test/util.clj | 9 +-
21 files changed, 545 insertions(+), 231 deletions(-)
create mode 100644 src/metabase/query_processor/writeback.clj
create mode 100644 test/metabase/api/emitter_test.clj
diff --git a/shared/src/metabase/mbql/normalize.cljc b/shared/src/metabase/mbql/normalize.cljc
index 154d1bf6bca..74c0fcebaef 100644
--- a/shared/src/metabase/mbql/normalize.cljc
+++ b/shared/src/metabase/mbql/normalize.cljc
@@ -251,8 +251,11 @@
(assoc :name tag-name))])))
template-tags))
-(defn- normalize-query-parameter [{:keys [type target], :as param}]
+(defn normalize-query-parameter
+ "Normalize a parameter in the query `:parameters` list."
+ [{:keys [type target id], :as param}]
(cond-> param
+ id (update :id mbql.u/qualified-name)
;; some things that get ran thru here, like dashcard param targets, do not have :type
type (update :type maybe-normalize-token)
target (update :target #(normalize-tokens % :ignore-path))))
diff --git a/src/metabase/api/actions.clj b/src/metabase/api/actions.clj
index e9e0bd57c17..06186c7babf 100644
--- a/src/metabase/api/actions.clj
+++ b/src/metabase/api/actions.clj
@@ -1,4 +1,5 @@
(ns metabase.api.actions
+ ;; TODO -- should probably rename this to `/api/action` for consistency since other API endpoints aren't plural
"`/api/actions/` endpoints."
(:require [cheshire.core :as json]
[compojure.core :as compojure :refer [POST]]
@@ -46,7 +47,6 @@
{database (s/maybe s/Int)}
(when database
(do-check-actions-enabled database nil))
- (api/check-superuser)
(let [cards+actions (db/query {:select [:card.*
[:db.settings :db_settings]
[:a.id :a_id]
@@ -103,4 +103,4 @@
(fn [driver]
(actions/row-action! (keyword action) driver query)))))
-(api/define-routes actions/+check-actions-enabled)
+(api/define-routes actions/+check-actions-enabled api/+check-superuser)
diff --git a/src/metabase/api/emitter.clj b/src/metabase/api/emitter.clj
index e31d17223a1..107f12669f4 100644
--- a/src/metabase/api/emitter.clj
+++ b/src/metabase/api/emitter.clj
@@ -1,10 +1,18 @@
(ns metabase.api.emitter
- (:require [compojure.core :refer [DELETE POST PUT]]
- [metabase.actions :as actions]
- [metabase.api.common :as api]
- [metabase.models :refer [CardEmitter DashboardEmitter Emitter]]
- [metabase.util.i18n :refer [tru]]
- [toucan.db :as db]))
+ (:require
+ [compojure.core :refer [DELETE POST PUT]]
+ [metabase.actions :as actions]
+ [metabase.api.common :as api]
+ [metabase.mbql.schema :as mbql.s]
+ [metabase.models
+ :refer [CardEmitter DashboardEmitter Emitter EmitterAction]]
+ [metabase.query-processor.writeback :as qp.writeback]
+ [metabase.util :as u]
+ [metabase.util.i18n :refer [tru]]
+ [metabase.util.schema :as su]
+ [schema.core :as s]
+ [toucan.db :as db]
+ [toucan.hydrate :refer [hydrate]]))
(api/defendpoint POST "/"
"Endpoint to create an emitter."
@@ -31,4 +39,40 @@
(db/delete! Emitter :id emitter-id)
api/generic-204-no-content)
-(api/define-routes actions/+check-actions-enabled)
+(defn- emitter
+ "Fetch the Emitter with `emitter-id` and extra info from FK-related tables for custom Emitter execution purposes."
+ [emitter-id]
+ (when-let [emitter (->> (db/query {:select [:*]
+ :from [[Emitter :emitter]]
+ :left-join [[EmitterAction :emitter_action]
+ [:= :emitter_action.emitter_id :emitter.id]
+ ;; TODO -- not 100% sure we need CardEmitter and DashboardEmitter, we'd
+ ;; only need that if we wanted to hydrate the Card or Dashboard they
+ ;; came from.
+ [CardEmitter :card_emitter]
+ [:= :card_emitter.emitter_id :emitter.id]
+ [DashboardEmitter :dashboard_emitter]
+ [:= :dashboard_emitter.emitter_id :emitter.id]]
+ :where [:= :id emitter-id]})
+ (db/do-post-select Emitter)
+ first)]
+ (hydrate emitter [:action :card])))
+
+(def ^:private CustomActionParametersMap
+ (-> {s/Keyword {:type (apply s/enum (map u/qualified-name (keys mbql.s/parameter-types))) ; type will come in as a string.
+ :value s/Any
+ s/Keyword s/Any}}
+ (su/with-api-error-message "map of parameter name or ID -> map of parameter `:value` and `:type` of the value")))
+
+(api/defendpoint POST "/:id/execute"
+ "Execute a custom emitter."
+ [id :as {{:keys [parameters]} :body}]
+ {parameters (s/maybe CustomActionParametersMap)}
+ (let [emitter (api/check-404 (emitter id))]
+ (or (get-in emitter [:action :card])
+ (throw (ex-info (tru "No Query Action found for Emitter {0}. Only Query Actions are supported at this point in time."
+ id)
+ {:status-code 400, :emitter emitter})))
+ (qp.writeback/execute-query-emitter! emitter parameters)))
+
+(api/define-routes actions/+check-actions-enabled api/+check-superuser)
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index 86cbbeeaadd..d044b08e63a 100644
--- a/src/metabase/driver.clj
+++ b/src/metabase/driver.clj
@@ -438,7 +438,8 @@
;; Does the driver support experimental "writeback" actions like "delete this row" or "insert a new row" from 44+?
:actions
- ;; Does the driver support custom writeback actions using `is_write` Saved Questions
+ ;; Does the driver support custom writeback actions using `is_write` Saved Questions. Drivers that support this must
+ ;; implement [[execute-write-query!]]
:actions/custom})
(defmulti supports?
@@ -712,3 +713,10 @@
(defmethod superseded-by :default
[_]
nil)
+
+(defmulti execute-write-query!
+ "Execute a writeback query (from an `is_write` Card) e.g. one powering a custom
+ `QueryAction` (see [[metabase.models.action]]). Drivers that support `:actions/custom` must implement this method."
+ {:added "0.44.0", :arglists '([driver query])}
+ dispatch-on-initialized-driver
+ :hierarchy #'hierarchy)
diff --git a/src/metabase/driver/h2.clj b/src/metabase/driver/h2.clj
index a8812ec9ab0..1d8bb1debb0 100644
--- a/src/metabase/driver/h2.clj
+++ b/src/metabase/driver/h2.clj
@@ -30,7 +30,8 @@
(doseq [[feature supported?] {:full-join false
:regex false
:percentile-aggregations false
- :actions true}]
+ :actions true
+ :actions/custom true}]
(defmethod driver/database-supports? [:h2 feature]
[_driver _feature _database]
supported?))
diff --git a/src/metabase/driver/postgres.clj b/src/metabase/driver/postgres.clj
index d039a50e9e3..068bd62a8bf 100644
--- a/src/metabase/driver/postgres.clj
+++ b/src/metabase/driver/postgres.clj
@@ -59,10 +59,11 @@
[_driver _feat db]
(-> db :options :persist-models-enabled))
-(defmethod driver/database-supports? [:postgres :actions]
- [driver _feat _db]
- ;; only supported for Postgres for right now. Not supported for child drivers like Redshift or whatever.
- (= driver :postgres))
+(doseq [feature [:actions :actions/custom]]
+ (defmethod driver/database-supports? [:postgres feature]
+ [driver _feat _db]
+ ;; only supported for Postgres for right now. Not supported for child drivers like Redshift or whatever.
+ (= driver :postgres)))
(defn- ->timestamp [honeysql-form]
(hx/cast-unless-type-in "timestamp" #{"timestamp" "timestamptz" "date"} honeysql-form))
diff --git a/src/metabase/driver/sql_jdbc/execute.clj b/src/metabase/driver/sql_jdbc/execute.clj
index 774e7ebb2d3..8e479560872 100644
--- a/src/metabase/driver/sql_jdbc/execute.clj
+++ b/src/metabase/driver/sql_jdbc/execute.clj
@@ -5,6 +5,7 @@
for JDBC drivers that do not support `java.time` classes can be found in
`metabase.driver.sql-jdbc.execute.legacy-impl`. "
(:require [clojure.core.async :as a]
+ [clojure.java.jdbc :as jdbc]
[clojure.string :as str]
[clojure.tools.logging :as log]
[java-time :as t]
@@ -329,29 +330,27 @@
(.close stmt)
(throw e)))))
-(defn- prepared-statement*
- ^PreparedStatement [driver conn sql params canceled-chan]
- ;; if canceled-chan gets a message, cancel the PreparedStatement
- (let [^PreparedStatement stmt (prepared-statement driver conn sql params)]
+(defn- wire-up-canceled-chan-to-cancel-Statement!
+ "If `canceled-chan` gets a message, cancel the Statement `stmt`."
+ [^Statement stmt canceled-chan]
+ (when canceled-chan
(a/go
(when (a/<! canceled-chan)
- (log/debug (trs "Query canceled, calling PreparedStatement.cancel()"))
+ (log/debug (trs "Query canceled, calling Statement.cancel()"))
(u/ignore-exceptions
- (.cancel stmt))))
- stmt))
+ (.cancel stmt))))))
+
+(defn- prepared-statement*
+ ^PreparedStatement [driver conn sql params canceled-chan]
+ (doto (prepared-statement driver conn sql params)
+ (wire-up-canceled-chan-to-cancel-Statement! canceled-chan)))
(defn- use-statement? [driver params]
(and (statement-supported? driver) (empty? params)))
(defn- statement* ^Statement [driver conn canceled-chan]
- ;; if canceled-chan gets a message, cancel the Statement
- (let [^Statement stmt (statement driver conn)]
- (a/go
- (when (a/<! canceled-chan)
- (log/debug (trs "Query canceled, calling Statement.cancel()"))
- (u/ignore-exceptions
- (.cancel stmt))))
- stmt))
+ (doto (statement driver conn)
+ (wire-up-canceled-chan-to-cancel-Statement! canceled-chan)))
(defn- statement-or-prepared-statement ^Statement [driver conn sql params canceled-chan]
(if (use-statement? driver params)
@@ -507,6 +506,27 @@
(respond results-metadata (reducible-rows driver rs rsmeta (qp.context/canceled-chan context)))))))
+;;; +----------------------------------------------------------------------------------------------------------------+
+;;; | Actions Stuff |
+;;; +----------------------------------------------------------------------------------------------------------------+
+
+(defmethod driver/execute-write-query! :sql-jdbc
+ [driver {{sql :query, :keys [params]} :native}]
+ {:pre [(string? sql)]}
+ (try
+ (let [{:keys [details]} (qp.store/database)
+ jdbc-spec (sql-jdbc.conn/connection-details->spec driver details)]
+ ;; TODO -- should this be done in a transaction? Should we set the isolation level?
+ (with-open [conn (jdbc/get-connection jdbc-spec)
+ stmt (statement-or-prepared-statement driver conn sql params nil)]
+ {:rows-affected (if (instance? PreparedStatement stmt)
+ (.executeUpdate ^PreparedStatement stmt)
+ (.executeUpdate stmt sql))}))
+ (catch Throwable e
+ (throw (ex-info (tru "Error executing write query: {0}" (ex-message e))
+ {:sql sql, :params params, :type qp.error-type/invalid-query}
+ e)))))
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Convenience Imports from Old Impl |
;;; +----------------------------------------------------------------------------------------------------------------+
diff --git a/src/metabase/models/action.clj b/src/metabase/models/action.clj
index d4723f3581c..ca014c5e38d 100644
--- a/src/metabase/models/action.clj
+++ b/src/metabase/models/action.clj
@@ -10,11 +10,10 @@
(u/strict-extend (class Action)
models/IModel
(merge models/IModelDefaults
- {:types (constantly {:type :keyword})
- :properties (constantly {:timestamped? true})}))
+ {:types (constantly {:type :keyword})
+ :properties (constantly {:timestamped? true})}))
-(u/strict-extend
- (class QueryAction)
+(u/strict-extend (class QueryAction)
models/IModel
(merge models/IModelDefaults
;; This is ok as long as we're 1:1
@@ -24,15 +23,20 @@
"Hydrates Action from Emitter"
{:batched-hydrate :action}
[emitters]
- (let [actions (->> {:select [:emitter_action.emitter_id
- :query_action.card_id
- :action.type]
- :from [[Action :action]]
- ;; Will need to change if we stop being 1:1
- :join [:emitter_action [:= :emitter_action.action_id :action.id]
- [QueryAction :query_action] [:= :query_action.action_id :action.id]]
- :where [:in :emitter_action.emitter_id (map :id emitters)]}
- (db/query)
- (db/do-post-select Action)
- (m/index-by :id))]
- (map #(assoc % :action (get actions (:action_id %))) emitters)))
+ ;; emitters apparently might actually be `[nil]` (not 100% sure why) so just make sure we're not doing anything dumb
+ ;; if this is the case.
+ (let [emitter-ids (filter some? (map :id emitters))
+ actions (when (seq emitter-ids)
+ (->> {:select [:emitter_action.emitter_id
+ :query_action.card_id
+ :action.type]
+ :from [[Action :action]]
+ ;; Will need to change if we stop being 1:1
+ :join [:emitter_action [:= :emitter_action.action_id :action.id]
+ [QueryAction :query_action] [:= :query_action.action_id :action.id]]
+ :where [:in :emitter_action.emitter_id emitter-ids]}
+ db/query
+ (db/do-post-select Action)
+ (m/index-by :emitter_id)))]
+ (for [{emitter-id :id, :as emitter} emitters]
+ (some-> emitter (assoc :action (get actions emitter-id))))))
diff --git a/src/metabase/models/emitter.clj b/src/metabase/models/emitter.clj
index 617dcc33fd5..49166e1e42e 100644
--- a/src/metabase/models/emitter.clj
+++ b/src/metabase/models/emitter.clj
@@ -1,23 +1,37 @@
(ns metabase.models.emitter
- (:require [medley.core :as m]
- [metabase.models.card :refer [Card]]
- [metabase.models.dashboard :refer [Dashboard]]
- [metabase.util :as u]
- [toucan.db :as db]
- [toucan.models :as models]))
+ (:require
+ [medley.core :as m]
+ [metabase.mbql.normalize :as mbql.normalize]
+ [metabase.models.card :refer [Card]]
+ [metabase.models.dashboard :refer [Dashboard]]
+ [metabase.models.interface :as mi]
+ [metabase.util :as u]
+ [toucan.db :as db]
+ [toucan.models :as models]))
(models/defmodel CardEmitter :card_emitter)
(models/defmodel DashboardEmitter :dashboard_emitter)
(models/defmodel Emitter :emitter)
(models/defmodel EmitterAction :emitter_action)
+(defn- normalize-parameter-mappings
+ [parameter-mappings]
+ (into {}
+ (map (fn [[param-id target]]
+ [param-id (:target (mbql.normalize/normalize-query-parameter {:target target}))]))
+ parameter-mappings))
+
+(models/add-type! ::parameter-mappings
+ :in (comp mi/json-in normalize-parameter-mappings)
+ :out (comp (mi/catch-normalization-exceptions normalize-parameter-mappings) mi/json-out-with-keywordization))
+
(u/strict-extend
- (class Emitter)
- models/IModel
- (merge models/IModelDefaults
- {:types (constantly {:parameter_mappings :json
- :options :json})
- :properties (constantly {:timestamped? true})}))
+ (class Emitter)
+ models/IModel
+ (merge models/IModelDefaults
+ {:types (constantly {:parameter_mappings ::parameter-mappings
+ :options :json})
+ :properties (constantly {:timestamped? true})}))
(u/strict-extend
(class EmitterAction)
@@ -27,14 +41,15 @@
{:primary-key (constantly :emitter_id)}))
(defn- pre-insert
- [emitter]
+ [{action-id :action_id, :as emitter}]
+ {:pre [(integer? action-id)]}
(let [base-emitter (db/insert! Emitter (select-keys emitter [:parameter_mappings :options]))]
(db/insert! EmitterAction (-> emitter
(select-keys [:action_id])
(assoc :emitter_id (:id base-emitter))))
(-> emitter
(select-keys [:dashboard_id :card_id])
- (assoc :emitter_id (:id base-emitter)))))
+ (assoc :emitter_id (u/the-id base-emitter)))))
(defn- pre-update
[emitter]
@@ -51,23 +66,21 @@
(db/delete! Emitter :id (:emitter_id emitter))
emitter)
-(u/strict-extend
- (class DashboardEmitter)
- models/IModel
+(def ^:private Emitter-subtype-IModel-impl
+ "[[models/IModel]] impl for `DashboardEmitter` and `CardEmitter`"
(merge models/IModelDefaults
- {:primary-key (constantly :emitter_id) ;; This is ok as long as we're 1:1
+ {:primary-key (constantly :emitter_id) ; This is ok as long as we're 1:1
:pre-delete pre-delete
:pre-update pre-update
:pre-insert pre-insert}))
-(u/strict-extend
- (class CardEmitter)
+(u/strict-extend (class DashboardEmitter)
models/IModel
- (merge models/IModelDefaults
- {:primary-key (constantly :emitter_id) ;; This is ok as long as we're 1:1
- :pre-delete pre-delete
- :pre-update pre-update
- :pre-insert pre-insert}))
+ Emitter-subtype-IModel-impl)
+
+(u/strict-extend (class CardEmitter)
+ models/IModel
+ Emitter-subtype-IModel-impl)
(defn emitters
"Hydrate emitters onto a list of dashboards or cards."
diff --git a/src/metabase/query_processor.clj b/src/metabase/query_processor.clj
index 816aae383d1..b3be3505785 100644
--- a/src/metabase/query_processor.clj
+++ b/src/metabase/query_processor.clj
@@ -178,7 +178,7 @@
rff
post-processing-middleware))
-(def ^:private around-middleware
+(def around-middleware
"Middleware that goes AROUND *all* the other middleware (even for pre-processing only or compilation only). Has the
form
diff --git a/src/metabase/query_processor/writeback.clj b/src/metabase/query_processor/writeback.clj
new file mode 100644
index 00000000000..bc8e43ec440
--- /dev/null
+++ b/src/metabase/query_processor/writeback.clj
@@ -0,0 +1,121 @@
+(ns metabase.query-processor.writeback
+ "Code for executing writeback queries."
+ (:require
+ [clojure.tools.logging :as log]
+ [metabase.api.actions :as api.actions]
+ [metabase.driver :as driver]
+ [metabase.query-processor :as qp]
+ [metabase.query-processor.error-type :as qp.error-type]
+ [metabase.query-processor.middleware.parameters :as parameters]
+ [metabase.query-processor.store :as qp.store]
+ [metabase.util :as u]
+ [metabase.util.i18n :refer [tru]]
+ [metabase.util.schema :as su]
+ [schema.core :as s]))
+
+(defn- map-parameters
+ "Take the `parameters` map passed in to an endpoint like `POST /api/emitter/:id/execute` and map it to the parameters
+ in the underlying `Action` so they can be attached to the query that gets passed to [[qp/execute-write-query!]].
+
+ Incoming API request `:parameters` should look like
+
+ {:parameters {\"my_id\" {:type :number/=
+ :value 10}}}
+
+ And `parameter_mappings` should look like
+
+ {:my_id [:template-tag \"id\"]}
+
+ We need to convert these to a list like
+
+ [{:target [:template-tag \"id\"]
+ :value 10}]
+
+ before passing to the QP code."
+ [parameters parameter-mappings]
+ (mapv (fn [[param-id param-value-info]]
+ (let [target (or (get parameter-mappings param-id)
+ (throw (ex-info (tru "No parameter mapping found for parameter {0}. Found: {1}"
+ (pr-str param-id)
+ (pr-str (set (keys parameter-mappings))))
+ {:status-code 400
+ :type qp.error-type/invalid-parameter
+ :parameters parameters
+ :mappings parameter-mappings})))]
+ (merge {:id param-id
+ :target target}
+ param-value-info)))
+ parameters))
+
+(defn- apply-middleware [qp middleware-fns]
+ (reduce
+ (fn [qp middleware]
+ (if middleware
+ (middleware qp)
+ qp))
+ qp
+ middleware-fns))
+
+(defn- writeback-qp []
+ ;; `rff` and `context` are not currently used by the writeback QP stuff, so these parameters can be ignored; we pass
+ ;; in `nil` for these below.
+ (letfn [(qp* [{database-id :database, :as query} _rff _context]
+ (let [query (parameters/substitute-parameters query)]
+ ;; make sure that the Database supports the `:actions/custom` feature
+ (when-not (driver/database-supports? driver/*driver* :actions/custom (qp.store/database))
+ (throw (ex-info (tru "{0} database {1} does not support executing custom actions."
+ (name driver/*driver*) database-id)
+ {:status-code 400
+ :type qp.error-type/invalid-query})))
+ ;; ok, now execute the query.
+ (log/debugf "Executing query\n\n%s" (u/pprint-to-str query))
+ (driver/execute-write-query! driver/*driver* query)))]
+ (apply-middleware qp* qp/around-middleware)))
+
+(defn execute-write-query!
+ "Execute an writeback query from an `is_write` SavedQuestion."
+ [{query-type :type, :as query}]
+ ;; make sure this is a native query.
+ (when-not (= query-type :native)
+ (throw (ex-info (tru "Only native queries can be executed as write queries.")
+ {:type qp.error-type/invalid-query, :status-code 400, :query query})))
+ ((writeback-qp) query nil nil))
+
+(def ^:private HydratedQueryEmitter
+ {:id su/IntGreaterThanZero
+ :action {:card {:dataset_query su/Map
+ :is_write s/Bool
+ s/Keyword s/Any}
+ s/Keyword s/Any}
+ s/Keyword s/Any})
+
+(s/defn execute-query-emitter!
+ "Execute a `QueryEmitter` with parameters as passed in to the `POST /api/emitter/:id/execute`
+ endpoint (see [[map-parameters]] for a description of their shape). `emitter` should already be hydrated with its `:action`
+ and the Action's `:card`."
+ [{{:keys [card]} :action, emitter-id :id, :as emitter} :- HydratedQueryEmitter
+ parameters]
+ (when-not (:is_write card)
+ (throw (ex-info (tru "Cannot execute emitter {0}: Card {1} is not marked as `is_write`"
+ emitter-id
+ (:id card))
+ {:status-code 400, :emitter emitter})))
+ (log/tracef "Executing emitter\n\n%s" (u/pprint-to-str emitter))
+ (try
+ (log/tracef "Mapping parameters\n\n%s\nwith mappings\n\n%s"
+ (u/pprint-to-str parameters)
+ (u/pprint-to-str (:parameter_mappings emitter)))
+ (let [parameters (map-parameters parameters (:parameter_mappings emitter))
+ {database-id :database, :as query} (assoc (:dataset_query card) :parameters parameters)]
+ (log/debugf "Query (before preprocessing):\n\n%s" (u/pprint-to-str query))
+ ;; make sure actions are enabled and supported for this Database. (We'll check the `:actions/custom` feature flag
+ ;; later once the DB is fetched in and the QP store -- see above)
+ (api.actions/do-check-actions-enabled
+ database-id
+ (fn [_driver]
+ (execute-write-query! query))))
+ (catch Throwable e
+ (throw (ex-info (tru "Error executing QueryEmitter: {0}" (ex-message e))
+ {:emitter emitter
+ :parameters parameters}
+ e)))))
diff --git a/src/metabase/util.clj b/src/metabase/util.clj
index 49594387dc8..2d25424d9f0 100644
--- a/src/metabase/util.clj
+++ b/src/metabase/util.clj
@@ -188,6 +188,7 @@
(.isReachable host-addr host-up-timeout))
(catch Throwable _ false)))
+;; TODO -- maybe renaming this to `adoto` or `doto<>` or something would be a little clearer.
(defmacro prog1
"Execute `first-form`, then any other expressions in `body`, presumably for side-effects; return the result of
`first-form`.
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index 504dcd6fb45..53e3e7fc6e7 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -732,16 +732,16 @@
(testing "GET /api/card/:id"
(testing "Fetch card with an emitter"
(mt/with-temp* [Card [read-card {:name "Test Read Card"}]
- Card [write-card {:is_write true :name "Test Write Card"}]
- CardEmitter [emitter {:action_id (db/select-field :action_id QueryAction :card_id (u/the-id write-card))
- :card_id (u/the-id read-card)}]]
+ Card [write-card {:is_write true :name "Test Write Card"}]]
+ (db/insert! CardEmitter {:action_id (u/the-id (db/select-one-field :action_id QueryAction :card_id (u/the-id write-card)))
+ :card_id (u/the-id read-card)})
(testing "admin sees emitters"
(is (partial=
- {:emitters [{:action {:type "row" :card {:name "Test Write Card"}}}]}
- (mt/user-http-request :crowberto :get 200 (format "card/%d" (u/the-id read-card))))))
+ {:emitters [{:action {:type "row" :card {:name "Test Write Card"}}}]}
+ (mt/user-http-request :crowberto :get 200 (format "card/%d" (u/the-id read-card))))))
(testing "non-admin does not see emitters"
(is (nil?
- (:emitters (mt/user-http-request :rasta :get 200 (format "card/%d" (u/the-id read-card)))))))))))
+ (:emitters (mt/user-http-request :rasta :get 200 (format "card/%d" (u/the-id read-card)))))))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | UPDATING A CARD |
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index b29a2aa6bcf..eb5dd0c70af 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -344,16 +344,16 @@
(testing "GET /api/dashboard/:id"
(testing "Fetch dashboard with an emitter"
(mt/with-temp* [Dashboard [dashboard {:name "Test Dashboard"}]
- Card [write-card {:is_write true :name "Test Write Card"}]
- DashboardEmitter [emitter {:action_id (db/select-field :action_id QueryAction :card_id (u/the-id write-card))
- :dashboard_id (u/the-id dashboard)}]]
+ Card [write-card {:is_write true :name "Test Write Card"}]]
+ (db/insert! DashboardEmitter {:action_id (u/the-id (db/select-one-field :action_id QueryAction :card_id (u/the-id write-card)))
+ :dashboard_id (u/the-id dashboard)})
(testing "admin sees emitters"
(is (partial=
- {:emitters [{:action {:type "row" :card {:name "Test Write Card"}}}]}
- (dashboard-response (mt/user-http-request :crowberto :get 200 (format "dashboard/%d" (u/the-id dashboard)))))))
+ {:emitters [{:action {:type "row" :card {:name "Test Write Card"}}}]}
+ (dashboard-response (mt/user-http-request :crowberto :get 200 (format "dashboard/%d" (u/the-id dashboard)))))))
(testing "non-admin does not see emitters"
(is (nil?
- (:emitters (dashboard-response (mt/user-http-request :rasta :get 200 (format "dashboard/%d" (u/the-id dashboard))))))))))))
+ (:emitters (dashboard-response (mt/user-http-request :rasta :get 200 (format "dashboard/%d" (u/the-id dashboard))))))))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | PUT /api/dashboard/:id |
diff --git a/test/metabase/api/emitter_test.clj b/test/metabase/api/emitter_test.clj
new file mode 100644
index 00000000000..f899c2edfca
--- /dev/null
+++ b/test/metabase/api/emitter_test.clj
@@ -0,0 +1,70 @@
+(ns metabase.api.emitter-test
+ (:require
+ [clojure.test :refer :all]
+ [metabase.actions.test-util :as actions.test-util]
+ [metabase.models
+ :refer [Action Card CardEmitter Database Emitter EmitterAction QueryAction]]
+ [metabase.test :as mt]
+ [toucan.db :as db]))
+
+(defn- do-with-query-action [f]
+ (mt/with-temp* [Card [{card-id :id} {:database_id (mt/id)
+ :dataset_query {:database (mt/id)
+ :type :native
+ :native {:query (str "UPDATE categories\n"
+ "SET name = 'Bird Shop'\n"
+ "WHERE id = {{id}}")
+ :template-tags {"id" {:name "id"
+ :display-name "ID"
+ :type :number
+ :required true}}}}
+ :is_write true}]
+ Action [{action-id :id} {:type :row}]]
+ ;; this is tied to the Card and Action 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! QueryAction {:action_id action-id
+ :card_id card-id})
+ (f {:query-action-card-id card-id
+ :action-id action-id})))
+
+(defn- do-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-actions-setup [thunk]
+ (actions.test-util/with-actions-test-data
+ (mt/with-temporary-setting-values [experimental-enable-actions true]
+ (mt/with-temp-vals-in-db Database (mt/id) {:settings {:database-enable-actions true}}
+ (thunk)))))
+
+(deftest execute-custom-action-test
+ (mt/test-drivers (mt/normal-drivers-with-feature :actions/custom)
+ (do-with-actions-setup
+ (fn []
+ (do-with-query-action
+ (fn [context]
+ (do-with-card-emitter
+ context
+ (fn [{:keys [emitter-id], :as _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]}))))))))))))
diff --git a/test/metabase/api/login_history_test.clj b/test/metabase/api/login_history_test.clj
index 0eb800abe74..5ce9444f318 100644
--- a/test/metabase/api/login_history_test.clj
+++ b/test/metabase/api/login_history_test.clj
@@ -3,7 +3,8 @@
[metabase.models :refer [LoginHistory Session User]]
[metabase.test :as mt]
[metabase.util :as u]
- [schema.core :as s]))
+ [schema.core :as s]
+ [toucan.db :as db]))
;; don't run these tests when running driver tests (i.e., `DRIVERS` is set) because they tend to flake
(use-fixtures :each (fn [thunk]
@@ -19,65 +20,65 @@
(deftest login-history-test
(testing "GET /api/login-history/current"
(let [session-id (str (java.util.UUID/randomUUID))]
- (mt/with-temp* [User [user]
- Session [session {:id session-id, :user_id (u/the-id user)}]
- LoginHistory [_ {:timestamp #t "2021-03-18T19:52:41.808482Z"
- :user_id (u/the-id user)
- :device_id "e9b49ec7-bc64-4a83-9b1a-ecd3ae26ba9d"
- :device_description windows-user-agent
- :ip_address "185.233.100.23"
- :session_id session-id}]
- LoginHistory [_ {:timestamp #t "2021-03-18T20:04:24.727300Z"
- :user_id (u/the-id user)
- :device_id "e9b49ec7-bc64-4a83-9b1a-ecd3ae26ba9d"
- :device_description "Apache-HttpClient/4.5.10 (Java/14.0.1)"
- :ip_address "127.0.0.1"}]
- LoginHistory [_ {:timestamp #t "2021-03-18T20:55:50.955232Z"
- :user_id (u/the-id user)
- :device_id "e9b49ec7-bc64-4a83-9b1a-ecd3ae26ba9d"
- :device_description ios-user-agent
- :ip_address "0:0:0:0:0:0:0:1"}]
- LoginHistory [_ {:timestamp #t "2021-03-18T19:52:20.172351Z"
- :user_id (u/the-id user)
- :device_id "e9b49ec7-bc64-4a83-9b1a-ecd3ae26ba9d"
- :device_description windows-user-agent
- :ip_address "52.206.149.9"}]
- ;; this one shouldn't show up because it's from a different User
- LoginHistory [_ {:timestamp #t "2021-03-17T19:00Z"
- :user_id (mt/user->id :rasta)
- :device_id "e9b49ec7-bc64-4a83-9b1a-ecd3ae26ba9d"
- :device_description windows-user-agent
- :ip_address "52.206.149.9"}]]
- (is (schema= [(s/one
- {:timestamp (s/eq "2021-03-18T20:55:50.955232Z")
- :device_description (s/eq "Mobile Browser (Mobile Safari/iOS)")
- :ip_address (s/eq "0:0:0:0:0:0:0:1")
- :active (s/eq false)
- :location (s/eq "Unknown location")
- :timezone (s/eq nil)}
- "localhost ipv6")
- (s/one
- {:timestamp (s/eq "2021-03-18T20:04:24.7273Z")
- :device_description (s/eq "Library (Apache-HttpClient/JVM (Java))")
- :ip_address (s/eq "127.0.0.1")
- :active (s/eq false)
- :location (s/eq "Unknown location")
- :timezone (s/eq nil)}
- "localhost ipv4")
- (s/one
- {:timestamp (s/eq "2021-03-18T20:52:41.808482+01:00")
- :device_description (s/eq "Browser (Chrome/Windows)")
- :ip_address (s/eq "185.233.100.23")
- :active (s/eq true)
- :location #"France"
- :timezone (s/eq "CET")}
- "France")
- (s/one
- {:timestamp (s/eq "2021-03-18T15:52:20.172351-04:00")
- :device_description (s/eq "Browser (Chrome/Windows)")
- :ip_address (s/eq "52.206.149.9")
- :active (s/eq false)
- :location (s/eq "Ashburn, Virginia, United States")
- :timezone (s/eq "ET")}
- "Virginia")]
- (mt/client session-id :get 200 "login-history/current")))))))
+ (mt/with-temp User [user]
+ (db/insert! Session {:id session-id, :user_id (u/the-id user)})
+ (mt/with-temp* [LoginHistory [_ {:timestamp #t "2021-03-18T19:52:41.808482Z"
+ :user_id (u/the-id user)
+ :device_id "e9b49ec7-bc64-4a83-9b1a-ecd3ae26ba9d"
+ :device_description windows-user-agent
+ :ip_address "185.233.100.23"
+ :session_id session-id}]
+ LoginHistory [_ {:timestamp #t "2021-03-18T20:04:24.727300Z"
+ :user_id (u/the-id user)
+ :device_id "e9b49ec7-bc64-4a83-9b1a-ecd3ae26ba9d"
+ :device_description "Apache-HttpClient/4.5.10 (Java/14.0.1)"
+ :ip_address "127.0.0.1"}]
+ LoginHistory [_ {:timestamp #t "2021-03-18T20:55:50.955232Z"
+ :user_id (u/the-id user)
+ :device_id "e9b49ec7-bc64-4a83-9b1a-ecd3ae26ba9d"
+ :device_description ios-user-agent
+ :ip_address "0:0:0:0:0:0:0:1"}]
+ LoginHistory [_ {:timestamp #t "2021-03-18T19:52:20.172351Z"
+ :user_id (u/the-id user)
+ :device_id "e9b49ec7-bc64-4a83-9b1a-ecd3ae26ba9d"
+ :device_description windows-user-agent
+ :ip_address "52.206.149.9"}]
+ ;; this one shouldn't show up because it's from a different User
+ LoginHistory [_ {:timestamp #t "2021-03-17T19:00Z"
+ :user_id (mt/user->id :rasta)
+ :device_id "e9b49ec7-bc64-4a83-9b1a-ecd3ae26ba9d"
+ :device_description windows-user-agent
+ :ip_address "52.206.149.9"}]]
+ (is (schema= [(s/one
+ {:timestamp (s/eq "2021-03-18T20:55:50.955232Z")
+ :device_description (s/eq "Mobile Browser (Mobile Safari/iOS)")
+ :ip_address (s/eq "0:0:0:0:0:0:0:1")
+ :active (s/eq false)
+ :location (s/eq "Unknown location")
+ :timezone (s/eq nil)}
+ "localhost ipv6")
+ (s/one
+ {:timestamp (s/eq "2021-03-18T20:04:24.7273Z")
+ :device_description (s/eq "Library (Apache-HttpClient/JVM (Java))")
+ :ip_address (s/eq "127.0.0.1")
+ :active (s/eq false)
+ :location (s/eq "Unknown location")
+ :timezone (s/eq nil)}
+ "localhost ipv4")
+ (s/one
+ {:timestamp (s/eq "2021-03-18T20:52:41.808482+01:00")
+ :device_description (s/eq "Browser (Chrome/Windows)")
+ :ip_address (s/eq "185.233.100.23")
+ :active (s/eq true)
+ :location #"France"
+ :timezone (s/eq "CET")}
+ "France")
+ (s/one
+ {:timestamp (s/eq "2021-03-18T15:52:20.172351-04:00")
+ :device_description (s/eq "Browser (Chrome/Windows)")
+ :ip_address (s/eq "52.206.149.9")
+ :active (s/eq false)
+ :location (s/eq "Ashburn, Virginia, United States")
+ :timezone (s/eq "ET")}
+ "Virginia")]
+ (mt/client session-id :get 200 "login-history/current"))))))))
diff --git a/test/metabase/models/user_test.clj b/test/metabase/models/user_test.clj
index 6bf5efd2344..437e498d68d 100644
--- a/test/metabase/models/user_test.clj
+++ b/test/metabase/models/user_test.clj
@@ -397,9 +397,9 @@
(db/select-one-field :reset_token User :id user-id)))))
(testing "should clear out all existing Sessions"
- (mt/with-temp* [User [{user-id :id}]
- Session [_ {:id (str (java.util.UUID/randomUUID)), :user_id user-id}]
- Session [_ {:id (str (java.util.UUID/randomUUID)), :user_id user-id}]]
+ (mt/with-temp* [User [{user-id :id}]]
+ (dotimes [_ 2]
+ (db/insert! Session {:id (str (java.util.UUID/randomUUID)), :user_id user-id}))
(letfn [(session-count [] (db/count Session :user_id user-id))]
(is (= 2
(session-count)))
diff --git a/test/metabase/server/middleware/auth_test.clj b/test/metabase/server/middleware/auth_test.clj
index 3340a6c46ba..978e913c8d9 100644
--- a/test/metabase/server/middleware/auth_test.clj
+++ b/test/metabase/server/middleware/auth_test.clj
@@ -9,8 +9,7 @@
[metabase.test.data.users :as test.users]
[metabase.test.fixtures :as fixtures]
[ring.mock.request :as ring.mock]
- [toucan.db :as db]
- [toucan.util.test :as tt])
+ [toucan.db :as db])
(:import java.util.UUID))
(use-fixtures :once (fixtures/initialize :db :test-users :web-server))
@@ -38,11 +37,13 @@
(deftest wrap-current-user-info-test
(testing "Valid requests should add `metabase-user-id` to requests with valid session info"
(let [session-id (random-session-id)]
- (tt/with-temp Session [_ {:id session-id
- :user_id (test.users/user->id :rasta)}]
+ (try
+ (db/insert! Session {:id session-id
+ :user_id (test.users/user->id :rasta)})
(is (= (test.users/user->id :rasta)
(-> (auth-enforced-handler (request-with-session-id session-id))
- :metabase-user-id))))))
+ :metabase-user-id)))
+ (finally (db/delete! Session :id session-id)))))
(testing "Invalid requests should return unauthed response"
(testing "when no session ID is sent with request"
@@ -54,23 +55,27 @@
;; create a new session (specifically created some time in the past so it's EXPIRED) should fail due to session
;; expiration
(let [session-id (random-session-id)]
- (tt/with-temp Session [_ {:id session-id
- :user_id (test.users/user->id :rasta)}]
+ (try
+ (db/insert! Session {:id session-id
+ :user_id (test.users/user->id :rasta)})
(db/update-where! Session {:id session-id}
:created_at (t/instant 0))
(is (= mw.util/response-unauthentic
- (auth-enforced-handler (request-with-session-id session-id)))))))
+ (auth-enforced-handler (request-with-session-id session-id))))
+ (finally (db/delete! Session :id session-id)))))
(testing "when a Session tied to an inactive User is sent with the request"
;; create a new session (specifically created some time in the past so it's EXPIRED)
;; should fail due to inactive user
;; NOTE that :trashbird is our INACTIVE test user
(let [session-id (random-session-id)]
- (tt/with-temp Session [_ {:id session-id
- :user_id (test.users/user->id :trashbird)}]
+ (try
+ (db/insert! Session {:id session-id
+ :user_id (test.users/user->id :trashbird)})
(is (= mw.util/response-unauthentic
(auth-enforced-handler
- (request-with-session-id session-id)))))))))
+ (request-with-session-id session-id))))
+ (finally (db/delete! Session :id session-id)))))))
;;; ------------------------------------------ TEST wrap-api-key middleware ------------------------------------------
diff --git a/test/metabase/server/middleware/session_test.clj b/test/metabase/server/middleware/session_test.clj
index 8b3e9334868..eb02f04201a 100644
--- a/test/metabase/server/middleware/session_test.clj
+++ b/test/metabase/server/middleware/session_test.clj
@@ -210,99 +210,98 @@
;; for some reason Toucan seems to be busted with models with non-integer IDs and `with-temp` doesn't seem to work
;; the way we'd expect :/
(try
- (mt/with-temp Session [session {:id (str test-uuid), :user_id (mt/user->id :lucky)}]
- (is (= {:metabase-user-id (mt/user->id :lucky), :is-superuser? false, :is-group-manager? false, :user-locale nil}
- (#'mw.session/current-user-info-for-session (str test-uuid) nil))))
+ (db/insert! Session {:id (str test-uuid), :user_id (mt/user->id :lucky)})
+ (is (= {:metabase-user-id (mt/user->id :lucky), :is-superuser? false, :is-group-manager? false, :user-locale nil}
+ (#'mw.session/current-user-info-for-session (str test-uuid) nil)))
(finally
(db/delete! Session :id (str test-uuid)))))
(testing "superusers should come back as `:is-superuser?`"
(try
- (mt/with-temp Session [session {:id (str test-uuid), :user_id (mt/user->id :crowberto)}]
- (is (= {:metabase-user-id (mt/user->id :crowberto), :is-superuser? true, :is-group-manager? false, :user-locale nil}
- (#'mw.session/current-user-info-for-session (str test-uuid) nil))))
+ (db/insert! Session {:id (str test-uuid), :user_id (mt/user->id :crowberto)})
+ (is (= {:metabase-user-id (mt/user->id :crowberto), :is-superuser? true, :is-group-manager? false, :user-locale nil}
+ (#'mw.session/current-user-info-for-session (str test-uuid) nil)))
(finally
(db/delete! Session :id (str test-uuid)))))
(testing "If user is a group manager of at least one group, `:is-group-manager?` "
(try
- (mt/with-user-in-groups
- [group-1 {:name "New Group 1"}
- group-2 {:name "New Group 2"}
- user [group-1 group-2]]
- (db/update-where! PermissionsGroupMembership {:user_id (:id user), :group_id (:id group-2)}
- :is_group_manager true)
- (mt/with-temp Session [_session {:id (str test-uuid)
- :user_id (:id user)}]
- (testing "is `false` if advanced-permisison is disabled"
- (premium-features-test/with-premium-features #{}
- (is (= false
- (:is-group-manager? (#'mw.session/current-user-info-for-session (str test-uuid) nil))))))
-
- (testing "is `true` if advanced-permisison is enabled"
- ;; a trick to run this test in OSS because even if advanced-permisison is enabled but EE ns is not evailable
- ;; `enable-advanced-permissions?` will still return false
- (with-redefs [premium-features/enable-advanced-permissions? (fn [& _args] true)]
- (is (= true
- (:is-group-manager? (#'mw.session/current-user-info-for-session (str test-uuid) nil))))))))
- (finally
- (db/delete! Session :id (str test-uuid)))))
+ (mt/with-user-in-groups [group-1 {:name "New Group 1"}
+ group-2 {:name "New Group 2"}
+ user [group-1 group-2]]
+ (db/update-where! PermissionsGroupMembership {:user_id (:id user), :group_id (:id group-2)}
+ :is_group_manager true)
+ (db/insert! Session {:id (str test-uuid)
+ :user_id (:id user)})
+ (testing "is `false` if advanced-permisison is disabled"
+ (premium-features-test/with-premium-features #{}
+ (is (= false
+ (:is-group-manager? (#'mw.session/current-user-info-for-session (str test-uuid) nil))))))
+
+ (testing "is `true` if advanced-permisison is enabled"
+ ;; a trick to run this test in OSS because even if advanced-permisison is enabled but EE ns is not evailable
+ ;; `enable-advanced-permissions?` will still return false
+ (with-redefs [premium-features/enable-advanced-permissions? (fn [& _args] true)]
+ (is (= true
+ (:is-group-manager? (#'mw.session/current-user-info-for-session (str test-uuid) nil)))))))
+ (finally
+ (db/delete! Session :id (str test-uuid)))))
(testing "full-app-embed sessions shouldn't come back if we don't explicitly specifiy the anti-csrf token"
(try
- (mt/with-temp Session [session {:id (str test-uuid)
- :user_id (mt/user->id :lucky)
- :anti_csrf_token test-anti-csrf-token}]
- (is (= nil
- (#'mw.session/current-user-info-for-session (str test-uuid) nil))))
+ (db/insert! Session {:id (str test-uuid)
+ :user_id (mt/user->id :lucky)
+ :anti_csrf_token test-anti-csrf-token})
+ (is (= nil
+ (#'mw.session/current-user-info-for-session (str test-uuid) nil)))
(finally
(db/delete! Session :id (str test-uuid))))
(testing "...but if we do specifiy the token, they should come back"
(try
- (mt/with-temp Session [session {:id (str test-uuid)
- :user_id (mt/user->id :lucky)
- :anti_csrf_token test-anti-csrf-token}]
- (is (= {:metabase-user-id (mt/user->id :lucky), :is-superuser? false, :is-group-manager? false, :user-locale nil}
- (#'mw.session/current-user-info-for-session (str test-uuid) test-anti-csrf-token))))
+ (db/insert! Session {:id (str test-uuid)
+ :user_id (mt/user->id :lucky)
+ :anti_csrf_token test-anti-csrf-token})
+ (is (= {:metabase-user-id (mt/user->id :lucky), :is-superuser? false, :is-group-manager? false, :user-locale nil}
+ (#'mw.session/current-user-info-for-session (str test-uuid) test-anti-csrf-token)))
(finally
(db/delete! Session :id (str test-uuid))))
(testing "(unless the token is wrong)"
(try
- (mt/with-temp Session [session {:id (str test-uuid)
- :user_id (mt/user->id :lucky)
- :anti_csrf_token test-anti-csrf-token}]
- (is (= nil
- (#'mw.session/current-user-info-for-session (str test-uuid) (str/join (reverse test-anti-csrf-token))))))
+ (db/insert! Session {:id (str test-uuid)
+ :user_id (mt/user->id :lucky)
+ :anti_csrf_token test-anti-csrf-token})
+ (is (= nil
+ (#'mw.session/current-user-info-for-session (str test-uuid) (str/join (reverse test-anti-csrf-token)))))
(finally
(db/delete! Session :id (str test-uuid)))))))
(testing "if we specify an anti-csrf token we shouldn't get back a session without that token"
(try
- (mt/with-temp Session [session {:id (str test-uuid)
- :user_id (mt/user->id :lucky)}]
- (is (= nil
- (#'mw.session/current-user-info-for-session (str test-uuid) test-anti-csrf-token))))
+ (db/insert! Session {:id (str test-uuid)
+ :user_id (mt/user->id :lucky)})
+ (is (= nil
+ (#'mw.session/current-user-info-for-session (str test-uuid) test-anti-csrf-token)))
(finally
(db/delete! Session :id (str test-uuid)))))
(testing "shouldn't fetch expired sessions"
(try
- (mt/with-temp Session [session {:id (str test-uuid)
- :user_id (mt/user->id :lucky)}]
- ;; use low-level `execute!` because updating is normally disallowed for Sessions
- (db/execute! {:update Session, :set {:created_at (java.sql.Date. 0)}, :where [:= :id (str test-uuid)]})
- (is (= nil
- (#'mw.session/current-user-info-for-session (str test-uuid) nil))))
+ (db/insert! Session {:id (str test-uuid)
+ :user_id (mt/user->id :lucky)})
+ ;; use low-level `execute!` because updating is normally disallowed for Sessions
+ (db/execute! {:update Session, :set {:created_at (java.sql.Date. 0)}, :where [:= :id (str test-uuid)]})
+ (is (= nil
+ (#'mw.session/current-user-info-for-session (str test-uuid) nil)))
(finally
(db/delete! Session :id (str test-uuid)))))
(testing "shouldn't fetch sessions for inactive users"
(try
- (mt/with-temp Session [session {:id (str test-uuid), :user_id (mt/user->id :trashbird)}]
- (is (= nil
- (#'mw.session/current-user-info-for-session (str test-uuid) nil))))
+ (db/insert! Session {:id (str test-uuid), :user_id (mt/user->id :trashbird)})
+ (is (= nil
+ (#'mw.session/current-user-info-for-session (str test-uuid) nil)))
(finally
(db/delete! Session :id (str test-uuid))))))
diff --git a/test/metabase/test/redefs.clj b/test/metabase/test/redefs.clj
index 0ffbfb63a30..ab7cf78b30e 100644
--- a/test/metabase/test/redefs.clj
+++ b/test/metabase/test/redefs.clj
@@ -21,18 +21,38 @@
(test-runner.parallel/assert-test-is-not-parallel "with-temp")
;; catch any Exceptions thrown when creating the object and rethrow them with some extra context to make them a
;; little easier to debug.
- (let [temp-object (try
- (db/insert! model (merge (tt/with-temp-defaults model)
- attributes))
- (catch Throwable e
- (throw (ex-info (str "with-temp error: " (ex-message e))
- {:model (name model), :attributes attributes}
- e))))
- primary-key (models/primary-key model)]
+ (let [attributes-with-defaults (merge (tt/with-temp-defaults model)
+ attributes)]
(try
- (f temp-object)
- (finally
- (db/delete! model primary-key (primary-key temp-object))))))
+ (let [temp-object (let [object (db/insert! model attributes-with-defaults)]
+ (assert (record? object)
+ (str (format "db/insert! for %s did not return a valid row. Got: %s."
+ (name model)
+ (pr-str object))
+ \newline
+ "(Tip: db/insert! doesn't seem to work with H2 with tables with compound PKs.)"))
+ object)
+ primary-key-column (models/primary-key model)
+ primary-key-value (get temp-object primary-key-column)]
+ (assert primary-key-value (format "No value for primary key %s for row %s"
+ (pr-str primary-key-column)
+ (pr-str temp-object)))
+ (try
+ (f temp-object)
+ (finally
+ (db/delete! model primary-key-column primary-key-value))))
+ (catch Throwable e
+ ;; only wrap `e` if it's not another `with-temp` error. This way we don't get "false positives" if something
+ ;; fails at the beginning of a big `with-temp*` form for all the other models
+ (throw (if (::with-temp-error? (ex-data e))
+ e
+ (ex-info (format "with-temp error for %s: %s" (name model) (ex-message e))
+ {:model (name model)
+ :attributes attributes
+ :attributes-with-defaults attributes-with-defaults
+ :primary-key-column (models/primary-key model)
+ ::with-temp-error? true}
+ e)))))))
(alter-var-root #'tt/do-with-temp (constantly do-with-temp))
diff --git a/test/metabase/test/util.clj b/test/metabase/test/util.clj
index d8c743ea6da..59e4c6f500c 100644
--- a/test/metabase/test/util.clj
+++ b/test/metabase/test/util.clj
@@ -697,7 +697,8 @@
(test-runner.parallel/assert-test-is-not-parallel "with-model-cleanup")
(initialize/initialize-if-needed! :db)
(let [model->old-max-id (into {} (for [model models]
- [model (:max-id (db/select-one [model [:%max.id :max-id]]))]))]
+ [model (:max-id (db/select-one [model [(keyword (str "%max." (name (models/primary-key model))))
+ :max-id]]))]))]
(try
(testing (str "\n" (pr-str (cons 'with-model-cleanup (map name models))) "\n")
(f))
@@ -706,7 +707,7 @@
;; might not have an old max ID if this is the first time the macro is used in this test run.
:let [old-max-id (or (get model->old-max-id model)
0)
- max-id-condition [:> :id old-max-id]
+ max-id-condition [:> (models/primary-key model) old-max-id]
additional-conditions (with-model-cleanup-additional-conditions model)]]
(db/execute!
{:delete-from model
@@ -723,7 +724,9 @@
(with-model-cleanup [Card]
(create-card-via-api!)
- (is (= ...)))"
+ (is (= ...)))
+
+ Only works for models that have a numeric primary key e.g. `:id`."
[models & body]
`(do-with-model-cleanup ~models (fn [] ~@body)))
--
GitLab