codeant-ai-for-open-source[bot] commented on code in PR #41611: URL: https://github.com/apache/superset/pull/41611#discussion_r3531357399
########## superset/mcp_service/semantic_layer/tool/list_metrics.py: ########## @@ -0,0 +1,294 @@ +# 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: list_metrics + +Unified metric discovery across built-in datasets and external semantic views. +""" + +import logging +from typing import Any + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.daos.dataset import DatasetDAO +from superset.daos.semantic_layer import SemanticViewDAO +from superset.extensions import db, 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 ( + DimensionInfo, + ListMetricsRequest, + MetricInfo, + MetricList, + SemanticLayerError, +) + +logger = logging.getLogger(__name__) + + +def _matches_search(text: str | None, search: str) -> bool: + if not text: + return False + return search.lower() in text.lower() + + +def _builtin_compatible_dims(dataset: Any) -> list[DimensionInfo]: + """Return groupby-enabled columns as compatible dimensions for a built-in metric.""" + return [ + DimensionInfo( + name=col.column_name, + verbose_name=col.verbose_name or None, + description=col.description or None, + type=col.type or None, + is_dttm=bool(col.is_dttm), + groupby=bool(col.groupby), + filterable=bool(col.filterable), + source="builtin", + ) + for col in dataset.columns + if col.groupby + ] + + +def _collect_builtin_metrics(request: ListMetricsRequest) -> list[MetricInfo]: + """Collect metrics from built-in SqlaTable datasets.""" + from sqlalchemy.orm import subqueryload + + from superset.connectors.sqla.models import SqlaTable + + with event_logger.log_context(action="mcp.list_metrics.builtin_query"): + if request.dataset_id is not None: + from sqlalchemy.orm import subqueryload + + from superset.connectors.sqla.models import SqlaTable + + dataset = DatasetDAO.find_by_id( + request.dataset_id, + query_options=[ + subqueryload(SqlaTable.columns), + subqueryload(SqlaTable.metrics), + ], + ) + datasets = [dataset] if dataset else [] + else: + from sqlalchemy.orm import subqueryload + + from superset.connectors.sqla.models import SqlaTable + + # Use _apply_base_filter with explicit eager loading to avoid + # N+1 queries when iterating dataset.metrics / dataset.columns. + query = db.session.query(SqlaTable).options( + subqueryload(SqlaTable.columns), + subqueryload(SqlaTable.metrics), + ) + datasets = DatasetDAO._apply_base_filter(query).all() + + results: list[MetricInfo] = [] + for dataset in datasets: + compat_dims = ( + _builtin_compatible_dims(dataset) + if request.include_compatible_dimensions + else [] + ) + for metric in dataset.metrics: + name = metric.metric_name or "" + desc = metric.description or "" + if request.search and not ( + _matches_search(name, request.search) + or _matches_search(desc, request.search) + ): + continue + results.append( + MetricInfo( + name=name, + verbose_name=metric.verbose_name or None, + description=desc or None, + expression=metric.expression or None, + d3format=metric.d3format or None, + warning_text=metric.warning_text or None, + source="builtin", + dataset_id=dataset.id, + dataset_name=dataset.table_name, + compatible_dimensions=compat_dims, + ) + ) + return results + + +async def _collect_external_metrics( + request: ListMetricsRequest, + ctx: Context, +) -> list[MetricInfo]: + """Collect metrics from external SemanticView models.""" + with event_logger.log_context(action="mcp.list_metrics.external_query"): + if request.view_id is not None: + view = SemanticViewDAO.find_by_id(request.view_id) + views = [view] if view else [] + else: + # find_accessible filters at SQL level, avoiding a per-row + # Python permission check and the audit noise of raise_for_access. + views = SemanticViewDAO.find_accessible() + + await ctx.debug("Found %d semantic views to scan for metrics" % len(views)) + + results: list[MetricInfo] = [] + for view in views: + # raise_for_access must be called outside the broad except block below + # so that auth errors are never silently swallowed. + view.raise_for_access() + try: + raw_metrics = view.metrics Review Comment: **Suggestion:** Access-control failures from external views are not handled as a first-class error type. `raise_for_access()` runs outside the per-view try/except, so a denied view can bubble up to the top-level generic handler and be returned as `InternalError` instead of `AccessDenied`, which breaks the semantic-layer tool contract used by related tools. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ list_metrics returns InternalError for forbidden external semantic views. - ⚠️ MCP clients cannot distinguish permission issues from server failures. - ⚠️ Error contract inconsistent with other semantic-layer MCP tools. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Invoke the MCP `list_metrics` tool entrypoint at `superset/mcp_service/semantic_layer/tool/list_metrics.py:211-215` with a `ListMetricsRequest` that sets `view_id` to an external semantic view the current user cannot access and leaves `dataset_id=None`. 2. Inside `list_metrics`, the function enters the `try` block at `list_metrics.py:256` and, because `request.view_id is not None` and `request.dataset_id is None`, calls `_collect_external_metrics(request, ctx)` at `list_metrics.py:264-266` to load external metrics. 3. `_collect_external_metrics` resolves the view via `SemanticViewDAO.find_by_id(request.view_id)` at `list_metrics.py:143-145`, then iterates `views` and calls `view.raise_for_access()` before the `try` block at `list_metrics.py:154-157`; for a forbidden view, this raises `SupersetSecurityException` in the same way as in `_resolve_external_view` in `get_table` (`superset/mcp_service/semantic_layer/tool/get_table.py:138-151`), where it is explicitly documented and handled. 4. Because `_collect_external_metrics` does not catch `SupersetSecurityException`, the exception bubbles back into `list_metrics`, where the outer `except Exception as exc` at `list_metrics.py:286-293` logs the error and returns `SemanticLayerError.create(error=f"Internal error listing metrics: {exc}", error_type="InternalError")`, so the MCP client receives an `error_type="InternalError"` instead of the `error_type="AccessDenied"` used for the same condition in `get_table`, `get_compatible_dimensions`, and `get_compatible_metrics` (`get_table.py:148-151`, `get_compatible_dimensions.py:179-185`, `get_compatible_metrics.py:172-178`). ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=22445bdcf1f34a0a827736ad709e8c7c&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=22445bdcf1f34a0a827736ad709e8c7c&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/list_metrics.py **Line:** 154:159 **Comment:** *Api Mismatch: Access-control failures from external views are not handled as a first-class error type. `raise_for_access()` runs outside the per-view try/except, so a denied view can bubble up to the top-level generic handler and be returned as `InternalError` instead of `AccessDenied`, which breaks the semantic-layer tool contract used by related 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%2F41611&comment_hash=71652b89b19da5a34b0dd29b9ac0ea176dae8189ac3549ffe12426b29f34c95a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41611&comment_hash=71652b89b19da5a34b0dd29b9ac0ea176dae8189ac3549ffe12426b29f34c95a&reaction=dislike'>👎</a> ########## superset/mcp_service/semantic_layer/tool/list_metrics.py: ########## @@ -0,0 +1,294 @@ +# 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: list_metrics + +Unified metric discovery across built-in datasets and external semantic views. +""" + +import logging +from typing import Any + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.daos.dataset import DatasetDAO +from superset.daos.semantic_layer import SemanticViewDAO +from superset.extensions import db, 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 ( + DimensionInfo, + ListMetricsRequest, + MetricInfo, + MetricList, + SemanticLayerError, +) + +logger = logging.getLogger(__name__) + + +def _matches_search(text: str | None, search: str) -> bool: + if not text: + return False + return search.lower() in text.lower() + + +def _builtin_compatible_dims(dataset: Any) -> list[DimensionInfo]: + """Return groupby-enabled columns as compatible dimensions for a built-in metric.""" + return [ + DimensionInfo( + name=col.column_name, + verbose_name=col.verbose_name or None, + description=col.description or None, + type=col.type or None, + is_dttm=bool(col.is_dttm), + groupby=bool(col.groupby), + filterable=bool(col.filterable), + source="builtin", + ) + for col in dataset.columns + if col.groupby + ] + + +def _collect_builtin_metrics(request: ListMetricsRequest) -> list[MetricInfo]: + """Collect metrics from built-in SqlaTable datasets.""" + from sqlalchemy.orm import subqueryload + + from superset.connectors.sqla.models import SqlaTable + + with event_logger.log_context(action="mcp.list_metrics.builtin_query"): + if request.dataset_id is not None: + from sqlalchemy.orm import subqueryload + + from superset.connectors.sqla.models import SqlaTable + + dataset = DatasetDAO.find_by_id( + request.dataset_id, + query_options=[ + subqueryload(SqlaTable.columns), + subqueryload(SqlaTable.metrics), + ], + ) + datasets = [dataset] if dataset else [] + else: + from sqlalchemy.orm import subqueryload + + from superset.connectors.sqla.models import SqlaTable + + # Use _apply_base_filter with explicit eager loading to avoid + # N+1 queries when iterating dataset.metrics / dataset.columns. + query = db.session.query(SqlaTable).options( + subqueryload(SqlaTable.columns), + subqueryload(SqlaTable.metrics), + ) + datasets = DatasetDAO._apply_base_filter(query).all() + + results: list[MetricInfo] = [] + for dataset in datasets: + compat_dims = ( + _builtin_compatible_dims(dataset) + if request.include_compatible_dimensions + else [] + ) + for metric in dataset.metrics: + name = metric.metric_name or "" + desc = metric.description or "" + if request.search and not ( + _matches_search(name, request.search) + or _matches_search(desc, request.search) + ): + continue + results.append( + MetricInfo( + name=name, + verbose_name=metric.verbose_name or None, + description=desc or None, + expression=metric.expression or None, + d3format=metric.d3format or None, + warning_text=metric.warning_text or None, + source="builtin", + dataset_id=dataset.id, + dataset_name=dataset.table_name, + compatible_dimensions=compat_dims, + ) + ) + return results + + +async def _collect_external_metrics( + request: ListMetricsRequest, + ctx: Context, +) -> list[MetricInfo]: + """Collect metrics from external SemanticView models.""" + with event_logger.log_context(action="mcp.list_metrics.external_query"): + if request.view_id is not None: + view = SemanticViewDAO.find_by_id(request.view_id) + views = [view] if view else [] + else: + # find_accessible filters at SQL level, avoiding a per-row + # Python permission check and the audit noise of raise_for_access. + views = SemanticViewDAO.find_accessible() + + await ctx.debug("Found %d semantic views to scan for metrics" % len(views)) + + results: list[MetricInfo] = [] + for view in views: + # raise_for_access must be called outside the broad except block below + # so that auth errors are never silently swallowed. + view.raise_for_access() + try: + raw_metrics = view.metrics + raw_cols = view.columns if request.include_compatible_dimensions else [] + compat_dims = [ + DimensionInfo( + name=col.column_name, + verbose_name=col.verbose_name, + description=col.description, + type=col.type, + is_dttm=col.is_dttm, + groupby=col.groupby, + filterable=col.filterable, + source="external", + ) + for col in raw_cols + ] + for metric in raw_metrics: + name = metric.metric_name or "" + desc = metric.description or "" + if request.search and not ( + _matches_search(name, request.search) + or _matches_search(desc, request.search) + ): + continue + results.append( + MetricInfo( + name=name, + description=desc or None, + expression=metric.expression or None, + source="external", + view_id=view.id, + view_name=view.name, + compatible_dimensions=compat_dims, + ) + ) + except Exception as exc: # noqa: BLE001 + # External registry may be empty in OSS — degrade gracefully + await ctx.warning( + "Could not load metrics for view id=%s: %s" % (view.id, str(exc)) + ) + return results + + +@tool( + tags=["data", "semantic"], + class_permission_name="Dataset", + annotations=ToolAnnotations( + title="List metrics", + readOnlyHint=True, + destructiveHint=False, + ), +) +@requires_data_model_metadata_access +async def list_metrics( + request: ListMetricsRequest | None = None, + ctx: Context | None = None, +) -> MetricList | SemanticLayerError: + """List available metrics across built-in datasets and external semantic views. + + This is the primary entry point for semantic layer exploration. Returns a + unified list of metrics from all data sources the current user can access, + with compatible dimensions included inline. + + Workflow: + 1. list_metrics -> discover available metrics and their compatible dimensions + 2. get_table -> query data using chosen metrics and dimensions + + Use ``search`` to filter by metric name or description. Use ``dataset_id`` + or ``view_id`` to scope to a specific data source. + + Example: + ```json + {"search": "revenue", "include_compatible_dimensions": true, "page": 1} + ``` + """ + if ctx is None: + raise RuntimeError("FastMCP context is required for list_metrics") + + request = request or ListMetricsRequest() + + await ctx.info( + "Listing metrics: search=%s, dataset_id=%s, view_id=%s, page=%s" + % (request.search, request.dataset_id, request.view_id, request.page) + ) + + if not user_can_view_data_model_metadata(): + await ctx.warning("Metric listing blocked by data-model privacy controls") + 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 not None and request.view_id is not None: + return SemanticLayerError.create( + error="Provide only one of dataset_id or view_id to scope the search, not both.", + error_type="ValidationError", + ) + + try: + all_metrics: list[MetricInfo] = [] + + if request.view_id is None: + all_metrics.extend(_collect_builtin_metrics(request)) + await ctx.debug("Collected %d built-in metrics" % len(all_metrics)) + + if request.dataset_id is None: + external = await _collect_external_metrics(request, ctx) + all_metrics.extend(external) + await ctx.debug("Collected %d external metrics" % len(external)) + + total_count = len(all_metrics) + total_pages = max(1, (total_count + request.page_size - 1) // request.page_size) + start = (request.page - 1) * request.page_size + page_metrics = all_metrics[start : start + request.page_size] Review Comment: **Suggestion:** Pagination is applied to an unsorted combined list, so page boundaries are unstable across calls and can produce duplicates/missing items between pages. Apply a deterministic sort key (for example source + dataset/view id + metric name) before slicing for pagination. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ list_metrics pagination may duplicate or omit metrics between pages. - ⚠️ LLM clients cannot reliably iterate full metric catalog. - ⚠️ Inconsistent ordering complicates caching and incremental sync strategies. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure Superset with enough built-in datasets and external semantic views so that the unified metric catalog returned by `list_metrics` exceeds the default `page_size` of 50 metrics, as defined in `ListMetricsRequest.page_size` (`schemas.py:112-115`). 2. From an MCP client, call the `list_metrics` tool (`superset/mcp_service/semantic_layer/tool/list_metrics.py:211-215`) twice with the same filter parameters (e.g. same `search`, `dataset_id`, `view_id`), once with `page=1` and once with `page=2`, relying on pagination to iterate through the complete set of metrics. 3. In each call, `list_metrics` rebuilds `all_metrics` from scratch by extending `_collect_builtin_metrics(request)` and `_collect_external_metrics(request, ctx)` at `list_metrics.py:257-266`; those helpers rely on `DatasetDAO._apply_base_filter(query).all()` and `SemanticViewDAO.find_accessible()` / `find_by_id()` (see `list_metrics.py:84-104` and `list_metrics.py:143-150`), which do not impose an explicit ordering, so the order of datasets, metrics, and views can change as new records are added or database behavior changes. 4. Because pagination is applied directly to this unsorted list via `total_count`, `start`, and `page_metrics = all_metrics[start : start + request.page_size]` at `list_metrics.py:268-271`, the boundaries between pages are not stable; metrics may move between pages or appear on multiple pages when the underlying ordering shifts, so clients paging sequentially over time can observe duplicates or miss metrics entirely. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9c8c2449884d41ccb9a2ddcd867d4955&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=9c8c2449884d41ccb9a2ddcd867d4955&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/list_metrics.py **Line:** 268:271 **Comment:** *Logic Error: Pagination is applied to an unsorted combined list, so page boundaries are unstable across calls and can produce duplicates/missing items between pages. Apply a deterministic sort key (for example source + dataset/view id + metric name) before slicing for pagination. 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=dab4a8109340a28455eedb0d24578f855d6543f4c78ffed6f251ebd8ef4aa4c7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41611&comment_hash=dab4a8109340a28455eedb0d24578f855d6543f4c78ffed6f251ebd8ef4aa4c7&reaction=dislike'>👎</a> ########## superset/mcp_service/semantic_layer/tool/list_metrics.py: ########## @@ -0,0 +1,294 @@ +# 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: list_metrics + +Unified metric discovery across built-in datasets and external semantic views. +""" + +import logging +from typing import Any + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.daos.dataset import DatasetDAO +from superset.daos.semantic_layer import SemanticViewDAO +from superset.extensions import db, 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 ( + DimensionInfo, + ListMetricsRequest, + MetricInfo, + MetricList, + SemanticLayerError, +) + +logger = logging.getLogger(__name__) + + +def _matches_search(text: str | None, search: str) -> bool: + if not text: + return False + return search.lower() in text.lower() + + +def _builtin_compatible_dims(dataset: Any) -> list[DimensionInfo]: + """Return groupby-enabled columns as compatible dimensions for a built-in metric.""" + return [ + DimensionInfo( + name=col.column_name, + verbose_name=col.verbose_name or None, + description=col.description or None, + type=col.type or None, + is_dttm=bool(col.is_dttm), + groupby=bool(col.groupby), + filterable=bool(col.filterable), + source="builtin", + ) + for col in dataset.columns + if col.groupby + ] + + +def _collect_builtin_metrics(request: ListMetricsRequest) -> list[MetricInfo]: + """Collect metrics from built-in SqlaTable datasets.""" + from sqlalchemy.orm import subqueryload + + from superset.connectors.sqla.models import SqlaTable + + with event_logger.log_context(action="mcp.list_metrics.builtin_query"): + if request.dataset_id is not None: + from sqlalchemy.orm import subqueryload + + from superset.connectors.sqla.models import SqlaTable + + dataset = DatasetDAO.find_by_id( + request.dataset_id, + query_options=[ + subqueryload(SqlaTable.columns), + subqueryload(SqlaTable.metrics), + ], + ) + datasets = [dataset] if dataset else [] + else: + from sqlalchemy.orm import subqueryload + + from superset.connectors.sqla.models import SqlaTable + + # Use _apply_base_filter with explicit eager loading to avoid + # N+1 queries when iterating dataset.metrics / dataset.columns. + query = db.session.query(SqlaTable).options( + subqueryload(SqlaTable.columns), + subqueryload(SqlaTable.metrics), + ) + datasets = DatasetDAO._apply_base_filter(query).all() + + results: list[MetricInfo] = [] + for dataset in datasets: + compat_dims = ( + _builtin_compatible_dims(dataset) + if request.include_compatible_dimensions + else [] + ) + for metric in dataset.metrics: + name = metric.metric_name or "" + desc = metric.description or "" + if request.search and not ( + _matches_search(name, request.search) + or _matches_search(desc, request.search) + ): + continue + results.append( + MetricInfo( + name=name, + verbose_name=metric.verbose_name or None, + description=desc or None, + expression=metric.expression or None, + d3format=metric.d3format or None, + warning_text=metric.warning_text or None, + source="builtin", + dataset_id=dataset.id, + dataset_name=dataset.table_name, + compatible_dimensions=compat_dims, + ) + ) + return results + + +async def _collect_external_metrics( + request: ListMetricsRequest, + ctx: Context, +) -> list[MetricInfo]: + """Collect metrics from external SemanticView models.""" + with event_logger.log_context(action="mcp.list_metrics.external_query"): + if request.view_id is not None: + view = SemanticViewDAO.find_by_id(request.view_id) + views = [view] if view else [] + else: + # find_accessible filters at SQL level, avoiding a per-row + # Python permission check and the audit noise of raise_for_access. + views = SemanticViewDAO.find_accessible() + + await ctx.debug("Found %d semantic views to scan for metrics" % len(views)) + + results: list[MetricInfo] = [] + for view in views: + # raise_for_access must be called outside the broad except block below + # so that auth errors are never silently swallowed. + view.raise_for_access() + try: + raw_metrics = view.metrics + raw_cols = view.columns if request.include_compatible_dimensions else [] + compat_dims = [ + DimensionInfo( + name=col.column_name, + verbose_name=col.verbose_name, + description=col.description, + type=col.type, + is_dttm=col.is_dttm, + groupby=col.groupby, + filterable=col.filterable, + source="external", + ) + for col in raw_cols + ] Review Comment: **Suggestion:** For external semantic views, each metric is currently assigned all view columns as `compatible_dimensions`, which ignores semantic compatibility rules and can return dimensions that are invalid for that metric. Build compatibility per metric via the view’s compatibility API so inline dimensions actually match the selected metric. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ External metrics list dimensions that may be incompatible. - ⚠️ LLM agents build queries using misleading compatible_dimensions hints. - ⚠️ Behavior diverges from get_compatible_dimensions per-metric compatibility semantics. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Call the MCP `list_metrics` tool (`superset/mcp_service/semantic_layer/tool/list_metrics.py:211-215`) with a `ListMetricsRequest` that targets an accessible external semantic view via `view_id` and leaves `include_compatible_dimensions` at its default `True` (see `ListMetricsRequest.include_compatible_dimensions` in `schemas.py:105-111`). 2. `list_metrics` enters the `try` block at `list_metrics.py:256` and, because `request.dataset_id is None`, calls `_collect_external_metrics(request, ctx)` at `list_metrics.py:263-266` to collect metrics from semantic views. 3. Inside `_collect_external_metrics`, for each view, all metrics are loaded into `raw_metrics = view.metrics` at `list_metrics.py:159`, and when `include_compatible_dimensions` is True, all columns on the view are loaded into `raw_cols = view.columns` at `list_metrics.py:160` and converted once into `compat_dims` (list of `DimensionInfo`) at `list_metrics.py:161-172`, without consulting any per-metric compatibility rules such as `view.get_compatible_dimensions` used by the `get_compatible_dimensions` tool (`get_compatible_dimensions.py:187-190`). 4. The code then loops over every metric in `raw_metrics` at `list_metrics.py:174-191`, attaching the same `compat_dims` list of all columns to each `MetricInfo.compatible_dimensions`, so the response for each external metric includes every column on the view regardless of whether that dimension is actually compatible with that metric according to the semantic model, leading MCP clients to treat incompatible dimensions as valid. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5af8b04d6eea4caa9900b87e7c64c38c&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=5af8b04d6eea4caa9900b87e7c64c38c&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/list_metrics.py **Line:** 160:173 **Comment:** *Incomplete Implementation: For external semantic views, each metric is currently assigned all view columns as `compatible_dimensions`, which ignores semantic compatibility rules and can return dimensions that are invalid for that metric. Build compatibility per metric via the view’s compatibility API so inline dimensions actually match the selected metric. 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=7a84a5a3fd3ff5f428ced11ed3a2784c33cc3c74fe2b9f9dcce320a45a5ec098&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41611&comment_hash=7a84a5a3fd3ff5f428ced11ed3a2784c33cc3c74fe2b9f9dcce320a45a5ec098&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]
