From 909acdbae41ff60628d7e3f2cd12c6204064d763 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Wed, 6 May 2026 12:41:28 -0700 Subject: [PATCH 1/5] chore(reports): deprecate Slack v1, default ALERT_REPORT_SLACK_V2 to True, harden v2 tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flips the ALERT_REPORT_SLACK_V2 feature flag default to True so the v2 auto-upgrade path runs out of the box, and adds one-shot DeprecationWarning + logger.warning emissions when v1 still runs (flag explicitly off, or bot missing the channels:read scope). Slack retired the legacy files.upload endpoint in 2025, so v1 file uploads are already broken at the API level — only text-only chat_postMessage sends still succeed via the legacy path. The bulk of the change is bulletproof unit-test coverage for SlackV2Notification ahead of v1 removal in the next major: - files_upload_v2 invocation with PNG (single + multiple), CSV, and PDF, asserting channel, file, title, filename, and initial_comment kwargs - multi-channel fan-out (3 channels x 2 files = 6 uploads) and text-only multi-channel chat_postMessage - inline-file precedence (CSV beats screenshots beats PDF) - parametrized exception mapping across 7 slack_sdk error types -> the 4 NotificationException subclasses - statsd .ok and .warning gauge emission via the @statsd_gauge decorator - execution_id propagation from g.logs_context to the success log, plus the falsy g.logs_context fallback path - end-to-end auto-upgrade round-trip: v1 SLACK recipient with channel names raises SlackV1NotificationError -> update_report_schedule_slack_v2 rewrites the row to channel IDs -> SlackV2Notification fast-paths the next send with no further channel resolution - should_use_v2_api() warning behavior: deprecation warning emitted exactly once across multiple calls in both the flag-off and scope-missing paths, with the scope-missing logger.warning continuing to fire each call so operators see the actionable scope hint in their report-execution logs Also locks in current behavior of the @backoff.on_exception(SlackApiError, ...) decorator on send(): because send() catches every SlackApiError internally and re-raises as NotificationUnprocessableException, backoff never sees the target exception type and no retries actually fire. Test asserts call_count == 1 with a docstring marking this as a known design issue to address separately. Co-Authored-By: Claude Opus 4.7 (1M context) --- UPDATING.md | 10 + docs/static/feature-flags.json | 4 +- superset/config.py | 10 +- superset/reports/notifications/slack.py | 6 +- superset/utils/slack.py | 35 +- .../reports/notifications/slack_tests.py | 496 +++++++++++++++++- tests/unit_tests/utils/slack_test.py | 98 +++- 7 files changed, 645 insertions(+), 14 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 3d42f2b3d4e1..5241c54ddb9b 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -24,6 +24,16 @@ assists people when migrating to a new version. ## Next +### Slack v1 deprecated; `ALERT_REPORT_SLACK_V2` defaults to `True` + +The legacy Slack integration for Alerts and Reports (`Slack` recipient type, `files.upload` API) is deprecated and will be removed in the next major release. The `ALERT_REPORT_SLACK_V2` feature flag now defaults to `True`. + +**Why this is changing:** Slack retired the `files.upload` endpoint in 2025, so v1 sends that include screenshots, CSVs, or PDFs already fail at the API level. Only text-only `chat_postMessage` sends still succeed via the legacy path. The v2 integration uses `files_upload_v2` and stores stable channel IDs instead of mutable channel names. + +**Operator action required:** Grant your Slack bot the `channels:read` scope (and `groups:read` if you use private channels). With that scope in place, existing `Slack` recipients are auto-upgraded to `SlackV2` on their next successful send — channel names are resolved to channel IDs and the recipient row is rewritten in place. Recipients whose channels can't be resolved will continue to send via the v1 `chat_postMessage` path for text-only alerts until v1 is removed. + +**If you previously set `ALERT_REPORT_SLACK_V2: False` explicitly:** you'll see a one-shot `DeprecationWarning` and a `logger.warning` describing the action above. To suppress, remove the override or grant the scope and let the auto-upgrade run. + ### Granular Export Controls A new feature flag `GRANULAR_EXPORT_CONTROLS` introduces three fine-grained permissions that replace the legacy `can_csv` permission: diff --git a/docs/static/feature-flags.json b/docs/static/feature-flags.json index 0da6171b46f7..51f7b47cf1b9 100644 --- a/docs/static/feature-flags.json +++ b/docs/static/feature-flags.json @@ -116,9 +116,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 the `channels:read` scope so existing v1 recipients can be auto-upgraded on their next send. Without that scope, 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." }, { "name": "ALERT_REPORT_WEBHOOK", diff --git a/superset/config.py b/superset/config.py index 30de5fa4f344..35323fb7b42b 100644 --- a/superset/config.py +++ b/superset/config.py @@ -611,9 +611,15 @@ class D3TimeFormat(TypedDict, total=False): # @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 the + # `channels:read` scope so existing v1 recipients can be auto-upgraded on + # their next send. Without that scope, 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 589fddb9aad5..ffb65bebc642 100644 --- a/superset/reports/notifications/slack.py +++ b/superset/reports/notifications/slack.py @@ -53,7 +53,11 @@ 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. Recipients with the `channels:read` scope 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/utils/slack.py b/superset/utils/slack.py index ce3d214af512..14e2552ed3ec 100644 --- a/superset/utils/slack.py +++ b/superset/utils/slack.py @@ -17,6 +17,7 @@ import logging +import warnings from typing import Callable, Optional from flask import current_app as app @@ -34,6 +35,17 @@ 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` scope so existing v1 recipients can be " + "auto-upgraded to SlackV2 on their next send." +) +_v1_flag_off_warning_emitted = False +_v1_scope_missing_warning_emitted = False + class SlackChannelTypes(StrEnum): PUBLIC = "public_channel" @@ -180,7 +192,18 @@ def get_channels_with_search( def should_use_v2_api() -> bool: + global _v1_flag_off_warning_emitted, _v1_scope_missing_warning_emitted # noqa: PLW0603 + if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_SLACK_V2"): + if not _v1_flag_off_warning_emitted: + _v1_flag_off_warning_emitted = True + warnings.warn( + _SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=2 + ) + logger.warning( + "ALERT_REPORT_SLACK_V2 is disabled; %s", + _SLACK_V1_DEPRECATION_MESSAGE, + ) return False try: client = get_slack_client() @@ -188,11 +211,15 @@ def should_use_v2_api() -> bool: logger.info("Slack API v2 is available") return True except SlackApiError: - # use the v1 api but warn with a deprecation message + if not _v1_scope_missing_warning_emitted: + _v1_scope_missing_warning_emitted = True + warnings.warn( + _SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=2 + ) 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 bot is missing the `channels:read` scope; falling back to " + "the deprecated v1 API. %s", + _SLACK_V1_DEPRECATION_MESSAGE, ) return False diff --git a/tests/unit_tests/reports/notifications/slack_tests.py b/tests/unit_tests/reports/notifications/slack_tests.py index d1a1a99d95ed..8ecf9dffd733 100644 --- a/tests/unit_tests/reports/notifications/slack_tests.py +++ b/tests/unit_tests/reports/notifications/slack_tests.py @@ -16,12 +16,27 @@ # under the License. import uuid -from unittest.mock import MagicMock, patch +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 superset.reports.notifications.exceptions import ( + NotificationAuthorizationException, + NotificationMalformedException, + NotificationParamException, + NotificationUnprocessableException, +) from superset.reports.notifications.slackv2 import SlackV2Notification from superset.utils.core import HeaderDataType @@ -310,6 +325,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 +338,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 +350,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 +451,471 @@ 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_backoff_decorator_does_not_retry_swallowed_slack_api_errors( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data, +) -> None: + """Locks in current behavior of the @backoff.on_exception(SlackApiError, ...) + decorator on `send()`. + + The decorator targets SlackApiError, but `send()` itself catches every + SlackApiError and re-raises it as a NotificationUnprocessableException + before the decorator sees it. Backoff only retries on the configured + exception type, so in practice no retries occur and exactly one Slack call + is attempted before the NotificationUnprocessableException propagates. + + If the surrounding `except` clauses are ever changed to let SlackApiError + escape (or backoff is reconfigured to retry on NotificationUnprocessable), + this assertion will start failing — at which point the retry budget should + be reviewed and this test updated. + """ + 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 ( + patch("backoff._sync.time.sleep"), + 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_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 without + # any further channel lookups. + get_channels_with_search_mock.reset_mock() + 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" + ) + get_channels_with_search_mock.assert_not_called() diff --git a/tests/unit_tests/utils/slack_test.py b/tests/unit_tests/utils/slack_test.py index 024d6cf96ee0..cdb96ddac71c 100644 --- a/tests/unit_tests/utils/slack_test.py +++ b/tests/unit_tests/utils/slack_test.py @@ -15,9 +15,17 @@ # specific language governing permissions and limitations # under the License. +import warnings + import pytest +from slack_sdk.errors import SlackApiError -from superset.utils.slack import get_channels_with_search, SlackChannelTypes +from superset.utils import slack as slack_module +from superset.utils.slack import ( + get_channels_with_search, + should_use_v2_api, + SlackChannelTypes, +) class MockResponse: @@ -216,3 +224,91 @@ def test_handle_pagination_multiple_pages(self, mocker): {"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_flags(): + """Each test sees a fresh process-level warning state.""" + slack_module._v1_flag_off_warning_emitted = False + slack_module._v1_scope_missing_warning_emitted = False + yield + slack_module._v1_flag_off_warning_emitted = False + slack_module._v1_scope_missing_warning_emitted = False + + +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) + + 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 "deprecated" in str(deprecation_warnings[0].message).lower() + # 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() + mock_client.conversations_list.side_effect = SlackApiError( + message="missing_scope", response={"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 + 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 + # 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] From dcfb70d1bd441aecde2feefc044727b9fd7a106b Mon Sep 17 00:00:00 2001 From: Joe Li Date: Wed, 6 May 2026 16:02:37 -0700 Subject: [PATCH 2/5] test(reports): mock get_channels_with_search in slack token-callable test With ALERT_REPORT_SLACK_V2 now defaulting to True, a SLACK recipient's first send triggers the v1->v2 auto-upgrade, which calls get_channels_with_search to resolve channel names to channel IDs. The existing test mocked WebClient.conversations_list to return a plain dict that lacked the `.data` attribute the upgrade path reads, so the upgrade raised "'dict' object has no attribute 'data'" and the test errored. Patch get_channels_with_search directly (matching the pattern already used by the other v2-conversion tests in this file) so the upgrade can resolve channels without going through the WebClient mock plumbing. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/integration_tests/reports/commands_tests.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index ae1c855d85ae..67e0ae2f6fc8 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -1975,11 +1975,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, ): """ @@ -1990,9 +1992,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}): From 9df1de106527f8dec7c6e6883ca8c742a3a44666 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Wed, 6 May 2026 16:11:40 -0700 Subject: [PATCH 3/5] fix(reports): make Slack v2 backoff retries actually fire on transient errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The @backoff.on_exception decorator on SlackV2Notification.send() was configured to retry on SlackApiError, but the function's own try/except catches every SlackApiError and re-raises as NotificationUnprocessableException before the decorator can see it. As a result, no retries were happening — a single transient failure (rate limit, connection blip) would fail the report immediately, defeating the intent of the 5-attempt retry budget. Switch the decorator to retry on NotificationUnprocessableException, which is the exception type that send() actually raises for transient Slack failures (SlackApiError, SlackClientNotConnectedError, and the SlackClientError catch-all). Mirrors the working pattern already in webhook.py. Non-transient errors (NotificationParamException, NotificationMalformedException, NotificationAuthorizationException) still surface immediately — they aren't retryable and shouldn't be retried. Test changes: - Replaces the prior "locks in broken behavior" regression test with test_v2_send_retries_on_transient_slack_api_error asserting call_count == 5 - Adds test_v2_send_does_not_retry_param_errors verifying that BotUserAccessError → NotificationParamException is NOT retried (call_count == 1) - Adds an autouse fixture that patches backoff._sync.time.sleep so unit-test retries complete in milliseconds rather than the ~150s of real exponential backoff. Without this, the parametrized exception-mapping cases that map to NotificationUnprocessableException balloon the test runtime by ~75s The v1 SlackNotification has the same bug but is being deprecated in this release; not worth fixing there since v1's file_uploads endpoint is already dead at Slack's side and only the text-only chat_postMessage path still works. Co-Authored-By: Claude Opus 4.7 (1M context) --- superset/reports/notifications/slackv2.py | 13 +++- .../reports/notifications/slack_tests.py | 67 ++++++++++++++----- 2 files changed, 61 insertions(+), 19 deletions(-) diff --git a/superset/reports/notifications/slackv2.py b/superset/reports/notifications/slackv2.py index 4f4cf84302f0..8bd66b6ebe33 100644 --- a/superset/reports/notifications/slackv2.py +++ b/superset/reports/notifications/slackv2.py @@ -77,7 +77,18 @@ def _get_inline_files( return ("pdf", [self._content.pdf]) return (None, []) - @backoff.on_exception(backoff.expo, SlackApiError, factor=10, base=2, max_tries=5) + # Retry on NotificationUnprocessableException (the wrapper that send() + # raises for transient Slack failures: SlackApiError, connection errors, + # and the SlackClientError catch-all). Retrying on SlackApiError directly + # would never fire because the try/except below converts it before the + # decorator can see it. Mirrors the pattern in webhook.py. + @backoff.on_exception( + backoff.expo, + NotificationUnprocessableException, + factor=10, + base=2, + max_tries=5, + ) @statsd_gauge("reports.slack.send") def send(self) -> None: global_logs_context = getattr(g, "logs_context", {}) or {} diff --git a/tests/unit_tests/reports/notifications/slack_tests.py b/tests/unit_tests/reports/notifications/slack_tests.py index 8ecf9dffd733..6bdc7c33b3f2 100644 --- a/tests/unit_tests/reports/notifications/slack_tests.py +++ b/tests/unit_tests/reports/notifications/slack_tests.py @@ -41,6 +41,20 @@ 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 { @@ -715,24 +729,18 @@ def test_v2_send_maps_slack_sdk_exceptions( @patch("superset.reports.notifications.slackv2.g") @patch("superset.reports.notifications.slackv2.get_slack_client") -def test_v2_send_backoff_decorator_does_not_retry_swallowed_slack_api_errors( +def test_v2_send_retries_on_transient_slack_api_error( slack_client_mock: MagicMock, flask_global_mock: MagicMock, mock_header_data, ) -> None: - """Locks in current behavior of the @backoff.on_exception(SlackApiError, ...) - decorator on `send()`. - - The decorator targets SlackApiError, but `send()` itself catches every - SlackApiError and re-raises it as a NotificationUnprocessableException - before the decorator sees it. Backoff only retries on the configured - exception type, so in practice no retries occur and exactly one Slack call - is attempted before the NotificationUnprocessableException propagates. - - If the surrounding `except` clauses are ever changed to let SlackApiError - escape (or backoff is reconfigured to retry on NotificationUnprocessable), - this assertion will start failing — at which point the retry budget should - be reviewed and this test updated. + """`@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( @@ -742,10 +750,33 @@ def test_v2_send_backoff_decorator_does_not_retry_swallowed_slack_api_errors( content = _make_content(mock_header_data) notification = _make_v2_notification(content, target="C12345") - with ( - patch("backoff._sync.time.sleep"), - pytest.raises(NotificationUnprocessableException), - ): + 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_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 From e67b52a6f410b08d19c1c3c496f5edcb024b8a5e Mon Sep 17 00:00:00 2001 From: Joe Li Date: Wed, 6 May 2026 16:30:15 -0700 Subject: [PATCH 4/5] docs(updating): rewrite slack v1 deprecation note in conventional one-liner style Replaces the multi-section paragraph form with the single-bullet, PR-link-prefixed style used by the historical entries in this file (see the original Slack v2 deprecation in 4.1.0 / #29264). Same information, less ceremony. Co-Authored-By: Claude Opus 4.7 (1M context) --- UPDATING.md | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 5241c54ddb9b..abfa11e186fb 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -24,15 +24,7 @@ assists people when migrating to a new version. ## Next -### Slack v1 deprecated; `ALERT_REPORT_SLACK_V2` defaults to `True` - -The legacy Slack integration for Alerts and Reports (`Slack` recipient type, `files.upload` API) is deprecated and will be removed in the next major release. The `ALERT_REPORT_SLACK_V2` feature flag now defaults to `True`. - -**Why this is changing:** Slack retired the `files.upload` endpoint in 2025, so v1 sends that include screenshots, CSVs, or PDFs already fail at the API level. Only text-only `chat_postMessage` sends still succeed via the legacy path. The v2 integration uses `files_upload_v2` and stores stable channel IDs instead of mutable channel names. - -**Operator action required:** Grant your Slack bot the `channels:read` scope (and `groups:read` if you use private channels). With that scope in place, existing `Slack` recipients are auto-upgraded to `SlackV2` on their next successful send — channel names are resolved to channel IDs and the recipient row is rewritten in place. Recipients whose channels can't be resolved will continue to send via the v1 `chat_postMessage` path for text-only alerts until v1 is removed. - -**If you previously set `ALERT_REPORT_SLACK_V2: False` explicitly:** you'll see a one-shot `DeprecationWarning` and a `logger.warning` describing the action above. To suppress, remove the override or grant the scope and let the auto-upgrade run. +- [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 retired `files.upload` in 2025, so v1 file-bearing sends already fail at the API level — only text-only `chat_postMessage` still works via the legacy path. Grant your Slack bot the `channels:read` scope (and `groups:read` for private channels) so existing `Slack` recipients can be auto-upgraded to `SlackV2` on next send. Operators who explicitly override the flag to `False` will see a one-shot `DeprecationWarning` plus a `logger.warning`; remove the override or grant the scope to clear it. ### Granular Export Controls From fe46ee6818a87588aad27143bdbb6983dfa31383 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Wed, 6 May 2026 17:14:11 -0700 Subject: [PATCH 5/5] refactor(reports): address review feedback on Slack v1 deprecation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review changes: - Replace module-level `_v1_*_warning_emitted` booleans with `functools.cache`- decorated `_emit_v1_*_deprecation` helpers. Bare module globals had a read-then-write race under multi-threaded WSGI workers; functools.cache is thread-safe under the GIL and produces actually-once-per-process semantics without the noqa: PLW0603 escape hatch. - Mention `groups:read` (in addition to `channels:read`) wherever the scope requirement appears: deprecation message constant, config.py comment, the scope-missing logger.warning, UPDATING.md, and (auto-synced) feature-flags.json. The v2 channel resolver queries both public_channel and private_channel types, so granting only `channels:read` silently breaks private-channel reports. - Add `test_propagates_non_slack_api_errors_from_probe` — locks in that any exception other than SlackApiError (network, transport) propagates out of should_use_v2_api rather than masquerading as a missing-scope warning. - Drop a tautological `assert_not_called()` on `get_channels_with_search` in the auto-upgrade round-trip test. SlackV2Notification.send() never calls that helper in any path, so the assertion was true by construction rather than by the test exercising a real fast path. - Pin assertions on the deprecation-warning *message* to the exported `_SLACK_V1_DEPRECATION_MESSAGE` constant instead of substring fragments. - Update the test autouse fixture to clear the new functools.cache caches rather than reset the now-removed module globals. Three architectural concerns from review (auto-upgrade transaction race, concurrent worker upgrade race, end-of-deprecation cleanup migration) are pre-existing on the upgrade path and tracked as separate follow-up tasks rather than expanded into this PR. Co-Authored-By: Claude Opus 4.7 (1M context) --- UPDATING.md | 2 +- docs/static/feature-flags.json | 2 +- superset/config.py | 9 ++-- superset/utils/slack.py | 50 +++++++++++-------- .../reports/notifications/slack_tests.py | 8 +-- tests/unit_tests/utils/slack_test.py | 46 +++++++++++++---- 6 files changed, 76 insertions(+), 41 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index abfa11e186fb..231fc7df3b20 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -24,7 +24,7 @@ assists people when migrating to a new version. ## Next -- [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 retired `files.upload` in 2025, so v1 file-bearing sends already fail at the API level — only text-only `chat_postMessage` still works via the legacy path. Grant your Slack bot the `channels:read` scope (and `groups:read` for private channels) so existing `Slack` recipients can be auto-upgraded to `SlackV2` on next send. Operators who explicitly override the flag to `False` will see a one-shot `DeprecationWarning` plus a `logger.warning`; remove the override or grant the scope to clear it. +- [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 retired `files.upload` in 2025, so v1 file-bearing sends already fail at the API level — only text-only `chat_postMessage` still works via the legacy path. Grant your Slack bot the `channels:read` scope (and `groups:read` if you use private channels) so existing `Slack` recipients can be auto-upgraded to `SlackV2` on next send. Operators who explicitly override the flag to `False` will see a one-shot `DeprecationWarning` plus a `logger.warning`; remove the override or grant the scopes to clear it. ### Granular Export Controls diff --git a/docs/static/feature-flags.json b/docs/static/feature-flags.json index 51f7b47cf1b9..1e1ec57f84fb 100644 --- a/docs/static/feature-flags.json +++ b/docs/static/feature-flags.json @@ -118,7 +118,7 @@ "name": "ALERT_REPORT_SLACK_V2", "default": true, "lifecycle": "testing", - "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 the `channels:read` scope so existing v1 recipients can be auto-upgraded on their next send. Without that scope, 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." + "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 the `channels:read` scope (and `groups:read` for private channels) 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." }, { "name": "ALERT_REPORT_WEBHOOK", diff --git a/superset/config.py b/superset/config.py index 35323fb7b42b..13546fa87dc8 100644 --- a/superset/config.py +++ b/superset/config.py @@ -614,10 +614,11 @@ class D3TimeFormat(TypedDict, total=False): # 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 the - # `channels:read` scope so existing v1 recipients can be auto-upgraded on - # their next send. Without that scope, 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. + # `channels:read` scope (and `groups:read` for private channels) 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": True, # Enables webhook integration for Alerts and Reports diff --git a/superset/utils/slack.py b/superset/utils/slack.py index 14e2552ed3ec..e6fa0052c7a8 100644 --- a/superset/utils/slack.py +++ b/superset/utils/slack.py @@ -16,6 +16,7 @@ # under the License. +import functools import logging import warnings from typing import Callable, Optional @@ -40,11 +41,27 @@ "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` scope so existing v1 recipients can be " - "auto-upgraded to SlackV2 on their next send." + "bot the `channels:read` (and `groups:read` if you use private channels) " + "scopes so existing v1 recipients can be auto-upgraded to SlackV2 on " + "their next send." ) -_v1_flag_off_warning_emitted = False -_v1_scope_missing_warning_emitted = False + + +# 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): @@ -192,18 +209,8 @@ def get_channels_with_search( def should_use_v2_api() -> bool: - global _v1_flag_off_warning_emitted, _v1_scope_missing_warning_emitted # noqa: PLW0603 - if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_SLACK_V2"): - if not _v1_flag_off_warning_emitted: - _v1_flag_off_warning_emitted = True - warnings.warn( - _SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=2 - ) - logger.warning( - "ALERT_REPORT_SLACK_V2 is disabled; %s", - _SLACK_V1_DEPRECATION_MESSAGE, - ) + _emit_v1_flag_off_deprecation() return False try: client = get_slack_client() @@ -211,14 +218,13 @@ def should_use_v2_api() -> bool: logger.info("Slack API v2 is available") return True except SlackApiError: - if not _v1_scope_missing_warning_emitted: - _v1_scope_missing_warning_emitted = True - warnings.warn( - _SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=2 - ) + # 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` scope; falling back to " - "the deprecated v1 API. %s", + "Slack bot is missing the `channels:read` (and `groups:read` for " + "private channels) scope; falling back to the deprecated v1 API. " + "%s", _SLACK_V1_DEPRECATION_MESSAGE, ) return False diff --git a/tests/unit_tests/reports/notifications/slack_tests.py b/tests/unit_tests/reports/notifications/slack_tests.py index 6bdc7c33b3f2..f1cb03d8cc75 100644 --- a/tests/unit_tests/reports/notifications/slack_tests.py +++ b/tests/unit_tests/reports/notifications/slack_tests.py @@ -936,9 +936,10 @@ def test_auto_upgrade_round_trip_v1_to_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 without - # any further channel lookups. - get_channels_with_search_mock.reset_mock() + # 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: @@ -949,4 +950,3 @@ def test_auto_upgrade_round_trip_v1_to_v2( v2_client_mock.return_value.chat_postMessage.call_args.kwargs["channel"] == "C12345" ) - get_channels_with_search_mock.assert_not_called() diff --git a/tests/unit_tests/utils/slack_test.py b/tests/unit_tests/utils/slack_test.py index cdb96ddac71c..b041b22b1e0c 100644 --- a/tests/unit_tests/utils/slack_test.py +++ b/tests/unit_tests/utils/slack_test.py @@ -18,10 +18,12 @@ import warnings import pytest -from slack_sdk.errors import SlackApiError +from slack_sdk.errors import SlackApiError, SlackClientNotConnectedError -from superset.utils import slack as slack_module 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, @@ -233,13 +235,18 @@ def test_handle_pagination_multiple_pages(self, mocker): @pytest.fixture(autouse=True) -def _reset_v1_warning_flags(): - """Each test sees a fresh process-level warning state.""" - slack_module._v1_flag_off_warning_emitted = False - slack_module._v1_scope_missing_warning_emitted = False +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 - slack_module._v1_flag_off_warning_emitted = False - slack_module._v1_scope_missing_warning_emitted = False + _emit_v1_flag_off_deprecation.cache_clear() + _emit_v1_scope_missing_deprecation.cache_clear() class TestShouldUseV2Api: @@ -277,7 +284,7 @@ def test_returns_false_when_flag_off_and_emits_deprecation_once(self, mocker): ] # Exactly one DeprecationWarning across three calls. assert len(deprecation_warnings) == 1 - assert "deprecated" in str(deprecation_warnings[0].message).lower() + 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 ( @@ -307,8 +314,29 @@ def test_returns_false_when_scope_missing_and_emits_deprecation_once(self, mocke ] # 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_propagates_non_slack_api_errors_from_probe(self, mocker): + """Any non-`SlackApiError` exception from the probe (network issue, + unexpected SDK error) propagates out of `should_use_v2_api` rather than + silently falling back to v1. Falling back on a non-API error would + mask real bugs as "you don't have channels:read", which is misleading. + """ + mocker.patch( + "superset.utils.slack.feature_flag_manager.is_feature_enabled", + return_value=True, + ) + mock_client = mocker.Mock() + mock_client.conversations_list.side_effect = SlackClientNotConnectedError( + "transport closed" + ) + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + + with pytest.raises(SlackClientNotConnectedError): + should_use_v2_api()