gkneighb commented on code in PR #41842: URL: https://github.com/apache/superset/pull/41842#discussion_r3565449295
########## superset/mcp_service/chart/tool/restore_chart.py: ########## @@ -0,0 +1,155 @@ +# 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_chart +""" + +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.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.chart.schemas import ( + RestoreChartRequest, + RestoreChartResponse, +) +from superset.mcp_service.utils import ( + escape_llm_context_delimiters, + sanitize_for_llm_context, +) + +logger = logging.getLogger(__name__) + + +def _find_chart_for_restore(identifier: int | str) -> Any | None: + """Resolve a chart by numeric ID or UUID, including soft-deleted rows. + + ``skip_visibility_filter`` is the only bypass — the DAO ``base_filter`` + stays in effect, so rows the user cannot see in the live UI stay hidden. + Ownership is then enforced by ``RestoreChartCommand``. + """ + from superset.daos.chart import ChartDAO + + return ChartDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True) + + +def _rollback() -> None: + from superset import db + + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning("Database rollback failed during restore_chart error handling") + + +@tool( + tags=["mutate"], + class_permission_name="Chart", + annotations=ToolAnnotations( + title="Restore chart", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def restore_chart( + request: RestoreChartRequest, ctx: Context +) -> RestoreChartResponse: + """Restore a soft-deleted chart from trash. + + Identify the chart by numeric ID or UUID string (NOT chart name). Only + charts that were soft-deleted (moved to trash while the ``SOFT_DELETE`` + feature flag was enabled) can be restored; permanently deleted charts are + unrecoverable. The caller must own the chart (or be an Admin). + + Example: + ```json + {"identifier": 123} + ``` + + Returns success with the restored chart's id/name, or an error. When the + caller lacks permission, ``permission_denied`` is true — do not retry; ask + the user. + """ + await ctx.info("Restoring chart: identifier=%s" % (request.identifier,)) + + chart = _find_chart_for_restore(request.identifier) + if not chart: Review Comment: Fixed in 2ab66a25ce — same hardening as the delete tools on #41472. Lookup db failures now return structured LookupFailed. ########## superset/mcp_service/dashboard/tool/restore_dashboard.py: ########## @@ -0,0 +1,162 @@ +# 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 +""" + +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 ( + DashboardForbiddenError, + DashboardNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + RestoreDashboardRequest, + RestoreDashboardResponse, +) +from superset.mcp_service.utils import ( + escape_llm_context_delimiters, + sanitize_for_llm_context, +) + +logger = logging.getLogger(__name__) + + +def _find_dashboard_for_restore(identifier: int | str) -> Any | None: + """Resolve a dashboard by numeric ID or UUID, including soft-deleted rows. + + ``skip_visibility_filter`` is the only bypass — the DAO ``base_filter`` + stays in effect, so rows the user cannot see in the live UI stay hidden. + Ownership is then enforced by ``RestoreDashboardCommand``. + """ + from superset.daos.dashboard import DashboardDAO + + return DashboardDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True) + + +def _rollback() -> None: + from superset import db + + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning( + "Database rollback failed during restore_dashboard error handling" + ) + + +@tool( + tags=["mutate"], + class_permission_name="Dashboard", + annotations=ToolAnnotations( + title="Restore dashboard", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def restore_dashboard( + request: RestoreDashboardRequest, ctx: Context +) -> RestoreDashboardResponse: + """Restore a soft-deleted dashboard from trash. + + Identify the dashboard by numeric ID or UUID string (slug lookup does not + cover trashed dashboards). Only dashboards that were soft-deleted (moved + to trash while the ``SOFT_DELETE`` feature flag was enabled) can be + restored; permanently deleted dashboards are unrecoverable. The caller + must own the dashboard (or be an Admin). + + Example: + ```json + {"identifier": 42} + ``` + + Returns success with the restored dashboard's id/title, or an error. When + the caller lacks permission, ``permission_denied`` is true — do not retry; + ask the user. + """ + await ctx.info("Restoring dashboard: identifier=%s" % (request.identifier,)) + + dashboard = _find_dashboard_for_restore(request.identifier) + if not dashboard: Review Comment: Fixed in 2ab66a25ce — same hardening as the delete tools on #41472. Same guard as restore_chart. ########## superset/mcp_service/chart/tool/restore_chart.py: ########## @@ -0,0 +1,154 @@ +# 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_chart +""" + +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.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.chart.schemas import ( + RestoreChartRequest, + RestoreChartResponse, +) +from superset.mcp_service.utils import ( + escape_llm_context_delimiters, + sanitize_for_llm_context, +) + +logger = logging.getLogger(__name__) + + +def _find_chart_for_restore(identifier: int | str) -> Any | None: + """Resolve a chart by numeric ID or UUID, including soft-deleted rows. + + ``skip_visibility_filter`` is the only bypass — the DAO ``base_filter`` + stays in effect, so rows the user cannot see in the live UI stay hidden. + Ownership is then enforced by ``RestoreChartCommand``. + """ + from superset.daos.chart import ChartDAO + + return ChartDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True) + + +def _rollback() -> None: + from superset import db + + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning("Database rollback failed during restore_chart error handling") + + +@tool( + tags=["mutate"], + class_permission_name="Chart", + annotations=ToolAnnotations( + title="Restore chart", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def restore_chart( + request: RestoreChartRequest, ctx: Context +) -> RestoreChartResponse: + """Restore a soft-deleted chart from trash. + + Identify the chart by numeric ID or UUID string (NOT chart name). Only + charts that were soft-deleted (moved to trash while the ``SOFT_DELETE`` + feature flag was enabled) can be restored; permanently deleted charts are + unrecoverable. The caller must be an editor of the chart (owners and + Admins qualify). + + Example: + ```json + {"identifier": 123} + ``` + + Returns success with the restored chart's id/name, or an error. When the + caller lacks permission, ``permission_denied`` is true — do not retry; ask + the user. + """ + await ctx.info("Restoring chart: identifier=%s" % (request.identifier,)) + + chart = _find_chart_for_restore(request.identifier) + if not chart: + safe_id = escape_llm_context_delimiters(str(request.identifier)[:200]) + msg = f"No chart found with identifier: {safe_id}." + return RestoreChartResponse(success=False, error=msg, error_type="NotFound") + + chart_id = chart.id + # Chart names are user-controlled; wrap before composing response text so + # a hostile name cannot inject prompt content into the tool output. + chart_name = sanitize_for_llm_context(chart.slice_name, field_path=("slice_name",)) + + if chart.deleted_at is None: + return RestoreChartResponse( + success=False, + error=( + f"Chart '{chart_name}' (id={chart_id}) is not in trash; " + "nothing to restore." + ), + error_type="NotDeleted", + ) + + try: + from superset.commands.chart.restore import RestoreChartCommand + + with event_logger.log_context(action="mcp.restore_chart"): + RestoreChartCommand(str(chart.uuid)).run() + + return RestoreChartResponse( + success=True, + restored_id=chart_id, + restored_name=chart_name, + message=f"Restored chart '{chart_name}' (id={chart_id}) from trash.", + ) + except ChartForbiddenError: + await ctx.warning("Permission denied restoring chart id=%s" % (chart_id,)) + return RestoreChartResponse( + success=False, + permission_denied=True, + error=( + f"You do not have permission to restore chart '{chart_name}' " + f"(id={chart_id}). Ask the user to restore it or grant access; " + "do not retry." + ), + error_type="Forbidden", + ) + except ChartNotFoundError: + msg = f"Chart id={chart_id} is no longer restorable." + return RestoreChartResponse(success=False, error=msg, error_type="NotFound") + except (CommandException, SQLAlchemyError, ValueError) as ex: + _rollback() + await ctx.error("Chart restore failed: %s: %s" % (type(ex).__name__, ex)) + return RestoreChartResponse( + success=False, + error=f"Chart restore failed: {ex}", + error_type=type(ex).__name__, + ) Review Comment: Fixed in 2ab66a25ce — same hardening as the delete tools on #41472. SQLAlchemyError now returns a generic message; CommandException text is user-facing by design. With a test. ########## superset/mcp_service/dashboard/tool/restore_dashboard.py: ########## @@ -0,0 +1,162 @@ +# 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 +""" + +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 ( + DashboardForbiddenError, + DashboardNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + RestoreDashboardRequest, + RestoreDashboardResponse, +) +from superset.mcp_service.utils import ( + escape_llm_context_delimiters, + sanitize_for_llm_context, +) + +logger = logging.getLogger(__name__) + + +def _find_dashboard_for_restore(identifier: int | str) -> Any | None: + """Resolve a dashboard by numeric ID or UUID, including soft-deleted rows. + + ``skip_visibility_filter`` is the only bypass — the DAO ``base_filter`` + stays in effect, so rows the user cannot see in the live UI stay hidden. + Ownership is then enforced by ``RestoreDashboardCommand``. + """ + from superset.daos.dashboard import DashboardDAO + + return DashboardDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True) + + +def _rollback() -> None: + from superset import db + + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning( + "Database rollback failed during restore_dashboard error handling" + ) + + +@tool( + tags=["mutate"], + class_permission_name="Dashboard", + annotations=ToolAnnotations( + title="Restore dashboard", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def restore_dashboard( + request: RestoreDashboardRequest, ctx: Context +) -> RestoreDashboardResponse: + """Restore a soft-deleted dashboard from trash. + + Identify the dashboard by numeric ID or UUID string (slug lookup does not + cover trashed dashboards). Only dashboards that were soft-deleted (moved + to trash while the ``SOFT_DELETE`` feature flag was enabled) can be + restored; permanently deleted dashboards are unrecoverable. The caller + must be an editor of the dashboard (owners and Admins qualify). + + Example: + ```json + {"identifier": 42} + ``` + + Returns success with the restored dashboard's id/title, or an error. When + the caller lacks permission, ``permission_denied`` is true — do not retry; + ask the user. + """ + await ctx.info("Restoring dashboard: identifier=%s" % (request.identifier,)) + + dashboard = _find_dashboard_for_restore(request.identifier) + if not dashboard: + safe_id = escape_llm_context_delimiters(str(request.identifier)[:200]) + msg = f"No dashboard found with identifier: {safe_id}." + return RestoreDashboardResponse(success=False, error=msg, error_type="NotFound") + + dashboard_id = dashboard.id + # Dashboard titles are user-controlled; wrap before composing response + # text so a hostile title cannot inject prompt content into the output. + dashboard_name = sanitize_for_llm_context( + dashboard.dashboard_title, field_path=("dashboard_title",) + ) + + if dashboard.deleted_at is None: + return RestoreDashboardResponse( + success=False, + error=( + f"Dashboard '{dashboard_name}' (id={dashboard_id}) is not in " + "trash; nothing to restore." + ), + error_type="NotDeleted", + ) + + try: + from superset.commands.dashboard.restore import RestoreDashboardCommand + + with event_logger.log_context(action="mcp.restore_dashboard"): + RestoreDashboardCommand(str(dashboard.uuid)).run() + + return RestoreDashboardResponse( + success=True, + restored_id=dashboard_id, + restored_name=dashboard_name, + message=( + f"Restored dashboard '{dashboard_name}' (id={dashboard_id}) from trash." + ), + ) + except DashboardForbiddenError: + await ctx.warning( + "Permission denied restoring dashboard id=%s" % (dashboard_id,) + ) + return RestoreDashboardResponse( + success=False, + permission_denied=True, + error=( + f"You do not have permission to restore dashboard " + f"'{dashboard_name}' (id={dashboard_id}). Ask the user to " + "restore it or grant access; do not retry." + ), + error_type="Forbidden", + ) + except DashboardNotFoundError: + msg = f"Dashboard id={dashboard_id} is no longer restorable." + return RestoreDashboardResponse(success=False, error=msg, error_type="NotFound") + except (CommandException, SQLAlchemyError, ValueError) as ex: + _rollback() + await ctx.error("Dashboard restore failed: %s: %s" % (type(ex).__name__, ex)) + return RestoreDashboardResponse( + success=False, + error=f"Dashboard restore failed: {ex}", + error_type=type(ex).__name__, Review Comment: Fixed in 2ab66a25ce — same hardening as the delete tools on #41472. Same sanitization as restore_chart. ########## superset/mcp_service/chart/tool/restore_chart.py: ########## @@ -0,0 +1,154 @@ +# 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_chart +""" + +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.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.chart.schemas import ( + RestoreChartRequest, + RestoreChartResponse, +) +from superset.mcp_service.utils import ( + escape_llm_context_delimiters, + sanitize_for_llm_context, +) + +logger = logging.getLogger(__name__) + + +def _find_chart_for_restore(identifier: int | str) -> Any | None: + """Resolve a chart by numeric ID or UUID, including soft-deleted rows. + + ``skip_visibility_filter`` is the only bypass — the DAO ``base_filter`` + stays in effect, so rows the user cannot see in the live UI stay hidden. + Ownership is then enforced by ``RestoreChartCommand``. + """ + from superset.daos.chart import ChartDAO + + return ChartDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True) Review Comment: Declining: keeping base_filter in the pre-lookup exactly mirrors BaseRestoreCommand.validate's own lookup (superset/commands/restore.py — find_by_id with skip_visibility_filter=True, base_filter intact, with a docstring stating 'Restore should not see rows the user cannot see in the live UI'). If the pre-lookup bypassed base_filter, the tool would surface rows the command itself would then refuse, producing a confusing found-then-denied flow that diverges from REST semantics. Editorship is enforced by the command after the visibility gate, same as the REST restore endpoint. ########## superset/mcp_service/dashboard/tool/restore_dashboard.py: ########## @@ -0,0 +1,162 @@ +# 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 +""" + +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 ( + DashboardForbiddenError, + DashboardNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + RestoreDashboardRequest, + RestoreDashboardResponse, +) +from superset.mcp_service.utils import ( + escape_llm_context_delimiters, + sanitize_for_llm_context, +) + +logger = logging.getLogger(__name__) + + +def _find_dashboard_for_restore(identifier: int | str) -> Any | None: + """Resolve a dashboard by numeric ID or UUID, including soft-deleted rows. + + ``skip_visibility_filter`` is the only bypass — the DAO ``base_filter`` + stays in effect, so rows the user cannot see in the live UI stay hidden. + Ownership is then enforced by ``RestoreDashboardCommand``. + """ + from superset.daos.dashboard import DashboardDAO + + return DashboardDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True) Review Comment: Declining: keeping base_filter in the pre-lookup exactly mirrors BaseRestoreCommand.validate's own lookup (superset/commands/restore.py — find_by_id with skip_visibility_filter=True, base_filter intact, with a docstring stating 'Restore should not see rows the user cannot see in the live UI'). If the pre-lookup bypassed base_filter, the tool would surface rows the command itself would then refuse, producing a confusing found-then-denied flow that diverges from REST semantics. Editorship is enforced by the command after the visibility gate, same as the REST restore endpoint. ########## tests/unit_tests/mcp_service/dashboard/tool/test_restore_dashboard.py: ########## @@ -0,0 +1,185 @@ +# 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_dashboard MCP tool. + +Run through the async MCP Client (not direct calls); auth is mocked via the +autouse mock_auth fixture, matching the other dashboard tool test files. +""" + +from collections.abc import Iterator +from datetime import datetime +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.dashboard.DashboardDAO.find_by_id_or_uuid" +_COMMAND = "superset.commands.dashboard.restore.RestoreDashboardCommand" + +_UUID = UUID("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee") Review Comment: Declining, standing rationale: module-level logger/string constants and locals with inferable types are unannotated across this codebase's mcp_service modules and test suites; mypy passes. Identical future findings will be declined the same way. -- 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]
