codeant-ai-for-open-source[bot] commented on code in PR #40956: URL: https://github.com/apache/superset/pull/40956#discussion_r3556088974
########## superset/mcp_service/dashboard/tool/restore_dashboard.py: ########## @@ -0,0 +1,165 @@ +# 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: restore_dashboard + +This tool restores a soft-deleted dashboard by clearing its ``deleted_at`` +timestamp. It is the recovery path for ``delete_dashboard`` when the +``SOFT_DELETE`` feature flag is enabled; hard-deleted dashboards cannot be +restored. +""" + +import logging + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DeletedDashboardSummary, + RestoreDashboardRequest, + RestoreDashboardResponse, +) + +logger = logging.getLogger(__name__) Review Comment: **Suggestion:** Add an explicit type annotation to the module logger variable to satisfy the type-hint requirement for relevant variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The module defines a long-lived logger variable without a type annotation even though its type is known and can be annotated. This matches the type-hint rule for relevant variables. </details> <details> <summary><b>Rule source 📖 </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c06b03fe931e43489565ed1f2ba2b387&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c06b03fe931e43489565ed1f2ba2b387&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/tool/restore_dashboard.py **Line:** 39:39 **Comment:** *Custom Rule: Add an explicit type annotation to the module logger variable to satisfy the type-hint requirement for relevant variables. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40956&comment_hash=65cf25fa54fd80eb5ab8d37c15110a6360d404a3a7356c1c6f12ff7039eddd4c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40956&comment_hash=65cf25fa54fd80eb5ab8d37c15110a6360d404a3a7356c1c6f12ff7039eddd4c&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/dashboard/tool/test_delete_dashboard.py: ########## @@ -0,0 +1,197 @@ +# 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_dashboard MCP tool. + +Covers: +- Successful delete with SOFT_DELETE on (recoverable) and off (permanent) +- confirm=false refusal (safety gate) +- Dashboard not found +- Permission denied (user does not own the dashboard) +- Delete failure (e.g. associated alerts or reports) +""" + +from collections.abc import Iterator +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + + [email protected] +def mcp_server() -> object: + """Return the FastMCP app instance for use in MCP client tests.""" + return mcp + + [email protected](autouse=True) +def mock_auth() -> Iterator[Mock]: + """Mock authentication for all tests.""" + with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user: + mock_user = Mock() + mock_user.id = 1 + mock_user.username = "admin" + mock_get_user.return_value = mock_user + yield mock_get_user + + +def _mock_dashboard(id: int = 1, title: str = "Sales Dashboard") -> Mock: + """Create a minimal mock Dashboard object.""" + dashboard = Mock() + dashboard.id = id + dashboard.dashboard_title = title + dashboard.slug = f"test-dashboard-{id}" + dashboard.uuid = f"dashboard-uuid-{id}" + return dashboard + + [email protected]("soft_delete_enabled", [True, False]) +@patch("superset.commands.dashboard.delete.DeleteDashboardCommand") +@patch("superset.daos.dashboard.DashboardDAO.find_by_id") [email protected] +async def test_successful_delete( + mock_find_by_id: Mock, + mock_delete_cmd_cls: Mock, + mcp_server: object, + soft_delete_enabled: bool, +) -> None: + """Happy path: dashboard deleted, summary echoed back, and the + ``permanent`` field reflects whether SOFT_DELETE is enabled.""" + mock_find_by_id.return_value = _mock_dashboard(id=1, title="Sales Dashboard") + mock_delete_cmd = Mock() + mock_delete_cmd.run.return_value = None + mock_delete_cmd_cls.return_value = mock_delete_cmd + + with patch( + "superset.is_feature_enabled", + side_effect=lambda flag: flag == "SOFT_DELETE" and soft_delete_enabled, Review Comment: **Suggestion:** Replace the untyped lambda with a named helper function that includes explicit parameter and return type hints. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The new lambda is an untyped Python function in modified code, and the project rule requires type hints on new or modified functions where possible. Replacing it with a named helper annotated with parameter and return types would satisfy the rule. </details> <details> <summary><b>Rule source 📖 </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=78a1c702dc0b495eb7e73768619b8d8d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=78a1c702dc0b495eb7e73768619b8d8d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/dashboard/tool/test_delete_dashboard.py **Line:** 84:84 **Comment:** *Custom Rule: Replace the untyped lambda with a named helper function that includes explicit parameter and return type hints. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40956&comment_hash=cfa51bdcc0ca8d8ff84bde2daee682b4773091681b02be9b5a7c0b5638a74045&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40956&comment_hash=cfa51bdcc0ca8d8ff84bde2daee682b4773091681b02be9b5a7c0b5638a74045&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/restore_dashboard.py: ########## @@ -0,0 +1,165 @@ +# 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: restore_dashboard + +This tool restores a soft-deleted dashboard by clearing its ``deleted_at`` +timestamp. It is the recovery path for ``delete_dashboard`` when the +``SOFT_DELETE`` feature flag is enabled; hard-deleted dashboards cannot be +restored. +""" + +import logging + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DeletedDashboardSummary, + RestoreDashboardRequest, + RestoreDashboardResponse, +) + +logger = logging.getLogger(__name__) + + +@tool( + tags=["mutate"], + class_permission_name="Dashboard", + method_permission_name="write", + annotations=ToolAnnotations( + title="Restore dashboard", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def restore_dashboard( + request: RestoreDashboardRequest, ctx: Context +) -> RestoreDashboardResponse: + """ + Restore a soft-deleted dashboard by ID, making it visible and usable + again. Only dashboards deleted while soft delete was enabled can be + restored; hard-deleted dashboards are gone permanently. + """ + from superset.commands.dashboard.exceptions import ( + DashboardForbiddenError, + DashboardNotFoundError, + DashboardRestoreFailedError, + DashboardSlugConflictError, + ) + from superset.commands.dashboard.restore import RestoreDashboardCommand + from superset.daos.dashboard import DashboardDAO + + summary: DeletedDashboardSummary | None = None + try: + with event_logger.log_context(action="mcp.restore_dashboard.validation"): + # Skip the visibility filter so the soft-deleted row is findable, + # and the base filter so an owner's own trash stays reachable; + # RestoreDashboardCommand re-checks ownership before restoring. + dashboard = DashboardDAO.find_by_id( + request.dashboard_id, + skip_base_filter=True, + skip_visibility_filter=True, + ) Review Comment: **Suggestion:** Annotate the dashboard lookup variable with its expected optional model type so this key local variable is explicitly typed. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The lookup result is assigned to a local variable without an explicit annotation, and the code later treats it as an optional dashboard object. That is a real omission under the type-hint rule. </details> <details> <summary><b>Rule source 📖 </b></summary> .cursor/rules/dev-standard.mdc (line 28) </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=01b353d45c0445bf914c09cfe8809506&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=01b353d45c0445bf914c09cfe8809506&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/tool/restore_dashboard.py **Line:** 75:79 **Comment:** *Custom Rule: Annotate the dashboard lookup variable with its expected optional model type so this key local variable is explicitly typed. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40956&comment_hash=ba3bf560a19bf193942a24782410ad43e9127bbda898278b85de988923395075&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40956&comment_hash=ba3bf560a19bf193942a24782410ad43e9127bbda898278b85de988923395075&reaction=dislike'>👎</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
