fix(mcp): fix form_data null, dataset URL, ASCII preview, and chart rename#39109
Conversation
…ename Four bugs discovered during eval suite run: 1. get_chart_info returns form_data=null for saved charts — now parses chart.params and includes form_data in the serialized response. 2. get_dataset_info returns relative URL (/tablemodelview/edit/1) — now uses get_superset_base_url() to construct absolute URLs, matching the pattern used by chart and dashboard serializers. 3. ASCII preview fails with "Columns missing in dataset" when chart has no groupby/x_axis columns — now returns a clear error message instead of crashing. 4. update_chart requires config parameter even for name-only updates — config is now optional, allowing chart_name to be updated alone. Extracted _find_chart and _build_update_payload helpers to keep complexity under the C901 threshold.
Code Review Agent Run #90acf4Actionable 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39109 +/- ##
==========================================
- Coverage 64.52% 64.51% -0.01%
==========================================
Files 2536 2536
Lines 131208 131231 +23
Branches 30457 30463 +6
==========================================
+ Hits 84661 84664 +3
- Misses 45084 45104 +20
Partials 1463 1463
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes several MCP eval-suite regressions in Superset’s MCP service by improving serialized metadata quality and hardening tool behavior around chart preview and chart updates.
Changes:
- Populate
ChartInfo.form_datafrom saved chartparamsJSON soget_chart_inforeturns usable configuration. - Make dataset edit URLs absolute in
get_dataset_infoby prefixing withget_superset_base_url(). - Prevent ASCII preview crashes when form_data has neither columns nor metrics by returning an
UnsupportedCharterror. - Allow
update_chartname-only updates by makingUpdateChartRequest.configoptional and extracting helper functions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
superset/mcp_service/dataset/schemas.py |
Builds absolute dataset edit URLs for MCP dataset info responses. |
superset/mcp_service/chart/tool/update_chart.py |
Refactors lookup/payload building and enables rename-only updates. |
superset/mcp_service/chart/tool/get_chart_preview.py |
Adds guardrails to avoid ASCII preview crashes on unsupported configs. |
superset/mcp_service/chart/schemas.py |
Populates chart form_data from params and makes update config optional. |
Comments suppressed due to low confidence (1)
superset/mcp_service/chart/tool/update_chart.py:176
- This "chart not found" response sets
errorto a string, butGenerateChartResponse.errorisChartGenerationError | Noneand other error paths in this tool return a structured{error_type, message, details}object. This will either fail Pydantic validation or produce an inconsistent API shape for clients. Return a structuredChartGenerationErrorhere too.
if not chart:
return GenerateChartResponse.model_validate(
{
"chart": None,
"error": f"No chart found with identifier: {request.identifier}",
"success": False,
"schema_version": "2.0",
"api_version": "v1",
}
| class UpdateChartRequest(QueryCacheControl): | ||
| identifier: int | str = Field(..., description="Chart ID or UUID") | ||
| config: ChartConfig | ||
| config: ChartConfig | None = Field( | ||
| None, | ||
| description=( | ||
| "Chart configuration. Required for visualization changes. " | ||
| "Omit to only update the chart name." | ||
| ), | ||
| ) |
There was a problem hiding this comment.
UpdateChartRequest.config is now optional to allow name-only updates, but there are existing unit tests for update_chart and none cover the new rename-only request shape (e.g. {identifier, chart_name} without config) or the error case when both config and chart_name are omitted. Add/adjust unit tests to lock in this new behavior and prevent regressions.
There was a problem hiding this comment.
Tests added in commit 05bfee1 and cde34cd:
TestBuildUpdatePayload.test_name_only_update— name-only rename returns dict payloadTestBuildUpdatePayload.test_no_config_no_name_error— neither config nor name returns ValidationErrorTestBuildUpdatePayload.test_config_update_uses_request_chart_name— config + name uses provided nameTestBuildUpdatePayload.test_config_update_keeps_existing_name— config without name keeps existingTestUpdateChartNameOnly.test_name_only_update_success— integration test via MCP ClientTestUpdateChartNameOnly.test_no_config_no_name_returns_error— integration error test
…tests Convert all plain string error values in update_chart to proper ChartGenerationError dicts (error_type/message/details) matching the GenerateChartResponse schema. Add unit tests for _find_chart, _build_update_payload helpers, and name-only update flow.
Code Review Agent Run #1c2396Actionable 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
Four bugs discovered during MCP eval suite testing:
1. get_chart_info returns form_data=null (sc-103137)
serialize_chart_object()never populated theform_datafield from the chart'sparamsJSON. Now parseschart.paramsand includes it in the response so LLMs can understand chart configuration.2. get_dataset_info returns relative URL (sc-103138)
Dataset URLs were relative (
/tablemodelview/edit/1) instead of absolute. Now usesget_superset_base_url()to construct absolute URLs, matching the pattern used by chart and dashboard serializers.3. ASCII preview crashes with "Columns missing" (sc-103139)
When a chart's form_data has no
groupby,x_axis, orall_columnsfields, the ASCII preview passed an empty columns list toQueryContextFactory, causing a crash. Now returns a clearUnsupportedCharterror.4. update_chart requires config for rename (sc-103140)
UpdateChartRequest.configwas required even for name-only updates. Now optional — users can rename a chart with just{identifier: 123, chart_name: "New Name"}. Extracted_find_chart()and_build_update_payload()helpers to keep complexity under C901 threshold.Testing
superset/mcp_service/