codeant-ai-for-open-source[bot] commented on code in PR #41611: URL: https://github.com/apache/superset/pull/41611#discussion_r3531360297
########## superset/mcp_service/semantic_layer/tool/get_compatible_metrics.py: ########## @@ -0,0 +1,226 @@ +# 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. + +"""MCP tool: get_compatible_metrics + +Returns metrics compatible with the current dimension/metric selection. +""" + +import logging + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.privacy import ( + DATA_MODEL_METADATA_ERROR_TYPE, + requires_data_model_metadata_access, + user_can_view_data_model_metadata, +) +from superset.mcp_service.semantic_layer.schemas import ( + CompatibleMetricsResponse, + GetCompatibleMetricsRequest, + MetricInfo, + SemanticLayerError, +) + +logger = logging.getLogger(__name__) + + +@tool( + tags=["data", "semantic"], + class_permission_name="Dataset", + annotations=ToolAnnotations( + title="Get compatible metrics", + readOnlyHint=True, + destructiveHint=False, + ), +) +@requires_data_model_metadata_access +async def get_compatible_metrics( + request: GetCompatibleMetricsRequest, + ctx: Context, +) -> CompatibleMetricsResponse | SemanticLayerError: + """Return metrics compatible with the current dimension/metric selection. + + Used to progressively refine a query: given a set of already-selected + metrics and dimensions, returns the additional metrics that can be + combined without breaking the underlying semantic constraints. + + Provide exactly one of ``dataset_id`` (built-in) or ``view_id`` (external). + + For built-in datasets, all metrics from the dataset are considered + compatible (SQL GROUP BY imposes no metric-level constraints). + + For external semantic views, delegates to the view's + ``get_compatible_metrics`` implementation. + + Example: + ```json + { + "selected_metrics": [], + "selected_dimensions": ["region"], + "view_id": 5 + } + ``` + """ + await ctx.info( + "Getting compatible metrics: dataset_id=%s, view_id=%s, " + "metrics=%s, dims=%s" + % ( + request.dataset_id, + request.view_id, + request.selected_metrics, + request.selected_dimensions, + ) + ) + + if not user_can_view_data_model_metadata(): + return SemanticLayerError.create( + error="You don't have permission to access dataset details for your role.", + error_type=DATA_MODEL_METADATA_ERROR_TYPE, + ) + + if request.dataset_id is None and request.view_id is None: + return SemanticLayerError.create( + error="Provide either dataset_id (built-in) or view_id (external).", + error_type="ValidationError", + ) + if request.dataset_id is not None and request.view_id is not None: + return SemanticLayerError.create( + error="Provide only one of dataset_id or view_id, not both.", + error_type="ValidationError", + ) + + try: + # ------------------------------------------------------------------ + # Built-in dataset path + # ------------------------------------------------------------------ + if request.dataset_id is not None: + from sqlalchemy.orm import subqueryload + + from superset.connectors.sqla.models import SqlaTable + from superset.daos.dataset import DatasetDAO + + with event_logger.log_context(action="mcp.get_compatible_metrics.builtin"): + dataset = DatasetDAO.find_by_id( + request.dataset_id, + query_options=[ + subqueryload(SqlaTable.columns), + subqueryload(SqlaTable.metrics), + ], + ) + + if dataset is None: + return SemanticLayerError.create( + error=f"No dataset found with id: {request.dataset_id}.", + error_type="NotFound", + ) + + # All metrics on a SQL dataset are always mutually compatible. + compatible = [ + MetricInfo( + name=m.metric_name, + verbose_name=m.verbose_name or None, + description=m.description or None, + expression=m.expression or None, + d3format=m.d3format or None, + warning_text=m.warning_text or None, + source="builtin", + dataset_id=dataset.id, + dataset_name=dataset.table_name, + ) + for m in dataset.metrics + ] Review Comment: **Suggestion:** The built-in path returns every metric from the dataset, including metrics already present in `selected_metrics`. This breaks the tool contract of returning metrics that can be added next, and can cause duplicate metric suggestions/selections in clients. Filter out already-selected names before building the response. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ MCP get_compatible_metrics returns duplicate already-selected metrics. - ⚠️ LLM clients may suggest redundant metric additions. - ⚠️ Confuses progressive query-building based on compatible metrics. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Invoke the MCP tool get_compatible_metrics defined in superset/mcp_service/semantic_layer/tool/get_compatible_metrics.py:44-57 with a GetCompatibleMetricsRequest whose dataset_id is set and selected_metrics contains one or more metric names from that dataset. 2. Because request.dataset_id is not None and request.view_id is None, execution follows the built-in dataset path (get_compatible_metrics.py:113-155), loading the SqlaTable via DatasetDAO.find_by_id() and accessing dataset.metrics. 3. The compatible list is built at get_compatible_metrics.py:134-148 by iterating over all metrics in dataset.metrics without filtering out names already present in request.selected_metrics, even though GetCompatibleMetricsRequest documents selected_metrics as "Metric names already selected" in superset/mcp_service/semantic_layer/schemas.py:121-124. 4. The function returns CompatibleMetricsResponse(compatible_metrics=compatible, source="builtin") at get_compatible_metrics.py:150-154, so the client receives metrics that are already selected, contradicting the tool docstring at lines 58-62 which says it should return additional metrics that can be combined next. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=002c9720b7e549e7bd58cafc3545e1af&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=002c9720b7e549e7bd58cafc3545e1af&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/semantic_layer/tool/get_compatible_metrics.py **Line:** 135:148 **Comment:** *Logic Error: The built-in path returns every metric from the dataset, including metrics already present in `selected_metrics`. This breaks the tool contract of returning metrics that can be added next, and can cause duplicate metric suggestions/selections in clients. Filter out already-selected names before building the response. 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%2F41611&comment_hash=29e2ccdd224a6ab80b8aa69afa885115b72edb32f257ebf2fbe8ae1bd780ac6b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41611&comment_hash=29e2ccdd224a6ab80b8aa69afa885115b72edb32f257ebf2fbe8ae1bd780ac6b&reaction=dislike'>👎</a> ########## superset/daos/semantic_layer.py: ########## @@ -201,6 +201,29 @@ def validate_update_uniqueness( for c in candidates ) + @classmethod + def find_accessible(cls) -> list[SemanticView]: + """Return all views the current user can access, filtered at SQL level. + + Mirrors the permission filter in ``DatasourceDAO.build_semantic_view_query`` + to avoid per-row Python-level access checks when listing all views. + """ + from sqlalchemy import or_ + + query = db.session.query(SemanticView) + if not security_manager.can_access_all_datasources(): + perms = security_manager.user_view_menu_names("datasource_access") + query = query.join( + SemanticLayer, + SemanticLayer.uuid == SemanticView.semantic_layer_uuid, + ).filter( + or_( + SemanticView.perm.in_(perms), + SemanticLayer.perm.in_(perms), + ) + ) + return query.all() Review Comment: **Suggestion:** This query joins `SemanticLayer` only for filtering but does not eager-load the relationship, while downstream callers invoke `view.raise_for_access()` per row and read `view.semantic_layer.perm`. That pattern triggers an extra lazy-load query per view (N+1) for layer-permission checks. Eager-load `SemanticView.semantic_layer` in this DAO method (or remove redundant per-row access checks in callers) to avoid repeated DB round-trips. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ MCP list_metrics tool slower with many semantic views. - ⚠️ Extra DB queries when scanning external semantic view metrics. - ⚠️ Increased DB load during semantic layer exploration workflows. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Call the MCP tool list_metrics defined in superset/mcp_service/semantic_layer/tool/list_metrics.py:201-214 without a specific view_id so that _collect_external_metrics() is used (same file, lines 137-198). 2. Inside _collect_external_metrics(), when request.view_id is None, SemanticViewDAO.find_accessible() is invoked (list_metrics.py:143-149), which executes the SQL in SemanticViewDAO.find_accessible() at superset/daos/semantic_layer.py:205-225 to load all accessible SemanticView rows, joining SemanticLayer only for filtering. 3. For each returned view, _collect_external_metrics() calls view.raise_for_access() (list_metrics.py:155-157); SemanticView.raise_for_access() in superset/semantic_layers/models.py:483-499 reads self.semantic_layer.perm, causing SQLAlchemy to lazily load the semantic_layer relationship for each view because find_accessible() did not eager-load it. 4. Still inside the same loop, _collect_external_metrics() accesses view.metrics and view.columns (list_metrics.py:159-160), and SemanticView.implementation in superset/semantic_layers/models.py:110-118 again dereferences self.semantic_layer, reinforcing the N+1 pattern where each of N views triggers at least one additional query against semantic_layers. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9c859c38ae6f45bdbbada48f6067de62&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=9c859c38ae6f45bdbbada48f6067de62&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/daos/semantic_layer.py **Line:** 213:225 **Comment:** *Performance: This query joins `SemanticLayer` only for filtering but does not eager-load the relationship, while downstream callers invoke `view.raise_for_access()` per row and read `view.semantic_layer.perm`. That pattern triggers an extra lazy-load query per view (N+1) for layer-permission checks. Eager-load `SemanticView.semantic_layer` in this DAO method (or remove redundant per-row access checks in callers) to avoid repeated DB round-trips. 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%2F41611&comment_hash=44762d466839632ea36ad2a457aebe46a06abc280bdc5c818e0339894f7f8499&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41611&comment_hash=44762d466839632ea36ad2a457aebe46a06abc280bdc5c818e0339894f7f8499&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]
