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" + + [email protected](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 + + [email protected](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 + + [email protected] +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 + + [email protected] +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) + + [email protected]("field", ["metric_name", "expression"]) [email protected]("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) + + [email protected]("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": "{"} + ) + + [email protected] [email protected]("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"] + + [email protected] +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"] + + [email protected] [email protected]("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 + + [email protected] [email protected]("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" + ] + + [email protected] [email protected]("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 + + [email protected] [email protected]("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"] + + [email protected] [email protected]("tool", TOOLS) [email protected]( + "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") + + [email protected]( + "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" + + [email protected](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 + + [email protected](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 + + [email protected] +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 + + [email protected] +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) + + [email protected]("field", ["metric_name", "expression"]) [email protected]("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) + + [email protected]("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": "{"} + ) + + [email protected] [email protected]("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"] + + [email protected] +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"] + + [email protected] [email protected]("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 + + [email protected] [email protected]("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" + ] + + [email protected] [email protected]("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 + + [email protected] [email protected]("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"] + + [email protected] [email protected]("tool", TOOLS) [email protected]( + "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") + + [email protected]( + "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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
