fix(mcp): Support form_data_key without chart identifier for unsaved charts#38628
Merged
kgabryje merged 3 commits intoapache:masterfrom Mar 13, 2026
Merged
fix(mcp): Support form_data_key without chart identifier for unsaved charts#38628kgabryje merged 3 commits intoapache:masterfrom
kgabryje merged 3 commits intoapache:masterfrom
Conversation
codeant-ai-for-open-source
bot
added
the
size:L
Contributor
Sequence DiagramThis PR allows get_chart_info, get_chart_data, and get_chart_preview to work when no chart identifier exists, as long as a form data key is provided. It also supports overriding saved chart state with cached unsaved form data, while enforcing that at least one of identifier or form data key is present. sequenceDiagram
participant Client
participant MCPChartTools
participant RequestSchema
participant FormDataCache
participant ChartStore
participant ChartEngine
Client->>MCPChartTools: Request chart info data or preview
MCPChartTools->>RequestSchema: Validate identifier or form data key
alt Form data key only
MCPChartTools->>FormDataCache: Load cached form data
MCPChartTools->>ChartEngine: Build response from cached form data
ChartEngine-->>Client: Return unsaved chart info data or preview
else Identifier provided
MCPChartTools->>ChartStore: Load saved chart by id or uuid
opt Form data key also provided
MCPChartTools->>FormDataCache: Load cached form data override
end
MCPChartTools->>ChartEngine: Build response from saved or overridden state
ChartEngine-->>Client: Return chart info data or preview
end
Generated by CodeAnt AI |
dosubot
bot
added
api
aminghadersohi
approved these changes
Mar 13, 2026
Antonio-RiveroMartnez
approved these changes
Mar 13, 2026
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+143
to
+144
| # Build query context entirely from cached form_data | ||
| return await _query_from_form_data(cached_form_data_dict, request, ctx) |
Contributor
There was a problem hiding this comment.
Suggestion: When only form_data_key is provided, the function returns immediately and ignores non-JSON export requests, so callers asking for CSV/Excel get a JSON-shaped response instead of the requested format. Guard this path and return an explicit unsupported-format error (or implement conversion) to preserve API contract behavior. [logic error]
Severity Level: Major ⚠️
- ⚠️ Unsaved CSV/Excel requests return JSON-style payload unexpectedly.
- ⚠️ Clients expecting export fields can mis-handle responses.
Suggested change
| # Build query context entirely from cached form_data | |
| return await _query_from_form_data(cached_form_data_dict, request, ctx) | |
| # Build query context entirely from cached form_data | |
| if request.format != "json": | |
| return ChartError( | |
| error=( | |
| "Only json format is supported when using form_data_key " | |
| "without a saved chart identifier." | |
| ), | |
| error_type="UnsupportedFormat", | |
| ) | |
| return await _query_from_form_data(cached_form_data_dict, request, ctx) |
Steps of Reproduction ✅
1. Send MCP `get_chart_data` request with only `form_data_key` and `format=\"csv\"` or
`\"excel\"`; formats are explicitly allowed by schema in
`superset/mcp_service/chart/schemas.py:1087-1088`.
2. Request enters unsaved path in
`superset/mcp_service/chart/tool/get_chart_data.py:115-144` and returns early at line 144
before export conversion logic.
3. Export conversion branches (`if request.format == "csv"` / `"excel"`) exist only later
in saved-chart flow at `get_chart_data.py:654-669`, so they never run for this
early-return path.
4. `_query_from_form_data` returns regular `ChartData` rows (`get_chart_data.py:858+`) and
does not populate export-specific `csv_data/excel_data/format` fields expected by
`ChartData` model (`superset/mcp_service/chart/schemas.py:113-120`), causing
response-shape mismatch for non-JSON callers.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_data.py
**Line:** 143:144
**Comment:**
*Logic Error: When only `form_data_key` is provided, the function returns immediately and ignores non-JSON export requests, so callers asking for CSV/Excel get a JSON-shaped response instead of the requested format. Guard this path and return an explicit unsupported-format error (or implement conversion) to preserve API contract behavior.
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.
Comment on lines
+779
to
+787
| # Extract metrics and groupby based on chart type | ||
| if viz_type in ("big_number", "big_number_total", "pop_kpi"): | ||
| metric = form_data.get("metric") | ||
| metrics = [metric] if metric else [] | ||
| groupby: list[str] = [] | ||
| else: | ||
| metrics = form_data.get("metrics", []) | ||
| groupby = list(form_data.get("groupby") or []) | ||
|
|
Contributor
There was a problem hiding this comment.
Suggestion: The unsaved-form-data metric/groupby extraction only handles three singular-metric chart types and otherwise assumes metrics+groupby, which breaks chart types like world_map, treemap_v2, sunburst_v2, gauge_chart, and bubble that use different fields. Expand extraction logic to include these known form_data patterns so query contexts are built correctly. [logic error]
Severity Level: Major ⚠️
- ❌ Unsaved world_map/treemap/sunburst/gauge/bubble queries can fail.
- ⚠️ Unsaved results diverge from Explore chart configuration.
Suggested change
| # Extract metrics and groupby based on chart type | |
| if viz_type in ("big_number", "big_number_total", "pop_kpi"): | |
| metric = form_data.get("metric") | |
| metrics = [metric] if metric else [] | |
| groupby: list[str] = [] | |
| else: | |
| metrics = form_data.get("metrics", []) | |
| groupby = list(form_data.get("groupby") or []) | |
| # Extract metrics and groupby based on chart type | |
| singular_metric_no_groupby = ("big_number", "big_number_total", "pop_kpi") | |
| singular_metric_types = ( | |
| *singular_metric_no_groupby, | |
| "world_map", | |
| "treemap_v2", | |
| "sunburst_v2", | |
| "gauge_chart", | |
| ) | |
| if viz_type == "bubble": | |
| metrics = [m for m in (form_data.get("x"), form_data.get("y"), form_data.get("size")) if m] | |
| groupby = list(form_data.get("groupby") or []) | |
| for field in (form_data.get("entity"), form_data.get("series")): | |
| if field and field not in groupby: | |
| groupby.append(field) | |
| elif viz_type in singular_metric_types: | |
| metric = form_data.get("metric") | |
| metrics = [metric] if metric else [] | |
| if viz_type in singular_metric_no_groupby: | |
| groupby = [] | |
| else: | |
| groupby = list(form_data.get("groupby") or []) | |
| for field in (form_data.get("entity"), form_data.get("series")): | |
| if field and field not in groupby: | |
| groupby.append(field) | |
| for col in form_data.get("columns") or []: | |
| if isinstance(col, str) and col not in groupby: | |
| groupby.append(col) | |
| else: | |
| metrics = form_data.get("metrics", []) | |
| groupby = list(form_data.get("groupby") or []) | |
| if not groupby: | |
| for col in form_data.get("columns") or []: | |
| if isinstance(col, str): | |
| groupby.append(col) |
Steps of Reproduction ✅
1. Invoke `get_chart_data` with `form_data_key` only (allowed by `GetChartDataRequest`
validator at `superset/mcp_service/chart/schemas.py:1072-1075`), which routes into
`_query_from_form_data` via `get_chart_data.py:115-144`.
2. In `_query_from_form_data`, extraction logic at `get_chart_data.py:779-787` handles
only `big_number/big_number_total/pop_kpi`; all other viz types default to `metrics` +
`groupby`.
3. For unsaved `world_map`, `treemap_v2`, `sunburst_v2`, `gauge_chart`, and `bubble`,
form_data commonly uses `metric`/`entity`/`series`/`columns`/`x,y,size`; this mapping is
already recognized in the same file's fallback logic (`get_chart_data.py:349-407`) and
mirrored by dedicated tests in
`tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py` (e.g., lines 69-75, 229+,
272+, 321+, 11+ for bubble section).
4. Because helper extraction misses those patterns, `QueryContextFactory.create(...)` at
`get_chart_data.py:790-803` receives incomplete queries, causing failed/empty query
outcomes (`EmptyQuery`/`NoData`) or incorrect unsaved-chart results.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_data.py
**Line:** 779:787
**Comment:**
*Logic Error: The unsaved-form-data metric/groupby extraction only handles three singular-metric chart types and otherwise assumes `metrics`+`groupby`, which breaks chart types like `world_map`, `treemap_v2`, `sunburst_v2`, `gauge_chart`, and `bubble` that use different fields. Expand extraction logic to include these known form_data patterns so query contexts are built correctly.
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.
Comment on lines
+806
to
+808
| with event_logger.log_context(action="mcp.get_chart_data.query_execution"): | ||
| command = ChartDataCommand(query_context) | ||
| result = command.run() |
Contributor
There was a problem hiding this comment.
Suggestion: The unsaved-chart query path runs ChartDataCommand without calling validate(), which skips the standard access/validation gate used everywhere else before query execution. Add command.validate() before run() so permission and datasource checks are enforced consistently. [security]
Severity Level: Critical 🚨
- ❌ Unsaved data queries skip `raise_for_access` authorization check.
- ⚠️ Security behavior differs from other chart query paths.
Suggested change
| with event_logger.log_context(action="mcp.get_chart_data.query_execution"): | |
| command = ChartDataCommand(query_context) | |
| result = command.run() | |
| with event_logger.log_context(action="mcp.get_chart_data.query_execution"): | |
| command = ChartDataCommand(query_context) | |
| command.validate() | |
| result = command.run() |
Steps of Reproduction ✅
1. Call MCP tool `get_chart_data` (registered in `superset/mcp_service/app.py:34-38`) with
`form_data_key` only; request is accepted by `GetChartDataRequest` validator in
`superset/mcp_service/chart/schemas.py:1072-1075`.
2. Execution enters unsaved fast path in
`superset/mcp_service/chart/tool/get_chart_data.py:115-144` and immediately delegates to
`_query_from_form_data(...)`.
3. In `_query_from_form_data`, query execution at
`superset/mcp_service/chart/tool/get_chart_data.py:806-808` creates `ChartDataCommand` and
calls `run()` directly.
4. `ChartDataCommand.validate()` is the method that enforces access
(`superset/commands/chart/data/get_data_command.py:72-73` calling
`QueryContext.raise_for_access()`), while `run()` (`.../get_data_command.py:40-70`) does
not perform that check, so this path skips the standard authorization gate used by
saved-chart path (`get_chart_data.py:501-503`) and preview flows
(`get_chart_preview.py:162-164`, `237-239`, `344-346`).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_data.py
**Line:** 806:808
**Comment:**
*Security: The unsaved-chart query path runs `ChartDataCommand` without calling `validate()`, which skips the standard access/validation gate used everywhere else before query execution. Add `command.validate()` before `run()` so permission and datasource checks are enforced consistently.
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.
Comment on lines
+70
to
+84
| try: | ||
| form_data = utils_json.loads(cached_form_data) | ||
| except (TypeError, ValueError) as e: | ||
| return ChartError( | ||
| error=f"Failed to parse cached form_data: {e}", | ||
| error_type="ParseError", | ||
| ) | ||
| return ChartInfo( | ||
| viz_type=form_data.get("viz_type"), | ||
| datasource_name=form_data.get("datasource_name"), | ||
| datasource_type=form_data.get("datasource_type"), | ||
| form_data=form_data, | ||
| form_data_key=form_data_key, | ||
| is_unsaved_state=True, | ||
| ) |
Contributor
There was a problem hiding this comment.
Suggestion: utils_json.loads can return non-dict JSON (e.g., list/null), but the code immediately calls .get(...) on it. That will raise AttributeError and return a 500 instead of a structured ChartError. Add a type check after parsing and return a parse error when the payload is not a JSON object. [type error]
Severity Level: Major ⚠️
- ❌ Unsaved `get_chart_info` can fail with internal ToolError.
- ⚠️ MCP chart-inspection flow breaks for malformed cached payloads.
Suggested change
| try: | |
| form_data = utils_json.loads(cached_form_data) | |
| except (TypeError, ValueError) as e: | |
| return ChartError( | |
| error=f"Failed to parse cached form_data: {e}", | |
| error_type="ParseError", | |
| ) | |
| return ChartInfo( | |
| viz_type=form_data.get("viz_type"), | |
| datasource_name=form_data.get("datasource_name"), | |
| datasource_type=form_data.get("datasource_type"), | |
| form_data=form_data, | |
| form_data_key=form_data_key, | |
| is_unsaved_state=True, | |
| ) | |
| try: | |
| form_data = utils_json.loads(cached_form_data) | |
| except (TypeError, ValueError) as e: | |
| return ChartError( | |
| error=f"Failed to parse cached form_data: {e}", | |
| error_type="ParseError", | |
| ) | |
| if not isinstance(form_data, dict): | |
| return ChartError( | |
| error="Cached form_data is not a valid JSON object.", | |
| error_type="ParseError", | |
| ) | |
| return ChartInfo( | |
| viz_type=form_data.get("viz_type"), | |
| datasource_name=form_data.get("datasource_name"), | |
| datasource_type=form_data.get("datasource_type"), | |
| form_data=form_data, | |
| form_data_key=form_data_key, | |
| is_unsaved_state=True, | |
| ) |
Steps of Reproduction ✅
1. Create cached form data with valid-but-non-object JSON via Explore form-data API:
`ExploreFormDataRestApi.post()` in `superset/explore/form_data/api.py:50-106` accepts
`form_data` as generic string (`superset/explore/form_data/schemas.py:33-37`), so payload
like `"null"` or `"[]"` is allowed.
2. Persistence path validates syntax only: `CreateFormDataCommand.validate()`
(`superset/commands/explore/form_data/create.py:74-76`) calls `validate_json`, which only
runs `loads` (`superset/utils/json.py:50-53`) and does not enforce dict/object shape.
3. Call MCP tool `get_chart_info` with only `form_data_key` (tool registered through
`superset/mcp_service/app.py:10-14`); execution enters unsaved branch in
`superset/mcp_service/chart/tool/get_chart_info.py:159-168`, then
`_build_unsaved_chart_info()`.
4. In `_build_unsaved_chart_info` (`get_chart_info.py:70-84`), `utils_json.loads` returns
`None`/list, then `form_data.get(...)` at lines `78-80` raises `AttributeError`; this is
converted by global middleware (`superset/mcp_service/middleware.py:109-116`) into a
generic internal ToolError instead of structured `ChartError(ParseError)`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_info.py
**Line:** 70:84
**Comment:**
*Type Error: `utils_json.loads` can return non-dict JSON (e.g., list/null), but the code immediately calls `.get(...)` on it. That will raise `AttributeError` and return a 500 instead of a structured `ChartError`. Add a type check after parsing and return a parse error when the payload is not a JSON object.
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.
Comment on lines
+93
to
+99
| result.form_data = utils_json.loads(cached_form_data) | ||
| result.form_data_key = form_data_key | ||
| result.is_unsaved_state = True | ||
|
|
||
| # Update viz_type from cached form_data if present | ||
| if result.form_data and "viz_type" in result.form_data: | ||
| result.viz_type = result.form_data["viz_type"] |
Contributor
There was a problem hiding this comment.
Suggestion: The unsaved-state override only updates viz_type, leaving datasource metadata from the saved chart even when cached unsaved form data changed datasource fields. This returns inconsistent chart info (mixed saved/unsaved state). Update datasource fields from cached form data during override. [logic error]
Severity Level: Major ⚠️
- ⚠️ `get_chart_info` may return mixed saved/unsaved chart metadata.
- ⚠️ MCP agents can reason from stale datasource fields.
Suggested change
| result.form_data = utils_json.loads(cached_form_data) | |
| result.form_data_key = form_data_key | |
| result.is_unsaved_state = True | |
| # Update viz_type from cached form_data if present | |
| if result.form_data and "viz_type" in result.form_data: | |
| result.viz_type = result.form_data["viz_type"] | |
| result.form_data = utils_json.loads(cached_form_data) | |
| result.form_data_key = form_data_key | |
| result.is_unsaved_state = True | |
| if isinstance(result.form_data, dict): | |
| # Update key metadata fields to match unsaved state | |
| if "viz_type" in result.form_data: | |
| result.viz_type = result.form_data["viz_type"] | |
| if "datasource_name" in result.form_data: | |
| result.datasource_name = result.form_data["datasource_name"] | |
| if "datasource_type" in result.form_data: | |
| result.datasource_type = result.form_data["datasource_type"] |
Steps of Reproduction ✅
1. Use normal saved-chart path by calling MCP `get_chart_info` with both `identifier` and
`form_data_key`; code enters lookup flow
(`superset/mcp_service/chart/tool/get_chart_info.py:181-205`) and applies unsaved
override.
2. Base metadata comes from saved Slice serialization (`serialize_chart_object` sets
`datasource_name`/`datasource_type` from chart attributes in
`superset/mcp_service/chart/schemas.py:213-219`).
3. Override logic `_apply_unsaved_state_override()` (`get_chart_info.py:87-100`) sets
`form_data` and updates only `viz_type`, leaving datasource metadata untouched even when
cached unsaved form_data contains datasource fields.
4. Response can therefore contain mixed state: `form_data` reflects unsaved edits while
top-level datasource metadata remains saved-chart metadata, creating inconsistent tool
output for clients consuming top-level fields.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_info.py
**Line:** 93:99
**Comment:**
*Logic Error: The unsaved-state override only updates `viz_type`, leaving datasource metadata from the saved chart even when cached unsaved form data changed datasource fields. This returns inconsistent chart info (mixed saved/unsaved state). Update datasource fields from cached form data during override.
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.|
|
||
| class TransientChartFromKey: | ||
| def __init__(self, fd: Dict[str, Any]): | ||
| self.id = None |
Contributor
There was a problem hiding this comment.
Suggestion: The unsaved-chart transient object sets the identifier to None, but the response schema requires an integer chart id. This causes response construction to fail at runtime for form_data_key-only previews. Use a numeric sentinel id for transient charts so the response can be serialized. [type error]
Severity Level: Critical 🚨
- ❌ form_data_key-only preview returns InternalError instead of preview.
- ⚠️ Unsaved Explore preview workflow becomes unusable.
Suggested change
| self.id = None | |
| self.id = 0 |
Steps of Reproduction ✅
1. Call MCP tool `get_chart_preview` (entrypoint
`superset/mcp_service/chart/tool/get_chart_preview.py:2193`) with `form_data_key` only
(allowed by validator in `superset/mcp_service/chart/schemas.py:1184-1189`) and a
non-`url` format like `table`.
2. Request flows into `_get_chart_preview_internal` (`get_chart_preview.py:1815`), unsaved
branch (`1842`), and builds `TransientChartFromKey` with `self.id = None` (`1866`).
3. After preview content generation, response model is built at `ChartPreview(...)`
(`get_chart_preview.py:2123`) using `chart_id=chart.id`; schema requires `chart_id: int`
(`schemas.py:1334-1338`).
4. Pydantic validation fails on `None` chart_id, caught by outer exception handler
(`get_chart_preview.py:2160+`), returning `ChartError(InternalError)` instead of
successful preview.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_preview.py
**Line:** 1866:1866
**Comment:**
*Type Error: The unsaved-chart transient object sets the identifier to `None`, but the response schema requires an integer chart id. This causes response construction to fail at runtime for form_data_key-only previews. Use a numeric sentinel id for transient charts so the response can be serialized.
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.
Comment on lines
+2042
to
+2044
| parsed = utils_json.loads(cached_form_data) | ||
| if isinstance(parsed, dict) and "viz_type" in parsed: | ||
| chart.viz_type = parsed["viz_type"] |
Contributor
There was a problem hiding this comment.
Suggestion: The unsaved-state override updates only params and optionally viz_type, but preview queries use chart.datasource_id/chart.datasource_type. If users changed datasource in Explore, preview generation still queries the saved datasource and can return wrong data or fail. Update datasource fields from cached form_data during override. [logic error]
Severity Level: Critical 🚨
- ❌ Preview may show data from wrong datasource.
- ⚠️ Unsaved datasource edits can trigger preview query errors.
Suggested change
| parsed = utils_json.loads(cached_form_data) | |
| if isinstance(parsed, dict) and "viz_type" in parsed: | |
| chart.viz_type = parsed["viz_type"] | |
| parsed = utils_json.loads(cached_form_data) | |
| if isinstance(parsed, dict): | |
| if "viz_type" in parsed: | |
| chart.viz_type = parsed["viz_type"] | |
| datasource = parsed.get("datasource") | |
| if isinstance(datasource, str) and "__" in datasource: | |
| ds_id, ds_type = datasource.split("__", 1) | |
| if ds_id.isdigit(): | |
| chart.datasource_id = int(ds_id) | |
| chart.datasource_type = ds_type | |
| else: | |
| if parsed.get("datasource_id") is not None: | |
| chart.datasource_id = int(parsed["datasource_id"]) | |
| if parsed.get("datasource_type"): | |
| chart.datasource_type = parsed["datasource_type"] |
Steps of Reproduction ✅
1. Call `get_chart_preview` with both `identifier` and `form_data_key` (override path at
`get_chart_preview.py:2020`) after changing datasource in Explore unsaved state.
2. Override block loads cached form_data but only sets `chart.params` and maybe
`chart.viz_type` (`2039-2044`), leaving `chart.datasource_id/type` from saved chart.
3. Preview strategies build query context from `self.chart.datasource_id/type` (e.g.,
Table strategy `219-223`, ASCII strategy `149-152`) while using unsaved params.
4. Query runs against old datasource with new-form fields, producing wrong preview data or
query failures (returned as `TableError`/`ASCIIError`).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_preview.py
**Line:** 2042:2044
**Comment:**
*Logic Error: The unsaved-state override updates only `params` and optionally `viz_type`, but preview queries use `chart.datasource_id`/`chart.datasource_type`. If users changed datasource in Explore, preview generation still queries the saved datasource and can return wrong data or fail. Update datasource fields from cached form_data during override.
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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #38628 +/- ##
==========================================
- Coverage 65.01% 64.33% -0.68%
==========================================
Files 1817 2529 +712
Lines 72318 129108 +56790
Branches 23032 29747 +6715
==========================================
+ Hits 47016 83064 +36048
- Misses 25302 44598 +19296
- Partials 0 1446 +1446
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:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #734860
Actionable Suggestions - 1
-
superset/mcp_service/chart/tool/get_chart_info.py - 1
- Use of assert statement detected · Line 170-173
Additional Suggestions - 2
-
superset/mcp_service/chart/tool/get_chart_data.py - 1
-
Security: Missing dataset access check for unsaved charts · Line 115-144The new unsaved chart handling bypasses dataset access validation, unlike saved charts which call validate_chart_dataset. This could allow unauthorized data access if cache isolation fails. Add a check in _query_from_form_data to validate user access to the dataset extracted from form_data.
-
-
superset/mcp_service/chart/tool/get_chart_preview.py - 1
-
Missing type hint on __init__ · Line 1865-1865The new TransientChartFromKey class's __init__ method lacks a return type hint, which violates the project's type hinting standards for all new Python code. This ensures MyPy compliance and improves code consistency.
Code suggestion
@@ -1865,1 +1865,1 @@ - def __init__(self, fd: Dict[str, Any]): + def __init__(self, fd: Dict[str, Any]) -> None:
-
Review Details
-
Files reviewed - 4 · Commit Range:
af15e42..af15e42- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/tool/get_chart_data.py
- superset/mcp_service/chart/tool/get_chart_info.py
- superset/mcp_service/chart/tool/get_chart_preview.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Comment on lines
+170
to
+173
| "No chart identifier provided - retrieving unsaved chart from cache: " | ||
| "form_data_key=%s" % (request.form_data_key,) | ||
| ) | ||
| return _build_unsaved_chart_info(request.form_data_key) |
Contributor
There was a problem hiding this comment.
Use of assert statement detected
Replace assert statement on line 173 with proper error handling. Assertions can be disabled at runtime and should not be used for runtime validation.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| "No chart identifier provided - retrieving unsaved chart from cache: " | |
| "form_data_key=%s" % (request.form_data_key,) | |
| ) | |
| return _build_unsaved_chart_info(request.form_data_key) | |
| # At this point identifier must be set (validator ensures at least one | |
| # of identifier/form_data_key is provided, and the form_data_key-only | |
| # branch returned above). | |
| if request.identifier is None: | |
| raise ValueError("Chart identifier must be provided when form_data_key is not supplied") |
Code Review Run #734860
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Contributor
|
Bito Automatic Review Skipped – PR Already Merged |
aminghadersohi
pushed a commit
to aminghadersohi/superset
that referenced
this pull request
Mar 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
SUMMARY
MCP chart tools (get_chart_info, get_chart_data, get_chart_preview) previously required a chart
identifier (ID or UUID) even when a form_data_key was provided. This broke the workflow for unsaved/new
charts in Explore, where no chart ID exists yet but the configuration is cached via form_data_key.
This change makes the identifier field optional across all three tools when form_data_key is provided,
and adds handling to query data, retrieve info, and render previews using only the cached form_data. A
model validator ensures at least one of identifier or form_data_key is provided.
Key changes:
GetChartPreviewRequest. Added model_validator to require at least one of identifier or form_data_key.
Added form_data field to ChartInfo.
to execute against the datasource directly via _query_from_form_data.
render previews without a saved chart. Also adds form_data_key override support for saved charts.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CodeAnt-AI Description
Support using form_data_key alone to inspect, preview, and query unsaved charts
What Changed
Impact
✅ Preview unsaved charts from Explore using form_data_key✅ Query data for unsaved charts using cached configuration✅ Retrieve chart metadata that reflects a user's unsaved edits💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.