diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 2a781228a553..92df7ce81c58 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -868,7 +868,7 @@ def check_chart_data_access(chart: Any) -> "DatasetValidationResult": """ from superset.mcp_service.chart.chart_utils import validate_chart_dataset - return validate_chart_dataset(chart, check_access=True) + return validate_chart_dataset(chart.datasource_id, check_access=True) def _log_user_resolution_failure(exc: ValueError | PermissionError) -> None: diff --git a/superset/mcp_service/chart/chart_utils.py b/superset/mcp_service/chart/chart_utils.py index ea8079ea1e9c..3138949bbc6e 100644 --- a/superset/mcp_service/chart/chart_utils.py +++ b/superset/mcp_service/chart/chart_utils.py @@ -68,7 +68,7 @@ class DatasetValidationResult: def validate_chart_dataset( - chart: Any, + datasource_id: int | None, check_access: bool = True, ) -> DatasetValidationResult: """ @@ -77,8 +77,12 @@ def validate_chart_dataset( This shared utility should be called by MCP tools after creating or retrieving charts to detect issues like missing or deleted datasets early. + Takes the datasource id rather than the chart so that callers holding an ORM + instance read it while that instance is attached; reading it here can raise + ``DetachedInstanceError`` when a concurrent request has torn down the session. + Args: - chart: A chart-like object with datasource_id, datasource_type attributes + datasource_id: The chart's ``datasource_id``, or None if it has none check_access: Whether to also check user permissions (default True) Returns: @@ -90,7 +94,6 @@ def validate_chart_dataset( from superset.mcp_service.auth import has_dataset_access warnings: list[str] = [] - datasource_id = getattr(chart, "datasource_id", None) # Check if chart has a datasource reference if datasource_id is None: @@ -1506,11 +1509,9 @@ def get_table_chart_type_label(viz_type: str | None) -> str | None: return TABLE_VIZ_TYPE_LABELS.get(viz_type) if viz_type is not None else None -def analyze_chart_capabilities(chart: Any | None, config: Any) -> ChartCapabilities: +def analyze_chart_capabilities(viz_type: str | None, config: Any) -> ChartCapabilities: """Analyze chart capabilities based on type and configuration.""" - if chart: - viz_type = getattr(chart, "viz_type", "unknown") - else: + if not viz_type: viz_type = _resolve_viz_type(config) # Determine interaction capabilities based on chart type @@ -1556,11 +1557,9 @@ def analyze_chart_capabilities(chart: Any | None, config: Any) -> ChartCapabilit ) -def analyze_chart_semantics(chart: Any | None, config: Any) -> ChartSemantics: +def analyze_chart_semantics(viz_type: str | None, config: Any) -> ChartSemantics: """Generate semantic understanding of the chart.""" - if chart: - viz_type = getattr(chart, "viz_type", "unknown") - else: + if not viz_type: viz_type = _resolve_viz_type(config) # Generate primary insight based on chart type diff --git a/superset/mcp_service/chart/tool/generate_chart.py b/superset/mcp_service/chart/tool/generate_chart.py index e84c925b1a73..a000b4e78dff 100644 --- a/superset/mcp_service/chart/tool/generate_chart.py +++ b/superset/mcp_service/chart/tool/generate_chart.py @@ -317,6 +317,9 @@ async def generate_chart( # noqa: C901 chart = None chart_id = None + chart_slice_name = None + chart_viz_type = None + chart_uuid = None explore_url = None form_data_key = None response_warnings: list[str] = form_data.pop("_mcp_warnings", []) @@ -424,7 +427,6 @@ async def generate_chart( # noqa: C901 ) chart = command.run() - chart_id = chart.id # Ensure chart was created successfully before committing if not chart or not chart.id: @@ -432,6 +434,19 @@ async def generate_chart( # noqa: C901 "Chart creation failed - no chart ID returned" ) + # Snapshot the scalar fields now, while the instance is + # known to be attached. The chart is already committed at + # this point, and every read further down happens after an + # await: under concurrency another in-flight request can + # tear down the shared session in between, which detaches + # this instance and turns any attribute access into a + # DetachedInstanceError. + chart_id = chart.id + chart_slice_name = chart.slice_name + chart_viz_type = chart.viz_type + chart_uuid = str(chart.uuid) if chart.uuid else None + chart_datasource_id = chart.datasource_id + # Reload server-generated timestamps (created_on, # changed_on) so the serializer sees real values. from superset import db @@ -442,20 +457,22 @@ async def generate_chart( # noqa: C901 logger.warning( "Chart %s created but refresh failed; " "continuing with current values", - chart.id, + chart_id, exc_info=True, ) await ctx.info( "Chart created successfully: chart_id=%s, chart_name=%s" % ( - chart.id, - chart.slice_name, + chart_id, + chart_slice_name, ) ) # Post-creation validation: verify the chart's dataset is accessible - dataset_check = validate_chart_dataset(chart, check_access=True) + dataset_check = validate_chart_dataset( + chart_datasource_id, check_access=True + ) if not dataset_check.is_valid: # Dataset validation failed - warn but don't fail the operation await ctx.warning( @@ -464,7 +481,7 @@ async def generate_chart( # noqa: C901 ) logger.warning( "Chart %s created but dataset validation failed: %s", - chart.id, + chart_id, dataset_check.error, ) if dataset_check.error: @@ -482,7 +499,7 @@ async def generate_chart( # noqa: C901 # Query failed — delete the broken chart and return an error logger.warning( "Compile check failed for chart %s: %s", - chart.id, + chart_id, compile_result.error, ) await ctx.warning( @@ -537,7 +554,7 @@ async def generate_chart( # noqa: C901 await ctx.error("Chart creation failed: error=%s" % (str(e),)) raise # Update explore URL to use saved chart - explore_url = f"{get_superset_base_url()}/explore/?slice_id={chart.id}" + explore_url = f"{get_superset_base_url()}/explore/?slice_id={chart_id}" # Generate form_data_key for saved charts (needed for chatbot rendering) try: @@ -561,7 +578,7 @@ async def generate_chart( # noqa: C901 cmd_params = CommandParameters( datasource_type=DatasourceType.TABLE, datasource_id=dataset.id, - chart_id=chart.id, + chart_id=chart_id, tab_id=None, form_data=json.dumps(form_data_with_datasource), ) @@ -666,8 +683,8 @@ async def generate_chart( # noqa: C901 response_warnings.extend(compile_result.warnings) # Generate semantic analysis - capabilities = analyze_chart_capabilities(chart, config) - semantics = analyze_chart_semantics(chart, config) + capabilities = analyze_chart_capabilities(chart_viz_type, config) + semantics = analyze_chart_semantics(chart_viz_type, config) # Create performance metadata execution_time = int((time.time() - start_time) * 1000) @@ -678,11 +695,7 @@ async def generate_chart( # noqa: C901 ) # Create accessibility metadata - chart_name = ( - chart.slice_name - if chart and hasattr(chart, "slice_name") - else generate_chart_name(config) - ) + chart_name = chart_slice_name or generate_chart_name(config) accessibility = AccessibilityMetadata( color_blind_safe=True, # Would need actual analysis alt_text=f"Chart showing {chart_name}", @@ -775,7 +788,7 @@ async def generate_chart( # noqa: C901 # Build chart info using serialize_chart_object for saved charts chart_info = None chart_data = None - if request.save_chart and chart: + if request.save_chart and chart_id: from sqlalchemy.orm import joinedload from superset import db @@ -793,7 +806,7 @@ async def generate_chart( # noqa: C901 try: chart = ( ChartDAO.find_by_id( - chart.id, + chart_id, query_options=[ joinedload(Slice.editors), joinedload(Slice.tags), @@ -804,7 +817,7 @@ async def generate_chart( # noqa: C901 except SQLAlchemyError: logger.warning( "Re-fetch of chart %s failed; returning minimal response", - chart.id, + chart_id, exc_info=True, ) try: @@ -815,11 +828,11 @@ async def generate_chart( # noqa: C901 exc_info=True, ) chart_data = { - "id": chart.id, - "slice_name": chart.slice_name, - "viz_type": chart.viz_type, + "id": chart_id, + "slice_name": chart_slice_name, + "viz_type": chart_viz_type, "url": explore_url, - "uuid": str(chart.uuid) if chart.uuid else None, + "uuid": chart_uuid, } if chart_data is None: @@ -849,14 +862,10 @@ async def generate_chart( # noqa: C901 "form_data": _sanitize_generate_chart_form_data_for_llm_context(form_data), "form_data_key": form_data_key, "api_endpoints": { - "data": f"{get_superset_base_url()}/api/v1/chart/{chart.id}/data/" - if chart - else None, - "export": f"{get_superset_base_url()}/api/v1/chart/{chart.id}/export/" - if chart - else None, + "data": f"{get_superset_base_url()}/api/v1/chart/{chart_id}/data/", + "export": f"{get_superset_base_url()}/api/v1/chart/{chart_id}/export/", } - if chart + if chart_id else {}, "performance": performance.model_dump() if performance else None, "accessibility": accessibility.model_dump() if accessibility else None, @@ -870,7 +879,7 @@ async def generate_chart( # noqa: C901 await ctx.info( "Chart generation completed successfully: chart_id=%s, execution_time_ms=%s" % ( - chart.id if chart else None, + chart_id, int((time.time() - start_time) * 1000), ) ) diff --git a/superset/mcp_service/chart/tool/get_chart_data.py b/superset/mcp_service/chart/tool/get_chart_data.py index 75cc3a6404e2..0ed596d0c87b 100644 --- a/superset/mcp_service/chart/tool/get_chart_data.py +++ b/superset/mcp_service/chart/tool/get_chart_data.py @@ -416,7 +416,9 @@ async def get_chart_data( # noqa: C901 # Skip the dataset RBAC pre-check for guests (see guest_scope.is_guest_read). if not guest_scope.is_guest_read(): - validation_result = validate_chart_dataset(chart, check_access=True) + validation_result = validate_chart_dataset( + chart.datasource_id, check_access=True + ) if not validation_result.is_valid: await ctx.warning( "Chart found but dataset is not accessible: %s" diff --git a/superset/mcp_service/chart/tool/get_chart_info.py b/superset/mcp_service/chart/tool/get_chart_info.py index 7e09984424f2..65d4c03a4f83 100644 --- a/superset/mcp_service/chart/tool/get_chart_info.py +++ b/superset/mcp_service/chart/tool/get_chart_info.py @@ -116,7 +116,7 @@ async def _validate_chart_dataset_access( chart = ChartDAO.find_by_id(result.id) if not chart: return None - validation_result = validate_chart_dataset(chart, check_access=True) + validation_result = validate_chart_dataset(chart.datasource_id, check_access=True) if not validation_result.is_valid: await ctx.warning( "Chart found but dataset is not accessible: %s" % (validation_result.error,) diff --git a/superset/mcp_service/chart/tool/get_chart_preview.py b/superset/mcp_service/chart/tool/get_chart_preview.py index 7f3b5f4ea4e6..f7e338adc9f6 100644 --- a/superset/mcp_service/chart/tool/get_chart_preview.py +++ b/superset/mcp_service/chart/tool/get_chart_preview.py @@ -1262,7 +1262,9 @@ def __init__(self, form_data: Dict[str, Any]): from superset.mcp_service import guest_scope if getattr(chart, "id", None) is not None and not guest_scope.is_guest_read(): - validation_result = validate_chart_dataset(chart, check_access=True) + validation_result = validate_chart_dataset( + chart.datasource_id, check_access=True + ) if not validation_result.is_valid: await ctx.warning( "Chart found but dataset is not accessible: %s" diff --git a/superset/mcp_service/chart/tool/get_chart_sql.py b/superset/mcp_service/chart/tool/get_chart_sql.py index c53af04d7458..c5f255c74a64 100644 --- a/superset/mcp_service/chart/tool/get_chart_sql.py +++ b/superset/mcp_service/chart/tool/get_chart_sql.py @@ -421,7 +421,7 @@ async def _handle_chart_sql_request( ) # Validate the chart's dataset is accessible - validation_result = validate_chart_dataset(chart, check_access=True) + validation_result = validate_chart_dataset(chart.datasource_id, check_access=True) if not validation_result.is_valid: await ctx.warning( "Chart found but dataset is not accessible: %s" % (validation_result.error,) diff --git a/superset/mcp_service/chart/tool/update_chart.py b/superset/mcp_service/chart/tool/update_chart.py index b18e8e10f8bc..c5070719d99e 100644 --- a/superset/mcp_service/chart/tool/update_chart.py +++ b/superset/mcp_service/chart/tool/update_chart.py @@ -579,8 +579,9 @@ async def update_chart( # noqa: C901 ) chart_for_analysis = updated_chart if saved else chart - capabilities = analyze_chart_capabilities(chart_for_analysis, parsed_config) - semantics = analyze_chart_semantics(chart_for_analysis, parsed_config) + viz_type_for_analysis = getattr(chart_for_analysis, "viz_type", None) + capabilities = analyze_chart_capabilities(viz_type_for_analysis, parsed_config) + semantics = analyze_chart_semantics(viz_type_for_analysis, parsed_config) execution_time = int((time.time() - start_time) * 1000) performance = PerformanceMetadata( diff --git a/tests/unit_tests/mcp_service/chart/test_chart_utils.py b/tests/unit_tests/mcp_service/chart/test_chart_utils.py index 256ee7032c43..b91132961434 100644 --- a/tests/unit_tests/mcp_service/chart/test_chart_utils.py +++ b/tests/unit_tests/mcp_service/chart/test_chart_utils.py @@ -1945,9 +1945,8 @@ class TestValidateChartDataset: def test_validate_chart_dataset_no_datasource_id( self, mock_find: MagicMock, mock_access: MagicMock ) -> None: - """Chart with no datasource_id returns invalid result.""" - chart = MagicMock(spec=[]) # no datasource_id attribute - result = validate_chart_dataset(chart) + """A chart with no datasource_id returns invalid result.""" + result = validate_chart_dataset(None) assert not result.is_valid assert result.dataset_id is None assert "no dataset reference" in (result.error or "").lower() @@ -1959,9 +1958,7 @@ def test_validate_chart_dataset_deleted_dataset( self, mock_find: MagicMock, mock_access: MagicMock ) -> None: """Chart whose dataset was deleted returns invalid result.""" - chart = MagicMock() - chart.datasource_id = 42 - result = validate_chart_dataset(chart) + result = validate_chart_dataset(42) assert not result.is_valid assert result.dataset_id == 42 assert "deleted" in (result.error or "").lower() @@ -1976,9 +1973,7 @@ def test_validate_chart_dataset_valid( dataset.table_name = "my_table" dataset.sql = None mock_find.return_value = dataset - chart = MagicMock() - chart.datasource_id = 7 - result = validate_chart_dataset(chart) + result = validate_chart_dataset(7) assert result.is_valid assert result.dataset_id == 7 assert result.dataset_name == "my_table" @@ -1994,9 +1989,7 @@ def test_validate_chart_dataset_virtual_warns( dataset.table_name = "virt_ds" dataset.sql = "SELECT 1" mock_find.return_value = dataset - chart = MagicMock() - chart.datasource_id = 10 - result = validate_chart_dataset(chart) + result = validate_chart_dataset(10) assert result.is_valid assert len(result.warnings) == 1 assert "virtual" in result.warnings[0].lower() @@ -2010,9 +2003,7 @@ def test_validate_chart_dataset_sqlalchemy_error( from sqlalchemy.exc import SQLAlchemyError mock_find.side_effect = SQLAlchemyError("connection lost") - chart = MagicMock() - chart.datasource_id = 99 - result = validate_chart_dataset(chart) + result = validate_chart_dataset(99) assert not result.is_valid assert result.dataset_id == 99 assert "error" in (result.error or "").lower() 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 52e59d62597f..bc23ae9b5a52 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,11 @@ Unit tests for MCP generate_chart tool """ +from typing import Any from unittest.mock import AsyncMock, MagicMock, Mock, patch import pytest +from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm.exc import DetachedInstanceError from superset.mcp_service.chart.schemas import ( @@ -438,6 +440,154 @@ def _make_mock_chart(chart_id: int = 42) -> Mock: return chart +class _DetachableSlice: + """A Slice stand-in that starts attached and can be detached at will. + + Once detached, every attribute read raises ``DetachedInstanceError``, which + is what SQLAlchemy does when a concurrent request tears down the session + this instance was loaded in. + """ + + def __init__(self, chart_id: int = 42) -> None: + self._values = { + "id": chart_id, + "slice_name": "Concurrent chart", + "viz_type": "table", + "uuid": "2a0e0e0e-0000-4000-8000-000000000042", + "datasource_id": 1, + } + self._detached = False + + def detach(self) -> None: + self._detached = True + + def __getattr__(self, name: str) -> Any: + if self._detached: + raise DetachedInstanceError( + f"Instance is not bound to a Session; " + f"attribute refresh operation cannot proceed ({name})" + ) + try: + return self._values[name] + except KeyError as ex: + raise AttributeError(name) from ex + + +async def _generate_saved_chart( + refetch: Any, +) -> tuple[Any, _DetachableSlice]: + """Run generate_chart(save_chart=True) with a chart that detaches on commit. + + ``refetch`` is used as the ``ChartDAO.find_by_id`` behaviour of the + serialization path. + """ + request = GenerateChartRequest( + dataset_id="1", + config=TableChartConfig(chart_type="table", columns=[ColumnRef(name="region")]), + save_chart=True, + generate_preview=False, + ) + ctx = MagicMock() + ctx.info = AsyncMock() + ctx.debug = AsyncMock() + ctx.warning = AsyncMock() + ctx.error = AsyncMock() + ctx.report_progress = AsyncMock() + + chart = _DetachableSlice() + dataset = Mock( + id=1, datasource_name="test_table", table_name="test_table", sql=None + ) + validation_result = Mock(is_valid=True, request=request, warnings={}, error=None) + session = MagicMock() + # The instance is detached right after the commit, before any of the reads + # that build the response. + session.refresh.side_effect = lambda _chart: chart.detach() + + with ( + patch( + "superset.mcp_service.auth.get_user_from_request", + return_value=Mock(id=1, username="admin", roles=[], groups=[]), + ), + patch( + "superset.mcp_service.chart.validation.ValidationPipeline." + "validate_request_with_warnings", + return_value=validation_result, + ), + patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset), + patch( + "superset.mcp_service.chart.tool.generate_chart.has_dataset_access", + return_value=True, + ), + # validate_chart_dataset is deliberately not mocked: it runs for real + # against the detached instance, which is where it used to raise. + patch("superset.mcp_service.auth.has_dataset_access", return_value=True), + patch( + "superset.commands.chart.create.CreateChartCommand", + return_value=Mock(run=Mock(return_value=chart)), + ), + patch("superset.db.session", session), + patch( + "superset.mcp_service.chart.tool.generate_chart._compile_chart", + return_value=CompileResult(success=True, warnings=[]), + ), + patch("superset.daos.chart.ChartDAO", Mock(find_by_id=refetch)), + patch( + "superset.mcp_service.commands.create_form_data.MCPCreateFormDataCommand", + return_value=Mock(run=Mock(return_value="form-data-key")), + ), + patch( + "superset.mcp_service.chart.tool.generate_chart.get_superset_base_url", + return_value="http://localhost:8088", + ), + ): + result = await generate_chart(request, ctx=ctx) + + return result, chart + + +class TestGenerateChartDetachedInstance: + """The committed chart must be reported even if its instance is detached. + + Regression tests for https://github.com/apache/superset/issues/42567: under + concurrency the chart was written to the database and the tool still + returned an error, because the response was built by reading attributes off + an instance another request had detached. + """ + + @pytest.mark.asyncio + async def test_detached_chart_is_reported_as_created(self) -> None: + """A detached instance no longer turns a committed chart into an error.""" + refetched = _make_mock_chart() + + result, chart = await _generate_saved_chart( + refetch=Mock(return_value=refetched) + ) + + assert chart._detached is True + assert result.success is True + assert result.error is None + assert result.chart is not None + assert result.chart.id == 42 + assert result.explore_url == "http://localhost:8088/explore/?slice_id=42" + assert result.api_endpoints["data"].endswith("/api/v1/chart/42/data/") + + @pytest.mark.asyncio + async def test_detached_chart_falls_back_to_captured_scalars(self) -> None: + """The minimal fallback response never reads the detached instance.""" + result, chart = await _generate_saved_chart( + refetch=Mock(side_effect=SQLAlchemyError("session is gone")) + ) + + assert chart._detached is True + assert result.success is True + assert result.error is None + assert result.chart is not None + assert result.chart.id == 42 + assert result.chart.slice_name == "Concurrent chart" + assert result.chart.viz_type == "table" + + class TestChartSerializationEagerLoading: """Tests for eager loading fix in generate_chart serialization path."""