codeant-ai-for-open-source[bot] commented on code in PR #40957:
URL: https://github.com/apache/superset/pull/40957#discussion_r3493321354
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -212,6 +308,10 @@ def update_dashboard(
if auth_error is not None:
return auth_error
+ validation_error = _validate_update_request(dashboard, request)
+ if validation_error is not None:
+ return validation_error
Review Comment:
**Suggestion:** The new pre-flight validation runs before the
`SQLAlchemyError` safety net, but this validation path performs database access
(tag lookups via `validate_tags`). If that query fails, the exception bubbles
out unhandled and the tool returns an internal error instead of the structured
`DashboardError` used elsewhere. Run validation inside the existing DB error
handling block (or catch `SQLAlchemyError` in `_validate_update_request`) so
transient DB failures are handled consistently. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP update_dashboard tool may surface raw DB errors.
- ⚠️ Database rollback logic in update_dashboard not exercised.
- ⚠️ Inconsistent error shape versus commit-phase SQLAlchemy failures.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/mcp_service/dashboard/tool/update_dashboard.py:273-369`,
note that
`update_dashboard()` calls `_validate_update_request(dashboard, request)` at
lines 311-313
before entering the `try: ... except SQLAlchemyError` block that wraps
`_apply_field_updates()` and `db.session.commit()`.
2. Within `_validate_update_request()` at
`superset/mcp_service/dashboard/tool/update_dashboard.py:212-260`, observe
the tag
validation path: when `request.tags` is not `None`, it calls
`validate_tags(ObjectType.dashboard, dashboard.tags, request.tags)` at line
254, catching
only `TagForbiddenError` and `TagNotFoundValidationError`.
3. Inspect `validate_tags()` in `superset/commands/utils.py:17-57`: it
computes
`additional_tags` and for each `tag_id` calls `TagDAO.find_by_id(tag_id)` at
lines 52-55.
`TagDAO.find_by_id()` is implemented via `BaseDAO.find_by_id()` in
`superset/daos/base.py:330-42`, which delegates to `_find_by_column()` (same
file, lines
320-27) that uses `db.session.query(...)` and
`query.filter(...).one_or_none()`. These
database calls can raise `SQLAlchemyError` (for example, if the database
connection is
unavailable or the query fails).
4. Because `_validate_update_request()` does not catch `SQLAlchemyError` and
its
invocation at lines 311-313 occurs outside the `try: ... except
SQLAlchemyError as db_err`
block at lines 318-359 in `update_dashboard()`, a `SQLAlchemyError` raised
from
`TagDAO.find_by_id()` during tag validation will propagate out of
`update_dashboard()`
without hitting the rollback-and-`DashboardError` safety net, causing the
MCP tool
invocation to fail with an unhandled database exception rather than
returning the
structured `DashboardError(error_type="DatabaseError")` used for other
SQLAlchemy failures
in this tool.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d600ee15f89b40efbf827b9980572338&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=d600ee15f89b40efbf827b9980572338&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:** 311:313
**Comment:**
*Possible Bug: The new pre-flight validation runs before the
`SQLAlchemyError` safety net, but this validation path performs database access
(tag lookups via `validate_tags`). If that query fails, the exception bubbles
out unhandled and the tool returns an internal error instead of the structured
`DashboardError` used elsewhere. Run validation inside the existing DB error
handling block (or catch `SQLAlchemyError` in `_validate_update_request`) so
transient DB failures are handled 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=86cd7e7fc72476eaaf5a165e3b6ee5d79f95473f4c1098e16025b2b3cd8e84ed&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=86cd7e7fc72476eaaf5a165e3b6ee5d79f95473f4c1098e16025b2b3cd8e84ed&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]
