codeant-ai-for-open-source[bot] commented on code in PR #40957:
URL: https://github.com/apache/superset/pull/40957#discussion_r3418067682
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -708,6 +708,113 @@ class GenerateDashboardResponse(BaseModel):
)
+class UpdateDashboardRequest(BaseModel):
+ """Request schema for partially updating dashboard metadata.
+
+ All fields except ``dashboard_id`` are optional — only the fields that
+ are provided are changed. ``None`` means "leave unchanged"; to clear a
+ text field (e.g. ``certified_by``) pass an empty string.
+ """
+
+ model_config = ConfigDict(populate_by_name=True)
Review Comment:
**Suggestion:** The request model silently ignores unknown fields because
`extra` is not forbidden, but the schema docstring explicitly advertises fields
like `certified_by` that are not actually defined. In practice, clients can
send unsupported update keys (for example `certified_by`, `owners`, or
`color_scheme`) and those inputs are dropped without validation, causing no-op
updates and contract-breaking behavior. Add the missing supported fields or set
`extra="forbid"` so unsupported keys fail fast instead of being ignored. [api
mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP update_dashboard silently drops certified_by, certification_details
updates.
- ⚠️ MCP update_dashboard silently ignores owners/roles/color_scheme
convenience fields.
- ⚠️ MCP clients see successful calls with no corresponding metadata change.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Locate the MCP tool entrypoint `update_dashboard` in
`superset/mcp_service/dashboard/tool/update_dashboard.py:227-252`, which is
registered as
an MCP tool and invoked via FastMCP clients (see
`tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py:110-114`
using
`Client.call_tool("update_dashboard", {"request": request})`).
2. Observe the request schema `UpdateDashboardRequest` in
`superset/mcp_service/dashboard/schemas.py:52-121`. Its docstring at `lines
53-57`
describes behavior for clearing a text field using ``certified_by`` as an
example, but the
class only declares fields `dashboard_id`, `dashboard_title`, `slug`,
`published`, `css`,
`tags`, and the three json_metadata convenience fields. There are no fields
for
`certified_by`, `certification_details`, `owners`, `roles`, or
`color_scheme`.
3. Note that `UpdateDashboardRequest` sets `model_config =
ConfigDict(populate_by_name=True)` at `schemas.py:60` and does not set
`extra="forbid"`.
Under Pydantic v2's default behavior, this means unknown input keys are
accepted and
stored as extras instead of raising a `ValidationError`.
4. Follow the update logic in `_build_update_properties` in
`update_dashboard.py:62-107`,
which is the only place that reads fields from `UpdateDashboardRequest`. It
iterates only
over `_DIRECT_FIELDS = ("dashboard_title", "slug", "published", "css",
"tags")` and
`_METADATA_FIELDS = ("cross_filters_enabled", "refresh_frequency",
"filter_bar_orientation")` (see `update_dashboard.py:44-59`), so any extra
keys on the
request (for example `{"dashboard_id": 1, "certified_by": "Alice",
"color_scheme":
"supersetColors"}` passed via `Client.call_tool` as in the tests) are
silently ignored:
Pydantic does not error, but `_build_update_properties` never reads them,
`UpdateDashboardCommand` never sees them, and the dashboard is not updated
for those
fields. This yields a contract-breaking no-op for documented-but-unsupported
inputs
instead of a clear validation failure.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9d230ac455024cc085498882486cccc9&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=9d230ac455024cc085498882486cccc9&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/dashboard/schemas.py
**Line:** 712:719
**Comment:**
*Api Mismatch: The request model silently ignores unknown fields
because `extra` is not forbidden, but the schema docstring explicitly
advertises fields like `certified_by` that are not actually defined. In
practice, clients can send unsupported update keys (for example `certified_by`,
`owners`, or `color_scheme`) and those inputs are dropped without validation,
causing no-op updates and contract-breaking behavior. Add the missing supported
fields or set `extra="forbid"` so unsupported keys fail fast instead of being
ignored.
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%2F40957&comment_hash=d821f2ff1ec72fe4b103472a5c24d97c47016d5e933808273eca0fad6d2b9b6e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=d821f2ff1ec72fe4b103472a5c24d97c47016d5e933808273eca0fad6d2b9b6e&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]
