codeant-ai-for-open-source[bot] commented on code in PR #36539:
URL: https://github.com/apache/superset/pull/36539#discussion_r2611396604
##########
superset/mcp_service/explore/tool/generate_explore_link.py:
##########
@@ -106,14 +106,21 @@ async def generate_explore_link(
# Generate explore link using shared utilities
explore_url = generate_url(dataset_id=request.dataset_id,
form_data=form_data)
+ # Extract form_data_key from the explore URL
+ form_data_key = None
+ if explore_url and "form_data_key=" in explore_url:
+ form_data_key =
explore_url.split("form_data_key=")[1].split("&")[0]
+
await ctx.report_progress(3, 3, "URL generation complete")
await ctx.info(
- "Explore link generated successfully: url_length=%s, dataset_id=%s"
- % (len(explore_url), request.dataset_id)
+ "Explore link generated successfully: url_length=%s,
dataset_id=%s, "
+ "form_data_key=%s" % (len(explore_url), request.dataset_id,
form_data_key)
Review Comment:
**Suggestion:** Calling len(explore_url) without ensuring `explore_url` is a
string can raise a TypeError if `generate_url` returns None; use a safe
fallback (e.g., len(explore_url or "")) so logging never throws. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
"form_data_key=%s" % (len(explore_url or ""),
request.dataset_id, form_data_key)
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current code calls len(explore_url) unguarded — if generate_url() ever
returns None (or another non-sequence), the logging call will raise a
TypeError and can break the tool's normal success path. Replacing with
len(explore_url or "") is a tiny, correct defensive change that prevents
a logging-induced crash without changing behavior when a valid URL exists.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/explore/tool/generate_explore_link.py
**Line:** 117:117
**Comment:**
*Possible Bug: Calling len(explore_url) without ensuring `explore_url`
is a string can raise a TypeError if `generate_url` returns None; use a safe
fallback (e.g., len(explore_url or "")) so logging never throws.
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/chart/tool/generate_chart.py:
##########
@@ -284,6 +284,43 @@ async def generate_chart( # noqa: C901
raise
# Update explore URL to use saved chart
explore_url =
f"{get_superset_base_url()}/explore/?slice_id={chart.id}"
+
+ # Generate form_data_key for saved charts (needed for chatbot
rendering)
+ try:
+ from superset.commands.explore.form_data.parameters import (
+ CommandParameters,
+ )
Review Comment:
**Suggestion:** Incorrect import path: the code imports `CommandParameters`
from `superset.commands.explore.form_data.parameters`, but the repository
exposes `CommandParameters` at `superset.commands.explore.parameters`; this
ImportError will prevent form_data_key generation for saved charts (it may be
caught by the surrounding try, but it's still a runtime bug and prevents the
intended behavior). [possible bug]
**Severity Level:** Critical 🚨
```suggestion
from superset.commands.explore.parameters import
CommandParameters
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The repository exposes CommandParameters in
superset.commands.explore.parameters (see the provided parameters.py).
Importing from superset.commands.explore.form_data.parameters will likely raise
ImportError at runtime and prevent form_data_key generation. Fixing the import
is a straightforward correctness fix and will allow the try-block to exercise
the intended code path instead of immediately failing at import.
</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/generate_chart.py
**Line:** 290:292
**Comment:**
*Possible Bug: Incorrect import path: the code imports
`CommandParameters` from `superset.commands.explore.form_data.parameters`, but
the repository exposes `CommandParameters` at
`superset.commands.explore.parameters`; this ImportError will prevent
form_data_key generation for saved charts (it may be caught by the surrounding
try, but it's still a runtime bug and prevents the intended 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.
```
</details>
##########
superset/mcp_service/explore/tool/generate_explore_link.py:
##########
@@ -106,14 +106,21 @@ async def generate_explore_link(
# Generate explore link using shared utilities
explore_url = generate_url(dataset_id=request.dataset_id,
form_data=form_data)
+ # Extract form_data_key from the explore URL
+ form_data_key = None
+ if explore_url and "form_data_key=" in explore_url:
+ form_data_key =
explore_url.split("form_data_key=")[1].split("&")[0]
Review Comment:
**Suggestion:** Naive string-splitting to extract query param
`form_data_key` is brittle and can break on URL encodings or if the key appears
outside the query; parse the URL query parameters using urllib.parse to
robustly extract the value. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
if explore_url:
from urllib.parse import urlparse, parse_qs
parsed = urlparse(explore_url)
qs = parse_qs(parsed.query)
form_data_key = qs.get("form_data_key", [None])[0]
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current naive split is brittle: it can be tripped by URL-encoded values,
repeated params, or if the substring occurs outside the query. Using
urllib.parse.urlparse + parse_qs (or parse_qsl) yields the correct query
extraction behavior and handles encoding/quoting and multiple values
robustly.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/explore/tool/generate_explore_link.py
**Line:** 110:112
**Comment:**
*Logic Error: Naive string-splitting to extract query param
`form_data_key` is brittle and can break on URL encodings or if the key appears
outside the query; parse the URL query parameters using urllib.parse to
robustly extract the value.
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/chart/tool/generate_chart.py:
##########
@@ -405,25 +442,27 @@ async def generate_chart( # noqa: C901
# Return enhanced data while maintaining backward compatibility
await ctx.report_progress(4, 5, "Building response")
+
+ # Build chart info using serialize_chart_object for saved charts
+ chart_info = None
+ if request.save_chart and chart:
+ from superset.mcp_service.chart.schemas import
serialize_chart_object
+
+ chart_info = serialize_chart_object(chart)
+ if chart_info:
+ # Override the URL with explore_url
+ chart_info.url = explore_url
+
result = {
- "chart": {
- "id": chart.id if chart else None,
- "slice_name": chart.slice_name
- if chart
- else generate_chart_name(request.config),
- "viz_type": chart.viz_type if chart else
form_data.get("viz_type"),
- "url": explore_url,
- "uuid": str(chart.uuid) if chart and chart.uuid else None,
- "saved": request.save_chart,
- }
- if request.save_chart and chart
- else None,
+ "chart": chart_info.model_dump() if chart_info else None,
Review Comment:
**Suggestion:** Unsafe assumption about `serialize_chart_object` return
type: the code assumes the result has a `model_dump()` method and sets `chart`
in the response using `chart_info.model_dump()`. If `serialize_chart_object`
returns a dict or other structure without `model_dump`, this will raise an
AttributeError; make the access safe by using `model_dump()` only when
available and otherwise pass through the dict/object. [type error]
**Severity Level:** Minor ⚠️
```suggestion
"chart": (chart_info.model_dump() if hasattr(chart_info,
"model_dump") else chart_info) if chart_info else None,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
serialize_chart_object may legitimately return a plain dict or another
structure without model_dump(); calling model_dump() unconditionally can raise
AttributeError. Using a hasattr check is a small defensive change that prevents
errors when serialize_chart_object returns non-Pydantic structures.
</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/generate_chart.py
**Line:** 457:457
**Comment:**
*Type Error: Unsafe assumption about `serialize_chart_object` return
type: the code assumes the result has a `model_dump()` method and sets `chart`
in the response using `chart_info.model_dump()`. If `serialize_chart_object`
returns a dict or other structure without `model_dump`, this will raise an
AttributeError; make the access safe by using `model_dump()` only when
available and otherwise pass through the dict/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.
```
</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]
