aminghadersohi commented on code in PR #43478:
URL: https://github.com/apache/superset/pull/43478#discussion_r3855580020
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:
##########
@@ -1177,3 +1418,99 @@ async def test_dataset_not_accessible(self, mock_find,
mock_validate, mcp_server
data = result.structured_content.get("result",
result.structured_content)
assert data["error_type"] == "DatasetNotAccessible"
assert "Access denied" in data["error"]
+
+ @patch.object(_get_chart_sql_mod, "_sql_from_form_data")
+ @patch.object(_get_chart_sql_mod, "_get_cached_form_data")
+ @pytest.mark.asyncio
+ async def test_unsaved_chart_extra_form_data_reaches_sql_builder(
+ self, mock_cached, mock_form_data_sql, mcp_server
+ ):
+ """Regression test: extra_form_data must reach the SQL builder on the
+ form_data_key-only (unsaved chart) path too, not just the saved-chart
+ paths."""
+ from fastmcp import Client
+
+ from superset.utils import json as _json
+
+ cached_form_data = {"datasource_id": 1, "datasource_type": "table"}
+ mock_cached.return_value = _json.dumps(cached_form_data)
+ mock_form_data_sql.return_value = ChartSql(
+ chart_id=0,
+ chart_name=None,
+ sql="SELECT * FROM sales WHERE country = 'USA'",
+ language="sql",
+ datasource_name="sales",
+ )
+
+ extra_form_data = {"filters": [{"col": "country", "op": "==", "val":
"USA"}]}
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_sql",
+ {
+ "request": {
+ "form_data_key": "cached-key",
+ "extra_form_data": extra_form_data,
+ }
+ },
+ )
+
+ data = result.structured_content.get("result",
result.structured_content)
+ assert "WHERE country = 'USA'" in data["sql"]
+
+ mock_form_data_sql.assert_called_once_with(
+ cached_form_data, chart=None, extra_form_data=extra_form_data
+ )
+
+ @patch.object(_get_chart_sql_mod, "validate_chart_dataset")
+ @patch.object(_get_chart_sql_mod, "_find_chart_by_identifier")
+ @pytest.mark.asyncio
+ async def test_malformed_extra_form_data_filter_returns_clean_error(
+ self, mock_find, mock_validate, mcp_server
+ ):
+ """A malformed extra_form_data filter (missing 'op') must return a
+ structured ChartError, not crash with an unhandled KeyError.
+
+ Regression test: merge_extra_form_data_filters_into_query normalizes
+ filters via simple_filter_to_adhoc, which raises KeyError on a filter
+ entry missing "col" or "op". That KeyError previously propagated out
+ of get_chart_sql uncaught.
+ """
+ from fastmcp import Client
+
+ from superset.mcp_service.chart.chart_utils import
DatasetValidationResult
+ from superset.utils import json as _json
+
+ mock_chart = Mock()
+ mock_chart.id = 40
+ mock_chart.slice_name = "Sales"
+ mock_chart.viz_type = "table"
+ mock_chart.query_context = _json.dumps(
+ {
+ "datasource": {"id": 1, "type": "table"},
+ "queries": [
+ {"columns": ["country"], "metrics": ["count"], "filters":
[]}
+ ],
+ }
+ )
+ mock_find.return_value = mock_chart
+
+ mock_validate.return_value = DatasetValidationResult(
+ is_valid=True, dataset_id=1, dataset_name="ds", warnings=[]
+ )
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_sql",
+ {
+ "request": {
+ "identifier": 40,
+ # missing "op" — malformed filter entry
+ "extra_form_data": {"filters": [{"col": "country"}]},
+ }
+ },
+ )
+
+ data = result.structured_content.get("result",
result.structured_content)
+ assert data["error_type"] == "ValidationError"
+ assert "Invalid chart query data" in data["error"]
Review Comment:
Confirmed and fixed. The saved-query-context test now expects `Invalid
extra_form_data filter`, matching the production path; the focused chart SQL
coverage and staged-file pre-commit checks pass.
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -168,8 +175,40 @@ def _sql_from_saved_query_context(
qc_json["result_type"] = ChartDataResultType.QUERY
qc_json["force"] = False
- query_context = ChartDataQueryContextSchema().load(qc_json)
+ if extra_form_data:
+ try:
+ for query in qc_json.get("queries", []):
+ merge_extra_form_data_filters_into_query(
+ query,
+ extra_form_data,
+ qc_json["datasource"]["id"],
+ qc_json["datasource"]["type"],
+ )
+ except (AttributeError, KeyError, TypeError) as ex:
+ return ChartError(
+ error=f"Invalid extra_form_data filter: {ex}",
+ error_type="ValidationError",
+ )
Review Comment:
Confirmed and fixed in dfedf65 (rebased in the latest branch). Datasource
resolution is separated from request-filter validation: a structurally stale
datasource returns `None` and falls back to `chart.params`, while malformed
request filters still return a `ValidationError`.
`test_stale_query_context_falls_back_instead_of_erroring` covers the fallback.
--
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]
