Add files.upload v2 support #1272

seratch · 2022-10-03T08:57:34Z

Summary

This pull request adds a new way to upload files to Slack.

The legacy files.upload API endpoint now has severe performance issues as described in the following reports:

Slack platform team decided to unlock a new way to upload files using the following endpoints:

Call https://api.slack.com/methods/files.getUploadURLExternal to fetch the URL to upload per file
Use the URL https://files.slack.com/upload/v1/*** to upload the file content per file
Call https://api.slack.com/methods/files.completeUploadExternal to finish the upload this time and share the files in a channel only once as the last step

This pull request adds supports for the new endpoints, so that now the low-level APIs are available for developers. Having said that, going through the above process for uploading files requires many lines of code on 3rd party app side. Also, following all the steps can be confusing for developers.

For this reason, in addition to the low-level API supports, I propose to add WebClient#files_upload_v2 method as a wrapper of the whole file-upload operation. Here are the code examples demonstrating how it works:

import os
from slack_sdk import WebClient
client = WebClient(token=os.environ["SLACK_BOT_TOKEN"])

# Legacy way
response = client.files_upload(
    file="./logo.png",
    title="New company logo",
    channels=["C12345"],
    initial_comment="Here is the latest version of our new company logo :wave:",
)
response.get("file")  # returns the full metadata of the uploaded file

# New way - the same parameters works in most cases
response = client.files_upload_v2(
    file="./logo.png",
    title="New company logo",
    # Note that channels still works but going with channel="C12345" is recommended
    # channels["C111", "C222"] is no longer supported. In this case, an exception can be thrown 
    channels=["C12345"],
    initial_comment="Here is the latest version of our new company logo :wave:",
)
response.get("file")  # returns the full metadata of the uploaded file

# New way with multiple files!
response = client.files_upload_v2(
    file_uploads=[
        {
            "file": "./logo.png",
            "title": "New company logo",
        },
        {
            "content": "Minutes ....",
            "filename": "team-meeting-minutes-2022-03-01.md",
            "title": "Team meeting minutes (2022-03-01)",
        },
    ],
    channel="C12345",
    initial_comment="Here is the latest version of our new company logo :wave:",
)
response.get("files")  # returns the full metadata of all the uploaded files

Category (place an `x` in each of the `[ ]`)

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've run python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

codecov · 2022-10-03T09:06:16Z

Codecov Report

Merging #1272 (1d0eed1) into main (2b40608) will decrease coverage by 0.37%.
The diff coverage is 21.27%.

@@            Coverage Diff             @@
##             main    #1272      +/-   ##
==========================================
- Coverage   86.07%   85.70%   -0.38%     
==========================================
  Files          84       84              
  Lines        7779     7826      +47     
==========================================
+ Hits         6696     6707      +11     
- Misses       1083     1119      +36

Impacted Files	Coverage Δ
slack_sdk/web/legacy_client.py	`83.33% <21.27%> (-2.91%)`	⬇️
slack_sdk/socket_mode/builtin/internals.py	`74.24% <0.00%> (+0.42%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

seratch

comments for reviewers

seratch · 2022-10-03T08:59:55Z

docs-src-v2/basic_usage.rst

-  response = client.files_upload(
-    channels="C3UKJTQAC",
-    file="files.pdf",
+  response = client.files_upload_v2(


I haven't yet generated the page. We will update the live site once we release a new version including the change.

seratch · 2022-10-03T09:00:21Z

integration_tests/web/test_web_client.py

@@ -242,7 +242,7 @@ async def test_uploading_binary_files_async(self):
        )
        self.assertIsNotNone(upload)

-        deletion = client.files_delete(file=upload["file"]["id"])
+        deletion = await client.files_delete(file=upload["file"]["id"])


Just improvements to existing test code

seratch · 2022-10-03T09:01:03Z

slack_sdk/web/async_client.py

@@ -2952,7 +2955,7 @@ async def files_upload(
        self,
        *,
        file: Optional[Union[str, bytes, IOBase]] = None,
-        content: Optional[str] = None,
+        content: Optional[Union[str, bytes]] = None,


While touching the code this time, I noticed that this type hint has been wrong

Good catch 💯

seratch · 2022-10-03T09:04:12Z

slack_sdk/web/async_client.py

+        # To upload multiple files at a time
+        upload_files: Optional[List[Dict[str, Any]]] = None,
+        channel: Optional[str] = None,
+        channels: Optional[Union[str, Sequence[str]]] = None,  # having n channels is no longer supported


I intentionally didn't remove this keyword argument. Unlike the Java SDK, there is no compiler detection in Python. Thus, removing the argument name that existed in files.upload (v1) can result in many developers' confusion.

That being said, sharing files in multiple channels is no longer supported. So, I've added runtime validation below to inform the breaking change.

I feel like we should go ahead and remove this parameter in order to make it unambiguous to the developer while they are writing code that sharing files with multiple channels is not supported by this method.

Maybe something as follows could be implemented to support runtime validation, without including the parameter in the method definition

if 'channels' in kwargs: raise e.SlackRequestError("Sharing files with multiple channels is no longer supported, share files in each channel separately")

let me know what you think of this approach

@WilliamBergamin Thanks. I think it's fine to move this argument to kwargs, but I still would like to support it as long as the list has only one item. I am pretty sure that most of the existing code has only a single channel in this parameter. Perhaps, we can add a warning logging even for the single item use case, but we can allow it for smoother migration.

seratch · 2022-10-03T09:05:04Z

slack_sdk/web/async_client.py

+            (isinstance(channels, (list, Tuple)) and len(channels) > 1)
+            or (isinstance(channels, str) and len(channels.split(",")) > 1)
+        ):
+            raise e.SlackRequestError("Sharing files with multiple channels is no longer supported")


Added runtime validation for the unsupported pattern

seratch · 2022-10-03T09:06:30Z

slack_sdk/web/async_client.py

+        ):
+            raise e.SlackRequestError("Sharing files with multiple channels is no longer supported")
+        if filetype is not None:
+            warnings.warn("filetype is no longer supported. Please remove it from the arguments.")


I hesitated to raise an exception for this pattern but if others strongly believe that we should do, I'm fine to change this. But I don't think this can be so beneficial particularly for the devs who migrate existing code.

Maybe kwargs validation could be used here instead of including filetype as a parameter in the method definition, but let me know what you think

if 'filetype' in kwargs: warnings.warn("filetype is no longer supported. Please remove it from the arguments.")

thanks, this is fine

slack_sdk/web/async_client.py

WilliamBergamin

This is really good and slick work 💯 I left a few ideas let me know what you think

WilliamBergamin · 2022-10-04T00:05:46Z

slack_sdk/web/async_client.py

@@ -2952,7 +2955,7 @@ async def files_upload(
        self,
        *,
        file: Optional[Union[str, bytes, IOBase]] = None,
-        content: Optional[str] = None,
+        content: Optional[Union[str, bytes]] = None,


Good catch 💯

WilliamBergamin · 2022-10-04T14:47:22Z

slack_sdk/web/async_client.py

+        # To upload multiple files at a time
+        upload_files: Optional[List[Dict[str, Any]]] = None,
+        channel: Optional[str] = None,
+        channels: Optional[Union[str, Sequence[str]]] = None,  # having n channels is no longer supported


I feel like we should go ahead and remove this parameter in order to make it unambiguous to the developer while they are writing code that sharing files with multiple channels is not supported by this method.

Maybe something as follows could be implemented to support runtime validation, without including the parameter in the method definition

if 'channels' in kwargs: raise e.SlackRequestError("Sharing files with multiple channels is no longer supported, share files in each channel separately")

let me know what you think of this approach

WilliamBergamin · 2022-10-04T14:53:02Z

slack_sdk/web/async_client.py

+        ):
+            raise e.SlackRequestError("Sharing files with multiple channels is no longer supported")
+        if filetype is not None:
+            warnings.warn("filetype is no longer supported. Please remove it from the arguments.")


Maybe kwargs validation could be used here instead of including filetype as a parameter in the method definition, but let me know what you think

if 'filetype' in kwargs: warnings.warn("filetype is no longer supported. Please remove it from the arguments.")

slack_sdk/web/async_client.py

slack_sdk/web/legacy_client.py

seratch

will update the code shortly

seratch · 2022-10-04T17:29:36Z

slack_sdk/web/async_client.py

+        # To upload multiple files at a time
+        upload_files: Optional[List[Dict[str, Any]]] = None,
+        channel: Optional[str] = None,
+        channels: Optional[Union[str, Sequence[str]]] = None,  # having n channels is no longer supported


@WilliamBergamin Thanks. I think it's fine to move this argument to kwargs, but I still would like to support it as long as the list has only one item. I am pretty sure that most of the existing code has only a single channel in this parameter. Perhaps, we can add a warning logging even for the single item use case, but we can allow it for smoother migration.

seratch · 2022-10-04T17:30:24Z

slack_sdk/web/async_client.py

+        ):
+            raise e.SlackRequestError("Sharing files with multiple channels is no longer supported")
+        if filetype is not None:
+            warnings.warn("filetype is no longer supported. Please remove it from the arguments.")


thanks, this is fine

slack_sdk/web/async_client.py

slack_sdk/web/legacy_client.py

seratch · 2022-10-04T17:46:13Z

slack_sdk/web/client.py

@@ -3051,8 +3060,8 @@ def files_upload_v2(
                token=kwargs.get("token"),
            )
            _validate_for_legacy_client(url_response)
-            f["file_id"] = url_response.get("file_id")
-            f["upload_url"] = url_response.get("upload_url")
+            f["file_id"] = url_response.get("file_id")  # type: ignore


this was unintentionally inconsistent

seratch · 2022-10-04T17:46:39Z

@WilliamBergamin I've updated the code and it's now ready for reviews again: a30f930

WilliamBergamin

This looks good to me 🚀

srajiang · 2022-10-04T19:45:47Z

slack_sdk/web/async_client.py

+        alt_txt: Optional[str] = None,
+        snippet_type: Optional[str] = None,
+        # To upload multiple files at a time
+        upload_files: Optional[List[Dict[str, Any]]] = None,


Nit: Upload is already in the method name, would it make sense to call this arg multiple_files or files?

@srajiang Good point. The reason why I hesitated to name this as files is that we have file for the single file upload and the meaning of file is quite different. file is a file content and it does not include its metadata such as alt_txt and so on. Thus, naming the list of dict objects as files can sound inconsistent. Thoughts?

Perhaps just switching the parts around to file_uploads then? More suggestive that there's content and possibly metadata as well and still very clear that it's plural and different from file.

@srajiang This makes sense! I've renamed the argument ✅

Also, renamed the same part in Java SDK PR: slackapi/java-slack-sdk#1065

slack_sdk/web/internal_utils.py

slack_sdk/web/async_client.py

srajiang · 2022-10-04T22:18:22Z

slack_sdk/web/async_client.py

@@ -21,6 +22,10 @@
    _update_call_participants,
    _warn_if_text_or_attachment_fallback_is_missing,
    _remove_none_values,
+    _to_v2_upload_file_item,


@seratch - very minor but this could then be named to _to_v2_file_upload_item for consistency

@srajiang Thanks for catching this, renamed ✅

srajiang

Looks great! Thank you :)

srajiang · 2022-10-04T23:08:49Z

@seratch A nice-to-have though not necessary could be on client.api_call(api_method='files.upload') would be to log out that this is a legacy method and to consider upgrading. I'm doing something like this in node sdk.

seratch · 2022-10-04T23:12:43Z

@srajiang Sounds great. I like the warning for files.upload! I will add it before merging this. However, I hesitate to add the ones for new endpoints. Some of developers may be using it intentionally for some reason (e.g., to take full control, need more detailed logging, and so on). Probably, we will receive a request to add some option to remove it. If we add the informative logging output, the log level may should be info/debug instead.

Alex-ley-scrub · 2022-10-29T21:33:04Z

slack_sdk/web/async_client.py

+        *,
+        # for sending a single file
+        filename: Optional[str] = None,  # you can skip this only when sending along with content parameter
+        file: Optional[Union[str, bytes, IOBase]] = None,


@seratch - is this type hint correct? I tried to pass in a io.BytesIO object here as the file as I thought IOBase covers that as well as a real file handle (e.g. from open("filename", "rb")) - but it failed with TypeError: object of type '_io.BytesIO' has no len() - len comes from _to_v2_file_upload_item I believe. So either the type hint is incorrect or the implementation of _to_v2_file_upload_item needs tweaking? You could probably just do something like this?

def _to_v2_file_upload_item(upload_file: Dict[str, Any]) -> Dict[str, Optional[Any]]: file = upload_file.get("file") content = upload_file.get("content") data: Optional[bytes] = None if file is not None: if isinstance(file, str): # filepath with open(file.encode("utf-8", "ignore"), "rb") as readable: data = readable.read() elif isinstance(file, io.IOBase): # new stuff data = file.read() if isinstance(data, str): data = data.encode() elif isinstance(file, bytes): data = file else: raise SlackRequestError("The given file must be either a filepath as str or data as bytes/IOBase") elif content is not None: if isinstance(content, str): # str content data = content.encode("utf-8") elif isinstance(content, bytes): data = content else: raise SlackRequestError("The given content given as str or as bytes") filename = upload_file.get("filename") if upload_file.get("filename") is None and isinstance(file, str): # use the local filename if filename is missing if upload_file.get("filename") is None: filename = file.split(os.path.sep)[-1] else: filename = "Uploaded file" title = upload_file.get("title", "Uploaded file") if data is None: raise SlackRequestError(f"File content not found for filename: {filename}, title: {title}") if title is None: title = filename # to be consistent with files.upload API return { "filename": filename, "data": data, "length": len(data), "title": title, "alt_txt": upload_file.get("alt_txt"), "snippet_type": upload_file.get("snippet_type"), }

@Alex-ley-scrub Thanks for flagging this! This should be improved. We will look into this later.

seratch added enhancement M-T: A feature request for new functionality web-client Version: 3x labels Oct 3, 2022

seratch added this to the 3.19.0 milestone Oct 3, 2022

seratch requested a review from WilliamBergamin October 3, 2022 08:57

seratch self-assigned this Oct 3, 2022

seratch commented Oct 3, 2022

View reviewed changes

seratch mentioned this pull request Oct 3, 2022

File Upload API succeeds despite 408 Request Timeout slackapi/node-slack-sdk#1541

Closed

7 tasks

seratch requested a review from srajiang October 3, 2022 21:18

WilliamBergamin requested changes Oct 4, 2022

View reviewed changes

seratch added 6 commits October 5, 2022 02:26

Add files.upload v2 support

2e7ebaa

Fix type hint errors

11ffaa2

Fix a bug detected by pytype!

a8448f0

Fix for legacy client

1e64791

Fix for legacy client

f7dcca0

Fix for pytype validation

0247d64

seratch commented Oct 4, 2022

View reviewed changes

Update the internals for deprecated parameters

a30f930

seratch force-pushed the files-upload-v2 branch from da0af3e to a30f930 Compare October 4, 2022 17:44

seratch commented Oct 4, 2022

View reviewed changes

WilliamBergamin approved these changes Oct 4, 2022

View reviewed changes

srajiang reviewed Oct 4, 2022

View reviewed changes

slack_sdk/web/internal_utils.py Show resolved Hide resolved

srajiang reviewed Oct 4, 2022

View reviewed changes

slack_sdk/web/async_client.py Show resolved Hide resolved

seratch added 2 commits October 5, 2022 06:15

Fix a bug

0ee6267

Rename upload_files to file_uploads

2588655

srajiang reviewed Oct 4, 2022

View reviewed changes

Rename internal method

7d11ad0

srajiang approved these changes Oct 4, 2022

View reviewed changes

Add warning log

1d0eed1

seratch merged commit 853e690 into slackapi:main Oct 5, 2022

seratch deleted the files-upload-v2 branch October 5, 2022 01:58

srajiang mentioned this pull request Oct 17, 2022

Add files.uploadv2 support (fixing #1541) slackapi/node-slack-sdk#1544

Merged

2 tasks

Alex-ley-scrub reviewed Oct 29, 2022

View reviewed changes

seratch mentioned this pull request Oct 31, 2022

files_upload_v2 does not work with io.BytesIO file parameters #1292

Closed

junseok-l2 mentioned this pull request Mar 1, 2023

Add file.upload v2 support for slackUploadFile jenkinsci/slack-plugin#884

Closed

2paperstar mentioned this pull request Jul 13, 2023

[CI/CD] "stream timeout" when upload symbol to slack gsainfoteam/ziggle-flutter#12

Closed

OODemi52 mentioned this pull request Sep 30, 2023

Implemented batch upload feature using file_uploadV2 functionality OODemi52/slackUpload#1

Merged

zimeg mentioned this pull request Feb 29, 2024

files.upload method returns warning message "files.upload may cause some issues like timeouts for relatively large files." slackapi/node-slack-sdk#1752

Closed

asaf400 mentioned this pull request May 6, 2024

Slack API files.upload v1 deprecation apache/superset#28353

Closed

3 tasks

martinjohndyer mentioned this pull request May 7, 2024

Changes to Slack upload API GOTO-OBS/gtecs-common#9

Closed

Add files.upload v2 support #1272

Add files.upload v2 support #1272

Conversation

seratch commented Oct 3, 2022 • edited Loading

Summary

Category (place an x in each of the [ ])

Requirements (place an x in each [ ])

codecov bot commented Oct 3, 2022 • edited Loading

Codecov Report

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamBergamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch commented Oct 4, 2022

WilliamBergamin left a comment

Choose a reason for hiding this comment

srajiang Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajiang left a comment

Choose a reason for hiding this comment

srajiang commented Oct 4, 2022

seratch commented Oct 4, 2022

Alex-ley-scrub Oct 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch commented Oct 3, 2022 •

edited

Loading

Category (place an `x` in each of the `[ ]`)

Requirements (place an `x` in each `[ ]`)

codecov bot commented Oct 3, 2022 •

edited

Loading

srajiang Oct 4, 2022 •

edited

Loading

Alex-ley-scrub Oct 29, 2022 •

edited

Loading