EnxDev commented on code in PR #44113: URL: https://github.com/apache/superset/pull/44113#discussion_r3976777046
########## superset/mcp_service/dashboard/tool/get_dashboard_data.py: ########## @@ -0,0 +1,198 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Get dashboard data FastMCP tool + +Returns a compact, filter-aware summary of the underlying data across a +dashboard's charts, so an agent can answer analytical questions about the whole +dashboard from a single call instead of fetching each chart separately. +""" + +import logging +from datetime import datetime, timezone +from time import monotonic +from typing import TYPE_CHECKING + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.chart.schemas import ( + ChartData, + ChartError, + GetChartDataRequest, +) +from superset.mcp_service.chart.tool.get_chart_data import execute_chart_data +from superset.mcp_service.dashboard.schemas import ( + _extract_layout_from_position, + DashboardChartData, + DashboardData, + DashboardError, + GetDashboardDataRequest, +) + +if TYPE_CHECKING: + from superset.models.slice import Slice + +logger = logging.getLogger(__name__) + + +def _order_by_layout(slices: list["Slice"], position_json: str | None) -> list["Slice"]: + """Order slices by their layout reading order so a bounded selection covers + the most prominent charts first; charts absent from the layout keep their + original order.""" + _, positions = _extract_layout_from_position(position_json) + order = { + position.chart_id: index + for index, position in enumerate(positions) + if position.chart_id is not None + } + return sorted(slices, key=lambda slc: order.get(slc.id, len(order))) + + +@tool( + tags=["data"], + class_permission_name="Dashboard", + annotations=ToolAnnotations( + title="Get dashboard data", + readOnlyHint=True, + destructiveHint=False, + openWorldHint=False, + ), +) +async def get_dashboard_data( + request: GetDashboardDataRequest, ctx: Context +) -> DashboardData | DashboardError: + """Get bounded, filter-aware data across a dashboard's charts. + + For each chart (up to max_charts, selected in dashboard layout order), + returns a compact view of the underlying data: column names, a few sample + rows, and row counts. Active dashboard filters are applied per chart via + applied_filters (keyed by chart id, each value extra_form_data). Use this to + answer analytical questions about a whole dashboard in one call instead of + calling get_chart_data for every chart. + + Example: + ```json + { + "identifier": 6, + "applied_filters": { + "56": {"filters": [{"col": "country", "op": "IN", "val": ["US"]}]} + } + } + ``` + """ + await ctx.info( + "Retrieving dashboard data: identifier=%s, max_charts=%s" + % (request.identifier, request.max_charts) + ) + + try: + from superset.daos.dashboard import DashboardDAO + + with event_logger.log_context(action="mcp.get_dashboard_data.lookup"): + dashboard = DashboardDAO.get_by_id_or_slug(str(request.identifier)) + except Exception as exc: # noqa: BLE001 + await ctx.warning( + "Dashboard not found or inaccessible: identifier=%s, error=%s" + % (request.identifier, str(exc)) + ) + return DashboardError( + error=f"Dashboard not found or inaccessible: {request.identifier}", + error_type="DashboardNotFound", + timestamp=datetime.now(timezone.utc), + ) + + slices = list(dashboard.slices or []) + applied_filters = request.applied_filters or {} + selected = _order_by_layout(slices, dashboard.position_json)[: request.max_charts] + + charts: list[DashboardChartData] = [] + deadline = monotonic() + request.time_budget_seconds + for slc in selected: + # `charts` guards the budget so at least one chart is always attempted. + if charts and monotonic() >= deadline: + await ctx.warning( + "Dashboard data time budget reached after %s charts" % len(charts) + ) + break + extra = applied_filters.get(str(slc.id)) + try: + chart_request = GetChartDataRequest( + identifier=slc.id, + extra_form_data=extra or None, + limit=request.fetch_row_limit, + ) + # Reuse get_chart_data's core so guest auth + filter handling match. + result = await execute_chart_data(chart_request, ctx) Review Comment: **[P1] Preserve the Chart permission boundary when composing the tools.** Calling the undecorated core skips `get_chart_data`’s `@tool(class_permission_name="Chart")` gate, so this path checks only Dashboard read permission/scope before executing chart queries. A concrete affected principal is a custom role granted `can_read` on Dashboard plus datasource access, but not `can_read` on Chart (or the same role using a token scoped only to `superset:dashboard:read`): this grants the custom-role capability matrix row more than its explicit grants, while the REST chart-data route and the standalone MCP tool both require Chart read. Dataset access and RLS still run, but they don’t replace that route-level capability. Could we add a shared authorization check for Chart RBAC *and* token scope before entering this core, with a regression for that principal? The explicitly allow-listed embedded-guest path can remain supported. ########## superset/mcp_service/dashboard/tool/get_dashboard_data.py: ########## @@ -0,0 +1,198 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Get dashboard data FastMCP tool + +Returns a compact, filter-aware summary of the underlying data across a +dashboard's charts, so an agent can answer analytical questions about the whole +dashboard from a single call instead of fetching each chart separately. +""" + +import logging +from datetime import datetime, timezone +from time import monotonic +from typing import TYPE_CHECKING + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.chart.schemas import ( + ChartData, + ChartError, + GetChartDataRequest, +) +from superset.mcp_service.chart.tool.get_chart_data import execute_chart_data +from superset.mcp_service.dashboard.schemas import ( + _extract_layout_from_position, + DashboardChartData, + DashboardData, + DashboardError, + GetDashboardDataRequest, +) + +if TYPE_CHECKING: + from superset.models.slice import Slice + +logger = logging.getLogger(__name__) + + +def _order_by_layout(slices: list["Slice"], position_json: str | None) -> list["Slice"]: + """Order slices by their layout reading order so a bounded selection covers + the most prominent charts first; charts absent from the layout keep their + original order.""" + _, positions = _extract_layout_from_position(position_json) + order = { + position.chart_id: index + for index, position in enumerate(positions) + if position.chart_id is not None + } + return sorted(slices, key=lambda slc: order.get(slc.id, len(order))) + + +@tool( + tags=["data"], + class_permission_name="Dashboard", + annotations=ToolAnnotations( + title="Get dashboard data", + readOnlyHint=True, + destructiveHint=False, + openWorldHint=False, + ), +) +async def get_dashboard_data( + request: GetDashboardDataRequest, ctx: Context +) -> DashboardData | DashboardError: + """Get bounded, filter-aware data across a dashboard's charts. + + For each chart (up to max_charts, selected in dashboard layout order), + returns a compact view of the underlying data: column names, a few sample + rows, and row counts. Active dashboard filters are applied per chart via + applied_filters (keyed by chart id, each value extra_form_data). Use this to + answer analytical questions about a whole dashboard in one call instead of + calling get_chart_data for every chart. + + Example: + ```json + { + "identifier": 6, + "applied_filters": { + "56": {"filters": [{"col": "country", "op": "IN", "val": ["US"]}]} + } + } + ``` + """ + await ctx.info( + "Retrieving dashboard data: identifier=%s, max_charts=%s" + % (request.identifier, request.max_charts) + ) + + try: + from superset.daos.dashboard import DashboardDAO + + with event_logger.log_context(action="mcp.get_dashboard_data.lookup"): + dashboard = DashboardDAO.get_by_id_or_slug(str(request.identifier)) + except Exception as exc: # noqa: BLE001 + await ctx.warning( + "Dashboard not found or inaccessible: identifier=%s, error=%s" + % (request.identifier, str(exc)) + ) + return DashboardError( + error=f"Dashboard not found or inaccessible: {request.identifier}", + error_type="DashboardNotFound", + timestamp=datetime.now(timezone.utc), + ) + + slices = list(dashboard.slices or []) + applied_filters = request.applied_filters or {} + selected = _order_by_layout(slices, dashboard.position_json)[: request.max_charts] + + charts: list[DashboardChartData] = [] + deadline = monotonic() + request.time_budget_seconds + for slc in selected: + # `charts` guards the budget so at least one chart is always attempted. + if charts and monotonic() >= deadline: + await ctx.warning( + "Dashboard data time budget reached after %s charts" % len(charts) + ) + break + extra = applied_filters.get(str(slc.id)) + try: + chart_request = GetChartDataRequest( + identifier=slc.id, + extra_form_data=extra or None, + limit=request.fetch_row_limit, + ) + # Reuse get_chart_data's core so guest auth + filter handling match. + result = await execute_chart_data(chart_request, ctx) + except Exception as exc: # noqa: BLE001 + await ctx.warning( + "Chart data failed: chart_id=%s, error=%s" % (slc.id, str(exc)) + ) + charts.append( + DashboardChartData( + chart_id=slc.id, + chart_name=slc.slice_name or "", + chart_type=slc.viz_type or "unknown", + filtered=bool(extra), + error=str(exc), + ) + ) + continue + + if isinstance(result, ChartData): + charts.append( + DashboardChartData( + chart_id=result.chart_id, + chart_name=result.chart_name, + chart_type=result.chart_type, + columns=[column.name for column in result.columns], + sample_data=result.data[: request.sample_rows], Review Comment: **[P1] Keep every query layer in the dashboard summary.** `ChartData.data` and `columns` are intentionally backward-compatible aliases for only the first query; `result.query_results` carries all layers for multi-query charts such as Mixed Timeseries. Dropping it here makes a common chart incomplete, and if its first layer is empty while the second has data, this response reports empty columns/sample data with no error. Could `DashboardChartData` preserve a bounded sample per query (or otherwise combine the layers without losing their identity) and add a two-query regression, including the empty-first/nonempty-second case? ########## superset/mcp_service/dashboard/tool/get_dashboard_data.py: ########## @@ -0,0 +1,198 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Get dashboard data FastMCP tool + +Returns a compact, filter-aware summary of the underlying data across a +dashboard's charts, so an agent can answer analytical questions about the whole +dashboard from a single call instead of fetching each chart separately. +""" + +import logging +from datetime import datetime, timezone +from time import monotonic +from typing import TYPE_CHECKING + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.chart.schemas import ( + ChartData, + ChartError, + GetChartDataRequest, +) +from superset.mcp_service.chart.tool.get_chart_data import execute_chart_data +from superset.mcp_service.dashboard.schemas import ( + _extract_layout_from_position, + DashboardChartData, + DashboardData, + DashboardError, + GetDashboardDataRequest, +) + +if TYPE_CHECKING: + from superset.models.slice import Slice + +logger = logging.getLogger(__name__) + + +def _order_by_layout(slices: list["Slice"], position_json: str | None) -> list["Slice"]: + """Order slices by their layout reading order so a bounded selection covers + the most prominent charts first; charts absent from the layout keep their + original order.""" + _, positions = _extract_layout_from_position(position_json) + order = { + position.chart_id: index + for index, position in enumerate(positions) + if position.chart_id is not None + } + return sorted(slices, key=lambda slc: order.get(slc.id, len(order))) + + +@tool( + tags=["data"], + class_permission_name="Dashboard", + annotations=ToolAnnotations( + title="Get dashboard data", + readOnlyHint=True, + destructiveHint=False, + openWorldHint=False, + ), +) +async def get_dashboard_data( + request: GetDashboardDataRequest, ctx: Context +) -> DashboardData | DashboardError: + """Get bounded, filter-aware data across a dashboard's charts. + + For each chart (up to max_charts, selected in dashboard layout order), + returns a compact view of the underlying data: column names, a few sample + rows, and row counts. Active dashboard filters are applied per chart via + applied_filters (keyed by chart id, each value extra_form_data). Use this to + answer analytical questions about a whole dashboard in one call instead of + calling get_chart_data for every chart. + + Example: + ```json + { + "identifier": 6, + "applied_filters": { + "56": {"filters": [{"col": "country", "op": "IN", "val": ["US"]}]} + } + } + ``` + """ + await ctx.info( + "Retrieving dashboard data: identifier=%s, max_charts=%s" + % (request.identifier, request.max_charts) + ) + + try: + from superset.daos.dashboard import DashboardDAO + + with event_logger.log_context(action="mcp.get_dashboard_data.lookup"): + dashboard = DashboardDAO.get_by_id_or_slug(str(request.identifier)) + except Exception as exc: # noqa: BLE001 + await ctx.warning( + "Dashboard not found or inaccessible: identifier=%s, error=%s" + % (request.identifier, str(exc)) + ) + return DashboardError( + error=f"Dashboard not found or inaccessible: {request.identifier}", + error_type="DashboardNotFound", + timestamp=datetime.now(timezone.utc), + ) + + slices = list(dashboard.slices or []) + applied_filters = request.applied_filters or {} + selected = _order_by_layout(slices, dashboard.position_json)[: request.max_charts] + + charts: list[DashboardChartData] = [] + deadline = monotonic() + request.time_budget_seconds + for slc in selected: + # `charts` guards the budget so at least one chart is always attempted. + if charts and monotonic() >= deadline: + await ctx.warning( + "Dashboard data time budget reached after %s charts" % len(charts) + ) + break + extra = applied_filters.get(str(slc.id)) + try: + chart_request = GetChartDataRequest( + identifier=slc.id, + extra_form_data=extra or None, + limit=request.fetch_row_limit, + ) + # Reuse get_chart_data's core so guest auth + filter handling match. + result = await execute_chart_data(chart_request, ctx) + except Exception as exc: # noqa: BLE001 + await ctx.warning( + "Chart data failed: chart_id=%s, error=%s" % (slc.id, str(exc)) + ) + charts.append( + DashboardChartData( + chart_id=slc.id, + chart_name=slc.slice_name or "", + chart_type=slc.viz_type or "unknown", + filtered=bool(extra), + error=str(exc), + ) + ) + continue + + if isinstance(result, ChartData): + charts.append( + DashboardChartData( + chart_id=result.chart_id, + chart_name=result.chart_name, + chart_type=result.chart_type, + columns=[column.name for column in result.columns], + sample_data=result.data[: request.sample_rows], + row_count=result.row_count, + total_rows=result.total_rows, Review Comment: **[P1] Avoid presenting the fetch cap as the full available row count.** `fetch_row_limit` is written into each query’s `row_limit`, and `QueryContextProcessor` sets the upstream `rowcount` to `len(cache.df.index)`. Consequently, a chart with more than the default 100 result rows returns both `row_count=100` and `total_rows=100`, even though the new schema promises that `total_rows` is the full available count. That can make an analytical answer confidently undercount. Could we either obtain a real total, or describe this as a capped count and expose an explicit `truncated`/unknown signal? A regression with more rows than `fetch_row_limit` would pin the intended contract. -- 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]
