codeant-ai-for-open-source[bot] commented on code in PR #40957:
URL: https://github.com/apache/superset/pull/40957#discussion_r3482547557
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -858,6 +889,20 @@ def sanitize_dashboard_title(cls, v: str | None) -> str |
None:
v, "Dashboard title", max_length=500, allow_empty=True
)
+ @field_validator("slug")
+ @classmethod
+ def normalize_slug(cls, v: str | None) -> str | None:
+ """Normalize the slug to match the REST DashboardPutSchema contract.
+
+ Mirrors ``BaseDashboardSchema.post_load``: strip, replace spaces with
+ hyphens, and drop characters outside ``[\\w-]`` so the tool cannot
+ persist slugs the REST update path would have cleaned.
+ """
+ if not v:
+ return v
+ v = v.strip().replace(" ", "-")
+ return re.sub(r"[^\w\-]+", "", v)
+
Review Comment:
**Suggestion:** This slug normalizer can turn a non-empty user input into an
empty string (for example, strings containing only stripped characters), which
downstream update logic treats as a slug-clear operation; that can
unintentionally erase an existing slug when the caller did not send an explicit
clear intent. After normalization, reject empty results as invalid (or preserve
original behavior without clearing) instead of silently collapsing to empty.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP update_dashboard may unintentionally clear dashboard slug.
- ⚠️ Dashboard URLs revert to numeric IDs unexpectedly.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use an MCP client to call the `update_dashboard` tool exported from
`superset/mcp_service/dashboard/tool/__init__.py:18-31`, passing a valid
dashboard
`identifier` and a slug like `"!!!"` (only non-word, non-hyphen characters).
2. The request is parsed into `UpdateDashboardRequest` in
`superset/mcp_service/dashboard/schemas.py:738-78`; the `normalize_slug`
field validator
at lines 892-906 strips whitespace, replaces spaces with hyphens, and
removes characters
outside `[\\w-]`.
3. For the input `"!!!"`, `normalize_slug()` produces an empty string `""`
(all characters
stripped), so `request.slug` becomes `""` even though the original input was
non-empty.
4. In `superset/mcp_service/dashboard/tool/update_dashboard.py`,
`_apply_field_updates()`
at actual lines ~180-193 sees `request.slug is not None` and executes
`dashboard.slug =
request.slug or None`, clearing any existing slug; downstream the dashboard
URL generation
in `superset/models/dashboard.py:198-205` switches from the slug to the
numeric ID,
effectively erasing the slug without an explicit clear intent.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b9a6bdc7c0f74fcab6ee8c833aa6a049&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=b9a6bdc7c0f74fcab6ee8c833aa6a049&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:** 892:906
**Comment:**
*Logic Error: This slug normalizer can turn a non-empty user input into
an empty string (for example, strings containing only stripped characters),
which downstream update logic treats as a slug-clear operation; that can
unintentionally erase an existing slug when the caller did not send an explicit
clear intent. After normalization, reject empty results as invalid (or preserve
original behavior without clearing) instead of silently collapsing to empty.
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=95540560aa4ad90ba1cd00399c020677a64ea27efe77ed7b37703e90874d41b3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=95540560aa4ad90ba1cd00399c020677a64ea27efe77ed7b37703e90874d41b3&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -152,57 +189,117 @@ def _apply_field_updates(dashboard: Any, request:
UpdateDashboardRequest) -> lis
dashboard.position_json = json.dumps(request.position_json)
changed.append("position_json")
- if request.json_metadata_overrides is not None:
- dashboard.json_metadata = _merge_json_metadata(
- dashboard, request.json_metadata_overrides
- )
+ metadata_overrides = _collect_metadata_overrides(request)
+ if metadata_overrides:
+ dashboard.json_metadata = _merge_json_metadata(dashboard,
metadata_overrides)
changed.append("json_metadata")
if request.css is not None:
dashboard.css = request.css or None
changed.append("css")
+ if request.tags is not None:
+ # Reuse the same helper the REST UpdateDashboardCommand uses so tag
+ # association semantics (custom-tag full replacement) stay identical.
+ from superset.commands.utils import update_tags
+ from superset.tags.models import ObjectType
+
+ update_tags(ObjectType.dashboard, dashboard.id, dashboard.tags,
request.tags)
+ changed.append("tags")
+
return changed
+def _validate_update_request(
+ dashboard: Any, request: UpdateDashboardRequest
+) -> DashboardError | None:
+ """Pre-flight validation mirroring the REST update path.
+
+ Runs before any mutation so the tool rejects the same payloads the REST
+ ``DashboardPutSchema`` / ``UpdateDashboardCommand`` would — invalid CSS,
+ conflicting metadata keys, and unauthorized or unknown tag IDs — returning
a
+ structured error instead of failing deep inside the commit.
+ """
+ from marshmallow import ValidationError as MarshmallowValidationError
+
+ from superset.commands.exceptions import (
+ TagForbiddenError,
+ TagNotFoundValidationError,
+ )
+ from superset.commands.utils import validate_tags
+ from superset.dashboards.schemas import validate_css
+ from superset.tags.models import ObjectType
+
+ # Empty string clears CSS (no validation needed); only validate real
content.
+ if request.css:
+ try:
+ validate_css(request.css)
+ except MarshmallowValidationError as ex:
+ detail = (
+ "; ".join(str(m) for m in ex.messages)
+ if isinstance(ex.messages, list)
+ else str(ex.messages)
+ )
+ return DashboardError(
+ error=f"Dashboard CSS is invalid: {detail}",
+ error_type="InvalidCSS",
+ )
+
+ try:
+ _collect_metadata_overrides(request)
+ except ValueError as ex:
+ return DashboardError(error=str(ex), error_type="InvalidRequest")
+
+ if request.tags is not None:
+ try:
+ validate_tags(ObjectType.dashboard, dashboard.tags, request.tags)
+ except TagForbiddenError as ex:
+ return DashboardError(error=str(ex), error_type="TagForbidden")
+ except TagNotFoundValidationError as ex:
+ return DashboardError(error=str(ex), error_type="TagNotFound")
Review Comment:
**Suggestion:** `validate_tags` performs database lookups, but this
pre-validation block only catches tag-specific domain errors; any
`SQLAlchemyError` raised here occurs before the main `try/except
SQLAlchemyError` block and will escape as an unhandled tool failure instead of
returning a structured `DashboardError`. Wrap this branch in SQLAlchemy error
handling (or run it inside the existing DB error guard) so DB failures during
validation are reported consistently. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP update_dashboard crashes on tag-validation database failures.
- ⚠️ LLM clients receive generic failures, not structured DashboardError.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Invoke the MCP `update_dashboard` tool defined in
`superset/mcp_service/dashboard/tool/update_dashboard.py:175-216` (and
exported from
`tool/__init__.py:18-31`) with a request that includes a non-None `tags`
list for a valid
dashboard identifier.
2. After `_find_and_authorize_dashboard()` succeeds, `update_dashboard()`
calls
`_validate_update_request(dashboard, request)` at lines 124-172 to mirror
REST
`DashboardPutSchema` / `UpdateDashboardCommand` validation before mutating
the dashboard.
3. Inside `_validate_update_request()`, when `request.tags is not None`, it
executes the
block at lines 253-259: `validate_tags(ObjectType.dashboard, dashboard.tags,
request.tags)`. That helper (see `superset/commands/utils.py:116-156`)
performs tag
lookups via `TagDAO` backed by SQLAlchemy (`BaseDAO.find_by_id()` uses
`db.session.query()` in `superset/daos/base.py:61-93`).
4. If those DB lookups raise a `SQLAlchemyError` (e.g., transient database
failure), the
exception is not caught by `_validate_update_request()`—it only handles
`TagForbiddenError` and `TagNotFoundValidationError`—and it occurs before
the main
`try/except SQLAlchemyError` around `_apply_field_updates()` at
`superset/mcp_service/dashboard/tool/update_dashboard.py:230-259`, so the MCP
`update_dashboard` call fails with an unstructured tool error instead of a
`DashboardError` payload describing the failure.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=05edc167da1041da89a19f4224f1e351&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=05edc167da1041da89a19f4224f1e351&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/tool/update_dashboard.py
**Line:** 253:259
**Comment:**
*Possible Bug: `validate_tags` performs database lookups, but this
pre-validation block only catches tag-specific domain errors; any
`SQLAlchemyError` raised here occurs before the main `try/except
SQLAlchemyError` block and will escape as an unhandled tool failure instead of
returning a structured `DashboardError`. Wrap this branch in SQLAlchemy error
handling (or run it inside the existing DB error guard) so DB failures during
validation are reported consistently.
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=6db0467a0d43ccf060660210a487ad0881203fcb08dd9044fe855067c241e71c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=6db0467a0d43ccf060660210a487ad0881203fcb08dd9044fe855067c241e71c&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]
