You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/07/04 23:41:45 UTC
(superset) branch master updated: chore(reports): deprecate Slack v1 and harden Slack v2 tests (#39914)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 1e50316bccd chore(reports): deprecate Slack v1 and harden Slack v2 tests (#39914)
1e50316bccd is described below
commit 1e50316bccd8a7c5f00057e7d2e571f986ed5c61
Author: Joe Li <jo...@preset.io>
AuthorDate: Sat Jul 4 16:41:39 2026 -0700
chore(reports): deprecate Slack v1 and harden Slack v2 tests (#39914)
Co-authored-by: Claude Opus 4.7 (1M context) <no...@anthropic.com>
---
UPDATING.md | 2 +
docs/static/feature-flags.json | 4 +-
superset/commands/report/execute.py | 88 ++-
superset/config.py | 10 +-
superset/reports/notifications/slack.py | 7 +-
superset/reports/notifications/slackv2.py | 57 +-
superset/utils/slack.py | 102 +++-
tests/integration_tests/reports/commands_tests.py | 13 +
tests/unit_tests/commands/report/execute_test.py | 108 ++--
.../reports/notifications/slack_tests.py | 676 ++++++++++++++++++++-
tests/unit_tests/utils/slack_test.py | 233 +++++++
11 files changed, 1196 insertions(+), 104 deletions(-)
diff --git a/UPDATING.md b/UPDATING.md
index d7f080d2afb..0388b3ce36e 100644
--- a/UPDATING.md
+++ b/UPDATING.md
@@ -302,6 +302,8 @@ Schedule the cutover in a quiet window. Runtime reads use only the single config
The migration is transactional (all-or-nothing) and idempotent — it can be safely re-run or resumed. Note that AES-GCM, unlike AES-CBC, does not support querying directly over encrypted columns; audit any code that filters on an encrypted column before switching. See the SIP at `docs/sip/authenticated-encryption-at-rest.md` for details.
+- [39914](https://github.com/apache/superset/pull/39914) `ALERT_REPORT_SLACK_V2` now defaults to `True` and the legacy Slack v1 integration (`Slack` recipient type, `files.upload` API) is deprecated for removal in the next major. Slack blocked new apps from `files.upload` in May 2024 and fully retired the method for all apps on November 12, 2025; because the v1 path sends files through `files.upload`, v1 file-bearing sends now fail at the API level — only text-only `chat_postMessage` sti [...]
+
### Soft delete and restore for dashboards
**Everything in this section applies only when the `SOFT_DELETE` feature flag is enabled. The flag defaults to `False`** (`@lifecycle: development`), so on a default deployment `DELETE /api/v1/dashboard/<id>` continues to **hard-delete permanently** — nothing is recoverable. Enable `SOFT_DELETE` to get the behavior described below.
diff --git a/docs/static/feature-flags.json b/docs/static/feature-flags.json
index 241412bc637..d97976b231c 100644
--- a/docs/static/feature-flags.json
+++ b/docs/static/feature-flags.json
@@ -122,9 +122,9 @@
},
{
"name": "ALERT_REPORT_SLACK_V2",
- "default": false,
+ "default": true,
"lifecycle": "testing",
- "description": "Enables Slack V2 integration for Alerts and Reports"
+ "description": "Enables Slack V2 integration for Alerts and Reports. Defaults to True; the legacy Slack v1 path is deprecated and will be removed in the next major release. Operators must grant the Slack bot both the `channels:read` and `groups:read` scopes so existing v1 recipients can be auto-upgraded on their next send. Without those scopes, file uploads fail (Slack retired the `files.upload` endpoint in 2025) and only text-only `chat_postMessage` sends will continue to work v [...]
},
{
"name": "ALERT_REPORT_WEBHOOK",
diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py
index cd217c599ea..f08442122f8 100644
--- a/superset/commands/report/execute.py
+++ b/superset/commands/report/execute.py
@@ -163,62 +163,56 @@ class BaseReportState:
"""
Update the report schedule type and channels for all slack recipients to v2.
V2 uses ids instead of names for channels.
+
+ Channel ids for every Slack recipient are resolved first and the
+ recipients are only mutated once all of them resolve. This keeps the
+ upgrade all-or-nothing: a single unresolvable channel can no longer
+ leave the schedule with some recipients already switched to v2 (and
+ persisted by a later error-log commit) while others are untouched.
"""
- # Track each recipient mutated in this pass with its original (type,
- # config) so a partial failure can revert ALL of them — not just the
- # loop variable. Restoring the in-memory values to their loaded state
- # keeps a later commit from persisting a half-migrated set.
- mutated: list[tuple[ReportRecipients, ReportRecipientType, str]] = []
+ resolved: list[tuple[ReportRecipients, str]] = []
try:
for recipient in self._report_schedule.recipients:
- if recipient.type == ReportRecipientType.SLACK:
- mutated.append(
- (recipient, recipient.type, recipient.recipient_config_json)
- )
- recipient.type = ReportRecipientType.SLACKV2
- slack_recipients = json.loads(recipient.recipient_config_json)
- # V1 method allowed to use leading `#` in the channel name
- channel_names = (slack_recipients["target"] or "").replace("#", "")
- # we need to ensure that existing reports can also fetch
- # ids from private channels
- channels = get_channels_with_search(
- search_string=channel_names,
- types=[
- SlackChannelTypes.PRIVATE,
- SlackChannelTypes.PUBLIC,
- ],
- exact_match=True,
- )
- channels_list = recipients_string_to_list(channel_names)
- if len(channels_list) != len(channels):
- missing_channels = set(channels_list) - {
- channel["name"] for channel in channels
- }
- msg = (
- "Could not find the following channels: "
- f"{', '.join(missing_channels)}"
- )
- raise UpdateFailedError(msg)
- channel_ids = ",".join(channel["id"] for channel in channels)
- recipient.recipient_config_json = json.dumps(
- {
- "target": channel_ids,
- }
+ if recipient.type != ReportRecipientType.SLACK:
+ continue
+ slack_recipients = json.loads(recipient.recipient_config_json)
+ # V1 method allowed to use leading `#` in the channel name
+ channel_names = (slack_recipients["target"] or "").replace("#", "")
+ # we need to ensure that existing reports can also fetch
+ # ids from private channels
+ channels = get_channels_with_search(
+ search_string=channel_names,
+ types=[
+ SlackChannelTypes.PRIVATE,
+ SlackChannelTypes.PUBLIC,
+ ],
+ exact_match=True,
+ )
+ channels_list = recipients_string_to_list(channel_names)
+ if len(channels_list) != len(channels):
+ missing_channels = set(channels_list) - {
+ channel["name"] for channel in channels
+ }
+ msg = (
+ "Could not find the following channels: "
+ f"{', '.join(missing_channels)}"
)
+ raise UpdateFailedError(msg)
+ channel_ids = ",".join(channel["id"] for channel in channels)
+ resolved.append((recipient, json.dumps({"target": channel_ids})))
except Exception as ex:
- # Revert every mutated recipient to v1 (both type AND config) to
- # preserve configuration (requires manual fix). Reverting the full
- # set — not just the loop variable — keeps earlier recipients
- # consistent; iterating the snapshot also avoids the UnboundLocalError
- # that a bare loop-variable reference raises on a pre-iteration
- # failure (which would mask the real error).
- for reverted_recipient, original_type, original_config in mutated:
- reverted_recipient.type = original_type
- reverted_recipient.recipient_config_json = original_config
+ # No recipient has been mutated yet, so there is no partial upgrade
+ # to revert; surface the failure so the configuration can be fixed
+ # manually.
msg = f"Failed to update slack recipients to v2: {str(ex)}"
logger.exception(msg)
raise UpdateFailedError(msg) from ex
+ # Every Slack recipient resolved; apply the upgrade atomically.
+ for recipient, recipient_config_json in resolved:
+ recipient.type = ReportRecipientType.SLACKV2
+ recipient.recipient_config_json = recipient_config_json
+
def create_log(self, error_message: Optional[str] = None) -> None:
"""
Creates a Report execution log, uses the current computed last_value for Alerts
diff --git a/superset/config.py b/superset/config.py
index f84493a44f2..910322d7542 100644
--- a/superset/config.py
+++ b/superset/config.py
@@ -705,9 +705,15 @@ DEFAULT_FEATURE_FLAGS: dict[str, bool] = {
# @lifecycle: testing
# @docs: https://superset.apache.org/docs/configuration/alerts-reports
"ALERT_REPORTS": False,
- # Enables Slack V2 integration for Alerts and Reports
+ # Enables Slack V2 integration for Alerts and Reports.
+ # Defaults to True; the legacy Slack v1 path is deprecated and will be removed
+ # in the next major release. Operators must grant the Slack bot both the
+ # `channels:read` and `groups:read` scopes so existing v1 recipients can be
+ # auto-upgraded on their next send. Without those scopes, file uploads fail
+ # (Slack retired the `files.upload` endpoint in 2025) and only text-only
+ # `chat_postMessage` sends will continue to work via the legacy path.
# @lifecycle: testing
- "ALERT_REPORT_SLACK_V2": False,
+ "ALERT_REPORT_SLACK_V2": True,
# Enables webhook integration for Alerts and Reports
# @lifecycle: testing
"ALERT_REPORT_WEBHOOK": False,
diff --git a/superset/reports/notifications/slack.py b/superset/reports/notifications/slack.py
index 589fddb9aad..bd527bdd3ce 100644
--- a/superset/reports/notifications/slack.py
+++ b/superset/reports/notifications/slack.py
@@ -53,7 +53,12 @@ from superset.utils.slack import (
logger = logging.getLogger(__name__)
-# TODO: Deprecated: Remove this class in Superset 6.0.0
+# Deprecated: Slack v1 will be removed in the next major release. The Slack
+# `files.upload` endpoint was retired in 2025, so file-bearing sends already
+# fail at the API level; only text-only `chat_postMessage` sends still work
+# here. When the Slack bot has the `channels:read` and `groups:read` scopes,
+# existing v1 recipients are auto-upgraded to SlackV2 on first send via
+# `update_report_schedule_slack_v2`.
class SlackNotification(SlackMixin, BaseNotification): # pylint: disable=too-few-public-methods
"""
Sends a slack notification for a report recipient
diff --git a/superset/reports/notifications/slackv2.py b/superset/reports/notifications/slackv2.py
index 4f4cf84302f..4e94f28701c 100644
--- a/superset/reports/notifications/slackv2.py
+++ b/superset/reports/notifications/slackv2.py
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
import logging
-from collections.abc import Sequence
+from collections.abc import Callable, Sequence
from io import IOBase
from typing import List, Union
@@ -48,6 +48,55 @@ from superset.utils.slack import get_slack_client
logger = logging.getLogger(__name__)
+_TRANSIENT_SLACK_API_ERROR_CODES = frozenset(
+ {
+ "fatal_error",
+ "internal_error",
+ "ratelimited",
+ "request_timeout",
+ "rollup_error",
+ "service_unavailable",
+ "timeout",
+ }
+)
+
+
+def _get_slack_api_error_code(ex: SlackApiError) -> str:
+ response = getattr(ex, "response", None)
+ data = getattr(response, "data", None)
+ if not isinstance(data, dict):
+ data = response if isinstance(response, dict) else {}
+ return str(data.get("error") or "")
+
+
+def _get_slack_api_status_code(ex: SlackApiError) -> int | None:
+ response = getattr(ex, "response", None)
+ return getattr(response, "status_code", None)
+
+
+def _give_up_slack_api_retry(ex: Exception) -> bool:
+ if not isinstance(ex, SlackApiError):
+ return False
+
+ status_code = _get_slack_api_status_code(ex)
+ if status_code == 429 or (status_code is not None and 500 <= status_code < 600):
+ return False
+
+ error_code = _get_slack_api_error_code(ex)
+ return bool(error_code and error_code not in _TRANSIENT_SLACK_API_ERROR_CODES)
+
+
+@backoff.on_exception(
+ backoff.expo,
+ (SlackApiError, SlackClientNotConnectedError),
+ factor=10,
+ base=2,
+ max_tries=5,
+ giveup=_give_up_slack_api_retry,
+)
+def _call_slack_api(method: Callable[..., object], **kwargs: object) -> None:
+ method(**kwargs)
+
class SlackV2Notification(SlackMixin, BaseNotification): # pylint: disable=too-few-public-methods
"""
@@ -77,7 +126,6 @@ class SlackV2Notification(SlackMixin, BaseNotification): # pylint: disable=too-
return ("pdf", [self._content.pdf])
return (None, [])
- @backoff.on_exception(backoff.expo, SlackApiError, factor=10, base=2, max_tries=5)
@statsd_gauge("reports.slack.send")
def send(self) -> None:
global_logs_context = getattr(g, "logs_context", {}) or {}
@@ -98,7 +146,8 @@ class SlackV2Notification(SlackMixin, BaseNotification): # pylint: disable=too-
for channel in channels:
if len(files) > 0:
for file in files:
- client.files_upload_v2(
+ _call_slack_api(
+ client.files_upload_v2,
channel=channel,
file=file,
initial_comment=body,
@@ -106,7 +155,7 @@ class SlackV2Notification(SlackMixin, BaseNotification): # pylint: disable=too-
filename=file_name,
)
else:
- client.chat_postMessage(channel=channel, text=body)
+ _call_slack_api(client.chat_postMessage, channel=channel, text=body)
logger.info(
"Report sent to slack",
diff --git a/superset/utils/slack.py b/superset/utils/slack.py
index 51fbd497171..0e0e422d525 100644
--- a/superset/utils/slack.py
+++ b/superset/utils/slack.py
@@ -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 @@ from superset.utils.core import recipients_string_to_list
logger = logging.getLogger(__name__)
+_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."
+)
+
+
+# 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
+ )
+
+
+@functools.cache
+def _emit_v1_scope_missing_deprecation() -> None:
+ warnings.warn(_SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=3)
+
class SlackChannelTypes(StrEnum):
PUBLIC = "public_channel"
PRIVATE = "private_channel"
+_SLACK_CONVERSATION_TYPES = ",".join(SlackChannelTypes)
+
+
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}
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"}
+)
+
+
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}
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 "
+ "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
diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py
index 2af9c9e88e0..6d8c743fc4e 100644
--- a/tests/integration_tests/reports/commands_tests.py
+++ b/tests/integration_tests/reports/commands_tests.py
@@ -1984,11 +1984,13 @@ def test_slack_chart_alert_no_attachment(email_mock, create_alert_email_chart):
"load_birth_names_dashboard_with_slices",
"create_report_slack_chart",
)
+@patch("superset.commands.report.execute.get_channels_with_search")
@patch("superset.utils.slack.WebClient")
@patch("superset.utils.screenshots.ChartScreenshot.get_screenshot")
def test_slack_token_callable_chart_report(
screenshot_mock,
slack_client_mock_class,
+ get_channels_with_search_mock,
create_report_slack_chart,
):
"""
@@ -1999,9 +2001,20 @@ def test_slack_token_callable_chart_report(
channel_name = notification_targets[0]
channel_id = "channel_id"
slack_client_mock_class.return_value = Mock()
+ # should_use_v2_api() probes via conversations_list(); a non-erroring return
+ # is enough — it doesn't read the response body. The v2 upgrade then resolves
+ # channel names through get_channels_with_search, which we mock directly.
slack_client_mock_class.return_value.conversations_list.return_value = {
"channels": [{"id": channel_id, "name": channel_name}]
}
+ get_channels_with_search_mock.return_value = [
+ {
+ "id": channel_id,
+ "name": channel_name,
+ "is_member": True,
+ "is_private": False,
+ }
+ ]
slack_token_mock = Mock(return_value="cool_code")
with patch.dict("flask.current_app.config", {"SLACK_API_TOKEN": slack_token_mock}):
diff --git a/tests/unit_tests/commands/report/execute_test.py b/tests/unit_tests/commands/report/execute_test.py
index 880bebf8d1a..73937d53dd0 100644
--- a/tests/unit_tests/commands/report/execute_test.py
+++ b/tests/unit_tests/commands/report/execute_test.py
@@ -1309,73 +1309,109 @@ def test_update_recipient_to_slack_v2_missing_channels(mocker: MockerFixture):
mock_cmmd.update_report_schedule_slack_v2()
-def test_update_recipient_to_slack_v2_reverts_all_on_partial_failure(
+def test_update_recipient_to_slack_v2_multiple_recipients(
mocker: MockerFixture,
) -> None:
- """
- When the second of two Slack recipients fails channel resolution, BOTH
- recipients are fully reverted — type AND exact original
- ``recipient_config_json`` string — not just the loop variable's type. This
- prevents the intervening ``create_log`` commit from flushing a half-migrated,
- inconsistent state.
+ """All Slack recipients are upgraded atomically when every channel resolves."""
+
+ def fake_get_channels(search_string, types, exact_match):
+ return {
+ "channel-1": [{"id": "C1", "name": "channel-1", "is_private": False}],
+ "channel-2": [{"id": "C2", "name": "channel-2", "is_private": False}],
+ }[search_string]
+
+ mocker.patch(
+ "superset.commands.report.execute.get_channels_with_search",
+ side_effect=fake_get_channels,
+ )
+ mock_report_schedule = ReportSchedule(
+ recipients=[
+ ReportRecipients(
+ type=ReportRecipientType.SLACK,
+ recipient_config_json=json.dumps({"target": "channel-1"}),
+ ),
+ ReportRecipients(
+ type=ReportRecipientType.SLACK,
+ recipient_config_json=json.dumps({"target": "channel-2"}),
+ ),
+ ],
+ )
+
+ mock_cmmd: BaseReportState = BaseReportState(
+ mock_report_schedule, "January 1, 2021", "execution_id_example"
+ )
+ mock_cmmd.update_report_schedule_slack_v2()
+
+ recipients = mock_cmmd._report_schedule.recipients
+ assert [r.type for r in recipients] == [
+ ReportRecipientType.SLACKV2,
+ ReportRecipientType.SLACKV2,
+ ]
+ assert recipients[0].recipient_config_json == '{"target": "C1"}'
+ assert recipients[1].recipient_config_json == '{"target": "C2"}'
+
+
+def test_update_recipient_to_slack_v2_partial_failure_is_atomic(
+ mocker: MockerFixture,
+) -> None:
+ """Regression: when one of several Slack recipients fails to resolve, no
+ recipient may be left partially upgraded.
+
+ The first recipient resolves cleanly and the second references a missing
+ channel. The upgrade must raise and leave *both* recipients untouched:
+ previously the already-resolved first recipient kept its mutated
+ ``type``/``recipient_config_json``, which a later error-log commit could
+ persist as a half-upgraded schedule.
"""
- def channels_side_effect(search_string, types, exact_match):
- if search_string == "Channel-1":
- return [
- {
- "id": "id_channel_1",
- "name": "Channel-1",
- "is_member": True,
- "is_private": False,
- }
- ]
- # Second recipient: no channel found → length mismatch → UpdateFailedError
+ def fake_get_channels(search_string, types, exact_match):
+ if search_string == "channel-1":
+ return [{"id": "C1", "name": "channel-1", "is_private": False}]
+ # "missing-channel" resolves to nothing -> triggers UpdateFailedError
return []
mocker.patch(
"superset.commands.report.execute.get_channels_with_search",
- side_effect=channels_side_effect,
+ side_effect=fake_get_channels,
)
- original_config_1 = json.dumps({"target": "Channel-1"})
- original_config_2 = json.dumps({"target": "Channel-2"})
+ first_config = json.dumps({"target": "channel-1"})
+ second_config = json.dumps({"target": "missing-channel"})
mock_report_schedule = ReportSchedule(
- name="Test Report",
recipients=[
ReportRecipients(
type=ReportRecipientType.SLACK,
- recipient_config_json=original_config_1,
+ recipient_config_json=first_config,
),
ReportRecipients(
type=ReportRecipientType.SLACK,
- recipient_config_json=original_config_2,
+ recipient_config_json=second_config,
),
],
)
- mock_cmmd = BaseReportState(
+ mock_cmmd: BaseReportState = BaseReportState(
mock_report_schedule, "January 1, 2021", "execution_id_example"
)
-
with pytest.raises(UpdateFailedError):
mock_cmmd.update_report_schedule_slack_v2()
- first, second = mock_report_schedule.recipients
- # The first recipient was mutated to v2 before the second failed; it must be
- # reverted to its exact original type AND config string.
- assert first.type == ReportRecipientType.SLACK
- assert first.recipient_config_json == original_config_1
- assert second.type == ReportRecipientType.SLACK
- assert second.recipient_config_json == original_config_2
+ recipients = mock_cmmd._report_schedule.recipients
+ # Neither recipient may be mutated: types stay SLACK and configs stay as the
+ # original channel-name targets (not resolved ids).
+ assert [r.type for r in recipients] == [
+ ReportRecipientType.SLACK,
+ ReportRecipientType.SLACK,
+ ]
+ assert recipients[0].recipient_config_json == first_config
+ assert recipients[1].recipient_config_json == second_config
def test_update_recipient_to_slack_v2_pre_iteration_failure(
mocker: MockerFixture,
) -> None:
"""
- A failure raised while accessing/iterating the recipients (before the loop
- variable is bound) surfaces as ``UpdateFailedError``, not a ``NameError``
- that would mask the real error.
+ A failure raised while accessing/iterating the recipients surfaces as
+ ``UpdateFailedError``, not a ``NameError`` that masks the real error.
"""
class _ExplodingRecipients:
diff --git a/tests/unit_tests/reports/notifications/slack_tests.py b/tests/unit_tests/reports/notifications/slack_tests.py
index d1a1a99d95e..226258f32d7 100644
--- a/tests/unit_tests/reports/notifications/slack_tests.py
+++ b/tests/unit_tests/reports/notifications/slack_tests.py
@@ -16,16 +16,50 @@
# under the License.
import uuid
-from unittest.mock import MagicMock, patch
+from typing import Any
+from unittest.mock import call, MagicMock, patch
import pandas as pd
import pytest
-from slack_sdk.errors import SlackApiError
+from slack_sdk.errors import (
+ BotUserAccessError,
+ SlackApiError,
+ SlackClientConfigurationError,
+ SlackClientError,
+ SlackClientNotConnectedError,
+ SlackObjectFormationError,
+ SlackRequestError,
+ SlackTokenRotationError,
+)
+from slack_sdk.web.slack_response import SlackResponse
-from superset.reports.notifications.slackv2 import SlackV2Notification
+from superset.reports.notifications.exceptions import (
+ NotificationAuthorizationException,
+ NotificationMalformedException,
+ NotificationParamException,
+ NotificationUnprocessableException,
+)
+from superset.reports.notifications.slackv2 import (
+ _give_up_slack_api_retry,
+ SlackV2Notification,
+)
from superset.utils.core import HeaderDataType
+@pytest.fixture(autouse=True)
+def _skip_backoff_sleep():
+ """Make any @backoff.on_exception retries instant.
+
+ SlackV2Notification.send() retries up to 5 times with `backoff.expo(factor=10,
+ base=2)` — that's ~150s of real sleep on a persistently-failing send. We
+ don't care about the wall-clock waits in unit tests; patching `time.sleep`
+ inside backoff's sync runner keeps the assertion semantics (call_count,
+ raised exception type) without the wait.
+ """
+ with patch("backoff._sync.time.sleep"):
+ yield
+
+
@pytest.fixture
def mock_header_data() -> HeaderDataType:
return {
@@ -310,6 +344,10 @@ def test_send_slack(
)
+@patch(
+ "superset.utils.slack.feature_flag_manager.is_feature_enabled",
+ return_value=False,
+)
@patch("superset.reports.notifications.slack.g")
@patch("superset.reports.notifications.slack.logger")
@patch("superset.utils.slack.get_slack_client")
@@ -319,6 +357,7 @@ def test_send_slack_no_feature_flag(
slack_client_mock_util: MagicMock,
logger_mock: MagicMock,
flask_global_mock: MagicMock,
+ feature_flag_mock: MagicMock,
mock_header_data,
) -> None:
# `superset.models.helpers`, a dependency of following imports,
@@ -330,7 +369,7 @@ def test_send_slack_no_feature_flag(
execution_id = uuid.uuid4()
flask_global_mock.logs_context = {"execution_id": execution_id}
slack_client_mock.return_value.chat_postMessage.return_value = {"ok": True}
- # scopes are valid but the feature flag is off. It should still run Slack v1
+ # Even with valid scopes, ALERT_REPORT_SLACK_V2=False forces the v1 path.
slack_client_mock_util.return_value.conversations_list.return_value = {
"channels": [{"id": "foo", "name": "bar"}]
}
@@ -431,3 +470,632 @@ def test_slack_mixin_get_body_truncates_large_table(
)
body = notification._get_body(content=content)
assert "(table was truncated)" in body
+
+
+# ---------------------------------------------------------------------------
+# Bulletproof v2 send-path coverage
+#
+# The tests above exercise the chat_postMessage path (text-only sends). The
+# tests below cover files_upload_v2 across screenshots/CSV/PDF, multi-channel
+# fan-out, exception mapping, backoff, statsd, and logs propagation. Together
+# they guarantee that every observable behavior of SlackV2Notification.send()
+# is locked down before Slack v1 is removed.
+# ---------------------------------------------------------------------------
+
+
+def _make_v2_notification(content, target: str = "C12345"):
+ """Helper to build a SlackV2Notification with a given target string."""
+ from superset.reports.models import ReportRecipients, ReportRecipientType
+
+ return SlackV2Notification(
+ recipient=ReportRecipients(
+ type=ReportRecipientType.SLACKV2,
+ recipient_config_json=f'{{"target": "{target}"}}',
+ ),
+ content=content,
+ )
+
+
+def _make_content(mock_header_data, **overrides):
+ """Helper to build a minimal NotificationContent."""
+ from superset.reports.notifications.base import NotificationContent
+
+ defaults: dict = {
+ "name": "test alert",
+ "header_data": mock_header_data,
+ "description": "desc",
+ }
+ defaults.update(overrides)
+ return NotificationContent(**defaults)
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_with_single_screenshot_calls_files_upload_v2(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ flask_global_mock.logs_context = {"execution_id": uuid.uuid4()}
+ content = _make_content(mock_header_data, screenshots=[b"screenshot-bytes"])
+ notification = _make_v2_notification(content, target="C12345")
+
+ notification.send()
+
+ upload = slack_client_mock.return_value.files_upload_v2
+ upload.assert_called_once()
+ kwargs = upload.call_args.kwargs
+ assert kwargs["channel"] == "C12345"
+ assert kwargs["file"] == b"screenshot-bytes"
+ assert kwargs["title"] == "test alert"
+ assert kwargs["filename"] == "test alert.png"
+ assert "test alert" in kwargs["initial_comment"]
+ # chat_postMessage should NOT be called when files are present
+ slack_client_mock.return_value.chat_postMessage.assert_not_called()
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_with_multiple_screenshots_uploads_each(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ flask_global_mock.logs_context = {}
+ content = _make_content(
+ mock_header_data, screenshots=[b"shot-1", b"shot-2", b"shot-3"]
+ )
+ notification = _make_v2_notification(content, target="C12345")
+
+ notification.send()
+
+ upload = slack_client_mock.return_value.files_upload_v2
+ assert upload.call_count == 3
+ uploaded_files = [c.kwargs["file"] for c in upload.call_args_list]
+ assert uploaded_files == [b"shot-1", b"shot-2", b"shot-3"]
+ # All three uploads target the same single channel
+ for c in upload.call_args_list:
+ assert c.kwargs["channel"] == "C12345"
+ assert c.kwargs["filename"] == "test alert.png"
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_with_csv_calls_files_upload_v2(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ flask_global_mock.logs_context = {}
+ content = _make_content(mock_header_data, csv=b"col1,col2\n1,2\n")
+ notification = _make_v2_notification(content, target="C12345")
+
+ notification.send()
+
+ upload = slack_client_mock.return_value.files_upload_v2
+ upload.assert_called_once()
+ kwargs = upload.call_args.kwargs
+ assert kwargs["file"] == b"col1,col2\n1,2\n"
+ assert kwargs["filename"] == "test alert.csv"
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_with_pdf_calls_files_upload_v2(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ flask_global_mock.logs_context = {}
+ content = _make_content(mock_header_data, pdf=b"%PDF-1.4...")
+ notification = _make_v2_notification(content, target="C12345")
+
+ notification.send()
+
+ upload = slack_client_mock.return_value.files_upload_v2
+ upload.assert_called_once()
+ kwargs = upload.call_args.kwargs
+ assert kwargs["file"] == b"%PDF-1.4..."
+ assert kwargs["filename"] == "test alert.pdf"
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_to_multiple_channels_uploads_per_channel(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ flask_global_mock.logs_context = {}
+ content = _make_content(mock_header_data, screenshots=[b"shot-1", b"shot-2"])
+ notification = _make_v2_notification(content, target="C12345,C67890,C11111")
+
+ notification.send()
+
+ upload = slack_client_mock.return_value.files_upload_v2
+ # 3 channels x 2 files = 6 uploads
+ assert upload.call_count == 6
+ seen = {(c.kwargs["channel"], c.kwargs["file"]) for c in upload.call_args_list}
+ assert seen == {
+ ("C12345", b"shot-1"),
+ ("C12345", b"shot-2"),
+ ("C67890", b"shot-1"),
+ ("C67890", b"shot-2"),
+ ("C11111", b"shot-1"),
+ ("C11111", b"shot-2"),
+ }
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_text_only_uses_chat_post_message(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ flask_global_mock.logs_context = {}
+ content = _make_content(mock_header_data)
+ notification = _make_v2_notification(content, target="C12345,C67890")
+
+ notification.send()
+
+ # No files → chat_postMessage per channel, no files_upload_v2 calls
+ slack_client_mock.return_value.files_upload_v2.assert_not_called()
+ chat = slack_client_mock.return_value.chat_postMessage
+ assert chat.call_count == 2
+ channels = sorted(c.kwargs["channel"] for c in chat.call_args_list)
+ assert channels == ["C12345", "C67890"]
+
+
+def test_v2_inline_files_precedence(mock_header_data) -> None:
+ """CSV beats screenshots beats PDF; only one inline-file type is sent."""
+ content = _make_content(
+ mock_header_data,
+ csv=b"a,b\n1,2",
+ screenshots=[b"shot-1"],
+ pdf=b"%PDF",
+ )
+ notification = _make_v2_notification(content, target="C12345")
+ file_type, files = notification._get_inline_files()
+ assert file_type == "csv"
+ assert files == [b"a,b\n1,2"]
+
+ content = _make_content(
+ mock_header_data,
+ screenshots=[b"shot-1"],
+ pdf=b"%PDF",
+ )
+ notification = _make_v2_notification(content, target="C12345")
+ file_type, files = notification._get_inline_files()
+ assert file_type == "png"
+ assert files == [b"shot-1"]
+
+ content = _make_content(mock_header_data, pdf=b"%PDF")
+ notification = _make_v2_notification(content, target="C12345")
+ file_type, files = notification._get_inline_files()
+ assert file_type == "pdf"
+ assert files == [b"%PDF"]
+
+
+@pytest.mark.parametrize(
+ ("slack_exc_factory", "expected_exc"),
+ [
+ (
+ lambda: BotUserAccessError("bot user blocked"),
+ NotificationParamException,
+ ),
+ (
+ lambda: SlackRequestError("bad request"),
+ NotificationParamException,
+ ),
+ (
+ lambda: SlackClientConfigurationError("misconfigured"),
+ NotificationParamException,
+ ),
+ (
+ lambda: SlackObjectFormationError("malformed"),
+ NotificationMalformedException,
+ ),
+ (
+ lambda: SlackTokenRotationError(
+ SlackApiError(message="rotation failed", response={"ok": False})
+ ),
+ NotificationAuthorizationException,
+ ),
+ (
+ lambda: SlackClientNotConnectedError("offline"),
+ NotificationUnprocessableException,
+ ),
+ (
+ # Fallback: any other SlackClientError becomes Unprocessable.
+ lambda: SlackClientError("misc client error"),
+ NotificationUnprocessableException,
+ ),
+ ],
+)
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_maps_slack_sdk_exceptions(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ slack_exc_factory,
+ expected_exc,
+ mock_header_data,
+) -> None:
+ flask_global_mock.logs_context = {}
+ slack_client_mock.return_value.chat_postMessage.side_effect = slack_exc_factory()
+
+ content = _make_content(mock_header_data)
+ notification = _make_v2_notification(content, target="C12345")
+
+ with pytest.raises(expected_exc):
+ notification.send()
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_retries_on_transient_slack_api_error(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ """`@backoff.on_exception(NotificationUnprocessableException, max_tries=5)`
+ retries the wrapped exception that send() actually raises.
+
+ A persistent Slack rate-limit (or any other transient failure that maps to
+ NotificationUnprocessableException) results in exactly max_tries=5 send
+ attempts before the final exception propagates. This mirrors the existing
+ pattern in webhook.py.
+ """
+ flask_global_mock.logs_context = {}
+ slack_client_mock.return_value.chat_postMessage.side_effect = SlackApiError(
+ message="rate limited", response={"ok": False, "error": "ratelimited"}
+ )
+
+ content = _make_content(mock_header_data)
+ notification = _make_v2_notification(content, target="C12345")
+
+ with pytest.raises(NotificationUnprocessableException):
+ notification.send()
+
+ assert slack_client_mock.return_value.chat_postMessage.call_count == 5
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_retries_then_succeeds_on_transient_failure(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ """The point of switching backoff to NotificationUnprocessableException is
+ that a *transient* failure now retries and the send ultimately succeeds —
+ behavior the old (dead) SlackApiError decorator never delivered. Fail twice,
+ then succeed: send() must return normally after exactly 3 attempts and still
+ record the success gauge.
+ """
+ flask_global_mock.logs_context = {}
+ slack_client_mock.return_value.chat_postMessage.side_effect = [
+ SlackApiError(
+ message="rate limited", response={"ok": False, "error": "ratelimited"}
+ ),
+ SlackApiError(
+ message="rate limited", response={"ok": False, "error": "ratelimited"}
+ ),
+ {"ok": True},
+ ]
+
+ content = _make_content(mock_header_data)
+ notification = _make_v2_notification(content, target="C12345")
+
+ with patch(
+ "superset.extensions.stats_logger_manager.instance.gauge"
+ ) as statsd_mock:
+ notification.send()
+
+ assert slack_client_mock.return_value.chat_postMessage.call_count == 3
+ statsd_mock.assert_called_with("reports.slack.send.ok", 1)
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_retries_only_failed_channel(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ flask_global_mock.logs_context = {}
+ failed_attempts = 0
+
+ def chat_side_effect(channel: str, text: str) -> dict[str, bool]:
+ nonlocal failed_attempts
+ if channel == "C67890" and failed_attempts < 2:
+ failed_attempts += 1
+ raise SlackApiError(
+ message="rate limited", response={"ok": False, "error": "ratelimited"}
+ )
+ return {"ok": True}
+
+ slack_client_mock.return_value.chat_postMessage.side_effect = chat_side_effect
+
+ content = _make_content(mock_header_data)
+ notification = _make_v2_notification(content, target="C12345,C67890")
+
+ notification.send()
+
+ assert [
+ slack_call.kwargs["channel"]
+ for slack_call in slack_client_mock.return_value.chat_postMessage.call_args_list
+ ] == ["C12345", "C67890", "C67890", "C67890"]
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_does_not_retry_permanent_slack_api_error(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ flask_global_mock.logs_context = {}
+ slack_client_mock.return_value.chat_postMessage.side_effect = SlackApiError(
+ message="channel not found",
+ response={"ok": False, "error": "channel_not_found"},
+ )
+
+ content = _make_content(mock_header_data)
+ notification = _make_v2_notification(content, target="C12345")
+
+ with pytest.raises(NotificationUnprocessableException):
+ notification.send()
+
+ assert slack_client_mock.return_value.chat_postMessage.call_count == 1
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_does_not_retry_param_errors(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ """Non-transient errors (config / auth / malformed) are NOT retried — only
+ NotificationUnprocessableException triggers backoff. A
+ NotificationParamException-class failure (BotUserAccessError → 422) hits
+ the API exactly once and surfaces immediately.
+ """
+ flask_global_mock.logs_context = {}
+ slack_client_mock.return_value.chat_postMessage.side_effect = BotUserAccessError(
+ "bot user blocked"
+ )
+
+ content = _make_content(mock_header_data)
+ notification = _make_v2_notification(content, target="C12345")
+
+ with pytest.raises(NotificationParamException):
+ notification.send()
+
+ assert slack_client_mock.return_value.chat_postMessage.call_count == 1
+
+
+def _make_slack_response(status_code: int, data: dict[str, Any]) -> SlackResponse:
+ """Build an SDK-faithful SlackResponse that carries a real ``status_code``.
+
+ The existing retry tests pass ``SlackApiError(response={...})`` — a plain
+ dict, which has no ``status_code`` attribute. That makes
+ ``_get_slack_api_status_code`` return ``None`` and the ``429 / 5xx → retry``
+ branch in ``_give_up_slack_api_retry`` is never exercised. The real SDK hands
+ back a ``SlackResponse`` with a populated ``status_code``, so we mirror that
+ here to cover the status-code branch faithfully.
+ """
+ return SlackResponse(
+ client=None,
+ http_verb="POST",
+ api_url="https://slack.com/api/chat.postMessage",
+ req_args={},
+ data=data,
+ headers={},
+ status_code=status_code,
+ )
+
+
+@pytest.mark.parametrize(
+ "status_code,data",
+ [
+ # 5xx with an empty / unknown body: no `error` code to classify, so the
+ # status-code branch is the only thing that can decide to retry. This is
+ # the case the dict-based tests silently lost.
+ (503, {"ok": False}),
+ (502, {}),
+ # Rate limiting is retryable by status code as well as error code.
+ (429, {"ok": False, "error": "ratelimited"}),
+ ],
+)
+def test_give_up_slack_api_retry_retries_on_status_code(
+ status_code: int, data: dict[str, Any]
+) -> None:
+ response = _make_slack_response(status_code, data)
+ ex = SlackApiError(message="transient", response=response)
+
+ # give_up == False -> the request WILL be retried by backoff.
+ assert _give_up_slack_api_retry(ex) is False
+
+
+def test_give_up_slack_api_retry_gives_up_on_permanent_status_code() -> None:
+ """Control: a 4xx (non-429) with a non-transient error code is not retried,
+ even when carried by a faithful SlackResponse — so the new helper above is
+ asserting the status-code branch, not merely that everything retries.
+ """
+ response = _make_slack_response(404, {"ok": False, "error": "channel_not_found"})
+ ex = SlackApiError(message="permanent", response=response)
+
+ assert _give_up_slack_api_retry(ex) is True
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_records_statsd_gauge_on_success(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ flask_global_mock.logs_context = {}
+ content = _make_content(mock_header_data)
+ notification = _make_v2_notification(content, target="C12345")
+
+ with patch(
+ "superset.extensions.stats_logger_manager.instance.gauge"
+ ) as statsd_mock:
+ notification.send()
+
+ statsd_mock.assert_called_with("reports.slack.send.ok", 1)
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_records_statsd_gauge_warning_on_param_error(
+ slack_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ """Status<500 exceptions (NotificationParamException is 422) → .warning."""
+ flask_global_mock.logs_context = {}
+ slack_client_mock.return_value.chat_postMessage.side_effect = (
+ SlackClientConfigurationError("bad config")
+ )
+
+ content = _make_content(mock_header_data)
+ notification = _make_v2_notification(content, target="C12345")
+
+ with patch(
+ "superset.extensions.stats_logger_manager.instance.gauge"
+ ) as statsd_mock:
+ with pytest.raises(NotificationParamException):
+ notification.send()
+
+ assert call("reports.slack.send.warning", 1) in statsd_mock.call_args_list
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.logger")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_propagates_execution_id_to_logs(
+ slack_client_mock: MagicMock,
+ logger_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ """The success log carries the execution_id from g.logs_context."""
+ execution_id = uuid.uuid4()
+ flask_global_mock.logs_context = {"execution_id": execution_id}
+
+ content = _make_content(mock_header_data, screenshots=[b"shot"])
+ notification = _make_v2_notification(content, target="C12345")
+ notification.send()
+
+ logger_mock.info.assert_called_with(
+ "Report sent to slack", extra={"execution_id": execution_id}
+ )
+
+
+@patch("superset.reports.notifications.slackv2.g")
+@patch("superset.reports.notifications.slackv2.logger")
+@patch("superset.reports.notifications.slackv2.get_slack_client")
+def test_v2_send_handles_missing_logs_context(
+ slack_client_mock: MagicMock,
+ logger_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ """When g.logs_context is None or missing, the log uses execution_id=None."""
+ # Mirrors `getattr(g, "logs_context", {}) or {}` falsy-coalescing path.
+ flask_global_mock.logs_context = None
+
+ content = _make_content(mock_header_data)
+ notification = _make_v2_notification(content, target="C12345")
+ notification.send()
+
+ logger_mock.info.assert_called_with(
+ "Report sent to slack", extra={"execution_id": None}
+ )
+
+
+# ---------------------------------------------------------------------------
+# End-to-end auto-upgrade: v1 recipient → SlackV1NotificationError → upgrade →
+# row mutated to IDs → second send takes the v2 fast path with no resolution.
+# ---------------------------------------------------------------------------
+
+
+@patch(
+ "superset.utils.slack.feature_flag_manager.is_feature_enabled",
+ return_value=True,
+)
+@patch("superset.reports.notifications.slack.g")
+@patch("superset.utils.slack.get_slack_client")
+@patch("superset.reports.notifications.slack.get_slack_client")
+@patch("superset.commands.report.execute.get_channels_with_search")
+def test_auto_upgrade_round_trip_v1_to_v2(
+ get_channels_with_search_mock: MagicMock,
+ v1_client_mock: MagicMock,
+ util_client_mock: MagicMock,
+ flask_global_mock: MagicMock,
+ feature_flag_mock: MagicMock,
+ mock_header_data,
+) -> None:
+ """A v1 SLACK recipient with channel names is auto-upgraded to SlackV2 with
+ channel IDs after the first send raises SlackV1NotificationError.
+
+ The second send-as-v2 then uses the resolved IDs without further lookups.
+ """
+ from superset.commands.report.execute import BaseReportState
+ from superset.reports.models import (
+ ReportRecipients,
+ ReportRecipientType,
+ ReportSchedule,
+ )
+ from superset.reports.notifications.exceptions import SlackV1NotificationError
+ from superset.reports.notifications.slack import SlackNotification
+
+ flask_global_mock.logs_context = {}
+ # Scopes are present → should_use_v2_api returns True → v1.send raises
+ util_client_mock.return_value.conversations_list.return_value = {
+ "channels": [{"id": "C12345", "name": "general"}]
+ }
+ get_channels_with_search_mock.return_value = [
+ {"id": "C12345", "name": "general", "is_member": True, "is_private": False}
+ ]
+
+ schedule = ReportSchedule(
+ recipients=[
+ ReportRecipients(
+ type=ReportRecipientType.SLACK,
+ recipient_config_json='{"target": "general"}',
+ )
+ ]
+ )
+ content = _make_content(mock_header_data)
+
+ # Step 1: v1 send raises SlackV1NotificationError because v2 is available.
+ v1 = SlackNotification(recipient=schedule.recipients[0], content=content)
+ with pytest.raises(SlackV1NotificationError):
+ v1.send()
+
+ # Step 2: command-level upgrade rewrites the row to SlackV2 with channel IDs.
+ state = BaseReportState(schedule, "January 1, 2021", "exec-id")
+ state.update_report_schedule_slack_v2()
+ assert schedule.recipients[0].type == ReportRecipientType.SLACKV2
+ assert schedule.recipients[0].recipient_config_json == '{"target": "C12345"}'
+
+ # Step 3: a fresh SlackV2Notification on the rewritten row sends to the
+ # resolved channel ID directly. (SlackV2Notification.send() never calls
+ # get_channels_with_search itself, so the upgraded row carrying IDs in
+ # its target is the load-bearing assertion here.)
+ with patch(
+ "superset.reports.notifications.slackv2.get_slack_client"
+ ) as v2_client_mock:
+ v2 = SlackV2Notification(recipient=schedule.recipients[0], content=content)
+ v2.send()
+ v2_client_mock.return_value.chat_postMessage.assert_called_once()
+ assert (
+ v2_client_mock.return_value.chat_postMessage.call_args.kwargs["channel"]
+ == "C12345"
+ )
diff --git a/tests/unit_tests/utils/slack_test.py b/tests/unit_tests/utils/slack_test.py
index ca3edc3f207..e2da1cd16cc 100644
--- a/tests/unit_tests/utils/slack_test.py
+++ b/tests/unit_tests/utils/slack_test.py
@@ -15,9 +15,19 @@
# specific language governing permissions and limitations
# under the License.
+import warnings
+
import pytest
+from slack_sdk.errors import (
+ SlackApiError,
+ SlackClientNotConnectedError,
+ SlackRequestError,
+)
from superset.utils.slack import (
+ _emit_v1_flag_off_deprecation,
+ _emit_v1_scope_missing_deprecation,
+ _SLACK_V1_DEPRECATION_MESSAGE,
get_channels_with_search,
should_use_v2_api,
SlackChannelTypes,
@@ -287,3 +297,226 @@ The server responded with: missing scope: channels:read"""
{"name": "general", "id": "C12345"},
{"name": "random", "id": "C67890"},
]
+
+
+# ---------------------------------------------------------------------------
+# should_use_v2_api: drives the v1→v2 auto-upgrade decision and emits
+# DeprecationWarning + logger.warning for both no-flag and missing-scope cases.
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture(autouse=True)
+def _reset_v1_warning_caches():
+ """Each test sees fresh once-per-process warning state.
+
+ The deprecation emitters are wrapped in `functools.cache` to give
+ thread-safe one-shot semantics in production. Tests need them to fire
+ again, so we clear the cache before and after each case.
+ """
+ _emit_v1_flag_off_deprecation.cache_clear()
+ _emit_v1_scope_missing_deprecation.cache_clear()
+ yield
+ _emit_v1_flag_off_deprecation.cache_clear()
+ _emit_v1_scope_missing_deprecation.cache_clear()
+
+
+class TestShouldUseV2Api:
+ def test_returns_true_when_flag_on_and_scopes_present(self, mocker):
+ mocker.patch(
+ "superset.utils.slack.feature_flag_manager.is_feature_enabled",
+ return_value=True,
+ )
+ mock_client = mocker.Mock()
+ mock_client.conversations_list.return_value = {
+ "channels": [{"id": "C1", "name": "general"}]
+ }
+ mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client)
+
+ with warnings.catch_warnings(record=True) as caught:
+ warnings.simplefilter("always")
+ assert should_use_v2_api() is True
+ assert not any(issubclass(w.category, DeprecationWarning) for w in caught)
+ mock_client.conversations_list.assert_called_once_with(
+ limit=1,
+ exclude_archived=True,
+ types="public_channel,private_channel",
+ )
+
+ def test_returns_false_when_flag_off_and_emits_deprecation_once(self, mocker):
+ mocker.patch(
+ "superset.utils.slack.feature_flag_manager.is_feature_enabled",
+ return_value=False,
+ )
+ logger_mock = mocker.patch("superset.utils.slack.logger")
+
+ with warnings.catch_warnings(record=True) as caught:
+ warnings.simplefilter("always")
+ assert should_use_v2_api() is False
+ assert should_use_v2_api() is False # second call: no new warning
+ assert should_use_v2_api() is False # third call: no new warning
+
+ deprecation_warnings = [
+ w for w in caught if issubclass(w.category, DeprecationWarning)
+ ]
+ # Exactly one DeprecationWarning across three calls.
+ assert len(deprecation_warnings) == 1
+ assert str(deprecation_warnings[0].message) == _SLACK_V1_DEPRECATION_MESSAGE
+ # logger.warning fires only once for the same reason.
+ assert logger_mock.warning.call_count == 1
+ assert (
+ "ALERT_REPORT_SLACK_V2 is disabled" in logger_mock.warning.call_args.args[0]
+ )
+
+ def test_returns_false_when_scope_missing_and_emits_deprecation_once(self, mocker):
+ mocker.patch(
+ "superset.utils.slack.feature_flag_manager.is_feature_enabled",
+ return_value=True,
+ )
+ mock_client = mocker.Mock()
+ # The Slack SDK exposes the error code as `response["error"]`; that is
+ # what `should_use_v2_api` branches on to decide whether the v1
+ # deprecation warning is the appropriate signal.
+ mock_client.conversations_list.side_effect = SlackApiError(
+ message="missing_scope", response={"ok": False, "error": "missing_scope"}
+ )
+ mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client)
+ logger_mock = mocker.patch("superset.utils.slack.logger")
+
+ with warnings.catch_warnings(record=True) as caught:
+ warnings.simplefilter("always")
+ assert should_use_v2_api() is False
+ assert should_use_v2_api() is False
+ assert should_use_v2_api() is False
+
+ deprecation_warnings = [
+ w for w in caught if issubclass(w.category, DeprecationWarning)
+ ]
+ # DeprecationWarning emitted exactly once across multiple calls.
+ assert len(deprecation_warnings) == 1
+ assert str(deprecation_warnings[0].message) == _SLACK_V1_DEPRECATION_MESSAGE
+ # The user-visible scope-missing log fires every time, since operators
+ # need to see the actionable message in their report-execution logs.
+ assert logger_mock.warning.call_count == 3
+ for c in logger_mock.warning.call_args_list:
+ assert "channels:read" in c.args[0]
+ assert "groups:read" in c.args[0]
+
+ def test_scope_missing_detected_via_slack_response_data_shape(self, mocker):
+ """The real Slack SDK sets `SlackApiError.response` to a `SlackResponse`
+ whose payload lives in `.data` — not a plain dict. This is the
+ production-default code path, so it must be exercised directly:
+ `should_use_v2_api` reads the error code via `getattr(response, "data")`
+ and the scope-missing branch must still fire.
+ """
+ mocker.patch(
+ "superset.utils.slack.feature_flag_manager.is_feature_enabled",
+ return_value=True,
+ )
+ mock_client = mocker.Mock()
+ # MockResponse mirrors SlackResponse: the error payload is on `.data`,
+ # exactly as the live SDK delivers it.
+ mock_client.conversations_list.side_effect = SlackApiError(
+ message="missing_scope",
+ response=MockResponse({"ok": False, "error": "missing_scope"}),
+ )
+ mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client)
+ logger_mock = mocker.patch("superset.utils.slack.logger")
+
+ with warnings.catch_warnings(record=True) as caught:
+ warnings.simplefilter("always")
+ assert should_use_v2_api() is False
+
+ deprecation_warnings = [
+ w for w in caught if issubclass(w.category, DeprecationWarning)
+ ]
+ assert len(deprecation_warnings) == 1
+ assert logger_mock.warning.call_count == 1
+ assert "channels:read" in logger_mock.warning.call_args.args[0]
+
+ @pytest.mark.parametrize(
+ "error_code",
+ ["invalid_auth", "ratelimited", "fatal_error", "account_inactive", ""],
+ )
+ def test_returns_false_without_scope_warning_on_other_slack_errors(
+ self, error_code: str, mocker
+ ):
+ """Non-scope `SlackApiError` codes must NOT be reported as a missing
+ scope — that mislabels invalid_auth, ratelimited, or server-side
+ failures as a permission problem and sends operators chasing the wrong
+ fix. The probe still falls back to v1 so the send isn't lost, but the
+ log line is generic and no DeprecationWarning fires.
+ """
+ mocker.patch(
+ "superset.utils.slack.feature_flag_manager.is_feature_enabled",
+ return_value=True,
+ )
+ mock_client = mocker.Mock()
+ mock_client.conversations_list.side_effect = SlackApiError(
+ message=error_code or "unknown",
+ response={"ok": False, "error": error_code}
+ if error_code
+ else {"ok": False},
+ )
+ mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client)
+ logger_mock = mocker.patch("superset.utils.slack.logger")
+
+ with warnings.catch_warnings(record=True) as caught:
+ warnings.simplefilter("always")
+ assert should_use_v2_api() is False
+
+ deprecation_warnings = [
+ w for w in caught if issubclass(w.category, DeprecationWarning)
+ ]
+ assert deprecation_warnings == []
+ assert logger_mock.warning.call_count == 1
+ msg = logger_mock.warning.call_args.args[0]
+ assert "probe failed" in msg
+ assert "channels:read" not in msg
+
+ @pytest.mark.parametrize(
+ "exception",
+ [
+ SlackClientNotConnectedError("transport closed"),
+ SlackRequestError("bad request args"),
+ ],
+ )
+ def test_returns_false_on_slack_sdk_client_error_from_probe(
+ self, exception: Exception, mocker
+ ):
+ """Non-`SlackApiError` SDK failures (e.g. `SlackClientNotConnectedError`,
+ `SlackRequestError`) are not subclasses of `SlackApiError`, so before the
+ fix they escaped the probe raw. Because `should_use_v2_api` runs *before*
+ the mapped Slack send `try`, that aborted the whole recipient loop instead
+ of failing a single recipient. The probe must now treat any SDK client
+ error as "v2 unavailable" and fall back to the deprecated v1 API so the
+ send still flows through the per-recipient error handling.
+ """
+ mocker.patch(
+ "superset.utils.slack.feature_flag_manager.is_feature_enabled",
+ return_value=True,
+ )
+ mock_client = mocker.Mock()
+ mock_client.conversations_list.side_effect = exception
+ mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client)
+ logger_mock = mocker.patch("superset.utils.slack.logger")
+
+ assert should_use_v2_api() is False
+ assert logger_mock.warning.call_count == 1
+ assert "probe failed to connect" in logger_mock.warning.call_args.args[0]
+
+ def test_propagates_non_sdk_errors_from_probe(self, mocker):
+ """A truly unexpected, non-SDK exception (e.g. a programming error) still
+ propagates out of `should_use_v2_api` rather than being silently swallowed
+ as a v1 fallback — only Slack SDK client errors are treated as a graceful
+ "v2 unavailable" signal.
+ """
+ mocker.patch(
+ "superset.utils.slack.feature_flag_manager.is_feature_enabled",
+ return_value=True,
+ )
+ mock_client = mocker.Mock()
+ mock_client.conversations_list.side_effect = RuntimeError("unexpected boom")
+ mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client)
+
+ with pytest.raises(RuntimeError):
+ should_use_v2_api()