codeant-ai-for-open-source[bot] commented on code in PR #41472: URL: https://github.com/apache/superset/pull/41472#discussion_r3485211163
########## superset/mcp_service/chart/schemas.py: ########## @@ -2550,3 +2550,32 @@ class ChartFiltersInfo(BaseModel): # Rebuild ChartInfo so Pydantic can resolve the ChartFiltersInfo forward reference. ChartInfo.model_rebuild() + + +class DeleteChartRequest(BaseModel): + """Request schema for delete_chart.""" + + identifier: int | str = Field( + ..., + description=( + "Chart identifier - numeric ID or UUID string (charts have no slug)." + ), + ) + + +class DeleteChartResponse(BaseModel): + """Result of a delete_chart operation.""" + + success: bool = Field(description="Whether the chart was deleted") + deleted_id: int | None = Field(None, description="ID of the deleted chart") Review Comment: **Suggestion:** Replace the integer deletion identifier in the response with a UUID-based field so the API response does not expose internal numeric IDs. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This response field exposes an internal numeric ID in a public API model. The custom rule forbids exposing internal integer IDs when a UUID public identifier is being added or changed, so this is a real violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=746cc7695ffa4c05a2c59e18c0f7a165&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=746cc7695ffa4c05a2c59e18c0f7a165&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/chart/schemas.py **Line:** 2570:2570 **Comment:** *Custom Rule: Replace the integer deletion identifier in the response with a UUID-based field so the API response does not expose internal numeric IDs. 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%2F41472&comment_hash=67d2c47736e72447495f583a94e3f9e80cf098a4ae0044913ab5d764f898e101&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=67d2c47736e72447495f583a94e3f9e80cf098a4ae0044913ab5d764f898e101&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/schemas.py: ########## @@ -2550,3 +2550,32 @@ class ChartFiltersInfo(BaseModel): # Rebuild ChartInfo so Pydantic can resolve the ChartFiltersInfo forward reference. ChartInfo.model_rebuild() + + +class DeleteChartRequest(BaseModel): + """Request schema for delete_chart.""" + + identifier: int | str = Field( + ..., + description=( + "Chart identifier - numeric ID or UUID string (charts have no slug)." + ), + ) Review Comment: **Suggestion:** Restrict the public delete request identifier to a UUID-only type and update the description accordingly so the API does not accept internal numeric IDs. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The rule flags public API identifiers that expose internal integer IDs when a UUID public identifier is being added or changed. This request schema explicitly allows a numeric ID alongside UUID, so it exposes the internal integer identifier in a public API contract. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fa2f26e943284a4c9ac4ef2d026ffda1&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=fa2f26e943284a4c9ac4ef2d026ffda1&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/chart/schemas.py **Line:** 2558:2563 **Comment:** *Custom Rule: Restrict the public delete request identifier to a UUID-only type and update the description accordingly so the API does not accept internal numeric IDs. 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%2F41472&comment_hash=2f80f755e8b538e7820a3b7a3840e9794b31739db913918f2fb6d22fbb311e66&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=2f80f755e8b538e7820a3b7a3840e9794b31739db913918f2fb6d22fbb311e66&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/delete_dashboard.py: ########## @@ -0,0 +1,166 @@ +# 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: delete_dashboard +""" + +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.commands.dashboard.exceptions import ( + DashboardDeleteFailedReportsExistError, + DashboardForbiddenError, + DashboardNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DeleteDashboardRequest, + DeleteDashboardResponse, +) +from superset.mcp_service.utils import escape_llm_context_delimiters + +logger = logging.getLogger(__name__) + + +def _find_dashboard_by_identifier(identifier: int | str) -> Any | None: + """Resolve a dashboard by numeric ID, UUID string, or slug. Returns None.""" + from superset.daos.dashboard import DashboardDAO + + if isinstance(identifier, int) or ( + isinstance(identifier, str) and identifier.isdigit() + ): + return DashboardDAO.find_by_id(int(identifier)) + # Try UUID, then fall back to slug. + dashboard = DashboardDAO.find_by_id(identifier, id_column="uuid") + if dashboard: + return dashboard + try: + return DashboardDAO.get_by_id_or_slug(identifier) + except DashboardNotFoundError: + return None + + +def _rollback() -> None: Review Comment: **Suggestion:** Add a docstring to this new helper function so all newly introduced functions are documented inline. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This is a newly introduced Python helper function and it has no docstring immediately under the definition. The custom rule requires newly added functions and classes to include docstrings. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9674729f54d14a54965bf516eb06df28&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=9674729f54d14a54965bf516eb06df28&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/delete_dashboard.py **Line:** 63:63 **Comment:** *Custom Rule: Add a docstring to this new helper function so all newly introduced functions are documented inline. 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%2F41472&comment_hash=db14532e03ba24d9ba3966722b50cf73894688c8d5565e750c2d6eb7f9860207&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=db14532e03ba24d9ba3966722b50cf73894688c8d5565e750c2d6eb7f9860207&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/tool/delete_chart.py: ########## @@ -0,0 +1,140 @@ +# 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: delete_chart +""" + +import logging + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.commands.chart.exceptions import ( + ChartDeleteFailedReportsExistError, + ChartForbiddenError, + ChartNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.chart.chart_helpers import find_chart_by_identifier +from superset.mcp_service.chart.schemas import ( + DeleteChartRequest, + DeleteChartResponse, +) +from superset.mcp_service.utils import escape_llm_context_delimiters + +logger = logging.getLogger(__name__) + + +def _rollback() -> None: + from superset import db + + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning("Database rollback failed during delete_chart error handling") Review Comment: **Suggestion:** Add an inline docstring to this new helper function to document its purpose and error-handling behavior. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This is a newly added Python helper function and it does not include a docstring, which violates the rule that new functions and classes should be documented inline. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4bb81f84e38d4c05b1ef49e5b5f4e01d&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=4bb81f84e38d4c05b1ef49e5b5f4e01d&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/chart/tool/delete_chart.py **Line:** 45:51 **Comment:** *Custom Rule: Add an inline docstring to this new helper function to document its purpose and error-handling behavior. 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%2F41472&comment_hash=bfead83bac435bcc0d598be8aa8848da5a4d69a940cebacf16ce293ef72b645a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=bfead83bac435bcc0d598be8aa8848da5a4d69a940cebacf16ce293ef72b645a&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/dashboard/tool/test_delete_dashboard.py: ########## @@ -0,0 +1,143 @@ +# 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. + +Run through the async MCP Client; auth is mocked via the autouse mock_auth +fixture, matching the other dashboard tool test files. +""" + +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_FIND = ( + "superset.mcp_service.dashboard.tool.delete_dashboard._find_dashboard_by_identifier" +) +_RUN = "superset.commands.dashboard.delete.DeleteDashboardCommand.run" + + [email protected] +def mcp_server() -> object: + return mcp + + [email protected](autouse=True) +def mock_auth(): + 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 Review Comment: **Suggestion:** Add an explicit return type annotation to this fixture function (for example, an iterator/generator type matching the yielded mock object). [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The new fixture function `mock_auth` has no return type annotation, and the surrounding code is newly added Python. This violates the rule requiring new Python functions to be fully typed. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=172e8a639f3f4c2a9947256ebcc71209&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=172e8a639f3f4c2a9947256ebcc71209&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:** 42:49 **Comment:** *Custom Rule: Add an explicit return type annotation to this fixture function (for example, an iterator/generator type matching the yielded mock object). 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%2F41472&comment_hash=50a4252c57abccc9fd6cf68d0e2c167a8196e227dfed5f6532588260bb485329&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=50a4252c57abccc9fd6cf68d0e2c167a8196e227dfed5f6532588260bb485329&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/dashboard/tool/test_delete_dashboard.py: ########## @@ -0,0 +1,143 @@ +# 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. + +Run through the async MCP Client; auth is mocked via the autouse mock_auth +fixture, matching the other dashboard tool test files. +""" + +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp + +_FIND = ( + "superset.mcp_service.dashboard.tool.delete_dashboard._find_dashboard_by_identifier" +) +_RUN = "superset.commands.dashboard.delete.DeleteDashboardCommand.run" + + [email protected] +def mcp_server() -> object: + return mcp Review Comment: **Suggestion:** Add an inline docstring to this fixture function describing what object it provides for tests. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The newly added `mcp_server` fixture is a new Python function and it does not include a docstring. That matches the rule requiring new functions and classes to be documented inline. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c53d012425ed48d48926aeb69bfe3e7a&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=c53d012425ed48d48926aeb69bfe3e7a&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:** 37:39 **Comment:** *Custom Rule: Add an inline docstring to this fixture function describing what object it provides for tests. 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%2F41472&comment_hash=b9eb547aef588c7905773a85b3f4627a7472454722c83914d0a0ee4554336373&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=b9eb547aef588c7905773a85b3f4627a7472454722c83914d0a0ee4554336373&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]
