Merge pull request #8786 from metabase/slack-perf-fix

Fix Slack channel API performance issues

project.clj

@@ -140,7 +140,8 @@
   :docstring-checker {:include [#"^metabase"]
                       :exclude [#"test"
                                 #"^metabase\.http-client$"]}
-  :profiles {:dev {:dependencies [[expectations "2.2.0-beta2"]        ; unit tests
+  :profiles {:dev {:dependencies [[clj-http-fake "1.0.3"]             ; Library to mock clj-http responses
+                                  [expectations "2.2.0-beta2"]        ; unit tests
                                   [ring/ring-mock "0.3.0"]]           ; Library to create mock Ring requests for unit tests
                    :plugins [[docstring-checker "1.0.2"]              ; Check that all public vars have docstrings. Run with 'lein docstring-checker'
                              [jonase/eastwood "0.3.1"

src/metabase/integrations/slack.clj

 (ns metabase.integrations.slack
   (:require [cheshire.core :as json]
             [clj-http.client :as http]
+            [clojure.java.io :as io]
             [clojure.tools.logging :as log]
             [metabase.models.setting :as setting :refer [defsetting]]
             [metabase.util.i18n :refer [tru]]
@@ -19,7 +20,7 @@
 
 
 (defn- handle-response [{:keys [status body]}]
-  (let [body (json/parse-string body keyword)]
+  (let [body (-> body io/reader (json/parse-stream keyword))]
     (if (and (= 200 status) (:ok body))
       body
       (let [error (if (= (:error body) "invalid_auth")
@@ -31,15 +32,16 @@
 (defn- do-slack-request [request-fn params-key endpoint & {:keys [token], :as params, :or {token (slack-token)}}]
   (when token
     (handle-response (request-fn (str slack-api-base-url "/" (name endpoint)) {params-key      (assoc params :token token)
-                                                                              :conn-timeout   1000
-                                                                              :socket-timeout 1000}))))
+                                                                               :as             :stream
+                                                                               :conn-timeout   1000
+                                                                               :socket-timeout 1000}))))
 
 (def ^{:arglists '([endpoint & {:as params}]), :style/indent 1} GET  "Make a GET request to the Slack API."  (partial do-slack-request http/get  :query-params))
 (def ^{:arglists '([endpoint & {:as params}]), :style/indent 1} POST "Make a POST request to the Slack API." (partial do-slack-request http/post :form-params))
 
 (def ^{:arglists '([& {:as args}])} channels-list
   "Calls Slack api `channels.list` function and returns the list of available channels."
-  (comp :channels (partial GET :channels.list, :exclude_archived 1)))
+  (comp :channels (partial GET :channels.list, :exclude_archived true, :exclude_members true)))
 
 (def ^{:arglists '([& {:as args}])} users-list
   "Calls Slack api `users.list` function and returns the list of available users."
@@ -55,7 +57,7 @@
   []
   (some (fn [channel] (when (= (:name channel) files-channel-name)
                         channel))
-        (channels-list :exclude_archived 0)))
+        (channels-list :exclude_archived false)))
 
 (defn files-channel
   "Calls Slack api `channels.info` to check whether a channel named #metabase_files exists. If it doesn't,

test/metabase/integrations/slack_test.clj

+(ns metabase.integrations.slack-test
+  (:require [cheshire.core :as json]
+            [clj-http.fake :as http-fake]
+            [clojure.java.io :as io]
+            [expectations :refer :all]
+            [metabase.integrations.slack :as slack-integ :refer :all]
+            [metabase.test.util :as tu]))
+
+(def ^:private default-channels-response
+  (delay (slurp (io/resource "slack_channels_response.json"))))
+
+(def ^:private default-channels
+  (delay (:channels (json/parse-string @default-channels-response keyword))))
+
+(def ^:private channels-request
+  {:address      "https://slack.com/api/channels.list"
+   :query-params {:token            "test-token"
+                  :exclude_archived "true"
+                  :exclude_members  "true"}})
+
+(defn- expected-200-response [body]
+  (fn [_]
+    {:status 200
+     :body (if (string? body)
+             body
+             (json/generate-string body))}))
+
+(def ^:private invalid-token-response
+  (expected-200-response
+   {:ok    false
+    :error "invalid_auth"}))
+
+(defn- exception-if-called [_]
+  (throw (Exception. "Failure, route should not have been invoked")))
+
+;; Channels should return nil if no Slack token has been configured
+(expect
+  nil
+  (http-fake/with-fake-routes {channels-request exception-if-called}
+    (tu/with-temporary-setting-values [slack-token nil]
+      (channels-list))))
+
+;; Test the channels call and expected response
+(expect
+  @default-channels
+  (http-fake/with-fake-routes {channels-request (expected-200-response @default-channels-response)}
+    (tu/with-temporary-setting-values [slack-token "test-token"]
+      (channels-list))))
+
+;; Test the invalid token auth flow
+(expect
+  {:ex-class clojure.lang.ExceptionInfo
+   :msg      nil
+   :data     {:errors {:slack-token "Invalid token"}}}
+  (http-fake/with-fake-routes {channels-request invalid-token-response}
+    (tu/with-temporary-setting-values [slack-token "test-token"]
+      (tu/exception-and-message
+       (channels-list)))))
+
+(def ^:private default-users-response
+  (delay (slurp (io/resource "slack_users_response.json"))))
+
+(def ^:private default-users
+  (delay (:members (json/parse-string @default-users-response keyword))))
+
+(def ^:private users-request
+  {:address "https://slack.com/api/users.list"
+   :query-params {:token "test-token"}})
+
+;; Users should return nil if no Slack token has been configured
+(expect
+  nil
+  (http-fake/with-fake-routes {users-request exception-if-called}
+    (tu/with-temporary-setting-values [slack-token nil]
+      (users-list))))
+
+;; Test the users call and the expected response
+(expect
+  @default-users
+  (http-fake/with-fake-routes {users-request (expected-200-response @default-users-response)}
+    (tu/with-temporary-setting-values [slack-token "test-token"]
+      (users-list))))
+
+;; Test the invalid token auth flow for users
+(expect
+  {:ex-class clojure.lang.ExceptionInfo
+   :msg      nil
+   :data     {:errors {:slack-token "Invalid token"}}}
+  (http-fake/with-fake-routes {users-request invalid-token-response}
+    (tu/with-temporary-setting-values [slack-token "test-token"]
+      (tu/exception-and-message
+       (users-list)))))
+
+(def ^:private files-request
+  (assoc-in channels-request [:query-params :exclude_archived] "false"))
+
+;; Asking for the files channel when slack is not configured throws an exception
+(expect
+  {:ex-class clojure.lang.ExceptionInfo
+   :msg      (var-get #'slack-integ/channel-missing-msg)
+   :data     {:status-code 400}}
+  (http-fake/with-fake-routes {files-request exception-if-called}
+    (tu/exception-and-message
+     (files-channel))))
+
+(defn- create-files-channel []
+  (let [channel-name (var-get #'slack-integ/files-channel-name)]
+    (-> @default-channels
+        first
+        (assoc
+            :name channel-name, :name_normalized channel-name,
+            :purpose {:value "Metabase file upload location", :creator "", :last_set 0}))))
+
+;; Testing the call that finds the metabase files channel
+(expect
+  (create-files-channel)
+  (http-fake/with-fake-routes {files-request (-> @default-channels-response
+                                                 json/parse-string
+                                                 (update :channels conj (create-files-channel))
+                                                 expected-200-response)}
+    (tu/with-temporary-setting-values [slack-token "test-token"]
+      (files-channel))))

test/metabase/query_processor/middleware/add_query_throttle_test.clj

@@ -10,14 +10,6 @@
             [metabase.util :as u])
   (:import java.util.concurrent.Semaphore))
 
-(defmacro ^:private exception-and-message [& body]
-  `(try
-     ~@body
-     (catch Exception e#
-       {:ex-class (class e#)
-        :msg      (.getMessage e#)
-        :data     (ex-data e#)})))
-
 (defmacro ^:private with-query-wait-time-in-seconds [time-in-seconds & body]
   `(with-redefs [throttle/max-query-wait-time-in-millis ~(* 1000 time-in-seconds)]
      ~@body))
@@ -30,7 +22,7 @@
    :data     {:status-code 503
               :type        ::throttle/concurrent-query-limit-reached}}
   (with-query-wait-time-in-seconds 1
-    (exception-and-message
+    (tu/exception-and-message
      (let [semaphore (Semaphore. 5)]
        (.acquire semaphore 5)
        ((#'throttle/throttle-queries semaphore (constantly "Should never be returned")) {})))))
@@ -43,7 +35,7 @@
    :data     {:status-code 503
               :type        ::throttle/concurrent-query-limit-reached}}
   (with-query-wait-time-in-seconds 1
-    (exception-and-message
+    (tu/exception-and-message
      (let [semaphore (Semaphore. 5)
            my-qp     (->> identity
                           (#'throttle/throttle-queries semaphore)

test/metabase/test/util.clj

@@ -675,3 +675,13 @@
 
     :else
     x))
+
+(defmacro exception-and-message
+  "Invokes `body`, catches the exception and returns a map with the exception class, message and data"
+  [& body]
+  `(try
+     ~@body
+     (catch Exception e#
+       {:ex-class (class e#)
+        :msg      (.getMessage e#)
+        :data     (ex-data e#)})))

test_resources/slack_channels_response.json

+{
+    "ok": true,
+    "channels": [
+        {
+            "id": "C0G9QF9GW",
+            "name": "random",
+            "is_channel": true,
+            "created": 1449709280,
+            "creator": "U0G9QF9C6",
+            "is_archived": false,
+            "is_general": false,
+            "name_normalized": "random",
+            "is_shared": false,
+            "is_org_shared": false,
+            "is_member": true,
+            "is_private": false,
+            "is_mpim": false,
+            "members": [
+                "U0G9QF9C6",
+                "U0G9WFXNZ"
+            ],
+            "topic": {
+                "value": "Other stuff",
+                "creator": "U0G9QF9C6",
+                "last_set": 1449709352
+            },
+            "purpose": {
+                "value": "A place for non-work-related flimflam, faffing, hodge-podge or jibber-jabber you'd prefer to keep out of more focused work-related channels.",
+                "creator": "",
+                "last_set": 0
+            },
+            "previous_names": [],
+            "num_members": 2
+        },
+        {
+            "id": "C0G9QKBBL",
+            "name": "general",
+            "is_channel": true,
+            "created": 1449709280,
+            "creator": "U0G9QF9C6",
+            "is_archived": false,
+            "is_general": true,
+            "name_normalized": "general",
+            "is_shared": false,
+            "is_org_shared": false,
+            "is_member": true,
+            "is_private": false,
+            "is_mpim": false,
+            "members": [
+                "U0G9QF9C6",
+                "U0G9WFXNZ"
+            ],
+            "topic": {
+                "value": "Talk about anything!",
+                "creator": "U0G9QF9C6",
+                "last_set": 1449709364
+            },
+            "purpose": {
+                "value": "To talk about anything!",
+                "creator": "U0G9QF9C6",
+                "last_set": 1449709334
+            },
+            "previous_names": [],
+            "num_members": 2
+        }
+    ],
+    "response_metadata": {
+        "next_cursor": "dGVhbTpDMUg5UkVTR0w="
+    }
+}

test_resources/slack_users_response.json

+{
+    "ok": true,
+    "members": [
+        {
+            "id": "W012A3CDE",
+            "team_id": "T012AB3C4",
+            "name": "spengler",
+            "deleted": false,
+            "color": "9f69e7",
+            "real_name": "spengler",
+            "tz": "America/Los_Angeles",
+            "tz_label": "Pacific Daylight Time",
+            "tz_offset": -25200,
+            "profile": {
+                "avatar_hash": "ge3b51ca72de",
+                "status_text": "Print is dead",
+                "status_emoji": ":books:",
+                "real_name": "Egon Spengler",
+                "display_name": "spengler",
+                "real_name_normalized": "Egon Spengler",
+                "display_name_normalized": "spengler",
+                "email": "spengler@ghostbusters.example.com",
+                "image_24": "https://.../avatar/e3b51ca72dee4ef87916ae2b9240df50.jpg",
+                "image_32": "https://.../avatar/e3b51ca72dee4ef87916ae2b9240df50.jpg",
+                "image_48": "https://.../avatar/e3b51ca72dee4ef87916ae2b9240df50.jpg",
+                "image_72": "https://.../avatar/e3b51ca72dee4ef87916ae2b9240df50.jpg",
+                "image_192": "https://.../avatar/e3b51ca72dee4ef87916ae2b9240df50.jpg",
+                "image_512": "https://.../avatar/e3b51ca72dee4ef87916ae2b9240df50.jpg",
+                "team": "T012AB3C4"
+            },
+            "is_admin": true,
+            "is_owner": false,
+            "is_primary_owner": false,
+            "is_restricted": false,
+            "is_ultra_restricted": false,
+            "is_bot": false,
+            "updated": 1502138686,
+            "is_app_user": false,
+            "has_2fa": false
+        },
+        {
+            "id": "W07QCRPA4",
+            "team_id": "T0G9PQBBK",
+            "name": "glinda",
+            "deleted": false,
+            "color": "9f69e7",
+            "real_name": "Glinda Southgood",
+            "tz": "America/Los_Angeles",
+            "tz_label": "Pacific Daylight Time",
+            "tz_offset": -25200,
+            "profile": {
+                "avatar_hash": "8fbdd10b41c6",
+                "image_24": "https://a.slack-edge.com...png",
+                "image_32": "https://a.slack-edge.com...png",
+                "image_48": "https://a.slack-edge.com...png",
+                "image_72": "https://a.slack-edge.com...png",
+                "image_192": "https://a.slack-edge.com...png",
+                "image_512": "https://a.slack-edge.com...png",
+                "image_1024": "https://a.slack-edge.com...png",
+                "image_original": "https://a.slack-edge.com...png",
+                "first_name": "Glinda",
+                "last_name": "Southgood",
+                "title": "Glinda the Good",
+                "phone": "",
+                "skype": "",
+                "real_name": "Glinda Southgood",
+                "real_name_normalized": "Glinda Southgood",
+                "display_name": "Glinda the Fairly Good",
+                "display_name_normalized": "Glinda the Fairly Good",
+                "email": "glenda@south.oz.coven"
+            },
+            "is_admin": true,
+            "is_owner": false,
+            "is_primary_owner": false,
+            "is_restricted": false,
+            "is_ultra_restricted": false,
+            "is_bot": false,
+            "updated": 1480527098,
+            "has_2fa": false
+        }
+    ],
+    "cache_ts": 1498777272,
+    "response_metadata": {
+        "next_cursor": "dXNlcjpVMEc5V0ZYTlo="
+    }
+}