aminghadersohi commented on code in PR #43338:
URL: https://github.com/apache/superset/pull/43338#discussion_r3816153782
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:
##########
@@ -995,6 +1055,127 @@ def test_resolves_combined_datasource_field(self,
mock_get_ds):
assert result == "combined_dataset"
+class TestSqlFromSavedQueryContextExtraFormData:
+ """Regression tests: extra_form_data must reach the query built from a
+ chart's saved query_context, not just the request-supplied form_data."""
+
+ def test_real_column_filter_merged_before_schema_load(self):
+ """A filter on a real column ends up in the query handed to
+ ChartDataQueryContextSchema.load, for both the 'filters' and
+ 'adhoc_filters' extra_form_data formats."""
Review Comment:
Fixed — added a second test case
(`test_real_column_filter_via_adhoc_filters_key`) exercising the
`adhoc_filters` format, alongside the existing `filters`-format one. Both now
actually run, matching the docstring.
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -249,11 +262,14 @@ def _resolve_datasource_name(
def _sql_from_form_data(
form_data: dict[str, Any],
chart: "Slice | None",
+ extra_form_data: dict[str, Any] | None = None,
) -> ChartSql | ChartError:
"""Build SQL from form_data (fallback path)."""
from superset.commands.chart.data.get_data_command import ChartDataCommand
- query_context = _build_query_context_from_form_data(form_data, chart)
+ query_context = _build_query_context_from_form_data(
+ form_data, chart, extra_form_data
Review Comment:
Fixed — switched to `extra_form_data=extra_form_data`.
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -451,7 +471,9 @@ async def _handle_chart_sql_request(
# Fallback: build query context from form_data
try:
- return _sql_from_form_data(effective_form_data, chart)
+ return _sql_from_form_data(
+ effective_form_data, chart, request.extra_form_data
+ )
Review Comment:
Confirmed and fixed. Reproduced: a filter entry missing `op` (e.g. `{"col":
"country"}`) raised an unhandled `KeyError` from `simple_filter_to_adhoc`,
uncaught anywhere in the `get_chart_sql` call chain. Added a `KeyError` handler
in `get_chart_sql` that returns a structured
`ChartError(error_type="ValidationError")`, plus a regression test
(`test_malformed_extra_form_data_filter_returns_clean_error`).
--
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]
