Skip to content

fix(web-api): complete file upload v2 calls if absolute urls are not allowed#2196

Merged
zimeg merged 6 commits intomainfrom
zimeg-fix-web-api-file-upload-v2-absolute-url
Mar 24, 2025
Merged

fix(web-api): complete file upload v2 calls if absolute urls are not allowed#2196
zimeg merged 6 commits intomainfrom
zimeg-fix-web-api-file-upload-v2-absolute-url

Conversation

@zimeg
Copy link
Copy Markdown
Member

@zimeg zimeg commented Mar 20, 2025

Summary

This PR fixes an issue where setting the allowAbsoluteUrls option to true causes the filesUploadV2 method to error!

Some investigating points at the "https://files.slack.com/..." URL gathered to POST files to having a different base URL than other API request at "https://slack.com/api/..." 🤖

Preview

The changes of this PR make the following example upload as expected:

import webapi, { LogLevel } from "@slack/web-api";

const client = new webapi.WebClient(process.env.SLACK_BOT_TOKEN, {
  allowAbsoluteUrls: false,
  logLevel: LogLevel.DEBUG,
});

const _response = await client.filesUploadV2({
  file: "./path/to/example.png",
  filename: "example.png",
  channel_id: process.env.SLACK_CHANNEL_ID,
});

Notes

Changes were made to derive the absolute URL to use in requests before the internal makeRequest function. This was done since both .apiCall and .filesUploadV2 make requests using different URLs but the same axios client 👾

Requirements

@zimeg zimeg added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch tests M-T: Testing work only pkg:web-api applies to `@slack/web-api` labels Mar 20, 2025
@zimeg zimeg added this to the [email protected] milestone Mar 20, 2025
@zimeg zimeg self-assigned this Mar 20, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 20, 2025 •
edited
Loading

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
2080 1 2079 0
View the full list of 2 ❄️ flaky tests 
should log warning and throw error if timeout exceeded WebClient has an option to override the Axios timeout value should log warning and throw error if timeout exceeded

Flake rate in main: 11.11% (Passed 16 times, Failed 2 times)

Stack Traces | 0.075s run time 
expected 'slack_webapi_platform_error' to equal 'slack_webapi_request_error'
should detect multiple consumption FileStateStore should detect multiple consumption

Flake rate in main: 22.22% (Passed 14 times, Failed 4 times)

Stack Traces | 2.96s run time 
Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (D:\a\node-slack-sdk\node-slack-sdk\packages\oauth\src\state-stores\file-state-store.spec.ts)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

WilliamBergamin
WilliamBergamin requested changes Mar 20, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this refactor, I believe it properly isolates the responsibility of apiCall and makeRequest, elegant fix 💯

But these changes affect a critical section of the web-api package (logic used by every user), this is why I left a comment highlighting the importance of properly scoping this PR

Comment thread packages/web-api/src/WebClient.spec.ts
@zimeg zimeg requested a review from WilliamBergamin March 20, 2025 16:04
@zimeg
Copy link
Copy Markdown
Member Author

zimeg commented Mar 20, 2025

@WilliamBergamin Thanks so much for the review and excellent suggestions as always ✨

I updated the test changes to be more focused on the filesUploadV2 and allowAbsoluteUrls changes, though did find testing this at the WebClient level to seem most right? Let me know if other updates seem best!

These changes are also tested to be working in production and development, with both standard API methods and filesUploadV2 and various allowAbsoluteUrls options for a bit more confidence 🙏

WilliamBergamin
WilliamBergamin approved these changes Mar 20, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@WilliamBergamin WilliamBergamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good 💯 thanks for catching this 🥇

@zimeg
Copy link
Copy Markdown
Member Author

zimeg commented Mar 24, 2025

@WilliamBergamin Thanks once more for these reviews and improvements to testing in the meantime! I'll merge this now to follow up with a patch soon 🚢 💨

@zimeg zimeg merged commit 0903f5f into main Mar 24, 2025
57 checks passed
@zimeg zimeg deleted the zimeg-fix-web-api-file-upload-v2-absolute-url branch March 24, 2025 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@WilliamBergamin WilliamBergamin WilliamBergamin approved these changes

@mwbrooks mwbrooks Awaiting requested review from mwbrooks

Assignees

@zimeg zimeg

Labels

bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented pkg:web-api applies to `@slack/web-api` semver:patch tests M-T: Testing work only

Projects

None yet

Milestone

[email protected]

Development

Successfully merging this pull request may close these issues.

2 participants

@zimeg @WilliamBergamin