codeant-ai-for-open-source[bot] commented on code in PR #38277:
URL: https://github.com/apache/superset/pull/38277#discussion_r2927042372
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -749,7 +749,7 @@ def _export_data_as_csv(
chart_id=chart.id,
chart_name=chart.slice_name or f"Chart {chart.id}",
chart_type=chart.viz_type or "unknown",
- columns=[], # Not needed for CSV export
+ columns=columns,
Review Comment:
**Suggestion:** `ChartData.columns` is defined as `List[DataColumn]`, but
this passes a plain list of strings. Pydantic validation will fail at runtime
for CSV exports. Build `DataColumn` objects from the column names instead.
[type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ `get_chart_data format=csv` fails response model validation.
- ❌ MCP clients receive error, not CSV export.
- ⚠️ Token guidance recommends CSV path frequently
(`token_utils.py:344-347`).
```
</details>
```suggestion
columns=[
DataColumn(
name=col,
display_name=col.replace("_", " ").title(),
data_type="string",
sample_values=[
row.get(col) for row in data[:3] if row.get(col) is not
None
][:3],
null_count=sum(1 for row in data if row.get(col) is None),
unique_count=len({str(row.get(col)) for row in data}),
)
for col in columns
],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start MCP service; `get_chart_data` is registered via imports in
`superset/mcp_service/app.py:393-396` and declared as a tool in
`superset/mcp_service/chart/tool/get_chart_data.py:66-70`.
2. Call MCP tool `get_chart_data` with a valid chart identifier and
`format="csv"`;
execution takes the CSV branch at
`superset/mcp_service/chart/tool/get_chart_data.py:617-621`.
3. `_export_data_as_csv()` builds `ChartData` with `columns=columns` at
`superset/mcp_service/chart/tool/get_chart_data.py:748-753`, where `columns`
is
`List[str]`.
4. `ChartData` requires `columns: List[DataColumn]`
(`superset/mcp_service/chart/schemas.py:1071`), so model validation fails
and the tool
returns error instead of CSV payload.
```
</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:** 752:752
**Comment:**
*Type Error: `ChartData.columns` is defined as `List[DataColumn]`, but
this passes a plain list of strings. Pydantic validation will fail at runtime
for CSV exports. Build `DataColumn` objects from the column names instead.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38277&comment_hash=0fe5f1abca6bef123d10a48f355eae20916448d199aa66fa34c328737574af32&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38277&comment_hash=0fe5f1abca6bef123d10a48f355eae20916448d199aa66fa34c328737574af32&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -904,7 +904,7 @@ def _create_excel_chart_data(
chart_id=chart.id,
chart_name=chart_name,
chart_type=chart.viz_type or "unknown",
- columns=[],
+ columns=list(data[0].keys()) if data else [],
Review Comment:
**Suggestion:** This Excel response path also passes a list of strings to
`ChartData.columns`, which expects `DataColumn` objects. That will raise a
validation error when building the response. Convert inferred column names into
`DataColumn` entries. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ `get_chart_data format=excel` fails on openpyxl path.
- ❌ Excel payload unavailable to MCP consumers.
- ⚠️ Export feature appears broken despite successful query execution.
```
</details>
```suggestion
columns=[
DataColumn(
name=col,
display_name=col.replace("_", " ").title(),
data_type="string",
sample_values=[
row.get(col) for row in data[:3] if row.get(col) is not
None
][:3],
null_count=sum(1 for row in data if row.get(col) is None),
unique_count=len({str(row.get(col)) for row in data}),
)
for col in (list(data[0].keys()) if data else [])
],
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Invoke MCP tool `get_chart_data` (registered in
`superset/mcp_service/app.py:393-396`)
with `format="excel"` and a chart that returns rows.
2. In `get_chart_data`, the excel branch is selected at
`superset/mcp_service/chart/tool/get_chart_data.py:628-632`, which calls
`_export_data_as_excel()`.
3. On normal environment path, `_create_excel_chart_data()` is used
(`superset/mcp_service/chart/tool/get_chart_data.py:890-904`) and sets
`columns=list(data[0].keys()) if data else []` at line `907`.
4. `ChartData` enforces `List[DataColumn]`
(`superset/mcp_service/chart/schemas.py:1071`),
so constructing the response fails validation and excel export request
returns error.
```
</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:** 907:907
**Comment:**
*Type Error: This Excel response path also passes a list of strings to
`ChartData.columns`, which expects `DataColumn` objects. That will raise a
validation error when building the response. Convert inferred column names into
`DataColumn` entries.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38277&comment_hash=ce57cfd71f788aa41c49633e30c23387d755b99802b6bb3c4ed836c00a0a4a4c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38277&comment_hash=ce57cfd71f788aa41c49633e30c23387d755b99802b6bb3c4ed836c00a0a4a4c&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -2037,7 +2038,10 @@ def __init__(self, form_data: Dict[str, Any]):
elif isinstance(content, TablePreview):
result.format = "table"
result.table_data = content.table_data
- # Base64 preview support removed
+ elif isinstance(content, VegaLitePreview):
+ result.format = "vega_lite"
+ elif isinstance(content, URLPreview):
+ result.format = "url"
Review Comment:
**Suggestion:** The URL branch only sets `format` and drops the
already-generated dimensions from `URLPreview`, so backward-compatible
`ChartPreview.width`/`height` stay `None` for URL responses. This creates
inconsistent response shape across formats and can break clients that rely on
those fields. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ URL preview top-level width/height are always null.
- ⚠️ Backward-compat response shape differs by preview format.
- ⚠️ Consumers reading legacy dimensions may mis-handle URL previews.
```
</details>
```suggestion
elif isinstance(content, URLPreview):
result.format = "url"
result.width = content.width
result.height = content.height
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call MCP `get_chart_preview`
(`superset/mcp_service/chart/tool/get_chart_preview.py:2074`) for any
persisted chart with
`format="url"` (or omit format; default is `"url"` in
`superset/mcp_service/chart/schemas.py:1126-1129`).
2. `URLPreviewStrategy.generate()` returns `URLPreview` including dimensions
(`width`/`height`) at
`superset/mcp_service/chart/tool/get_chart_preview.py:101-105`.
3. `_get_chart_preview_internal()` builds `ChartPreview` and handles
backward-compatible
fields at `:2032-2047`; ASCII branch copies width/height (`:2036-2037`) but
URL branch
only sets format (`:2043-2044`).
4. Final response keeps `ChartPreview.width` and `ChartPreview.height` as
`None` defaults
(`superset/mcp_service/chart/schemas.py:1302-1307`), producing inconsistent
top-level
shape for URL responses.
```
</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_preview.py
**Line:** 2043:2044
**Comment:**
*Logic Error: The URL branch only sets `format` and drops the
already-generated dimensions from `URLPreview`, so backward-compatible
`ChartPreview.width`/`height` stay `None` for URL responses. This creates
inconsistent response shape across formats and can break clients that rely on
those fields.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38277&comment_hash=e73f9e537e132266dce2b60b1be62dfbc6c6808e736e06076b0d2b7f38ac2196&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38277&comment_hash=e73f9e537e132266dce2b60b1be62dfbc6c6808e736e06076b0d2b7f38ac2196&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -89,19 +89,20 @@ def generate(self) -> ChartPreview | ChartError:
class URLPreviewStrategy(PreviewFormatStrategy):
- """Generate URL-based image preview."""
+ """Generate URL-based preview with explore link."""
def generate(self) -> URLPreview | ChartError:
- # Screenshot-based URL previews are not supported.
- # Users should use the explore_url to view the chart interactively,
- # or use other preview formats like 'ascii', 'table', or 'vega_lite'.
- return ChartError(
- error=(
- "URL-based screenshot previews are not supported. "
- "Use the explore_url to view the chart interactively, "
- "or try formats: 'ascii', 'table', or 'vega_lite'."
- ),
- error_type="UnsupportedFormat",
+ chart = self.chart
+ if not chart.id:
+ return ChartError(
+ error="URL preview not available for transient charts without
an ID",
+ error_type="UnsupportedFormat",
+ )
+ explore_url = f"{get_superset_base_url()}/explore/?slice_id={chart.id}"
+ return URLPreview(
+ preview_url=explore_url,
+ width=self.request.width or 800,
+ height=self.request.height or 600,
)
Review Comment:
**Suggestion:** The URL strategy now rejects all transient/unsaved charts
when `chart.id` is missing, but unsaved previews are a supported flow via
`form_data_key` and the default preview format is `url`. This causes valid
unsaved preview requests to fail with `UnsupportedFormat` instead of returning
an explore link. Build a `form_data_key`-based explore URL when no chart ID
exists. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Unsaved Explore preview requests fail in default URL mode.
- ⚠️ `form_data_key` preview flow documented but not honored.
- ⚠️ MCP clients receive UnsupportedFormat for valid transient charts.
```
</details>
```suggestion
chart = self.chart
base_url = get_superset_base_url()
if chart.id is not None:
explore_url = f"{base_url}/explore/?slice_id={chart.id}"
else:
form_data_key = self.request.form_data_key
if not form_data_key and isinstance(self.request.identifier,
str):
form_data_key = self.request.identifier
if not form_data_key:
return ChartError(
error="URL preview requires either a chart ID or
form_data_key",
error_type="UnsupportedFormat",
)
explore_url =
f"{base_url}/explore/?form_data_key={form_data_key}"
return URLPreview(
preview_url=explore_url,
width=self.request.width or 800,
height=self.request.height or 600,
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Invoke MCP tool `get_chart_preview` (registered in
`superset/mcp_service/app.py:14-19`)
with an unsaved Explore cache key as `identifier` and omit `format` (default
is `"url"`
per `superset/mcp_service/chart/schemas.py:1126-1129`).
2. Request enters `_get_chart_preview_internal()` at
`superset/mcp_service/chart/tool/get_chart_preview.py:1816` and, when no
chart is found by
UUID, executes the transient fallback at `:1861-1897`, creating
`TransientChart` with
`self.id = None` (`:1888-1896`).
3. Preview generation uses `PreviewFormatGenerator` in
`_get_chart_preview_internal()`
(`:1972-1977`), selecting URL strategy because format is `"url"`.
4. `URLPreviewStrategy.generate()` (`:94-106`) hits `if not chart.id` at
`:96`, returning
`ChartError(error_type="UnsupportedFormat")` (`:97-100`) instead of a usable
explore link
for unsaved state.
```
</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_preview.py
**Line:** 95:106
**Comment:**
*Logic Error: The URL strategy now rejects all transient/unsaved charts
when `chart.id` is missing, but unsaved previews are a supported flow via
`form_data_key` and the default preview format is `url`. This causes valid
unsaved preview requests to fail with `UnsupportedFormat` instead of returning
an explore link. Build a `form_data_key`-based explore URL when no chart ID
exists.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38277&comment_hash=6bff57691f807becd36fae5b2473f16ba7380b4728262cf60ca2bfe4931354aa&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38277&comment_hash=6bff57691f807becd36fae5b2473f16ba7380b4728262cf60ca2bfe4931354aa&reaction=dislike'>👎</a>
--
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]
