fix(mcp): apply extra_form_data filters when rendering chart SQL - #43338
fix(mcp): apply extra_form_data filters when rendering chart SQL#43338aminghadersohi wants to merge 9 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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43338 +/- ##
==========================================
- Coverage 66.81% 66.06% -0.76%
==========================================
Files 2876 2877 +1
Lines 164237 170549 +6312
Branches 37917 39265 +1348
==========================================
+ Hits 109743 112667 +2924
- Misses 52310 55479 +3169
- Partials 2184 2403 +219
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:
|
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes get_chart_sql so request-supplied extra_form_data (dashboard-style filters) is preserved by the request schema and merged into query construction, matching get_chart_data behavior.
Changes:
- Add
extra_form_datatoGetChartSqlRequest(Pydantic schema) so the field is validated/stored instead of silently dropped. - Forward/merge
extra_form_datathrough both SQL build paths: savedquery_contextandform_datafallback. - Add unit tests covering request schema acceptance + propagation/merging into rendered SQL (and add missing coverage for
get_chart_data’s already-correct path).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unit_tests/mcp_service/chart/tool/test_get_chart_sql.py | Adds regression/unit tests verifying extra_form_data is accepted and applied in both saved-query_context and form_data SQL paths. |
| tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py | Adds regression tests validating get_chart_data applies extra_form_data when called with a saved chart identifier. |
| superset/mcp_service/chart/tool/get_chart_sql.py | Wires extra_form_data into query context building and merges it into saved query_context before SQL rendering. |
| superset/mcp_service/chart/schemas.py | Extends GetChartSqlRequest with extra_form_data to prevent silent dropping of filters. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Adds coverage for the form_data_key-only path, closing a patch-coverage gap flagged on the extra_form_data fix.
✅ 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}
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).
Code Review Agent Run #ad9faeActionable 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 |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard’s agent here:
I found three correctness issues and one smaller error-handling concern at d03fafa:
1. Temporal filters are not exposed to Jinja rendering.
get_chart_sql merges extra_form_data into the query but never calls set_query_context_form_data before ChartDataCommand; the fallback path has the same omission. Because Jinja’s get_time_filter() reads request-local form data, a TEMPORAL_RANGE is unavailable while rendering virtual-dataset SQL. With remove_filter=True, it also cannot be marked consumed. Could we mirror the bridge used by get_chart_data on both SQL paths?
2. relative_start and relative_end are silently dropped.
These are standard extra_form_data overrides used when calculating relative time bounds. Normalization places them in form_data["extras"], but neither the saved-query merge nor the fresh-query builder copies those extras into the query object. Valid requests can therefore render SQL using the configured default anchor rather than the requested one. Could we merge normalized extras into every saved and newly built query?
3. Unsupported nested filter values can escape the structured error contract.
extra_form_data remains an unconstrained dictionary. A filter containing all required keys but an unsupported operator reaches ChartDataQueryContextSchema.load, which raises marshmallow.ValidationError; neither handler catches it. Malformed client input can therefore still become a tool-execution failure instead of ChartError(error_type="ValidationError"). Could we validate the nested filter shapes and translate schema-validation failures at the normalization/load boundary?
4. Small suggestion: narrow the KeyError translation.
The top-level handler currently reports every KeyError from the complete request path as malformed extra_form_data, including failures originating elsewhere. Scoping this conversion to filter normalization would preserve accurate error reporting.
For regression coverage, could we exercise actual query objects or rendered SQL across saved, fallback, and unsaved paths—covering get_time_filter(remove_filter=True), relative-time anchors, unsupported operators, and invalid nested container types? The current tests largely stop at raw dictionaries or mocked SQL, so they do not prove these behaviors.
|
Addressed all feedback from the latest review in
Validation: staged-file pre-commit passed mypy, ruff-format, ruff, and all other available hooks. The pylint hook could not run because |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
superset/mcp_service/chart/tool/get_chart_sql.py:210
_sql_from_saved_query_contextreturns aChartErroronChartDataQueryContextSchema.load()ValidationError, which prevents the caller from falling back to the (more resilient) form_data-based SQL builder. This can turn a stale/invalid savedquery_contextinto a hard failure even though the fallback path could still produce SQL (and applyextra_form_data). Consider treating schema validation failures the same as other parse failures here by returningNoneso_handle_chart_sql_requestcan fall back.
try:
query_context = ChartDataQueryContextSchema().load(qc_json)
except MarshmallowValidationError as ex:
return ChartError(error=str(ex), error_type="ValidationError")
|
Addressed Copilot’s follow-up suggestion in the latest commit: saved |
Code Review Agent Run #0e2ddfActionable 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 |
richardfogaca
left a comment
richardfogaca
left a comment
There was a problem hiding this comment.
Richard's agent here:
I rechecked the PR diff at 6a081ed after the latest master merge. The provider diff is unchanged from the reviewed version, and the earlier Jinja form-data bridge, relative-time propagation, nested validation, and broad KeyError issues are addressed. I don't see a blocking correctness issue, so I'm approving.
Non-blocking suggestions:
- Small suggestion: could we narrow the exception translation in
_sql_from_form_dataso unrelated datasource-resolution or query-construction failures are not reported as invalidextra_form_data? - Small suggestion: could we update the PR summary's “No behavior change for
get_chart_data” statement? The shared helper change now propagates relative-time extras there as well.
The remaining coverage gap is behavioral: the current tests mostly assert mocked query dictionaries or bridge invocation rather than rendered SQL for Jinja get_time_filter(remove_filter=True) and relative-time anchors. That would be worth adding, but I don't think it needs to block this merge.
|
Addressed the approver's final suggestions:
All staged-file pre-commit hooks pass, including mypy, ruff-format, ruff, and pylint. |
Code Review Agent Run #875cd6Actionable 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 |
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