codeant-ai-for-open-source[bot] commented on code in PR #40399: URL: https://github.com/apache/superset/pull/40399#discussion_r3422507988
########## superset/mcp_service/dashboard/tool/update_dashboard.py: ########## @@ -0,0 +1,257 @@ +# 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. + +""" +Update dashboard FastMCP tool + +This module contains the FastMCP tool for updating an existing dashboard's +layout, theme, and styling. Companion to ``generate_dashboard`` for +incremental edits without re-creating the dashboard. +""" + +import logging +from typing import Any + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.commands.dashboard.exceptions import DashboardNotFoundError +from superset.exceptions import SupersetSecurityException +from superset.extensions import db, event_logger +from superset.mcp_service.dashboard.schemas import ( + dashboard_serializer, + DashboardError, + UpdateDashboardRequest, + UpdateDashboardResponse, +) +from superset.mcp_service.utils.url_utils import get_superset_base_url +from superset.utils import json + +logger = logging.getLogger(__name__) + + +def _build_dashboard_url(dashboard: Any) -> str: + """Build the user-facing dashboard URL, preferring slug over id.""" + return ( + f"{get_superset_base_url()}/superset/dashboard/" + f"{dashboard.slug or dashboard.id}/" + ) + + +def _find_and_authorize_dashboard( + identifier: int | str, +) -> tuple[Any, UpdateDashboardResponse | DashboardError | None]: + """Return (dashboard, None) on success or (None, error_response) on failure. + + Mirrors the helper in ``add_chart_to_existing_dashboard``: combines + the not-found and forbidden cases so the main tool body has a single + pre-condition branch. Returns ``DashboardError`` on not-found and + ``UpdateDashboardResponse`` (with ``permission_denied=True``) on + ownership failure — the two shapes carry different information for + the caller. + """ + # avoids ImportError before Flask app initialisation: + # `Exception: App not initialized yet. Please call init_app first` + # raised from superset.utils.encrypt when DashboardDAO is imported + # (via Slice's encrypted Column types). `security_manager` is a + # LocalProxy that needs the same app context to resolve at call + # time, so it is co-located with the DAO it accompanies. + from superset import security_manager + from superset.daos.dashboard import DashboardDAO + + try: + dashboard = DashboardDAO.get_by_id_or_slug(identifier) + except (DashboardNotFoundError, SQLAlchemyError): + return None, DashboardError( + error=f"Dashboard not found: {identifier!r}", + error_type="DashboardNotFound", + ) Review Comment: **Suggestion:** Treating all `SQLAlchemyError` cases as "not found" is incorrect and hides real operational/database failures as a 404-like outcome. This can mislead callers into retrying with a different identifier instead of handling backend failure. Split database failures from not-found and return a database error response for SQLAlchemy exceptions. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ DB outages misreported as DashboardNotFound in update_dashboard. - ⚠️ Callers may retry with different IDs instead of handling outage. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The helper `_find_and_authorize_dashboard` used by `update_dashboard` is defined in `superset/mcp_service/dashboard/tool/update_dashboard.py:56-102` and is invoked at `update_dashboard.py:197-199` for every `update_dashboard` MCP call. 2. Inside this helper, `DashboardDAO.get_by_id_or_slug(identifier)` is called in a `try` block at `update_dashboard.py:77-79`; the `except` block at `update_dashboard.py:80-83` catches both `DashboardNotFoundError` and `SQLAlchemyError`. 3. To reproduce a database failure, in a test similar to `test_update_missing_dashboard_returns_error` (`tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py:85-104`), patch `superset.daos.dashboard.DashboardDAO.get_by_id_or_slug` to raise a concrete `SQLAlchemyError` subclass (e.g. `OperationalError`) instead of `DashboardNotFoundError`. 4. When `update_dashboard` is called under this condition, `_find_and_authorize_dashboard` returns a `DashboardError` with `error="Dashboard not found: {identifier!r}"` and `error_type="DashboardNotFound"` at `update_dashboard.py:80-83`, so the tool surfaces a 404-like "not found" error for a backend/database failure, misleading callers into treating an operational issue as a bad identifier. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=61be57c56cba4a139108fa6e5d560df3&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=61be57c56cba4a139108fa6e5d560df3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/tool/update_dashboard.py **Line:** 77:83 **Comment:** *Logic Error: Treating all `SQLAlchemyError` cases as "not found" is incorrect and hides real operational/database failures as a 404-like outcome. This can mislead callers into retrying with a different identifier instead of handling backend failure. Split database failures from not-found and return a database error response for SQLAlchemy exceptions. 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%2F40399&comment_hash=2e88ec597068b804e8a55192f69ea80348262882929a6051ee54ed53618dfd5d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=2e88ec597068b804e8a55192f69ea80348262882929a6051ee54ed53618dfd5d&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/update_dashboard.py: ########## @@ -0,0 +1,257 @@ +# 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. + +""" +Update dashboard FastMCP tool + +This module contains the FastMCP tool for updating an existing dashboard's +layout, theme, and styling. Companion to ``generate_dashboard`` for +incremental edits without re-creating the dashboard. +""" + +import logging +from typing import Any + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.commands.dashboard.exceptions import DashboardNotFoundError +from superset.exceptions import SupersetSecurityException +from superset.extensions import db, event_logger +from superset.mcp_service.dashboard.schemas import ( + dashboard_serializer, + DashboardError, + UpdateDashboardRequest, + UpdateDashboardResponse, +) +from superset.mcp_service.utils.url_utils import get_superset_base_url +from superset.utils import json + +logger = logging.getLogger(__name__) + + +def _build_dashboard_url(dashboard: Any) -> str: + """Build the user-facing dashboard URL, preferring slug over id.""" + return ( + f"{get_superset_base_url()}/superset/dashboard/" + f"{dashboard.slug or dashboard.id}/" + ) + + +def _find_and_authorize_dashboard( + identifier: int | str, +) -> tuple[Any, UpdateDashboardResponse | DashboardError | None]: + """Return (dashboard, None) on success or (None, error_response) on failure. + + Mirrors the helper in ``add_chart_to_existing_dashboard``: combines + the not-found and forbidden cases so the main tool body has a single + pre-condition branch. Returns ``DashboardError`` on not-found and + ``UpdateDashboardResponse`` (with ``permission_denied=True``) on + ownership failure — the two shapes carry different information for + the caller. + """ + # avoids ImportError before Flask app initialisation: + # `Exception: App not initialized yet. Please call init_app first` + # raised from superset.utils.encrypt when DashboardDAO is imported + # (via Slice's encrypted Column types). `security_manager` is a + # LocalProxy that needs the same app context to resolve at call + # time, so it is co-located with the DAO it accompanies. + from superset import security_manager + from superset.daos.dashboard import DashboardDAO + + try: + dashboard = DashboardDAO.get_by_id_or_slug(identifier) + except (DashboardNotFoundError, SQLAlchemyError): + return None, DashboardError( + error=f"Dashboard not found: {identifier!r}", + error_type="DashboardNotFound", + ) + + if dashboard is None: + return None, DashboardError( + error=f"Dashboard not found: {identifier!r}", + error_type="DashboardNotFound", + ) + + try: + security_manager.raise_for_ownership(dashboard) + except SupersetSecurityException: + return None, UpdateDashboardResponse( + permission_denied=True, + error=( + f"You don't have permission to edit dashboard " + f"'{dashboard.dashboard_title}' (ID: {dashboard.id})." + ), + ) + + return dashboard, None + + +def _apply_field_updates( + dashboard: Any, request: UpdateDashboardRequest +) -> list[str]: + """Apply each explicitly-passed field to the dashboard. + + Returns the names of fields actually changed. Mutates ``dashboard`` + in place. ``json_metadata_overrides`` is merged shallowly with the + existing ``json_metadata``; an empty string in ``slug`` or ``css`` + clears the underlying value. + """ + changed: list[str] = [] + + if request.dashboard_title is not None: + dashboard.dashboard_title = request.dashboard_title + changed.append("dashboard_title") + + if request.description is not None: + dashboard.description = request.description + changed.append("description") + + if request.slug is not None: + dashboard.slug = request.slug or None + changed.append("slug") + + if request.published is not None: + dashboard.published = request.published + changed.append("published") + + if request.position_json is not None: + dashboard.position_json = json.dumps(request.position_json) + changed.append("position_json") + + if request.json_metadata_overrides is not None: + existing = ( + json.loads(dashboard.json_metadata or "{}") + if dashboard.json_metadata + else {} + ) + existing.update(request.json_metadata_overrides) + dashboard.json_metadata = json.dumps(existing) + changed.append("json_metadata") Review Comment: **Suggestion:** Parsing existing `json_metadata` assumes valid JSON and does not handle decode/type errors. If stored metadata is malformed, `json.loads(...)` raises and bypasses the outer `except SQLAlchemyError`, producing an unhandled runtime error. Add safe parsing (or catch decode errors) and fall back to `{}` or a structured error. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ update_dashboard crashes on dashboards with malformed json_metadata. - ⚠️ LLM clients cannot update theme on affected dashboards. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The MCP tool `update_dashboard` at `superset/mcp_service/dashboard/tool/update_dashboard.py:164-194` applies field changes via `_apply_field_updates` inside the main `try` block at `update_dashboard.py:204-207`. 2. `_apply_field_updates` is defined at `update_dashboard.py:105-151`; when `request.json_metadata_overrides` is not `None`, it executes the block at `update_dashboard.py:137-145`, which computes `existing = json.loads(dashboard.json_metadata or "{}") if dashboard.json_metadata else {}` using `superset.utils.json`. 3. If an existing dashboard has malformed `json_metadata` (e.g. direct DB manipulation or legacy data), then `dashboard.json_metadata` is a non-empty, invalid JSON string; `json.loads(...)` raises a `json.JSONDecodeError` (a `ValueError`), which is not a subclass of `SQLAlchemyError`. 4. Because this `json.loads` call happens inside `_apply_field_updates` (called within the `event_logger.log_context` at `update_dashboard.py:204-207`) and the surrounding `try/except` at `update_dashboard.py:204-247` only catches `SQLAlchemyError`, the `JSONDecodeError` is not caught. In a test, you can reproduce by constructing a `_mock_dashboard` with `json_metadata="not-json"` (using the helper at `test_update_dashboard.py:1-7`), patching `DashboardDAO.get_by_id_or_slug` to return it, passing any `json_metadata_overrides` in the request, and observing that `client.call_tool("update_dashboard", ...)` raises a FastMCP `ToolError` instead of returning a structured `DashboardError` or falling back to `{}`. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b116a05e28654495b05f50ac49ec6c9c&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=b116a05e28654495b05f50ac49ec6c9c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/tool/update_dashboard.py **Line:** 137:145 **Comment:** *Type Error: Parsing existing `json_metadata` assumes valid JSON and does not handle decode/type errors. If stored metadata is malformed, `json.loads(...)` raises and bypasses the outer `except SQLAlchemyError`, producing an unhandled runtime error. Add safe parsing (or catch decode errors) and fall back to `{}` or a structured error. 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%2F40399&comment_hash=0ed5aada51801da93d76cc742b5c4e428b30381ba6b92b2954d56ddd6313eae2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=0ed5aada51801da93d76cc742b5c4e428b30381ba6b92b2954d56ddd6313eae2&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/update_dashboard.py: ########## @@ -0,0 +1,257 @@ +# 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. + +""" +Update dashboard FastMCP tool + +This module contains the FastMCP tool for updating an existing dashboard's +layout, theme, and styling. Companion to ``generate_dashboard`` for +incremental edits without re-creating the dashboard. +""" + +import logging +from typing import Any + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.commands.dashboard.exceptions import DashboardNotFoundError +from superset.exceptions import SupersetSecurityException +from superset.extensions import db, event_logger +from superset.mcp_service.dashboard.schemas import ( + dashboard_serializer, + DashboardError, + UpdateDashboardRequest, + UpdateDashboardResponse, +) +from superset.mcp_service.utils.url_utils import get_superset_base_url +from superset.utils import json + +logger = logging.getLogger(__name__) + + +def _build_dashboard_url(dashboard: Any) -> str: + """Build the user-facing dashboard URL, preferring slug over id.""" + return ( + f"{get_superset_base_url()}/superset/dashboard/" + f"{dashboard.slug or dashboard.id}/" + ) + + +def _find_and_authorize_dashboard( + identifier: int | str, +) -> tuple[Any, UpdateDashboardResponse | DashboardError | None]: + """Return (dashboard, None) on success or (None, error_response) on failure. + + Mirrors the helper in ``add_chart_to_existing_dashboard``: combines + the not-found and forbidden cases so the main tool body has a single + pre-condition branch. Returns ``DashboardError`` on not-found and + ``UpdateDashboardResponse`` (with ``permission_denied=True``) on + ownership failure — the two shapes carry different information for + the caller. + """ + # avoids ImportError before Flask app initialisation: + # `Exception: App not initialized yet. Please call init_app first` + # raised from superset.utils.encrypt when DashboardDAO is imported + # (via Slice's encrypted Column types). `security_manager` is a + # LocalProxy that needs the same app context to resolve at call + # time, so it is co-located with the DAO it accompanies. + from superset import security_manager + from superset.daos.dashboard import DashboardDAO + + try: + dashboard = DashboardDAO.get_by_id_or_slug(identifier) + except (DashboardNotFoundError, SQLAlchemyError): + return None, DashboardError( + error=f"Dashboard not found: {identifier!r}", + error_type="DashboardNotFound", + ) + + if dashboard is None: + return None, DashboardError( + error=f"Dashboard not found: {identifier!r}", + error_type="DashboardNotFound", + ) + + try: + security_manager.raise_for_ownership(dashboard) + except SupersetSecurityException: + return None, UpdateDashboardResponse( + permission_denied=True, + error=( + f"You don't have permission to edit dashboard " + f"'{dashboard.dashboard_title}' (ID: {dashboard.id})." + ), + ) + + return dashboard, None + + +def _apply_field_updates( + dashboard: Any, request: UpdateDashboardRequest +) -> list[str]: + """Apply each explicitly-passed field to the dashboard. + + Returns the names of fields actually changed. Mutates ``dashboard`` + in place. ``json_metadata_overrides`` is merged shallowly with the + existing ``json_metadata``; an empty string in ``slug`` or ``css`` + clears the underlying value. + """ + changed: list[str] = [] + + if request.dashboard_title is not None: + dashboard.dashboard_title = request.dashboard_title + changed.append("dashboard_title") + + if request.description is not None: + dashboard.description = request.description + changed.append("description") + + if request.slug is not None: + dashboard.slug = request.slug or None + changed.append("slug") + + if request.published is not None: + dashboard.published = request.published + changed.append("published") + + if request.position_json is not None: + dashboard.position_json = json.dumps(request.position_json) + changed.append("position_json") + + if request.json_metadata_overrides is not None: + existing = ( + json.loads(dashboard.json_metadata or "{}") + if dashboard.json_metadata + else {} + ) + existing.update(request.json_metadata_overrides) + dashboard.json_metadata = json.dumps(existing) + changed.append("json_metadata") + + if request.css is not None: + dashboard.css = request.css or None + changed.append("css") + + return changed + + +@tool( + tags=["mutate"], + class_permission_name="Dashboard", + method_permission_name="write", + annotations=ToolAnnotations( + title="Update dashboard layout/theme/CSS", + readOnlyHint=False, + destructiveHint=False, + ), +) +def update_dashboard( + request: UpdateDashboardRequest, ctx: Context +) -> UpdateDashboardResponse | DashboardError: + """Patch an existing dashboard's layout, theme, or styling. + + Companion to ``generate_dashboard`` for incremental edits. Accepts + the same layout/theme/CSS fields that ``generate_dashboard`` does, so + an LLM can: + + - Set or replace ``position_json`` after auto-generation + - Apply brand ``label_colors`` and ``color_scheme`` via + ``json_metadata_overrides`` + - Toggle ``cross_filters_enabled`` via ``json_metadata_overrides`` + - Inject ``css`` to hide chrome on print-ready dashboards + - Update ``dashboard_title``, ``description``, ``slug``, ``published`` + + Only the fields explicitly passed are applied; other fields are left + unchanged. ``json_metadata_overrides`` is merged shallowly with the + existing json_metadata — pass only the keys you want to change. + + Example:: + + update_dashboard(request={ + "identifier": 42, + "json_metadata_overrides": { + "label_colors": {"Electronics": "#4C78A8"}, + "cross_filters_enabled": False, + }, + "css": ".header-controls {display: none;}", + }) + """ + ctx.info(f"Updating dashboard: identifier={request.identifier}") + + dashboard, auth_error = _find_and_authorize_dashboard(request.identifier) + if auth_error is not None: + return auth_error + + changed_fields: list[str] = [] + warnings: list[str] = list(request.sanitization_warnings) + + try: + with event_logger.log_context(action="mcp.update_dashboard.apply"): + changed_fields = _apply_field_updates(dashboard, request) + + if not changed_fields: + warnings.append("No fields provided; dashboard unchanged.") + return UpdateDashboardResponse( + dashboard=dashboard_serializer(dashboard), + dashboard_url=_build_dashboard_url(dashboard), + error=None, + changed_fields=[], + warnings=warnings, + ) + + db.session.commit() # pylint: disable=consider-using-transaction + try: + db.session.refresh(dashboard) + except SQLAlchemyError: + logger.warning( + "Dashboard %s updated but refresh failed; " + "continuing with current values", + dashboard.id, + exc_info=True, + ) + warnings.append( + "Dashboard updated but post-update refresh failed; " + "returned values may not reflect database state." + ) + + except SQLAlchemyError as db_err: + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning( + "Database rollback failed during error handling", + exc_info=True, + ) + logger.error( + "Dashboard update failed: %s", db_err, exc_info=True + ) + return DashboardError( + error="Failed to update dashboard due to a database error.", + error_type="DatabaseError", + ) + + ctx.info(f"Dashboard {dashboard.id} updated: changed={changed_fields}") + + return UpdateDashboardResponse( + dashboard=dashboard_serializer(dashboard), + dashboard_url=_build_dashboard_url(dashboard), + error=None, + changed_fields=changed_fields, + warnings=warnings, + ) Review Comment: **Suggestion:** Serialization runs after the protected DB block, so session/lazy-load failures during `dashboard_serializer(...)` are uncaught. Similar MCP dashboard tools already guard this path because session invalidation can happen after commit; this path should also wrap/fallback to avoid post-commit crashes. [stale reference] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ update_dashboard may fail after successful DB commit. - ⚠️ Clients lose structured info on layout/theme changes. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. After applying field updates and committing the transaction in `update_dashboard` (`superset/mcp_service/dashboard/tool/update_dashboard.py:204-248`), the code exits the `try/except SQLAlchemyError` block and logs a message at `update_dashboard.py:249`. 2. The final response is constructed and returned at `update_dashboard.py:251-257` by calling `UpdateDashboardResponse(dashboard=dashboard_serializer(dashboard), ...)` where `dashboard_serializer` is imported from `superset.mcp_service.dashboard.schemas` at `update_dashboard.py:36-41`. 3. `dashboard_serializer` is implemented in `superset/mcp_service/dashboard/schemas.py:10-73` and accesses relationships like `dashboard.slices` and `dashboard.tags` and iterates them to build `charts` and `tags`; these are lazily loaded ORM relationships and may trigger additional database queries during serialization. 4. In a multi-tenant or failure scenario where the session is invalidated after commit (the generate-dashboard tool explicitly guards this path; see its post-commit re-fetch and `except SQLAlchemyError` fallback at `superset/mcp_service/dashboard/tool/generate_dashboard.py:116-165`), or simply by patching `dashboard_serializer` to raise `SQLAlchemyError` in a test, calling `update_dashboard` with any field that causes `changed_fields` to be non-empty leads to the serializer raising outside the `try/except` block, causing an uncaught exception and FastMCP `ToolError` instead of a structured `UpdateDashboardResponse` with error or minimal data. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0062e75805db494485ac59fcd3142be6&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=0062e75805db494485ac59fcd3142be6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/tool/update_dashboard.py **Line:** 251:257 **Comment:** *Stale Reference: Serialization runs after the protected DB block, so session/lazy-load failures during `dashboard_serializer(...)` are uncaught. Similar MCP dashboard tools already guard this path because session invalidation can happen after commit; this path should also wrap/fallback to avoid post-commit crashes. 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%2F40399&comment_hash=da4d692e6f06eda48a4ed1720689f3b8cd9ecd7709e7930f52fdedc230930c0a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=da4d692e6f06eda48a4ed1720689f3b8cd9ecd7709e7930f52fdedc230930c0a&reaction=dislike'>👎</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
