slack-go / slack

Slack API in Go, originally by @nlopes; Maintainers needed, contact @parsley42
https://pkg.go.dev/github.com/slack-go/slack
BSD 2-Clause "Simplified" License
4.63k stars 1.12k forks source link

Fix UploadFileV2 to return File instead of FileSummary #1296

Closed vaijab closed 1 week ago

vaijab commented 1 month ago

By returning FileSummary instead of a File object, in most cases, we have to make another call to get file info, which requires files:read permission scope. This in fact breaks many existing apps.

This change makes the Go sdk somewhat more in-line with other SDKs.

Pull Request Guidelines

These are recommendations for pull requests. They are strictly guidelines to help manage expectations.

PR preparation

Run make pr-prep from the root of the repository to run formatting, linting and tests.

Should this be an issue instead
API changes

Since API changes have to be maintained they undergo a more detailed review and are more likely to require changes.

Examples of API changes that do not meet guidelines:
lorenzoaiello commented 1 month ago

Thanks for opening a pull request @vaijab

I don't conceptually have an issue with returning more information from the UploadFileV2 function if it was available, but the FileSummary struct accurately represents the response from the files.completeUploadExternal API method which is the last call made in the new upload flow.

The other API Method called in the same UploadFileV2 function (files.getUploadURLExternal) also doesn't provide any unique information worth retaining.

The Slack documentation for the file.getUploadURLExternal has an information banner in the Usage info section that states:

If you have migrated to the v2 method, note that this method requires both files:write and files:read scopes. If your existing apps have only the files:write scope for uploading files, you must add files:read, then re-install the app to issue an updated token. Refer to files.upload v2 in WebClient/AsyncWebClient for more information.

This was subsequently retracted in v3.23.0 but only because the internal calls to files.info were eliminated meaning that files:read was no longer required to use the function in the SDK instead requiring it to be explicitly called but not fundamentally changing how the Slack API behaves in this respect.

Unfortunately, I don't think it's going to be possible to circumvent the need for the files:read scope to get the information you are after (without knowing exactly what you are after, but knowing you want more than is currently returned).