Skip to content

Fix #1333 Enable using RetryHandler for 200 OK response patterns#1334

Merged
seratch merged 2 commits intoslackapi:mainfrom
seratch:issue-1333-retry-handler
Feb 17, 2023
Merged

Fix #1333 Enable using RetryHandler for 200 OK response patterns#1334
seratch merged 2 commits intoslackapi:mainfrom
seratch:issue-1333-retry-handler

Conversation

@seratch
Copy link
Copy Markdown
Contributor

@seratch seratch commented Feb 17, 2023

Summary

This pull request resolves #1333

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

  • slack_sdk.web.WebClient (sync/async) (Web API client)
  • slack_sdk.webhook.WebhookClient (sync/async) (Incoming Webhook, response_url sender)
  • slack_sdk.socket_mode (Socket Mode client)
  • slack_sdk.signature (Request Signature Verifier)
  • slack_sdk.oauth (OAuth Flow Utilities)
  • slack_sdk.models (UI component builders)
  • slack_sdk.scim (SCIM API client)
  • slack_sdk.audit_logs (Audit Logs API client)
  • slack_sdk.rtm_v2 (RTM client)
  • /docs-src (Documents, have you run ./scripts/docs.sh?)
  • /docs-src-v2 (Documents, have you run ./scripts/docs-v2.sh?)
  • /tutorial (PythOnBoardingBot tutorial)
  • tests/integration_tests (Automated tests for this library)

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.

@seratch seratch added enhancement M-T: A feature request for new functionality web-client Version: 3x labels Feb 17, 2023
@seratch seratch added this to the 3.20.0 milestone Feb 17, 2023
@seratch seratch self-assigned this Feb 17, 2023
seratch
seratch commented Feb 17, 2023
View reviewed changes
integration_tests/web/test_admin_analytics.py
client = self.sync_client

response = client.admin_analytics_getFile(date="2020-10-20", type="member")
response = client.admin_analytics_getFile(date="2022-10-20", type="member")
Copy link
Copy Markdown
Contributor Author

@seratch seratch Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the old date no longer works for my sandbox org, let me change the date here

integration_tests/web/test_issue_594.py
external_url=external_url,
title="Good Old Slack Logo",
indexable_file_contents="Good Old Slack Logo",
indexable_file_contents="Good Old Slack Logo".encode("utf-8"),
Copy link
Copy Markdown
Contributor Author

@seratch seratch Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test code needed to be changed when I fixed the code back then

slack_sdk/web/async_internal_utils.py
f"body: {body}"
)

if res.status == 429:
Copy link
Copy Markdown
Contributor Author

@seratch seratch Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the if condition to check http status

slack_sdk/web/base_client.py
resp = self._perform_urllib_http_request_internal(url, req)
# The resp is a 200 OK response
return resp
if len(self.retry_handlers) > 0:
Copy link
Copy Markdown
Contributor Author

@seratch seratch Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added retry handler support for 200 OK responses here

tests/slack_sdk/web/mock_web_api_server.py
header = self.headers["Authorization"]
if header is not None and "xoxp-" in header:
pattern = str(header).split("xoxp-", 1)[1]
if header is not None and ("xoxb-" in header or "xoxp-" in header):
Copy link
Copy Markdown
Contributor Author

@seratch seratch Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved the mock server for unit tests

tests/slack_sdk/web/test_web_client_http_retry_ratelimited.py
@@ -1,25 +0,0 @@
import unittest
Copy link
Copy Markdown
Contributor Author

@seratch seratch Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed redundant and incomplete tests

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 17, 2023 •
edited
Loading

Codecov Report

Merging #1334 (c1489ab) into main (8dc50a8) will decrease coverage by 0.02%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main    #1334      +/-   ##
==========================================
- Coverage   85.40%   85.39%   -0.02%     
==========================================
  Files         111      111              
  Lines       11394    11406      +12     
==========================================
+ Hits         9731     9740       +9     
- Misses       1663     1666       +3
Impacted Files Coverage Δ
slack_sdk/web/async_internal_utils.py 78.43% <33.33%> (-4.10%) ⬇️
slack_sdk/web/base_client.py 89.62% <100.00%> (+0.52%) ⬆️
slack_sdk/http_retry/builtin_handlers.py 95.23% <0.00%> (+2.38%) ⬆️

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

@seratch seratch merged commit cecec9b into slackapi:main Feb 17, 2023
@seratch seratch deleted the issue-1333-retry-handler branch February 17, 2023 05:56
@tjstum tjstum mentioned this pull request Jul 31, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@seratch seratch

Labels

enhancement M-T: A feature request for new functionality Version: 3x web-client

Projects

None yet

Milestone

3.20.0

Development

Successfully merging this pull request may close these issues.

Enable using RetryHandler for 200 OK response patterns

1 participant

@seratch