codeant-ai-for-open-source[bot] commented on code in PR #40957: URL: https://github.com/apache/superset/pull/40957#discussion_r3417316298
########## superset/mcp_service/dashboard/tool/update_dashboard.py: ########## @@ -0,0 +1,363 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +MCP tool: update_dashboard + +This tool performs a partial update of dashboard metadata (title, slug, +published state, CSS, and selected json_metadata settings). +""" + +import logging +from typing import Any + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DashboardInfo, + serialize_chart_summary, + UpdateDashboardRequest, + UpdateDashboardResponse, +) +from superset.mcp_service.privacy import user_can_view_data_model_metadata +from superset.mcp_service.utils.url_utils import get_superset_base_url +from superset.utils import json + +logger = logging.getLogger(__name__) + +# Direct dashboard columns accepted by UpdateDashboardCommand +# (subset of DashboardPutSchema). +_DIRECT_FIELDS = ( + "dashboard_title", + "slug", + "published", + "css", +) + +# Convenience fields stored inside the dashboard's json_metadata blob. +_METADATA_FIELDS = ( + "cross_filters_enabled", + "refresh_frequency", + "filter_bar_orientation", +) Review Comment: **Suggestion:** `color_scheme` is not included in the metadata field allowlist, so requests that try to update only this documented field are silently ignored and end up returning "No fields provided to update." Add `color_scheme` to the metadata merge list so the tool actually applies that update. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ update_dashboard MCP tool cannot modify dashboard color_scheme. - ⚠️ Users see misleading "No fields provided to update." ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In the MCP client, call the `update_dashboard` tool defined in `superset/mcp_service/dashboard/tool/update_dashboard.py:252-350` with a payload like `{"dashboard_id": 1, "color_scheme": "supersetColors"}` (only `color_scheme` plus the required `dashboard_id`). 2. The request is parsed into `UpdateDashboardRequest` (`superset/mcp_service/dashboard/schemas.py:72-124`), which currently defines `cross_filters_enabled`, `refresh_frequency`, and `filter_bar_orientation` but no `color_scheme`; with Pydantic's default `extra="ignore"`, the `color_scheme` field is silently dropped and never appears as an attribute on the request model. 3. Inside `update_dashboard`, `_build_update_properties` is called at `update_dashboard.py:289`, and it constructs `metadata_changes` from `_METADATA_FIELDS` (`update_dashboard.py:56-60`), which contains only `cross_filters_enabled`, `refresh_frequency`, and `filter_bar_orientation`; since `request` has no attributes for those and `color_scheme` is not in the allowlist, `metadata_changes` and `properties` both remain empty. 4. Because `properties` is empty, `update_dashboard` returns early at `update_dashboard.py:289-296` with `UpdateDashboardResponse(error="No fields provided to update...")`, so a caller attempting to update only the documented `color_scheme` setting sees a no-op error and the dashboard's `json_metadata.color_scheme` (managed by `DashboardDAO.set_dash_metadata` in `superset/daos/dashboard.py:14-21`) is never updated. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4327e27a42634896b98b4c7864246b66&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=4327e27a42634896b98b4c7864246b66&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:** 56:60 **Comment:** *Incomplete Implementation: `color_scheme` is not included in the metadata field allowlist, so requests that try to update only this documented field are silently ignored and end up returning "No fields provided to update." Add `color_scheme` to the metadata merge list so the tool actually applies that update. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=2ea06430763ce46b131818f592b8ff066ac25619f08436f3f59e633fe28abde5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=2ea06430763ce46b131818f592b8ff066ac25619f08436f3f59e633fe28abde5&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/update_dashboard.py: ########## @@ -0,0 +1,363 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +MCP tool: update_dashboard + +This tool performs a partial update of dashboard metadata (title, slug, +published state, CSS, and selected json_metadata settings). +""" + +import logging +from typing import Any + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DashboardInfo, + serialize_chart_summary, + UpdateDashboardRequest, + UpdateDashboardResponse, +) +from superset.mcp_service.privacy import user_can_view_data_model_metadata +from superset.mcp_service.utils.url_utils import get_superset_base_url +from superset.utils import json + +logger = logging.getLogger(__name__) + +# Direct dashboard columns accepted by UpdateDashboardCommand +# (subset of DashboardPutSchema). +_DIRECT_FIELDS = ( + "dashboard_title", + "slug", + "published", + "css", +) + +# Convenience fields stored inside the dashboard's json_metadata blob. +_METADATA_FIELDS = ( + "cross_filters_enabled", + "refresh_frequency", + "filter_bar_orientation", +) + + +def _build_update_properties( + request: UpdateDashboardRequest, dashboard: Any +) -> tuple[dict[str, Any], list[str]]: + """Build the UpdateDashboardCommand properties dict from the request. + + Returns ``(properties, updated_fields)`` where *updated_fields* lists + the request fields that will be changed. + + json_metadata is a stringified JSON blob and + ``DashboardDAO.set_dash_metadata`` resets absent keys to defaults + (e.g. ``expanded_slices`` -> {}). To avoid silently destroying state, + the dashboard's FULL current json_metadata is read, the requested + changes are merged in, and the complete blob is written back. + """ + properties: dict[str, Any] = {} + updated_fields: list[str] = [] + + for field in _DIRECT_FIELDS: + value = getattr(request, field) + if value is not None: + properties[field] = value + updated_fields.append(field) + + metadata_changes = { + field: value + for field in _METADATA_FIELDS + if (value := getattr(request, field)) is not None + } + if metadata_changes: + try: + current_metadata = json.loads(dashboard.json_metadata or "{}") + except (ValueError, TypeError): + logger.warning( + "Failed to parse existing json_metadata for dashboard %s; " + "starting from an empty metadata object", + dashboard.id, + ) + current_metadata = {} + if not isinstance(current_metadata, dict): + current_metadata = {} + properties["json_metadata"] = json.dumps( + {**current_metadata, **metadata_changes} + ) + updated_fields.extend(metadata_changes) + + return properties, updated_fields + + +def _find_and_authorize_dashboard( + dashboard_id: int, +) -> tuple[Any, UpdateDashboardResponse | None]: + """Return (dashboard, None) on success or (None, error_response) on failure.""" + from superset import security_manager + from superset.daos.dashboard import DashboardDAO + from superset.exceptions import SupersetSecurityException + + dashboard = DashboardDAO.find_by_id(dashboard_id) + if not dashboard: + return None, UpdateDashboardResponse( + error=( + f"Dashboard with ID {dashboard_id} not found." + " Use list_dashboards to get valid dashboard IDs." + ), + ) + + try: + security_manager.raise_for_ownership(dashboard) + except SupersetSecurityException: + return None, UpdateDashboardResponse( + permission_denied=True, + error=( + f"You don't have permission to edit dashboard " + f"'{dashboard.dashboard_title}' (ID: {dashboard_id})." + ), + ) + + return dashboard, None + + +def _serialize_updated_dashboard( + updated_dashboard: Any, updated_fields: list[str] +) -> UpdateDashboardResponse: + """Build the success response, re-fetching with eager-loaded relationships. + + The preceding command commit may invalidate the session in multi-tenant + environments; on re-fetch failure, return a minimal response using only + scalar attributes that are already loaded — relationship fields (tags, + slices) would trigger lazy-loading on the same dead session. + """ + from sqlalchemy.orm import subqueryload + + from superset import db + from superset.daos.dashboard import DashboardDAO + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + + dashboard_url = ( + f"{get_superset_base_url()}/superset/dashboard/{updated_dashboard.id}/" + ) + + try: + updated_dashboard = ( + DashboardDAO.find_by_id( + updated_dashboard.id, + query_options=[ + subqueryload(Dashboard.slices).subqueryload(Slice.tags), + subqueryload(Dashboard.tags), + ], + ) + or updated_dashboard + ) + except SQLAlchemyError: + logger.warning( + "Re-fetch of dashboard %s failed; returning minimal response", + updated_dashboard.id, + exc_info=True, + ) + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning( + "Database rollback failed during dashboard re-fetch error handling", + exc_info=True, + ) + return UpdateDashboardResponse( + dashboard=DashboardInfo( + id=updated_dashboard.id, + dashboard_title=updated_dashboard.dashboard_title, + published=updated_dashboard.published, + url=dashboard_url, + ), + dashboard_url=dashboard_url, + updated_fields=updated_fields, + error=None, + ) + + from superset.mcp_service.dashboard.schemas import serialize_tag_object + + include_data_model_metadata = user_can_view_data_model_metadata() + dashboard_info = DashboardInfo( + id=updated_dashboard.id, + dashboard_title=updated_dashboard.dashboard_title, + slug=updated_dashboard.slug, + description=updated_dashboard.description, + css=updated_dashboard.css, + certified_by=updated_dashboard.certified_by, + certification_details=updated_dashboard.certification_details, + published=updated_dashboard.published, + created_on=updated_dashboard.created_on, + changed_on=updated_dashboard.changed_on, + uuid=str(updated_dashboard.uuid) if updated_dashboard.uuid else None, + url=dashboard_url, + chart_count=len(updated_dashboard.slices), + tags=[ + obj + for tag in getattr(updated_dashboard, "tags", []) + if (obj := serialize_tag_object(tag)) is not None + ], + charts=[ + obj + for chart in getattr(updated_dashboard, "slices", []) + if ( + obj := serialize_chart_summary( + chart, + include_data_model_metadata=include_data_model_metadata, + ) + ) + is not None + ], + ) Review Comment: **Suggestion:** The success response builds `DashboardInfo` directly from database fields without passing through the project's dashboard sanitization serializer, so stored dashboard/chart/tag text can be returned to the LLM context unsanitized. Reuse the sanitized dashboard serializer path before returning this payload. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ update_dashboard leaks unsanitized dashboard fields into LLM context. - ⚠️ Stored prompt-injection text can bypass dashboard serializers. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Create or edit a dashboard through the regular Superset UI or REST API so that fields like `dashboard_title`, `description`, `css`, or tag/chart text contain attacker-controlled prompt-injection patterns (these are persisted on the `Dashboard` model and surfaced via `DashboardInfo` in `superset/mcp_service/dashboard/schemas.py:35-55`). 2. From an MCP client, invoke the `update_dashboard` tool (`superset/mcp_service/dashboard/tool/update_dashboard.py:252-350`) to change some benign property (for example, `{"dashboard_id": <id>, "published": true}`); on success, the tool calls `_serialize_updated_dashboard(updated_dashboard, updated_fields)` at `update_dashboard.py:349`. 3. Inside `_serialize_updated_dashboard`, the code bypasses the shared sanitizing serializer pipeline in `superset/mcp_service/dashboard/schemas.py` (specifically `_sanitize_dashboard_info_for_llm_context` and `dashboard_serializer` at `schemas.py:1188-80`) and instead constructs `dashboard_info = DashboardInfo(...)` directly from ORM attributes at `update_dashboard.py:202-232`, pulling raw `dashboard_title`, `description`, `css`, tag objects (via `serialize_tag_object`), and chart summaries (via `serialize_chart_summary`), none of which have been wrapped with `sanitize_for_llm_context` or `escape_llm_context_delimiters`. 4. The resulting `UpdateDashboardResponse` from `_serialize_updated_dashboard` is returned to the MCP caller at `update_dashboard.py:234-239`, and because this tool does not use `dashboard_serializer` (as `get_dashboard_info` does at `superset/mcp_service/dashboard/tool/get_dashboard_info.py:164-173`) or otherwise call `_sanitize_dashboard_info_for_llm_context`, any stored prompt-injection strings in those fields are exposed directly into the LLM tool response, contrary to the sanitization strategy applied in the read-path MCP dashboard tools. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fa614ec39fa24653863b4a8a8af99bcb&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=fa614ec39fa24653863b4a8a8af99bcb&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:** 202:232 **Comment:** *Security: The success response builds `DashboardInfo` directly from database fields without passing through the project's dashboard sanitization serializer, so stored dashboard/chart/tag text can be returned to the LLM context unsanitized. Reuse the sanitized dashboard serializer path before returning this 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%2F40957&comment_hash=6eea97157cfc1151ada4b74968d724202b99321ae326274ec702d50791adffb9&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=6eea97157cfc1151ada4b74968d724202b99321ae326274ec702d50791adffb9&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/update_dashboard.py: ########## @@ -0,0 +1,363 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +MCP tool: update_dashboard + +This tool performs a partial update of dashboard metadata (title, slug, +published state, CSS, and selected json_metadata settings). +""" + +import logging +from typing import Any + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DashboardInfo, + serialize_chart_summary, + UpdateDashboardRequest, + UpdateDashboardResponse, +) +from superset.mcp_service.privacy import user_can_view_data_model_metadata +from superset.mcp_service.utils.url_utils import get_superset_base_url +from superset.utils import json + +logger = logging.getLogger(__name__) + +# Direct dashboard columns accepted by UpdateDashboardCommand +# (subset of DashboardPutSchema). +_DIRECT_FIELDS = ( + "dashboard_title", + "slug", + "published", + "css", +) + +# Convenience fields stored inside the dashboard's json_metadata blob. +_METADATA_FIELDS = ( + "cross_filters_enabled", + "refresh_frequency", + "filter_bar_orientation", +) + + +def _build_update_properties( + request: UpdateDashboardRequest, dashboard: Any +) -> tuple[dict[str, Any], list[str]]: + """Build the UpdateDashboardCommand properties dict from the request. + + Returns ``(properties, updated_fields)`` where *updated_fields* lists + the request fields that will be changed. + + json_metadata is a stringified JSON blob and + ``DashboardDAO.set_dash_metadata`` resets absent keys to defaults + (e.g. ``expanded_slices`` -> {}). To avoid silently destroying state, + the dashboard's FULL current json_metadata is read, the requested + changes are merged in, and the complete blob is written back. + """ + properties: dict[str, Any] = {} + updated_fields: list[str] = [] + + for field in _DIRECT_FIELDS: + value = getattr(request, field) + if value is not None: + properties[field] = value + updated_fields.append(field) + + metadata_changes = { + field: value + for field in _METADATA_FIELDS + if (value := getattr(request, field)) is not None + } + if metadata_changes: + try: + current_metadata = json.loads(dashboard.json_metadata or "{}") + except (ValueError, TypeError): + logger.warning( + "Failed to parse existing json_metadata for dashboard %s; " + "starting from an empty metadata object", + dashboard.id, + ) + current_metadata = {} + if not isinstance(current_metadata, dict): + current_metadata = {} + properties["json_metadata"] = json.dumps( + {**current_metadata, **metadata_changes} + ) + updated_fields.extend(metadata_changes) + + return properties, updated_fields + + +def _find_and_authorize_dashboard( + dashboard_id: int, +) -> tuple[Any, UpdateDashboardResponse | None]: + """Return (dashboard, None) on success or (None, error_response) on failure.""" + from superset import security_manager + from superset.daos.dashboard import DashboardDAO + from superset.exceptions import SupersetSecurityException + + dashboard = DashboardDAO.find_by_id(dashboard_id) + if not dashboard: + return None, UpdateDashboardResponse( + error=( + f"Dashboard with ID {dashboard_id} not found." + " Use list_dashboards to get valid dashboard IDs." + ), + ) + + try: + security_manager.raise_for_ownership(dashboard) + except SupersetSecurityException: + return None, UpdateDashboardResponse( + permission_denied=True, + error=( + f"You don't have permission to edit dashboard " + f"'{dashboard.dashboard_title}' (ID: {dashboard_id})." + ), + ) + + return dashboard, None + + +def _serialize_updated_dashboard( + updated_dashboard: Any, updated_fields: list[str] +) -> UpdateDashboardResponse: + """Build the success response, re-fetching with eager-loaded relationships. + + The preceding command commit may invalidate the session in multi-tenant + environments; on re-fetch failure, return a minimal response using only + scalar attributes that are already loaded — relationship fields (tags, + slices) would trigger lazy-loading on the same dead session. + """ + from sqlalchemy.orm import subqueryload + + from superset import db + from superset.daos.dashboard import DashboardDAO + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + + dashboard_url = ( + f"{get_superset_base_url()}/superset/dashboard/{updated_dashboard.id}/" + ) + + try: + updated_dashboard = ( + DashboardDAO.find_by_id( + updated_dashboard.id, + query_options=[ + subqueryload(Dashboard.slices).subqueryload(Slice.tags), + subqueryload(Dashboard.tags), + ], + ) + or updated_dashboard + ) + except SQLAlchemyError: + logger.warning( + "Re-fetch of dashboard %s failed; returning minimal response", + updated_dashboard.id, + exc_info=True, + ) + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning( + "Database rollback failed during dashboard re-fetch error handling", + exc_info=True, + ) + return UpdateDashboardResponse( + dashboard=DashboardInfo( + id=updated_dashboard.id, + dashboard_title=updated_dashboard.dashboard_title, + published=updated_dashboard.published, + url=dashboard_url, + ), + dashboard_url=dashboard_url, + updated_fields=updated_fields, + error=None, + ) Review Comment: **Suggestion:** In the re-fetch failure fallback, the code still dereferences ORM attributes from `updated_dashboard` after a session/DB failure; those attributes may be expired/detached and trigger another SQLAlchemy error, turning a successful update into an error response. Capture primitive values before the risky re-fetch or build fallback data without touching potentially detached ORM attributes. [stale reference] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Successful dashboard updates may be reported as failures. - ⚠️ Error-handling path can raise additional SQLAlchemy errors. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. An MCP client calls the `update_dashboard` tool (`superset/mcp_service/dashboard/tool/update_dashboard.py:252-350`) with a valid request, and `UpdateDashboardCommand(request.dashboard_id, properties).run()` at `update_dashboard.py:321-322` successfully updates the dashboard and commits the transaction (via the `@transaction` decorator in `superset/commands/dashboard/update.py:58-79`), returning an ORM instance `updated_dashboard`. 2. After the commit, `update_dashboard` calls `_serialize_updated_dashboard(updated_dashboard, updated_fields)` at `update_dashboard.py:349`; inside this helper, `DashboardDAO.find_by_id` with `subqueryload` options is invoked at `update_dashboard.py:165-171` to re-fetch the dashboard with eager-loaded relationships, but in a multi-tenant or DB-error scenario this call can raise `SQLAlchemyError` (e.g., "Can't reconnect until invalid transaction is rolled back"). 3. The `SQLAlchemyError` is caught by the `except SQLAlchemyError:` block at `update_dashboard.py:174-187`, which calls `db.session.rollback()` at `update_dashboard.py:181-186` to clear the invalid transaction; the code then immediately builds a fallback `UpdateDashboardResponse` using `updated_dashboard.id`, `updated_dashboard.dashboard_title`, and `updated_dashboard.published` at `update_dashboard.py:188-23`, even though `updated_dashboard` is still bound to the now-rolled-back session and its non-primary-key columns may be expired due to `expire_on_commit=True`. 4. Accessing `updated_dashboard.dashboard_title` or `updated_dashboard.published` in this state can trigger a second lazy refresh using the invalidated session, raising another `SQLAlchemyError` that escapes `_serialize_updated_dashboard` (no inner try/except) and is only caught by the outer `update_dashboard` try/except at `update_dashboard.py:351-362`, which logs an error and returns `UpdateDashboardResponse(error="Failed to update dashboard: ...")` — even though the dashboard update already committed successfully in step 1 — causing callers to see the operation as a failure. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ce1ae69534dc4270b9763104063607a5&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=ce1ae69534dc4270b9763104063607a5&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:** 174:197 **Comment:** *Stale Reference: In the re-fetch failure fallback, the code still dereferences ORM attributes from `updated_dashboard` after a session/DB failure; those attributes may be expired/detached and trigger another SQLAlchemy error, turning a successful update into an error response. Capture primitive values before the risky re-fetch or build fallback data without touching potentially detached ORM attributes. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=dc4602616332059a70e927ebeb0e9077d742dfc302112b542ad358c3ee8035e4&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40957&comment_hash=dc4602616332059a70e927ebeb0e9077d742dfc302112b542ad358c3ee8035e4&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]
