codeant-ai-for-open-source[bot] commented on code in PR #44580: URL: https://github.com/apache/superset/pull/44580#discussion_r4109444706
########## superset/mcp_service/dataset/tool/delete_dataset_metric.py: ########## @@ -0,0 +1,247 @@ +# 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. + +"""Delete a saved metric on a dataset (FastMCP tool).""" + +from typing import Any + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset import security_manager +from superset.extensions import event_logger +from superset.mcp_service.dataset.schemas import ( + DeleteDatasetMetricRequest, + DeleteDatasetMetricResponse, + MetricChartReference, +) +from superset.mcp_service.dataset.tool.update_dataset_metric import ( + _find_metric, + _metric_not_found_message, + _serialize_metric, +) + + +def _references_metric(value: Any, metric_name: str) -> bool: + """Inspect metric controls, HAVING filters, and query-context metric names. + + Only named metric references count, not SQL expressions, adhoc labels, + column names, or arbitrary strings elsewhere in the chart configuration. + """ + if isinstance(value, list): + return any(_references_metric(item, metric_name) for item in value) + if not isinstance(value, dict): + return False + if value.get("saved_metric") is True and value.get("name") == metric_name: + return True + if value.get("clause") == "HAVING" and value.get("subject") == metric_name: + return True + for key, item in value.items(): + if {"metric", "metrics"}.intersection(key.split("_")): + candidates = item if isinstance(item, list) else [item] + if any(candidate == metric_name for candidate in candidates): + return True + if ( + key == "orderby" + and isinstance(item, list) + and any( + isinstance(order, list) and order and order[0] == metric_name + for order in item + ) + ): + return True + if _references_metric(item, metric_name): + return True + return False + + +def _find_affected_charts( + dataset_id: int, metric_name: str +) -> list[MetricChartReference]: + """Return only accessible charts on this dataset referencing the metric.""" + from superset import db + from superset.exceptions import SupersetSecurityException + from superset.models.slice import Slice + from superset.utils import json + from superset.utils.core import DatasourceType + + charts = ( + db.session.query(Slice) + .filter( + Slice.datasource_id == dataset_id, + Slice.datasource_type == DatasourceType.TABLE, + ) + .order_by(Slice.id) + .all() + ) + references = [] + for chart in charts: + try: + security_manager.raise_for_access(chart=chart) + except SupersetSecurityException: + continue + configs = [chart.form_data] + if chart.query_context: + configs.append(json.loads(chart.query_context)) Review Comment: **Suggestion:** A malformed `query_context` on one accessible chart raises during impact analysis, so the metric is not deleted even though chart reporting is only advisory. **Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Error handling` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9b2d19ebb8c740e9971520b2b6afa38d&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=9b2d19ebb8c740e9971520b2b6afa38d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dataset/tool/delete_dataset_metric.py **Line:** 97:99 **Comment:** *Error Handling: A malformed `query_context` on one accessible chart raises during impact analysis, so the metric is not deleted even though chart reporting is only advisory. 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%2F44580&comment_hash=2673f5d4e633d1d0a3b1c453fd3a0a86b3c01b66687dc27555dc85618e9c7372&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44580&comment_hash=2673f5d4e633d1d0a3b1c453fd3a0a86b3c01b66687dc27555dc85618e9c7372&reaction=dislike'>๐</a> ########## superset/mcp_service/dataset/tool/create_dataset_metric.py: ########## @@ -0,0 +1,173 @@ +# 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. + +"""Create a saved metric on a dataset (FastMCP tool).""" + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset import security_manager +from superset.extensions import event_logger +from superset.mcp_service.dataset.schemas import ( + CreateDatasetMetricRequest, + CreateDatasetMetricResponse, +) +from superset.mcp_service.dataset.tool.update_dataset_metric import ( + _serialize_metric, +) + + +@tool( + tags=["mutate"], + class_permission_name="Dataset", + method_permission_name="write", + annotations=ToolAnnotations( + title="Create dataset metric", + readOnlyHint=False, + destructiveHint=False, + idempotentHint=False, + openWorldHint=False, + ), +) +async def create_dataset_metric( + request: CreateDatasetMetricRequest, ctx: Context +) -> CreateDatasetMetricResponse: + """Add a saved metric to a dataset identified by ID or UUID. + + Requires metric_name and expression; accepts the same optional properties + as update_dataset_metric. Existing metrics are preserved. Requires dataset + editorship (or Admin), just like updating a dataset. Use get_dataset_info + to inspect existing names before creating a metric. + """ + updates = request.updates() + await ctx.info("Creating dataset metric: dataset_id=%s" % (request.dataset_id,)) + + try: + from sqlalchemy.orm import joinedload, subqueryload + + from superset.commands.dataset.exceptions import ( + DatasetForbiddenError, + DatasetInvalidError, + DatasetNotFoundError, + DatasetSoftDeletedTwinExistsError, + DatasetUpdateFailedError, + ) + from superset.commands.dataset.update import UpdateDatasetCommand + from superset.connectors.sqla.models import SqlaTable + from superset.exceptions import SupersetSecurityException + from superset.mcp_service.dataset.dataset_utils import resolve_dataset + from superset.mcp_service.utils.url_utils import get_superset_base_url + + eager_options = [ + subqueryload(SqlaTable.metrics), + joinedload(SqlaTable.database), + ] + + with event_logger.log_context(action="mcp.create_dataset_metric.lookup"): + dataset = resolve_dataset(request.dataset_id, eager_options) + + if dataset is None: + await ctx.warning("Dataset not found: %s" % (request.dataset_id,)) + return CreateDatasetMetricResponse( + error=( + f"No dataset found with identifier: {request.dataset_id}." + " Use list_datasets to get valid dataset IDs." + ), + ) + + # Check editorship before inspecting metric names. The command repeats + # this check and enforces all dataset update validation on persistence. + try: + security_manager.raise_for_editorship(dataset) + except SupersetSecurityException: + await ctx.warning( + "Dataset metric create forbidden: dataset_id=%s" % (dataset.id,) + ) + return CreateDatasetMetricResponse( + error="You must be an owner of this dataset (or an Admin) " + "to create its metrics.", + ) + + metrics = list(dataset.metrics) + if any(metric.metric_name == request.metric_name for metric in metrics): + await ctx.warning("Metric already exists: %s" % (request.metric_name,)) + return CreateDatasetMetricResponse( + dataset_id=dataset.id, + dataset_name=dataset.table_name, + error=f"Metric '{request.metric_name}' already exists on this dataset. " + "Choose a unique metric_name or use update_dataset_metric.", + ) + + # The DAO deletes omitted metrics, so preserve every existing row. + metrics_payload = [ + {"id": metric.id, "metric_name": metric.metric_name} for metric in metrics + ] + metrics_payload.append(updates) + with event_logger.log_context(action="mcp.create_dataset_metric.create"): + updated_dataset = UpdateDatasetCommand( + dataset.id, {"metrics": metrics_payload} + ).run() Review Comment: **Suggestion:** A concurrent metric change after lookup is absent from this replacement payload, so creation can overwrite that change or delete the newly added metric. **Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Race condition` [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e120c0cfc3ee485d96ca4a50afd3ace1&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=e120c0cfc3ee485d96ca4a50afd3ace1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dataset/tool/create_dataset_metric.py **Line:** 116:123 **Comment:** *Race Condition: A concurrent metric change after lookup is absent from this replacement payload, so creation can overwrite that change or delete the newly added 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%2F44580&comment_hash=814c771fc539bc9a73a3f54e133260d48b086a92616378c55856ee9285256f80&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44580&comment_hash=814c771fc539bc9a73a3f54e133260d48b086a92616378c55856ee9285256f80&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]
