diff --git a/superset/charts/api.py b/superset/charts/api.py index 617129d49c6d..615f4e570942 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -35,6 +35,7 @@ ChartAllTextFilter, ChartCertifiedFilter, ChartCreatedByMeFilter, + ChartDeletedStateFilter, ChartFavoriteFilter, ChartFilter, ChartHasCreatedByFilter, @@ -63,12 +64,14 @@ ChartForbiddenError, ChartInvalidError, ChartNotFoundError, + ChartRestoreFailedError, ChartUpdateFailedError, DashboardsForbiddenError, ) from superset.commands.chart.export import ExportChartsCommand from superset.commands.chart.fave import AddFavoriteChartCommand from superset.commands.chart.importers.dispatcher import ImportChartsCommand +from superset.commands.chart.restore import RestoreChartCommand from superset.commands.chart.unfave import DelFavoriteChartCommand from superset.commands.chart.update import UpdateChartCommand from superset.commands.chart.warm_up_cache import ChartWarmUpCacheCommand @@ -100,12 +103,16 @@ requires_json, statsd_metrics, ) -from superset.views.filters import BaseFilterRelatedUsers, FilterRelatedOwners +from superset.views.filters import ( + BaseFilterRelatedUsers, + FilterRelatedOwners, + SoftDeleteApiMixin, +) logger = logging.getLogger(__name__) -class ChartRestApi(BaseSupersetModelRestApi): +class ChartRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi): datamodel = SQLAInterface(Slice) resource_name = "chart" @@ -122,6 +129,7 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]: RouteMethod.IMPORT, RouteMethod.RELATED, "bulk_delete", # not using RouteMethod since locally defined + "restore", "viz_types", "favorite_status", "add_favorite", @@ -220,6 +228,7 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]: "id": [ ChartFavoriteFilter, ChartCertifiedFilter, + ChartDeletedStateFilter, ChartOwnedCreatedFavoredByMeFilter, ], "slice_name": [ChartAllTextFilter], @@ -568,6 +577,62 @@ def bulk_delete(self, **kwargs: Any) -> Response: except ChartDeleteFailedError as ex: return self.response_422(message=str(ex)) + @expose("//restore", methods=("POST",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.restore", + log_to_statsd=False, + ) + def restore(self, uuid: str) -> Response: + """Restore a soft-deleted chart. + --- + post: + summary: Restore a soft-deleted chart + parameters: + - in: path + schema: + type: string + format: uuid + name: uuid + responses: + 200: + description: Chart restored + content: + application/json: + schema: + type: object + properties: + message: + type: string + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + try: + RestoreChartCommand(uuid).run() + return self.response(200, message="OK") + except ChartNotFoundError: + return self.response_404() + except ChartForbiddenError: + return self.response_403() + except ChartRestoreFailedError as ex: + logger.error( + "Error restoring model %s: %s", + self.__class__.__name__, + str(ex), + exc_info=True, + ) + return self.response_422(message=str(ex)) + @expose("//cache_screenshot/", methods=("GET",)) @protect() @parse_rison(screenshot_query_schema) diff --git a/superset/charts/filters.py b/superset/charts/filters.py index f9748dd0ecb6..3fc5af210c08 100644 --- a/superset/charts/filters.py +++ b/superset/charts/filters.py @@ -31,6 +31,7 @@ from superset.utils.filters import get_dataset_access_filters from superset.views.base import BaseFilter from superset.views.base_api import BaseFavoriteFilter +from superset.views.filters import BaseDeletedStateFilter class ChartAllTextFilter(BaseFilter): # pylint: disable=too-few-public-methods @@ -180,3 +181,10 @@ def apply(self, query: Query, value: Any) -> Query: FavStar.user_id == get_user_id(), ) ) + + +class ChartDeletedStateFilter(BaseDeletedStateFilter): # pylint: disable=too-few-public-methods + """Rison filter for the GET list that exposes soft-deleted charts.""" + + arg_name = "chart_deleted_state" + model = Slice diff --git a/superset/commands/chart/delete.py b/superset/commands/chart/delete.py index 00e6d201bcc9..b6a302c59cad 100644 --- a/superset/commands/chart/delete.py +++ b/superset/commands/chart/delete.py @@ -46,6 +46,7 @@ def __init__(self, model_ids: list[int]): def run(self) -> None: self.validate() assert self._models + ChartDAO.delete(self._models) def validate(self) -> None: diff --git a/superset/commands/chart/exceptions.py b/superset/commands/chart/exceptions.py index 72ef71f466e8..853bfb514799 100644 --- a/superset/commands/chart/exceptions.py +++ b/superset/commands/chart/exceptions.py @@ -123,6 +123,10 @@ class ChartDeleteFailedError(DeleteFailedError): message = _("Charts could not be deleted.") +class ChartRestoreFailedError(CommandException): + message = _("Chart could not be restored.") + + class ChartDeleteFailedReportsExistError(ChartDeleteFailedError): message = _("There are associated alerts or reports") diff --git a/superset/commands/chart/importers/v1/utils.py b/superset/commands/chart/importers/v1/utils.py index 33e95279a229..fcfef681e714 100644 --- a/superset/commands/chart/importers/v1/utils.py +++ b/superset/commands/chart/importers/v1/utils.py @@ -49,9 +49,14 @@ def import_chart( ignore_permissions: bool = False, ) -> Slice: can_write = ignore_permissions or security_manager.can_access("can_write", "Chart") - existing = db.session.query(Slice).filter_by(uuid=config["uuid"]).first() + from superset.commands.importers.v1.utils import ( + clear_soft_deleted_for_import, + find_existing_for_import, + ) + user = get_user() - if existing: + + if existing := find_existing_for_import(Slice, config["uuid"]): if overwrite and can_write and user: if not security_manager.can_access_chart(existing) or ( user not in existing.owners and not security_manager.is_admin() @@ -60,9 +65,15 @@ def import_chart( "A chart already exists and user doesn't " "have permissions to overwrite it" ) - if not overwrite or not can_write: + # Permission check passed. Hard-delete a soft-deleted match + # now (after the check) so the fresh insert below doesn't + # collide on the UUID unique constraint. + if existing.deleted_at is not None: + clear_soft_deleted_for_import(existing) + else: + config["id"] = existing.id + elif not overwrite or not can_write: return existing - config["id"] = existing.id elif not can_write: raise ImportFailedError( "Chart doesn't exist and user doesn't have permission to create charts" diff --git a/superset/commands/chart/restore.py b/superset/commands/chart/restore.py new file mode 100644 index 000000000000..1cec1b3e17f9 --- /dev/null +++ b/superset/commands/chart/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 chart.""" + +import logging + +from superset.commands.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, + ChartRestoreFailedError, +) +from superset.commands.restore import BaseRestoreCommand +from superset.daos.chart import ChartDAO +from superset.models.slice import Slice + +logger = logging.getLogger(__name__) + + +class RestoreChartCommand(BaseRestoreCommand[Slice]): + """Restore a soft-deleted chart by clearing its ``deleted_at`` field. + + All transactional wrapping and validation lives in + ``BaseRestoreCommand``; this subclass is purely declarative. + """ + + dao = ChartDAO + not_found_exc = ChartNotFoundError + forbidden_exc = ChartForbiddenError + restore_failed_exc = ChartRestoreFailedError diff --git a/superset/commands/importers/v1/utils.py b/superset/commands/importers/v1/utils.py index 26442342fd45..397b23bbcd2c 100644 --- a/superset/commands/importers/v1/utils.py +++ b/superset/commands/importers/v1/utils.py @@ -30,6 +30,7 @@ from superset.extensions import feature_flag_manager from superset.models.core import Database from superset.models.dashboard import dashboard_slices +from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES from superset.tags.models import Tag, TaggedObject from superset.utils import json from superset.utils.core import check_is_safe_zip @@ -400,3 +401,51 @@ def get_resource_mappings_batched( mapping.update({str(x.uuid): value_func(x) for x in batch}) offset += batch_size return mapping + + +def find_existing_for_import(model_cls: type[Any], uuid: str) -> Any | None: + """Look up an existing row by UUID for an import operation, + bypassing the soft-delete visibility filter so soft-deleted matches + are returned too. + + Side-effect-free: returns the row as-is whether it's live or + soft-deleted (or ``None`` if no row exists). The caller is + responsible for deciding what to do with a soft-deleted match — + typically calling :func:`clear_soft_deleted_for_import` to remove + it before re-import, but only after the caller has validated + overwrite/permission decisions. + + Splitting the lookup from the destructive cleanup keeps the + destructive action explicit at the call site, so a future change + that adds a permission check on the overwrite path doesn't + silently leave a "duck around it via soft-delete" backdoor. + """ + return ( + db.session.query(model_cls) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {model_cls}}) + .filter_by(uuid=uuid) + .first() + ) + + +def clear_soft_deleted_for_import(existing: Any) -> None: + """Hard-delete a soft-deleted row so a subsequent import of the + same UUID does not collide with the unique constraint. + + Uses ``db.session.delete()`` rather than a raw Core ``DELETE`` so + the ORM ``after_delete`` event listeners fire. Cleanup that depends + on those listeners would otherwise be skipped — notably tag rows in + ``tagged_object`` (cleaned up by ``ObjectUpdater.after_delete`` in + ``superset/tags/core.py``; the table's ``object_id`` is a plain + integer, not a foreign key, so the database cannot cascade them) + and dataset permission-view rows (cleaned up by + ``SqlaTable.after_delete`` in ``superset/connectors/sqla/models.py``). + + Caller contract: ``existing`` must be a soft-deleted row returned + from :func:`find_existing_for_import`. Callers should run their + overwrite / permission validation *before* invoking this so the + destructive action only happens once the import path is committed + to proceeding. + """ + db.session.delete(existing) + db.session.flush() diff --git a/superset/commands/restore.py b/superset/commands/restore.py new file mode 100644 index 000000000000..e90a569959bd --- /dev/null +++ b/superset/commands/restore.py @@ -0,0 +1,98 @@ +# 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. +"""Base class shared by all soft-delete restore commands.""" + +from functools import partial +from typing import Any, ClassVar, Generic, TypeVar + +from superset import security_manager +from superset.commands.base import BaseCommand +from superset.exceptions import SupersetSecurityException +from superset.models.helpers import SoftDeleteMixin +from superset.utils.decorators import on_error, transaction + +T = TypeVar("T", bound=SoftDeleteMixin) + + +class BaseRestoreCommand(BaseCommand, Generic[T]): + """Base class for soft-delete restore commands. + + Subclasses provide the entity-specific bindings as class variables — + no method override required: + + - ``dao``: the DAO class (e.g. ``ChartDAO``) + - ``not_found_exc``: raised when the row doesn't exist OR isn't + soft-deleted + - ``forbidden_exc``: raised when the caller doesn't have ownership + - ``restore_failed_exc``: re-raised by the transactional wrapper + when an underlying SQLAlchemy error aborts the commit + + The transactional wrapper is applied by this class's ``run()`` + using ``restore_failed_exc`` as the rethrow type, so each subclass + just declares the four ClassVars and is done. There is no + subclass-managed decorator contract — earlier iterations of this + PR required subclasses to override ``run()`` purely to add a + ``@transaction`` decorator, which was fragile (every new entity + rollout had to remember). + + The model returned from ``validate()`` is the soft-deleted row, + type-narrowed via ``Generic[T]``. ``run()`` calls ``model.restore()`` + on it (the method comes from ``SoftDeleteMixin``). + """ + + dao: ClassVar[Any] + not_found_exc: ClassVar[type[Exception]] + forbidden_exc: ClassVar[type[Exception]] + restore_failed_exc: ClassVar[type[Exception]] + + def __init__(self, model_uuid: str) -> None: + self._model_uuid = model_uuid + + def run(self) -> None: + # Build the transactional wrapper at call time so ``on_error`` can + # reference ``self.restore_failed_exc`` — a per-subclass ClassVar + # that isn't available when this method is defined on the base. + @transaction(on_error=partial(on_error, reraise=self.restore_failed_exc)) + def _perform() -> None: + model = self.validate() + model.restore() + + _perform() + + def validate(self) -> T: # type: ignore[override] + # ``skip_visibility_filter=True`` is the *only* bypass — the + # entity's RBAC ``base_filter`` stays in effect, matching the + # behavior of ``find_by_ids`` on the existing delete paths. + # Restore should not see rows the user cannot see in the live + # UI; ownership is then verified by ``raise_for_ownership``. + model = self.dao.find_by_id( + self._model_uuid, + id_column="uuid", + skip_visibility_filter=True, + ) + if model is None: + raise self.not_found_exc(f"No row with uuid={self._model_uuid!r}") + if model.deleted_at is None: + raise self.not_found_exc( + f"Row with uuid={self._model_uuid!r} is not soft-deleted; " + "nothing to restore" + ) + try: + security_manager.raise_for_ownership(model) + except SupersetSecurityException as ex: + raise self.forbidden_exc() from ex + return model diff --git a/superset/constants.py b/superset/constants.py index d285fb1f9014..3125a708279d 100644 --- a/superset/constants.py +++ b/superset/constants.py @@ -174,6 +174,7 @@ class RouteMethod: # pylint: disable=too-few-public-methods "put_filters": "write", "put_colors": "write", "sync_permissions": "write", + "restore": "write", } EXTRA_FORM_DATA_APPEND_KEYS = { diff --git a/superset/daos/base.py b/superset/daos/base.py index fe72f1c63d05..042734ceedab 100644 --- a/superset/daos/base.py +++ b/superset/daos/base.py @@ -48,6 +48,7 @@ DAOFindFailedError, ) from superset.extensions import db +from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES, SoftDeleteMixin T = TypeVar("T", bound=CoreModel) @@ -87,8 +88,8 @@ def apply(self, column: Any, value: Any) -> Any: ColumnOperatorEnum.in_: lambda col, val: col.in_( val if isinstance(val, (list, tuple)) else [val] ), - ColumnOperatorEnum.nin: lambda col, val: ~col.in_( - val if isinstance(val, (list, tuple)) else [val] + ColumnOperatorEnum.nin: lambda col, val: ( + ~col.in_(val if isinstance(val, (list, tuple)) else [val]) ), ColumnOperatorEnum.gt: lambda col, val: col > val, ColumnOperatorEnum.gte: lambda col, val: col >= val, @@ -181,11 +182,17 @@ def find_by_id_or_uuid( cls, model_id_or_uuid: str, skip_base_filter: bool = False, + *, + skip_visibility_filter: bool = False, ) -> T | None: """ Find a model by id or uuid, if defined applies `base_filter` """ query = db.session.query(cls.model_cls) + if skip_visibility_filter: + query = query.execution_options( + **{SKIP_VISIBILITY_FILTER_CLASSES: {cls.model_cls}} + ) if cls.base_filter and not skip_base_filter: data_model = SQLAInterface(cls.model_cls, db.session) query = cls.base_filter( # pylint: disable=not-callable @@ -249,6 +256,8 @@ def _find_by_column( value: str | int, skip_base_filter: bool = False, query_options: list[Any] | None = None, + *, + skip_visibility_filter: bool = False, ) -> T | None: """ Private method to find a model by any column value. @@ -257,6 +266,7 @@ def _find_by_column( column_name: Name of the column to search by value: Value to search for skip_base_filter: Whether to skip base filtering + skip_visibility_filter: Whether to skip the soft-delete visibility filter query_options: SQLAlchemy query options (e.g., joinedload, subqueryload) to apply to the query for eager loading @@ -264,6 +274,10 @@ def _find_by_column( Model instance or None if not found """ query = db.session.query(cls.model_cls) + if skip_visibility_filter: + query = query.execution_options( + **{SKIP_VISIBILITY_FILTER_CLASSES: {cls.model_cls}} + ) query = cls._apply_base_filter(query, skip_base_filter) if query_options: @@ -290,6 +304,8 @@ def find_by_id( skip_base_filter: bool = False, id_column: str | None = None, query_options: list[Any] | None = None, + *, + skip_visibility_filter: bool = False, ) -> T | None: """ Find a model by ID using specified or default ID column. @@ -300,12 +316,20 @@ def find_by_id( id_column: Column name to use (defaults to cls.id_column_name) query_options: SQLAlchemy query options (e.g., joinedload, subqueryload) to apply to the query for eager loading + skip_visibility_filter: Keyword-only. Whether to skip the + soft-delete visibility filter Returns: Model instance or None if not found """ column = id_column or cls.id_column_name - return cls._find_by_column(column, model_id, skip_base_filter, query_options) + return cls._find_by_column( + column, + model_id, + skip_base_filter, + query_options, + skip_visibility_filter=skip_visibility_filter, + ) @classmethod def find_by_ids( @@ -313,6 +337,8 @@ def find_by_ids( model_ids: Sequence[str | int], skip_base_filter: bool = False, id_column: str | None = None, + *, + skip_visibility_filter: bool = False, ) -> list[T]: """ Find a List of models by a list of ids, if defined applies `base_filter` @@ -321,6 +347,8 @@ def find_by_ids( :param skip_base_filter: If true, skip applying the base filter :param id_column: Optional column name to use for ID lookup (defaults to id_column_name) + :param skip_visibility_filter: Keyword-only. If true, skip the + soft-delete visibility filter so soft-deleted rows are returned """ column = id_column or cls.id_column_name id_col = getattr(cls.model_cls, column, None) @@ -347,7 +375,12 @@ def find_by_ids( if not converted_ids: return [] - query = db.session.query(cls.model_cls).filter(id_col.in_(converted_ids)) + query = db.session.query(cls.model_cls) + if skip_visibility_filter: + query = query.execution_options( + **{SKIP_VISIBILITY_FILTER_CLASSES: {cls.model_cls}} + ) + query = query.filter(id_col.in_(converted_ids)) query = cls._apply_base_filter(query, skip_base_filter) try: @@ -429,25 +462,51 @@ def update( return item # type: ignore @classmethod - def delete(cls, items: list[T]) -> None: + def soft_delete(cls, items: list[T]) -> None: + """Mark items as soft-deleted by setting ``deleted_at``. + + Only valid for models that include ``SoftDeleteMixin``. + + :param items: The items to soft-delete """ - Delete the specified items including their associated relationships. + for item in items: + item.soft_delete() + + @classmethod + def hard_delete(cls, items: list[T]) -> None: + """Permanently remove rows from the database. - Note that bulk deletion via `delete` is not invoked in the base class as this - does not dispatch the ORM `after_delete` event which may be required to augment - additional records loosely defined via implicit relationships. Instead ORM - objects are deleted one-by-one via `Session.delete`. + Note that bulk deletion via ``delete`` is not invoked in the base + class as this does not dispatch the ORM ``after_delete`` event which + may be required to augment additional records loosely defined via + implicit relationships. Instead ORM objects are deleted one-by-one + via ``Session.delete``. - Subclasses may invoke bulk deletion but are responsible for instrumenting any - post-deletion logic. + Subclasses may invoke bulk deletion but are responsible for + instrumenting any post-deletion logic. :param items: The items to delete :see: https://docs.sqlalchemy.org/en/latest/orm/queryguide/dml.html """ - for item in items: db.session.delete(item) + @classmethod + def delete(cls, items: list[T]) -> None: + """Route to soft or hard delete based on whether the model supports + soft delete. + + For models that include ``SoftDeleteMixin``, this calls + ``soft_delete()``. For all other models, this calls ``hard_delete()`` + (the original behaviour). + + :param items: The items to delete + """ + if cls.model_cls is not None and issubclass(cls.model_cls, SoftDeleteMixin): + cls.soft_delete(items) + else: + cls.hard_delete(items) + @classmethod def query(cls, query: Query) -> list[T]: """ diff --git a/superset/daos/database.py b/superset/daos/database.py index 5b1b33b38393..70c1c3aa38b5 100644 --- a/superset/daos/database.py +++ b/superset/daos/database.py @@ -30,6 +30,7 @@ from superset.extensions import db from superset.models.core import Database, DatabaseUserOAuth2Tokens from superset.models.dashboard import Dashboard +from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES from superset.models.slice import Slice from superset.models.sql_lab import TabState from superset.utils.core import DatasourceType @@ -71,6 +72,8 @@ def find_by_id( skip_base_filter: bool = False, id_column: str | None = None, query_options: list[Any] | None = None, + *, + skip_visibility_filter: bool = False, ) -> Database | None: """ Find a database by id, eagerly loading the SSH tunnel relationship. @@ -79,6 +82,10 @@ def find_by_id( if query_options: all_options.extend(query_options) query = db.session.query(cls.model_cls).options(*all_options) + if skip_visibility_filter: + query = query.execution_options( + **{SKIP_VISIBILITY_FILTER_CLASSES: {cls.model_cls}} + ) query = cls._apply_base_filter(query, skip_base_filter) column_name = id_column or cls.id_column_name diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py index 4f7268458e89..7b1c94237ec5 100644 --- a/superset/initialization/__init__.py +++ b/superset/initialization/__init__.py @@ -767,6 +767,13 @@ def init_app(self) -> None: with self.superset_app.app_context(): self.init_app_in_ctx() + # Registered outside ``init_app_in_ctx`` because the SQLAlchemy + # event hook attaches to the ``Session`` *class* (a process-wide + # global), not to a Session instance — it has no dependency on + # the Flask app context. ``setup_db()`` ran earlier in + # ``init_app``, so the ``Session`` import has already been + # initialised by the time we get here. + self.setup_soft_delete_listener() self.post_init() def set_db_default_isolation(self) -> None: @@ -949,6 +956,23 @@ def setup_db(self) -> None: migrate.init_app(self.superset_app, db=db, directory=APP_DIR + "/migrations") + def setup_soft_delete_listener(self) -> None: + """Register the global soft-delete filter on the SQLAlchemy Session. + + Must be called after ``setup_db()`` so the Session class is + available. Uses the ``do_orm_execute`` + ``with_loader_criteria`` + pattern recommended by SQLAlchemy maintainer Mike Bayer for + soft deletion in SQLAlchemy 1.4+: + https://github.com/sqlalchemy/sqlalchemy/issues/7973#issuecomment-1112561295 + """ + from sqlalchemy import event + from sqlalchemy.orm import Session + + from superset.models.helpers import _add_soft_delete_filter + + if not event.contains(Session, "do_orm_execute", _add_soft_delete_filter): + event.listen(Session, "do_orm_execute", _add_soft_delete_filter) + def configure_wtf(self) -> None: if self.config["WTF_CSRF_ENABLED"]: csrf.init_app(self.superset_app) diff --git a/superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.py b/superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.py new file mode 100644 index 000000000000..16586cc3c887 --- /dev/null +++ b/superset/migrations/versions/2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices.py @@ -0,0 +1,52 @@ +# 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 to slices for soft delete + +Adds a nullable ``deleted_at`` column and index to the ``slices`` table +to support soft deletion of charts (sc-103157 / sc-106189). Companion +to the ``SoftDeleteMixin`` infrastructure shipped in sc-106188. + +Revision ID: 7c4a8d09ca37 +Revises: 33d7e0e21daa +Create Date: 2026-05-08 12:00: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 = "7c4a8d09ca37" +down_revision = "33d7e0e21daa" + +TABLE_NAME = "slices" +INDEX_NAME = f"ix_{TABLE_NAME}_deleted_at" + + +def upgrade() -> None: + add_columns(TABLE_NAME, Column("deleted_at", DateTime(), nullable=True)) + create_index(TABLE_NAME, INDEX_NAME, ["deleted_at"]) + + +def downgrade() -> None: + drop_index(TABLE_NAME, INDEX_NAME) + drop_columns(TABLE_NAME, "deleted_at") diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 7a2668a49ed7..ad8c3403ffb8 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -25,12 +25,14 @@ import logging import re import uuid -from collections.abc import Hashable +from collections.abc import Hashable, Iterator +from contextlib import contextmanager from datetime import datetime, timedelta from typing import ( Any, Callable, cast, + ClassVar, NamedTuple, Optional, TYPE_CHECKING, @@ -57,7 +59,9 @@ from sqlalchemy import and_, Column, or_, UniqueConstraint from sqlalchemy.exc import MultipleResultsFound from sqlalchemy.ext.declarative import declared_attr -from sqlalchemy.orm import Mapper, validates +from sqlalchemy.ext.hybrid import hybrid_property +from sqlalchemy.orm import Mapper, Session, validates, with_loader_criteria +from sqlalchemy.orm.session import ORMExecuteState from sqlalchemy.sql.elements import ColumnElement, Grouping, literal_column, TextClause from sqlalchemy.sql.expression import Label, Select, TextAsFrom from sqlalchemy.sql.selectable import Alias, TableClause @@ -610,7 +614,7 @@ def changed_by_(self) -> Union[Markup, str]: @renders("changed_on") def changed_on_(self) -> Markup: - return Markup(f'{self.changed_on}') + return Markup(f'{self.changed_on}') # noqa: S704 @renders("changed_on") def changed_on_delta_humanized(self) -> str: @@ -654,7 +658,255 @@ def created_on_humanized(self) -> str: @renders("changed_on") def modified(self) -> Markup: - return Markup(f'{self.changed_on_humanized}') + return Markup(f'{self.changed_on_humanized}') # noqa: S704 + + +SKIP_VISIBILITY_FILTER_CLASSES = "_skip_visibility_filter_classes" + +# Shared sentinel for "no bypass requested" — returned by +# ``_collect_bypass_classes`` on the common path so every primary SELECT +# does not allocate a fresh empty set. ``frozenset`` so accidental +# mutation by a caller is a TypeError, not silent corruption. +_NO_BYPASS: frozenset[type] = frozenset() + + +class SoftDeleteMixin: + """Mixin that adds soft-delete support to a SQLAlchemy model. + + Adds a nullable ``deleted_at`` column. When set, the row is treated as + deleted and excluded from standard ORM queries via a global + ``do_orm_execute`` listener registered at app init. + + Delete commands route through ``BaseDAO.delete()``, which detects + the mixin and calls ``soft_delete()`` to mark the row as deleted + (without removing it). ``BaseDAO.hard_delete()`` is the permanent + hard-deletion path; it bypasses the mixin and calls + ``session.delete()`` directly. + + The listener can be bypassed per-entity, either for one statement (via + ``execution_options``) or for a session-scoped block (via + ``session.info`` / the ``skip_visibility_filter`` context manager). + See ``_add_soft_delete_filter`` for the precise semantics. + + Subclass registry: every concrete subclass registers itself in + ``_registered_subclasses`` via ``__init_subclass__``. The listener + iterates this cached list rather than walking ``__subclasses__()`` + on every primary SELECT — important because the listener fires on + every ORM query in the app, and the walk grows with each adopted + entity. + """ + + _registered_subclasses: ClassVar[list[type[SoftDeleteMixin]]] = [] + + def __init_subclass__(cls, **kwargs: Any) -> None: + super().__init_subclass__(**kwargs) + # Cache the subclass once, sorted by qualname so the listener's + # ``with_loader_criteria`` options attach in a deterministic + # order across processes (stable compiled-statement cache key). + if cls in SoftDeleteMixin._registered_subclasses: + return + SoftDeleteMixin._registered_subclasses.append(cls) + SoftDeleteMixin._registered_subclasses.sort(key=lambda c: c.__qualname__) + + deleted_at = sa.Column(sa.DateTime, nullable=True, index=True) + + @hybrid_property + def is_deleted(self) -> bool: + return self.deleted_at is not None + + @is_deleted.expression # type: ignore + def is_deleted(cls) -> ColumnElement: # noqa: N805 + return cls.deleted_at.is_not(None) + + @classmethod + def where_not_deleted(cls) -> ColumnElement: + """Return a SQL WHERE clause that excludes soft-deleted rows. + + Returns a ``ColumnElement`` (a SQL expression), not a Python bool — + use this in a query's ``.filter(...)`` call. The name reflects the + intent ("WHERE NOT deleted") rather than reading like a predicate. + """ + return cls.deleted_at.is_(None) + + def soft_delete(self) -> None: + """Mark this object as soft-deleted.""" + # Naive datetime, mirroring AuditMixinNullable.changed_on. PR #33693 + # reverted a UTC migration on the audit columns; if/when those move + # to UTC-aware, this assignment should follow. + self.deleted_at = datetime.now() + + def restore(self) -> None: + """Clear the soft-delete marker, making this object active again.""" + self.deleted_at = None + + +def _collect_bypass_classes(execute_state: ORMExecuteState) -> frozenset[type]: + """Union of bypass class sets from per-query and per-session sources. + + Per-query: ``execution_options[SKIP_VISIBILITY_FILTER_CLASSES]`` — set + by ``BaseDAO.find_by_id(skip_visibility_filter=True)``, + ``find_existing_for_import``, ``raise_for_ownership``, etc., for + narrow one-statement bypass. + + Per-session: ``session.info[SKIP_VISIBILITY_FILTER_CLASSES]`` — set by + ``BaseDeletedStateFilter.apply`` and the ``skip_visibility_filter`` + context manager, for request-scoped bypass across multiple queries + issued on the same session (e.g., FAB list endpoints that build a + count query plus an inner+outer pair, where per-query options are + stripped between count and outer fetch). + + Returns the shared empty ``_NO_BYPASS`` sentinel when neither source + has a bypass set — the common case for every ORM SELECT in the app — + so the listener's hot path does not allocate per-call empty sets. + """ + per_query = execute_state.execution_options.get(SKIP_VISIBILITY_FILTER_CLASSES) + per_session = execute_state.session.info.get(SKIP_VISIBILITY_FILTER_CLASSES) + if not per_query and not per_session: + return _NO_BYPASS + return frozenset(per_query or ()) | frozenset(per_session or ()) + + +def _should_attach_soft_delete_criteria(execute_state: ORMExecuteState) -> bool: + """The event classes where the listener attaches loader criteria. + + Column loads are excluded: they re-execute against already-loaded + parents to refresh specific attribute values; the soft-delete + criteria isn't meaningful at that level and would add noise. + + Relationship loads ARE included. The Bayer canonical pattern + excludes them on the assumption that + ``with_loader_criteria(..., propagate_to_loaders=True)`` carries + the criteria from the parent statement to its relationship loads + automatically. In practice this propagation isn't reliable when + the criteria targets a class that doesn't appear in the parent + statement (e.g., loading a ``Dashboard`` whose listener-attached + criteria targets ``Slice`` — Slice never appears in the parent + query, and the criteria doesn't always reach the + ``dashboard.slices`` lazy load). Re-attaching on the + relationship-load event closes that gap. The resulting WHERE + clause may have ``deleted_at IS NULL`` twice when propagation + DOES work — harmless redundancy, idempotent SQL. + """ + return execute_state.is_select and not execute_state.is_column_load + + +def _all_soft_delete_subclasses() -> list[type[SoftDeleteMixin]]: + """The cached subclass registry maintained by + ``SoftDeleteMixin.__init_subclass__``. Returned in a stable + qualname-sorted order so SQLAlchemy's compiled-statement cache key + is deterministic across processes. + + Assumes all soft-deletable models have been imported by the time + the listener fires. Superset imports models eagerly at app init via + ``superset.models``; if that ever changes to lazy import, the + listener would silently stop filtering un-imported classes — but + since registration happens at class-definition time, the cache is + automatically updated as new subclasses are introduced (including + test-defined synthetic subclasses). + """ + return SoftDeleteMixin._registered_subclasses + + +def _add_soft_delete_filter(execute_state: ORMExecuteState) -> None: + """Global ``do_orm_execute`` listener that automatically excludes + soft-deleted rows from every ORM SELECT. + + Uses SQLAlchemy's recommended soft-delete pattern + (``do_orm_execute`` + ``with_loader_criteria`` — see + https://github.com/sqlalchemy/sqlalchemy/issues/7973#issuecomment-1112561295). + + Skips relationship and column loader paths: those propagate the + criteria from the parent statement via + ``with_loader_criteria(..., propagate_to_loaders=True)`` (the default) + rather than re-attaching it here, which would stack redundant + ``deleted_at IS NULL`` clauses. + + Per-class scoping: the listener iterates concrete ``SoftDeleteMixin`` + subclasses and attaches a ``with_loader_criteria`` only for those + NOT in the request's bypass set. A bypass for ``Dashboard`` therefore + does not unhide soft-deleted ``Slice`` or ``SqlaTable`` rows in the + same statement. Each criteria is a concrete SQL expression rather + than a callable, so SQLAlchemy compiles it normally (passing a + callable triggers ``DeferredLambdaElement`` parsing, which does not + support Python control flow like ``if cls in bypass``). + + Opt-out: + + * **One statement**: attach + ``execution_options(_skip_visibility_filter_classes={Model})`` to a + Query, or pass ``skip_visibility_filter=True`` to ``BaseDAO`` + methods (which translate the boolean into a one-class set + internally). + * **Session-scoped**: set + ``session.info[SKIP_VISIBILITY_FILTER_CLASSES] = {Model, ...}`` or + use the ``skip_visibility_filter`` context manager. Survives FAB's + inner/outer query reconstruction (and any future framework that + strips per-query options). + + Performance: the listener attaches one ``with_loader_criteria`` + option per non-bypassed ``SoftDeleteMixin`` subclass to every + primary SELECT, including queries that don't reference any + soft-deletable entity. SQLAlchemy no-ops the criteria when the + targeted class isn't in the statement, so the cost is small per + query, but linear in the number of soft-delete classes. At ~10 + entities this is still negligible on typical endpoints; profile + before adding many more. + """ + if not _should_attach_soft_delete_criteria(execute_state): + return + + bypass_classes = _collect_bypass_classes(execute_state) + + for cls in _all_soft_delete_subclasses(): + if cls in bypass_classes: + continue + # Pass the criteria as a lambda — SQLAlchemy adapts the column + # reference to whatever alias the class wears at each occurrence + # in the statement (critical for FAB's outer/inner reconstruction + # and any other code that aliases the same model under different + # names). A concrete SQL expression — ``cls.where_not_deleted()`` + # — would render as the raw ``slices.deleted_at`` even when the + # statement actually aliases ``slices AS chart``, producing + # ``Unknown column 'slices.deleted_at' in 'on clause'``. The + # lambda's body is trivial so the ``DeferredLambdaElement`` + # parser handles it without issue; complex control flow inside + # the lambda is what trips the parser, not simple attribute + # access. + execute_state.statement = execute_state.statement.options( + with_loader_criteria( + cls, + lambda c: c.deleted_at.is_(None), + include_aliases=True, + ) + ) + + +@contextmanager +def skip_visibility_filter(session: Session, *classes: type) -> Iterator[None]: + """Bypass the soft-delete listener for the given classes within this + session for the duration of the ``with`` block. + + Adds ``classes`` to ``session.info[SKIP_VISIBILITY_FILTER_CLASSES]`` + on entry and removes them on exit, restoring the prior state. Nesting + is safe: an inner block only removes the classes *it* added, so the + outer block's bypass remains in effect. + + Usage:: + + with skip_visibility_filter(session, Dashboard): + return session.query(Dashboard).filter_by(uuid=u).first() + + Prefer this over manually setting ``session.info`` so the cleanup is + guaranteed even on exceptions. Calling with no classes is a no-op + (the block runs with no bypass added). + """ + bypass = session.info.setdefault(SKIP_VISIBILITY_FILTER_CLASSES, set()) + added = set(classes) - bypass + bypass.update(added) + try: + yield + finally: + bypass -= added class QueryResult: # pylint: disable=too-few-public-methods diff --git a/superset/models/slice.py b/superset/models/slice.py index 04c698ce95da..02ccbd1d2f0a 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -42,7 +42,11 @@ from superset import db, is_feature_enabled, security_manager from superset.legacy import update_time_range -from superset.models.helpers import AuditMixinNullable, ImportExportMixin +from superset.models.helpers import ( + AuditMixinNullable, + ImportExportMixin, + SoftDeleteMixin, +) from superset.tasks.thumbnails import cache_chart_thumbnail from superset.tasks.utils import get_current_user from superset.thumbnails.digest import get_chart_digest @@ -66,7 +70,7 @@ class Slice( # pylint: disable=too-many-public-methods - CoreChart, AuditMixinNullable, ImportExportMixin + CoreChart, SoftDeleteMixin, AuditMixinNullable, ImportExportMixin ): """A slice is essentially a report or a view on data""" diff --git a/superset/security/manager.py b/superset/security/manager.py index 9a5055d43fe0..9084c8d5c904 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -3548,13 +3548,55 @@ def raise_for_ownership(self, resource: Model) -> None: Note admins are deemed owners of all resources. + The internal re-query opts out of the soft-delete visibility + listener via ``execution_options(_skip_visibility_filter_classes= + {resource.__class__})`` so callers passing a soft-deleted resource + (e.g., ``BaseRestoreCommand``) get the correct ownership + decision. The bypass is scoped to ``resource.__class__`` only — + any soft-deletable relationships read from ``orig_resource`` + (none today; ``.owners`` is a User) remain filtered. + :param resource: The dashboard, dataset, chart, etc. resource :raises SupersetSecurityException: If the current user is not an owner """ + # Inline import: ``superset.models.helpers`` transitively imports + # ``superset.models.core``, which depends on lazily-initialised + # ``superset.feature_flag_manager``. A top-level import here would + # create a circular dependency (security ↔ models.core ↔ superset). + from superset.models.helpers import ( # pylint: disable=import-outside-toplevel # noqa: E501 + SKIP_VISIBILITY_FILTER_CLASSES, + ) if self.is_admin(): return - orig_resource = self.session.query(resource.__class__).get(resource.id) + + # The internal re-query below is filtered by the global soft-delete + # listener for any ``SoftDeleteMixin`` model. Callers that have + # intentionally loaded a soft-deleted resource (e.g., + # ``BaseRestoreCommand``) need the re-query to see the row so the + # owners list can be read. Attach the bypass scoped to this + # resource's class only — the per-query option is enough here + # because ``.get()`` resolves directly without going through any + # framework that strips options. + orig_resource = ( + self.session.query(resource.__class__) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {resource.__class__}}) + .get(resource.id) + ) + # Explicit guard: ``orig_resource`` is ``None`` only if a parallel + # writer hard-deleted the row between the caller's load and this + # re-query. Falling through with ``owners=[]`` would surface as a + # misleading "ownership" error; raise the real cause instead. + if orig_resource is None: + raise SupersetSecurityException( + SupersetError( + error_type=SupersetErrorType.MISSING_OWNERSHIP_ERROR, + message=_( + "Resource was removed before ownership could be verified", + ), + level=ErrorLevel.ERROR, + ) + ) owners = orig_resource.owners if hasattr(orig_resource, "owners") else [] if g.user.is_anonymous or g.user not in owners: diff --git a/superset/views/filters.py b/superset/views/filters.py index c077e1910f5a..4e26d17e1a1e 100644 --- a/superset/views/filters.py +++ b/superset/views/filters.py @@ -15,15 +15,18 @@ # specific language governing permissions and limitations # under the License. import logging -from typing import Any, cast, Optional +from datetime import datetime +from typing import Any, cast, ClassVar, Optional -from flask import current_app as app +from flask import current_app as app, g from flask_appbuilder.models.filters import BaseFilter from flask_babel import lazy_gettext from sqlalchemy import and_, or_ from sqlalchemy.orm import Query from superset import security_manager +from superset.extensions import db +from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES, SoftDeleteMixin logger = logging.getLogger(__name__) @@ -121,3 +124,209 @@ def apply(self, query: Query, value: Optional[Any]) -> Query: like_value = "%" + cast(str, value) + "%" return query.filter(SqlaTable.table_name.ilike(like_value)) + + +AUGMENT_RESPONSE_WITH_DELETED_AT = "_augment_response_with_deleted_at" + +# Tracks the classes that ``BaseDeletedStateFilter`` added to +# ``session.info[SKIP_VISIBILITY_FILTER_CLASSES]`` for this request, +# so ``SoftDeleteApiMixin.pre_get_list`` can remove only those (and not +# any entries a programmatic caller — context manager, DAO bypass — +# may have placed there independently). +DELETED_STATE_ADDED_CLASSES = "_deleted_state_added_classes" + + +class BaseDeletedStateFilter(BaseFilter): # pylint: disable=too-few-public-methods + """Base class for ``*_deleted_state`` rison filters. + + Subclasses set ``arg_name`` (e.g. ``"chart_deleted_state"``) and + ``model`` (the SoftDeleteMixin model class). Values: + + * ``include`` — return live + soft-deleted rows + * ``only`` — return only soft-deleted rows + * absent / any other value — default behaviour (live rows only) + + Scope decisions: + + * The visibility-filter bypass is applied at the **session** level + and scoped to the filter's own ``model`` class only. FAB list + endpoints construct multiple statements per request (count, then + an inner + outer pair for many-to-many ``list_columns``) and the + outer fetch is built from a fresh ``session.query(self.obj)`` + that drops per-query ``execution_options``. Session-scoped + bypass survives that reconstruction; per-class scoping prevents + the bypass from unhiding soft-deleted rows of any *other* + ``SoftDeleteMixin`` entity that the request might touch. + * The bypass is **released after the list response is augmented** + by ``SoftDeleteApiMixin.pre_get_list``, so any code that runs + later in the same request (audit hooks, ``after_request`` + handlers, dependent operations during response serialisation) + sees normal filtered visibility. The release is scoped to the + classes *this filter* added — programmatic callers using the + ``skip_visibility_filter`` context manager or DAO bypass are + unaffected. Classes added are tracked in + ``g._deleted_state_added_classes`` (request-scoped, auto-cleans + at request teardown). + * The response-augmentation step (which adds a ``deleted_at`` + field to each result row) is signalled via a separate + request-scoped flag ``g._augment_response_with_deleted_at``. + Two concerns, two channels. + """ + + name = lazy_gettext("Deleted state") + # Subclasses bind ``model`` to a concrete ``SoftDeleteMixin`` + # subclass. Typed as ``type[SoftDeleteMixin]`` so a subclass that + # accidentally binds to a non-soft-deletable entity fails mypy + # rather than crashing at runtime on ``.deleted_at``. + model: ClassVar[type[SoftDeleteMixin]] + + def apply(self, query: Query, value: Any) -> Query: + normalized = str(value).lower().strip() if value is not None else "" + if normalized not in {"include", "only"}: + return query + self._opt_into_deleted_state(query) + if normalized == "only": + return query.filter(self.model.deleted_at.is_not(None)) + return query + + def _opt_into_deleted_state(self, query: Query) -> None: + """The two-step opt-in shared by ``include`` and ``only``: install + the per-class session bypass so the listener stops filtering this + entity, and signal to ``SoftDeleteApiMixin.pre_get_list`` that + result rows should carry a ``deleted_at`` field. + """ + self._add_session_bypass(query) + self._mark_response_for_deleted_at_augmentation() + + def _add_session_bypass(self, query: Query) -> None: + """Add ``self.model`` to the session's bypass class set, so the + listener stops filtering this entity for FAB's count + inner + + outer queries. The class is removed from the bypass set by + ``SoftDeleteApiMixin._release_session_bypass`` after + ``pre_get_list`` augments the response — so any code that runs + later in the same request sees normal filtered visibility. + + The class is also recorded in ``g._deleted_state_added_classes`` + so the release step removes only the entries *this filter* + added, leaving any entries placed by the ``skip_visibility_filter`` + context manager or DAO bypass intact. + """ + bypass = query.session.info.setdefault(SKIP_VISIBILITY_FILTER_CLASSES, set()) + bypass.add(self.model) + # Track for release in ``SoftDeleteApiMixin._release_session_bypass``. + added: set[type[SoftDeleteMixin]] = getattr( + g, DELETED_STATE_ADDED_CLASSES, set() + ) | {self.model} + setattr(g, DELETED_STATE_ADDED_CLASSES, added) + + @staticmethod + def _mark_response_for_deleted_at_augmentation() -> None: + """Signal to ``SoftDeleteApiMixin.pre_get_list`` that this request + opted into surfacing soft-deleted rows, so the response rows + should be augmented with their ``deleted_at`` value. + + Distinct from the visibility-filter bypass, which is applied at + the session level on the filter's own model class. + """ + setattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT, True) + + +class SoftDeleteApiMixin: + """API mixin that augments list responses with a ``deleted_at`` + field on each row when the request opted into surfacing soft-deleted + rows via the entity's ``BaseDeletedStateFilter`` subclass. + + Mount this on concrete REST API classes for entities that include + ``SoftDeleteMixin``:: + + class ChartRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi): + ... + + The mixin chains via ``super().pre_get_list(data)``, so other + ``pre_get_list`` behaviour in the inheritance chain still runs. + When the request has not opted into soft-deleted visibility, the + augmentation is a no-op. + """ + + # Concrete subclasses bind these via FAB's ModelRestApi machinery. + datamodel: Any # SQLAInterface providing get_pk_name() and .obj + + def pre_get_list(self, data: dict[str, Any]) -> None: + super().pre_get_list(data) # type: ignore[misc] + if not self._consume_augmentation_flag(): + return + try: + self._inject_deleted_at(data) + finally: + # Release the session-scoped bypass now that FAB is done with + # the list query. Code that runs later in the same request + # (after_request handlers, post-response audit hooks) sees + # normal filtered visibility rather than the widened scope + # the filter installed for the list query. + self._release_session_bypass() + + @staticmethod + def _release_session_bypass() -> None: + """Remove from ``session.info[SKIP_VISIBILITY_FILTER_CLASSES]`` + only the classes ``BaseDeletedStateFilter._add_session_bypass`` + added for this request. Programmatic bypasses installed by the + ``skip_visibility_filter`` context manager or DAO methods (which + manage their own lifecycle) remain untouched. + """ + added: set[type[SoftDeleteMixin]] = getattr( + g, DELETED_STATE_ADDED_CLASSES, set() + ) + if not added: + return + bypass: set[type[SoftDeleteMixin]] = db.session.info.get( + SKIP_VISIBILITY_FILTER_CLASSES, set() + ) + bypass -= added + setattr(g, DELETED_STATE_ADDED_CLASSES, set()) + + @staticmethod + def _consume_augmentation_flag() -> bool: + """Read-and-clear the request-scoped augmentation flag. Returning + ``True`` means the caller should inject ``deleted_at`` into the + response. Clearing prevents the flag from leaking to a later + list operation within the same request (e.g., a batch endpoint + dispatching multiple list views). + """ + requested = getattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT, False) + setattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT, False) + return requested + + def _inject_deleted_at(self, data: dict[str, Any]) -> None: + """Augment each result row with its ``deleted_at`` value, fetched + from the DB in a single projection query keyed by the IDs FAB + already collected. + """ + ids = cast(list[Any], data.get("ids", [])) + deleted_at_map = self._get_deleted_at_map(ids) + for row, row_id in zip(data.get("result", []), ids, strict=False): + row["deleted_at"] = deleted_at_map.get(row_id) + + def _get_deleted_at_map(self, ids: list[Any]) -> dict[Any, str | None]: + if not ids: + return {} + # Raw session query — read-only projection of two columns on + # already-known IDs, not a general entity lookup. The + # primary-key column is resolved via the datamodel rather than + # hardcoded to ``id`` so entities with non-integer PKs work + # without changes here. + pk_name = self.datamodel.get_pk_name() + pk_col = getattr(self.datamodel.obj, pk_name) + rows = ( + db.session.query(pk_col, self.datamodel.obj.deleted_at) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {self.datamodel.obj}}) + .filter(pk_col.in_(ids)) + .all() + ) + return { + row_id: self._serialize_deleted_at(deleted_at) + for row_id, deleted_at in rows + } + + @staticmethod + def _serialize_deleted_at(value: datetime | None) -> str | None: + return value.isoformat() if value else None diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py index b5fa05b72646..86e519d40679 100644 --- a/tests/integration_tests/charts/commands_tests.py +++ b/tests/integration_tests/charts/commands_tests.py @@ -656,7 +656,7 @@ def test_fave_unfave_chart_command(self): def test_fave_unfave_chart_command_not_found(self): """Test that faving / unfaving a non-existing chart raises an exception""" with self.client.application.test_request_context(): - example_chart_id = 1234 + example_chart_id = 0 with override_user(security_manager.find_user("admin")): with self.assertRaises(ChartNotFoundError): # noqa: PT027 diff --git a/tests/integration_tests/charts/soft_delete_tests.py b/tests/integration_tests/charts/soft_delete_tests.py new file mode 100644 index 000000000000..a21df8ae8a8a --- /dev/null +++ b/tests/integration_tests/charts/soft_delete_tests.py @@ -0,0 +1,376 @@ +# 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. +"""Integration tests for chart soft-delete and restore (sc-103157).""" + +from superset.extensions import db +from superset.models.dashboard import Dashboard +from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES +from superset.models.slice import Slice +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME +from tests.integration_tests.insert_chart_mixin import InsertChartMixin + + +def _hard_delete_chart(chart_id: int) -> None: + """Hard-delete a chart row regardless of soft-delete state.""" + row = ( + db.session.query(Slice) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}}) + .filter(Slice.id == chart_id) + .one_or_none() + ) + if row: + db.session.delete(row) + db.session.commit() + + +def _hard_delete_dashboard_for_charts_test(dashboard_id: int) -> None: + """Hard-delete a dashboard row regardless of soft-delete state.""" + row = ( + db.session.query(Dashboard) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Dashboard}}) + .filter(Dashboard.id == dashboard_id) + .one_or_none() + ) + if row: + db.session.delete(row) + db.session.commit() + + +class TestChartSoftDelete(InsertChartMixin, SupersetTestCase): + """Tests for chart soft-delete behaviour (T013, T016).""" + + def test_delete_chart_soft_deletes(self): + """DELETE /api/v1/chart/ sets deleted_at instead of removing.""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("soft_delete_test", [admin_id], 1) + chart_id = chart.id + self.login(ADMIN_USERNAME) + + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200 + + # Row still exists in DB with deleted_at set + row = ( + db.session.query(Slice) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}}) + .filter(Slice.id == chart_id) + .one_or_none() + ) + assert row is not None + assert row.deleted_at is not None + + # Cleanup + _hard_delete_chart(chart_id) + + def test_soft_deleted_chart_excluded_from_get(self): + """GET /api/v1/chart/ returns 404 for a soft-deleted chart.""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("invisible_chart", [admin_id], 1) + chart_id = chart.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/chart/{chart_id}") + rv = self.client.get(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 404 + + # Cleanup + _hard_delete_chart(chart_id) + + def test_soft_deleted_chart_excluded_from_list(self): + """GET /api/v1/chart/ should not include soft-deleted charts.""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("listed_then_deleted", [admin_id], 1) + chart_id = chart.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/chart/{chart_id}") + rv = self.client.get("/api/v1/chart/") + data = json.loads(rv.data) + chart_ids = [c["id"] for c in data["result"]] + assert chart_id not in chart_ids + + # Cleanup + _hard_delete_chart(chart_id) + + def test_soft_deleted_chart_included_in_list_when_requested(self): + """GET /api/v1/chart/ with chart_deleted_state=include returns deleted charts.""" # noqa: E501 + admin_id = self.get_user("admin").id + chart = self.insert_chart("listed_with_deleted", [admin_id], 1) + chart_id = chart.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/chart/{chart_id}") + + rison_query = "(filters:!((col:id,opr:chart_deleted_state,value:include)))" + rv = self.client.get(f"/api/v1/chart/?q={rison_query}") + assert rv.status_code == 200 + + data = json.loads(rv.data) + deleted_row = next( + (row for row in data["result"] if row["id"] == chart_id), + None, + ) + assert deleted_row is not None + assert deleted_row["deleted_at"] is not None + + # Cleanup + _hard_delete_chart(chart_id) + + def test_only_filter_returns_only_soft_deleted_charts(self): + """chart_deleted_state=only excludes live rows and returns only deleted ones.""" + admin_id = self.get_user("admin").id + live_chart = self.insert_chart("only_live", [admin_id], 1) + deleted_chart = self.insert_chart("only_deleted", [admin_id], 1) + live_id = live_chart.id + deleted_id = deleted_chart.id + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/chart/{deleted_id}") + + rison_query = "(filters:!((col:id,opr:chart_deleted_state,value:only)))" + rv = self.client.get(f"/api/v1/chart/?q={rison_query}") + assert rv.status_code == 200 + + data = json.loads(rv.data) + returned_ids = {row["id"] for row in data["result"]} + assert deleted_id in returned_ids + assert live_id not in returned_ids + + # Cleanup + _hard_delete_chart(live_id) + _hard_delete_chart(deleted_id) + + def test_delete_already_soft_deleted_chart_returns_404(self): + """DELETE on an already soft-deleted chart returns 404 (FR-008).""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("double_delete_test", [admin_id], 1) + chart_id = chart.id + self.login(ADMIN_USERNAME) + + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200 + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 404 + + # Cleanup + _hard_delete_chart(chart_id) + + def test_delete_chart_blocked_when_active_report_references_it(self): + """DELETE /api/v1/chart/ returns 422 when a report references it. + + Pins down the existing API protection in `DeleteChartCommand.validate()`: + when a `report_schedule` row references the chart, the validation + raises `ChartDeleteFailedReportsExistError` *before* `ChartDAO.delete()` + is invoked, so no soft-delete routing happens. This is the contract + soft-delete inherits from the pre-existing API and is what makes the + "report-execution against soft-deleted target" crash class + (commands/report/execute.py:_get_url) unreachable through the API. + """ + from superset.reports.models import ( + ReportCreationMethod, + ReportSchedule, + ReportScheduleType, + ) + + admin_id = self.get_user("admin").id + chart = self.insert_chart("blocked_by_report_test", [admin_id], 1) + chart_id = chart.id + + report = ReportSchedule( + type=ReportScheduleType.REPORT, + name="blocking_report_for_chart_delete", + description="Report that should block chart deletion", + crontab="0 9 * * *", + chart=chart, + creation_method=ReportCreationMethod.ALERTS_REPORTS, + ) + db.session.add(report) + db.session.commit() + report_id = report.id + + self.login(ADMIN_USERNAME) + + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 422 + body = json.loads(rv.data) + assert "associated alerts or reports" in body.get("message", "").lower() or ( + "associated" in body.get("message", "").lower() + and "report" in body.get("message", "").lower() + ) + assert "blocking_report_for_chart_delete" in body.get("message", "") + + # Confirm the chart was NOT soft-deleted (deleted_at remains NULL). + row = ( + db.session.query(Slice) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}}) + .filter(Slice.id == chart_id) + .one() + ) + assert row.deleted_at is None + + # Cleanup + db.session.delete( + db.session.query(ReportSchedule) + .filter(ReportSchedule.id == report_id) + .one() + ) + db.session.commit() + _hard_delete_chart(chart_id) + + +class TestChartRestore(InsertChartMixin, SupersetTestCase): + """Tests for chart restore behaviour (T025).""" + + def test_restore_soft_deleted_chart(self): + """POST /api/v1/chart//restore makes the chart visible again.""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("restore_test", [admin_id], 1) + chart_id = chart.id + chart_uuid = str(chart.uuid) + self.login(ADMIN_USERNAME) + + self.client.delete(f"/api/v1/chart/{chart_id}") + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 200 + + rv = self.client.get(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200 + + # Cleanup + _hard_delete_chart(chart_id) + + def test_restore_nonexistent_chart_returns_404(self): + """POST /api/v1/chart//restore returns 404 for unknown UUID.""" + self.login(ADMIN_USERNAME) + rv = self.client.post( + "/api/v1/chart/00000000-0000-0000-0000-000000000000/restore" + ) + assert rv.status_code == 404 + + def test_restore_active_chart_returns_404(self): + """POST /api/v1/chart//restore on active chart returns 404.""" + admin_id = self.get_user("admin").id + chart = self.insert_chart("active_restore_test", [admin_id], 1) + chart_id = chart.id + chart_uuid = str(chart.uuid) + self.login(ADMIN_USERNAME) + + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 404 + + # Cleanup + _hard_delete_chart(chart_id) + + def test_restore_chart_reattaches_to_dashboards(self): + """Soft-deleting a chart preserves dashboard_slices junction rows; + restore makes the chart reappear in its dashboards automatically. + + This is the positive test that pins down the SIP's "no cascade" + contract and the corrected commit ``feat(soft-delete): preserve + dashboard_slices on chart soft-delete (MissingChart handles UI)``. + Soft-delete leaves the junction intact so: + + - dashboards continue to render the chart slot (frontend uses + ``MissingChart`` placeholder while the chart is hidden via the + visibility filter) + - on restore the chart is automatically a member of every + dashboard it was a member of before, with no manual + re-attachment step + """ + from superset.models.dashboard import dashboard_slices + + admin = self.get_user("admin") + admin_id = admin.id + + chart = self.insert_chart("reattach_test_chart", [admin_id], 1) + chart_id = chart.id + chart_uuid = str(chart.uuid) + + dashboard = Dashboard( + dashboard_title="reattach_test_dashboard", + slug="slug_reattach_test", + owners=[admin], + published=True, + ) + dashboard.slices = [chart] + db.session.add(dashboard) + db.session.commit() + dashboard_id = dashboard.id + + # Sanity: the junction row exists + junction_count = ( + db.session.query(dashboard_slices) + .filter( + dashboard_slices.c.dashboard_id == dashboard_id, + dashboard_slices.c.slice_id == chart_id, + ) + .count() + ) + assert junction_count == 1, "junction row should exist after dashboard creation" + + self.login(ADMIN_USERNAME) + + # Soft-delete the chart + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200 + + # The junction row is preserved (no cascade) + junction_count_after_delete = ( + db.session.query(dashboard_slices) + .filter( + dashboard_slices.c.dashboard_id == dashboard_id, + dashboard_slices.c.slice_id == chart_id, + ) + .count() + ) + assert junction_count_after_delete == 1, ( + "junction row should remain intact on chart soft-delete; " + "MissingChart placeholder handles the UI gap" + ) + + # The dashboard's loaded `slices` collection no longer includes the + # soft-deleted chart (the global visibility filter applies to + # relationship loads via `with_loader_criteria(..., include_aliases=True)`). + db.session.expire_all() + dashboard_after_delete = ( + db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one() + ) + assert chart_id not in [s.id for s in dashboard_after_delete.slices], ( + "soft-deleted chart should be filtered out of dashboard.slices " + "by the visibility-filter listener" + ) + + # Restore the chart + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 200 + + # The chart automatically reappears in the dashboard — junction row + # was preserved, so no manual reattach was needed. + db.session.expire_all() + dashboard_after_restore = ( + db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one() + ) + assert chart_id in [s.id for s in dashboard_after_restore.slices], ( + "restored chart should reappear in dashboard.slices automatically; " + "the junction row was never removed by soft-delete" + ) + + # Cleanup + _hard_delete_dashboard_for_charts_test(dashboard_id) + _hard_delete_chart(chart_id) diff --git a/tests/unit_tests/commands/chart/restore_test.py b/tests/unit_tests/commands/chart/restore_test.py new file mode 100644 index 000000000000..5f1cb8cd13ec --- /dev/null +++ b/tests/unit_tests/commands/chart/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 RestoreChartCommand.""" + +from __future__ import annotations + +from datetime import datetime +from unittest.mock import MagicMock, patch + +import pytest + + +def test_restore_chart_clears_deleted_at(app_context: None) -> None: + """RestoreChartCommand.run() restores a soft-deleted chart.""" + from superset.commands.chart.restore import RestoreChartCommand + + chart = MagicMock() + chart.deleted_at = datetime(2026, 1, 1) + chart.id = 1 + + with ( + patch( + "superset.daos.chart.ChartDAO.find_by_id", return_value=chart + ) as mock_find, + patch("superset.commands.restore.security_manager") as mock_sec, + ): + mock_sec.raise_for_ownership.return_value = None + + cmd = RestoreChartCommand("1") + cmd.run() + + mock_find.assert_called_once() + chart.restore.assert_called_once() + + +def test_restore_chart_not_found_raises(app_context: None) -> None: + """RestoreChartCommand raises ChartNotFoundError for missing chart.""" + from superset.commands.chart.exceptions import ChartNotFoundError + from superset.commands.chart.restore import RestoreChartCommand + + with patch("superset.daos.chart.ChartDAO.find_by_id", return_value=None): + cmd = RestoreChartCommand("999") + with pytest.raises(ChartNotFoundError): + cmd.run() + + +def test_restore_active_chart_raises_not_found(app_context: None) -> None: + """RestoreChartCommand raises ChartNotFoundError for non-deleted chart.""" + from superset.commands.chart.exceptions import ChartNotFoundError + from superset.commands.chart.restore import RestoreChartCommand + + chart = MagicMock() + chart.deleted_at = None # not soft-deleted + + with patch("superset.daos.chart.ChartDAO.find_by_id", return_value=chart): + cmd = RestoreChartCommand("1") + with pytest.raises(ChartNotFoundError): + cmd.run() + + +def test_restore_chart_forbidden_raises(app_context: None) -> None: + """RestoreChartCommand raises ChartForbiddenError on permission check.""" + from superset.commands.chart.exceptions import ChartForbiddenError + from superset.commands.chart.restore import RestoreChartCommand + from superset.exceptions import SupersetSecurityException + + chart = MagicMock() + chart.deleted_at = datetime(2026, 1, 1) + + def raise_security(*args: object, **kwargs: object) -> None: + raise SupersetSecurityException(MagicMock()) + + with ( + patch("superset.daos.chart.ChartDAO.find_by_id", return_value=chart), + patch("superset.commands.restore.security_manager") as mock_sec, + ): + mock_sec.raise_for_ownership = raise_security + + cmd = RestoreChartCommand("1") + with pytest.raises(ChartForbiddenError): + cmd.run() diff --git a/tests/unit_tests/commands/importers/v1/test_find_existing_for_import.py b/tests/unit_tests/commands/importers/v1/test_find_existing_for_import.py new file mode 100644 index 000000000000..e15e5f85af3c --- /dev/null +++ b/tests/unit_tests/commands/importers/v1/test_find_existing_for_import.py @@ -0,0 +1,180 @@ +# 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 ``find_existing_for_import`` / +``clear_soft_deleted_for_import``. + +These pin the side-effect-free / side-effect split that addresses +Richard's review on PR #39977: ``find`` returns soft-deleted matches +as-is so the importer can decide overwrite/permission before +``clear_soft_deleted_for_import`` performs the destructive op. +""" + +from __future__ import annotations + +from collections.abc import Generator + +import pytest +from sqlalchemy import Column, Integer, String +from sqlalchemy.orm import declarative_base +from sqlalchemy.orm.session import Session +from sqlalchemy_utils import UUIDType + +from superset.commands.importers.v1.utils import ( + clear_soft_deleted_for_import, + find_existing_for_import, +) +from superset.models.helpers import SoftDeleteMixin + +_TestBase = declarative_base() + + +class _ImportableSoftDeletable(SoftDeleteMixin, _TestBase): # type: ignore[misc, valid-type] + __tablename__ = "_importable_soft_deletable_test" + id = Column(Integer, primary_key=True) + name = Column(String, nullable=False) + uuid = Column(UUIDType(binary=False), unique=True) + + +@pytest.fixture +def _synthetic_table(session: Session) -> Generator[None, None, None]: + _ImportableSoftDeletable.metadata.create_all(session.get_bind()) + yield + _ImportableSoftDeletable.metadata.drop_all(session.get_bind()) + + +@pytest.mark.usefixtures("_synthetic_table") +def test_find_returns_none_when_no_match(app_context: None, session: Session) -> None: + """No row with this UUID — function returns ``None``.""" + result = find_existing_for_import( + _ImportableSoftDeletable, + "00000000-0000-0000-0000-000000000000", + ) + assert result is None + + +@pytest.mark.usefixtures("_synthetic_table") +def test_find_returns_live_row_as_is( + app_context: None, session: Session, mocker: object +) -> None: + """A live (not soft-deleted) row is returned as the existing row + so the caller can treat it as an overwrite target.""" + import uuid as uuid_lib + + obj_uuid = uuid_lib.uuid4() + obj = _ImportableSoftDeletable(name="live", uuid=obj_uuid) + session.add(obj) + session.flush() + + result = find_existing_for_import(_ImportableSoftDeletable, str(obj_uuid)) + assert result is not None + assert result.deleted_at is None + assert result.name == "live" + + +@pytest.mark.usefixtures("_synthetic_table") +def test_find_returns_soft_deleted_row_as_is( + app_context: None, session: Session +) -> None: + """A soft-deleted row is returned *with its ``deleted_at`` set*. + Caller is responsible for deciding what to do with it — typically + calling ``clear_soft_deleted_for_import`` after a permission + check. The function does NOT hard-delete as a side effect. + """ + import uuid as uuid_lib + + obj_uuid = uuid_lib.uuid4() + obj = _ImportableSoftDeletable(name="deleted_target", uuid=obj_uuid) + session.add(obj) + session.flush() + obj.soft_delete() + session.flush() + + result = find_existing_for_import(_ImportableSoftDeletable, str(obj_uuid)) + assert result is not None + assert result.deleted_at is not None + assert result.name == "deleted_target" + + +@pytest.mark.usefixtures("_synthetic_table") +def test_clear_hard_deletes_the_row(app_context: None, session: Session) -> None: + """``clear_soft_deleted_for_import`` calls + ``db.session.delete(existing)`` so the row is permanently removed + (not just re-soft-deleted) and ORM ``after_delete`` listeners fire. + A subsequent insert with the same UUID would not collide. + """ + import uuid as uuid_lib + + obj_uuid = uuid_lib.uuid4() + obj = _ImportableSoftDeletable(name="to_clear", uuid=obj_uuid) + session.add(obj) + session.flush() + obj.soft_delete() + session.flush() + obj_id = obj.id + + clear_soft_deleted_for_import(obj) + + # Row is gone — bypass-aware lookup returns None. + found = find_existing_for_import(_ImportableSoftDeletable, str(obj_uuid)) + assert found is None + # And session.query confirms it's hard-deleted, not just soft. + from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES + + raw = ( + session.query(_ImportableSoftDeletable) + .execution_options( + **{SKIP_VISIBILITY_FILTER_CLASSES: {_ImportableSoftDeletable}} + ) + .filter_by(id=obj_id) + .one_or_none() + ) + assert raw is None + + +@pytest.mark.usefixtures("_synthetic_table") +def test_find_then_clear_is_the_intended_caller_sequence( + app_context: None, session: Session +) -> None: + """The two functions compose as the documented contract: find + first, decide based on the returned row, then clear if needed. + Pinning this sequence ensures a refactor doesn't reverse the + intent (find side-effects, clear pure).""" + import uuid as uuid_lib + + obj_uuid = uuid_lib.uuid4() + obj = _ImportableSoftDeletable(name="reimport_target", uuid=obj_uuid) + session.add(obj) + session.flush() + obj.soft_delete() + session.flush() + + # Step 1: find (pure). Caller now has the row to make decisions on. + existing = find_existing_for_import(_ImportableSoftDeletable, str(obj_uuid)) + assert existing is not None + assert existing.deleted_at is not None + + # Step 2: caller decides to clear (in real code, after overwrite + + # permission checks pass). + clear_soft_deleted_for_import(existing) + + # Step 3: caller can now re-insert with the same UUID without + # colliding. + fresh = _ImportableSoftDeletable(name="fresh", uuid=obj_uuid) + session.add(fresh) + session.flush() + assert fresh.id is not None + assert fresh.deleted_at is None diff --git a/tests/unit_tests/commands/test_base_restore_command.py b/tests/unit_tests/commands/test_base_restore_command.py new file mode 100644 index 000000000000..927cdda5a02a --- /dev/null +++ b/tests/unit_tests/commands/test_base_restore_command.py @@ -0,0 +1,187 @@ +# 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 ``BaseRestoreCommand.validate``. + +Concrete entity restore commands (chart, dashboard, dataset) have their +own integration tests on the entity-rollout PRs. This module exercises +the abstract base class directly so the validation contract is pinned +at the infrastructure level — refactors to ``BaseRestoreCommand`` get +fast local feedback rather than waiting for entity-branch integration +CI. +""" + +from __future__ import annotations + +from datetime import datetime +from typing import ClassVar +from unittest.mock import MagicMock, patch + +import pytest + +from superset.commands.restore import BaseRestoreCommand +from superset.exceptions import SupersetSecurityException +from superset.models.helpers import SoftDeleteMixin + + +class _NotFoundError(Exception): + """Synthetic not-found exception for tests.""" + + +class _ForbiddenError(Exception): + """Synthetic forbidden exception for tests.""" + + +class _RestoreFailedError(Exception): + """Synthetic restore-failed exception used by the transaction wrapper.""" + + +def _make_command( + dao_find_result: object, +) -> BaseRestoreCommand[SoftDeleteMixin]: + """Build a concrete ``BaseRestoreCommand`` subclass whose DAO is a + ``MagicMock`` returning ``dao_find_result`` from ``find_by_id``. + """ + dao_mock = MagicMock() + dao_mock.find_by_id.return_value = dao_find_result + + class _SyntheticRestoreCommand(BaseRestoreCommand[SoftDeleteMixin]): + dao: ClassVar[MagicMock] = dao_mock + not_found_exc: ClassVar[type[Exception]] = _NotFoundError + forbidden_exc: ClassVar[type[Exception]] = _ForbiddenError + restore_failed_exc: ClassVar[type[Exception]] = _RestoreFailedError + + return _SyntheticRestoreCommand("uuid-1") + + +def test_validate_raises_not_found_when_model_missing(app_context: None) -> None: + """If the DAO can't find the row, validate() raises ``not_found_exc``.""" + cmd = _make_command(dao_find_result=None) + + with pytest.raises(_NotFoundError, match="No row with uuid"): + cmd.validate() + + +def test_validate_raises_not_found_when_model_is_live(app_context: None) -> None: + """A live row (``deleted_at is None``) has nothing to restore; + validate() raises ``not_found_exc`` with a message that points at + the "not soft-deleted" case (distinguishable from "no such row").""" + live = MagicMock() + live.deleted_at = None + cmd = _make_command(dao_find_result=live) + + with pytest.raises(_NotFoundError, match="not soft-deleted"): + cmd.validate() + + +def test_validate_returns_model_when_owned_and_soft_deleted( + app_context: None, +) -> None: + """A soft-deleted row owned by the caller passes the ownership check + and is returned to ``run()`` for restoration.""" + soft_deleted = MagicMock() + soft_deleted.deleted_at = datetime(2026, 1, 1) + cmd = _make_command(dao_find_result=soft_deleted) + + with patch("superset.commands.restore.security_manager") as mock_sec: + mock_sec.raise_for_ownership = MagicMock(return_value=None) + result = cmd.validate() + + assert result is soft_deleted + mock_sec.raise_for_ownership.assert_called_once_with(soft_deleted) + + +def test_validate_raises_forbidden_when_ownership_check_fails( + app_context: None, +) -> None: + """The security manager's raise_for_ownership raises + ``SupersetSecurityException`` for non-owners; validate() translates + that to the command's ``forbidden_exc`` (keeping the security-layer + exception type out of caller code).""" + soft_deleted = MagicMock() + soft_deleted.deleted_at = datetime(2026, 1, 1) + cmd = _make_command(dao_find_result=soft_deleted) + + def reject_ownership(_resource: object) -> None: + raise SupersetSecurityException(MagicMock()) + + with patch("superset.commands.restore.security_manager") as mock_sec: + mock_sec.raise_for_ownership = reject_ownership + with pytest.raises(_ForbiddenError): + cmd.validate() + + +def test_validate_calls_dao_with_visibility_bypass_only(app_context: None) -> None: + """The DAO load uses ``skip_visibility_filter=True`` (so the + soft-deleted row is visible) and ``id_column='uuid'`` — but does + NOT bypass the entity's ``base_filter``. Restore should honor RBAC + the same way ``delete`` does (which loads through ``find_by_ids`` + without ``skip_base_filter=True``); the visibility bypass is the + only escape hatch needed for restore.""" + soft_deleted = MagicMock() + soft_deleted.deleted_at = datetime(2026, 1, 1) + cmd = _make_command(dao_find_result=soft_deleted) + + with patch("superset.commands.restore.security_manager") as mock_sec: + mock_sec.raise_for_ownership = MagicMock(return_value=None) + cmd.validate() + + cmd.dao.find_by_id.assert_called_once_with( + "uuid-1", + id_column="uuid", + skip_visibility_filter=True, + ) + + +def test_run_calls_model_restore_on_success(app_context: None) -> None: + """The happy path: ``run()`` resolves the model via ``validate()`` and + calls ``model.restore()`` on it. The transactional wrapper applied + by the base ``run()`` commits the cleared ``deleted_at`` to the DB. + """ + soft_deleted = MagicMock() + soft_deleted.deleted_at = datetime(2026, 1, 1) + cmd = _make_command(dao_find_result=soft_deleted) + + with patch("superset.commands.restore.security_manager") as mock_sec: + mock_sec.raise_for_ownership = MagicMock(return_value=None) + cmd.run() + + soft_deleted.restore.assert_called_once_with() + + +def test_run_translates_sqlalchemy_errors_via_restore_failed_exc( + app_context: None, +) -> None: + """The base ``run()`` wraps the operation in ``@transaction(on_error= + partial(on_error, reraise=self.restore_failed_exc))``. A SQLAlchemy + error raised below (e.g., during commit) gets caught and re-raised + as the subclass's ``restore_failed_exc``. Pinning this prevents a + refactor from accidentally dropping the transaction wrapper or + pointing it at the wrong exception type. + """ + from sqlalchemy.exc import SQLAlchemyError + + soft_deleted = MagicMock() + soft_deleted.deleted_at = datetime(2026, 1, 1) + # model.restore() raises during the transactional block — simulates a + # SQLAlchemy failure inside the unit-of-work. + soft_deleted.restore.side_effect = SQLAlchemyError("simulated commit failure") + cmd = _make_command(dao_find_result=soft_deleted) + + with patch("superset.commands.restore.security_manager") as mock_sec: + mock_sec.raise_for_ownership = MagicMock(return_value=None) + with pytest.raises(_RestoreFailedError): + cmd.run() diff --git a/tests/unit_tests/daos/test_base_dao_soft_delete.py b/tests/unit_tests/daos/test_base_dao_soft_delete.py new file mode 100644 index 000000000000..f0ef17948574 --- /dev/null +++ b/tests/unit_tests/daos/test_base_dao_soft_delete.py @@ -0,0 +1,100 @@ +# 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. +"""Tests for BaseDAO soft_delete / hard_delete / delete routing. + +Uses a synthetic model + DAO so the routing logic is exercised in +isolation. Concrete entity DAOs (ChartDAO, DashboardDAO, DatasetDAO) +acquire ``SoftDeleteMixin`` via their model classes in the entity- +rollout PRs and cover end-to-end behaviour there. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +from sqlalchemy import Column, Integer, String +from sqlalchemy.orm import declarative_base + +from superset.daos.base import BaseDAO +from superset.models.helpers import SoftDeleteMixin + +_TestBase = declarative_base() + + +class _SoftDeletable(SoftDeleteMixin, _TestBase): # type: ignore[misc, valid-type] + __tablename__ = "_soft_deletable_dao_test" + + id = Column(Integer, primary_key=True) + name = Column(String, nullable=False) + + +class _Plain(_TestBase): # type: ignore[misc, valid-type] + """Plain model — does NOT inherit SoftDeleteMixin.""" + + __tablename__ = "_plain_dao_test" + + id = Column(Integer, primary_key=True) + name = Column(String, nullable=False) + + +class _SoftDeletableDAO(BaseDAO[_SoftDeletable]): + model_cls = _SoftDeletable + + +class _PlainDAO(BaseDAO[_Plain]): + model_cls = _Plain + + +def test_delete_routes_to_soft_delete_for_mixin_models(app_context: None) -> None: + """delete() calls soft_delete() when model_cls includes SoftDeleteMixin.""" + items = [MagicMock(), MagicMock()] + + with patch.object(_SoftDeletableDAO, "soft_delete") as mock_soft: + _SoftDeletableDAO.delete(items) + mock_soft.assert_called_once_with(items) + + +def test_delete_routes_to_hard_delete_for_non_mixin_models(app_context: None) -> None: + """delete() calls hard_delete() for non-SoftDeleteMixin models.""" + items = [MagicMock(), MagicMock()] + + with patch.object(_PlainDAO, "hard_delete") as mock_hard: + _PlainDAO.delete(items) + mock_hard.assert_called_once_with(items) + + +@patch("superset.daos.base.db") +def test_hard_delete_calls_session_delete( + mock_db: MagicMock, app_context: None +) -> None: + """hard_delete() calls db.session.delete() on each item.""" + items = [MagicMock(), MagicMock()] + + BaseDAO.hard_delete(items) + + assert mock_db.session.delete.call_count == 2 + mock_db.session.delete.assert_any_call(items[0]) + mock_db.session.delete.assert_any_call(items[1]) + + +def test_soft_delete_calls_item_soft_delete(app_context: None) -> None: + """soft_delete() calls soft_delete() on each item.""" + items = [MagicMock(), MagicMock()] + + BaseDAO.soft_delete(items) + items[0].soft_delete.assert_called_once() + items[1].soft_delete.assert_called_once() diff --git a/tests/unit_tests/models/test_soft_delete_mixin.py b/tests/unit_tests/models/test_soft_delete_mixin.py new file mode 100644 index 000000000000..364d016d9f9a --- /dev/null +++ b/tests/unit_tests/models/test_soft_delete_mixin.py @@ -0,0 +1,566 @@ +# 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. +"""Tests for SoftDeleteMixin and the do_orm_execute visibility filter. + +Synthetic models (``_SoftDeletable`` + ``_SoftDeletableTwo``) rather than +real Superset entities so the infrastructure is exercised in isolation +from any concrete adoption. Two soft-deletable models lets us pin +per-class scoping: a bypass for one class must not unhide soft-deleted +rows of the other. +""" + +from __future__ import annotations + +from collections.abc import Generator +from datetime import datetime + +import pytest +from sqlalchemy import Column, ForeignKey, Integer, String +from sqlalchemy.orm import aliased, declarative_base, relationship +from sqlalchemy.orm.session import Session + +from superset.models.helpers import ( + skip_visibility_filter, + SKIP_VISIBILITY_FILTER_CLASSES, + SoftDeleteMixin, +) + +_TestBase = declarative_base() + + +class _SoftDeletable(SoftDeleteMixin, _TestBase): # type: ignore[misc, valid-type] + """In-memory synthetic model for exercising SoftDeleteMixin.""" + + __tablename__ = "_soft_deletable_test" + + id = Column(Integer, primary_key=True) + name = Column(String, nullable=False) + + +class _SoftDeletableTwo(SoftDeleteMixin, _TestBase): # type: ignore[misc, valid-type] + """A second soft-deletable model so isolation tests can prove a + bypass for one class does not affect the other.""" + + __tablename__ = "_soft_deletable_test_two" + + id = Column(Integer, primary_key=True) + name = Column(String, nullable=False) + + +class _SoftDeletableParent(_TestBase): # type: ignore[misc, valid-type] + """A non-soft-deletable parent so relationship-load tests can verify + that ``with_loader_criteria``'s ``propagate_to_loaders`` carries the + per-class criteria along to lazy/selectin loads.""" + + __tablename__ = "_soft_deletable_parent" + + id = Column(Integer, primary_key=True) + name = Column(String) + children = relationship("_SoftDeletableChild", back_populates="parent") + + +class _SoftDeletableChild(SoftDeleteMixin, _TestBase): # type: ignore[misc, valid-type] + __tablename__ = "_soft_deletable_child" + + id = Column(Integer, primary_key=True) + name = Column(String, nullable=False) + parent_id = Column(Integer, ForeignKey("_soft_deletable_parent.id")) + parent = relationship("_SoftDeletableParent", back_populates="children") + + +@pytest.fixture +def _synthetic_tables(session: Session) -> Generator[None, None, None]: + """Create the synthetic tables for the test session and drop them after.""" + _TestBase.metadata.create_all(session.get_bind()) + yield + _TestBase.metadata.drop_all(session.get_bind()) + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_soft_delete_sets_deleted_at(app_context: None, session: Session) -> None: + """soft_delete() sets deleted_at to a non-null datetime.""" + obj = _SoftDeletable(name="row1") + session.add(obj) + session.flush() + + assert obj.deleted_at is None + assert not obj.is_deleted + + obj.soft_delete() + session.flush() + + assert obj.deleted_at is not None + assert isinstance(obj.deleted_at, datetime) + assert obj.is_deleted + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_restore_clears_deleted_at(app_context: None, session: Session) -> None: + """restore() clears deleted_at back to None.""" + obj = _SoftDeletable(name="row1") + session.add(obj) + session.flush() + + obj.soft_delete() + session.flush() + assert obj.is_deleted + + obj.restore() + session.flush() + assert obj.deleted_at is None + assert not obj.is_deleted + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_where_not_deleted_filter_clause(app_context: None, session: Session) -> None: + """where_not_deleted() returns a SQL WHERE clause usable in queries.""" + active = _SoftDeletable(name="active") + deleted = _SoftDeletable(name="deleted") + session.add_all([active, deleted]) + session.flush() + + deleted.soft_delete() + session.flush() + + results = ( + session.query(_SoftDeletable) + .filter(_SoftDeletable.where_not_deleted()) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {_SoftDeletable}}) + .all() + ) + + assert len(results) == 1 + assert results[0].name == "active" + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_global_filter_excludes_soft_deleted_rows( + app_context: None, session: Session +) -> None: + """The do_orm_execute listener excludes soft-deleted rows by default.""" + obj = _SoftDeletable(name="will_be_deleted") + session.add(obj) + session.flush() + obj_id = obj.id + + obj.soft_delete() + session.flush() + session.expire_all() + + result = ( + session.query(_SoftDeletable).filter(_SoftDeletable.id == obj_id).one_or_none() + ) + assert result is None + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_listener_adapts_criteria_to_aliased_table_in_joins( + app_context: None, session: Session +) -> None: + """When the same soft-deletable table appears under an alias in a + JOIN (e.g., ``slices AS chart``), the listener's loader_criteria + must reference the **alias**, not the raw table name. Passing the + criteria as ``Slice.deleted_at.is_(None)`` (a concrete SQL + expression bound to the base class) renders as + ``slices.deleted_at`` even when the statement aliases + ``slices AS chart`` — producing + ``Unknown column 'slices.deleted_at' in 'on clause'``. The fix is + to pass the criteria as a lambda so SQLAlchemy adapts the column + reference per occurrence. + + This regression test reproduces the FAB shape that surfaced the + bug on the chart-rollout PR: a parent table joined to an aliased + soft-deletable child. The query must run without + ``OperationalError``. + """ + parent_a = _SoftDeletableParent(name="p_with_child") + child = _SoftDeletableChild(name="c1") + parent_a.children = [child] + session.add(parent_a) + session.flush() + session.expunge_all() + + # Alias the child so the JOIN renders as ``_soft_deletable_child AS + # aliased_child`` — same shape as FAB's ``slices AS chart``. + aliased_child = aliased(_SoftDeletableChild) + results = ( + session.query(_SoftDeletableParent, aliased_child) + .outerjoin(aliased_child, _SoftDeletableParent.id == aliased_child.parent_id) + .all() + ) + # Query must execute without OperationalError. The exact result + # shape isn't the point — that the listener-attached criteria + # references the alias rather than the raw table name is. + assert len(results) == 1 + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_listener_does_not_affect_non_soft_deletable_queries( + app_context: None, session: Session +) -> None: + """Queries against a class that does NOT inherit ``SoftDeleteMixin`` + pass through the listener unchanged. The listener still iterates + soft-delete subclasses and attaches a ``with_loader_criteria`` per + class to every primary SELECT, but each is a no-op when the targeted + class isn't in the statement. Pins that the listener does not + silently break unrelated queries. + """ + parent_a = _SoftDeletableParent(name="a") + parent_b = _SoftDeletableParent(name="b") + session.add_all([parent_a, parent_b]) + session.flush() + + rows = session.query(_SoftDeletableParent).order_by(_SoftDeletableParent.id).all() + assert [r.name for r in rows] == ["a", "b"] + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_per_query_class_bypass_returns_soft_deleted_rows( + app_context: None, session: Session +) -> None: + """Per-query bypass set on ``execution_options`` (scoped to a specific + class) makes that class's soft-deleted rows visible. Used by + ``BaseDAO.find_by_id(skip_visibility_filter=True)``, + ``find_existing_for_import``, and ``raise_for_ownership``. + """ + obj = _SoftDeletable(name="soon_deleted") + session.add(obj) + session.flush() + obj_id = obj.id + + obj.soft_delete() + session.flush() + session.expire_all() + + visible = ( + session.query(_SoftDeletable) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {_SoftDeletable}}) + .filter(_SoftDeletable.id == obj_id) + .one_or_none() + ) + assert visible is not None + assert visible.name == "soon_deleted" + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_per_query_bypass_for_one_class_does_not_unhide_other( + app_context: None, session: Session +) -> None: + """A per-query bypass scoped to ``_SoftDeletable`` does not let a + statement that also queries ``_SoftDeletableTwo`` see its soft-deleted + rows. Pins per-class scoping at the listener level: the loader + criteria evaluates the bypass set per concrete subclass, not as a + blanket exemption. + """ + one = _SoftDeletable(name="one") + two = _SoftDeletableTwo(name="two") + session.add_all([one, two]) + session.flush() + one.soft_delete() + two.soft_delete() + session.flush() + session.expire_all() + + # Bypass scoped to _SoftDeletable only. + one_seen = ( + session.query(_SoftDeletable) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {_SoftDeletable}}) + .one_or_none() + ) + assert one_seen is not None + assert one_seen.name == "one" + + # Same execution_options on a query for the OTHER class. _SoftDeletableTwo + # is not in the bypass set, so it stays filtered. + two_seen = ( + session.query(_SoftDeletableTwo) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {_SoftDeletable}}) + .one_or_none() + ) + assert two_seen is None + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_get_without_bypass_filters_out_soft_deleted_row( + app_context: None, session: Session +) -> None: + """Baseline: ``Query.get()`` without bypass does not find soft-deleted + rows. ``session.expunge_all()`` empties the identity map so ``.get()`` + is forced to issue SQL through the listener. + """ + obj = _SoftDeletable(name="hidden_by_listener") + session.add(obj) + session.flush() + obj_id = obj.id + + obj.soft_delete() + session.flush() + session.expunge_all() + + result = session.query(_SoftDeletable).get(obj_id) + assert result is None, ( + ".get() with no bypass and an empty identity map should be filtered " + "by the listener" + ) + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_per_query_bypass_via_get_finds_soft_deleted_row( + app_context: None, session: Session +) -> None: + """Per-query class bypass propagates through ``Query.get()`` — the + path ``security_manager.raise_for_ownership`` relies on. Identity-map + cleared via ``session.expunge_all()`` to force SQL through the + listener. + """ + obj = _SoftDeletable(name="per_query_via_get") + session.add(obj) + session.flush() + obj_id = obj.id + + obj.soft_delete() + session.flush() + session.expunge_all() + + result = ( + session.query(_SoftDeletable) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {_SoftDeletable}}) + .get(obj_id) + ) + + assert result is not None, ( + "per-query class bypass should let .get() find soft-deleted row" + ) + assert result.deleted_at is not None + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_session_bypass_survives_query_reconstruction( + app_context: None, session: Session +) -> None: + """The FAB list-endpoint failure mode: a derived query is built from + a fresh ``session.query(Model)`` (no ``execution_options``) and + joined to a previously-filtered subquery. Per-query + ``execution_options`` would not survive that construction. Per-session + bypass via ``session.info`` does — because the listener reads from + ``execute_state.session.info`` regardless of how the statement was + built. + """ + obj = _SoftDeletable(name="visible_via_session_bypass") + session.add(obj) + session.flush() + obj_id = obj.id + + obj.soft_delete() + session.flush() + session.expire_all() + + # Set up a FAB-style fork: an "inner" query that selects the row's id + # via the bypass, then an outer query that joins to the inner as a + # subquery — but the outer query is built fresh from session.query() + # and has NO execution_options on it. This is exactly the shape that + # SQLAInterface.get_outer_query_from_inner_query produces. + session.info[SKIP_VISIBILITY_FILTER_CLASSES] = {_SoftDeletable} + + inner_subquery = ( + session.query(_SoftDeletable.id).filter(_SoftDeletable.id == obj_id).subquery() + ) + outer = session.query(_SoftDeletable).join( + inner_subquery, _SoftDeletable.id == inner_subquery.c.id + ) + rows = outer.all() + + # Clean up so subsequent tests in the same session see a fresh state. + session.info.pop(SKIP_VISIBILITY_FILTER_CLASSES, None) + + assert len(rows) == 1 + assert rows[0].id == obj_id + assert rows[0].deleted_at is not None + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_session_bypass_does_not_leak_across_classes( + app_context: None, session: Session +) -> None: + """A session-level bypass for ``_SoftDeletable`` only does NOT make + soft-deleted ``_SoftDeletableTwo`` rows visible — even though both + queries run on the same session that has the bypass flag set. The + per-class scoping in the listener's loader-criteria lambda is what + prevents this leak. + """ + one = _SoftDeletable(name="one") + two = _SoftDeletableTwo(name="two") + session.add_all([one, two]) + session.flush() + one.soft_delete() + two.soft_delete() + session.flush() + session.expire_all() + + session.info[SKIP_VISIBILITY_FILTER_CLASSES] = {_SoftDeletable} + try: + one_seen = session.query(_SoftDeletable).one_or_none() + two_seen = session.query(_SoftDeletableTwo).one_or_none() + finally: + session.info.pop(SKIP_VISIBILITY_FILTER_CLASSES, None) + + assert one_seen is not None + assert one_seen.name == "one" + assert two_seen is None, ( + "_SoftDeletableTwo should still be filtered — only _SoftDeletable " + "is in the bypass set" + ) + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_context_manager_adds_and_removes_bypass( + app_context: None, session: Session +) -> None: + """``skip_visibility_filter`` context manager adds classes on entry, + removes them on exit, restoring the prior visibility state. + """ + obj = _SoftDeletable(name="cm_target") + session.add(obj) + session.flush() + obj_id = obj.id + + obj.soft_delete() + session.flush() + session.expire_all() + + # Filtered before the block. + assert ( + session.query(_SoftDeletable).filter(_SoftDeletable.id == obj_id).one_or_none() + is None + ) + + # Visible inside the block. + with skip_visibility_filter(session, _SoftDeletable): + inside = ( + session.query(_SoftDeletable) + .filter(_SoftDeletable.id == obj_id) + .one_or_none() + ) + assert inside is not None + assert inside.id == obj_id + + # Filtered again after the block (state restored). + session.expire_all() + assert ( + session.query(_SoftDeletable).filter(_SoftDeletable.id == obj_id).one_or_none() + is None + ) + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_context_manager_nested_preserves_outer_scope( + app_context: None, session: Session +) -> None: + """Nested ``skip_visibility_filter`` blocks compose correctly: an + inner block only removes the classes it added, so the outer block's + bypass remains in effect after the inner exits. + """ + one = _SoftDeletable(name="outer_target") + two = _SoftDeletableTwo(name="inner_target") + session.add_all([one, two]) + session.flush() + one.soft_delete() + two.soft_delete() + session.flush() + session.expire_all() + + with skip_visibility_filter(session, _SoftDeletable): + # Outer bypass: _SoftDeletable visible, _SoftDeletableTwo still filtered. + assert session.query(_SoftDeletable).one_or_none() is not None + assert session.query(_SoftDeletableTwo).one_or_none() is None + + with skip_visibility_filter(session, _SoftDeletableTwo): + # Both visible inside the inner block. + session.expire_all() + assert session.query(_SoftDeletable).one_or_none() is not None + assert session.query(_SoftDeletableTwo).one_or_none() is not None + + # Inner exited — _SoftDeletableTwo back to filtered, outer still + # in effect. + session.expire_all() + assert session.query(_SoftDeletable).one_or_none() is not None + assert session.query(_SoftDeletableTwo).one_or_none() is None + + # Outermost exited — both filtered. + session.expire_all() + assert session.query(_SoftDeletable).one_or_none() is None + assert session.query(_SoftDeletableTwo).one_or_none() is None + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_relationship_load_filters_child_independently_of_parent_bypass( + app_context: None, session: Session +) -> None: + """``with_loader_criteria(..., propagate_to_loaders=True)`` carries + the criteria *function* along to relationship loads, where it is + re-evaluated per concrete child class. So even if the parent's + statement is processed with a bypass set, a lazy/selectin load of + soft-deletable children whose class is NOT in the bypass set still + gets filtered. + """ + parent = _SoftDeletableParent(name="p") + live_child = _SoftDeletableChild(name="live") + deleted_child = _SoftDeletableChild(name="deleted") + parent.children = [live_child, deleted_child] + session.add(parent) + session.flush() + deleted_child.soft_delete() + session.flush() + session.expunge_all() + + # Parent itself isn't soft-deletable; bypass set has no effect on the + # parent query. But _SoftDeletableChild is soft-deletable, and is NOT + # in the bypass set, so children should be filtered via propagation. + session.info[SKIP_VISIBILITY_FILTER_CLASSES] = {_SoftDeletable} + try: + p = session.query(_SoftDeletableParent).first() + assert p is not None + kids = list(p.children) + finally: + session.info.pop(SKIP_VISIBILITY_FILTER_CLASSES, None) + + assert [c.name for c in kids] == ["live"] + + +@pytest.mark.usefixtures("_synthetic_tables") +def test_session_delete_permanently_removes_row( + app_context: None, session: Session +) -> None: + """session.delete() permanently removes the row (hard delete). The + mixin does not intercept session.delete() — that is handled by + BaseDAO.delete() routing to soft_delete() at the DAO level.""" + obj = _SoftDeletable(name="hard_delete_test") + session.add(obj) + session.flush() + obj_id = obj.id + + session.delete(obj) + session.flush() + session.expire_all() + + result = ( + session.query(_SoftDeletable) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {_SoftDeletable}}) + .filter(_SoftDeletable.id == obj_id) + .one_or_none() + ) + assert result is None diff --git a/tests/unit_tests/views/test_soft_delete_filter.py b/tests/unit_tests/views/test_soft_delete_filter.py new file mode 100644 index 000000000000..b3f1165e4b60 --- /dev/null +++ b/tests/unit_tests/views/test_soft_delete_filter.py @@ -0,0 +1,356 @@ +# 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 ``BaseDeletedStateFilter`` and ``SoftDeleteApiMixin``. + +These tests use a synthetic ``_SoftDeletable`` model + ``MagicMock`` +collaborators rather than real Superset entities, so the filter and +mixin behavior is pinned at the infrastructure level independently of +which entity has adopted ``SoftDeleteMixin``. +""" + +from __future__ import annotations + +from datetime import datetime +from typing import Any, ClassVar +from unittest.mock import MagicMock + +import pytest +from flask import Flask, g +from sqlalchemy import Column, Integer, String +from sqlalchemy.orm import declarative_base +from sqlalchemy.orm.session import Session + +from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES, SoftDeleteMixin +from superset.views.filters import ( + AUGMENT_RESPONSE_WITH_DELETED_AT, + BaseDeletedStateFilter, + DELETED_STATE_ADDED_CLASSES, + SoftDeleteApiMixin, +) + +_TestBase = declarative_base() + + +class _SoftDeletable(SoftDeleteMixin, _TestBase): # type: ignore[misc, valid-type] + __tablename__ = "_soft_deletable_filter_test" + id = Column(Integer, primary_key=True) + name = Column(String, nullable=False) + + +class _ConcreteFilter(BaseDeletedStateFilter): + arg_name = "deleted_state" + model: ClassVar[type[SoftDeleteMixin]] = _SoftDeletable + + +@pytest.fixture +def flask_request_ctx() -> Any: + """Minimal Flask request context so the filter and mixin can read / + write ``g`` without booting the full Superset app.""" + app = Flask(__name__) + with app.test_request_context("/"): + yield + + +@pytest.fixture +def query_with_session() -> MagicMock: + """A MagicMock that quacks like a SQLAlchemy Query with a real + Session whose ``info`` dict the filter can mutate.""" + q = MagicMock() + real_session = Session() + q.session = real_session + return q + + +@pytest.fixture +def concrete_filter() -> _ConcreteFilter: + """Filter wired with a datamodel whose ``.obj`` is the synthetic + soft-deletable. FAB's ``BaseFilter.__init__`` sets + ``self.model = datamodel.obj`` — without setting this on the mock, + ``self.model`` becomes a MagicMock attribute and the filter + misroutes the bypass. + """ + datamodel = MagicMock() + datamodel.obj = _SoftDeletable + return _ConcreteFilter("id", datamodel) + + +def test_filter_value_absent_is_noop( + flask_request_ctx: None, + concrete_filter: _ConcreteFilter, + query_with_session: MagicMock, +) -> None: + """A missing / unrecognised filter value returns the query unchanged + and leaves the session info untouched. No augmentation flag set.""" + result = concrete_filter.apply(query_with_session, None) + assert result is query_with_session + assert SKIP_VISIBILITY_FILTER_CLASSES not in query_with_session.session.info + assert not getattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT, False) + + +def test_filter_value_include_sets_session_bypass_and_flag( + flask_request_ctx: None, + concrete_filter: _ConcreteFilter, + query_with_session: MagicMock, +) -> None: + """``include`` adds the filter's model to ``session.info`` (bypass + set) and signals augmentation. Returns the query unchanged + (no additional WHERE clause — listener does the filtering).""" + result = concrete_filter.apply(query_with_session, "include") + assert ( + _SoftDeletable + in query_with_session.session.info[SKIP_VISIBILITY_FILTER_CLASSES] + ) + assert getattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT) is True + # No .filter() call on the query for ``include`` — the query is + # returned as-is so the listener-bypass alone surfaces both live + # and soft-deleted rows. + assert result is query_with_session + + +def test_filter_value_only_adds_deleted_at_predicate( + flask_request_ctx: None, + concrete_filter: _ConcreteFilter, + query_with_session: MagicMock, +) -> None: + """``only`` sets the bypass + augmentation flag AND adds a + ``deleted_at IS NOT NULL`` filter to restrict the result to + soft-deleted rows.""" + concrete_filter.apply(query_with_session, "only") + assert ( + _SoftDeletable + in query_with_session.session.info[SKIP_VISIBILITY_FILTER_CLASSES] + ) + assert getattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT) is True + query_with_session.filter.assert_called_once() + + +def test_filter_value_case_insensitive( + flask_request_ctx: None, + concrete_filter: _ConcreteFilter, + query_with_session: MagicMock, +) -> None: + """The filter normalises the value (lowercase, stripped) so + ``"INCLUDE"`` and ``" only "`` work.""" + concrete_filter.apply(query_with_session, " INCLUDE ") + assert ( + _SoftDeletable + in query_with_session.session.info[SKIP_VISIBILITY_FILTER_CLASSES] + ) + + +def test_filter_unknown_value_is_noop( + flask_request_ctx: None, + concrete_filter: _ConcreteFilter, + query_with_session: MagicMock, +) -> None: + """An unrecognised value (e.g., ``"all"``) is a no-op — same as + absent.""" + result = concrete_filter.apply(query_with_session, "all") + assert result is query_with_session + assert SKIP_VISIBILITY_FILTER_CLASSES not in query_with_session.session.info + + +# --- SoftDeleteApiMixin --------------------------------------------------- + + +class _StubDatamodel: + """Mimics FAB's ``SQLAInterface`` for the mixin's PK resolution.""" + + obj = _SoftDeletable + + def get_pk_name(self) -> str: + return "id" + + +class _StubBaseApi: + """Stub for the FAB ``ModelRestApi`` base. The mixin's + ``pre_get_list`` calls ``super().pre_get_list(data)``; without a + super-chain that defines the method, the call would raise + ``AttributeError`` and the mixin's own logic wouldn't run. + """ + + def pre_get_list(self, _data: dict[str, Any]) -> None: + pass + + +class _ConcreteApi(SoftDeleteApiMixin, _StubBaseApi): + """Concrete REST API class that doesn't depend on FAB's + ``ModelRestApi`` — sufficient for testing the mixin's + ``pre_get_list`` behaviour in isolation.""" + + datamodel = _StubDatamodel() + + def __init__(self, deleted_at_map: dict[Any, str | None]) -> None: + self._deleted_at_map = deleted_at_map + + def _get_deleted_at_map(self, _ids: list[Any]) -> dict[Any, str | None]: + # Tests provide the map directly so they don't need the DB. + return self._deleted_at_map + + +def test_mixin_pre_get_list_noop_when_flag_unset(flask_request_ctx: None) -> None: + """Without the augmentation flag, ``pre_get_list`` leaves the + response untouched — the typical case for any request that didn't + use ``deleted_state``.""" + api = _ConcreteApi(deleted_at_map={}) + data: dict[str, Any] = {"ids": [1, 2], "result": [{"id": 1}, {"id": 2}]} + api.pre_get_list(data) + assert "deleted_at" not in data["result"][0] + assert "deleted_at" not in data["result"][1] + + +def test_mixin_pre_get_list_injects_deleted_at_when_flag_set( + flask_request_ctx: None, +) -> None: + """With the flag set, each result row gains a ``deleted_at`` field + (None for live rows, ISO timestamp for soft-deleted).""" + setattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT, True) + api = _ConcreteApi(deleted_at_map={1: None, 2: "2026-05-14T12:00:00"}) + data: dict[str, Any] = {"ids": [1, 2], "result": [{"id": 1}, {"id": 2}]} + api.pre_get_list(data) + assert data["result"][0]["deleted_at"] is None + assert data["result"][1]["deleted_at"] == "2026-05-14T12:00:00" + + +def test_mixin_pre_get_list_consumes_flag(flask_request_ctx: None) -> None: + """The augmentation flag is read-and-cleared. A second list + operation within the same request (e.g., a batch endpoint + dispatching multiple list views) won't see the flag set by an + earlier filter.""" + setattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT, True) + api = _ConcreteApi(deleted_at_map={}) + api.pre_get_list({"ids": [], "result": []}) + assert getattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT) is False + + +def test_serialize_deleted_at_handles_none() -> None: + """``_serialize_deleted_at(None)`` returns ``None`` (live row); + a datetime value returns its ISO 8601 form. Pins the contract + that's used by every entity's list response augmentation.""" + serialize = SoftDeleteApiMixin._serialize_deleted_at + assert serialize(None) is None + assert serialize(datetime(2026, 5, 14, 12, 0, 0)) == "2026-05-14T12:00:00" + + +# --- F-002: session bypass cleanup ---------------------------------------- + + +def test_filter_records_added_classes_on_g( + flask_request_ctx: None, + concrete_filter: _ConcreteFilter, + query_with_session: MagicMock, +) -> None: + """The filter records each class it adds to the bypass set on + ``g._deleted_state_added_classes`` so the mixin's release step can + distinguish filter-added entries from those installed by + programmatic callers (the context manager or DAO bypass). + """ + concrete_filter.apply(query_with_session, "include") + added: set[type] = getattr(g, DELETED_STATE_ADDED_CLASSES, set()) + assert added == {_SoftDeletable} + + +def test_mixin_releases_bypass_after_inject( + flask_request_ctx: None, query_with_session: MagicMock +) -> None: + """After the augmentation step in ``pre_get_list``, the classes the + filter added are removed from ``session.info[SKIP_VISIBILITY_FILTER_CLASSES]``. + Code that runs later in the same request (audit hooks, after_request + handlers) sees normal filtered visibility rather than the widened + scope the filter installed for the list query. + """ + # Simulate the filter having installed the bypass. + query_with_session.session.info[SKIP_VISIBILITY_FILTER_CLASSES] = {_SoftDeletable} + setattr(g, DELETED_STATE_ADDED_CLASSES, {_SoftDeletable}) + setattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT, True) + + # Patch db.session to point at our test session so the release + # step finds the same info dict. + import superset.views.filters as filters_module + + original_db = filters_module.db + filters_module.db = MagicMock() + filters_module.db.session = query_with_session.session + try: + api = _ConcreteApi(deleted_at_map={}) + api.pre_get_list({"ids": [], "result": []}) + finally: + filters_module.db = original_db + + bypass = query_with_session.session.info.get(SKIP_VISIBILITY_FILTER_CLASSES, set()) + assert _SoftDeletable not in bypass + # And the per-request tracker is itself cleared so a second list + # operation in the same request starts clean. + assert getattr(g, DELETED_STATE_ADDED_CLASSES) == set() + + +def test_mixin_release_does_not_touch_unrelated_bypass_entries( + flask_request_ctx: None, query_with_session: MagicMock +) -> None: + """If a programmatic caller (e.g., the ``skip_visibility_filter`` + context manager or a DAO call) installed a bypass for a *different* + class earlier in the request, the mixin's release step must leave + that entry alone. Pinning this guards against the filter cleanup + accidentally clobbering bypasses owned by other callers. + """ + + class _OtherSoftDeletable(SoftDeleteMixin): + pass + + # Both classes are in the bypass set, but only ``_SoftDeletable`` + # was added by the filter (the other was installed by some other + # caller). The release must remove ``_SoftDeletable`` only. + query_with_session.session.info[SKIP_VISIBILITY_FILTER_CLASSES] = { + _SoftDeletable, + _OtherSoftDeletable, + } + setattr(g, DELETED_STATE_ADDED_CLASSES, {_SoftDeletable}) + setattr(g, AUGMENT_RESPONSE_WITH_DELETED_AT, True) + + import superset.views.filters as filters_module + + original_db = filters_module.db + filters_module.db = MagicMock() + filters_module.db.session = query_with_session.session + try: + api = _ConcreteApi(deleted_at_map={}) + api.pre_get_list({"ids": [], "result": []}) + finally: + filters_module.db = original_db + + bypass = query_with_session.session.info[SKIP_VISIBILITY_FILTER_CLASSES] + assert _SoftDeletable not in bypass + assert _OtherSoftDeletable in bypass + + +def test_mixin_release_is_noop_when_filter_was_not_invoked( + flask_request_ctx: None, query_with_session: MagicMock +) -> None: + """If ``pre_get_list`` runs without the augmentation flag (e.g., a + request that didn't use ``deleted_state``), the release step has + nothing to do and must not raise or touch unrelated state. + """ + query_with_session.session.info[SKIP_VISIBILITY_FILTER_CLASSES] = {_SoftDeletable} + + api = _ConcreteApi(deleted_at_map={}) + api.pre_get_list({"ids": [], "result": []}) + + # Unrelated entry is left alone. + assert ( + _SoftDeletable + in query_with_session.session.info[SKIP_VISIBILITY_FILTER_CLASSES] + )