codeant-ai-for-open-source[bot] commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510300847
##########
superset/commands/dataset/exceptions.py:
##########
@@ -170,6 +170,26 @@ class DatasetUpdateFailedError(UpdateFailedError):
message = _("Dataset could not be updated.")
+class DatasetRestoreFailedError(UpdateFailedError):
+ # Restore semantically clears ``deleted_at``; it is an UPDATE, not a new
+ # row. ``UpdateFailedError`` is the nearest typed middle-tier base in the
+ # codebase. A dedicated ``RestoreFailedError`` in
+ # ``superset/commands/exceptions.py`` would be more precise across the
+ # entity rollouts but lives in already-merged infrastructure (#39977);
+ # introducing it can be a cross-entity follow-up.
+ message = _("Dataset could not be restored.")
+
+
+class DatasetLogicalDuplicateError(CommandException):
+ status = 422
+ message = _(
+ "Dataset cannot be restored because another active dataset already "
+ "references the same physical table (same database, catalog, schema, "
+ "and table name). Delete the duplicate or rename the table before "
+ "restoring."
+ )
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag test-scaffolding duplication when setup blocks are
intentionally kept local to each test for readability.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require docstrings for internal helper functions in test
scaffolding; follow the surrounding test-file style conventions instead.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require concrete type annotations for local test-scaffolding scalar
variables in test files when their types are unambiguous from context; avoid
flagging this as a style issue.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag missing type annotations for local test-scaffolding constants
in test files when the types are unambiguous from context and annotations would
add noise.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag missing type annotations for local test-scaffolding variables
in test files when their types are unambiguous from context and annotations
would add noise without improving checks.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
--
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]
