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 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