Skip to content

Commit

Permalink
[backport to 48] Move away from deprecated slack upload endpoints (#4…
Browse files Browse the repository at this point in the history 
…1974) (#42037)
  • Loading branch information
@calherries
calherries committed Apr 30, 2024
1 parent 00faf54 commit 7a5ccd8
Show file tree
Hide file tree
Showing 4 changed files with 211 additions and 123 deletions.
91 changes: 74 additions & 17 deletions src/metabase/integrations/slack.clj
View file
Expand Up @@ -317,28 +317,85 @@
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])
(log/debug (trs "Uploaded image") <>))))
;; 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!
"Calls Slack API `chat.postMessage` endpoint and posts a message to a channel. `attachments` should be serialized
Expand Down
2 changes: 2 additions & 0 deletions src/metabase/pulse.clj
View file
Expand Up @@ -34,6 +34,8 @@
(:import
(clojure.lang ExceptionInfo)))

(set! *warn-on-reflection* true)

;;; ------------------------------------------------- PULSE SENDING --------------------------------------------------

(defn- is-card-empty?
Expand Down
100 changes: 56 additions & 44 deletions test/metabase/integrations/slack_test.clj
View file
Expand Up @@ -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)]
Expand Down
141 changes: 79 additions & 62 deletions test_resources/slack_upload_file_response.json
View file
@@ -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"
}
]
}

0 comments on commit 7a5ccd8

Please sign in to comment.