codeant-ai-for-open-source[bot] commented on code in PR #37183:
URL: https://github.com/apache/superset/pull/37183#discussion_r2774486545
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -119,20 +144,102 @@ async def get_chart_data( # noqa: C901
)
logger.info("Getting data for chart %s: %s", chart.id,
chart.slice_name)
- import time
-
start_time = time.time()
+ # Track whether we're using unsaved state
+ using_unsaved_state = False
+ cached_form_data_dict = None
+
try:
await ctx.report_progress(2, 4, "Preparing data query")
from superset.charts.schemas import ChartDataQueryContextSchema
from superset.commands.chart.data.get_data_command import
ChartDataCommand
+ # Check if form_data_key is provided - use cached form_data instead
+ if request.form_data_key:
+ await ctx.info(
+ "Retrieving unsaved chart state from cache:
form_data_key=%s"
+ % (request.form_data_key,)
+ )
+ if cached_form_data :=
_get_cached_form_data(request.form_data_key):
+ try:
+ cached_form_data_dict =
utils_json.loads(cached_form_data)
+ using_unsaved_state = True
+ await ctx.info(
+ "Using cached form_data from form_data_key for
data query"
+ )
Review Comment:
**Suggestion:** If the cached form_data parses into a non-dict JSON value
(for example JSON 'null'), the code sets the unsaved-state flag but has no
usable form_data dict, so the later condition that builds the query context is
skipped while the fallback path is disabled, leaving the query_context variable
undefined and causing a runtime error when executing the query; you should only
set the unsaved-state flag when the parsed JSON is actually a dictionary and
otherwise fall back to the saved chart configuration. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ get_chart_data tool crashes when cached form_data is non-object.
- ⚠️ Unsaved-chart data retrieval silently falls back incorrectly.
- ⚠️ Affects MCP chart data retrieval flow only.
```
</details>
```suggestion
if isinstance(cached_form_data_dict, dict):
using_unsaved_state = True
await ctx.info(
"Using cached form_data from
form_data_key for data query"
)
else:
await ctx.warning(
"Cached form_data is not a JSON object. "
"Falling back to saved chart
configuration."
)
cached_form_data_dict = None
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP tool get_chart_data() in
superset/mcp_service/chart/tool/get_chart_data.py
(function get_chart_data). Provide a GetChartDataRequest with form_data_key
set (client
sends form_data_key). The function enters the "if request.form_data_key"
branch and
reaches the cached-form-data handling at
superset/mcp_service/chart/tool/get_chart_data.py:165-188 where
cached_form_data is
retrieved.
2. _get_cached_form_data() calls GetFormDataCommand.run() (see
superset/commands/explore/form_data/get.py:35-63). That command returns
state["form_data"]
verbatim. If the cached entry's form_data is the JSON string "null" (or any
JSON value
that is not a JSON object), GetFormDataCommand.run() will return that string.
3. Back in get_chart_data, the code executes
utils_json.loads(cached_form_data) at
superset/mcp_service/chart/tool/get_chart_data.py:166.
utils_json.loads("null") yields
Python None (not a dict). The current code then unconditionally sets
using_unsaved_state =
True at superset/mcp_service/chart/tool/get_chart_data.py:167 even though
cached_form_data_dict is None.
4. Later the code checks "if using_unsaved_state and cached_form_data_dict
is not None:"
at superset/mcp_service/chart/tool/get_chart_data.py:188. Because
cached_form_data_dict is
None the branch is skipped. The fallback that constructs a query_context
when no unsaved
state is used is guarded by "if query_context_json is None and not
using_unsaved_state:" —
but using_unsaved_state is True, so that fallback is also skipped. As a
result
query_context is never set, and the code later calls
ChartDataCommand(query_context)
(superset/mcp_service/chart/tool/get_chart_data.py around the query
execution section),
causing an UnboundLocalError / runtime failure. This reproduces reliably when
form_data_key exists and the cached form_data parses to a non-dict JSON
value (e.g.,
"null").
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/get_chart_data.py
**Line:** 168:170
**Comment:**
*Logic Error: If the cached form_data parses into a non-dict JSON value
(for example JSON 'null'), the code sets the unsaved-state flag but has no
usable form_data dict, so the later condition that builds the query context is
skipped while the fallback path is disabled, leaving the query_context variable
undefined and causing a runtime error when executing the query; you should only
set the unsaved-state flag when the parsed JSON is actually a dictionary and
otherwise fall back to the saved chart configuration.
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.
```
</details>
##########
superset/mcp_service/dashboard/tool/get_dashboard_info.py:
##########
@@ -71,14 +110,67 @@ async def get_dashboard_info(
result = tool.run_tool(request.identifier)
if isinstance(result, DashboardInfo):
+ # If permalink_key is provided, retrieve filter state
+ if request.permalink_key:
+ await ctx.info(
+ "Retrieving filter state from permalink: permalink_key=%s"
+ % (request.permalink_key,)
+ )
+ permalink_value = _get_permalink_state(request.permalink_key)
+
+ if permalink_value:
+ # Verify the permalink belongs to the requested dashboard
+ # dashboardId in permalink is stored as str, result.id is
int
+ permalink_dashboard_id = permalink_value.get("dashboardId")
+ try:
+ permalink_dashboard_id_int = (
+ int(permalink_dashboard_id)
+ if permalink_dashboard_id
+ else None
+ )
+ except (ValueError, TypeError):
+ permalink_dashboard_id_int = None
+
+ if (
+ permalink_dashboard_id_int is not None
+ and permalink_dashboard_id_int != result.id
+ ):
+ await ctx.warning(
+ "permalink_key dashboardId (%s) does not match "
+ "requested dashboard id (%s); ignoring permalink "
+ "filter state." % (permalink_dashboard_id,
result.id)
+ )
+ else:
+ # Extract the state from permalink value
+ # Cast to dict for Pydantic compatibility
+ permalink_state = dict(permalink_value.get("state",
{}))
Review Comment:
**Suggestion:** The code assumes that the `state` field in the permalink
value is always a dict and passes it directly to `dict()`, which will raise at
runtime if the stored value is `None` or any non-mapping type, causing the
whole tool to fail instead of just ignoring malformed state. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ get_dashboard_info may raise, returning InternalError.
- ⚠️ Per-link filter state retrieval can silently fail.
```
</details>
```suggestion
raw_state = permalink_value.get("state") or {}
permalink_state = dict(raw_state) if
isinstance(raw_state, dict) else {}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start MCP service and call get_dashboard_info with a valid permalink_key
(superset/mcp_service/dashboard/tool/get_dashboard_info.py:113-119).
2. _get_permalink_state() returns a DashboardPermalinkValue whose "state"
key is None or a non-mapping value (value returned by
superset/commands/dashboard/permalink/get.py:38-63).
3. Execution reaches
superset/mcp_service/dashboard/tool/get_dashboard_info.py:146,
where dict(permalink_value.get("state", {})) is executed. If the raw
state is
None or a list, dict(None) or dict(list) raises TypeError at this line.
4. The exception escapes this block and is handled by the surrounding
except Exception handler in the same file, causing get_dashboard_info to
return a DashboardError (internal error) instead of returning dashboard
info.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/get_dashboard_info.py
**Line:** 146:146
**Comment:**
*Possible Bug: The code assumes that the `state` field in the permalink
value is always a dict and passes it directly to `dict()`, which will raise at
runtime if the stored value is `None` or any non-mapping type, causing the
whole tool to fail instead of just ignoring malformed state.
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.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
