aminghadersohi commented on code in PR #40959:
URL: https://github.com/apache/superset/pull/40959#discussion_r3493691271
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -914,6 +914,138 @@ class GenerateDashboardResponse(BaseModel):
)
+class DuplicateDashboardRequest(BaseModel):
+ """Request schema for duplicating an existing dashboard."""
+
+ model_config = ConfigDict(populate_by_name=True)
+
+ dashboard_id: Annotated[
+ int | str,
+ Field(
+ description=(
+ "Source dashboard identifier - can be numeric ID, UUID string,
or slug"
+ )
+ ),
+ ]
Review Comment:
Declining. Restricting to UUID/slug-only would narrow `int | str` without
benefit here:
1. **Inconsistent with existing tools** —
`AddChartToDashboardRequest.dashboard_id` in the same file (line 547) uses
plain `int`; accepting `int | str` is already _more_ UUID-friendly than the
status quo.
2. **UUID migration scope** — the CLAUDE.md UUID migration guidance targets
*new DB model primary keys*, not MCP request schemas. Request schemas should
reflect what the backing DAO accepts, and `DashboardDAO.get_by_id_or_slug`
already handles int IDs, UUID strings, and slugs.
3. **Breaking callers** — users who look up dashboards by numeric ID (e.g.
from a list response) would get an immediate schema validation error. That's a
regression, not a safety improvement.
No change.
--
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]
