codeant-ai-for-open-source[bot] commented on code in PR #40128: URL: https://github.com/apache/superset/pull/40128#discussion_r3349989818
########## tests/integration_tests/dashboards/soft_delete_tests.py: ########## @@ -0,0 +1,453 @@ +# 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 dashboard soft-delete and restore.""" + +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.extensions import db +from superset.models.dashboard import Dashboard +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME, ALPHA_USERNAME + + +def _hard_delete_dashboard(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 TestDashboardSoftDelete(SupersetTestCase): + """Tests for dashboard soft-delete behaviour (T014, T017).""" + + def _create_dashboard(self, title: str = "soft_delete_test") -> Dashboard: + admin = self.get_user("admin") + dashboard = Dashboard( + dashboard_title=title, + slug=f"slug_{title}", + owners=[admin], + published=True, + ) + db.session.add(dashboard) + db.session.commit() + return dashboard + + def test_delete_dashboard_soft_deletes(self) -> None: + """DELETE should set deleted_at instead of removing the row.""" + dashboard = self._create_dashboard("sd_test_1") + dashboard_id = dashboard.id + self.login(ADMIN_USERNAME) + + rv = self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + assert rv.status_code == 200 + + row = ( + db.session.query(Dashboard) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}}) + .filter(Dashboard.id == dashboard_id) + .one_or_none() + ) + assert row is not None + assert row.deleted_at is not None + + # Cleanup + _hard_delete_dashboard(dashboard_id) + + def test_soft_deleted_dashboard_excluded_from_list(self) -> None: + """GET /api/v1/dashboard/ should not include soft-deleted.""" + dashboard = self._create_dashboard("sd_list_test") + dashboard_id = dashboard.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + + rv = self.client.get("/api/v1/dashboard/") + data = json.loads(rv.data) + ids = [d["id"] for d in data["result"]] + assert dashboard_id not in ids + + # Cleanup + _hard_delete_dashboard(dashboard_id) + + def test_soft_deleted_dashboard_included_in_list_when_requested(self) -> None: + """GET /api/v1/dashboard/ with dashboard_deleted_state=include returns deleted dashboards.""" # noqa: E501 + dashboard = self._create_dashboard("sd_list_with_deleted") + dashboard_id = dashboard.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + + rison_query = "(filters:!((col:id,opr:dashboard_deleted_state,value:include)))" + rv = self.client.get(f"/api/v1/dashboard/?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"] == dashboard_id), + None, + ) + assert deleted_row is not None + assert deleted_row["deleted_at"] is not None + + # Cleanup + _hard_delete_dashboard(dashboard_id) + + def test_only_filter_returns_only_soft_deleted_dashboards(self) -> None: + """dashboard_deleted_state=only excludes live rows and returns only deleted ones.""" # noqa: E501 + live_dashboard = self._create_dashboard("only_live_dash") + deleted_dashboard = self._create_dashboard("only_deleted_dash") + live_id = live_dashboard.id + deleted_id = deleted_dashboard.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/dashboard/{deleted_id}") + + rison_query = "(filters:!((col:id,opr:dashboard_deleted_state,value:only)))" + rv = self.client.get(f"/api/v1/dashboard/?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_dashboard(live_id) + _hard_delete_dashboard(deleted_id) + + def test_embedded_dashboard_with_soft_deleted_parent(self) -> None: + """Embedded URL keeps loading after the parent dashboard is soft-deleted. + + The embedded view (`embedded/view.py:embedded`) only reads + `embedded.allowed_domains` and `embedded.dashboard_id` (FK column, + not relationship), so it never dereferences the soft-deleted + Dashboard via `embedded.dashboard`. Iframe still returns 200; the + frontend's subsequent `/api/v1/dashboard/<id>` fetch returns 404 + cleanly via the visibility filter, and the user sees the standard + "dashboard not found" UI rather than a 500. + + This pins down the contract documented in pr-readiness.md #8 and + prevents future changes to either the embedded view or the schema + from regressing it into a 500. + """ + from unittest import mock + + from superset.daos.dashboard import EmbeddedDashboardDAO + + dashboard = self._create_dashboard("embedded_soft_delete_test") + dashboard_id = dashboard.id + embedded = EmbeddedDashboardDAO.upsert(dashboard, []) + db.session.flush() + embedded_uuid = str(embedded.uuid) + self.login(ADMIN_USERNAME) + + # Soft-delete the parent + self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + + # The dashboard fetch returns 404 cleanly (visibility filter applies). + rv = self.client.get(f"/api/v1/dashboard/{dashboard_id}") + assert rv.status_code == 404, ( + "Soft-deleted dashboard should fetch 404, not 500; got " + f"{rv.status_code}. Body: {rv.data[:200]!r}" + ) + + # The embedded iframe URL still loads (200) — embedded.dashboard is + # never dereferenced by the view. Done last because the embedded + # handler clears the session in CI, which would 401 any follow-up + # API call. + with mock.patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", + EMBEDDED_SUPERSET=True, + ): + rv = self.client.get(f"/embedded/{embedded_uuid}") + assert rv.status_code == 200, ( + "Embedded view should still load 200 with a soft-deleted parent; " + f"got {rv.status_code}. Body: {rv.data[:200]!r}" + ) + + # Cleanup: hard-deleting the dashboard cascades to the embedded + # row via the FK ondelete=CASCADE. + _hard_delete_dashboard(dashboard_id) + + +class TestDashboardRestore(SupersetTestCase): + """Tests for dashboard restore behaviour (T026, T028).""" + + def _skip_if_no_partial_index(self) -> None: + """Skip the test on dialects that fall back to the full unique slug + constraint. + + The 9e1f3b8c4d2a migration installs a partial unique index on + ``slug WHERE deleted_at IS NULL`` on PostgreSQL and MySQL 8.0+ + (excluding MariaDB). SQLite and MySQL <8 / MariaDB keep the + original full constraint, so tests that exercise slug reuse + across the soft-delete boundary are not meaningful on those + backends. + """ + dialect = db.session.bind.dialect.name + is_mariadb = getattr(db.session.bind.dialect, "is_mariadb", False) + server_version = db.session.bind.dialect.server_version_info or () + partial_index_supported = dialect == "postgresql" or ( + dialect == "mysql" and not is_mariadb and server_version >= (8, 0) + ) Review Comment: **Suggestion:** The dialect gate treats all MySQL 8.0.x versions as supporting the partial slug index, but the migration only enables that path for MySQL 8.0.13+. On MySQL 8.0.0–8.0.12 these tests will run when they should be skipped and will fail for environment/version reasons instead of product behavior; align this check with the migration's functional-index minimum. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Dashboard soft-delete tests fail on MySQL 8.0.0–8.0.12. - ⚠️ CI pipelines using old MySQL versions report spurious failures. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure the test environment to use a MySQL 8.0.0–8.0.12 database so that `bind.dialect.server_version_info` is `(8, 0, X)` where `X < 13`. 2. Apply migration `superset/migrations/versions/2026-05-08_12-05_9e1f3b8c4d2a_add_deleted_at_to_dashboards.py`, where `_mysql_supports_functional_index()` at lines 76–95 only returns True for MySQL versions `>= (8, 0, 13)`, so `_replace_slug_constraint_with_partial_index()` at lines 119–148 leaves the original full unique slug index in place on 8.0.0–8.0.12. 3. Run the integration tests in `tests/integration_tests/dashboards/soft_delete_tests.py`; in `TestDashboardRestore._skip_if_no_partial_index()` at lines 196–213, `partial_index_supported` is computed as True whenever `dialect == "mysql"` and `server_version >= (8, 0)` (lines 207–212), so on MySQL 8.0.0–8.0.12 this method does *not* call `skipTest`. 4. Execute `TestDashboardRestore.test_restore_blocked_by_active_slug_twin` at lines 70–135: after creating and soft-deleting the first dashboard (lines 86–100), the test calls `_skip_if_no_partial_index()` at line 105, which incorrectly returns without skipping, then creates a second `Dashboard` with the same `slug` at lines 107–115; because the migration never installed the partial/functional index on this MySQL version, the full unique slug constraint still applies and the insert raises a unique-index violation instead of allowing the configuration the test expects. 5. Similarly, run `TestDashboardRestore.test_partial_index_allows_multiple_soft_deleted_with_same_slug` at lines 137–194 (after the initial `_skip_if_no_partial_index()` gate at line 145 is incorrectly passed on MySQL 8.0.0–8.0.12); when the test attempts to soft-delete two dashboards sharing the same slug (lines 150–177), the underlying schema still enforces a full unique index on `slug`, causing integrity errors or assertion failures unrelated to product behavior. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5448d396aa3a4ef8bb49e8422abbe00d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5448d396aa3a4ef8bb49e8422abbe00d&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/integration_tests/dashboards/soft_delete_tests.py **Line:** 210:212 **Comment:** *Logic Error: The dialect gate treats all MySQL 8.0.x versions as supporting the partial slug index, but the migration only enables that path for MySQL 8.0.13+. On MySQL 8.0.0–8.0.12 these tests will run when they should be skipped and will fail for environment/version reasons instead of product behavior; align this check with the migration's functional-index minimum. 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%2F40128&comment_hash=e2cd5b77d2bb60bafb0ed13c51bd7ec563180b29efd43204a8f48b7bdb60a74b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=e2cd5b77d2bb60bafb0ed13c51bd7ec563180b29efd43204a8f48b7bdb60a74b&reaction=dislike'>👎</a> ########## tests/integration_tests/dashboards/soft_delete_tests.py: ########## @@ -0,0 +1,453 @@ +# 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 dashboard soft-delete and restore.""" + +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.extensions import db +from superset.models.dashboard import Dashboard +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME, ALPHA_USERNAME + + +def _hard_delete_dashboard(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 TestDashboardSoftDelete(SupersetTestCase): + """Tests for dashboard soft-delete behaviour (T014, T017).""" + + def _create_dashboard(self, title: str = "soft_delete_test") -> Dashboard: + admin = self.get_user("admin") + dashboard = Dashboard( + dashboard_title=title, + slug=f"slug_{title}", + owners=[admin], + published=True, + ) + db.session.add(dashboard) + db.session.commit() + return dashboard + + def test_delete_dashboard_soft_deletes(self) -> None: + """DELETE should set deleted_at instead of removing the row.""" + dashboard = self._create_dashboard("sd_test_1") + dashboard_id = dashboard.id + self.login(ADMIN_USERNAME) + + rv = self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + assert rv.status_code == 200 + + row = ( + db.session.query(Dashboard) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}}) + .filter(Dashboard.id == dashboard_id) + .one_or_none() + ) + assert row is not None + assert row.deleted_at is not None + + # Cleanup + _hard_delete_dashboard(dashboard_id) + + def test_soft_deleted_dashboard_excluded_from_list(self) -> None: + """GET /api/v1/dashboard/ should not include soft-deleted.""" + dashboard = self._create_dashboard("sd_list_test") + dashboard_id = dashboard.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + + rv = self.client.get("/api/v1/dashboard/") + data = json.loads(rv.data) + ids = [d["id"] for d in data["result"]] + assert dashboard_id not in ids + + # Cleanup + _hard_delete_dashboard(dashboard_id) + + def test_soft_deleted_dashboard_included_in_list_when_requested(self) -> None: + """GET /api/v1/dashboard/ with dashboard_deleted_state=include returns deleted dashboards.""" # noqa: E501 + dashboard = self._create_dashboard("sd_list_with_deleted") + dashboard_id = dashboard.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + + rison_query = "(filters:!((col:id,opr:dashboard_deleted_state,value:include)))" + rv = self.client.get(f"/api/v1/dashboard/?q={rison_query}") + assert rv.status_code == 200 + + data = json.loads(rv.data) + deleted_row = next( Review Comment: **Suggestion:** The test calls `skipTest` only after creating and soft-deleting a dashboard, so on unsupported dialects the skip aborts before cleanup and leaves persisted test data behind. Move the skip check to the top of the test (before any writes) or guard setup/cleanup with `try/finally` to avoid leaking state across integration tests. [missing cleanup] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Skipped test leaves soft-deleted dashboard rows in integration database. - ⚠️ Later dashboard tests may flake due to leaked state. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the integration test suite against a dialect without partial-index support, for example SQLite (`dialect.name == "sqlite"`) or MySQL <8.0 / MariaDB, using the app created in `tests/integration_tests/test_app.py:28–32`. 2. Execute `TestDashboardRestore.test_restore_blocked_by_active_slug_twin` in `tests/integration_tests/dashboards/soft_delete_tests.py` starting at line 70: the test creates a `Dashboard` named `conflict_first` with slug `slug_conflict_test` and commits it at lines 86–95. 3. At lines 98–100, the test logs in as admin and calls `self.client.delete(f"/api/v1/dashboard/{first_id}")`, which exercises the `/api/v1/dashboard/<id>` DELETE endpoint and sets `Dashboard.deleted_at`, committing through the normal application session. 4. Immediately after the soft-delete, the test calls `self._skip_if_no_partial_index()` at line 105; for unsupported dialects, `_skip_if_no_partial_index()` (lines 196–213) computes `partial_index_supported` as False and calls `self.skipTest(...)` at lines 213–217, raising `SkipTest` and aborting the test. 5. Because `skipTest` is raised before the cleanup block at lines 133–135, `_hard_delete_dashboard(first_id)` is never invoked for the first dashboard; `SupersetTestCase.tearDown()` in `tests/integration_tests/base_tests.py:123–125` only performs `self.logout()` and does not clean the `Dashboard` table, so the soft-deleted row remains in the integration database. 6. Subsequent tests in the same suite that assume a clean dashboard table or rely on slug uniqueness (for example, other dashboard creation or soft-delete tests in `soft_delete_tests.py`) now see this leaked soft-deleted row, making outcomes depend on test ordering and backend dialect. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=195dd44251a944db9c0a5fce03fc4143&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=195dd44251a944db9c0a5fce03fc4143&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/integration_tests/dashboards/soft_delete_tests.py **Line:** 98:105 **Comment:** *Missing Cleanup: The test calls `skipTest` only after creating and soft-deleting a dashboard, so on unsupported dialects the skip aborts before cleanup and leaves persisted test data behind. Move the skip check to the top of the test (before any writes) or guard setup/cleanup with `try/finally` to avoid leaking state across integration 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%2F40128&comment_hash=991d9ccadf0a965eb99405b82e1b8744ddaf6535aca37514bd10f3816fe70665&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=991d9ccadf0a965eb99405b82e1b8744ddaf6535aca37514bd10f3816fe70665&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]
