Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
zimeg merged 6 commits into from
Mar 24, 2025
Merged

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

zimeg merged 6 commits into from
Mar 24, 2025

Conversation

@zimeg
Copy link
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 web-api@7.9.1 milestone Mar 20, 2025
@zimeg zimeg self-assigned this Mar 20, 2025
@codecov
Copy link

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
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

@zimeg zimeg requested a review from WilliamBergamin March 20, 2025 16:04
@zimeg
Copy link
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
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
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

web-api@7.9.1

Development

Successfully merging this pull request may close these issues.

3 participants

@zimeg @WilliamBergamin