You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/06/11 00:06:37 UTC
[PR] feat(mcp): add duplicate_dashboard tool [superset]
aminghadersohi opened a new pull request, #40959:
URL: https://github.com/apache/superset/pull/40959
### SUMMARY
Adds a new `duplicate_dashboard` MCP tool to the Superset MCP service. The canonical AI workflow this enables: "create a regional/staging variant of this dashboard" — clone a template dashboard, then edit the copy.
Design:
- Wraps the existing `CopyDashboardCommand` (the same backend used by the frontend "Save as" flow on `/api/v1/dashboard/<id>/copy/`).
- The source dashboard can be identified by numeric ID, UUID, or slug (resolved via `DashboardDAO.get_by_id_or_slug`, which applies dashboard access filters).
- `DashboardCopySchema`'s copy data contract requires `json_metadata`; the tool builds it server-side from the source dashboard's current state (`json_metadata` plus a `positions` key from `position_json`), mirroring exactly what the frontend "Save as" sends. `json_metadata` is intentionally not exposed as a tool parameter.
- `duplicate_slices: bool = false` — when true, every chart on the source dashboard is deep-copied into a new chart object owned by the caller; when false the copy references the same charts.
- The new title is sanitized for XSS using the same pattern as `generate_dashboard` (nh3-based `sanitize_user_input`), with a warning surfaced when content is stripped and a hard error when the title sanitizes to nothing.
- Structured error responses for source-not-found, access-denied, forbidden (`DASHBOARD_RBAC`), and copy failures; errors are wrapped for LLM-context safety.
- Registered as a `mutate` tool (`class_permission_name="Dashboard"`, `method_permission_name="write"`) and added to `MCP_CACHE_CONFIG["excluded_tools"]` so responses are never cached.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — MCP service tool, no UI changes.
### TESTING INSTRUCTIONS
1. Start the MCP service and connect an MCP client.
2. Call `duplicate_dashboard` with `{"dashboard_id": <existing id|uuid|slug>, "dashboard_title": "My Copy"}` — verify a new dashboard is created referencing the same charts, and the returned URL opens it.
3. Call again with `"duplicate_slices": true` — verify the new dashboard contains new chart objects (different chart IDs) with the same layout.
4. Verify error responses for a non-existent dashboard ID and for a dashboard the user cannot access.
5. Run unit tests: `pytest tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py`
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3482163034
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,324 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload and a flag indicating whether the layout maps any
+ chart, so the caller can refuse to produce a silently empty copy.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+ return payload, _positions_reference_charts(positions)
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
+ return _sanitize_dashboard_info_for_llm_context(info), dashboard_url
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Duplicate dashboard",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def duplicate_dashboard(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> DuplicateDashboardResponse:
+ """
+ Duplicate an existing dashboard under a new title.
+
+ By default the copy references the same charts as the source.
+ Set duplicate_slices=true to also deep-copy every chart into new
+ chart objects owned by you, so edits to the copies never affect
+ the originals.
+
+ The source dashboard can be identified by numeric ID, UUID, or slug.
+ Returns the new dashboard's ID, title, and URL.
+ """
+ await ctx.info(
+ "Duplicating dashboard: dashboard_id=%s, duplicate_slices=%s"
+ % (request.dashboard_id, request.duplicate_slices)
+ )
+
+ from superset.commands.dashboard.copy import CopyDashboardCommand
+ from superset.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardCopyError,
+ DashboardForbiddenError,
+ DashboardInvalidError,
+ DashboardNotFoundError,
+ )
+ from superset.daos.dashboard import DashboardDAO
+
+ try:
+ with event_logger.log_context(action="mcp.duplicate_dashboard.lookup"):
+ try:
+ source = DashboardDAO.get_by_id_or_slug(str(request.dashboard_id))
+ except DashboardNotFoundError:
+ await ctx.warning(
+ "Dashboard not found for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' not found. "
+ "Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+ except DashboardAccessDeniedError:
+ await ctx.warning(
+ "Dashboard access denied for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"You don't have access to dashboard "
+ f"'{request.dashboard_id}', so it cannot be duplicated."
+ ),
+ )
+
+ data, layout_has_charts = _build_copy_payload(
+ source, request.dashboard_title, request.duplicate_slices
+ )
+
+ if getattr(source, "slices", None) and not layout_has_charts:
+ await ctx.warning(
+ "Source layout maps no charts; refusing to duplicate to "
+ "avoid an empty copy: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' has charts but its "
+ "saved layout is missing or invalid, so duplicating it "
+ "would produce a dashboard with no charts. Open and "
+ "re-save the source dashboard to repair its layout, then "
+ "try again."
+ ),
+ )
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.copy"):
+ new_dashboard = CopyDashboardCommand(source, data).run()
+
+ # Re-fetch with eager-loaded relationships to avoid lazy-loading on
+ # a session that the command's commit may have invalidated.
+ from sqlalchemy.orm import subqueryload
+
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ try:
+ new_dashboard = (
+ DashboardDAO.find_by_id(
+ new_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or new_dashboard
+ )
+ info, dashboard_url = _serialize_new_dashboard(new_dashboard)
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ new_dashboard.id,
+ exc_info=True,
+ )
+ from superset import db
+
+ 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,
+ )
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"
+ )
+ info = _sanitize_dashboard_info_for_llm_context(
+ DashboardInfo(
+ id=new_dashboard.id,
+ dashboard_title=request.dashboard_title,
+ url=dashboard_url,
+ )
+ )
+
+ logger.info(
+ "Duplicated dashboard %s into dashboard %s (duplicate_slices=%s)",
+ request.dashboard_id,
+ new_dashboard.id,
+ request.duplicate_slices,
+ )
+
+ return DuplicateDashboardResponse(
+ dashboard=info,
+ dashboard_url=dashboard_url,
+ duplicated_slices=request.duplicate_slices,
+ warnings=list(request.sanitization_warnings),
+ )
+
+ except DashboardForbiddenError:
+ await ctx.error(
+ "Dashboard duplication forbidden: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"You don't have permission to duplicate dashboard "
+ f"'{request.dashboard_id}'."
+ ),
+ )
+ except DashboardInvalidError:
+ return DuplicateDashboardResponse(
+ error=(
+ "Dashboard duplication parameters were invalid. "
+ "Provide a non-empty dashboard_title."
+ ),
+ )
+ except DashboardCopyError as exc:
+ from superset import db
+
+ try:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ except SQLAlchemyError:
+ logger.warning(
+ "Database rollback failed during error handling", exc_info=True
+ )
+ await ctx.error("Dashboard duplication failed: %s" % (str(exc),))
+ return DuplicateDashboardResponse(
+ error=f"Failed to duplicate dashboard: {exc}",
+ )
+ except Exception as exc:
Review Comment:
**Suggestion:** The tool can still crash with an unhandled exception when the source dashboard has malformed stored metadata. `_build_copy_payload` tolerates bad `json_metadata`, but `CopyDashboardCommand` later reads `source.params` again via `DashboardDAO.copy_dashboard -> set_dash_metadata -> params_dict`, which can raise `JSONDecodeError`/`ValueError`. That path falls into the broad `except Exception` and re-raises, so callers get a hard failure instead of a structured `DuplicateDashboardResponse`. Catch this validation/parsing failure and return a normal tool error response. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP duplicate_dashboard tool crashes on malformed dashboard metadata.
- ⚠️ AI workflows cloning dashboards fail for affected dashboards.
- ⚠️ MCP clients see unexpected tool failure instead of structured error.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or identify a dashboard record whose `json_metadata` column contains malformed
JSON (e.g. manually edited in the DB or produced by an older bug). This is the same field
referenced at `superset/models/dashboard.py:143` and loaded with
`json.loads(dashboard.json_metadata)` under `contextlib.suppress(json.JSONDecodeError)` in
`superset/views/utils.py:371-373`, demonstrating that persisted metadata can be invalid
without being automatically repaired.
2. From an MCP client connected to the Superset MCP service, invoke the
`duplicate_dashboard` tool (entrypoint at
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:152-165`) with `dashboard_id`
pointing to the malformed dashboard and a valid `dashboard_title`, e.g. `{"dashboard_id":
<id>, "dashboard_title": "Copy", "duplicate_slices": true}`.
3. The tool resolves the source dashboard via `DashboardDAO.get_by_id_or_slug` and builds
the copy payload using `_build_copy_payload` at `duplicate_dashboard.py:208-210`, then
calls `CopyDashboardCommand(source, data).run()` inside the `event_logger` context at
`duplicate_dashboard.py:227-228`. `CopyDashboardCommand.run()` (implemented at
`superset/commands/dashboard/copy.py:40-43`) calls
`DashboardDAO.copy_dashboard(self._original_dash, self._properties)`.
4. In `DashboardDAO.copy_dashboard` (`superset/daos/dashboard.py:368-38`), the code sets
`dash.params = original_dash.params` at line 35, copying the malformed JSON string into
the new dashboard, and then immediately calls `cls.set_dash_metadata(dash, metadata,
old_to_new_slice_ids)` at line 36. `set_dash_metadata` reads `dashboard.params_dict` at
`superset/daos/dashboard.py:269`, which attempts to deserialize `dash.params`. With
malformed JSON, this property raises a parsing exception (e.g.
`json.JSONDecodeError`/`ValueError`) that is not a `SQLAlchemyError`, so the transaction
wrapper in `CopyDashboardCommand.run()` (see `@transaction(on_error=partial(on_error,
reraise=DashboardCopyError))` in `superset/commands/dashboard/copy.py:40` and the handler
in `superset/utils/decorators.py:210-23`) re-raises the original parsing exception instead
of wrapping it as `DashboardCopyError`. The exception bypasses the `except
DashboardCopyError as exc:` handler in `duplicate_dashboard.py:306-318` and is caught only
by the broad `except Exception as exc:` at `duplicate_dashboard.py:319-323`, which logs
`ctx.error("Unexpected error duplicating dashboard: %s: %s" % (type(exc).__name__,
str(exc)))` and then re-raises. The MCP client receives a hard tool failure / transport
error instead of a structured `DuplicateDashboardResponse.error` describing the metadata
problem.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ad30a8b3c67f410c8ca4a8db83f732cf&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=ad30a8b3c67f410c8ca4a8db83f732cf&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/duplicate_dashboard.py
**Line:** 227:319
**Comment:**
*Logic Error: The tool can still crash with an unhandled exception when the source dashboard has malformed stored metadata. `_build_copy_payload` tolerates bad `json_metadata`, but `CopyDashboardCommand` later reads `source.params` again via `DashboardDAO.copy_dashboard -> set_dash_metadata -> params_dict`, which can raise `JSONDecodeError`/`ValueError`. That path falls into the broad `except Exception` and re-raises, so callers get a hard failure instead of a structured `DuplicateDashboardResponse`. Catch this validation/parsing failure and return a normal tool error response.
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%2F40959&comment_hash=50a5c5dbae06da7f5c16774286cf8582b7922ad30b1cbbe62b667270024fcd2f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=50a5c5dbae06da7f5c16774286cf8582b7922ad30b1cbbe62b667270024fcd2f&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40959:
URL: https://github.com/apache/superset/pull/40959#issuecomment-4696729576
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40959:
URL: https://github.com/apache/superset/pull/40959#issuecomment-4704384447
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3477196233
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,315 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload and a flag indicating whether the layout maps any
+ chart, so the caller can refuse to produce a silently empty copy.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+ return payload, _positions_reference_charts(positions)
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
+ return _sanitize_dashboard_info_for_llm_context(info), dashboard_url
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Duplicate dashboard",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def duplicate_dashboard(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> DuplicateDashboardResponse:
+ """
+ Duplicate an existing dashboard under a new title.
+
+ By default the copy references the same charts as the source.
+ Set duplicate_slices=true to also deep-copy every chart into new
+ chart objects owned by you, so edits to the copies never affect
+ the originals.
+
+ The source dashboard can be identified by numeric ID, UUID, or slug.
+ Returns the new dashboard's ID, title, and URL.
+ """
+ await ctx.info(
+ "Duplicating dashboard: dashboard_id=%s, duplicate_slices=%s"
+ % (request.dashboard_id, request.duplicate_slices)
+ )
+
+ from superset.commands.dashboard.copy import CopyDashboardCommand
+ from superset.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardCopyError,
+ DashboardForbiddenError,
+ DashboardInvalidError,
+ DashboardNotFoundError,
+ )
+ from superset.daos.dashboard import DashboardDAO
+
+ try:
+ with event_logger.log_context(action="mcp.duplicate_dashboard.lookup"):
+ try:
+ source = DashboardDAO.get_by_id_or_slug(str(request.dashboard_id))
+ except DashboardNotFoundError:
+ await ctx.warning(
+ "Dashboard not found for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' not found. "
+ "Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+ except DashboardAccessDeniedError:
+ await ctx.warning(
+ "Dashboard access denied for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"You don't have access to dashboard "
+ f"'{request.dashboard_id}', so it cannot be duplicated."
+ ),
+ )
+
+ data, layout_has_charts = _build_copy_payload(
+ source, request.dashboard_title, request.duplicate_slices
+ )
+
+ if getattr(source, "slices", None) and not layout_has_charts:
+ await ctx.warning(
+ "Source layout maps no charts; refusing to duplicate to "
+ "avoid an empty copy: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' has charts but its "
+ "saved layout is missing or invalid, so duplicating it "
+ "would produce a dashboard with no charts. Open and "
+ "re-save the source dashboard to repair its layout, then "
+ "try again."
+ ),
+ )
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.copy"):
+ new_dashboard = CopyDashboardCommand(source, data).run()
+
+ # Re-fetch with eager-loaded relationships to avoid lazy-loading on
+ # a session that the command's commit may have invalidated.
+ from sqlalchemy.orm import subqueryload
+
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ try:
+ new_dashboard = (
+ DashboardDAO.find_by_id(
+ new_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or new_dashboard
+ )
+ info, dashboard_url = _serialize_new_dashboard(new_dashboard)
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ new_dashboard.id,
+ exc_info=True,
+ )
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"
+ )
+ info = _sanitize_dashboard_info_for_llm_context(
+ DashboardInfo(
+ id=new_dashboard.id,
+ dashboard_title=request.dashboard_title,
+ url=dashboard_url,
+ )
+ )
Review Comment:
Good catch — fixed in 2c889759cc. The re-fetch's `except SQLAlchemyError` block now calls `db.session.rollback()` (guarded by a nested try/except, matching `generate_dashboard` and `add_chart_to_existing_dashboard`) before building the minimal response, so a failed re-fetch no longer leaves the shared session in a broken transaction state. Added a regression test (`test_refetch_failure_rolls_back_and_returns_minimal_response`) that asserts the rollback runs and the fallback response is still returned.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3493540122
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,414 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool, str | None]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload, a flag indicating whether the layout maps any
+ chart (so the caller can refuse to produce a silently empty copy),
+ and an optional warning string when ``json_metadata`` could not be
+ decoded. The caller surfaces the warning rather than hard-failing:
+ metadata loss degrades aesthetic settings (colors, native filters)
+ but does not corrupt the copy's chart content.
+ """
+ metadata_warning: str | None = None
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) could not "
+ "be decoded and were not copied. The duplicate uses default "
+ "settings for colors, native filters, and other metadata. "
+ "Open and re-save the source dashboard to repair its settings."
+ )
+ if not isinstance(metadata, dict):
+ metadata = {}
+ if metadata_warning is None:
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) were not "
+ "a valid JSON object and were not copied. The duplicate uses "
+ "default settings for colors, native filters, and other metadata."
+ )
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
Review Comment:
Declining. Annotating short-lived local variables in function bodies is not a Superset convention and adds noise without improving clarity. The type of payload is immediately apparent from context.
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,414 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool, str | None]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload, a flag indicating whether the layout maps any
+ chart (so the caller can refuse to produce a silently empty copy),
+ and an optional warning string when ``json_metadata`` could not be
+ decoded. The caller surfaces the warning rather than hard-failing:
+ metadata loss degrades aesthetic settings (colors, native filters)
+ but does not corrupt the copy's chart content.
+ """
+ metadata_warning: str | None = None
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) could not "
+ "be decoded and were not copied. The duplicate uses default "
+ "settings for colors, native filters, and other metadata. "
+ "Open and re-save the source dashboard to repair its settings."
+ )
+ if not isinstance(metadata, dict):
+ metadata = {}
+ if metadata_warning is None:
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) were not "
+ "a valid JSON object and were not copied. The duplicate uses "
+ "default settings for colors, native filters, and other metadata."
+ )
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+ return payload, _positions_reference_charts(positions), metadata_warning
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
+ return _sanitize_dashboard_info_for_llm_context(info), dashboard_url
+
+
+def _safe_rollback(context_label: str) -> None:
+ """Roll back the current DB session, swallowing rollback failures.
+
+ A failed operation can leave the shared session in an invalid
+ transaction state; rolling back keeps later ORM use in the same request
+ lifecycle from inheriting the broken transaction.
+ """
+ from superset import db
+
+ try:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ except SQLAlchemyError:
+ logger.warning(
+ "Database rollback failed during %s error handling",
+ context_label,
+ exc_info=True,
+ )
+
+
+def _refetch_and_serialize(
+ new_dashboard: Any, dashboard_title: str
+) -> tuple[DashboardInfo, str]:
+ """Re-fetch the new dashboard with eager-loaded relationships.
+
+ The eager load avoids lazy-loading on a session the command's commit may
+ have invalidated. If the re-fetch fails, the failed transaction is rolled
+ back and a minimal response is returned instead.
+ """
+ from sqlalchemy.orm import subqueryload
+
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ try:
+ dashboard = (
+ DashboardDAO.find_by_id(
+ new_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or new_dashboard
+ )
+ return _serialize_new_dashboard(dashboard)
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ new_dashboard.id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard re-fetch")
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"
+ )
+ info = _sanitize_dashboard_info_for_llm_context(
+ DashboardInfo(
+ id=new_dashboard.id,
+ dashboard_title=dashboard_title,
+ url=dashboard_url,
+ )
+ )
+ return info, dashboard_url
+
+
+async def _resolve_source(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> tuple[Any, DuplicateDashboardResponse | None]:
+ """Resolve and authorize the source dashboard.
+
+ Returns ``(source, None)`` on success, or ``(None, error_response)`` when
+ the dashboard is missing or inaccessible.
+ """
+ from superset.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardNotFoundError,
+ )
+ from superset.daos.dashboard import DashboardDAO
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.lookup"):
+ try:
+ return DashboardDAO.get_by_id_or_slug(str(request.dashboard_id)), None
+ except DashboardNotFoundError:
+ await ctx.warning(
+ "Dashboard not found for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' not found. "
+ "Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+ except DashboardAccessDeniedError:
+ await ctx.warning(
+ "Dashboard access denied for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"You don't have access to dashboard "
+ f"'{request.dashboard_id}', so it cannot be duplicated."
+ ),
+ )
+ except SQLAlchemyError:
+ # Transient DB/session failures during lookup must surface as a
+ # structured response, not a hard tool failure. The raw error is
+ # logged with a traceback; the response stays generic because
+ # ``str(exc)`` can leak table/column/constraint names.
+ logger.error(
+ "Database error resolving dashboard %s for duplication",
+ request.dashboard_id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard lookup")
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' could not be "
+ "duplicated due to a database error. Please try again."
+ ),
+ )
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Duplicate dashboard",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def duplicate_dashboard(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> DuplicateDashboardResponse:
+ """
+ Duplicate an existing dashboard under a new title.
+
+ By default the copy references the same charts as the source.
+ Set duplicate_slices=true to also deep-copy every chart into new
+ chart objects owned by you, so edits to the copies never affect
+ the originals.
+
+ The source dashboard can be identified by numeric ID, UUID, or slug.
+ Returns the new dashboard's ID, title, and URL.
+ """
+ await ctx.info(
+ "Duplicating dashboard: dashboard_id=%s, duplicate_slices=%s"
+ % (request.dashboard_id, request.duplicate_slices)
+ )
+
+ from superset.commands.dashboard.copy import CopyDashboardCommand
+ from superset.commands.dashboard.exceptions import (
+ DashboardCopyError,
+ DashboardForbiddenError,
+ DashboardInvalidError,
+ )
+
+ try:
+ source, error_response = await _resolve_source(request, ctx)
+ if error_response is not None:
+ return error_response
+
+ data, layout_has_charts, metadata_warning = _build_copy_payload(
+ source, request.dashboard_title, request.duplicate_slices
+ )
+
+ if getattr(source, "slices", None) and not layout_has_charts:
+ await ctx.warning(
+ "Source layout maps no charts; refusing to duplicate to "
+ "avoid an empty copy: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' has charts but its "
+ "saved layout is missing or invalid, so duplicating it "
+ "would produce a dashboard with no charts. Open and "
+ "re-save the source dashboard to repair its layout, then "
+ "try again."
+ ),
+ )
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.copy"):
+ new_dashboard = CopyDashboardCommand(source, data).run()
+
+ info, dashboard_url = _refetch_and_serialize(
+ new_dashboard, request.dashboard_title
+ )
+
+ logger.info(
+ "Duplicated dashboard %s into dashboard %s (duplicate_slices=%s)",
+ request.dashboard_id,
+ new_dashboard.id,
+ request.duplicate_slices,
+ )
+
+ warnings = list(request.sanitization_warnings)
Review Comment:
Declining. Same rationale as the payload comment — local variable annotations in function bodies are not a Superset convention.
##########
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:
##########
@@ -0,0 +1,580 @@
+# 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.
+
+"""
+Unit tests for the duplicate_dashboard MCP tool.
+
+Follows the same pattern used in test_add_chart_to_existing_dashboard.py:
+- Tests run through the async MCP Client (not direct function calls)
+- Patches applied at source locations (superset.daos.dashboard.*,
+ superset.commands.dashboard.copy.*)
+- auth is mocked via the autouse mock_auth fixture
+
+Covers:
+- Duplicate referencing the same charts (duplicate_slices=False, default)
+- Duplicate with deep-copied charts (duplicate_slices=True)
+- Source dashboard not found
+- Source dashboard access denied / copy forbidden
+- Title sanitization (XSS stripped, XSS-only title rejected)
+"""
+
+import logging
+from collections.abc import Iterator
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.utils.sanitization import (
+ LLM_CONTEXT_CLOSE_DELIMITER,
+ LLM_CONTEXT_OPEN_DELIMITER,
+)
+from superset.utils import json
+
+
+def _wrapped(value: str) -> str:
+ """Return the LLM-context-wrapped form a sanitized field should have."""
+ return f"{LLM_CONTEXT_OPEN_DELIMITER}\n{value}\n{LLM_CONTEXT_CLOSE_DELIMITER}"
+
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
Review Comment:
Declining. Module-level loggers are not annotated in Superset test files (or any other file). This is a universal project convention.
##########
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:
##########
@@ -0,0 +1,580 @@
+# 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.
+
+"""
+Unit tests for the duplicate_dashboard MCP tool.
+
+Follows the same pattern used in test_add_chart_to_existing_dashboard.py:
+- Tests run through the async MCP Client (not direct function calls)
+- Patches applied at source locations (superset.daos.dashboard.*,
+ superset.commands.dashboard.copy.*)
+- auth is mocked via the autouse mock_auth fixture
+
+Covers:
+- Duplicate referencing the same charts (duplicate_slices=False, default)
+- Duplicate with deep-copied charts (duplicate_slices=True)
+- Source dashboard not found
+- Source dashboard access denied / copy forbidden
+- Title sanitization (XSS stripped, XSS-only title rejected)
+"""
+
+import logging
+from collections.abc import Iterator
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.utils.sanitization import (
+ LLM_CONTEXT_CLOSE_DELIMITER,
+ LLM_CONTEXT_OPEN_DELIMITER,
+)
+from superset.utils import json
+
+
+def _wrapped(value: str) -> str:
+ """Return the LLM-context-wrapped form a sanitized field should have."""
+ return f"{LLM_CONTEXT_OPEN_DELIMITER}\n{value}\n{LLM_CONTEXT_CLOSE_DELIMITER}"
+
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
+
+
+# ---------------------------------------------------------------------------
+# Fixtures
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+def mcp_server() -> object:
+ """Return the FastMCP app instance for use in MCP client tests."""
+ return mcp
+
+
+@pytest.fixture(autouse=True)
+def mock_auth() -> Iterator[MagicMock]:
+ """Mock authentication for all tests."""
+ with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user:
+ mock_user = Mock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_get_user.return_value = mock_user
+ yield mock_get_user
+
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+SOURCE_POSITIONS = {
Review Comment:
Declining. Test module-level constants are not explicitly annotated in Superset's test suite. The type of SOURCE_POSITIONS is clear from the literal.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3477036493
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,315 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload and a flag indicating whether the layout maps any
+ chart, so the caller can refuse to produce a silently empty copy.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+ return payload, _positions_reference_charts(positions)
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
+ return _sanitize_dashboard_info_for_llm_context(info), dashboard_url
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Duplicate dashboard",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def duplicate_dashboard(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> DuplicateDashboardResponse:
+ """
+ Duplicate an existing dashboard under a new title.
+
+ By default the copy references the same charts as the source.
+ Set duplicate_slices=true to also deep-copy every chart into new
+ chart objects owned by you, so edits to the copies never affect
+ the originals.
+
+ The source dashboard can be identified by numeric ID, UUID, or slug.
+ Returns the new dashboard's ID, title, and URL.
+ """
+ await ctx.info(
+ "Duplicating dashboard: dashboard_id=%s, duplicate_slices=%s"
+ % (request.dashboard_id, request.duplicate_slices)
+ )
+
+ from superset.commands.dashboard.copy import CopyDashboardCommand
+ from superset.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardCopyError,
+ DashboardForbiddenError,
+ DashboardInvalidError,
+ DashboardNotFoundError,
+ )
+ from superset.daos.dashboard import DashboardDAO
+
+ try:
+ with event_logger.log_context(action="mcp.duplicate_dashboard.lookup"):
+ try:
+ source = DashboardDAO.get_by_id_or_slug(str(request.dashboard_id))
+ except DashboardNotFoundError:
+ await ctx.warning(
+ "Dashboard not found for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' not found. "
+ "Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+ except DashboardAccessDeniedError:
+ await ctx.warning(
+ "Dashboard access denied for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"You don't have access to dashboard "
+ f"'{request.dashboard_id}', so it cannot be duplicated."
+ ),
+ )
+
+ data, layout_has_charts = _build_copy_payload(
+ source, request.dashboard_title, request.duplicate_slices
+ )
+
+ if getattr(source, "slices", None) and not layout_has_charts:
+ await ctx.warning(
+ "Source layout maps no charts; refusing to duplicate to "
+ "avoid an empty copy: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' has charts but its "
+ "saved layout is missing or invalid, so duplicating it "
+ "would produce a dashboard with no charts. Open and "
+ "re-save the source dashboard to repair its layout, then "
+ "try again."
+ ),
+ )
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.copy"):
+ new_dashboard = CopyDashboardCommand(source, data).run()
+
+ # Re-fetch with eager-loaded relationships to avoid lazy-loading on
+ # a session that the command's commit may have invalidated.
+ from sqlalchemy.orm import subqueryload
+
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ try:
+ new_dashboard = (
+ DashboardDAO.find_by_id(
+ new_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or new_dashboard
+ )
+ info, dashboard_url = _serialize_new_dashboard(new_dashboard)
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ new_dashboard.id,
+ exc_info=True,
+ )
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"
+ )
+ info = _sanitize_dashboard_info_for_llm_context(
+ DashboardInfo(
+ id=new_dashboard.id,
+ dashboard_title=request.dashboard_title,
+ url=dashboard_url,
+ )
+ )
Review Comment:
**Suggestion:** The SQLAlchemy error fallback for the post-copy re-fetch does not roll back the failed transaction before continuing. If the failed query leaves the session in an invalid transaction state, later DB use in the same request lifecycle (including logging hooks or downstream ORM access) can fail with "transaction rolled back"/"can't reconnect" errors. Add a `db.session.rollback()` in this `except SQLAlchemyError` block before building the minimal response, matching the existing recovery pattern used in other MCP dashboard tools. [missing cleanup]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP duplicate_dashboard leaves db.session in failed state on error.
- ⚠️ Subsequent ORM work in same lifecycle may hit rollback errors.
- ⚠️ Inconsistent with other MCP dashboard tools' SQLAlchemyError handling.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the MCP service (FastMCP app created in `superset/mcp_service/app.py:18-29`) and
call the `duplicate_dashboard` tool via an MCP client as done in tests (see
`tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:18-22`, which
calls `client.call_tool(\"duplicate_dashboard\", {\"request\": {...}})`).
2. Within `duplicate_dashboard()` in
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:152-165`, execution reaches
the post-copy re-fetch block at lines 238-248 where
`DashboardDAO.find_by_id(new_dashboard.id, query_options=[...])` is called to eagerly load
`slices` and `tags`.
3. Simulate a database/connection failure so that this re-fetch raises `SQLAlchemyError`
(e.g., by configuring the DB to drop connections or, in a unit test, patching
`DashboardDAO.find_by_id` to raise `SQLAlchemyError`). The exception is caught by the
`except SQLAlchemyError:` block at lines 249-255, which only logs a warning and builds a
minimal `DashboardInfo` response (lines 255-264) without calling `db.session.rollback()`.
4. Because the MCP auth wrapper `mcp_auth_hook` in `superset/mcp_service/auth.py:112-127`
only calls `_cleanup_session_on_error()` (which does `db.session.rollback()` and
`db.session.remove()` at `auth.py:27-37`) when an exception propagates out of the tool,
this caught `SQLAlchemyError` leaves the shared `superset.extensions.db.session` in a
failed-transaction state for the remainder of the tool call. In contrast, other MCP
dashboard tools explicitly roll back on the same "re-fetch failed" path (see
`generate_dashboard` at
`superset/mcp_service/dashboard/tool/generate_dashboard.py:452-461` and
`add_chart_to_existing_dashboard` at
`superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:567-579`),
demonstrating that the intended design is to clear the session after such errors; the
missing rollback here is an inconsistent error-handling bug that would cause any
additional DB usage in this tool (or shared-session consumers during the same lifecycle)
to hit "transaction has been rolled back" errors until a later cleanup.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0317cbedc9cb4e6a8ca97367f4005941&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=0317cbedc9cb4e6a8ca97367f4005941&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/duplicate_dashboard.py
**Line:** 249:264
**Comment:**
*Missing Cleanup: The SQLAlchemy error fallback for the post-copy re-fetch does not roll back the failed transaction before continuing. If the failed query leaves the session in an invalid transaction state, later DB use in the same request lifecycle (including logging hooks or downstream ORM access) can fail with "transaction rolled back"/"can't reconnect" errors. Add a `db.session.rollback()` in this `except SQLAlchemyError` block before building the minimal response, matching the existing recovery pattern used in other MCP dashboard tools.
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%2F40959&comment_hash=cb993ed6a53f050f9175f8ee66b230ed814e0394bf0a256f85f1014ed837770d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=cb993ed6a53f050f9175f8ee66b230ed814e0394bf0a256f85f1014ed837770d&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3482446213
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,376 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload and a flag indicating whether the layout maps any
+ chart, so the caller can refuse to produce a silently empty copy.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+ return payload, _positions_reference_charts(positions)
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
+ return _sanitize_dashboard_info_for_llm_context(info), dashboard_url
+
+
+def _safe_rollback(context_label: str) -> None:
+ """Roll back the current DB session, swallowing rollback failures.
+
+ A failed operation can leave the shared session in an invalid
+ transaction state; rolling back keeps later ORM use in the same request
+ lifecycle from inheriting the broken transaction.
+ """
+ from superset import db
+
+ try:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ except SQLAlchemyError:
+ logger.warning(
+ "Database rollback failed during %s error handling",
+ context_label,
+ exc_info=True,
+ )
+
+
+def _refetch_and_serialize(
+ new_dashboard: Any, dashboard_title: str
+) -> tuple[DashboardInfo, str]:
+ """Re-fetch the new dashboard with eager-loaded relationships.
+
+ The eager load avoids lazy-loading on a session the command's commit may
+ have invalidated. If the re-fetch fails, the failed transaction is rolled
+ back and a minimal response is returned instead.
+ """
+ from sqlalchemy.orm import subqueryload
+
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ try:
+ dashboard = (
+ DashboardDAO.find_by_id(
+ new_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or new_dashboard
+ )
+ return _serialize_new_dashboard(dashboard)
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ new_dashboard.id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard re-fetch")
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"
+ )
+ info = _sanitize_dashboard_info_for_llm_context(
+ DashboardInfo(
+ id=new_dashboard.id,
+ dashboard_title=dashboard_title,
+ url=dashboard_url,
+ )
+ )
+ return info, dashboard_url
+
+
+async def _resolve_source(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> tuple[Any, DuplicateDashboardResponse | None]:
+ """Resolve and authorize the source dashboard.
+
+ Returns ``(source, None)`` on success, or ``(None, error_response)`` when
+ the dashboard is missing or inaccessible.
+ """
+ from superset.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardNotFoundError,
+ )
+ from superset.daos.dashboard import DashboardDAO
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.lookup"):
+ try:
+ return DashboardDAO.get_by_id_or_slug(str(request.dashboard_id)), None
+ except DashboardNotFoundError:
+ await ctx.warning(
+ "Dashboard not found for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' not found. "
+ "Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+ except DashboardAccessDeniedError:
+ await ctx.warning(
+ "Dashboard access denied for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"You don't have access to dashboard "
+ f"'{request.dashboard_id}', so it cannot be duplicated."
+ ),
+ )
Review Comment:
**Suggestion:** Database lookup failures while resolving the source dashboard are not handled as structured tool errors. If `DashboardDAO.get_by_id_or_slug` raises `SQLAlchemyError` (for example, transient DB/session failures), the exception falls through to the outer `except Exception` and is re-raised, causing a hard MCP tool failure instead of a normal `DuplicateDashboardResponse`. Catch operational DB errors in source resolution (or convert the outer catch to a structured response) so callers always get a contract-consistent error payload. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ `duplicate_dashboard` MCP tool crashes on DB lookup failures.
- ⚠️ MCP clients receive internal errors, not structured error payloads.
- ⚠️ LLM workflows must special-case unexpected tool failures.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with the MCP service enabled and ensure the MCP dashboard tools module
is loaded; the `duplicate_dashboard` tool is exported in
`superset/mcp_service/dashboard/tool/__init__.py:18-32` and implemented in
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:259-372` (per PR diff).
2. From an MCP client, invoke the `duplicate_dashboard` tool with any valid-looking
payload (for example, `{"dashboard_id": 1, "dashboard_title": "Copy"}`), which routes into
`duplicate_dashboard()` at
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:259-272`, where the first
operation inside the `try` block is `source, error_response = await
_resolve_source(request, ctx)` at lines 276-279.
3. Inside `_resolve_source()` at
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:208-246`, the code enters the
`with event_logger.log_context(...):` block and executes
`DashboardDAO.get_by_id_or_slug(str(request.dashboard_id))` at line 224. This DAO method,
implemented in `superset/daos/dashboard.py:17-33`, issues a
`db.session.query(Dashboard)...one_or_none()`. If the database is temporarily unavailable
or the session is broken (e.g., connection loss, transaction error), SQLAlchemy can raise
an `SQLAlchemyError`/`OperationalError` from this call.
4. Because `_resolve_source()` only catches `DashboardNotFoundError` and
`DashboardAccessDeniedError` (lines 225-246) and does not catch `SQLAlchemyError`, the
database error propagates back into `duplicate_dashboard()`, where it is caught by the
broad `except Exception as exc:` block at
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:371-376`. That block logs via
`await ctx.error(...)` but then `raise`s the exception instead of returning a
`DuplicateDashboardResponse`, causing the MCP tool invocation to fail with an unstructured
internal error rather than the contract-consistent `DuplicateDashboardResponse(error=...)`
that all other failure paths in this tool return.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ef7debd271b7480f81913379469af06e&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=ef7debd271b7480f81913379469af06e&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/duplicate_dashboard.py
**Line:** 223:246
**Comment:**
*Api Mismatch: Database lookup failures while resolving the source dashboard are not handled as structured tool errors. If `DashboardDAO.get_by_id_or_slug` raises `SQLAlchemyError` (for example, transient DB/session failures), the exception falls through to the outer `except Exception` and is re-raised, causing a hard MCP tool failure instead of a normal `DuplicateDashboardResponse`. Catch operational DB errors in source resolution (or convert the outer catch to a structured response) so callers always get a contract-consistent error payload.
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%2F40959&comment_hash=64ffb573c273883357e41947d62d9fdf51ffe5f717cfb0fa6ed1f22c2ab48907&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=64ffb573c273883357e41947d62d9fdf51ffe5f717cfb0fa6ed1f22c2ab48907&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3413990887
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,277 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> dict[str, Any]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ return {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
Review Comment:
Fixed in 5e9923e7b3. The response now routes the new dashboard's `DashboardInfo` through `_sanitize_dashboard_info_for_llm_context` (same path as `dashboard_serializer`/`serialize_dashboard_object`), so title/description/chart/tag text is wrapped as untrusted before reaching LLM context. Added a test asserting the response title comes back LLM-context-wrapped.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3493547947
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -914,6 +914,138 @@ class GenerateDashboardResponse(BaseModel):
)
+class DuplicateDashboardRequest(BaseModel):
+ """Request schema for duplicating an existing dashboard."""
+
+ model_config = ConfigDict(populate_by_name=True)
+
+ dashboard_id: Annotated[
+ int | str,
+ Field(
+ description=(
+ "Source dashboard identifier - can be numeric ID, UUID string, or slug"
+ )
+ ),
+ ]
Review Comment:
**Suggestion:** For this new API request schema, remove support for integer identifiers and accept only UUID-based string identifiers (optionally slug) so the interface does not expose auto-increment primary-key IDs. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new request schema explicitly accepts a numeric dashboard ID, which exposes an auto-incrementing primary-key style identifier in a public API surface. That matches the UUID-primary-key rule’s intent to avoid integer IDs for new public-facing APIs, so the suggestion is verified.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=81f3d22d51964f7ea33b1cf3af20d08d&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=81f3d22d51964f7ea33b1cf3af20d08d&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:** 922:929
**Comment:**
*Custom Rule: For this new API request schema, remove support for integer identifiers and accept only UUID-based string identifiers (optionally slug) so the interface does not expose auto-increment primary-key IDs.
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%2F40959&comment_hash=2b8caa1c5f0764151b39641ebc52e06e56e732e7ad8c4768c90d64d389efcdf8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=2b8caa1c5f0764151b39641ebc52e06e56e732e7ad8c4768c90d64d389efcdf8&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3493322124
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,414 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool, str | None]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload, a flag indicating whether the layout maps any
+ chart (so the caller can refuse to produce a silently empty copy),
+ and an optional warning string when ``json_metadata`` could not be
+ decoded. The caller surfaces the warning rather than hard-failing:
+ metadata loss degrades aesthetic settings (colors, native filters)
+ but does not corrupt the copy's chart content.
+ """
+ metadata_warning: str | None = None
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) could not "
+ "be decoded and were not copied. The duplicate uses default "
+ "settings for colors, native filters, and other metadata. "
+ "Open and re-save the source dashboard to repair its settings."
+ )
+ if not isinstance(metadata, dict):
+ metadata = {}
+ if metadata_warning is None:
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) were not "
+ "a valid JSON object and were not copied. The duplicate uses "
+ "default settings for colors, native filters, and other metadata."
+ )
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+ return payload, _positions_reference_charts(positions), metadata_warning
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
+ return _sanitize_dashboard_info_for_llm_context(info), dashboard_url
+
+
+def _safe_rollback(context_label: str) -> None:
+ """Roll back the current DB session, swallowing rollback failures.
+
+ A failed operation can leave the shared session in an invalid
+ transaction state; rolling back keeps later ORM use in the same request
+ lifecycle from inheriting the broken transaction.
+ """
+ from superset import db
+
+ try:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ except SQLAlchemyError:
+ logger.warning(
+ "Database rollback failed during %s error handling",
+ context_label,
+ exc_info=True,
+ )
+
+
+def _refetch_and_serialize(
+ new_dashboard: Any, dashboard_title: str
+) -> tuple[DashboardInfo, str]:
+ """Re-fetch the new dashboard with eager-loaded relationships.
+
+ The eager load avoids lazy-loading on a session the command's commit may
+ have invalidated. If the re-fetch fails, the failed transaction is rolled
+ back and a minimal response is returned instead.
+ """
+ from sqlalchemy.orm import subqueryload
+
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ try:
+ dashboard = (
+ DashboardDAO.find_by_id(
+ new_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or new_dashboard
+ )
+ return _serialize_new_dashboard(dashboard)
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ new_dashboard.id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard re-fetch")
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"
+ )
+ info = _sanitize_dashboard_info_for_llm_context(
+ DashboardInfo(
+ id=new_dashboard.id,
+ dashboard_title=dashboard_title,
+ url=dashboard_url,
+ )
+ )
+ return info, dashboard_url
+
+
+async def _resolve_source(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> tuple[Any, DuplicateDashboardResponse | None]:
+ """Resolve and authorize the source dashboard.
+
+ Returns ``(source, None)`` on success, or ``(None, error_response)`` when
+ the dashboard is missing or inaccessible.
+ """
+ from superset.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardNotFoundError,
+ )
+ from superset.daos.dashboard import DashboardDAO
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.lookup"):
+ try:
+ return DashboardDAO.get_by_id_or_slug(str(request.dashboard_id)), None
+ except DashboardNotFoundError:
+ await ctx.warning(
+ "Dashboard not found for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' not found. "
+ "Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+ except DashboardAccessDeniedError:
+ await ctx.warning(
+ "Dashboard access denied for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"You don't have access to dashboard "
+ f"'{request.dashboard_id}', so it cannot be duplicated."
+ ),
+ )
+ except SQLAlchemyError:
+ # Transient DB/session failures during lookup must surface as a
+ # structured response, not a hard tool failure. The raw error is
+ # logged with a traceback; the response stays generic because
+ # ``str(exc)`` can leak table/column/constraint names.
+ logger.error(
+ "Database error resolving dashboard %s for duplication",
+ request.dashboard_id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard lookup")
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' could not be "
+ "duplicated due to a database error. Please try again."
+ ),
+ )
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Duplicate dashboard",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def duplicate_dashboard(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> DuplicateDashboardResponse:
+ """
+ Duplicate an existing dashboard under a new title.
+
+ By default the copy references the same charts as the source.
+ Set duplicate_slices=true to also deep-copy every chart into new
+ chart objects owned by you, so edits to the copies never affect
+ the originals.
+
+ The source dashboard can be identified by numeric ID, UUID, or slug.
+ Returns the new dashboard's ID, title, and URL.
+ """
+ await ctx.info(
+ "Duplicating dashboard: dashboard_id=%s, duplicate_slices=%s"
+ % (request.dashboard_id, request.duplicate_slices)
+ )
+
+ from superset.commands.dashboard.copy import CopyDashboardCommand
+ from superset.commands.dashboard.exceptions import (
+ DashboardCopyError,
+ DashboardForbiddenError,
+ DashboardInvalidError,
+ )
+
+ try:
+ source, error_response = await _resolve_source(request, ctx)
+ if error_response is not None:
+ return error_response
+
+ data, layout_has_charts, metadata_warning = _build_copy_payload(
+ source, request.dashboard_title, request.duplicate_slices
+ )
+
+ if getattr(source, "slices", None) and not layout_has_charts:
+ await ctx.warning(
+ "Source layout maps no charts; refusing to duplicate to "
+ "avoid an empty copy: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' has charts but its "
+ "saved layout is missing or invalid, so duplicating it "
+ "would produce a dashboard with no charts. Open and "
+ "re-save the source dashboard to repair its layout, then "
+ "try again."
+ ),
+ )
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.copy"):
+ new_dashboard = CopyDashboardCommand(source, data).run()
+
+ info, dashboard_url = _refetch_and_serialize(
+ new_dashboard, request.dashboard_title
+ )
+
+ logger.info(
+ "Duplicated dashboard %s into dashboard %s (duplicate_slices=%s)",
+ request.dashboard_id,
+ new_dashboard.id,
+ request.duplicate_slices,
+ )
+
+ warnings = list(request.sanitization_warnings)
+ if metadata_warning:
+ warnings.append(metadata_warning)
+
+ return DuplicateDashboardResponse(
+ dashboard=info,
+ dashboard_url=dashboard_url,
+ duplicated_slices=request.duplicate_slices,
+ warnings=warnings,
+ )
+
+ except DashboardForbiddenError:
+ await ctx.error(
+ "Dashboard duplication forbidden: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"You don't have permission to duplicate dashboard "
+ f"'{request.dashboard_id}'."
+ ),
+ )
+ except DashboardInvalidError:
+ return DuplicateDashboardResponse(
+ error=(
+ "Dashboard duplication parameters were invalid. "
+ "Provide a non-empty dashboard_title."
+ ),
+ )
+ except DashboardCopyError as exc:
+ _safe_rollback("dashboard duplication")
+ await ctx.error("Dashboard duplication failed: %s" % (str(exc),))
+ return DuplicateDashboardResponse(
+ error=f"Failed to duplicate dashboard: {exc}",
+ )
+ except (ValueError, TypeError) as exc:
+ # Malformed stored metadata on the source (e.g. invalid json_metadata
+ # or params) surfaces as a JSON/parse error from CopyDashboardCommand
+ # rather than a DashboardCopyError, because the transaction handler
+ # only wraps SQLAlchemyError. Return a structured response instead of
+ # letting it escape as a hard tool failure.
+ _safe_rollback("dashboard duplication")
+ await ctx.error(
+ "Dashboard duplication failed parsing source metadata: %s: %s"
+ % (type(exc).__name__, str(exc))
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' could not be duplicated "
+ "because its stored metadata is invalid. Open and re-save the "
+ "source dashboard to repair it, then try again."
+ ),
+ )
+ except Exception as exc:
+ await ctx.error(
+ "Unexpected error duplicating dashboard: %s: %s"
+ % (type(exc).__name__, str(exc))
+ )
+ raise
Review Comment:
**Suggestion:** Malformed `position_json` structures can raise `KeyError` inside `DashboardDAO.set_dash_metadata` (for example when a `CHART` node has no `meta.chartId`), but this tool only handles `ValueError`/`TypeError` and then re-raises in the broad `except Exception` path. That causes a hard tool failure instead of the intended structured error response. Include `KeyError` (and keep rollback) in the structured metadata-error handling branch so corrupted source dashboards do not crash the tool. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ duplicate_dashboard crashes when source layout metadata is malformed.
- ⚠️ AI workflows duplicating dashboards fail on corrupted layouts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. An MCP client invokes the `duplicate_dashboard` tool entrypoint in
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:293-306` with a
`DuplicateDashboardRequest` targeting a dashboard whose stored `position_json` contains a
`CHART` node missing `meta.chartId` (corrupted layout metadata in the DB row).
2. Inside `duplicate_dashboard` (file
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:319-327`),
`_build_copy_payload()` at `duplicate_dashboard.py:64-119` loads `source.json_metadata`
and `source.position_json` and writes the decoded layout into `metadata["positions"]`
without validating that every `CHART` object has a `meta.chartId` key.
3. The tool constructs and runs `CopyDashboardCommand` at
`duplicate_dashboard.py:343-345`; `CopyDashboardCommand.run()` in
`superset/commands/dashboard/copy.py:35-43` is wrapped in `transaction`
(`superset/utils/decorators.py:235-260`) and calls
`DashboardDAO.copy_dashboard(self._original_dash, self._properties)` at `copy.py:43`,
which in turn loads `metadata = json.loads(data["json_metadata"])` and calls
`DashboardDAO.set_dash_metadata(dash, metadata, old_to_new_slice_ids)` in
`superset/daos/dashboard.py:69-105`.
4. In `DashboardDAO.set_dash_metadata` (`superset/daos/dashboard.py:84-116`), the loop
over `positions.values()` executes `obj["type"] == "CHART" and obj["meta"]["chartId"]` at
lines 109-112; for a `CHART` object missing the `meta` or `chartId` key,
`obj["meta"]["chartId"]` raises `KeyError`, which is not a `SQLAlchemyError` and thus is
re-raised by the transaction error handler (`on_error` in
`superset/utils/decorators.py:210-233`). This `KeyError` bubbles back to
`duplicate_dashboard`, is caught only by the broad `except Exception as exc` block at
`duplicate_dashboard.py:409-414`, logged via `ctx.error`, and then re-raised at line 414,
causing a hard MCP tool failure instead of returning the structured
`DuplicateDashboardResponse` metadata-error branch at `duplicate_dashboard.py:391-408`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b02ea87185d944dcb04555e98e4b4458&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=b02ea87185d944dcb04555e98e4b4458&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/duplicate_dashboard.py
**Line:** 391:414
**Comment:**
*Incomplete Implementation: Malformed `position_json` structures can raise `KeyError` inside `DashboardDAO.set_dash_metadata` (for example when a `CHART` node has no `meta.chartId`), but this tool only handles `ValueError`/`TypeError` and then re-raises in the broad `except Exception` path. That causes a hard tool failure instead of the intended structured error response. Include `KeyError` (and keep rollback) in the structured metadata-error handling branch so corrupted source dashboards do not crash the tool.
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%2F40959&comment_hash=0e13f57481ed10754d2f86b4a0fdab2e2bdf99839bc15bfa859fe5b75a45c4aa&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=0e13f57481ed10754d2f86b4a0fdab2e2bdf99839bc15bfa859fe5b75a45c4aa&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40959:
URL: https://github.com/apache/superset/pull/40959#issuecomment-4675877886
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40959:
URL: https://github.com/apache/superset/pull/40959#issuecomment-4703674515
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 8ee4891bc9d1320fc6092860c6ac4bf38d1875dc |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a2f4ce79d09330009ca4a99 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-40959--superset-docs-preview.netlify.app](https://deploy-preview-40959--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoAgEADAv-y5QFKK7Qc9Y901DDQXs0NEf8_bHOYFrSVrgxVgAF_p5NidWUe5NR1MLYxCV_SFqvQiQVN5Nulppml3HBYUg9YaY5DJESJ8Pw) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40959--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #40959:
URL: https://github.com/apache/superset/pull/40959
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3493539119
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,414 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool, str | None]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload, a flag indicating whether the layout maps any
+ chart (so the caller can refuse to produce a silently empty copy),
+ and an optional warning string when ``json_metadata`` could not be
+ decoded. The caller surfaces the warning rather than hard-failing:
+ metadata loss degrades aesthetic settings (colors, native filters)
+ but does not corrupt the copy's chart content.
+ """
+ metadata_warning: str | None = None
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) could not "
+ "be decoded and were not copied. The duplicate uses default "
+ "settings for colors, native filters, and other metadata. "
+ "Open and re-save the source dashboard to repair its settings."
+ )
+ if not isinstance(metadata, dict):
+ metadata = {}
+ if metadata_warning is None:
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) were not "
+ "a valid JSON object and were not copied. The duplicate uses "
+ "default settings for colors, native filters, and other metadata."
+ )
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+ return payload, _positions_reference_charts(positions), metadata_warning
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
+ return _sanitize_dashboard_info_for_llm_context(info), dashboard_url
+
+
+def _safe_rollback(context_label: str) -> None:
+ """Roll back the current DB session, swallowing rollback failures.
+
+ A failed operation can leave the shared session in an invalid
+ transaction state; rolling back keeps later ORM use in the same request
+ lifecycle from inheriting the broken transaction.
+ """
+ from superset import db
+
+ try:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ except SQLAlchemyError:
+ logger.warning(
+ "Database rollback failed during %s error handling",
+ context_label,
+ exc_info=True,
+ )
+
+
+def _refetch_and_serialize(
+ new_dashboard: Any, dashboard_title: str
+) -> tuple[DashboardInfo, str]:
+ """Re-fetch the new dashboard with eager-loaded relationships.
+
+ The eager load avoids lazy-loading on a session the command's commit may
+ have invalidated. If the re-fetch fails, the failed transaction is rolled
+ back and a minimal response is returned instead.
+ """
+ from sqlalchemy.orm import subqueryload
+
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ try:
+ dashboard = (
+ DashboardDAO.find_by_id(
+ new_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or new_dashboard
+ )
+ return _serialize_new_dashboard(dashboard)
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ new_dashboard.id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard re-fetch")
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"
+ )
+ info = _sanitize_dashboard_info_for_llm_context(
+ DashboardInfo(
+ id=new_dashboard.id,
+ dashboard_title=dashboard_title,
+ url=dashboard_url,
+ )
+ )
+ return info, dashboard_url
+
+
+async def _resolve_source(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> tuple[Any, DuplicateDashboardResponse | None]:
+ """Resolve and authorize the source dashboard.
+
+ Returns ``(source, None)`` on success, or ``(None, error_response)`` when
+ the dashboard is missing or inaccessible.
+ """
+ from superset.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardNotFoundError,
+ )
+ from superset.daos.dashboard import DashboardDAO
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.lookup"):
+ try:
+ return DashboardDAO.get_by_id_or_slug(str(request.dashboard_id)), None
+ except DashboardNotFoundError:
+ await ctx.warning(
+ "Dashboard not found for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' not found. "
+ "Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+ except DashboardAccessDeniedError:
+ await ctx.warning(
+ "Dashboard access denied for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"You don't have access to dashboard "
+ f"'{request.dashboard_id}', so it cannot be duplicated."
+ ),
+ )
+ except SQLAlchemyError:
+ # Transient DB/session failures during lookup must surface as a
+ # structured response, not a hard tool failure. The raw error is
+ # logged with a traceback; the response stays generic because
+ # ``str(exc)`` can leak table/column/constraint names.
+ logger.error(
+ "Database error resolving dashboard %s for duplication",
+ request.dashboard_id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard lookup")
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' could not be "
+ "duplicated due to a database error. Please try again."
+ ),
+ )
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Duplicate dashboard",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def duplicate_dashboard(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> DuplicateDashboardResponse:
+ """
+ Duplicate an existing dashboard under a new title.
+
+ By default the copy references the same charts as the source.
+ Set duplicate_slices=true to also deep-copy every chart into new
+ chart objects owned by you, so edits to the copies never affect
+ the originals.
+
+ The source dashboard can be identified by numeric ID, UUID, or slug.
+ Returns the new dashboard's ID, title, and URL.
+ """
+ await ctx.info(
+ "Duplicating dashboard: dashboard_id=%s, duplicate_slices=%s"
+ % (request.dashboard_id, request.duplicate_slices)
+ )
+
+ from superset.commands.dashboard.copy import CopyDashboardCommand
+ from superset.commands.dashboard.exceptions import (
+ DashboardCopyError,
+ DashboardForbiddenError,
+ DashboardInvalidError,
+ )
+
+ try:
+ source, error_response = await _resolve_source(request, ctx)
+ if error_response is not None:
+ return error_response
+
+ data, layout_has_charts, metadata_warning = _build_copy_payload(
+ source, request.dashboard_title, request.duplicate_slices
+ )
+
+ if getattr(source, "slices", None) and not layout_has_charts:
+ await ctx.warning(
+ "Source layout maps no charts; refusing to duplicate to "
+ "avoid an empty copy: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' has charts but its "
+ "saved layout is missing or invalid, so duplicating it "
+ "would produce a dashboard with no charts. Open and "
+ "re-save the source dashboard to repair its layout, then "
+ "try again."
+ ),
+ )
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.copy"):
+ new_dashboard = CopyDashboardCommand(source, data).run()
+
+ info, dashboard_url = _refetch_and_serialize(
+ new_dashboard, request.dashboard_title
+ )
+
+ logger.info(
+ "Duplicated dashboard %s into dashboard %s (duplicate_slices=%s)",
+ request.dashboard_id,
+ new_dashboard.id,
+ request.duplicate_slices,
+ )
+
+ warnings = list(request.sanitization_warnings)
+ if metadata_warning:
+ warnings.append(metadata_warning)
+
+ return DuplicateDashboardResponse(
+ dashboard=info,
+ dashboard_url=dashboard_url,
+ duplicated_slices=request.duplicate_slices,
+ warnings=warnings,
+ )
+
+ except DashboardForbiddenError:
+ await ctx.error(
+ "Dashboard duplication forbidden: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"You don't have permission to duplicate dashboard "
+ f"'{request.dashboard_id}'."
+ ),
+ )
+ except DashboardInvalidError:
+ return DuplicateDashboardResponse(
+ error=(
+ "Dashboard duplication parameters were invalid. "
+ "Provide a non-empty dashboard_title."
+ ),
+ )
+ except DashboardCopyError as exc:
+ _safe_rollback("dashboard duplication")
+ await ctx.error("Dashboard duplication failed: %s" % (str(exc),))
+ return DuplicateDashboardResponse(
+ error=f"Failed to duplicate dashboard: {exc}",
+ )
+ except (ValueError, TypeError) as exc:
+ # Malformed stored metadata on the source (e.g. invalid json_metadata
+ # or params) surfaces as a JSON/parse error from CopyDashboardCommand
+ # rather than a DashboardCopyError, because the transaction handler
+ # only wraps SQLAlchemyError. Return a structured response instead of
+ # letting it escape as a hard tool failure.
+ _safe_rollback("dashboard duplication")
+ await ctx.error(
+ "Dashboard duplication failed parsing source metadata: %s: %s"
+ % (type(exc).__name__, str(exc))
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' could not be duplicated "
+ "because its stored metadata is invalid. Open and re-save the "
+ "source dashboard to repair it, then try again."
+ ),
+ )
+ except Exception as exc:
+ await ctx.error(
+ "Unexpected error duplicating dashboard: %s: %s"
+ % (type(exc).__name__, str(exc))
+ )
+ raise
Review Comment:
Fixed in d8e1d7e6fd. Added KeyError to the except (ValueError, TypeError) handler (now except (ValueError, TypeError, KeyError)) and added a regression test test_key_error_in_copy_command_returns_structured_error. Also added dashboard_id to the ctx.error log message in this handler for consistency with all other error branches.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -914,6 +914,138 @@ class GenerateDashboardResponse(BaseModel):
)
+class DuplicateDashboardRequest(BaseModel):
+ """Request schema for duplicating an existing dashboard."""
+
+ model_config = ConfigDict(populate_by_name=True)
Review Comment:
Declining. Pydantic v2 models use model_config = ConfigDict(...) as a class-level sentinel — the type is known to Pydantic's metaclass and mypy infers it correctly. Annotating it as ClassVar[ConfigDict] is not done anywhere in the Superset codebase and would add noise without improving type safety.
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,414 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
Review Comment:
Declining. Module-level loggers are universally left unannotated in the Superset codebase. logging.getLogger returns logging.Logger and mypy infers this correctly. Adding an explicit annotation would be inconsistent with every other file in the project.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3493693856
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -914,6 +914,138 @@ class GenerateDashboardResponse(BaseModel):
)
+class DuplicateDashboardRequest(BaseModel):
+ """Request schema for duplicating an existing dashboard."""
+
+ model_config = ConfigDict(populate_by_name=True)
+
+ dashboard_id: Annotated[
+ int | str,
+ Field(
+ description=(
+ "Source dashboard identifier - can be numeric ID, UUID string, or slug"
+ )
+ ),
+ ]
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> For MCP request schemas, keep identifier fields aligned with the backing DAO/API capabilities; do not force UUID-only validation when existing code paths intentionally accept numeric IDs, UUIDs, and slugs.
**Applied to:**
- `superset/mcp_service/**`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3482394220
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,324 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload and a flag indicating whether the layout maps any
+ chart, so the caller can refuse to produce a silently empty copy.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+ return payload, _positions_reference_charts(positions)
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
+ return _sanitize_dashboard_info_for_llm_context(info), dashboard_url
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Duplicate dashboard",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def duplicate_dashboard(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> DuplicateDashboardResponse:
+ """
+ Duplicate an existing dashboard under a new title.
+
+ By default the copy references the same charts as the source.
+ Set duplicate_slices=true to also deep-copy every chart into new
+ chart objects owned by you, so edits to the copies never affect
+ the originals.
+
+ The source dashboard can be identified by numeric ID, UUID, or slug.
+ Returns the new dashboard's ID, title, and URL.
+ """
+ await ctx.info(
+ "Duplicating dashboard: dashboard_id=%s, duplicate_slices=%s"
+ % (request.dashboard_id, request.duplicate_slices)
+ )
+
+ from superset.commands.dashboard.copy import CopyDashboardCommand
+ from superset.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardCopyError,
+ DashboardForbiddenError,
+ DashboardInvalidError,
+ DashboardNotFoundError,
+ )
+ from superset.daos.dashboard import DashboardDAO
+
+ try:
+ with event_logger.log_context(action="mcp.duplicate_dashboard.lookup"):
+ try:
+ source = DashboardDAO.get_by_id_or_slug(str(request.dashboard_id))
+ except DashboardNotFoundError:
+ await ctx.warning(
+ "Dashboard not found for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' not found. "
+ "Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+ except DashboardAccessDeniedError:
+ await ctx.warning(
+ "Dashboard access denied for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"You don't have access to dashboard "
+ f"'{request.dashboard_id}', so it cannot be duplicated."
+ ),
+ )
+
+ data, layout_has_charts = _build_copy_payload(
+ source, request.dashboard_title, request.duplicate_slices
+ )
+
+ if getattr(source, "slices", None) and not layout_has_charts:
+ await ctx.warning(
+ "Source layout maps no charts; refusing to duplicate to "
+ "avoid an empty copy: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' has charts but its "
+ "saved layout is missing or invalid, so duplicating it "
+ "would produce a dashboard with no charts. Open and "
+ "re-save the source dashboard to repair its layout, then "
+ "try again."
+ ),
+ )
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.copy"):
+ new_dashboard = CopyDashboardCommand(source, data).run()
+
+ # Re-fetch with eager-loaded relationships to avoid lazy-loading on
+ # a session that the command's commit may have invalidated.
+ from sqlalchemy.orm import subqueryload
+
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ try:
+ new_dashboard = (
+ DashboardDAO.find_by_id(
+ new_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or new_dashboard
+ )
+ info, dashboard_url = _serialize_new_dashboard(new_dashboard)
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ new_dashboard.id,
+ exc_info=True,
+ )
+ from superset import db
+
+ 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,
+ )
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"
+ )
+ info = _sanitize_dashboard_info_for_llm_context(
+ DashboardInfo(
+ id=new_dashboard.id,
+ dashboard_title=request.dashboard_title,
+ url=dashboard_url,
+ )
+ )
+
+ logger.info(
+ "Duplicated dashboard %s into dashboard %s (duplicate_slices=%s)",
+ request.dashboard_id,
+ new_dashboard.id,
+ request.duplicate_slices,
+ )
+
+ return DuplicateDashboardResponse(
+ dashboard=info,
+ dashboard_url=dashboard_url,
+ duplicated_slices=request.duplicate_slices,
+ warnings=list(request.sanitization_warnings),
+ )
+
+ except DashboardForbiddenError:
+ await ctx.error(
+ "Dashboard duplication forbidden: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"You don't have permission to duplicate dashboard "
+ f"'{request.dashboard_id}'."
+ ),
+ )
+ except DashboardInvalidError:
+ return DuplicateDashboardResponse(
+ error=(
+ "Dashboard duplication parameters were invalid. "
+ "Provide a non-empty dashboard_title."
+ ),
+ )
+ except DashboardCopyError as exc:
+ from superset import db
+
+ try:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ except SQLAlchemyError:
+ logger.warning(
+ "Database rollback failed during error handling", exc_info=True
+ )
+ await ctx.error("Dashboard duplication failed: %s" % (str(exc),))
+ return DuplicateDashboardResponse(
+ error=f"Failed to duplicate dashboard: {exc}",
+ )
+ except Exception as exc:
Review Comment:
Good catch — fixed in d36e38cb55. You're right that the transaction handler only wraps `SQLAlchemyError` (its `catches` stays `(SQLAlchemyError,)`), so a `ValueError`/`JSONDecodeError` raised by `set_dash_metadata` → `params_dict` re-propagates unchanged and bypassed the `DashboardCopyError` handler. The tool now catches `(ValueError, TypeError)` and returns a structured `DuplicateDashboardResponse` error ("its stored metadata is invalid … re-save the source dashboard") with a session rollback, instead of letting it escape as a hard tool failure. Added a regression test (`test_malformed_metadata_returns_structured_error`). The handler logic was also split into `_resolve_source`/`_refetch_and_serialize`/`_safe_rollback` helpers to stay under the C901 complexity limit.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3410570192
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,277 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> dict[str, Any]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ return {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
Review Comment:
**Suggestion:** The tool returns dashboard, chart, and tag text fields without LLM-context sanitization, unlike the standard dashboard serializers. Since these fields are user-controlled, this can surface prompt-injection payloads directly in MCP structured responses. Sanitize the response payload (or reuse the existing sanitized dashboard serializer path) before returning it. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MCP duplicate_dashboard returns unsanitized dashboard text to LLM.
- ❌ User dashboards can inject prompts into MCP tool output.
- ⚠️ Inconsistent with sanitized dashboard serializers in schemas module.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In Superset, create or edit a dashboard so that `dashboard_title` or `description` on
the `Dashboard` model (`superset/models/dashboard.py:131-143`) contains a prompt-injection
payload (for example, instructions to the LLM), and add charts and tags whose
`slice_name`/`description` and `Tag.name`/`Tag.description` also contain crafted text.
2. From an MCP client, call the `duplicate_dashboard` tool implemented in
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:131-144`, passing this
dashboard's identifier in `DuplicateDashboardRequest.dashboard_id`; the tool copies the
dashboard and then calls `_serialize_new_dashboard(new_dashboard)` at
`duplicate_dashboard.py:212`.
3. `_serialize_new_dashboard` at `duplicate_dashboard.py:84-118` builds a `DashboardInfo`
instance directly from ORM attributes: `dashboard_title`, `slug`, `description`, `css`,
and iterates `dashboard.tags` using `serialize_tag_object`
(`superset/mcp_service/dashboard/schemas.py:137-147`) and `dashboard.slices` using
`serialize_chart_summary` (`superset/mcp_service/dashboard/schemas.py:5-29`), all without
passing the data through `_sanitize_dashboard_info_for_llm_context`.
4. The populated `DashboardInfo` is returned to the MCP client inside
`DuplicateDashboardResponse` at `duplicate_dashboard.py:235-240` with `dashboard=info`;
unlike other dashboard serializers such as `dashboard_serializer` and
`serialize_dashboard_object` in `superset/mcp_service/dashboard/schemas.py:41-120`, which
wrap their `DashboardInfo` via `_sanitize_dashboard_info_for_llm_context` (lines 43-38 in
that file) to call `sanitize_for_llm_context` and `escape_llm_context_delimiters`, this
path leaves all user-controlled fields unsanitized, exposing prompt-injection payloads
directly in structured MCP responses consumed by the LLM.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9fd6f20a60cb4c28bd6e86e2639d49d1&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=9fd6f20a60cb4c28bd6e86e2639d49d1&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/duplicate_dashboard.py
**Line:** 90:117
**Comment:**
*Security: The tool returns dashboard, chart, and tag text fields without LLM-context sanitization, unlike the standard dashboard serializers. Since these fields are user-controlled, this can surface prompt-injection payloads directly in MCP structured responses. Sanitize the response payload (or reuse the existing sanitized dashboard serializer path) before returning it.
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%2F40959&comment_hash=6bd945206f2ca5c2c1f825ab8b996fc85c19fdf2751b14852a4fc99ec5410b81&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=6bd945206f2ca5c2c1f825ab8b996fc85c19fdf2751b14852a4fc99ec5410b81&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,277 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> dict[str, Any]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
Review Comment:
**Suggestion:** Falling back to an empty `positions` object when `position_json` is missing/invalid will silently drop all charts in the copied dashboard. `DashboardDAO.copy_dashboard`/`set_dash_metadata` rebuilds dashboard slices from `positions`, so `{}` produces an empty slice list even when the source dashboard had charts. Fail fast on invalid/missing layout or build a positions map from source charts before calling the copy command. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP duplicate_dashboard can create empty dashboards unexpectedly.
- ⚠️ Source dashboards with bad layout copy without any charts.
- ⚠️ LLM workflows see misleading empty dashboard copies.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or identify a dashboard object with charts but invalid or missing layout JSON:
in `superset/models/dashboard.py:131-147` the `Dashboard` model has `position_json` and
`slices` fields; use a DB migration or shell to set `position_json` to `NULL` or a
non-JSON string for a dashboard that still has non-empty `slices`.
2. Call the MCP tool `duplicate_dashboard` defined in
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:131-144`, passing that
dashboard's ID/UUID/slug via `DuplicateDashboardRequest.dashboard_id`.
3. Inside `duplicate_dashboard`, `_build_copy_payload` is invoked at
`duplicate_dashboard.py:187-189`, which parses `source.position_json` at lines `67-72`;
because `position_json` is `NULL`/invalid, the `except` block and type check coerce
`positions` to `{}`, then `metadata["positions"] = positions` at line 74 ensures the
copied payload always contains an empty `positions` dict.
4. `CopyDashboardCommand.run()` in `superset/commands/dashboard/copy.py:11-14` calls
`DashboardDAO.copy_dashboard`, which in turn calls `DashboardDAO.set_dash_metadata(dash,
metadata, old_to_new_slice_ids)` at `superset/daos/dashboard.py:43-44`; since
`data.get("positions")` is `{}` (non-None), `set_dash_metadata` at `dashboard.py:22-36`
computes `slice_ids = []`, sets `dashboard.slices = current_slices` where `current_slices`
is `[]`, and writes the new dashboard with no charts even though the source had charts,
causing the duplicated dashboard to lose all charts.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cc3b61e9ddfd4a7eafef0b51bb09a01a&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=cc3b61e9ddfd4a7eafef0b51bb09a01a&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/duplicate_dashboard.py
**Line:** 67:74
**Comment:**
*Logic Error: Falling back to an empty `positions` object when `position_json` is missing/invalid will silently drop all charts in the copied dashboard. `DashboardDAO.copy_dashboard`/`set_dash_metadata` rebuilds dashboard slices from `positions`, so `{}` produces an empty slice list even when the source dashboard had charts. Fail fast on invalid/missing layout or build a positions map from source charts before calling the copy command.
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%2F40959&comment_hash=2ded76a9cd113cdf3e039b0e7096bc070770a62c5f0c5dead14985accb02430b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=2ded76a9cd113cdf3e039b0e7096bc070770a62c5f0c5dead14985accb02430b&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #40959:
URL: https://github.com/apache/superset/pull/40959#issuecomment-4835724992
These two guards (empty/stale layout) don't exist on the REST /copy/ endpoint, which would just produce the copy. Dashboards created via API or import can legitimately carry slices without position_json entries; for those this tool now hard-fails where "Save as" would succeed. I'm fine with the fail-fast, but please call out this intentional divergence in the docstring/PR description.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3482520981
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,376 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload and a flag indicating whether the layout maps any
+ chart, so the caller can refuse to produce a silently empty copy.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+ return payload, _positions_reference_charts(positions)
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
+ return _sanitize_dashboard_info_for_llm_context(info), dashboard_url
+
+
+def _safe_rollback(context_label: str) -> None:
+ """Roll back the current DB session, swallowing rollback failures.
+
+ A failed operation can leave the shared session in an invalid
+ transaction state; rolling back keeps later ORM use in the same request
+ lifecycle from inheriting the broken transaction.
+ """
+ from superset import db
+
+ try:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ except SQLAlchemyError:
+ logger.warning(
+ "Database rollback failed during %s error handling",
+ context_label,
+ exc_info=True,
+ )
+
+
+def _refetch_and_serialize(
+ new_dashboard: Any, dashboard_title: str
+) -> tuple[DashboardInfo, str]:
+ """Re-fetch the new dashboard with eager-loaded relationships.
+
+ The eager load avoids lazy-loading on a session the command's commit may
+ have invalidated. If the re-fetch fails, the failed transaction is rolled
+ back and a minimal response is returned instead.
+ """
+ from sqlalchemy.orm import subqueryload
+
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ try:
+ dashboard = (
+ DashboardDAO.find_by_id(
+ new_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or new_dashboard
+ )
+ return _serialize_new_dashboard(dashboard)
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ new_dashboard.id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard re-fetch")
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"
+ )
+ info = _sanitize_dashboard_info_for_llm_context(
+ DashboardInfo(
+ id=new_dashboard.id,
+ dashboard_title=dashboard_title,
+ url=dashboard_url,
+ )
+ )
+ return info, dashboard_url
+
+
+async def _resolve_source(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> tuple[Any, DuplicateDashboardResponse | None]:
+ """Resolve and authorize the source dashboard.
+
+ Returns ``(source, None)`` on success, or ``(None, error_response)`` when
+ the dashboard is missing or inaccessible.
+ """
+ from superset.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardNotFoundError,
+ )
+ from superset.daos.dashboard import DashboardDAO
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.lookup"):
+ try:
+ return DashboardDAO.get_by_id_or_slug(str(request.dashboard_id)), None
+ except DashboardNotFoundError:
+ await ctx.warning(
+ "Dashboard not found for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' not found. "
+ "Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+ except DashboardAccessDeniedError:
+ await ctx.warning(
+ "Dashboard access denied for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"You don't have access to dashboard "
+ f"'{request.dashboard_id}', so it cannot be duplicated."
+ ),
+ )
Review Comment:
Fixed in 594d264eca (rebased; was 56733fc9a3). `_resolve_source` now also catches `SQLAlchemyError` from `get_by_id_or_slug`, rolls back, logs the full error with a traceback, and returns a structured `DuplicateDashboardResponse` with a generic "database error" message — deliberately not echoing `str(exc)` since SQLAlchemy errors can carry table/column/constraint names. This matches the established pattern in `generate_dashboard` and `update_dashboard`. The copy path (DB errors there are wrapped as `DashboardCopyError` by the `@transaction` handler) and the re-fetch path were already structured, so lookup was the last unstructured DB path. Added regression test `test_lookup_db_error_returns_structured_error` (also asserts no DB internals leak into the response).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3493543096
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,414 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool, str | None]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload, a flag indicating whether the layout maps any
+ chart (so the caller can refuse to produce a silently empty copy),
+ and an optional warning string when ``json_metadata`` could not be
+ decoded. The caller surfaces the warning rather than hard-failing:
+ metadata loss degrades aesthetic settings (colors, native filters)
+ but does not corrupt the copy's chart content.
+ """
+ metadata_warning: str | None = None
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) could not "
+ "be decoded and were not copied. The duplicate uses default "
+ "settings for colors, native filters, and other metadata. "
+ "Open and re-save the source dashboard to repair its settings."
+ )
+ if not isinstance(metadata, dict):
+ metadata = {}
+ if metadata_warning is None:
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) were not "
+ "a valid JSON object and were not copied. The duplicate uses "
+ "default settings for colors, native filters, and other metadata."
+ )
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+ return payload, _positions_reference_charts(positions), metadata_warning
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
+ return _sanitize_dashboard_info_for_llm_context(info), dashboard_url
+
+
+def _safe_rollback(context_label: str) -> None:
+ """Roll back the current DB session, swallowing rollback failures.
+
+ A failed operation can leave the shared session in an invalid
+ transaction state; rolling back keeps later ORM use in the same request
+ lifecycle from inheriting the broken transaction.
+ """
+ from superset import db
+
+ try:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ except SQLAlchemyError:
+ logger.warning(
+ "Database rollback failed during %s error handling",
+ context_label,
+ exc_info=True,
+ )
+
+
+def _refetch_and_serialize(
+ new_dashboard: Any, dashboard_title: str
+) -> tuple[DashboardInfo, str]:
+ """Re-fetch the new dashboard with eager-loaded relationships.
+
+ The eager load avoids lazy-loading on a session the command's commit may
+ have invalidated. If the re-fetch fails, the failed transaction is rolled
+ back and a minimal response is returned instead.
+ """
+ from sqlalchemy.orm import subqueryload
+
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ try:
+ dashboard = (
+ DashboardDAO.find_by_id(
+ new_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or new_dashboard
+ )
+ return _serialize_new_dashboard(dashboard)
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ new_dashboard.id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard re-fetch")
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"
+ )
+ info = _sanitize_dashboard_info_for_llm_context(
+ DashboardInfo(
+ id=new_dashboard.id,
+ dashboard_title=dashboard_title,
+ url=dashboard_url,
+ )
+ )
+ return info, dashboard_url
+
+
+async def _resolve_source(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> tuple[Any, DuplicateDashboardResponse | None]:
+ """Resolve and authorize the source dashboard.
+
+ Returns ``(source, None)`` on success, or ``(None, error_response)`` when
+ the dashboard is missing or inaccessible.
+ """
+ from superset.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardNotFoundError,
+ )
+ from superset.daos.dashboard import DashboardDAO
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.lookup"):
+ try:
+ return DashboardDAO.get_by_id_or_slug(str(request.dashboard_id)), None
+ except DashboardNotFoundError:
+ await ctx.warning(
+ "Dashboard not found for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' not found. "
+ "Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+ except DashboardAccessDeniedError:
+ await ctx.warning(
+ "Dashboard access denied for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"You don't have access to dashboard "
+ f"'{request.dashboard_id}', so it cannot be duplicated."
+ ),
+ )
+ except SQLAlchemyError:
+ # Transient DB/session failures during lookup must surface as a
+ # structured response, not a hard tool failure. The raw error is
+ # logged with a traceback; the response stays generic because
+ # ``str(exc)`` can leak table/column/constraint names.
+ logger.error(
+ "Database error resolving dashboard %s for duplication",
+ request.dashboard_id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard lookup")
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' could not be "
+ "duplicated due to a database error. Please try again."
+ ),
+ )
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Duplicate dashboard",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def duplicate_dashboard(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> DuplicateDashboardResponse:
+ """
+ Duplicate an existing dashboard under a new title.
+
+ By default the copy references the same charts as the source.
+ Set duplicate_slices=true to also deep-copy every chart into new
+ chart objects owned by you, so edits to the copies never affect
+ the originals.
+
+ The source dashboard can be identified by numeric ID, UUID, or slug.
+ Returns the new dashboard's ID, title, and URL.
+ """
+ await ctx.info(
+ "Duplicating dashboard: dashboard_id=%s, duplicate_slices=%s"
+ % (request.dashboard_id, request.duplicate_slices)
+ )
+
+ from superset.commands.dashboard.copy import CopyDashboardCommand
+ from superset.commands.dashboard.exceptions import (
+ DashboardCopyError,
+ DashboardForbiddenError,
+ DashboardInvalidError,
+ )
+
+ try:
+ source, error_response = await _resolve_source(request, ctx)
+ if error_response is not None:
+ return error_response
+
+ data, layout_has_charts, metadata_warning = _build_copy_payload(
+ source, request.dashboard_title, request.duplicate_slices
+ )
+
+ if getattr(source, "slices", None) and not layout_has_charts:
+ await ctx.warning(
+ "Source layout maps no charts; refusing to duplicate to "
+ "avoid an empty copy: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' has charts but its "
+ "saved layout is missing or invalid, so duplicating it "
+ "would produce a dashboard with no charts. Open and "
+ "re-save the source dashboard to repair its layout, then "
+ "try again."
+ ),
+ )
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.copy"):
+ new_dashboard = CopyDashboardCommand(source, data).run()
+
+ info, dashboard_url = _refetch_and_serialize(
+ new_dashboard, request.dashboard_title
+ )
+
+ logger.info(
+ "Duplicated dashboard %s into dashboard %s (duplicate_slices=%s)",
+ request.dashboard_id,
+ new_dashboard.id,
+ request.duplicate_slices,
+ )
+
+ warnings = list(request.sanitization_warnings)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag missing local variable type annotations in function bodies; Superset does not follow this convention.
**Applied to:**
- `**/*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:
##########
@@ -0,0 +1,580 @@
+# 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.
+
+"""
+Unit tests for the duplicate_dashboard MCP tool.
+
+Follows the same pattern used in test_add_chart_to_existing_dashboard.py:
+- Tests run through the async MCP Client (not direct function calls)
+- Patches applied at source locations (superset.daos.dashboard.*,
+ superset.commands.dashboard.copy.*)
+- auth is mocked via the autouse mock_auth fixture
+
+Covers:
+- Duplicate referencing the same charts (duplicate_slices=False, default)
+- Duplicate with deep-copied charts (duplicate_slices=True)
+- Source dashboard not found
+- Source dashboard access denied / copy forbidden
+- Title sanitization (XSS stripped, XSS-only title rejected)
+"""
+
+import logging
+from collections.abc import Iterator
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.utils.sanitization import (
+ LLM_CONTEXT_CLOSE_DELIMITER,
+ LLM_CONTEXT_OPEN_DELIMITER,
+)
+from superset.utils import json
+
+
+def _wrapped(value: str) -> str:
+ """Return the LLM-context-wrapped form a sanitized field should have."""
+ return f"{LLM_CONTEXT_OPEN_DELIMITER}\n{value}\n{LLM_CONTEXT_CLOSE_DELIMITER}"
+
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag missing type annotations for module-level logger variables; Superset does not annotate loggers as a project-wide convention.
**Applied to:**
- `**/*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40959:
URL: https://github.com/apache/superset/pull/40959#issuecomment-4675897927
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40959?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `30.32787%` with `85 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.50%. Comparing base ([`f79a88c`](https://app.codecov.io/gh/apache/superset/commit/f79a88c6857a9a44faade421a17aff9938be4c68?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`5495f98`](https://app.codecov.io/gh/apache/superset/commit/5495f989f38430141863ce7f403d7c6327b4d15d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40959?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../mcp\_service/dashboard/tool/duplicate\_dashboard.py](https://app.codecov.io/gh/apache/superset/pull/40959?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fdashboard%2Ftool%2Fduplicate_dashboard.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvZGFzaGJvYXJkL3Rvb2wvZHVwbGljYXRlX2Rhc2hib2FyZC5weQ==) | 19.48% | [62 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40959?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/mcp\_service/dashboard/schemas.py](https://app.codecov.io/gh/apache/superset/pull/40959?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fdashboard%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvZGFzaGJvYXJkL3NjaGVtYXMucHk=) | 47.72% | [23 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40959?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (f79a88c) and HEAD (5495f98). Click for more details.
>
> <details><summary>HEAD has 8 uploads less than BASE</summary>
>
>| Flag | BASE (f79a88c) | HEAD (5495f98) |
>|------|------|------|
>|python|10|2|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40959 +/- ##
==========================================
- Coverage 64.19% 55.50% -8.69%
==========================================
Files 2655 2656 +1
Lines 143925 144047 +122
Branches 33181 33191 +10
==========================================
- Hits 92387 79958 -12429
- Misses 49918 63372 +13454
+ Partials 1620 717 -903
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40959/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40959/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.44% <30.32%> (-0.02%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40959/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40959/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40959/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.02% <30.32%> (-0.02%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/40959/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.09% <30.32%> (-18.64%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40959/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40959/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/40959?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3410560569
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestDuplicateDashboardRequestTitleSanitization:
+ """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title."""
+
+ def test_plain_title_passes_without_warning(self) -> None:
Review Comment:
**Suggestion:** Add a short docstring to this newly added test function to comply with the rule requiring docstrings on new functions. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added test method in the PR and it has no docstring. The rule requires new Python functions and classes to include docstrings, so the suggestion correctly identifies a real violation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d37f642b69364aa2a9a0ac09e44a0dad&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=d37f642b69364aa2a9a0ac09e44a0dad&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:** tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
**Line:** 635:635
**Comment:**
*Custom Rule: Add a short docstring to this newly added test function to comply with the rule requiring docstrings on new functions.
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%2F40959&comment_hash=db93e779950d776df2e9ed4d90e2bd63aac6f67a719a28147287404c64e8abef&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=db93e779950d776df2e9ed4d90e2bd63aac6f67a719a28147287404c64e8abef&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestDuplicateDashboardRequestTitleSanitization:
+ """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title."""
+
+ def test_plain_title_passes_without_warning(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy")
+ assert req.dashboard_title == "Regional Copy"
+ assert req.sanitization_warnings == []
+
+ def test_title_accepts_aliases(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id="my-slug", name="From Name")
+ assert req.dashboard_title == "From Name"
+
+ def test_script_only_title_is_rejected(self) -> None:
+ with pytest.raises(ValidationError, match="removed entirely by sanitization"):
+ DuplicateDashboardRequest(
+ dashboard_id=1, dashboard_title="<script>alert(1)</script>"
+ )
+
+ def test_empty_title_is_rejected(self) -> None:
+ with pytest.raises(ValidationError):
+ DuplicateDashboardRequest(dashboard_id=1, dashboard_title="")
+
+ def test_partial_strip_emits_warning(self) -> None:
+ req = DuplicateDashboardRequest(
+ dashboard_id=1, dashboard_title="Q1 <b>Review</b>"
+ )
+ assert req.dashboard_title == "Q1 Review"
+ assert len(req.sanitization_warnings) == 1
+ assert "dashboard_title" in req.sanitization_warnings[0]
+
+ def test_client_supplied_warnings_are_discarded(self) -> None:
+ """``sanitization_warnings`` is server-only; client input is dropped."""
+ req = DuplicateDashboardRequest(
+ dashboard_id=1,
+ dashboard_title="Plain Title",
+ sanitization_warnings=["<script>fake notice</script>"],
+ )
+ assert req.sanitization_warnings == []
+
+
+class TestDuplicateDashboardResponse:
+ """Serialization and error sanitization for DuplicateDashboardResponse."""
+
+ def test_defaults(self) -> None:
Review Comment:
**Suggestion:** Add a brief docstring for this new test method so the function is documented inline. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new test method lacks a docstring. The custom rule explicitly requires docstrings on newly added Python functions, so the suggestion is verified.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f7fb4108d1ab45b7a38f895f6527d74c&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=f7fb4108d1ab45b7a38f895f6527d74c&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:** tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
**Line:** 675:675
**Comment:**
*Custom Rule: Add a brief docstring for this new test method so the function is documented inline.
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%2F40959&comment_hash=89bf0f5a795d7e92cfb4c38615c53912a82d454a76e81df8d63f7a94e0ff8106&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=89bf0f5a795d7e92cfb4c38615c53912a82d454a76e81df8d63f7a94e0ff8106&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestDuplicateDashboardRequestTitleSanitization:
+ """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title."""
+
+ def test_plain_title_passes_without_warning(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy")
+ assert req.dashboard_title == "Regional Copy"
+ assert req.sanitization_warnings == []
+
+ def test_title_accepts_aliases(self) -> None:
Review Comment:
**Suggestion:** Add an inline docstring describing the alias behavior being validated so the new function is documented. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added test method does not include a docstring. That matches the stated rule requiring docstrings for new functions, so this is a verified violation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f20a4a92594f4e98a71caffd7a708cc8&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=f20a4a92594f4e98a71caffd7a708cc8&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:** tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
**Line:** 640:640
**Comment:**
*Custom Rule: Add an inline docstring describing the alias behavior being validated so the new function is documented.
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%2F40959&comment_hash=e07d577628a2ddd30cd2bb56c9707988fd0f4ba2a3ebd72302e6fa9e9249c000&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=e07d577628a2ddd30cd2bb56c9707988fd0f4ba2a3ebd72302e6fa9e9249c000&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestDuplicateDashboardRequestTitleSanitization:
+ """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title."""
+
+ def test_plain_title_passes_without_warning(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy")
+ assert req.dashboard_title == "Regional Copy"
+ assert req.sanitization_warnings == []
+
+ def test_title_accepts_aliases(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id="my-slug", name="From Name")
+ assert req.dashboard_title == "From Name"
+
+ def test_script_only_title_is_rejected(self) -> None:
Review Comment:
**Suggestion:** Add a concise docstring explaining the rejection case being tested to satisfy the new-function docstring requirement. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The method is newly added and has no docstring. Since the rule says new Python functions should be documented inline, this is a real violation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9a605d3c226e4442972166aa84c7d794&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=9a605d3c226e4442972166aa84c7d794&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:** tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
**Line:** 644:644
**Comment:**
*Custom Rule: Add a concise docstring explaining the rejection case being tested to satisfy the new-function docstring requirement.
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%2F40959&comment_hash=fd2307080ad987e0ee28619554437bdcb816d5ec7d81923a0ad4abfa6d5dcf47&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=fd2307080ad987e0ee28619554437bdcb816d5ec7d81923a0ad4abfa6d5dcf47&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:
##########
@@ -0,0 +1,361 @@
+# 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.
+
+"""
+Unit tests for the duplicate_dashboard MCP tool.
+
+Follows the same pattern used in test_add_chart_to_existing_dashboard.py:
+- Tests run through the async MCP Client (not direct function calls)
+- Patches applied at source locations (superset.daos.dashboard.*,
+ superset.commands.dashboard.copy.*)
+- auth is mocked via the autouse mock_auth fixture
+
+Covers:
+- Duplicate referencing the same charts (duplicate_slices=False, default)
+- Duplicate with deep-copied charts (duplicate_slices=True)
+- Source dashboard not found
+- Source dashboard access denied / copy forbidden
+- Title sanitization (XSS stripped, XSS-only title rejected)
+"""
+
+import logging
+from unittest.mock import Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.utils import json
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
+
+
+# ---------------------------------------------------------------------------
+# Fixtures
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+def mcp_server() -> object:
+ """Return the FastMCP app instance for use in MCP client tests."""
+ return mcp
+
+
+@pytest.fixture(autouse=True)
+def mock_auth():
Review Comment:
**Suggestion:** Add an explicit return type annotation to `mock_auth` so the new fixture function is fully typed according to the rule for new Python code. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python function in a new test file, and it lacks a return type annotation. The custom rule requires new Python code to be fully typed, so the suggestion correctly identifies a real violation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=af302f9f4f994da0a1bf6f5aff0e693a&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=af302f9f4f994da0a1bf6f5aff0e693a&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:** tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py
**Line:** 60:60
**Comment:**
*Custom Rule: Add an explicit return type annotation to `mock_auth` so the new fixture function is fully typed according to the rule for new Python code.
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%2F40959&comment_hash=f95257b63b03277396b3d8e24eee395a221ad290a2bc745addd260c0d1e2b338&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=f95257b63b03277396b3d8e24eee395a221ad290a2bc745addd260c0d1e2b338&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestDuplicateDashboardRequestTitleSanitization:
+ """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title."""
+
+ def test_plain_title_passes_without_warning(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy")
+ assert req.dashboard_title == "Regional Copy"
+ assert req.sanitization_warnings == []
+
+ def test_title_accepts_aliases(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id="my-slug", name="From Name")
+ assert req.dashboard_title == "From Name"
+
+ def test_script_only_title_is_rejected(self) -> None:
+ with pytest.raises(ValidationError, match="removed entirely by sanitization"):
+ DuplicateDashboardRequest(
+ dashboard_id=1, dashboard_title="<script>alert(1)</script>"
+ )
+
+ def test_empty_title_is_rejected(self) -> None:
+ with pytest.raises(ValidationError):
+ DuplicateDashboardRequest(dashboard_id=1, dashboard_title="")
+
+ def test_partial_strip_emits_warning(self) -> None:
+ req = DuplicateDashboardRequest(
+ dashboard_id=1, dashboard_title="Q1 <b>Review</b>"
+ )
+ assert req.dashboard_title == "Q1 Review"
+ assert len(req.sanitization_warnings) == 1
+ assert "dashboard_title" in req.sanitization_warnings[0]
+
+ def test_client_supplied_warnings_are_discarded(self) -> None:
+ """``sanitization_warnings`` is server-only; client input is dropped."""
+ req = DuplicateDashboardRequest(
+ dashboard_id=1,
+ dashboard_title="Plain Title",
+ sanitization_warnings=["<script>fake notice</script>"],
+ )
+ assert req.sanitization_warnings == []
+
+
+class TestDuplicateDashboardResponse:
+ """Serialization and error sanitization for DuplicateDashboardResponse."""
+
+ def test_defaults(self) -> None:
+ resp = DuplicateDashboardResponse()
+ assert resp.dashboard is None
+ assert resp.dashboard_url is None
+ assert resp.duplicated_slices is False
+ assert resp.error is None
+ assert resp.warnings == []
+
+ def test_error_is_wrapped_for_llm_context(self) -> None:
Review Comment:
**Suggestion:** Add a docstring describing the error-wrapping assertion to document this newly added function. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This added test method has no docstring in the final file state. That directly violates the rule for newly added Python functions/classes to be documented inline.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ae21d9a124e34c1e8741d12e544559a9&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=ae21d9a124e34c1e8741d12e544559a9&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:** tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
**Line:** 683:683
**Comment:**
*Custom Rule: Add a docstring describing the error-wrapping assertion to document this newly added function.
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%2F40959&comment_hash=5d2780a77e3b8271fde52a96868da2f145e415ea4721fdf4be914660127a0a4a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=5d2780a77e3b8271fde52a96868da2f145e415ea4721fdf4be914660127a0a4a&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3413991602
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,277 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> dict[str, Any]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
Review Comment:
Good catch — fixed in 5e9923e7b3. The tool now detects when the source has charts but its layout maps none (missing/invalid `position_json`) and fails fast with a clear error, instead of letting `set_dash_metadata` rebuild the copy with zero slices and silently produce an empty dashboard. Added a regression test.
##########
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:
##########
@@ -0,0 +1,361 @@
+# 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.
+
+"""
+Unit tests for the duplicate_dashboard MCP tool.
+
+Follows the same pattern used in test_add_chart_to_existing_dashboard.py:
+- Tests run through the async MCP Client (not direct function calls)
+- Patches applied at source locations (superset.daos.dashboard.*,
+ superset.commands.dashboard.copy.*)
+- auth is mocked via the autouse mock_auth fixture
+
+Covers:
+- Duplicate referencing the same charts (duplicate_slices=False, default)
+- Duplicate with deep-copied charts (duplicate_slices=True)
+- Source dashboard not found
+- Source dashboard access denied / copy forbidden
+- Title sanitization (XSS stripped, XSS-only title rejected)
+"""
+
+import logging
+from unittest.mock import Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.utils import json
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
+
+
+# ---------------------------------------------------------------------------
+# Fixtures
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+def mcp_server() -> object:
+ """Return the FastMCP app instance for use in MCP client tests."""
+ return mcp
+
+
+@pytest.fixture(autouse=True)
+def mock_auth():
Review Comment:
Addressed in 5e9923e7b3 — added `-> Iterator[MagicMock]` return type to the fixture.
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestDuplicateDashboardRequestTitleSanitization:
+ """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title."""
+
+ def test_plain_title_passes_without_warning(self) -> None:
Review Comment:
Addressed in 5e9923e7b3 — added a docstring to this test method.
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestDuplicateDashboardRequestTitleSanitization:
+ """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title."""
+
+ def test_plain_title_passes_without_warning(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy")
+ assert req.dashboard_title == "Regional Copy"
+ assert req.sanitization_warnings == []
+
+ def test_title_accepts_aliases(self) -> None:
Review Comment:
Addressed in 5e9923e7b3 — added a docstring to this test method.
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestDuplicateDashboardRequestTitleSanitization:
+ """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title."""
+
+ def test_plain_title_passes_without_warning(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy")
+ assert req.dashboard_title == "Regional Copy"
+ assert req.sanitization_warnings == []
+
+ def test_title_accepts_aliases(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id="my-slug", name="From Name")
+ assert req.dashboard_title == "From Name"
+
+ def test_script_only_title_is_rejected(self) -> None:
Review Comment:
Addressed in 5e9923e7b3 — added a docstring to this test method.
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestDuplicateDashboardRequestTitleSanitization:
+ """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title."""
+
+ def test_plain_title_passes_without_warning(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy")
+ assert req.dashboard_title == "Regional Copy"
+ assert req.sanitization_warnings == []
+
+ def test_title_accepts_aliases(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id="my-slug", name="From Name")
+ assert req.dashboard_title == "From Name"
+
+ def test_script_only_title_is_rejected(self) -> None:
+ with pytest.raises(ValidationError, match="removed entirely by sanitization"):
+ DuplicateDashboardRequest(
+ dashboard_id=1, dashboard_title="<script>alert(1)</script>"
+ )
+
+ def test_empty_title_is_rejected(self) -> None:
+ with pytest.raises(ValidationError):
+ DuplicateDashboardRequest(dashboard_id=1, dashboard_title="")
+
+ def test_partial_strip_emits_warning(self) -> None:
+ req = DuplicateDashboardRequest(
+ dashboard_id=1, dashboard_title="Q1 <b>Review</b>"
+ )
+ assert req.dashboard_title == "Q1 Review"
+ assert len(req.sanitization_warnings) == 1
+ assert "dashboard_title" in req.sanitization_warnings[0]
+
+ def test_client_supplied_warnings_are_discarded(self) -> None:
+ """``sanitization_warnings`` is server-only; client input is dropped."""
+ req = DuplicateDashboardRequest(
+ dashboard_id=1,
+ dashboard_title="Plain Title",
+ sanitization_warnings=["<script>fake notice</script>"],
+ )
+ assert req.sanitization_warnings == []
+
+
+class TestDuplicateDashboardResponse:
+ """Serialization and error sanitization for DuplicateDashboardResponse."""
+
+ def test_defaults(self) -> None:
Review Comment:
Addressed in 5e9923e7b3 — added a docstring to this test method.
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestDuplicateDashboardRequestTitleSanitization:
+ """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title."""
+
+ def test_plain_title_passes_without_warning(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy")
+ assert req.dashboard_title == "Regional Copy"
+ assert req.sanitization_warnings == []
+
+ def test_title_accepts_aliases(self) -> None:
+ req = DuplicateDashboardRequest(dashboard_id="my-slug", name="From Name")
+ assert req.dashboard_title == "From Name"
+
+ def test_script_only_title_is_rejected(self) -> None:
+ with pytest.raises(ValidationError, match="removed entirely by sanitization"):
+ DuplicateDashboardRequest(
+ dashboard_id=1, dashboard_title="<script>alert(1)</script>"
+ )
+
+ def test_empty_title_is_rejected(self) -> None:
+ with pytest.raises(ValidationError):
+ DuplicateDashboardRequest(dashboard_id=1, dashboard_title="")
+
+ def test_partial_strip_emits_warning(self) -> None:
+ req = DuplicateDashboardRequest(
+ dashboard_id=1, dashboard_title="Q1 <b>Review</b>"
+ )
+ assert req.dashboard_title == "Q1 Review"
+ assert len(req.sanitization_warnings) == 1
+ assert "dashboard_title" in req.sanitization_warnings[0]
+
+ def test_client_supplied_warnings_are_discarded(self) -> None:
+ """``sanitization_warnings`` is server-only; client input is dropped."""
+ req = DuplicateDashboardRequest(
+ dashboard_id=1,
+ dashboard_title="Plain Title",
+ sanitization_warnings=["<script>fake notice</script>"],
+ )
+ assert req.sanitization_warnings == []
+
+
+class TestDuplicateDashboardResponse:
+ """Serialization and error sanitization for DuplicateDashboardResponse."""
+
+ def test_defaults(self) -> None:
+ resp = DuplicateDashboardResponse()
+ assert resp.dashboard is None
+ assert resp.dashboard_url is None
+ assert resp.duplicated_slices is False
+ assert resp.error is None
+ assert resp.warnings == []
+
+ def test_error_is_wrapped_for_llm_context(self) -> None:
Review Comment:
Addressed in 5e9923e7b3 — added a docstring to this test method.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3493542140
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,414 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag module-level Python logger declarations like `logger = logging.getLogger(__name__)` for missing type annotations; Superset intentionally leaves these unannotated and relies on mypy's inference.
**Applied to:**
- `**/*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -914,6 +914,138 @@ class GenerateDashboardResponse(BaseModel):
)
+class DuplicateDashboardRequest(BaseModel):
+ """Request schema for duplicating an existing dashboard."""
+
+ model_config = ConfigDict(populate_by_name=True)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag unannotated `model_config = ConfigDict(...)` class-level configuration in Pydantic v2 models; it is a recognized sentinel and should remain unannotated unless there is a real type-safety issue.
**Applied to:**
- `**/*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:
##########
@@ -0,0 +1,580 @@
+# 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.
+
+"""
+Unit tests for the duplicate_dashboard MCP tool.
+
+Follows the same pattern used in test_add_chart_to_existing_dashboard.py:
+- Tests run through the async MCP Client (not direct function calls)
+- Patches applied at source locations (superset.daos.dashboard.*,
+ superset.commands.dashboard.copy.*)
+- auth is mocked via the autouse mock_auth fixture
+
+Covers:
+- Duplicate referencing the same charts (duplicate_slices=False, default)
+- Duplicate with deep-copied charts (duplicate_slices=True)
+- Source dashboard not found
+- Source dashboard access denied / copy forbidden
+- Title sanitization (XSS stripped, XSS-only title rejected)
+"""
+
+import logging
+from collections.abc import Iterator
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.utils.sanitization import (
+ LLM_CONTEXT_CLOSE_DELIMITER,
+ LLM_CONTEXT_OPEN_DELIMITER,
+)
+from superset.utils import json
+
+
+def _wrapped(value: str) -> str:
+ """Return the LLM-context-wrapped form a sanitized field should have."""
+ return f"{LLM_CONTEXT_OPEN_DELIMITER}\n{value}\n{LLM_CONTEXT_CLOSE_DELIMITER}"
+
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
+
+
+# ---------------------------------------------------------------------------
+# Fixtures
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+def mcp_server() -> object:
+ """Return the FastMCP app instance for use in MCP client tests."""
+ return mcp
+
+
+@pytest.fixture(autouse=True)
+def mock_auth() -> Iterator[MagicMock]:
+ """Mock authentication for all tests."""
+ with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user:
+ mock_user = Mock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_get_user.return_value = mock_user
+ yield mock_get_user
+
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+SOURCE_POSITIONS = {
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require explicit type annotations for module-level constants in test files; clear literals in tests are fine without annotations.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,414 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool, str | None]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload, a flag indicating whether the layout maps any
+ chart (so the caller can refuse to produce a silently empty copy),
+ and an optional warning string when ``json_metadata`` could not be
+ decoded. The caller surfaces the warning rather than hard-failing:
+ metadata loss degrades aesthetic settings (colors, native filters)
+ but does not corrupt the copy's chart content.
+ """
+ metadata_warning: str | None = None
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) could not "
+ "be decoded and were not copied. The duplicate uses default "
+ "settings for colors, native filters, and other metadata. "
+ "Open and re-save the source dashboard to repair its settings."
+ )
+ if not isinstance(metadata, dict):
+ metadata = {}
+ if metadata_warning is None:
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) were not "
+ "a valid JSON object and were not copied. The duplicate uses "
+ "default settings for colors, native filters, and other metadata."
+ )
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag missing type annotations for short-lived local variables inside Python function bodies when the type is already obvious from context; reserve these comments for ambiguous cases or public interfaces.
**Applied to:**
- `**/*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40959:
URL: https://github.com/apache/superset/pull/40959#issuecomment-4836348189
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3493291299
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -914,6 +914,138 @@ class GenerateDashboardResponse(BaseModel):
)
+class DuplicateDashboardRequest(BaseModel):
+ """Request schema for duplicating an existing dashboard."""
+
+ model_config = ConfigDict(populate_by_name=True)
Review Comment:
**Suggestion:** Add an explicit type annotation for this class-level configuration variable to satisfy the type-hint requirement for relevant annotatable variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new `DuplicateDashboardRequest` class defines `model_config` without a type annotation even though it is a class-level variable that can be annotated. This matches the Python type-hint rule for relevant annotatable variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1f5da1bb979d4b2f8b3b540f23963591&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=1f5da1bb979d4b2f8b3b540f23963591&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:** 920:920
**Comment:**
*Custom Rule: Add an explicit type annotation for this class-level configuration variable to satisfy the type-hint requirement for relevant annotatable variables.
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%2F40959&comment_hash=3c8f21bfb259e8eb1fb4197571658d22596402b5e58ce7221c3b82a6f88e5f2c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=3c8f21bfb259e8eb1fb4197571658d22596402b5e58ce7221c3b82a6f88e5f2c&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,414 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
Review Comment:
**Suggestion:** Add an explicit type annotation for this module-level logger variable. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The rule requires type hints on relevant variables that can be annotated. This module-level logger is unannotated even though it can be typed explicitly, so the suggestion identifies a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6ca3f5edbb084d2c9444d383e84880b3&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=6ca3f5edbb084d2c9444d383e84880b3&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/duplicate_dashboard.py
**Line:** 45:45
**Comment:**
*Custom Rule: Add an explicit type annotation for this module-level logger variable.
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%2F40959&comment_hash=c5760cbaa134b4841fa5242e65d94931eb913ee86ad80d555990fb788f5916e4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=c5760cbaa134b4841fa5242e65d94931eb913ee86ad80d555990fb788f5916e4&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,414 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool, str | None]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload, a flag indicating whether the layout maps any
+ chart (so the caller can refuse to produce a silently empty copy),
+ and an optional warning string when ``json_metadata`` could not be
+ decoded. The caller surfaces the warning rather than hard-failing:
+ metadata loss degrades aesthetic settings (colors, native filters)
+ but does not corrupt the copy's chart content.
+ """
+ metadata_warning: str | None = None
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) could not "
+ "be decoded and were not copied. The duplicate uses default "
+ "settings for colors, native filters, and other metadata. "
+ "Open and re-save the source dashboard to repair its settings."
+ )
+ if not isinstance(metadata, dict):
+ metadata = {}
+ if metadata_warning is None:
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) were not "
+ "a valid JSON object and were not copied. The duplicate uses "
+ "default settings for colors, native filters, and other metadata."
+ )
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
Review Comment:
**Suggestion:** Annotate this local variable with its dictionary type to satisfy variable type-hint requirements. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This local dictionary is introduced without a type annotation and is clearly annotatable as a dict[str, Any]. That matches the custom rule about omitting type hints on relevant variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7e90084deab7449f935d2120e3b04086&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=7e90084deab7449f935d2120e3b04086&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/duplicate_dashboard.py
**Line:** 113:118
**Comment:**
*Custom Rule: Annotate this local variable with its dictionary type to satisfy variable type-hint requirements.
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%2F40959&comment_hash=965bf7693e471d8ede18e33d1987b31592ef1c1777a9a89be1b7eeff11e4186f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=965bf7693e471d8ede18e33d1987b31592ef1c1777a9a89be1b7eeff11e4186f&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:
##########
@@ -0,0 +1,580 @@
+# 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.
+
+"""
+Unit tests for the duplicate_dashboard MCP tool.
+
+Follows the same pattern used in test_add_chart_to_existing_dashboard.py:
+- Tests run through the async MCP Client (not direct function calls)
+- Patches applied at source locations (superset.daos.dashboard.*,
+ superset.commands.dashboard.copy.*)
+- auth is mocked via the autouse mock_auth fixture
+
+Covers:
+- Duplicate referencing the same charts (duplicate_slices=False, default)
+- Duplicate with deep-copied charts (duplicate_slices=True)
+- Source dashboard not found
+- Source dashboard access denied / copy forbidden
+- Title sanitization (XSS stripped, XSS-only title rejected)
+"""
+
+import logging
+from collections.abc import Iterator
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.utils.sanitization import (
+ LLM_CONTEXT_CLOSE_DELIMITER,
+ LLM_CONTEXT_OPEN_DELIMITER,
+)
+from superset.utils import json
+
+
+def _wrapped(value: str) -> str:
+ """Return the LLM-context-wrapped form a sanitized field should have."""
+ return f"{LLM_CONTEXT_OPEN_DELIMITER}\n{value}\n{LLM_CONTEXT_CLOSE_DELIMITER}"
+
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
Review Comment:
**Suggestion:** Add a type annotation for this module-level logger variable to satisfy the type-hint requirement for relevant variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The logger is a module-level variable that can be annotated with a concrete type such as `logging.Logger`, but it currently has no type hint. This is a real omission under the type-hint rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fe64de1a41cd4f23906c97869bc3c22d&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=fe64de1a41cd4f23906c97869bc3c22d&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:** tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py
**Line:** 56:56
**Comment:**
*Custom Rule: Add a type annotation for this module-level logger variable to satisfy the type-hint requirement for relevant variables.
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%2F40959&comment_hash=30d2115c24b8f66c1ef4cc740a14c779b32d57e15fa569195347b874e053f7af&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=30d2115c24b8f66c1ef4cc740a14c779b32d57e15fa569195347b874e053f7af&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,414 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool, str | None]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload, a flag indicating whether the layout maps any
+ chart (so the caller can refuse to produce a silently empty copy),
+ and an optional warning string when ``json_metadata`` could not be
+ decoded. The caller surfaces the warning rather than hard-failing:
+ metadata loss degrades aesthetic settings (colors, native filters)
+ but does not corrupt the copy's chart content.
+ """
+ metadata_warning: str | None = None
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) could not "
+ "be decoded and were not copied. The duplicate uses default "
+ "settings for colors, native filters, and other metadata. "
+ "Open and re-save the source dashboard to repair its settings."
+ )
+ if not isinstance(metadata, dict):
+ metadata = {}
+ if metadata_warning is None:
+ metadata_warning = (
+ "Source dashboard's stored settings (json_metadata) were not "
+ "a valid JSON object and were not copied. The duplicate uses "
+ "default settings for colors, native filters, and other metadata."
+ )
+
+ try:
+ positions = json.loads(source.position_json or "{}")
+ except (json.JSONDecodeError, TypeError):
+ positions = {}
+ if not isinstance(positions, dict):
+ positions = {}
+
+ metadata["positions"] = positions
+
+ payload = {
+ "dashboard_title": dashboard_title,
+ "css": source.css,
+ "duplicate_slices": duplicate_slices,
+ "json_metadata": json.dumps(metadata),
+ }
+ return payload, _positions_reference_charts(positions), metadata_warning
+
+
+def _serialize_new_dashboard(dashboard: Any) -> tuple[DashboardInfo, str]:
+ """Build the response ``DashboardInfo`` and URL for the new dashboard."""
+ from superset.mcp_service.dashboard.schemas import serialize_tag_object
+
+ dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
+ include_data_model_metadata = user_can_view_data_model_metadata()
+ info = DashboardInfo(
+ id=dashboard.id,
+ dashboard_title=dashboard.dashboard_title,
+ slug=dashboard.slug,
+ description=dashboard.description,
+ published=dashboard.published,
+ created_on=dashboard.created_on,
+ changed_on=dashboard.changed_on,
+ uuid=str(dashboard.uuid) if dashboard.uuid else None,
+ url=dashboard_url,
+ chart_count=len(dashboard.slices),
+ tags=[
+ obj
+ for tag in getattr(dashboard, "tags", [])
+ if (obj := serialize_tag_object(tag)) is not None
+ ],
+ charts=[
+ obj
+ for chart in getattr(dashboard, "slices", [])
+ if (
+ obj := serialize_chart_summary(
+ chart,
+ include_data_model_metadata=include_data_model_metadata,
+ )
+ )
+ is not None
+ ],
+ )
+ return _sanitize_dashboard_info_for_llm_context(info), dashboard_url
+
+
+def _safe_rollback(context_label: str) -> None:
+ """Roll back the current DB session, swallowing rollback failures.
+
+ A failed operation can leave the shared session in an invalid
+ transaction state; rolling back keeps later ORM use in the same request
+ lifecycle from inheriting the broken transaction.
+ """
+ from superset import db
+
+ try:
+ db.session.rollback() # pylint: disable=consider-using-transaction
+ except SQLAlchemyError:
+ logger.warning(
+ "Database rollback failed during %s error handling",
+ context_label,
+ exc_info=True,
+ )
+
+
+def _refetch_and_serialize(
+ new_dashboard: Any, dashboard_title: str
+) -> tuple[DashboardInfo, str]:
+ """Re-fetch the new dashboard with eager-loaded relationships.
+
+ The eager load avoids lazy-loading on a session the command's commit may
+ have invalidated. If the re-fetch fails, the failed transaction is rolled
+ back and a minimal response is returned instead.
+ """
+ from sqlalchemy.orm import subqueryload
+
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ try:
+ dashboard = (
+ DashboardDAO.find_by_id(
+ new_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or new_dashboard
+ )
+ return _serialize_new_dashboard(dashboard)
+ except SQLAlchemyError:
+ logger.warning(
+ "Re-fetch of dashboard %s failed; returning minimal response",
+ new_dashboard.id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard re-fetch")
+ dashboard_url = (
+ f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"
+ )
+ info = _sanitize_dashboard_info_for_llm_context(
+ DashboardInfo(
+ id=new_dashboard.id,
+ dashboard_title=dashboard_title,
+ url=dashboard_url,
+ )
+ )
+ return info, dashboard_url
+
+
+async def _resolve_source(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> tuple[Any, DuplicateDashboardResponse | None]:
+ """Resolve and authorize the source dashboard.
+
+ Returns ``(source, None)`` on success, or ``(None, error_response)`` when
+ the dashboard is missing or inaccessible.
+ """
+ from superset.commands.dashboard.exceptions import (
+ DashboardAccessDeniedError,
+ DashboardNotFoundError,
+ )
+ from superset.daos.dashboard import DashboardDAO
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.lookup"):
+ try:
+ return DashboardDAO.get_by_id_or_slug(str(request.dashboard_id)), None
+ except DashboardNotFoundError:
+ await ctx.warning(
+ "Dashboard not found for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' not found. "
+ "Use list_dashboards to get valid dashboard IDs."
+ ),
+ )
+ except DashboardAccessDeniedError:
+ await ctx.warning(
+ "Dashboard access denied for duplication: dashboard_id=%s"
+ % (request.dashboard_id,)
+ )
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"You don't have access to dashboard "
+ f"'{request.dashboard_id}', so it cannot be duplicated."
+ ),
+ )
+ except SQLAlchemyError:
+ # Transient DB/session failures during lookup must surface as a
+ # structured response, not a hard tool failure. The raw error is
+ # logged with a traceback; the response stays generic because
+ # ``str(exc)`` can leak table/column/constraint names.
+ logger.error(
+ "Database error resolving dashboard %s for duplication",
+ request.dashboard_id,
+ exc_info=True,
+ )
+ _safe_rollback("dashboard lookup")
+ return None, DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' could not be "
+ "duplicated due to a database error. Please try again."
+ ),
+ )
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Duplicate dashboard",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def duplicate_dashboard(
+ request: DuplicateDashboardRequest, ctx: Context
+) -> DuplicateDashboardResponse:
+ """
+ Duplicate an existing dashboard under a new title.
+
+ By default the copy references the same charts as the source.
+ Set duplicate_slices=true to also deep-copy every chart into new
+ chart objects owned by you, so edits to the copies never affect
+ the originals.
+
+ The source dashboard can be identified by numeric ID, UUID, or slug.
+ Returns the new dashboard's ID, title, and URL.
+ """
+ await ctx.info(
+ "Duplicating dashboard: dashboard_id=%s, duplicate_slices=%s"
+ % (request.dashboard_id, request.duplicate_slices)
+ )
+
+ from superset.commands.dashboard.copy import CopyDashboardCommand
+ from superset.commands.dashboard.exceptions import (
+ DashboardCopyError,
+ DashboardForbiddenError,
+ DashboardInvalidError,
+ )
+
+ try:
+ source, error_response = await _resolve_source(request, ctx)
+ if error_response is not None:
+ return error_response
+
+ data, layout_has_charts, metadata_warning = _build_copy_payload(
+ source, request.dashboard_title, request.duplicate_slices
+ )
+
+ if getattr(source, "slices", None) and not layout_has_charts:
+ await ctx.warning(
+ "Source layout maps no charts; refusing to duplicate to "
+ "avoid an empty copy: dashboard_id=%s" % (request.dashboard_id,)
+ )
+ return DuplicateDashboardResponse(
+ error=(
+ f"Dashboard '{request.dashboard_id}' has charts but its "
+ "saved layout is missing or invalid, so duplicating it "
+ "would produce a dashboard with no charts. Open and "
+ "re-save the source dashboard to repair its layout, then "
+ "try again."
+ ),
+ )
+
+ with event_logger.log_context(action="mcp.duplicate_dashboard.copy"):
+ new_dashboard = CopyDashboardCommand(source, data).run()
+
+ info, dashboard_url = _refetch_and_serialize(
+ new_dashboard, request.dashboard_title
+ )
+
+ logger.info(
+ "Duplicated dashboard %s into dashboard %s (duplicate_slices=%s)",
+ request.dashboard_id,
+ new_dashboard.id,
+ request.duplicate_slices,
+ )
+
+ warnings = list(request.sanitization_warnings)
Review Comment:
**Suggestion:** Add a concrete list type annotation for this local variable when it is initialized. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This local list variable is not annotated even though it can be typed explicitly, so it falls under the rule requiring type hints on relevant variables.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=91798785cf5d4bbcb0b805adda3b8e93&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=91798785cf5d4bbcb0b805adda3b8e93&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/duplicate_dashboard.py
**Line:** 357:357
**Comment:**
*Custom Rule: Add a concrete list type annotation for this local variable when it is initialized.
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%2F40959&comment_hash=a7c8b0df692997037b7a27187fa289fc6185ece9cc381d31ab9b841bbb3cf81c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=a7c8b0df692997037b7a27187fa289fc6185ece9cc381d31ab9b841bbb3cf81c&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:
##########
@@ -0,0 +1,580 @@
+# 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.
+
+"""
+Unit tests for the duplicate_dashboard MCP tool.
+
+Follows the same pattern used in test_add_chart_to_existing_dashboard.py:
+- Tests run through the async MCP Client (not direct function calls)
+- Patches applied at source locations (superset.daos.dashboard.*,
+ superset.commands.dashboard.copy.*)
+- auth is mocked via the autouse mock_auth fixture
+
+Covers:
+- Duplicate referencing the same charts (duplicate_slices=False, default)
+- Duplicate with deep-copied charts (duplicate_slices=True)
+- Source dashboard not found
+- Source dashboard access denied / copy forbidden
+- Title sanitization (XSS stripped, XSS-only title rejected)
+"""
+
+import logging
+from collections.abc import Iterator
+from unittest.mock import MagicMock, Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.utils.sanitization import (
+ LLM_CONTEXT_CLOSE_DELIMITER,
+ LLM_CONTEXT_OPEN_DELIMITER,
+)
+from superset.utils import json
+
+
+def _wrapped(value: str) -> str:
+ """Return the LLM-context-wrapped form a sanitized field should have."""
+ return f"{LLM_CONTEXT_OPEN_DELIMITER}\n{value}\n{LLM_CONTEXT_CLOSE_DELIMITER}"
+
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
+
+
+# ---------------------------------------------------------------------------
+# Fixtures
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+def mcp_server() -> object:
+ """Return the FastMCP app instance for use in MCP client tests."""
+ return mcp
+
+
+@pytest.fixture(autouse=True)
+def mock_auth() -> Iterator[MagicMock]:
+ """Mock authentication for all tests."""
+ with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user:
+ mock_user = Mock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_get_user.return_value = mock_user
+ yield mock_get_user
+
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+SOURCE_POSITIONS = {
Review Comment:
**Suggestion:** Add an explicit type annotation to this module-level constant so it complies with the required type-hint rule. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new module-level constant is a plainly annotatable Python variable, but it is defined without a type hint. That matches the required rule to flag relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=48cfc4df8359498e94bd185c76eb307a&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=48cfc4df8359498e94bd185c76eb307a&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:** tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py
**Line:** 85:85
**Comment:**
*Custom Rule: Add an explicit type annotation to this module-level constant so it complies with the required type-hint rule.
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%2F40959&comment_hash=c2d9c292ea2972b0b0bfb622b61176ec909bf0a87d50f5cb841c88a853b06114&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=c2d9c292ea2972b0b0bfb622b61176ec909bf0a87d50f5cb841c88a853b06114&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40959:
URL: https://github.com/apache/superset/pull/40959#issuecomment-4804594863
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3482569042
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,393 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
Review Comment:
**Suggestion:** The layout guard only checks whether any `CHART` node has a truthy `chartId`, but it does not verify that those IDs actually resolve to real charts on the source dashboard. If `position_json` contains stale/mismatched chart IDs, the guard passes and `set_dash_metadata` later rebuilds slices from non-existent IDs, yielding an empty copy despite source charts existing. Validate chart IDs against `source.slices` (or at least DB-resolved IDs) before allowing duplication. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP duplicate_dashboard may return empty dashboards unexpectedly.
- ⚠️ Users lose charts in copies when layout IDs go stale.
- ⚠️ AI-assisted cloning workflow can silently produce broken dashboards.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a dashboard whose `slices` relationship still contains charts (queried by
`DashboardDAO.copy_dashboard` at `superset/daos/dashboard.py:5-21`), but whose persisted
`position_json` only references chart IDs that do not correspond to any real `Slice` rows
(e.g. manually edit `dashboards.position_json` to include `{"type": "CHART", "meta":
{"chartId": 999999}}` where `Slice.id=999999` does not exist).
2. Start the Superset MCP service so that the `duplicate_dashboard` tool from
`superset/mcp_service/dashboard/tool/__init__.py:18-32` is registered, and connect an MCP
client.
3. Call the `duplicate_dashboard()` MCP tool at
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:276-289` with `dashboard_id`
pointing to the corrupted dashboard; inside the tool, `_build_copy_payload()` at
`duplicate_dashboard.py:64-102` loads `position_json` and passes it to
`_positions_reference_charts()` at `duplicate_dashboard.py:48-61`.
4. `_positions_reference_charts()` returns `True` at lines 56-61 because it only checks
for any `CHART` node with a truthy `meta["chartId"]`, without verifying that those IDs map
to existing slices on the source. The guard at `duplicate_dashboard.py:311-324` (`if
getattr(source, "slices", None) and not layout_has_charts`) is therefore bypassed,
`CopyDashboardCommand(source, data).run()` executes, and
`DashboardDAO.set_dash_metadata()` at `superset/daos/dashboard.py:5-25` computes
`slice_ids` from the stale `chartId`s and queries `Slice.id.in_(slice_ids)` at line 22.
Because there are no matching `Slice` rows, `dashboard.slices` is set to an empty list at
line 25, yielding a duplicated dashboard with no charts even though the source
`source.slices` was non-empty and the tool did not return an error.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=970466adf4704544a0bd51164f40ccc3&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=970466adf4704544a0bd51164f40ccc3&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/duplicate_dashboard.py
**Line:** 56:61
**Comment:**
*Logic Error: The layout guard only checks whether any `CHART` node has a truthy `chartId`, but it does not verify that those IDs actually resolve to real charts on the source dashboard. If `position_json` contains stale/mismatched chart IDs, the guard passes and `set_dash_metadata` later rebuilds slices from non-existent IDs, yielding an empty copy despite source charts existing. Validate chart IDs against `source.slices` (or at least DB-resolved IDs) before allowing duplication.
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%2F40959&comment_hash=20ca186b48b4321173e5003f77781b806b546e57d44b4001de06f52b7d4639f3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=20ca186b48b4321173e5003f77781b806b546e57d44b4001de06f52b7d4639f3&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,393 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload and a flag indicating whether the layout maps any
+ chart, so the caller can refuse to produce a silently empty copy.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
Review Comment:
**Suggestion:** Parsing failures for the source dashboard's `json_metadata` are silently converted to `{}`, so malformed persisted metadata produces a "successful" duplicate with defaulted/stripped settings instead of surfacing a repairable error. This can silently lose filter config, color config, and other dashboard behavior. Return a structured duplication error when `json_metadata` cannot be decoded (instead of defaulting), matching the tool's stated behavior of mirroring the source state. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP duplicate_dashboard can drop all JSON-based settings.
- ⚠️ Copied dashboards may lose filters and customizations silently.
- ⚠️ AI workflows see copies behave differently than source dashboards.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or identify a dashboard record whose persisted `json_metadata` column is
malformed JSON (e.g. set `dashboards.json_metadata` to `"{invalid"` in the database so
that `source.json_metadata` read at
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:81` is not valid JSON).
2. Start the Superset MCP service and connect an MCP client that exposes the
`duplicate_dashboard` tool from `superset/mcp_service/dashboard/tool/__init__.py:18-32`.
3. Invoke the MCP tool `duplicate_dashboard()` defined at
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:276-289` with `dashboard_id`
pointing to the malformed dashboard and a valid `dashboard_title`.
4. During execution, `_build_copy_payload()` at
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:64-102` calls
`json.loads(source.json_metadata or "{}")` inside the `try` at line 80; the malformed JSON
raises `json.JSONDecodeError`, which is caught at line 82 and replaced with `metadata =
{}` at lines 83-85. The function then builds a payload with
`json_metadata=json.dumps(metadata)` (line 100), so `CopyDashboardCommand` (instantiated
at line 327 and implemented in `superset/daos/dashboard.py:372-34`) receives a sanitized
`{}` instead of failing. The duplicate dashboard is successfully created but loses any
settings that were present in the original `json_metadata` (filter scopes, default
filters, chart customization config as consumed in `superset/daos/dashboard.py:36-76` and
`superset/views/utils.py:372-27`), contrary to the expectation that invalid stored
metadata would surface as an error as handled in the `(ValueError, TypeError)` block at
`duplicate_dashboard.py:370-387`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2626d74076644f1d9ee531e8a1c85a5b&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=2626d74076644f1d9ee531e8a1c85a5b&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/duplicate_dashboard.py
**Line:** 80:85
**Comment:**
*Logic Error: Parsing failures for the source dashboard's `json_metadata` are silently converted to `{}`, so malformed persisted metadata produces a "successful" duplicate with defaulted/stripped settings instead of surfacing a repairable error. This can silently lose filter config, color config, and other dashboard behavior. Return a structured duplication error when `json_metadata` cannot be decoded (instead of defaulting), matching the tool's stated behavior of mirroring the source state.
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%2F40959&comment_hash=5f29b4444f3b5d5a64369dded2975ee81726238c3ebadcae5fcb27431fa8da9f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40959&comment_hash=5f29b4444f3b5d5a64369dded2975ee81726238c3ebadcae5fcb27431fa8da9f&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40959:
URL: https://github.com/apache/superset/pull/40959#issuecomment-4811987413
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3493283358
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,393 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
+
+
+def _build_copy_payload(
+ source: Any, dashboard_title: str, duplicate_slices: bool
+) -> tuple[dict[str, Any], bool]:
+ """Build the data payload expected by ``CopyDashboardCommand``.
+
+ Mirrors what the frontend "Save as" flow sends to the
+ ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's
+ current ``json_metadata`` with a ``positions`` key holding the current
+ layout (``position_json``). ``DashboardCopySchema`` requires
+ ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads
+ ``positions`` from it to remap chart IDs when ``duplicate_slices``
+ is enabled.
+
+ Returns the payload and a flag indicating whether the layout maps any
+ chart, so the caller can refuse to produce a silently empty copy.
+ """
+ try:
+ metadata = json.loads(source.json_metadata or "{}")
+ except (json.JSONDecodeError, TypeError):
+ metadata = {}
+ if not isinstance(metadata, dict):
+ metadata = {}
Review Comment:
Fixed in 9d07579476. Rather than failing hard (which would block duplication of an otherwise valid dashboard just because of aesthetic metadata), the tool now surfaces a non-fatal warning in the response when json_metadata cannot be decoded or is not a JSON object. The warning tells the caller that colors, native filters, and other settings were reset to defaults while chart content was preserved from position_json. A regression test (test_malformed_json_metadata_warns_not_fails) asserts the copy proceeds, the dashboard is returned, and the warnings list contains the metadata-loss notice.
##########
superset/mcp_service/dashboard/tool/duplicate_dashboard.py:
##########
@@ -0,0 +1,393 @@
+# 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: duplicate_dashboard
+
+Duplicates an existing dashboard, optionally deep-copying its charts.
+Canonical workflow: clone a template dashboard, then edit the copy
+(e.g. to create a regional or staging variant).
+"""
+
+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.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ _sanitize_dashboard_info_for_llm_context,
+ DashboardInfo,
+ DuplicateDashboardRequest,
+ DuplicateDashboardResponse,
+ serialize_chart_summary,
+)
+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__)
+
+
+def _positions_reference_charts(positions: dict[str, Any]) -> bool:
+ """Return whether a layout maps any chart into the dashboard.
+
+ ``DashboardDAO.set_dash_metadata`` rebuilds the new dashboard's slice
+ list solely from the chart IDs found in ``positions``, so a layout
+ with no ``CHART`` entries yields an empty dashboard regardless of the
+ source's ``slices`` relationship.
+ """
+ return any(
+ isinstance(value, dict)
+ and value.get("type") == "CHART"
+ and value.get("meta", {}).get("chartId")
+ for value in positions.values()
+ )
Review Comment:
Declining to add cross-validation of position chart IDs against source.slices. The stale-IDs scenario (position_json references chart IDs that don't match the source's actual slices) is a pre-existing data consistency problem with the source dashboard — not a failure mode introduced by this tool. The existing guard already covers the primary case: if position_json maps no chart IDs at all while the source has charts, the tool fails fast with a clear error. Extending that guard to compare the two ID sets would add an O(n) set intersection check and extra code complexity for a rare edge case, and could generate confusing errors in partial-overlap scenarios. An empty copy is immediately visible and actionable. If this scenario surfaces in production it can be addressed as a follow-up.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3493691271
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -914,6 +914,138 @@ class GenerateDashboardResponse(BaseModel):
)
+class DuplicateDashboardRequest(BaseModel):
+ """Request schema for duplicating an existing dashboard."""
+
+ model_config = ConfigDict(populate_by_name=True)
+
+ dashboard_id: Annotated[
+ int | str,
+ Field(
+ description=(
+ "Source dashboard identifier - can be numeric ID, UUID string, or slug"
+ )
+ ),
+ ]
Review Comment:
Declining. Restricting to UUID/slug-only would narrow `int | str` without benefit here:
1. **Inconsistent with existing tools** — `AddChartToDashboardRequest.dashboard_id` in the same file (line 547) uses plain `int`; accepting `int | str` is already _more_ UUID-friendly than the status quo.
2. **UUID migration scope** — the CLAUDE.md UUID migration guidance targets *new DB model primary keys*, not MCP request schemas. Request schemas should reflect what the backing DAO accepts, and `DashboardDAO.get_by_id_or_slug` already handles int IDs, UUID strings, and slugs.
3. **Breaking callers** — users who look up dashboards by numeric ID (e.g. from a list response) would get an immediate schema validation error. That's a regression, not a safety improvement.
No change.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add duplicate_dashboard tool [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40959:
URL: https://github.com/apache/superset/pull/40959#issuecomment-4710278029
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org