You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by di...@apache.org on 2026/09/08 11:06:19 UTC
(superset) branch fix/mcp-get-chart-data-detached-instance created (now aa1e9406781)
This is an automated email from the ASF dual-hosted git repository.
geido pushed a change to branch fix/mcp-get-chart-data-detached-instance
in repository https://gitbox.apache.org/repos/asf/superset.git
at aa1e9406781 fix(mcp): refresh chart after lookup in get_chart_data
This branch includes the following new commits:
new aa1e9406781 fix(mcp): refresh chart after lookup in get_chart_data
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 01/01: fix(mcp): refresh chart after lookup in get_chart_data
Posted by di...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
geido pushed a commit to branch fix/mcp-get-chart-data-detached-instance
in repository https://gitbox.apache.org/repos/asf/superset.git
commit aa1e9406781e2d8e88fb3db9c18dda6ad09517ae
Author: Diego Pucci <di...@gmail.com>
AuthorDate: Tue Sep 8 14:06:03 2026 +0300
fix(mcp): refresh chart after lookup in get_chart_data
get_chart_data fetches the Slice, then crosses many await points and
nested event_logger.log_context() blocks whose exit commits the DB
session (DBEventLogger.log -> db.session.commit). That commit expires
every loaded ORM attribute, so a later read on the chart raises
DetachedInstanceError. The tool's broad SQLAlchemyError handler swallows
it and returns an internal-session error to the caller instead of the
chart data.
Call db.session.refresh(chart) immediately after the lookup, while the
session is still live, so all column values are loaded upfront and a
later expiry has nothing left to fetch. This is the same fix already
applied to the sibling get_chart_preview tool.
Verified on SQLAlchemy 2.0.52 that refresh() reloads eagerly-loaded
relationships with their original loader strategy, so the
subqueryload(Slice.table).subqueryload(SqlaTable.metrics) eager-load
that the Excel export path depends on survives the refresh.
Existing tests that stood in a SimpleNamespace for the chart now use a
stub_session_refresh fixture, since SQLAlchemy rejects refresh() on an
unmapped object; production always gets a persistent Slice from the DAO.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
superset/mcp_service/chart/tool/get_chart_data.py | 9 +-
.../mcp_service/chart/tool/test_get_chart_data.py | 152 ++++++++++++++++++++-
2 files changed, 155 insertions(+), 6 deletions(-)
diff --git a/superset/mcp_service/chart/tool/get_chart_data.py b/superset/mcp_service/chart/tool/get_chart_data.py
index 1016ea01b9c..86b8704e08d 100644
--- a/superset/mcp_service/chart/tool/get_chart_data.py
+++ b/superset/mcp_service/chart/tool/get_chart_data.py
@@ -35,7 +35,7 @@ if TYPE_CHECKING:
from superset.charts.data.form_data import set_query_context_form_data
from superset.commands.exceptions import CommandException
from superset.exceptions import OAuth2Error, OAuth2RedirectError, SupersetException
-from superset.extensions import event_logger
+from superset.extensions import db, event_logger
from superset.mcp_service import guest_scope
from superset.mcp_service.chart.chart_helpers import (
build_query_context_from_form_data,
@@ -471,6 +471,13 @@ async def get_chart_data( # noqa: C901
request.identifier, query_options=chart_query_options
)
if chart is not None:
+ # Eagerly load every column value while the session is still
+ # active. SQLAlchemy expires object attributes on commit, and
+ # this tool crosses many await points and nested
+ # event_logger.log_context() blocks that commit on exit; a
+ # later attribute read on the expired instance would raise
+ # DetachedInstanceError. Mirrors get_chart_preview.
+ db.session.refresh(chart)
guest_dashboard_id = guest_scope.guest_dashboard_id(chart)
if not chart:
diff --git a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
index 789d53409f5..0fd6dae4fb3 100644
--- a/tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
+++ b/tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
@@ -19,6 +19,7 @@
Tests for the get_chart_data request schema and chart type fallback handling.
"""
+import contextlib
import importlib
from contextlib import nullcontext
from types import SimpleNamespace
@@ -1331,6 +1332,22 @@ def mock_auth():
yield mock_get_user
+@pytest.fixture
+def stub_session_refresh():
+ """No-op db.session.refresh() for tests using an unmapped chart double.
+
+ get_chart_data refreshes the chart right after lookup so its columns are
+ loaded before the session can expire. SQLAlchemy rejects that call on a
+ SimpleNamespace; in production the DAO always returns a persistent Slice.
+ """
+ from unittest.mock import patch
+
+ from superset.extensions import db
+
+ with patch.object(db.session, "refresh", return_value=None):
+ yield
+
+
def _extract_metrics_load_path(load_opt: Any) -> list[str]:
"""Walk a SQLAlchemy Load option and return the attr chain.
@@ -1565,7 +1582,7 @@ class TestSavedChartExtraFormDataFilters:
@pytest.mark.asyncio
async def test_filters_key_reaches_executed_query(
- self, mcp_server: Any, mock_auth: Any
+ self, mcp_server: Any, mock_auth: Any, stub_session_refresh: Any
) -> None:
"""extra_form_data using the native 'filters' format is applied."""
loaded, _ = await self._run(
@@ -1576,7 +1593,7 @@ class TestSavedChartExtraFormDataFilters:
@pytest.mark.asyncio
async def test_adhoc_filters_key_reaches_executed_query(
- self, mcp_server: Any, mock_auth: Any
+ self, mcp_server: Any, mock_auth: Any, stub_session_refresh: Any
) -> None:
"""extra_form_data using the 'adhoc_filters' format is also applied."""
loaded, _ = await self._run(
@@ -1598,7 +1615,7 @@ class TestSavedChartExtraFormDataFilters:
@pytest.mark.asyncio
async def test_temporal_range_filter_reaches_executed_query(
- self, mcp_server: Any, mock_auth: Any
+ self, mcp_server: Any, mock_auth: Any, stub_session_refresh: Any
) -> None:
"""A TEMPORAL_RANGE filter narrows the query, not just simple filters."""
loaded, _ = await self._run(
@@ -1622,7 +1639,7 @@ class TestSavedChartExtraFormDataFilters:
@pytest.mark.asyncio
async def test_unknown_adhoc_filter_column_returns_validation_error(
- self, mcp_server: Any, mock_auth: Any
+ self, mcp_server: Any, mock_auth: Any, stub_session_refresh: Any
) -> None:
"""A rejected request filter must not return plausible unfiltered data."""
_, result = await self._run(
@@ -1736,6 +1753,7 @@ class TestOAuthErrorRouting:
self,
mcp_server: Any,
mock_auth: Any,
+ stub_session_refresh: Any,
monkeypatch: pytest.MonkeyPatch,
) -> None:
from fastmcp import Client
@@ -1767,6 +1785,7 @@ class TestOAuthErrorRouting:
self,
mcp_server: Any,
mock_auth: Any,
+ stub_session_refresh: Any,
monkeypatch: pytest.MonkeyPatch,
) -> None:
from fastmcp import Client
@@ -2207,7 +2226,7 @@ class TestGuestScoping:
@pytest.mark.asyncio
async def test_guest_query_pinned_to_dashboard_with_existence_only_check(
- self, mcp_server, mock_auth
+ self, mcp_server, mock_auth, stub_session_refresh
) -> None:
from unittest.mock import patch
@@ -2591,3 +2610,126 @@ async def test_query_from_form_data_refreshed_reflects_force_refresh_only(
assert isinstance(result, ChartData)
assert result.cache_status is not None
assert result.cache_status.refreshed is expected_refreshed
+
+
+class _AccessRecordingChart:
+ """Slice stand-in that appends every attribute read to a shared event log.
+
+ Lets a test assert the *order* of session.refresh() relative to the first
+ chart attribute access, which is the property that makes the fix work.
+ """
+
+ _COLUMNS = {
+ "id": 42,
+ "slice_name": "Sales Chart",
+ "viz_type": "table",
+ "datasource_id": 1,
+ "datasource_type": "table",
+ "params": "{}",
+ "query_context": None,
+ }
+
+ def __init__(self, events: list[str]) -> None:
+ object.__setattr__(self, "_events", events)
+
+ def __getattr__(self, name: str) -> Any:
+ object.__getattribute__(self, "_events").append(f"attr:{name}")
+ try:
+ return self._COLUMNS[name]
+ except KeyError:
+ raise AttributeError(name) from None
+
+
+class TestDetachedInstanceError:
+ """Regression tests for the DetachedInstanceError on the chart Slice.
+
+ get_chart_data is async and crosses many await points; the nested
+ event_logger.log_context() blocks commit the DB session on exit, which
+ expires every loaded ORM attribute. Later attribute access on the chart
+ then raises DetachedInstanceError, which the broad SQLAlchemyError handler
+ turns into a confusing internal-session error instead of chart data.
+
+ Calling db.session.refresh() immediately after the lookup loads all column
+ values into the instance while the session is still live, so a later
+ expiry has nothing left to lazily fetch. Mirrors the fix already applied
+ to get_chart_preview.
+ """
+
+ def _patches(self, module: Any, chart: Any, refresh: Any) -> tuple[Any, ...]:
+ from unittest.mock import patch
+
+ from superset.extensions import db
+
+ return (
+ patch.object(module, "find_chart_by_identifier", return_value=chart),
+ # Patched on the shared extension session rather than the tool
+ # module, so a missing refresh surfaces as a failed assertion
+ # rather than an AttributeError on the module's db import.
+ patch.object(db.session, "refresh", side_effect=refresh),
+ # Short-circuits the tool right after the lookup so the assertions
+ # target the lookup block rather than the query pipeline.
+ patch.object(
+ module,
+ "validate_chart_dataset",
+ return_value=SimpleNamespace(
+ is_valid=False, error="Dataset gone", warnings=[]
+ ),
+ ),
+ )
+
+ @pytest.mark.asyncio
+ async def test_session_refresh_called_after_chart_load(self, mcp_server, mock_auth):
+ """db.session.refresh() runs once on the chart returned by the lookup."""
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ chart = _AccessRecordingChart([])
+ refreshed: list[object] = []
+
+ with contextlib.ExitStack() as stack:
+ for patcher in self._patches(module, chart, refreshed.append):
+ stack.enter_context(patcher)
+ async with Client(mcp_server) as client:
+ await client.call_tool(
+ "get_chart_data", {"request": {"identifier": 42}}
+ )
+
+ assert refreshed == [chart], (
+ "db.session.refresh() must be called exactly once with the chart "
+ f"loaded by find_chart_by_identifier; got {refreshed!r}"
+ )
+
+ @pytest.mark.asyncio
+ async def test_session_refresh_precedes_chart_attribute_access(
+ self, mcp_server, mock_auth
+ ):
+ """The refresh must happen before anything reads a chart attribute.
+
+ A refresh placed after the first attribute read would leave that read
+ exposed to an expired session, which is the bug being fixed.
+ """
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ events: list[str] = []
+ chart = _AccessRecordingChart(events)
+
+ with contextlib.ExitStack() as stack:
+ for patcher in self._patches(
+ module, chart, lambda _obj: events.append("refresh")
+ ):
+ stack.enter_context(patcher)
+ async with Client(mcp_server) as client:
+ await client.call_tool(
+ "get_chart_data", {"request": {"identifier": 42}}
+ )
+
+ assert "refresh" in events, "db.session.refresh() was never called"
+ assert events[0] == "refresh", (
+ "db.session.refresh() must run before the first chart attribute "
+ f"access; event order was {events[:5]}"
+ )