You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2026/03/20 19:43:32 UTC
(superset) 31/32: fix(mcp): fix detached Slice instance error in chart/dashboard serialization (#38767)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 6.1
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 9239db5a32c1daf30a7193413a0002a0644bf96d
Author: Kamil Gabryjelski <ka...@gmail.com>
AuthorDate: Fri Mar 20 18:23:51 2026 +0100
fix(mcp): fix detached Slice instance error in chart/dashboard serialization (#38767)
Co-authored-by: Claude Opus 4.6 <no...@anthropic.com>
---
superset/mcp_service/chart/tool/generate_chart.py | 19 +++++
.../tool/add_chart_to_existing_dashboard.py | 22 +++++
.../dashboard/tool/generate_dashboard.py | 21 +++++
.../mcp_service/chart/tool/test_generate_chart.py | 96 +++++++++++++++++++++-
.../dashboard/tool/test_dashboard_generation.py | 94 ++++++++++++++++++---
5 files changed, 240 insertions(+), 12 deletions(-)
diff --git a/superset/mcp_service/chart/tool/generate_chart.py b/superset/mcp_service/chart/tool/generate_chart.py
index 3c9b0192cf8..64f9e03804f 100644
--- a/superset/mcp_service/chart/tool/generate_chart.py
+++ b/superset/mcp_service/chart/tool/generate_chart.py
@@ -707,7 +707,26 @@ async def generate_chart( # noqa: C901
# Build chart info using serialize_chart_object for saved charts
chart_info = None
if request.save_chart and chart:
+ from sqlalchemy.orm import joinedload
+
+ from superset.daos.chart import ChartDAO
from superset.mcp_service.chart.schemas import serialize_chart_object
+ from superset.models.slice import Slice
+
+ # Re-fetch with eager-loaded relationships to avoid detached
+ # instance errors when serialize_chart_object accesses .tags
+ # and .owners. Use joinedload (single JOIN query) since we
+ # are fetching a single chart.
+ chart = (
+ ChartDAO.find_by_id(
+ chart.id,
+ query_options=[
+ joinedload(Slice.owners),
+ joinedload(Slice.tags),
+ ],
+ )
+ or chart
+ )
chart_info = serialize_chart_object(chart)
if chart_info:
diff --git a/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py b/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
index 9765cace49c..93888701451 100644
--- a/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
@@ -406,6 +406,28 @@ def add_chart_to_existing_dashboard(
command = UpdateDashboardCommand(request.dashboard_id, update_data)
updated_dashboard = command.run()
+ # Re-fetch the dashboard with eager-loaded relationships to avoid
+ # "Instance is not bound to a Session" errors when serializing
+ # chart .tags and .owners.
+ from sqlalchemy.orm import subqueryload
+
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+ from superset.models.slice import Slice
+
+ updated_dashboard = (
+ DashboardDAO.find_by_id(
+ updated_dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.owners),
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.owners),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or updated_dashboard
+ )
+
# Convert to response format
from superset.mcp_service.dashboard.schemas import (
serialize_tag_object,
diff --git a/superset/mcp_service/dashboard/tool/generate_dashboard.py b/superset/mcp_service/dashboard/tool/generate_dashboard.py
index 52bdb8b501a..3cddf6d4c5e 100644
--- a/superset/mcp_service/dashboard/tool/generate_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/generate_dashboard.py
@@ -274,6 +274,27 @@ def generate_dashboard(
command = CreateDashboardCommand(dashboard_data)
dashboard = command.run()
+ # Re-fetch the dashboard with eager-loaded relationships to avoid
+ # "Instance is not bound to a Session" errors when serializing
+ # chart .tags and .owners.
+ from sqlalchemy.orm import subqueryload
+
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+
+ dashboard = (
+ DashboardDAO.find_by_id(
+ dashboard.id,
+ query_options=[
+ subqueryload(Dashboard.slices).subqueryload(Slice.owners),
+ subqueryload(Dashboard.slices).subqueryload(Slice.tags),
+ subqueryload(Dashboard.owners),
+ subqueryload(Dashboard.tags),
+ ],
+ )
+ or dashboard
+ )
+
# Convert to our response format
from superset.mcp_service.dashboard.schemas import (
serialize_tag_object,
diff --git a/tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py b/tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
index f4d834c5306..ef6ce5383f3 100644
--- a/tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
+++ b/tests/unit_tests/mcp_service/chart/tool/test_generate_chart.py
@@ -19,9 +19,10 @@
Unit tests for MCP generate_chart tool
"""
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock, Mock, patch
import pytest
+from sqlalchemy.orm.exc import DetachedInstanceError
from superset.mcp_service.chart.schemas import (
AxisConfig,
@@ -351,3 +352,96 @@ class TestCompileChart:
assert result.success is False
assert "invalid metric" in (result.error or "")
+
+
+def _make_mock_chart(chart_id: int = 42) -> Mock:
+ """Create a mock chart with all attributes needed by serialize_chart_object."""
+ chart = Mock()
+ chart.id = chart_id
+ chart.slice_name = "Test Chart"
+ chart.viz_type = "echarts_timeseries_bar"
+ chart.datasource_name = "test_table"
+ chart.datasource_type = "table"
+ chart.description = None
+ chart.cache_timeout = None
+ chart.changed_by = None
+ chart.changed_by_name = "admin"
+ chart.changed_on = None
+ chart.changed_on_humanized = "1 day ago"
+ chart.created_by = None
+ chart.created_by_name = "admin"
+ chart.created_on = None
+ chart.created_on_humanized = "2 days ago"
+ chart.uuid = "test-uuid-42"
+ chart.tags = []
+ chart.owners = []
+ return chart
+
+
+class TestChartSerializationEagerLoading:
+ """Tests for eager loading fix in generate_chart serialization path."""
+
+ def test_serialize_chart_object_succeeds_with_loaded_relationships(self):
+ """serialize_chart_object works when tags/owners are already loaded."""
+ from superset.mcp_service.chart.schemas import serialize_chart_object
+
+ chart = _make_mock_chart()
+ result = serialize_chart_object(chart)
+
+ assert result is not None
+ assert result.id == 42
+ assert result.slice_name == "Test Chart"
+ assert result.tags == []
+ assert result.owners == []
+
+ def test_serialize_chart_object_fails_on_detached_instance(self):
+ """serialize_chart_object raises when accessing lazy attrs on detached
+ instance — this is the bug scenario that the eager-loading fix prevents."""
+ from superset.mcp_service.chart.schemas import serialize_chart_object
+
+ chart = _make_mock_chart()
+ # Simulate detached instance: accessing .tags raises DetachedInstanceError
+ type(chart).tags = property(
+ lambda self: (_ for _ in ()).throw(
+ DetachedInstanceError("Instance <Slice> is not bound to a Session")
+ )
+ )
+
+ with pytest.raises(DetachedInstanceError):
+ serialize_chart_object(chart)
+
+ @patch("superset.daos.chart.ChartDAO.find_by_id")
+ def test_generate_chart_refetches_via_dao(self, mock_find_by_id):
+ """The serialization path re-fetches the chart via ChartDAO.find_by_id
+ with joinedload query_options for owners and tags."""
+ refetched_chart = _make_mock_chart()
+ refetched_chart.tags = [Mock(id=1, name="tag1", type="custom")]
+ refetched_chart.tags[0].description = ""
+
+ mock_find_by_id.return_value = refetched_chart
+
+ from superset.daos.chart import ChartDAO
+
+ chart = (
+ ChartDAO.find_by_id(42, query_options=["dummy_option"])
+ or _make_mock_chart()
+ )
+
+ assert chart is refetched_chart
+ mock_find_by_id.assert_called_once_with(42, query_options=["dummy_option"])
+
+ @patch("superset.daos.chart.ChartDAO.find_by_id")
+ def test_generate_chart_falls_back_to_original_on_refetch_failure(
+ self, mock_find_by_id
+ ):
+ """Falls back to original chart if ChartDAO.find_by_id returns None."""
+ original_chart = _make_mock_chart()
+ mock_find_by_id.return_value = None
+
+ from superset.daos.chart import ChartDAO
+
+ chart = (
+ ChartDAO.find_by_id(original_chart.id, query_options=[]) or original_chart
+ )
+
+ assert chart is original_chart
diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
index b382f1f4ab2..7d299d32ef1 100644
--- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
+++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
@@ -107,10 +107,11 @@ class TestGenerateDashboard:
"""Tests for generate_dashboard MCP tool."""
@patch("superset.commands.dashboard.create.CreateDashboardCommand")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
@patch("superset.db.session")
@pytest.mark.asyncio
async def test_generate_dashboard_basic(
- self, mock_db_session, mock_create_command, mcp_server
+ self, mock_db_session, mock_find_by_id, mock_create_command, mcp_server
):
"""Test basic dashboard generation with valid charts."""
mock_query = Mock()
@@ -125,6 +126,7 @@ class TestGenerateDashboard:
mock_dashboard = _mock_dashboard(id=10, title="Analytics Dashboard")
mock_create_command.return_value.run.return_value = mock_dashboard
+ mock_find_by_id.return_value = mock_dashboard
request = {
"chart_ids": [1, 2],
@@ -170,10 +172,11 @@ class TestGenerateDashboard:
assert result.structured_content["dashboard_url"] is None
@patch("superset.commands.dashboard.create.CreateDashboardCommand")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
@patch("superset.db.session")
@pytest.mark.asyncio
async def test_generate_dashboard_single_chart(
- self, mock_db_session, mock_create_command, mcp_server
+ self, mock_db_session, mock_find_by_id, mock_create_command, mcp_server
):
"""Test dashboard generation with a single chart."""
mock_query = Mock()
@@ -185,6 +188,7 @@ class TestGenerateDashboard:
mock_dashboard = _mock_dashboard(id=20, title="Single Chart Dashboard")
mock_create_command.return_value.run.return_value = mock_dashboard
+ mock_find_by_id.return_value = mock_dashboard
request = {
"chart_ids": [5],
@@ -200,10 +204,11 @@ class TestGenerateDashboard:
assert result.structured_content["dashboard"]["published"] is True
@patch("superset.commands.dashboard.create.CreateDashboardCommand")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
@patch("superset.db.session")
@pytest.mark.asyncio
async def test_generate_dashboard_many_charts(
- self, mock_db_session, mock_create_command, mcp_server
+ self, mock_db_session, mock_find_by_id, mock_create_command, mcp_server
):
"""Test dashboard generation with many charts (grid layout)."""
chart_ids = list(range(1, 7))
@@ -218,6 +223,7 @@ class TestGenerateDashboard:
mock_dashboard = _mock_dashboard(id=30, title="Multi Chart Dashboard")
mock_create_command.return_value.run.return_value = mock_dashboard
+ mock_find_by_id.return_value = mock_dashboard
request = {"chart_ids": chart_ids, "dashboard_title": "Multi Chart Dashboard"}
@@ -295,10 +301,11 @@ class TestGenerateDashboard:
assert result.structured_content["dashboard"] is None
@patch("superset.commands.dashboard.create.CreateDashboardCommand")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
@patch("superset.db.session")
@pytest.mark.asyncio
async def test_generate_dashboard_minimal_request(
- self, mock_db_session, mock_create_command, mcp_server
+ self, mock_db_session, mock_find_by_id, mock_create_command, mcp_server
):
"""Test dashboard generation with minimal required parameters."""
mock_query = Mock()
@@ -310,6 +317,7 @@ class TestGenerateDashboard:
mock_dashboard = _mock_dashboard(id=40, title="Minimal Dashboard")
mock_create_command.return_value.run.return_value = mock_dashboard
+ mock_find_by_id.return_value = mock_dashboard
request = {
"chart_ids": [3],
@@ -332,10 +340,11 @@ class TestGenerateDashboard:
)
@patch("superset.commands.dashboard.create.CreateDashboardCommand")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
@patch("superset.db.session")
@pytest.mark.asyncio
async def test_generate_dashboard_auto_title_from_charts(
- self, mock_db_session, mock_create_command, mcp_server
+ self, mock_db_session, mock_find_by_id, mock_create_command, mcp_server
):
"""Test that omitting dashboard_title generates a title from chart names."""
mock_query = Mock()
@@ -350,6 +359,7 @@ class TestGenerateDashboard:
mock_dashboard = _mock_dashboard(id=50, title="Sales Revenue & Customer Count")
mock_create_command.return_value.run.return_value = mock_dashboard
+ mock_find_by_id.return_value = mock_dashboard
# No dashboard_title provided
request = {"chart_ids": [1, 2]}
@@ -363,10 +373,11 @@ class TestGenerateDashboard:
assert call_args["dashboard_title"] == "Sales Revenue & Customer Count"
@patch("superset.commands.dashboard.create.CreateDashboardCommand")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
@patch("superset.db.session")
@pytest.mark.asyncio
async def test_generate_dashboard_empty_string_title_preserved(
- self, mock_db_session, mock_create_command, mcp_server
+ self, mock_db_session, mock_find_by_id, mock_create_command, mcp_server
):
"""Test that an explicit empty-string title is NOT replaced by auto-gen."""
mock_query = Mock()
@@ -380,6 +391,7 @@ class TestGenerateDashboard:
mock_dashboard = _mock_dashboard(id=60, title="")
mock_create_command.return_value.run.return_value = mock_dashboard
+ mock_find_by_id.return_value = mock_dashboard
# Explicit empty string title
request = {"chart_ids": [1], "dashboard_title": ""}
@@ -439,17 +451,19 @@ class TestAddChartToExistingDashboard:
"DASHBOARD_VERSION_KEY": "v2",
}
)
- mock_find_dashboard.return_value = mock_dashboard
-
- mock_chart = _mock_chart(id=30, slice_name="New Chart")
- mock_db_session.get.return_value = mock_chart
-
updated_dashboard = _mock_dashboard(id=1, title="Existing Dashboard")
updated_dashboard.slices = [
_mock_chart(id=10),
_mock_chart(id=20),
_mock_chart(id=30),
]
+ # First call: initial validation returns original dashboard
+ # Second call: re-fetch after update returns updated dashboard
+ mock_find_dashboard.side_effect = [mock_dashboard, updated_dashboard]
+
+ mock_chart = _mock_chart(id=30, slice_name="New Chart")
+ mock_db_session.get.return_value = mock_chart
+
mock_update_command.return_value.run.return_value = updated_dashboard
request = {"dashboard_id": 1, "chart_id": 30}
@@ -1065,3 +1079,61 @@ class TestGenerateTitleFromCharts:
title = _generate_title_from_charts(charts)
assert len(title) <= 150
assert title.endswith("\u2026")
+
+
+class TestDashboardSerializationEagerLoading:
+ """Tests for eager loading fix in dashboard serialization paths."""
+
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+ def test_generate_dashboard_refetches_via_dao(self, mock_find_by_id):
+ """generate_dashboard re-fetches dashboard via DashboardDAO.find_by_id
+ with eager-loaded slice relationships before serialization."""
+ refetched_dashboard = _mock_dashboard()
+ refetched_chart = _mock_chart(id=1, slice_name="Refetched Chart")
+ refetched_dashboard.slices = [refetched_chart]
+
+ mock_find_by_id.return_value = refetched_dashboard
+
+ from superset.daos.dashboard import DashboardDAO
+
+ result = (
+ DashboardDAO.find_by_id(1, query_options=["dummy"]) or _mock_dashboard()
+ )
+
+ assert result is refetched_dashboard
+ mock_find_by_id.assert_called_once_with(1, query_options=["dummy"])
+
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+ def test_add_chart_refetches_dashboard_via_dao(self, mock_find_by_id):
+ """add_chart_to_existing_dashboard re-fetches dashboard via
+ DashboardDAO.find_by_id with eager-loaded slice relationships."""
+ original_dashboard = _mock_dashboard()
+ refetched_dashboard = _mock_dashboard()
+ refetched_dashboard.slices = [_mock_chart()]
+
+ mock_find_by_id.return_value = refetched_dashboard
+
+ from superset.daos.dashboard import DashboardDAO
+
+ result = (
+ DashboardDAO.find_by_id(original_dashboard.id, query_options=["dummy"])
+ or original_dashboard
+ )
+
+ assert result is refetched_dashboard
+
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+ def test_add_chart_falls_back_on_refetch_failure(self, mock_find_by_id):
+ """add_chart_to_existing_dashboard falls back to original dashboard
+ if DashboardDAO.find_by_id returns None."""
+ original_dashboard = _mock_dashboard()
+ mock_find_by_id.return_value = None
+
+ from superset.daos.dashboard import DashboardDAO
+
+ result = (
+ DashboardDAO.find_by_id(original_dashboard.id, query_options=["dummy"])
+ or original_dashboard
+ )
+
+ assert result is original_dashboard