feat(charts): soft-delete and restore (sc-106189)

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("/<uuid>/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("/<pk>/cache_screenshot/", methods=("GET",))
     @protect()
     @parse_rison(screenshot_query_schema)

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

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:

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")
 

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"

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

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()

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