Copilot commented on code in PR #43338:
URL: https://github.com/apache/superset/pull/43338#discussion_r3815504572
##########
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:
This docstring claims the test covers both `'filters'` and `'adhoc_filters'`
formats, but the test body only exercises the `'filters'` format. Either update
the docstring to match what’s tested, or (preferably) parameterize/add an
assertion that runs the same path with an `'adhoc_filters'` payload so the doc
remains accurate.
##########
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:
Passing `extra_form_data` as a positional argument here makes the call-site
easier to misread and more fragile if `_build_query_context_from_form_data`’s
parameters evolve. Prefer using a keyword argument
(`extra_form_data=extra_form_data`) for clarity and safer refactoring.
--
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]
