Skip to content
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.

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

UploadV2 fails to recognize csv-stringify stream as a ReadableStream and errors #1586

Closed
1 of 7 tasks
elrob opened this issue Jan 30, 2023 · 9 comments
Closed
1 of 7 tasks
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented
Milestone

Comments

@elrob
Copy link

elrob commented Jan 30, 2023

(Filling out the following with as much detail as you can provide will help us solve your issue sooner.)

Packages:

Select all that apply:

  • @slack/web-api
  • @slack/rtm-api
  • @slack/webhooks
  • @slack/oauth
  • @slack/socket-mode
  • @slack/types
  • I don't know

Reproducible in:

The Slack SDK version

"slack/web-api": "^6.8.0",

Node.js runtime version

v18.12.1

OS info

Linux

Steps to reproduce:

  1. Generate a basic CSV with https://csv.js.org/stringify/
  2. This returns a node.js stream.Transform which extends Duplex which extends Readable which implements NodeJS.ReadableStream
  3. However, file upload v2 fails and it enters this if statement:
    if (file && !(typeof file === 'string' || Buffer.isBuffer(file) || (file as any) instanceof ReadStream)) {
  4. For some reason, (file as any) instanceof ReadStream must be returning false, I believe.

Expected result:

API successfully uploads the file from the stream.
It worked fine before switching to uploadV2.

Actual result:

error:

"errorMessage": "file must be a valid string path, buffer or ReadStream",
    "code": "slack_webapi_file_upload_invalid_args_error",
    "stack": [
        "Error: file must be a valid string path, buffer or ReadStream",
        "    at errorIfInvalidOrMissingFileData (/var/task/node_modules/@slack/web-api/dist/file-upload.js:234:43)",
        "    at getFileData (/var/task/node_modules/@slack/web-api/dist/file-upload.js:91:5)",
        "    at getFileUploadJob (/var/task/node_modules/@slack/web-api/dist/file-upload.js:20:28)",
        "    at WebClient.getAllFileUploads (/var/task/node_modules/@slack/web-api/dist/WebClient.js:377:71)",
        "    at WebClient.filesUploadV2 (/var/task/node_modules/@slack/web-api/dist/WebClient.js:286:40)",
        "    at file:///var/task/dist/slack-api.js:47:44",
@elrob
Copy link
Author

elrob commented Jan 30, 2023

Update, I think it may be because the check is checking if it is an fs ReadStream rather than a generic ReadableStream...

@zimeg zimeg added bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented and removed untriaged labels Jan 30, 2023
@zimeg zimeg added this to the web-api@6.8.1 milestone Jan 30, 2023
@zimeg
Copy link
Member

zimeg commented Jan 30, 2023

Hi @elrob! 👋 Thank you for sharing the steps to reproduce this error – I am seeing this same thing. Good call with the ReadStream vs generic ReadableStream check too. It appears that this is exactly the issue!

Testing this against the changes from #1577 seems to remove the error and successfully upload Readable file types. Once this PR is merged and released, this error should be gone!

@seratch
Copy link
Member

seratch commented Jan 31, 2023

Hey @elrob, it's been a while! (Hope you remember me😸) Thanks a lot for taking the time to report this!

@elrob
Copy link
Author

elrob commented Jan 31, 2023

Hey @seratch ! I wondered whether to ping you as I saw that you were a top contributor on this repo but I thought I'd be patient and polite first. 😆 Of course I remember you!

You're still the emoji for :forever_supporter: in our slack!

image

@elrob
Copy link
Author

elrob commented Jan 31, 2023

Related but not the same issue...
I noticed that this method also requires additional permission scopes:
It looks like files:read is required where it wasn't before.
Is there anything else that is newly required?

 "errorType": "Error",
    "errorMessage": "An API error occurred: missing_scope",
    "code": "slack_webapi_platform_error",
    "data": {
        "ok": false,
        "error": "missing_scope",
        "needed": "files:read",
        "provided": "chat:write,chat:write.public,files:write",
        "response_metadata": {
            "scopes": [
                "chat:write",
                "chat:write.public",
                "files:write"
            ],
            "acceptedScopes": [
                "files:read"
            ]
        }
    },

@seratch
Copy link
Member

seratch commented Jan 31, 2023

@elrob As you observed, files:read scope is necessary for the v2 method by default. The additional scope is used to make returned data compatible with files.upload's. If you don't need any file metadata properties apart from file IDs, you can pass request_file_info: false to turn the additional API calls off.

@elrob
Copy link
Author

elrob commented Jan 31, 2023

Thank you!

@filmaj
Copy link
Contributor

filmaj commented Feb 1, 2023

This was resolved in #1577 and will be shipped (shortly!) in @slack/web-api@6.8.1 - working on the release now.

@filmaj filmaj closed this as completed Feb 1, 2023
@elrob
Copy link
Author

elrob commented Feb 2, 2023

Thank you!

I can confirm that uploadV2 is now working for us with a stream and using request_file_info: false to avoid needing to add another permission scope.

Thanks for the quick response and turnaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: A confirmed bug report. Issues are confirmed when the reproduction steps are documented
Projects
None yet
Development

No branches or pull requests

4 participants