msyavuz commented on code in PR #44004:
URL: https://github.com/apache/superset/pull/44004#discussion_r4004842431
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -2897,3 +2897,228 @@ def test_xlsxwriter_preserves_nonfinite_group_rows() ->
None:
assert [row[0] for row in list(workbook.active.values)[1:]] == [
row["team"] for row in rows
]
+
+
+class _DetachAfterLookupChart:
+ """Slice stand-in that starts attached and detaches on demand.
+
+ After ``detach()`` every attribute read raises ``DetachedInstanceError``,
+ which is what a real Slice does once the session has committed (expiring
+ its attributes) and then been torn down.
+ """
+
+ _COLUMNS = {
+ "id": 9,
+ "slice_name": "Sales",
+ "viz_type": "table",
+ "datasource_id": 1,
+ "datasource_type": "table",
+ "params": None,
+ "query_context": (
+ '{"datasource": {"id": 1, "type": "table"},'
+ ' "queries": [{"columns": ["country"], "metrics": ["count"],'
+ ' "filters": [], "row_limit": 100}],'
+ ' "result_format": "json", "result_type": "full"}'
+ ),
+ }
+
+ def __init__(self) -> None:
+ object.__setattr__(self, "_detached", False)
+
+ def detach(self) -> None:
+ object.__setattr__(self, "_detached", True)
+
+ def __getattr__(self, name: str) -> Any:
+ from sqlalchemy.orm.exc import DetachedInstanceError
+
+ if object.__getattribute__(self, "_detached"):
+ raise DetachedInstanceError(
+ "Instance <Slice at 0x0> is not bound to a Session; "
+ f"attribute refresh operation cannot proceed (attribute:
{name})"
+ )
+ try:
+ return self._COLUMNS[name]
+ except KeyError:
+ raise AttributeError(name) from None
+
+
[email protected]("export_format", ["json", "csv", "excel"])
[email protected]
+async def test_chart_data_survives_chart_detached_after_lookup(
+ export_format: str, mcp_server: Any, mock_auth: Any
+) -> None:
+ """The tool must still return data when the Slice detaches after lookup.
+
+ Reproduces the reported failure: the session commits and is torn down
+ partway through the request, so every later read on the chart instance
+ raises DetachedInstanceError and the broad SQLAlchemyError handler returns
+ an internal-session error instead of chart data. The chart is detached at
+ the end of the lookup block, right after its last legitimate ORM use.
+ """
+ from unittest.mock import patch
Review Comment:
nit: `patch` and `Client` are already imported at module level (lines
26/29), same at 3038-3040.
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -2897,3 +2897,228 @@ def test_xlsxwriter_preserves_nonfinite_group_rows() ->
None:
assert [row[0] for row in list(workbook.active.values)[1:]] == [
row["team"] for row in rows
]
+
+
+class _DetachAfterLookupChart:
+ """Slice stand-in that starts attached and detaches on demand.
+
+ After ``detach()`` every attribute read raises ``DetachedInstanceError``,
+ which is what a real Slice does once the session has committed (expiring
+ its attributes) and then been torn down.
+ """
+
+ _COLUMNS = {
+ "id": 9,
+ "slice_name": "Sales",
+ "viz_type": "table",
+ "datasource_id": 1,
+ "datasource_type": "table",
+ "params": None,
+ "query_context": (
+ '{"datasource": {"id": 1, "type": "table"},'
+ ' "queries": [{"columns": ["country"], "metrics": ["count"],'
+ ' "filters": [], "row_limit": 100}],'
+ ' "result_format": "json", "result_type": "full"}'
+ ),
+ }
+
+ def __init__(self) -> None:
+ object.__setattr__(self, "_detached", False)
+
+ def detach(self) -> None:
+ object.__setattr__(self, "_detached", True)
+
+ def __getattr__(self, name: str) -> Any:
+ from sqlalchemy.orm.exc import DetachedInstanceError
+
+ if object.__getattribute__(self, "_detached"):
+ raise DetachedInstanceError(
+ "Instance <Slice at 0x0> is not bound to a Session; "
+ f"attribute refresh operation cannot proceed (attribute:
{name})"
+ )
+ try:
+ return self._COLUMNS[name]
+ except KeyError:
+ raise AttributeError(name) from None
+
+
[email protected]("export_format", ["json", "csv", "excel"])
[email protected]
+async def test_chart_data_survives_chart_detached_after_lookup(
+ export_format: str, mcp_server: Any, mock_auth: Any
+) -> None:
+ """The tool must still return data when the Slice detaches after lookup.
+
+ Reproduces the reported failure: the session commits and is torn down
+ partway through the request, so every later read on the chart instance
+ raises DetachedInstanceError and the broad SQLAlchemyError handler returns
+ an internal-session error instead of chart data. The chart is detached at
+ the end of the lookup block, right after its last legitimate ORM use.
+ """
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module =
importlib.import_module("superset.mcp_service.chart.tool.get_chart_data")
+
+ chart = _DetachAfterLookupChart()
+
+ def _detach_at_end_of_lookup(instance: Any) -> None:
+ instance.detach()
+ return None
+
+ def fake_load(self: Any, data: dict[str, Any]) -> Any:
+ queries = [
+ SimpleNamespace(
+ filter=query.get("filters", []),
+ time_range=query.get("time_range"),
+ to_dict=lambda query=query: dict(query),
+ )
+ for query in data.get("queries", [])
+ ]
+ return SimpleNamespace(queries=queries,
form_data=data.get("form_data", {}))
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {
+ "queries": [
+ {
+ "data": [{"country": "USA"}],
+ "colnames": ["country"],
+ "rowcount": 1,
+ }
+ ]
+ }
+
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=chart),
+ patch.object(
+ module,
+ "validate_chart_dataset",
+ return_value=SimpleNamespace(is_valid=True, warnings=[],
error=None),
+ ),
+ patch.object(
+ module.guest_scope, "guest_dashboard_id", _detach_at_end_of_lookup
+ ),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand",
_Command
+ ),
+ patch("superset.charts.schemas.ChartDataQueryContextSchema.load",
fake_load),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data",
+ {"request": {"identifier": 9, "format": export_format}},
+ )
+
+ data = json.loads(result.content[0].text)
+ assert "error_type" not in data, (
+ f"format={export_format}: chart detached after lookup produced "
+ f"{data.get('error_type')}: {data.get('error')}"
+ )
+ assert data["chart_id"] == 9
+ assert data["chart_name"] == "Sales"
+
+
[email protected]
+async def test_guest_authorization_reads_an_attached_chart_after_detachment(
+ mcp_server: Any, mock_auth: Any
+) -> None:
+ """The guest tamper guard must be handed an attached Slice.
+
+ guest_scope.authorize_query pins query_context.slice_ for
+ security_manager.query_context_modified, which reads id, query_context and
+ params_dict off that instance. The lookup's log context has committed by
+ then, so reusing the looked-up Slice fails once it is detached -- and the
+ snapshotted scalars cannot stand in, because the guard has to compare the
+ guest payload against the stored chart itself.
+ """
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module =
importlib.import_module("superset.mcp_service.chart.tool.get_chart_data")
+
+ detached = _DetachAfterLookupChart()
+ # What a re-fetch returns: a live instance the guard can read.
+ attached = SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ params=None,
+ params_dict={},
+ query_context=_DetachAfterLookupChart._COLUMNS["query_context"],
+ )
+
+ def _detach_at_end_of_lookup(instance: Any) -> int:
+ instance.detach()
+ return 6
+
+ captured: dict[str, Any] = {}
+
+ def fake_load(self: Any, data: dict[str, Any]) -> Any:
+ query_context = SimpleNamespace(
+ queries=[
+ SimpleNamespace(
+ filter=q.get("filters", []),
+ time_range=q.get("time_range"),
+ to_dict=lambda q=q: dict(q),
+ )
+ for q in data.get("queries", [])
+ ],
+ form_data={},
+ slice_=None,
+ )
+ captured["query_context"] = query_context
+ return query_context
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {"queries": [{"data": [{"a": 1}], "colnames": ["a"],
"rowcount": 1}]}
+
+ with (
+ patch.object(
+ module,
+ "find_chart_by_identifier",
+ side_effect=[detached, attached],
+ ),
+ patch.object(
+ module,
+ "validate_chart_dataset",
+ return_value=SimpleNamespace(is_valid=True, warnings=[],
error=None),
+ ),
+ patch.object(module.guest_scope, "is_guest_read", return_value=True),
+ patch.object(
+ module.guest_scope, "guest_dashboard_id", _detach_at_end_of_lookup
+ ),
+ # Real guest_scope.authorize_query -- it is the code under test here.
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand",
_Command
+ ),
+ patch("superset.charts.schemas.ChartDataQueryContextSchema.load",
fake_load),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data", {"request": {"identifier": 9}}
+ )
+
+ data = json.loads(result.content[0].text)
+ assert "error_type" not in data, (
+ f"guest request failed after detachment: "
+ f"{data.get('error_type')}: {data.get('error')}"
+ )
+
+ stored_chart = captured["query_context"].slice_
+ assert stored_chart is attached, (
+ "authorize_query must pin the re-fetched chart, not the detached one"
+ )
+ # The three attributes query_context_modified() reads must be readable.
+ assert stored_chart.id == 9
+ assert stored_chart.query_context is not None
+ assert stored_chart.params_dict == {}
Review Comment:
These assert on the `SimpleNamespace` built above, so they can't fail.
`stored_chart is attached` is the real check.
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -2897,3 +2897,228 @@ def test_xlsxwriter_preserves_nonfinite_group_rows() ->
None:
assert [row[0] for row in list(workbook.active.values)[1:]] == [
row["team"] for row in rows
]
+
+
+class _DetachAfterLookupChart:
+ """Slice stand-in that starts attached and detaches on demand.
+
+ After ``detach()`` every attribute read raises ``DetachedInstanceError``,
+ which is what a real Slice does once the session has committed (expiring
+ its attributes) and then been torn down.
+ """
+
+ _COLUMNS = {
+ "id": 9,
+ "slice_name": "Sales",
+ "viz_type": "table",
+ "datasource_id": 1,
+ "datasource_type": "table",
+ "params": None,
+ "query_context": (
+ '{"datasource": {"id": 1, "type": "table"},'
+ ' "queries": [{"columns": ["country"], "metrics": ["count"],'
+ ' "filters": [], "row_limit": 100}],'
+ ' "result_format": "json", "result_type": "full"}'
+ ),
+ }
+
+ def __init__(self) -> None:
+ object.__setattr__(self, "_detached", False)
+
+ def detach(self) -> None:
+ object.__setattr__(self, "_detached", True)
+
+ def __getattr__(self, name: str) -> Any:
+ from sqlalchemy.orm.exc import DetachedInstanceError
+
+ if object.__getattribute__(self, "_detached"):
+ raise DetachedInstanceError(
+ "Instance <Slice at 0x0> is not bound to a Session; "
+ f"attribute refresh operation cannot proceed (attribute:
{name})"
+ )
+ try:
+ return self._COLUMNS[name]
+ except KeyError:
+ raise AttributeError(name) from None
+
+
[email protected]("export_format", ["json", "csv", "excel"])
[email protected]
+async def test_chart_data_survives_chart_detached_after_lookup(
+ export_format: str, mcp_server: Any, mock_auth: Any
+) -> None:
+ """The tool must still return data when the Slice detaches after lookup.
+
+ Reproduces the reported failure: the session commits and is torn down
+ partway through the request, so every later read on the chart instance
+ raises DetachedInstanceError and the broad SQLAlchemyError handler returns
+ an internal-session error instead of chart data. The chart is detached at
+ the end of the lookup block, right after its last legitimate ORM use.
+ """
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module =
importlib.import_module("superset.mcp_service.chart.tool.get_chart_data")
+
+ chart = _DetachAfterLookupChart()
+
+ def _detach_at_end_of_lookup(instance: Any) -> None:
+ instance.detach()
+ return None
+
+ def fake_load(self: Any, data: dict[str, Any]) -> Any:
+ queries = [
+ SimpleNamespace(
+ filter=query.get("filters", []),
+ time_range=query.get("time_range"),
+ to_dict=lambda query=query: dict(query),
+ )
+ for query in data.get("queries", [])
+ ]
+ return SimpleNamespace(queries=queries,
form_data=data.get("form_data", {}))
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {
+ "queries": [
+ {
+ "data": [{"country": "USA"}],
+ "colnames": ["country"],
+ "rowcount": 1,
+ }
+ ]
+ }
+
+ with (
+ patch.object(module, "find_chart_by_identifier", return_value=chart),
+ patch.object(
+ module,
+ "validate_chart_dataset",
+ return_value=SimpleNamespace(is_valid=True, warnings=[],
error=None),
+ ),
+ patch.object(
+ module.guest_scope, "guest_dashboard_id", _detach_at_end_of_lookup
+ ),
+ patch(
+ "superset.commands.chart.data.get_data_command.ChartDataCommand",
_Command
+ ),
+ patch("superset.charts.schemas.ChartDataQueryContextSchema.load",
fake_load),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data",
+ {"request": {"identifier": 9, "format": export_format}},
+ )
+
+ data = json.loads(result.content[0].text)
+ assert "error_type" not in data, (
+ f"format={export_format}: chart detached after lookup produced "
+ f"{data.get('error_type')}: {data.get('error')}"
+ )
+ assert data["chart_id"] == 9
+ assert data["chart_name"] == "Sales"
+
+
[email protected]
+async def test_guest_authorization_reads_an_attached_chart_after_detachment(
+ mcp_server: Any, mock_auth: Any
+) -> None:
+ """The guest tamper guard must be handed an attached Slice.
+
+ guest_scope.authorize_query pins query_context.slice_ for
+ security_manager.query_context_modified, which reads id, query_context and
+ params_dict off that instance. The lookup's log context has committed by
+ then, so reusing the looked-up Slice fails once it is detached -- and the
+ snapshotted scalars cannot stand in, because the guard has to compare the
+ guest payload against the stored chart itself.
+ """
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module =
importlib.import_module("superset.mcp_service.chart.tool.get_chart_data")
+
+ detached = _DetachAfterLookupChart()
+ # What a re-fetch returns: a live instance the guard can read.
+ attached = SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ params=None,
+ params_dict={},
+ query_context=_DetachAfterLookupChart._COLUMNS["query_context"],
+ )
+
+ def _detach_at_end_of_lookup(instance: Any) -> int:
+ instance.detach()
+ return 6
+
+ captured: dict[str, Any] = {}
+
+ def fake_load(self: Any, data: dict[str, Any]) -> Any:
+ query_context = SimpleNamespace(
+ queries=[
+ SimpleNamespace(
+ filter=q.get("filters", []),
+ time_range=q.get("time_range"),
+ to_dict=lambda q=q: dict(q),
+ )
+ for q in data.get("queries", [])
+ ],
+ form_data={},
+ slice_=None,
Review Comment:
With `slice_=None` hard-coded and `ChartDataCommand` patched out, this never
covers the common case where the factory already pinned `slice_` from
`form_data.slice_id`, and never runs `query_context_modified` itself. Worth a
variant for that?
--
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]
