fix(reports): require a Slack channel id in report recipients - #42850
fix(reports): require a Slack channel id in report recipients#42850AryaKetanShCt wants to merge 5 commits into
Conversation
dosubot
Bot
added
alert-reports
Code Review Agent Run #9bc9d3Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if data.get("type") not in ( | ||
| ReportRecipientType.SLACK.value, | ||
| ReportRecipientType.SLACKV2.value, | ||
| ): | ||
| return |
There was a problem hiding this comment.
Suggestion: The new validator rejects existing Slack V1 targets such as #general and channel, so the unchanged schema and API tests now fail and existing clients can no longer create or update legacy Slack recipients. This conflicts with the legacy notification path, which still accepts channel names and resolves them during the Slack V1-to-V2 upgrade. Either limit this validation to SlackV2, or migrate/resolve legacy names before applying the ID-only validation and update the affected tests and API contract together. [api mismatch]
Severity Level: Major ⚠️
- ❌ Legacy Slack API creation rejects previously supported channel names.
- ❌ Existing clients cannot update name-based Slack recipients.
- ⚠️ Runtime migration remains unreachable for newly submitted names.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/reports/schemas.py
**Line:** 183:187
**Comment:**
*Api Mismatch: The new validator rejects existing Slack V1 targets such as `#general` and `channel`, so the unchanged schema and API tests now fail and existing clients can no longer create or update legacy Slack recipients. This conflicts with the legacy notification path, which still accepts channel names and resolves them during the Slack V1-to-V2 upgrade. Either limit this validation to `SlackV2`, or migrate/resolve legacy names before applying the ID-only validation and update the affected tests and API contract together.
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 fixThere was a problem hiding this comment.
Good catch, and correct on all three counts. Fixed in the latest commits by limiting the validation to SlackV2.
Verifying before changing it, the deprecated v1 path really does accept names:
SlackNotification._get_channel()returns the raw target and sends viafiles_upload(channels=...)/chat_postMessage(channel=...), both of which resolve a channel name.slack.pysays so itself: "existing v1 recipients are auto-upgraded to SlackV2 on first send viaupdate_report_schedule_slack_v2". Rejecting names at creation would have blocked the input that upgrade path exists to convert.
And it would have broken existing tests, which I should have run against before opening:
tests/unit_tests/reports/schemas_test.py::test_report_recipient_schema_slack_skips_email_validationloads{"type": "Slack", "target": "#general"}and asserts success.tests/integration_tests/reports/api_tests.pycreatesReportRecipientType.SLACKwith{"target": "channel"}in three places.
The validator now returns early for anything that is not SlackV2, with a comment recording why. My test asserting v1 rejection is replaced by one asserting v1 acceptance, so the carve-out is pinned rather than incidental.
Re-checked against the real schema, loaded in a running instance:
| type | target | result |
|---|---|---|
| SlackV2 | C08CSCSDCSY |
accepted |
| SlackV2 | data-alerts |
rejected |
| SlackV2 | C08CSCSDCSY,data-alerts |
rejected |
| Slack | #general |
accepted |
| Slack | channel |
accepted |
| Slack | C08CSCSDCSY |
accepted |
a@b.com |
accepted |
This also lines up with v1 being removed next major: the validation applies only to the type that survives, and v1 recipients keep flowing through the auto-upgrade untouched.
|
The flagged issue is correct. The new validator in To implement this fix, update the @validates_schema
def validate_slack_recipients(self, data: dict[str, Any], **kwargs: Any) -> None:
if data.get("type") != ReportRecipientType.SLACKV2.value:
return
target = ((data.get("recipient_config_json") or {}).get("target") or "").strip()
# ... (rest of the validation logic)Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset/reports/schemas.py |
Code Review Agent Run #fa5f18Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42850 +/- ##
==========================================
- Coverage 65.73% 65.73% -0.01%
==========================================
Files 2843 2843
Lines 162659 162678 +19
Branches 37239 37242 +3
==========================================
+ Hits 106926 106929 +3
- Misses 53641 53655 +14
- Partials 2092 2094 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Addressed the coverage report as well. Patch coverage was 33% because the flags Codecov aggregates ( Added
That last case is worth having in the integration suite specifically, since three existing tests in this file already create v1 recipients with The unit tests stay as they are — they cover the branch logic cheaply; the integration test covers the wiring and the coverage flags. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #9d490bActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
1 participant
SUMMARY
A report or alert saved with a Slack channel name rather than a channel id validates cleanly, runs on schedule, and then never delivers.
ReportRecipientConfigJSONSchema.targetis a barefields.String(), and the only validator onReportRecipientSchemabegins:So email addresses are regex-checked and Slack targets are not checked at all.
POST /api/v1/report/accepts{"type": "SlackV2", "recipient_config_json": {"target": "data-alerts"}}and returns 201.But
SlackV2Notification.send()passes the target straight tofiles_upload_v2(channel=...), which requires a channel id and answersinvalid_argumentsfor a name. The only signal is an error notification to the report's owner, long after the alert was created, and only if someone reads it.The UI is unaffected because its picker submits ids from
/api/v1/report/slack_channels/. The gap is the API.This is not theoretical. On one instance 46 recipients had accumulated a channel name, 21 of them on active alerts, the oldest from 2023. Of the 19 distinct names, 5 still matched a live channel, 2 matched archived ones and the rest matched nothing at all, so most could not be repaired even by hand and 15 alerts had to be switched off.
FIX
Validate SlackV2 targets the way Email targets are already validated: every comma, semicolon or whitespace separated part must look like a channel id (
^[CGD][A-Z0-9]{6,}$). The error names the offending part and says where to find the id.The deprecated Slack v1 type is deliberately left alone. v1 sends with
files_upload/chat_postMessage, both of which resolve a channel name, andslack.pynotes that existing v1 recipients are auto-upgraded to SlackV2 on first send viaupdate_report_schedule_slack_v2. Rejecting names for v1 would break that upgrade path and the v1 contract, and it would fail existing tests that create v1 recipients with names. Since v1 is removed next major, the validation applies only to the type that survives.The message is deliberately explicit about why, because "invalid channel" reads like a typo when the real cause is that names work with Slack's older
chat.postMessageand not with the upload API used for attachments.TESTING INSTRUCTIONS
Six unit tests in
tests/unit_tests/reports/schemas_test.py, following the existing email cases: a single id and multiple ids are accepted; a name, a mixed id-and-name target, and an empty target are rejected; and legacySlackwith a channel name is asserted accepted, pinning the carve-out. The mixed case asserts the message names the bad part and not the good one.Verified against the schema loaded in a running instance:
C08CSCSDCSYdata-alertsC08CSCSDCSY,data-alerts#generalchannela@b.comManually:
POST /api/v1/report/with a SlackV2 recipient whose target is a channel name now returns 400 instead of 201.ADDITIONAL INFORMATION