You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2026/09/16 15:15:49 UTC
(superset) 01/01: fix(mcp): eagerly refresh chart after lookup in get_chart_sql (SC-121078)
This is an automated email from the ASF dual-hosted git repository.
eschutho pushed a commit to branch fix-mcp-get-chart-sql-detached-instance
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 252c780d3372f06703de8467d5208313b5b0fd95
Author: Elizabeth Thompson <es...@gmail.com>
AuthorDate: Wed Sep 16 15:15:28 2026 +0000
fix(mcp): eagerly refresh chart after lookup in get_chart_sql (SC-121078)
Add db.session.refresh(chart) immediately after _find_chart_by_identifier()
returns in _handle_chart_sql_request, mirroring the identical fix already
present in get_chart_preview.py, generate_chart.py, and the dashboard tools.
Without the refresh, the chart ORM instance's attributes expire after the
event_logger.log_context commit (DBEventLogger.log calls session.commit with
expire_on_commit=True). The subsequent await ctx.info/warning calls are real
event-loop yield points, after which accessing chart.params in
_resolve_effective_form_data or chart.query_context in
_sql_from_saved_query_context triggers a lazy reload on a detached session,
raising DetachedInstanceError.
As defense-in-depth, also add an except SQLAlchemyError handler to the outer
get_chart_sql() try/except, so any residual session-related errors return a
structured ChartError instead of propagating as unhandled exceptions.
Fixes SUPERSET-PYTHON-1739
Co-Authored-By: Claude Opus 4.6 <no...@anthropic.com>
---
superset/mcp_service/chart/tool/get_chart_sql.py | 23 ++-
.../mcp_service/chart/tool/test_get_chart_sql.py | 158 +++++++++++++++++++++
2 files changed, 180 insertions(+), 1 deletion(-)
diff --git a/superset/mcp_service/chart/tool/get_chart_sql.py b/superset/mcp_service/chart/tool/get_chart_sql.py
index 33a7276ba95..b6a5164318d 100644
--- a/superset/mcp_service/chart/tool/get_chart_sql.py
+++ b/superset/mcp_service/chart/tool/get_chart_sql.py
@@ -24,6 +24,7 @@ from typing import Any, TYPE_CHECKING
from fastmcp import Context
from marshmallow import ValidationError as MarshmallowValidationError
+from sqlalchemy.exc import SQLAlchemyError
from superset_core.mcp.decorators import tool, ToolAnnotations
if TYPE_CHECKING:
@@ -33,7 +34,7 @@ from superset.charts.data.form_data import set_query_context_form_data
from superset.commands.exceptions import CommandException
from superset.commands.explore.form_data.parameters import CommandParameters
from superset.exceptions import SupersetException, SupersetSecurityException
-from superset.extensions import event_logger
+from superset.extensions import db, event_logger
from superset.mcp_service.chart.chart_helpers import (
build_query_context_from_form_data,
extract_x_axis_col,
@@ -449,6 +450,17 @@ async def get_chart_sql(
try:
return await _handle_chart_sql_request(request, ctx)
+ except SQLAlchemyError as e:
+ await ctx.error(
+ "Chart SQL retrieval failed due to database session error: "
+ "identifier=%s, error_type=%s, error=%s"
+ % (request.identifier, type(e).__name__, str(e))
+ )
+ logger.exception("SQLAlchemy error in get_chart_sql: %s", e)
+ return ChartError(
+ error=f"Chart SQL retrieval failed due to a database session error: {e}",
+ error_type="DatabaseError",
+ )
except SupersetException as e:
logger.exception("Superset error in get_chart_sql")
return ChartError(
@@ -498,6 +510,15 @@ async def _handle_chart_sql_request(
error_type="NotFound",
)
+ # Eagerly refresh all attributes while the session is still active. The
+ # log_context commit above (DBEventLogger.log) expires this instance via
+ # SQLAlchemy's expire_on_commit, and the awaits below are real event-loop
+ # yield points after which the MCP per-call session can be torn down —
+ # a later lazy reload (e.g. chart.params in _resolve_effective_form_data,
+ # chart.query_context in _sql_from_saved_query_context) would then raise
+ # DetachedInstanceError. See get_chart_preview.py for the same pattern.
+ db.session.refresh(chart)
+
await ctx.info(
"Chart found: chart_id=%s, chart_name=%s, viz_type=%s"
% (chart.id, chart.slice_name, chart.viz_type)
diff --git a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
index 50bcfcf9272..0ef5bfd703b 100644
--- a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
+++ b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
@@ -1654,3 +1654,161 @@ class TestRejectedFilterColumnsAreSurfaced:
)
assert isinstance(result, ChartSql)
+
+
+class TestDetachedInstanceError:
+ """Tests that DetachedInstanceError is handled gracefully.
+
+ When the SQLAlchemy session commits mid-request (e.g. DBEventLogger.log),
+ ORM objects expire and become detached. Accessing lazy attributes on a
+ detached Slice raises DetachedInstanceError. The tool must:
+ 1. Call db.session.refresh() immediately after loading the chart so all
+ column values are loaded upfront before any downstream operation.
+ 2. Catch SQLAlchemyError (the base class) and return a ChartError
+ instead of propagating the exception.
+ """
+
+ @pytest.mark.asyncio
+ async def test_session_refresh_called_after_chart_load(self):
+ """db.session.refresh() is invoked right after _find_chart_by_identifier."""
+ from contextlib import nullcontext
+ from unittest.mock import MagicMock, patch
+
+ from superset.utils import json
+
+ mock_chart = MagicMock()
+ mock_chart.id = 42
+ mock_chart.slice_name = "Sales Chart"
+ mock_chart.viz_type = "table"
+ mock_chart.datasource_id = 1
+ mock_chart.datasource_type = "table"
+ mock_chart.params = "{}"
+
+ refresh_calls: list[object] = []
+
+ def _fake_refresh(obj: object) -> None:
+ refresh_calls.append(obj)
+
+ chart_sql_result = ChartSql(
+ chart_id=42,
+ chart_name="Sales Chart",
+ sql="SELECT COUNT(*) FROM sales",
+ language="sql",
+ datasource_name="sales",
+ )
+
+ with (
+ patch.object(
+ _get_chart_sql_mod,
+ "_find_chart_by_identifier",
+ return_value=mock_chart,
+ ),
+ patch.object(
+ _get_chart_sql_mod.db,
+ "session",
+ **{"refresh.side_effect": _fake_refresh},
+ ),
+ patch.object(
+ _get_chart_sql_mod,
+ "validate_chart_dataset",
+ return_value=MagicMock(is_valid=True, warnings=[]),
+ ),
+ patch.object(
+ _get_chart_sql_mod.event_logger,
+ "log_context",
+ return_value=nullcontext(),
+ ),
+ patch.object(
+ _get_chart_sql_mod,
+ "_resolve_effective_form_data",
+ return_value=({"metrics": ["count"]}, False),
+ ),
+ patch.object(
+ _get_chart_sql_mod,
+ "_sql_from_saved_query_context",
+ return_value=chart_sql_result,
+ ),
+ ):
+ from fastmcp import Client
+
+ from superset.mcp_service.app import mcp
+
+ with patch("superset.mcp_service.auth.get_user_from_request") as mu:
+ mu.return_value = MagicMock(id=1, username="admin")
+ with patch(
+ "superset.mcp_service.auth.check_tool_permission",
+ return_value=True,
+ ):
+ async with Client(mcp) as client:
+ response = await client.call_tool(
+ "get_chart_sql",
+ {"request": GetChartSqlRequest(identifier=42).model_dump()},
+ )
+
+ data = json.loads(response.content[0].text)
+ assert "error_type" not in data, (
+ f"Expected ChartSql but got ChartError: {data.get('error')}"
+ )
+ assert data.get("chart_id") == 42
+
+ assert len(refresh_calls) == 1, (
+ "db.session.refresh() should be called once after loading the chart"
+ )
+ assert refresh_calls[0] is mock_chart
+
+ @pytest.mark.asyncio
+ async def test_detached_instance_error_returns_chart_error(self):
+ """DetachedInstanceError during SQL retrieval returns ChartError."""
+ from contextlib import nullcontext
+ from unittest.mock import MagicMock, patch
+
+ from sqlalchemy.orm.exc import DetachedInstanceError
+
+ from superset.utils import json
+
+ mock_chart = MagicMock()
+ mock_chart.id = 42
+ mock_chart.slice_name = "Sales Chart"
+ mock_chart.viz_type = "table"
+ mock_chart.datasource_id = 1
+
+ with (
+ patch.object(
+ _get_chart_sql_mod,
+ "_find_chart_by_identifier",
+ return_value=mock_chart,
+ ),
+ patch.object(
+ _get_chart_sql_mod.db,
+ "session",
+ **{
+ "refresh.side_effect": DetachedInstanceError(
+ "Instance is not bound to a Session"
+ )
+ },
+ ),
+ patch.object(
+ _get_chart_sql_mod.event_logger,
+ "log_context",
+ return_value=nullcontext(),
+ ),
+ ):
+ from fastmcp import Client
+
+ from superset.mcp_service.app import mcp
+
+ with patch("superset.mcp_service.auth.get_user_from_request") as mu:
+ mu.return_value = MagicMock(id=1, username="admin")
+ with patch(
+ "superset.mcp_service.auth.check_tool_permission",
+ return_value=True,
+ ):
+ async with Client(mcp) as client:
+ response = await client.call_tool(
+ "get_chart_sql",
+ {"request": GetChartSqlRequest(identifier=42).model_dump()},
+ )
+
+ data = json.loads(response.content[0].text)
+ assert data["error_type"] == "DatabaseError"
+ assert "database session error" in data["error"].lower()