mikebridge commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510301919
##########
superset/datasets/filters.py:
##########
@@ -51,3 +57,40 @@ def apply(self, query: Query, value: bool) -> Query:
)
)
return query
+
+
+class DatasetDeletedStateFilter( # pylint: disable=too-few-public-methods
+ BaseDeletedStateFilter
+):
+ """Rison filter for the GET list that exposes soft-deleted datasets.
+
+ Soft-deleted rows are additionally scoped to the **restore audience**: only
+ the dataset's owners (or admins) may enumerate them. This mirrors
+ ``RestoreDatasetCommand``'s ``raise_for_ownership`` check, so a read-access
+ non-owner (who can see the dataset via ``datasource_access``) cannot list
+ soft-deleted datasets they could never restore. Live rows are unaffected —
+ they keep their normal ``DatasourceFilter`` visibility. The ownership
+ scoping is part of the cross-entity deleted-state contract: only the
+ restore audience may enumerate soft-deleted rows (kept consistent with the
+ deleted-state filters in the dashboard and chart soft-delete rollouts).
+ """
+
+ arg_name = "dataset_deleted_state"
Review Comment:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. Declining as
style.
##########
superset/datasets/filters.py:
##########
@@ -51,3 +57,40 @@ def apply(self, query: Query, value: bool) -> Query:
)
)
return query
+
+
+class DatasetDeletedStateFilter( # pylint: disable=too-few-public-methods
+ BaseDeletedStateFilter
+):
+ """Rison filter for the GET list that exposes soft-deleted datasets.
+
+ Soft-deleted rows are additionally scoped to the **restore audience**: only
+ the dataset's owners (or admins) may enumerate them. This mirrors
+ ``RestoreDatasetCommand``'s ``raise_for_ownership`` check, so a read-access
+ non-owner (who can see the dataset via ``datasource_access``) cannot list
+ soft-deleted datasets they could never restore. Live rows are unaffected —
+ they keep their normal ``DatasourceFilter`` visibility. The ownership
+ scoping is part of the cross-entity deleted-state contract: only the
+ restore audience may enumerate soft-deleted rows (kept consistent with the
+ deleted-state filters in the dashboard and chart soft-delete rollouts).
+ """
+
+ arg_name = "dataset_deleted_state"
+ model = SqlaTable
Review Comment:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. Declining as
style.
##########
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:
Good catch — fixed in `b463c484b4`. The purge is now restricted to rows with
`deleted_at` set (matching its comment's stated intent), so it can no longer
hard-delete live datasets other suites created over the same AB tables.
##########
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:
Fixed in `b463c484b4` — wrapped in `try/finally` to match the sibling tests,
so a failed assertion can no longer strand the soft-deleted dataset/database in
the shared test DB.
##########
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:
Considered — keeping as-is: these are internal helpers/test scaffolding
following the surrounding file's documentation and style conventions. Declining
as style-tier; happy to revisit if a maintainer feels differently.
##########
superset/daos/base.py:
##########
@@ -81,9 +81,15 @@ def apply(self, column: Any, value: Any) -> Any:
return op_func(column, value)
-def _escape_like(value: str) -> str:
- """Escape LIKE/ILIKE wildcards to prevent wildcard injection."""
- return value.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")
+def _escape_like(value: Any) -> str:
+ """Escape LIKE/ILIKE wildcards to prevent wildcard injection.
+
+ ``ColumnOperator.value`` is typed ``Any``, so a non-string payload (e.g. a
+ numeric JSON value) can reach a LIKE-family operator. Coerce to ``str`` so
+ such input degrades to a literal match instead of raising
``AttributeError``.
+ """
+ text = "" if value is None else str(value)
+ return text.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")
Review Comment:
Resolved by extraction: the `_escape_like` change was removed from this PR
(review feedback — unrelated to soft delete) and lives in #41653, which also
fixes the issue this thread describes properly — `None` now matches **no** rows
(SQL three-valued logic, `x LIKE NULL`) instead of coercing to `""` and
matching everything, with tests reproducing the original failure.
##########
superset/translations/ar/LC_MESSAGES/messages.po:
##########
@@ -4697,6 +4697,9 @@ msgstr "تعذر إنشاء مجموعة بيانات."
msgid "Dataset could not be duplicated."
msgstr "تعذر تكرار مجموعة البيانات."
+msgid "Dataset could not be restored."
+msgstr ""
Review Comment:
An empty `msgstr` is expected for newly-extracted strings — catalogs carry
new message ids untranslated until translators fill them in; untranslated
strings fall back to the source text. Declining.
##########
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()
+
datasets = []
admin = self.get_user("admin")
main_db = get_main_database()
for tables_name in self.fixture_tables_names:
datasets.append(self.insert_dataset(tables_name, [admin.id],
main_db))
+ # Capture IDs eagerly — dataset objects may be detached after yield
+ dataset_ids = [ds.id for ds in datasets]
+
yield datasets
- # rollback changes
- for dataset in datasets:
- state = inspect(dataset)
- if not state.was_deleted:
- db.session.delete(dataset)
+ # rollback changes (including soft-deleted rows)
+ for dataset_id in dataset_ids:
Review Comment:
Following the surrounding file's conventions — these are
local/test-scaffolding variables whose types are unambiguous from context;
annotating each would add noise without changing what mypy checks. Declining as
style.
--
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]
