Expose private File Upload V2 methods to support multiple file uploads in a single message #1376
Conversation
| type mockGetUploadURLExternalHttpClient struct { | ||
| ResponseStatus int | ||
| ResponseBody []byte | ||
| } | ||
|
|
||
| func (m *mockGetUploadURLExternalHttpClient) Do(req *http.Request) (*http.Response, error) { | ||
| if req.URL.Path != "files.getUploadURLExternal" { | ||
| return nil, fmt.Errorf("invalid path: %s", req.URL.Path) | ||
| } | ||
|
|
||
| return &http.Response{ | ||
| StatusCode: m.ResponseStatus, | ||
| Body: io.NopCloser(bytes.NewBuffer(m.ResponseBody)), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
I considered following the existing once.Do(startServer) approach for testing, but found that simply mocking Client.httpclient would be sufficient. Therefore, I decided to mock it instead.
| Files: []FileSummary{{ | ||
| ID: u.FileID, | ||
| Title: params.Title, | ||
| }}, |
There was a problem hiding this comment.
For CompleteUploadURLExternal, I've not only made it public but also updated its signature so that multiple fileID parameters (with IDs and titles) can be specified, aligning it more closely with Slack’s official API.
|
thanks for doing this. I just was revisiting a way to grab the urls so I could publish a single "gallery style" message and this probably will fix much of that by providing back the upload urls back. cheers 🙇 |
There was a problem hiding this comment.
@YutoKashiwagi thank you so much for sending this and providing a well reasoned approach here.
Also very smart to match with the ability to send multiple files per the Slack API as part of opening this up to users of the library.
There's a tiny change I think is worth doing and once that's out of the way I will ✅ this.
files.go
Outdated
| // Slack API docs: https://api.slack.com/methods/files.getUploadURLExternal | ||
| func (api *Client) GetUploadURLExternalContext(ctx context.Context, params GetUploadURLExternalParameters) (*GetUploadURLExternalResponse, error) { | ||
| if params.FileName == "" { | ||
| return nil, fmt.Errorf("GetUploadURLExternalContext: fileName cannot be empty") |
There was a problem hiding this comment.
Do you mind removing GetUploadURLExternalContext from the error? Happy for the message to indicate where it's at but this format doesn't quite fit with everything else.
I am aware we already have file.upload.v2: in fmt.Errorf in this file so happy if you want to change to be sort of that format but what I'd really prefer is to not have the name of the function in the error returned.
There was a problem hiding this comment.
Thank you. I’ve updated it to include the API name instead of the function name.
|
thank you for this! I'm not seeing a release containing it, so do you have a general timeframe @nlopes you plan on cutting a release? If I missed something on this already being said, my apologies 🙇🏻 |
I intend to cut a breaking change release this coming weekend, stay tuned 🙏 |
|
Quick follow-up. I'm trying to use this to handle multiple generated images being uploaded at once, but I think the part I'm missing for this to be useful is Is there further changes I'm missing for
If this is intended to stay private, I can do the steps myself, but with all the release changes in preview, I wanted to follow-up in the context where change was made. |
Hi, thank you for maintaining slack-go!
Currently,
UploadFileV2Contextworks by sequentially calling three private methods in the following flow:Because these methods are private, developers can only upload one file per message via
UploadFileV2Context. However, the Slack API itself supports uploading multiple files in a single message by running these steps for each file and then calling completeUploadURLExternal for all file IDs together.To unlock this functionality while still using slack-go, I’ve made the following changes in this PR:
getUploadURLExternalandcompleteUploadExternalpublicuploadToURLremains private for now. This method is simply a regular POST request handler, not part of Slack’s official API. Since making getUploadURLExternal and completeUploadExternal public already addresses the main use case of uploading multiple files, this might be sufficient.Thank you for considering this proposal. Being able to handle multi-file uploads in a single Slack message through slack-go would be extremely helpful. If you have any suggestions or an alternative approach, I’d love to hear them!
Related Issues
Pull Request Guidelines
These are recommendations for pull requests.
They are strictly guidelines to help manage expectations.
PR preparation
Run
make pr-prepfrom 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: