mikebridge commented on code in PR #40130: URL: https://github.com/apache/superset/pull/40130#discussion_r3510299174
########## superset/translations/fi/LC_MESSAGES/messages.po: ########## @@ -8222,6 +8222,9 @@ msgstr "Tietojoukkoa ei voitu monistaa." # de, es, fa, fr, it, ja, lv, mi, nl, pl, pt, pt_BR, ru, sk, sl, tr, uk, zh, # zh_TW] #, fuzzy +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/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) 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. ########## tests/integration_tests/security_tests.py: ########## @@ -560,6 +560,72 @@ def test_after_update_database__perm_datasource_access(self): db.session.delete(tmp_db1) db.session.commit() + def test_after_update_database__perm_datasource_access_soft_deleted(self): + """A soft-deleted dataset's perm strings must still be rewritten on a + database rename. The maintenance query bypasses the soft-delete + visibility filter so hidden datasets are updated too; otherwise + restoring the dataset later would resurrect stale dataset/schema/catalog + permission strings pointing at the old database name. + """ + from datetime import datetime # noqa: PLC0415 + + from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES # noqa: PLC0415 + + security_manager.on_view_menu_after_update = Mock() + + tmp_db1 = Database(database_name="tmp_db1", sqlalchemy_uri="sqlite://") + db.session.add(tmp_db1) + db.session.commit() + + table1 = SqlaTable( + schema="tmp_schema", + table_name="tmp_table1", + database=tmp_db1, + ) + db.session.add(table1) + db.session.commit() + table1 = db.session.query(SqlaTable).filter_by(table_name="tmp_table1").one() + table1_id = table1.id 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/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: 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/unit_tests/commands/dataset/test_duplicate.py: ########## @@ -101,3 +102,66 @@ def test_duplicate_dataset_access_check_passes_through() -> None: command.validate() # should not raise # Confirm access check was called with the base dataset mock_access.assert_called_once_with(datasource=mock_dataset) + + +def test_duplicate_dataset_blocked_by_soft_deleted_twin(session: Session) -> None: + """validate() rejects a duplicate whose name matches a soft-deleted + dataset at the same (database, schema). + + The previous name-only ``find_one_or_none`` lookup ran through the + soft-delete visibility filter, so a soft-deleted twin was invisible and + the duplicate proceeded — hitting whichever DB constraint applies as an + opaque IntegrityError, or creating an active twin that permanently + blocks restore where none does. The shared ``validate_uniqueness`` + check bypasses the filter and refuses with a clean validation error. + """ + from datetime import datetime, timezone + + from superset import db + from superset.commands.dataset.duplicate import DuplicateDatasetCommand + from superset.commands.dataset.exceptions import DatasetInvalidError + from superset.connectors.sqla.models import SqlaTable + from superset.models.core import Database + + SqlaTable.metadata.create_all(session.get_bind()) + + database = Database(database_name="dup_db", sqlalchemy_uri="sqlite://") + base = SqlaTable( + table_name="base_view", + schema="main", + database=database, + sql="select 1", + ) + hidden_twin = SqlaTable( + table_name="dup_name", + schema="main", + database=database, + deleted_at=datetime(2026, 1, 1, tzinfo=timezone.utc), + ) 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/commands/database/delete.py: ########## @@ -61,6 +61,28 @@ def validate(self) -> None: report_names=",".join(report_names), ) ) - # Check if there are datasets for this database - if self._model.tables: + # Check if there are datasets for this database. ``self._model.tables`` + # would now hide soft-deleted datasets (``SqlaTable`` inherits + # ``SoftDeleteMixin``, so the relationship lazy-load applies the + # visibility filter), letting a database whose datasets are all + # soft-deleted look empty and be hard-deleted while ``tables.database_id`` + # rows still reference it. Count with the visibility filter bypassed so + # soft-deleted datasets still block the delete. + from superset.connectors.sqla.models import ( # pylint: disable=import-outside-toplevel + SqlaTable, + ) + from superset.extensions import ( # pylint: disable=import-outside-toplevel + db, + ) + from superset.models.helpers import ( # pylint: disable=import-outside-toplevel + skip_visibility_filter, + ) + + with skip_visibility_filter(db.session, SqlaTable): + has_datasets = db.session.query( + db.session.query(SqlaTable.id) + .filter(SqlaTable.database_id == self._model_id) + .exists() + ).scalar() 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/commands/dataset/importers/v1/utils.py: ########## @@ -200,27 +203,159 @@ def import_dataset( # noqa: C901 force_data: bool = False, ignore_permissions: bool = False, ) -> SqlaTable: + """Import a dataset 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 dataset 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 charts/dashboards to a deleted dataset). + """ can_write = ignore_permissions or security_manager.can_access( "can_write", "Dataset", ) - existing = db.session.query(SqlaTable).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 user not in existing.owners and not security_manager.is_admin(): + # Tracks whether we entered the soft-deleted mutation path so the + # downstream `sync` decision (below) can reflect that an + # implicit-restore re-import is a clean replacement, not a merge. + is_soft_deleted_match = False 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]
