aminghadersohi commented on code in PR #43338:
URL: https://github.com/apache/superset/pull/43338#discussion_r3816155554
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -1360,6 +1360,161 @@ async def
test_json_format_also_eager_loads_metrics(self, mcp_server, mock_auth)
assert _extract_metrics_load_path(query_options[0]) == ["table",
"metrics"]
+class TestSavedChartExtraFormDataFilters:
+ """Regression tests: extra_form_data filters passed alongside a saved
+ chart identifier must reach the executed query, not just the cached
+ form_data / unsaved-chart path already covered elsewhere.
+
+ A chart with a saved query_context is the common case (any chart that
+ has been opened and saved through Explore), so this is the primary path
+ exercised when a caller passes extra_form_data with a chart identifier.
+ """
+
+ def _chart(self) -> SimpleNamespace:
+ from superset.utils import json as utils_json
+
+ return SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ query_context=utils_json.dumps(
+ {
+ "datasource": {"id": 1, "type": "table"},
+ "queries": [
+ {
+ "columns": ["country"],
+ "metrics": ["count"],
+ "filters": [],
+ "row_limit": 100,
+ }
+ ],
+ "result_format": "json",
+ "result_type": "full",
+ }
+ ),
+ params=None,
+ )
+
+ async def _run(self, extra_form_data: dict[str, Any], mcp_server: Any) ->
Any:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+
+ captured: dict[str, Any] = {}
+
+ def fake_load(self: Any, data: dict[str, Any]) -> Any:
+ captured["loaded_query_context_json"] = data
+ return SimpleNamespace(queries=[SimpleNamespace(filter=[])])
+
+ 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,
+ }
+ ]
+ }
Review Comment:
Fair point on scope, but I'll push back on doing a full
real-schema/real-execution rewrite here. These tests intentionally stub
`ChartDataQueryContextSchema.load` and `ChartDataCommand` to isolate exactly
one thing: that `extra_form_data` is merged into the raw query dict *before*
it's handed to schema loading. That's precisely the layer where the original
bug report (a filter silently never reaching query construction) would have
lived, and it's the same stubbing pattern already used elsewhere in this file
(e.g. `TestBuildQueryContextFromFormData`,
`test_success_via_saved_query_context`) and in `test_get_chart_data.py`.
Going further — real schema load + real `QueryObject` + real query execution
— would mean standing up a real datasource/dataset fixture, which re-tests
Superset's own core query-construction pipeline (already covered extensively by
its own test suite under `tests/unit_tests/common/` and
`tests/integration_tests/charts/`) rather than this MCP-specific merge logic. I
don't think that's warranted for this PR's scope. Happy to revisit if a
maintainer wants deeper integration coverage as a follow-up.
--
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]
