codeant-ai-for-open-source[bot] commented on code in PR #40399:
URL: https://github.com/apache/superset/pull/40399#discussion_r3467074194
##########
superset/mcp_service/dashboard/tool/generate_dashboard.py:
##########
@@ -276,27 +288,32 @@ def generate_dashboard( # noqa: C901
from superset.models.dashboard import Dashboard
with
event_logger.log_context(action="mcp.generate_dashboard.db_write"):
- json_metadata = json.dumps(
- {
- "filter_scopes": {},
- "expanded_slices": {},
- "refresh_frequency": 0,
- "timed_refresh_immune_slices": [],
- "color_scheme": None,
- "label_colors": {},
- "shared_label_colors": {},
- "color_scheme_domain": [],
- "cross_filters_enabled": False,
- "native_filter_configuration": [],
- "global_chart_configuration": {
- "scope": {
- "rootPath": ["ROOT_ID"],
- "excluded": [],
- }
- },
- "chart_configuration": {},
- }
- )
+ # Build the default json_metadata that every new dashboard gets.
+ # When the caller supplied json_metadata_overrides, merge those
+ # in shallowly so the LLM can override label_colors / color_scheme
+ # / cross_filters_enabled without re-specifying the whole shape.
+ json_metadata_dict: Dict[str, Any] = {
+ "filter_scopes": {},
+ "expanded_slices": {},
+ "refresh_frequency": 0,
+ "timed_refresh_immune_slices": [],
+ "color_scheme": None,
+ "label_colors": {},
+ "shared_label_colors": {},
+ "color_scheme_domain": [],
+ "cross_filters_enabled": False,
+ "native_filter_configuration": [],
+ "global_chart_configuration": {
+ "scope": {
+ "rootPath": ["ROOT_ID"],
+ "excluded": [],
+ }
+ },
+ "chart_configuration": {},
+ }
+ if request.json_metadata_overrides:
+ json_metadata_dict.update(request.json_metadata_overrides)
+ json_metadata = json.dumps(json_metadata_dict)
Review Comment:
**Suggestion:** `json_metadata_overrides` is accepted as arbitrary `Any`
content and merged directly before JSON serialization, but
non-JSON-serializable values (e.g., sets, custom objects) will raise
`TypeError`, which is not caught by this handler path. This can crash the tool
instead of returning a structured error; validate/normalize overrides to
JSON-safe primitives (or catch `TypeError` and return a controlled failure).
[type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ generate_dashboard can crash on json_metadata_overrides custom objects.
- ⚠️ MCP caller sees generic failure instead of structured error.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The MCP create tool `generate_dashboard` is the entrypoint for dashboard
creation
(`superset/mcp_service/dashboard/tool/generate_dashboard.py:195-212`), and
its request
schema `GenerateDashboardRequest` (in
`superset/mcp_service/dashboard/schemas.py:620-645`)
exposes `json_metadata_overrides: Dict[str, Any] | None` at lines 642-653,
with `Any`
permitting arbitrary Python values.
2. In a Python context (for example, a unit test or internal helper),
construct a
`GenerateDashboardRequest` object with valid `chart_ids` and
`json_metadata_overrides={"label_colors": CustomObject()}`, where
`CustomObject` is a
user-defined class instance that is not one of the types handled by
Superset's JSON
helpers.
3. Call `generate_dashboard(request, ctx)`; inside the
`mcp.generate_dashboard.db_write`
block at
`superset/mcp_service/dashboard/tool/generate_dashboard.py:290-317`, the code
builds `json_metadata_dict` (lines 295-313), then executes
`json_metadata_dict.update(request.json_metadata_overrides)` at line 315,
inserting
`CustomObject` into the metadata dict, and then calls `json_metadata =
json.dumps(json_metadata_dict)` at line 316 using
`superset.utils.json.dumps`.
4. `superset.utils.json.dumps` (implemented in
`superset/utils/json.py:188-229`) uses
`json_iso_dttm_ser` and `base_json_conv` (lines 73-137) to serialize
non-primitive
objects; when it encounters `CustomObject`, none of the `isinstance` checks
in
`base_json_conv` match, so `base_json_conv` raises
`TypeError("Unserializable object
...")` at line 111.
5. This `TypeError` is not one of the exceptions caught by the outer handler
`except
(SQLAlchemyError, ValueError, AttributeError, ValidationError) as e:` at
`superset/mcp_service/dashboard/tool/generate_dashboard.py:126-137, so it
propagates out
of `generate_dashboard` without returning a `GenerateDashboardResponse` and
causes an
internal error for the MCP caller instead of a structured error message
about invalid
`json_metadata_overrides`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dfe2a1d236ee4573b12d1e974b307fc0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=dfe2a1d236ee4573b12d1e974b307fc0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/dashboard/tool/generate_dashboard.py
**Line:** 314:316
**Comment:**
*Type Error: `json_metadata_overrides` is accepted as arbitrary `Any`
content and merged directly before JSON serialization, but
non-JSON-serializable values (e.g., sets, custom objects) will raise
`TypeError`, which is not caught by this handler path. This can crash the tool
instead of returning a structured error; validate/normalize overrides to
JSON-safe primitives (or catch `TypeError` and return a controlled failure).
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%2F40399&comment_hash=3f1088e364f9909406a8e6a30704e408d8a1631bd42fd6007b2c0989c7aca62f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=3f1088e364f9909406a8e6a30704e408d8a1631bd42fd6007b2c0989c7aca62f&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/generate_dashboard.py:
##########
@@ -363,6 +418,13 @@ def generate_dashboard( # noqa: C901
error="Failed to create dashboard due to a database
error.",
)
+ # ``dashboard.id`` is fixed at create-commit time; the post-commit
+ # re-fetch below either returns the same row or fails — in either
+ # outcome the URL doesn't change. Bind it once so the three
+ # downstream consumers (partial response, DashboardInfo.url,
+ # response.dashboard_url) share a single source.
+ dashboard_url =
f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"
Review Comment:
**Suggestion:** The generated dashboard URL is always built from the numeric
ID, even when a slug was successfully set. This creates a contract mismatch
with the rest of dashboard tooling (which prefers slug URLs) and returns the
wrong canonical link for slugged dashboards; build the URL from `slug or id`.
[api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ New dashboards with slugs return ID-based URLs.
- ⚠️ URL shape inconsistent with update_dashboard `_build_dashboard_url`.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The MCP create tool `generate_dashboard` is implemented in
`superset/mcp_service/dashboard/tool/generate_dashboard.py:195-212` and
called by MCP
clients via the FastMCP tooling aggregator in
`superset/mcp_service/dashboard/tool/__init__.py:18-31`.
2. The request schema `GenerateDashboardRequest` in
`superset/mcp_service/dashboard/schemas.py:620-645` defines a `slug` field
(lines 623-631)
and documents: "When set, the dashboard is reachable at
`/superset/dashboard/<slug>/`
instead of `/superset/dashboard/<id>/`."
3. Call `generate_dashboard` with a non-empty `slug` (for example
`"q4-exec-review"`) so
that, inside `generate_dashboard`, `request.slug` is truthy and the slug is
applied to the
ORM object at
`superset/mcp_service/dashboard/tool/generate_dashboard.py:328-329` via
`dashboard.slug = request.slug`.
4. After committing the dashboard at lines 359-360, the code constructs
`dashboard_url` at
line 426 as
`f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"`, always
using the numeric `dashboard.id` and ignoring the slug, and then passes this
value both to
the partial minimal response (lines 63-71) and the full `DashboardInfo`
response at lines
80-96.
5. In contrast, the update tool's `_build_dashboard_url` helper in
`superset/mcp_service/dashboard/tool/update_dashboard.py:48-53` prefers slugs
(`f"{get_superset_base_url()}/superset/dashboard/{dashboard.slug or
dashboard.id}/"`), so
clients that create a dashboard with a slug via `generate_dashboard` and
later update it
via `update_dashboard` will receive non-canonical ID-based URLs from create
and slug-based
URLs from update for the same dashboard.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4fb7b9bcd0704737a98112aef185f9a3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4fb7b9bcd0704737a98112aef185f9a3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/dashboard/tool/generate_dashboard.py
**Line:** 426:426
**Comment:**
*Api Mismatch: The generated dashboard URL is always built from the
numeric ID, even when a slug was successfully set. This creates a contract
mismatch with the rest of dashboard tooling (which prefers slug URLs) and
returns the wrong canonical link for slugged dashboards; build the URL from
`slug or id`.
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%2F40399&comment_hash=320f4faf81ee3b77ce947fffdb5fec1ff75411d9a17454b2cca06c3c9202bb27&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=320f4faf81ee3b77ce947fffdb5fec1ff75411d9a17454b2cca06c3c9202bb27&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]
