From 32dbd9eb21796625fe87faf4dd710c5cd17e1af5 Mon Sep 17 00:00:00 2001
From: Cam Saul <cammsaul@gmail.com>
Date: Mon, 29 Apr 2019 20:40:06 -0700
Subject: [PATCH] CSV/JSON/XLSX downloads should not be subject to default
constraints :1234:
---
src/metabase/api/card.clj | 12 +--
src/metabase/api/common.clj | 11 ++-
src/metabase/api/dataset.clj | 2 +
src/metabase/api/pulse.clj | 11 ++-
src/metabase/api/tiles.clj | 34 ++++++--
src/metabase/pulse.clj | 32 ++++----
.../middleware/constraints.clj | 27 ++++---
src/metabase/server.clj | 21 ++---
test/metabase/api/card_test.clj | 39 ++++++++-
test/metabase/api/dataset_test.clj | 50 +++++++++---
test/metabase/api/pulse_test.clj | 17 +++-
test/metabase/api/tiles_test.clj | 46 ++++++++---
test/metabase/pulse_test.clj | 79 +++++++++++--------
.../middleware/constraints_test.clj | 58 ++++++++++++++
test/metabase/task/sync_databases_test.clj | 20 +++--
15 files changed, 334 insertions(+), 125 deletions(-)
create mode 100644 test/metabase/query_processor/middleware/constraints_test.clj
diff --git a/src/metabase/api/card.clj b/src/metabase/api/card.clj
index c4c7b671620..0bc8a0c602f 100644
--- a/src/metabase/api/card.clj
+++ b/src/metabase/api/card.clj
@@ -595,11 +595,13 @@
(u/emoji "💾"))
ttl-seconds))))
-(defn- query-for-card [card parameters constraints middleware]
- (let [query (assoc (:dataset_query card)
- :constraints constraints
- :parameters parameters
- :middleware middleware)
+(defn- query-for-card [{query :dataset_query, :as card} parameters constraints middleware]
+ (let [query (-> query
+ ;; don't want default constraints overridding anything that's already there
+ (m/dissoc-in [:middleware :add-default-userland-constraints?])
+ (assoc :constraints constraints
+ :parameters parameters
+ :middleware middleware))
ttl (when (public-settings/enable-query-caching)
(or (:cache_ttl card)
(query-magic-ttl query)))]
diff --git a/src/metabase/api/common.clj b/src/metabase/api/common.clj
index 24d5a4666f0..e75beddc48a 100644
--- a/src/metabase/api/common.clj
+++ b/src/metabase/api/common.clj
@@ -260,13 +260,16 @@
- calls `auto-parse` to automatically parse certain args. e.g. `id` is converted from `String` to `Integer` via
`Integer/parseInt`
+
- converts ROUTE from a simple form like `\"/:id\"` to a typed one like `[\"/:id\" :id #\"[0-9]+\"]`
+
- sequentially applies specified annotation functions on args to validate them.
- - executes BODY inside a `try-catch` block that handles exceptions; if exception is an instance of `ExceptionInfo`
- and includes a `:status-code`, that code will be returned
+
- automatically calls `wrap-response-if-needed` on the result of BODY
- - tags function's metadata in a way that subsequent calls to `define-routes` (see below)
- will automatically include the function in the generated `defroutes` form.
+
+ - tags function's metadata in a way that subsequent calls to `define-routes` (see below) will automatically include
+ the function in the generated `defroutes` form.
+
- Generates a super-sophisticated Markdown-formatted docstring"
{:arglists '([method route docstr? args schemas-map? & body])}
[method route & more]
diff --git a/src/metabase/api/dataset.clj b/src/metabase/api/dataset.clj
index b376f000b54..85e5507eee6 100644
--- a/src/metabase/api/dataset.clj
+++ b/src/metabase/api/dataset.clj
@@ -5,6 +5,7 @@
[clojure.string :as str]
[clojure.tools.logging :as log]
[compojure.core :refer [POST]]
+ [medley.core :as m]
[metabase.api.common :as api]
[metabase.models
[card :refer [Card]]
@@ -147,6 +148,7 @@
(qp.async/process-query-and-save-execution!
(-> query
(dissoc :constraints)
+ (m/dissoc-in [:middleware :add-default-userland-constraints?])
(assoc-in [:middleware :skip-results-metadata?] true))
{:executed-by api/*current-user-id*, :context (export-format->context export-format)}))))
diff --git a/src/metabase/api/pulse.clj b/src/metabase/api/pulse.clj
index 7941a535449..9e706fcef96 100644
--- a/src/metabase/api/pulse.clj
+++ b/src/metabase/api/pulse.clj
@@ -134,10 +134,13 @@
(catch Throwable e
(assoc-in chan-types [:slack :error] (.getMessage e)))))}))
-(defn- pulse-card-query-results [card]
- (qp/process-query-and-save-execution! (:dataset_query card) {:executed-by api/*current-user-id*
- :context :pulse
- :card-id (u/get-id card)}))
+(defn- pulse-card-query-results
+ {:arglists '([card])}
+ [{query :dataset_query, card-id :id}]
+ (qp/process-query-and-save-execution! (assoc query :async? false)
+ {:executed-by api/*current-user-id*
+ :context :pulse
+ :card-id card-id}))
(api/defendpoint GET "/preview_card/:id"
"Get HTML rendering of a Card with `id`."
diff --git a/src/metabase/api/tiles.clj b/src/metabase/api/tiles.clj
index a2c9be71ef5..4b0d1c8e7b6 100644
--- a/src/metabase/api/tiles.clj
+++ b/src/metabase/api/tiles.clj
@@ -14,7 +14,7 @@
[schema :as su]])
(:import java.awt.Color
java.awt.image.BufferedImage
- java.io.ByteArrayOutputStream
+ [java.io ByteArrayInputStream ByteArrayOutputStream]
javax.imageio.ImageIO))
;;; --------------------------------------------------- CONSTANTS ----------------------------------------------------
@@ -116,6 +116,7 @@
;;; ---------------------------------------------------- ENDPOINT ----------------------------------------------------
+;; TODO - this can be reworked to be `defendpoint-async` instead
(api/defendpoint GET "/:zoom/:x/:y/:lat-field-id/:lon-field-id/:lat-col-idx/:lon-col-idx/"
"This endpoints provides an image with the appropriate pins rendered given a MBQL QUERY (passed as a GET query
string param). We evaluate the query and find the set of lat/lon pairs which are relevant and then render the
@@ -135,15 +136,34 @@
y (Integer/parseInt y)
lat-col-idx (Integer/parseInt lat-col-idx)
lon-col-idx (Integer/parseInt lon-col-idx)
- query (normalize/normalize (json/parse-string query keyword))
- updated-query (update query :query (u/rpartial query-with-inside-filter lat-field-id lon-field-id x y zoom))
- result (qp/process-query-and-save-execution! updated-query {:executed-by api/*current-user-id*, :context :map-tiles})
- points (for [row (-> result :data :rows)]
- [(nth row lat-col-idx) (nth row lon-col-idx)])]
+
+ query
+ (normalize/normalize (json/parse-string query keyword))
+
+ updated-query
+ (-> query
+ (update :query query-with-inside-filter lat-field-id lon-field-id x y zoom)
+ (assoc :async? false))
+
+ {:keys [status], {:keys [rows]} :data, :as result}
+ (qp/process-query-and-save-execution! updated-query
+ {:executed-by api/*current-user-id*
+ :context :map-tiles})
+
+ ;; make sure query completed successfully, or API endpoint should return 400
+ _
+ (when-not (= status :completed)
+ (throw (ex-info (str (tru "Query failed"))
+ ;; `result` might be a `core.async` channel or something we're not expecting
+ (assoc (when (map? result) result) :status-code 400))))
+
+ points
+ (for [row rows]
+ [(nth row lat-col-idx) (nth row lon-col-idx)])]
;; manual ring response here. we simply create an inputstream from the byte[] of our image
{:status 200
:headers {"Content-Type" "image/png"}
- :body (java.io.ByteArrayInputStream. (tile->byte-array (create-tile zoom points)))}))
+ :body (ByteArrayInputStream. (tile->byte-array (create-tile zoom points)))}))
(api/define-routes)
diff --git a/src/metabase/pulse.clj b/src/metabase/pulse.clj
index e6206bd2426..e20cb6c77a4 100644
--- a/src/metabase/pulse.clj
+++ b/src/metabase/pulse.clj
@@ -24,22 +24,24 @@
;;; ------------------------------------------------- PULSE SENDING --------------------------------------------------
-;; TODO: this is probably something that could live somewhere else and just be reused
+;; TODO - this is probably something that could live somewhere else and just be reused
+;; TODO - this should be done async
(defn execute-card
- "Execute the query for a single card with CARD-ID. OPTIONS are passed along to `dataset-query`."
- [card-id & {:as options}]
- {:pre [(integer? card-id)]}
- (try
- (when-let [card (Card :id card-id, :archived false)]
- (let [{:keys [creator_id dataset_query]} card]
- {:card card
- :result (qp/process-query-and-save-with-max-results-constraints! dataset_query
- (merge {:executed-by creator_id
- :context :pulse
- :card-id card-id}
- options))}))
- (catch Throwable t
- (log/warn t (trs "Error running query for Card {0}" card-id)))))
+ "Execute the query for a single Card. `options` are passed along to the Query Processor."
+ [card-or-id & {:as options}]
+ (let [card-id (u/get-id card-or-id)]
+ (try
+ (when-let [card (Card :id card-id, :archived false)]
+ (let [{:keys [creator_id dataset_query]} card
+ query (assoc dataset_query :async? false)]
+ {:card card
+ :result (qp/process-query-and-save-with-max-results-constraints! query
+ (merge {:executed-by creator_id
+ :context :pulse
+ :card-id card-id}
+ options))}))
+ (catch Throwable e
+ (log/warn e (trs "Error running query for Card {0}" card-id))))))
(defn- database-id [card]
(or (:database_id card)
diff --git a/src/metabase/query_processor/middleware/constraints.clj b/src/metabase/query_processor/middleware/constraints.clj
index 3aaf520c908..253fce9d5c9 100644
--- a/src/metabase/query_processor/middleware/constraints.clj
+++ b/src/metabase/query_processor/middleware/constraints.clj
@@ -1,4 +1,6 @@
-(ns metabase.query-processor.middleware.constraints)
+(ns metabase.query-processor.middleware.constraints
+ "Middleware that adds default constraints to limit the maximum number of rows returned to queries that specify the
+ `:add-default-userland-constraints?` `:middleware` option.")
(def ^:private max-results-bare-rows
"Maximum number of rows to return specifically on :rows type queries via the API."
@@ -13,21 +15,24 @@
{:max-results max-results
:max-results-bare-rows max-results-bare-rows})
-(defn- merge-default-constraints [{:keys [max-results], :as constraints}]
- (merge
- default-query-constraints
- ;; `:max-results-bare-rows` must be less than or equal to `:max-results`, so if someone sets `:max-results` but not
- ;; `:max-results-bare-rows` use the same value for both. Otherwise the default bare rows value could end up being
- ;; higher than the custom `:max-rows` value, causing an error
- (when max-results
- {:max-results-bare-rows max-results})
- constraints))
+(defn- ensure-valid-constraints
+ "`:max-results-bare-rows` must be less than or equal to `:max-results`, so if someone sets `:max-results` but not
+ `:max-results-bare-rows` or sets an both but sets an invalid value for ``:max-results-bare-rows` use the same value
+ for both. Otherwise the default bare rows value could end up being higher than the custom `:max-rows` value, causing
+ an error."
+ [{:keys [max-results max-results-bare-rows], :as constraints}]
+ (if (<= max-results-bare-rows max-results)
+ constraints
+ (assoc constraints :max-results-bare-rows max-results)))
+
+(defn- merge-default-constraints [constraints]
+ (merge default-query-constraints constraints))
(defn- add-default-userland-constraints*
"Add default values of `:max-results` and `:max-results-bare-rows` to `:constraints` map `m`."
[{{:keys [add-default-userland-constraints?]} :middleware, :as query}]
(cond-> query
- add-default-userland-constraints? (update :constraints merge-default-constraints)))
+ add-default-userland-constraints? (update :constraints (comp ensure-valid-constraints merge-default-constraints))))
(defn add-default-userland-constraints
"Middleware that optionally adds default `max-results` and `max-results-bare-rows` constraints to queries, meant for
diff --git a/src/metabase/server.clj b/src/metabase/server.clj
index 71c19a5442f..81b1bba0dd0 100644
--- a/src/metabase/server.clj
+++ b/src/metabase/server.clj
@@ -53,16 +53,17 @@
creating one-off web servers for tests and REPL usage."
^Server [handler options]
(doto ^Server (#'ring-jetty/create-server (assoc options :async? true))
- (.setHandler (#'ring-jetty/async-proxy-handler
- handler
- ;; if any API endpoint functions aren't at the very least returning a channel to fetch the results
- ;; later after 10 minutes we're in serious trouble. (Almost everything 'slow' should be returning a
- ;; channel before then, but some things like CSV downloads don't currently return channels at this
- ;; time)
- ;;
- ;; TODO - I suppose the default value should be moved to the `metabase.config` namespace?
- (or (config/config-int :mb-jetty-async-response-timeout)
- (* 10 60 1000))))))
+ (.setHandler
+ (#'ring-jetty/async-proxy-handler
+ handler
+ ;; if any API endpoint functions aren't at the very least returning a channel to fetch the results
+ ;; later after 10 minutes we're in serious trouble. (Almost everything 'slow' should be returning a
+ ;; channel before then, but some things like CSV downloads don't currently return channels at this
+ ;; time)
+ ;;
+ ;; TODO - I suppose the default value should be moved to the `metabase.config` namespace?
+ (or (config/config-int :mb-jetty-async-response-timeout)
+ (* 10 60 1000))))))
(defn start-web-server!
"Start the embedded Jetty web server. Returns `:started` if a new server was started; `nil` if there was already a
diff --git a/test/metabase/api/card_test.clj b/test/metabase/api/card_test.clj
index bfc0b23ca09..78a7fe8ac47 100644
--- a/test/metabase/api/card_test.clj
+++ b/test/metabase/api/card_test.clj
@@ -1,6 +1,7 @@
(ns metabase.api.card-test
"Tests for /api/card endpoints."
(:require [cheshire.core :as json]
+ [clojure.data.csv :as csv]
[dk.ative.docjure.spreadsheet :as spreadsheet]
[expectations :refer :all]
[medley.core :as m]
@@ -25,11 +26,13 @@
[table :refer [Table]]
[view-log :refer [ViewLog]]]
[metabase.query-processor.async :as qp.async]
- [metabase.query-processor.middleware.results-metadata :as results-metadata]
+ [metabase.query-processor.middleware
+ [constraints :as constraints]
+ [results-metadata :as results-metadata]]
[metabase.test
[data :as data]
[util :as tu :refer [match-$ random-name]]]
- [metabase.test.data.users :refer :all]
+ [metabase.test.data.users :as test-users :refer :all]
[metabase.util.date :as du]
[toucan.db :as db]
[toucan.util.test :as tt])
@@ -1246,6 +1249,38 @@
(spreadsheet/select-sheet "Query result")
(spreadsheet/select-columns {:A :col})))))
+;; Downloading CSV/JSON/XLSX results shouldn't be subject to the default query constraints -- even if the query comes
+;; in with `add-default-userland-constraints` (as will be the case if the query gets saved from one that had it -- see
+;; #9831)
+(expect
+ 101
+ (with-redefs [constraints/default-query-constraints {:max-results 10, :max-results-bare-rows 10}]
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)}
+ :middleware
+ {:add-default-userland-constraints? true
+ :userland-query? true}}}]
+ (with-cards-in-readable-collection card
+ (let [results ((user->client :rasta) :post 200 (format "card/%d/query/csv" (u/get-id card)))]
+ (count (csv/read-csv results)))))))
+
+;; non-"download" queries should still get the default constraints
+;; (this also is a sanitiy check to make sure the `with-redefs` in the test above actually works)
+(expect
+ 10
+ (with-redefs [constraints/default-query-constraints {:max-results 10, :max-results-bare-rows 10}]
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)}
+ :middleware
+ {:add-default-userland-constraints? true
+ :userland-query? true}}}]
+ (with-cards-in-readable-collection card
+ (let [{row-count :row_count, :as result}
+ ((user->client :rasta) :post 200 (format "card/%d/query" (u/get-id card)))]
+ (or row-count result))))))
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | COLLECTIONS |
diff --git a/test/metabase/api/dataset_test.clj b/test/metabase/api/dataset_test.clj
index dc426351eed..c25a1717e3d 100644
--- a/test/metabase/api/dataset_test.clj
+++ b/test/metabase/api/dataset_test.clj
@@ -24,18 +24,20 @@
(:import com.fasterxml.jackson.core.JsonGenerator))
(defn- format-response [m]
- (into {} (for [[k v] (-> m
- (m/dissoc-in [:data :results_metadata])
- (m/dissoc-in [:data :insights]))]
- (cond
- (contains? #{:id :started_at :running_time :hash} k)
- [k (boolean v)]
+ (into
+ {}
+ (for [[k v] (-> m
+ (m/dissoc-in [:data :results_metadata])
+ (m/dissoc-in [:data :insights]))]
+ (cond
+ (contains? #{:id :started_at :running_time :hash} k)
+ [k (boolean v)]
- (and (= :data k) (contains? v :native_form))
- [k (update v :native_form boolean)]
+ (and (= :data k) (contains? v :native_form))
+ [k (update v :native_form boolean)]
- :else
- [k v]))))
+ :else
+ [k v]))))
(defn- most-recent-query-execution [] (db/select-one QueryExecution {:order-by [[:id :desc]]}))
@@ -233,3 +235,31 @@
:type :query
:query {:source-table (str "card__" (u/get-id card))}}))]
(count (csv/read-csv result)))))
+
+;; POST /api/dataset/:format Downloading CSV/JSON/XLSX results shouldn't be subject to the default query constraints
+;; -- even if the query comes in with `add-default-userland-constraints` (as will be the case if the query gets saved
+;; from one that had it -- see #9831)
+(expect
+ 101
+ (with-redefs [constraints/default-query-constraints {:max-results 10, :max-results-bare-rows 10}]
+ (let [result ((test-users/user->client :rasta) :post 200 "dataset/csv"
+ :query (json/generate-string
+ {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)}
+ :middleware
+ {:add-default-userland-constraints? true
+ :userland-query? true}}))]
+ (count (csv/read-csv result)))))
+
+;; non-"download" queries should still get the default constraints
+;; (this also is a sanitiy check to make sure the `with-redefs` in the test above actually works)
+(expect
+ 10
+ (with-redefs [constraints/default-query-constraints {:max-results 10, :max-results-bare-rows 10}]
+ (let [{row-count :row_count, :as result}
+ ((test-users/user->client :rasta) :post 200 "dataset"
+ {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)}})]
+ (or row-count result))))
diff --git a/test/metabase/api/pulse_test.clj b/test/metabase/api/pulse_test.clj
index 95ef7b7bcfc..1c575c4ddcf 100644
--- a/test/metabase/api/pulse_test.clj
+++ b/test/metabase/api/pulse_test.clj
@@ -1,11 +1,13 @@
(ns metabase.api.pulse-test
"Tests for /api/pulse endpoints."
- (:require [expectations :refer :all]
+ (:require [expectations :refer [expect]]
[metabase
[email-test :as et]
[http-client :as http]
[util :as u]]
- [metabase.api.card-test :as card-api-test]
+ [metabase.api
+ [card-test :as card-api-test]
+ [pulse :as pulse-api]]
[metabase.integrations.slack :as slack]
[metabase.middleware.util :as middleware.u]
[metabase.models
@@ -913,6 +915,17 @@
{:response ((user->client :rasta) :post 200 "pulse/test" (assoc result :channels [email-channel]))
:emails (et/regex-email-bodies #"A Pulse")}))))))
+;; A Card saved with `:async?` true should not be ran async for a Pulse
+(expect
+ map?
+ (#'pulse-api/pulse-card-query-results
+ {:id 1
+ :dataset_query {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)
+ :limit 1}
+ :async? true}}))
+
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | GET /api/pulse/form_input |
diff --git a/test/metabase/api/tiles_test.clj b/test/metabase/api/tiles_test.clj
index f8f17775f11..0bce63247a0 100644
--- a/test/metabase/api/tiles_test.clj
+++ b/test/metabase/api/tiles_test.clj
@@ -1,16 +1,42 @@
(ns metabase.api.tiles-test
"Tests for `/api/tiles` endpoints."
(:require [cheshire.core :as json]
- [expectations :refer :all]
- [metabase.test.data :as data]
- [metabase.test.data.users :refer :all]))
+ [expectations :refer [expect]]
+ [metabase.test
+ [data :as data]
+ [util :as tu]]
+ [metabase.test.data.users :as test-users]
+ [schema.core :as s]))
;;; GET /api/tiles/:zoom/:x/:y/:lat-field-id/:lon-field-id/:lat-col-idx/:lon-col-idx/
(expect
- String
- ((user->client :rasta) :get 200 (format "tiles/1/1/1/%d/%d/1/1/"
- (data/id :venues :latitude)
- (data/id :venues :longitude))
- :query (json/generate-string {:database (data/id)
- :type :query
- :query {:source-table (data/id :venues)}})))
+ #".*PNG.*"
+ ((test-users/user->client :rasta) :get 200 (format "tiles/1/1/1/%d/%d/1/1/"
+ (data/id :venues :latitude)
+ (data/id :venues :longitude))
+ :query (json/generate-string
+ {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)}})))
+
+;; if the query fails, don't attempt to generate a map without any points -- the endpoint should return a 400
+(tu/expect-schema
+ {:status (s/eq "failed")
+ s/Keyword s/Any}
+ ((test-users/user->client :rasta) :get 400 (format "tiles/1/1/1/%d/%d/1/1/"
+ (data/id :venues :latitude)
+ (data/id :venues :longitude))
+ :query "{}"))
+
+;; even if the original query was saved as `:async?` we shouldn't run the query as async (and get a core.async channel
+;; and fail)
+(expect
+ #".*PNG.*"
+ ((test-users/user->client :rasta) :get 200 (format "tiles/1/1/1/%d/%d/1/1/"
+ (data/id :venues :latitude)
+ (data/id :venues :longitude))
+ :query (json/generate-string
+ {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)}
+ :async? true})))
diff --git a/test/metabase/pulse_test.clj b/test/metabase/pulse_test.clj
index 965279850e3..ecd6fbddfae 100644
--- a/test/metabase/pulse_test.clj
+++ b/test/metabase/pulse_test.clj
@@ -6,11 +6,11 @@
[medley.core :as m]
[metabase
[email-test :as et]
- [pulse :refer :all]]
+ [pulse :as pulse]]
[metabase.integrations.slack :as slack]
[metabase.models
[card :refer [Card]]
- [pulse :refer [Pulse retrieve-notification retrieve-pulse]]
+ [pulse :as pulse.model :refer [Pulse]]
[pulse-card :refer [PulseCard]]
[pulse-channel :refer [PulseChannel]]
[pulse-channel-recipient :refer [PulseChannelRecipient]]]
@@ -22,6 +22,7 @@
[metabase.test.data
[dataset-definitions :as defs]
[users :as users]]
+ [schema.core :as s]
[toucan.db :as db]
[toucan.util.test :as tt]))
@@ -101,7 +102,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name"))))
;; Basic test, 1 card, 1 recipient, 21 results results in a CSV being attached and a table being sent
@@ -121,7 +122,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name" #"More results have been included" #"ID</th>"))))
;; Validate pulse queries are limited by `default-query-constraints`
@@ -139,7 +140,7 @@
(email-test-setup
(with-redefs [constraints/default-query-constraints {:max-results 10000
:max-results-bare-rows 30}]
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
;; Slurp in the generated CSV and count the lines found in the file
(-> @et/inbox
vals
@@ -167,7 +168,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name" #"More results have been included" #"ID</th>"))))
;; Pulse should be sent to two recipients
@@ -190,7 +191,7 @@
PulseChannelRecipient [_ {:user_id (users/user->id :crowberto)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name"))))
;; 1 pulse that has 2 cards, should contain two attachments
@@ -212,7 +213,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name"))))
;; Pulse where the card has no results, but skip_if_empty is false, so should still send
@@ -229,7 +230,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name"))))
;; Pulse where the card has no results, skip_if_empty is true, so no pulse should be sent
@@ -246,7 +247,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
@et/inbox)))
;; Rows alert with no data
@@ -263,7 +264,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
@et/inbox)))
(defn- rasta-alert-email
@@ -288,7 +289,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
(et/summarize-multipart-email test-card-regex #"More results have been included"))))
;; Rows alert with too much data will attach as CSV and include a table
@@ -309,7 +310,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
(et/summarize-multipart-email test-card-regex #"More results have been included" #"ID</th>"))))
;; Above goal alert with data
@@ -330,7 +331,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
(et/summarize-multipart-email test-card-regex))))
;; Native query with user-specified x and y axis
@@ -358,7 +359,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
(et/summarize-multipart-email test-card-regex))))
;; Above goal alert, with no data above goal
@@ -378,7 +379,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
@et/inbox)))
;; Below goal alert with no satisfying data
@@ -398,7 +399,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
@et/inbox)))
;; Below goal alert with data
@@ -420,7 +421,7 @@
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
(et/summarize-multipart-email test-card-regex))))
(defn- thunk->boolean [{:keys [attachments] :as result}]
@@ -483,7 +484,7 @@
:channel-id "FOO",
:fallback card-name}]}
(slack-test-setup
- (-> (send-pulse! (retrieve-pulse pulse-id))
+ (-> (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
first
thunk->boolean)))
@@ -521,7 +522,7 @@
]
(slack-test-setup
(with-redefs [render/attached-results-text (wrap-function (var-get #'render/attached-results-text))]
- (let [[pulse-results] (send-pulse! (retrieve-pulse pulse-id))]
+ (let [[pulse-results] (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))]
;; If we don't force the thunk, the rendering code will never execute and attached-results-text won't be called
(force-bytes-thunk pulse-results)
[(thunk->boolean pulse-results)
@@ -564,7 +565,7 @@
:fallback "Test card 2"}]}
true]
(slack-test-setup
- (let [[slack-data] (send-pulse! (retrieve-pulse pulse-id))]
+ (let [[slack-data] (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))]
[(thunk->boolean slack-data)
(every? produces-bytes? (:attachments slack-data))])))
@@ -607,7 +608,7 @@
true
true]
(slack-test-setup
- (let [pulse-data (send-pulse! (retrieve-pulse pulse-id))
+ (let [pulse-data (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
slack-data (m/find-first #(contains? % :channel-id) pulse-data)
email-data (m/find-first #(contains? % :subject) pulse-data)]
[(thunk->boolean slack-data)
@@ -635,7 +636,7 @@
:fallback card-name}]}
true]
(slack-test-setup
- (let [[result] (send-pulse! (retrieve-notification pulse-id))]
+ (let [[result] (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))]
[(thunk->boolean result)
(every? produces-bytes? (:attachments result))])))
@@ -663,7 +664,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
(et/summarize-multipart-email test-card-regex))))
;; Below goal alert with progress bar
@@ -683,7 +684,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
(et/summarize-multipart-email test-card-regex))))
;; Rows alert, first run only with data
@@ -701,7 +702,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
(et/summarize-multipart-email test-card-regex
#"stop sending you alerts"))))
@@ -719,7 +720,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
[@et/inbox
(db/exists? Pulse :id pulse-id)])))
@@ -743,7 +744,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name"))))
;; Basic alert test, 1 card, 1 recipient, with CSV attachment
@@ -761,7 +762,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
(et/summarize-multipart-email test-card-regex))))
;; With a "rows" type of pulse (table visualization) we should include the CSV by default
@@ -784,7 +785,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name"))))
;; If the pulse is already configured to send an XLS, no need to include a CSV
@@ -808,7 +809,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name"))))
;; Basic test of card with CSV and XLS attachments, but no data. Should not include an attachment
@@ -828,7 +829,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name"))))
;; Basic test, 1 card, 1 recipient, with XLS attachment
@@ -846,7 +847,7 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-pulse pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-pulse pulse-id))
(et/summarize-multipart-email #"Pulse Name"))))
;; Rows alert with data and a CSV + XLS attachment
@@ -865,5 +866,15 @@
PulseChannelRecipient [_ {:user_id (rasta-id)
:pulse_channel_id pc-id}]]
(email-test-setup
- (send-pulse! (retrieve-notification pulse-id))
+ (pulse/send-pulse! (pulse.model/retrieve-notification pulse-id))
(et/summarize-multipart-email test-card-regex))))
+
+;; even if Card is saved as `:async?` we shouldn't run the query async
+(tu/expect-schema
+ {:card (s/pred map?)
+ :result (s/pred map?)}
+ (tt/with-temp Card [card {:dataset_query {:database (data/id)
+ :type :query
+ :query {:source-table (data/id :venues)}
+ :async? true}}]
+ (pulse/execute-card card)))
diff --git a/test/metabase/query_processor/middleware/constraints_test.clj b/test/metabase/query_processor/middleware/constraints_test.clj
new file mode 100644
index 00000000000..5d463368857
--- /dev/null
+++ b/test/metabase/query_processor/middleware/constraints_test.clj
@@ -0,0 +1,58 @@
+(ns metabase.query-processor.middleware.constraints-test
+ (:require [expectations :refer [expect]]
+ [metabase.query-processor.middleware.constraints :as constraints]))
+
+(defn- add-default-userland-constraints [query]
+ ((constraints/add-default-userland-constraints
+ (fn [query respond _ _]
+ (respond query)))
+ query
+ identity
+ identity
+ nil))
+
+;; don't do anything to queries without [:middleware :add-default-userland-constraints?] set
+(expect
+ {}
+ (add-default-userland-constraints {}))
+
+;; if it is *truthy* add the constraints
+(expect
+ {:middleware {:add-default-userland-constraints? true},
+ :constraints {:max-results @#'constraints/max-results
+ :max-results-bare-rows @#'constraints/max-results-bare-rows}}
+ (add-default-userland-constraints
+ {:middleware {:add-default-userland-constraints? true}}))
+
+;; don't do anything if it's not truthy
+(expect
+ {:middleware {:add-default-userland-constraints? false}}
+ (add-default-userland-constraints
+ {:middleware {:add-default-userland-constraints? false}}))
+
+;; if it already has constraints, don't overwrite those!
+(expect
+ {:middleware {:add-default-userland-constraints? true}
+ :constraints {:max-results @#'constraints/max-results
+ :max-results-bare-rows 1}}
+ (add-default-userland-constraints
+ {:constraints {:max-results-bare-rows 1}
+ :middleware {:add-default-userland-constraints? true}}))
+
+;; if you specify just `:max-results` it should make sure `:max-results-bare-rows` is <= `:max-results`
+(expect
+ {:middleware {:add-default-userland-constraints? true}
+ :constraints {:max-results 5
+ :max-results-bare-rows 5}}
+ (add-default-userland-constraints
+ {:constraints {:max-results 5}
+ :middleware {:add-default-userland-constraints? true}}))
+
+;; if you specify both it should still make sure `:max-results-bare-rows` is <= `:max-results`
+(expect
+ {:middleware {:add-default-userland-constraints? true}
+ :constraints {:max-results 5
+ :max-results-bare-rows 5}}
+ (add-default-userland-constraints
+ {:constraints {:max-results 5, :max-results-bare-rows 10}
+ :middleware {:add-default-userland-constraints? true}}))
diff --git a/test/metabase/task/sync_databases_test.clj b/test/metabase/task/sync_databases_test.clj
index b5ae6a245ff..46cce1f9384 100644
--- a/test/metabase/task/sync_databases_test.clj
+++ b/test/metabase/task/sync_databases_test.clj
@@ -151,19 +151,17 @@
;;; +----------------------------------------------------------------------------------------------------------------+
(defn- check-if-sync-processes-ran-for-db {:style/indent 0} [db-info]
- (let [sync-db-metadata-counter (atom 0)
- analyze-db-counter (atom 0)
- update-field-values-counter (atom 0)]
- (with-redefs [metabase.sync.sync-metadata/sync-db-metadata! (fn [& _] (swap! sync-db-metadata-counter inc))
- metabase.sync.analyze/analyze-db! (fn [& _] (swap! analyze-db-counter inc))
- metabase.sync.field-values/update-field-values! (fn [& _] (swap! update-field-values-counter inc))]
+ (let [sync-db-metadata-ran? (promise)
+ analyze-db-ran? (promise)
+ update-field-values-ran? (promise)]
+ (with-redefs [metabase.sync.sync-metadata/sync-db-metadata! (fn [& _] (deliver sync-db-metadata-ran? true))
+ metabase.sync.analyze/analyze-db! (fn [& _] (deliver analyze-db-ran? true))
+ metabase.sync.field-values/update-field-values! (fn [& _] (deliver update-field-values-ran? true))]
(with-scheduler-setup
(tt/with-temp Database [database db-info]
- ;; give tasks some time to run
- (Thread/sleep 2000)
- {:ran-sync? (not (zero? @sync-db-metadata-counter))
- :ran-analyze? (not (zero? @analyze-db-counter))
- :ran-update-field-values? (not (zero? @update-field-values-counter))})))))
+ {:ran-sync? (deref sync-db-metadata-ran? 500 false)
+ :ran-analyze? (deref analyze-db-ran? 100 false)
+ :ran-update-field-values? (deref update-field-values-ran? 500 false)})))))
(defn- cron-schedule-for-next-year []
(format "0 15 10 * * ? %d" (inc (du/date-extract :year))))
--
GitLab