codeant-ai-for-open-source[bot] commented on code in PR #40957:
URL: https://github.com/apache/superset/pull/40957#discussion_r3417024663
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -708,6 +708,138 @@ 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)
+
+ dashboard_id: int = Field(..., description="ID of the dashboard to update")
+
+ # Direct dashboard fields (match DashboardPutSchema)
+ dashboard_title: str | None = Field(
+ None,
+ description="New dashboard title.",
+ validation_alias=AliasChoices("dashboard_title", "title", "name"),
+ )
+ slug: str | None = Field(
+ None, description="New URL slug for the dashboard (must be unique)."
+ )
+ published: bool | None = Field(
+ None,
+ description=(
+ "Set to true to publish the dashboard (visible to other users), "
+ "false to unpublish it (draft)."
+ ),
+ )
+ css: str | None = Field(None, description="Custom CSS applied to the
dashboard.")
+ theme_id: int | None = Field(None, description="Theme ID for the
dashboard.")
+ certified_by: str | None = Field(
+ None,
+ description=(
+ "Person or group that certified this dashboard. "
+ "Pass an empty string to clear."
+ ),
+ )
+ certification_details: str | None = Field(
+ None,
+ description=("Details of the certification. Pass an empty string to
clear."),
+ )
+ roles: list[int] | None = Field(
+ None,
+ description=(
+ "FULL REPLACEMENT list of role IDs with dashboard access "
+ "(DASHBOARD_RBAC). The provided list replaces existing roles."
+ ),
+ )
+ tags: list[int] | None = Field(
+ None,
+ description=(
+ "FULL REPLACEMENT list of tag IDs. The provided list replaces "
+ "existing tags — include existing tag IDs to keep them."
+ ),
+ )
Review Comment:
**Suggestion:** The request schema says it matches `DashboardPutSchema`
direct fields, but it omits `owners`, so callers cannot update dashboard
ownership through this tool even though the dashboard update command supports
it. Add an `owners: list[int] | None` field in the request schema and pass it
through in the tool properties so ownership updates work as intended.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP update_dashboard cannot change dashboard owners as advertised.
- ⚠️ LLM workflows can't adjust ownership via MCP service.
- ⚠️ Confusing mismatch between MCP docs and actual behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the MCP service so tools from
`superset/mcp_service/dashboard/tool/__init__.py:3-16` (including
`update_dashboard`) are
registered in the MCP app described in `superset/mcp_service/app.py:8-15`.
2. From an MCP client, call the `update_dashboard` tool (implemented in
`superset/mcp_service/dashboard/tool/update_dashboard.py:60-86`) with a
payload containing
an `owners` list, for example `{"dashboard_id": 1, "owners": [5, 6]}`.
3. The request is parsed into `UpdateDashboardRequest` defined in
`superset/mcp_service/dashboard/schemas.py:711-136`, which declares direct
fields at
`723-106` but does not include an `owners: list[int] | None` field, so the
`owners` key
from the JSON payload is ignored and not available on the `request` object.
4. `_build_update_properties()` in
`superset/mcp_service/dashboard/tool/update_dashboard.py:31-52` builds the
properties dict
for `UpdateDashboardCommand` using `_DIRECT_FIELDS` defined at
`update_dashboard.py:10-19`, which also omits `"owners"`, so even if
ownership were
present on the request it would never be passed to `UpdateDashboardCommand`
(`update_dashboard.py:93-111`), and the dashboard owners remain unchanged
despite the tool
being documented as supporting roles/tags (and the PR description
advertising owners).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3a5d7f86ff1f47c4a67e90e730e40098&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=3a5d7f86ff1f47c4a67e90e730e40098&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:** 723:765
**Comment:**
*Incomplete Implementation: The request schema says it matches
`DashboardPutSchema` direct fields, but it omits `owners`, so callers cannot
update dashboard ownership through this tool even though the dashboard update
command supports it. Add an `owners: list[int] | None` field in the request
schema and pass it through in the tool properties so ownership updates work as
intended.
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=3dab01bc15ab30aaa272b240ce7552c379be7c5fd42093bc2c691b0c043a80cc&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=3dab01bc15ab30aaa272b240ce7552c379be7c5fd42093bc2c691b0c043a80cc&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -0,0 +1,352 @@
+# 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.
+
+"""
+MCP tool: update_dashboard
+
+This tool performs a partial update of dashboard metadata (title, slug,
+published state, certification, roles, tags, CSS, theme, and selected
+json_metadata settings).
+"""
+
+import logging
+from typing import Any
+
+from fastmcp import Context
+from sqlalchemy.exc import SQLAlchemyError
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.commands.exceptions import CommandException
+from superset.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ DashboardInfo,
+ serialize_chart_summary,
+ UpdateDashboardRequest,
+ UpdateDashboardResponse,
+)
+from superset.mcp_service.privacy import user_can_view_data_model_metadata
+from superset.mcp_service.utils.url_utils import get_superset_base_url
+from superset.utils import json
+
+logger = logging.getLogger(__name__)
+
+# Direct dashboard columns accepted by UpdateDashboardCommand
+# (subset of DashboardPutSchema).
+_DIRECT_FIELDS = (
+ "dashboard_title",
+ "slug",
+ "published",
+ "css",
+ "theme_id",
+ "certified_by",
+ "certification_details",
+ "roles",
+ "tags",
+)
+
+# Convenience fields stored inside the dashboard's json_metadata blob.
+_METADATA_FIELDS = (
+ "color_scheme",
+ "cross_filters_enabled",
+ "refresh_frequency",
+ "filter_bar_orientation",
+)
+
+
+def _build_update_properties(
+ request: UpdateDashboardRequest, dashboard: Any
+) -> tuple[dict[str, Any], list[str]]:
+ """Build the UpdateDashboardCommand properties dict from the request.
+
+ Returns ``(properties, updated_fields)`` where *updated_fields* lists
+ the request fields that will be changed.
+
+ json_metadata is a stringified JSON blob and
+ ``DashboardDAO.set_dash_metadata`` resets absent keys to defaults
+ (e.g. ``expanded_slices`` -> {}). To avoid silently destroying state,
+ the dashboard's FULL current json_metadata is read, the requested
+ changes are merged in, and the complete blob is written back.
+ """
+ properties: dict[str, Any] = {}
+ updated_fields: list[str] = []
+
+ for field in _DIRECT_FIELDS:
+ value = getattr(request, field)
+ if value is not None:
+ properties[field] = value
+ updated_fields.append(field)
+
+ metadata_changes = {
+ field: value
+ for field in _METADATA_FIELDS
+ if (value := getattr(request, field)) is not None
+ }
+ if metadata_changes:
+ try:
+ current_metadata = json.loads(dashboard.json_metadata or "{}")
+ except (ValueError, TypeError):
+ logger.warning(
+ "Failed to parse existing json_metadata for dashboard %s; "
+ "starting from an empty metadata object",
+ dashboard.id,
+ )
+ current_metadata = {}
+ if not isinstance(current_metadata, dict):
+ current_metadata = {}
+ properties["json_metadata"] = json.dumps(
+ {**current_metadata, **metadata_changes}
+ )
+ updated_fields.extend(metadata_changes)
+
+ return properties, updated_fields
+
+
+def _find_and_authorize_dashboard(
+ dashboard_id: int,
+) -> tuple[Any, UpdateDashboardResponse | None]:
+ """Return (dashboard, None) on success or (None, error_response) on
failure."""
+ from superset import security_manager
+ from superset.daos.dashboard import DashboardDAO
+ from superset.exceptions import SupersetSecurityException
+
+ dashboard = DashboardDAO.find_by_id(dashboard_id)
+ if not dashboard:
+ return None, UpdateDashboardResponse(
+ error=(
+ f"Dashboard with ID {dashboard_id} not found."
+ " Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+
+ try:
+ security_manager.raise_for_ownership(dashboard)
+ except SupersetSecurityException:
+ return None, UpdateDashboardResponse(
+ permission_denied=True,
+ error=(
+ f"You don't have permission to edit dashboard "
+ f"'{dashboard.dashboard_title}' (ID: {dashboard_id})."
+ ),
+ )
+
+ return dashboard, None
+
+
+def _serialize_updated_dashboard(
+ updated_dashboard: Any, updated_fields: list[str]
+) -> UpdateDashboardResponse:
+ """Build the success response, re-fetching with eager-loaded relationships.
+
+ The preceding command commit may invalidate the session in multi-tenant
+ environments; on re-fetch failure, return a minimal response using only
+ scalar attributes that are already loaded — relationship fields (tags,
+ slices) would trigger lazy-loading on the same dead session.
+ """
+ from sqlalchemy.orm import subqueryload
+
+ from superset import db
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{updated_dashboard.id}/"
+ )
+
+ try:
+ updated_dashboard = (
+ DashboardDAO.find_by_id(
+ updated_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or updated_dashboard
+ )
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ updated_dashboard.id,
+ exc_info=True,
+ )
+ try:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ except SQLAlchemyError:
+ logger.warning(
+ "Database rollback failed during dashboard re-fetch error
handling",
+ exc_info=True,
+ )
+ return UpdateDashboardResponse(
+ dashboard=DashboardInfo(
+ id=updated_dashboard.id,
+ dashboard_title=updated_dashboard.dashboard_title,
+ published=updated_dashboard.published,
+ url=dashboard_url,
+ ),
+ dashboard_url=dashboard_url,
+ updated_fields=updated_fields,
+ error=None,
+ )
+
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ dashboard_info = DashboardInfo(
+ id=updated_dashboard.id,
+ dashboard_title=updated_dashboard.dashboard_title,
+ slug=updated_dashboard.slug,
+ description=updated_dashboard.description,
+ css=updated_dashboard.css,
+ certified_by=updated_dashboard.certified_by,
+ certification_details=updated_dashboard.certification_details,
+ published=updated_dashboard.published,
+ created_on=updated_dashboard.created_on,
+ changed_on=updated_dashboard.changed_on,
+ uuid=str(updated_dashboard.uuid) if updated_dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(updated_dashboard.slices),
+ tags=[
+ serialize_tag_object(tag)
+ for tag in getattr(updated_dashboard, "tags", [])
+ if serialize_tag_object(tag) is not None
+ ],
Review Comment:
**Suggestion:** Each tag is serialized twice in the list comprehension (once
for filtering and once for value construction), which does unnecessary repeated
work and can produce inconsistent output if serialization behavior changes.
Serialize each tag once (for example with a walrus assignment) and reuse that
value in both filter and output. [performance]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Extra tag serialization on every update_dashboard success.
- ⚠️ Minor CPU overhead when dashboards have many tags.
- ⚠️ No functional bug; only micro-optimization opportunity.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the `update_dashboard` MCP tool implemented in
`superset/mcp_service/dashboard/tool/update_dashboard.py:60-139` so that an
update
succeeds and `_serialize_updated_dashboard()` is invoked to build the
response.
2. `_serialize_updated_dashboard()` at `update_dashboard.py:110-141`
constructs a
`DashboardInfo` object, including the `tags` field built by the list
comprehension at
lines 223-227.
3. For each tag in `getattr(updated_dashboard, "tags", [])`, the
comprehension expression
`serialize_tag_object(tag)` at line 224 and the filter `if
serialize_tag_object(tag) is
not None` at line 226 both call `serialize_tag_object`, causing each tag to
be serialized
twice.
4. This double serialization happens on every successful `update_dashboard`
call that
returns full dashboard info, adding avoidable per-tag overhead even though a
single
serialization result could be reused for both the filter and the output
element.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cf33d2dc2baf4151ba2da95069af70c6&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=cf33d2dc2baf4151ba2da95069af70c6&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:** 223:227
**Comment:**
*Performance: Each tag is serialized twice in the list comprehension
(once for filtering and once for value construction), which does unnecessary
repeated work and can produce inconsistent output if serialization behavior
changes. Serialize each tag once (for example with a walrus assignment) and
reuse that value in both filter and output.
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=fdcc3bbe5cff3b4fa44582f6686d556362b626b4ad06d3746f82631aa3240398&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=fdcc3bbe5cff3b4fa44582f6686d556362b626b4ad06d3746f82631aa3240398&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]
