codeant-ai-for-open-source[bot] commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510302469
##########
superset/daos/dataset.py:
##########
@@ -186,16 +236,111 @@ def validate_update_uniqueness(
) -> bool:
# The catalog might not be set even if the database supports catalogs,
in case
# multi-catalog is disabled.
- catalog = table.catalog or database.get_default_catalog()
-
- dataset_query = db.session.query(SqlaTable).filter(
- SqlaTable.table_name == table.table,
- SqlaTable.database_id == database.id,
- SqlaTable.schema == table.schema,
- SqlaTable.catalog == catalog,
- SqlaTable.id != dataset_id,
+ default_catalog = database.get_default_catalog()
+ catalog = table.catalog or default_catalog
+
+ # Same rationale as ``validate_uniqueness`` above: a soft-deleted
+ # twin must block the update rather than being silently ignored.
+ # The bypass is session-scoped, not per-query, because the EXISTS
+ # subquery pattern below leaves the inner Query's execution_options
+ # invisible to the outer execute (where the listener fires).
+ #
+ # avoid app-init regression: see ``validate_uniqueness`` above — a
+ # module-top import from ``superset.models.helpers`` raises
+ # "App not initialized yet" outside an app context (see PR #40573).
+ from superset.models.helpers import ( # pylint:
disable=import-outside-toplevel
+ skip_visibility_filter,
)
- return not db.session.query(dataset_query.exists()).scalar()
+
+ with skip_visibility_filter(db.session, SqlaTable):
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == table.table,
+ SqlaTable.database_id == database.id,
+ SqlaTable.schema == table.schema,
+ DatasetDAO._catalog_identity_filter(catalog, default_catalog),
+ SqlaTable.id != dataset_id,
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def has_active_logical_duplicate(model: SqlaTable) -> bool:
+ """Return True iff another *active* dataset shares model's physical
table.
+
+ Physical identity is ``(database_id, catalog, schema, table_name)``.
+ ``model``'s catalog is normalized to the database default when unset —
+ the same rule ``validate_uniqueness``/``validate_update_uniqueness``
+ apply — so create, update, restore, and re-import agree on what counts
+ as the same physical table. Catalog matching is null-aware via
+ ``_catalog_identity_filter``: when the normalized catalog is the
database
+ default, both ``catalog = default`` and ``catalog IS NULL`` rows match,
+ so a default-catalog twin is caught whichever way either row stored its
+ catalog.
+
+ Unlike ``validate_uniqueness``, this does NOT use
+ ``skip_visibility_filter``: it relies on the ``SoftDeleteMixin``
listener
+ to auto-append ``deleted_at IS NULL`` so only *active* rows match. Do
not
+ add the bypass here — it would broaden the check to soft-deleted rows
and
+ silently refuse legitimate restores. ``id != model.id`` excludes the
row
+ itself.
+
+ Assumes an active app context and a session-attached ``model`` so
+ ``db.session`` and the lazy ``model.database`` relationship resolve.
+ """
+ # The catalog might not be set even if the database supports catalogs,
+ # in case multi-catalog is disabled.
+ default_catalog = model.database.get_default_catalog()
+ catalog = model.catalog or default_catalog
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag test-scaffolding duplication or local setup blocks in test
files when they are intentionally kept local for readability and follow the
suite's testing conventions.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
superset/daos/dataset.py:
##########
@@ -186,16 +236,111 @@ def validate_update_uniqueness(
) -> bool:
# The catalog might not be set even if the database supports catalogs,
in case
# multi-catalog is disabled.
- catalog = table.catalog or database.get_default_catalog()
-
- dataset_query = db.session.query(SqlaTable).filter(
- SqlaTable.table_name == table.table,
- SqlaTable.database_id == database.id,
- SqlaTable.schema == table.schema,
- SqlaTable.catalog == catalog,
- SqlaTable.id != dataset_id,
+ default_catalog = database.get_default_catalog()
+ catalog = table.catalog or default_catalog
+
+ # Same rationale as ``validate_uniqueness`` above: a soft-deleted
+ # twin must block the update rather than being silently ignored.
+ # The bypass is session-scoped, not per-query, because the EXISTS
+ # subquery pattern below leaves the inner Query's execution_options
+ # invisible to the outer execute (where the listener fires).
+ #
+ # avoid app-init regression: see ``validate_uniqueness`` above — a
+ # module-top import from ``superset.models.helpers`` raises
+ # "App not initialized yet" outside an app context (see PR #40573).
+ from superset.models.helpers import ( # pylint:
disable=import-outside-toplevel
+ skip_visibility_filter,
)
- return not db.session.query(dataset_query.exists()).scalar()
+
+ with skip_visibility_filter(db.session, SqlaTable):
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == table.table,
+ SqlaTable.database_id == database.id,
+ SqlaTable.schema == table.schema,
+ DatasetDAO._catalog_identity_filter(catalog, default_catalog),
+ SqlaTable.id != dataset_id,
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def has_active_logical_duplicate(model: SqlaTable) -> bool:
+ """Return True iff another *active* dataset shares model's physical
table.
+
+ Physical identity is ``(database_id, catalog, schema, table_name)``.
+ ``model``'s catalog is normalized to the database default when unset —
+ the same rule ``validate_uniqueness``/``validate_update_uniqueness``
+ apply — so create, update, restore, and re-import agree on what counts
+ as the same physical table. Catalog matching is null-aware via
+ ``_catalog_identity_filter``: when the normalized catalog is the
database
+ default, both ``catalog = default`` and ``catalog IS NULL`` rows match,
+ so a default-catalog twin is caught whichever way either row stored its
+ catalog.
+
+ Unlike ``validate_uniqueness``, this does NOT use
+ ``skip_visibility_filter``: it relies on the ``SoftDeleteMixin``
listener
+ to auto-append ``deleted_at IS NULL`` so only *active* rows match. Do
not
+ add the bypass here — it would broaden the check to soft-deleted rows
and
+ silently refuse legitimate restores. ``id != model.id`` excludes the
row
+ itself.
+
+ Assumes an active app context and a session-attached ``model`` so
+ ``db.session`` and the lazy ``model.database`` relationship resolve.
+ """
+ # The catalog might not be set even if the database supports catalogs,
+ # in case multi-catalog is disabled.
+ default_catalog = model.database.get_default_catalog()
+ catalog = model.catalog or default_catalog
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag test-scaffolding duplication or local setup blocks in test
files when they are intentionally kept local for readability and follow the
suite's testing conventions.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
tests/integration_tests/dashboard_utils.py:
##########
@@ -35,10 +37,29 @@ def get_table(
schema: Optional[str] = None,
):
schema = schema or get_example_default_schema()
+ # Bypass the soft-delete listener so the helper finds rows previously
+ # soft-deleted by other tests in the same session. Without the
+ # bypass, the listener hides them and a subsequent INSERT collides
+ # with the underlying ``(database_id, catalog, schema, table_name)``
+ # unique constraint that survives soft-delete.
+ #
+ # Match rows whose catalog is either unset (NULL) or the database
+ # default — these are "the same physical table" the way
+ # ``DatasetDAO.validate_uniqueness`` treats them (``table.catalog or
+ # default``). Example rows are seeded with ``catalog = NULL`` while a
+ # connection that reports a default catalog (e.g. Postgres) would
+ # normalize to that default, so both must qualify. This still excludes
+ # an unrelated third catalog variant of the same table_name. Within the
+ # matched set, prefer live rows over soft-deleted ones via
+ # ``ORDER BY deleted_at IS NULL DESC``.
+ catalog = database.get_default_catalog()
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> In test and test-scaffolding code, do not require explicit type
annotations for local variables when the type is already unambiguous from
context and adding hints would be 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/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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not require type annotations for local test-scaffolding variables in
test files when their types are already obvious from context; keep this style
guidance scoped to tests.
**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/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:
✅ **Customized review instruction saved!**
**Instruction:**
> Do not flag missing concrete type annotations for local test-scaffolding
variables when their types are already unambiguous from context; in test files,
keep this style noise low unless the annotation affects correctness.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *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),
+ )
+ 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:
✅ **Customized review instruction saved!**
**Instruction:**
> Avoid flagging missing explicit type annotations for local
test-scaffolding variables in test files when the type is already unambiguous
from context and the annotation would add noise.
**Applied to:**
- `**/test/**`
- `**/tests/**`
- `**/*test*.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI
Settings](https://app.codeant.ai/org/settings/learnings)*
##########
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:
✅ **Customized review instruction saved!**
**Instruction:**
> Avoid flagging missing type annotations for local/test-scaffolding
variables whose types are obvious from context and do not change mypy behavior;
prefer keeping these cases unannotated to reduce noise.
**Applied to:**
- `**/*.py`
---
💡 *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]
