mikebridge commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510301182
##########
superset/translations/it/LC_MESSAGES/messages.po:
##########
@@ -4602,6 +4602,9 @@ msgstr "La tua query non può essere salvata"
msgid "Dataset could not be duplicated."
msgstr "La tua query non può essere salvata"
+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.
##########
superset/translations/fa/LC_MESSAGES/messages.po:
##########
@@ -4634,6 +4634,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()
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()
+
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]
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:
##########
@@ -1957,17 +1974,32 @@ def test_delete_dataset_item(self):
"""
dataset = self.insert_default_dataset()
+ dataset_id = dataset.id
view_menu = security_manager.find_view_menu(dataset.get_perm())
assert view_menu is not None
view_menu_id = view_menu.id
self.login(ADMIN_USERNAME)
uri = f"api/v1/dataset/{dataset.id}"
rv = self.client.delete(uri)
assert rv.status_code == 200
+ # With soft delete, the row still exists (with deleted_at set) so
+ # FAB permissions are preserved for potential restore.
non_view_menu = db.session.query(security_manager.viewmenu_model).get(
view_menu_id
)
- assert non_view_menu is None
+ assert non_view_menu is not None
+
+ # Hard-delete the soft-deleted row to avoid unique constraint
+ # collisions in subsequent tests
+ row = (
+ db.session.query(SqlaTable)
+ .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {SqlaTable}})
+ .filter(SqlaTable.id == dataset_id)
+ .one_or_none()
+ )
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())
+
+
+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, 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.
##########
tests/integration_tests/datasets/api_tests.py:
##########
@@ -1957,17 +1974,32 @@ def test_delete_dataset_item(self):
"""
dataset = self.insert_default_dataset()
+ dataset_id = dataset.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/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),
+ )
+ db.session.add_all([database, base, hidden_twin])
+ db.session.flush()
+
+ with (
+ # find_by_id applies the security base filter, which needs a request
+ # user; return the seeded row directly. validate_uniqueness — the
+ # behaviour under test — runs for real against the session.
+ patch(
+ "superset.commands.dataset.duplicate.DatasetDAO.find_by_id",
+ return_value=base,
+ ),
+
patch("superset.commands.dataset.duplicate.security_manager.raise_for_access"),
+ patch(
+ "superset.commands.dataset.duplicate."
+ "DuplicateDatasetCommand.populate_owners",
+ return_value=[],
+ ),
+ ):
+ command = DuplicateDatasetCommand(
+ {
+ "base_model_id": base.id,
+ "table_name": "dup_name",
+ "is_managed_externally": 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.
##########
superset/commands/database/exceptions.py:
##########
@@ -156,6 +156,18 @@ class
DatabaseDeleteDatasetsExistFailedError(DeleteFailedError):
message = _("Cannot delete a database that has datasets attached")
+class DatabaseDeleteSoftDeletedDatasetsExistFailedError(
+ DatabaseDeleteDatasetsExistFailedError
+):
+ # Subclasses the live-datasets error so the existing API handler catches
+ # both; only the message differs, telling the operator that the blockers
+ # are hidden (soft-deleted) rows even though their dataset list looks
empty.
+ message = _(
+ "Cannot delete a database whose only remaining datasets are "
+ "soft-deleted. Restore or permanently purge them first."
+ )
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]
