-
Notifications
You must be signed in to change notification settings - Fork 17.9k
chore(reports): deprecate Slack v1 and harden Slack v2 tests #39914
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
Changes from all commits
3a46222
92b2750
398e4a4
d394025
af800ba
05a5217
a4aa1f4
b4ce1ff
3f60bf2
e1c63fa
1378c28
e773de2
026e5c4
e975e18
b2d1fa7
17c8d68
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,12 +16,14 @@ | |
| # under the License. | ||
|
|
||
|
|
||
| import functools | ||
| import logging | ||
| import warnings | ||
| from typing import Any, Callable, Optional | ||
|
|
||
| from flask import current_app as app | ||
| from slack_sdk import WebClient | ||
| from slack_sdk.errors import SlackApiError | ||
| from slack_sdk.errors import SlackApiError, SlackClientError as SlackSDKClientError | ||
| from slack_sdk.http_retry.builtin_handlers import RateLimitErrorRetryHandler | ||
|
|
||
| from superset import feature_flag_manager | ||
|
|
@@ -34,12 +36,42 @@ | |
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Add an explicit type annotation to this module-level constant to satisfy the type-hint requirement for relevant variables. [custom_rule] Severity Level: Minor Why it matters? 🤔This new module-level constant is unannotated even though its type is clear and stable, so it matches the rule requiring type hints on newly added Python variables that can be annotated. (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** superset/utils/slack.py
**Line:** 38:38
**Comment:**
*Custom Rule: Add an explicit type annotation to this module-level constant to satisfy the type-hint requirement for relevant variables.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| _SLACK_V1_DEPRECATION_MESSAGE = ( | ||
| "Slack v1 (the legacy `Slack` recipient type and `files.upload` API) is " | ||
| "deprecated and will be removed in the next major release. Slack retired " | ||
| "the `files.upload` endpoint in 2025, so v1 file uploads no longer work; " | ||
| "only text-only `chat_postMessage` sends still succeed. Grant your Slack " | ||
| "bot the `channels:read` and `groups:read` scopes so existing v1 " | ||
| "recipients can be auto-upgraded to SlackV2 on " | ||
| "their next send." | ||
| ) | ||
|
sadpandajoe marked this conversation as resolved.
|
||
|
|
||
|
|
||
| # functools.cache gives us a process-lifetime, thread-safe one-shot guard | ||
| # without the read-then-write race that bare module globals would have under | ||
| # multi-threaded WSGI workers. The cached return value (None) is irrelevant — | ||
| # we only care that the body executes at most once per process. | ||
| @functools.cache | ||
| def _emit_v1_flag_off_deprecation() -> None: | ||
| warnings.warn(_SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=3) | ||
| logger.warning( | ||
| "ALERT_REPORT_SLACK_V2 is disabled; %s", _SLACK_V1_DEPRECATION_MESSAGE | ||
| ) | ||
|
sadpandajoe marked this conversation as resolved.
|
||
|
|
||
|
|
||
| @functools.cache | ||
| def _emit_v1_scope_missing_deprecation() -> None: | ||
| warnings.warn(_SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=3) | ||
|
|
||
|
sadpandajoe marked this conversation as resolved.
|
||
|
|
||
| class SlackChannelTypes(StrEnum): | ||
| PUBLIC = "public_channel" | ||
| PRIVATE = "private_channel" | ||
|
|
||
|
|
||
| _SLACK_CONVERSATION_TYPES = ",".join(SlackChannelTypes) | ||
|
sadpandajoe marked this conversation as resolved.
|
||
|
|
||
|
|
||
| class SlackClientError(Exception): | ||
| pass | ||
|
|
||
|
|
@@ -117,7 +149,7 @@ def _get_channels( | |
| client = get_slack_client() | ||
| channel_schema = SlackChannelSchema() | ||
| channels: list[SlackChannelSchema] = [] | ||
| extra_params = {"types": ",".join(SlackChannelTypes)} | ||
| extra_params = {"types": _SLACK_CONVERSATION_TYPES} | ||
|
sadpandajoe marked this conversation as resolved.
|
||
| if team_id: | ||
| extra_params["team_id"] = team_id | ||
| cursor = None | ||
|
|
@@ -224,21 +256,75 @@ def get_channels_with_search( | |
| return channels | ||
|
|
||
|
|
||
| _SCOPE_MISSING_ERROR_CODES = frozenset( | ||
| {"missing_scope", "not_allowed_token_type", "no_permission"} | ||
| ) | ||
|
sadpandajoe marked this conversation as resolved.
|
||
|
|
||
|
|
||
| def should_use_v2_api() -> bool: | ||
| if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_SLACK_V2"): | ||
| _emit_v1_flag_off_deprecation() | ||
| return False | ||
| try: | ||
| client = get_slack_client() | ||
| extra_params = {"types": _SLACK_CONVERSATION_TYPES} | ||
|
sadpandajoe marked this conversation as resolved.
|
||
| team_id = get_team_id() | ||
| client.conversations_list(**({"team_id": team_id} if team_id else {})) | ||
| if team_id: | ||
| extra_params["team_id"] = team_id | ||
| client.conversations_list( | ||
| limit=1, | ||
| exclude_archived=True, | ||
| **extra_params, | ||
| ) | ||
| logger.info("Slack API v2 is available") | ||
| return True | ||
| except SlackApiError: | ||
| # use the v1 api but warn with a deprecation message | ||
| except SlackApiError as ex: | ||
| # Only the scope-missing branch is a v1-deprecation signal; other | ||
| # SlackApiError codes (invalid_auth, ratelimited, server errors, etc.) | ||
| # are unrelated probe failures and should not be reported as a missing | ||
| # scope. We still fall back to v1 in both cases so a transient probe | ||
| # failure doesn't break sends — operators get an actionable log either | ||
| # way. | ||
| # `response` is normally a SlackResponse whose payload lives in `.data`, | ||
| # but the SDK (and our tests) can also hand back a plain dict. Read the | ||
| # error code in either shape so the scope-missing branch isn't missed. | ||
| response = getattr(ex, "response", None) | ||
| data = getattr(response, "data", None) | ||
| if not isinstance(data, dict): | ||
| data = response if isinstance(response, dict) else {} | ||
| error_code = data.get("error", "") | ||
| if error_code in _SCOPE_MISSING_ERROR_CODES: | ||
| # The DeprecationWarning fires once per process, but the actionable | ||
| # log line fires every send so operators see it in their report logs. | ||
| _emit_v1_scope_missing_deprecation() | ||
| logger.warning( | ||
| "Slack bot is missing the `channels:read` and `groups:read` " | ||
| "scopes; falling back to the deprecated " | ||
| "v1 API. %s", | ||
| _SLACK_V1_DEPRECATION_MESSAGE, | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| "Slack v2 probe failed with error %r; falling back to the " | ||
| "deprecated v1 API for this send. Investigate the underlying " | ||
|
sadpandajoe marked this conversation as resolved.
|
||
| "Slack API error — this is not a missing-scope problem.", | ||
| error_code or str(ex), | ||
| ) | ||
| return False | ||
| except SlackSDKClientError as ex: | ||
| # Non-API SDK failures (e.g. SlackClientNotConnectedError, | ||
| # SlackRequestError, SlackClientConfigurationError) are not subclasses | ||
| # of SlackApiError, so without this branch they would escape the probe | ||
| # raw. The caller runs this probe *before* the mapped Slack send `try`, | ||
| # so an un-caught probe error aborts the entire recipient loop instead | ||
| # of failing a single recipient. Treat any probe connection/transport | ||
| # failure as "v2 unavailable" and fall back to the deprecated v1 API, | ||
| # matching the SlackApiError behavior above. | ||
| logger.warning( | ||
| "Your current Slack scopes are missing `channels:read`. Please add " | ||
| "this to your Slack app in order to continue using the v1 API. Support " | ||
| "for the old Slack API will be removed in Superset version 6.0.0." | ||
| "Slack v2 probe failed to connect (%s: %s); falling back to the " | ||
| "deprecated v1 API for this send.", | ||
| type(ex).__name__, | ||
| ex, | ||
| ) | ||
| return False | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.