mikebridge commented on code in PR #40130:
URL: https://github.com/apache/superset/pull/40130#discussion_r3510300413
##########
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:
Test-scaffolding duplication noted; the setup blocks intentionally stay
local to each test for readability (per the avoid-nesting-when-testing
convention this suite follows). Declining as style.
##########
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"
+ model = SqlaTable
+
+ def apply(self, query: Query, value: Any) -> Query:
+ query = super().apply(query, value)
+ normalized = str(value).lower().strip() if value is not None else ""
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/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"
+ model = SqlaTable
+
+ def apply(self, query: Query, value: Any) -> Query:
+ query = super().apply(query, value)
+ normalized = str(value).lower().strip() if value is not None else ""
+ if normalized not in {"include", "only"} or
security_manager.is_admin():
+ return query
+
+ # Non-admins may only see soft-deleted datasets they own. ``any()``
emits
+ # an EXISTS subquery so it composes with the base access filter without
+ # producing duplicate rows from a join.
+ owned = SqlaTable.owners.any(security_manager.user_model.id ==
get_user_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.
##########
superset/datasets/api.py:
##########
@@ -87,25 +94,40 @@
statsd_metrics,
)
from superset.views.error_handling import handle_api_exception
-from superset.views.filters import BaseFilterRelatedUsers, FilterRelatedOwners
+from superset.views.filters import (
+ BaseFilterRelatedUsers,
+ FilterRelatedOwners,
+ SoftDeleteApiMixin,
+)
logger = logging.getLogger(__name__)
-class DatasetRestApi(BaseSupersetModelRestApi):
+class DatasetRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi):
datamodel = SQLAInterface(SqlaTable)
base_filters = [["id", DatasourceFilter, lambda: []]]
resource_name = "dataset"
allow_browser_login = True
class_permission_name = "Dataset"
- method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP
+ # Custom methods (``restore``) need an explicit entry; FAB's @protect()
+ # decorator falls back to ``can_<method>_<class>`` (i.e.
+ # ``can_restore_Dataset``) when the mapping is missing, which standard
+ # roles don't carry. Mirrors the permission model documented for
+ # ``DELETE`` / ``bulk_delete``: endpoint-level ``can_write`` plus
+ # resource-level ``raise_for_ownership``. See themes/api.py for the
+ # established pattern.
+ method_permission_name = {
+ **MODEL_API_RW_METHOD_PERMISSION_MAP,
+ "restore": "write",
+ }
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/migrations/versions/2026-05-08_12-10_3a8e6f2c1b95_add_deleted_at_to_tables.py:
##########
@@ -0,0 +1,54 @@
+# 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.
+"""Add deleted_at column and index to tables for soft-delete.
+
+Adds a nullable ``deleted_at`` column and an index on it to the
+``tables`` table to support soft deletion of datasets. Companion to
+the ``SoftDeleteMixin`` infrastructure shipped in PR #39977.
+
+Revision ID: 3a8e6f2c1b95
+Revises: 78a40c08b4be
+Create Date: 2026-05-08 12:10:00.000000
+"""
+
+from sqlalchemy import Column, DateTime
+
+from superset.migrations.shared.utils import (
+ add_columns,
+ create_index,
+ drop_columns,
+ drop_index,
+)
+
+# revision identifiers, used by Alembic.
+revision = "3a8e6f2c1b95"
+down_revision = "78a40c08b4be"
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/migrations/versions/2026-05-08_12-10_3a8e6f2c1b95_add_deleted_at_to_tables.py:
##########
@@ -0,0 +1,54 @@
+# 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.
+"""Add deleted_at column and index to tables for soft-delete.
+
+Adds a nullable ``deleted_at`` column and an index on it to the
+``tables`` table to support soft deletion of datasets. Companion to
+the ``SoftDeleteMixin`` infrastructure shipped in PR #39977.
+
+Revision ID: 3a8e6f2c1b95
+Revises: 78a40c08b4be
+Create Date: 2026-05-08 12:10:00.000000
+"""
+
+from sqlalchemy import Column, DateTime
+
+from superset.migrations.shared.utils import (
+ add_columns,
+ create_index,
+ drop_columns,
+ drop_index,
+)
+
+# revision identifiers, used by Alembic.
+revision = "3a8e6f2c1b95"
+down_revision = "78a40c08b4be"
+
+TABLE_NAME = "tables"
+INDEX_NAME = f"ix_{TABLE_NAME}_deleted_at"
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/security/manager.py:
##########
@@ -2344,11 +2347,16 @@ def _update_vm_datasources_access( # pylint:
disable=too-many-locals
sqlatable_table = SqlaTable.__table__ # pylint: disable=no-member
chart_table = Slice.__table__ # pylint: disable=no-member
new_database_name = target.database_name
- datasets = (
- self.session.query(SqlaTable)
- .filter(SqlaTable.database_id == target.id)
- .all()
- )
+ # Bypass the soft-delete visibility filter: a soft-deleted dataset's
perm
+ # strings (and its charts') must still be rewritten on a database
rename,
+ # otherwise restoring that dataset later brings back stale
dataset/schema/
+ # catalog permission strings referencing the old database name.
+ with skip_visibility_filter(self.session, SqlaTable):
+ datasets = (
+ self.session.query(SqlaTable)
+ .filter(SqlaTable.database_id == target.id)
+ .all()
+ )
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/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:
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]
