fix(mcp): apply extra_form_data filters when rendering chart SQL - #43478
fix(mcp): apply extra_form_data filters when rendering chart SQL#43478aminghadersohi wants to merge 10 commits into
Conversation
get_chart_sql had no extra_form_data field on its request schema, so any filters passed alongside a chart identifier were silently dropped before validation and the tool always rendered the chart's unfiltered baseline SQL. get_chart_data already merges extra_form_data correctly; this wires the same merge helpers into get_chart_sql's saved query_context and form_data fallback paths.
Adds coverage for the form_data_key-only path, closing a patch-coverage gap flagged on the extra_form_data fix.
Address PR review feedback: an extra_form_data filter entry missing a required key (e.g. "op") raised an unhandled KeyError out of get_chart_sql instead of a structured ChartError. Also use a keyword argument for extra_form_data at the _sql_from_form_data call site for clarity, and strengthen the saved-query_context regression tests to actually exercise both the "filters" and "adhoc_filters" extra_form_data formats (previously only "filters" was tested despite the docstring claiming both).
dosubot
Bot
added
api
Code Review Agent Run #b67a05Actionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| assert data["error_type"] == "ValidationError" | ||
| assert "Invalid chart query data" in data["error"] |
There was a problem hiding this comment.
Suggestion: The assertion expects the fallback-path message, but malformed filters on the saved query_context path are caught earlier and return an error beginning with Invalid extra_form_data filter, so this test will fail even when the production error handling behaves as implemented. Assert the actual saved-context validation message or change the production path if the fallback message is intended. [logic error]
Severity Level: Major ⚠️
- ❌ The new malformed-filter regression test fails during the chart SQL test suite.
- ⚠️ CI cannot pass until the expected error message matches the saved-context path.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py
**Line:** 1515:1516
**Comment:**
*Logic Error: The assertion expects the fallback-path message, but malformed filters on the saved `query_context` path are caught earlier and return an error beginning with `Invalid extra_form_data filter`, so this test will fail even when the production error handling behaves as implemented. Assert the actual saved-context validation message or change the production path if the fallback message is intended.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. In tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py |
Filter merging on the saved query_context path needs the context's datasource id/type. Resolving those inside the same try as the merge meant a stale context (missing or malformed "datasource") was reported as "Invalid extra_form_data filter", skipping the form_data fallback so charts with valid params failed instead of rendering. Resolve the datasource up front and return None on failure so the caller rebuilds from form_data; keep the ValidationError for merge failures, which really are bad request filters. Also fixes the malformed-filter regression test, which still asserted the fallback path's "Invalid chart query data" message after the saved-context error branch was added. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — both flagged issues were accurate, confirmed by reproducing them locally. Fixed in dfedf65. 1. Test asserted the wrong error message (codeant + bito). Confirmed real; the test failed: The malformed-filter regression test was written against the fallback path's message, but a later commit in this branch added the saved-context error branch, which reports the malformed filter directly. Updated the assertion to expect 2.
Returning 3. Test gap on the invalid-
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43478 +/- ##
===========================================
+ Coverage 66.81% 78.84% +12.02%
===========================================
Files 2876 2877 +1
Lines 164237 164786 +549
Branches 37917 38001 +84
===========================================
+ Hits 109743 129919 +20176
+ Misses 52310 32416 -19894
- Partials 2184 2451 +267
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:
|
Code Review Agent Run #b18805Actionable Suggestions - 0Review 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
Recreates #43338, which GitHub auto-closed when its head fork was deleted. Same commits, no content changes.
SUMMARY
get_chart_sql's request schema had noextra_form_datafield. Pydantic silentlydrops unrecognized fields by default, so any filters passed alongside a chart
identifier were dropped before validation ever ran — the tool always rendered the
chart's unfiltered baseline SQL regardless of what was passed in.
get_chart_dataalready mergesextra_form_datacorrectly viamerge_extra_form_data_filters_into_query/build_query_context_from_form_data(added in a prior fix). This PR adds the same
extra_form_datafield toGetChartSqlRequestand wires it into both ofget_chart_sql's SQL-constructionpaths (saved
query_context, and theform_datafallback), reusing the existingmerge helpers instead of adding new filter-merging logic.
get_chart_dataalready applied filters correctly. The shared helper update in thisPR additionally preserves normalized relative-time extras (
relative_startandrelative_end) for both chart-data and chart-SQL query construction.TESTING INSTRUCTIONS
tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py:GetChartSqlRequestaccepts and storesextra_form_data(previously silentlydropped).
_build_query_context_from_form_dataforwardsextra_form_datato the queryfactory.
_sql_from_saved_query_contextmergesextra_form_datainto the query handedto
ChartDataQueryContextSchema.loadbefore rendering SQL.request.extra_form_datareaches the saved-query_context SQLbuilder and shows up in the rendered SQL.
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.pycover the (already-correct, previously untested)
get_chart_datapath where achart identifier +
extra_form_dataare both provided — both thefiltersandadhoc_filtersextra_form_data formats, plus aTEMPORAL_RANGEfilter.pytest tests/unit_tests/mcp_service/— full suite passes.pre-commit runon all changed files — clean.ADDITIONAL INFORMATION