From 34e22f90dc71e0f24f30b61066ff1b446d8e2e75 Mon Sep 17 00:00:00 2001
From: Cal Herries <39073188+calherries@users.noreply.github.com>
Date: Tue, 30 Apr 2024 15:18:59 +0100
Subject: [PATCH] Move away from deprecated slack upload endpoints (#41974)
---
src/metabase/integrations/slack.clj | 89 +++++++++--
src/metabase/pulse.clj | 2 +
test/metabase/integrations/slack_test.clj | 100 +++++++------
.../slack_upload_file_response.json | 141 ++++++++++--------
4 files changed, 210 insertions(+), 122 deletions(-)
diff --git a/src/metabase/integrations/slack.clj b/src/metabase/integrations/slack.clj
index 8dd8349fbbe..14326a3580a 100644
--- a/src/metabase/integrations/slack.clj
+++ b/src/metabase/integrations/slack.clj
@@ -318,27 +318,84 @@
channel-id' (get name->id channel-id channel-id)]
channel-id'))
+(defn- poll
+ "Returns `(thunk)` if the result satisfies the `done?` predicate within the timeout and nil otherwise."
+ [{:keys [thunk done? timeout-ms interval-ms]}]
+ (let [start-time (System/currentTimeMillis)]
+ (loop []
+ (let [response (thunk)]
+ (if (done? response)
+ response
+ (let [current-time (System/currentTimeMillis)
+ elapsed-time (- current-time start-time)]
+ (if (>= elapsed-time timeout-ms)
+ nil ; timeout reached
+ (do
+ (Thread/sleep interval-ms)
+ (recur)))))))))
+
+(defn complete!
+ "Completes the file upload to a Slack channel by calling the `files.completeUploadExternal` endpoint, and polls the
+ same endpoint until the file is uploaded to the channel. Returns the URL of the uploaded file."
+ [& {:keys [channel-id file-id filename]}]
+ (let [complete! (fn []
+ (POST "files.completeUploadExternal"
+ {:query-params {:files (json/generate-string [{:id file-id, :title filename}])
+ :channel_id channel-id}}))
+ complete-response (try
+ (complete!)
+ (catch Throwable e
+ ;; If file upload fails with a "not_in_channel" error, we join the channel and try again.
+ ;; This is expected to happen the first time a Slack subscription is sent.
+ (if (= "not_in_channel" (:error-code (ex-data e)))
+ (do (join-channel! channel-id)
+ (complete!))
+ (throw (ex-info (ex-message e)
+ (assoc (ex-data e) :channel-id channel-id, :filename filename))))))
+ ;; Step 4: Poll the endpoint to confirm the file is uploaded to the channel
+ uploaded-to-channel? (fn [response]
+ (boolean (some-> response :files first :shares not-empty)))
+ _ (when-not (or
+ (uploaded-to-channel? complete-response)
+ (poll {:thunk complete!
+ :done? uploaded-to-channel?
+ ;; Cal 2024-04-30: this typically takes 1-2 seconds to succeed.
+ ;; If it takes more than 10 seconds, something else is wrong and we should abort.
+ :timeout-ms 3000
+ :interval-ms 500}))
+ (throw (ex-info "Timed out waiting to confirm the file was uploaded to a Slack channel."
+ {:channel-id channel-id, :filename filename})))]
+ (get-in complete-response [:files 0 :url_private])))
+
+(defn- get-upload-url! [filename file]
+ (POST "files.getUploadURLExternal" {:query-params {:filename filename
+ :length (count file)}}))
+
+
+(defn- upload-file-to-url! [upload-url file]
+ (let [response (http/post upload-url {:multipart [{:name "file", :content file}]})]
+ (if (= (:status response) 200)
+ response
+ (throw (ex-info "Failed to upload file to Slack:" (select-keys response [:status :body]))))))
+
(mu/defn upload-file!
- "Calls Slack API `files.upload` endpoint and returns the URL of the uploaded file."
+ "Calls Slack API `files.getUploadURLExternal` and `files.completeUploadExternal` endpoints to upload a file and returns
+ the URL of the uploaded file."
[file :- NonEmptyByteArray
filename :- ms/NonBlankString
channel-id :- ms/NonBlankString]
{:pre [(slack-configured?)]}
- (let [request {:multipart [{:name "file", :content file}
- {:name "filename", :content filename}
- {:name "channels", :content channel-id}]}
- response (try
- (POST "files.upload" request)
- (catch Throwable e
- ;; If file upload fails with a "not_in_channel" error, we join the channel and try again.
- ;; This is expected to happen the first time a Slack subscription is sent.
- (if (= "not_in_channel" (:error-code (ex-data e)))
- (do (-> channel-id
- (maybe-lookup-id (slack-cached-channels-and-usernames))
- join-channel!)
- (POST "files.upload" request))
- (throw e))))]
- (u/prog1 (get-in response [:file :url_private])
+ ;; TODO: we could make uploading files a lot faster by uploading the files in parallel.
+ ;; Steps 1 and 2 can be done for all files in parallel, and step 3 can be done once at the end.
+ (let [;; Step 1: Get the upload URL using files.getUploadURLExternal
+ {:keys [upload_url file_id]} (get-upload-url! filename file)
+ ;; Step 2: Upload the file to the obtained upload URL
+ _ (upload-file-to-url! upload_url file)
+ ;; Step 3: Complete the upload using files.completeUploadExternal
+ file-url (complete! {:channel-id (maybe-lookup-id channel-id (slack-cached-channels-and-usernames))
+ :file-id file_id
+ :filename filename})]
+ (u/prog1 file-url
(log/debug "Uploaded image" <>))))
(mu/defn post-chat-message!
diff --git a/src/metabase/pulse.clj b/src/metabase/pulse.clj
index 43ff4778239..df0774978be 100644
--- a/src/metabase/pulse.clj
+++ b/src/metabase/pulse.clj
@@ -35,6 +35,8 @@
(:import
(clojure.lang ExceptionInfo)))
+(set! *warn-on-reflection* true)
+
;;; ------------------------------------------------- PULSE SENDING --------------------------------------------------
(defn- is-card-empty?
diff --git a/test/metabase/integrations/slack_test.clj b/test/metabase/integrations/slack_test.clj
index ee96cefc1b8..f812e882488 100644
--- a/test/metabase/integrations/slack_test.clj
+++ b/test/metabase/integrations/slack_test.clj
@@ -174,56 +174,68 @@
(testing "upload-file!"
(let [image-bytes (.getBytes "fake-picture")
filename "wow.gif"
- channel-id "C13372B6X"]
- (http-fake/with-fake-routes {#"^https://slack.com/api/files\.upload.*"
- (fn [_] (mock-200-response (slurp "./test_resources/slack_upload_file_response.json")))}
+ channel-id "C13372B6X"
+ upload-url "https://files.slack.com/upload/v1/CwABAAAAWgoAAZnBg"
+ fake-upload-routes {#"^https://slack.com/api/files\.getUploadURLExternal.*"
+ (fn [_] (mock-200-response {:ok true
+ :upload_url upload-url
+ :file_id "DDDDDDDDD-EEEEEEEEE"}))
+
+ upload-url
+ (fn [_] (mock-200-response "OK"))
+
+ #"^https://slack.com/api/files\.completeUploadExternal.*"
+ (fn [_] (mock-200-response (slurp "./test_resources/slack_upload_file_response.json")))}]
+ (http-fake/with-fake-routes fake-upload-routes
(tu/with-temporary-setting-values [slack-token "test-token"
slack-app-token nil]
- (is (= "https://files.slack.com/files-pri/T078VLEET-F017C3TSBK6/wow.gif"
+ (is (= "https://files.slack.com/files-pri/DDDDDDDDD-EEEEEEEEE/wow.gif"
(slack/upload-file! image-bytes filename channel-id)))))
;; Slack app token requires joining the `metabase_files` channel before uploading a file
- (http-fake/with-fake-routes {#"^https://slack.com/api/files\.upload.*"
- (fn [_] (mock-200-response (slurp "./test_resources/slack_upload_file_response.json")))
- #"^https://slack.com/api/conversations\.join.*"
- (fn [_] (mock-200-response (slurp "./test_resources/slack_conversations_join_response.json")))}
+ (http-fake/with-fake-routes
+ (assoc fake-upload-routes
+ #"^https://slack.com/api/conversations\.join.*"
+ (fn [_] (mock-200-response (slurp "./test_resources/slack_conversations_join_response.json"))))
(tu/with-temporary-setting-values [slack-token nil
slack-app-token "test-token"]
- (is (= "https://files.slack.com/files-pri/T078VLEET-F017C3TSBK6/wow.gif"
- (slack/upload-file! image-bytes filename channel-id)))))))
- (testing (str "upload-file! will attempt to join channels by internal slack id"
- " but we can continue to use the channel name for posting")
- (let [filename "wow.gif"
- channel-id "metabase_files"
- slack-id "CQXPZKNQ3RK"
- joined? (atom false)
- channel-info [{:display-name "#random",
- :name "random",
- :id "CT2FNGZSRPL",
- :type "channel"}
- {:display-name "#general",
- :name "general",
- :id "C4Q6LXLRA46",
- :type "channel"}
- {:display-name "#metabase_files",
- :name channel-id,
- ;; must look up "metabase_files" and find the id below
- :id slack-id,
- :type "channel"}]]
- (tu/with-temporary-setting-values [slack/slack-app-token "slack-configured?"
- slack/slack-cached-channels-and-usernames
- {:channels channel-info}]
- (with-redefs [slack/POST (fn [endpoint payload]
- (case endpoint
- "files.upload"
- (if @joined?
- {:file {:url_private filename}}
- (throw (ex-info "Not in that channel"
- {:error-code "not_in_channel"})))
- "conversations.join"
- (reset! joined? (= (-> payload :form-params :channel)
- slack-id))))]
- (slack/upload-file! (.getBytes "fake-picture") filename channel-id)
- (is @joined? (str "Did not attempt to join with slack-id " slack-id)))))))
+ (is (= "https://files.slack.com/files-pri/DDDDDDDDD-EEEEEEEEE/wow.gif"
+ (slack/upload-file! image-bytes filename channel-id))))
+ (testing (str "upload-file! will attempt to join channels by internal slack id"
+ " but we can continue to use the channel name for posting")
+ (let [filename "wow.gif"
+ channel-id "metabase_files"
+ slack-id "CQXPZKNQ3RK"
+ joined? (atom false)
+ channel-info [{:display-name "#random",
+ :name "random",
+ :id "CT2FNGZSRPL",
+ :type "channel"}
+ {:display-name "#general",
+ :name "general",
+ :id "C4Q6LXLRA46",
+ :type "channel"}
+ {:display-name "#metabase_files",
+ :name channel-id,
+ ;; must look up "metabase_files" and find the id below
+ :id slack-id,
+ :type "channel"}]
+ post (var-get #'slack/POST)]
+ (with-redefs [slack/POST (fn [endpoint payload]
+ (case endpoint
+ "files.completeUploadExternal"
+ (if @joined?
+ (json/parse-string (slurp "./test_resources/slack_upload_file_response.json") keyword)
+ (throw (ex-info "Not in that channel"
+ {:error-code "not_in_channel"})))
+ "conversations.join"
+ (reset! joined? (= (-> payload :form-params :channel)
+ slack-id))
+ (post endpoint payload)))]
+ (tu/with-temporary-setting-values [slack/slack-app-token "slack-configured?"
+ slack/slack-cached-channels-and-usernames
+ {:channels channel-info}]
+ (slack/upload-file! (.getBytes "fake-picture") filename channel-id)
+ (is @joined? (str "Did not attempt to join with slack-id " slack-id))))))))))
(deftest maybe-lookup-id-test
(let [f (var-get #'slack/maybe-lookup-id)]
diff --git a/test_resources/slack_upload_file_response.json b/test_resources/slack_upload_file_response.json
index b22a27bf5e3..930cf3a95cc 100644
--- a/test_resources/slack_upload_file_response.json
+++ b/test_resources/slack_upload_file_response.json
@@ -1,64 +1,81 @@
{
- "ok" : true,
- "file" : {
- "ims" : [ ],
- "thumb_80" : "https://files.slack.com/files-tmb/T078VLEET-F017C3TSBK6-4f9ea75cc9/wow_80.png",
- "thumb_360_h" : 152,
- "channels" : [ "C94712B6X" ],
- "editable" : false,
- "is_external" : false,
- "thumb_160" : "https://files.slack.com/files-tmb/T078VLEET-F017C3TSBK6-4f9ea75cc9/wow_160.png",
- "original_w" : 480,
- "is_starred" : false,
- "thumb_360_gif" : "https://files.slack.com/files-tmb/T078VLEET-F017C3TSBK6-4f9ea75cc9/wow_360.gif",
- "url_private_download" : "https://files.slack.com/files-pri/T078VLEET-F017C3TSBK6/download/wow.gif",
- "name" : "wow.gif",
- "permalink" : "https://metaboat.slack.com/files/U016YSX55QW/F017C3TSBK6/wow.gif",
- "username" : "",
- "mode" : "hosted",
- "thumb_480_h" : 203,
- "created" : 1594847102,
- "display_as_bot" : false,
- "thumb_480" : "https://files.slack.com/files-tmb/T078VLEET-F017C3TSBK6-4f9ea75cc9/wow_480.png",
- "deanimate_gif" : "https://files.slack.com/files-tmb/T078VLEET-F017C3TSBK6-4f9ea75cc9/wow_deanimate_gif.png",
- "mimetype" : "image/gif",
- "size" : 1929620,
- "title" : "wow",
- "is_public" : true,
- "id" : "F017C3TSBK6",
- "original_h" : 203,
- "comments_count" : 0,
- "external_type" : "",
- "thumb_480_w" : 480,
- "thumb_360_w" : 360,
- "thumb_tiny" : "AwAUADDOJ5qXeViABOSeuaYkLycqufxp8kTJGC/BzwM0gJrZQR8zZ+ppLnK5Az+eadZsPMU4HAOfpSvGbn/VFSfQnFJDZBA+HzV0zKFJPaoBZSxgs5QYGcbuaV9u00CY2yOFJqo7tIxZjkmrdn9xvpVKmBJvZEKqcBhg09ZGEisvBJzxUTdBTx96P/PemgZtSjMLPnkKTWdAv2gM8hOd2MDgVpSf8er/AO4f5Vn2P+qb/fNSB//Z",
- "public_url_shared" : false,
- "thumb_360" : "https://files.slack.com/files-tmb/T078VLEET-F017C3TSBK6-4f9ea75cc9/wow_360.png",
- "groups" : [ ],
- "filetype" : "gif",
- "url_private" : "https://files.slack.com/files-pri/T078VLEET-F017C3TSBK6/wow.gif",
- "pretty_type" : "GIF",
- "has_rich_preview" : false,
- "timestamp" : 1594847102,
- "thumb_480_gif" : "https://files.slack.com/files-tmb/T078VLEET-F017C3TSBK6-4f9ea75cc9/wow_480.gif",
- "user" : "U016YSX55QW",
- "thumb_64" : "https://files.slack.com/files-tmb/T078VLEET-F017C3TSBK6-4f9ea75cc9/wow_64.png",
- "shares" : {
- "public" : {
- "C94712B6X" : [ {
- "reply_users" : [ ],
- "reply_users_count" : 0,
- "reply_count" : 0,
- "ts" : "1594847103.002500",
- "channel_name" : "wow",
- "team_id" : "T078VLEET"
- } ]
- }
- },
- "permalink_public" : "https://slack-files.com/T078VLEET-F017C3TSBK6-d9c051e26f"
- },
- "warning" : "superfluous_charset",
- "response_metadata" : {
- "warnings" : [ "superfluous_charset" ]
- }
+ "ok": true,
+ "files": [
+ {
+ "thumb_1024_w": 1024,
+ "ims": [],
+ "thumb_1024_h": 734,
+ "has_more_shares": false,
+ "thumb_1024": "https://files.slack.com/files-tmb/DDDDDDDDD-EEEEEEEEE-5e53e73e7a/image_1024.png",
+ "thumb_80": "https://files.slack.com/files-tmb/DDDDDDDDD-EEEEEEEEE-5e53e73e7a/image_80.png",
+ "thumb_360_h": 258,
+ "channels": [
+ "C0713QAHUAX"
+ ],
+ "editable": false,
+ "is_external": false,
+ "thumb_160": "https://files.slack.com/files-tmb/DDDDDDDDD-EEEEEEEEE-5e53e73e7a/image_160.png",
+ "thumb_960": "https://files.slack.com/files-tmb/DDDDDDDDD-EEEEEEEEE-5e53e73e7a/image_960.png",
+ "thumb_960_w": 960,
+ "original_w": 1200,
+ "is_starred": false,
+ "url_private_download": "https://files.slack.com/files-pri/DDDDDDDDD-EEEEEEEEE/download/wow.gif",
+ "name": "wow.gif",
+ "permalink": "https://metaboat.slack.com/files/UUUUUUUUUU/EEEEEEEEE/wow.gif",
+ "username": "",
+ "mode": "hosted",
+ "thumb_480_h": 344,
+ "created": 1714470800,
+ "display_as_bot": false,
+ "thumb_480": "https://files.slack.com/files-tmb/DDDDDDDDD-EEEEEEEEE-5e53e73e7a/image_480.png",
+ "mimetype": "image/png",
+ "size": 89612,
+ "title": "wow.gif",
+ "media_display_type": "unknown",
+ "thumb_800": "https://files.slack.com/files-tmb/DDDDDDDDD-EEEEEEEEE-5e53e73e7a/image_800.png",
+ "is_public": true,
+ "id": "EEEEEEEEE",
+ "original_h": 860,
+ "comments_count": 0,
+ "external_type": "",
+ "thumb_480_w": 480,
+ "thumb_360_w": 360,
+ "thumb_720_h": 516,
+ "thumb_720_w": 720,
+ "thumb_tiny": "AwAiADDTNNYkKee1KTTW+6fpQBD5j/3jR5j/AN40z/P+eaKoB/mP/eNPidmfBYkVDUsH3ifagCc01vun6U401gSpHtUgVqKf5L+35/8A1qPJf2/P/wCtVAMqaD7pPvTPJf2/P/61SxKVXB9aTAfRRRSAKKKKACiiigD/2Q==",
+ "public_url_shared": false,
+ "user_team": "T078VCLCR",
+ "thumb_360": "https://files.slack.com/files-tmb/DDDDDDDDD-EEEEEEEEE-5e53e73e7a/image_360.png",
+ "groups": [],
+ "filetype": "png",
+ "url_private": "https://files.slack.com/files-pri/DDDDDDDDD-EEEEEEEEE/wow.gif",
+ "thumb_720": "https://files.slack.com/files-tmb/DDDDDDDDD-EEEEEEEEE-5e53e73e7a/image_720.png",
+ "thumb_960_h": 688,
+ "pretty_type": "PNG",
+ "has_rich_preview": false,
+ "timestamp": 1714470800,
+ "thumb_800_w": 800,
+ "user": "UUUUUUUUUU",
+ "thumb_64": "https://files.slack.com/files-tmb/DDDDDDDDD-EEEEEEEEE-5e53e73e7a/image_64.png",
+ "shares": {
+ "public": {
+ "C0713QAHUAX": [
+ {
+ "reply_users": [],
+ "reply_users_count": 0,
+ "reply_count": 0,
+ "ts": "1714470801.942809",
+ "channel_name": "some-channel",
+ "team_id": "T078VCLCR",
+ "share_user_id": "UUUUUUUUUU",
+ "source": "UNKNOWN"
+ }
+ ]
+ }
+ },
+ "thumb_800_h": 573,
+ "permalink_public": "https://slack-files.com/DDDDDDDDD-EEEEEEEEE-d533f610b6",
+ "file_access": "visible"
+ }
+ ]
}
\ No newline at end of file
--
GitLab