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

Fix #1496 Async client uses blocking call when uploading file with v2 #1498

Merged
merged 1 commit into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ def run(self):
" await self.files_getUploadURLExternal(",
async_source,
)
async_source = re.sub(
r" self._upload_file\(",
" await self._upload_file(",
async_source,
)
async_source = re.sub(
r" self.files_completeUploadExternal\(",
" await self.files_completeUploadExternal(",
Expand Down
26 changes: 26 additions & 0 deletions slack_sdk/web/async_base_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
) # type: ignore
from .async_slack_response import AsyncSlackResponse
from .deprecation import show_deprecation_warning_if_any
from .file_upload_v2_result import FileUploadV2Result
from .internal_utils import (
convert_bool_to_0_or_1,
_build_req_args,
Expand Down Expand Up @@ -214,3 +215,28 @@
req_args=req_args,
retry_handlers=self.retry_handlers,
)

async def _upload_file(
self,
*,
url: str,
data: bytes,
logger: logging.Logger,
timeout: int,
proxy: Optional[str],
ssl: Optional[SSLContext],
) -> FileUploadV2Result:
"""Upload a file using the issued upload URL"""
result = await _request_with_session(

Check warning on line 230 in slack_sdk/web/async_base_client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/async_base_client.py#L230

Added line #L230 was not covered by tests
current_session=self.session,
timeout=timeout,
logger=logger,
http_verb="POST",
api_url=url,
req_args={"data": data, "proxy": proxy, "ssl": ssl},
retry_handlers=self.retry_handlers,
)
return FileUploadV2Result(

Check warning on line 239 in slack_sdk/web/async_base_client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/async_base_client.py#L239

Added line #L239 was not covered by tests
status=result.get("status_code"),
body=result.get("body"),
)
9 changes: 4 additions & 5 deletions slack_sdk/web/async_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
_warn_if_text_or_attachment_fallback_is_missing,
_remove_none_values,
_to_v2_file_upload_item,
_upload_file_via_v2_url,
_validate_for_legacy_client,
_print_files_upload_v2_suggestion,
)
Expand Down Expand Up @@ -3584,17 +3583,17 @@

# step2: "https://files.slack.com/upload/v1/..." per file
for f in files:
upload_result = _upload_file_via_v2_url(
upload_result = await self._upload_file(

Check warning on line 3586 in slack_sdk/web/async_client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/async_client.py#L3586

Added line #L3586 was not covered by tests
url=f["upload_url"],
data=f["data"],
logger=self._logger,
timeout=self.timeout,
proxy=self.proxy,
ssl=self.ssl,
)
if upload_result.get("status") != 200:
status = upload_result.get("status")
body = upload_result.get("body")
if upload_result.status != 200:
status = upload_result.status
body = upload_result.body

Check warning on line 3596 in slack_sdk/web/async_client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/async_client.py#L3594-L3596

Added lines #L3594 - L3596 were not covered by tests
message = (
"Failed to upload a file "
f"(status: {status}, body: {body}, filename: {f.get('filename')}, title: {f.get('title')})"
Expand Down
14 changes: 12 additions & 2 deletions slack_sdk/web/async_internal_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@

try:
async with session.request(http_verb, api_url, **req_args) as res:
data: Union[dict, bytes] = {}
data: Union[dict, bytes, str] = {}
if res.content_type == "application/gzip":
# admin.analytics.getFile
data = await res.read()
Expand All @@ -117,6 +117,14 @@
headers=res.headers,
data=data,
)
elif res.content_type == "text/plain":
# https://files.slack.com/upload/v1/...
data = await res.text()
retry_response = RetryHttpResponse(

Check warning on line 123 in slack_sdk/web/async_internal_utils.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/async_internal_utils.py#L122-L123

Added lines #L122 - L123 were not covered by tests
status_code=res.status,
headers=res.headers,
data=data,
)
else:
try:
data = await res.json()
Expand All @@ -143,7 +151,9 @@
)

if logger.level <= logging.DEBUG:
body = data if isinstance(data, dict) else "(binary)"
body = "(binary)"
if isinstance(data, dict) or isinstance(data, str):
body = data
logger.debug(
"Received the following response - "
f"status: {res.status}, "
Expand Down
28 changes: 27 additions & 1 deletion slack_sdk/web/base_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@

from slack_sdk.errors import SlackRequestError
from .deprecation import show_deprecation_warning_if_any
from .file_upload_v2_result import FileUploadV2Result
from .internal_utils import (
convert_bool_to_0_or_1,
get_user_agent,
_get_url,
_build_req_args,
_build_unexpected_body_error_message,
_upload_file_via_v2_url,
)
from .slack_response import SlackResponse
from slack_sdk.http_retry import default_retry_handlers
Expand Down Expand Up @@ -560,10 +562,34 @@
if has_json:
headers.update({"Content-Type": "application/json;charset=utf-8"})
if has_files:
# will be set afterwards
# will be set afterward
headers.pop("Content-Type", None)
return headers

def _upload_file(
self,
*,
url: str,
data: bytes,
logger: logging.Logger,
timeout: int,
proxy: Optional[str],
ssl: Optional[SSLContext],
) -> FileUploadV2Result:
"""Upload a file using the issued upload URL"""
result = _upload_file_via_v2_url(

Check warning on line 580 in slack_sdk/web/base_client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/base_client.py#L580

Added line #L580 was not covered by tests
url=url,
data=data,
logger=logger,
timeout=timeout,
proxy=proxy,
ssl=ssl,
)
return FileUploadV2Result(

Check warning on line 588 in slack_sdk/web/base_client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/base_client.py#L588

Added line #L588 was not covered by tests
status=result.get("status"),
body=result.get("body"),
)

# =================================================================

@staticmethod
Expand Down
9 changes: 4 additions & 5 deletions slack_sdk/web/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
_warn_if_text_or_attachment_fallback_is_missing,
_remove_none_values,
_to_v2_file_upload_item,
_upload_file_via_v2_url,
_validate_for_legacy_client,
_print_files_upload_v2_suggestion,
)
Expand Down Expand Up @@ -3575,17 +3574,17 @@

# step2: "https://files.slack.com/upload/v1/..." per file
for f in files:
upload_result = _upload_file_via_v2_url(
upload_result = self._upload_file(

Check warning on line 3577 in slack_sdk/web/client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/client.py#L3577

Added line #L3577 was not covered by tests
url=f["upload_url"],
data=f["data"],
logger=self._logger,
timeout=self.timeout,
proxy=self.proxy,
ssl=self.ssl,
)
if upload_result.get("status") != 200:
status = upload_result.get("status")
body = upload_result.get("body")
if upload_result.status != 200:
status = upload_result.status
body = upload_result.body

Check warning on line 3587 in slack_sdk/web/client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/client.py#L3585-L3587

Added lines #L3585 - L3587 were not covered by tests
message = (
"Failed to upload a file "
f"(status: {status}, body: {body}, filename: {f.get('filename')}, title: {f.get('title')})"
Expand Down
7 changes: 7 additions & 0 deletions slack_sdk/web/file_upload_v2_result.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class FileUploadV2Result:
status: int
body: str

def __init__(self, status: int, body: str):
self.status = status
self.body = body

Check warning on line 7 in slack_sdk/web/file_upload_v2_result.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/file_upload_v2_result.py#L6-L7

Added lines #L6 - L7 were not covered by tests
9 changes: 7 additions & 2 deletions slack_sdk/web/internal_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,9 @@
else:
raise SlackRequestError(f"Invalid proxy detected: {proxy} must be a str value")

if logger.level <= logging.DEBUG:
logger.debug(f"Sending a request: POST {url}")

Check warning on line 381 in slack_sdk/web/internal_utils.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/internal_utils.py#L380-L381

Added lines #L380 - L381 were not covered by tests

resp: Optional[HTTPResponse] = None
req: Request = Request(method="POST", url=url, data=data, headers={})
if opener:
Expand All @@ -388,8 +391,10 @@
body: str = resp.read().decode(charset) # read the response body here
if logger.level <= logging.DEBUG:
message = (
"Received the following response - ",
f"status: {resp.status}, " f"headers: {dict(resp.headers)}, " f"body: {body}",
"Received the following response - "
f"status: {resp.status}, "
f"headers: {dict(resp.headers)}, "
f"body: {body}"
)
logger.debug(message)

Expand Down
27 changes: 26 additions & 1 deletion slack_sdk/web/legacy_base_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@
from slack_sdk.errors import SlackRequestError
from .async_internal_utils import _files_to_data, _get_event_loop, _request_with_session
from .deprecation import show_deprecation_warning_if_any
from .file_upload_v2_result import FileUploadV2Result
from .internal_utils import (
convert_bool_to_0_or_1,
get_user_agent,
_get_url,
_build_req_args,
_build_unexpected_body_error_message,
_upload_file_via_v2_url,
)
from .legacy_slack_response import LegacySlackResponse as SlackResponse
from ..proxy_env_variable_loader import load_http_proxy_from_env
Expand Down Expand Up @@ -521,10 +523,33 @@
if has_json:
headers.update({"Content-Type": "application/json;charset=utf-8"})
if has_files:
# will be set afterwards
# will be set afterward
headers.pop("Content-Type", None)
return headers

def _upload_file(
self,
*,
url: str,
data: bytes,
logger: logging.Logger,
timeout: int,
proxy: Optional[str],
ssl: Optional[SSLContext],
) -> FileUploadV2Result:
result = _upload_file_via_v2_url(

Check warning on line 540 in slack_sdk/web/legacy_base_client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/legacy_base_client.py#L540

Added line #L540 was not covered by tests
url=url,
data=data,
logger=logger,
timeout=timeout,
proxy=proxy,
ssl=ssl,
)
return FileUploadV2Result(

Check warning on line 548 in slack_sdk/web/legacy_base_client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/legacy_base_client.py#L548

Added line #L548 was not covered by tests
status=result.get("status"),
body=result.get("body"),
)

# =================================================================

@staticmethod
Expand Down
9 changes: 4 additions & 5 deletions slack_sdk/web/legacy_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
_warn_if_text_or_attachment_fallback_is_missing,
_remove_none_values,
_to_v2_file_upload_item,
_upload_file_via_v2_url,
_validate_for_legacy_client,
_print_files_upload_v2_suggestion,
)
Expand Down Expand Up @@ -3586,17 +3585,17 @@

# step2: "https://files.slack.com/upload/v1/..." per file
for f in files:
upload_result = _upload_file_via_v2_url(
upload_result = self._upload_file(

Check warning on line 3588 in slack_sdk/web/legacy_client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/legacy_client.py#L3588

Added line #L3588 was not covered by tests
url=f["upload_url"],
data=f["data"],
logger=self._logger,
timeout=self.timeout,
proxy=self.proxy,
ssl=self.ssl,
)
if upload_result.get("status") != 200:
status = upload_result.get("status")
body = upload_result.get("body")
if upload_result.status != 200:
status = upload_result.status
body = upload_result.body

Check warning on line 3598 in slack_sdk/web/legacy_client.py

View check run for this annotation

Codecov / codecov/patch

slack_sdk/web/legacy_client.py#L3596-L3598

Added lines #L3596 - L3598 were not covered by tests
message = (
"Failed to upload a file "
f"(status: {status}, body: {body}, filename: {f.get('filename')}, title: {f.get('title')})"
Expand Down