fix(mcp): surface rejected filter columns in get_chart_data - #43598
Conversation
get_chart_data validated request filters by reading rejected_filter_columns off the ChartDataCommand result. That key never survives to the tool: the chart-data payload is materialized before it is returned, which deletes rejected_filter_columns and emits rejected_filters entries in its place. The check therefore always intersected against an empty set, and a filter naming a column that does not exist on the dataset was silently dropped, returning unfiltered rows as a successful response. Read rejected_filters, keeping the raw key as a fallback, and fix the regression test that mocked the pre-materialization shape and so passed against the broken check.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43598 +/- ##
=======================================
Coverage 79.13% 79.13%
=======================================
Files 2879 2879
Lines 165745 165770 +25
Branches 38315 38321 +6
=======================================
+ Hits 131159 131180 +21
- Misses 32103 32106 +3
- Partials 2483 2484 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @aminghadersohi, good catch on closing the gap in your own guard from #43478, and the writeup made this easy to verify.
I verified the mechanism and ran everything locally on this branch, in a fresh venv on current master deps:
_materialize_full_payloadinsuperset/common/query_actions.pydoes deleterejected_filter_columnsand emitrejected_filtersentries in its place, so the old guard was intersecting against an empty set. I also verified it live rather than only through the mocked tests: with a real physical dataset and the realChartDataCommand.run(), a filter on a nonexistent column produces a payload carryingrejected_filters: [{"reason": "not_in_datasource", "column": "does_not_exist"}], norejected_filter_columnskey, and every row of the table. On this branch the helper returns["does_not_exist"]and the tool answers with the ValidationError, while the same payload under master's logic yields[]and the unfiltered rows come back as a success.- Regression validity: reverting just the source hunk with the new tests in place, exactly
test_rejected_requested_filter_columns_reads_materialized_payloadandTestSavedChartExtraFormDataFilters::test_unknown_adhoc_filter_column_returns_validation_errorfail and the remaining 105 pass, matching the PR description. - Full
tests/unit_tests/mcp_servicesuite on this branch: 3655 passed. - Error shape: the guard returns a
ChartErrormodel, so the rejection reaches the client as structured content witherror_type: "ValidationError"instead of a raisedToolError, consistent with the other guidance errors in this tool. The message only echoes back column names the caller itself supplied via therequested & rejectedintersection, so nothing new is disclosed.
Just a small nit on the description, not the code: time-extra rejections are not excluded by the string check, since get_time_filter_status rejections do carry string columns like __time_range. It is the intersection with the caller's requested columns that keeps them out, and the new test pins the right behavior either way.
LGTM.
|
Thanks @gabotorresruiz — you're right, and thanks for verifying it against a real dataset rather than just the mocks. Corrected the description: the string check only guards against malformed entries. |
Code Review Agent Run #c2c368Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
Why
get_chart_dataaccepts filters throughextra_form_data. When a filter names a columnthat 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 readsrejected_filter_columnsoff theChartDataCommandresult, and that key never reaches thetool:
_materialize_full_payloaddeletes it and emitsrejected_filtersentries(
{"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 achart-data payload sees, and keep
rejected_filter_columnsas a fallback for payloadscaptured before that conversion. Only entries carrying a string
columnare considered, whichguards against malformed entries rather than against time extras:
get_time_filter_statusrejections do carry a string
columnsuch as__time_range. What keeps those from beingattributed to the caller is the intersection with the columns the request actually named, and
test_rejected_requested_filter_columns_ignores_rejected_time_filterspins that behavior.Blast radius
Limited to the
get_chart_dataMCP tool. No change to query construction, execution, or thechart-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.pytest_rejected_requested_filter_columns_reads_materialized_payload— fails on master.test_rejected_requested_filter_columns_ignores_rejected_time_filters— pins that arejected time extra is not reported as a rejected request filter.
TestSavedChartExtraFormDataFilters::test_unknown_adhoc_filter_column_returns_validation_errornow 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
ValidationErrorinstead of unfiltered rows — the intended behavior, and the reason theguard 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_queryinget_chart_data.py, then the updated_Command.run()mock in the test file — the mock shape is the crux of why this went unnoticed.