bito-code-review[bot] commented on code in PR #44337: URL: https://github.com/apache/superset/pull/44337#discussion_r4036857062
########## tests/unit_tests/mcp_service/dataset/tool/test_restore_dataset.py: ########## @@ -0,0 +1,377 @@ +# 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. + +"""Unit tests for the restore_dataset MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dataset tool test files. +""" + +from collections.abc import Iterator +from datetime import datetime +from typing import Any +from unittest.mock import Mock, patch +from uuid import UUID + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_FIND = "superset.daos.dataset.DatasetDAO.find_by_id_or_uuid" +_COMMAND = "superset.commands.dataset.restore.RestoreDatasetCommand" + +_UUID = UUID("11111111-2222-3333-4444-555555555555") + + [email protected] +def mcp_server() -> object: + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + # The tool's editorship gate calls the real security manager; default + # it to a no-op (caller is an editor) so unrelated tests keep passing. + # The disclosure tests below re-patch it to raise. + with patch("superset.security_manager.raise_for_editorship"): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dataset( + dataset_id: int = 10, + table_name: str = "orders", + deleted: bool = True, +) -> Mock: + dataset = Mock() + dataset.id = dataset_id + dataset.table_name = table_name + dataset.uuid = _UUID + dataset.deleted_at = datetime(2026, 7, 1) if deleted else None + return dataset + + +@patch(_FIND) [email protected] +async def test_restore_dataset_not_found(mock_find: Mock, mcp_server: object) -> None: + mock_find.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 999}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotFound" + assert "999" in (content["error"] or "") + + +@patch(_FIND) [email protected] +async def test_restore_dataset_not_in_trash( Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing test docstring</b></div> <div id="fix"> Org rule (BITO 12148) requires docstrings on every newly added test function, and sibling tests in this file (e.g. `test_restore_dataset_logical_duplicate`) include one. Add a brief docstring stating the scenario: a dataset with `deleted_at=None` must return `success=False`, `error_type='NotDeleted'`, and never reach `RestoreDatasetCommand`. </div> </div> <small><i>Code Review Run #81edb1</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_restore_dataset.py: ########## @@ -0,0 +1,377 @@ +# 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. + +"""Unit tests for the restore_dataset MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dataset tool test files. +""" + +from collections.abc import Iterator +from datetime import datetime +from typing import Any +from unittest.mock import Mock, patch +from uuid import UUID + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_FIND = "superset.daos.dataset.DatasetDAO.find_by_id_or_uuid" +_COMMAND = "superset.commands.dataset.restore.RestoreDatasetCommand" + +_UUID = UUID("11111111-2222-3333-4444-555555555555") + + [email protected] +def mcp_server() -> object: + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + # The tool's editorship gate calls the real security manager; default + # it to a no-op (caller is an editor) so unrelated tests keep passing. + # The disclosure tests below re-patch it to raise. + with patch("superset.security_manager.raise_for_editorship"): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dataset( + dataset_id: int = 10, + table_name: str = "orders", + deleted: bool = True, +) -> Mock: + dataset = Mock() + dataset.id = dataset_id + dataset.table_name = table_name + dataset.uuid = _UUID + dataset.deleted_at = datetime(2026, 7, 1) if deleted else None + return dataset + + +@patch(_FIND) [email protected] +async def test_restore_dataset_not_found(mock_find: Mock, mcp_server: object) -> None: + mock_find.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 999}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotFound" + assert "999" in (content["error"] or "") + + +@patch(_FIND) [email protected] +async def test_restore_dataset_not_in_trash( + mock_find: Mock, mcp_server: object +) -> None: + mock_find.return_value = _mock_dataset(deleted=False) + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotDeleted" + assert "not in trash" in (content["error"] or "").lower() + + +@patch(_COMMAND) +@patch(_FIND) [email protected] +async def test_restore_dataset_success_by_numeric_id( Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing test docstring</b></div> <div id="fix"> Org rule (BITO 12148) requires docstrings on every newly added test function; sibling tests in this file include them. Document the scenario: lookup by numeric ID restores via `RestoreDatasetCommand(str(dataset.uuid)).run()` and returns `restored_id`/`restored_name` with `permission_denied=False`. </div> </div> <small><i>Code Review Run #81edb1</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_restore_dataset.py: ########## @@ -0,0 +1,377 @@ +# 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. + +"""Unit tests for the restore_dataset MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dataset tool test files. +""" + +from collections.abc import Iterator +from datetime import datetime +from typing import Any +from unittest.mock import Mock, patch +from uuid import UUID + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_FIND = "superset.daos.dataset.DatasetDAO.find_by_id_or_uuid" +_COMMAND = "superset.commands.dataset.restore.RestoreDatasetCommand" + +_UUID = UUID("11111111-2222-3333-4444-555555555555") + + [email protected] +def mcp_server() -> object: + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + # The tool's editorship gate calls the real security manager; default + # it to a no-op (caller is an editor) so unrelated tests keep passing. + # The disclosure tests below re-patch it to raise. + with patch("superset.security_manager.raise_for_editorship"): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dataset( + dataset_id: int = 10, + table_name: str = "orders", + deleted: bool = True, +) -> Mock: + dataset = Mock() + dataset.id = dataset_id + dataset.table_name = table_name + dataset.uuid = _UUID + dataset.deleted_at = datetime(2026, 7, 1) if deleted else None + return dataset + + +@patch(_FIND) [email protected] +async def test_restore_dataset_not_found(mock_find: Mock, mcp_server: object) -> None: + mock_find.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 999}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotFound" + assert "999" in (content["error"] or "") + + +@patch(_FIND) [email protected] +async def test_restore_dataset_not_in_trash( + mock_find: Mock, mcp_server: object +) -> None: + mock_find.return_value = _mock_dataset(deleted=False) + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotDeleted" + assert "not in trash" in (content["error"] or "").lower() + + +@patch(_COMMAND) +@patch(_FIND) [email protected] +async def test_restore_dataset_success_by_numeric_id( + mock_find: Mock, mock_command: Mock, mcp_server: object +) -> None: + mock_find.return_value = _mock_dataset(dataset_id=10, table_name="orders") + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["restored_id"] == 10 + assert "orders" in content["restored_name"] + assert content["permission_denied"] is False + mock_command.assert_called_once_with(str(_UUID)) + mock_command.return_value.run.assert_called_once() + + +@patch(_COMMAND) +@patch(_FIND) [email protected] +async def test_restore_dataset_success_by_uuid( + mock_find: Mock, mock_command: Mock, mcp_server: object +) -> None: + mock_find.return_value = _mock_dataset(dataset_id=10, table_name="orders") + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": str(_UUID)}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["restored_id"] == 10 + mock_find.assert_called_once_with( + str(_UUID), skip_base_filter=True, skip_visibility_filter=True + ) + mock_command.assert_called_once_with(str(_UUID)) + + +@patch(_COMMAND) +@patch(_FIND) [email protected] +async def test_restore_dataset_permission_denied( + mock_find: Mock, mock_command: Mock, mcp_server: object +) -> None: + from superset.commands.dataset.exceptions import DatasetForbiddenError Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Inline import in test</b></div> <div id="fix"> BITO rule 12745 requires module-level imports unless a circular dependency is documented. None applies here: `superset/mcp_service/dataset/tool/restore_dataset.py` imports these exceptions at module level. Hoist `DatasetForbiddenError` to the top-level import block with the other `superset` imports. </div> </div> <small><i>Code Review Run #81edb1</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_restore_dataset.py: ########## @@ -0,0 +1,377 @@ +# 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. + +"""Unit tests for the restore_dataset MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dataset tool test files. +""" + +from collections.abc import Iterator +from datetime import datetime +from typing import Any +from unittest.mock import Mock, patch +from uuid import UUID + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_FIND = "superset.daos.dataset.DatasetDAO.find_by_id_or_uuid" +_COMMAND = "superset.commands.dataset.restore.RestoreDatasetCommand" + +_UUID = UUID("11111111-2222-3333-4444-555555555555") + + [email protected] +def mcp_server() -> object: + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + # The tool's editorship gate calls the real security manager; default + # it to a no-op (caller is an editor) so unrelated tests keep passing. + # The disclosure tests below re-patch it to raise. + with patch("superset.security_manager.raise_for_editorship"): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dataset( + dataset_id: int = 10, + table_name: str = "orders", + deleted: bool = True, +) -> Mock: + dataset = Mock() + dataset.id = dataset_id + dataset.table_name = table_name + dataset.uuid = _UUID + dataset.deleted_at = datetime(2026, 7, 1) if deleted else None + return dataset + + +@patch(_FIND) [email protected] +async def test_restore_dataset_not_found(mock_find: Mock, mcp_server: object) -> None: + mock_find.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 999}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotFound" + assert "999" in (content["error"] or "") + + +@patch(_FIND) [email protected] +async def test_restore_dataset_not_in_trash( + mock_find: Mock, mcp_server: object +) -> None: + mock_find.return_value = _mock_dataset(deleted=False) + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotDeleted" + assert "not in trash" in (content["error"] or "").lower() + + +@patch(_COMMAND) +@patch(_FIND) [email protected] +async def test_restore_dataset_success_by_numeric_id( + mock_find: Mock, mock_command: Mock, mcp_server: object +) -> None: + mock_find.return_value = _mock_dataset(dataset_id=10, table_name="orders") + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["restored_id"] == 10 + assert "orders" in content["restored_name"] + assert content["permission_denied"] is False + mock_command.assert_called_once_with(str(_UUID)) + mock_command.return_value.run.assert_called_once() + + +@patch(_COMMAND) +@patch(_FIND) [email protected] +async def test_restore_dataset_success_by_uuid( + mock_find: Mock, mock_command: Mock, mcp_server: object +) -> None: + mock_find.return_value = _mock_dataset(dataset_id=10, table_name="orders") + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": str(_UUID)}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["restored_id"] == 10 + mock_find.assert_called_once_with( + str(_UUID), skip_base_filter=True, skip_visibility_filter=True + ) + mock_command.assert_called_once_with(str(_UUID)) + + +@patch(_COMMAND) +@patch(_FIND) [email protected] +async def test_restore_dataset_permission_denied( Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing test docstring</b></div> <div id="fix"> Org rule (BITO 12148) requires docstrings on every newly added test function; sibling tests in this file include them. Document the scenario: `DatasetForbiddenError` raised from `RestoreDatasetCommand.run` maps to `permission_denied=True` with a permission-worded error. </div> </div> <small><i>Code Review Run #81edb1</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_restore_dataset.py: ########## @@ -0,0 +1,377 @@ +# 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. + +"""Unit tests for the restore_dataset MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dataset tool test files. +""" + +from collections.abc import Iterator +from datetime import datetime +from typing import Any +from unittest.mock import Mock, patch +from uuid import UUID + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_FIND = "superset.daos.dataset.DatasetDAO.find_by_id_or_uuid" +_COMMAND = "superset.commands.dataset.restore.RestoreDatasetCommand" + +_UUID = UUID("11111111-2222-3333-4444-555555555555") + + [email protected] +def mcp_server() -> object: + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + # The tool's editorship gate calls the real security manager; default + # it to a no-op (caller is an editor) so unrelated tests keep passing. + # The disclosure tests below re-patch it to raise. + with patch("superset.security_manager.raise_for_editorship"): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dataset( + dataset_id: int = 10, + table_name: str = "orders", + deleted: bool = True, +) -> Mock: + dataset = Mock() + dataset.id = dataset_id + dataset.table_name = table_name + dataset.uuid = _UUID + dataset.deleted_at = datetime(2026, 7, 1) if deleted else None + return dataset + + +@patch(_FIND) [email protected] +async def test_restore_dataset_not_found(mock_find: Mock, mcp_server: object) -> None: + mock_find.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 999}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotFound" + assert "999" in (content["error"] or "") + + +@patch(_FIND) [email protected] +async def test_restore_dataset_not_in_trash( + mock_find: Mock, mcp_server: object +) -> None: + mock_find.return_value = _mock_dataset(deleted=False) + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotDeleted" + assert "not in trash" in (content["error"] or "").lower() + + +@patch(_COMMAND) +@patch(_FIND) [email protected] +async def test_restore_dataset_success_by_numeric_id( + mock_find: Mock, mock_command: Mock, mcp_server: object +) -> None: + mock_find.return_value = _mock_dataset(dataset_id=10, table_name="orders") + + async with Client(mcp_server) as client: + result = await client.call_tool( + "restore_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["restored_id"] == 10 + assert "orders" in content["restored_name"] + assert content["permission_denied"] is False + mock_command.assert_called_once_with(str(_UUID)) + mock_command.return_value.run.assert_called_once() + + +@patch(_COMMAND) +@patch(_FIND) [email protected] +async def test_restore_dataset_success_by_uuid( Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing test docstring</b></div> <div id="fix"> Org rule (BITO 12148) requires docstrings on every newly added test function; sibling tests in this file include them. Document the scenario: a UUID-string identifier resolves through the unfiltered `DatasetDAO.find_by_id_or_uuid` lookup and restores the same dataset. </div> </div> <small><i>Code Review Run #81edb1</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_delete_dataset.py: ########## @@ -0,0 +1,336 @@ +# 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. + +"""Unit tests for the delete_dataset MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dataset tool test files. +""" + +from collections.abc import Iterator +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_RESOLVE = "superset.mcp_service.dataset.tool.delete_dataset.resolve_dataset" +_COUNT = "superset.mcp_service.dataset.tool.delete_dataset._count_affected_objects" +_RUN = "superset.commands.dataset.delete.DeleteDatasetCommand.run" +_VALIDATE = "superset.commands.dataset.delete.DeleteDatasetCommand.validate" +_FLAG = "superset.mcp_service.dataset.tool.delete_dataset.is_feature_enabled" + + [email protected] +def mcp_server() -> object: + """Provide the FastMCP app instance under test.""" + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + """Authenticate every tool call as a mock admin user.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + # The tool validates the command (lookup + editorship) before counting + # dependents; default it to a pass so tests that patch run() alone keep + # working. The permission test re-patches it to raise. + with patch(_VALIDATE): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dataset(dataset_id: int = 10, table_name: str = "orders") -> Mock: + """Build a minimal dataset stand-in with the attributes the tool reads.""" + dataset = Mock() + dataset.id = dataset_id + dataset.table_name = table_name + return dataset + + +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_not_found(mock_resolve: Mock, mcp_server: object) -> None: + mock_resolve.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": 999}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotFound" + assert "999" in (content["error"] or "") + + +@patch(_COUNT, return_value=(0, 0)) +@patch(_RUN) +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_success( + mock_resolve: Mock, mock_run: Mock, mock_count: Mock, mcp_server: object +) -> None: + mock_resolve.return_value = _mock_dataset(dataset_id=10, table_name="orders") + mock_run.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["deleted_id"] == 10 + assert "orders" in content["deleted_name"] + assert content["permission_denied"] is False + assert content["affected_chart_count"] == 0 + mock_run.assert_called_once() + mock_count.assert_called_once_with(10) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing test docstring</b></div> <div id="fix"> `test_delete_dataset_success` is a new test function with no docstring, unlike `test_delete_dataset_not_found` and the other siblings in this file which document intent. BITO.md adaptive rule 12148 requires docstrings on every newly added test function; adding one keeps this file consistent and makes the covered scenario (happy path: resolve, run, count) explicit. </div> </div> <small><i>Code Review Run #81edb1</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_delete_dataset.py: ########## @@ -0,0 +1,336 @@ +# 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. + +"""Unit tests for the delete_dataset MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dataset tool test files. +""" + +from collections.abc import Iterator +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_RESOLVE = "superset.mcp_service.dataset.tool.delete_dataset.resolve_dataset" +_COUNT = "superset.mcp_service.dataset.tool.delete_dataset._count_affected_objects" +_RUN = "superset.commands.dataset.delete.DeleteDatasetCommand.run" +_VALIDATE = "superset.commands.dataset.delete.DeleteDatasetCommand.validate" +_FLAG = "superset.mcp_service.dataset.tool.delete_dataset.is_feature_enabled" + + [email protected] +def mcp_server() -> object: + """Provide the FastMCP app instance under test.""" + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + """Authenticate every tool call as a mock admin user.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + # The tool validates the command (lookup + editorship) before counting + # dependents; default it to a pass so tests that patch run() alone keep + # working. The permission test re-patches it to raise. + with patch(_VALIDATE): + mock_user = Mock() Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing fixture local annotations</b></div> <div id="fix"> `mock_auth` assigns `mock_user = Mock()` (line 53) and `_mock_dataset` assigns `dataset = Mock()` (line 62) without annotations, despite the fixtures themselves being fully annotated. Repo rule 13153 covers these locals; add `: Mock` to both. </div> </div> <small><i>Code Review Run #81edb1</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_delete_dataset.py: ########## @@ -0,0 +1,336 @@ +# 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. + +"""Unit tests for the delete_dataset MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dataset tool test files. +""" + +from collections.abc import Iterator +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_RESOLVE = "superset.mcp_service.dataset.tool.delete_dataset.resolve_dataset" +_COUNT = "superset.mcp_service.dataset.tool.delete_dataset._count_affected_objects" +_RUN = "superset.commands.dataset.delete.DeleteDatasetCommand.run" +_VALIDATE = "superset.commands.dataset.delete.DeleteDatasetCommand.validate" +_FLAG = "superset.mcp_service.dataset.tool.delete_dataset.is_feature_enabled" + + [email protected] +def mcp_server() -> object: + """Provide the FastMCP app instance under test.""" + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + """Authenticate every tool call as a mock admin user.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + # The tool validates the command (lookup + editorship) before counting + # dependents; default it to a pass so tests that patch run() alone keep + # working. The permission test re-patches it to raise. + with patch(_VALIDATE): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dataset(dataset_id: int = 10, table_name: str = "orders") -> Mock: + """Build a minimal dataset stand-in with the attributes the tool reads.""" + dataset = Mock() + dataset.id = dataset_id + dataset.table_name = table_name + return dataset + + +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_not_found(mock_resolve: Mock, mcp_server: object) -> None: + mock_resolve.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": 999}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotFound" + assert "999" in (content["error"] or "") + + +@patch(_COUNT, return_value=(0, 0)) +@patch(_RUN) +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_success( + mock_resolve: Mock, mock_run: Mock, mock_count: Mock, mcp_server: object +) -> None: + mock_resolve.return_value = _mock_dataset(dataset_id=10, table_name="orders") + mock_run.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["deleted_id"] == 10 + assert "orders" in content["deleted_name"] + assert content["permission_denied"] is False + assert content["affected_chart_count"] == 0 + mock_run.assert_called_once() + mock_count.assert_called_once_with(10) + + +@patch(_COUNT, return_value=(0, 0)) +@patch(_RUN) +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_by_uuid( + mock_resolve: Mock, mock_run: Mock, mock_count: Mock, mcp_server: object +) -> None: + uuid = "11111111-2222-3333-4444-555555555555" + mock_resolve.return_value = _mock_dataset(dataset_id=10) + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": uuid}} + ) + + assert result.structured_content["success"] is True + mock_resolve.assert_called_once_with(uuid) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing test docstring</b></div> <div id="fix"> `test_delete_dataset_by_uuid` is a new test function with no docstring, unlike its siblings (`test_delete_dataset_not_found`, `test_count_affected_objects_only_counts_accessible`, `test_delete_dataset_rejects_boolean_identifier`) which document intent. BITO.md adaptive rule 12148 requires docstrings on newly added test functions; one line stating the UUID pass-through scenario keeps the file uniform. </div> </div> <div id="suggestion"> <div id="issue"><b>Untyped local variable</b></div> <div id="fix"> The local `uuid` at line 116 is the only unannotated local in the changed tests; siblings (`content`, `result`, `mock_user`, `security_manager`) are all annotated per BITO.md adaptive rule 13153. Adding `: str` keeps the file consistent with the repo's strict test-typing convention. </div> </div> <small><i>Code Review Run #81edb1</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_delete_dataset.py: ########## @@ -0,0 +1,336 @@ +# 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. + +"""Unit tests for the delete_dataset MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dataset tool test files. +""" + +from collections.abc import Iterator +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_RESOLVE = "superset.mcp_service.dataset.tool.delete_dataset.resolve_dataset" +_COUNT = "superset.mcp_service.dataset.tool.delete_dataset._count_affected_objects" +_RUN = "superset.commands.dataset.delete.DeleteDatasetCommand.run" +_VALIDATE = "superset.commands.dataset.delete.DeleteDatasetCommand.validate" +_FLAG = "superset.mcp_service.dataset.tool.delete_dataset.is_feature_enabled" + + [email protected] +def mcp_server() -> object: + """Provide the FastMCP app instance under test.""" + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + """Authenticate every tool call as a mock admin user.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + # The tool validates the command (lookup + editorship) before counting + # dependents; default it to a pass so tests that patch run() alone keep + # working. The permission test re-patches it to raise. + with patch(_VALIDATE): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dataset(dataset_id: int = 10, table_name: str = "orders") -> Mock: + """Build a minimal dataset stand-in with the attributes the tool reads.""" + dataset = Mock() + dataset.id = dataset_id + dataset.table_name = table_name + return dataset + + +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_not_found(mock_resolve: Mock, mcp_server: object) -> None: Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Test missing docstring</b></div> <div id="fix"> `test_delete_dataset_not_found` (line 70) is the only test function in this new file without a docstring; every sibling test documents its scenario. Repo rule 12148 requires docstrings on all new test functions. Add a one-line docstring describing the not-found path. </div> </div> <small><i>Code Review Run #81edb1</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_delete_dataset.py: ########## @@ -0,0 +1,336 @@ +# 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. + +"""Unit tests for the delete_dataset MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dataset tool test files. +""" + +from collections.abc import Iterator +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_RESOLVE = "superset.mcp_service.dataset.tool.delete_dataset.resolve_dataset" +_COUNT = "superset.mcp_service.dataset.tool.delete_dataset._count_affected_objects" +_RUN = "superset.commands.dataset.delete.DeleteDatasetCommand.run" +_VALIDATE = "superset.commands.dataset.delete.DeleteDatasetCommand.validate" +_FLAG = "superset.mcp_service.dataset.tool.delete_dataset.is_feature_enabled" + + [email protected] +def mcp_server() -> object: + """Provide the FastMCP app instance under test.""" + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + """Authenticate every tool call as a mock admin user.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + # The tool validates the command (lookup + editorship) before counting + # dependents; default it to a pass so tests that patch run() alone keep + # working. The permission test re-patches it to raise. + with patch(_VALIDATE): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dataset(dataset_id: int = 10, table_name: str = "orders") -> Mock: + """Build a minimal dataset stand-in with the attributes the tool reads.""" + dataset = Mock() + dataset.id = dataset_id + dataset.table_name = table_name + return dataset + + +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_not_found(mock_resolve: Mock, mcp_server: object) -> None: + mock_resolve.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": 999}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotFound" + assert "999" in (content["error"] or "") + + +@patch(_COUNT, return_value=(0, 0)) +@patch(_RUN) +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_success( + mock_resolve: Mock, mock_run: Mock, mock_count: Mock, mcp_server: object +) -> None: + mock_resolve.return_value = _mock_dataset(dataset_id=10, table_name="orders") + mock_run.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["deleted_id"] == 10 + assert "orders" in content["deleted_name"] + assert content["permission_denied"] is False + assert content["affected_chart_count"] == 0 + mock_run.assert_called_once() + mock_count.assert_called_once_with(10) + + +@patch(_COUNT, return_value=(0, 0)) +@patch(_RUN) +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_by_uuid( + mock_resolve: Mock, mock_run: Mock, mock_count: Mock, mcp_server: object +) -> None: + uuid = "11111111-2222-3333-4444-555555555555" + mock_resolve.return_value = _mock_dataset(dataset_id=10) + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": uuid}} + ) + + assert result.structured_content["success"] is True + mock_resolve.assert_called_once_with(uuid) + + +@patch(_COUNT, return_value=(0, 0)) +@patch(_FLAG) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Untyped lambda side_effect</b></div> <div id="fix"> `test_delete_dataset_soft_delete_reports_restorable` sets `mock_flag.side_effect = lambda flag: flag == "SOFT_DELETE"` (line 129). Repo rule 13350 requires typed helper functions over untyped lambdas. Extract a typed module-level helper and assign it. </div> </div> <small><i>Code Review Run #81edb1</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_delete_dataset.py: ########## @@ -0,0 +1,336 @@ +# 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. + +"""Unit tests for the delete_dataset MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dataset tool test files. +""" + +from collections.abc import Iterator +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_RESOLVE = "superset.mcp_service.dataset.tool.delete_dataset.resolve_dataset" +_COUNT = "superset.mcp_service.dataset.tool.delete_dataset._count_affected_objects" +_RUN = "superset.commands.dataset.delete.DeleteDatasetCommand.run" +_VALIDATE = "superset.commands.dataset.delete.DeleteDatasetCommand.validate" +_FLAG = "superset.mcp_service.dataset.tool.delete_dataset.is_feature_enabled" + + [email protected] +def mcp_server() -> object: + """Provide the FastMCP app instance under test.""" + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + """Authenticate every tool call as a mock admin user.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + # The tool validates the command (lookup + editorship) before counting + # dependents; default it to a pass so tests that patch run() alone keep + # working. The permission test re-patches it to raise. + with patch(_VALIDATE): + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dataset(dataset_id: int = 10, table_name: str = "orders") -> Mock: + """Build a minimal dataset stand-in with the attributes the tool reads.""" + dataset = Mock() + dataset.id = dataset_id + dataset.table_name = table_name + return dataset + + +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_not_found(mock_resolve: Mock, mcp_server: object) -> None: + mock_resolve.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": 999}} + ) + + content = result.structured_content + assert content["success"] is False + assert content["error_type"] == "NotFound" + assert "999" in (content["error"] or "") + + +@patch(_COUNT, return_value=(0, 0)) +@patch(_RUN) +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_success( + mock_resolve: Mock, mock_run: Mock, mock_count: Mock, mcp_server: object +) -> None: + mock_resolve.return_value = _mock_dataset(dataset_id=10, table_name="orders") + mock_run.return_value = None + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["deleted_id"] == 10 + assert "orders" in content["deleted_name"] + assert content["permission_denied"] is False + assert content["affected_chart_count"] == 0 + mock_run.assert_called_once() + mock_count.assert_called_once_with(10) + + +@patch(_COUNT, return_value=(0, 0)) +@patch(_RUN) +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_by_uuid( + mock_resolve: Mock, mock_run: Mock, mock_count: Mock, mcp_server: object +) -> None: + uuid = "11111111-2222-3333-4444-555555555555" + mock_resolve.return_value = _mock_dataset(dataset_id=10) + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": uuid}} + ) + + assert result.structured_content["success"] is True + mock_resolve.assert_called_once_with(uuid) + + +@patch(_COUNT, return_value=(0, 0)) +@patch(_FLAG) +@patch(_RUN) +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_soft_delete_reports_restorable( + mock_resolve: Mock, + mock_run: Mock, + mock_flag: Mock, + mock_count: Mock, + mcp_server: object, +) -> None: + mock_resolve.return_value = _mock_dataset(dataset_id=10, table_name="orders") + mock_run.return_value = None + mock_flag.side_effect = lambda flag: flag == "SOFT_DELETE" + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["soft_deleted"] is True + assert "restor" in (content["message"] or "").lower() + + +@patch(_COUNT, return_value=(0, 0)) +@patch(_FLAG) +@patch(_RUN) +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_hard_delete_reports_permanent( + mock_resolve: Mock, + mock_run: Mock, + mock_flag: Mock, + mock_count: Mock, + mcp_server: object, +) -> None: + mock_resolve.return_value = _mock_dataset(dataset_id=10, table_name="orders") + mock_run.return_value = None + mock_flag.return_value = False + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["soft_deleted"] is False + assert "permanent" in (content["message"] or "").lower() + + +@patch(_COUNT, return_value=(3, 2)) +@patch(_RUN) +@patch(_RESOLVE) [email protected] +async def test_delete_dataset_reports_affected_charts( + mock_resolve: Mock, mock_run: Mock, mock_count: Mock, mcp_server: object +) -> None: + mock_resolve.return_value = _mock_dataset(dataset_id=10, table_name="orders") + + async with Client(mcp_server) as client: + result = await client.call_tool( + "delete_dataset", {"request": {"identifier": 10}} + ) + + content = result.structured_content + assert content["success"] is True + assert content["affected_chart_count"] == 3 + assert content["affected_dashboard_count"] == 2 + assert "3 chart(s)" in (content["message"] or "") + + +def test_count_affected_objects_only_counts_accessible() -> None: + """Counts must not disclose charts/dashboards the caller cannot access.""" + from superset.mcp_service.dataset.tool.delete_dataset import ( + _count_affected_objects, + ) + + visible_chart, hidden_chart = Mock(), Mock() + visible_dashboard, hidden_dashboard = Mock(), Mock() + security_manager = Mock() Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing mock type annotations</b></div> <div id="fix"> `test_count_affected_objects_only_counts_accessible` assigns `visible_chart, hidden_chart = Mock(), Mock()` (and `visible_dashboard`, `security_manager`) with no annotations. Repo rule 13153 requires explicit annotations for test-file locals including mocks. Add `: Mock` to each. </div> </div> <small><i>Code Review Run #81edb1</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/test_rbac_tool_enforcement.py: ########## @@ -110,6 +110,24 @@ "write", "Dataset", ), + ( + "update_dataset", + {"dataset_id": 1, "description": "denied"}, + "write", + "Dataset", + ), + ( + "delete_dataset", + {"identifier": 1}, + "write", + "Dataset", + ), + ( + "restore_dataset", + {"identifier": 1}, + "write", + "Dataset", + ), Review Comment: <div> <div id="suggestion"> <div id="issue"><b>RBAC permission mismatch</b></div> <div id="fix"> These tests assert `can_access("can_write", "Dataset")`, but `delete_dataset`/`restore_dataset` declare only `class_permission_name="Dataset"` with no `method_permission_name`, so `mcp_auth_hook` defaults to `can_read` (auth.py:397). The `assert_called_with("can_write", ...)` will fail against the current tools. Add `method_permission_name="write"` to both tool decorators, or align the test expectation. </div> </div> <small><i>Code Review Run #81edb1</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 ########## superset/mcp_service/dataset/tool/update_dataset.py: ########## @@ -0,0 +1,320 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +MCP tool: update_dataset +""" + +import logging +from typing import Any + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset import security_manager +from superset.exceptions import SupersetException +from superset.extensions import event_logger +from superset.mcp_service.dataset.schemas import ( + UpdateDatasetRequest, + UpdateDatasetResponse, +) + +logger = logging.getLogger(__name__) + + +def _column_names(dataset: Any) -> set[str]: + return {column.column_name for column in dataset.columns} + + +def _sync_error_message(ex: Exception) -> str: + # Raw SQLAlchemy text can leak SQL or connection details; Superset + # exception messages are user-facing by design. + if isinstance(ex, SQLAlchemyError): + return "a database error occurred" + return str(ex) + + +@tool( + tags=["mutate"], + class_permission_name="Dataset", + method_permission_name="write", + annotations=ToolAnnotations( + title="Update dataset", + readOnlyHint=False, + # Rewriting a virtual dataset's SQL or re-syncing its columns changes + # what every chart built on it queries — non-additive, like + # update_chart. + destructiveHint=True, + idempotentHint=False, + openWorldHint=False, + ), +) +async def update_dataset( # noqa: C901 Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Overloaded tool function</b></div> <div id="fix"> `update_dataset` is ~250 lines with `# noqa: C901`, combining identifier resolution, editorship enforcement, sql-vs-physical validation, sync_columns computation, main_dttm_col validation/application, column re-sync, and response building. Multiple unrelated responsibilities in one unit (dim 14) make it hard to test and maintain. </div> </div> <small><i>Code Review Run #81edb1</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 ########## superset/mcp_service/dataset/schemas.py: ########## @@ -718,6 +739,246 @@ class UpdateDatasetMetricResponse(BaseModel): ) +class DeleteDatasetRequest(BaseModel): + """Request schema for delete_dataset.""" + + identifier: int | str = Field( + ..., + description=( + "Dataset identifier - numeric ID or UUID string (NOT the table name)." + ), + ) + + @field_validator("identifier", mode="before") + @classmethod + def reject_bool_identifier(cls, value: object) -> object: + """bool is a subclass of int, so identifier=true would coerce to + dataset ID 1 and delete the wrong object; reject it outright.""" + if isinstance(value, bool): + raise ValueError("identifier must be an integer ID or UUID string") + return value + + +class DeleteDatasetResponse(BaseModel): + """Result of a delete_dataset operation.""" + + success: bool = Field(description="Whether the dataset was deleted") + deleted_id: int | None = Field(None, description="ID of the deleted dataset") + deleted_name: str | None = Field( + None, description="Table name of the deleted dataset" + ) + soft_deleted: bool = Field( + False, + description=( + "True when the dataset was soft-deleted (moved to trash, because the " + "SOFT_DELETE feature flag is enabled) and can be restored by an " + "owner or Admin. False means the delete was permanent." + ), + ) + affected_chart_count: int = Field( + 0, + description=( + "Number of charts (visible to the caller) built on this dataset. " + "They stop working while the dataset is deleted." + ), + ) + affected_dashboard_count: int = Field( + 0, + description=( + "Number of dashboards (visible to the caller) containing those charts." + ), + ) + message: str | None = Field(None, description="Human-readable outcome message") + error: str | None = Field(None, description="Error message if the delete failed") + error_type: str | None = Field(None, description="Type of error if failed") + permission_denied: bool = Field( + False, + description=( + "True when the caller lacks permission to delete the dataset (do not " + "retry; ask the user)." + ), + ) + + +class RestoreDatasetRequest(BaseModel): + """Request schema for restore_dataset.""" + + identifier: int | str = Field( + ..., + description=( + "Dataset identifier - numeric ID or UUID string (NOT the table name)." + ), + ) + + @field_validator("identifier", mode="before") + @classmethod + def reject_bool_identifier(cls, value: object) -> object: + """bool is a subclass of int, so identifier=true would coerce to + dataset ID 1 and target the wrong object; reject it outright.""" + if isinstance(value, bool): + raise ValueError("identifier must be an integer ID or UUID string") + return value + + +class RestoreDatasetResponse(BaseModel): + """Result of a restore_dataset operation.""" + + success: bool = Field(description="Whether the dataset was restored from trash") + restored_id: int | None = Field(None, description="ID of the restored dataset") + restored_name: str | None = Field( + None, description="Table name of the restored dataset" + ) + message: str | None = Field(None, description="Human-readable outcome message") + error: str | None = Field(None, description="Error message if the restore failed") + error_type: str | None = Field(None, description="Type of error if failed") + permission_denied: bool = Field( + False, + description=( + "True when the caller lacks permission to restore the dataset (do not " + "retry; ask the user)." + ), + ) + + +UPDATABLE_DATASET_FIELDS: frozenset[str] = frozenset( + { + "table_name", + "sql", + "description", + "main_dttm_col", + "cache_timeout", + } +) + + +class UpdateDatasetRequest(BaseModel): + """Request schema for update_dataset.""" + + model_config = ConfigDict(populate_by_name=True) + + dataset_id: int | str = Field( Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Inconsistent identifier naming</b></div> <div id="fix"> Related request schemas use inconsistent identifiers: `DeleteDatasetRequest.identifier` and `RestoreDatasetRequest.identifier` vs `UpdateDatasetRequest.dataset_id`. Equivalent operations should use a consistent field name to reduce cognitive load for MCP callers. </div> </div> <small><i>Code Review Run #81edb1</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]
