aminghadersohi commented on code in PR #39921:
URL: https://github.com/apache/superset/pull/39921#discussion_r3198839013
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1140,6 +1140,14 @@ def __init__(self, fd: Dict[str, Any]):
)
chart = find_chart_by_identifier(request.identifier)
+ # Eagerly refresh all attributes while the session is still
+ # active. Without this, a commit or expiry inside
+ # validate_chart_dataset() can leave the Slice object detached,
+ # causing DetachedInstanceError when strategy classes access
+ # lazy attributes later.
Review Comment:
Addressed. — agor claude on Amin's behalf
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py:
##########
@@ -595,3 +595,183 @@ async def test_ascii_art_variations(self):
"""
# These demonstrate the expected ASCII formats for different chart
types
+
+
+class TestDetachedInstanceError:
+ """Tests that DetachedInstanceError is handled gracefully.
+
+ When the SQLAlchemy session expires mid-request (e.g. after a commit
+ inside validate_chart_dataset), accessing lazy attributes of a Slice
+ object raises DetachedInstanceError. The tool must:
+ 1. Call db.session.refresh() immediately after loading the chart so the
+ object stays bound to the session before any downstream operations.
+ 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."""
+ import importlib
+ from contextlib import nullcontext
+ from unittest.mock import MagicMock, patch
+
+ get_chart_preview_module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+
+ 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)
+
+ with (
+ patch.object(
+ get_chart_preview_module,
+ "find_chart_by_identifier",
+ return_value=mock_chart,
+ ),
+ patch.object(
+ get_chart_preview_module.db,
+ "session",
+ **{"refresh.side_effect": _fake_refresh},
+ ),
+ patch.object(
+ get_chart_preview_module,
+ "validate_chart_dataset",
+ return_value=MagicMock(is_valid=True, warnings=[]),
+ ),
+ patch.object(
+ get_chart_preview_module.event_logger,
+ "log_context",
+ return_value=nullcontext(),
+ ),
+ # Short-circuit preview generation so the test stays fast
+ patch.object(
+ get_chart_preview_module.PreviewFormatGenerator,
+ "generate",
+ return_value=MagicMock(
+ __class__=get_chart_preview_module.URLPreview,
+ spec=get_chart_preview_module.URLPreview,
+ type="url",
+ preview_url="http://localhost/explore/?slice_id=42";,
+ width=800,
+ height=600,
+ supports_interaction=True,
+ ),
Review Comment:
Addressed. — agor claude on Amin's behalf
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1371,6 +1379,21 @@ def __init__(self, form_data: Dict[str, Any]):
return _sanitize_chart_preview_for_llm_context(result)
+ except SQLAlchemyError as e:
+ # Catch DetachedInstanceError and other SQLAlchemy errors that can
+ # surface when the ORM session expires mid-request (e.g. after a
+ # commit inside validate_chart_dataset).
+ await ctx.error(
+ "Chart preview failed due to database session error: "
+ "identifier=%s, error_type=%s, error=%s"
+ % (request.identifier, type(e).__name__, str(e))
+ )
+ logger.error("SQLAlchemy error in get_chart_preview: %s", e)
Review Comment:
Addressed. — agor claude on Amin's behalf
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
