diff --git a/UPDATING.md b/UPDATING.md index 03f851a9264..0c61ff36561 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -870,6 +870,8 @@ With the flag enabled: `DELETE /api/v1/chart/` no longer hard-deletes the ch - [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` still works via the legacy path. Grant your Slack bot the `channels:read` and `groups:read` scopes so existing `Slack` recipients can be auto-upgraded to `SlackV2` on next send. Operators who explicitly override the flag to `False`, or whose Slack bot is missing those scopes, will see deprecation warnings while text-only sends continue through the legacy path. +- [42089](https://github.com/apache/superset/pull/42089) automatically upgrades resolvable Slack v1 recipients, preserves text-only v1 delivery with execution warnings when migration cannot finish, and rejects retired v1 file uploads with actionable scope guidance. Slack delivery uses at-most-once terminal writes and a schedule-wide retry budget configured by `SLACK_SEND_RETRY_MAX_TIME`, clamped to the report's remaining working timeout. Deployments using `SupersetMetastoreCache` for the Slack channel cache must schedule the `slack.cache_channels` Celery task to repopulate misses outside report transactions; see [Alerts and Reports](https://superset.apache.org/admin-docs/configuration/alerts-reports#slack-delivery-timeouts-and-retries). + ### 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/` continues to **hard-delete permanently** — nothing is recoverable. Enable `SOFT_DELETE` to get the behavior described below. diff --git a/docs/admin_docs/configuration/alerts-reports.mdx b/docs/admin_docs/configuration/alerts-reports.mdx index caa822fa1f3..d07dead9de2 100644 --- a/docs/admin_docs/configuration/alerts-reports.mdx +++ b/docs/admin_docs/configuration/alerts-reports.mdx @@ -83,6 +83,28 @@ SLACK_CACHE_TIMEOUT = int(timedelta(days=2).total_seconds()) SLACK_API_RATE_LIMIT_RETRY_COUNT = 5 ``` +When the cache backend is `SupersetMetastoreCache`, report execution does not +write channel listings into the cache because that backend commits the report's +database session. Schedule the dedicated warm-up task so cache misses are +repopulated outside report transactions: + +```python +from celery.schedules import crontab + +from superset.config import CeleryConfig + +class CustomCeleryConfig(CeleryConfig): + beat_schedule = { + **CeleryConfig.beat_schedule, + "slack.cache_channels": { + "task": "slack.cache_channels", + "schedule": crontab(minute="0", hour="*"), + }, + } + +CELERY_CONFIG = CustomCeleryConfig +``` + #### Slack Enterprise Grid (org-scoped tokens) On a Slack Enterprise Grid org, an org-scoped token spans multiple workspaces, so @@ -98,6 +120,38 @@ SLACK_TEAM_ID = "T01234567" This defaults to `None` and only needs to be set when using an org-scoped token; it is accepted but ignored for standard workspace-level tokens. +#### Slack delivery timeouts and retries + +Slack delivery uses a request timeout and an application retry budget: + +```python +# Timeout for one Slack API request, in seconds +SLACK_API_TIMEOUT = 30 + +# Retry budget shared by every Slack destination and upload phase +SLACK_SEND_RETRY_MAX_TIME = 150 + +# Number of explicit HTTP 429 responses retried using Slack's Retry-After value +SLACK_API_RATE_LIMIT_RETRY_COUNT = 2 + +# Cooldown after an on-demand channel-cache refresh +SLACK_CHANNEL_REFRESH_COOLDOWN_SECONDS = 300 +``` + +All channels and upload phases in one report execution share a single +`SLACK_SEND_RETRY_MAX_TIME` budget. This prevents a large recipient list from +multiplying the report's wall-clock retry time. The budget is also clamped to +the report's remaining working timeout, leaving Celery's configured timeout lag +available for final state persistence. The effective configured budget is at +least one second longer than `SLACK_API_TIMEOUT`. + +To avoid posting the same report twice, Superset does not replay terminal +`chat.postMessage` or `files.completeUploadExternal` operations after ambiguous +server or transport failures. Explicit Slack HTTP 429 responses remain +retryable. These delivery settings and semantics apply to Slack v2 reports and +legacy text-only Slack delivery, independently of the +`ALERT_REPORT_SLACK_V2` feature flag. + ### Webhook integration Superset can send alert and report notifications to any HTTP endpoint — useful for chat platforms, incident management tools, or custom automation. diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 81789f2a7c9..0e6b3ff34a0 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -57,6 +57,7 @@ from superset.commands.report.exceptions import ( ReportScheduleXlsxFailedError, ReportScheduleXlsxTimeout, ) +from superset.commands.report.slack_upgrade import SlackV1UpgradeCoordinator from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType from superset.daos.report import ( REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER, @@ -81,14 +82,20 @@ from superset.reports.notifications.base import NotificationContent from superset.reports.notifications.exceptions import ( NotificationError, NotificationParamException, - SlackV1NotificationError, +) +from superset.reports.notifications.slack import SlackNotification +from superset.reports.notifications.slack_transport import ( + get_slack_send_retry_deadline, ) from superset.subjects.types import SubjectType from superset.tasks.utils import get_executor from superset.utils import json -from superset.utils.core import HeaderDataType, override_user, recipients_string_to_list +from superset.utils.core import HeaderDataType, override_user from superset.utils.csv import get_chart_csv_data, get_chart_dataframe -from superset.utils.decorators import logs_context, transaction +from superset.utils.decorators import ( + logs_context, + transaction, +) from superset.utils.file import sanitize_title from superset.utils.pdf import build_pdf_from_screenshots from superset.utils.report_execution import ( @@ -98,7 +105,6 @@ from superset.utils.report_execution import ( resolve_report_execution_budget_seconds, ) from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot -from superset.utils.slack import get_channels_with_search, SlackChannelTypes from superset.utils.urls import get_url_path if TYPE_CHECKING: @@ -279,7 +285,26 @@ class BaseReportState: self._start_dttm: datetime = datetime.now(timezone.utc).replace(tzinfo=None) self._execution_id = execution_id self._report_execution_context = report_execution_context - self._filter_warnings: list[str] = [] + self._execution_warnings: list[str] = [] + self._slack_v1_upgrade = SlackV1UpgradeCoordinator( + report_schedule, + execution_id, + self._execution_warnings, + ) + + def _get_slack_retry_deadline(self) -> float: + """Return the monotonic deadline for Slack delivery in this execution.""" + report_deadline = None + if self._report_schedule.working_timeout is not None: + elapsed = ( + datetime.now(timezone.utc).replace(tzinfo=None) - self._start_dttm + ).total_seconds() + remaining = max( + float(self._report_schedule.working_timeout) - elapsed, + 0, + ) + report_deadline = time.monotonic() + remaining + return get_slack_send_retry_deadline(report_deadline) @property def _log_context(self) -> str: @@ -321,13 +346,18 @@ class BaseReportState: self, state: ReportState, error_message: Optional[str] = None, + *, + include_execution_warnings: bool = True, ) -> None: """ Update the report schedule state et al. and reflect the change in the execution log. """ self.update_report_schedule(state) - self.create_log(error_message) + self.create_log( + error_message, + include_execution_warnings=include_execution_warnings, + ) if state != ReportState.WORKING: elapsed, remaining = self._budget_values() logger.info( @@ -359,63 +389,14 @@ class BaseReportState: ) def update_report_schedule_slack_v2(self) -> None: - """ - 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. - """ - resolved: list[tuple[ReportRecipients, str]] = [] - try: - for recipient in self._report_schedule.recipients: - 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: - # 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 + """Update every Slack v1 recipient atomically to Slack v2.""" + self._slack_v1_upgrade.update_recipients() def create_log( self, error_message: Optional[str] = None, *, + include_execution_warnings: bool = True, log_state: ReportState | None = None, reuse_working_log: bool = True, ) -> None: @@ -437,6 +418,15 @@ class BaseReportState: """ from sqlalchemy.orm.exc import StaleDataError + log_message: Optional[str] + if error_message == REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER: + log_message = error_message + else: + messages = [*self._execution_warnings] if include_execution_warnings else [] + if error_message: + messages.append(error_message) + log_message = ";".join(messages) if messages else None + try: # Reuse the in-flight WORKING trigger row for this execution, if any, # so a single execution surfaces as a single log entry. @@ -464,7 +454,7 @@ class BaseReportState: log.value = self._report_schedule.last_value log.value_row_json = self._report_schedule.last_value_row_json log.state = effective_state - log.error_message = error_message + log.error_message = log_message db.session.commit() # pylint: disable=consider-using-transaction except StaleDataError as ex: # Report schedule was modified or deleted by another process @@ -586,7 +576,7 @@ class BaseReportState: self._report_schedule.get_native_filters_params() ) if filter_warnings: - self._filter_warnings.extend(filter_warnings) + self._execution_warnings.extend(filter_warnings) if anchor := dashboard_state.get("anchor"): try: anchor_list = json.loads(anchor) @@ -630,7 +620,7 @@ class BaseReportState: self._report_schedule.get_native_filters_params() ) if filter_warnings: - self._filter_warnings.extend(filter_warnings) + self._execution_warnings.extend(filter_warnings) if native_filter_params and native_filter_params != "()": # Preserve any urlParams from extra.dashboard (e.g. standalone=true) # set via API even when ALERT_REPORT_TABS is off — same merge @@ -1280,6 +1270,7 @@ class BaseReportState: text=error_text, header_data=header_data, url=url, + slack_retry_deadline=self._get_slack_retry_deadline(), include_cta=include_cta, ) @@ -1313,9 +1304,40 @@ class BaseReportState: xlsx=xlsx_data, embedded_data=embedded_data, header_data=header_data, + slack_retry_deadline=self._get_slack_retry_deadline(), include_cta=include_cta, ) + def _send_notification( + self, + notification_content: NotificationContent, + recipient: ReportRecipients, + ) -> None: + """Send one notification, upgrading Slack v1 recipients when required.""" + notification = create_notification(recipient, notification_content) + if app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"]: + logger.info( + "Would send notification for alert %s, to %s. " + "ALERT_REPORTS_NOTIFICATION_DRY_RUN is enabled, " + "set it to False to send notifications.", + self._report_schedule.name, + recipient.recipient_config_json, + ) + return + + if isinstance(notification, SlackNotification): + self._slack_v1_upgrade.send( + notification, + notification_content, + create_upgraded_notification=lambda: create_notification( + recipient, + notification_content, + ), + ) + return + + notification.send() + def _send( self, notification_content: NotificationContent, @@ -1327,54 +1349,34 @@ class BaseReportState: :raises: CommandException """ notification_errors: list[SupersetError] = [] + upgraded_delivery_failed = False + self._slack_v1_upgrade.reset() report_context = getattr(self, "_report_execution_context", None) for recipient in recipients: - notification = create_notification(recipient, notification_content) try: - try: - log_report_delivery_phase( - report_context, - getattr(recipient, "type", None), - "start", - enforce_budget=True, - ) - if app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"]: - logger.info( - "Would send notification for alert %s, to %s. " - "ALERT_REPORTS_NOTIFICATION_DRY_RUN is enabled, " - "set it to False to send notifications.", - self._report_schedule.name, - recipient.recipient_config_json, - ) - else: - notification.send() - log_report_delivery_phase( - report_context, - getattr(recipient, "type", None), - "complete", - enforce_budget=False, - ) - except SlackV1NotificationError as ex: - # The slack notification should be sent with the v2 api - logger.info( - "Attempting to upgrade the report to Slackv2: %s", str(ex) - ) - self.update_report_schedule_slack_v2() - recipient.type = ReportRecipientType.SLACKV2 - notification = create_notification(recipient, notification_content) - log_report_delivery_phase( - report_context, - recipient.type, - "retry", - enforce_budget=True, - ) - notification.send() + log_report_delivery_phase( + report_context, + getattr(recipient, "type", None), + "start", + enforce_budget=True, + ) + self._send_notification(notification_content, recipient) + log_report_delivery_phase( + report_context, + getattr(recipient, "type", None), + "complete", + enforce_budget=False, + ) except ( UpdateFailedError, NotificationParamException, NotificationError, SupersetException, ) as ex: + upgraded_delivery_failed = ( + upgraded_delivery_failed + or self._slack_v1_upgrade.is_upgraded_recipient(recipient) + ) # collect errors but keep processing them notification_errors.append( SupersetError( @@ -1385,6 +1387,12 @@ class BaseReportState: ), ) ) + except Exception: + if self._slack_v1_upgrade.is_upgraded_recipient(recipient): + self._slack_v1_upgrade.restore_upgraded_recipients() + raise + if upgraded_delivery_failed: + self._slack_v1_upgrade.restore_upgraded_recipients() if notification_errors: # log all errors but raise based on the most severe for error in notification_errors: @@ -1735,12 +1743,11 @@ class ReportNotTriggeredErrorState(BaseReportState): ) return self.send() - # Include filter warnings in the log if any were collected - warning_message = ( - ";".join(self._filter_warnings) if self._filter_warnings else None - ) # Clear any retry state from previous failed attempts in this window. self._reset_retry_counter() + warning_message = ( + ";".join(self._execution_warnings) if self._execution_warnings else None + ) self.update_report_schedule_and_log( ReportState.SUCCESS, error_message=warning_message ) @@ -1810,7 +1817,9 @@ class ReportNotTriggeredErrorState(BaseReportState): finally: try: self.update_report_schedule_and_log( - ReportState.ERROR, error_message=second_error_message + ReportState.ERROR, + error_message=second_error_message, + include_execution_warnings=False, ) except ReportScheduleUnexpectedError: # Logging failed again, log it but don't let it hide first_ex @@ -1985,15 +1994,10 @@ class ReportSuccessState(BaseReportState): raise ex from logging_ex raise - # send() succeeded — clear retry state and log success. - # Include filter warnings in the log if any were collected. - warning_message = ( - ";".join(self._filter_warnings) if self._filter_warnings else None - ) + # send() succeeded — clear retry state and log success. Any execution + # warnings are incorporated by create_log(). self._reset_retry_counter() - self.update_report_schedule_and_log( - ReportState.SUCCESS, error_message=warning_message - ) + self.update_report_schedule_and_log(ReportState.SUCCESS, error_message=None) class ReportScheduleStateMachine: # pylint: disable=too-few-public-methods diff --git a/superset/commands/report/slack_upgrade.py b/superset/commands/report/slack_upgrade.py new file mode 100644 index 00000000000..ca9f0437fb6 --- /dev/null +++ b/superset/commands/report/slack_upgrade.py @@ -0,0 +1,225 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import logging +from collections.abc import Callable +from uuid import UUID + +from flask import current_app as app + +from superset.commands.exceptions import UpdateFailedError +from superset.reports.models import ( + ReportRecipients, + ReportRecipientType, + ReportSchedule, +) +from superset.reports.notifications.base import BaseNotification, NotificationContent +from superset.reports.notifications.exceptions import ( + NotificationParamException, + SlackV1NotificationError, +) +from superset.reports.notifications.slack import ( + SLACK_V1_FILE_UPLOAD_MESSAGE, + SlackNotification, +) +from superset.reports.notifications.slack_channel_resolver import ( + resolve_slack_channel_ids, +) +from superset.utils import json +from superset.utils.decorators import record_statsd_gauge_failure +from superset.utils.slack import ( + NO_SLACK_RECIPIENTS_MESSAGE, + parse_slack_recipient_targets, + SlackChannelListingClientError, +) + +logger = logging.getLogger(__name__) + + +class SlackV1UpgradeCoordinator: + """Coordinate one atomic Slack v1 upgrade and its per-recipient fallbacks.""" + + def __init__( + self, + report_schedule: ReportSchedule, + execution_id: UUID, + execution_warnings: list[str], + ) -> None: + self._report_schedule = report_schedule + self._execution_id = execution_id + self._execution_warnings = execution_warnings + self.reset() + + def reset(self) -> None: + """Reset execution-scoped upgrade and fallback state.""" + self._upgrade_error: NotificationParamException | UpdateFailedError | None = ( + None + ) + self._fallback_recorded = False + self._upgraded_recipient_state: list[ + tuple[ReportRecipients, ReportRecipientType, str] + ] = [] + + def is_upgraded_recipient(self, recipient: ReportRecipients) -> bool: + """Return whether this execution converted the recipient to Slack v2.""" + return any( + upgraded is recipient for upgraded, _, _ in self._upgraded_recipient_state + ) + + def restore_upgraded_recipients(self) -> None: + """Restore recipients when an upgraded Slack delivery does not complete.""" + for ( + recipient, + recipient_type, + recipient_config_json, + ) in self._upgraded_recipient_state: + recipient.type = recipient_type + recipient.recipient_config_json = recipient_config_json + self._upgraded_recipient_state = [] + + def update_recipients(self) -> None: + """Resolve and atomically convert every Slack v1 recipient to Slack v2.""" + pending: list[tuple[ReportRecipients, list[str]]] = [] + try: + for recipient in self._report_schedule.recipients: + if recipient.type != ReportRecipientType.SLACK: + continue + try: + slack_recipients = json.loads(recipient.recipient_config_json) + except (TypeError, ValueError) as ex: + raise NotificationParamException( + "Invalid Slack recipient configuration" + ) from ex + target = ( + slack_recipients.get("target") + if isinstance(slack_recipients, dict) + else None + ) + if not isinstance(target, str): + raise NotificationParamException(NO_SLACK_RECIPIENTS_MESSAGE) + channels = parse_slack_recipient_targets(target.replace("#", "")) + if not channels: + raise NotificationParamException(NO_SLACK_RECIPIENTS_MESSAGE) + pending.append((recipient, channels)) + + all_targets = list( + dict.fromkeys( + channel for _, channels in pending for channel in channels + ) + ) + channel_ids = resolve_slack_channel_ids(all_targets) if all_targets else {} + resolved = [ + ( + recipient, + json.dumps( + { + "target": ",".join( + channel_ids[channel] for channel in channels + ) + } + ), + ) + for recipient, channels in pending + ] + except (NotificationParamException, SlackChannelListingClientError) as ex: + message = f"Failed to update slack recipients to v2: {ex}" + logger.warning(message) + raise NotificationParamException(message) from ex + except Exception as ex: + message = f"Failed to update slack recipients to v2: {ex}" + logger.exception(message) + raise UpdateFailedError(message) from ex + + self._upgraded_recipient_state = [ + (recipient, recipient.type, recipient.recipient_config_json) + for recipient, _ in resolved + ] + for recipient, recipient_config_json in resolved: + recipient.type = ReportRecipientType.SLACKV2 + recipient.recipient_config_json = recipient_config_json + + def send_fallback( + self, + notification: SlackNotification, + content: NotificationContent, + update_error: NotificationParamException | UpdateFailedError, + ) -> None: + """Deliver text through Slack v1 and record the first successful fallback.""" + if content.has_attachments: + record_statsd_gauge_failure("reports.slack.send", update_error) + message = ( + f"{SLACK_V1_FILE_UPLOAD_MESSAGE} " + f"Slack v2 upgrade failed: {update_error}" + ) + if isinstance(update_error, UpdateFailedError): + raise UpdateFailedError(message) from update_error + raise NotificationParamException(message) from update_error + + notification.send_legacy_text() + if self._fallback_recorded: + return + + self._execution_warnings.append( + "Slack v2 upgrade unavailable; delivered the text-only report " + f"through deprecated Slack v1: {update_error}" + ) + app.config["STATS_LOGGER"].incr("reports.slack.v1_fallback") + if isinstance(update_error, UpdateFailedError): + app.config["STATS_LOGGER"].incr("reports.slack.v1_fallback.system_error") + logger.error( + "Slack v2 upgrade failed with a system error; delivered the " + "text-only report through Slack v1 for this execution: %s", + update_error, + extra={ + "execution_id": self._execution_id, + "report_schedule_id": self._report_schedule.id, + }, + ) + else: + logger.warning( + "Slack v2 upgrade unavailable; delivered the text-only report " + "through Slack v1 for this execution: %s", + update_error, + ) + self._fallback_recorded = True + + def send( + self, + notification: SlackNotification, + content: NotificationContent, + *, + create_upgraded_notification: Callable[[], BaseNotification], + ) -> None: + """Send one Slack v1 recipient, upgrading or falling back when required.""" + if self._upgrade_error is not None: + self.send_fallback(notification, content, self._upgrade_error) + return + + try: + notification.send() + except SlackV1NotificationError as ex: + logger.info("Attempting to upgrade the report to Slackv2: %s", ex) + try: + self.update_recipients() + except ( + NotificationParamException, + UpdateFailedError, + ) as update_error: + self._upgrade_error = update_error + self.send_fallback(notification, content, update_error) + else: + create_upgraded_notification().send() diff --git a/superset/config.py b/superset/config.py index 41a2d1cdebb..27a84408cf8 100644 --- a/superset/config.py +++ b/superset/config.py @@ -2582,12 +2582,21 @@ SLACK_CACHE_TIMEOUT = int(timedelta(days=1).total_seconds()) # For workspaces with 10k+ channels, consider increasing to 10 SLACK_API_RATE_LIMIT_RETRY_COUNT = 2 +# Cooldown (in seconds) after an on-demand Slack channel-cache refresh. +SLACK_CHANNEL_REFRESH_COOLDOWN_SECONDS = 300 + # Timeout (in seconds) for outbound Slack API calls. The Slack SDK defaults to 30s; # exposing it here lets operators grant more time for large file uploads (multi-MB # CSVs, PDFs, screenshot sets) to congested or rate-limited Slack endpoints without # patching code, consistent with the SMTP/CSV/screenshot timeouts. SLACK_API_TIMEOUT = 30 +# Application retry budget (in seconds) shared by all Slack channels, files, and +# upload phases in one report execution. Increase this for slow or large-file +# reports. The effective configured value is floored at one second longer than +# SLACK_API_TIMEOUT, then clamped to the report's remaining working timeout. +SLACK_SEND_RETRY_MAX_TIME = 150 + # Window size - this will impact the rendering of the data WEBDRIVER_WINDOW = { "dashboard": (1600, 2000), diff --git a/superset/reports/notifications/base.py b/superset/reports/notifications/base.py index 5dc2d44c09a..63b08d42247 100644 --- a/superset/reports/notifications/base.py +++ b/superset/reports/notifications/base.py @@ -35,11 +35,17 @@ class NotificationContent: description: Optional[str] = "" url: Optional[str] = None # url to chart/dashboard for this screenshot embedded_data: Optional[pd.DataFrame] = None + slack_retry_deadline: Optional[float] = None # Populated only when this is a per-retry or final-failure notification retry_attempt: Optional[int] = None retry_max_attempts: Optional[int] = None include_cta: bool = True # include the call-to-action link back to Superset + @property + def has_attachments(self) -> bool: + """Return whether the notification contains any file attachment.""" + return bool(self.csv or self.xlsx or self.pdf or self.screenshots) + class BaseNotification: # pylint: disable=too-few-public-methods """ diff --git a/superset/reports/notifications/exceptions.py b/superset/reports/notifications/exceptions.py index 0776641d25e..f45d7348b32 100644 --- a/superset/reports/notifications/exceptions.py +++ b/superset/reports/notifications/exceptions.py @@ -24,6 +24,10 @@ class NotificationError(SupersetException): """ +class NotificationTransientError(NotificationError): + """Temporary third-party delivery failure surfaced as a report system error.""" + + class SlackV1NotificationError(SupersetException): """ Report should not be run with the slack v1 api diff --git a/superset/reports/notifications/slack.py b/superset/reports/notifications/slack.py index b4c68fe291a..c96bda1772c 100644 --- a/superset/reports/notifications/slack.py +++ b/superset/reports/notifications/slack.py @@ -15,23 +15,19 @@ # specific language governing permissions and limitations # under the License. import logging -from collections.abc import Sequence -from io import IOBase -from typing import Union -import backoff from flask import g +from slack_sdk import WebClient from slack_sdk.errors import ( BotUserAccessError, - SlackApiError, SlackClientConfigurationError, SlackClientError, - SlackClientNotConnectedError, SlackObjectFormationError, SlackRequestError, SlackTokenRotationError, ) +from superset import feature_flag_manager from superset.reports.models import ReportRecipientType from superset.reports.notifications.base import BaseNotification from superset.reports.notifications.exceptions import ( @@ -42,23 +38,33 @@ from superset.reports.notifications.exceptions import ( SlackV1NotificationError, ) from superset.reports.notifications.slack_mixin import SlackMixin -from superset.utils import json -from superset.utils.core import recipients_string_to_list +from superset.reports.notifications.slack_transport import ( + send_slack_text, + send_to_slack_channels, +) from superset.utils.decorators import statsd_gauge from superset.utils.slack import ( get_slack_client, + NO_SLACK_RECIPIENTS_MESSAGE, should_use_v2_api, ) logger = logging.getLogger(__name__) +SLACK_V1_FILE_UPLOAD_MESSAGE = ( + "Slack v1 file uploads are no longer supported because Slack retired " + "`files.upload`. Enable `ALERT_REPORT_SLACK_V2` and grant the Slack bot " + "both the `channels:read` and `groups:read` scopes so the recipient can " + "be upgraded to Slack v2." +) + # 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`. +# fail before attempting the retired v1 upload; 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 their first eligible send. class SlackNotification(SlackMixin, BaseNotification): # pylint: disable=too-few-public-methods """ Sends a slack notification for a report recipient @@ -66,58 +72,42 @@ class SlackNotification(SlackMixin, BaseNotification): # pylint: disable=too-fe type = ReportRecipientType.SLACK - def _get_channel(self) -> str: - """ - Get the recipient's channel(s). - Note Slack SDK uses "channel" to refer to one or more - channels. Multiple channels are demarcated by a comma. - :returns: The comma separated list of channel(s) - """ - recipient_str = json.loads(self._recipient.recipient_config_json)["target"] + @staticmethod + def _send_text( + client: WebClient, + channels: list[str], + body: str, + retry_deadline: float | None = None, + ) -> None: + """Send a text notification once to each configured channel.""" + if not channels: + raise NotificationParamException(NO_SLACK_RECIPIENTS_MESSAGE) + send_to_slack_channels( + channels, + lambda target, retry_deadline: send_slack_text( + client, + target, + body, + retry_deadline=retry_deadline, + ), + retry_deadline=retry_deadline, + ) - return ",".join(recipients_string_to_list(recipient_str)) + def _send_legacy_text(self) -> None: + if self._content.has_attachments: + raise NotificationParamException(SLACK_V1_FILE_UPLOAD_MESSAGE) - def _get_inline_files( - self, - ) -> tuple[Union[str, None], Sequence[Union[str, IOBase, bytes]]]: - if self._content.csv: - return ("csv", [self._content.csv]) - if self._content.xlsx: - return ("xlsx", [self._content.xlsx]) - if self._content.screenshots: - return ("png", self._content.screenshots) - if self._content.pdf: - 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: - file_type, files = self._get_inline_files() - title = self._content.name body = self._get_body(content=self._content) global_logs_context = getattr(g, "logs_context", {}) or {} - - # see if the v2 api will work - if should_use_v2_api(): - # if we can fetch channels, then raise an error and use the v2 api - raise SlackV1NotificationError - try: - client = get_slack_client() - channel = self._get_channel() - # files_upload returns SlackResponse as we run it in sync mode. - if files: - for file in files: - client.files_upload( - channels=channel, - file=file, - initial_comment=body, - title=title, - filetype=file_type, - ) - else: - client.chat_postMessage(channel=channel, text=body) + client = get_slack_client(for_delivery=True) + channels = self._get_channels() + self._send_text( + client, + channels, + body, + retry_deadline=self._content.slack_retry_deadline, + ) logger.info( "Report sent to slack", extra={ @@ -134,9 +124,25 @@ class SlackNotification(SlackMixin, BaseNotification): # pylint: disable=too-fe raise NotificationMalformedException(str(ex)) from ex except SlackTokenRotationError as ex: raise NotificationAuthorizationException(str(ex)) from ex - except (SlackClientNotConnectedError, SlackApiError) as ex: - raise NotificationUnprocessableException(str(ex)) from ex except SlackClientError as ex: - # this is the base class for all slack client errors - # keep it last so that it doesn't interfere with @backoff + # SlackClientError is the base class; keep it last so subclasses + # retain their more specific notification classification. raise NotificationUnprocessableException(str(ex)) from ex + + @statsd_gauge("reports.slack.send") + def send_legacy_text(self) -> None: + """Send through Slack v1 without repeating the v2 availability probe.""" + self._send_legacy_text() + + @statsd_gauge( + "reports.slack.send", + ignored_exceptions=(SlackV1NotificationError,), + ) + def send(self) -> None: + if should_use_v2_api(raise_on_error=self._content.has_attachments): + raise SlackV1NotificationError + if feature_flag_manager.is_feature_enabled("ALERT_REPORT_SLACK_V2"): + # A text-only probe can fail transiently. Still enter the coordinator + # so a successful legacy fallback records its warning and metric. + raise SlackV1NotificationError + self._send_legacy_text() diff --git a/superset/reports/notifications/slack_channel_resolver.py b/superset/reports/notifications/slack_channel_resolver.py new file mode 100644 index 00000000000..099ada0dd09 --- /dev/null +++ b/superset/reports/notifications/slack_channel_resolver.py @@ -0,0 +1,94 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from superset.reports.notifications.exceptions import NotificationParamException +from superset.utils.slack import ( + get_channels_with_search_and_cache_status, + refresh_cached_slack_channels_with_search, + SlackChannel, + SlackChannelTypes, +) + + +def _match_slack_channel( + target: str, + channels: list[SlackChannel], +) -> SlackChannel | None: + """Resolve one target with deterministic exact-ID, name, then folded-ID order.""" + match_groups = ( + [channel for channel in channels if channel["id"] == target], + [ + channel + for channel in channels + if channel["name"].casefold() == target.casefold() + ], + [ + channel + for channel in channels + if channel["id"].casefold() == target.casefold() + ], + ) + for matches in match_groups: + if len(matches) > 1: + raise NotificationParamException( + f"Slack channel target is ambiguous: {target}" + ) + if matches: + return matches[0] + return None + + +def _match_slack_channels( + targets: list[str], + channels: list[SlackChannel], +) -> tuple[dict[str, SlackChannel], list[str]]: + resolved: dict[str, SlackChannel] = {} + missing: list[str] = [] + for target in targets: + if channel := _match_slack_channel(target, channels): + resolved[target] = channel + else: + missing.append(target) + return resolved, missing + + +def resolve_slack_channel_ids( + targets: list[str], +) -> dict[str, str]: + """Resolve Slack names or IDs, refreshing only a stale cached listing.""" + search_string = ",".join(targets) + channels, used_cached_channels = get_channels_with_search_and_cache_status( + search_string=search_string, + types=[ + SlackChannelTypes.PRIVATE, + SlackChannelTypes.PUBLIC, + ], + exact_match=True, + ) + channels_by_target, missing_channels = _match_slack_channels(targets, channels) + if missing_channels and used_cached_channels: + channels = refresh_cached_slack_channels_with_search( + search_string=search_string, + types=[SlackChannelTypes.PRIVATE, SlackChannelTypes.PUBLIC], + exact_match=True, + ) + channels_by_target, missing_channels = _match_slack_channels(targets, channels) + if missing_channels: + raise NotificationParamException( + f"Could not find the following channels: {', '.join(missing_channels)}" + ) + return {target: channel["id"] for target, channel in channels_by_target.items()} diff --git a/superset/reports/notifications/slack_mixin.py b/superset/reports/notifications/slack_mixin.py index f85924a2d71..2d38e746492 100644 --- a/superset/reports/notifications/slack_mixin.py +++ b/superset/reports/notifications/slack_mixin.py @@ -18,7 +18,14 @@ import pandas as pd from flask_babel import gettext as __ +from superset.reports.models import ReportRecipients from superset.reports.notifications.base import NotificationContent +from superset.reports.notifications.exceptions import NotificationParamException +from superset.utils import json +from superset.utils.slack import ( + NO_SLACK_RECIPIENTS_MESSAGE, + parse_slack_recipient_targets, +) # Slack only allows Markdown messages up to 4k chars MAXIMUM_MESSAGE_SIZE = 4000 @@ -26,6 +33,20 @@ MAXIMUM_MESSAGE_SIZE = 4000 # pylint: disable=too-few-public-methods class SlackMixin: + _recipient: ReportRecipients + + def _get_channels(self) -> list[str]: + """Return normalized Slack targets without duplicates.""" + try: + recipient_str = json.loads(self._recipient.recipient_config_json)["target"] + except (KeyError, TypeError, ValueError) as ex: + raise NotificationParamException(NO_SLACK_RECIPIENTS_MESSAGE) from ex + + if not isinstance(recipient_str, str): + raise NotificationParamException(NO_SLACK_RECIPIENTS_MESSAGE) + + return parse_slack_recipient_targets(recipient_str) + def _message_template( self, content: NotificationContent, diff --git a/superset/reports/notifications/slack_transport.py b/superset/reports/notifications/slack_transport.py new file mode 100644 index 00000000000..3dd9225577c --- /dev/null +++ b/superset/reports/notifications/slack_transport.py @@ -0,0 +1,310 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import math +import time +from collections.abc import Callable +from functools import partial +from typing import TypeVar +from urllib.error import HTTPError + +import backoff +from flask import current_app as app +from slack_sdk import WebClient +from slack_sdk.errors import SlackApiError, SlackRequestError + +from superset.reports.notifications.exceptions import ( + NotificationTransientError, + NotificationUnprocessableException, +) +from superset.utils.slack import ( + get_slack_api_error_code, + get_slack_api_status_code, + is_retryable_slack_transport_error, + is_transient_slack_api_error, + is_transient_slack_transport_error, + SLACK_TRANSIENT_TRANSPORT_ERRORS, +) + +SLACK_API_TIMEOUT_MARGIN = 1 + +_SlackApiResult = TypeVar("_SlackApiResult") + +_SLACK_RETRY_DEADLINE_MESSAGE = ( + "Slack send retry deadline exceeded; increase SLACK_SEND_RETRY_MAX_TIME " + "for slow Slack workspaces or large-file reports" +) + + +class SlackRetryDeadlineError(Exception): + """A Slack operation was skipped because the shared send budget expired.""" + + def __init__(self) -> None: + super().__init__(_SLACK_RETRY_DEADLINE_MESSAGE) + + +class SlackChannelResponseError(SlackRequestError): + """Slack returned malformed channel-specific data before a terminal write.""" + + +_SLACK_RETRY_ERRORS = (SlackApiError, *SLACK_TRANSIENT_TRANSPORT_ERRORS) +_SLACK_CHANNEL_FAILURES = ( + *_SLACK_RETRY_ERRORS, + SlackChannelResponseError, + SlackRetryDeadlineError, +) + + +def _get_slack_send_retry_max_time() -> float: + """Return the effective schedule-wide Slack send budget in seconds.""" + configured_budget = float(app.config.get("SLACK_SEND_RETRY_MAX_TIME", 150)) + request_timeout = float(app.config.get("SLACK_API_TIMEOUT", 30)) + return max( + configured_budget, + request_timeout + SLACK_API_TIMEOUT_MARGIN, + ) + + +def get_slack_send_retry_deadline(max_deadline: float | None = None) -> float: + """Return one configured Slack deadline, optionally clamped by its caller.""" + configured_deadline = time.monotonic() + _get_slack_send_retry_max_time() + if max_deadline is None: + return configured_deadline + return min(max_deadline, configured_deadline) + + +def get_slack_request_timeout( + client_timeout: int | float, + retry_deadline: float, +) -> int: + """Clamp one Slack request timeout to the remaining delivery budget.""" + remaining = retry_deadline - time.monotonic() + if remaining <= 0: + raise SlackRetryDeadlineError + request_timeout = int(min(float(client_timeout), remaining)) + if request_timeout <= 0: + raise SlackRetryDeadlineError + return request_timeout + + +def _give_up_slack_api_retry( + ex: Exception, + *, + retry_transient_errors: bool = True, + retry_transport_errors: bool = False, +) -> bool: + """Return whether application backoff should stop retrying a Slack call.""" + if isinstance(ex, HTTPError) and ex.code == 429: + return True + if not isinstance(ex, SlackApiError): + if not retry_transient_errors: + return True + return not ( + is_transient_slack_transport_error(ex) + if retry_transport_errors + else is_retryable_slack_transport_error(ex) + ) + + status_code = get_slack_api_status_code(ex) + # call_slack_api handles HTTP 429 within the shared monotonic deadline. + # Retrying an exhausted 429 through the outer backoff would multiply the + # operator-configured rate-limit budget. + error_code = get_slack_api_error_code(ex) + if status_code == 429: + return True + if not retry_transient_errors: + return True + if is_transient_slack_api_error(ex, error_code): + return False + if status_code is not None and 400 <= status_code < 500: + return True + return bool(error_code) + + +def _get_slack_retry_after(ex: SlackApiError | HTTPError) -> float | None: + response = getattr(ex, "response", ex) + headers = getattr(response, "headers", None) + if headers is None: + return None + for name in headers.keys(): + if name.lower() != "retry-after": + continue + value = headers.get(name) + raw_value = value[0] if isinstance(value, list) else value + try: + retry_after = float(raw_value) + return max(retry_after, 0.0) if math.isfinite(retry_after) else None + except (TypeError, ValueError): + return None + return None + + +def _get_slack_rate_limit_status(ex: SlackApiError | HTTPError) -> int | None: + if isinstance(ex, HTTPError): + return ex.code + return get_slack_api_status_code(ex) + + +def call_slack_api( + method: Callable[..., _SlackApiResult], + *, + retry_deadline: float | None = None, + retry_transient_errors: bool = True, + retry_transport_errors: bool = False, + retry_rate_limits: bool = True, + **kwargs: object, +) -> _SlackApiResult: + """Call Slack with bounded retries, optionally sharing an outer deadline.""" + if retry_deadline is None: + retry_deadline = time.monotonic() + _get_slack_send_retry_max_time() + max_time = retry_deadline - time.monotonic() + if max_time <= 0: + raise SlackRetryDeadlineError + + max_rate_limit_retries = ( + max( + int(app.config.get("SLACK_API_RATE_LIMIT_RETRY_COUNT", 2)), + 0, + ) + if retry_rate_limits + else 0 + ) + rate_limit_retries = 0 + + @backoff.on_exception( + backoff.expo, + _SLACK_RETRY_ERRORS, + factor=10, + base=2, + max_tries=5, + max_time=max_time, + giveup=partial( + _give_up_slack_api_retry, + retry_transient_errors=retry_transient_errors, + retry_transport_errors=retry_transport_errors, + ), + ) + def call() -> _SlackApiResult: + nonlocal rate_limit_retries + while True: + if time.monotonic() >= retry_deadline: + raise SlackRetryDeadlineError + try: + return method(**kwargs) + except (SlackApiError, HTTPError) as ex: + if ( + _get_slack_rate_limit_status(ex) != 429 + or rate_limit_retries >= max_rate_limit_retries + ): + raise + retry_after = _get_slack_retry_after(ex) + if retry_after is None: + raise + remaining = retry_deadline - time.monotonic() + if retry_after >= remaining: + raise SlackRetryDeadlineError from ex + time.sleep(retry_after) + rate_limit_retries += 1 + + return call() + + +def call_slack_api_with_timeout( + client: WebClient, + method: Callable[..., _SlackApiResult], + *, + retry_deadline: float, + retry_transient_errors: bool = True, + retry_transport_errors: bool = False, + retry_rate_limits: bool = True, + **kwargs: object, +) -> _SlackApiResult: + """Call Slack with the SDK request timeout capped by the shared budget.""" + original_timeout = client.timeout + + def call() -> _SlackApiResult: + client.timeout = get_slack_request_timeout(original_timeout, retry_deadline) + try: + return method(**kwargs) + finally: + client.timeout = original_timeout + + return call_slack_api( + call, + retry_deadline=retry_deadline, + retry_transient_errors=retry_transient_errors, + retry_transport_errors=retry_transport_errors, + retry_rate_limits=retry_rate_limits, + ) + + +def send_slack_text( + client: WebClient, + channel: str, + text: str, + retry_deadline: float, +) -> None: + """Post one Slack text message without replaying an ambiguous terminal write.""" + call_slack_api_with_timeout( + client, + client.chat_postMessage, + retry_deadline=retry_deadline, + retry_transient_errors=False, + channel=channel, + text=text, + ) + + +def _is_transient_slack_channel_failure(error: Exception) -> bool: + """Return whether any failed destination requires retrying the report.""" + return bool( + isinstance(error, SlackRetryDeadlineError) + or isinstance(error, SlackChannelResponseError) + or is_transient_slack_transport_error(error) + or ( + isinstance(error, SlackApiError) + and is_transient_slack_api_error( + error, + get_slack_api_error_code(error), + ) + ) + ) + + +def send_to_slack_channels( + channels: list[str], + send_to_channel: Callable[[str, float], None], + *, + retry_deadline: float | None = None, +) -> None: + """Send to each channel within one schedule-wide application deadline.""" + retry_deadline = get_slack_send_retry_deadline(retry_deadline) + failures: list[tuple[str, Exception]] = [] + for channel in channels: + try: + send_to_channel(channel, retry_deadline) + except _SLACK_CHANNEL_FAILURES as ex: + failures.append((channel, ex)) + + if not failures: + return + + details = "; ".join(f"{channel}: {error}" for channel, error in failures) + message = f"Slack delivery failed for the following channels: {details}" + if any(_is_transient_slack_channel_failure(error) for _, error in failures): + raise NotificationTransientError(message) from failures[0][1] + raise NotificationUnprocessableException(message) from failures[0][1] diff --git a/superset/reports/notifications/slackv2.py b/superset/reports/notifications/slackv2.py index 7f477c2e4a3..19ad1a6ea49 100644 --- a/superset/reports/notifications/slackv2.py +++ b/superset/reports/notifications/slackv2.py @@ -15,18 +15,25 @@ # specific language governing permissions and limitations # under the License. import logging -from collections.abc import Callable, Sequence -from io import IOBase -from typing import List, Union +from contextlib import closing +from email.message import Message +from ssl import SSLContext +from urllib.error import HTTPError +from urllib.parse import urlparse +from urllib.request import ( + build_opener, + HTTPSHandler, + ProxyHandler, + Request, + urlopen, +) -import backoff from flask import g +from slack_sdk import WebClient from slack_sdk.errors import ( BotUserAccessError, - SlackApiError, SlackClientConfigurationError, SlackClientError, - SlackClientNotConnectedError, SlackObjectFormationError, SlackRequestError, SlackTokenRotationError, @@ -41,61 +48,120 @@ from superset.reports.notifications.exceptions import ( NotificationUnprocessableException, ) from superset.reports.notifications.slack_mixin import SlackMixin -from superset.utils import json -from superset.utils.core import recipients_string_to_list +from superset.reports.notifications.slack_transport import ( + call_slack_api, + call_slack_api_with_timeout, + get_slack_request_timeout, + send_slack_text, + send_to_slack_channels, + SlackChannelResponseError, +) from superset.utils.decorators import statsd_gauge -from superset.utils.slack import get_slack_client +from superset.utils.slack import ( + get_slack_client, + NO_SLACK_RECIPIENTS_MESSAGE, +) logger = logging.getLogger(__name__) -_TRANSIENT_SLACK_API_ERROR_CODES = frozenset( - { - "fatal_error", - "internal_error", - "ratelimited", - "request_timeout", - "rollup_error", - "service_unavailable", - "timeout", - } -) + +def _upload_file_data( + *, + url: str, + data: bytes, + timeout: int, + proxy: str | None, + ssl: SSLContext | None, +) -> tuple[int, str]: + """Upload bytes to Slack's issued URL using stable stdlib HTTP APIs.""" + if urlparse(url).scheme != "https": + raise SlackRequestError("Slack upload URL must use HTTPS") + request = Request(method="POST", url=url, data=data) # noqa: S310 + if proxy is not None: + if not isinstance(proxy, str): + raise SlackRequestError( + f"Invalid proxy detected: {proxy} must be a str value" + ) + response = build_opener( + ProxyHandler({"http": proxy, "https": proxy}), + HTTPSHandler(context=ssl), + ).open(request, timeout=timeout) + else: + response = urlopen(request, context=ssl, timeout=timeout) # noqa: S310 + + with closing(response): + charset = response.headers.get_content_charset() or "utf-8" + body = response.read().decode(charset) + return response.status, body -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 _upload_file_to_slack( + client: WebClient, + *, + channel: str, + file: bytes, + initial_comment: str, + title: str, + filename: str, + retry_deadline: float, +) -> None: + """Upload one file without replaying completed phases during retries.""" + data = file + upload_url_response = call_slack_api_with_timeout( + client, + client.files_getUploadURLExternal, + retry_deadline=retry_deadline, + retry_transport_errors=True, + filename=filename, + length=len(data), + ) + try: + file_id = upload_url_response.get("file_id") + upload_url = upload_url_response.get("upload_url") + except (AttributeError, TypeError) as ex: + raise SlackChannelResponseError( + "Slack did not return valid upload metadata" + ) from ex + if ( + not isinstance(file_id, str) + or not file_id + or not isinstance(upload_url, str) + or not upload_url + ): + raise SlackChannelResponseError("Slack did not return a file ID and upload URL") + def upload_file() -> None: + timeout = get_slack_request_timeout(client.timeout, retry_deadline) + status, response_body = _upload_file_data( + url=upload_url, + data=data, + timeout=timeout, + proxy=client.proxy, + ssl=client.ssl, + ) + if status != 200: + raise HTTPError( + upload_url, + status, + f"Slack external upload failed: {response_body}", + Message(), + None, + ) -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) + call_slack_api( + upload_file, + retry_deadline=retry_deadline, + retry_transport_errors=True, + ) + call_slack_api_with_timeout( + client, + client.files_completeUploadExternal, + retry_deadline=retry_deadline, + retry_transient_errors=False, + files=[{"id": file_id, "title": title}], + channel_id=channel, + initial_comment=initial_comment, + ) class SlackV2Notification(SlackMixin, BaseNotification): # pylint: disable=too-few-public-methods @@ -105,19 +171,9 @@ class SlackV2Notification(SlackMixin, BaseNotification): # pylint: disable=too- type = ReportRecipientType.SLACKV2 - def _get_channels(self) -> List[str]: - """ - Get the recipient's channel(s). - :returns: A list of channel ids: "EID676L" - :raises NotificationParamException or SlackApiError: If the recipient is not found - """ # noqa: E501 - recipient_str = json.loads(self._recipient.recipient_config_json)["target"] - - return recipients_string_to_list(recipient_str) - def _get_inline_files( self, - ) -> tuple[Union[str, None], Sequence[Union[str, IOBase, bytes]]]: + ) -> tuple[str | None, list[bytes]]: if self._content.csv: return ("csv", [self._content.csv]) if self._content.xlsx: @@ -132,24 +188,28 @@ class SlackV2Notification(SlackMixin, BaseNotification): # pylint: disable=too- def send(self) -> None: global_logs_context = getattr(g, "logs_context", {}) or {} try: - client = get_slack_client() + client = get_slack_client(for_delivery=True) title = self._content.name body = self._get_body(content=self._content) channels = self._get_channels() if not channels: - raise NotificationParamException("No recipients saved in the report") + raise NotificationParamException(NO_SLACK_RECIPIENTS_MESSAGE) file_type, files = self._get_inline_files() - file_name = f"{title}.{file_type}" - # files_upload returns SlackResponse as we run it in sync mode. - for channel in channels: + def send_to_channel(channel: str, retry_deadline: float) -> None: if len(files) > 0: + if file_type is None: + raise SlackChannelResponseError( + "Slack upload file type was not provided" + ) + file_name = f"{title}.{file_type}" for file in files: - _call_slack_api( - client.files_upload_v2, + _upload_file_to_slack( + client, + retry_deadline=retry_deadline, channel=channel, file=file, initial_comment=body, @@ -157,7 +217,18 @@ class SlackV2Notification(SlackMixin, BaseNotification): # pylint: disable=too- filename=file_name, ) else: - _call_slack_api(client.chat_postMessage, channel=channel, text=body) + send_slack_text( + client, + channel, + body, + retry_deadline=retry_deadline, + ) + + send_to_slack_channels( + channels, + send_to_channel, + retry_deadline=self._content.slack_retry_deadline, + ) logger.info( "Report sent to slack", @@ -175,9 +246,7 @@ class SlackV2Notification(SlackMixin, BaseNotification): # pylint: disable=too- raise NotificationMalformedException(str(ex)) from ex except SlackTokenRotationError as ex: raise NotificationAuthorizationException(str(ex)) from ex - except (SlackClientNotConnectedError, SlackApiError) as ex: - raise NotificationUnprocessableException(str(ex)) from ex except SlackClientError as ex: - # this is the base class for all slack client errors - # keep it last so that it doesn't interfere with @backoff + # SlackClientError is the base class; keep it last so subclasses + # retain their more specific notification classification. raise NotificationUnprocessableException(str(ex)) from ex diff --git a/superset/tasks/slack.py b/superset/tasks/slack.py index db9047171bc..ec89f3a2101 100644 --- a/superset/tasks/slack.py +++ b/superset/tasks/slack.py @@ -18,17 +18,27 @@ import logging from flask import current_app +from superset.constants import CACHE_DISABLED_TIMEOUT from superset.extensions import celery_app +from superset.utils.decorators import transaction from superset.utils.slack import get_channels logger = logging.getLogger(__name__) @celery_app.task(name="slack.cache_channels") +@transaction() def cache_channels() -> None: cache_timeout = current_app.config["SLACK_CACHE_TIMEOUT"] retry_count = current_app.config.get("SLACK_API_RATE_LIMIT_RETRY_COUNT", 2) + if cache_timeout == CACHE_DISABLED_TIMEOUT: + logger.warning( + "Skipping Slack channels cache warm-up because " + "SLACK_CACHE_TIMEOUT disables caching" + ) + return + logger.info( "Starting Slack channels cache warm-up task " "(cache_timeout=%ds, retry_count=%d)", @@ -37,7 +47,11 @@ def cache_channels() -> None: ) try: - get_channels(force=True, cache_timeout=cache_timeout) + get_channels( + force=True, + cache_timeout=cache_timeout, + raise_on_cache_write_error=True, + ) except Exception as ex: logger.exception( "Failed to cache Slack channels: %s. " diff --git a/superset/utils/decorators.py b/superset/utils/decorators.py index cb5711452ba..8b23531886f 100644 --- a/superset/utils/decorators.py +++ b/superset/utils/decorators.py @@ -36,7 +36,21 @@ if TYPE_CHECKING: from superset.stats_logger import BaseStatsLogger -def statsd_gauge(metric_prefix: str | None = None) -> Callable[..., Any]: +def record_statsd_gauge_failure(metric_prefix: str, ex: Exception) -> None: + """Record a warning or error gauge using the shared exception contract.""" + try: + status = getattr(ex, "status", None) + except Exception: # pylint: disable=broad-exception-caught + status = None + suffix = "warning" if isinstance(status, int) and status < 500 else "error" + app.config["STATS_LOGGER"].gauge(f"{metric_prefix}.{suffix}", 1) + + +def statsd_gauge( + metric_prefix: str | None = None, + *, + ignored_exceptions: tuple[type[Exception], ...] = (), +) -> Callable[..., Any]: def decorate(f: Callable[..., Any]) -> Callable[..., Any]: """ Handle sending statsd gauge metric from any method or function @@ -48,13 +62,10 @@ def statsd_gauge(metric_prefix: str | None = None) -> Callable[..., Any]: result = f(*args, **kwargs) app.config["STATS_LOGGER"].gauge(f"{metric_prefix_}.ok", 1) return result + except ignored_exceptions: + raise except Exception as ex: - if ( - hasattr(ex, "status") and ex.status < 500 # pylint: disable=no-member - ): - app.config["STATS_LOGGER"].gauge(f"{metric_prefix_}.warning", 1) - else: - app.config["STATS_LOGGER"].gauge(f"{metric_prefix_}.error", 1) + record_statsd_gauge_failure(metric_prefix_, ex) raise return wrapped diff --git a/superset/utils/slack.py b/superset/utils/slack.py index 0e0e422d525..aa4b5c12db3 100644 --- a/superset/utils/slack.py +++ b/superset/utils/slack.py @@ -19,18 +19,31 @@ import functools import logging import warnings -from typing import Any, Callable, Optional +from http.client import RemoteDisconnected +from typing import ( + Any, + Callable, + NotRequired, + Optional, + TypedDict, +) +from urllib.error import HTTPError, URLError from flask import current_app as app from slack_sdk import WebClient -from slack_sdk.errors import SlackApiError, SlackClientError as SlackSDKClientError +from slack_sdk.errors import ( + SlackApiError, + SlackClientError as SlackSDKClientError, + SlackClientNotConnectedError, +) from slack_sdk.http_retry.builtin_handlers import RateLimitErrorRetryHandler from superset import feature_flag_manager +from superset.constants import CACHE_DISABLED_TIMEOUT from superset.exceptions import SupersetException from superset.extensions import cache_manager +from superset.extensions.metastore_cache import SupersetMetastoreCache from superset.reports.schemas import SlackChannelSchema -from superset.utils import cache as cache_util from superset.utils.backports import StrEnum from superset.utils.core import recipients_string_to_list @@ -47,10 +60,9 @@ _SLACK_V1_DEPRECATION_MESSAGE = ( ) -# 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 suppresses repeated calls after the first one completes. +# Concurrent first calls may both emit, which is acceptable for a deprecation +# warning and avoids managing additional process-local synchronization. @functools.cache def _emit_v1_flag_off_deprecation() -> None: warnings.warn(_SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning, stacklevel=3) @@ -72,11 +84,124 @@ class SlackChannelTypes(StrEnum): _SLACK_CONVERSATION_TYPES = ",".join(SlackChannelTypes) +class SlackChannel(TypedDict): + """Normalized Slack channel fields used for report-recipient resolution.""" + + id: str + name: str + is_private: bool + is_member: NotRequired[bool] + + class SlackClientError(Exception): pass -def get_slack_client() -> WebClient: +class SlackV2ProbeError(SupersetException): + """Transient probe failure classified by ``_send`` as a system error. + + ``BaseReportState._send`` catches this through ``SupersetException`` and + uses the inherited status code to select ERROR severity. + """ + + +class SlackV2ProbeClientError(SlackV2ProbeError): + """Permanent probe failure classified by ``_send`` as a client warning. + + ``BaseReportState._send`` catches this through ``SupersetException`` and + uses this status code to select WARNING severity. + """ + + status = 422 + + +class SlackChannelListingError(SupersetException): + """Slack channel listing failed due to a transient service condition.""" + + +class SlackChannelListingClientError(SlackChannelListingError): + """Slack channel listing failed due to permanent token or client setup.""" + + status = 422 + + +class SlackChannelCacheWriteError(SupersetException): + """The dedicated Slack channel cache warm-up could not store its result.""" + + +_TRANSIENT_SLACK_API_ERROR_CODES = frozenset( + { + "fatal_error", + "internal_error", + "ratelimited", + "request_timeout", + "rollup_error", + "service_unavailable", + "timeout", + } +) + +SLACK_TRANSIENT_TRANSPORT_ERRORS: tuple[type[Exception], ...] = ( + SlackClientNotConnectedError, + URLError, + ConnectionResetError, + RemoteDisconnected, + TimeoutError, +) + +NO_SLACK_RECIPIENTS_MESSAGE = "No recipients saved in the report" + + +def parse_slack_recipient_targets(target: str) -> list[str]: + """Parse Slack targets, removing duplicates while preserving their order.""" + return list(dict.fromkeys(recipients_string_to_list(target))) + + +def get_slack_api_error_data(ex: SlackApiError) -> dict[str, Any]: + """Return a Slack API error payload across SDK response representations.""" + response = getattr(ex, "response", None) + data = getattr(response, "data", None) + if not isinstance(data, dict): + data = response if isinstance(response, dict) else {} + return data + + +def get_slack_api_error_code(ex: SlackApiError) -> str: + """Return Slack's application-level error code, if present.""" + return str(get_slack_api_error_data(ex).get("error") or "") + + +def get_slack_api_status_code(ex: SlackApiError) -> int | None: + """Return the HTTP status carried by a Slack API error, if present.""" + return getattr(getattr(ex, "response", None), "status_code", None) + + +def is_transient_slack_api_error(ex: SlackApiError, error_code: str) -> bool: + """Return whether Slack reported a retryable API or HTTP condition.""" + status_code = get_slack_api_status_code(ex) + return bool( + status_code in {408, 429} + or (status_code is not None and 500 <= status_code < 600) + or error_code in _TRANSIENT_SLACK_API_ERROR_CODES + ) + + +def is_transient_slack_transport_error(ex: Exception) -> bool: + """Classify raw Slack WebClient transport and external-upload failures.""" + if isinstance(ex, HTTPError): + return ex.code in {408, 429} or 500 <= ex.code < 600 + return isinstance(ex, SLACK_TRANSIENT_TRANSPORT_ERRORS) + + +def is_retryable_slack_transport_error(ex: Exception) -> bool: + """Return whether an application retry cannot duplicate an accepted write.""" + if isinstance(ex, HTTPError): + return is_transient_slack_transport_error(ex) + return isinstance(ex, SlackClientNotConnectedError) + + +def get_slack_client(*, for_delivery: bool = False) -> WebClient: + """Build a Slack client without nested SDK retries for delivery writes.""" token: str = app.config["SLACK_API_TOKEN"] if callable(token): token = token() @@ -84,13 +209,20 @@ def get_slack_client() -> WebClient: token=token, proxy=app.config["SLACK_PROXY"], timeout=app.config["SLACK_API_TIMEOUT"], + retry_handlers=[] if for_delivery else None, ) max_retry_count = app.config.get("SLACK_API_RATE_LIMIT_RETRY_COUNT", 2) - rate_limit_handler = RateLimitErrorRetryHandler(max_retry_count=max_retry_count) - client.retry_handlers.append(rate_limit_handler) - - logger.debug("Slack client configured with %d rate limit retries", max_retry_count) + if not for_delivery: + rate_limit_handler = RateLimitErrorRetryHandler( + max_retry_count=max_retry_count, + ) + client.retry_handlers.append(rate_limit_handler) + logger.debug( + "Slack client configured with %d rate limit retries", max_retry_count + ) + else: + logger.debug("Slack delivery client configured with SDK retries disabled") return client @@ -112,9 +244,24 @@ def get_team_id() -> Optional[str]: return team_id or None +def _get_slack_channels_cache_key(team_id: Optional[str]) -> str: + cache_key = "slack_conversations_list" + return f"{cache_key}_{team_id}" if team_id else cache_key + + +def _slack_channel_cache_uses_report_session() -> bool: + """Return whether cache writes commit or roll back the report DB session.""" + return isinstance(cache_manager.cache.cache, SupersetMetastoreCache) + + def get_channels( - team_id: Optional[str] = None, **kwargs: Any -) -> list[SlackChannelSchema]: + team_id: Optional[str] = None, + *, + force: bool = False, + cache_timeout: int | None = None, + cache: bool = True, + raise_on_cache_write_error: bool = False, +) -> list[SlackChannel]: """ Retrieves a list of all conversations accessible by the bot from the Slack API, and caches results (to avoid rate limits). @@ -128,27 +275,87 @@ def get_channels( distinct workspaces never share cached channel lists; when unset, the legacy cache key is used so that upgrading does not invalidate existing caches. - :param kwargs: forwarded to the memoized fetch (``force``, ``cache_timeout``, - ``cache``). + Cache reads and writes are best-effort; a backend failure does not replace + successfully fetched Slack data. """ if team_id is None: team_id = get_team_id() - cache_key = "slack_conversations_list" - if team_id: - cache_key = f"{cache_key}_{team_id}" - return _get_channels(cache_key, team_id=team_id, **kwargs) + channels, _ = _get_channels_safely( + team_id=team_id, + force=force, + cache=cache, + cache_timeout=cache_timeout, + write_cache=True, + raise_on_cache_write_error=raise_on_cache_write_error, + ) + return channels -@cache_util.memoized_func( - key="{cache_key}", - cache=cache_manager.cache, -) -def _get_channels( - cache_key: str, team_id: Optional[str] = None -) -> list[SlackChannelSchema]: +def _get_channels_safely( + *, + team_id: str | None, + force: bool, + cache: bool, + write_cache: bool, + cache_timeout: int | None = None, + raise_on_cache_write_error: bool = False, +) -> tuple[list[SlackChannel], bool]: + """Fetch channels with best-effort cache access and hit provenance.""" + cache_key = _get_slack_channels_cache_key(team_id) + effective_timeout = ( + app.config["SLACK_CACHE_TIMEOUT"] if cache_timeout is None else cache_timeout + ) + cache_enabled = cache and effective_timeout != CACHE_DISABLED_TIMEOUT + + if cache_enabled and not force: + try: + cached_channels = cache_manager.cache.get(cache_key) + except Exception: # pylint: disable=broad-exception-caught + cached_channels = None + logger.warning( + "Could not read cached Slack channels; fetching from Slack", + exc_info=True, + ) + if cached_channels is not None: + return cached_channels, True + + channels = _get_channels(team_id=team_id) + if cache_enabled and write_cache: + cache_write_result: bool | None = None + try: + cache_write_result = cache_manager.cache.set( + cache_key, + channels, + timeout=effective_timeout, + ) + except Exception: # pylint: disable=broad-exception-caught + logger.warning( + "Could not cache Slack channels", + exc_info=True, + ) + if raise_on_cache_write_error: + raise + if raise_on_cache_write_error and cache_write_result is False: + raise SlackChannelCacheWriteError( + "Slack channels were fetched but the cache rejected the write" + ) + return channels, False + + +def _get_channels_with_cache_status() -> tuple[list[SlackChannel], bool]: + """Fetch channels and cache-hit provenance using one cache read.""" + return _get_channels_safely( + team_id=get_team_id(), + force=False, + cache=True, + write_cache=not _slack_channel_cache_uses_report_session(), + ) + + +def _get_channels(team_id: Optional[str] = None) -> list[SlackChannel]: client = get_slack_client() channel_schema = SlackChannelSchema() - channels: list[SlackChannelSchema] = [] + channels: list[SlackChannel] = [] extra_params = {"types": _SLACK_CONVERSATION_TYPES} if team_id: extra_params["team_id"] = team_id @@ -194,37 +401,16 @@ def _get_channels( raise -def get_channels_with_search( - search_string: str = "", - types: Optional[list[SlackChannelTypes]] = None, - exact_match: bool = False, - force: bool = False, -) -> list[SlackChannelSchema]: - """ - The slack api is paginated but does not include search, so we need to fetch - all channels and filter them ourselves - This will search by slack name or id - """ - try: - channels = get_channels( - force=force, - cache_timeout=app.config["SLACK_CACHE_TIMEOUT"], - ) - except SlackApiError as ex: - # Check if it's a rate limit error - status_code = getattr(ex.response, "status_code", None) - if status_code == 429: - raise SupersetException( - f"Slack API rate limit exceeded: {ex}. " - "For large workspaces, consider increasing " - "SLACK_API_RATE_LIMIT_RETRY_COUNT" - ) from ex - raise SupersetException(f"Failed to list channels: {ex}") from ex - except SlackClientError as ex: - raise SupersetException(f"Failed to list channels: {ex}") from ex - - if types and not len(types) == len(SlackChannelTypes): - conditions: list[Callable[[SlackChannelSchema], bool]] = [] +def _filter_slack_channels( + channels: list[SlackChannel], + *, + search_string: str, + types: Optional[list[SlackChannelTypes]], + exact_match: bool, +) -> list[SlackChannel]: + """Filter a complete Slack channel listing by type and target.""" + if types and len(types) != len(SlackChannelTypes): + conditions: list[Callable[[SlackChannel], bool]] = [] if SlackChannelTypes.PUBLIC in types: conditions.append(lambda channel: not channel["is_private"]) if SlackChannelTypes.PRIVATE in types: @@ -234,34 +420,235 @@ def get_channels_with_search( channel for channel in channels if any(cond(channel) for cond in conditions) ] - # The search string can be multiple channels separated by commas - if search_string: - search_array = recipients_string_to_list(search_string) - channels = [ - channel - for channel in channels - if any( - ( - search.lower() == channel["name"].lower() - or search.lower() == channel["id"].lower() - if exact_match - else ( - search.lower() in channel["name"].lower() - or search.lower() in channel["id"].lower() - ) + if not search_string: + return channels + + search_array = recipients_string_to_list(search_string) + return [ + channel + for channel in channels + if any( + ( + search.casefold() == channel["name"].casefold() + or search.casefold() == channel["id"].casefold() + if exact_match + else ( + search.casefold() in channel["name"].casefold() + or search.casefold() in channel["id"].casefold() ) - for search in search_array ) - ] + for search in search_array + ) + ] + + +def _get_channels_with_search( + search_string: str = "", + types: Optional[list[SlackChannelTypes]] = None, + exact_match: bool = False, + force: bool = False, + cache: bool = True, + *, + return_cache_status: bool = False, +) -> tuple[list[SlackChannel], bool]: + """ + The slack api is paginated but does not include search, so we need to fetch + all channels and filter them ourselves + This will search by slack name or id + """ + used_cache = False + cache_timeout = app.config["SLACK_CACHE_TIMEOUT"] + cache_enabled = cache and cache_timeout != CACHE_DISABLED_TIMEOUT + try: + if return_cache_status and cache_enabled and not force: + channels, used_cache = _get_channels_with_cache_status() + else: + channels = get_channels( + force=force, + cache=cache_enabled, + cache_timeout=cache_timeout, + ) + except SlackApiError as ex: + error_code = get_slack_api_error_code(ex) + error_class = ( + SlackChannelListingError + if is_transient_slack_api_error(ex, error_code) + else SlackChannelListingClientError + ) + message = f"Failed to list channels: {ex}" + if get_slack_api_status_code(ex) == 429: + message = ( + f"Slack API rate limit exceeded: {ex}. For large workspaces, " + "consider increasing SLACK_API_RATE_LIMIT_RETRY_COUNT" + ) + raise error_class(message) from ex + except SLACK_TRANSIENT_TRANSPORT_ERRORS as ex: + error_class = ( + SlackChannelListingError + if is_transient_slack_transport_error(ex) + else SlackChannelListingClientError + ) + raise error_class(f"Failed to list channels: {ex}") from ex + except (SlackSDKClientError, SlackClientError) as ex: + raise SlackChannelListingClientError(f"Failed to list channels: {ex}") from ex + + channels = _filter_slack_channels( + channels, + search_string=search_string, + types=types, + exact_match=exact_match, + ) + return channels, used_cache + + +def get_channels_with_search( + search_string: str = "", + types: Optional[list[SlackChannelTypes]] = None, + exact_match: bool = False, + force: bool = False, + cache: bool = True, +) -> list[SlackChannel]: + """Fetch and filter Slack channels without exposing cache provenance.""" + channels, _ = _get_channels_with_search( + search_string=search_string, + types=types, + exact_match=exact_match, + force=force, + cache=cache, + ) return channels +def get_channels_with_search_and_cache_status( + search_string: str = "", + types: Optional[list[SlackChannelTypes]] = None, + exact_match: bool = False, +) -> tuple[list[SlackChannel], bool]: + """Fetch filtered Slack channels and report whether the listing was cached.""" + return _get_channels_with_search( + search_string=search_string, + types=types, + exact_match=exact_match, + return_cache_status=True, + ) + + +def refresh_cached_slack_channels_with_search( + search_string: str = "", + types: Optional[list[SlackChannelTypes]] = None, + exact_match: bool = False, +) -> list[SlackChannel]: + """Refresh stale channels with a best-effort atomic cache claim. + + External cache backends atomically claim the refresh cooldown before listing + channels. A failed listing or cache write releases that claim. Disabled and + metastore-backed caches use an uncached request without a claim because + metastore writes commit the report transaction. + """ + team_id = get_team_id() + cache_key = _get_slack_channels_cache_key(team_id) + cooldown_key = f"{cache_key}_refresh_cooldown" + cache_timeout = app.config["SLACK_CACHE_TIMEOUT"] + + if ( + _slack_channel_cache_uses_report_session() + or cache_timeout == CACHE_DISABLED_TIMEOUT + ): + return get_channels_with_search( + search_string=search_string, + types=types, + exact_match=exact_match, + force=True, + cache=False, + ) + + cooldown_timeout = app.config.get( + "SLACK_CHANNEL_REFRESH_COOLDOWN_SECONDS", + 300, + ) + claim_recorded = False + should_refresh = True + try: + claim_recorded = ( + cache_manager.cache.add( + cooldown_key, + True, + timeout=cooldown_timeout, + ) + is not False + ) + should_refresh = claim_recorded + except Exception: # pylint: disable=broad-exception-caught + logger.warning( + "Could not claim Slack channel refresh cooldown; refreshing from Slack", + exc_info=True, + ) + + if not should_refresh: + try: + cached_channels = cache_manager.cache.get(cache_key) + except Exception: # pylint: disable=broad-exception-caught + cached_channels = None + logger.warning( + "Could not read Slack channels after another worker claimed refresh", + exc_info=True, + ) + channels: list[SlackChannel] = ( + cached_channels if isinstance(cached_channels, list) else [] + ) + return _filter_slack_channels( + channels, + search_string=search_string, + types=types, + exact_match=exact_match, + ) + + keep_claim = False + try: + refreshed_channels = get_channels_with_search( + force=True, + cache=False, + ) + try: + cache_updated = ( + cache_manager.cache.set( + cache_key, + refreshed_channels, + timeout=cache_timeout, + ) + is not False + ) + except Exception: # pylint: disable=broad-exception-caught + cache_updated = False + logger.warning( + "Could not cache refreshed Slack channels", + exc_info=True, + ) + + keep_claim = cache_updated + return _filter_slack_channels( + refreshed_channels, + search_string=search_string, + types=types, + exact_match=exact_match, + ) + finally: + if not keep_claim: + try: + cache_manager.cache.delete(cooldown_key) + except Exception: # pylint: disable=broad-exception-caught + logger.warning( + "Could not release Slack channel refresh cooldown", + exc_info=True, + ) + + _SCOPE_MISSING_ERROR_CODES = frozenset( {"missing_scope", "not_allowed_token_type", "no_permission"} ) -def should_use_v2_api() -> bool: +def should_use_v2_api(*, raise_on_error: bool = False) -> bool: if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_SLACK_V2"): _emit_v1_flag_off_deprecation() return False @@ -282,17 +669,11 @@ def should_use_v2_api() -> bool: # 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", "") + # scope. Scope errors continue through v1 for compatibility. Other + # failures also fall back for text-only reports, while file-bearing + # reports request an exception so monitoring retains the system/client + # classification. + error_code = get_slack_api_error_code(ex) 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. @@ -305,27 +686,49 @@ def should_use_v2_api() -> bool: ) 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.", + "Slack v2 probe failed with error %r. Investigate the underlying " + "Slack API error; this is not a missing-scope problem.", error_code or str(ex), ) + if raise_on_error: + error_class = ( + SlackV2ProbeError + if is_transient_slack_api_error(ex, error_code) + else SlackV2ProbeClientError + ) + raise error_class( + f"Slack v2 availability probe failed: {error_code or str(ex)}" + ) from 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. + except SLACK_TRANSIENT_TRANSPORT_ERRORS as ex: logger.warning( - "Slack v2 probe failed to connect (%s: %s); falling back to the " - "deprecated v1 API for this send.", + "Slack v2 probe failed (%s: %s).", type(ex).__name__, ex, ) + if raise_on_error: + error_class = ( + SlackV2ProbeError + if is_transient_slack_transport_error(ex) + else SlackV2ProbeClientError + ) + raise error_class( + f"Slack v2 availability probe failed: {type(ex).__name__}: {ex}" + ) from ex + return False + except SlackSDKClientError as ex: + # Permanent SDK request/configuration failures are operator-fixable. + # Text reports retain v1 compatibility, while file-bearing reports ask + # the command to preserve the client-error classification. + logger.warning( + "Slack v2 probe failed (%s: %s).", + type(ex).__name__, + ex, + ) + if raise_on_error: + raise SlackV2ProbeClientError( + f"Slack v2 availability probe failed: {type(ex).__name__}: {ex}" + ) from ex return False diff --git a/tests/integration_tests/reports/commands_tests.py b/tests/integration_tests/reports/commands_tests.py index 533f8eacb6c..92ef47cc633 100644 --- a/tests/integration_tests/reports/commands_tests.py +++ b/tests/integration_tests/reports/commands_tests.py @@ -22,6 +22,7 @@ from typing import Optional from unittest.mock import ANY, call, Mock, patch from uuid import UUID, uuid4 +import pandas as pd import pytest from flask.ctx import AppContext from flask_appbuilder.security.sqla.models import User @@ -67,6 +68,7 @@ from superset.commands.report.execute import ( BaseReportState, ) from superset.commands.report.log_prune import AsyncPruneReportScheduleLogCommand +from superset.daos.report import ReportScheduleDAO from superset.exceptions import SupersetException from superset.key_value.models import KeyValueEntry from superset.models.core import Database @@ -75,6 +77,7 @@ from superset.models.slice import Slice from superset.reports.models import ( ReportDataFormat, ReportExecutionLog, + ReportRecipients, ReportRecipientType, ReportSchedule, ReportScheduleType, @@ -90,6 +93,7 @@ from superset.utils import json from superset.utils.database import get_example_database from superset.utils.report_execution import ReportExecutionContext from superset.utils.webdriver import PlaywrightTimeout +from tests.integration_tests.conftest import with_feature_flags from tests.integration_tests.fixtures.birth_names_dashboard import ( load_birth_names_dashboard_with_slices, # noqa: F401 load_birth_names_data, # noqa: F401 @@ -119,6 +123,28 @@ pytestmark = pytest.mark.usefixtures( ) +def _configure_v2_upload_client(client: Mock) -> Mock: + """Configure a Slack SDK-shaped three-phase file upload mock.""" + client.timeout = 30 + client.proxy = None + client.ssl = None + client.files_getUploadURLExternal.return_value = { + "file_id": "F1", + "upload_url": "https://files.slack.com/upload/F1", + } + client.files_completeUploadExternal.return_value = {"files": [{"id": "F1"}]} + return client + + +@pytest.fixture(autouse=True) +def slack_raw_upload_mock(mocker): + """Keep report integration tests off Slack's issued upload URL.""" + return mocker.patch( + "superset.reports.notifications.slackv2._upload_file_data", + return_value=(200, "ok"), + ) + + def get_target_from_report_schedule(report_schedule: ReportSchedule) -> list[str]: return [ json.loads(recipient.recipient_config_json)["target"] @@ -1496,7 +1522,9 @@ def test_email_dashboard_report_schedule_force_screenshot( @pytest.mark.usefixtures("create_report_slack_chart") -@patch("superset.commands.report.execute.get_channels_with_search") +@patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status" +) @patch("superset.reports.notifications.slack.should_use_v2_api", return_value=True) @patch("superset.reports.notifications.slackv2.get_slack_client") @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") @@ -1506,6 +1534,7 @@ def test_slack_chart_report_schedule_converts_to_v2( slack_should_use_v2_api_mock, get_channels_with_search_mock, create_report_slack_chart, + slack_raw_upload_mock, ): """ ExecuteReport Command: Test chart slack report schedule @@ -1513,15 +1542,19 @@ def test_slack_chart_report_schedule_converts_to_v2( """ # setup screenshot mock screenshot_mock.return_value = SCREENSHOT_FILE + slack_client = _configure_v2_upload_client(slack_client_mock.return_value) channel_id = "slack_channel_id" - get_channels_with_search_mock.return_value = [ - { - "id": channel_id, - "name": "slack_channel", - "is_member": True, - "is_private": False, - }, - ] + get_channels_with_search_mock.return_value = ( + [ + { + "id": channel_id, + "name": "slack_channel", + "is_member": True, + "is_private": False, + }, + ], + False, + ) with freeze_time("2020-01-01T00:00:00Z"): with patch( @@ -1532,13 +1565,10 @@ def test_slack_chart_report_schedule_converts_to_v2( ).run() assert ( - slack_client_mock.return_value.files_upload_v2.call_args[1]["channel"] + slack_client.files_completeUploadExternal.call_args[1]["channel_id"] == channel_id ) - assert ( - slack_client_mock.return_value.files_upload_v2.call_args[1]["file"] - == SCREENSHOT_FILE - ) + assert slack_raw_upload_mock.call_args.kwargs["data"] == SCREENSHOT_FILE # Assert that the report recipients were updated assert create_report_slack_chart.recipients[ @@ -1551,14 +1581,12 @@ def test_slack_chart_report_schedule_converts_to_v2( # Assert logs are correct assert_log(ReportState.SUCCESS) - # this will send a warning - assert statsd_mock.call_args_list[0] == call( - "reports.slack.send.warning", 1 - ) - assert statsd_mock.call_args_list[1] == call("reports.slack.send.ok", 1) + statsd_mock.assert_called_once_with("reports.slack.send.ok", 1) -@patch("superset.commands.report.execute.get_channels_with_search") +@patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status" +) @patch("superset.reports.notifications.slack.should_use_v2_api", return_value=True) @patch("superset.reports.notifications.slackv2.get_slack_client") @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") @@ -1567,6 +1595,7 @@ def test_slack_chart_report_schedule_converts_to_v2_channel_with_hash( slack_client_mock, slack_should_use_v2_api_mock, get_channels_with_search_mock, + slack_raw_upload_mock, ): """ ExecuteReport Command: Test converting a Slack report to v2 when @@ -1574,19 +1603,23 @@ def test_slack_chart_report_schedule_converts_to_v2_channel_with_hash( """ # setup screenshot mock screenshot_mock.return_value = SCREENSHOT_FILE + slack_client = _configure_v2_upload_client(slack_client_mock.return_value) channel_id = "slack_channel_id" chart = db.session.query(Slice).first() report_schedule = create_report_notification( slack_channel="#slack_channel", chart=chart ) - get_channels_with_search_mock.return_value = [ - { - "id": channel_id, - "name": "slack_channel", - "is_member": True, - "is_private": False, - }, - ] + get_channels_with_search_mock.return_value = ( + [ + { + "id": channel_id, + "name": "slack_channel", + "is_member": True, + "is_private": False, + }, + ], + False, + ) with freeze_time("2020-01-01T00:00:00Z"): with patch( @@ -1597,13 +1630,10 @@ def test_slack_chart_report_schedule_converts_to_v2_channel_with_hash( ).run() assert ( - slack_client_mock.return_value.files_upload_v2.call_args[1]["channel"] + slack_client.files_completeUploadExternal.call_args[1]["channel_id"] == channel_id ) - assert ( - slack_client_mock.return_value.files_upload_v2.call_args[1]["file"] - == SCREENSHOT_FILE - ) + assert slack_raw_upload_mock.call_args.kwargs["data"] == SCREENSHOT_FILE # Assert that the report recipients were updated assert report_schedule.recipients[0].recipient_config_json == json.dumps( @@ -1613,28 +1643,24 @@ def test_slack_chart_report_schedule_converts_to_v2_channel_with_hash( # Assert logs are correct assert_log(ReportState.SUCCESS) - # this will send a warning - assert statsd_mock.call_args_list[0] == call( - "reports.slack.send.warning", 1 - ) - assert statsd_mock.call_args_list[1] == call("reports.slack.send.ok", 1) + statsd_mock.assert_called_once_with("reports.slack.send.ok", 1) cleanup_report_schedule(report_schedule) -@patch("superset.commands.report.execute.get_channels_with_search") +@patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status" +) @patch("superset.reports.notifications.slack.should_use_v2_api", return_value=True) @patch("superset.reports.notifications.slackv2.get_slack_client") @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") -def test_slack_chart_report_schedule_fails_to_converts_to_v2( +def test_slack_chart_report_schedule_failed_v2_conversion_rejects_v1_file_upload( screenshot_mock, slack_client_mock, slack_should_use_v2_api_mock, get_channels_with_search_mock, ): - """ - ExecuteReport Command: Test converting a Slack report to v2 fails. - """ + """A failed Slack v2 conversion rejects unsupported Slack v1 file uploads.""" # setup screenshot mock screenshot_mock.return_value = SCREENSHOT_FILE channel_id = "slack_channel_id" @@ -1642,34 +1668,50 @@ def test_slack_chart_report_schedule_fails_to_converts_to_v2( report_schedule = create_report_notification( slack_channel="#slack_channel,my_member_ID", chart=chart ) - get_channels_with_search_mock.return_value = [ - { - "id": channel_id, - "name": "slack_channel", - "is_member": True, - "is_private": False, - }, - ] - - with pytest.raises(ReportScheduleSystemErrorsException): - AsyncExecuteReportScheduleCommand( - TEST_ID, report_schedule.id, datetime.utcnow() - ).run() - - # Assert failuer with proper log - expected_message = ( - "Failed to update slack recipients to v2: " - "Could not find the following channels: my_member_ID" + get_channels_with_search_mock.return_value = ( + [ + { + "id": channel_id, + "name": "slack_channel", + "is_member": True, + "is_private": False, + }, + ], + False, ) - assert_log(ReportState.ERROR, error_message=expected_message) - # Assert that previous configuration was kept for manual correction - assert report_schedule.recipients[0].recipient_config_json == json.dumps( - {"target": "#slack_channel,my_member_ID"} - ) - assert report_schedule.recipients[0].type == ReportRecipientType.SLACK + try: + with ( + patch( + "superset.extensions.stats_logger_manager.instance.gauge" + ) as statsd_mock, + pytest.raises(ReportScheduleClientErrorsException), + ): + AsyncExecuteReportScheduleCommand( + TEST_ID, report_schedule.id, datetime.utcnow() + ).run() - cleanup_report_schedule(report_schedule) + expected_message = ( + "Slack v1 file uploads are no longer supported because Slack retired " + "`files.upload`. Enable `ALERT_REPORT_SLACK_V2` and grant the Slack bot " + "both the `channels:read` and `groups:read` scopes so the recipient can " + "be upgraded to Slack v2. Slack v2 upgrade failed: Failed to update " + "slack recipients to v2: Could not find the following channels: " + "my_member_ID" + ) + assert_log(ReportState.ERROR, error_message=expected_message) + + # Keep the previous configuration for manual correction. + assert report_schedule.recipients[0].recipient_config_json == json.dumps( + {"target": "#slack_channel,my_member_ID"} + ) + assert report_schedule.recipients[0].type == ReportRecipientType.SLACK + slack_client_mock.assert_not_called() + assert ( + statsd_mock.call_args_list.count(call("reports.slack.send.warning", 1)) == 1 + ) + finally: + cleanup_report_schedule(report_schedule) @pytest.mark.usefixtures("create_report_slack_chartv2") @@ -1681,12 +1723,14 @@ def test_slack_chart_report_schedule_v2( slack_client_mock, slack_should_use_v2_api_mock, create_report_slack_chartv2, + slack_raw_upload_mock, ): """ ExecuteReport Command: Test chart slack report schedule using Slack v2. """ # setup screenshot mock screenshot_mock.return_value = SCREENSHOT_FILE + slack_client = _configure_v2_upload_client(slack_client_mock.return_value) with freeze_time("2020-01-01T00:00:00Z"): with patch( @@ -1697,13 +1741,10 @@ def test_slack_chart_report_schedule_v2( ).run() assert ( - slack_client_mock.return_value.files_upload_v2.call_args[1]["channel"] + slack_client.files_completeUploadExternal.call_args[1]["channel_id"] == "slack_channel_id" ) - assert ( - slack_client_mock.return_value.files_upload_v2.call_args[1]["file"] - == SCREENSHOT_FILE - ) + assert slack_raw_upload_mock.call_args.kwargs["data"] == SCREENSHOT_FILE # Assert logs are correct assert_log(ReportState.SUCCESS) @@ -1767,6 +1808,7 @@ def test_slack_chart_report_schedule_with_errors( @pytest.mark.usefixtures( "load_birth_names_dashboard_with_slices", "create_report_slack_chart_with_csv" ) +@with_feature_flags(ALERT_REPORT_SLACK_V2=False) @patch("superset.reports.notifications.slack.should_use_v2_api", return_value=False) @patch("superset.reports.notifications.slack.get_slack_client") @patch("superset.utils.csv.urllib.request.urlopen") @@ -1780,9 +1822,7 @@ def test_slack_chart_report_schedule_with_csv( slack_should_use_v2_api_mock, create_report_slack_chart_with_csv, ): - """ - ExecuteReport Command: Test chart slack report V1 schedule with CSV - """ + """A v1 CSV report fails before calling Slack's retired upload API.""" # setup csv mock response = Mock() mock_open.return_value = response @@ -1790,33 +1830,26 @@ def test_slack_chart_report_schedule_with_csv( mock_urlopen.return_value.getcode.return_value = 200 response.read.return_value = CSV_FILE - notification_targets = get_target_from_report_schedule( - create_report_slack_chart_with_csv - ) - - channel_name = notification_targets[0] - with freeze_time("2020-01-01T00:00:00Z"): - AsyncExecuteReportScheduleCommand( - TEST_ID, create_report_slack_chart_with_csv.id, datetime.utcnow() - ).run() + with pytest.raises(ReportScheduleClientErrorsException): + AsyncExecuteReportScheduleCommand( + TEST_ID, create_report_slack_chart_with_csv.id, datetime.utcnow() + ).run() - assert ( - slack_client_mock_class.return_value.files_upload.call_args[1]["channels"] - == channel_name + expected_message = ( + "Slack v1 file uploads are no longer supported because Slack retired " + "`files.upload`. Enable `ALERT_REPORT_SLACK_V2` and grant the Slack bot " + "both the `channels:read` and `groups:read` scopes so the recipient can " + "be upgraded to Slack v2." ) - assert ( - slack_client_mock_class.return_value.files_upload.call_args[1]["file"] - == CSV_FILE - ) - - # Assert logs are correct - assert_log(ReportState.SUCCESS) + assert_log(ReportState.ERROR, error_message=expected_message) + slack_client_mock_class.assert_not_called() @pytest.mark.usefixtures( "load_birth_names_dashboard_with_slices", "create_report_slack_chart_with_xlsx" ) +@with_feature_flags(ALERT_REPORT_SLACK_V2=False) @patch("superset.reports.notifications.slack.should_use_v2_api", return_value=False) @patch("superset.reports.notifications.slack.get_slack_client") @patch("superset.utils.csv.urllib.request.urlopen") @@ -1830,9 +1863,7 @@ def test_slack_chart_report_schedule_with_xlsx( slack_should_use_v2_api_mock: Mock, create_report_slack_chart_with_xlsx: ReportSchedule, ) -> None: - """ - ExecuteReport Command: Test chart slack report V1 schedule with Excel - """ + """A v1 XLSX report fails before calling Slack's retired upload API.""" # setup xlsx mock response = Mock() mock_open.return_value = response @@ -1840,28 +1871,20 @@ def test_slack_chart_report_schedule_with_xlsx( mock_urlopen.return_value.getcode.return_value = 200 response.read.return_value = XLSX_FILE - notification_targets = get_target_from_report_schedule( - create_report_slack_chart_with_xlsx - ) - - channel_name = notification_targets[0] - with freeze_time("2020-01-01T00:00:00Z"): - AsyncExecuteReportScheduleCommand( - TEST_ID, create_report_slack_chart_with_xlsx.id, datetime.utcnow() - ).run() + with pytest.raises(ReportScheduleClientErrorsException): + AsyncExecuteReportScheduleCommand( + TEST_ID, create_report_slack_chart_with_xlsx.id, datetime.utcnow() + ).run() - assert ( - slack_client_mock_class.return_value.files_upload.call_args[1]["channels"] - == channel_name + expected_message = ( + "Slack v1 file uploads are no longer supported because Slack retired " + "`files.upload`. Enable `ALERT_REPORT_SLACK_V2` and grant the Slack bot " + "both the `channels:read` and `groups:read` scopes so the recipient can " + "be upgraded to Slack v2." ) - assert ( - slack_client_mock_class.return_value.files_upload.call_args[1]["file"] - == XLSX_FILE - ) - - # Assert logs are correct - assert_log(ReportState.SUCCESS) + assert_log(ReportState.ERROR, error_message=expected_message) + slack_client_mock_class.assert_not_called() @pytest.mark.usefixtures( @@ -1931,6 +1954,207 @@ def test_slack_chart_report_schedule_with_text( assert_log(ReportState.SUCCESS) +@pytest.mark.usefixtures( + "load_birth_names_dashboard_with_slices", "create_report_slack_chart_with_text" +) +@patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=([], False), +) +@patch("superset.reports.notifications.slack.should_use_v2_api", return_value=True) +@patch("superset.reports.notifications.slack.get_slack_client") +@patch("superset.commands.report.execute.get_chart_dataframe") +def test_slack_text_fallback_persists_success_for_multiple_recipient_rows( + dataframe_mock, + slack_client_mock, + slack_should_use_v2_api_mock, + get_channels_with_search_mock, + create_report_slack_chart_with_text, +): + """Failed migration sends every text recipient and persists v1 success.""" + dataframe_mock.return_value = pd.DataFrame({"value": [1]}) + original_configs = [ + json.dumps({"target": "private-a"}), + json.dumps({"target": "private-b"}), + ] + create_report_slack_chart_with_text.recipients[ + 0 + ].recipient_config_json = original_configs[0] + create_report_slack_chart_with_text.recipients.append( + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=original_configs[1], + ) + ) + db.session.commit() + report_schedule_id = create_report_slack_chart_with_text.id + + with patch( + "superset.extensions.stats_logger_manager.instance.gauge" + ) as statsd_mock: + AsyncExecuteReportScheduleCommand( + TEST_ID, + report_schedule_id, + datetime.utcnow(), + ).run() + + db.session.expire_all() + persisted_schedule = db.session.get(ReportSchedule, report_schedule_id) + assert persisted_schedule is not None + assert persisted_schedule.last_state == ReportState.SUCCESS + assert all( + recipient.type == ReportRecipientType.SLACK + for recipient in persisted_schedule.recipients + ) + assert { + recipient.recipient_config_json for recipient in persisted_schedule.recipients + } == set(original_configs) + assert { + slack_call.kwargs["channel"] + for slack_call in slack_client_mock.return_value.chat_postMessage.call_args_list + } == {"private-a", "private-b"} + assert slack_client_mock.return_value.chat_postMessage.call_count == 2 + assert statsd_mock.call_args_list == [ + call("reports.slack.send.ok", 1), + call("reports.slack.send.ok", 1), + ] + assert slack_should_use_v2_api_mock.call_count == 1 + get_channels_with_search_mock.assert_called_once_with( + search_string=ANY, + types=ANY, + exact_match=True, + ) + success_logs = ( + db.session.query(ReportExecutionLog) + .filter( + ReportExecutionLog.report_schedule_id == report_schedule_id, + ReportExecutionLog.state == ReportState.SUCCESS, + ) + .all() + ) + assert len(success_logs) == 1 + assert "deprecated Slack v1" in success_logs[0].error_message + + +@pytest.mark.usefixtures( + "load_birth_names_dashboard_with_slices", "create_report_slack_chart_with_text" +) +@patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=([], False), +) +@patch("superset.reports.notifications.slack.should_use_v2_api", return_value=True) +@patch("superset.reports.notifications.slack.get_slack_client") +@patch("superset.commands.report.execute.get_chart_dataframe") +@patch("superset.reports.notifications.email.send_email_smtp") +@pytest.mark.parametrize( + "error_notification_fails", + [False, True], + ids=["notification-succeeds", "notification-fails"], +) +def test_slack_text_fallback_persists_later_recipient_ambiguous_failure( + email_mock, + dataframe_mock, + slack_client_mock, + slack_should_use_v2_api_mock, + get_channels_with_search_mock, + create_report_slack_chart_with_text, + error_notification_fails, +): + """Mixed-outcome errors persist the fallback warning exactly once.""" + dataframe_mock.return_value = pd.DataFrame({"value": [1]}) + original_configs = [ + json.dumps({"target": "private-a"}), + json.dumps({"target": "private-b"}), + ] + create_report_slack_chart_with_text.recipients[ + 0 + ].recipient_config_json = original_configs[0] + create_report_slack_chart_with_text.recipients.append( + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=original_configs[1], + ) + ) + db.session.commit() + report_schedule_id = create_report_slack_chart_with_text.id + + successful_channels: list[str] = [] + + def chat_side_effect(channel, text): + if not successful_channels: + successful_channels.append(channel) + return {"ok": True} + if channel != successful_channels[0]: + raise SlackApiError( + message="service unavailable", + response={"ok": False, "error": "service_unavailable"}, + ) + return {"ok": True} + + slack_client_mock.return_value.chat_postMessage.side_effect = chat_side_effect + if error_notification_fails: + email_mock.side_effect = RuntimeError("SMTP unavailable") + + with ( + patch("time.sleep"), + pytest.raises(ReportScheduleSystemErrorsException), + ): + AsyncExecuteReportScheduleCommand( + TEST_ID, + report_schedule_id, + datetime.utcnow(), + ).run() + + db.session.expire_all() + persisted_schedule = db.session.get(ReportSchedule, report_schedule_id) + assert persisted_schedule is not None + assert persisted_schedule.last_state == ReportState.ERROR + assert all( + recipient.type == ReportRecipientType.SLACK + for recipient in persisted_schedule.recipients + ) + assert { + recipient.recipient_config_json for recipient in persisted_schedule.recipients + } == set(original_configs) + call_channels = [ + slack_call.kwargs["channel"] + for slack_call in slack_client_mock.return_value.chat_postMessage.call_args_list + ] + successful_channel = call_channels[0] + failed_channel = ({"private-a", "private-b"} - {successful_channel}).pop() + assert call_channels == [successful_channel, failed_channel] + execution_logs = ( + db.session.query(ReportExecutionLog) + .filter(ReportExecutionLog.report_schedule_id == report_schedule_id) + .all() + ) + log_states = {log.state for log in execution_logs} + assert log_states == {ReportState.ERROR} + error_messages = [ + log.error_message or "" + for log in execution_logs + if log.state == ReportState.ERROR + ] + warning_messages = [ + message for message in error_messages if "deprecated Slack v1" in message + ] + assert len(warning_messages) == 1 + assert warning_messages[0].count("deprecated Slack v1") == 1 + assert "service unavailable" in warning_messages[0] + last_error_notification = ReportScheduleDAO.find_last_error_notification( + persisted_schedule + ) + if error_notification_fails: + assert any("SMTP unavailable" in message for message in error_messages) + assert last_error_notification is None + else: + assert last_error_notification is not None + assert slack_should_use_v2_api_mock.call_count == 1 + assert get_channels_with_search_mock.call_count == 1 + email_mock.assert_called_once() + + @pytest.mark.usefixtures("create_report_slack_chart") def test_report_schedule_not_found(create_report_slack_chart): """ @@ -2093,20 +2317,24 @@ def test_report_schedule_success_grace(create_alert_slack_chart_success): @pytest.mark.usefixtures("create_alert_slack_chart_grace") -@patch("superset.utils.slack.WebClient.files_upload") +@patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status" +) +@patch("superset.reports.notifications.slack.should_use_v2_api", return_value=True) +@patch("superset.reports.notifications.slackv2.get_slack_client") @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") -@patch("superset.reports.notifications.slack.get_slack_client") def test_report_schedule_success_grace_end( - slack_client_mock_class, screenshot_mock, - file_upload_mock, + slack_client_mock, + slack_should_use_v2_api_mock, + get_channels_with_search_mock, create_alert_slack_chart_grace, + slack_raw_upload_mock, ): - """ - ExecuteReport Command: Test report schedule on grace to noop - """ + """A Slack alert leaving grace upgrades to v2 and sends successfully.""" screenshot_mock.return_value = SCREENSHOT_FILE + slack_client = _configure_v2_upload_client(slack_client_mock.return_value) # set current time to after the grace period current_time = create_alert_slack_chart_grace.last_eval_dttm + timedelta( @@ -2120,9 +2348,17 @@ def test_report_schedule_success_grace_end( channel_name = notification_targets[0] channel_id = "channel_id" - 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": True, + } + ], + False, + ) with freeze_time(current_time): AsyncExecuteReportScheduleCommand( @@ -2131,6 +2367,13 @@ def test_report_schedule_success_grace_end( db.session.commit() assert create_alert_slack_chart_grace.last_state == ReportState.SUCCESS + recipient = create_alert_slack_chart_grace.recipients[0] + assert recipient.type == ReportRecipientType.SLACKV2 + assert json.loads(recipient.recipient_config_json) == {"target": channel_id} + slack_should_use_v2_api_mock.assert_called_once_with(raise_on_error=True) + completion_call = slack_client.files_completeUploadExternal.call_args + assert completion_call.kwargs["channel_id"] == channel_id + assert slack_raw_upload_mock.call_args.kwargs["data"] == SCREENSHOT_FILE @pytest.mark.usefixtures("create_alert_email_chart") @@ -2284,7 +2527,9 @@ 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.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status" +) @patch("superset.utils.slack.WebClient") @patch("superset.utils.screenshots.ChartScreenshot.get_screenshot") def test_slack_token_callable_chart_report( @@ -2301,20 +2546,24 @@ def test_slack_token_callable_chart_report( channel_name = notification_targets[0] channel_id = "channel_id" slack_client_mock_class.return_value = Mock() + _configure_v2_upload_client(slack_client_mock_class.return_value) # 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, - } - ] + get_channels_with_search_mock.return_value = ( + [ + { + "id": channel_id, + "name": channel_name, + "is_member": True, + "is_private": False, + } + ], + False, + ) slack_token_mock = Mock(return_value="cool_code") with patch.dict("flask.current_app.config", {"SLACK_API_TOKEN": slack_token_mock}): @@ -2326,11 +2575,20 @@ def test_slack_token_callable_chart_report( TEST_ID, create_report_slack_chart.id, datetime.utcnow() ).run() slack_token_mock.assert_called() - slack_client_mock_class.assert_called_with( - token="cool_code", # noqa: S106 - proxy=None, - timeout=30, - ) + assert slack_client_mock_class.call_args_list == [ + call( + token="cool_code", # noqa: S106 + proxy=None, + timeout=30, + retry_handlers=None, + ), + call( + token="cool_code", # noqa: S106 + proxy=None, + timeout=30, + retry_handlers=[], + ), + ] assert_log(ReportState.SUCCESS) @@ -2827,9 +3085,10 @@ def test_prune_log_soft_time_out(bulk_delete_logs, create_report_email_dashboard def test__send_with_client_errors(notification_mock, logger_mock): notification_content = "I am some content" recipients = ["test@foo.com"] + report_state = BaseReportState(ReportSchedule(), datetime.utcnow(), uuid4()) notification_mock.return_value.send.side_effect = NotificationParamException() with pytest.raises(ReportScheduleClientErrorsException) as excinfo: - BaseReportState._send(BaseReportState, notification_content, recipients) + report_state._send(notification_content, recipients) assert excinfo.errisinstance(SupersetException) logger_mock.warning.assert_called_with( @@ -2842,13 +3101,14 @@ def test__send_with_client_errors(notification_mock, logger_mock): def test__send_with_multiple_errors(notification_mock, logger_mock): notification_content = "I am some content" recipients = ["test@foo.com", "test2@bar.com"] + report_state = BaseReportState(ReportSchedule(), datetime.utcnow(), uuid4()) notification_mock.return_value.send.side_effect = [ NotificationParamException(), NotificationError(), ] # it raises the error with a 500 status if present with pytest.raises(ReportScheduleSystemErrorsException) as excinfo: - BaseReportState._send(BaseReportState, notification_content, recipients) + report_state._send(notification_content, recipients) assert excinfo.errisinstance(SupersetException) # it logs both errors as warnings @@ -2869,9 +3129,10 @@ def test__send_with_multiple_errors(notification_mock, logger_mock): def test__send_with_server_errors(notification_mock, logger_mock): notification_content = "I am some content" recipients = ["test@foo.com"] + report_state = BaseReportState(ReportSchedule(), datetime.utcnow(), uuid4()) notification_mock.return_value.send.side_effect = NotificationError() with pytest.raises(ReportScheduleSystemErrorsException) as excinfo: - BaseReportState._send(BaseReportState, notification_content, recipients) + report_state._send(notification_content, recipients) assert excinfo.errisinstance(SupersetException) # it logs the error diff --git a/tests/unit_tests/commands/report/execute_test.py b/tests/unit_tests/commands/report/execute_test.py index 65f5d23ecf1..4120481d638 100644 --- a/tests/unit_tests/commands/report/execute_test.py +++ b/tests/unit_tests/commands/report/execute_test.py @@ -19,6 +19,7 @@ import json # noqa: TID251 import time from datetime import datetime, timedelta from typing import Any +from unittest import mock from unittest.mock import MagicMock, Mock, patch from urllib.error import URLError from uuid import UUID, uuid4 @@ -31,6 +32,7 @@ from superset.app import SupersetApp from superset.commands.exceptions import UpdateFailedError from superset.commands.report.exceptions import ( ReportScheduleAlertGracePeriodError, + ReportScheduleClientErrorsException, ReportScheduleCsvFailedError, ReportScheduleExecuteUnexpectedError, ReportScheduleExecutorNotFoundError, @@ -38,6 +40,7 @@ from superset.commands.report.exceptions import ( ReportScheduleScreenshotFailedError, ReportScheduleScreenshotTimeout, ReportScheduleStateNotFoundError, + ReportScheduleSystemErrorsException, ReportScheduleUnexpectedError, ReportScheduleWorkingTimeoutError, ReportScheduleXlsxFailedError, @@ -54,6 +57,7 @@ from superset.commands.report.execute import ( from superset.common.chart_data import ChartDataResultFormat, ChartDataResultType from superset.daos.report import REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER from superset.dashboards.permalink.types import DashboardPermalinkState +from superset.exceptions import SupersetException from superset.reports.models import ( ReportDataFormat, ReportRecipients, @@ -63,6 +67,13 @@ from superset.reports.models import ( ReportSourceFormat, ReportState, ) +from superset.reports.notifications.base import BaseNotification, NotificationContent +from superset.reports.notifications.exceptions import ( + NotificationParamException, + SlackV1NotificationError, +) +from superset.reports.notifications.slack import SlackNotification +from superset.reports.notifications.slack_channel_resolver import _match_slack_channel from superset.subjects.types import SubjectType from superset.utils.core import HeaderDataType from superset.utils.report_execution import ( @@ -71,9 +82,25 @@ from superset.utils.report_execution import ( ReportExecutionDeadline, ) from superset.utils.screenshots import ChartScreenshot +from superset.utils.slack import ( + SlackChannel, + SlackChannelListingClientError, + SlackV2ProbeClientError, + SlackV2ProbeError, +) from tests.integration_tests.conftest import with_feature_flags +def test_match_slack_channel_rejects_ambiguous_casefolded_names() -> None: + channels: list[SlackChannel] = [ + {"id": "C1", "name": "Private-Channel", "is_private": True}, + {"id": "C2", "name": "private-channel", "is_private": True}, + ] + + with pytest.raises(NotificationParamException, match="ambiguous"): + _match_slack_channel("PRIVATE-CHANNEL", channels) + + def _make_mock_editors(mocker: MockerFixture, user_ids: list[int]) -> list[Mock]: """Create mock editor subjects with user-type attributes.""" editors = [] @@ -88,6 +115,20 @@ def _make_mock_editors(mocker: MockerFixture, user_ids: list[int]) -> list[Mock] return editors +def _make_notification_header() -> HeaderDataType: + """Build the minimum complete report header used by notification tests.""" + return { + "notification_format": "TEXT", + "notification_type": "Report", + "editors": [], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": None, + "execution_id": "execution_id_example", + } + + def test_log_data_with_chart(mocker: MockerFixture) -> None: mock_report_schedule: ReportSchedule = mocker.Mock(spec=ReportSchedule) mock_report_schedule.chart = True @@ -1785,27 +1826,30 @@ def test_update_recipient_to_slack_v2(mocker: MockerFixture): Test converting a Slack recipient to Slack v2 format. """ mocker.patch( - "superset.commands.report.execute.get_channels_with_search", - return_value=[ - { - "id": "abc124f", - "name": "channel-1", - "is_member": True, - "is_private": False, - }, - { - "id": "blah_!channel_2", - "name": "Channel_2", - "is_member": True, - "is_private": False, - }, - ], + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=( + [ + { + "id": "abc124f", + "name": "channel-1", + "is_member": True, + "is_private": False, + }, + { + "id": "blah_!channel_2", + "name": "Straße", + "is_member": True, + "is_private": False, + }, + ], + False, + ), ) mock_report_schedule = ReportSchedule( recipients=[ ReportRecipients( type=ReportRecipientType.SLACK, - recipient_config_json=json.dumps({"target": "Channel-1, Channel_2"}), + recipient_config_json=json.dumps({"target": "Channel-1, STRASSE"}), ), ], ) @@ -1828,15 +1872,18 @@ def test_update_recipient_to_slack_v2_missing_channels(mocker: MockerFixture): in case it can't find all channels. """ mocker.patch( - "superset.commands.report.execute.get_channels_with_search", - return_value=[ - { - "id": "blah_!channel_2", - "name": "Channel 2", - "is_member": True, - "is_private": False, - }, - ], + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=( + [ + { + "id": "blah_!channel_2", + "name": "Channel 2", + "is_member": True, + "is_private": False, + }, + ], + False, + ), ) mock_report_schedule = ReportSchedule( name="Test Report", @@ -1851,24 +1898,186 @@ def test_update_recipient_to_slack_v2_missing_channels(mocker: MockerFixture): mock_cmmd: BaseReportState = BaseReportState( mock_report_schedule, "January 1, 2021", "execution_id_example" ) - with pytest.raises(UpdateFailedError): + with pytest.raises(NotificationParamException): mock_cmmd.update_report_schedule_slack_v2() +def test_update_recipient_to_slack_v2_preserves_permanent_listing_failure( + mocker: MockerFixture, +) -> None: + """Permanent listing failures remain client errors during migration.""" + mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + side_effect=SlackChannelListingClientError("invalid_auth"), + ) + state = BaseReportState( + ReportSchedule( + recipients=[ + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "private-channel"}), + ) + ] + ), + "January 1, 2021", + "execution_id_example", + ) + + with pytest.raises(NotificationParamException, match="invalid_auth"): + state.update_report_schedule_slack_v2() + + +def test_update_recipient_to_slack_v2_refreshes_stale_channel_cache( + mocker: MockerFixture, +) -> None: + """A cache miss gets one fresh lookup before the upgrade falls back.""" + channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=([], True), + ) + refreshed_channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.refresh_cached_slack_channels_with_search", + return_value=[ + {"id": "C2", "name": "second", "is_private": True}, + {"id": "C1", "name": "channel-1", "is_private": False}, + ], + ) + recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "Channel-1,C2"}), + ) + state = BaseReportState( + ReportSchedule(recipients=[recipient]), + "January 1, 2021", + "execution_id_example", + ) + + state.update_report_schedule_slack_v2() + + channel_search.assert_called_once_with( + search_string="Channel-1,C2", + types=mocker.ANY, + exact_match=True, + ) + refreshed_channel_search.assert_called_once_with( + search_string="Channel-1,C2", + types=mocker.ANY, + exact_match=True, + ) + assert recipient.type == ReportRecipientType.SLACKV2 + assert recipient.recipient_config_json == '{"target": "C1,C2"}' + + +def test_update_recipient_to_slack_v2_skips_refresh_after_live_cache_miss( + mocker: MockerFixture, +) -> None: + """A live lookup is not repeated when no cached channel list existed.""" + channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=([], False), + ) + recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "private-channel"}), + ) + state = BaseReportState( + ReportSchedule(recipients=[recipient]), + "January 1, 2021", + "execution_id_example", + ) + + with pytest.raises(NotificationParamException, match="private-channel"): + state.update_report_schedule_slack_v2() + + channel_search.assert_called_once_with( + search_string="private-channel", + types=mocker.ANY, + exact_match=True, + ) + + +def test_update_recipient_to_slack_v2_prefers_exact_id_over_name_collision( + mocker: MockerFixture, +) -> None: + """A canonical Slack ID cannot be shadowed by another channel's name.""" + mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=( + [ + {"id": "C012AB3CD", "name": "reports", "is_private": True}, + {"id": "C999ZZ9ZZ", "name": "c012ab3cd", "is_private": False}, + ], + False, + ), + ) + id_recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "C012AB3CD"}), + ) + name_recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "c012ab3cd"}), + ) + state = BaseReportState( + ReportSchedule(recipients=[id_recipient, name_recipient]), + "January 1, 2021", + "execution_id_example", + ) + + state.update_report_schedule_slack_v2() + + assert id_recipient.recipient_config_json == '{"target": "C012AB3CD"}' + assert name_recipient.recipient_config_json == '{"target": "C999ZZ9ZZ"}' + + +def test_update_recipient_to_slack_v2_reports_only_unresolved_channels( + mocker: MockerFixture, +) -> None: + """Diagnostics use the same case-insensitive name-or-id match as resolution.""" + mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=( + [ + {"id": "C123", "name": "private-channel", "is_private": True}, + {"id": "C999", "name": "other", "is_private": False}, + ], + False, + ), + ) + recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps( + {"target": "PRIVATE-CHANNEL,c999,missing-channel"} + ), + ) + state = BaseReportState( + ReportSchedule(recipients=[recipient]), + "January 1, 2021", + "execution_id_example", + ) + + with pytest.raises(NotificationParamException) as exc_info: + state.update_report_schedule_slack_v2() + + assert "missing-channel" in str(exc_info.value) + assert "PRIVATE-CHANNEL" not in str(exc_info.value) + assert "c999" not in str(exc_info.value) + + def test_update_recipient_to_slack_v2_multiple_recipients( mocker: MockerFixture, ) -> None: - """All Slack recipients are upgraded atomically when every channel resolves.""" + """All recipients share one live listing, including metastore cache misses.""" - 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, + channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=( + [ + {"id": "C1", "name": "channel-1", "is_private": False}, + {"id": "C2", "name": "channel-2", "is_private": False}, + ], + False, + ), ) mock_report_schedule = ReportSchedule( recipients=[ @@ -1895,6 +2104,57 @@ def test_update_recipient_to_slack_v2_multiple_recipients( ] assert recipients[0].recipient_config_json == '{"target": "C1"}' assert recipients[1].recipient_config_json == '{"target": "C2"}' + channel_search.assert_called_once_with( + search_string="channel-1,channel-2", + types=mocker.ANY, + exact_match=True, + ) + + +def test_update_recipient_to_slack_v2_multiple_recipients_share_stale_refresh( + mocker: MockerFixture, +) -> None: + """All recipients share one initial cache read and one stale-cache refresh.""" + channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=([], True), + ) + refreshed_channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.refresh_cached_slack_channels_with_search", + return_value=[ + {"id": "C1", "name": "channel-1", "is_private": False}, + {"id": "C2", "name": "channel-2", "is_private": True}, + ], + ) + recipients = [ + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": channel}), + ) + for channel in ("channel-1", "channel-2") + ] + state = BaseReportState( + ReportSchedule(recipients=recipients), + "January 1, 2021", + "execution_id_example", + ) + + state.update_report_schedule_slack_v2() + + channel_search.assert_called_once_with( + search_string="channel-1,channel-2", + types=mocker.ANY, + exact_match=True, + ) + refreshed_channel_search.assert_called_once_with( + search_string="channel-1,channel-2", + types=mocker.ANY, + exact_match=True, + ) + assert [recipient.recipient_config_json for recipient in recipients] == [ + '{"target": "C1"}', + '{"target": "C2"}', + ] def test_update_recipient_to_slack_v2_partial_failure_is_atomic( @@ -1910,15 +2170,12 @@ def test_update_recipient_to_slack_v2_partial_failure_is_atomic( persist as a half-upgraded schedule. """ - 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=fake_get_channels, + channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=( + [{"id": "C1", "name": "channel-1", "is_private": False}], + False, + ), ) first_config = json.dumps({"target": "channel-1"}) second_config = json.dumps({"target": "missing-channel"}) @@ -1938,7 +2195,7 @@ def test_update_recipient_to_slack_v2_partial_failure_is_atomic( mock_cmmd: BaseReportState = BaseReportState( mock_report_schedule, "January 1, 2021", "execution_id_example" ) - with pytest.raises(UpdateFailedError): + with pytest.raises(NotificationParamException): mock_cmmd.update_report_schedule_slack_v2() recipients = mock_cmmd._report_schedule.recipients @@ -1950,6 +2207,11 @@ def test_update_recipient_to_slack_v2_partial_failure_is_atomic( ] assert recipients[0].recipient_config_json == first_config assert recipients[1].recipient_config_json == second_config + channel_search.assert_called_once_with( + search_string="channel-1,missing-channel", + types=mocker.ANY, + exact_match=True, + ) def test_update_recipient_to_slack_v2_pre_iteration_failure( @@ -1983,7 +2245,7 @@ def test_update_recipient_to_slack_v2_no_slack_recipients_is_noop( without raising and leaves the non-Slack recipients untouched. """ mock_search = mocker.patch( - "superset.commands.report.execute.get_channels_with_search", + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", ) mock_report_schedule = ReportSchedule( recipients=[ @@ -2007,6 +2269,830 @@ def test_update_recipient_to_slack_v2_no_slack_recipients_is_noop( mock_search.assert_not_called() +@pytest.mark.parametrize( + "recipient_config_json", + [ + "{not-json", + json.dumps({"target": ["private-channel"]}), + ], + ids=["malformed-json", "non-string-target"], +) +def test_update_recipient_to_slack_v2_rejects_invalid_config_without_traceback( + mocker: MockerFixture, + recipient_config_json: str, +) -> None: + """Operator-fixable recipient configuration logs no exception traceback.""" + recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=recipient_config_json, + ) + state = BaseReportState( + ReportSchedule(recipients=[recipient]), + "January 1, 2021", + "execution_id_example", + ) + logger = mocker.patch("superset.commands.report.slack_upgrade.logger") + + with pytest.raises(NotificationParamException): + state.update_report_schedule_slack_v2() + + logger.warning.assert_called_once() + logger.exception.assert_not_called() + assert recipient.type == ReportRecipientType.SLACK + assert recipient.recipient_config_json == recipient_config_json + + +def test_update_recipient_to_slack_v2_deduplicates_channels( + mocker: MockerFixture, +) -> None: + """Repeated channel names resolve once and persist one channel id.""" + channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=( + [ + { + "id": "C1", + "name": "private-channel", + "is_private": True, + } + ], + False, + ), + ) + recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps( + {"target": "private-channel, private-channel"} + ), + ) + state = BaseReportState( + ReportSchedule(recipients=[recipient]), + "January 1, 2021", + "execution_id_example", + ) + + state.update_report_schedule_slack_v2() + + channel_search.assert_called_once_with( + search_string="private-channel", + types=mocker.ANY, + exact_match=True, + ) + assert recipient.type == ReportRecipientType.SLACKV2 + assert recipient.recipient_config_json == '{"target": "C1"}' + + +@pytest.mark.parametrize("probe_result", [True, False]) +def test_send_falls_back_to_slack_v1_when_private_channels_upgrade_fails( + app: SupersetApp, + mocker: MockerFixture, + probe_result: bool, +) -> None: + """A failed probe or migration must record fallback for every recipient row.""" + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + original_configs = [ + json.dumps({"target": "private-a"}), + json.dumps({"target": "private-b"}), + ] + recipients = [ + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=config, + ) + for config in original_configs + ] + report_schedule = ReportSchedule( + name="Private channel report", + recipients=recipients, + ) + report_state = BaseReportState( + report_schedule, + "January 1, 2021", + "execution_id_example", + ) + notification_content = NotificationContent( + name="Private channel report", + header_data={ + "notification_format": "TEXT", + "notification_type": "Report", + "editors": [], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": ["private-a", "private-b"], + "execution_id": "execution_id_example", + }, + description="Text-only report", + url="https://superset.example/report", + ) + v2_probe = mocker.patch( + "superset.reports.notifications.slack.should_use_v2_api", + return_value=probe_result, + ) + mocker.patch( + "superset.reports.notifications.slack.feature_flag_manager.is_feature_enabled", + return_value=True, + ) + channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=([], False), + ) + slack_client = mocker.patch("superset.reports.notifications.slack.get_slack_client") + stats_logger = mocker.Mock() + mocker.patch.dict(app.config, {"STATS_LOGGER": stats_logger}) + mocker.patch("superset.reports.notifications.slack.g", logs_context={}) + + report_state._send(notification_content, report_schedule.recipients) + + assert v2_probe.call_count == 1 + channel_search.assert_called_once_with( + search_string="private-a,private-b", + types=mocker.ANY, + exact_match=True, + ) + stats_logger.incr.assert_called_once_with("reports.slack.v1_fallback") + assert len(report_state._execution_warnings) == 1 + assert "deprecated Slack v1" in report_state._execution_warnings[0] + assert "private-a" in report_state._execution_warnings[0] + assert slack_client.return_value.chat_postMessage.call_count == 2 + assert [ + call.kwargs["channel"] + for call in slack_client.return_value.chat_postMessage.call_args_list + ] == ["private-a", "private-b"] + assert [recipient.type for recipient in recipients] == [ + ReportRecipientType.SLACK, + ReportRecipientType.SLACK, + ] + assert [recipient.recipient_config_json for recipient in recipients] == ( + original_configs + ) + + +def test_failed_upgraded_delivery_restores_slack_v1_recipients( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + """A failed v2 delivery must not persist the execution's recipient upgrade.""" + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + original_configs = [ + json.dumps({"target": "private-a"}), + json.dumps({"target": "private-b"}), + ] + recipients = [ + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=config, + ) + for config in original_configs + ] + state = BaseReportState( + ReportSchedule(name="Private channel report", recipients=recipients), + "January 1, 2021", + "execution_id_example", + ) + content = NotificationContent( + name="Private channel report", + header_data=_make_notification_header(), + ) + channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=( + [ + {"id": "C1", "name": "private-a", "is_private": True}, + {"id": "C2", "name": "private-b", "is_private": True}, + ], + False, + ), + ) + legacy = mocker.Mock(spec=SlackNotification) + legacy.send.side_effect = SlackV1NotificationError + failed_upgrade = mocker.Mock(spec=BaseNotification) + failed_upgrade.send.side_effect = NotificationParamException("v2 send failed") + later_upgrade = mocker.Mock(spec=BaseNotification) + create_notification_mock = mocker.patch( + "superset.commands.report.execute.create_notification", + side_effect=[legacy, failed_upgrade, later_upgrade], + ) + + with pytest.raises(ReportScheduleClientErrorsException, match="v2 send failed"): + state._send(content, recipients) + + assert create_notification_mock.call_count == 3 + channel_search.assert_called_once() + later_upgrade.send.assert_called_once_with() + assert [recipient.type for recipient in recipients] == [ + ReportRecipientType.SLACK, + ReportRecipientType.SLACK, + ] + assert [recipient.recipient_config_json for recipient in recipients] == ( + original_configs + ) + + +def test_unexpected_upgraded_delivery_failure_restores_slack_v1_recipient( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + """Unexpected transport failures must not leak a pending recipient upgrade.""" + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + original_config = json.dumps({"target": "private-channel"}) + recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=original_config, + ) + state = BaseReportState( + ReportSchedule(name="Private channel report", recipients=[recipient]), + "January 1, 2021", + "execution_id_example", + ) + mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=( + [{"id": "C1", "name": "private-channel", "is_private": True}], + False, + ), + ) + legacy = mocker.Mock(spec=SlackNotification) + legacy.send.side_effect = SlackV1NotificationError + failed_upgrade = mocker.Mock(spec=BaseNotification) + failed_upgrade.send.side_effect = RuntimeError("unexpected v2 failure") + mocker.patch( + "superset.commands.report.execute.create_notification", + side_effect=[legacy, failed_upgrade], + ) + + with pytest.raises(RuntimeError, match="unexpected v2 failure"): + state._send( + NotificationContent( + name="Private channel report", + header_data=_make_notification_header(), + ), + [recipient], + ) + + assert recipient.type == ReportRecipientType.SLACK + assert recipient.recipient_config_json == original_config + + +def test_send_records_system_upgrade_failure_when_text_fallback_succeeds( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + """Delivery continuity retains an observable system-failure signal.""" + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + stats_logger = mocker.Mock() + mocker.patch.dict(app.config, {"STATS_LOGGER": stats_logger}) + recipients = [ + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": channel}), + ) + for channel in ("private-a", "private-b") + ] + report_schedule = ReportSchedule( + id=42, + name="Private channel report", + recipients=recipients, + ) + report_state = BaseReportState( + report_schedule, + "January 1, 2021", + "execution_id_example", + ) + notification_content = NotificationContent( + name="Private channel report", + header_data={ + "notification_format": "TEXT", + "notification_type": "Report", + "editors": [], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": ["private-channel"], + "execution_id": "execution_id_example", + }, + description="Text-only report", + url="https://superset.example/report", + ) + mocker.patch( + "superset.reports.notifications.slack.should_use_v2_api", + return_value=True, + ) + mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + side_effect=SupersetException("Slack channel listing unavailable"), + ) + slack_client = mocker.patch("superset.reports.notifications.slack.get_slack_client") + logger = mocker.patch("superset.commands.report.slack_upgrade.logger") + mocker.patch("superset.reports.notifications.slack.g", logs_context={}) + + report_state._send(notification_content, recipients) + + assert [ + slack_call.kwargs["channel"] + for slack_call in slack_client.return_value.chat_postMessage.call_args_list + ] == ["private-a", "private-b"] + assert stats_logger.incr.call_args_list == [ + mock.call("reports.slack.v1_fallback"), + mock.call("reports.slack.v1_fallback.system_error"), + ] + logger.error.assert_called_once() + assert logger.error.call_args.kwargs["extra"] == { + "execution_id": "execution_id_example", + "report_schedule_id": 42, + } + + +def test_failed_slack_v1_fallback_does_not_record_delivery( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + stats_logger = mocker.Mock() + mocker.patch.dict(app.config, {"STATS_LOGGER": stats_logger}) + report_schedule = ReportSchedule(id=42, name="Private channel report") + report_state = BaseReportState( + report_schedule, + "January 1, 2021", + "execution_id_example", + ) + notification = mocker.Mock() + notification.send_legacy_text.side_effect = NotificationParamException( + "Slack delivery failed" + ) + content = mocker.Mock() + content.has_attachments = False + + with pytest.raises(NotificationParamException, match="Slack delivery failed"): + report_state._slack_v1_upgrade.send_fallback( + notification, + content, + UpdateFailedError("Slack upgrade failed"), + ) + + assert report_state._execution_warnings == [] + stats_logger.incr.assert_not_called() + + +def test_later_successful_fallback_records_delivery_after_first_failure( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + """Observability is recorded after the first successful fallback recipient.""" + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + stats_logger = mocker.Mock() + mocker.patch.dict(app.config, {"STATS_LOGGER": stats_logger}) + recipients = [ + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": channel}), + ) + for channel in ("private-a", "private-b") + ] + state = BaseReportState( + ReportSchedule(id=42, name="Private channel report", recipients=recipients), + "January 1, 2021", + "execution_id_example", + ) + content = NotificationContent( + name="Private channel report", + header_data={ + "notification_format": "TEXT", + "notification_type": "Report", + "editors": [], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": ["private-a", "private-b"], + "execution_id": "execution_id_example", + }, + description="Text-only report", + url="https://superset.example/report", + ) + notifications = [mocker.Mock(spec=SlackNotification) for _ in recipients] + notifications[0].send.side_effect = SlackV1NotificationError + notifications[0].send_legacy_text.side_effect = NotificationParamException( + "first delivery failed" + ) + mocker.patch( + "superset.commands.report.execute.create_notification", + side_effect=notifications, + ) + mocker.patch.object( + state._slack_v1_upgrade, + "update_recipients", + side_effect=UpdateFailedError("Slack upgrade failed"), + ) + + with pytest.raises(ReportScheduleClientErrorsException): + state._send(content, recipients) + + notifications[1].send_legacy_text.assert_called_once_with() + stats_logger.incr.assert_has_calls( + [ + mock.call("reports.slack.v1_fallback"), + mock.call("reports.slack.v1_fallback.system_error"), + ] + ) + assert len(state._execution_warnings) == 1 + + +def test_failed_slack_upgrade_fallback_does_not_affect_other_recipient_types( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + """Only the legacy Slack recipient uses fallback in a mixed schedule.""" + from superset.reports.notifications.slack import SlackNotification + + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + stats_logger = mocker.Mock() + mocker.patch.dict(app.config, {"STATS_LOGGER": stats_logger}) + recipients = [ + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "private-channel"}), + ), + ReportRecipients( + type=ReportRecipientType.SLACKV2, + recipient_config_json=json.dumps({"target": "C123"}), + ), + ReportRecipients( + type=ReportRecipientType.EMAIL, + recipient_config_json=json.dumps({"target": "user@example.com"}), + ), + ] + report_schedule = ReportSchedule( + name="Mixed recipient report", + recipients=recipients, + ) + state = BaseReportState( + report_schedule, + "January 1, 2021", + "execution_id_example", + ) + content = NotificationContent( + name="Mixed recipient report", + header_data={ + "notification_format": "TEXT", + "notification_type": "Report", + "editors": [], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": ["private-channel"], + "execution_id": "execution_id_example", + }, + description="Text-only report", + url="https://superset.example/report", + ) + legacy_notification = SlackNotification(recipients[0], content) + v2_notification = mocker.Mock() + email_notification = mocker.Mock() + mocker.patch( + "superset.commands.report.execute.create_notification", + side_effect=[legacy_notification, v2_notification, email_notification], + ) + mocker.patch( + "superset.reports.notifications.slack.should_use_v2_api", + return_value=True, + ) + channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=([], False), + ) + slack_client = mocker.patch("superset.reports.notifications.slack.get_slack_client") + mocker.patch("superset.reports.notifications.slack.g", logs_context={}) + + state._send(content, recipients) + + assert channel_search.call_count == 1 + slack_client.return_value.chat_postMessage.assert_called_once_with( + channel="private-channel", + text=mocker.ANY, + ) + v2_notification.send.assert_called_once_with() + email_notification.send.assert_called_once_with() + stats_logger.incr.assert_called_once_with("reports.slack.v1_fallback") + + +def test_send_malformed_slack_recipient_does_not_suppress_later_recipient( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + """A malformed recipient is aggregated while later recipients still send.""" + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + recipients = [ + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({}), + ), + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "private-b"}), + ), + ] + report_schedule = ReportSchedule( + name="Private channel report", + recipients=recipients, + ) + report_state = BaseReportState( + report_schedule, + "January 1, 2021", + "execution_id_example", + ) + notification_content = NotificationContent( + name="Private channel report", + header_data={ + "notification_format": "TEXT", + "notification_type": "Report", + "editors": [], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": ["private-b"], + "execution_id": "execution_id_example", + }, + description="Text-only report", + url="https://superset.example/report", + ) + v2_probe = mocker.patch( + "superset.reports.notifications.slack.should_use_v2_api", + return_value=True, + ) + channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + ) + slack_client = mocker.patch("superset.reports.notifications.slack.get_slack_client") + mocker.patch("superset.reports.notifications.slack.g", logs_context={}) + + with pytest.raises(ReportScheduleClientErrorsException) as exc_info: + report_state._send(notification_content, recipients) + + assert exc_info.value.errors[0].message == "No recipients saved in the report" + slack_client.return_value.chat_postMessage.assert_called_once_with( + channel="private-b", + text=mocker.ANY, + ) + assert v2_probe.call_count == 1 + channel_search.assert_not_called() + assert [recipient.type for recipient in recipients] == [ + ReportRecipientType.SLACK, + ReportRecipientType.SLACK, + ] + + +@pytest.mark.parametrize( + "attachment", + [ + {"screenshots": [b"screenshot"]}, + {"xlsx": b"xlsx_content"}, + ], + ids=["screenshot", "xlsx"], +) +def test_send_preserves_transient_upgrade_failure_for_file_reports( + app: SupersetApp, + mocker: MockerFixture, + attachment: dict[str, Any], +) -> None: + """A transient v2 migration failure remains a system error for files.""" + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "private-channel"}), + ) + report_schedule = ReportSchedule( + name="Private channel report", + recipients=[recipient], + ) + report_state = BaseReportState( + report_schedule, + "January 1, 2021", + "execution_id_example", + ) + notification_content = NotificationContent( + name="Private channel report", + header_data={ + "notification_format": "PNG", + "notification_type": "Report", + "editors": [], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": ["private-channel"], + "execution_id": "execution_id_example", + }, + description="File-bearing report", + url="https://superset.example/report", + **attachment, + ) + mocker.patch( + "superset.reports.notifications.slack.should_use_v2_api", + return_value=True, + ) + mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + side_effect=SupersetException("Slack channel listing unavailable"), + ) + slack_client = mocker.patch("superset.reports.notifications.slack.get_slack_client") + mocker.patch("superset.reports.notifications.slack.g", logs_context={}) + statsd_mock = mocker.patch( + "superset.extensions.stats_logger_manager.instance.gauge" + ) + + with pytest.raises(ReportScheduleSystemErrorsException) as exc_info: + report_state._send(notification_content, [recipient]) + + error_message = exc_info.value.errors[0].message + assert "Slack v1 file uploads are no longer supported" in error_message + assert "channels:read" in error_message + assert "groups:read" in error_message + assert "Slack channel listing unavailable" in error_message + slack_client.assert_not_called() + statsd_mock.assert_called_once_with("reports.slack.send.error", 1) + assert recipient.type == ReportRecipientType.SLACK + assert recipient.recipient_config_json == '{"target": "private-channel"}' + + +@pytest.mark.parametrize( + "probe_error,expected_exception", + [ + ( + SlackV2ProbeError( + "Slack v2 availability probe failed: service_unavailable" + ), + ReportScheduleSystemErrorsException, + ), + ( + SlackV2ProbeClientError("Slack v2 availability probe failed: invalid_auth"), + ReportScheduleClientErrorsException, + ), + ], + ids=["system", "client"], +) +def test_send_classifies_probe_failure_for_file_reports( + app: SupersetApp, + mocker: MockerFixture, + probe_error: SlackV2ProbeError, + expected_exception: type[Exception], +) -> None: + """Slack capability probe failures retain system/client classification.""" + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "private-channel"}), + ) + report_schedule = ReportSchedule( + name="Private channel report", + recipients=[recipient], + ) + report_state = BaseReportState( + report_schedule, + "January 1, 2021", + "execution_id_example", + ) + notification_content = NotificationContent( + name="Private channel report", + header_data={ + "notification_format": "PNG", + "notification_type": "Report", + "editors": [], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": ["private-channel"], + "execution_id": "execution_id_example", + }, + screenshots=[b"screenshot"], + description="File-bearing report", + url="https://superset.example/report", + ) + mocker.patch( + "superset.reports.notifications.slack.should_use_v2_api", + side_effect=probe_error, + ) + slack_client = mocker.patch("superset.reports.notifications.slack.get_slack_client") + mocker.patch("superset.reports.notifications.slack.g", logs_context={}) + + with pytest.raises(expected_exception) as exc_info: + report_state._send(notification_content, [recipient]) + + assert str(probe_error) in exc_info.value.errors[0].message + slack_client.assert_not_called() + + +def test_send_classifies_malformed_file_recipient_as_client_error( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + """Malformed file recipients retain actionable client classification.""" + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({}), + ) + report_schedule = ReportSchedule( + name="Private channel report", + recipients=[recipient], + ) + report_state = BaseReportState( + report_schedule, + "January 1, 2021", + "execution_id_example", + ) + notification_content = NotificationContent( + name="Private channel report", + header_data={ + "notification_format": "PNG", + "notification_type": "Report", + "editors": [], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": [], + "execution_id": "execution_id_example", + }, + screenshots=[b"screenshot"], + description="File-bearing report", + url="https://superset.example/report", + ) + mocker.patch( + "superset.reports.notifications.slack.should_use_v2_api", + return_value=True, + ) + channel_search = mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + ) + slack_client = mocker.patch("superset.reports.notifications.slack.get_slack_client") + mocker.patch("superset.reports.notifications.slack.g", logs_context={}) + + with pytest.raises(ReportScheduleClientErrorsException) as exc_info: + report_state._send(notification_content, [recipient]) + + assert "No recipients saved in the report" in exc_info.value.errors[0].message + slack_client.assert_not_called() + channel_search.assert_not_called() + assert recipient.type == ReportRecipientType.SLACK + assert recipient.recipient_config_json == "{}" + + +def test_send_does_not_fall_back_to_slack_v1_for_file_uploads( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + """A failed v2 migration must not retry a retired v1 file upload.""" + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + recipient = ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=json.dumps({"target": "private-channel"}), + ) + report_schedule = ReportSchedule( + name="Private channel report", + recipients=[recipient], + ) + report_state = BaseReportState( + report_schedule, + "January 1, 2021", + "execution_id_example", + ) + notification_content = NotificationContent( + name="Private channel report", + header_data={ + "notification_format": "PNG", + "notification_type": "Report", + "editors": [], + "notification_source": None, + "chart_id": None, + "dashboard_id": None, + "slack_channels": ["private-channel"], + "execution_id": "execution_id_example", + }, + screenshots=[b"screenshot"], + description="File-bearing report", + url="https://superset.example/report", + ) + mocker.patch( + "superset.reports.notifications.slack.should_use_v2_api", + return_value=True, + ) + mocker.patch( + "superset.reports.notifications.slack_channel_resolver.get_channels_with_search_and_cache_status", + return_value=([], False), + ) + slack_client = mocker.patch("superset.reports.notifications.slack.get_slack_client") + mocker.patch("superset.reports.notifications.slack.g", logs_context={}) + + with pytest.raises(ReportScheduleClientErrorsException) as exc_info: + report_state._send(notification_content, report_schedule.recipients) + + error_message = str(exc_info.value.errors[0].message) + assert "Slack v1 file uploads are no longer supported" in error_message + assert "`channels:read` and `groups:read`" in error_message + assert "Could not find the following channels: private-channel" in error_message + slack_client.return_value.files_upload.assert_not_called() + slack_client.return_value.chat_postMessage.assert_not_called() + assert recipient.type == ReportRecipientType.SLACK + assert recipient.recipient_config_json == '{"target": "private-channel"}' + + # --------------------------------------------------------------------------- # Tier 1: _update_query_context + create_log # --------------------------------------------------------------------------- @@ -2104,6 +3190,7 @@ def _make_notification_state( schedule.description = "desc" schedule.email_subject = email_subject schedule.force_screenshot = False + schedule.working_timeout = None schedule.recipients = [] schedule.editors = [] @@ -2143,6 +3230,52 @@ def test_get_notification_content_png_screenshot( assert content.text is None +def test_slack_retry_deadline_flows_from_report_state_to_transport( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + """One absolute execution deadline reaches every Slack v2 destination.""" + app.config["ALERT_REPORTS_NOTIFICATION_DRY_RUN"] = False + state = _make_notification_state(mocker, report_format=ReportDataFormat.PNG) + mocker.patch.object(state, "_get_screenshots", return_value=[b"img"]) + deadline_factory = mocker.patch( + "superset.commands.report.execute.get_slack_send_retry_deadline", + return_value=123.0, + ) + mocker.patch("superset.reports.notifications.slackv2.get_slack_client") + send_to_channels = mocker.patch( + "superset.reports.notifications.slackv2.send_to_slack_channels" + ) + recipient = ReportRecipients( + type=ReportRecipientType.SLACKV2, + recipient_config_json='{"target": "C1"}', + ) + + content = state._get_notification_content() + state._send_notification(content, recipient) + + assert content.slack_retry_deadline == 123.0 + deadline_factory.assert_called_once_with(None) + assert send_to_channels.call_args.kwargs["retry_deadline"] == 123.0 + + +def test_slack_retry_deadline_clamps_elapsed_working_timeout( + mocker: MockerFixture, +) -> None: + """An exhausted report timeout produces an already-expired Slack deadline.""" + state = _make_notification_state(mocker) + state._report_schedule.working_timeout = 10 + state._start_dttm = datetime.utcnow() - timedelta(seconds=11) + mocker.patch("superset.commands.report.execute.time.monotonic", return_value=50.0) + deadline_factory = mocker.patch( + "superset.commands.report.execute.get_slack_send_retry_deadline", + side_effect=lambda deadline: deadline, + ) + + assert state._get_slack_retry_deadline() == 50.0 + deadline_factory.assert_called_once_with(50.0) + + @patch("superset.commands.report.execute.feature_flag_manager") def test_get_notification_content_png_empty_returns_error( mock_ff, mocker: MockerFixture @@ -2525,6 +3658,30 @@ def test_not_triggered_error_state_send_failure_logs_error_and_reraises( assert "send failed" in error_msg +def test_not_triggered_error_state_success_clears_retry_state( + mocker: MockerFixture, +) -> None: + """A successful retry clears its persisted retry-window state.""" + state = _make_state_instance( + mocker, + ReportNotTriggeredErrorState, + schedule_type=ReportScheduleType.REPORT, + ) + state._report_schedule.retry_attempt = 2 + state._report_schedule.retry_scheduled_dttm = datetime.utcnow() + mocker.patch.object(state, "send") + mock_update = mocker.patch.object(state, "update_report_schedule_and_log") + + state.next() + + assert state._report_schedule.retry_attempt == 0 + assert state._report_schedule.retry_scheduled_dttm is None + assert mock_update.call_args_list == [ + mocker.call(ReportState.WORKING), + mocker.call(ReportState.SUCCESS, error_message=None), + ] + + # --------------------------------------------------------------------------- # Phase 1 remaining gaps # --------------------------------------------------------------------------- @@ -2678,6 +3835,86 @@ def test_create_log_success_commits(mocker: MockerFixture) -> None: mock_db.session.rollback.assert_not_called() +def test_create_log_includes_execution_warnings_with_error( + mocker: MockerFixture, +) -> None: + """A recipient failure does not hide warnings from successful recipients.""" + schedule = mocker.Mock(spec=ReportSchedule) + schedule.last_value = None + schedule.last_value_row_json = None + schedule.last_state = ReportState.ERROR + + state = BaseReportState(schedule, datetime.utcnow(), uuid4()) + state._execution_warnings.append("Slack v1 fallback is deprecated") + + mock_db = mocker.patch("superset.commands.report.execute.db") + working_log = mocker.Mock() + mock_db.session.query.return_value.filter.return_value.first.return_value = ( + working_log + ) + + state.create_log(error_message="Email delivery failed") + + assert working_log.error_message == ( + "Slack v1 fallback is deprecated;Email delivery failed" + ) + + +def test_create_log_preserves_error_notification_marker( + mocker: MockerFixture, +) -> None: + """Execution warnings do not alter the grace-period lookup marker.""" + schedule = mocker.Mock(spec=ReportSchedule) + schedule.last_value = None + schedule.last_value_row_json = None + schedule.last_state = ReportState.ERROR + + state = BaseReportState(schedule, datetime.utcnow(), uuid4()) + state._execution_warnings.append("Slack v1 fallback is deprecated") + + mock_db = mocker.patch("superset.commands.report.execute.db") + mock_db.session.query.return_value.filter.return_value.first.return_value = None + marker_log = mocker.Mock() + mocker.patch( + "superset.commands.report.execute.ReportExecutionLog", + return_value=marker_log, + ) + + state.create_log(error_message=REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER) + + assert marker_log.error_message == REPORT_SCHEDULE_ERROR_NOTIFICATION_MARKER + mock_db.session.add.assert_called_once_with(marker_log) + + +def test_create_log_excludes_warnings_from_secondary_error( + mocker: MockerFixture, +) -> None: + """A failed error notification does not duplicate the primary warning.""" + schedule = mocker.Mock(spec=ReportSchedule) + schedule.last_value = None + schedule.last_value_row_json = None + schedule.last_state = ReportState.ERROR + + state = BaseReportState(schedule, datetime.utcnow(), uuid4()) + state._execution_warnings.append("Slack v1 fallback is deprecated") + + mock_db = mocker.patch("superset.commands.report.execute.db") + mock_db.session.query.return_value.filter.return_value.first.return_value = None + notification_failure_log = mocker.Mock() + mocker.patch( + "superset.commands.report.execute.ReportExecutionLog", + return_value=notification_failure_log, + ) + + state.create_log( + error_message="Error notification failed", + include_execution_warnings=False, + ) + + assert notification_failure_log.error_message == "Error notification failed" + mock_db.session.add.assert_called_once_with(notification_failure_log) + + def test_create_log_promotes_same_execution_working_row_without_duplicate( mocker: MockerFixture, ) -> None: @@ -2885,12 +4122,16 @@ def test_success_state_report_sends_and_logs_success( ReportSuccessState, schedule_type=ReportScheduleType.REPORT, ) + state._report_schedule.retry_attempt = 2 + state._report_schedule.retry_scheduled_dttm = datetime.utcnow() mock_send = mocker.patch.object(state, "send") mock_update = mocker.patch.object(state, "update_report_schedule_and_log") state.next() mock_send.assert_called_once() + assert state._report_schedule.retry_attempt == 0 + assert state._report_schedule.retry_scheduled_dttm is None # WORKING is set before send() (concurrency guard against duplicate sends), # then SUCCESS after. assert mock_update.call_args_list == [ diff --git a/tests/unit_tests/reports/notifications/slack_tests.py b/tests/unit_tests/reports/notifications/slack_tests.py index 8b976331765..7368c4606ac 100644 --- a/tests/unit_tests/reports/notifications/slack_tests.py +++ b/tests/unit_tests/reports/notifications/slack_tests.py @@ -16,8 +16,11 @@ # under the License. import uuid -from typing import Any -from unittest.mock import call, MagicMock, patch +from email.message import Message +from http.client import RemoteDisconnected +from typing import Any, TYPE_CHECKING +from unittest.mock import ANY, call, MagicMock, patch +from urllib.error import HTTPError, URLError import pandas as pd import pytest @@ -35,28 +38,40 @@ from slack_sdk.web.slack_response import SlackResponse from superset.reports.notifications.exceptions import ( NotificationAuthorizationException, + NotificationError, NotificationMalformedException, NotificationParamException, + NotificationTransientError, NotificationUnprocessableException, + SlackV1NotificationError, +) +from superset.reports.notifications.slack_transport import ( + _give_up_slack_api_retry, + call_slack_api, + send_to_slack_channels, + SlackRetryDeadlineError, ) from superset.reports.notifications.slackv2 import ( - _give_up_slack_api_retry, + _upload_file_data, + _upload_file_to_slack, SlackV2Notification, ) from superset.utils.core import HeaderDataType +from superset.utils.slack import SlackV2ProbeClientError, SlackV2ProbeError + +if TYPE_CHECKING: + from superset.reports.notifications.slack import SlackNotification @pytest.fixture(autouse=True) def _skip_backoff_sleep(): - """Make any @backoff.on_exception retries instant. + """Make phase-level Slack retry waits 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. + Classified API and transport failures can use exponential backoff, while + terminal writes avoid retries for ambiguous outcomes. Unit tests care about + attempts and error classification rather than wall-clock delays. """ - with patch("backoff._sync.time.sleep"): + with patch("time.sleep"): yield @@ -74,10 +89,34 @@ def mock_header_data() -> HeaderDataType: } -def test_get_channel_with_multi_recipients(mock_header_data) -> None: +def _make_v1_notification( + mock_header_data: HeaderDataType, + *, + target: str, + **content_overrides: Any, +) -> "SlackNotification": + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + from superset.reports.notifications.slack import SlackNotification + + content_values: dict[str, Any] = { + "name": "test alert", + "header_data": mock_header_data, + "description": "desc", + } + content_values.update(content_overrides) + return SlackNotification( + recipient=ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=f'{{"target": "{target}"}}', + ), + content=NotificationContent(**content_values), + ) + + +def test_get_channels_with_multi_recipients(mock_header_data) -> None: """ - Test the _get_channel function to ensure it will return a string - with recipients separated by commas without interstitial spacing + Test _get_channels returns normalized recipients without duplicates. """ from superset.reports.models import ReportRecipients, ReportRecipientType from superset.reports.notifications.base import NotificationContent @@ -98,16 +137,69 @@ def test_get_channel_with_multi_recipients(mock_header_data) -> None: slack_notification = SlackNotification( recipient=ReportRecipients( type=ReportRecipientType.SLACK, - recipient_config_json='{"target": "some_channel; second_channel, third_channel"}', # noqa: E501 + recipient_config_json='{"target": "some_channel; second_channel, third_channel, some_channel"}', # noqa: E501 ), content=content, ) - result = slack_notification._get_channel() + result = slack_notification._get_channels() - assert result == "some_channel,second_channel,third_channel" + assert result == ["some_channel", "second_channel", "third_channel"] - # Test if the recipient configuration JSON is valid when using a SlackV2 recipient type # noqa: E501 + +@pytest.mark.parametrize( + "recipient_config_json", + ["{not-json", '{"target": ["private-channel"]}'], + ids=["malformed-json", "non-string-target"], +) +def test_get_channels_rejects_invalid_recipient_config( + mock_header_data: HeaderDataType, + recipient_config_json: str, +) -> None: + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + from superset.reports.notifications.slack import SlackNotification + + notification = SlackNotification( + recipient=ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=recipient_config_json, + ), + content=NotificationContent( + name="test alert", + header_data=mock_header_data, + ), + ) + + with pytest.raises(NotificationParamException, match="No recipients"): + notification._get_channels() + + +@pytest.mark.parametrize( + "recipient_config_json", + ["{not-json", '{"target": ["C12345"]}'], + ids=["malformed-json", "non-string-target"], +) +def test_slackv2_get_channels_rejects_invalid_recipient_config( + mock_header_data: HeaderDataType, + recipient_config_json: str, +) -> None: + from superset.reports.models import ReportRecipients, ReportRecipientType + from superset.reports.notifications.base import NotificationContent + + notification = SlackV2Notification( + recipient=ReportRecipients( + type=ReportRecipientType.SLACKV2, + recipient_config_json=recipient_config_json, + ), + content=NotificationContent( + name="test alert", + header_data=mock_header_data, + ), + ) + + with pytest.raises(NotificationParamException, match="No recipients"): + notification._get_channels() def test_valid_recipient_config_json_slackv2(mock_header_data) -> None: @@ -145,93 +237,31 @@ def test_valid_recipient_config_json_slackv2(mock_header_data) -> None: # Ensure _get_inline_files function returns the correct tuple when content has screenshots # noqa: E501 -def test_get_inline_files_with_screenshots(mock_header_data) -> None: - """ - Test the _get_inline_files function to ensure it will return the correct tuple - when content has screenshots - """ - from superset.reports.models import ReportRecipients, ReportRecipientType +@pytest.mark.parametrize( + "content_overrides,expected", + [ + ({"screenshots": [b"screenshot"]}, True), + ({"csv": b"csv_content"}, True), + ({"xlsx": b"xlsx_content"}, True), + ({"pdf": b"pdf_content"}, True), + ({}, False), + ], + ids=["screenshots", "csv", "xlsx", "pdf", "none"], +) +def test_notification_content_has_attachments( + mock_header_data: HeaderDataType, + content_overrides: dict[str, Any], + expected: bool, +) -> None: from superset.reports.notifications.base import NotificationContent - from superset.reports.notifications.slack import SlackNotification content = NotificationContent( name="test alert", header_data=mock_header_data, - embedded_data=pd.DataFrame( - { - "A": [1, 2, 3], - "B": [4, 5, 6], - "C": ["111", "222", '333'], - } - ), - description='

This is a test alert


', - screenshots=[b"screenshot1", b"screenshot2"], - ) - slack_notification = SlackNotification( - recipient=ReportRecipients( - type=ReportRecipientType.SLACK, - recipient_config_json='{"target": "some_channel"}', - ), - content=content, + **content_overrides, ) - result = slack_notification._get_inline_files() - - assert result == ("png", [b"screenshot1", b"screenshot2"]) - - -def test_get_inline_files_with_csv(mock_header_data: HeaderDataType) -> None: - """ - Test the _get_inline_files function to ensure it returns the correct tuple - when content has a CSV attachment - """ - from superset.reports.models import ReportRecipients, ReportRecipientType - from superset.reports.notifications.base import NotificationContent - from superset.reports.notifications.slack import SlackNotification - - content = NotificationContent( - name="test alert", - header_data=mock_header_data, - csv=b"csv_content", - ) - slack_notification = SlackNotification( - recipient=ReportRecipients( - type=ReportRecipientType.SLACK, - recipient_config_json='{"target": "some_channel"}', - ), - content=content, - ) - - result = slack_notification._get_inline_files() - - assert result == ("csv", [b"csv_content"]) - - -def test_get_inline_files_with_xlsx(mock_header_data: HeaderDataType) -> None: - """ - Test the _get_inline_files function to ensure it returns the correct tuple - when content has an Excel attachment - """ - from superset.reports.models import ReportRecipients, ReportRecipientType - from superset.reports.notifications.base import NotificationContent - from superset.reports.notifications.slack import SlackNotification - - content = NotificationContent( - name="test alert", - header_data=mock_header_data, - xlsx=b"xlsx_content", - ) - slack_notification = SlackNotification( - recipient=ReportRecipients( - type=ReportRecipientType.SLACK, - recipient_config_json='{"target": "some_channel"}', - ), - content=content, - ) - - result = slack_notification._get_inline_files() - - assert result == ("xlsx", [b"xlsx_content"]) + assert content.has_attachments is expected def test_get_inline_files_with_xlsx_slackv2(mock_header_data: HeaderDataType) -> None: @@ -260,38 +290,295 @@ def test_get_inline_files_with_xlsx_slackv2(mock_header_data: HeaderDataType) -> # Ensure _get_inline_files function returns None when content has no screenshots or csv # noqa: E501 -def test_get_inline_files_with_no_screenshots_or_csv(mock_header_data) -> None: - """ - Test the _get_inline_files function to ensure it will return None - when content has no screenshots or csv - """ - from superset.reports.models import ReportRecipients, ReportRecipientType - from superset.reports.notifications.base import NotificationContent - from superset.reports.notifications.slack import SlackNotification +@patch( + "superset.reports.notifications.slack.feature_flag_manager.is_feature_enabled", + return_value=False, +) +@patch("superset.reports.notifications.slack.g") +@patch("superset.reports.notifications.slack.should_use_v2_api", return_value=False) +@patch("superset.reports.notifications.slack.get_slack_client") +def test_v1_send_without_channels_raises( + slack_client_mock: MagicMock, + should_use_v2_api_mock: MagicMock, + flask_global_mock: MagicMock, + feature_flag_mock: MagicMock, + mock_header_data: HeaderDataType, +) -> None: + flask_global_mock.logs_context = {} + notification = _make_v1_notification(mock_header_data, target="") - content = NotificationContent( - name="test alert", - header_data=mock_header_data, - embedded_data=pd.DataFrame( - { - "A": [1, 2, 3], - "B": [4, 5, 6], - "C": ["111", "222", '333'], - } - ), - description='

This is a test alert


', - ) - slack_notification = SlackNotification( - recipient=ReportRecipients( - type=ReportRecipientType.SLACK, - recipient_config_json='{"target": "some_channel"}', - ), - content=content, + with pytest.raises(NotificationParamException, match="No recipients"): + notification.send() + + should_use_v2_api_mock.assert_called_once_with(raise_on_error=False) + feature_flag_mock.assert_called_once_with("ALERT_REPORT_SLACK_V2") + slack_client_mock.return_value.chat_postMessage.assert_not_called() + + +@patch("superset.reports.notifications.slack.g") +@patch("superset.reports.notifications.slack.should_use_v2_api") +@patch("superset.reports.notifications.slack.get_slack_client") +def test_send_legacy_text_skips_v2_probe( + slack_client_mock: MagicMock, + should_use_v2_api_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data: HeaderDataType, +) -> None: + flask_global_mock.logs_context = {} + notification = _make_v1_notification( + mock_header_data, + target="private-channel", ) - result = slack_notification._get_inline_files() + notification.send_legacy_text() - assert result == (None, []) + should_use_v2_api_mock.assert_not_called() + slack_client_mock.assert_called_once_with(for_delivery=True) + slack_client_mock.return_value.chat_postMessage.assert_called_once_with( + channel="private-channel", + text=ANY, + ) + + +@patch("superset.reports.notifications.slack.should_use_v2_api", return_value=True) +def test_v1_upgrade_handoff_does_not_record_terminal_send_metric( + should_use_v2_api_mock: MagicMock, + mock_header_data: HeaderDataType, +) -> None: + """The v1-to-v2 handoff is routing, not a terminal Slack delivery.""" + notification = _make_v1_notification( + mock_header_data, + target="private-channel", + ) + + with ( + patch("superset.extensions.stats_logger_manager.instance.gauge") as statsd_mock, + pytest.raises(SlackV1NotificationError), + ): + notification.send() + + should_use_v2_api_mock.assert_called_once_with(raise_on_error=False) + statsd_mock.assert_not_called() + + +@patch("superset.reports.notifications.slack.get_slack_client") +def test_send_legacy_text_rejects_attachments( + slack_client_mock: MagicMock, + mock_header_data: HeaderDataType, +) -> None: + notification = _make_v1_notification( + mock_header_data, + target="private-channel", + screenshots=[b"screenshot"], + ) + + with pytest.raises( + NotificationParamException, + match="Slack v1 file uploads are no longer supported", + ): + notification.send_legacy_text() + + slack_client_mock.assert_not_called() + + +@patch( + "superset.reports.notifications.slack.feature_flag_manager.is_feature_enabled", + return_value=False, +) +@patch("superset.reports.notifications.slack.g") +@patch("superset.reports.notifications.slack.should_use_v2_api", return_value=False) +@patch("superset.reports.notifications.slack.get_slack_client") +def test_v1_send_rejects_files_when_v2_probe_is_unavailable( + slack_client_mock: MagicMock, + should_use_v2_api_mock: MagicMock, + flask_global_mock: MagicMock, + feature_flag_mock: MagicMock, + mock_header_data: HeaderDataType, +) -> None: + flask_global_mock.logs_context = {} + notification = _make_v1_notification( + mock_header_data, + target="private-channel", + screenshots=[b"screenshot"], + ) + + with pytest.raises( + NotificationParamException, + match="Slack v1 file uploads are no longer supported", + ): + notification.send() + + should_use_v2_api_mock.assert_called_once_with(raise_on_error=True) + feature_flag_mock.assert_called_once_with("ALERT_REPORT_SLACK_V2") + slack_client_mock.return_value.files_upload.assert_not_called() + + +@pytest.mark.parametrize( + ("probe_error", "metric"), + [ + ( + SlackV2ProbeError( + "Slack v2 availability probe failed: service_unavailable" + ), + "reports.slack.send.error", + ), + ( + SlackV2ProbeClientError("Slack v2 availability probe failed: invalid_auth"), + "reports.slack.send.warning", + ), + ], + ids=["system", "client"], +) +@patch("superset.reports.notifications.slack.g") +@patch("superset.reports.notifications.slack.should_use_v2_api") +@patch("superset.reports.notifications.slack.get_slack_client") +def test_v1_file_send_records_v2_probe_failure_metric( + slack_client_mock: MagicMock, + should_use_v2_api_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data: HeaderDataType, + probe_error: SlackV2ProbeError, + metric: str, +) -> None: + flask_global_mock.logs_context = {} + should_use_v2_api_mock.side_effect = probe_error + notification = _make_v1_notification( + mock_header_data, + target="private-channel", + screenshots=[b"screenshot"], + ) + + with ( + patch("superset.extensions.stats_logger_manager.instance.gauge") as statsd_mock, + pytest.raises(type(probe_error)), + ): + notification.send() + + should_use_v2_api_mock.assert_called_once_with(raise_on_error=True) + slack_client_mock.assert_not_called() + statsd_mock.assert_called_once_with(metric, 1) + + +@patch( + "superset.reports.notifications.slack.feature_flag_manager.is_feature_enabled", + return_value=False, +) +@patch("superset.reports.notifications.slack.g") +@patch("superset.reports.notifications.slack.should_use_v2_api", return_value=False) +@patch("superset.reports.notifications.slack.get_slack_client") +def test_v1_send_retries_only_the_failed_channel( + slack_client_mock: MagicMock, + should_use_v2_api_mock: MagicMock, + flask_global_mock: MagicMock, + feature_flag_mock: MagicMock, + mock_header_data: HeaderDataType, +) -> 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 == "private-b" and failed_attempts < 2: + failed_attempts += 1 + response = _make_slack_response( + 429, + {"ok": False, "error": "ratelimited"}, + headers={"Retry-After": ["0"]}, + ) + raise SlackApiError( + message="rate limited", + response=response, + ) + return {"ok": True} + + slack_client_mock.return_value.chat_postMessage.side_effect = chat_side_effect + notification = _make_v1_notification( + mock_header_data, + target="private-a,private-b", + ) + + notification.send() + + should_use_v2_api_mock.assert_called_once_with(raise_on_error=False) + feature_flag_mock.assert_called_once_with("ALERT_REPORT_SLACK_V2") + assert [ + slack_call.kwargs["channel"] + for slack_call in slack_client_mock.return_value.chat_postMessage.call_args_list + ] == ["private-a", "private-b", "private-b", "private-b"] + + +@patch( + "superset.reports.notifications.slack.feature_flag_manager.is_feature_enabled", + return_value=False, +) +@patch("superset.reports.notifications.slack.g") +@patch("superset.reports.notifications.slack.should_use_v2_api", return_value=False) +@patch("superset.reports.notifications.slack.get_slack_client") +def test_v1_send_reports_failed_channel_and_continues_later_channels( + slack_client_mock: MagicMock, + should_use_v2_api_mock: MagicMock, + flask_global_mock: MagicMock, + feature_flag_mock: MagicMock, + mock_header_data: HeaderDataType, +) -> None: + flask_global_mock.logs_context = {} + + def chat_side_effect(channel: str, text: str) -> dict[str, bool]: + if channel == "private-b": + raise SlackApiError( + message="channel not found", + response={"ok": False, "error": "channel_not_found"}, + ) + return {"ok": True} + + slack_client_mock.return_value.chat_postMessage.side_effect = chat_side_effect + notification = _make_v1_notification( + mock_header_data, + target="private-a,private-b,private-c", + ) + + with pytest.raises( + NotificationUnprocessableException, + match="private-b", + ): + notification.send() + + should_use_v2_api_mock.assert_called_once_with(raise_on_error=False) + feature_flag_mock.assert_called_once_with("ALERT_REPORT_SLACK_V2") + assert [ + slack_call.kwargs["channel"] + for slack_call in slack_client_mock.return_value.chat_postMessage.call_args_list + ] == ["private-a", "private-b", "private-c"] + + +@patch( + "superset.reports.notifications.slack.feature_flag_manager.is_feature_enabled", + return_value=False, +) +@patch("superset.reports.notifications.slack.g") +@patch("superset.reports.notifications.slack.should_use_v2_api", return_value=False) +@patch("superset.reports.notifications.slack.get_slack_client") +def test_v1_send_deduplicates_channels( + slack_client_mock: MagicMock, + should_use_v2_api_mock: MagicMock, + flask_global_mock: MagicMock, + feature_flag_mock: MagicMock, + mock_header_data: HeaderDataType, +) -> None: + flask_global_mock.logs_context = {} + notification = _make_v1_notification( + mock_header_data, + target="private-a, private-a", + ) + + notification.send() + + should_use_v2_api_mock.assert_called_once_with(raise_on_error=False) + feature_flag_mock.assert_called_once_with("ALERT_REPORT_SLACK_V2") + slack_client_mock.return_value.chat_postMessage.assert_called_once_with( + channel="private-a", + text=ANY, + ) @patch("superset.reports.notifications.slackv2.g") @@ -332,6 +619,7 @@ def test_send_slackv2( content=content, ) notification.send() + slack_client_mock.assert_called_once_with(for_delivery=True) logger_mock.info.assert_called_with( "Report sent to slack", extra={"execution_id": execution_id} ) @@ -364,6 +652,7 @@ def test_send_slack( logger_mock: MagicMock, flask_global_mock: MagicMock, mock_header_data, + mocker, ) -> None: # `superset.models.helpers`, a dependency of following imports, # requires app context @@ -373,6 +662,10 @@ def test_send_slack( execution_id = uuid.uuid4() flask_global_mock.logs_context = {"execution_id": execution_id} + mocker.patch( + "superset.reports.notifications.slack.feature_flag_manager.is_feature_enabled", + return_value=False, + ) slack_client_mock.return_value.chat_postMessage.return_value = {"ok": True} slack_client_mock_util.return_value.conversations_list.side_effect = SlackApiError( "scope not found", "error" @@ -652,10 +945,8 @@ def test_slack_mixin_truncated_body_omits_cta_when_include_cta_is_false( # 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. +# tests below cover the three-phase v2 upload across screenshots/CSV/PDF, +# multi-channel fan-out, exception mapping, backoff, statsd, and logs propagation. # --------------------------------------------------------------------------- @@ -685,29 +976,438 @@ def _make_content(mock_header_data, **overrides): return NotificationContent(**defaults) +def _configure_v2_upload_client(client: MagicMock) -> MagicMock: + """Configure an SDK-shaped client for the three-phase upload flow.""" + client.timeout = 30 + client.proxy = None + client.ssl = None + + def create_upload_url(**kwargs: object) -> dict[str, str]: + index = client.files_getUploadURLExternal.call_count + return { + "file_id": f"F{index}", + "upload_url": f"https://files.slack.com/upload/{index}", + } + + client.files_getUploadURLExternal.side_effect = create_upload_url + client.files_completeUploadExternal.return_value = {"files": [{"id": "F1"}]} + return client + + +@pytest.fixture +def raw_upload_mock(mocker) -> MagicMock: + """Stub Slack's issued upload URL without depending on SDK internals.""" + return mocker.patch( + "superset.reports.notifications.slackv2._upload_file_data", + return_value=(200, "ok"), + ) + + +def test_raw_upload_uses_stable_http_api_with_integer_timeout(mocker) -> None: + response = MagicMock(status=200) + response.headers.get_content_charset.return_value = "utf-8" + response.read.return_value = b"ok" + urlopen_mock = mocker.patch( + "superset.reports.notifications.slackv2.urlopen", + return_value=response, + ) + + assert _upload_file_data( + url="https://files.slack.com/upload/1", + data=b"report", + timeout=7, + proxy=None, + ssl=None, + ) == (200, "ok") + + request = urlopen_mock.call_args.args[0] + assert request.full_url == "https://files.slack.com/upload/1" + assert request.data == b"report" + assert request.method == "POST" + assert urlopen_mock.call_args.kwargs["timeout"] == 7 + assert isinstance(urlopen_mock.call_args.kwargs["timeout"], int) + + @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( +def test_v2_send_with_single_screenshot_uses_three_phase_upload( slack_client_mock: MagicMock, flask_global_mock: MagicMock, mock_header_data, + raw_upload_mock: MagicMock, ) -> None: flask_global_mock.logs_context = {"execution_id": uuid.uuid4()} + client = _configure_v2_upload_client(slack_client_mock.return_value) 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"] + client.files_getUploadURLExternal.assert_called_once_with( + filename="test alert.png", + length=len(b"screenshot-bytes"), + ) + assert raw_upload_mock.call_args.kwargs["data"] == b"screenshot-bytes" + completion_kwargs = client.files_completeUploadExternal.call_args.kwargs + assert completion_kwargs["channel_id"] == "C12345" + assert completion_kwargs["files"] == [{"id": "F1", "title": "test alert"}] + assert "test alert" in completion_kwargs["initial_comment"] + client.files_upload_v2.assert_not_called() # chat_postMessage should NOT be called when files are present - slack_client_mock.return_value.chat_postMessage.assert_not_called() + client.chat_postMessage.assert_not_called() + + +@pytest.mark.parametrize( + ("status_code", "expected_exception", "expected_calls"), + [ + (413, NotificationUnprocessableException, 1), + (429, NotificationTransientError, 3), + (504, NotificationTransientError, 5), + ], +) +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_file_upload_classifies_raw_http_status( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + status_code: int, + expected_exception: type[Exception], + expected_calls: int, + mock_header_data: HeaderDataType, + raw_upload_mock: MagicMock, +) -> None: + """Classify raw-upload HTTP failures and honor exposed Retry-After headers.""" + flask_global_mock.logs_context = {} + client = _configure_v2_upload_client(slack_client_mock.return_value) + headers = Message() + if status_code == 429: + headers["Retry-After"] = "0" + raw_upload_mock.side_effect = HTTPError( + "https://files.slack.com/upload", + status_code, + "upload failed", + headers, + None, + ) + notification = _make_v2_notification( + _make_content(mock_header_data, screenshots=[b"screenshot-bytes"]), + target="C12345", + ) + + with pytest.raises(expected_exception, match="C12345"): + notification.send() + + assert raw_upload_mock.call_count == expected_calls + client.files_getUploadURLExternal.assert_called_once() + client.files_completeUploadExternal.assert_not_called() + + +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_raw_upload_result_429_without_headers_does_not_retry( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data: HeaderDataType, + raw_upload_mock: MagicMock, +) -> None: + """A raw result without headers cannot safely synthesize a retry delay.""" + flask_global_mock.logs_context = {} + client = _configure_v2_upload_client(slack_client_mock.return_value) + raw_upload_mock.return_value = (429, "rate limited") + notification = _make_v2_notification( + _make_content(mock_header_data, screenshots=[b"screenshot-bytes"]), + target="C12345", + ) + + with pytest.raises(NotificationTransientError, match="C12345"): + notification.send() + + raw_upload_mock.assert_called_once() + client.files_getUploadURLExternal.assert_called_once() + client.files_completeUploadExternal.assert_not_called() + + +def test_raw_http_rate_limit_respects_zero_retry_config(mocker) -> None: + headers = Message() + headers["Retry-After"] = "0" + error = HTTPError( + "https://files.slack.com/upload", + 429, + "upload rate limited", + headers, + None, + ) + method = MagicMock(side_effect=error) + mocker.patch.dict( + "superset.reports.notifications.slack_transport.app.config", + {"SLACK_API_RATE_LIMIT_RETRY_COUNT": 0}, + ) + + with pytest.raises(HTTPError): + call_slack_api(method) + + method.assert_called_once_with() + + +def test_raw_http_rate_limit_respects_shared_deadline(mocker) -> None: + headers = Message() + headers["Retry-After"] = "120" + error = HTTPError( + "https://files.slack.com/upload", + 429, + "upload rate limited", + headers, + None, + ) + method = MagicMock(side_effect=error) + clock = [0.0] + sleep = mocker.patch("superset.reports.notifications.slack_transport.time.sleep") + sleep.side_effect = lambda duration: clock.__setitem__(0, clock[0] + duration) + mocker.patch( + "superset.reports.notifications.slack_transport.time.monotonic", + side_effect=lambda: clock[0], + ) + mocker.patch.dict( + "superset.reports.notifications.slack_transport.app.config", + {"SLACK_API_RATE_LIMIT_RETRY_COUNT": 2}, + ) + + with pytest.raises(SlackRetryDeadlineError, match="deadline exceeded"): + call_slack_api(method, retry_deadline=150.0) + + assert method.call_count == 2 + sleep.assert_called_once_with(120.0) + + +@pytest.mark.parametrize("retry_after", ["NaN", "inf", "-inf", ""]) +def test_malformed_retry_after_preserves_channel_isolation( + retry_after: str, +) -> None: + response = _make_slack_response( + 429, + {"ok": False, "error": "ratelimited"}, + headers={"Retry-After": [retry_after]}, + ) + methods = { + "C1": MagicMock( + side_effect=SlackApiError(message="rate limited", response=response) + ), + "C2": MagicMock(return_value={"ok": True}), + } + + def send(channel: str, retry_deadline: float) -> None: + call_slack_api(methods[channel], retry_deadline=retry_deadline) + + with pytest.raises(NotificationTransientError, match="C1"): + send_to_slack_channels(["C1", "C2"], send) + + methods["C1"].assert_called_once_with() + methods["C2"].assert_called_once_with() + + +@pytest.mark.parametrize( + ("status_code", "expected_calls", "expected_exception"), + [ + (429, 2, None), + (503, 1, NotificationTransientError), + ], +) +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_completion_retry_does_not_replay_prior_upload_phases( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + status_code: int, + expected_calls: int, + expected_exception: type[Exception] | None, + mock_header_data: HeaderDataType, + raw_upload_mock: MagicMock, +) -> None: + flask_global_mock.logs_context = {} + client = _configure_v2_upload_client(slack_client_mock.return_value) + response = _make_slack_response( + status_code, + {"ok": False, "error": "ratelimited" if status_code == 429 else "fatal_error"}, + ) + if status_code == 429: + response.headers["Retry-After"] = "0" + client.files_completeUploadExternal.side_effect = [ + SlackApiError(message="completion failed", response=response), + {"files": [{"id": "F1"}]}, + ] + notification = _make_v2_notification( + _make_content(mock_header_data, screenshots=[b"screenshot"]), + ) + + if expected_exception: + with pytest.raises(expected_exception, match="C12345"): + notification.send() + else: + notification.send() + + client.files_getUploadURLExternal.assert_called_once() + raw_upload_mock.assert_called_once() + assert client.files_completeUploadExternal.call_count == expected_calls + + +@pytest.mark.parametrize( + "invalid_response", + [ + object(), + {"file_id": None, "upload_url": None}, + {"file_id": "", "upload_url": ""}, + ], + ids=["non-mapping", "missing-values", "empty-values"], +) +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_upload_aggregates_missing_metadata_per_channel( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + invalid_response: object, + mock_header_data: HeaderDataType, + raw_upload_mock: MagicMock, +) -> None: + flask_global_mock.logs_context = {} + client = _configure_v2_upload_client(slack_client_mock.return_value) + client.files_getUploadURLExternal.side_effect = [ + invalid_response, + {"file_id": "F2", "upload_url": "https://files.slack.com/upload/2"}, + ] + notification = _make_v2_notification( + _make_content(mock_header_data, screenshots=[b"screenshot"]), + target="C12345,C67890", + ) + + with pytest.raises(NotificationTransientError, match="C12345"): + notification.send() + + assert client.files_getUploadURLExternal.call_count == 2 + raw_upload_mock.assert_called_once() + client.files_completeUploadExternal.assert_called_once() + + +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_upload_url_creation_retries_transient_transport_failure( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data: HeaderDataType, + raw_upload_mock: MagicMock, +) -> None: + flask_global_mock.logs_context = {} + client = _configure_v2_upload_client(slack_client_mock.return_value) + client.files_getUploadURLExternal.side_effect = [ + URLError("connection reset"), + { + "file_id": "F1", + "upload_url": "https://files.slack.com/upload/1", + }, + ] + notification = _make_v2_notification( + _make_content(mock_header_data, screenshots=[b"screenshot"]), + ) + + notification.send() + + assert client.files_getUploadURLExternal.call_count == 2 + raw_upload_mock.assert_called_once() + client.files_completeUploadExternal.assert_called_once() + + +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_raw_upload_retries_transient_transport_failure( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data: HeaderDataType, + raw_upload_mock: MagicMock, +) -> None: + """The non-terminal raw upload can safely retry a transient socket error.""" + flask_global_mock.logs_context = {} + client = _configure_v2_upload_client(slack_client_mock.return_value) + raw_upload_mock.side_effect = [ + URLError("connection reset"), + (200, "ok"), + ] + notification = _make_v2_notification( + _make_content(mock_header_data, screenshots=[b"screenshot"]), + ) + + notification.send() + + assert raw_upload_mock.call_count == 2 + client.files_getUploadURLExternal.assert_called_once() + client.files_completeUploadExternal.assert_called_once() + + +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_upload_does_not_start_completion_after_deadline( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data: HeaderDataType, + mocker, + raw_upload_mock: MagicMock, +) -> None: + flask_global_mock.logs_context = {} + client = _configure_v2_upload_client(slack_client_mock.return_value) + clock = [0.0] + mocker.patch( + "superset.reports.notifications.slack_transport.time.monotonic", + side_effect=lambda: clock[0], + ) + + def cross_deadline(**kwargs: object) -> tuple[int, str]: + clock[0] = 151.0 + return 200, "ok" + + raw_upload_mock.side_effect = cross_deadline + notification = _make_v2_notification( + _make_content(mock_header_data, screenshots=[b"screenshot"]), + ) + + with pytest.raises(NotificationTransientError, match="deadline exceeded"): + notification.send() + + client.files_getUploadURLExternal.assert_called_once() + raw_upload_mock.assert_called_once() + client.files_completeUploadExternal.assert_not_called() + + +def test_raw_upload_timeout_is_clamped_to_remaining_deadline( + mocker, + raw_upload_mock: MagicMock, +) -> None: + client = _configure_v2_upload_client(MagicMock()) + client.timeout = 300 + clock = [0.0] + + def return_upload_metadata(**kwargs: object) -> dict[str, str]: + clock[0] = 140.0 + return { + "file_id": "F1", + "upload_url": "https://files.slack.com/upload/F1", + } + + client.files_getUploadURLExternal.side_effect = return_upload_metadata + mocker.patch( + "superset.reports.notifications.slack_transport.time.monotonic", + side_effect=lambda: clock[0], + ) + + _upload_file_to_slack( + client, + channel="C12345", + file=b"screenshot", + initial_comment="report", + title="Report", + filename="report.png", + retry_deadline=150.0, + ) + + assert raw_upload_mock.call_args.kwargs["timeout"] == 10 + assert isinstance(raw_upload_mock.call_args.kwargs["timeout"], int) @patch("superset.reports.notifications.slackv2.g") @@ -716,8 +1416,10 @@ def test_v2_send_with_multiple_screenshots_uploads_each( slack_client_mock: MagicMock, flask_global_mock: MagicMock, mock_header_data, + raw_upload_mock: MagicMock, ) -> None: flask_global_mock.logs_context = {} + client = _configure_v2_upload_client(slack_client_mock.return_value) content = _make_content( mock_header_data, screenshots=[b"shot-1", b"shot-2", b"shot-3"] ) @@ -725,14 +1427,13 @@ def test_v2_send_with_multiple_screenshots_uploads_each( 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 raw_upload_mock.call_count == 3 + uploaded_files = [c.kwargs["data"] for c in raw_upload_mock.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" + assert [ + c.kwargs["channel_id"] + for c in client.files_completeUploadExternal.call_args_list + ] == ["C12345", "C12345", "C12345"] @patch("superset.reports.notifications.slackv2.g") @@ -741,18 +1442,21 @@ def test_v2_send_with_csv_calls_files_upload_v2( slack_client_mock: MagicMock, flask_global_mock: MagicMock, mock_header_data, + raw_upload_mock: MagicMock, ) -> None: flask_global_mock.logs_context = {} + client = _configure_v2_upload_client(slack_client_mock.return_value) 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" + raw_upload_mock.assert_called_once() + assert raw_upload_mock.call_args.kwargs["data"] == b"col1,col2\n1,2\n" + client.files_getUploadURLExternal.assert_called_once_with( + filename="test alert.csv", + length=len(b"col1,col2\n1,2\n"), + ) @patch("superset.reports.notifications.slackv2.g") @@ -761,18 +1465,21 @@ def test_v2_send_with_pdf_calls_files_upload_v2( slack_client_mock: MagicMock, flask_global_mock: MagicMock, mock_header_data, + raw_upload_mock: MagicMock, ) -> None: flask_global_mock.logs_context = {} + client = _configure_v2_upload_client(slack_client_mock.return_value) 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" + raw_upload_mock.assert_called_once() + assert raw_upload_mock.call_args.kwargs["data"] == b"%PDF-1.4..." + client.files_getUploadURLExternal.assert_called_once_with( + filename="test alert.pdf", + length=len(b"%PDF-1.4..."), + ) @patch("superset.reports.notifications.slackv2.g") @@ -781,17 +1488,25 @@ def test_v2_send_to_multiple_channels_uploads_per_channel( slack_client_mock: MagicMock, flask_global_mock: MagicMock, mock_header_data, + raw_upload_mock: MagicMock, ) -> None: flask_global_mock.logs_context = {} + client = _configure_v2_upload_client(slack_client_mock.return_value) 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 raw_upload_mock.call_count == 6 + seen = { + (completion.kwargs["channel_id"], upload_call.kwargs["data"]) + for completion, upload_call in zip( + client.files_completeUploadExternal.call_args_list, + raw_upload_mock.call_args_list, + strict=True, + ) + } assert seen == { ("C12345", b"shot-1"), ("C12345", b"shot-2"), @@ -802,6 +1517,27 @@ def test_v2_send_to_multiple_channels_uploads_per_channel( } +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_send_deduplicates_channels( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data: HeaderDataType, +) -> None: + flask_global_mock.logs_context = {} + notification = _make_v2_notification( + _make_content(mock_header_data), + target="C12345,C12345", + ) + + notification.send() + + slack_client_mock.return_value.chat_postMessage.assert_called_once_with( + channel="C12345", + text=ANY, + ) + + @patch("superset.reports.notifications.slackv2.g") @patch("superset.reports.notifications.slackv2.get_slack_client") def test_v2_send_text_only_uses_chat_post_message( @@ -880,7 +1616,7 @@ def test_v2_inline_files_precedence(mock_header_data) -> None: ), ( lambda: SlackClientNotConnectedError("offline"), - NotificationUnprocessableException, + NotificationError, ), ( # Fallback: any other SlackClientError becomes Unprocessable. @@ -910,54 +1646,44 @@ 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_retries_on_transient_slack_api_error( +def test_v2_send_does_not_retry_ambiguous_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. - """ + """An outcome-ambiguous terminal write is attempted at most once.""" flask_global_mock.logs_context = {} slack_client_mock.return_value.chat_postMessage.side_effect = SlackApiError( - message="rate limited", response={"ok": False, "error": "ratelimited"} + message="service unavailable", + response={"ok": False, "error": "service_unavailable"}, ) content = _make_content(mock_header_data) notification = _make_v2_notification(content, target="C12345") - with pytest.raises(NotificationUnprocessableException): + with pytest.raises(NotificationError, match="C12345"): notification.send() - assert slack_client_mock.return_value.chat_postMessage.call_count == 5 + slack_client_mock.return_value.chat_postMessage.assert_called_once() @patch("superset.reports.notifications.slackv2.g") @patch("superset.reports.notifications.slackv2.get_slack_client") -def test_v2_send_retries_then_succeeds_on_transient_failure( +def test_v2_send_retries_then_succeeds_on_explicit_http_rate_limit( 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. - """ + """An explicit HTTP 429 safely retries a rejected terminal write.""" flask_global_mock.logs_context = {} + response = _make_slack_response( + 429, + {"ok": False, "error": "ratelimited"}, + headers={"Retry-After": ["0"]}, + ) 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"} - ), + SlackApiError(message="rate limited", response=response), + SlackApiError(message="rate limited", response=response), {"ok": True}, ] @@ -987,8 +1713,14 @@ def test_v2_send_retries_only_failed_channel( nonlocal failed_attempts if channel == "C67890" and failed_attempts < 2: failed_attempts += 1 + response = _make_slack_response( + 429, + {"ok": False, "error": "ratelimited"}, + headers={"Retry-After": ["0"]}, + ) raise SlackApiError( - message="rate limited", response={"ok": False, "error": "ratelimited"} + message="rate limited", + response=response, ) return {"ok": True} @@ -1005,6 +1737,49 @@ def test_v2_send_retries_only_failed_channel( ] == ["C12345", "C67890", "C67890", "C67890"] +@pytest.mark.parametrize("send_fails", [False, True], ids=["success", "failure"]) +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_text_send_clamps_timeout_to_shared_deadline( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + send_fails: bool, + mock_header_data: HeaderDataType, +) -> None: + flask_global_mock.logs_context = {} + client = slack_client_mock.return_value + client.timeout = 30 + + def assert_timeout(**kwargs: object) -> dict[str, bool]: + assert client.timeout == 10 + assert type(client.timeout) is int + if send_fails: + raise SlackApiError( + message="service unavailable", + response={"ok": False, "error": "service_unavailable"}, + ) + return {"ok": True} + + client.chat_postMessage.side_effect = assert_timeout + notification = _make_v2_notification( + _make_content(mock_header_data), + target="C12345", + ) + + clock = iter([0.0, 140.0]) + with patch( + "superset.reports.notifications.slack_transport.time.monotonic", + side_effect=lambda: next(clock, 140.0), + ): + if send_fails: + with pytest.raises(NotificationTransientError, match="C12345"): + notification.send() + else: + notification.send() + + assert client.timeout == 30 + + @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( @@ -1027,6 +1802,62 @@ def test_v2_send_does_not_retry_permanent_slack_api_error( assert slack_client_mock.return_value.chat_postMessage.call_count == 1 +@pytest.mark.parametrize( + ("content_overrides", "method_name", "target_keyword"), + [ + ({}, "chat_postMessage", "channel"), + ( + {"screenshots": [b"screenshot"]}, + "files_completeUploadExternal", + "channel_id", + ), + ], + ids=["text", "file"], +) +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_send_reports_failed_channel_and_continues_later_channels( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + content_overrides: dict[str, Any], + method_name: str, + target_keyword: str, + mock_header_data: HeaderDataType, + raw_upload_mock: MagicMock, +) -> None: + flask_global_mock.logs_context = {} + if content_overrides: + _configure_v2_upload_client(slack_client_mock.return_value) + raw_upload_mock.return_value = (200, "ok") + + def send_side_effect(**kwargs: object) -> dict[str, bool]: + channel = kwargs[target_keyword] + if channel == "C2": + raise SlackApiError( + message="channel not found", + response={"ok": False, "error": "channel_not_found"}, + ) + return {"ok": True} + + method = getattr(slack_client_mock.return_value, method_name) + method.side_effect = send_side_effect + notification = _make_v2_notification( + _make_content(mock_header_data, **content_overrides), + target="C1,C2,C3", + ) + + with pytest.raises(NotificationUnprocessableException, match="C2"): + notification.send() + + assert [ + slack_call.kwargs[target_keyword] for slack_call in method.call_args_list + ] == [ + "C1", + "C2", + "C3", + ] + + @patch("superset.reports.notifications.slackv2.g") @patch("superset.reports.notifications.slackv2.get_slack_client") def test_v2_send_does_not_retry_param_errors( @@ -1034,9 +1865,9 @@ def test_v2_send_does_not_retry_param_errors( 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 + """Non-transient configuration and authorization errors are not retried. + + A NotificationParamException-class failure (BotUserAccessError → 422) hits the API exactly once and surfaces immediately. """ flask_global_mock.logs_context = {} @@ -1053,12 +1884,16 @@ def test_v2_send_does_not_retry_param_errors( assert slack_client_mock.return_value.chat_postMessage.call_count == 1 -def _make_slack_response(status_code: int, data: dict[str, Any]) -> SlackResponse: +def _make_slack_response( + status_code: int, + data: dict[str, Any], + headers: dict[str, list[str]] | None = None, +) -> 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`` + ``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. @@ -1069,7 +1904,7 @@ def _make_slack_response(status_code: int, data: dict[str, Any]) -> SlackRespons api_url="https://slack.com/api/chat.postMessage", req_args={}, data=data, - headers={}, + headers=headers or {}, status_code=status_code, ) @@ -1082,8 +1917,6 @@ def _make_slack_response(status_code: int, data: dict[str, Any]) -> SlackRespons # 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( @@ -1096,6 +1929,400 @@ def test_give_up_slack_api_retry_retries_on_status_code( assert _give_up_slack_api_retry(ex) is False +def test_give_up_slack_api_retry_retries_http_408() -> None: + """Request timeout stays transient even though it is a 4xx response.""" + response = _make_slack_response( + 408, + {"ok": False, "error": "request_timeout"}, + ) + ex = SlackApiError(message="request timed out", response=response) + + assert _give_up_slack_api_retry(ex) is False + + +def test_give_up_slack_api_retry_stops_after_rate_limit_budget() -> None: + """Do not multiply call_slack_api's configured rate-limit retry budget.""" + response = _make_slack_response( + 429, + {"ok": False, "error": "ratelimited"}, + ) + ex = SlackApiError(message="rate limited", response=response) + + assert _give_up_slack_api_retry(ex) is True + + +def test_call_slack_api_rate_limit_retries_respect_shared_deadline(mocker) -> None: + """A Retry-After wait cannot begin an SDK write past the shared deadline.""" + response = _make_slack_response( + 429, + {"ok": False, "error": "ratelimited"}, + headers={"Retry-After": ["120"]}, + ) + method = MagicMock( + side_effect=SlackApiError(message="rate limited", response=response) + ) + clock = [0.0] + sleep = mocker.patch("superset.reports.notifications.slack_transport.time.sleep") + sleep.side_effect = lambda duration: clock.__setitem__(0, clock[0] + duration) + mocker.patch( + "superset.reports.notifications.slack_transport.time.monotonic", + side_effect=lambda: clock[0], + ) + mocker.patch.dict( + "superset.reports.notifications.slack_transport.app.config", + {"SLACK_API_RATE_LIMIT_RETRY_COUNT": 2}, + ) + + with pytest.raises(SlackRetryDeadlineError, match="deadline exceeded"): + call_slack_api(method, retry_deadline=150.0) + + assert method.call_count == 2 + sleep.assert_called_once_with(120.0) + + +def test_call_slack_api_rate_limit_budget_spans_server_error_retries(mocker) -> None: + """A server-error retry does not reset the configured HTTP 429 budget.""" + rate_limit_error = SlackApiError( + message="rate limited", + response=_make_slack_response( + 429, + {"ok": False, "error": "ratelimited"}, + headers={"Retry-After": ["0"]}, + ), + ) + server_error = SlackApiError( + message="service unavailable", + response=_make_slack_response(503, {"ok": False}), + ) + method = MagicMock(side_effect=[rate_limit_error, server_error, rate_limit_error]) + mocker.patch.dict( + "superset.reports.notifications.slack_transport.app.config", + {"SLACK_API_RATE_LIMIT_RETRY_COUNT": 1}, + ) + + with pytest.raises(SlackApiError): + call_slack_api(method) + + assert method.call_count == 3 + + +def test_call_slack_api_retries_ratelimited_code_without_http_429() -> None: + """A ratelimited response without HTTP 429 uses application backoff.""" + ex = SlackApiError( + message="rate limited", + response={"ok": False, "error": "ratelimited"}, + ) + method = MagicMock(side_effect=ex) + + with pytest.raises(SlackApiError): + call_slack_api(method) + + assert method.call_count == 5 + + +def test_send_to_slack_channels_shares_one_delivery_budget() -> None: + """A failed channel cannot multiply the schedule-wide delivery budget.""" + clock = [0.0] + starts: dict[str, float] = {} + deadlines: dict[str, float] = {} + error = SlackApiError( + message="service unavailable", + response={"ok": False, "error": "service_unavailable"}, + ) + first_method = MagicMock() + + def fail_first_channel() -> None: + clock[0] = 151.0 + raise error + + first_method.side_effect = fail_first_channel + methods = { + "private-a": first_method, + "private-b": MagicMock(return_value={"ok": True}), + "private-c": MagicMock(return_value={"ok": True}), + } + + def send(channel: str, retry_deadline: float) -> None: + starts[channel] = clock[0] + deadlines[channel] = retry_deadline + call_slack_api(methods[channel], retry_deadline=retry_deadline) + + with ( + patch( + "superset.reports.notifications.slack_transport.time.monotonic", + side_effect=lambda: clock[0], + ), + pytest.raises(NotificationTransientError, match="private-a"), + ): + send_to_slack_channels(["private-a", "private-b", "private-c"], send) + + methods["private-a"].assert_called_once_with() + methods["private-b"].assert_not_called() + methods["private-c"].assert_not_called() + assert deadlines == pytest.approx( + { + "private-a": 150.0, + "private-b": 150.0, + "private-c": 150.0, + } + ) + assert starts == {"private-a": 0.0, "private-b": 151.0, "private-c": 151.0} + + +def test_send_to_slack_channels_uses_one_deadline_for_high_fanout(mocker) -> None: + """High fanout cannot multiply the configured delivery budget.""" + mocker.patch.dict( + "superset.reports.notifications.slack_transport.app.config", + { + "SLACK_SEND_RETRY_MAX_TIME": 150, + "SLACK_API_TIMEOUT": 30, + }, + ) + monotonic = mocker.patch( + "superset.reports.notifications.slack_transport.time.monotonic", + side_effect=[10.0, *range(20, 120, 10)], + ) + deadlines: list[float] = [] + + def send(_channel: str, retry_deadline: float) -> None: + deadlines.append(retry_deadline) + + send_to_slack_channels([f"C{index}" for index in range(10)], send) + + assert deadlines == [160.0] * 10 + monotonic.assert_called_once_with() + + +def test_send_to_slack_channels_clamps_to_report_deadline(mocker) -> None: + """The report task deadline wins when it is shorter than Slack's budget.""" + mocker.patch.dict( + "superset.reports.notifications.slack_transport.app.config", + { + "SLACK_SEND_RETRY_MAX_TIME": 150, + "SLACK_API_TIMEOUT": 30, + }, + ) + mocker.patch( + "superset.reports.notifications.slack_transport.time.monotonic", + return_value=10.0, + ) + deadlines: list[float] = [] + + send_to_slack_channels( + ["C1", "C2"], + lambda _channel, retry_deadline: deadlines.append(retry_deadline), + retry_deadline=40.0, + ) + + assert deadlines == [40.0, 40.0] + + +@pytest.mark.parametrize( + "transport_error", + [ + URLError("connection reset"), + ConnectionResetError("connection reset"), + RemoteDisconnected("connection closed"), + TimeoutError("timed out"), + ], +) +def test_send_to_slack_channels_does_not_retry_outcome_unknown_transport_errors( + transport_error: Exception, +) -> None: + """Response-loss errors remain transient without duplicating accepted writes.""" + methods = { + "private-a": MagicMock(side_effect=transport_error), + "private-b": MagicMock(return_value={"ok": True}), + } + + def send(channel: str, retry_deadline: float) -> None: + call_slack_api(methods[channel], retry_deadline=retry_deadline) + + with pytest.raises(NotificationTransientError, match="private-a"): + send_to_slack_channels(["private-a", "private-b"], send) + + methods["private-a"].assert_called_once_with() + methods["private-b"].assert_called_once_with() + + +def test_mixed_channel_failures_escalate_to_transient() -> None: + """One transient failure makes a mixed batch retryable.""" + permanent = SlackApiError( + message="channel not found", + response={"ok": False, "error": "channel_not_found"}, + ) + transient = SlackApiError( + message="service unavailable", + response={"ok": False, "error": "service_unavailable"}, + ) + failures = { + "permanent": permanent, + "transient": transient, + } + + def send(channel: str, _retry_deadline: float) -> None: + raise failures[channel] + + with pytest.raises( + NotificationTransientError, + match=r"(?s)permanent.*transient", + ): + send_to_slack_channels(["permanent", "transient"], send) + + +def test_call_slack_api_checks_monotonic_deadline_before_each_retry() -> None: + """A failed call that consumes the budget cannot start another attempt.""" + clock = [0.0] + error = SlackApiError( + message="service unavailable", + response={"ok": False, "error": "service_unavailable"}, + ) + method = MagicMock() + + def fail_after_deadline() -> None: + clock[0] = 151.0 + raise error + + method.side_effect = fail_after_deadline + + with ( + patch( + "superset.reports.notifications.slack_transport.time.monotonic", + side_effect=lambda: clock[0], + ), + pytest.raises(SlackRetryDeadlineError, match="deadline exceeded"), + ): + call_slack_api(method, retry_deadline=150.0) + + method.assert_called_once_with() + + +def test_call_slack_api_without_explicit_deadline_is_still_bounded() -> None: + """The helper's default cannot create an unbounded retry operation.""" + clock = [0.0] + error = SlackApiError( + message="service unavailable", + response={"ok": False, "error": "service_unavailable"}, + ) + method = MagicMock() + + def fail_after_default_deadline() -> None: + clock[0] = 151.0 + raise error + + method.side_effect = fail_after_default_deadline + + with ( + patch( + "superset.reports.notifications.slack_transport.time.monotonic", + side_effect=lambda: clock[0], + ), + pytest.raises(SlackRetryDeadlineError, match="deadline exceeded"), + ): + call_slack_api(method) + + method.assert_called_once_with() + + +@pytest.mark.parametrize( + ("configured_budget", "request_timeout", "expected_budget"), + [ + (150, 30, 150), + (150, 300, 301), + (600, 300, 600), + ], +) +def test_send_deadline_respects_channel_budget_and_request_timeout( + mocker, + configured_budget: int, + request_timeout: int, + expected_budget: int, +) -> None: + """The send budget is configurable and cannot neutralize request timeout.""" + mocker.patch.dict( + "superset.reports.notifications.slack_transport.app.config", + { + "SLACK_SEND_RETRY_MAX_TIME": configured_budget, + "SLACK_API_TIMEOUT": request_timeout, + }, + ) + monotonic = mocker.patch( + "superset.reports.notifications.slack_transport.time.monotonic", + return_value=10.0, + ) + send = MagicMock() + + send_to_slack_channels(["C1"], send) + + send.assert_called_once_with("C1", 10.0 + expected_budget) + assert monotonic.called + + +def test_send_deadline_uses_default_budget_when_config_is_absent(mocker) -> None: + """Custom configs predating the setting retain the documented default.""" + mocker.patch.dict( + "superset.reports.notifications.slack_transport.app.config", + {"SLACK_API_TIMEOUT": 30}, + clear=True, + ) + mocker.patch( + "superset.reports.notifications.slack_transport.time.monotonic", + return_value=10.0, + ) + send = MagicMock() + + send_to_slack_channels(["C1"], send) + + send.assert_called_once_with("C1", 160.0) + + +def test_deadline_error_names_operator_setting() -> None: + """Expired sends identify the config knob that controls channel budgets.""" + with ( + patch( + "superset.reports.notifications.slack_transport.time.monotonic", + return_value=151.0, + ), + pytest.raises( + SlackRetryDeadlineError, + match="SLACK_SEND_RETRY_MAX_TIME", + ), + ): + call_slack_api(MagicMock(), retry_deadline=150.0) + + +@patch("superset.reports.notifications.slackv2.g") +@patch("superset.reports.notifications.slackv2.get_slack_client") +def test_v2_send_uses_configured_application_rate_limit_budget( + slack_client_mock: MagicMock, + flask_global_mock: MagicMock, + mock_header_data: HeaderDataType, +) -> None: + """Delivery 429 retries use one configured application-owned budget.""" + flask_global_mock.logs_context = {} + response = _make_slack_response( + 429, + {"ok": False, "error": "ratelimited"}, + headers={"Retry-After": ["0"]}, + ) + slack_client_mock.return_value.chat_postMessage.side_effect = SlackApiError( + message="rate limited", + response=response, + ) + notification = _make_v2_notification( + _make_content(mock_header_data), + target="C12345", + ) + + with ( + patch("superset.reports.notifications.slack_transport.time.sleep"), + pytest.raises(NotificationError, match="C12345"), + ): + notification.send() + + assert slack_client_mock.return_value.chat_postMessage.call_count == 3 + + 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 @@ -1107,6 +2334,18 @@ def test_give_up_slack_api_retry_gives_up_on_permanent_status_code() -> None: assert _give_up_slack_api_retry(ex) is True +def test_call_slack_api_does_not_retry_empty_client_error_response() -> None: + """A non-429 4xx is permanent even when Slack omits an error code.""" + response = _make_slack_response(400, {}) + ex = SlackApiError(message="bad request", response=response) + method = MagicMock(side_effect=ex) + + with pytest.raises(SlackApiError): + call_slack_api(method) + + method.assert_called_once_with() + + @patch("superset.reports.notifications.slackv2.g") @patch("superset.reports.notifications.slackv2.get_slack_client") def test_v2_send_records_statsd_gauge_on_success( @@ -1159,10 +2398,12 @@ def test_v2_send_propagates_execution_id_to_logs( logger_mock: MagicMock, flask_global_mock: MagicMock, mock_header_data, + raw_upload_mock: MagicMock, ) -> 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} + _configure_v2_upload_client(slack_client_mock.return_value) content = _make_content(mock_header_data, screenshots=[b"shot"]) notification = _make_v2_notification(content, target="C12345") @@ -1171,6 +2412,7 @@ def test_v2_send_propagates_execution_id_to_logs( logger_mock.info.assert_called_with( "Report sent to slack", extra={"execution_id": execution_id} ) + raw_upload_mock.assert_called_once() @patch("superset.reports.notifications.slackv2.g") @@ -1208,7 +2450,10 @@ def test_v2_send_handles_missing_logs_context( @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") +@patch( + "superset.reports.notifications.slack_channel_resolver" + ".get_channels_with_search_and_cache_status" +) def test_auto_upgrade_round_trip_v1_to_v2( get_channels_with_search_mock: MagicMock, v1_client_mock: MagicMock, @@ -1236,9 +2481,17 @@ def test_auto_upgrade_round_trip_v1_to_v2( 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} - ] + get_channels_with_search_mock.return_value = ( + [ + { + "id": "C12345", + "name": "general", + "is_member": True, + "is_private": False, + } + ], + False, + ) schedule = ReportSchedule( recipients=[ diff --git a/tests/unit_tests/tasks/test_slack.py b/tests/unit_tests/tasks/test_slack.py new file mode 100644 index 00000000000..12626676601 --- /dev/null +++ b/tests/unit_tests/tasks/test_slack.py @@ -0,0 +1,75 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +import pytest +from pytest_mock import MockerFixture + +from superset.app import SupersetApp +from superset.constants import CACHE_DISABLED_TIMEOUT +from superset.tasks.slack import cache_channels + + +def test_cache_channels_requires_a_successful_cache_write( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + app.config["SLACK_CACHE_TIMEOUT"] = 123 + app.config["SLACK_API_RATE_LIMIT_RETRY_COUNT"] = 4 + get_channels = mocker.patch("superset.tasks.slack.get_channels") + + cache_channels.run() + + get_channels.assert_called_once_with( + force=True, + cache_timeout=123, + raise_on_cache_write_error=True, + ) + + +def test_cache_channels_warns_when_caching_is_disabled( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + app.config["SLACK_CACHE_TIMEOUT"] = CACHE_DISABLED_TIMEOUT + get_channels = mocker.patch("superset.tasks.slack.get_channels") + logger = mocker.patch("superset.tasks.slack.logger") + + cache_channels.run() + + get_channels.assert_not_called() + logger.warning.assert_called_once_with( + "Skipping Slack channels cache warm-up because " + "SLACK_CACHE_TIMEOUT disables caching" + ) + + +def test_cache_channels_rolls_back_a_failed_cache_write( + app: SupersetApp, + mocker: MockerFixture, +) -> None: + app.config["SLACK_CACHE_TIMEOUT"] = 123 + mocker.patch( + "superset.tasks.slack.get_channels", + side_effect=ConnectionError("metastore unavailable"), + ) + db = mocker.patch("superset.db") + + with pytest.raises(ConnectionError, match="metastore unavailable"): + cache_channels.run() + + db.session.commit.assert_not_called() + db.session.rollback.assert_called_once_with() diff --git a/tests/unit_tests/utils/slack_test.py b/tests/unit_tests/utils/slack_test.py index e2da1cd16cc..d9ed9227a40 100644 --- a/tests/unit_tests/utils/slack_test.py +++ b/tests/unit_tests/utils/slack_test.py @@ -16,33 +16,92 @@ # under the License. import warnings +from email.message import Message +from http.client import RemoteDisconnected +from unittest.mock import call +from urllib.error import HTTPError, URLError import pytest from slack_sdk.errors import ( SlackApiError, + SlackClientConfigurationError, SlackClientNotConnectedError, SlackRequestError, + SlackTokenRotationError, +) +from slack_sdk.http_retry.builtin_handlers import ( + ConnectionErrorRetryHandler, + RateLimitErrorRetryHandler, ) +from superset.constants import CACHE_DISABLED_TIMEOUT +from superset.exceptions import SupersetException from superset.utils.slack import ( _emit_v1_flag_off_deprecation, _emit_v1_scope_missing_deprecation, _SLACK_V1_DEPRECATION_MESSAGE, + get_channels, get_channels_with_search, + get_channels_with_search_and_cache_status, + get_slack_client, + is_transient_slack_api_error, + refresh_cached_slack_channels_with_search, should_use_v2_api, + SlackChannelListingClientError, + SlackChannelListingError, SlackChannelTypes, + SlackV2ProbeClientError, + SlackV2ProbeError, ) class MockResponse: - def __init__(self, data): + def __init__(self, data, status_code: int | None = None): self._data = data + self.status_code = status_code @property def data(self): return self._data +def test_delivery_client_disables_outcome_unknown_connection_retries(mocker) -> None: + mocker.patch.dict( + "superset.utils.slack.app.config", + { + "SLACK_API_TOKEN": "xoxb-test", + "SLACK_PROXY": None, + "SLACK_API_TIMEOUT": 30, + "SLACK_API_RATE_LIMIT_RETRY_COUNT": 2, + }, + ) + + logger = mocker.patch("superset.utils.slack.logger") + delivery_client = get_slack_client(for_delivery=True) + discovery_client = get_slack_client() + + assert not any( + isinstance(handler, RateLimitErrorRetryHandler) + for handler in delivery_client.retry_handlers + ) + assert not any( + isinstance(handler, ConnectionErrorRetryHandler) + for handler in delivery_client.retry_handlers + ) + assert any( + isinstance(handler, ConnectionErrorRetryHandler) + for handler in discovery_client.retry_handlers + ) + assert any( + isinstance(handler, RateLimitErrorRetryHandler) + for handler in discovery_client.retry_handlers + ) + assert logger.debug.call_args_list == [ + call("Slack delivery client configured with SDK retries disabled"), + call("Slack client configured with %d rate limit retries", 2), + ] + + class TestGetChannelsWithSearch: # Fetch all channels when no search string is provided def test_fetch_all_channels_no_search_string(self, mocker): @@ -100,6 +159,20 @@ class TestGetChannelsWithSearch: # Assert that the result is a list with a single channel dictionary assert result == [{"name": "general", "id": "C12345"}] + def test_exact_match_uses_unicode_casefolding(self, mocker) -> None: + mock_client = mocker.Mock() + mock_client.conversations_list.return_value = MockResponse( + { + "channels": [{"name": "Straße", "id": "C12345"}], + "response_metadata": {"next_cursor": None}, + } + ) + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + + result = get_channels_with_search(search_string="STRASSE", exact_match=True) + + assert result == [{"name": "Straße", "id": "C12345"}] + def test_handle_exact_match_search_string_multiple_channels(self, mocker): mock_data = { "channels": [ @@ -164,6 +237,43 @@ class TestGetChannelsWithSearch: The server responded with: missing scope: channels:read""" ) + @pytest.mark.parametrize( + ("error_code", "expected_exception"), + [ + ("invalid_auth", SlackChannelListingClientError), + ("service_unavailable", SlackChannelListingError), + ], + ) + def test_channel_listing_preserves_slack_error_classification( + self, + mocker, + error_code: str, + expected_exception: type[Exception], + ) -> None: + mock_client = mocker.Mock() + mock_client.conversations_list.side_effect = SlackApiError( + "listing failed", + {"ok": False, "error": error_code}, + ) + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + + with pytest.raises(expected_exception): + get_channels_with_search(force=True) + + def test_channel_listing_rate_limit_retains_operator_hint(self, mocker) -> None: + mock_client = mocker.Mock() + mock_client.conversations_list.side_effect = SlackApiError( + "rate limited", + MockResponse({"ok": False, "error": "ratelimited"}, status_code=429), + ) + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + + with pytest.raises( + SlackChannelListingError, + match="consider increasing SLACK_API_RATE_LIMIT_RETRY_COUNT", + ): + get_channels_with_search(force=True) + @pytest.mark.parametrize( "types, expected_channel_ids", [ @@ -217,13 +327,595 @@ The server responded with: missing scope: channels:read""" mock_client.conversations_list.return_value = MockResponse(mock_data) mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) mocker.patch.dict( - "superset.utils.slack.app.config", {"SLACK_TEAM_ID": "T123456"} + "superset.utils.slack.app.config", + {"SLACK_TEAM_ID": "T123456"}, ) get_channels_with_search(force=True) assert mock_client.conversations_list.call_args.kwargs["team_id"] == "T123456" + def test_cache_hit_and_channels_come_from_one_read(self, mocker) -> None: + cached_channels = [{"id": "C1", "name": "cached"}] + cache_get = mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + return_value=cached_channels, + ) + channel_fetch = mocker.patch("superset.utils.slack._get_channels") + mocker.patch.dict( + "superset.utils.slack.app.config", + {"SLACK_TEAM_ID": "T123456"}, + ) + + channels, used_cache = get_channels_with_search_and_cache_status() + + assert channels == cached_channels + assert used_cache is True + cache_get.assert_called_once_with("slack_conversations_list_T123456") + channel_fetch.assert_not_called() + + def test_disabled_cache_default_search_ignores_stale_entry(self, mocker) -> None: + cache_get = mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + return_value=[{"id": "C0", "name": "stale"}], + ) + cache_set = mocker.patch("superset.utils.slack.cache_manager.cache.set") + channel_fetch = mocker.patch( + "superset.utils.slack._get_channels", + side_effect=[ + [{"id": "C1", "name": "first"}], + [{"id": "C2", "name": "second"}], + ], + ) + mocker.patch.dict( + "superset.utils.slack.app.config", + {"SLACK_CACHE_TIMEOUT": CACHE_DISABLED_TIMEOUT}, + ) + + assert get_channels_with_search() == [{"id": "C1", "name": "first"}] + assert get_channels_with_search() == [{"id": "C2", "name": "second"}] + + cache_get.assert_not_called() + cache_set.assert_not_called() + assert channel_fetch.call_count == 2 + + def test_cache_miss_fetches_once_and_reports_live_provenance(self, mocker) -> None: + live_channels = [{"id": "C2", "name": "live"}] + cache_get = mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + return_value=None, + ) + channel_fetch = mocker.patch( + "superset.utils.slack._get_channels", + return_value=live_channels, + ) + cache_set = mocker.patch("superset.utils.slack.cache_manager.cache.set") + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + + channels, used_cache = get_channels_with_search_and_cache_status() + + assert channels == live_channels + assert used_cache is False + cache_get.assert_called_once_with("slack_conversations_list") + channel_fetch.assert_called_once_with(team_id=None) + cache_set.assert_called_once_with( + "slack_conversations_list", + live_channels, + timeout=mocker.ANY, + ) + + def test_cache_read_failure_falls_back_to_live_channels(self, mocker) -> None: + live_channels = [{"id": "C2", "name": "live"}] + mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + side_effect=ConnectionError("Redis unavailable"), + ) + channel_fetch = mocker.patch( + "superset.utils.slack._get_channels", + return_value=live_channels, + ) + cache_set = mocker.patch("superset.utils.slack.cache_manager.cache.set") + logger = mocker.patch("superset.utils.slack.logger") + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + + channels, used_cache = get_channels_with_search_and_cache_status() + + assert channels == live_channels + assert used_cache is False + channel_fetch.assert_called_once() + cache_set.assert_called_once() + logger.warning.assert_called_once() + + def test_default_search_cache_read_failure_uses_live_channels(self, mocker) -> None: + live_channels = [{"id": "C2", "name": "live"}] + mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + side_effect=ConnectionError("Redis unavailable"), + ) + channel_fetch = mocker.patch( + "superset.utils.slack._get_channels", + return_value=live_channels, + ) + cache_set = mocker.patch("superset.utils.slack.cache_manager.cache.set") + logger = mocker.patch("superset.utils.slack.logger") + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + + assert get_channels_with_search() == live_channels + + channel_fetch.assert_called_once() + cache_set.assert_called_once() + logger.warning.assert_called_once() + + def test_forced_search_cache_write_failure_preserves_live_channels( + self, mocker + ) -> None: + live_channels = [{"id": "C2", "name": "live"}] + cache_get = mocker.patch("superset.utils.slack.cache_manager.cache.get") + channel_fetch = mocker.patch( + "superset.utils.slack._get_channels", + return_value=live_channels, + ) + mocker.patch( + "superset.utils.slack.cache_manager.cache.set", + side_effect=ConnectionError("Redis unavailable"), + ) + logger = mocker.patch("superset.utils.slack.logger") + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + + assert get_channels_with_search(force=True) == live_channels + + cache_get.assert_not_called() + channel_fetch.assert_called_once() + logger.warning.assert_called_once() + + def test_cache_write_failure_preserves_live_channels(self, mocker) -> None: + live_channels = [{"id": "C2", "name": "live"}] + mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + return_value=None, + ) + mocker.patch( + "superset.utils.slack._get_channels", + return_value=live_channels, + ) + mocker.patch( + "superset.utils.slack.cache_manager.cache.set", + side_effect=ConnectionError("Redis unavailable"), + ) + logger = mocker.patch("superset.utils.slack.logger") + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + + channels, used_cache = get_channels_with_search_and_cache_status() + + assert channels == live_channels + assert used_cache is False + logger.warning.assert_called_once() + + def test_metastore_cache_miss_fetches_without_cache_write(self, mocker) -> None: + live_channels = [{"id": "C2", "name": "live"}] + mocker.patch("superset.utils.slack.cache_manager.cache.get", return_value=None) + channel_fetch = mocker.patch( + "superset.utils.slack._get_channels", + return_value=live_channels, + ) + cache_set = mocker.patch("superset.utils.slack.cache_manager.cache.set") + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=True, + ) + + channels, used_cache = get_channels_with_search_and_cache_status() + + assert channels == live_channels + assert used_cache is False + channel_fetch.assert_called_once_with(team_id=None) + cache_set.assert_not_called() + + def test_forced_warmup_writes_metastore_channel_cache(self, mocker) -> None: + live_channels = [{"id": "C2", "name": "live"}] + channel_fetch = mocker.patch( + "superset.utils.slack._get_channels", + return_value=live_channels, + ) + cache_set = mocker.patch("superset.utils.slack.cache_manager.cache.set") + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=True, + ) + + assert get_channels(force=True, cache_timeout=123) == live_channels + + channel_fetch.assert_called_once_with(team_id=None) + cache_set.assert_called_once_with( + "slack_conversations_list", + live_channels, + timeout=123, + ) + + def test_strict_warmup_propagates_cache_write_failure(self, mocker) -> None: + mocker.patch( + "superset.utils.slack._get_channels", + return_value=[{"id": "C2", "name": "live"}], + ) + mocker.patch( + "superset.utils.slack.cache_manager.cache.set", + side_effect=ConnectionError("metastore unavailable"), + ) + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=True, + ) + + with pytest.raises(ConnectionError, match="metastore unavailable"): + get_channels( + force=True, + cache_timeout=123, + raise_on_cache_write_error=True, + ) + + def test_strict_warmup_rejects_false_cache_write_result(self, mocker) -> None: + mocker.patch( + "superset.utils.slack._get_channels", + return_value=[{"id": "C2", "name": "live"}], + ) + mocker.patch( + "superset.utils.slack.cache_manager.cache.set", + return_value=False, + ) + logger = mocker.patch("superset.utils.slack.logger") + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=True, + ) + + with pytest.raises( + SupersetException, + match="cache rejected the write", + ): + get_channels( + force=True, + cache_timeout=123, + raise_on_cache_write_error=True, + ) + + logger.warning.assert_not_called() + + @pytest.mark.parametrize("cache_set_result", [True, None]) + def test_refreshes_cached_channels_once_per_workspace_cooldown( + self, mocker, cache_set_result + ) -> None: + refreshed_channels = [{"id": "C2", "name": "new", "is_private": False}] + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + channel_search = mocker.patch( + "superset.utils.slack.get_channels_with_search", + return_value=refreshed_channels, + ) + cache_add = mocker.patch( + "superset.utils.slack.cache_manager.cache.add", + return_value=True, + ) + cache_set = mocker.patch( + "superset.utils.slack.cache_manager.cache.set", + return_value=cache_set_result, + ) + mocker.patch.dict( + "superset.utils.slack.app.config", + { + "SLACK_TEAM_ID": "T123456", + "SLACK_CHANNEL_REFRESH_COOLDOWN_SECONDS": 42, + }, + ) + + assert ( + refresh_cached_slack_channels_with_search( + search_string="new", + types=[SlackChannelTypes.PUBLIC], + exact_match=True, + ) + == refreshed_channels + ) + + channel_search.assert_called_once_with( + force=True, + cache=False, + ) + cache_add.assert_called_once_with( + "slack_conversations_list_T123456_refresh_cooldown", + True, + timeout=42, + ) + assert cache_set.call_args_list == [ + mocker.call( + "slack_conversations_list_T123456", + refreshed_channels, + timeout=mocker.ANY, + ), + ] + + def test_concurrent_refresh_uses_cache_after_losing_atomic_claim( + self, mocker + ) -> None: + """Only the worker that atomically claims the cooldown fetches Slack.""" + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + cache_add = mocker.patch( + "superset.utils.slack.cache_manager.cache.add", + return_value=False, + ) + cache_get = mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + return_value=[{"id": "C2", "name": "new", "is_private": False}], + ) + channel_search = mocker.patch("superset.utils.slack.get_channels_with_search") + mocker.patch.dict( + "superset.utils.slack.app.config", + { + "SLACK_TEAM_ID": "T123456", + "SLACK_CHANNEL_REFRESH_COOLDOWN_SECONDS": 42, + }, + ) + + assert refresh_cached_slack_channels_with_search(search_string="new") == [ + {"id": "C2", "name": "new", "is_private": False} + ] + + cache_add.assert_called_once_with( + "slack_conversations_list_T123456_refresh_cooldown", + True, + timeout=42, + ) + cache_get.assert_called_once_with("slack_conversations_list_T123456") + channel_search.assert_not_called() + + def test_concurrent_refresh_cache_miss_does_not_duplicate_slack_listing( + self, mocker + ) -> None: + """A losing worker waits for a later execution instead of listing Slack.""" + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + mocker.patch( + "superset.utils.slack.cache_manager.cache.add", + return_value=False, + ) + mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + return_value=None, + ) + channel_fetch = mocker.patch("superset.utils.slack._get_channels") + + assert refresh_cached_slack_channels_with_search(search_string="new") == [] + + channel_fetch.assert_not_called() + + def test_failed_channel_cache_write_does_not_record_cooldown(self, mocker) -> None: + refreshed_channels = [{"id": "C2", "name": "new"}] + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + channel_search = mocker.patch( + "superset.utils.slack.get_channels_with_search", + return_value=refreshed_channels, + ) + mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + return_value=None, + ) + cache_set = mocker.patch( + "superset.utils.slack.cache_manager.cache.set", + return_value=False, + ) + cache_delete = mocker.patch("superset.utils.slack.cache_manager.cache.delete") + + assert refresh_cached_slack_channels_with_search(search_string="new") == [ + {"id": "C2", "name": "new"} + ] + assert refresh_cached_slack_channels_with_search(search_string="new") == [ + {"id": "C2", "name": "new"} + ] + + assert channel_search.call_count == 2 + assert cache_set.call_args_list == [ + mocker.call( + "slack_conversations_list", + refreshed_channels, + timeout=mocker.ANY, + ), + mocker.call( + "slack_conversations_list", + refreshed_channels, + timeout=mocker.ANY, + ), + ] + assert cache_delete.call_count == 2 + + def test_disabled_cache_refresh_ignores_stale_cooldown(self, mocker) -> None: + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + cache_get = mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + return_value=True, + ) + cache_set = mocker.patch("superset.utils.slack.cache_manager.cache.set") + channel_search = mocker.patch( + "superset.utils.slack.get_channels_with_search", + return_value=[{"id": "C2", "name": "new"}], + ) + mocker.patch.dict( + "superset.utils.slack.app.config", + {"SLACK_CACHE_TIMEOUT": CACHE_DISABLED_TIMEOUT}, + ) + + assert refresh_cached_slack_channels_with_search(search_string="new") == [ + {"id": "C2", "name": "new"} + ] + assert refresh_cached_slack_channels_with_search(search_string="new") == [ + {"id": "C2", "name": "new"} + ] + + assert channel_search.call_args_list == [ + mocker.call( + search_string="new", + types=None, + exact_match=False, + force=True, + cache=False, + ), + mocker.call( + search_string="new", + types=None, + exact_match=False, + force=True, + cache=False, + ), + ] + cache_get.assert_not_called() + cache_set.assert_not_called() + + def test_recent_refresh_uses_cached_channels_without_another_force( + self, mocker + ) -> None: + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + mocker.patch("superset.utils.slack.cache_manager.cache.add", return_value=False) + cache_get = mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + return_value=[{"id": "C2", "name": "new", "is_private": False}], + ) + + assert refresh_cached_slack_channels_with_search(search_string="new") == [ + {"id": "C2", "name": "new", "is_private": False} + ] + + cache_get.assert_called_once_with("slack_conversations_list") + + def test_refresh_cooldown_claim_failure_uses_live_channels(self, mocker) -> None: + refreshed_channels = [{"id": "C2", "name": "new", "is_private": False}] + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + mocker.patch( + "superset.utils.slack.cache_manager.cache.add", + side_effect=ConnectionError("Redis unavailable"), + ) + channel_search = mocker.patch( + "superset.utils.slack.get_channels_with_search", + return_value=refreshed_channels, + ) + logger = mocker.patch("superset.utils.slack.logger") + + assert refresh_cached_slack_channels_with_search(search_string="new") == [ + {"id": "C2", "name": "new", "is_private": False} + ] + + channel_search.assert_called_once_with(force=True, cache=False) + logger.warning.assert_called_once() + + def test_concurrent_refresh_cache_read_failure_skips_live_listing( + self, mocker + ) -> None: + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + cache_get = mocker.patch( + "superset.utils.slack.cache_manager.cache.get", + side_effect=ConnectionError("Redis unavailable"), + ) + mocker.patch( + "superset.utils.slack.cache_manager.cache.add", + return_value=False, + ) + channel_fetch = mocker.patch( + "superset.utils.slack._get_channels", + ) + + assert refresh_cached_slack_channels_with_search(search_string="new") == [] + + cache_get.assert_called_once() + channel_fetch.assert_not_called() + + def test_session_backed_cache_uses_uncached_refresh(self, mocker) -> None: + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=True, + ) + cache_get = mocker.patch("superset.utils.slack.cache_manager.cache.get") + cache_set = mocker.patch("superset.utils.slack.cache_manager.cache.set") + channel_search = mocker.patch( + "superset.utils.slack.get_channels_with_search", + return_value=[{"id": "C2", "name": "new"}], + ) + + assert refresh_cached_slack_channels_with_search(search_string="new") == [ + {"id": "C2", "name": "new"} + ] + + channel_search.assert_called_once_with( + search_string="new", + types=None, + exact_match=False, + force=True, + cache=False, + ) + cache_get.assert_not_called() + cache_set.assert_not_called() + + def test_failed_forced_refresh_does_not_record_cooldown(self, mocker) -> None: + mocker.patch( + "superset.utils.slack._slack_channel_cache_uses_report_session", + return_value=False, + ) + channel_search = mocker.patch( + "superset.utils.slack.get_channels_with_search", + side_effect=SupersetException("Slack listing failed"), + ) + mocker.patch( + "superset.utils.slack.cache_manager.cache.add", + return_value=True, + ) + cache_delete = mocker.patch("superset.utils.slack.cache_manager.cache.delete") + cache_set = mocker.patch("superset.utils.slack.cache_manager.cache.set") + + with pytest.raises(SupersetException, match="Slack listing failed"): + refresh_cached_slack_channels_with_search(search_string="new") + + channel_search.assert_called_once_with( + force=True, + cache=False, + ) + cache_set.assert_not_called() + cache_delete.assert_called_once_with( + "slack_conversations_list_refresh_cooldown" + ) + def test_resolves_callable_team_id(self, mocker): # SLACK_TEAM_ID may be a callable (e.g. to fetch from a secrets store), # mirroring SLACK_API_TOKEN; it is resolved before being forwarded. @@ -309,9 +1001,9 @@ The server responded with: missing scope: channels:read""" 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. + The deprecation emitters use `functools.cache` to suppress calls after the + first one completes. 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() @@ -384,7 +1076,7 @@ class TestShouldUseV2Api: with warnings.catch_warnings(record=True) as caught: warnings.simplefilter("always") - assert should_use_v2_api() is False + assert should_use_v2_api(raise_on_error=True) is False assert should_use_v2_api() is False assert should_use_v2_api() is False @@ -433,6 +1125,19 @@ class TestShouldUseV2Api: assert logger_mock.warning.call_count == 1 assert "channels:read" in logger_mock.warning.call_args.args[0] + @pytest.mark.parametrize("status_code", [429, 503]) + def test_transient_probe_error_detected_from_http_status( + self, + status_code: int, + ) -> None: + """Rate-limit and server HTTP statuses remain system failures.""" + error = SlackApiError( + message="probe failed", + response=MockResponse({"ok": False}, status_code=status_code), + ) + + assert is_transient_slack_api_error(error, "") is True + @pytest.mark.parametrize( "error_code", ["invalid_auth", "ratelimited", "fatal_error", "account_inactive", ""], @@ -478,9 +1183,10 @@ class TestShouldUseV2Api: [ SlackClientNotConnectedError("transport closed"), SlackRequestError("bad request args"), + URLError("connection reset"), ], ) - def test_returns_false_on_slack_sdk_client_error_from_probe( + def test_returns_false_on_slack_client_or_transport_error_from_probe( self, exception: Exception, mocker ): """Non-`SlackApiError` SDK failures (e.g. `SlackClientNotConnectedError`, @@ -502,7 +1208,78 @@ class TestShouldUseV2Api: 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] + assert "probe failed" in logger_mock.warning.call_args.args[0] + + @pytest.mark.parametrize( + "exception", + [ + SlackApiError( + message="service unavailable", + response={"ok": False, "error": "service_unavailable"}, + ), + SlackApiError( + message="rate limited", + response={"ok": False, "error": "ratelimited"}, + ), + SlackClientNotConnectedError("transport closed"), + URLError("connection reset"), + ConnectionResetError("connection reset"), + RemoteDisconnected("connection closed"), + TimeoutError("timed out"), + HTTPError("https://slack.com", 504, "unavailable", Message(), None), + ], + ) + def test_raises_system_error_for_transient_probe_when_requested( + self, + exception: Exception, + mocker, + ) -> None: + """File sends preserve transient probe failures for system 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) + + with pytest.raises(SlackV2ProbeError, match="probe failed"): + should_use_v2_api(raise_on_error=True) + + @pytest.mark.parametrize( + "exception", + [ + SlackApiError( + message="invalid auth", + response={"ok": False, "error": "invalid_auth"}, + ), + SlackRequestError("bad request args"), + SlackClientConfigurationError("invalid client configuration"), + SlackTokenRotationError("token rotation failed"), + HTTPError("https://slack.com", 413, "too large", Message(), None), + ], + ) + def test_raises_client_error_for_permanent_probe_failure_when_requested( + self, + exception: Exception, + mocker, + ) -> None: + """Permanent probe failures remain operator-fixable client errors.""" + 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) + + with pytest.raises(SlackV2ProbeClientError, match="probe failed") as exc_info: + should_use_v2_api(raise_on_error=True) + + assert exc_info.value.status == 422 + + def test_client_probe_error_has_common_probe_error_parent(self) -> None: + assert issubclass(SlackV2ProbeClientError, SlackV2ProbeError) def test_propagates_non_sdk_errors_from_probe(self, mocker): """A truly unexpected, non-SDK exception (e.g. a programming error) still diff --git a/tests/unit_tests/utils/test_decorators.py b/tests/unit_tests/utils/test_decorators.py index 8073fbb25e8..a650c6ee467 100644 --- a/tests/unit_tests/utils/test_decorators.py +++ b/tests/unit_tests/utils/test_decorators.py @@ -36,6 +36,20 @@ class ResponseValues(StrEnum): OK = "ok" +class WarningError(Exception): + status = 400 + + +class InvalidStatusError(Exception): + status = "400" + + +class UnreadableStatusError(Exception): + @property + def status(self) -> int: + raise RuntimeError("status is unavailable") + + def test_debounce() -> None: mock = Mock() @@ -63,7 +77,7 @@ def test_debounce() -> None: [ (ResponseValues.OK, None, "custom.prefix.ok"), (ResponseValues.FAIL, ValueError, "custom.prefix.error"), - (ResponseValues.WARN, FileNotFoundError, "custom.prefix.warn"), + (ResponseValues.WARN, WarningError, "custom.prefix.warning"), ], ) def test_statsd_gauge( @@ -74,7 +88,7 @@ def test_statsd_gauge( if response == ResponseValues.FAIL: raise ValueError("Error") if response == ResponseValues.WARN: - raise FileNotFoundError("Not found") + raise WarningError("Warning") return "OK" with patch("superset.extensions.stats_logger_manager.instance.gauge") as mock: @@ -86,7 +100,47 @@ def test_statsd_gauge( with cm: my_func(response_value, 1, 2) - mock.assert_called_once_with(expected_result, 1) + + mock.assert_called_once_with(expected_result, 1) + + +def test_statsd_gauge_ignores_configured_exception() -> None: + class RoutingSignalError(Exception): + pass + + @decorators.statsd_gauge( + "custom.prefix", + ignored_exceptions=(RoutingSignalError,), + ) + def my_func() -> None: + raise RoutingSignalError + + with ( + patch("superset.extensions.stats_logger_manager.instance.gauge") as mock, + pytest.raises(RoutingSignalError), + ): + my_func() + + mock.assert_not_called() + + +@pytest.mark.parametrize( + ("exception", "expected_metric"), + [ + (ValueError("failure"), "custom.prefix.error"), + (WarningError("warning"), "custom.prefix.warning"), + (InvalidStatusError("invalid status"), "custom.prefix.error"), + (UnreadableStatusError("unreadable status"), "custom.prefix.error"), + ], +) +def test_record_statsd_gauge_failure_uses_shared_severity_contract( + exception: Exception, + expected_metric: str, +) -> None: + with patch("superset.extensions.stats_logger_manager.instance.gauge") as mock: + decorators.record_statsd_gauge_failure("custom.prefix", exception) + + mock.assert_called_once_with(expected_metric, 1) @patch("superset.utils.decorators.g")