codeant-ai-for-open-source[bot] commented on code in PR #42074:
URL: https://github.com/apache/superset/pull/42074#discussion_r3584621140
##########
superset/commands/report/execute.py:
##########
@@ -187,14 +187,15 @@ def update_report_schedule_slack_v2(self) -> None:
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(
+ channels_data = get_channels_with_search(
search_string=channel_names,
types=[
SlackChannelTypes.PRIVATE,
SlackChannelTypes.PUBLIC,
],
exact_match=True,
)
+ channels = channels_data["result"]
Review Comment:
**Suggestion:** This migration lookup now calls a paginated API but never
overrides the default page size, so it only inspects the first 100 matching
channels. If a legacy Slack recipient contains more than 100 channel targets,
the upgrade will incorrectly report channels as missing and fail even though
they exist. Pass an explicit limit based on the number of targets (and/or
follow pagination until all requested exact matches are resolved). [api
mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Auto-upgrade fails for multi-channel Slack recipients over 100.
❌ Affected reports fail to send Slack notifications.
⚠️ Large orgs’ Slack alert migrations become brittle and error-prone.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Slack v1 report recipient whose
`recipient_config_json["target"]` contains
more than 100 comma-separated channels (permitted by
`SlackNotification._get_channel` in
`superset/reports/notifications/slack.py:30-39`, which joins
`recipients_string_to_list`,
and by `recipients_string_to_list` in `superset/utils/core.py:10-20` that
splits an
arbitrary-length comma/semicolon-separated string).
2. Trigger the report execution so that `_send` in
`superset/commands/report/execute.py:940-969` runs; when
`should_use_v2_api()` returns
True, `SlackNotification.send()` raises `SlackV1NotificationError` and
`_send` catches it
at `execute.py:26-32`, calling `self.update_report_schedule_slack_v2()` to
auto-upgrade
the Slack recipient.
3. Inside `BaseReportState.update_report_schedule_slack_v2` in
`superset/commands/report/execute.py:169-210`, the legacy target string
(with all >100
channels) is assigned to `channel_names` and passed directly to
`get_channels_with_search`
at `execute.py:190-197` with `exact_match=True` but without a `limit`
argument, so the
default `limit=100` from `get_channels_with_search` in
`superset/utils/slack.py:172-178`
is used.
4. `_fetch_channels_with_search` in `superset/utils/slack.py:179-241` stops
collecting
matches once `len(matches) >= limit` (100) and returns at most 100 channels
in
`channels_data["result"]`; back in `update_report_schedule_slack_v2` at
`execute.py:199-208`, `channels_list =
recipients_string_to_list(channel_names)` contains
all >100 targets, so `len(channels_list) != len(channels)` and the code
raises
`UpdateFailedError("Could not find the following channels: ...")`, causing
the
auto-upgrade (and thus the report send) to fail even though the extra
channels exist in
Slack but were never fetched due to the 100-result limit.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=553286d3a2a54937a60c0e6d564a47b5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=553286d3a2a54937a60c0e6d564a47b5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/report/execute.py
**Line:** 190:198
**Comment:**
*Api Mismatch: This migration lookup now calls a paginated API but
never overrides the default page size, so it only inspects the first 100
matching channels. If a legacy Slack recipient contains more than 100 channel
targets, the upgrade will incorrectly report channels as missing and fail even
though they exist. Pass an explicit limit based on the number of targets
(and/or follow pagination until all requested exact matches are resolved).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=a6bacf818bd7e0d77f183d9fa3342da0898e34ac4034ec4e3c835e4e7c48083d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=a6bacf818bd7e0d77f183d9fa3342da0898e34ac4034ec4e3c835e4e7c48083d&reaction=dislike'>👎</a>
##########
superset/reports/api.py:
##########
@@ -682,14 +687,29 @@ def slack_channels(self, **kwargs: Any) -> Response:
search_string = params.get("search_string")
types = params.get("types", [])
exact_match = params.get("exact_match", False)
+ cursor = params.get("cursor")
+ limit = params.get("limit", 100)
force = params.get("force", False)
- channels = get_channels_with_search(
+
+ # Clear cache if force refresh requested
+ if force:
+ cache_manager.cache.delete(SLACK_CHANNELS_CACHE_KEY)
+
cache_manager.cache.delete(SLACK_CHANNELS_CONTINUATION_CURSOR_KEY)
+ logger.info("Slack channels cache cleared due to force=True")
+
+ # Trigger async cache warmup if caching is enabled
+ if current_app.config.get("SLACK_ENABLE_CACHING", True):
+ cache_channels.apply_async(**get_cache_warmup_options())
+ logger.info("Triggered async cache warmup task")
+
Review Comment:
**Suggestion:** The forced refresh path directly dispatches a Celery task
from the request, but this call can raise broker/transport exceptions when
Celery is unavailable or misconfigured; those exceptions are not converted to a
handled API response here, so the endpoint can fail with a 500 on `force=true`.
Wrap task dispatch failures and continue serving channels (or return a
controlled 422/503) instead of letting the request crash. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Slack channel refresh fails with unhandled server error.
- ⚠️ Alert Slack recipient dropdown breaks on cache refresh.
- ⚠️ Admins must debug Celery errors from user actions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with this PR deployed, and configure alerts to use Slack
v2; the backend
Slack channels endpoint lives at `superset/reports/api.py:645-76` (the
`slack_channels`
method annotated with the OpenAPI docstring).
2. In the Alerts & Reports UI, open the notification method editor for a
Slack v2 alert;
this uses `NotificationMethod` in
`superset-frontend/src/features/alerts/components/NotificationMethod.tsx:115-162`,
which
in turn calls `useSlackChannels` from
`superset-frontend/src/features/alerts/hooks/useSlackChannels.ts:20-32`.
3. Click the UI action that refreshes Slack channels (wired through
`handleRefreshSlackChannels = refreshChannels` at
`NotificationMethod.tsx:30`);
`refreshChannels` in `useSlackChannels.ts:8-26` clears local caches and calls
`fetchChannels({ search: '', page: 0, pageSize: 999, force: true })`,
causing a GET to
`/api/v1/report/slack_channels/?q=...` with `force=true` in the rison
payload.
4. The request reaches `slack_channels` in `superset/reports/api.py:46-76`,
which enters
the `if force:` block at lines 55-64, clears cache keys, and executes
`cache_channels.apply_async(**get_cache_warmup_options())`. If the Celery
broker or
transport used by `celery_app` (imported from `superset.extensions` in this
module) is
unavailable or misconfigured, `apply_async` raises a Celery/Kombu exception
that is not a
`SupersetException`; since the try/except in `slack_channels` only catches
`SupersetException` at line 74, this exception propagates and produces an
HTTP 500 for the
Slack channels endpoint instead of a controlled 4xx/5xx response.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aaad66e9d4a249229ad1c83bc13dad29&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=aaad66e9d4a249229ad1c83bc13dad29&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/api.py
**Line:** 702:704
**Comment:**
*Api Mismatch: The forced refresh path directly dispatches a Celery
task from the request, but this call can raise broker/transport exceptions when
Celery is unavailable or misconfigured; those exceptions are not converted to a
handled API response here, so the endpoint can fail with a 500 on `force=true`.
Wrap task dispatch failures and continue serving channels (or return a
controlled 422/503) instead of letting the request crash.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=19feea15951f208a27145e365a9563a0c80505014d444d49c0431141a7258223&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=19feea15951f208a27145e365a9563a0c80505014d444d49c0431141a7258223&reaction=dislike'>👎</a>
##########
superset/tasks/slack.py:
##########
@@ -15,29 +15,160 @@
# specific language governing permissions and limitations
# under the License.
import logging
+from typing import Any, Optional
from flask import current_app
-from superset.extensions import celery_app
-from superset.utils.slack import get_channels
+from superset.extensions import cache_manager, celery_app
+from superset.utils.slack import (
+ _fetch_from_api,
+ SLACK_CHANNELS_CACHE_KEY,
+ SLACK_CHANNELS_CONTINUATION_CURSOR_KEY,
+ SlackChannelTypes,
+)
logger = logging.getLogger(__name__)
+# Fallback time limits used when the task is dispatched without explicit
+# options (e.g. a bare Celery beat schedule entry). On-demand callers should
+# pass config-driven limits via ``cache_channels.apply_async`` using
+# ``get_cache_warmup_options`` so that ``SLACK_CACHE_WARMUP_TIMEOUT`` is
honored.
+DEFAULT_CACHE_WARMUP_TIME_LIMIT = 300 # 5 minutes
+DEFAULT_CACHE_WARMUP_SOFT_TIME_LIMIT = int(DEFAULT_CACHE_WARMUP_TIME_LIMIT *
0.8)
-@celery_app.task(name="slack.cache_channels")
+
+def get_cache_warmup_options() -> dict[str, int]:
+ """
+ Build Celery ``apply_async`` options with config-driven time limits.
+
+ Read at dispatch time (inside an application context) rather than at import
+ so operator overrides of ``SLACK_CACHE_WARMUP_TIMEOUT`` are respected. The
+ soft limit is derived as 80% of the hard limit so the task can wind down
+ before Celery force-kills it.
+ """
+ time_limit = current_app.config.get(
+ "SLACK_CACHE_WARMUP_TIMEOUT", DEFAULT_CACHE_WARMUP_TIME_LIMIT
+ )
+ return {
+ "time_limit": time_limit,
+ "soft_time_limit": int(time_limit * 0.8),
+ }
+
+
+@celery_app.task(
+ name="slack.cache_channels",
+ time_limit=DEFAULT_CACHE_WARMUP_TIME_LIMIT,
+ soft_time_limit=DEFAULT_CACHE_WARMUP_SOFT_TIME_LIMIT,
+)
def cache_channels() -> None:
+ """
+ Celery task to warm up the Slack channels cache.
+
+ This task fetches all Slack channels using pagination and stores them in
cache.
+
+ Respects the following config variables:
+ - SLACK_ENABLE_CACHING: If False, this task does nothing
+ - SLACK_CACHE_TIMEOUT: How long to keep cached data
+ - SLACK_CACHE_MAX_CHANNELS: Maximum number of channels to cache. When the
+ workspace has more channels than this, the cache is truncated and a
+ continuation cursor is stored so paginated lookups can stream the
+ remainder from the Slack API.
+ """
+ enable_caching = current_app.config.get("SLACK_ENABLE_CACHING", True)
+ if not enable_caching:
+ logger.info(
+ "Slack caching disabled (SLACK_ENABLE_CACHING=False), skipping
cache warmup"
+ )
+ return
+
cache_timeout = current_app.config["SLACK_CACHE_TIMEOUT"]
+ max_channels = current_app.config.get("SLACK_CACHE_MAX_CHANNELS", 20000)
retry_count = current_app.config.get("SLACK_API_RATE_LIMIT_RETRY_COUNT", 2)
logger.info(
"Starting Slack channels cache warm-up task "
- "(cache_timeout=%ds, retry_count=%d)",
+ "(cache_timeout=%ds, max_channels=%d, retry_count=%d)",
cache_timeout,
+ max_channels,
retry_count,
)
try:
- get_channels(force=True, cache_timeout=cache_timeout)
+ all_channels: list[Any] = []
+ cursor: Optional[str] = None
+ page_count = 0
+ # When the workspace exceeds ``max_channels`` we cache the first
+ # ``max_channels`` and remember the Slack cursor for the rest so that
+ # in-memory cache pagination can fall back to the API at the boundary.
+ continuation_cursor: Optional[str] = None
+
+ # Fetch directly from the Slack API (bypassing the cache-aware
Review Comment:
**Suggestion:** The continuation cursor is taken from the page *after* the
page that overflowed the cache cap, and then the overflowing page is truncated.
When `SLACK_CACHE_MAX_CHANNELS` is not aligned to Slack page boundaries,
channels between the cap and the next page cursor are permanently skipped.
Store/handle in-page overflow (or resume from the current page with an offset)
so no channels are dropped. [cache]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Some Slack channels never visible in recipient selector.
- ⚠️ Large workspaces lose mid-range channels behind cache cap.
- ⚠️ Users cannot target alerts to skipped Slack channels.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure Superset with Slack caching enabled; the Celery task
`cache_channels` is
defined in `superset/tasks/slack.py:19-24` and uses
`SLACK_CACHE_MAX_CHANNELS` (default
20000) at line 45. Use a Slack workspace whose total channel count exceeds
`SLACK_CACHE_MAX_CHANNELS`, which is exactly the large-workspace scenario
this PR targets.
2. Trigger the `slack.cache_channels` Celery task (either via periodic beat
or via the
backend `force` refresh path in `superset/reports/api.py:55-64`). Inside
`cache_channels`,
the loop at `superset/tasks/slack.py:69-103` repeatedly calls
`_fetch_from_api` (imported
from `superset/utils/slack.py`) with `limit=999`, appending
`result["result"]` to
`all_channels` at lines 80-82 and updating `cursor =
result.get("next_cursor")` at line
90.
3. Once `len(all_channels) >= max_channels` while `result.get("has_more")`
is still true,
the branch at `superset/tasks/slack.py:95-103` executes: it sets
`continuation_cursor =
cursor` (the Slack `next_cursor` for the just-fetched page) and breaks the
loop.
Immediately afterward, `all_channels = all_channels[:max_channels]` at line
105 truncates
the combined list to the first `max_channels` entries, discarding the tail
of the last
fetched page (e.g., with `max_channels=20000`, only the first 20 entries
from page 21 are
cached and the remaining 979 are dropped).
4. Later, when a user paginates Slack channels via the frontend hook
`useSlackChannels`
(`superset-frontend/src/features/alerts/hooks/useSlackChannels.ts:28-35`),
the backend
`get_channels_with_search` in `superset/utils/slack.py:13-38` first serves
pages from
cache using `_fetch_from_cache` (the in-memory paginator at
`superset/utils/slack.py:19-61`). When the client advances past the cached
boundary,
`_fetch_from_cache` detects `offset >= len(channels)` at lines 27-35 and
returns `None`,
causing `get_channels_with_search` to fall back to the API using
`continuation_cursor` at
`superset/utils/slack.py:52-60` (either via the `"api:continue"` cursor path
or the
cache-boundary fallback at lines 2-20 in the 498-577 block). Because
`continuation_cursor`
points to the start of the *next* Slack page after the truncated one, the
dropped tail of
the last cached page is never re-fetched and those channels permanently
disappear from
both the cached pages and the API continuation path, so a slice of valid
Slack channels
cannot be listed or selected as alert recipients.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a1c9a983ea114aa8bb46ebf81c6d2cbb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a1c9a983ea114aa8bb46ebf81c6d2cbb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/tasks/slack.py
**Line:** 80:105
**Comment:**
*Cache: The continuation cursor is taken from the page *after* the page
that overflowed the cache cap, and then the overflowing page is truncated. When
`SLACK_CACHE_MAX_CHANNELS` is not aligned to Slack page boundaries, channels
between the cap and the next page cursor are permanently skipped. Store/handle
in-page overflow (or resume from the current page with an offset) so no
channels are dropped.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=d652913085f80490a1b3993c6cf5f38c56408fe1404088c9279b4e5d4c2a799f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=d652913085f80490a1b3993c6cf5f38c56408fe1404088c9279b4e5d4c2a799f&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
