aminghadersohi opened a new pull request, #43680:
URL: https://github.com/apache/superset/pull/43680
## Why
`get_chart_data` fails closed when a filter names a column the dataset does
not have: the
datasource drops the predicate during query construction and the tool
returns a
`ValidationError` rather than unfiltered rows. `get_chart_sql` accepts the
same
`extra_form_data` filters but had no equivalent check, so it returned the
unfiltered SQL as a
success — presenting it as the SQL for filters that were never applied.
The check could not simply be copied across, because the signal was not
available: the `query`
result type discards the `applied_filter_columns` /
`rejected_filter_columns` that the
datasource has already computed while building the statement, keeping only
the SQL string. So
anyone asking for just the SQL — the MCP tool, and the "View query" modal —
had no way to tell
that a filter had been dropped.
## What
Carry the filter status through the `query` result type using the same
`applied_filters` /
`rejected_filters` entries the data-bearing results already expose, then
have `get_chart_sql`
fail closed on a rejected request filter, matching `get_chart_data`.
`_get_query` now prefers `get_query_str_extended`, which returns that status
alongside the SQL;
datasources that do not implement it (e.g. semantic layers) keep the plain
string form.
`QueryStringExtended.full_sql` holds the prequery-joining logic that
`ExploreMixin.get_query_str`
already had, so there is one definition of it rather than two.
The three filter-column helpers move from `get_chart_data` into
`chart_helpers` so both tools
share one implementation instead of a copy each.
## Blast radius
`_get_query` is the `query` result type used by the chart-data API and the
"View query" modal.
The change is additive — two extra keys on that payload — and the SQL string
itself is produced
by the same code path as before (`ExploreMixin.get_query_str` was already
`get_query_str_extended` plus a join). Behavior changes only for
`get_chart_sql`, which now
errors instead of returning unfiltered SQL. No change to query construction
or execution.
## How to test
-
`tests/unit_tests/common/test_query_actions.py::test_get_query_surfaces_rejected_filter_columns`
— the `query` payload reports the rejected column.
- `..._falls_back_when_datasource_has_no_extended_form` — datasources
without the extended form
keep the old shape.
-
`tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py::TestRejectedFilterColumnsAreSurfaced`
— rejected request filter returns `ValidationError`; a stale filter the
caller did not ask for
does not fail the request; no rejections still returns SQL.
Verified by reverting the `get_chart_sql` change with the tests in place:
the three new cases
fail, the rest pass. Suites run: `tests/unit_tests/mcp_service/`,
`tests/unit_tests/common/`,
`tests/unit_tests/charts/` — 1363 passed locally, plus the wider run clean
apart from
`test_tools_call_health_check_over_real_asgi_transport`, which fails
identically on an unmodified
master.
Three tests in `test_get_data_command.py` mocked `get_query_str` to raise a
parse error; they now
mock `get_query_str_extended`, since that is the method that raises. The
assertions are unchanged.
## Risk & rollback
Low. The new failure mode is an explicit error where the previous behavior
was a misleading
success. A caller depending on unfiltered SQL being returned for an unknown
column would see a
`ValidationError` instead; that response was incorrect. Straight revert if
needed.
## Review guidance
Read `_get_query` in `superset/common/query_actions.py` first — that is the
only change outside
the MCP service, and the `getattr` fallback is the hunk worth scrutinising.
Then the guard in
`_extract_sql_from_result`, which both `get_chart_sql` paths funnel through.
--
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]
