From 972435fbcee711888a406dfe1b7756ea9ebb839d Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Thu, 20 Feb 2020 17:57:02 -0800
Subject: [PATCH] Streaming API response tweaks (#11949)
* Fix streaming downloads -- send correct headers
* Don't include file download headers for normal QP API responses
* GZIP compress streaming responses
* Startup perf improvement
---
src/metabase/api/common.clj | 6 +-
src/metabase/api/common/internal.clj | 47 +++--
src/metabase/api/session.clj | 3 +-
src/metabase/api/setting.clj | 6 +-
src/metabase/async/streaming_response.clj | 67 ++++---
src/metabase/core.clj | 6 +-
src/metabase/driver/util.clj | 27 ---
src/metabase/middleware/misc.clj | 33 ++--
.../query_processor/streaming/json.clj | 2 +-
test/metabase/api/dashboard_test.clj | 10 +-
test/metabase/api/database_test.clj | 7 +-
test/metabase/api/dataset_test.clj | 26 ++-
test/metabase/api/geojson_test.clj | 41 ++--
test/metabase/api/session_test.clj | 118 ++++++------
test/metabase/api/setting_test.clj | 177 ++++++++----------
test/metabase/email_test.clj | 2 +-
test/metabase/pulse_test.clj | 6 +-
test/metabase/test/integrations/ldap.clj | 4 +-
18 files changed, 308 insertions(+), 280 deletions(-)
diff --git a/src/metabase/api/common.clj b/src/metabase/api/common.clj
index 5f8b84a540b..020c03234f5 100644
--- a/src/metabase/api/common.clj
+++ b/src/metabase/api/common.clj
@@ -215,7 +215,7 @@
[bindings & body]
`(do-api-let ~generic-500 ~bindings ~@body))
-(def ^:const generic-204-no-content
+(def generic-204-no-content
"A 'No Content' response for `DELETE` endpoints to return."
{:status 204, :body nil})
@@ -231,11 +231,11 @@
- 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]+\"]`
+ - 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.
- - automatically calls `wrap-response-if-needed` on the result of BODY
+ - 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.
diff --git a/src/metabase/api/common/internal.clj b/src/metabase/api/common/internal.clj
index 50a621b0874..c2ec790cbe1 100644
--- a/src/metabase/api/common/internal.clj
+++ b/src/metabase/api/common/internal.clj
@@ -3,7 +3,6 @@
These are primarily used as the internal implementation of `defendpoint`."
(:require [clojure.string :as str]
[clojure.tools.logging :as log]
- [medley.core :as m]
[metabase
[config :as config]
[util :as u]]
@@ -11,6 +10,7 @@
[metabase.util
[i18n :as ui18n :refer [tru]]
[schema :as su]]
+ [potemkin.types :as p.types]
[schema.core :as s])
(:import clojure.core.async.impl.channels.ManyToManyChannel
metabase.async.streaming_response.StreamingResponse))
@@ -254,17 +254,36 @@
(^String .replace "/" "_")
symbol)))
-(defn wrap-response-if-needed
- "If `response` isn't already a map with keys `:status` and `:body`, wrap it in one (using status 200)."
- [response]
+(p.types/defprotocol+ EndpointResponse
+ "Protocol for transformations that should be done to the value returned by a `defendpoint` form before it
+ Compojure/Ring see it."
+ (wrap-response-if-needed [this]
+ "Transform the value returned by a `defendpoint` form as needed, e.g. by adding `:status` and `:body`."))
+
+(extend-protocol EndpointResponse
+ Object
+ (wrap-response-if-needed [this]
+ {:status 200, :body this})
+
+ nil
+ (wrap-response-if-needed [_]
+ {:status 204, :body nil})
+
+ StreamingResponse
+ (wrap-response-if-needed [this]
+ this)
+
+ ManyToManyChannel
+ (wrap-response-if-needed [chan]
+ {:status 202, :body chan})
+
+ clojure.lang.IPersistentMap
+ (wrap-response-if-needed [m]
+ (if (and (:status m) (contains? m :body))
+ m
+ {:status 200, :body m}))
+
;; Not sure why this is but the JSON serialization middleware barfs if response is just a plain boolean
- (when (m/boolean? response)
- (throw (Exception. (tru "Attempted to return a boolean as an API response. This is not allowed!"))))
- (if (and (map? response)
- (contains? response :status)
- (contains? response :body))
- response
- {:status (if (some #(instance? % response) [ManyToManyChannel StreamingResponse])
- 202
- 200)
- :body response}))
+ Boolean
+ (wrap-response-if-needed [_]
+ (throw (Exception. (tru "Attempted to return a boolean as an API response. This is not allowed!")))))
diff --git a/src/metabase/api/session.clj b/src/metabase/api/session.clj
index be9beeaea24..c55eca95c8c 100644
--- a/src/metabase/api/session.clj
+++ b/src/metabase/api/session.clj
@@ -183,7 +183,8 @@
(let [reset-token (user/set-password-reset-token! user-id)
password-reset-url (str (public-settings/site-url) "/auth/reset_password/" reset-token)]
(email/send-password-reset-email! email google-auth? server-name password-reset-url)
- (log/info password-reset-url)))))
+ (log/info password-reset-url))))
+ api/generic-204-no-content)
(def ^:private ^:const reset-token-ttl-ms
diff --git a/src/metabase/api/setting.clj b/src/metabase/api/setting.clj
index 5e87b2b4c49..4e5f85fb77c 100644
--- a/src/metabase/api/setting.clj
+++ b/src/metabase/api/setting.clj
@@ -15,7 +15,8 @@
"Update multiple `Settings` values. You must be a superuser to do this."
[:as {settings :body}]
(api/check-superuser)
- (setting/set-many! settings))
+ (setting/set-many! settings)
+ api/generic-204-no-content)
(api/defendpoint GET "/:key"
"Fetch a single `Setting`. You must be a superuser to do this."
@@ -30,7 +31,8 @@
[key :as {{:keys [value]} :body}]
{key su/NonBlankString}
(api/check-superuser)
- (setting/set! key value))
+ (setting/set! key value)
+ api/generic-204-no-content)
(api/define-routes)
diff --git a/src/metabase/async/streaming_response.clj b/src/metabase/async/streaming_response.clj
index cd5625d6bf2..b705fbae1a9 100644
--- a/src/metabase/async/streaming_response.clj
+++ b/src/metabase/async/streaming_response.clj
@@ -9,6 +9,7 @@
[ring.util.response :as ring.response])
(:import [java.io BufferedWriter FilterOutputStream OutputStream OutputStreamWriter]
java.nio.charset.StandardCharsets
+ java.util.zip.GZIPOutputStream
org.eclipse.jetty.io.EofException))
(def ^:private keepalive-interval-ms
@@ -107,22 +108,33 @@
(.flush writer))
(catch Throwable _))))
-(defn- write-to-stream! [f {:keys [write-keepalive-newlines?]} ^OutputStream os finished-chan]
- (with-open-chan [canceled-chan (a/promise-chan)]
- (with-open [os os
- os (jetty-eof-canceling-output-stream os canceled-chan)
- os (keepalive-output-stream os write-keepalive-newlines?)]
-
- (try
- (f os canceled-chan)
- (catch Throwable e
- (write-error! os {:message (.getMessage e)}))
- (finally
- (.flush os)
- (a/>!! finished-chan (if (a/poll! canceled-chan)
- :canceled
- :done))
- (a/close! finished-chan))))))
+(defn- write-to-stream! [f {:keys [write-keepalive-newlines? gzip?], :as options} ^OutputStream os finished-chan]
+ (if gzip?
+ (with-open [gzos (GZIPOutputStream. os true)]
+ (write-to-stream! f (dissoc options :gzip?) gzos finished-chan))
+ (with-open-chan [canceled-chan (a/promise-chan)]
+ (with-open [os os
+ os (jetty-eof-canceling-output-stream os canceled-chan)
+ os (keepalive-output-stream os write-keepalive-newlines?)]
+
+ (try
+ (f os canceled-chan)
+ (catch Throwable e
+ (write-error! os {:message (.getMessage e)}))
+ (finally
+ (.flush os)
+ (a/>!! finished-chan (if (a/poll! canceled-chan)
+ :canceled
+ :done))
+ (a/close! finished-chan)))))))
+
+;; `ring.middleware.gzip` doesn't work on our StreamingResponse class.
+(defn- should-gzip-response?
+ "GZIP a response if the client accepts GZIP encoding, and, if quality is specified, quality > 0."
+ [{{:strs [accept-encoding]} :headers}]
+ (re-find #"gzip|\*" accept-encoding))
+
+(declare render)
(p.types/deftype+ StreamingResponse [f options donechan]
pretty/PrettyPrintable
@@ -134,13 +146,26 @@
(write-body-to-stream [_ _ os]
(write-to-stream! f options os donechan))
+ ;; sync responses only
+ compojure.response/Renderable
+ (render [this request]
+ (render this (should-gzip-response? request)))
+
;; async responses only
compojure.response/Sendable
- (send* [this _ respond _]
- (respond (merge (ring.response/response this)
- {:content-type (:content-type options)
- :headers (:headers options)
- :status 202}))))
+ (send* [this request respond _]
+ (respond (compojure.response/render this request))))
+
+(defn- render [^StreamingResponse streaming-response gzip?]
+ (let [{:keys [headers content-type], :as options} (.options streaming-response)]
+ (assoc (ring.response/response (if gzip?
+ (StreamingResponse. (.f streaming-response)
+ (assoc options :gzip? true)
+ (.donechan streaming-response))
+ streaming-response))
+ :headers (cond-> (assoc headers "Content-Type" content-type)
+ gzip? (assoc "Content-Encoding" "gzip"))
+ :status 202)))
(defn finished-chan
"Fetch a promise channel that will get a message when a `StreamingResponse` is completely finished. Provided primarily
diff --git a/src/metabase/core.clj b/src/metabase/core.clj
index 0cd1da1d276..78ecc4fd268 100644
--- a/src/metabase/core.clj
+++ b/src/metabase/core.clj
@@ -1,4 +1,3 @@
-;; -*- comment-column: 35; -*-
(ns metabase.core
(:gen-class)
(:require [clojure.string :as str]
@@ -19,7 +18,6 @@
[troubleshooting :as troubleshooting]
[util :as u]]
[metabase.core.initialization-status :as init-status]
- [metabase.driver.util :as driver.u]
[metabase.models
[setting :as setting]
[user :refer [User]]]
@@ -70,8 +68,8 @@
(plugins/load-plugins!)
(init-status/set-progress! 0.3)
- ;; Load up all of our Database drivers, which are used for app db work
- (driver.u/find-and-load-all-drivers!)
+ ;; Load up the drivers shipped as part of the main codebase, so they will show up in the list of available DB types
+ (classloader/require 'metabase.driver.h2 'metabase.driver.postgres 'metabase.driver.mysql)
(init-status/set-progress! 0.4)
;; startup database. validates connection & runs any necessary migrations
diff --git a/src/metabase/driver/util.clj b/src/metabase/driver/util.clj
index c148c144440..13c811771d0 100644
--- a/src/metabase/driver/util.clj
+++ b/src/metabase/driver/util.clj
@@ -1,13 +1,11 @@
(ns metabase.driver.util
"Utility functions for common operations on drivers."
(:require [clojure.core.memoize :as memoize]
- [clojure.string :as str]
[clojure.tools.logging :as log]
[metabase
[config :as config]
[driver :as driver]
[util :as u]]
- [metabase.driver.impl :as impl]
[metabase.util.i18n :refer [trs]]
[toucan.db :as db]))
@@ -68,31 +66,6 @@
(memoize/ttl database->driver* :ttl/threshold 1000))
-;;; +----------------------------------------------------------------------------------------------------------------+
-;;; | Loading all Drivers |
-;;; +----------------------------------------------------------------------------------------------------------------+
-
-(defn find-and-load-all-drivers!
- "Search classpath for namespaces that start with `metabase.driver.`, then `require` them, which should register them
- as a side-effect. Note that this will not load drivers added by 3rd-party plugins; they must register themselves
- appropriately when initialized by `load-plugins!`.
-
- This really only needs to be done by the public settings API endpoint to populate the list of available drivers.
- Please avoid using this function elsewhere, as loading all of these namespaces can be quite expensive!"
- []
- (doseq [ns-symb u/metabase-namespace-symbols
- :when (re-matches #"^metabase\.driver\.[a-z0-9_]+$" (name ns-symb))
- :let [driver (keyword (-> (last (str/split (name ns-symb) #"\."))
- (str/replace #"_" "-")))]
- ;; let's go ahead and ignore namespaces we know for a fact do not contain drivers
- :when (not (#{:common :util :query-processor :google :impl}
- driver))]
- (try
- (impl/load-driver-namespace-if-needed! driver)
- (catch Throwable e
- (log/error e (trs "Error loading namespace"))))))
-
-
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | Available Drivers Info |
;;; +----------------------------------------------------------------------------------------------------------------+
diff --git a/src/metabase/middleware/misc.clj b/src/metabase/middleware/misc.clj
index d5d4e950d70..f86dc4ae3aa 100644
--- a/src/metabase/middleware/misc.clj
+++ b/src/metabase/middleware/misc.clj
@@ -14,26 +14,23 @@
(comment metabase.async.streaming-response/keep-me)
-(defn- add-content-type* [request response]
- (update-in
- response
- [:headers "Content-Type"]
- (fn [content-type]
- (or content-type
- (when (middleware.u/api-call? request)
- (if (string? (:body response))
- "text/plain"
- "application/json; charset=utf-8"))))))
+(defn- add-content-type* [request {:keys [body], {:strs [Content-Type]} :headers, :as response}]
+ (cond-> response
+ (not Content-Type)
+ (assoc-in [:headers "Content-Type"] (if (string? body)
+ "text/plain"
+ "application/json; charset=utf-8"))))
(defn add-content-type
"Add an appropriate Content-Type header to response if it doesn't already have one. Most responses should already
have one, so this is a fallback for ones that for one reason or another do not."
[handler]
(fn [request respond raise]
- (handler
- request
- (comp respond (partial add-content-type* request))
- raise)))
+ (handler request
+ (if-not (middleware.u/api-call? request)
+ respond
+ (comp respond (partial add-content-type* request)))
+ raise)))
;;; ------------------------------------------------ SETTING SITE-URL ------------------------------------------------
@@ -81,10 +78,10 @@
;;; ------------------------------------------ Disable Streaming Buffering -------------------------------------------
(defn- maybe-add-disable-buffering-header [{:keys [body], :as response}]
- (if (or (instance? StreamingResponse body)
- (instance? ManyToManyChannel body))
- (assoc-in response [:headers "X-Accel-Buffering"] "no")
- response))
+ (cond-> response
+ (or (instance? StreamingResponse body)
+ (instance? ManyToManyChannel body))
+ (assoc-in [:headers "X-Accel-Buffering"] "no")))
(defn disable-streaming-buffering
"Tell nginx not to batch streaming responses -- otherwise the keepalive bytes aren't written and
diff --git a/src/metabase/query_processor/streaming/json.clj b/src/metabase/query_processor/streaming/json.clj
index 064ae19b96d..2b6075f0405 100644
--- a/src/metabase/query_processor/streaming/json.clj
+++ b/src/metabase/query_processor/streaming/json.clj
@@ -40,7 +40,7 @@
(defmethod i/stream-options :api
[stream-type]
- ((get-method i/stream-options :json) stream-type))
+ {:content-type "applicaton/json; charset=utf-8"})
(defn- map->serialized-json-kvs
"{:a 100, :b 200} ; -> \"a\":100,\"b\":200"
diff --git a/test/metabase/api/dashboard_test.clj b/test/metabase/api/dashboard_test.clj
index 570d4dcd677..9bc6a5c1c6c 100644
--- a/test/metabase/api/dashboard_test.clj
+++ b/test/metabase/api/dashboard_test.clj
@@ -584,13 +584,13 @@
;;; | DELETE /api/dashboard/:id |
;;; +----------------------------------------------------------------------------------------------------------------+
-(expect
- [nil nil]
+(deftest delete-test
(tt/with-temp Dashboard [{dashboard-id :id}]
(with-dashboards-in-writeable-collection [dashboard-id]
- [((user->client :rasta) :delete 204 (format "dashboard/%d" dashboard-id))
- (Dashboard dashboard-id)])))
-
+ (is (= nil
+ ((user->client :rasta) :delete 204 (format "dashboard/%d" dashboard-id))))
+ (is (= nil
+ (Dashboard dashboard-id))))))
;;; +----------------------------------------------------------------------------------------------------------------+
;;; | COPY /api/dashboard/:id/copy |
diff --git a/test/metabase/api/database_test.clj b/test/metabase/api/database_test.clj
index 71b6d5f1279..01aeacd6c6c 100644
--- a/test/metabase/api/database_test.clj
+++ b/test/metabase/api/database_test.clj
@@ -475,10 +475,11 @@
((test-users/user->client :crowberto) :get 200
(format "database/%d/metadata" mbql.s/saved-questions-virtual-database-id))))))
-;; if no eligible Saved Questions exist the virtual DB metadata endpoint should just return `nil`
(deftest return-nil-when-no-eligible-saved-questions
- (is (nil? ((test-users/user->client :crowberto) :get 200
- (format "database/%d/metadata" mbql.s/saved-questions-virtual-database-id)))))
+ (testing "if no eligible Saved Questions exist the virtual DB metadata endpoint should just return `nil`"
+ (is (= nil
+ ((test-users/user->client :crowberto) :get 204
+ (format "database/%d/metadata" mbql.s/saved-questions-virtual-database-id))))))
;;; +----------------------------------------------------------------------------------------------------------------+
diff --git a/test/metabase/api/dataset_test.clj b/test/metabase/api/dataset_test.clj
index 242bd1a0b2d..7a169453df1 100644
--- a/test/metabase/api/dataset_test.clj
+++ b/test/metabase/api/dataset_test.clj
@@ -3,11 +3,14 @@
(:require [cheshire
[core :as json]
[generate :as generate]]
+ [clojure
+ [string :as str]
+ [test :refer :all]]
[clojure.data.csv :as csv]
- [clojure.test :refer :all]
[dk.ative.docjure.spreadsheet :as spreadsheet]
[medley.core :as m]
[metabase
+ [http-client :as http-client]
[query-processor-test :as qp.test]
[test :as mt]
[util :as u]]
@@ -189,6 +192,21 @@
(json/generate-string (data/mbql-query checkins)))]
(take 5 (parse-and-sort-csv result))))))
+(deftest download-response-headers-test
+ (testing "Make sure CSV/etc. download requests come back with the correct headers"
+ (is (= {"Cache-Control" "max-age=0, no-cache, must-revalidate, proxy-revalidate"
+ "Content-Disposition" "attachment; filename=\"query_result_<timestamp>.csv\""
+ "Content-Type" "text/csv"
+ "Expires" "Tue, 03 Jul 2001 06:00:00 GMT"
+ "X-Accel-Buffering" "no"}
+ (-> (http-client/client-full-response (test-users/username->token :rasta)
+ :post 202 "dataset/csv"
+ :query (json/generate-string (data/mbql-query checkins {:limit 1})))
+ :headers
+ (select-keys ["Cache-Control" "Content-Disposition" "Content-Type" "Expires" "X-Accel-Buffering"])
+ (update "Content-Disposition" #(some-> % (str/replace #"query_result_.+(\.\w+)"
+ "query_result_<timestamp>$1"))))))))
+
(deftest check-an-empty-date-column
(is (= [["1" "2014-04-07" "" "5" "12"]
["2" "2014-09-18" "" "1" "31"]
@@ -196,9 +214,9 @@
["4" "2014-03-11" "" "5" "4"]
["5" "2013-05-05" "" "3" "49"]]
(data/dataset defs/test-data-with-null-date-checkins
- (let [result ((test-users/user->client :rasta) :post 202 "dataset/csv" :query
- (json/generate-string (data/mbql-query checkins)))]
- (take 5 (parse-and-sort-csv result)))))))
+ (let [result ((test-users/user->client :rasta) :post 202 "dataset/csv" :query
+ (json/generate-string (data/mbql-query checkins)))]
+ (take 5 (parse-and-sort-csv result)))))))
(deftest sqlite-datetime-test
(mt/test-driver :sqlite
diff --git a/test/metabase/api/geojson_test.clj b/test/metabase/api/geojson_test.clj
index 5c3b65b6f3b..d02e8608ffc 100644
--- a/test/metabase/api/geojson_test.clj
+++ b/test/metabase/api/geojson_test.clj
@@ -35,17 +35,19 @@
(deftest invalid-url-test
(testing "test that you're not allowed to set invalid URLs"
- (is (thrown? Exception
- (geojson-api/custom-geojson {:name "Middle Earth"
- :url "ABC"
- :region_key nil
- :region_name nil})))
+ (is (thrown?
+ Exception
+ (geojson-api/custom-geojson {:name "Middle Earth"
+ :url "ABC"
+ :region_key nil
+ :region_name nil})))
- (is (thrown? Exception
- (geojson-api/custom-geojson {:name "Middle Earth"
- :url "http://google.com"
- :region_key nil
- :region_name nil})))))
+ (is (thrown?
+ Exception
+ (geojson-api/custom-geojson {:name "Middle Earth"
+ :url "http://google.com"
+ :region_key nil
+ :region_name nil})))))
(deftest update-endpoint-test
(testing "PUT /api/setting/custom-geojson"
@@ -56,24 +58,27 @@
;; bind a temporary value so it will get set back to its old value here after the API calls are done
;; stomping all over it
(mt/with-temporary-setting-values [custom-geojson nil]
- ((mt/user->client :crowberto) :put 200 "setting/custom-geojson" {:value test-custom-geojson})
+ ((mt/user->client :crowberto) :put 204 "setting/custom-geojson" {:value test-custom-geojson})
((mt/user->client :crowberto) :get 200 "setting/custom-geojson"))))))
+
(testing "error conditions"
(letfn [(put-bad-url [url]
(mt/suppress-output
- ;; try this up to 3 times since Circle's outbound connections likes to randomly stop working
- (u/auto-retry 3
- ;; bind a temporary value so it will get set back to its old value here after the API calls are done
- ;; stomping all over it
- (mt/with-temporary-setting-values [custom-geojson nil]
- (let [url (assoc-in test-custom-geojson [:middle-earth :url] url)]
- (:message ((mt/user->client :crowberto) :put 500 "setting/custom-geojson" {:value url})))))))]
+ ;; try this up to 3 times since Circle's outbound connections likes to randomly stop working
+ (u/auto-retry 3
+ ;; bind a temporary value so it will get set back to its old value here after the API calls are done
+ ;; stomping all over it
+ (mt/with-temporary-setting-values [custom-geojson nil]
+ (let [url (assoc-in test-custom-geojson [:middle-earth :url] url)]
+ (:message ((mt/user->client :crowberto) :put 500 "setting/custom-geojson" {:value url})))))))]
(testing "Test that a bad url will return a descriptive error message"
(is (re= #"^Unable to retrieve resource.*"
(put-bad-url "https://raw.githubusercontent.com/metabase/metabase/master/test_resources/something-random"))))
+
(testing "Test that a bad host will return a connection refused error"
(is (re= #"^Unable to connect.*"
(put-bad-url "https://somethingrandom.metabase.com"))))
+
(testing "Test out the error message for a relative path file we can't find"
(is (re= #"^Unable to find JSON via relative path.*"
(put-bad-url "some/relative/path"))))))))
diff --git a/test/metabase/api/session_test.clj b/test/metabase/api/session_test.clj
index dffa272dffb..61f0e404583 100644
--- a/test/metabase/api/session_test.clj
+++ b/test/metabase/api/session_test.clj
@@ -51,8 +51,6 @@
(client :post 400 "session" (-> (test-users/user->credentials :rasta)
(assoc :password "something else"))))
-;;
-;;
(deftest login-throttling-test
(testing (str "Test that people get blocked from attempting to login if they try too many times (Check that"
" throttling works at the API level -- more tests in the throttle library itself:"
@@ -131,65 +129,67 @@
(is (re= #"^Too many attempts! You must wait 4\d seconds before trying again\.$"
(error)))))))
-
-;; ## DELETE /api/session
-;; Test that we can logout
-(expect
- nil
- (do
- ;; clear out cached session tokens so next time we make an API request it log in & we'll know we have a valid
- ;; Session
- (test-users/clear-cached-session-tokens!)
- (let [session-id (test-users/username->token :rasta)]
- ;; Ok, calling the logout endpoint should delete the Session in the DB. Don't worry, `test-users` will log back
- ;; in on the next API call
- ((test-users/user->client :rasta) :delete 204 "session")
- ;; check whether it's still there -- should be GONE
- (Session session-id))))
-
-
-;; ## POST /api/session/forgot_password
-;; Test that we can initiate password reset
-(expect
- (et/with-fake-inbox
- (let [reset-fields-set? (fn []
- (let [{:keys [reset_token reset_triggered]} (db/select-one [User :reset_token :reset_triggered]
- :id (test-users/user->id :rasta))]
- (boolean (and reset_token reset_triggered))))]
- ;; make sure user is starting with no values
- (db/update! User (test-users/user->id :rasta), :reset_token nil, :reset_triggered nil)
- (assert (not (reset-fields-set?)))
- ;; issue reset request (token & timestamp should be saved)
- ((test-users/user->client :rasta) :post 200 "session/forgot_password" {:email (:username (test-users/user->credentials :rasta))})
- ;; TODO - how can we test email sent here?
- (reset-fields-set?))))
-
-;; Test that email is required
-(expect {:errors {:email "value must be a valid email address."}}
- (client :post 400 "session/forgot_password" {}))
-
-;; Test that email not found also gives 200 as to not leak existence of user
-(expect nil
- (client :post 200 "session/forgot_password" {:email "not-found@metabase.com"}))
-
-;; Test that email based throttling kicks in after the login failure threshold (10) has been reached
-(defn- send-password-reset [& [expected-status & more]]
- (client :post (or expected-status 200) "session/forgot_password" {:email "not-found@metabase.com"}))
+(deftest logout-test
+ (testing "DELETE /api/session"
+ (testing "Test that we can logout"
+ ;; clear out cached session tokens so next time we make an API request it log in & we'll know we have a valid
+ ;; Session
+ (test-users/clear-cached-session-tokens!)
+ (let [session-id (test-users/username->token :rasta)]
+ ;; Ok, calling the logout endpoint should delete the Session in the DB. Don't worry, `test-users` will log back
+ ;; in on the next API call
+ ((test-users/user->client :rasta) :delete 204 "session")
+ ;; check whether it's still there -- should be GONE
+ (is (= nil
+ (Session session-id)))))))
+
+(deftest forgot-password-test
+ (testing "POST /api/session/forgot_password"
+ (testing "Test that we can initiate password reset"
+ (et/with-fake-inbox
+ (letfn [(reset-fields-set? []
+ (let [{:keys [reset_token reset_triggered]} (db/select-one [User :reset_token :reset_triggered]
+ :id (test-users/user->id :rasta))]
+ (boolean (and reset_token reset_triggered))))]
+ ;; make sure user is starting with no values
+ (db/update! User (test-users/user->id :rasta), :reset_token nil, :reset_triggered nil)
+ (assert (not (reset-fields-set?)))
+ ;; issue reset request (token & timestamp should be saved)
+ (is (= nil
+ ((test-users/user->client :rasta) :post 204 "session/forgot_password" {:email (:username (test-users/user->credentials :rasta))}))
+ "Request should return no content")
+ (is (= true
+ (reset-fields-set?))
+ "User `:reset_token` and `:reset_triggered` should be updated")
+ (is (= "[Metabase] Password Reset Request"
+ (-> @et/inbox (get "rasta@metabase.com") first :subject))
+ "User should get a password reset email"))))
+
+ (testing "test that email is required"
+ (is (= {:errors {:email "value must be a valid email address."}}
+ (client :post 400 "session/forgot_password" {}))))
+
+ (testing "Test that email not found also gives 200 as to not leak existence of user"
+ (is (= nil
+ (client :post 204 "session/forgot_password" {:email "not-found@metabase.com"}))))))
(deftest forgot-password-throttling-test
- (with-redefs [session-api/forgot-password-throttlers (cleaned-throttlers #'session-api/forgot-password-throttlers
- [:email :ip-address])]
- (dotimes [n 10]
- (send-password-reset))
- (let [error (fn []
- (-> (send-password-reset 400)
- :errors
- :email))]
- (is (= "Too many attempts! You must wait 15 seconds before trying again."
- (error)))
- ;;`throttling/check` gives 15 in stead of 42
- (is (= "Too many attempts! You must wait 15 seconds before trying again."
- (error))))))
+ (testing "Test that email based throttling kicks in after the login failure threshold (10) has been reached"
+ (letfn [(send-password-reset [& [expected-status & more]]
+ (client :post (or expected-status 204) "session/forgot_password" {:email "not-found@metabase.com"}))]
+ (with-redefs [session-api/forgot-password-throttlers (cleaned-throttlers #'session-api/forgot-password-throttlers
+ [:email :ip-address])]
+ (dotimes [n 10]
+ (send-password-reset))
+ (let [error (fn []
+ (-> (send-password-reset 400)
+ :errors
+ :email))]
+ (is (= "Too many attempts! You must wait 15 seconds before trying again."
+ (error)))
+ ;;`throttling/check` gives 15 in stead of 42
+ (is (= "Too many attempts! You must wait 15 seconds before trying again."
+ (error))))))))
;; POST /api/session/reset_password
diff --git a/test/metabase/api/setting_test.clj b/test/metabase/api/setting_test.clj
index 0f32abb24f8..ceb736d305e 100644
--- a/test/metabase/api/setting_test.clj
+++ b/test/metabase/api/setting_test.clj
@@ -1,7 +1,7 @@
(ns metabase.api.setting-test
(:require [clojure.test :refer :all]
- [expectations :refer [expect]]
[metabase.models.setting-test :refer [test-sensitive-setting test-setting-1 test-setting-2 test-setting-3]]
+ [metabase.test :as mt]
[metabase.test.data.users :as test-users]
[metabase.test.fixtures :as fixtures]))
@@ -16,106 +16,93 @@
(defn- fetch-setting [setting-name status]
((test-users/user->client :crowberto) :get status (format "setting/%s" (name setting-name))))
-;; ## GET /api/setting
-;; Check that we can fetch all Settings for Org, except `:visiblity :internal` ones
-(expect
- [{:key "test-setting-1"
- :value nil
- :is_env_setting true
- :env_name "MB_TEST_SETTING_1"
- :description "Test setting - this only shows up in dev (1)"
- :default "Using value of env var $MB_TEST_SETTING_1"}
- {:key "test-setting-2"
- :value "FANCY"
- :is_env_setting false
- :env_name "MB_TEST_SETTING_2"
- :description "Test setting - this only shows up in dev (2)"
- :default "[Default Value]"}]
- (do
- (test-setting-1 nil)
- (test-setting-2 "FANCY")
- ; internal setting that should not be returned:
- (test-setting-3 "oh hai")
- (fetch-test-settings)))
-
-;; Check that non-superusers are denied access
-(expect "You don't have permissions to do that."
- ((test-users/user->client :rasta) :get 403 "setting"))
-
-
-;; ## GET /api/setting/:key
-;; Test that we can fetch a single setting
-(expect
- "OK!"
- (do (test-setting-2 "OK!")
- (fetch-setting :test-setting-2 200)))
-
-;; Test that we can't fetch internal settings
-(expect
- "Setting :test-setting-3 is internal"
- (do (test-setting-3 "NOPE!")
- (:message (fetch-setting :test-setting-3 500))))
+(deftest fetch-setting-test
+ (testing "GET /api/setting"
+ (testing "Check that we can fetch all Settings, except `:visiblity :internal` ones"
+ (test-setting-1 nil)
+ (test-setting-2 "FANCY")
+ (test-setting-3 "oh hai") ; internal setting that should not be returned
+ (is (= [{:key "test-setting-1"
+ :value nil
+ :is_env_setting true
+ :env_name "MB_TEST_SETTING_1"
+ :description "Test setting - this only shows up in dev (1)"
+ :default "Using value of env var $MB_TEST_SETTING_1"}
+ {:key "test-setting-2"
+ :value "FANCY"
+ :is_env_setting false
+ :env_name "MB_TEST_SETTING_2"
+ :description "Test setting - this only shows up in dev (2)"
+ :default "[Default Value]"}]
+ (fetch-test-settings))))
+
+ (testing "Check that non-superusers are denied access"
+ (is (= "You don't have permissions to do that."
+ ((test-users/user->client :rasta) :get 403 "setting")))))
+
+ (testing "GET /api/setting/:key"
+ (testing "Test that we can fetch a single setting"
+ (test-setting-2 "OK!")
+ (is (= "OK!"
+ (fetch-setting :test-setting-2 200))))))
+
+(deftest fetch-internal-settings-test
+ (testing "Test that we can't fetch internal settings"
+ (test-setting-3 "NOPE!")
+ (is (= "Setting :test-setting-3 is internal"
+ (mt/suppress-output
+ (:message (fetch-setting :test-setting-3 500)))))))
(deftest update-settings-test
(testing "PUT /api/setting/:key"
- ((test-users/user->client :crowberto) :put 200 "setting/test-setting-1" {:value "NICE!"})
+ ((test-users/user->client :crowberto) :put 204 "setting/test-setting-1" {:value "NICE!"})
(is (= "NICE!"
(test-setting-1))
"Updated setting should be visible from setting getter")
(is (= "NICE!"
(fetch-setting :test-setting-1 200))
- "Updated setting should be visible from API endpoint")))
-
-;; ## Check non-superuser can't set a Setting
-(expect "You don't have permissions to do that."
- ((test-users/user->client :rasta) :put 403 "setting/test-setting-1" {:value "NICE!"}))
-
-
-;;; ----------------------------------------------- Sensitive Settings -----------------------------------------------
-
-;; Sensitive settings should always come back obfuscated
-
-;; GET /api/setting/:name should obfuscate sensitive settings
-(expect
- "**********EF"
- (do
- (test-sensitive-setting "ABCDEF")
- (fetch-setting :test-sensitive-setting 200)))
-
-;; GET /api/setting should obfuscate sensitive settings
-(expect
- {:key "test-sensitive-setting"
- :value "**********LM"
- :is_env_setting false
- :env_name "MB_TEST_SENSITIVE_SETTING"
- :description "This is a sample sensitive Setting."
- :default nil}
- (do
- (test-sensitive-setting "GHIJKLM")
- (some (fn [{setting-name :key, :as setting}]
- (when (= setting-name "test-sensitive-setting")
- setting))
- ((test-users/user->client :crowberto) :get 200 "setting"))))
-
-;; Setting the Setting via an endpoint should still work as expected; the normal getter functions should *not*
-;; obfuscate sensitive Setting values -- that should be done by the API
-(expect
- "123456"
- (do
- ((test-users/user->client :crowberto) :put 200 "setting/test-sensitive-setting" {:value "123456"})
- (test-sensitive-setting)))
-
-;; Attempts to set the Setting to an obfuscated value should be ignored
-(expect
- "123456"
- (do
- (test-sensitive-setting "123456")
- ((test-users/user->client :crowberto) :put 200 "setting/test-sensitive-setting" {:value "**********56"})
- (test-sensitive-setting)))
-
-(expect
- (do
- (test-sensitive-setting "123456")
- ((test-users/user->client :crowberto) :put 200 "setting" {:test-sensitive-setting "**********56"})
- (test-sensitive-setting)))
+ "Updated setting should be visible from API endpoint")
+
+ (testing "Check non-superuser can't set a Setting"
+ (= "You don't have permissions to do that."
+ ((test-users/user->client :rasta) :put 403 "setting/test-setting-1" {:value "NICE!"})))))
+
+(deftest fetch-sensitive-setting-test
+ (testing "Sensitive settings should always come back obfuscated"
+ (testing "GET /api/setting/:name"
+ (test-sensitive-setting "ABCDEF")
+ (is (= "**********EF"
+ (fetch-setting :test-sensitive-setting 200))))
+
+ (testing "GET /api/setting"
+ (test-sensitive-setting "GHIJKLM")
+ (is (= {:key "test-sensitive-setting"
+ :value "**********LM"
+ :is_env_setting false
+ :env_name "MB_TEST_SENSITIVE_SETTING"
+ :description "This is a sample sensitive Setting."
+ :default nil}
+ (some (fn [{setting-name :key, :as setting}]
+ (when (= setting-name "test-sensitive-setting")
+ setting))
+ ((test-users/user->client :crowberto) :get 200 "setting")))))))
+
+(deftest set-sensitive-setting-test
+ (testing "Setting the Setting via an endpoint should still work as expected; the normal getter functions should *not* obfuscate sensitive Setting values -- that should be done by the API"
+ ((test-users/user->client :crowberto) :put 204 "setting/test-sensitive-setting" {:value "123456"})
+ (is (= "123456"
+ (test-sensitive-setting))))
+
+ (testing "Attempts to set the Setting to an obfuscated value should be ignored"
+ (testing "PUT /api/setting/:name"
+ (test-sensitive-setting "123456")
+ ((test-users/user->client :crowberto) :put 204 "setting/test-sensitive-setting" {:value "**********56"})
+ (is (= "123456"
+ (test-sensitive-setting))))
+
+ (testing "PUT /api/setting"
+ (test-sensitive-setting "123456")
+ ((test-users/user->client :crowberto) :put 204 "setting" {:test-sensitive-setting "**********56"})
+ (is (= "123456"
+ (test-sensitive-setting))))))
diff --git a/test/metabase/email_test.clj b/test/metabase/email_test.clj
index e72bcb97893..c540d53e486 100644
--- a/test/metabase/email_test.clj
+++ b/test/metabase/email_test.clj
@@ -69,7 +69,7 @@
(defmacro with-fake-inbox
"Clear `inbox`, bind `send-email!` to `fake-inbox-email-fn`, set temporary settings for `email-smtp-username`
- and `email-smtp-password` (which will cause `metabase.email/email-configured?` to return `true`, and execute BODY.
+ and `email-smtp-password` (which will cause `metabase.email/email-configured?` to return `true`, and execute `body`.
Fetch the emails send by dereffing `inbox`.
diff --git a/test/metabase/pulse_test.clj b/test/metabase/pulse_test.clj
index 8bbe1a64a6a..add3c4c0b66 100644
--- a/test/metabase/pulse_test.clj
+++ b/test/metabase/pulse_test.clj
@@ -908,6 +908,8 @@
(is (= [[1 "2014-04-07T00:00:00Z" 5 12]]
(send-pulse-created-by-user! :crowberto)))
(is (thrown-with-msg?
- clojure.lang.ExceptionInfo #"^You do not have permissions to view Card [\d,]+."
- (send-pulse-created-by-user! :rasta))
+ clojure.lang.ExceptionInfo
+ #"^You do not have permissions to view Card [\d,]+."
+ (mt/suppress-output
+ (send-pulse-created-by-user! :rasta)))
"If the current user doesn't have permissions to execute the Card for a Pulse, an Exception should be thrown."))))
diff --git a/test/metabase/test/integrations/ldap.clj b/test/metabase/test/integrations/ldap.clj
index 2a94dafa6ab..bacbc0ff273 100644
--- a/test/metabase/test/integrations/ldap.clj
+++ b/test/metabase/test/integrations/ldap.clj
@@ -18,9 +18,9 @@
(defn- start-ldap-server! []
(with-open [ldif (LDIFReader. (or (io/file (io/resource "ldap.ldif"))
+ (io/file "test_resources/ldap.ldif")
(throw
- (FileNotFoundException.
- "ldap.ldif does not exist! (You may need to copy it into your resources dir.)"))))]
+ (FileNotFoundException. "ldap.ldif does not exist!"))))]
(doto (InMemoryDirectoryServer. (get-server-config))
(.importFromLDIF true ldif)
(.startListening))))
--
GitLab