codeant-ai-for-open-source[bot] commented on code in PR #40128: URL: https://github.com/apache/superset/pull/40128#discussion_r3397959447
########## tests/integration_tests/dashboards/soft_delete_tests.py: ########## @@ -0,0 +1,717 @@ +# 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 datetime import datetime +from typing import Any + +from superset import security_manager +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, + GAMMA_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_deleted_state_list_shows_owner_their_own_deleted(self) -> None: + """A non-admin owner can still enumerate their own soft-deleted + dashboards. Deleted-state scoping mirrors the restore audience, so it + must not lock owners out of their own trash.""" + alpha = self.get_user("alpha") + dashboard = Dashboard( + dashboard_title="sd_owner_dash", + slug="sd_owner_dash", + owners=[alpha], + published=True, + ) + db.session.add(dashboard) + db.session.commit() + dashboard_id = dashboard.id + + self.login(ALPHA_USERNAME) + self.client.delete(f"/api/v1/dashboard/{dashboard_id}") + + rison_query = ( + "(filters:!((col:dashboard_title,opr:title_or_slug,value:sd_owner_dash)," + "(col:id,opr:dashboard_deleted_state,value:only)))" + ) + rv = self.client.get(f"/api/v1/dashboard/?q={rison_query}") + assert rv.status_code == 200 + ids = [row["id"] for row in json.loads(rv.data)["result"]] + assert dashboard_id in ids + + # Cleanup + _hard_delete_dashboard(dashboard_id) + + def test_deleted_state_list_hides_non_owned_from_read_access_user(self) -> None: + """A read-access non-owner must not be able to enumerate a dashboard + once it is soft-deleted. + + Gamma is granted ``datasource_access`` to a published dashboard's + dataset, so ``DashboardAccessFilter`` makes the dashboard 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``. + """ + from superset.connectors.sqla.models import SqlaTable + from superset.models.core import Database + from superset.models.slice import Slice + + admin = self.get_user("admin") + database = Database(database_name="sd_acl_db", sqlalchemy_uri="sqlite://") + db.session.add(database) + db.session.flush() + table = SqlaTable(table_name="sd_acl_tbl", database=database) + db.session.add(table) + db.session.flush() + chart = Slice( + slice_name="sd_acl_slice", + datasource_id=table.id, + datasource_type="table", + viz_type="table", + ) + db.session.add(chart) + dashboard = Dashboard( + dashboard_title="sd_acl_dash", + slug="sd_acl_dash", + owners=[admin], + slices=[chart], + published=True, + ) + db.session.add(dashboard) + db.session.commit() + dashboard_id = dashboard.id + + 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() + + title_filter = "(col:dashboard_title,opr:title_or_slug,value:sd_acl_dash)" + try: + # Precondition: gamma can see the dashboard while it is live. + self.login(GAMMA_USERNAME) + rv = self.client.get(f"/api/v1/dashboard/?q=(filters:!({title_filter}))") + live_ids = [row["id"] for row in json.loads(rv.data)["result"]] + assert dashboard_id in live_ids, ( + "precondition failed: gamma should see the live dashboard via " + "datasource access" + ) + + # Soft-delete directly (no admin re-login needed; the DELETE + # endpoint's auth is exercised elsewhere). This isolates the + # deleted-state visibility behaviour under test. + dashboard.deleted_at = datetime(2026, 1, 1, 12, 0, 0) + db.session.commit() + + # Gamma (still logged in) must not see the soft-deleted dashboard. + for value in ("include", "only"): + rison_query = ( + f"(filters:!({title_filter}," + f"(col:id,opr:dashboard_deleted_state,value:{value})))" + ) + rv = self.client.get(f"/api/v1/dashboard/?q={rison_query}") + assert rv.status_code == 200 + ids = [row["id"] for row in json.loads(rv.data)["result"]] + assert dashboard_id not in ids, ( + "read-access non-owner must not enumerate a soft-deleted " + f"dashboard via 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) + _hard_delete_dashboard(dashboard_id) + db.session.delete(chart) + db.session.delete(table) + db.session.delete(database) + db.session.commit() + + 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) Review Comment: **Suggestion:** The embedded record is only flushed, not committed, before issuing HTTP requests. Flask request teardown can remove/rollback the session, so the uncommitted `EmbeddedDashboard` row may disappear before `/embedded/<uuid>` is fetched, causing intermittent 404s unrelated to soft-delete behavior. Commit after `upsert` so the test persists its setup across request boundaries. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Embedded soft-delete test can intermittently fail with 404. - ⚠️ Flaky CI for embedded dashboards and soft-delete behavior. - ⚠️ Makes reproducing genuine embedded/soft-delete regressions harder. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run the integration test `TestDashboardSoftDelete.test_embedded_dashboard_with_soft_deleted_parent` in `tests/integration_tests/dashboards/soft_delete_tests.py:262-315`, which sets up a `Dashboard` and then calls `EmbeddedDashboardDAO.upsert(dashboard, [])` followed only by `db.session.flush()` at lines 283–285. 2. Observe that the test then performs multiple HTTP requests via `self.client`: first `POST /login/` in `SupersetTestCase.login` (base_tests.py:36-37), then `DELETE /api/v1/dashboard/<id>` and finally `GET /embedded/<uuid>` in the test body (soft_delete_tests.py:286-307), all using the Flask test client backed by the global Flask-SQLAlchemy session (`superset.db`). 3. During these requests, Flask-SQLAlchemy's teardown hooks roll back or remove the active session at the end of a request when there are uncommitted changes; because the `EmbeddedDashboard` row was only flushed, not committed, this teardown can discard the row between the DELETE and the subsequent `GET /embedded/<uuid>` call. 4. When `/embedded/<uuid>` is fetched, `EmbeddedView.embedded` in `superset/embedded/view.py:36-55` calls `EmbeddedDashboardDAO.find_by_id(uuid)`, which queries the database; if the uncommitted row was rolled back, `embedded` is `None`, `abort(404)` is triggered, and the test's final assertion expecting HTTP 200 at soft_delete_tests.py:47-51 fails intermittently with a 404 unrelated to the soft-delete behavior under test. ``` </details> [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9add6c3f82964bcc8e33048cf2769ffe&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=9add6c3f82964bcc8e33048cf2769ffe&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:** 283:285 **Comment:** *Logic Error: The embedded record is only flushed, not committed, before issuing HTTP requests. Flask request teardown can remove/rollback the session, so the uncommitted `EmbeddedDashboard` row may disappear before `/embedded/<uuid>` is fetched, causing intermittent 404s unrelated to soft-delete behavior. Commit after `upsert` so the test persists its setup across request boundaries. 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=67936e3084c69148b4d593626528ff67bd025e65d113d5a9069db7c62c0cc097&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40128&comment_hash=67936e3084c69148b4d593626528ff67bd025e65d113d5a9069db7c62c0cc097&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]
