codeant-ai-for-open-source[bot] commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3501639180
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -191,19 +193,34 @@ def create_virtual_datasets(self):
@pytest.fixture
def create_datasets(self):
with self.create_app().app_context():
+ # Purge any soft-deleted rows that occupy the unique constraint
+ stale = self.get_fixture_datasets()
+ for ds in stale:
+ db.session.delete(ds)
+ if stale:
+ db.session.commit()
Review Comment:
**Suggestion:** The pre-test purge is intended to remove only soft-deleted
fixture rows, but it currently deletes every matching dataset returned by
`get_fixture_datasets()`. Because that query now bypasses visibility filters,
this can hard-delete active rows too and cause cross-test state corruption.
Restrict the purge to rows where `deleted_at` is set before deleting.
[incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Fixture purge hard-deletes live datasets for AB tables.
- ⚠️ Later tests may run without expected fixture datasets.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `TestDatasetApi`
(tests/integration_tests/datasets/api_tests.py:70-120), create a
live dataset for one of `fixture_tables_names` by calling
`insert_default_dataset()` or
`insert_dataset()` so a `SqlaTable` row with `table_name` in
`("ab_permission",
"ab_permission_view", "ab_view_menu")` exists with `deleted_at is None`.
2. Verify the live dataset exists by calling `get_fixture_datasets()`
(tests/integration_tests/datasets/api_tests.py:155-162), which queries
`SqlaTable` with
`.execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {SqlaTable}})` and
`.filter(SqlaTable.table_name.in_(self.fixture_tables_names))`, returning
both active and
soft-deleted rows.
3. Run a test using the `create_datasets` fixture, e.g.
`test_delete_dataset_column` at
tests/integration_tests/datasets/api_tests.py:2045-2058, which triggers the
fixture body
at lines 196-203.
4. Observe in the fixture that the loop at
tests/integration_tests/datasets/api_tests.py:197-201 calls
`db.session.delete(ds)` for
every row returned by `get_fixture_datasets()` and commits, hard-deleting
active datasets
(not just soft-deleted ones) and potentially removing rows relied on by
other tests.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e25cab74025441be8ee678017798e9b7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e25cab74025441be8ee678017798e9b7&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/datasets/api_tests.py
**Line:** 196:201
**Comment:**
*Incorrect Condition Logic: The pre-test purge is intended to remove
only soft-deleted fixture rows, but it currently deletes every matching dataset
returned by `get_fixture_datasets()`. Because that query now bypasses
visibility filters, this can hard-delete active rows too and cause cross-test
state corruption. Restrict the purge to rows where `deleted_at` is set before
deleting.
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%2F40130&comment_hash=41a6238c5ae2299b26c28db4e50cd84be712f02be0d20a78aaadb28ae81628a1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=41a6238c5ae2299b26c28db4e50cd84be712f02be0d20a78aaadb28ae81628a1&reaction=dislike'>👎</a>
##########
tests/integration_tests/datasets/soft_delete_tests.py:
##########
@@ -0,0 +1,462 @@
+# 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 dataset 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.core import Database
+from superset.models.slice import Slice
+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 _restore_dataset(dataset_id: int) -> None:
+ """Restore a soft-deleted dataset (cleanup helper).
+
+ Module-level so every test class in this file can use it. Used in
+ ``finally`` blocks so a failed assertion can't strand a soft-deleted
+ row and leak it into later tests; re-queries with the
+ visibility-filter bypass and only restores if still soft-deleted.
+ """
+ row = (
+ db.session.query(SqlaTable)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {SqlaTable}})
+ .filter(SqlaTable.id == dataset_id)
+ .one_or_none()
+ )
+ if row is not None and row.deleted_at is not None:
+ row.restore()
+ db.session.commit()
+
+
+class TestDatasetSoftDelete(SupersetTestCase):
+ """Tests for dataset soft-delete behaviour (T015, T018)."""
+
+ def _get_example_dataset_id(self) -> int:
+ """Get an existing example dataset ID for testing."""
+ dataset = db.session.query(SqlaTable).first()
+ assert dataset is not None, "No datasets found — load examples first"
+ return dataset.id
+
+ def _restore_dataset(self, dataset_id: int) -> None:
+ """Class-method shim over the module-level helper (existing call
sites)."""
+ _restore_dataset(dataset_id)
+
+ def test_delete_dataset_soft_deletes(self) -> None:
+ """DELETE should set deleted_at instead of removing the row."""
+ dataset_id = self._get_example_dataset_id()
+ self.login(ADMIN_USERNAME)
+
+ try:
+ rv = self.client.delete(f"/api/v1/dataset/{dataset_id}")
+ assert rv.status_code == 200
+
+ row = (
+ db.session.query(SqlaTable)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES:
{SqlaTable}})
+ .filter(SqlaTable.id == dataset_id)
+ .one_or_none()
+ )
+ assert row is not None
+ assert row.deleted_at is not None
+ finally:
+ self._restore_dataset(dataset_id)
+
+ def test_soft_deleted_dataset_excluded_from_list(self) -> None:
+ """GET /api/v1/dataset/ should not include soft-deleted datasets."""
+ dataset_id = self._get_example_dataset_id()
+ self.login(ADMIN_USERNAME)
+
+ try:
+ rv = self.client.delete(f"/api/v1/dataset/{dataset_id}")
+ assert rv.status_code == 200
+
+ rv = self.client.get("/api/v1/dataset/")
+ data = json.loads(rv.data)
+ ids = [d["id"] for d in data["result"]]
+ assert dataset_id not in ids
+ finally:
+ self._restore_dataset(dataset_id)
+
+ def test_soft_deleted_dataset_included_in_list_when_requested(self) ->
None:
+ """GET /api/v1/dataset/ with dataset_deleted_state=include returns
deleted datasets.""" # noqa: E501
+ dataset_id = self._get_example_dataset_id()
+ self.login(ADMIN_USERNAME)
+
+ try:
+ rv = self.client.delete(f"/api/v1/dataset/{dataset_id}")
+ assert rv.status_code == 200
+
+ rison_query = (
+ "(filters:!((col:id,opr:dataset_deleted_state,value:include)))"
+ )
+ rv = self.client.get(f"/api/v1/dataset/?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"] == dataset_id),
+ None,
+ )
+ assert deleted_row is not None
+ assert deleted_row["deleted_at"] is not None
+ finally:
+ self._restore_dataset(dataset_id)
+
+ def test_only_filter_returns_only_soft_deleted_datasets(self) -> None:
+ """dataset_deleted_state=only excludes live rows and returns only
deleted ones.""" # noqa: E501
+ ids = [row.id for row in db.session.query(SqlaTable).limit(2).all()]
+ assert len(ids) >= 2, "Need at least two example datasets for this
test"
+ live_id, deleted_id = ids[0], ids[1]
+ self.login(ADMIN_USERNAME)
+
+ try:
+ rv = self.client.delete(f"/api/v1/dataset/{deleted_id}")
+ assert rv.status_code == 200
+
+ rison_query =
"(filters:!((col:id,opr:dataset_deleted_state,value:only)))"
+ rv = self.client.get(f"/api/v1/dataset/?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
+ finally:
+ self._restore_dataset(deleted_id)
+
+ def _hard_delete_created(self, dataset_id: int, database: Database) ->
None:
+ """Remove a test-created dataset + its database (visibility
bypassed)."""
+ row = (
+ db.session.query(SqlaTable)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {SqlaTable}})
+ .filter(SqlaTable.id == dataset_id)
+ .one_or_none()
+ )
+ if row:
+ db.session.delete(row)
+ db.session.delete(database)
+ db.session.commit()
+
+ def test_deleted_state_list_shows_owner_their_own_deleted(self) -> None:
+ """A non-admin owner can still enumerate their own soft-deleted
datasets.
+ Deleted-state scoping mirrors the restore audience, so it must not lock
+ owners out of their own trash."""
+ alpha = self.get_user(ALPHA_USERNAME)
+ database = Database(database_name="sd_owner_db",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+ dataset = SqlaTable(
+ table_name="sd_owner_tbl", database=database, owners=[alpha]
+ )
+ db.session.add(dataset)
+ db.session.commit()
+ dataset_id = dataset.id
+
+ dataset.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
+ db.session.commit()
+
+ self.login(ALPHA_USERNAME)
+ rison_query = (
+
"(filters:!((col:id,opr:dataset_deleted_state,value:only)),page_size:200)"
+ )
+ rv = self.client.get(f"/api/v1/dataset/?q={rison_query}")
+ assert rv.status_code == 200
+ ids = [r["id"] for r in json.loads(rv.data)["result"]]
+ assert dataset_id in ids
+
+ self._hard_delete_created(dataset_id, database)
Review Comment:
**Suggestion:** This test creates and soft-deletes persistent rows but
performs teardown only at the end of the happy path. If any assertion fails
earlier, cleanup is skipped and leaves leaked dataset/database rows that can
trigger later uniqueness collisions and test flakiness. Move teardown into a
`finally` block. [missing cleanup]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Test-created database and dataset can leak on failures.
- ⚠️ Subsequent tests may see stray ACL-scoped datasets.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`TestDatasetSoftDelete.test_deleted_state_list_shows_owner_their_own_deleted` in
tests/integration_tests/datasets/soft_delete_tests.py:164-191; it creates a
`Database`
named `"sd_owner_db"` and a `SqlaTable` `"sd_owner_tbl"` owned by Alpha
(lines 168-176)
and sets `deleted_at` on the dataset (lines 179-180).
2. The test logs in as Alpha and calls `GET
/api/v1/dataset/?q=...dataset_deleted_state=value:only` (lines 182-187),
then asserts the
soft-deleted dataset’s ID appears in the response (lines 187-189).
3. Cleanup is performed by `self._hard_delete_created(dataset_id, database)`
at line 191,
which hard-deletes the test-created dataset and database using a
visibility-filter-bypassing query (lines 151-162), but this call is not
inside a
`try/finally` block.
4. If any earlier step raises (for example, the response does not include
`dataset_id` and
the assertion at line 189 fails), the test aborts before calling
`_hard_delete_created`,
leaving the extra `Database` and `SqlaTable` rows in the database and
risking later
uniqueness collisions or unexpected extra rows with the same names.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e2e5221992264a09911a9383d5d62357&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e2e5221992264a09911a9383d5d62357&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/datasets/soft_delete_tests.py
**Line:** 182:191
**Comment:**
*Missing Cleanup: This test creates and soft-deletes persistent rows
but performs teardown only at the end of the happy path. If any assertion fails
earlier, cleanup is skipped and leaves leaked dataset/database rows that can
trigger later uniqueness collisions and test flakiness. Move teardown into a
`finally` block.
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%2F40130&comment_hash=483dcf74bf5976305432ca46df35e699bd956c9d840429884733a8f1a925a9b3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=483dcf74bf5976305432ca46df35e699bd956c9d840429884733a8f1a925a9b3&reaction=dislike'>👎</a>
##########
tests/unit_tests/commands/dataset/restore_test.py:
##########
@@ -0,0 +1,172 @@
+# 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 RestoreDatasetCommand."""
+
+from __future__ import annotations
+
+import uuid
+from datetime import datetime, timezone
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+# Stable UUIDs make the test boundary realistic: the production command
+# loads by UUID, not integer ID. The test only mocks the DAO lookup, but
+# the argument shape should still match.
+_DATASET_UUID = str(uuid.uuid4())
+_MISSING_UUID = str(uuid.uuid4())
Review Comment:
**Suggestion:** The comment says these UUIDs are stable, but using
`uuid.uuid4()` makes them random on every run. Update the comment (or switch to
fixed UUID literals) so the test documentation matches the actual behavior and
avoids misleading future maintainers. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Misleading comment about UUID stability in restore tests.
- ⚠️ Could confuse maintainers reading dataset restore test setup.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `tests/unit_tests/commands/dataset/restore_test.py` and inspect
lines 27-31, where
the comment states “Stable UUIDs make the test boundary realistic”.
2. Observe that `_DATASET_UUID` and `_MISSING_UUID` are defined on lines
30-31 as
`str(uuid.uuid4())`, which creates new random UUIDs on every test run.
3. Confirm there is no seeding, fixture, or override elsewhere in the file
that would make
these UUIDs deterministic or stable across runs.
4. Conclude that the comment is misleading because the UUIDs are not stable;
this is a
documentation mismatch, not a functional test failure.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=84a38bdbff1040a2b9a5f92f29658ae8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=84a38bdbff1040a2b9a5f92f29658ae8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/commands/dataset/restore_test.py
**Line:** 27:31
**Comment:**
*Comment Mismatch: The comment says these UUIDs are stable, but using
`uuid.uuid4()` makes them random on every run. Update the comment (or switch to
fixed UUID literals) so the test documentation matches the actual behavior and
avoids misleading future maintainers.
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%2F40130&comment_hash=ed583c5e0ee84e7fa6cf04808aa1eecfb220aaf6ba46468404a18ea9c65e4d6b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40130&comment_hash=ed583c5e0ee84e7fa6cf04808aa1eecfb220aaf6ba46468404a18ea9c65e4d6b&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]
