mikebridge commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3501461373
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -140,21 +141,93 @@ 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()
+ from superset.commands.importers.v1.utils import find_existing_for_import
Review Comment:
Addressed — the import was moved to the top-level import block
(`importers/v1/utils.py:40`); it's no longer inline in `import_dataset()`.
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
superset/commands/dataset/restore.py:
##########
@@ -0,0 +1,43 @@
+# 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.
+"""Command to restore a soft-deleted dataset."""
+
+import logging
+
+from superset.commands.dataset.exceptions import (
+ DatasetForbiddenError,
+ DatasetNotFoundError,
+ DatasetRestoreFailedError,
+)
+from superset.commands.restore import BaseRestoreCommand
+from superset.connectors.sqla.models import SqlaTable
+from superset.daos.dataset import DatasetDAO
+
+logger = logging.getLogger(__name__)
+
+
+class RestoreDatasetCommand(BaseRestoreCommand[SqlaTable]):
+ """Restore a soft-deleted dataset by clearing its ``deleted_at`` field.
+
+ All transactional wrapping and validation lives in
+ ``BaseRestoreCommand``; this subclass is purely declarative.
+ """
+
+ dao = DatasetDAO
+ not_found_exc = DatasetNotFoundError
+ forbidden_exc = DatasetForbiddenError
+ restore_failed_exc = DatasetRestoreFailedError
Review Comment:
Addressed — `RestoreDatasetCommand` now overrides `validate()` and refuses
the restore when `DatasetDAO.has_active_logical_duplicate(model)` finds a live
row with the same logical identity `(database_id, catalog, schema,
table_name)`, before `deleted_at` is cleared.
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
superset/commands/dataset/exceptions.py:
##########
@@ -170,6 +170,10 @@ class DatasetUpdateFailedError(UpdateFailedError):
message = _("Dataset could not be updated.")
+class DatasetRestoreFailedError(CommandException):
+ message = _("Dataset could not be restored.")
Review Comment:
Addressed — `DatasetRestoreFailedError` now inherits `UpdateFailedError`
(the nearest domain-specific middle-tier base) rather than `CommandException`
directly.
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -1014,6 +1015,331 @@ def test_import_dataset_access_check(
import_dataset(config)
+def test_import_soft_deleted_dataset_overwrite_restores_in_place(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ Overwrite-importing a soft-deleted dataset must restore the row in
+ place rather than hard-delete-and-replace. A hard delete would
+ cascade through the chart back-reference and table_columns /
+ sql_metrics rows; in-place restore preserves them.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = copy.deepcopy(dataset_fixture)
+ config["database_id"] = database.id
+
+ initial = import_dataset(config)
+ original_id = initial.id
+
+ existing = db.session.query(SqlaTable).filter_by(uuid=config["uuid"]).one()
+ existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
+ db.session.flush()
+
+ admin = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="[email protected]",
+ username="admin",
+ roles=[Role(name="Admin")],
+ )
+
+ with override_user(admin):
+ restored = import_dataset(config, overwrite=True)
+
+ assert restored.id == original_id
+ assert restored.deleted_at is None
+
+
+def test_import_soft_deleted_dataset_non_overwrite_restores_for_owner(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ Non-overwrite re-import of a soft-deleted UUID is implicitly a
+ restore-and-update: the user is bringing the dataset back by
+ uploading it again. The same ownership rule as the overwrite path
+ applies, so an owner (or admin) succeeds without setting
+ overwrite=True.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = copy.deepcopy(dataset_fixture)
+ config["database_id"] = database.id
+
+ initial = import_dataset(config)
+ original_id = initial.id
+
+ existing = db.session.query(SqlaTable).filter_by(uuid=config["uuid"]).one()
+ existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
+ db.session.flush()
+
+ admin = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="[email protected]",
+ username="admin",
+ roles=[Role(name="Admin")],
+ )
+
+ with override_user(admin):
+ restored = import_dataset(config, overwrite=False)
+
+ assert restored.id == original_id
+ assert restored.deleted_at is None
+
+
+def test_import_soft_deleted_dataset_non_overwrite_raises_for_non_owner(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ Non-overwrite re-import that would resurrect a soft-deleted dataset
+ must respect ownership: a non-owner without admin role cannot
+ restore-via-import. Mirrors the explicit /restore endpoint's check.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = copy.deepcopy(dataset_fixture)
+ config["database_id"] = database.id
+
+ import_dataset(config)
+ existing = db.session.query(SqlaTable).filter_by(uuid=config["uuid"]).one()
+ existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
+ db.session.flush()
+
+ non_owner = User(
+ first_name="Bob",
+ last_name="Roe",
+ email="[email protected]",
+ username="bob",
+ roles=[Role(name="Gamma")],
+ )
+
+ with override_user(non_owner):
+ with pytest.raises(ImportFailedError) as excinfo:
+ import_dataset(config, overwrite=False)
+ assert "permissions to restore" in str(excinfo.value)
+
Review Comment:
Addressed — added `assert existing.deleted_at is not None` after the
`pytest.raises(ImportFailedError)` block, so a regression that clears
`deleted_at` before raising would fail the test.
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -1014,6 +1015,331 @@ def test_import_dataset_access_check(
import_dataset(config)
+def test_import_soft_deleted_dataset_overwrite_restores_in_place(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ Overwrite-importing a soft-deleted dataset must restore the row in
+ place rather than hard-delete-and-replace. A hard delete would
+ cascade through the chart back-reference and table_columns /
+ sql_metrics rows; in-place restore preserves them.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = copy.deepcopy(dataset_fixture)
+ config["database_id"] = database.id
+
+ initial = import_dataset(config)
+ original_id = initial.id
+
+ existing = db.session.query(SqlaTable).filter_by(uuid=config["uuid"]).one()
+ existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
+ db.session.flush()
+
+ admin = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="[email protected]",
+ username="admin",
+ roles=[Role(name="Admin")],
+ )
+
+ with override_user(admin):
+ restored = import_dataset(config, overwrite=True)
+
+ assert restored.id == original_id
+ assert restored.deleted_at is None
+
+
+def test_import_soft_deleted_dataset_non_overwrite_restores_for_owner(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ Non-overwrite re-import of a soft-deleted UUID is implicitly a
+ restore-and-update: the user is bringing the dataset back by
+ uploading it again. The same ownership rule as the overwrite path
+ applies, so an owner (or admin) succeeds without setting
+ overwrite=True.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = copy.deepcopy(dataset_fixture)
+ config["database_id"] = database.id
+
+ initial = import_dataset(config)
+ original_id = initial.id
+
+ existing = db.session.query(SqlaTable).filter_by(uuid=config["uuid"]).one()
+ existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
+ db.session.flush()
+
+ admin = User(
+ first_name="Alice",
+ last_name="Doe",
+ email="[email protected]",
+ username="admin",
+ roles=[Role(name="Admin")],
+ )
+
+ with override_user(admin):
+ restored = import_dataset(config, overwrite=False)
+
+ assert restored.id == original_id
+ assert restored.deleted_at is None
+
+
+def test_import_soft_deleted_dataset_non_overwrite_raises_for_non_owner(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ Non-overwrite re-import that would resurrect a soft-deleted dataset
+ must respect ownership: a non-owner without admin role cannot
+ restore-via-import. Mirrors the explicit /restore endpoint's check.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = copy.deepcopy(dataset_fixture)
+ config["database_id"] = database.id
+
+ import_dataset(config)
+ existing = db.session.query(SqlaTable).filter_by(uuid=config["uuid"]).one()
+ existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0)
+ db.session.flush()
+
+ non_owner = User(
+ first_name="Bob",
+ last_name="Roe",
+ email="[email protected]",
+ username="bob",
+ roles=[Role(name="Gamma")],
+ )
+
+ with override_user(non_owner):
+ with pytest.raises(ImportFailedError) as excinfo:
+ import_dataset(config, overwrite=False)
+ assert "permissions to restore" in str(excinfo.value)
+
+
+def test_import_soft_deleted_dataset_raises_when_caller_lacks_can_write(
+ mocker: MockerFixture,
+ session: Session,
+) -> None:
+ """
+ Case B: re-import of a soft-deleted UUID by a caller without
+ can_write must raise, not silently return the soft-deleted row.
+
+ Real-world scenario: a user has can_write Dashboard but not
+ can_write Dataset, and they import a dashboard zip that references
+ a soft-deleted dataset. Silently returning the row would let the
+ dashboard importer wire the dashboard's charts to a deleted dataset
+ and produce broken chart loads.
+ """
+ mocker.patch.object(security_manager, "can_access", return_value=False)
+
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ config = copy.deepcopy(dataset_fixture)
+ config["database_id"] = database.id
+
+ # Seed a soft-deleted dataset with the matching UUID directly, so the
+ # test doesn't need to flip permissions mid-test.
+ existing = SqlaTable(
+ table_name="soft_deleted_dataset",
+ database_id=database.id,
+ uuid=config["uuid"],
+ deleted_at=datetime(2026, 1, 1, 12, 0, 0),
+ )
+ db.session.add(existing)
+ db.session.flush()
+
+ with pytest.raises(ImportFailedError) as excinfo:
+ import_dataset(config, overwrite=False)
+ assert "can_write" in str(excinfo.value)
+
Review Comment:
Addressed — the same post-exception `assert existing.deleted_at is not None`
was added on the Case B path.
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
tests/unit_tests/commands/dataset/restore_test.py:
##########
@@ -0,0 +1,95 @@
+# 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
+
+from datetime import datetime
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+
+def test_restore_dataset_clears_deleted_at(app_context: None) -> None:
+ """RestoreDatasetCommand.run() restores a soft-deleted dataset."""
+ from superset.commands.dataset.restore import RestoreDatasetCommand
+
+ dataset = MagicMock()
+ dataset.deleted_at = datetime(2026, 1, 1)
+ dataset.id = 1
+
+ with (
+ patch(
+ "superset.daos.dataset.DatasetDAO.find_by_id", return_value=dataset
+ ) as mock_find,
+ patch("superset.commands.restore.security_manager") as mock_sec,
+ ):
+ mock_sec.raise_for_ownership.return_value = None
+
+ cmd = RestoreDatasetCommand("1")
+ cmd.run()
Review Comment:
Addressed — the restore test (now at
`tests/unit_tests/commands/dataset/restore_test.py`) uses real UUIDs:
`_DATASET_UUID = str(uuid.uuid4())` / `_MISSING_UUID`, not an integer-like
string.
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -152,8 +151,11 @@ def insert_database(self, name: str, allow_multi_catalog:
bool = False) -> Datab
return db_connection
def get_fixture_datasets(self) -> list[SqlaTable]:
+ from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES
Review Comment:
Addressed — `SKIP_VISIBILITY_FILTER_CLASSES` is now a module-level import
(`api_tests.py:37`).
_— posted by Claude (AI agent) on behalf of @mikebridge_
##########
superset/datasets/api.py:
##########
@@ -706,8 +717,9 @@ def refresh(self, pk: int) -> Response:
@safe
@statsd_metrics
@event_logger.log_this_with_context(
Review Comment:
These are normalizations of the existing action lambdas to a single-line
form, matching the new `restore` action lambda — kept for stylistic
consistency. Happy to revert them to keep the diff minimal if you'd prefer;
leaving this thread open for your call.
_— posted by Claude (AI agent) on behalf of @mikebridge_
--
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]
