You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "aminghadersohi (via GitHub)" <gi...@apache.org> on 2026/09/23 18:01:40 UTC
[PR] feat(mcp): add create_dataset_metric and delete_dataset_metric tools [superset]
aminghadersohi opened a new pull request, #44580:
URL: https://github.com/apache/superset/pull/44580
### SUMMARY
Add the two missing saved-metric operations to the MCP dataset tools:
- `create_dataset_metric`: identify a dataset by ID or UUID, supply a required `metric_name` and `expression`, and optionally set every property supported by `update_dataset_metric`. Returns the same `DatasetMetricDetail` shape.
- `delete_dataset_metric`: identify a metric by ID, UUID, or name, remove only that metric, and return its details plus accessible charts that reference its name.
Investigation and implementation notes:
- Both tools use `Dataset` / `write` permissions, check dataset editorship before inspecting metric names, and persist through `UpdateDatasetCommand`. Its editorship, externally managed dataset, uniqueness, SQL-expression, and transaction checks are not bypassed.
- The dataset DAO treats its metrics payload as a replacement list. Creation preserves existing metrics as ID/name stubs and appends a new definition; deletion omits only the target. Tests also exercise the real command/DAO, including deleting the final metric.
- Creation shares writable-property validation with updates. Deletion reuses the existing metric lookup and did-you-mean error. Duplicate names, missing datasets/metrics, and permission failures have explicit errors.
- Chart impact reporting is scoped to the dataset and checks each chart's access before returning its identity. It examines saved metric controls, HAVING filters, and query contexts, not SQL-expression text; it does not rewrite charts.
- Includes tool annotations, event logging, registration, cache exclusions, committed-write response-size handling, and documentation alongside `update_dataset_metric` (following #40975 and #43596).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable: MCP tools only; no UI changes.
### TESTING INSTRUCTIONS
Automated validation:
```bash
pytest -q -rs \
tests/unit_tests/mcp_service/dataset \
tests/unit_tests/mcp_service/test_mcp_tool_registration.py \
tests/unit_tests/mcp_service/test_rbac_tool_enforcement.py \
tests/unit_tests/mcp_service/utils/test_response_size_utils.py
```
- Result: **408 passed, 1 skipped** (the optional `sqlalchemy_bigquery` dialect is not installed). The focused new/update tool, registration, and RBAC run also passed: **102/102**.
- Includes the existing `update_dataset_metric` tests, both new tools, real command/DAO persistence, schema validation, chart-reference detection, and RBAC denial through the registered MCP tool path.
- Pre-commit (including Ruff and mypy) run on all changed files; standalone pylint run on the new tools and shared schemas.
- No live-server/UI smoke test: MCP behavior was exercised through the in-process client.
Manual verification on a disposable dataset owned/editable by the caller:
1. Call `create_dataset_metric` with `{"dataset_id": 123, "metric_name": "sum_revenue", "expression": "SUM(revenue)", "d3format": ",.2f"}`. Confirm the returned ID/UUID and that existing metrics remain unchanged.
2. Repeat creation with the same name and verify the duplicate-name error. Try a missing dataset and a caller without dataset write permission/editorship.
3. Save a chart referencing the metric. Call `delete_dataset_metric` with `{"dataset_id": 123, "metric": "sum_revenue"}`. Confirm the metric is removed, other metrics remain, and the chart appears in `affected_charts` without its definition being rewritten.
4. Try an unknown metric name and verify did-you-mean guidance. Repeat using metric IDs/UUIDs and dataset UUIDs.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add create_dataset_metric and delete_dataset_metric tools [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44580:
URL: https://github.com/apache/superset/pull/44580#issuecomment-5800199179
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | e99280cbb5322c9475b6d3a62fcfbde42de17bf3 |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ab414057b0a910008e173d9 |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-44580--superset-docs-preview.netlify.app](https://deploy-preview-44580--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">๐ฑ</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">๐ค</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpJDoAgDADAv_QsscTdH_iMAjWSyBKsB2P8u9zmMC_kkkIWWAEaMIWiPap3JmmDzcqR0MWiAkvxVtm7kPgU63Wcz_Rsru6RTK97HCaDtGhEnFlPnVvg-wE) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44580--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add create_dataset_metric and delete_dataset_metric tools [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add create_dataset_metric and delete_dataset_metric tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44580:
URL: https://github.com/apache/superset/pull/44580#issuecomment-5800171388
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add create_dataset_metric and delete_dataset_metric tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44580:
URL: https://github.com/apache/superset/pull/44580#issuecomment-5855329653
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add create_dataset_metric and delete_dataset_metric tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44580:
URL: https://github.com/apache/superset/pull/44580#discussion_r4114600066
##########
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:
<!-- Bito Reply -->
The suggestion to wrap `json.loads(chart.query_context)` in a try/except block is correct and appropriate. It prevents the tool from failing entirely when encountering malformed JSON in `query_context`, allowing it to fall back to `form_data` scanning as intended.
**superset/mcp_service/dataset/tool/delete_dataset_metric.py**
```
configs = [chart.form_data]
if chart.query_context:
try:
configs.append(json.loads(chart.query_context))
except (json.JSONDecodeError, TypeError):
pass
```
##########
tests/unit_tests/mcp_service/dataset/tool/test_dataset_metric_mutations.py:
##########
@@ -0,0 +1,421 @@
+# 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.
+
+"""Creation and deletion exercise the registered tools and dataset update path."""
+
+from collections.abc import Iterator
+from typing import Any
+from unittest.mock import MagicMock, patch, PropertyMock
+
+import pytest
+from fastmcp import Client
+from pydantic import ValidationError
+from sqlalchemy.orm import Session
+
+from superset.commands.dataset.exceptions import (
+ DatasetForbiddenError,
+ DatasetInvalidError,
+ DatasetMetricsDuplicateValidationError,
+ DatasetNotFoundError,
+ DatasetSoftDeletedTwinExistsError,
+ DatasetUpdateFailedError,
+)
+from superset.connectors.sqla.models import SqlaTable, SqlMetric
+from superset.exceptions import SupersetSecurityException
+from superset.mcp_service.app import mcp
+from superset.mcp_service.dataset.schemas import CreateDatasetMetricRequest
+from superset.mcp_service.dataset.tool.delete_dataset_metric import (
+ _find_affected_charts,
+ _references_metric,
+)
+from superset.models.core import Database
+from superset.models.slice import Slice
+from superset.utils import json
+
+TOOLS = ["create_dataset_metric", "delete_dataset_metric"]
+UUID = "a1b2c3d4-5678-90ab-cdef-1234567890ab"
+
+
+@pytest.fixture(autouse=True)
+def mock_auth() -> Iterator[None]:
+ """Authenticate an MCP caller without testing authentication itself."""
+ with patch("superset.mcp_service.auth.get_user_from_request") as get_user:
+ get_user.return_value = MagicMock(id=1, username="admin")
+ yield
+
+
+@pytest.fixture(autouse=True)
+def editorship() -> Iterator[MagicMock]:
+ """Permit editorship except in tests explicitly denying it."""
+ with patch(
+ "superset.security.SupersetSecurityManager.raise_for_editorship"
+ ) as check:
+ yield check
+
+
+@pytest.fixture
+def dataset() -> MagicMock:
+ """Supply existing metrics whose metadata must survive mutations."""
+ dataset = MagicMock(id=1, table_name="sales")
+ dataset.metrics = [
+ SqlMetric(id=10, metric_name="revenue", expression="SUM(amount)", uuid=UUID),
+ SqlMetric(
+ id=11, metric_name="count", expression="COUNT(*)", description="keep"
+ ),
+ ]
+ return dataset
+
+
+@pytest.fixture
+def references() -> Iterator[MagicMock]:
+ """Avoid database access in tool orchestration tests."""
+ with patch(
+ "superset.mcp_service.dataset.tool.delete_dataset_metric._find_affected_charts",
+ return_value=[],
+ ) as lookup:
+ yield lookup
+
+
+def request_for(tool: str) -> dict[str, Any]:
+ """Build a minimal valid request for each mutation."""
+ if tool == "create_dataset_metric":
+ return {"dataset_id": 1, "metric_name": "profit", "expression": "SUM(profit)"}
+ return {"dataset_id": 1, "metric": "revenue"}
+
+
+async def call(tool: str, request: dict[str, Any]) -> dict[str, Any]:
+ """Call the registered tool through MCP and decode its response."""
+ async with Client(mcp) as client:
+ result = await client.call_tool(tool, {"request": request})
+ return json.loads(result.content[0].text)
+
+
+@pytest.mark.parametrize("field", ["metric_name", "expression"])
+@pytest.mark.parametrize("value", [None, "", " "])
+def test_create_rejects_blank_required_properties(field: str, value: Any) -> None:
+ """Names and SQL expressions cannot be null, empty, or whitespace."""
+ request = request_for(TOOLS[0])
+ request[field] = value
+ with pytest.raises(ValidationError):
+ CreateDatasetMetricRequest.model_validate(request)
+
+
+@pytest.mark.parametrize("field", ["metric_name", "expression"])
+def test_create_requires_properties(field: str) -> None:
+ """Both name and expression are mandatory."""
+ request = request_for(TOOLS[0])
+ del request[field]
+ with pytest.raises(ValidationError):
+ CreateDatasetMetricRequest.model_validate(request)
+
+
+def test_create_rejects_invalid_extra() -> None:
+ """Reuse the update request's extra JSON validation."""
+ with pytest.raises(ValidationError, match="extra must be a valid JSON"):
+ CreateDatasetMetricRequest.model_validate(
+ {**request_for(TOOLS[0]), "extra": "{"}
+ )
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("identifier", [1, "1", UUID])
+async def test_create_success(dataset: MagicMock, identifier: int | str) -> None:
+ """All optional properties pass through; other metrics remain as stubs."""
+ request = {
+ **request_for(TOOLS[0]),
+ "dataset_id": identifier,
+ "verbose_name": "Profit",
+ "description": "Exact <UNTRUSTED-CONTENT> text",
+ "d3format": ",.2f",
+ "currency": {"symbol": "USD", "symbolPosition": "prefix"},
+ "warning_text": "Estimated",
+ "metric_type": "sum",
+ "extra": '{"key": "value"}',
+ }
+ properties = {key: value for key, value in request.items() if key != "dataset_id"}
+ created = SqlMetric(id=12, uuid=UUID, **properties)
+ updated = MagicMock(id=1, table_name="sales", metrics=[*dataset.metrics, created])
+ with (
+ patch(
+ "superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset
+ ) as find,
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.return_value = updated
+ result = await call(TOOLS[0], request)
+ assert find.call_args.args == (1 if identifier != UUID else UUID,)
+ if identifier == UUID:
+ assert find.call_args.kwargs["id_column"] == "uuid"
+ command.assert_called_once_with(
+ 1,
+ {
+ "metrics": [
+ {"id": 10, "metric_name": "revenue"},
+ {"id": 11, "metric_name": "count"},
+ properties,
+ ]
+ },
+ )
+ assert result["error"] is None
+ assert result["metric"] == {"id": 12, "uuid": UUID, **properties}
+ assert result["dataset_id"] == 1
+ assert "datasource_id=1" in result["url"]
+
+
+@pytest.mark.asyncio
+async def test_create_duplicate(dataset: MagicMock) -> None:
+ """Duplicate names produce actionable errors without invoking persistence."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(
+ TOOLS[0], {**request_for(TOOLS[0]), "metric_name": "revenue"}
+ )
+ command.assert_not_called()
+ assert result["metric"] is None
+ assert "'revenue' already exists" in result["error"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("identifier", [10, "10", UUID.upper(), "revenue"])
+async def test_delete_success(
+ dataset: MagicMock, references: MagicMock, identifier: int | str
+) -> None:
+ """Delete by any supported identifier and report referencing charts."""
+ references.return_value = [{"id": 5, "uuid": UUID, "slice_name": "Revenue"}]
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.return_value = dataset
+ result = await call(TOOLS[1], {"dataset_id": UUID, "metric": identifier})
+ command.assert_called_once_with(
+ 1, {"metrics": [{"id": 11, "metric_name": "count"}]}
+ )
+ references.assert_called_once_with(1, "revenue")
+ assert result["error"] is None
+ assert result["metric"]["id"] == 10
+ assert result["metric"]["metric_name"] == "revenue"
+ assert result["affected_charts"] == references.return_value
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("empty", [False, True])
+async def test_delete_unknown_metric(dataset: MagicMock, empty: bool) -> None:
+ """Reuse did-you-mean guidance, including the empty dataset case."""
+ if empty:
+ dataset.metrics = []
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(TOOLS[1], {"dataset_id": 1, "metric": "revenu"})
+ command.assert_not_called()
+ assert result["metric"] is None
+ assert "not found" in result["error"]
+ assert ("no saved metrics" if empty else "Did you mean: revenue?") in result[
+ "error"
+ ]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+async def test_unknown_dataset(tool: str) -> None:
+ """An unknown dataset yields a clear error and no writes."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=None),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(tool, request_for(tool))
+ command.assert_not_called()
+ assert "No dataset found" in result["error"]
+ assert result["metric"] is None
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+async def test_non_editor_cannot_inspect_metrics(
+ tool: str, dataset: MagicMock, editorship: MagicMock
+) -> None:
+ """Deny before duplicate checks, name suggestions, or chart inspection."""
+ editorship.side_effect = SupersetSecurityException(MagicMock(message="denied"))
+ type(dataset).metrics = PropertyMock(side_effect=AssertionError("metrics accessed"))
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(tool, request_for(tool))
+ command.assert_not_called()
+ assert "owner" in result["error"]
+ assert "revenue" not in result["error"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+@pytest.mark.parametrize(
+ "exception, message",
+ [
+ (DatasetNotFoundError(), "No dataset found"),
+ (DatasetForbiddenError(), "owner"),
+ (DatasetUpdateFailedError(), "Failed to"),
+ (DatasetSoftDeletedTwinExistsError(UUID), f"/api/v1/dataset/{UUID}/restore"),
+ (
+ DatasetInvalidError(exceptions=[DatasetMetricsDuplicateValidationError()]),
+ "duplicate",
+ ),
+ ],
+)
+async def test_command_errors(
+ tool: str,
+ exception: Exception,
+ message: str,
+ dataset: MagicMock,
+ references: MagicMock,
+) -> None:
+ """Preserve the existing update path's command validation and failures."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.side_effect = exception
+ result = await call(tool, request_for(tool))
+ assert result["metric"] is None
+ assert message in result["error"]
+ assert not result.get("affected_charts")
+
+
+@pytest.mark.parametrize(
+ "config, expected",
+ [
+ ({"metric": "revenue"}, True),
+ ({"metrics": ["count", "revenue"]}, True),
+ ({"metrics_b": ["revenue"]}, True),
+ ({"timeseries_limit_metric": "revenue"}, True),
+ ({"matrixify_topn_metric_rows": "revenue"}, True),
+ ({"queries": [{"metrics": ["revenue"]}]}, True),
+ ({"metric": {"saved_metric": True, "name": "revenue"}}, True),
+ ({"orderby": [["revenue", False]]}, True),
+ ({"adhoc_filters": [{"clause": "HAVING", "subject": "revenue"}]}, True),
+ ({"metrics": [{"label": "revenue", "sqlExpression": "SUM(amount)"}]}, False),
+ ({"groupby": ["revenue"], "title": "revenue"}, False),
+ ({"metrics": ["net_revenue"]}, False),
+ ({"adhoc_filters": [{"clause": "WHERE", "subject": "revenue"}]}, False),
+ ({}, False),
+ ],
+)
+def test_metric_references(config: dict[str, Any], expected: bool) -> None:
+ """Detect named references without treating labels or columns as metrics."""
+ assert _references_metric(config, "revenue") is expected
+
+
+def test_reference_lookup_is_dataset_scoped_and_access_checked(
+ session: Session,
+) -> None:
+ """Do not expose inaccessible or unrelated charts in the impact report."""
+ Database.metadata.create_all(session.bind)
+ charts = [
+ Slice(
+ slice_name="Match",
+ datasource_id=1,
+ datasource_type="table",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Hidden",
+ datasource_id=1,
+ datasource_type="table",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Other dataset",
+ datasource_id=2,
+ datasource_type="table",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Other type",
+ datasource_id=1,
+ datasource_type="druid",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Other metric",
+ datasource_id=1,
+ datasource_type="table",
+ params='{"metric":"count"}',
+ ),
+ Slice(
+ slice_name="Query context",
+ datasource_id=1,
+ datasource_type="table",
+ params="{}",
+ query_context='{"queries":[{"metrics":["revenue"]}]}',
+ ),
+ ]
+ session.add_all(charts)
+ session.commit()
+
+ def check_access(*, chart: Slice) -> None:
+ """Deny just one of the matching charts."""
+ if chart.slice_name == "Hidden":
+ raise SupersetSecurityException(MagicMock(message="denied"))
+
+ with patch(
+ "superset.security.SupersetSecurityManager.raise_for_access",
+ side_effect=check_access,
+ ) as check:
+ result = _find_affected_charts(1, "revenue")
+ assert [chart.slice_name for chart in result] == ["Match", "Query context"]
+ assert check.call_count == 4
Review Comment:
<!-- Bito Reply -->
The suggestion is appropriate as it improves code readability and maintainability by replacing a magic number with a descriptive expectation. Applying this change makes the test intent clearer and more robust against future changes to the fixture data.
**tests/unit_tests/mcp_service/dataset/tool/test_dataset_metric_mutations.py**
```
# Suggested change:
expected_chart_count = 4
assert check.call_count == expected_chart_count
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add create_dataset_metric and delete_dataset_metric tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44580:
URL: https://github.com/apache/superset/pull/44580#issuecomment-5842547755
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add create_dataset_metric and delete_dataset_metric tools [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44580:
URL: https://github.com/apache/superset/pull/44580#issuecomment-5800375642
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44580?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `25.00000%` with `141 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.12%. Comparing base ([`c0c688d`](https://app.codecov.io/gh/apache/superset/commit/c0c688d3e4b2696bf481757f5364a5dcc4184fbe?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e99280c`](https://app.codecov.io/gh/apache/superset/commit/e99280cbb5322c9475b6d3a62fcfbde42de17bf3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/44580?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../mcp\_service/dataset/tool/delete\_dataset\_metric.py](https://app.codecov.io/gh/apache/superset/pull/44580?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fdataset%2Ftool%2Fdelete_dataset_metric.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvZGF0YXNldC90b29sL2RlbGV0ZV9kYXRhc2V0X21ldHJpYy5weQ==) | 11.00% | [89 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44580?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [.../mcp\_service/dataset/tool/create\_dataset\_metric.py](https://app.codecov.io/gh/apache/superset/pull/44580?src=pr&el=tree&filepath=superset%2Fmcp_service%2Fdataset%2Ftool%2Fcreate_dataset_metric.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvbWNwX3NlcnZpY2UvZGF0YXNldC90b29sL2NyZWF0ZV9kYXRhc2V0X21ldHJpYy5weQ==) | 13.33% | [52 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/44580?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (c0c688d) and HEAD (e99280c). Click for more details.
>
> <details><summary>HEAD has 5 uploads less than BASE</summary>
>
>| Flag | BASE (c0c688d) | HEAD (e99280c) |
>|------|------|------|
>|python|7|2|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44580 +/- ##
===========================================
- Coverage 81.03% 57.12% -23.91%
===========================================
Files 2952 2954 +2
Lines 177996 178181 +185
Branches 41259 41275 +16
===========================================
- Hits 144231 101782 -42449
- Misses 31048 75424 +44376
+ Partials 2717 975 -1742
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44580/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage ฮ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/44580/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.80% <25.00%> (-0.03%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/44580/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/44580/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/44580/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.72% <25.00%> (-0.04%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/44580/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `38.77% <25.00%> (-46.55%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/44580/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/44580/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44580?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add create_dataset_metric and delete_dataset_metric tools [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44580:
URL: https://github.com/apache/superset/pull/44580#discussion_r4110054711
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Unguarded JSON parse</b></div>
<div id="fix">
`json.loads(chart.query_context)` raises on malformed JSON, and unlike `Slice.form_data` (which catches parse errors in `Slice.params`), this has no guard. The exception propagates to the tool's broad handler and re-raises, failing the whole delete after lookup succeeded. Wrap the parse in try/except and fall back to form_data-only scanning, mirroring `Slice.form_data`.
</div>
</div>
<small><i>Code Review Run #892c57</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/dataset/tool/test_dataset_metric_mutations.py:
##########
@@ -0,0 +1,421 @@
+# 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.
+
+"""Creation and deletion exercise the registered tools and dataset update path."""
+
+from collections.abc import Iterator
+from typing import Any
+from unittest.mock import MagicMock, patch, PropertyMock
+
+import pytest
+from fastmcp import Client
+from pydantic import ValidationError
+from sqlalchemy.orm import Session
+
+from superset.commands.dataset.exceptions import (
+ DatasetForbiddenError,
+ DatasetInvalidError,
+ DatasetMetricsDuplicateValidationError,
+ DatasetNotFoundError,
+ DatasetSoftDeletedTwinExistsError,
+ DatasetUpdateFailedError,
+)
+from superset.connectors.sqla.models import SqlaTable, SqlMetric
+from superset.exceptions import SupersetSecurityException
+from superset.mcp_service.app import mcp
+from superset.mcp_service.dataset.schemas import CreateDatasetMetricRequest
+from superset.mcp_service.dataset.tool.delete_dataset_metric import (
+ _find_affected_charts,
+ _references_metric,
+)
+from superset.models.core import Database
+from superset.models.slice import Slice
+from superset.utils import json
+
+TOOLS = ["create_dataset_metric", "delete_dataset_metric"]
+UUID = "a1b2c3d4-5678-90ab-cdef-1234567890ab"
+
+
+@pytest.fixture(autouse=True)
+def mock_auth() -> Iterator[None]:
+ """Authenticate an MCP caller without testing authentication itself."""
+ with patch("superset.mcp_service.auth.get_user_from_request") as get_user:
+ get_user.return_value = MagicMock(id=1, username="admin")
+ yield
+
+
+@pytest.fixture(autouse=True)
+def editorship() -> Iterator[MagicMock]:
+ """Permit editorship except in tests explicitly denying it."""
+ with patch(
+ "superset.security.SupersetSecurityManager.raise_for_editorship"
+ ) as check:
+ yield check
+
+
+@pytest.fixture
+def dataset() -> MagicMock:
+ """Supply existing metrics whose metadata must survive mutations."""
+ dataset = MagicMock(id=1, table_name="sales")
+ dataset.metrics = [
+ SqlMetric(id=10, metric_name="revenue", expression="SUM(amount)", uuid=UUID),
+ SqlMetric(
+ id=11, metric_name="count", expression="COUNT(*)", description="keep"
+ ),
+ ]
+ return dataset
+
+
+@pytest.fixture
+def references() -> Iterator[MagicMock]:
+ """Avoid database access in tool orchestration tests."""
+ with patch(
+ "superset.mcp_service.dataset.tool.delete_dataset_metric._find_affected_charts",
+ return_value=[],
+ ) as lookup:
+ yield lookup
+
+
+def request_for(tool: str) -> dict[str, Any]:
+ """Build a minimal valid request for each mutation."""
+ if tool == "create_dataset_metric":
+ return {"dataset_id": 1, "metric_name": "profit", "expression": "SUM(profit)"}
+ return {"dataset_id": 1, "metric": "revenue"}
+
+
+async def call(tool: str, request: dict[str, Any]) -> dict[str, Any]:
+ """Call the registered tool through MCP and decode its response."""
+ async with Client(mcp) as client:
+ result = await client.call_tool(tool, {"request": request})
+ return json.loads(result.content[0].text)
+
+
+@pytest.mark.parametrize("field", ["metric_name", "expression"])
+@pytest.mark.parametrize("value", [None, "", " "])
+def test_create_rejects_blank_required_properties(field: str, value: Any) -> None:
+ """Names and SQL expressions cannot be null, empty, or whitespace."""
+ request = request_for(TOOLS[0])
+ request[field] = value
+ with pytest.raises(ValidationError):
+ CreateDatasetMetricRequest.model_validate(request)
+
+
+@pytest.mark.parametrize("field", ["metric_name", "expression"])
+def test_create_requires_properties(field: str) -> None:
+ """Both name and expression are mandatory."""
+ request = request_for(TOOLS[0])
+ del request[field]
+ with pytest.raises(ValidationError):
+ CreateDatasetMetricRequest.model_validate(request)
+
+
+def test_create_rejects_invalid_extra() -> None:
+ """Reuse the update request's extra JSON validation."""
+ with pytest.raises(ValidationError, match="extra must be a valid JSON"):
+ CreateDatasetMetricRequest.model_validate(
+ {**request_for(TOOLS[0]), "extra": "{"}
+ )
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("identifier", [1, "1", UUID])
+async def test_create_success(dataset: MagicMock, identifier: int | str) -> None:
+ """All optional properties pass through; other metrics remain as stubs."""
+ request = {
+ **request_for(TOOLS[0]),
+ "dataset_id": identifier,
+ "verbose_name": "Profit",
+ "description": "Exact <UNTRUSTED-CONTENT> text",
+ "d3format": ",.2f",
+ "currency": {"symbol": "USD", "symbolPosition": "prefix"},
+ "warning_text": "Estimated",
+ "metric_type": "sum",
+ "extra": '{"key": "value"}',
+ }
+ properties = {key: value for key, value in request.items() if key != "dataset_id"}
+ created = SqlMetric(id=12, uuid=UUID, **properties)
+ updated = MagicMock(id=1, table_name="sales", metrics=[*dataset.metrics, created])
+ with (
+ patch(
+ "superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset
+ ) as find,
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.return_value = updated
+ result = await call(TOOLS[0], request)
+ assert find.call_args.args == (1 if identifier != UUID else UUID,)
+ if identifier == UUID:
+ assert find.call_args.kwargs["id_column"] == "uuid"
+ command.assert_called_once_with(
+ 1,
+ {
+ "metrics": [
+ {"id": 10, "metric_name": "revenue"},
+ {"id": 11, "metric_name": "count"},
+ properties,
+ ]
+ },
+ )
+ assert result["error"] is None
+ assert result["metric"] == {"id": 12, "uuid": UUID, **properties}
+ assert result["dataset_id"] == 1
+ assert "datasource_id=1" in result["url"]
+
+
+@pytest.mark.asyncio
+async def test_create_duplicate(dataset: MagicMock) -> None:
+ """Duplicate names produce actionable errors without invoking persistence."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(
+ TOOLS[0], {**request_for(TOOLS[0]), "metric_name": "revenue"}
+ )
+ command.assert_not_called()
+ assert result["metric"] is None
+ assert "'revenue' already exists" in result["error"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("identifier", [10, "10", UUID.upper(), "revenue"])
+async def test_delete_success(
+ dataset: MagicMock, references: MagicMock, identifier: int | str
+) -> None:
+ """Delete by any supported identifier and report referencing charts."""
+ references.return_value = [{"id": 5, "uuid": UUID, "slice_name": "Revenue"}]
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.return_value = dataset
+ result = await call(TOOLS[1], {"dataset_id": UUID, "metric": identifier})
+ command.assert_called_once_with(
+ 1, {"metrics": [{"id": 11, "metric_name": "count"}]}
+ )
+ references.assert_called_once_with(1, "revenue")
+ assert result["error"] is None
+ assert result["metric"]["id"] == 10
+ assert result["metric"]["metric_name"] == "revenue"
+ assert result["affected_charts"] == references.return_value
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("empty", [False, True])
+async def test_delete_unknown_metric(dataset: MagicMock, empty: bool) -> None:
+ """Reuse did-you-mean guidance, including the empty dataset case."""
+ if empty:
+ dataset.metrics = []
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(TOOLS[1], {"dataset_id": 1, "metric": "revenu"})
+ command.assert_not_called()
+ assert result["metric"] is None
+ assert "not found" in result["error"]
+ assert ("no saved metrics" if empty else "Did you mean: revenue?") in result[
+ "error"
+ ]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+async def test_unknown_dataset(tool: str) -> None:
+ """An unknown dataset yields a clear error and no writes."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=None),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(tool, request_for(tool))
+ command.assert_not_called()
+ assert "No dataset found" in result["error"]
+ assert result["metric"] is None
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+async def test_non_editor_cannot_inspect_metrics(
+ tool: str, dataset: MagicMock, editorship: MagicMock
+) -> None:
+ """Deny before duplicate checks, name suggestions, or chart inspection."""
+ editorship.side_effect = SupersetSecurityException(MagicMock(message="denied"))
+ type(dataset).metrics = PropertyMock(side_effect=AssertionError("metrics accessed"))
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(tool, request_for(tool))
+ command.assert_not_called()
+ assert "owner" in result["error"]
+ assert "revenue" not in result["error"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+@pytest.mark.parametrize(
+ "exception, message",
+ [
+ (DatasetNotFoundError(), "No dataset found"),
+ (DatasetForbiddenError(), "owner"),
+ (DatasetUpdateFailedError(), "Failed to"),
+ (DatasetSoftDeletedTwinExistsError(UUID), f"/api/v1/dataset/{UUID}/restore"),
+ (
+ DatasetInvalidError(exceptions=[DatasetMetricsDuplicateValidationError()]),
+ "duplicate",
+ ),
+ ],
+)
+async def test_command_errors(
+ tool: str,
+ exception: Exception,
+ message: str,
+ dataset: MagicMock,
+ references: MagicMock,
+) -> None:
+ """Preserve the existing update path's command validation and failures."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.side_effect = exception
+ result = await call(tool, request_for(tool))
+ assert result["metric"] is None
+ assert message in result["error"]
+ assert not result.get("affected_charts")
+
+
+@pytest.mark.parametrize(
+ "config, expected",
+ [
+ ({"metric": "revenue"}, True),
+ ({"metrics": ["count", "revenue"]}, True),
+ ({"metrics_b": ["revenue"]}, True),
+ ({"timeseries_limit_metric": "revenue"}, True),
+ ({"matrixify_topn_metric_rows": "revenue"}, True),
+ ({"queries": [{"metrics": ["revenue"]}]}, True),
+ ({"metric": {"saved_metric": True, "name": "revenue"}}, True),
+ ({"orderby": [["revenue", False]]}, True),
+ ({"adhoc_filters": [{"clause": "HAVING", "subject": "revenue"}]}, True),
+ ({"metrics": [{"label": "revenue", "sqlExpression": "SUM(amount)"}]}, False),
+ ({"groupby": ["revenue"], "title": "revenue"}, False),
+ ({"metrics": ["net_revenue"]}, False),
+ ({"adhoc_filters": [{"clause": "WHERE", "subject": "revenue"}]}, False),
+ ({}, False),
+ ],
+)
+def test_metric_references(config: dict[str, Any], expected: bool) -> None:
+ """Detect named references without treating labels or columns as metrics."""
+ assert _references_metric(config, "revenue") is expected
+
+
+def test_reference_lookup_is_dataset_scoped_and_access_checked(
+ session: Session,
+) -> None:
+ """Do not expose inaccessible or unrelated charts in the impact report."""
+ Database.metadata.create_all(session.bind)
+ charts = [
+ Slice(
+ slice_name="Match",
+ datasource_id=1,
+ datasource_type="table",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Hidden",
+ datasource_id=1,
+ datasource_type="table",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Other dataset",
+ datasource_id=2,
+ datasource_type="table",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Other type",
+ datasource_id=1,
+ datasource_type="druid",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Other metric",
+ datasource_id=1,
+ datasource_type="table",
+ params='{"metric":"count"}',
+ ),
+ Slice(
+ slice_name="Query context",
+ datasource_id=1,
+ datasource_type="table",
+ params="{}",
+ query_context='{"queries":[{"metrics":["revenue"]}]}',
+ ),
+ ]
+ session.add_all(charts)
+ session.commit()
+
+ def check_access(*, chart: Slice) -> None:
+ """Deny just one of the matching charts."""
+ if chart.slice_name == "Hidden":
+ raise SupersetSecurityException(MagicMock(message="denied"))
+
+ with patch(
+ "superset.security.SupersetSecurityManager.raise_for_access",
+ side_effect=check_access,
+ ) as check:
+ result = _find_affected_charts(1, "revenue")
+ assert [chart.slice_name for chart in result] == ["Match", "Query context"]
+ assert check.call_count == 4
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Magic count in assertion</b></div>
<div id="fix">
`check.call_count == 4` encodes the dataset/type scoping filter (6 fixtures minus `Other dataset` minus `Other type`) as a bare literal; a reader must re-derive it from the fixture list. A named expected count or brief comment would preserve the business rule this assertion verifies.
</div>
</div>
<small><i>Code Review Run #892c57</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/dataset/tool/test_dataset_metric_mutations.py:
##########
@@ -0,0 +1,421 @@
+# 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.
+
+"""Creation and deletion exercise the registered tools and dataset update path."""
+
+from collections.abc import Iterator
+from typing import Any
+from unittest.mock import MagicMock, patch, PropertyMock
+
+import pytest
+from fastmcp import Client
+from pydantic import ValidationError
+from sqlalchemy.orm import Session
+
+from superset.commands.dataset.exceptions import (
+ DatasetForbiddenError,
+ DatasetInvalidError,
+ DatasetMetricsDuplicateValidationError,
+ DatasetNotFoundError,
+ DatasetSoftDeletedTwinExistsError,
+ DatasetUpdateFailedError,
+)
+from superset.connectors.sqla.models import SqlaTable, SqlMetric
+from superset.exceptions import SupersetSecurityException
+from superset.mcp_service.app import mcp
+from superset.mcp_service.dataset.schemas import CreateDatasetMetricRequest
+from superset.mcp_service.dataset.tool.delete_dataset_metric import (
+ _find_affected_charts,
+ _references_metric,
+)
+from superset.models.core import Database
+from superset.models.slice import Slice
+from superset.utils import json
+
+TOOLS = ["create_dataset_metric", "delete_dataset_metric"]
+UUID = "a1b2c3d4-5678-90ab-cdef-1234567890ab"
+
+
+@pytest.fixture(autouse=True)
+def mock_auth() -> Iterator[None]:
+ """Authenticate an MCP caller without testing authentication itself."""
+ with patch("superset.mcp_service.auth.get_user_from_request") as get_user:
+ get_user.return_value = MagicMock(id=1, username="admin")
+ yield
+
+
+@pytest.fixture(autouse=True)
+def editorship() -> Iterator[MagicMock]:
+ """Permit editorship except in tests explicitly denying it."""
+ with patch(
+ "superset.security.SupersetSecurityManager.raise_for_editorship"
+ ) as check:
+ yield check
+
+
+@pytest.fixture
+def dataset() -> MagicMock:
+ """Supply existing metrics whose metadata must survive mutations."""
+ dataset = MagicMock(id=1, table_name="sales")
+ dataset.metrics = [
+ SqlMetric(id=10, metric_name="revenue", expression="SUM(amount)", uuid=UUID),
+ SqlMetric(
+ id=11, metric_name="count", expression="COUNT(*)", description="keep"
+ ),
+ ]
+ return dataset
+
+
+@pytest.fixture
+def references() -> Iterator[MagicMock]:
+ """Avoid database access in tool orchestration tests."""
+ with patch(
+ "superset.mcp_service.dataset.tool.delete_dataset_metric._find_affected_charts",
+ return_value=[],
+ ) as lookup:
+ yield lookup
+
+
+def request_for(tool: str) -> dict[str, Any]:
+ """Build a minimal valid request for each mutation."""
+ if tool == "create_dataset_metric":
+ return {"dataset_id": 1, "metric_name": "profit", "expression": "SUM(profit)"}
+ return {"dataset_id": 1, "metric": "revenue"}
+
+
+async def call(tool: str, request: dict[str, Any]) -> dict[str, Any]:
+ """Call the registered tool through MCP and decode its response."""
+ async with Client(mcp) as client:
+ result = await client.call_tool(tool, {"request": request})
+ return json.loads(result.content[0].text)
+
+
+@pytest.mark.parametrize("field", ["metric_name", "expression"])
+@pytest.mark.parametrize("value", [None, "", " "])
+def test_create_rejects_blank_required_properties(field: str, value: Any) -> None:
+ """Names and SQL expressions cannot be null, empty, or whitespace."""
+ request = request_for(TOOLS[0])
+ request[field] = value
+ with pytest.raises(ValidationError):
+ CreateDatasetMetricRequest.model_validate(request)
+
+
+@pytest.mark.parametrize("field", ["metric_name", "expression"])
+def test_create_requires_properties(field: str) -> None:
+ """Both name and expression are mandatory."""
+ request = request_for(TOOLS[0])
+ del request[field]
+ with pytest.raises(ValidationError):
+ CreateDatasetMetricRequest.model_validate(request)
+
+
+def test_create_rejects_invalid_extra() -> None:
+ """Reuse the update request's extra JSON validation."""
+ with pytest.raises(ValidationError, match="extra must be a valid JSON"):
+ CreateDatasetMetricRequest.model_validate(
+ {**request_for(TOOLS[0]), "extra": "{"}
+ )
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("identifier", [1, "1", UUID])
+async def test_create_success(dataset: MagicMock, identifier: int | str) -> None:
+ """All optional properties pass through; other metrics remain as stubs."""
+ request = {
+ **request_for(TOOLS[0]),
+ "dataset_id": identifier,
+ "verbose_name": "Profit",
+ "description": "Exact <UNTRUSTED-CONTENT> text",
+ "d3format": ",.2f",
+ "currency": {"symbol": "USD", "symbolPosition": "prefix"},
+ "warning_text": "Estimated",
+ "metric_type": "sum",
+ "extra": '{"key": "value"}',
+ }
+ properties = {key: value for key, value in request.items() if key != "dataset_id"}
+ created = SqlMetric(id=12, uuid=UUID, **properties)
+ updated = MagicMock(id=1, table_name="sales", metrics=[*dataset.metrics, created])
+ with (
+ patch(
+ "superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset
+ ) as find,
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.return_value = updated
+ result = await call(TOOLS[0], request)
+ assert find.call_args.args == (1 if identifier != UUID else UUID,)
+ if identifier == UUID:
+ assert find.call_args.kwargs["id_column"] == "uuid"
+ command.assert_called_once_with(
+ 1,
+ {
+ "metrics": [
+ {"id": 10, "metric_name": "revenue"},
+ {"id": 11, "metric_name": "count"},
+ properties,
+ ]
+ },
+ )
+ assert result["error"] is None
+ assert result["metric"] == {"id": 12, "uuid": UUID, **properties}
+ assert result["dataset_id"] == 1
+ assert "datasource_id=1" in result["url"]
+
+
+@pytest.mark.asyncio
+async def test_create_duplicate(dataset: MagicMock) -> None:
+ """Duplicate names produce actionable errors without invoking persistence."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(
+ TOOLS[0], {**request_for(TOOLS[0]), "metric_name": "revenue"}
+ )
+ command.assert_not_called()
+ assert result["metric"] is None
+ assert "'revenue' already exists" in result["error"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("identifier", [10, "10", UUID.upper(), "revenue"])
+async def test_delete_success(
+ dataset: MagicMock, references: MagicMock, identifier: int | str
+) -> None:
+ """Delete by any supported identifier and report referencing charts."""
+ references.return_value = [{"id": 5, "uuid": UUID, "slice_name": "Revenue"}]
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.return_value = dataset
+ result = await call(TOOLS[1], {"dataset_id": UUID, "metric": identifier})
+ command.assert_called_once_with(
+ 1, {"metrics": [{"id": 11, "metric_name": "count"}]}
+ )
+ references.assert_called_once_with(1, "revenue")
+ assert result["error"] is None
+ assert result["metric"]["id"] == 10
+ assert result["metric"]["metric_name"] == "revenue"
+ assert result["affected_charts"] == references.return_value
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("empty", [False, True])
+async def test_delete_unknown_metric(dataset: MagicMock, empty: bool) -> None:
+ """Reuse did-you-mean guidance, including the empty dataset case."""
+ if empty:
+ dataset.metrics = []
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(TOOLS[1], {"dataset_id": 1, "metric": "revenu"})
+ command.assert_not_called()
+ assert result["metric"] is None
+ assert "not found" in result["error"]
+ assert ("no saved metrics" if empty else "Did you mean: revenue?") in result[
+ "error"
+ ]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+async def test_unknown_dataset(tool: str) -> None:
+ """An unknown dataset yields a clear error and no writes."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=None),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(tool, request_for(tool))
+ command.assert_not_called()
+ assert "No dataset found" in result["error"]
+ assert result["metric"] is None
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+async def test_non_editor_cannot_inspect_metrics(
+ tool: str, dataset: MagicMock, editorship: MagicMock
+) -> None:
+ """Deny before duplicate checks, name suggestions, or chart inspection."""
+ editorship.side_effect = SupersetSecurityException(MagicMock(message="denied"))
+ type(dataset).metrics = PropertyMock(side_effect=AssertionError("metrics accessed"))
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(tool, request_for(tool))
+ command.assert_not_called()
+ assert "owner" in result["error"]
+ assert "revenue" not in result["error"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+@pytest.mark.parametrize(
+ "exception, message",
+ [
+ (DatasetNotFoundError(), "No dataset found"),
+ (DatasetForbiddenError(), "owner"),
+ (DatasetUpdateFailedError(), "Failed to"),
+ (DatasetSoftDeletedTwinExistsError(UUID), f"/api/v1/dataset/{UUID}/restore"),
+ (
+ DatasetInvalidError(exceptions=[DatasetMetricsDuplicateValidationError()]),
+ "duplicate",
+ ),
+ ],
+)
+async def test_command_errors(
+ tool: str,
+ exception: Exception,
+ message: str,
+ dataset: MagicMock,
+ references: MagicMock,
+) -> None:
+ """Preserve the existing update path's command validation and failures."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.side_effect = exception
+ result = await call(tool, request_for(tool))
+ assert result["metric"] is None
+ assert message in result["error"]
+ assert not result.get("affected_charts")
+
+
+@pytest.mark.parametrize(
+ "config, expected",
+ [
+ ({"metric": "revenue"}, True),
+ ({"metrics": ["count", "revenue"]}, True),
+ ({"metrics_b": ["revenue"]}, True),
+ ({"timeseries_limit_metric": "revenue"}, True),
+ ({"matrixify_topn_metric_rows": "revenue"}, True),
+ ({"queries": [{"metrics": ["revenue"]}]}, True),
+ ({"metric": {"saved_metric": True, "name": "revenue"}}, True),
+ ({"orderby": [["revenue", False]]}, True),
+ ({"adhoc_filters": [{"clause": "HAVING", "subject": "revenue"}]}, True),
+ ({"metrics": [{"label": "revenue", "sqlExpression": "SUM(amount)"}]}, False),
+ ({"groupby": ["revenue"], "title": "revenue"}, False),
+ ({"metrics": ["net_revenue"]}, False),
+ ({"adhoc_filters": [{"clause": "WHERE", "subject": "revenue"}]}, False),
+ ({}, False),
+ ],
+)
+def test_metric_references(config: dict[str, Any], expected: bool) -> None:
+ """Detect named references without treating labels or columns as metrics."""
+ assert _references_metric(config, "revenue") is expected
+
+
+def test_reference_lookup_is_dataset_scoped_and_access_checked(
+ session: Session,
+) -> None:
+ """Do not expose inaccessible or unrelated charts in the impact report."""
+ Database.metadata.create_all(session.bind)
+ charts = [
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing local type annotation</b></div>
<div id="fix">
BITO.md rule 13153 requires explicit annotations for local variables in test files even when inferable, and sibling tests in this file comply (e.g. `references: MagicMock`). Annotate this collection as `list[Slice]` for consistency and mypy coverage.
</div>
</div>
<small><i>Code Review Run #892c57</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add create_dataset_metric and delete_dataset_metric tools [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #44580:
URL: https://github.com/apache/superset/pull/44580#discussion_r4114599501
##########
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:
Fixed in 3f1ebecdc88a0f3afcd5bbed3064f1c170b2e098: creation uses an additive write without replaying stale metrics; a regression test preserves an intervening addition and rename.
##########
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:
Fixed in 3f1ebecdc88a0f3afcd5bbed3064f1c170b2e098: malformed query-context JSON falls back to form-data scanning and does not block later charts; regression tests cover both paths.
##########
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:
Fixed in 3f1ebecdc88a0f3afcd5bbed3064f1c170b2e098: guarded JSON parsing preserves form-data and subsequent-chart impact checks, with failing-before regression tests.
##########
tests/unit_tests/mcp_service/dataset/tool/test_dataset_metric_mutations.py:
##########
@@ -0,0 +1,421 @@
+# 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.
+
+"""Creation and deletion exercise the registered tools and dataset update path."""
+
+from collections.abc import Iterator
+from typing import Any
+from unittest.mock import MagicMock, patch, PropertyMock
+
+import pytest
+from fastmcp import Client
+from pydantic import ValidationError
+from sqlalchemy.orm import Session
+
+from superset.commands.dataset.exceptions import (
+ DatasetForbiddenError,
+ DatasetInvalidError,
+ DatasetMetricsDuplicateValidationError,
+ DatasetNotFoundError,
+ DatasetSoftDeletedTwinExistsError,
+ DatasetUpdateFailedError,
+)
+from superset.connectors.sqla.models import SqlaTable, SqlMetric
+from superset.exceptions import SupersetSecurityException
+from superset.mcp_service.app import mcp
+from superset.mcp_service.dataset.schemas import CreateDatasetMetricRequest
+from superset.mcp_service.dataset.tool.delete_dataset_metric import (
+ _find_affected_charts,
+ _references_metric,
+)
+from superset.models.core import Database
+from superset.models.slice import Slice
+from superset.utils import json
+
+TOOLS = ["create_dataset_metric", "delete_dataset_metric"]
+UUID = "a1b2c3d4-5678-90ab-cdef-1234567890ab"
+
+
+@pytest.fixture(autouse=True)
+def mock_auth() -> Iterator[None]:
+ """Authenticate an MCP caller without testing authentication itself."""
+ with patch("superset.mcp_service.auth.get_user_from_request") as get_user:
+ get_user.return_value = MagicMock(id=1, username="admin")
+ yield
+
+
+@pytest.fixture(autouse=True)
+def editorship() -> Iterator[MagicMock]:
+ """Permit editorship except in tests explicitly denying it."""
+ with patch(
+ "superset.security.SupersetSecurityManager.raise_for_editorship"
+ ) as check:
+ yield check
+
+
+@pytest.fixture
+def dataset() -> MagicMock:
+ """Supply existing metrics whose metadata must survive mutations."""
+ dataset = MagicMock(id=1, table_name="sales")
+ dataset.metrics = [
+ SqlMetric(id=10, metric_name="revenue", expression="SUM(amount)", uuid=UUID),
+ SqlMetric(
+ id=11, metric_name="count", expression="COUNT(*)", description="keep"
+ ),
+ ]
+ return dataset
+
+
+@pytest.fixture
+def references() -> Iterator[MagicMock]:
+ """Avoid database access in tool orchestration tests."""
+ with patch(
+ "superset.mcp_service.dataset.tool.delete_dataset_metric._find_affected_charts",
+ return_value=[],
+ ) as lookup:
+ yield lookup
+
+
+def request_for(tool: str) -> dict[str, Any]:
+ """Build a minimal valid request for each mutation."""
+ if tool == "create_dataset_metric":
+ return {"dataset_id": 1, "metric_name": "profit", "expression": "SUM(profit)"}
+ return {"dataset_id": 1, "metric": "revenue"}
+
+
+async def call(tool: str, request: dict[str, Any]) -> dict[str, Any]:
+ """Call the registered tool through MCP and decode its response."""
+ async with Client(mcp) as client:
+ result = await client.call_tool(tool, {"request": request})
+ return json.loads(result.content[0].text)
+
+
+@pytest.mark.parametrize("field", ["metric_name", "expression"])
+@pytest.mark.parametrize("value", [None, "", " "])
+def test_create_rejects_blank_required_properties(field: str, value: Any) -> None:
+ """Names and SQL expressions cannot be null, empty, or whitespace."""
+ request = request_for(TOOLS[0])
+ request[field] = value
+ with pytest.raises(ValidationError):
+ CreateDatasetMetricRequest.model_validate(request)
+
+
+@pytest.mark.parametrize("field", ["metric_name", "expression"])
+def test_create_requires_properties(field: str) -> None:
+ """Both name and expression are mandatory."""
+ request = request_for(TOOLS[0])
+ del request[field]
+ with pytest.raises(ValidationError):
+ CreateDatasetMetricRequest.model_validate(request)
+
+
+def test_create_rejects_invalid_extra() -> None:
+ """Reuse the update request's extra JSON validation."""
+ with pytest.raises(ValidationError, match="extra must be a valid JSON"):
+ CreateDatasetMetricRequest.model_validate(
+ {**request_for(TOOLS[0]), "extra": "{"}
+ )
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("identifier", [1, "1", UUID])
+async def test_create_success(dataset: MagicMock, identifier: int | str) -> None:
+ """All optional properties pass through; other metrics remain as stubs."""
+ request = {
+ **request_for(TOOLS[0]),
+ "dataset_id": identifier,
+ "verbose_name": "Profit",
+ "description": "Exact <UNTRUSTED-CONTENT> text",
+ "d3format": ",.2f",
+ "currency": {"symbol": "USD", "symbolPosition": "prefix"},
+ "warning_text": "Estimated",
+ "metric_type": "sum",
+ "extra": '{"key": "value"}',
+ }
+ properties = {key: value for key, value in request.items() if key != "dataset_id"}
+ created = SqlMetric(id=12, uuid=UUID, **properties)
+ updated = MagicMock(id=1, table_name="sales", metrics=[*dataset.metrics, created])
+ with (
+ patch(
+ "superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset
+ ) as find,
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.return_value = updated
+ result = await call(TOOLS[0], request)
+ assert find.call_args.args == (1 if identifier != UUID else UUID,)
+ if identifier == UUID:
+ assert find.call_args.kwargs["id_column"] == "uuid"
+ command.assert_called_once_with(
+ 1,
+ {
+ "metrics": [
+ {"id": 10, "metric_name": "revenue"},
+ {"id": 11, "metric_name": "count"},
+ properties,
+ ]
+ },
+ )
+ assert result["error"] is None
+ assert result["metric"] == {"id": 12, "uuid": UUID, **properties}
+ assert result["dataset_id"] == 1
+ assert "datasource_id=1" in result["url"]
+
+
+@pytest.mark.asyncio
+async def test_create_duplicate(dataset: MagicMock) -> None:
+ """Duplicate names produce actionable errors without invoking persistence."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(
+ TOOLS[0], {**request_for(TOOLS[0]), "metric_name": "revenue"}
+ )
+ command.assert_not_called()
+ assert result["metric"] is None
+ assert "'revenue' already exists" in result["error"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("identifier", [10, "10", UUID.upper(), "revenue"])
+async def test_delete_success(
+ dataset: MagicMock, references: MagicMock, identifier: int | str
+) -> None:
+ """Delete by any supported identifier and report referencing charts."""
+ references.return_value = [{"id": 5, "uuid": UUID, "slice_name": "Revenue"}]
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.return_value = dataset
+ result = await call(TOOLS[1], {"dataset_id": UUID, "metric": identifier})
+ command.assert_called_once_with(
+ 1, {"metrics": [{"id": 11, "metric_name": "count"}]}
+ )
+ references.assert_called_once_with(1, "revenue")
+ assert result["error"] is None
+ assert result["metric"]["id"] == 10
+ assert result["metric"]["metric_name"] == "revenue"
+ assert result["affected_charts"] == references.return_value
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("empty", [False, True])
+async def test_delete_unknown_metric(dataset: MagicMock, empty: bool) -> None:
+ """Reuse did-you-mean guidance, including the empty dataset case."""
+ if empty:
+ dataset.metrics = []
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(TOOLS[1], {"dataset_id": 1, "metric": "revenu"})
+ command.assert_not_called()
+ assert result["metric"] is None
+ assert "not found" in result["error"]
+ assert ("no saved metrics" if empty else "Did you mean: revenue?") in result[
+ "error"
+ ]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+async def test_unknown_dataset(tool: str) -> None:
+ """An unknown dataset yields a clear error and no writes."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=None),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(tool, request_for(tool))
+ command.assert_not_called()
+ assert "No dataset found" in result["error"]
+ assert result["metric"] is None
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+async def test_non_editor_cannot_inspect_metrics(
+ tool: str, dataset: MagicMock, editorship: MagicMock
+) -> None:
+ """Deny before duplicate checks, name suggestions, or chart inspection."""
+ editorship.side_effect = SupersetSecurityException(MagicMock(message="denied"))
+ type(dataset).metrics = PropertyMock(side_effect=AssertionError("metrics accessed"))
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(tool, request_for(tool))
+ command.assert_not_called()
+ assert "owner" in result["error"]
+ assert "revenue" not in result["error"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+@pytest.mark.parametrize(
+ "exception, message",
+ [
+ (DatasetNotFoundError(), "No dataset found"),
+ (DatasetForbiddenError(), "owner"),
+ (DatasetUpdateFailedError(), "Failed to"),
+ (DatasetSoftDeletedTwinExistsError(UUID), f"/api/v1/dataset/{UUID}/restore"),
+ (
+ DatasetInvalidError(exceptions=[DatasetMetricsDuplicateValidationError()]),
+ "duplicate",
+ ),
+ ],
+)
+async def test_command_errors(
+ tool: str,
+ exception: Exception,
+ message: str,
+ dataset: MagicMock,
+ references: MagicMock,
+) -> None:
+ """Preserve the existing update path's command validation and failures."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.side_effect = exception
+ result = await call(tool, request_for(tool))
+ assert result["metric"] is None
+ assert message in result["error"]
+ assert not result.get("affected_charts")
+
+
+@pytest.mark.parametrize(
+ "config, expected",
+ [
+ ({"metric": "revenue"}, True),
+ ({"metrics": ["count", "revenue"]}, True),
+ ({"metrics_b": ["revenue"]}, True),
+ ({"timeseries_limit_metric": "revenue"}, True),
+ ({"matrixify_topn_metric_rows": "revenue"}, True),
+ ({"queries": [{"metrics": ["revenue"]}]}, True),
+ ({"metric": {"saved_metric": True, "name": "revenue"}}, True),
+ ({"orderby": [["revenue", False]]}, True),
+ ({"adhoc_filters": [{"clause": "HAVING", "subject": "revenue"}]}, True),
+ ({"metrics": [{"label": "revenue", "sqlExpression": "SUM(amount)"}]}, False),
+ ({"groupby": ["revenue"], "title": "revenue"}, False),
+ ({"metrics": ["net_revenue"]}, False),
+ ({"adhoc_filters": [{"clause": "WHERE", "subject": "revenue"}]}, False),
+ ({}, False),
+ ],
+)
+def test_metric_references(config: dict[str, Any], expected: bool) -> None:
+ """Detect named references without treating labels or columns as metrics."""
+ assert _references_metric(config, "revenue") is expected
+
+
+def test_reference_lookup_is_dataset_scoped_and_access_checked(
+ session: Session,
+) -> None:
+ """Do not expose inaccessible or unrelated charts in the impact report."""
+ Database.metadata.create_all(session.bind)
+ charts = [
Review Comment:
Fixed in 3f1ebecdc88a0f3afcd5bbed3064f1c170b2e098: annotated the chart fixture collection as list[Slice].
##########
tests/unit_tests/mcp_service/dataset/tool/test_dataset_metric_mutations.py:
##########
@@ -0,0 +1,421 @@
+# 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.
+
+"""Creation and deletion exercise the registered tools and dataset update path."""
+
+from collections.abc import Iterator
+from typing import Any
+from unittest.mock import MagicMock, patch, PropertyMock
+
+import pytest
+from fastmcp import Client
+from pydantic import ValidationError
+from sqlalchemy.orm import Session
+
+from superset.commands.dataset.exceptions import (
+ DatasetForbiddenError,
+ DatasetInvalidError,
+ DatasetMetricsDuplicateValidationError,
+ DatasetNotFoundError,
+ DatasetSoftDeletedTwinExistsError,
+ DatasetUpdateFailedError,
+)
+from superset.connectors.sqla.models import SqlaTable, SqlMetric
+from superset.exceptions import SupersetSecurityException
+from superset.mcp_service.app import mcp
+from superset.mcp_service.dataset.schemas import CreateDatasetMetricRequest
+from superset.mcp_service.dataset.tool.delete_dataset_metric import (
+ _find_affected_charts,
+ _references_metric,
+)
+from superset.models.core import Database
+from superset.models.slice import Slice
+from superset.utils import json
+
+TOOLS = ["create_dataset_metric", "delete_dataset_metric"]
+UUID = "a1b2c3d4-5678-90ab-cdef-1234567890ab"
+
+
+@pytest.fixture(autouse=True)
+def mock_auth() -> Iterator[None]:
+ """Authenticate an MCP caller without testing authentication itself."""
+ with patch("superset.mcp_service.auth.get_user_from_request") as get_user:
+ get_user.return_value = MagicMock(id=1, username="admin")
+ yield
+
+
+@pytest.fixture(autouse=True)
+def editorship() -> Iterator[MagicMock]:
+ """Permit editorship except in tests explicitly denying it."""
+ with patch(
+ "superset.security.SupersetSecurityManager.raise_for_editorship"
+ ) as check:
+ yield check
+
+
+@pytest.fixture
+def dataset() -> MagicMock:
+ """Supply existing metrics whose metadata must survive mutations."""
+ dataset = MagicMock(id=1, table_name="sales")
+ dataset.metrics = [
+ SqlMetric(id=10, metric_name="revenue", expression="SUM(amount)", uuid=UUID),
+ SqlMetric(
+ id=11, metric_name="count", expression="COUNT(*)", description="keep"
+ ),
+ ]
+ return dataset
+
+
+@pytest.fixture
+def references() -> Iterator[MagicMock]:
+ """Avoid database access in tool orchestration tests."""
+ with patch(
+ "superset.mcp_service.dataset.tool.delete_dataset_metric._find_affected_charts",
+ return_value=[],
+ ) as lookup:
+ yield lookup
+
+
+def request_for(tool: str) -> dict[str, Any]:
+ """Build a minimal valid request for each mutation."""
+ if tool == "create_dataset_metric":
+ return {"dataset_id": 1, "metric_name": "profit", "expression": "SUM(profit)"}
+ return {"dataset_id": 1, "metric": "revenue"}
+
+
+async def call(tool: str, request: dict[str, Any]) -> dict[str, Any]:
+ """Call the registered tool through MCP and decode its response."""
+ async with Client(mcp) as client:
+ result = await client.call_tool(tool, {"request": request})
+ return json.loads(result.content[0].text)
+
+
+@pytest.mark.parametrize("field", ["metric_name", "expression"])
+@pytest.mark.parametrize("value", [None, "", " "])
+def test_create_rejects_blank_required_properties(field: str, value: Any) -> None:
+ """Names and SQL expressions cannot be null, empty, or whitespace."""
+ request = request_for(TOOLS[0])
+ request[field] = value
+ with pytest.raises(ValidationError):
+ CreateDatasetMetricRequest.model_validate(request)
+
+
+@pytest.mark.parametrize("field", ["metric_name", "expression"])
+def test_create_requires_properties(field: str) -> None:
+ """Both name and expression are mandatory."""
+ request = request_for(TOOLS[0])
+ del request[field]
+ with pytest.raises(ValidationError):
+ CreateDatasetMetricRequest.model_validate(request)
+
+
+def test_create_rejects_invalid_extra() -> None:
+ """Reuse the update request's extra JSON validation."""
+ with pytest.raises(ValidationError, match="extra must be a valid JSON"):
+ CreateDatasetMetricRequest.model_validate(
+ {**request_for(TOOLS[0]), "extra": "{"}
+ )
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("identifier", [1, "1", UUID])
+async def test_create_success(dataset: MagicMock, identifier: int | str) -> None:
+ """All optional properties pass through; other metrics remain as stubs."""
+ request = {
+ **request_for(TOOLS[0]),
+ "dataset_id": identifier,
+ "verbose_name": "Profit",
+ "description": "Exact <UNTRUSTED-CONTENT> text",
+ "d3format": ",.2f",
+ "currency": {"symbol": "USD", "symbolPosition": "prefix"},
+ "warning_text": "Estimated",
+ "metric_type": "sum",
+ "extra": '{"key": "value"}',
+ }
+ properties = {key: value for key, value in request.items() if key != "dataset_id"}
+ created = SqlMetric(id=12, uuid=UUID, **properties)
+ updated = MagicMock(id=1, table_name="sales", metrics=[*dataset.metrics, created])
+ with (
+ patch(
+ "superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset
+ ) as find,
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.return_value = updated
+ result = await call(TOOLS[0], request)
+ assert find.call_args.args == (1 if identifier != UUID else UUID,)
+ if identifier == UUID:
+ assert find.call_args.kwargs["id_column"] == "uuid"
+ command.assert_called_once_with(
+ 1,
+ {
+ "metrics": [
+ {"id": 10, "metric_name": "revenue"},
+ {"id": 11, "metric_name": "count"},
+ properties,
+ ]
+ },
+ )
+ assert result["error"] is None
+ assert result["metric"] == {"id": 12, "uuid": UUID, **properties}
+ assert result["dataset_id"] == 1
+ assert "datasource_id=1" in result["url"]
+
+
+@pytest.mark.asyncio
+async def test_create_duplicate(dataset: MagicMock) -> None:
+ """Duplicate names produce actionable errors without invoking persistence."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(
+ TOOLS[0], {**request_for(TOOLS[0]), "metric_name": "revenue"}
+ )
+ command.assert_not_called()
+ assert result["metric"] is None
+ assert "'revenue' already exists" in result["error"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("identifier", [10, "10", UUID.upper(), "revenue"])
+async def test_delete_success(
+ dataset: MagicMock, references: MagicMock, identifier: int | str
+) -> None:
+ """Delete by any supported identifier and report referencing charts."""
+ references.return_value = [{"id": 5, "uuid": UUID, "slice_name": "Revenue"}]
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.return_value = dataset
+ result = await call(TOOLS[1], {"dataset_id": UUID, "metric": identifier})
+ command.assert_called_once_with(
+ 1, {"metrics": [{"id": 11, "metric_name": "count"}]}
+ )
+ references.assert_called_once_with(1, "revenue")
+ assert result["error"] is None
+ assert result["metric"]["id"] == 10
+ assert result["metric"]["metric_name"] == "revenue"
+ assert result["affected_charts"] == references.return_value
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("empty", [False, True])
+async def test_delete_unknown_metric(dataset: MagicMock, empty: bool) -> None:
+ """Reuse did-you-mean guidance, including the empty dataset case."""
+ if empty:
+ dataset.metrics = []
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(TOOLS[1], {"dataset_id": 1, "metric": "revenu"})
+ command.assert_not_called()
+ assert result["metric"] is None
+ assert "not found" in result["error"]
+ assert ("no saved metrics" if empty else "Did you mean: revenue?") in result[
+ "error"
+ ]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+async def test_unknown_dataset(tool: str) -> None:
+ """An unknown dataset yields a clear error and no writes."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=None),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(tool, request_for(tool))
+ command.assert_not_called()
+ assert "No dataset found" in result["error"]
+ assert result["metric"] is None
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+async def test_non_editor_cannot_inspect_metrics(
+ tool: str, dataset: MagicMock, editorship: MagicMock
+) -> None:
+ """Deny before duplicate checks, name suggestions, or chart inspection."""
+ editorship.side_effect = SupersetSecurityException(MagicMock(message="denied"))
+ type(dataset).metrics = PropertyMock(side_effect=AssertionError("metrics accessed"))
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ result = await call(tool, request_for(tool))
+ command.assert_not_called()
+ assert "owner" in result["error"]
+ assert "revenue" not in result["error"]
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("tool", TOOLS)
+@pytest.mark.parametrize(
+ "exception, message",
+ [
+ (DatasetNotFoundError(), "No dataset found"),
+ (DatasetForbiddenError(), "owner"),
+ (DatasetUpdateFailedError(), "Failed to"),
+ (DatasetSoftDeletedTwinExistsError(UUID), f"/api/v1/dataset/{UUID}/restore"),
+ (
+ DatasetInvalidError(exceptions=[DatasetMetricsDuplicateValidationError()]),
+ "duplicate",
+ ),
+ ],
+)
+async def test_command_errors(
+ tool: str,
+ exception: Exception,
+ message: str,
+ dataset: MagicMock,
+ references: MagicMock,
+) -> None:
+ """Preserve the existing update path's command validation and failures."""
+ with (
+ patch("superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset),
+ patch("superset.commands.dataset.update.UpdateDatasetCommand") as command,
+ ):
+ command.return_value.run.side_effect = exception
+ result = await call(tool, request_for(tool))
+ assert result["metric"] is None
+ assert message in result["error"]
+ assert not result.get("affected_charts")
+
+
+@pytest.mark.parametrize(
+ "config, expected",
+ [
+ ({"metric": "revenue"}, True),
+ ({"metrics": ["count", "revenue"]}, True),
+ ({"metrics_b": ["revenue"]}, True),
+ ({"timeseries_limit_metric": "revenue"}, True),
+ ({"matrixify_topn_metric_rows": "revenue"}, True),
+ ({"queries": [{"metrics": ["revenue"]}]}, True),
+ ({"metric": {"saved_metric": True, "name": "revenue"}}, True),
+ ({"orderby": [["revenue", False]]}, True),
+ ({"adhoc_filters": [{"clause": "HAVING", "subject": "revenue"}]}, True),
+ ({"metrics": [{"label": "revenue", "sqlExpression": "SUM(amount)"}]}, False),
+ ({"groupby": ["revenue"], "title": "revenue"}, False),
+ ({"metrics": ["net_revenue"]}, False),
+ ({"adhoc_filters": [{"clause": "WHERE", "subject": "revenue"}]}, False),
+ ({}, False),
+ ],
+)
+def test_metric_references(config: dict[str, Any], expected: bool) -> None:
+ """Detect named references without treating labels or columns as metrics."""
+ assert _references_metric(config, "revenue") is expected
+
+
+def test_reference_lookup_is_dataset_scoped_and_access_checked(
+ session: Session,
+) -> None:
+ """Do not expose inaccessible or unrelated charts in the impact report."""
+ Database.metadata.create_all(session.bind)
+ charts = [
+ Slice(
+ slice_name="Match",
+ datasource_id=1,
+ datasource_type="table",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Hidden",
+ datasource_id=1,
+ datasource_type="table",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Other dataset",
+ datasource_id=2,
+ datasource_type="table",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Other type",
+ datasource_id=1,
+ datasource_type="druid",
+ params='{"metric":"revenue"}',
+ ),
+ Slice(
+ slice_name="Other metric",
+ datasource_id=1,
+ datasource_type="table",
+ params='{"metric":"count"}',
+ ),
+ Slice(
+ slice_name="Query context",
+ datasource_id=1,
+ datasource_type="table",
+ params="{}",
+ query_context='{"queries":[{"metrics":["revenue"]}]}',
+ ),
+ ]
+ session.add_all(charts)
+ session.commit()
+
+ def check_access(*, chart: Slice) -> None:
+ """Deny just one of the matching charts."""
+ if chart.slice_name == "Hidden":
+ raise SupersetSecurityException(MagicMock(message="denied"))
+
+ with patch(
+ "superset.security.SupersetSecurityManager.raise_for_access",
+ side_effect=check_access,
+ ) as check:
+ result = _find_affected_charts(1, "revenue")
+ assert [chart.slice_name for chart in result] == ["Match", "Query context"]
+ assert check.call_count == 4
Review Comment:
Fixed in 3f1ebecdc88a0f3afcd5bbed3064f1c170b2e098: replaced the magic count with a named expected count derived from dataset/type-scoped fixtures.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(mcp): add create_dataset_metric and delete_dataset_metric tools [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #44580:
URL: https://github.com/apache/superset/pull/44580
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org