codeant-ai-for-open-source[bot] commented on code in PR #40961: URL: https://github.com/apache/superset/pull/40961#discussion_r3482172914
########## superset/mcp_service/dashboard/tool/get_dashboard_datasets.py: ########## @@ -0,0 +1,128 @@ +# 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 datasets FastMCP tool + +Returns the datasets used by a dashboard's charts, including columns and +metrics. This is the prerequisite context an agent needs before configuring +native filters on a dashboard (e.g. picking filter target columns). +""" + +import logging +from datetime import datetime, timezone + +from fastmcp import Context +from sqlalchemy.orm import subqueryload +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + dashboard_datasets_serializer, + DashboardDatasets, + DashboardError, + GetDashboardDatasetsRequest, +) +from superset.mcp_service.mcp_core import ModelGetInfoCore + +logger = logging.getLogger(__name__) + + +@tool( + tags=["core"], + class_permission_name="Dashboard", + annotations=ToolAnnotations( + title="Get dashboard datasets", + readOnlyHint=True, + destructiveHint=False, + ), +) +async def get_dashboard_datasets( + request: GetDashboardDatasetsRequest, ctx: Context +) -> DashboardDatasets | DashboardError: Review Comment: **Suggestion:** This tool exposes dataset/database metadata (columns, metrics, schema, backend) but is missing the data-model metadata privacy gate used by dataset tools. Users with dashboard read access but without dataset metadata permissions can still call this tool and retrieve restricted metadata. Add the `@requires_data_model_metadata_access` decorator (and import it) so visibility and execution are blocked consistently with other metadata tools. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Dashboard MCP tool leaks dataset columns/metrics to restricted roles. - ⚠️ Data-model metadata privacy controls inconsistently applied across tools. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Inspect the MCP dashboard datasets tool in `superset/mcp_service/dashboard/tool/get_dashboard_datasets.py:45-56` where `get_dashboard_datasets` is declared with `@tool(tags=["core"], class_permission_name="Dashboard", ...)` but without `@requires_data_model_metadata_access`. 2. Inspect dataset metadata tools in `superset/mcp_service/dataset/tool/get_dataset_info.py:50-60`, `list_datasets.py:80-90`, and `query_dataset.py:45-49,103-113` which all import `requires_data_model_metadata_access` from `superset.mcp_service.privacy` and decorate their tool functions, and additionally check `user_can_view_data_model_metadata()` inside the body. 3. Inspect `superset/mcp_service/privacy.py:110-143`, where `requires_data_model_metadata_access` sets the `_requires_data_model_metadata_access` attribute and `user_can_view_data_model_metadata()` returns True only when the current user has Dataset metadata permissions (`can_get_drill_info`, `can_get_or_create_dataset`, or `can_write` on `Dataset`), intentionally excluding pure dashboard viewers. 4. Inspect MCP auth visibility logic in `superset/mcp_service/auth.py:131-160`, where `is_tool_visible_to_current_user()` hides tools when `tool_requires_data_model_metadata_access(tool.fn)` is True and `user_can_view_data_model_metadata()` is False; because `get_dashboard_datasets` lacks the decorator, `tool_requires_data_model_metadata_access` returns False, so any user with `Dashboard` read permission can discover and call `get_dashboard_datasets`, and the serializer `dashboard_datasets_serializer` in `superset/mcp_service/dashboard/schemas.py:140-188` returns dataset columns, metrics, and database metadata even when `user_can_view_data_model_metadata()` would be False, leaking restricted metadata to dashboard-only viewers. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=add09c3487a640ba94753394c700619e&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=add09c3487a640ba94753394c700619e&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/get_dashboard_datasets.py **Line:** 45:56 **Comment:** *Security: This tool exposes dataset/database metadata (columns, metrics, schema, backend) but is missing the data-model metadata privacy gate used by dataset tools. Users with dashboard read access but without dataset metadata permissions can still call this tool and retrieve restricted metadata. Add the `@requires_data_model_metadata_access` decorator (and import it) so visibility and execution are blocked consistently with other metadata tools. 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%2F40961&comment_hash=561a48779b725b7bc2d3101730de7eb7f8ddca979f34bcf8935b3fc000e238f0&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40961&comment_hash=561a48779b725b7bc2d3101730de7eb7f8ddca979f34bcf8935b3fc000e238f0&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/get_dashboard_datasets.py: ########## @@ -0,0 +1,128 @@ +# 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 datasets FastMCP tool + +Returns the datasets used by a dashboard's charts, including columns and +metrics. This is the prerequisite context an agent needs before configuring +native filters on a dashboard (e.g. picking filter target columns). +""" + +import logging +from datetime import datetime, timezone + +from fastmcp import Context +from sqlalchemy.orm import subqueryload +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + dashboard_datasets_serializer, + DashboardDatasets, + DashboardError, + GetDashboardDatasetsRequest, +) +from superset.mcp_service.mcp_core import ModelGetInfoCore + +logger = logging.getLogger(__name__) + + +@tool( + tags=["core"], + class_permission_name="Dashboard", + annotations=ToolAnnotations( + title="Get dashboard datasets", + readOnlyHint=True, + destructiveHint=False, + ), +) +async def get_dashboard_datasets( + request: GetDashboardDatasetsRequest, ctx: Context +) -> DashboardDatasets | DashboardError: + """ + List the datasets used by a dashboard's charts, by ID, UUID, or slug. + + Each dataset includes its table name, schema, database connection + (id, name, backend), columns (name, type, is_dttm, verbose_name) and + metrics (name, expression, verbose_name). Use this to understand which + columns and metrics are available before configuring native filters or + analyzing a dashboard's data model. + + Datasets the current user cannot access are excluded from the response + and reported via inaccessible_dataset_count. Column and metric lists are + capped per dataset; when truncated, columns_truncated/metrics_truncated + are set and total counts are reported. + + Example usage: + ```json + { + "identifier": 123 + } + ``` + """ + await ctx.info( + "Retrieving dashboard datasets: identifier=%s" % (request.identifier,) + ) + + try: + from superset.daos.dashboard import DashboardDAO + from superset.models.dashboard import Dashboard + + # Eager load slices to avoid N+1 queries when grouping by datasource. + eager_options = [subqueryload(Dashboard.slices)] Review Comment: **Suggestion:** The eager loading only preloads `Dashboard.slices`, but serialization accesses each slice datasource plus dataset `columns`, `metrics`, and `database`. Those relationships will lazy-load per dataset, causing N+1 query amplification on dashboards with many datasets/charts and potentially large latency spikes/timeouts. Eager-load nested relationships (`Dashboard.slices -> Slice.table -> SqlaTable.columns/metrics/database`) in this query. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ get_dashboard_datasets issues N+1 queries for each dataset. - ⚠️ Large dashboards may experience slow MCP responses or timeouts. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Inspect the tool implementation in `superset/mcp_service/dashboard/tool/get_dashboard_datasets.py:82-99` where `get_dashboard_datasets` builds a `ModelGetInfoCore` with `query_options = [subqueryload(Dashboard.slices)]` and calls `core.run_tool(request.identifier)` to load the dashboard and its charts. 2. Inspect `ModelGetInfoCore._find_object()` and `_base_filtered_query()` in `superset/mcp_service/mcp_core.py:166-184,247-258,520-33` which apply `self.query_options` only at the Dashboard level; no eager options are added for `Slice.table`/`Slice.datasource` or for dataset relationships like `SqlaTable.columns`, `SqlaTable.metrics`, and `SqlaTable.database`. 3. Inspect `dashboard_datasets_serializer` in `superset/mcp_service/dashboard/schemas.py:140-188`, which groups `dashboard.slices` by `datasource_id`, finds each `datasource` via `slc.datasource`, and calls `_serialize_dashboard_dataset(datasource, len(slices))` for each dataset used on the dashboard. 4. Inspect `_serialize_dashboard_dataset` in `superset/mcp_service/dashboard/schemas.py:70-137`, which accesses `datasource.columns`, `datasource.metrics`, and `datasource.database` to build `DashboardDatasetSummary`; because no eager loading is configured for these relationships (contrast with `get_dataset_info` in `superset/mcp_service/dataset/tool/get_dataset_info.py:115-122` which uses `subqueryload(SqlaTable.columns)`, `subqueryload(SqlaTable.metrics)`, and `joinedload(SqlaTable.database)` specifically to avoid N+1), SQLAlchemy lazily issues additional queries per dataset when `get_dashboard_datasets` runs on dashboards with many datasets, amplifying to N+1 query patterns and causing slow responses or timeouts under load. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d4f3a8f40ad3486ca549e2c8208f4a4c&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=d4f3a8f40ad3486ca549e2c8208f4a4c&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/get_dashboard_datasets.py **Line:** 86:87 **Comment:** *Performance: The eager loading only preloads `Dashboard.slices`, but serialization accesses each slice datasource plus dataset `columns`, `metrics`, and `database`. Those relationships will lazy-load per dataset, causing N+1 query amplification on dashboards with many datasets/charts and potentially large latency spikes/timeouts. Eager-load nested relationships (`Dashboard.slices -> Slice.table -> SqlaTable.columns/metrics/database`) in this query. 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%2F40961&comment_hash=71dd102c75629b14c1c0c8b4da0ae31694c2f8357f4c364ab15fdf9e60da5243&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40961&comment_hash=71dd102c75629b14c1c0c8b4da0ae31694c2f8357f4c364ab15fdf9e60da5243&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]
