geido commented on code in PR #44004:
URL: https://github.com/apache/superset/pull/44004#discussion_r4006131985
##########
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:
Added in 220201c03b —
`test_guest_authorization_with_slice_already_pinned_by_the_factory`.
Traced it: `QueryContextFactory.create()` pins `slice_` from
`form_data.slice_id` (`query_context_factory.py:69-70`), and
`ChartDataQueryContextSchema.load()` goes through that factory via
`make_query_context`, so a saved chart normally arrives with `slice_` set and
`authorize_query`'s assignment is skipped (`guest_scope.py:61`). The new
variant covers that branch: it asserts the pre-pinned `slice_` is left alone,
while the re-fetched chart is still what pins `form_data["slice_id"]` — which
is the read that raises on a detached instance, so the re-fetch matters on this
path too. Both guest tests now fail without it.
It also calls the real `query_context_modified` instead of patching it out.
I assert it returns a bool rather than a specific verdict — the point is that
reaching into `id` / `query_context` / `params_dict` doesn't raise, and the
True/False outcome depends on payload-comparison details that would make the
test brittle without testing anything this PR changes.
##########
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:
Correct — those three read values straight off the `SimpleNamespace` the
test had just constructed, so they could never fail. Removed. `stored_chart is
attached` plus the `form_data["slice_id"]` wiring are the assertions that can
actually break, and the new variant exercises the guard for real.
##########
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:
Removed both shadowing import blocks.
--
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]
