aminghadersohi opened a new pull request, #43598:
URL: https://github.com/apache/superset/pull/43598
## Why
`get_chart_data` accepts filters through `extra_form_data`. When a filter
names a column
that does not exist on the dataset, the datasource drops that predicate and
runs the query
unfiltered. A guard was added to catch this and return a `ValidationError`,
but it reads
`rejected_filter_columns` off the `ChartDataCommand` result, and that key
never reaches the
tool: `_materialize_full_payload` deletes it and emits `rejected_filters`
entries
(`{"reason": ..., "column": ...}`) in its place before the payload is
returned.
The guard therefore intersected the requested columns against an empty set
and never fired,
so the original failure mode was still live: a filter on an unknown column
returns **every**
row with a success response. On an agent-facing API this is worse than an
error, because the
caller has no signal that the filter was dropped and will present unfiltered
data as filtered.
The existing regression test did not catch this because it mocked
`ChartDataCommand.run()`
with the pre-materialization shape, so it exercised a payload production
never produces.
## What
Read the rejected columns from `rejected_filters`, which is the shape every
consumer of a
chart-data payload sees, and keep `rejected_filter_columns` as a fallback
for payloads
captured before that conversion. Only entries carrying a string `column` are
considered, so
time-extra rejections do not get attributed to caller-supplied filter
columns.
## Blast radius
Limited to the `get_chart_data` MCP tool. No change to query construction,
execution, or the
chart-data REST API: this only reads a field that was already present on the
payload. Filters
supplied by the request are the only ones validated, so a stale filter saved
on an older chart
config still cannot fail the call.
## How to test
`tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py`
- `test_rejected_requested_filter_columns_reads_materialized_payload` —
fails on master.
- `test_rejected_requested_filter_columns_ignores_rejected_time_filters` —
pins that a
rejected time extra is not reported as a rejected request filter.
-
`TestSavedChartExtraFormDataFilters::test_unknown_adhoc_filter_column_returns_validation_error`
now drives the tool with the materialized payload shape; it fails on
master, where it
previously passed against the mocked shape.
Verified by reverting the source change with the tests in place: the two
cases above fail,
the remaining 105 pass.
## Risk & rollback
Low. The behavior change is that a request naming an unknown filter column
now returns a
`ValidationError` instead of unfiltered rows — the intended behavior, and
the reason the
guard exists. A caller relying on the silent-unfiltered response would see
an error instead;
that response was incorrect. Straight revert if needed.
## Review guidance
Start with `_rejected_columns_in_query` in `get_chart_data.py`, then the
updated `_Command.run()`
mock in the test file — the mock shape is the crux of why this went
unnoticed.
--
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]
