codeant-ai-for-open-source[bot] commented on code in PR #40129: URL: https://github.com/apache/superset/pull/40129#discussion_r3510308863
########## tests/integration_tests/charts/soft_delete_tests.py: ########## @@ -0,0 +1,560 @@ +# 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. +"""Integration tests for chart soft-delete and restore.""" + +from datetime import datetime + +from superset import security_manager +from superset.connectors.sqla.models import SqlaTable +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.extensions import db +from superset.models.dashboard import Dashboard, dashboard_slices +from superset.models.slice import Slice +from superset.reports.models import ( + ReportCreationMethod, + ReportSchedule, + ReportScheduleType, +) +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ( + ADMIN_USERNAME, + ALPHA_USERNAME, + GAMMA_USERNAME, +) +from tests.integration_tests.insert_chart_mixin import InsertChartMixin + + +def _hard_delete_chart(chart_id: int) -> None: + """Hard-delete a chart row regardless of soft-delete state.""" + row = ( + db.session.query(Slice) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}}) + .filter(Slice.id == chart_id) + .one_or_none() + ) Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Avoid flagging missing type annotations for local test-scaffolding variables in test files when the type is already unambiguous from context. **Applied to:** - `**/test/**` - `**/tests/**` - `**/*test*.<ext>` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## tests/integration_tests/charts/soft_delete_tests.py: ########## @@ -0,0 +1,560 @@ +# 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. +"""Integration tests for chart soft-delete and restore.""" + +from datetime import datetime + +from superset import security_manager +from superset.connectors.sqla.models import SqlaTable +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.extensions import db +from superset.models.dashboard import Dashboard, dashboard_slices +from superset.models.slice import Slice +from superset.reports.models import ( + ReportCreationMethod, + ReportSchedule, + ReportScheduleType, +) +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ( + ADMIN_USERNAME, + ALPHA_USERNAME, + GAMMA_USERNAME, +) +from tests.integration_tests.insert_chart_mixin import InsertChartMixin + + +def _hard_delete_chart(chart_id: int) -> None: + """Hard-delete a chart row regardless of soft-delete state.""" + row = ( + db.session.query(Slice) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}}) + .filter(Slice.id == chart_id) + .one_or_none() + ) + if row: + db.session.delete(row) + db.session.commit() + + +def _hard_delete_dashboard_for_charts_test(dashboard_id: int) -> None: + """Hard-delete a dashboard row regardless of soft-delete state.""" + row = ( + db.session.query(Dashboard) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}}) + .filter(Dashboard.id == dashboard_id) + .one_or_none() + ) Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Avoid flagging missing concrete type annotations for local/test-scaffolding variables in test files when the type is unambiguous from context. **Applied to:** - `**/test/**` - `**/tests/**` - `**/*test*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.py: ########## @@ -0,0 +1,52 @@ +# 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. +"""Add deleted_at column and index to slices for soft-delete. + +Adds a nullable ``deleted_at`` column and an index on it to the +``slices`` table to support soft deletion of charts. Companion to +the ``SoftDeleteMixin`` infrastructure shipped in PR #39977. + +Revision ID: 7c4a8d09ca37 +Revises: 78a40c08b4be +Create Date: 2026-05-08 12:00:00.000000 +""" + +from sqlalchemy import Column, DateTime + +from superset.migrations.shared.utils import ( + add_columns, + create_index, + drop_columns, + drop_index, +) + +# revision identifiers, used by Alembic. +revision = "7c4a8d09ca37" +down_revision = "78a40c08b4be" + +TABLE_NAME = "slices" +INDEX_NAME = f"ix_{TABLE_NAME}_deleted_at" Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not flag missing type annotations for obvious module-level constants in migration files; follow the surrounding migration file conventions. **Applied to:** - `**/migration*/**` - `**/*migration*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## tests/unit_tests/commands/chart/restore_test.py: ########## @@ -0,0 +1,90 @@ +# 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 RestoreChartCommand.""" + +from __future__ import annotations + +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest + +from superset.commands.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, +) +from superset.commands.chart.restore import RestoreChartCommand +from superset.exceptions import SupersetSecurityException + + +def test_restore_chart_clears_deleted_at(app_context: None) -> None: + """RestoreChartCommand.run() restores a soft-deleted chart.""" + chart = MagicMock() + chart.deleted_at = datetime(2026, 1, 1, tzinfo=timezone.utc) + chart.id = 1 + + with ( + patch( + "superset.daos.chart.ChartDAO.find_by_id", return_value=chart + ) as mock_find, + patch("superset.commands.restore.security_manager") as mock_sec, + ): + mock_sec.raise_for_ownership.return_value = None + + cmd = RestoreChartCommand("1") Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > In test files, do not require explicit type annotations for local test-scaffolding variables when the type is already unambiguous from context. **Applied to:** - `**/test/**` - `**/tests/**` - `**/*test*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## tests/integration_tests/charts/soft_delete_tests.py: ########## @@ -0,0 +1,560 @@ +# 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. +"""Integration tests for chart soft-delete and restore.""" + +from datetime import datetime + +from superset import security_manager +from superset.connectors.sqla.models import SqlaTable +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.extensions import db +from superset.models.dashboard import Dashboard, dashboard_slices +from superset.models.slice import Slice +from superset.reports.models import ( + ReportCreationMethod, + ReportSchedule, + ReportScheduleType, +) +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ( + ADMIN_USERNAME, + ALPHA_USERNAME, + GAMMA_USERNAME, +) +from tests.integration_tests.insert_chart_mixin import InsertChartMixin + + +def _hard_delete_chart(chart_id: int) -> None: + """Hard-delete a chart row regardless of soft-delete state.""" + row = ( + db.session.query(Slice) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}}) + .filter(Slice.id == chart_id) + .one_or_none() + ) + if row: + db.session.delete(row) + db.session.commit() + + +def _hard_delete_dashboard_for_charts_test(dashboard_id: int) -> None: + """Hard-delete a dashboard row regardless of soft-delete state.""" + row = ( + db.session.query(Dashboard) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}}) + .filter(Dashboard.id == dashboard_id) + .one_or_none() + ) + if row: + db.session.delete(row) + db.session.commit() + + +class TestChartSoftDelete(InsertChartMixin, SupersetTestCase): + """Tests for chart soft-delete behaviour (T013, T016).""" + + def test_delete_chart_soft_deletes(self) -> None: + """DELETE /api/v1/chart/<pk> sets deleted_at instead of removing.""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("soft_delete_test", [admin_id], 1) + chart_id = chart.id + self.login(ADMIN_USERNAME) + + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200 + + # Row still exists in DB with deleted_at set + row = ( + db.session.query(Slice) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}}) + .filter(Slice.id == chart_id) + .one_or_none() + ) + assert row is not None + assert row.deleted_at is not None + + # Cleanup + _hard_delete_chart(chart_id) + + def test_soft_deleted_chart_excluded_from_get(self) -> None: + """GET /api/v1/chart/<pk> returns 404 for a soft-deleted chart.""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("invisible_chart", [admin_id], 1) + chart_id = chart.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/chart/{chart_id}") + rv = self.client.get(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 404 + + # Cleanup + _hard_delete_chart(chart_id) + + def test_soft_deleted_chart_excluded_from_list(self) -> None: + """GET /api/v1/chart/ should not include soft-deleted charts.""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("listed_then_deleted", [admin_id], 1) + chart_id = chart.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/chart/{chart_id}") + rv = self.client.get("/api/v1/chart/") + data = json.loads(rv.data) + chart_ids = [c["id"] for c in data["result"]] + assert chart_id not in chart_ids + + # Cleanup + _hard_delete_chart(chart_id) + + def test_soft_deleted_chart_included_in_list_when_requested(self) -> None: + """GET /api/v1/chart/ with chart_deleted_state=include returns deleted charts.""" # noqa: E501 + admin_id = self.get_user("admin").id + chart = self.insert_chart("listed_with_deleted", [admin_id], 1) + chart_id = chart.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/chart/{chart_id}") + + rison_query = "(filters:!((col:id,opr:chart_deleted_state,value:include)))" + rv = self.client.get(f"/api/v1/chart/?q={rison_query}") + assert rv.status_code == 200 + + data = json.loads(rv.data) + deleted_row = next( + (row for row in data["result"] if row["id"] == chart_id), + None, + ) + assert deleted_row is not None + assert deleted_row["deleted_at"] is not None + + # Cleanup + _hard_delete_chart(chart_id) + + def test_only_filter_returns_only_soft_deleted_charts(self) -> None: + """chart_deleted_state=only excludes live rows and returns only deleted ones.""" + admin_id = self.get_user("admin").id + live_chart = self.insert_chart("only_live", [admin_id], 1) + deleted_chart = self.insert_chart("only_deleted", [admin_id], 1) + live_id = live_chart.id + deleted_id = deleted_chart.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/chart/{deleted_id}") + + rison_query = "(filters:!((col:id,opr:chart_deleted_state,value:only)))" + rv = self.client.get(f"/api/v1/chart/?q={rison_query}") + assert rv.status_code == 200 + + data = json.loads(rv.data) + returned_ids = {row["id"] for row in data["result"]} + assert deleted_id in returned_ids + assert live_id not in returned_ids + + # Cleanup + _hard_delete_chart(live_id) + _hard_delete_chart(deleted_id) + + def test_deleted_state_list_shows_owner_their_own_deleted(self) -> None: + """A non-admin owner can still enumerate their own soft-deleted charts. + Deleted-state scoping mirrors the restore audience, so it must not lock + owners out of their own trash.""" + alpha_id = self.get_user(ALPHA_USERNAME).id + chart = self.insert_chart("sd_owner_chart", [alpha_id], 1) + chart_id = chart.id + + chart.deleted_at = datetime(2026, 1, 1, 12, 0, 0) + db.session.commit() + + self.login(ALPHA_USERNAME) + rison_query = ( + "(filters:!((col:id,opr:chart_deleted_state,value:only)),page_size:200)" + ) + rv = self.client.get(f"/api/v1/chart/?q={rison_query}") + assert rv.status_code == 200 + ids = [c["id"] for c in json.loads(rv.data)["result"]] + assert chart_id in ids + + # Cleanup + _hard_delete_chart(chart_id) + + def test_deleted_state_list_hides_non_owned_from_read_access_user(self) -> None: + """A read-access non-owner must not enumerate a chart once it is + soft-deleted. + + Gamma is granted ``datasource_access`` to the chart's dataset, so + ``ChartFilter`` makes the chart visible to gamma while it is live. After + soft-delete, the deleted-state list is scoped to the restore audience + (owners/admins), so gamma — who could never restore it — must not see it + via ``include`` or ``only``. + """ + admin_id = self.get_user(ADMIN_USERNAME).id + chart = self.insert_chart("sd_acl_chart", [admin_id], 1) + chart_id = chart.id + + table = db.session.query(SqlaTable).get(1) + gamma_role = security_manager.find_role("Gamma") + pvm = security_manager.add_permission_view_menu("datasource_access", table.perm) + gamma_role.permissions.append(pvm) + db.session.commit() + + try: + # Precondition: gamma can see the chart while it is live. + self.login(GAMMA_USERNAME) + rv = self.client.get("/api/v1/chart/?q=(page_size:200)") + assert chart_id in [c["id"] for c in json.loads(rv.data)["result"]], ( + "precondition: gamma should see the live chart via datasource access" + ) + + # Soft-delete directly (avoids a mid-test re-login to admin). + reloaded = ( + db.session.query(Slice) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}}) + .filter(Slice.id == chart_id) + .one() + ) + reloaded.deleted_at = datetime(2026, 1, 1, 12, 0, 0) + db.session.commit() + + # Gamma must not see the soft-deleted chart in either mode. + for value in ("include", "only"): + rison_query = ( + f"(filters:!((col:id,opr:chart_deleted_state,value:{value}))," + "page_size:200)" + ) + rv = self.client.get(f"/api/v1/chart/?q={rison_query}") + assert rv.status_code == 200 + ids = [c["id"] for c in json.loads(rv.data)["result"]] + assert chart_id not in ids, ( + "read-access non-owner must not enumerate a soft-deleted " + f"chart via chart_deleted_state={value}" + ) + finally: + pvm = security_manager.find_permission_view_menu( + "datasource_access", table.perm + ) + if pvm: + security_manager.del_permission_role(gamma_role, pvm) + db.session.commit() + _hard_delete_chart(chart_id) + + def test_delete_already_soft_deleted_chart_returns_404(self) -> None: + """DELETE on an already soft-deleted chart returns 404 (FR-008).""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("double_delete_test", [admin_id], 1) + chart_id = chart.id + self.login(ADMIN_USERNAME) + + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200 + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 404 + + # Cleanup + _hard_delete_chart(chart_id) + + def test_delete_chart_blocked_when_active_report_references_it(self) -> None: + """DELETE /api/v1/chart/<id> returns 422 when a report references it. + + Pins down the existing API protection in `DeleteChartCommand.validate()`: + when a `report_schedule` row references the chart, the validation + raises `ChartDeleteFailedReportsExistError` *before* `ChartDAO.delete()` + is invoked, so no soft-delete routing happens. This is the contract + soft-delete inherits from the pre-existing API and is what makes the + "report-execution against soft-deleted target" crash class + (commands/report/execute.py:_get_url) unreachable through the API. + """ + admin_id = self.get_user("admin").id + chart = self.insert_chart("blocked_by_report_test", [admin_id], 1) + chart_id = chart.id + + report = ReportSchedule( + type=ReportScheduleType.REPORT, + name="blocking_report_for_chart_delete", + description="Report that should block chart deletion", + crontab="0 9 * * *", + chart=chart, + creation_method=ReportCreationMethod.ALERTS_REPORTS, + ) + db.session.add(report) + db.session.commit() + report_id = report.id + + self.login(ADMIN_USERNAME) + + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 422 + body = json.loads(rv.data) + assert "associated alerts or reports" in body.get("message", "").lower() or ( + "associated" in body.get("message", "").lower() + and "report" in body.get("message", "").lower() + ) + assert "blocking_report_for_chart_delete" in body.get("message", "") + + # Confirm the chart was NOT soft-deleted (deleted_at remains NULL). + row = ( + db.session.query(Slice) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}}) + .filter(Slice.id == chart_id) + .one() + ) + assert row.deleted_at is None + + # Cleanup + db.session.delete( + db.session.query(ReportSchedule) + .filter(ReportSchedule.id == report_id) + .one() + ) + db.session.commit() + _hard_delete_chart(chart_id) + + +class TestChartRestore(InsertChartMixin, SupersetTestCase): + """Tests for chart restore behaviour (T025).""" + + def test_restore_soft_deleted_chart(self) -> None: + """POST /api/v1/chart/<uuid>/restore makes the chart visible again.""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("restore_test", [admin_id], 1) + chart_id = chart.id + chart_uuid = str(chart.uuid) + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/chart/{chart_id}") + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 200 + + rv = self.client.get(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200 + + # Cleanup + _hard_delete_chart(chart_id) + + def test_restore_failure_returns_422(self) -> None: + """A failure during restore surfaces as a clean 422 via the + ``ChartRestoreFailedError`` handler rather than an unhandled 500. + + ``RestoreChartCommand.run`` wraps the restore in ``@transaction`` + and rethrows ``ChartRestoreFailedError`` on any underlying + SQLAlchemy error; this pins that the endpoint maps it to 422. + """ + from unittest.mock import patch + + from superset.commands.chart.exceptions import ( + ChartRestoreFailedError, + ) + + admin_id = self.get_user("admin").id + chart = self.insert_chart("restore_fail_test", [admin_id], 1) + chart_id = chart.id + chart_uuid = str(chart.uuid) + self.login(ADMIN_USERNAME) + self.client.delete(f"/api/v1/chart/{chart_id}") + + with patch( + "superset.commands.chart.restore.RestoreChartCommand.run", + side_effect=ChartRestoreFailedError(), + ): + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 422 + + # Cleanup + _hard_delete_chart(chart_id) + + def test_restore_nonexistent_chart_returns_404(self) -> None: + """POST /api/v1/chart/<uuid>/restore returns 404 for unknown UUID.""" + self.login(ADMIN_USERNAME) + rv = self.client.post( + "/api/v1/chart/00000000-0000-0000-0000-000000000000/restore" + ) + assert rv.status_code == 404 + + def test_restore_active_chart_returns_404(self) -> None: + """POST /api/v1/chart/<uuid>/restore on active chart returns 404.""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("active_restore_test", [admin_id], 1) + chart_id = chart.id + chart_uuid = str(chart.uuid) + self.login(ADMIN_USERNAME) + + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 404 + + # Cleanup + _hard_delete_chart(chart_id) + + def test_restore_uses_can_write_permission(self) -> None: + """Non-admin owner with ``can_write_Chart`` can hit the restore + endpoint. + + Pins the permission contract: ``method_permission_name`` must map + ``restore`` to ``write`` so FAB's ``@protect`` resolves the gate to + ``can_write_Chart`` (which Alpha already carries), not the implicit + fallback ``can_restore_Chart`` (which no standard role carries). + + Without the mapping FAB defaults to ``can_<method>_<class>`` and + every non-admin would get 403 here — admins bypass FAB permission + checks entirely, so the admin-authed restore tests above don't + exercise the mapping. + """ + alpha = self.get_user(ALPHA_USERNAME) + chart = self.insert_chart("restore_perm_test", [alpha.id], 1) + chart_id = chart.id + chart_uuid = str(chart.uuid) + + self.login(ALPHA_USERNAME) + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200, ( + f"Alpha owner soft-delete failed: {rv.status_code} {rv.data!r}" + ) + + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 200, ( + f"Expected 200 from Alpha owner restore (can_write_Chart), got " + f"{rv.status_code}: {rv.data!r}. If 403, " + "method_permission_name is missing 'restore': 'write'." + ) + + # Cleanup + _hard_delete_chart(chart_id) + + def test_restore_chart_reattaches_to_dashboards(self) -> None: + """Soft-deleting a chart preserves dashboard_slices junction rows; + restore makes the chart reappear in its dashboards automatically. + + This is the positive test that pins down the SIP's "no cascade" + contract and the corrected commit ``feat(soft-delete): preserve + dashboard_slices on chart soft-delete (MissingChart handles UI)``. + Soft-delete leaves the junction intact so: + + - dashboards continue to render the chart slot (frontend uses + ``MissingChart`` placeholder while the chart is hidden via the + visibility filter) + - on restore the chart is automatically a member of every + dashboard it was a member of before, with no manual + re-attachment step + """ + admin = self.get_user("admin") + admin_id = admin.id + + chart = self.insert_chart("reattach_test_chart", [admin_id], 1) + chart_id = chart.id + chart_uuid = str(chart.uuid) + + dashboard = Dashboard( + dashboard_title="reattach_test_dashboard", + slug="slug_reattach_test", + owners=[admin], + published=True, + ) + dashboard.slices = [chart] + db.session.add(dashboard) + db.session.commit() + dashboard_id = dashboard.id + + # Sanity: the junction row exists + junction_count = ( + db.session.query(dashboard_slices) + .filter( + dashboard_slices.c.dashboard_id == dashboard_id, + dashboard_slices.c.slice_id == chart_id, + ) + .count() + ) + assert junction_count == 1, "junction row should exist after dashboard creation" + + self.login(ADMIN_USERNAME) + + # Soft-delete the chart + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200 + + # The junction row is preserved (no cascade) + junction_count_after_delete = ( + db.session.query(dashboard_slices) + .filter( + dashboard_slices.c.dashboard_id == dashboard_id, + dashboard_slices.c.slice_id == chart_id, + ) + .count() + ) + assert junction_count_after_delete == 1, ( + "junction row should remain intact on chart soft-delete; " + "MissingChart placeholder handles the UI gap" + ) + + # The dashboard's loaded `slices` collection no longer includes the + # soft-deleted chart (the global visibility filter applies to + # relationship loads via `with_loader_criteria(..., include_aliases=True)`). + db.session.expire_all() + dashboard_after_delete = ( + db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one() + ) + assert chart_id not in [s.id for s in dashboard_after_delete.slices], ( + "soft-deleted chart should be filtered out of dashboard.slices " + "by the visibility-filter listener" + ) + + # Restore the chart + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 200 + + # The chart automatically reappears in the dashboard — junction row + # was preserved, so no manual reattach was needed. + db.session.expire_all() + dashboard_after_restore = ( + db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one() + ) + assert chart_id in [s.id for s in dashboard_after_restore.slices], ( + "restored chart should reappear in dashboard.slices automatically; " + "the junction row was never removed by soft-delete" + ) + + # Cleanup + _hard_delete_dashboard_for_charts_test(dashboard_id) + _hard_delete_chart(chart_id) + + def test_restore_chart_by_non_admin_owner(self) -> None: + """Non-admin owners can restore their own soft-deleted charts. + + The unit-level restore command tests mock security; this + integration test exercises the FAB security wiring end-to-end + so a future change that breaks the owner check on a non-admin + path can't slip through. + """ + alpha = self.get_user(ALPHA_USERNAME) + alpha_id = alpha.id + + chart = self.insert_chart("alpha_owned_chart", [alpha_id], 1) + chart_id = chart.id + chart_uuid = str(chart.uuid) + + self.login(ALPHA_USERNAME) + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200 + + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 200, rv.data + + db.session.expire_all() + restored = db.session.query(Slice).filter(Slice.id == chart_id).one_or_none() Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not require explicit type annotations for local test-scaffolding variables when the type is unambiguous from context; follow the surrounding file's existing conventions. **Applied to:** - `tests/integration_tests/charts/**` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## tests/unit_tests/dao/dashboard_test.py: ########## @@ -0,0 +1,95 @@ +# 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. +from datetime import datetime, timezone +from typing import Any + +from sqlalchemy.orm.session import Session + + +def test_set_dash_metadata_preserves_soft_deleted_members( + session: Session, +) -> None: + """Saving a dashboard must not sever a soft-deleted member chart. + + ``set_dash_metadata`` rebuilds ``dashboard.slices`` wholesale from the + incoming position data. The slice-resolution query must bypass the + soft-delete visibility filter: with the filter active, a member chart + sitting in the trash would be silently dropped from the assignment — + deleting its ``dashboard_slices`` junction row (breaking the + restore-reattach contract) and writing ``uuid: None`` into its + position slot. This test fails if the bypass is removed. + """ + from superset import db + from superset.connectors.sqla.models import Database, SqlaTable + from superset.daos.dashboard import DashboardDAO + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + + Dashboard.metadata.create_all(session.get_bind()) + + dataset = SqlaTable( + table_name="dash_meta_table", + database=Database(database_name="dash_meta_db", sqlalchemy_uri="sqlite://"), + ) + db.session.add(dataset) + db.session.flush() + + live_chart = Slice( + slice_name="live_chart", + datasource_id=dataset.id, + datasource_type="table", + ) + trashed_chart = Slice( + slice_name="trashed_chart", + datasource_id=dataset.id, + datasource_type="table", + deleted_at=datetime(2026, 1, 1, tzinfo=timezone.utc), + ) + dashboard = Dashboard( + dashboard_title="meta_test_dash", + slices=[live_chart, trashed_chart], + published=True, + ) + db.session.add_all([live_chart, trashed_chart, dashboard]) + db.session.flush() + + positions: dict[str, dict[str, Any]] = { + "CHART-live": { + "type": "CHART", + "id": "CHART-live", + "children": [], + "meta": {"chartId": live_chart.id, "width": 4, "height": 50}, + }, + "CHART-trashed": { + "type": "CHART", + "id": "CHART-trashed", + "children": [], + "meta": {"chartId": trashed_chart.id, "width": 4, "height": 50}, + }, + } + + DashboardDAO.set_dash_metadata(dashboard, {"positions": positions}) + + member_ids = {chart.id for chart in dashboard.slices} Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not require explicit type annotations for local test-scaffolding variables whose types are unambiguous from context in test files. **Applied to:** - `**/test/**` - `**/tests/**` - `**/*test*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## superset/commands/chart/importers/v1/utils.py: ########## @@ -48,21 +49,107 @@ def import_chart( overwrite: bool = False, ignore_permissions: bool = False, ) -> Slice: + """Import a chart from a config dict, handling existing matches. + + Permission model for an existing UUID match: + + +--------------+---------------+---------------------+-----------------+ + | Existing row | overwrite arg | Caller has perms? | Outcome | + +==============+===============+=====================+=================+ + | alive | False | (n/a) | return existing | + +--------------+---------------+---------------------+-----------------+ + | alive | True | can_write + owner | UPDATE in place | + +--------------+---------------+---------------------+-----------------+ + | alive | True | can_write, | raise | + | | | not owner/admin | | + +--------------+---------------+---------------------+-----------------+ + | soft-deleted | False or True | can_write + owner | restore + UPDATE| + +--------------+---------------+---------------------+-----------------+ + | soft-deleted | False or True | can_write, | raise | + | | | not owner/admin | | + +--------------+---------------+---------------------+-----------------+ + | soft-deleted | False or True | not can_write | raise (Case B) | + +--------------+---------------+---------------------+-----------------+ + + Re-importing a soft-deleted UUID is implicitly a restore-with-update: + the user is bringing the chart back by uploading it again. We apply + the same ownership check as the explicit overwrite path so non-owners + cannot resurrect via re-import, and we raise rather than silently + returning a soft-deleted row to callers without write permission + (which would let them reattach dashboards to a deleted chart). + """ can_write = ignore_permissions or security_manager.can_access("can_write", "Chart") - existing = db.session.query(Slice).filter_by(uuid=config["uuid"]).first() + # `user` is None for background / example-loader paths (no Flask request + # user). Combined with ``can_write=True`` (typically from + # ``ignore_permissions=True``), the ownership checks in the restore / + # overwrite branches below are intentionally skipped because the caller has + # already established trust at the command level. user = get_user() - if existing: - if overwrite and can_write and user: - if not security_manager.can_access_chart(existing) or ( - user not in existing.owners and not security_manager.is_admin() + + if existing := find_existing_for_import(Slice, config["uuid"]): Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not require explicit type annotations for clearly inferred local variables when this file's existing conventions already use unannotated, context-obvious locals. **Applied to:** - `superset/commands/chart/importers/v1/**` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## tests/unit_tests/commands/chart/restore_test.py: ########## @@ -0,0 +1,90 @@ +# 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 RestoreChartCommand.""" + +from __future__ import annotations + +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch + +import pytest + +from superset.commands.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, +) +from superset.commands.chart.restore import RestoreChartCommand +from superset.exceptions import SupersetSecurityException + + +def test_restore_chart_clears_deleted_at(app_context: None) -> None: + """RestoreChartCommand.run() restores a soft-deleted chart.""" + chart = MagicMock() + chart.deleted_at = datetime(2026, 1, 1, tzinfo=timezone.utc) + chart.id = 1 + + with ( + patch( + "superset.daos.chart.ChartDAO.find_by_id", return_value=chart + ) as mock_find, + patch("superset.commands.restore.security_manager") as mock_sec, + ): + mock_sec.raise_for_ownership.return_value = None + + cmd = RestoreChartCommand("1") + cmd.run() + + mock_find.assert_called_once() + chart.restore.assert_called_once() + + +def test_restore_chart_not_found_raises(app_context: None) -> None: + """RestoreChartCommand raises ChartNotFoundError for missing chart.""" + with patch("superset.daos.chart.ChartDAO.find_by_id", return_value=None): + cmd = RestoreChartCommand("999") + with pytest.raises(ChartNotFoundError): + cmd.run() + + +def test_restore_active_chart_raises_not_found(app_context: None) -> None: + """RestoreChartCommand raises ChartNotFoundError for non-deleted chart.""" + chart = MagicMock() + chart.deleted_at = None # not soft-deleted + + with patch("superset.daos.chart.ChartDAO.find_by_id", return_value=chart): + cmd = RestoreChartCommand("1") + with pytest.raises(ChartNotFoundError): + cmd.run() + + +def test_restore_chart_forbidden_raises(app_context: None) -> None: + """RestoreChartCommand raises ChartForbiddenError on permission check.""" + chart = MagicMock() Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not flag missing type annotations for local test-scaffolding variables in test files when the type is already unambiguous from context. **Applied to:** - `**/test/**` - `**/tests/**` - `**/*test*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* -- 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]
