codeant-ai-for-open-source[bot] commented on code in PR #42074:
URL: https://github.com/apache/superset/pull/42074#discussion_r3584613053
##########
superset/utils/slack.py:
##########
@@ -112,104 +115,305 @@ def get_team_id() -> Optional[str]:
return team_id or None
-def get_channels(
- team_id: Optional[str] = None, **kwargs: Any
-) -> list[SlackChannelSchema]:
- """
- Retrieves a list of all conversations accessible by the bot
- from the Slack API, and caches results (to avoid rate limits).
-
- The Slack API does not provide search so to apply a search use
- get_channels_with_search instead.
-
- :param team_id: workspace (team) ID to target, required for org-scoped
tokens
- on an Enterprise Grid org. Defaults to the configured ``SLACK_TEAM_ID``
- (via get_team_id) when not given. When set it also keys the cache so
that
- 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``).
+def _fetch_channels_without_search(
+ client: WebClient,
+ channel_schema: SlackChannelSchema,
+ types_param: str,
+ cursor: Optional[str],
+ limit: int,
+ cache_full_list: bool = False,
+) -> dict[str, Any]:
+ """Fetch channels without search filtering, paginating for large limits.
+
+ When ``cache_full_list`` is True and the entire workspace is returned in a
+ single page (``has_more`` is False), the complete channel list is written
+ through to the cache so instances without a Celery warmup worker keep the
+ in-memory fast path on subsequent requests. Larger, multi-page workspaces
+ are left to the async warmup task to avoid blocking the request.
"""
- 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: list[SlackChannelSchema] = []
+ slack_cursor = cursor
+ while True:
+ response = client.conversations_list(
+ limit=999,
+ cursor=slack_cursor,
+ exclude_archived=True,
+ types=types_param,
+ )
Review Comment:
**Suggestion:** The new channel-fetching API calls no longer pass `team_id`,
so Enterprise Grid setups with org-scoped tokens can query the wrong workspace
or fail when `SLACK_TEAM_ID` is required. Include the `team_id` from
`get_team_id()` in `conversations_list` calls (as done in the v2 probe) to
preserve workspace-scoped behavior. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Enterprise Grid workspaces may see wrong-channel listings.
- ⚠️ Channel dropdown may fail when team_id required.
- ⚠️ Slack v2 recipient upgrade can resolve channels incorrectly.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure Superset with an Enterprise Grid org-scoped Slack token and set
`SLACK_TEAM_ID` to the desired workspace ID; `get_team_id()` in
`superset/utils/slack.py:101-115` documents that `conversations.list` needs
`team_id` on
Enterprise Grid to scope to a specific workspace.
2. Enable Slack v2 alerts (`ALERT_REPORT_SLACK_V2`) and use the Alerts &
Reports UI to
open the Slack channel dropdown, which issues `GET
/api/v1/report/slack_channels/`; this
hits `ReportScheduleRestApi.slack_channels` in
`superset/reports/api.py:374-453`, which
delegates to `get_channels_with_search(...)` at lines 446-452.
3. With caching cold or when falling back to the API,
`get_channels_with_search` calls
`_fetch_from_api(search_string, types, exact_match, cursor, limit,
cache_full_list)`
(`superset/utils/slack.py:364-400`), which in turn calls
`_fetch_channels_without_search(...)` or `_fetch_channels_with_search(...)`
(`superset/utils/slack.py:118-241`) depending on whether `search_string` is
empty.
4. Both `_fetch_channels_without_search` and `_fetch_channels_with_search`
call
`client.conversations_list(limit=999, cursor=slack_cursor,
exclude_archived=True,
types=types_param)` without including `team_id` (lines 138-143 and 196-201),
whereas the
v2 probe `should_use_v2_api()` at `superset/utils/slack.py:137-151`
explicitly passes
`team_id = get_team_id()` to `conversations_list`. This omission means
Enterprise Grid
org-scoped tokens are not scoped to the configured workspace for channel
listing; per the
`get_team_id` docstring, `conversations.list` may return channels from an
unintended
workspace or fail when `team_id` is required, breaking channel discovery for
Slack alerts
in multi-workspace setups.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=66405622b8334b9f8e7ca7ea3906f1c8&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=66405622b8334b9f8e7ca7ea3906f1c8&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/utils/slack.py
**Line:** 138:143
**Comment:**
*Api Mismatch: The new channel-fetching API calls no longer pass
`team_id`, so Enterprise Grid setups with org-scoped tokens can query the wrong
workspace or fail when `SLACK_TEAM_ID` is required. Include the `team_id` from
`get_team_id()` in `conversations_list` calls (as done in the v2 probe) to
preserve workspace-scoped behavior.
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=0331c17281b7fd5455cb17eb3f34b83f5a5e65dcad837f3506de85123296d7a2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=0331c17281b7fd5455cb17eb3f34b83f5a5e65dcad837f3506de85123296d7a2&reaction=dislike'>👎</a>
##########
superset/utils/slack.py:
##########
@@ -112,104 +115,305 @@ def get_team_id() -> Optional[str]:
return team_id or None
-def get_channels(
- team_id: Optional[str] = None, **kwargs: Any
-) -> list[SlackChannelSchema]:
- """
- Retrieves a list of all conversations accessible by the bot
- from the Slack API, and caches results (to avoid rate limits).
-
- The Slack API does not provide search so to apply a search use
- get_channels_with_search instead.
-
- :param team_id: workspace (team) ID to target, required for org-scoped
tokens
- on an Enterprise Grid org. Defaults to the configured ``SLACK_TEAM_ID``
- (via get_team_id) when not given. When set it also keys the cache so
that
- 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``).
+def _fetch_channels_without_search(
+ client: WebClient,
+ channel_schema: SlackChannelSchema,
+ types_param: str,
+ cursor: Optional[str],
+ limit: int,
+ cache_full_list: bool = False,
+) -> dict[str, Any]:
+ """Fetch channels without search filtering, paginating for large limits.
+
+ When ``cache_full_list`` is True and the entire workspace is returned in a
+ single page (``has_more`` is False), the complete channel list is written
+ through to the cache so instances without a Celery warmup worker keep the
+ in-memory fast path on subsequent requests. Larger, multi-page workspaces
+ are left to the async warmup task to avoid blocking the request.
"""
- 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: list[SlackChannelSchema] = []
+ slack_cursor = cursor
+ while True:
+ response = client.conversations_list(
+ limit=999,
+ cursor=slack_cursor,
+ exclude_archived=True,
+ types=types_param,
+ )
-@cache_util.memoized_func(
- key="{cache_key}",
- cache=cache_manager.cache,
-)
-def _get_channels(
- cache_key: str, team_id: Optional[str] = None
-) -> list[SlackChannelSchema]:
- client = get_slack_client()
- channel_schema = SlackChannelSchema()
- channels: list[SlackChannelSchema] = []
- extra_params = {"types": _SLACK_CONVERSATION_TYPES}
- if team_id:
- extra_params["team_id"] = team_id
- cursor = None
- page_count = 0
+ page_channels = [
+ channel_schema.load(channel) for channel in
response.data["channels"]
+ ]
+ channels.extend(page_channels)
- logger.info("Starting Slack channels fetch")
+ slack_cursor = response.data.get("response_metadata",
{}).get("next_cursor")
- try:
- while True:
- page_count += 1
+ if not slack_cursor or len(channels) >= limit:
+ break
- response = client.conversations_list(
- limit=999, cursor=cursor, exclude_archived=True, **extra_params
- )
- page_channels = response.data["channels"]
- channels.extend(channel_schema.load(channel) for channel in
page_channels)
-
- logger.debug(
- "Fetched page %d: %d channels (total: %d)",
- page_count,
- len(page_channels),
- len(channels),
- )
+ has_more = bool(slack_cursor)
+
+ # Write-through cache: only when the full, unfiltered workspace fits in the
+ # pages already fetched (has_more is False). ``channels`` is the complete
+ # list here, so it can back the in-memory search/pagination path.
+ if cache_full_list and not has_more:
+ cache_timeout = app.config["SLACK_CACHE_TIMEOUT"]
+ cache_manager.cache.set(
+ SLACK_CHANNELS_CACHE_KEY, channels, timeout=cache_timeout
+ )
+ cache_manager.cache.delete(SLACK_CHANNELS_CONTINUATION_CURSOR_KEY)
+ logger.info(
+ "Warmed Slack channel cache with %d channels without a Celery "
+ "worker (single-page workspace)",
+ len(channels),
+ )
- cursor = response.data.get("response_metadata",
{}).get("next_cursor")
- if not cursor:
+ return {
+ "result": channels[:limit],
+ "next_cursor": slack_cursor,
+ "has_more": has_more,
+ }
+
+
+def _fetch_channels_with_search(
+ client: WebClient,
+ channel_schema: SlackChannelSchema,
+ types_param: str,
+ search_string: str,
+ exact_match: bool,
+ cursor: Optional[str],
+ limit: int,
+) -> dict[str, Any]:
+ """Fetch channels with search filtering, streaming through pages."""
+ matches: list[SlackChannelSchema] = []
+ slack_cursor = cursor
+ search_terms = [
+ term.strip().lower() for term in search_string.split(",") if
term.strip()
+ ]
+
+ while len(matches) < limit:
+ response = client.conversations_list(
+ limit=999,
+ cursor=slack_cursor,
+ exclude_archived=True,
+ types=types_param,
+ )
+
+ for channel_data in response.data["channels"]:
+ channel_name_lower = channel_data["name"].lower()
+ channel_id_lower = channel_data["id"].lower()
+
+ is_match = False
+ for search_term in search_terms:
+ if exact_match:
+ if (
+ search_term == channel_name_lower
+ or search_term == channel_id_lower
+ ):
+ is_match = True
+ break
+ else:
+ if (
+ search_term in channel_name_lower
+ or search_term in channel_id_lower
+ ):
+ is_match = True
+ break
+
+ if is_match:
+ channel = channel_schema.load(channel_data)
+ matches.append(channel)
+
+ if len(matches) >= limit:
break
+ slack_cursor = response.data.get("response_metadata",
{}).get("next_cursor")
Review Comment:
**Suggestion:** The search pagination path stops scanning a Slack page as
soon as `limit` matches are found, then stores the Slack `next_cursor` for the
following page; this drops any additional matches later in the current page and
makes them permanently unreachable. Continue scanning the full current Slack
page (or maintain an internal offset cursor) before moving to the next Slack
cursor so matched channels are not skipped. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Channel search omits some matches on later page entries.
- ⚠️ Users cannot page through all search results.
- ⚠️ Large workspaces see incomplete search-resolved channel lists.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with Slack v2 enabled and Slack channels caching either
cold or
disabled; the channel search endpoint is
`ReportScheduleRestApi.slack_channels` in
`superset/reports/api.py:374-453`.
2. Issue a search request with a limit lower than the number of matches in
the first Slack
page, e.g. `GET
/api/v1/report/slack_channels/?q=(search_string:"team",limit:50)`; in
`slack_channels` at `superset/reports/api.py:426-452`, this calls
`get_channels_with_search(search_string=search_string, types=types,
exact_match=exact_match, cursor=cursor, limit=limit)`.
3. In `get_channels_with_search` (`superset/utils/slack.py:32-129`), because
`search_string` is non-empty, the API path `_fetch_from_api(search_string,
types,
exact_match, cursor, limit, cache_full_list)` is used
(`superset/utils/slack.py:364-400`),
which dispatches to `_fetch_channels_with_search(client, channel_schema,
types_param,
search_string, exact_match, cursor, limit)`
(`superset/utils/slack.py:179-241`).
4. Inside `_fetch_channels_with_search`, the loop `while len(matches) <
limit:` (line 195)
calls `client.conversations_list(limit=999, cursor=slack_cursor,
exclude_archived=True,
types=types_param)` (lines 196-201) and iterates
`response.data["channels"]`. As soon as
`len(matches) >= limit`, it breaks the inner `for` loop (lines 228-229) but
still sets
`slack_cursor` to the Slack `next_cursor` for the following page (line 231).
Since the
outer `while` then exits (condition `len(matches) < limit` fails),
`_fetch_channels_with_search` returns only the first `limit` matches and a
`next_cursor`
that starts after the current Slack page. Any additional matches later in
that same page
are never scanned and become unreachable even if the caller uses the returned
`next_cursor` to paginate, causing some valid search results to be skipped.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6c8785944cfc43e8a3b75ce3708060dc&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=6c8785944cfc43e8a3b75ce3708060dc&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/utils/slack.py
**Line:** 228:231
**Comment:**
*Logic Error: The search pagination path stops scanning a Slack page as
soon as `limit` matches are found, then stores the Slack `next_cursor` for the
following page; this drops any additional matches later in the current page and
makes them permanently unreachable. Continue scanning the full current Slack
page (or maintain an internal offset cursor) before moving to the next Slack
cursor so matched channels are not skipped.
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=f07f508059d1f69e51ae6d6fbf1a21434c999faffbf39db1c377c77cd9e8eb8e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42074&comment_hash=f07f508059d1f69e51ae6d6fbf1a21434c999faffbf39db1c377c77cd9e8eb8e&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]
