codeant-ai-for-open-source[bot] commented on code in PR #40359: URL: https://github.com/apache/superset/pull/40359#discussion_r3326678447
########## superset/mcp_service/report/schemas.py: ########## @@ -0,0 +1,340 @@ +# 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 pydantic import BaseModel, ConfigDict, Field, field_validator + + +class RecipientConfig(BaseModel): + """Configuration for a single report recipient.""" + + model_config = ConfigDict(populate_by_name=True) + + type: str = Field( + ..., + description=( + "Recipient channel type. One of: 'Email', 'Slack', 'SlackV2', 'Webhook'." + ), + ) + target: str = Field( + ..., + description=( + "Destination for the notification. " + "For Email: an email address. " + "For Slack/SlackV2: a channel name or ID. " + "For Webhook: the URL." + ), + ) + + @field_validator("type") + @classmethod + def type_must_be_valid(cls, v: str) -> str: + if v not in (valid_types := {"Email", "Slack", "SlackV2", "Webhook"}): + raise ValueError( + f"Invalid recipient type {v!r}. Must be one of: {sorted(valid_types)}" + ) + return v + + @field_validator("target") + @classmethod + def target_must_not_be_empty(cls, v: str) -> str: + if not v.strip(): + raise ValueError("target must not be empty") + return v.strip() + + +class CreateReportRequest(BaseModel): + """Request schema for create_report.""" + + model_config = ConfigDict(populate_by_name=True) + + name: str = Field( + ..., + min_length=1, + max_length=150, + description="Name for the new report or alert schedule.", + ) + type: str = Field( + ..., + description=( + "Schedule type: 'Report' to deliver a snapshot, " + "'Alert' to notify on a SQL condition." + ), + ) + crontab: str = Field( + ..., + description=( + "Cron expression defining the execution schedule. " + "Examples: '0 9 * * 1' (every Monday at 9 AM), " + "'0 8 * * 1-5' (weekdays at 8 AM)." + ), + ) + description: str | None = Field( + None, + description="Optional human-readable description of the schedule.", + ) + active: bool = Field( + True, + description="Whether the schedule is active immediately after creation.", + ) + timezone: str | None = Field( + None, + description=( + "Timezone for interpreting the cron schedule " + "(e.g., 'America/New_York', 'UTC'). Defaults to server timezone." + ), + ) Review Comment: **Suggestion:** `timezone` is accepted as an arbitrary string in both create and update schemas, unlike the core report API schemas which restrict to known timezones. Invalid values silently fall back to UTC during schedule window computation, causing schedules to fire at the wrong local time without clear user feedback. Validate `timezone` against the supported timezone list. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Invalid timezone values cause schedules to run in UTC. - ⚠️ Alerts/reports may fire at unexpected local times. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Call the MCP `create_report` tool (`superset/mcp_service/report/tool/create_report.py:43-46`) from a client as in `tests/unit_tests/mcp_service/report/tool/test_create_report.py:127-161`, but set `timezone` in the request to an invalid value such as `"Mars/Phobos"` while keeping other fields valid. 2. The `CreateReportRequest` schema (`superset/mcp_service/report/schemas.py:59-107`) defines `timezone: str | None = Field(None, ...)` at lines 93-99 with no validator, so Pydantic accepts any arbitrary string; `request.timezone` becomes `"Mars/Phobos"`, and `create_report` includes it in the `optional` dict as `"timezone": request.timezone` (create_report.py:106-113) before calling `CreateReportScheduleCommand(properties).run()` (create_report.py:116-118), which ultimately persists a `ReportSchedule` row whose `timezone` column (`superset/reports/models.py:139`) is set to `"Mars/Phobos"`. `UpdateReportRequest` has the same unvalidated `timezone` field at `superset/mcp_service/report/schemas.py:241-247`, and `update_report` similarly forwards `request.timezone` into its `properties` dict at `superset/mcp_service/report/tool/update_report.py:94-104`. 3. When the Celery beat scheduler runs (`superset/tasks/scheduler.py:80-20`), it loads active schedules via `ReportScheduleDAO.find_active()` and, for each `active_schedule`, passes `active_schedule.crontab` and `active_schedule.timezone` to `cron_schedule_window(triggered_at, active_schedule.crontab, active_schedule.timezone)` (`superset/tasks/scheduler.py:16-19; superset/tasks/cron_util.py:1-3`). 4. `cron_schedule_window` (`superset/tasks/cron_util.py:1-10`) attempts to resolve the timezone with `tz = pytz_timezone(timezone)` at line 5 and catches `UnknownTimeZoneError`; for the invalid `"Mars/Phobos"` value this exception is raised, the except block logs a warning and falls back to `tz = pytz_timezone("UTC")` at lines 6-10, so all subsequent schedule window calculations (cron_util.py:12-21) use UTC instead of the user-supplied timezone, causing alerts/reports created or updated via these MCP schemas with invalid `timezone` strings to fire according to UTC rather than the intended local timezone. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c964704f1c0249e594147c519fe6c080&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c964704f1c0249e594147c519fe6c080&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/mcp_service/report/schemas.py **Line:** 93:99 **Comment:** *Logic Error: `timezone` is accepted as an arbitrary string in both create and update schemas, unlike the core report API schemas which restrict to known timezones. Invalid values silently fall back to UTC during schedule window computation, causing schedules to fire at the wrong local time without clear user feedback. Validate `timezone` against the supported timezone list. 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%2F40359&comment_hash=4c945f9572538c37c30434ba8c2cd5654d124ea28c92ed29372e755ad7e30993&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40359&comment_hash=4c945f9572538c37c30434ba8c2cd5654d124ea28c92ed29372e755ad7e30993&reaction=dislike'>👎</a> ########## superset/mcp_service/report/schemas.py: ########## @@ -0,0 +1,340 @@ +# 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 pydantic import BaseModel, ConfigDict, Field, field_validator + + +class RecipientConfig(BaseModel): + """Configuration for a single report recipient.""" + + model_config = ConfigDict(populate_by_name=True) + + type: str = Field( + ..., + description=( + "Recipient channel type. One of: 'Email', 'Slack', 'SlackV2', 'Webhook'." + ), + ) + target: str = Field( + ..., + description=( + "Destination for the notification. " + "For Email: an email address. " + "For Slack/SlackV2: a channel name or ID. " + "For Webhook: the URL." + ), + ) + + @field_validator("type") + @classmethod + def type_must_be_valid(cls, v: str) -> str: + if v not in (valid_types := {"Email", "Slack", "SlackV2", "Webhook"}): + raise ValueError( + f"Invalid recipient type {v!r}. Must be one of: {sorted(valid_types)}" + ) + return v + + @field_validator("target") + @classmethod + def target_must_not_be_empty(cls, v: str) -> str: + if not v.strip(): + raise ValueError("target must not be empty") + return v.strip() + + +class CreateReportRequest(BaseModel): + """Request schema for create_report.""" + + model_config = ConfigDict(populate_by_name=True) + + name: str = Field( + ..., + min_length=1, + max_length=150, + description="Name for the new report or alert schedule.", + ) + type: str = Field( + ..., + description=( + "Schedule type: 'Report' to deliver a snapshot, " + "'Alert' to notify on a SQL condition." + ), + ) + crontab: str = Field( + ..., + description=( + "Cron expression defining the execution schedule. " + "Examples: '0 9 * * 1' (every Monday at 9 AM), " + "'0 8 * * 1-5' (weekdays at 8 AM)." + ), + ) + description: str | None = Field( + None, + description="Optional human-readable description of the schedule.", + ) + active: bool = Field( + True, + description="Whether the schedule is active immediately after creation.", + ) + timezone: str | None = Field( + None, + description=( + "Timezone for interpreting the cron schedule " + "(e.g., 'America/New_York', 'UTC'). Defaults to server timezone." + ), + ) + recipients: list[RecipientConfig] = Field( + default_factory=list, + description=( + "List of notification recipients. " + "For reports/alerts created via this tool, provide at least one recipient. " + "Example: [{'type': 'Email', 'target': '[email protected]'}]" + ), + ) Review Comment: **Suggestion:** `create_report` currently allows an empty `recipients` list because the field defaults to `[]` with no minimum length check. Since this tool always creates schedules with `ReportCreationMethod.ALERTS_REPORTS` (which does not auto-populate recipients), this can create schedules that execute but never notify anyone. Enforce at least one recipient at schema level for create requests. [incomplete implementation] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ MCP create_report can persist schedules without recipients. - ⚠️ Executed alerts/reports silently produce no notifications. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Invoke the MCP tool `create_report` defined in `superset/mcp_service/report/tool/create_report.py:43-46` from an MCP client (pattern shown in `tests/unit_tests/mcp_service/report/tool/test_create_report.py:127-161`), but omit the `recipients` field from the JSON payload or set `"recipients": []`. 2. The payload is parsed into `CreateReportRequest` (`superset/mcp_service/report/schemas.py:59-107`), where `recipients` is declared with `Field(default_factory=list, ...)` at lines 100-107 and has no length validator, so Pydantic accepts the request and sets `request.recipients` to an empty list with no validation error. 3. Inside `create_report` (`superset/mcp_service/report/tool/create_report.py:83-95`), the `recipient_type_map`/list comprehension iterates over `request.recipients`; for an empty list this yields `recipients = []`, which is then included in the `properties` dict as `"recipients": recipients` at lines 97-104 before calling `CreateReportScheduleCommand(properties).run()` (`superset/commands/report/create.py:49-56`). 4. `CreateReportScheduleCommand.validate` (`superset/commands/report/create.py:87-164`) calls `_populate_recipients` at lines 58-86, which only auto-fills recipients for `ReportCreationMethod.CHARTS` and `DASHBOARDS`; for the MCP path the `creation_method` is set to `ReportCreationMethod.ALERTS_REPORTS` in `create_report` at line 102, so `_populate_recipients` leaves the empty list untouched, the schedule is persisted via `ReportScheduleDAO.create` (create.py:56), and the Celery scheduler in `superset/tasks/scheduler.py:16-20` later executes this schedule with no `ReportRecipients`, resulting in runs that never send any notifications. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=eb5d43138e8848e18d00fe525c3ef726&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=eb5d43138e8848e18d00fe525c3ef726&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/mcp_service/report/schemas.py **Line:** 100:107 **Comment:** *Incomplete Implementation: `create_report` currently allows an empty `recipients` list because the field defaults to `[]` with no minimum length check. Since this tool always creates schedules with `ReportCreationMethod.ALERTS_REPORTS` (which does not auto-populate recipients), this can create schedules that execute but never notify anyone. Enforce at least one recipient at schema level for create requests. 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%2F40359&comment_hash=9a8459c636bc0719df08616b639de72e60932ff4503b34ddd5470eb89125b412&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40359&comment_hash=9a8459c636bc0719df08616b639de72e60932ff4503b34ddd5470eb89125b412&reaction=dislike'>👎</a> ########## superset/mcp_service/report/schemas.py: ########## @@ -0,0 +1,340 @@ +# 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 pydantic import BaseModel, ConfigDict, Field, field_validator + + +class RecipientConfig(BaseModel): + """Configuration for a single report recipient.""" + + model_config = ConfigDict(populate_by_name=True) + + type: str = Field( + ..., + description=( + "Recipient channel type. One of: 'Email', 'Slack', 'SlackV2', 'Webhook'." + ), + ) + target: str = Field( + ..., + description=( + "Destination for the notification. " + "For Email: an email address. " + "For Slack/SlackV2: a channel name or ID. " + "For Webhook: the URL." + ), + ) + + @field_validator("type") + @classmethod + def type_must_be_valid(cls, v: str) -> str: + if v not in (valid_types := {"Email", "Slack", "SlackV2", "Webhook"}): + raise ValueError( + f"Invalid recipient type {v!r}. Must be one of: {sorted(valid_types)}" + ) + return v + + @field_validator("target") + @classmethod + def target_must_not_be_empty(cls, v: str) -> str: + if not v.strip(): + raise ValueError("target must not be empty") + return v.strip() + + +class CreateReportRequest(BaseModel): + """Request schema for create_report.""" + + model_config = ConfigDict(populate_by_name=True) + + name: str = Field( + ..., + min_length=1, + max_length=150, + description="Name for the new report or alert schedule.", + ) + type: str = Field( + ..., + description=( + "Schedule type: 'Report' to deliver a snapshot, " + "'Alert' to notify on a SQL condition." + ), + ) + crontab: str = Field( + ..., + description=( + "Cron expression defining the execution schedule. " + "Examples: '0 9 * * 1' (every Monday at 9 AM), " + "'0 8 * * 1-5' (weekdays at 8 AM)." + ), + ) + description: str | None = Field( + None, + description="Optional human-readable description of the schedule.", + ) + active: bool = Field( + True, + description="Whether the schedule is active immediately after creation.", + ) + timezone: str | None = Field( + None, + description=( + "Timezone for interpreting the cron schedule " + "(e.g., 'America/New_York', 'UTC'). Defaults to server timezone." + ), + ) + recipients: list[RecipientConfig] = Field( + default_factory=list, + description=( + "List of notification recipients. " + "For reports/alerts created via this tool, provide at least one recipient. " + "Example: [{'type': 'Email', 'target': '[email protected]'}]" + ), + ) + dashboard_id: int | None = Field( + None, + description=( + "ID of the dashboard to attach this report to. " + "Use list_dashboards to find valid IDs. " + "Provide either dashboard_id or chart_id, not both." + ), + ) + chart_id: int | None = Field( + None, + description=( + "ID of the chart to attach this report to. " + "Use list_charts to find valid IDs. " + "Provide either chart_id or dashboard_id, not both." + ), + ) + database_id: int | None = Field( + None, + description=( + "ID of the database connection for evaluating the alert condition. " + "Required when type is 'Alert'. Use list_databases to find valid IDs." + ), + ) + sql: str | None = Field( + None, + description=( + "SQL query whose result is evaluated as the alert condition. " + "Used when type is 'Alert'." + ), + ) + + @field_validator("type") + @classmethod + def type_must_be_valid(cls, v: str) -> str: + if v not in (valid_types := {"Report", "Alert"}): + raise ValueError( + f"Invalid type {v!r}. Must be one of: {sorted(valid_types)}" + ) + return v + + @field_validator("name") + @classmethod + def name_must_not_be_empty(cls, v: str) -> str: + if not v.strip(): + raise ValueError("name must not be empty") + return v.strip() + + @field_validator("crontab") + @classmethod + def crontab_must_not_be_empty(cls, v: str) -> str: + if not v.strip(): + raise ValueError("crontab must not be empty") + return v.strip() Review Comment: **Suggestion:** The create request only checks that `crontab` is non-empty, so syntactically invalid cron strings pass schema validation and fail later inside command execution (`croniter`), surfacing as create-failed instead of a proper validation error. Add cron syntax validation in the schema to fail fast with a deterministic input error. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Invalid crontab strings cause generic create_failed errors. - ⚠️ Users cannot distinguish cron typos from server issues. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. From an MCP client (pattern in `tests/unit_tests/mcp_service/report/tool/test_create_report.py:127-161`), construct a `CreateReportRequest` with an invalid cron string such as `crontab="not-a-cron"` and otherwise valid fields, and call the `create_report` tool (`superset/mcp_service/report/tool/create_report.py:43-46`). 2. The request is validated by `CreateReportRequest` (`superset/mcp_service/report/schemas.py:59-160`); the `crontab_must_not_be_empty` validator at lines 155-160 only strips whitespace and checks for emptiness, so `crontab="not-a-cron"` passes schema validation and `request.crontab` remains the invalid cron expression. 3. In `create_report` (`superset/mcp_service/report/tool/create_report.py:97-104`), the `properties` dict is built with `"crontab": request.crontab` and passed into `CreateReportScheduleCommand(properties).run()` (create_report.py:116-118), so `CreateReportScheduleCommand` sees the malformed cron string. 4. `CreateReportScheduleCommand.validate` (`superset/commands/report/create.py:87-164`) reads `cron_schedule = self._properties["crontab"]` (line 96) and passes it to `validate_report_frequency` (`superset/commands/report/base.py:109-151`), which calls `schedule = croniter(cron_schedule)` at line 142; `croniter("not-a-cron")` raises `ValueError` (not `ValidationError`), so it is not caught by the `except ValidationError` block at create.py:130-137, bubbles out of `run()` (create.py:53-56), and is wrapped by the `@transaction(on_error=partial(on_error, reraise=ReportScheduleCreateFailedError))` decorator into a `ReportScheduleCreateFailedError` that is caught in `create_report` at lines 147-157 and returned as a generic create-failed error instead of a deterministic schema validation error. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9ce2ec7cfec2494d8c613b0a415a01ca&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9ce2ec7cfec2494d8c613b0a415a01ca&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/mcp_service/report/schemas.py **Line:** 155:160 **Comment:** *Incomplete Implementation: The create request only checks that `crontab` is non-empty, so syntactically invalid cron strings pass schema validation and fail later inside command execution (`croniter`), surfacing as create-failed instead of a proper validation error. Add cron syntax validation in the schema to fail fast with a deterministic input error. 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%2F40359&comment_hash=4b6b39c3c1b83f2a744425460a82fd35d6049d98754fdde87e0e8714f86d6bef&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40359&comment_hash=4b6b39c3c1b83f2a744425460a82fd35d6049d98754fdde87e0e8714f86d6bef&reaction=dislike'>👎</a> ########## superset/mcp_service/report/schemas.py: ########## @@ -0,0 +1,340 @@ +# 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 pydantic import BaseModel, ConfigDict, Field, field_validator + + +class RecipientConfig(BaseModel): + """Configuration for a single report recipient.""" + + model_config = ConfigDict(populate_by_name=True) + + type: str = Field( + ..., + description=( + "Recipient channel type. One of: 'Email', 'Slack', 'SlackV2', 'Webhook'." + ), + ) + target: str = Field( + ..., + description=( + "Destination for the notification. " + "For Email: an email address. " + "For Slack/SlackV2: a channel name or ID. " + "For Webhook: the URL." + ), + ) + + @field_validator("type") + @classmethod + def type_must_be_valid(cls, v: str) -> str: + if v not in (valid_types := {"Email", "Slack", "SlackV2", "Webhook"}): + raise ValueError( + f"Invalid recipient type {v!r}. Must be one of: {sorted(valid_types)}" + ) + return v + + @field_validator("target") + @classmethod + def target_must_not_be_empty(cls, v: str) -> str: + if not v.strip(): + raise ValueError("target must not be empty") + return v.strip() + + +class CreateReportRequest(BaseModel): + """Request schema for create_report.""" + + model_config = ConfigDict(populate_by_name=True) + + name: str = Field( + ..., + min_length=1, + max_length=150, + description="Name for the new report or alert schedule.", + ) + type: str = Field( + ..., + description=( + "Schedule type: 'Report' to deliver a snapshot, " + "'Alert' to notify on a SQL condition." + ), + ) + crontab: str = Field( + ..., + description=( + "Cron expression defining the execution schedule. " + "Examples: '0 9 * * 1' (every Monday at 9 AM), " + "'0 8 * * 1-5' (weekdays at 8 AM)." + ), + ) + description: str | None = Field( + None, + description="Optional human-readable description of the schedule.", + ) + active: bool = Field( + True, + description="Whether the schedule is active immediately after creation.", + ) + timezone: str | None = Field( + None, + description=( + "Timezone for interpreting the cron schedule " + "(e.g., 'America/New_York', 'UTC'). Defaults to server timezone." + ), + ) + recipients: list[RecipientConfig] = Field( + default_factory=list, + description=( + "List of notification recipients. " + "For reports/alerts created via this tool, provide at least one recipient. " + "Example: [{'type': 'Email', 'target': '[email protected]'}]" + ), + ) + dashboard_id: int | None = Field( + None, + description=( + "ID of the dashboard to attach this report to. " + "Use list_dashboards to find valid IDs. " + "Provide either dashboard_id or chart_id, not both." + ), + ) + chart_id: int | None = Field( + None, + description=( + "ID of the chart to attach this report to. " + "Use list_charts to find valid IDs. " + "Provide either chart_id or dashboard_id, not both." + ), + ) + database_id: int | None = Field( + None, + description=( + "ID of the database connection for evaluating the alert condition. " + "Required when type is 'Alert'. Use list_databases to find valid IDs." + ), + ) + sql: str | None = Field( + None, + description=( + "SQL query whose result is evaluated as the alert condition. " + "Used when type is 'Alert'." + ), + ) + + @field_validator("type") + @classmethod + def type_must_be_valid(cls, v: str) -> str: + if v not in (valid_types := {"Report", "Alert"}): + raise ValueError( + f"Invalid type {v!r}. Must be one of: {sorted(valid_types)}" + ) + return v + + @field_validator("name") + @classmethod + def name_must_not_be_empty(cls, v: str) -> str: + if not v.strip(): + raise ValueError("name must not be empty") + return v.strip() + + @field_validator("crontab") + @classmethod + def crontab_must_not_be_empty(cls, v: str) -> str: + if not v.strip(): + raise ValueError("crontab must not be empty") + return v.strip() + + +class CreateReportResponse(BaseModel): + """Response schema for create_report.""" + + id: int | None = Field( + None, + description="ID of the created report schedule. None if creation failed.", + ) + name: str | None = Field( + None, + description="Name of the created schedule.", + ) + type: str | None = Field( + None, + description="Type of the schedule ('Report' or 'Alert').", + ) + crontab: str | None = Field( + None, + description="Cron expression for the schedule.", + ) + active: bool | None = Field( + None, + description="Whether the schedule is active.", + ) + url: str | None = Field( + None, + description=( + "URL to manage the report schedule in Superset. None if creation failed." + ), + ) + error: str | None = Field( + None, + description="Error message if creation failed, otherwise null.", + ) + + +class UpdateReportRequest(BaseModel): + """Request schema for update_report. All fields except id are optional.""" + + model_config = ConfigDict(populate_by_name=True) + + id: int = Field( + ..., + description=( + "ID of the report schedule to update. " + "Use list_reports or the id returned by create_report." + ), + ) + name: str | None = Field( + None, + max_length=150, + description="New name for the schedule. Omit to keep the current name.", + ) + type: str | None = Field( + None, + description=( + "Change the schedule type: 'Report' or 'Alert'. " + "Omit to keep the current type." + ), + ) + crontab: str | None = Field( + None, + description=( + "New cron expression for the schedule " + "(e.g., '0 9 * * 1' for every Monday at 9 AM). " + "Omit to keep the current schedule." + ), + ) + description: str | None = Field( + None, + description="New description. Omit to keep the current description.", + ) + active: bool | None = Field( + None, + description=( + "Set to True to enable or False to disable the schedule. " + "Omit to keep the current state." + ), + ) + timezone: str | None = Field( + None, + description=( + "New timezone for interpreting the cron schedule " + "(e.g., 'America/New_York'). Omit to keep the current timezone." + ), + ) + recipients: list[RecipientConfig] | None = Field( + None, + description=( + "Replacement list of notification recipients. " + "Replaces the entire recipient list when provided. " + "Omit to keep the current recipients." + ), + ) + dashboard_id: int | None = Field( + None, + description=( + "ID of the dashboard to attach this schedule to. " + "Omit to keep the current dashboard association." + ), + ) + chart_id: int | None = Field( + None, + description=( + "ID of the chart to attach this schedule to. " + "Omit to keep the current chart association." + ), + ) + database_id: int | None = Field( + None, + description=( + "ID of the database for the alert condition. " + "Required when changing type to 'Alert'." + ), + ) + sql: str | None = Field( + None, + description=( + "New SQL query for the alert condition. Omit to keep the current query." + ), + ) + + @field_validator("type") + @classmethod + def type_must_be_valid(cls, v: str | None) -> str | None: + if v is not None and v not in (valid_types := {"Report", "Alert"}): + raise ValueError( + f"Invalid type {v!r}. Must be one of: {sorted(valid_types)}" + ) + return v + + @field_validator("name") + @classmethod + def name_must_not_be_empty(cls, v: str | None) -> str | None: + if v is not None and not v.strip(): + raise ValueError("name must not be empty") + return v.strip() if v is not None else v + + @field_validator("crontab") + @classmethod + def crontab_must_not_be_empty(cls, v: str | None) -> str | None: + if v is not None and not v.strip(): + raise ValueError("crontab must not be empty") + return v.strip() if v is not None else v Review Comment: **Suggestion:** The update request has the same problem: it only strips `crontab` and does not validate cron syntax, so malformed schedules are accepted and then converted into runtime update failures. Add cron format validation here too so bad inputs are rejected as validation errors before command execution. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Malformed cron in updates yields generic update_failed errors. - ⚠️ Users lack precise feedback on bad cron schedules. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. From an MCP client (pattern shown in `tests/unit_tests/mcp_service/report/tool/test_update_report.py:124-153`), build an `UpdateReportRequest` with a valid `id` and an invalid cron, e.g. `crontab="bad"`, then call the `update_report` tool defined in `superset/mcp_service/report/tool/update_report.py:43-46`. 2. `UpdateReportRequest` (`superset/mcp_service/report/schemas.py:198-305`) validates the payload; the `crontab_must_not_be_empty` validator at lines 300-305 only rejects `None`/whitespace, so `crontab="bad"` is accepted and `request.crontab` holds a syntactically invalid expression. 3. Inside `update_report` (`superset/mcp_service/report/tool/update_report.py:94-104`), `all_props` is assembled with `"crontab": request.crontab`, filtered into `properties`, and passed to `UpdateReportScheduleCommand(request.id, properties).run()` (update_report.py:115-117), so the malformed cron value reaches the command layer. 4. `UpdateReportScheduleCommand.validate` (`superset/commands/report/update.py:66-185`) computes `cron_schedule = self._properties.get("crontab", self._model.crontab)` (line 80) and passes it to `validate_report_frequency` (`superset/commands/report/base.py:109-151`); `croniter("bad")` at base.py:142 raises `ValueError` that is not caught by the `except ValidationError` block at update.py:151-158, causing `run()` (update.py:61-64) to raise and the `@transaction(on_error=partial(on_error, reraise=ReportScheduleUpdateFailedError))` decorator to wrap it as `ReportScheduleUpdateFailedError`, which `update_report` catches at lines 147-152 and returns as a generic update-failed error instead of a clear schema-level cron validation error. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=01ac37ab9beb4fb9b6a2418e7b730899&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=01ac37ab9beb4fb9b6a2418e7b730899&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/mcp_service/report/schemas.py **Line:** 300:305 **Comment:** *Incomplete Implementation: The update request has the same problem: it only strips `crontab` and does not validate cron syntax, so malformed schedules are accepted and then converted into runtime update failures. Add cron format validation here too so bad inputs are rejected as validation errors before command execution. 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%2F40359&comment_hash=a8020d7fef39ee55e60fd8ddd538718c31342a9fd01aed29b8c95ea95d91983e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40359&comment_hash=a8020d7fef39ee55e60fd8ddd538718c31342a9fd01aed29b8c95ea95d91983e&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]
