gabotorresruiz commented on code in PR #44797:
URL: https://github.com/apache/superset/pull/44797#discussion_r4160578385
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -371,6 +389,8 @@ def _require_identifier_or_permalink(self) ->
"GetDashboardLayoutRequest":
)
if identifier_is_blank and permalink_is_blank:
raise ValueError("Provide identifier or permalink_key")
+ if self.untabbed_only and (self.tabs_only or self.tab is not None):
+ raise ValueError("untabbed_only cannot be combined with tab or
tabs_only")
Review Comment:
Not a blocker, and not something this PR broke, but worth knowing before
this ships as an agent recovery feature: this message never reaches the caller.
`validation_message` maps Pydantic error types through a fixed `_REASONS`
vocabulary (`superset/mcp_service/utils/validation.py:204`) that deliberately
does not echo validator text, and `value_error` is not in that map. Against the
running server, `{"identifier": 1, "untabbed_only": true, "tabs_only": true}`
comes back as `Validation error in get_dashboard_layout: request: Invalid
value; check the field schema`, and `tab: " "` as `request.tab: Invalid
value; check the field schema` rather than `tab must not be blank`. The
pre-existing `Provide identifier or permalink_key` behaves exactly the same
way, and `test_layout_rejects_untabbed_only_with_tab_options` asserts on the
`ValidationError` directly rather than through `Client`, which is why it passes
while the caller-visible text stays generic.
Since `_resolve_tab` already returns an actionable `DashboardError` for
`tab_not_found` and `ambiguous_tab`, would you rather do the exclusivity check
in `_scope_layout` and return a `DashboardError` that names the flag to drop?
Or do you read the field descriptions as carrying enough for this one?
##########
superset/mcp_service/dashboard/tool/get_dashboard_layout.py:
##########
@@ -39,13 +39,134 @@
dashboard_layout_serializer,
DashboardError,
DashboardLayout,
+ DashboardLayoutScope,
+ DashboardTab,
+ DashboardTabSummary,
GetDashboardLayoutRequest,
)
from superset.mcp_service.mcp_core import ModelGetInfoCore
logger = logging.getLogger(__name__)
+def _resolve_tab(
+ tabs: list[DashboardTab], selector: str
+) -> DashboardTab | DashboardError:
+ """Match a tab by ID first, then by exact title."""
+ if not tabs:
+ return DashboardError.create(
+ "This dashboard has no tabs. Omit tab to get the full layout.",
+ "tab_not_found",
+ )
+ matches = [tab for tab in tabs if tab.id == selector] or [
+ tab for tab in tabs if tab.name == selector
+ ]
+ if not matches:
+ return DashboardError.create(
+ "Tab not found. Use tabs_only=true to discover tab IDs and
titles.",
+ "tab_not_found",
+ )
+ if len(matches) > 1:
+ return DashboardError.create(
+ "Multiple tabs have that title. Use tabs_only=true to discover "
+ "their IDs, then pass a unique tab ID.",
+ "ambiguous_tab",
+ )
+ return matches[0]
+
+
+def _subtree_ids(tab_id: str, children: dict[str | None, list[str]]) ->
set[str]:
+ """Return a tab ID and the IDs of every tab nested under it."""
+ selected_ids: set[str] = set()
+ pending = [tab_id]
+ while pending:
+ current = pending.pop()
+ if current not in selected_ids:
+ selected_ids.add(current)
+ pending.extend(children.get(current, []))
+ return selected_ids
+
+
+def _tab_summaries(
+ tabs: list[DashboardTab], children: dict[str | None, list[str]]
+) -> list[DashboardTabSummary]:
+ """Summarize tabs with absolute depths from the full dashboard tree."""
+ # The parser only emits reachable tabs and assigns each tab a single
+ # enclosing parent, so walking from the top-level tabs reaches every tab.
+ depths: dict[str, int] = {}
+ stack = [(tab_id, 0) for tab_id in children.get(None, [])]
+ while stack:
+ tab_id, depth = stack.pop()
+ depths[tab_id] = depth
+ stack.extend((child, depth + 1) for child in children.get(tab_id, []))
+ return [
+ DashboardTabSummary(
+ id=tab.id,
+ name=tab.name,
+ parent_tab_id=tab.parent_tab_id,
+ depth=depths[tab.id],
+ chart_count=len(tab.chart_ids),
+ )
+ for tab in tabs
+ ]
+
+
+def _scope_layout(
+ layout: DashboardLayout, request: GetDashboardLayoutRequest
+) -> DashboardLayout | DashboardError:
+ """Project the parsed layout without changing permalink or ancestry
context.
+
+ Scoping only ever removes tabs and chart placements from the parsed layout,
+ so a scoped response never contains a chart the full layout would not.
+ """
+ if not request.tabs_only and request.tab is None and not
request.untabbed_only:
+ return layout
+
+ if request.untabbed_only:
+ return layout.model_copy(
+ update={
+ "tabs": [],
+ "charts": [chart for chart in layout.charts if chart.tab_id is
None],
+ "scope": DashboardLayoutScope(untabbed_only=True),
+ }
+ )
+
+ tabs = layout.tabs
+ children: dict[str | None, list[str]] = {}
+ for tab in tabs:
+ children.setdefault(tab.parent_tab_id, []).append(tab.id)
+
+ selected_tab_id: str | None = None
+ if request.tab is not None:
+ selected = _resolve_tab(tabs, request.tab)
+ if isinstance(selected, DashboardError):
+ return selected
+ selected_tab_id = selected.id
+ selected_ids = _subtree_ids(selected.id, children)
+ tabs = [tab for tab in tabs if tab.id in selected_ids]
+
+ scope = DashboardLayoutScope(tabs_only=request.tabs_only,
tab_id=selected_tab_id)
+ if request.tabs_only:
Review Comment:
Small asymmetry I noticed while walking the edge inputs. On a dashboard with
no tabs, `tab="Anything"` returns the very helpful `tab_not_found` with `This
dashboard has no tabs. Omit tab to get the full layout.`, but `tabs_only=true`
on that same dashboard returns a success payload with `tabs: []`, `tab_tree:
[]` and `charts: []`, so the only thing separating "this dashboard has no tabs"
from "this dashboard is empty" is `untabbed_chart_count`. I confirmed both
against a flat dashboard on the running server.
Not a blocker, the field descriptions do say `tabs_only` empties `tabs` and
`charts` by request. Would the same `DashboardError` shape, pointing at
`untabbed_only`, be friendlier than an all empty payload here?
##########
superset/mcp_service/dashboard/tool/get_dashboard_layout.py:
##########
@@ -39,13 +39,134 @@
dashboard_layout_serializer,
DashboardError,
DashboardLayout,
+ DashboardLayoutScope,
+ DashboardTab,
+ DashboardTabSummary,
GetDashboardLayoutRequest,
)
from superset.mcp_service.mcp_core import ModelGetInfoCore
logger = logging.getLogger(__name__)
+def _resolve_tab(
+ tabs: list[DashboardTab], selector: str
+) -> DashboardTab | DashboardError:
+ """Match a tab by ID first, then by exact title."""
+ if not tabs:
+ return DashboardError.create(
+ "This dashboard has no tabs. Omit tab to get the full layout.",
+ "tab_not_found",
+ )
+ matches = [tab for tab in tabs if tab.id == selector] or [
+ tab for tab in tabs if tab.name == selector
+ ]
+ if not matches:
+ return DashboardError.create(
+ "Tab not found. Use tabs_only=true to discover tab IDs and
titles.",
+ "tab_not_found",
+ )
+ if len(matches) > 1:
+ return DashboardError.create(
+ "Multiple tabs have that title. Use tabs_only=true to discover "
+ "their IDs, then pass a unique tab ID.",
+ "ambiguous_tab",
+ )
+ return matches[0]
+
+
+def _subtree_ids(tab_id: str, children: dict[str | None, list[str]]) ->
set[str]:
+ """Return a tab ID and the IDs of every tab nested under it."""
+ selected_ids: set[str] = set()
+ pending = [tab_id]
+ while pending:
+ current = pending.pop()
+ if current not in selected_ids:
+ selected_ids.add(current)
+ pending.extend(children.get(current, []))
+ return selected_ids
+
+
+def _tab_summaries(
+ tabs: list[DashboardTab], children: dict[str | None, list[str]]
+) -> list[DashboardTabSummary]:
+ """Summarize tabs with absolute depths from the full dashboard tree."""
+ # The parser only emits reachable tabs and assigns each tab a single
+ # enclosing parent, so walking from the top-level tabs reaches every tab.
+ depths: dict[str, int] = {}
+ stack = [(tab_id, 0) for tab_id in children.get(None, [])]
+ while stack:
+ tab_id, depth = stack.pop()
+ depths[tab_id] = depth
+ stack.extend((child, depth + 1) for child in children.get(tab_id, []))
+ return [
+ DashboardTabSummary(
+ id=tab.id,
+ name=tab.name,
+ parent_tab_id=tab.parent_tab_id,
+ depth=depths[tab.id],
+ chart_count=len(tab.chart_ids),
+ )
+ for tab in tabs
+ ]
+
+
+def _scope_layout(
+ layout: DashboardLayout, request: GetDashboardLayoutRequest
+) -> DashboardLayout | DashboardError:
+ """Project the parsed layout without changing permalink or ancestry
context.
+
+ Scoping only ever removes tabs and chart placements from the parsed layout,
+ so a scoped response never contains a chart the full layout would not.
+ """
+ if not request.tabs_only and request.tab is None and not
request.untabbed_only:
+ return layout
+
+ if request.untabbed_only:
+ return layout.model_copy(
+ update={
+ "tabs": [],
+ "charts": [chart for chart in layout.charts if chart.tab_id is
None],
+ "scope": DashboardLayoutScope(untabbed_only=True),
+ }
+ )
+
+ tabs = layout.tabs
+ children: dict[str | None, list[str]] = {}
+ for tab in tabs:
+ children.setdefault(tab.parent_tab_id, []).append(tab.id)
+
+ selected_tab_id: str | None = None
+ if request.tab is not None:
+ selected = _resolve_tab(tabs, request.tab)
+ if isinstance(selected, DashboardError):
+ return selected
+ selected_tab_id = selected.id
+ selected_ids = _subtree_ids(selected.id, children)
+ tabs = [tab for tab in tabs if tab.id in selected_ids]
+
+ scope = DashboardLayoutScope(tabs_only=request.tabs_only,
tab_id=selected_tab_id)
+ if request.tabs_only:
+ return layout.model_copy(
+ update={
+ "tabs": [],
+ "tab_tree": _tab_summaries(tabs, children),
+ "charts": [],
+ "scope": scope,
+ }
+ )
+
+ return layout.model_copy(
+ update={
+ "tabs": tabs,
+ "charts": [
+ chart for chart in layout.charts if chart.tab_id in
selected_ids
Review Comment:
Just a small NIT, and I know Bito asked for the opposite on the thread
above. `selected_ids` is now bound only inside `if request.tab is not None`, so
this line is correct only because the `tabs_only` branch returns above it and
the no-scope case returns at the top of the function. The empty set default
Bito objected to was misleading, but a meaningful one is not:
```python
selected_ids: set[str] = {tab.id for tab in tabs}
if request.tab is not None:
...
```
That keeps the "all tabs" reading if a future branch ever reaches this
return with `tab` unset, instead of an `UnboundLocalError`. Entirely your call.
--
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]
