codeant-ai-for-open-source[bot] commented on code in PR #40077:
URL: https://github.com/apache/superset/pull/40077#discussion_r3230462105
##########
superset/mcp_service/chart/tool/update_chart_preview.py:
##########
@@ -229,9 +234,35 @@ def update_chart_preview( # noqa: C901
high_contrast_available=False,
)
- # Note: Screenshot-based previews are not supported.
- # Use the explore_url to view the chart interactively.
previews: Dict[str, Any] = {}
+ if request.generate_preview:
+ try:
+ with event_logger.log_context(
+ action="mcp.update_chart_preview.preview"
+ ):
+ for format_type in request.preview_formats:
+ # URL previews are represented by
explore_url/chart.url.
+ # Screenshot-based previews are not supported.
+ if format_type not in
SUPPORTED_FORM_DATA_PREVIEW_FORMATS:
+ continue
+
+ preview_result = generate_preview_from_form_data(
+ form_data=new_form_data,
+ dataset_id=dataset.id,
+ preview_format=format_type,
+ )
+
+ if isinstance(preview_result, ChartError):
+ logger.warning(
+ "Preview '%s' failed: %s",
+ format_type,
+ preview_result.error,
+ )
+ else:
+ previews[format_type] = preview_result
+
Review Comment:
**Suggestion:** `generate_preview_from_form_data` returns Pydantic preview
models (`ASCIIPreview`/`TablePreview`/`VegaLitePreview`), but this tool returns
a raw `dict` response and stores those model instances directly. That can break
MCP/JSON serialization at runtime (or produce inconsistent wire payloads)
because `previews` may contain non-JSON-native objects. Serialize each preview
model (for example with `model_dump(mode="json")`) before inserting it into the
response map. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ update_chart_preview fails when returning table/ascii previews.
- ❌ MCP clients may receive non-JSON-serializable preview objects.
- ⚠️ Behavior inconsistent with generate_chart Pydantic response handling.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The MCP FastMCP server is created in `superset/mcp_service/app.py:25-29`
and exposes
the `update_chart_preview` tool (listed in `get_default_instructions()` at
`superset/mcp_service/app.py:67-77`), so external MCP clients can call it.
2. A client (mirroring `Client.call_tool("update_chart_preview", {"request":
...})` as in
`tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py:169-172`)
sends a
request with `generate_preview=True` and `preview_formats=["table"]` and a
numeric
`dataset_id` that resolves to a real dataset, so the code in
`update_chart_preview()` at
`superset/mcp_service/chart/tool/update_chart_preview.py:117-151`
successfully loads the
dataset and passes validation.
3. Inside `update_chart_preview()`, the preview generation block at
`superset/mcp_service/chart/tool/update_chart_preview.py:237-253` iterates
`request.preview_formats`, sees `"table"` in
`SUPPORTED_FORM_DATA_PREVIEW_FORMATS`, and
calls `generate_preview_from_form_data(form_data=new_form_data,
dataset_id=dataset.id,
preview_format="table")` (lines 249-253). In
`superset/mcp_service/chart/preview_utils.py:65-79`,
`generate_preview_from_form_data()`
executes the query and, for `preview_format == "table"`, returns a
`TablePreview` Pydantic
model (`_generate_table_preview_from_data()` annotated to return
`TablePreview` at
`preview_utils.py:235-237`, which constructs a `TablePreview` instance at
`preview_utils.py:240-242` or `292-293`).
4. Back in `update_chart_preview()`, the `isinstance(preview_result,
ChartError)` check at
`update_chart_preview.py:255-259` fails (the result is a `TablePreview`, not
`ChartError`), so the `else` branch at line 263 executes and assigns
`previews["table"] =
preview_result`, where `preview_result` is a Pydantic `TablePreview` object.
The function
then builds a plain `dict` `result` and returns it directly at
`update_chart_preview.py:23-50` with `"previews": previews`. Because
`update_chart_preview()`'s return type is `Dict[str, Any]` and it is not
wrapped in a
Pydantic response model, the FastMCP tooling must JSON-serialize this plain
dict; with a
`TablePreview` instance stored under `previews["table"]`, standard JSON
encoding cannot
natively serialize the object, leading to runtime serialization failures or
inconsistent
wire payloads when the MCP server attempts to respond.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fupdate_chart_preview.py%0A%2A%2ALine%3A%2A%2A%20263%3A263%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20%60generate_preview_from_form_data%60%20returns%20Pydantic%20preview%20models%20%28%60ASCIIPreview%60%2F%60TablePreview%60%2F%60VegaLitePreview%60%29%2C%20but%20this%20tool%20returns%20a%20raw%20%60dict%60%20response%20and%20stores%20those%20model%20instances%20directly.%20That%20can%20break%20MCP%2FJSON%20serialization%20at%20runtime%20%28or%20produce%20inconsistent%20wire%20payloads%29%20because%20%60previews%60%20may%20contain%20non-JSON-native%20objects.%20Serialize%20each%20preview%20model%20%28for%20example%20with%20%60model_dump%28mode%3D%22json%22%29%60%29%20before%20inserting%20it%20into%20the%20response%20map.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20is
sue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fmcp_service%2Fchart%2Ftool%2Fupdate_chart_preview.py%0A%2A%2ALine%3A%2A%2A%20263%3A263%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20%60generate_preview_from_form_data%60%20returns%20Pydantic%20preview%20models%20%28%60ASCIIPreview%60%2F%60TablePreview%60%2F%60VegaLitePreview%60%29%2C%20but%20this%20tool%20returns%20a%20raw%2
0%60dict%60%20response%20and%20stores%20those%20model%20instances%20directly.%20That%20can%20break%20MCP%2FJSON%20serialization%20at%20runtime%20%28or%20produce%20inconsistent%20wire%20payloads%29%20because%20%60previews%60%20may%20contain%20non-JSON-native%20objects.%20Serialize%20each%20preview%20model%20%28for%20example%20with%20%60model_dump%28mode%3D%22json%22%29%60%29%20before%20inserting%20it%20into%20the%20response%20map.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<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/update_chart_preview.py
**Line:** 263:263
**Comment:**
*Api Mismatch: `generate_preview_from_form_data` returns Pydantic
preview models (`ASCIIPreview`/`TablePreview`/`VegaLitePreview`), but this tool
returns a raw `dict` response and stores those model instances directly. That
can break MCP/JSON serialization at runtime (or produce inconsistent wire
payloads) because `previews` may contain non-JSON-native objects. Serialize
each preview model (for example with `model_dump(mode="json")`) before
inserting it into the response map.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40077&comment_hash=65cca1c481aeabaa624dd63449138c8de4f294f3b23d7ba1638b6fdd1b4a73bb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40077&comment_hash=65cca1c481aeabaa624dd63449138c8de4f294f3b23d7ba1638b6fdd1b4a73bb&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py:
##########
@@ -698,6 +698,74 @@ async def
test_preserves_previous_adhoc_filters_without_warning(
assert result["warnings"] == []
mock_get_previous_form_data.assert_called_once_with("valid_key_12345")
+ @patch.object(update_chart_preview_module, "validate_and_compile")
+ @patch.object(update_chart_preview_module, "has_dataset_access",
return_value=True)
+ @patch("superset.daos.dataset.DatasetDAO.find_by_id")
+
@patch("superset.mcp_service.chart.preview_utils.generate_preview_from_form_data")
Review Comment:
**Suggestion:** This patch target is incorrect for the code under test:
`update_chart_preview.py` imports `generate_preview_from_form_data` directly
into its module namespace, so patching
`superset.mcp_service.chart.preview_utils.generate_preview_from_form_data` does
not replace the function actually called by `update_chart_preview`. The test
can execute real preview generation instead of the mock, making it flaky and
potentially failing unexpectedly. Patch
`update_chart_preview_module.generate_preview_from_form_data` instead. [api
mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unit test may hit real database/query stack.
- ❌ Assertions on preview content and mock calls unreliable.
- ⚠️ Harder to reason about failures in preview update tests.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The test module imports the tool module once at import time via
`update_chart_preview_module =
importlib.import_module("superset.mcp_service.chart.tool.update_chart_preview")`
in
`tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py:39-45`.
In that
module, `generate_preview_from_form_data` is imported into the module
namespace with `from
superset.mcp_service.chart.preview_utils import
generate_preview_from_form_data` at
`superset/mcp_service/chart/tool/update_chart_preview.py:42-46`, binding a
local name
inside `update_chart_preview_module`.
2. The test `test_returns_requested_table_preview` is decorated with
`@patch("superset.mcp_service.chart.preview_utils.generate_preview_from_form_data")`
at
`tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py:52-61`
(line 55 in
the hunk). This patch changes the attribute
`generate_preview_from_form_data` on the
`superset.mcp_service.chart.preview_utils` module, but **does not** modify
the
already-imported
`update_chart_preview_module.generate_preview_from_form_data` reference
that `update_chart_preview()` actually calls.
3. When the test invokes
`update_chart_preview_module.update_chart_preview(request=request,
ctx=Mock())` at
`tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py:108-110`,
the
function enters the preview block at
`superset/mcp_service/chart/tool/update_chart_preview.py:237-253` (because
`request.generate_preview=True` and `"table"` is present in
`request.preview_formats`). It
calls the unpatched local symbol `generate_preview_from_form_data`, which
still points to
the real implementation in
`superset/mcp_service/chart/preview_utils.py:65-79`, issuing
real `ChartDataCommand` queries and DB lookups instead of the intended mock.
4. As a result, the patched argument `mock_generate_preview_from_form_data`
passed into
the test (from the decorator at line 55) is never invoked, so assertions like
`mock_generate_preview_from_form_data.assert_called_once()` and
`result["previews"] ==
{"table": table_preview}` at
`tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py:112-118`
can fail or
become flaky depending on whether the real
`generate_preview_from_form_data()` hits a real
database, returns a `ChartError`, or raises an exception. The root cause is
that the patch
target string should be
`"update_chart_preview_module.generate_preview_from_form_data"` to
replace the symbol actually used by `update_chart_preview()`.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_update_chart_preview.py%0A%2A%2ALine%3A%2A%2A%20704%3A704%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20This%20patch%20target%20is%20incorrect%20for%20the%20code%20under%20test%3A%20%60update_chart_preview.py%60%20imports%20%60generate_preview_from_form_data%60%20directly%20into%20its%20module%20namespace%2C%20so%20patching%20%60superset.mcp_service.chart.preview_utils.generate_preview_from_form_data%60%20does%20not%20replace%20the%20function%20actually%20called%20by%20%60update_chart_preview%60.%20The%20test%20can%20execute%20real%20preview%20generation%20instead%20of%20the%20mock%2C%20making%20it%20flaky%20and%20potentially%20failing%20unexpectedly.%20Patch%20%60update_chart_preview_module.generate_preview_from_form_data%60%20instead.%0A%0AValidate%20the%20correct
ness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20tests%2Funit_tests%2Fmcp_service%2Fchart%2Ftool%2Ftest_update_chart_preview.py%0A%2A%2ALine%3A%2A%2A%20704%3A704%0A%2A%2AComment%3A%2A%2A%0A%09%2AApi%20Mismatch%3A%20This%20patch%20target%20is%20incorrect%20for%20the%20code%20under%20test%3A%20%60update_chart_preview.py%60%20imports%20%60generate_preview_from_form
_data%60%20directly%20into%20its%20module%20namespace%2C%20so%20patching%20%60superset.mcp_service.chart.preview_utils.generate_preview_from_form_data%60%20does%20not%20replace%20the%20function%20actually%20called%20by%20%60update_chart_preview%60.%20The%20test%20can%20execute%20real%20preview%20generation%20instead%20of%20the%20mock%2C%20making%20it%20flaky%20and%20potentially%20failing%20unexpectedly.%20Patch%20%60update_chart_preview_module.generate_preview_from_form_data%60%20instead.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%2
0implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py
**Line:** 704:704
**Comment:**
*Api Mismatch: This patch target is incorrect for the code under test:
`update_chart_preview.py` imports `generate_preview_from_form_data` directly
into its module namespace, so patching
`superset.mcp_service.chart.preview_utils.generate_preview_from_form_data` does
not replace the function actually called by `update_chart_preview`. The test
can execute real preview generation instead of the mock, making it flaky and
potentially failing unexpectedly. Patch
`update_chart_preview_module.generate_preview_from_form_data` 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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40077&comment_hash=6247b701e49bbf5cd683c631340214eaa610c65e8fececdc4c881d66eefb2649&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40077&comment_hash=6247b701e49bbf5cd683c631340214eaa610c65e8fececdc4c881d66eefb2649&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]
