diff --git a/UPDATING.md b/UPDATING.md index d0208eb58944..d78b9d01d2d7 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -110,6 +110,20 @@ Both default to empty (no behavior change). They apply to both the `LOCAL_EXTENS The Dynamic Group By chart customization now orders its display values according to the "Sort display control values" toggle: ascending (A–Z), descending (Z–A), or the dataset's source order when the toggle is unset. Previously the dropdown always sorted alphabetically. Existing dashboards where the toggle was never set will show options in source order instead of A–Z; open the customization and enable the toggle to restore alphabetical ordering. +### Soft delete and restore for charts + +`DELETE /api/v1/chart/` no longer hard-deletes the chart (the bulk-delete endpoint behaves the same way). The row is marked with a `deleted_at` timestamp and hidden from all list, detail, and lookup endpoints. Charts in this state are excluded from default queries and from relationship loads (e.g. `dashboard.slices`). + +**New endpoint** — `POST /api/v1/chart//restore` clears `deleted_at` and returns the chart to active state. Requires `can_write on Chart` and ownership of the row (or admin). Soft-deleted charts can also be surfaced in the list endpoint via the new `chart_deleted_state` rison filter: `include` returns both live and soft-deleted rows, `only` returns just the soft-deleted ones. Any other value is ignored. For non-admin users, soft-deleted rows are limited to charts they own — the same audience that can restore them. + +**Permissions migration:** existing role grants of `can_write on Chart` cover the new restore endpoint automatically; no role migration is required. + +**Schema migration:** the migration adds a nullable `deleted_at` column and an index on it (`ix_slices_deleted_at`) to the `slices` table. The column add is instant; the index build runs inline (no `CONCURRENTLY`) and may briefly block writes on the `slices` table (INSERT/UPDATE/DELETE are queued while the index builds; reads are unaffected) on large Postgres deployments. MySQL InnoDB builds the index online (no blocking). + +**Rollback note:** if the application code is rolled back after charts have been soft-deleted, the older code path's visibility filter no longer applies and previously hidden rows become visible to the older code. Pair the rollback with a data decision (restore the rows, hard-delete them, or also downgrade the migration) rather than assuming the old hard-delete semantics still hold. + +**Importer behavior:** importing a chart YAML whose UUID matches an existing **soft-deleted** chart is treated as an implicit restore-with-update. The owner (or an admin) gets the chart back in place — `deleted_at` is cleared and the contents from the upload are applied — preserving the original PK and all out-of-archive references (`dashboard_slices` junctions, `report.chart_id`, tag rows). Non-owners get `ImportFailedError`. Callers without `can_write` get `ImportFailedError` instead of silently receiving the soft-deleted row. + ### Granular Export Controls A new feature flag `GRANULAR_EXPORT_CONTROLS` introduces three fine-grained permissions that replace the legacy `can_csv` permission: diff --git a/superset/charts/api.py b/superset/charts/api.py index 2a37aca374d7..cdedbb7be448 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -35,6 +35,7 @@ ChartAllTextFilter, ChartCertifiedFilter, ChartCreatedByMeFilter, + ChartDeletedStateFilter, ChartFavoriteFilter, ChartFilter, ChartHasCreatedByFilter, @@ -64,12 +65,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 @@ -102,12 +105,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" @@ -124,6 +131,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", @@ -134,7 +142,17 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]: "warm_up_cache", } class_permission_name = "Chart" - method_permission_name = MODEL_API_RW_METHOD_PERMISSION_MAP + # Custom methods (``restore``) need an explicit entry; FAB's @protect() + # decorator falls back to ``can__`` (i.e. + # ``can_restore_Chart``) when the mapping is missing, which standard + # roles don't carry. Mirrors the permission model documented for + # ``DELETE`` / ``bulk_delete``: endpoint-level ``can_write`` plus + # resource-level ``raise_for_ownership``. See themes/api.py for the + # established pattern. + method_permission_name = { + **MODEL_API_RW_METHOD_PERMISSION_MAP, + "restore": "write", + } list_columns = [ "is_managed_externally", @@ -222,6 +240,7 @@ def ensure_thumbnails_enabled(self) -> Optional[Response]: "id": [ ChartFavoriteFilter, ChartCertifiedFilter, + ChartDeletedStateFilter, ChartOwnedCreatedFavoredByMeFilter, ], "slice_name": [ChartAllTextFilter], @@ -468,10 +487,15 @@ def put(self, pk: int) -> Response: log_to_statsd=False, ) def delete(self, pk: int) -> Response: - """Delete a chart. + """Soft-delete a chart. + + Marks the chart as deleted (sets ``deleted_at``) and hides it from + list/detail endpoints and relationship loads; the row is preserved + and recoverable via ``POST /api/v1/chart//restore`` by an + owner or admin. --- delete: - summary: Delete a chart + summary: Delete a chart (soft delete; recoverable via restore) parameters: - in: path schema: @@ -524,10 +548,15 @@ def delete(self, pk: int) -> Response: log_to_statsd=False, ) def bulk_delete(self, **kwargs: Any) -> Response: - """Bulk delete charts. + """Bulk soft-delete charts. + + Marks each chart as deleted (sets ``deleted_at``) and hides it from + list/detail endpoints and relationship loads; rows are preserved + and recoverable via ``POST /api/v1/chart//restore`` by an + owner or admin. --- delete: - summary: Bulk delete charts + summary: Bulk delete charts (soft delete; recoverable via restore) parameters: - in: query name: q @@ -572,6 +601,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..c782de6b9c81 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,40 @@ def apply(self, query: Query, value: Any) -> Query: FavStar.user_id == get_user_id(), ) ) + + +class ChartDeletedStateFilter( # pylint: disable=too-few-public-methods + BaseDeletedStateFilter +): + """Rison filter for the GET list that exposes soft-deleted charts. + + Soft-deleted rows are additionally scoped to the **restore audience**: only + the chart's owners (or admins) may enumerate them. This mirrors + ``RestoreChartCommand``'s ``raise_for_ownership`` check, so a read-access + non-owner (who can see the chart via datasource access) cannot list + soft-deleted charts they could never restore. Live rows are unaffected — + they keep their normal ``ChartFilter`` visibility. The ownership scoping is + part of the cross-entity deleted-state contract: only the restore audience + may enumerate soft-deleted rows (kept consistent with the deleted-state + filters in the dashboard and dataset soft-delete rollouts). + """ + + arg_name = "chart_deleted_state" + model = Slice + + def apply(self, query: Query, value: Any) -> Query: + query = super().apply(query, value) + normalized = str(value).lower().strip() if value is not None else "" + if normalized not in {"include", "only"} or security_manager.is_admin(): + return query + + # Non-admins may only see soft-deleted charts they own. ``any()`` emits + # an EXISTS subquery so it composes with the base access filter without + # producing duplicate rows from a join. + owned = Slice.owners.any(security_manager.user_model.id == get_user_id()) + if normalized == "only": + # ``super().apply`` already restricted to ``deleted_at IS NOT NULL``. + return query.filter(owned) + # ``include``: keep all live rows (normal access) and add only the + # soft-deleted rows this user owns. + return query.filter(or_(Slice.deleted_at.is_(None), owned)) 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..43fef980249e 100644 --- a/superset/commands/chart/exceptions.py +++ b/superset/commands/chart/exceptions.py @@ -123,6 +123,16 @@ class ChartDeleteFailedError(DeleteFailedError): message = _("Charts could not be deleted.") +class ChartRestoreFailedError(UpdateFailedError): + # Restore semantically clears ``deleted_at``; it is an UPDATE, not a new + # row. ``UpdateFailedError`` is the nearest typed middle-tier base in the + # codebase. A dedicated ``RestoreFailedError`` in + # ``superset/commands/exceptions.py`` would be more precise across the + # entity rollouts but lives in already-merged infrastructure (#39977); + # introducing it can be a cross-entity follow-up. + message = _("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..209caa74558f 100644 --- a/superset/commands/chart/importers/v1/utils.py +++ b/superset/commands/chart/importers/v1/utils.py @@ -21,6 +21,7 @@ from superset import db, security_manager from superset.commands.exceptions import ImportFailedError +from superset.commands.importers.v1.utils import find_existing_for_import from superset.migrations.shared.migrate_viz import processors from superset.migrations.shared.migrate_viz.base import MigrateViz from superset.models.slice import Slice @@ -48,21 +49,107 @@ def import_chart( overwrite: bool = False, ignore_permissions: bool = False, ) -> Slice: + """Import a chart from a config dict, handling existing matches. + + Permission model for an existing UUID match: + + +--------------+---------------+---------------------+-----------------+ + | Existing row | overwrite arg | Caller has perms? | Outcome | + +==============+===============+=====================+=================+ + | alive | False | (n/a) | return existing | + +--------------+---------------+---------------------+-----------------+ + | alive | True | can_write + owner | UPDATE in place | + +--------------+---------------+---------------------+-----------------+ + | alive | True | can_write, | raise | + | | | not owner/admin | | + +--------------+---------------+---------------------+-----------------+ + | soft-deleted | False or True | can_write + owner | restore + UPDATE| + +--------------+---------------+---------------------+-----------------+ + | soft-deleted | False or True | can_write, | raise | + | | | not owner/admin | | + +--------------+---------------+---------------------+-----------------+ + | soft-deleted | False or True | not can_write | raise (Case B) | + +--------------+---------------+---------------------+-----------------+ + + Re-importing a soft-deleted UUID is implicitly a restore-with-update: + the user is bringing the chart back by uploading it again. We apply + the same ownership check as the explicit overwrite path so non-owners + cannot resurrect via re-import, and we raise rather than silently + returning a soft-deleted row to callers without write permission + (which would let them reattach dashboards to a deleted chart). + """ can_write = ignore_permissions or security_manager.can_access("can_write", "Chart") - existing = db.session.query(Slice).filter_by(uuid=config["uuid"]).first() + # `user` is None for background / example-loader paths (no Flask request + # user). Combined with ``can_write=True`` (typically from + # ``ignore_permissions=True``), the ownership checks in the restore / + # overwrite branches below are intentionally skipped because the caller has + # already established trust at the command level. user = get_user() - if existing: - 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() + + if existing := find_existing_for_import(Slice, config["uuid"]): + if existing.deleted_at is not None: + # RESTORE path — re-importing a soft-deleted UUID is an implicit + # restore-with-update, a distinct operation from overwriting an + # alive row, so it is handled in its own branch. + if not can_write: + # Case B: don't silently return a soft-deleted row to a caller + # without write permission — that would let the dashboard + # importer reattach to a deleted chart and produce a broken + # dashboard. + # Name the chart: a dashboard bundle imports many charts, and + # without the identity the operator can't tell which of N + # charts in the bundle hit the soft-deleted match. + raise ImportFailedError( + f"Chart {existing.slice_name!r} (uuid {config['uuid']}) " + f"was deleted and re-import requires can_write " + f"permission to restore it" + ) + # ``user`` is None on background / example-loader paths; combined + # with ``can_write`` (typically from ``ignore_permissions=True``) + # the ownership check is intentionally skipped because the caller + # already established trust. + if user and ( + not security_manager.can_access_chart(existing) + or (user not in existing.owners and not security_manager.is_admin()) + ): + raise ImportFailedError( + f"Chart {existing.slice_name!r} (uuid {config['uuid']}) " + f"already exists and user doesn't have permissions to " + f"restore it" + ) + # Restore in place (clear ``deleted_at``) rather than + # hard-delete-and-replace: a hard delete would cascade to + # dashboard_slices and other FK references, breaking the dashboards + # that previously embedded this chart. + # + # How the restore lands as an UPDATE: clearing + # ``existing.deleted_at`` marks the in-session row dirty and the + # explicit flush emits the ``deleted_at = NULL`` UPDATE before + # ``Slice.import_from_dict`` (below) does its own query-by-uuid + # lookup. Without the flush we would rely on autoflush ahead of that + # internal query — correct under default session config but a hidden + # contract; the explicit flush makes it robust. The lookup then + # finds the now-live row (the listener filters ``deleted_at IS + # NULL``) and ``import_from_dict`` applies the config as field + # updates on the existing object, preserving the PK. + existing.restore() + db.session.flush() + config["id"] = existing.id + else: + # OVERWRITE path — existing alive row. Without ``overwrite`` or + # write permission, return it unchanged (the pre-soft-delete + # overwrite-without-permission behaviour). + if not overwrite or not can_write: + return existing + if user and ( + not security_manager.can_access_chart(existing) + or (user not in existing.owners and not security_manager.is_admin()) ): raise ImportFailedError( - "A chart already exists and user doesn't " - "have permissions to overwrite it" + "A chart already exists and user doesn't have " + "permissions to overwrite it" ) - if not overwrite or not can_write: - return existing - config["id"] = existing.id + config["id"] = existing.id elif not can_write: raise ImportFailedError( "Chart doesn't exist and user doesn't have permission to create charts" @@ -80,7 +167,7 @@ def import_chart( if chart.id is None: db.session.flush() - if (user := get_user()) and user not in chart.owners: + if user and user not in chart.owners: chart.owners.append(user) return chart diff --git a/superset/commands/chart/restore.py b/superset/commands/chart/restore.py new file mode 100644 index 000000000000..a0979ba2a275 --- /dev/null +++ b/superset/commands/chart/restore.py @@ -0,0 +1,39 @@ +# 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.""" + +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 + + +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/daos/dashboard.py b/superset/daos/dashboard.py index b4fcba6888b5..fe36f96c399e 100644 --- a/superset/daos/dashboard.py +++ b/superset/daos/dashboard.py @@ -33,6 +33,7 @@ DashboardNotFoundError, DashboardUpdateFailedError, ) +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES from superset.daos.base import BaseDAO, ColumnOperator, ColumnOperatorEnum from superset.dashboards.filters import DashboardAccessFilter, is_uuid from superset.exceptions import SupersetSecurityException @@ -276,8 +277,19 @@ def set_dash_metadata( if isinstance(value, dict) ] + # Bypass the soft-delete visibility filter when resolving the + # incoming chart ids: a dashboard's ``position_json`` may still + # reference a chart that is currently soft-deleted, and this + # assignment REBUILDS ``dashboard.slices`` wholesale. With the + # filter active, the hidden member would be silently dropped — + # deleting its ``dashboard_slices`` junction row (breaking the + # documented restore-reattach contract) and writing + # ``uuid: None`` into its position slot via ``uuid_map`` below. current_slices = ( - db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all() + db.session.query(Slice) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}}) + .filter(Slice.id.in_(slice_ids)) + .all() ) dashboard.slices = current_slices 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..165e4f87f067 --- /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 column and index to slices for soft-delete. + +Adds a nullable ``deleted_at`` column and an index on it to the +``slices`` table to support soft deletion of charts. Companion to +the ``SoftDeleteMixin`` infrastructure shipped in PR #39977. + +Revision ID: 7c4a8d09ca37 +Revises: 31dae2559c05 +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 = "31dae2559c05" + +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/slice.py b/superset/models/slice.py index e10b373d945c..9cdad007133a 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/translations/ar/LC_MESSAGES/messages.po b/superset/translations/ar/LC_MESSAGES/messages.po index ceafac0a093d..9d0a71dd6045 100644 --- a/superset/translations/ar/LC_MESSAGES/messages.po +++ b/superset/translations/ar/LC_MESSAGES/messages.po @@ -3103,6 +3103,9 @@ msgstr "تغييرات المخطط" msgid "Chart could not be created." msgstr "تعذر إنشاء مخطط." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "تعذر تحديث المخطط." diff --git a/superset/translations/ca/LC_MESSAGES/messages.po b/superset/translations/ca/LC_MESSAGES/messages.po index fa697990f3f4..7708536e1d7f 100644 --- a/superset/translations/ca/LC_MESSAGES/messages.po +++ b/superset/translations/ca/LC_MESSAGES/messages.po @@ -3121,6 +3121,9 @@ msgstr "Canvis del gràfic" msgid "Chart could not be created." msgstr "El gràfic no s'ha pogut crear." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "El gràfic no s'ha pogut actualitzar." diff --git a/superset/translations/cs/LC_MESSAGES/messages.po b/superset/translations/cs/LC_MESSAGES/messages.po index 15d1c2efb0ea..af7eae922271 100644 --- a/superset/translations/cs/LC_MESSAGES/messages.po +++ b/superset/translations/cs/LC_MESSAGES/messages.po @@ -3106,6 +3106,9 @@ msgstr "Změny grafu" msgid "Chart could not be created." msgstr "Graf se nepodařilo vytvořit." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Graf se nepodařilo aktualizovat." diff --git a/superset/translations/de/LC_MESSAGES/messages.po b/superset/translations/de/LC_MESSAGES/messages.po index 95668a732dc0..0037e17d4726 100644 --- a/superset/translations/de/LC_MESSAGES/messages.po +++ b/superset/translations/de/LC_MESSAGES/messages.po @@ -3149,6 +3149,9 @@ msgstr "Diagrammänderungen" msgid "Chart could not be created." msgstr "Diagramm konnte nicht erstellt werden." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Diagramm konnte nicht aktualisiert werden." diff --git a/superset/translations/en/LC_MESSAGES/messages.po b/superset/translations/en/LC_MESSAGES/messages.po index d65afc6ef840..4bc66411fd72 100644 --- a/superset/translations/en/LC_MESSAGES/messages.po +++ b/superset/translations/en/LC_MESSAGES/messages.po @@ -2791,6 +2791,9 @@ msgstr "" msgid "Chart could not be created." msgstr "" +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "" diff --git a/superset/translations/es/LC_MESSAGES/messages.po b/superset/translations/es/LC_MESSAGES/messages.po index e15a23885532..79c399bc5b28 100644 --- a/superset/translations/es/LC_MESSAGES/messages.po +++ b/superset/translations/es/LC_MESSAGES/messages.po @@ -3215,6 +3215,9 @@ msgstr "Cambios en el gráfico" msgid "Chart could not be created." msgstr "No se ha podido crear el gráfico." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "No se ha podido actualizar el gráfico." diff --git a/superset/translations/fa/LC_MESSAGES/messages.po b/superset/translations/fa/LC_MESSAGES/messages.po index d8bb40161401..c2478494ef11 100644 --- a/superset/translations/fa/LC_MESSAGES/messages.po +++ b/superset/translations/fa/LC_MESSAGES/messages.po @@ -3065,6 +3065,9 @@ msgstr "تغییرات نمودار" msgid "Chart could not be created." msgstr "نمودار نتوانسته ایجاد شود." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "نمودار نتوانست به‌روزرسانی شود." diff --git a/superset/translations/fi/LC_MESSAGES/messages.po b/superset/translations/fi/LC_MESSAGES/messages.po index afbdd4ca0048..987cb9854552 100644 --- a/superset/translations/fi/LC_MESSAGES/messages.po +++ b/superset/translations/fi/LC_MESSAGES/messages.po @@ -5342,6 +5342,9 @@ msgstr "Kaaviota ei voitu luoda." # de, es, fa, fr, it, ja, ko, lv, mi, nl, pl, pt, pt_BR, ru, sk, sl, tr, uk, # zh, zh_TW] #, fuzzy +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Kaaviota ei voitu päivittää." diff --git a/superset/translations/fr/LC_MESSAGES/messages.po b/superset/translations/fr/LC_MESSAGES/messages.po index fdad5b822329..2a83a86a2b8b 100644 --- a/superset/translations/fr/LC_MESSAGES/messages.po +++ b/superset/translations/fr/LC_MESSAGES/messages.po @@ -3117,6 +3117,9 @@ msgstr "Changements de graphique" msgid "Chart could not be created." msgstr "Le graphique n'a pas pu être créé." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Le graphique n'a pas pu être mis à jour." diff --git a/superset/translations/it/LC_MESSAGES/messages.po b/superset/translations/it/LC_MESSAGES/messages.po index 2b7d33087fa4..a38ebb320f85 100644 --- a/superset/translations/it/LC_MESSAGES/messages.po +++ b/superset/translations/it/LC_MESSAGES/messages.po @@ -2999,6 +2999,9 @@ msgstr "Ultima Modifica" msgid "Chart could not be created." msgstr "La tua query non può essere salvata" +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "La tua query non può essere salvata" diff --git a/superset/translations/ja/LC_MESSAGES/messages.po b/superset/translations/ja/LC_MESSAGES/messages.po index e6cba1bff4bc..6c94bf422313 100644 --- a/superset/translations/ja/LC_MESSAGES/messages.po +++ b/superset/translations/ja/LC_MESSAGES/messages.po @@ -2866,6 +2866,9 @@ msgstr "チャートの変更" msgid "Chart could not be created." msgstr "チャートを作成できませんでした。" +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "チャートを更新できませんでした。" diff --git a/superset/translations/ko/LC_MESSAGES/messages.po b/superset/translations/ko/LC_MESSAGES/messages.po index 82c74d193753..696973fc5862 100644 --- a/superset/translations/ko/LC_MESSAGES/messages.po +++ b/superset/translations/ko/LC_MESSAGES/messages.po @@ -2982,6 +2982,9 @@ msgstr "" msgid "Chart could not be created." msgstr "차트를 생성할 수 없습니다." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "차트를 업데이트할 수 없습니다." diff --git a/superset/translations/lv/LC_MESSAGES/messages.po b/superset/translations/lv/LC_MESSAGES/messages.po index 6594cc85eb47..5df46eec7a27 100644 --- a/superset/translations/lv/LC_MESSAGES/messages.po +++ b/superset/translations/lv/LC_MESSAGES/messages.po @@ -3072,6 +3072,9 @@ msgstr "Diagrammas izmaiņas" msgid "Chart could not be created." msgstr "Diagrammu nevarēja izveidot." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Diagrammu nevarēja atjaunināt." diff --git a/superset/translations/messages.pot b/superset/translations/messages.pot index f60a45bdae52..d1986c86bab0 100644 --- a/superset/translations/messages.pot +++ b/superset/translations/messages.pot @@ -2795,6 +2795,9 @@ msgstr "" msgid "Chart could not be created." msgstr "" +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "" diff --git a/superset/translations/mi/LC_MESSAGES/messages.po b/superset/translations/mi/LC_MESSAGES/messages.po index 87a9601b9310..47b1e6cf6cbe 100644 --- a/superset/translations/mi/LC_MESSAGES/messages.po +++ b/superset/translations/mi/LC_MESSAGES/messages.po @@ -3063,6 +3063,9 @@ msgstr "Huringa kauwhata" msgid "Chart could not be created." msgstr "Kāore i taea te hanga i te kauwhata." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Kāore i taea te whakahou i te kauwhata." diff --git a/superset/translations/nl/LC_MESSAGES/messages.po b/superset/translations/nl/LC_MESSAGES/messages.po index c2b74854f274..8b2acc87dd23 100644 --- a/superset/translations/nl/LC_MESSAGES/messages.po +++ b/superset/translations/nl/LC_MESSAGES/messages.po @@ -3118,6 +3118,9 @@ msgstr "Veranderingen in de grafiek" msgid "Chart could not be created." msgstr "Grafiek kon niet worden aangemaakt." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "De grafiek kon niet worden bijgewerkt." diff --git a/superset/translations/pl/LC_MESSAGES/messages.po b/superset/translations/pl/LC_MESSAGES/messages.po index 5913560dee2b..fe0326dd262b 100644 --- a/superset/translations/pl/LC_MESSAGES/messages.po +++ b/superset/translations/pl/LC_MESSAGES/messages.po @@ -3155,6 +3155,9 @@ msgstr "Zmiany wykresu" msgid "Chart could not be created." msgstr "Nie można utworzyć wykresu." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Nie można zaktualizować wykresu." diff --git a/superset/translations/pt/LC_MESSAGES/messages.po b/superset/translations/pt/LC_MESSAGES/messages.po index 6cba7ea17f7b..2e7b631aa638 100644 --- a/superset/translations/pt/LC_MESSAGES/messages.po +++ b/superset/translations/pt/LC_MESSAGES/messages.po @@ -3045,6 +3045,9 @@ msgstr "Modificado pela última vez" msgid "Chart could not be created." msgstr "Não foi possível gravar a sua query" +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Não foi possível gravar a sua query" diff --git a/superset/translations/pt_BR/LC_MESSAGES/messages.po b/superset/translations/pt_BR/LC_MESSAGES/messages.po index c08310bf0557..b2a8fa922868 100644 --- a/superset/translations/pt_BR/LC_MESSAGES/messages.po +++ b/superset/translations/pt_BR/LC_MESSAGES/messages.po @@ -3194,6 +3194,9 @@ msgstr "Alterações no gráfico" msgid "Chart could not be created." msgstr "Não foi possível criar o gráfico." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Não foi possível atualizar o gráfico." diff --git a/superset/translations/ru/LC_MESSAGES/messages.po b/superset/translations/ru/LC_MESSAGES/messages.po index 4a031d56339d..9ff5acf2e836 100644 --- a/superset/translations/ru/LC_MESSAGES/messages.po +++ b/superset/translations/ru/LC_MESSAGES/messages.po @@ -3060,6 +3060,9 @@ msgstr "Изменения диаграммы" msgid "Chart could not be created." msgstr "Не удалось создать диаграмму" +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Не удалось обновить диаграмму" diff --git a/superset/translations/sk/LC_MESSAGES/messages.po b/superset/translations/sk/LC_MESSAGES/messages.po index f51331b19bc5..d554ae12e7fb 100644 --- a/superset/translations/sk/LC_MESSAGES/messages.po +++ b/superset/translations/sk/LC_MESSAGES/messages.po @@ -3104,6 +3104,9 @@ msgstr "Zmeny grafu" msgid "Chart could not be created." msgstr "Graf sa nepodarilo vytvoriť." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Graf sa nepodarilo aktualizovať." diff --git a/superset/translations/sl/LC_MESSAGES/messages.po b/superset/translations/sl/LC_MESSAGES/messages.po index ec55264802b6..5815f7e37211 100644 --- a/superset/translations/sl/LC_MESSAGES/messages.po +++ b/superset/translations/sl/LC_MESSAGES/messages.po @@ -3123,6 +3123,9 @@ msgstr "Spremembe grafikona" msgid "Chart could not be created." msgstr "Grafikona ni mogoče ustvariti." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Grafikona ni mogoče posodobiti." diff --git a/superset/translations/th/LC_MESSAGES/messages.po b/superset/translations/th/LC_MESSAGES/messages.po index 0256e60dbb60..d3dac448d685 100644 --- a/superset/translations/th/LC_MESSAGES/messages.po +++ b/superset/translations/th/LC_MESSAGES/messages.po @@ -5236,6 +5236,9 @@ msgstr "ไม่สามารถสร้างแผนภูมิได้ # de, es, fa, fr, it, ja, ko, lv, mi, nl, pl, pt, pt_BR, ru, sk, sl, tr, uk, # zh, zh_TW] #, fuzzy +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "ไม่สามารถอัปเดตแผนภูมิได้" diff --git a/superset/translations/tr/LC_MESSAGES/messages.po b/superset/translations/tr/LC_MESSAGES/messages.po index 86b48b3846bf..9f0d08f87d93 100644 --- a/superset/translations/tr/LC_MESSAGES/messages.po +++ b/superset/translations/tr/LC_MESSAGES/messages.po @@ -2874,6 +2874,9 @@ msgstr "" msgid "Chart could not be created." msgstr "Grafik oluşturulamadı." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Grafik gencellenemedi." diff --git a/superset/translations/uk/LC_MESSAGES/messages.po b/superset/translations/uk/LC_MESSAGES/messages.po index c18720953ee0..2000c96634cf 100644 --- a/superset/translations/uk/LC_MESSAGES/messages.po +++ b/superset/translations/uk/LC_MESSAGES/messages.po @@ -3052,6 +3052,9 @@ msgstr "Зміни діаграми" msgid "Chart could not be created." msgstr "Не вдалося створити діаграму." +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "Не вдалося оновити діаграму." diff --git a/superset/translations/zh/LC_MESSAGES/messages.po b/superset/translations/zh/LC_MESSAGES/messages.po index 920893918e20..4786081be82d 100644 --- a/superset/translations/zh/LC_MESSAGES/messages.po +++ b/superset/translations/zh/LC_MESSAGES/messages.po @@ -3028,6 +3028,9 @@ msgstr "图表变化" msgid "Chart could not be created." msgstr "您的图表无法创建。" +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "您的图表无法更新。" diff --git a/superset/translations/zh_TW/LC_MESSAGES/messages.po b/superset/translations/zh_TW/LC_MESSAGES/messages.po index ac9b7e54b54b..064fc1f0234a 100644 --- a/superset/translations/zh_TW/LC_MESSAGES/messages.po +++ b/superset/translations/zh_TW/LC_MESSAGES/messages.po @@ -3030,6 +3030,9 @@ msgstr "圖表變化" msgid "Chart could not be created." msgstr "您的圖表無法創建。" +msgid "Chart could not be restored." +msgstr "" + msgid "Chart could not be updated." msgstr "您的圖表無法更新。" diff --git a/tests/integration_tests/charts/commands_tests.py b/tests/integration_tests/charts/commands_tests.py index fecfa64f9d23..fa9fc72ecc4e 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..8b57fcdb176e --- /dev/null +++ b/tests/integration_tests/charts/soft_delete_tests.py @@ -0,0 +1,560 @@ +# 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.""" + +from datetime import datetime + +from superset import security_manager +from superset.connectors.sqla.models import SqlaTable +from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES +from superset.extensions import db +from superset.models.dashboard import Dashboard, dashboard_slices +from superset.models.slice import Slice +from superset.reports.models import ( + ReportCreationMethod, + ReportSchedule, + ReportScheduleType, +) +from superset.utils import json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ( + ADMIN_USERNAME, + ALPHA_USERNAME, + GAMMA_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) -> None: + """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) -> None: + """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) -> None: + """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) -> None: + """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) -> None: + """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_deleted_state_list_shows_owner_their_own_deleted(self) -> None: + """A non-admin owner can still enumerate their own soft-deleted charts. + Deleted-state scoping mirrors the restore audience, so it must not lock + owners out of their own trash.""" + alpha_id = self.get_user(ALPHA_USERNAME).id + chart = self.insert_chart("sd_owner_chart", [alpha_id], 1) + chart_id = chart.id + + chart.deleted_at = datetime(2026, 1, 1, 12, 0, 0) + db.session.commit() + + self.login(ALPHA_USERNAME) + rison_query = ( + "(filters:!((col:id,opr:chart_deleted_state,value:only)),page_size:200)" + ) + rv = self.client.get(f"/api/v1/chart/?q={rison_query}") + assert rv.status_code == 200 + ids = [c["id"] for c in json.loads(rv.data)["result"]] + assert chart_id in ids + + # Cleanup + _hard_delete_chart(chart_id) + + def test_deleted_state_list_hides_non_owned_from_read_access_user(self) -> None: + """A read-access non-owner must not enumerate a chart once it is + soft-deleted. + + Gamma is granted ``datasource_access`` to the chart's dataset, so + ``ChartFilter`` makes the chart visible to gamma while it is live. After + soft-delete, the deleted-state list is scoped to the restore audience + (owners/admins), so gamma — who could never restore it — must not see it + via ``include`` or ``only``. + """ + admin_id = self.get_user(ADMIN_USERNAME).id + chart = self.insert_chart("sd_acl_chart", [admin_id], 1) + chart_id = chart.id + + table = db.session.query(SqlaTable).get(1) + gamma_role = security_manager.find_role("Gamma") + pvm = security_manager.add_permission_view_menu("datasource_access", table.perm) + gamma_role.permissions.append(pvm) + db.session.commit() + + try: + # Precondition: gamma can see the chart while it is live. + self.login(GAMMA_USERNAME) + rv = self.client.get("/api/v1/chart/?q=(page_size:200)") + assert chart_id in [c["id"] for c in json.loads(rv.data)["result"]], ( + "precondition: gamma should see the live chart via datasource access" + ) + + # Soft-delete directly (avoids a mid-test re-login to admin). + reloaded = ( + db.session.query(Slice) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {Slice}}) + .filter(Slice.id == chart_id) + .one() + ) + reloaded.deleted_at = datetime(2026, 1, 1, 12, 0, 0) + db.session.commit() + + # Gamma must not see the soft-deleted chart in either mode. + for value in ("include", "only"): + rison_query = ( + f"(filters:!((col:id,opr:chart_deleted_state,value:{value}))," + "page_size:200)" + ) + rv = self.client.get(f"/api/v1/chart/?q={rison_query}") + assert rv.status_code == 200 + ids = [c["id"] for c in json.loads(rv.data)["result"]] + assert chart_id not in ids, ( + "read-access non-owner must not enumerate a soft-deleted " + f"chart via chart_deleted_state={value}" + ) + finally: + pvm = security_manager.find_permission_view_menu( + "datasource_access", table.perm + ) + if pvm: + security_manager.del_permission_role(gamma_role, pvm) + db.session.commit() + _hard_delete_chart(chart_id) + + def test_delete_already_soft_deleted_chart_returns_404(self) -> None: + """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) -> None: + """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. + """ + 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) -> None: + """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_failure_returns_422(self) -> None: + """A failure during restore surfaces as a clean 422 via the + ``ChartRestoreFailedError`` handler rather than an unhandled 500. + + ``RestoreChartCommand.run`` wraps the restore in ``@transaction`` + and rethrows ``ChartRestoreFailedError`` on any underlying + SQLAlchemy error; this pins that the endpoint maps it to 422. + """ + from unittest.mock import patch + + from superset.commands.chart.exceptions import ( + ChartRestoreFailedError, + ) + + admin_id = self.get_user("admin").id + chart = self.insert_chart("restore_fail_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}") + + with patch( + "superset.commands.chart.restore.RestoreChartCommand.run", + side_effect=ChartRestoreFailedError(), + ): + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 422 + + # Cleanup + _hard_delete_chart(chart_id) + + def test_restore_nonexistent_chart_returns_404(self) -> None: + """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) -> None: + """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_uses_can_write_permission(self) -> None: + """Non-admin owner with ``can_write_Chart`` can hit the restore + endpoint. + + Pins the permission contract: ``method_permission_name`` must map + ``restore`` to ``write`` so FAB's ``@protect`` resolves the gate to + ``can_write_Chart`` (which Alpha already carries), not the implicit + fallback ``can_restore_Chart`` (which no standard role carries). + + Without the mapping FAB defaults to ``can__`` and + every non-admin would get 403 here — admins bypass FAB permission + checks entirely, so the admin-authed restore tests above don't + exercise the mapping. + """ + alpha = self.get_user(ALPHA_USERNAME) + chart = self.insert_chart("restore_perm_test", [alpha.id], 1) + chart_id = chart.id + chart_uuid = str(chart.uuid) + + self.login(ALPHA_USERNAME) + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200, ( + f"Alpha owner soft-delete failed: {rv.status_code} {rv.data!r}" + ) + + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 200, ( + f"Expected 200 from Alpha owner restore (can_write_Chart), got " + f"{rv.status_code}: {rv.data!r}. If 403, " + "method_permission_name is missing 'restore': 'write'." + ) + + # Cleanup + _hard_delete_chart(chart_id) + + def test_restore_chart_reattaches_to_dashboards(self) -> None: + """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 + """ + 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) + + def test_restore_chart_by_non_admin_owner(self) -> None: + """Non-admin owners can restore their own soft-deleted charts. + + The unit-level restore command tests mock security; this + integration test exercises the FAB security wiring end-to-end + so a future change that breaks the owner check on a non-admin + path can't slip through. + """ + alpha = self.get_user(ALPHA_USERNAME) + alpha_id = alpha.id + + chart = self.insert_chart("alpha_owned_chart", [alpha_id], 1) + chart_id = chart.id + chart_uuid = str(chart.uuid) + + self.login(ALPHA_USERNAME) + rv = self.client.delete(f"/api/v1/chart/{chart_id}") + assert rv.status_code == 200 + + rv = self.client.post(f"/api/v1/chart/{chart_uuid}/restore") + assert rv.status_code == 200, rv.data + + db.session.expire_all() + restored = db.session.query(Slice).filter(Slice.id == chart_id).one_or_none() + assert restored is not None + assert restored.deleted_at is None + + # Cleanup + _hard_delete_chart(chart_id) diff --git a/tests/unit_tests/charts/commands/importers/v1/import_test.py b/tests/unit_tests/charts/commands/importers/v1/import_test.py index 42a4afb779cf..31094115dee9 100644 --- a/tests/unit_tests/charts/commands/importers/v1/import_test.py +++ b/tests/unit_tests/charts/commands/importers/v1/import_test.py @@ -18,6 +18,7 @@ import copy from collections.abc import Generator +from datetime import datetime, timezone from unittest.mock import patch import pytest @@ -287,6 +288,186 @@ def test_import_existing_chart_with_permission( mock_can_access_chart.assert_called_once_with(slice) +def _soft_delete_existing_chart(session: Session) -> int: + """Soft-delete the seeded chart (by fixture UUID) and return its original id. + + Shared setup for the soft-delete import tests: locate the chart, stamp + ``deleted_at``, flush, and return the id so callers can assert the restore + happened in place (same id). + """ + existing = ( + session.query(Slice).filter(Slice.uuid == chart_config["uuid"]).one_or_none() + ) + assert existing is not None + existing.deleted_at = datetime(2026, 1, 1, 12, 0, 0, tzinfo=timezone.utc) + session.flush() + return existing.id + + +def test_import_soft_deleted_chart_overwrite_restores_in_place( + mocker: MockerFixture, + session_with_data: Session, +) -> None: + """ + Overwrite-importing a soft-deleted chart must restore the row in place, + not hard-delete-and-replace. Otherwise out-of-archive references + (dashboard_slices junctions, report.chart_id) would cascade away. + """ + mocker.patch.object(security_manager, "can_access", return_value=True) + mocker.patch.object(security_manager, "can_access_chart", return_value=True) + + original_id = _soft_delete_existing_chart(session_with_data) + + admin = User( + first_name="Alice", + last_name="Doe", + email="adoe@example.org", + username="admin", + roles=[Role(name="Admin")], + ) + + config = copy.deepcopy(chart_config) + config["datasource_id"] = 1 + config["datasource_type"] = "table" + + with override_user(admin): + chart = import_chart(config, overwrite=True) + + assert chart.id == original_id + assert chart.deleted_at is None + + +def test_import_soft_deleted_chart_ignore_permissions_restores_in_place( + mocker: MockerFixture, + session_with_data: Session, +) -> None: + """ + The example loader path: ignore_permissions=True with no logged-in + user. The if/elif structure must preserve config["id"] on the + fallthrough overwrite path so the example loader can re-import over + a soft-deleted match without colliding on the UUID unique index. + """ + original_id = _soft_delete_existing_chart(session_with_data) + + config = copy.deepcopy(chart_config) + config["datasource_id"] = 1 + config["datasource_type"] = "table" + + chart = import_chart(config, overwrite=True, ignore_permissions=True) + + assert chart.id == original_id + assert chart.deleted_at is None + + +def test_import_soft_deleted_chart_non_overwrite_restores_for_owner( + mocker: MockerFixture, + session_with_data: Session, +) -> None: + """ + Non-overwrite re-import of a soft-deleted UUID is implicitly a + restore-and-update: the user is bringing the chart back by uploading + it again. The same ownership rule as the overwrite path applies, so + an owner (or admin) succeeds without setting overwrite=True. + """ + mocker.patch.object(security_manager, "can_access", return_value=True) + mocker.patch.object(security_manager, "can_access_chart", return_value=True) + + original_id = _soft_delete_existing_chart(session_with_data) + + admin = User( + first_name="Alice", + last_name="Doe", + email="adoe@example.org", + username="admin", + roles=[Role(name="Admin")], + ) + + config = copy.deepcopy(chart_config) + config["datasource_id"] = 1 + config["datasource_type"] = "table" + + with override_user(admin): + chart = import_chart(config, overwrite=False) + + assert chart.id == original_id + assert chart.deleted_at is None + + +def test_import_soft_deleted_chart_non_overwrite_raises_for_non_owner( + mocker: MockerFixture, + session_with_data: Session, +) -> None: + """ + Non-overwrite re-import that would resurrect a soft-deleted chart + must respect ownership: a non-owner without admin role cannot + restore-via-import. Mirrors the explicit /restore endpoint's check. + """ + mocker.patch.object(security_manager, "can_access", return_value=True) + mocker.patch.object(security_manager, "can_access_chart", return_value=True) + + _soft_delete_existing_chart(session_with_data) + + non_owner = User( + first_name="Bob", + last_name="Roe", + email="bob@example.org", + username="bob", + roles=[Role(name="Gamma")], + ) + + with override_user(non_owner): + with pytest.raises(ImportFailedError) as excinfo: + import_chart(chart_config, overwrite=False) + assert "permissions to restore" in str(excinfo.value) + + +def test_import_soft_deleted_chart_raises_when_caller_lacks_can_write( + mocker: MockerFixture, + session_with_data: Session, +) -> None: + """ + Case B: re-import of a soft-deleted UUID by a caller without + can_write must raise, not silently return the soft-deleted row. + + Real-world scenario: a user has can_write Dashboard but not + can_write Chart, and they import a dashboard zip that references a + soft-deleted chart. Silently returning the row would let the + dashboard importer reattach to it via chart_ids[uuid] = existing.id + and produce a dashboard with hidden (broken) charts. + """ + mocker.patch.object(security_manager, "can_access", return_value=False) + + _soft_delete_existing_chart(session_with_data) + + with pytest.raises(ImportFailedError) as excinfo: + import_chart(chart_config, overwrite=False) + assert "can_write" in str(excinfo.value) + + +def test_import_existing_active_chart_overwrite_without_can_write_returns_existing( + mocker: MockerFixture, + session_with_data: Session, +) -> None: + """ + An *active* (not soft-deleted) chart re-imported with overwrite=True by a + caller without can_write must fall through to returning the existing row, + not raise the restore error. Case B is keyed on ``is_soft_deleted``, so the + fused ``needs_mutation`` condition must not pull active rows into the + restore-without-permission branch (pre-soft-delete overwrite behaviour). + """ + mocker.patch.object(security_manager, "can_access", return_value=False) + + existing = ( + session_with_data.query(Slice).filter(Slice.uuid == chart_config["uuid"]).one() + ) + assert existing.deleted_at is None + + result = import_chart(chart_config, overwrite=True) + + assert result.id == existing.id + assert result.deleted_at is None + + def test_import_tag_logic_for_charts(session_with_schema: Session): contents = { "tags.yaml": yaml.dump( 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..62309a50366c --- /dev/null +++ b/tests/unit_tests/commands/chart/restore_test.py @@ -0,0 +1,90 @@ +# 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, timezone +from unittest.mock import MagicMock, patch + +import pytest + +from superset.commands.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, +) +from superset.commands.chart.restore import RestoreChartCommand +from superset.exceptions import SupersetSecurityException + + +def test_restore_chart_clears_deleted_at(app_context: None) -> None: + """RestoreChartCommand.run() restores a soft-deleted chart.""" + chart = MagicMock() + chart.deleted_at = datetime(2026, 1, 1, tzinfo=timezone.utc) + 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.""" + 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.""" + 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.""" + chart = MagicMock() + chart.deleted_at = datetime(2026, 1, 1, tzinfo=timezone.utc) + + 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/dao/dashboard_test.py b/tests/unit_tests/dao/dashboard_test.py new file mode 100644 index 000000000000..c47ad947690d --- /dev/null +++ b/tests/unit_tests/dao/dashboard_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. +from datetime import datetime, timezone +from typing import Any + +from sqlalchemy.orm.session import Session + + +def test_set_dash_metadata_preserves_soft_deleted_members( + session: Session, +) -> None: + """Saving a dashboard must not sever a soft-deleted member chart. + + ``set_dash_metadata`` rebuilds ``dashboard.slices`` wholesale from the + incoming position data. The slice-resolution query must bypass the + soft-delete visibility filter: with the filter active, a member chart + sitting in the trash would be silently dropped from the assignment — + deleting its ``dashboard_slices`` junction row (breaking the + restore-reattach contract) and writing ``uuid: None`` into its + position slot. This test fails if the bypass is removed. + """ + from superset import db + from superset.connectors.sqla.models import Database, SqlaTable + from superset.daos.dashboard import DashboardDAO + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + + Dashboard.metadata.create_all(session.get_bind()) + + dataset = SqlaTable( + table_name="dash_meta_table", + database=Database(database_name="dash_meta_db", sqlalchemy_uri="sqlite://"), + ) + db.session.add(dataset) + db.session.flush() + + live_chart = Slice( + slice_name="live_chart", + datasource_id=dataset.id, + datasource_type="table", + ) + trashed_chart = Slice( + slice_name="trashed_chart", + datasource_id=dataset.id, + datasource_type="table", + deleted_at=datetime(2026, 1, 1, tzinfo=timezone.utc), + ) + dashboard = Dashboard( + dashboard_title="meta_test_dash", + slices=[live_chart, trashed_chart], + published=True, + ) + db.session.add_all([live_chart, trashed_chart, dashboard]) + db.session.flush() + + positions: dict[str, dict[str, Any]] = { + "CHART-live": { + "type": "CHART", + "id": "CHART-live", + "children": [], + "meta": {"chartId": live_chart.id, "width": 4, "height": 50}, + }, + "CHART-trashed": { + "type": "CHART", + "id": "CHART-trashed", + "children": [], + "meta": {"chartId": trashed_chart.id, "width": 4, "height": 50}, + }, + } + + DashboardDAO.set_dash_metadata(dashboard, {"positions": positions}) + + member_ids = {chart.id for chart in dashboard.slices} + assert live_chart.id in member_ids + assert trashed_chart.id in member_ids, ( + "soft-deleted member chart was severed from dashboard.slices; " + "set_dash_metadata must bypass the visibility filter when " + "resolving incoming chart ids" + ) + # And the position slot kept its UUID rather than being nulled. + assert positions["CHART-trashed"]["meta"]["uuid"] == str(trashed_chart.uuid) diff --git a/tests/unit_tests/migrations/test_add_deleted_at_to_slices.py b/tests/unit_tests/migrations/test_add_deleted_at_to_slices.py new file mode 100644 index 000000000000..1c3ce6b916e5 --- /dev/null +++ b/tests/unit_tests/migrations/test_add_deleted_at_to_slices.py @@ -0,0 +1,186 @@ +# 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 migration ``7c4a8d09ca37_add_deleted_at_to_slices``. + +Runs the migration's ``upgrade()`` and ``downgrade()`` against an +in-memory SQLite engine with a real Alembic ``Operations`` context. +The behaviour being pinned is the operator-facing contract documented +in ``UPDATING.md``: ``downgrade()`` reverses the schema but does not +hard-delete or otherwise mutate rows that were soft-deleted before +the migration was reversed — those rows survive the downgrade and +become visible to any code path that no longer applies the +soft-delete visibility filter. +""" + +from __future__ import annotations + +from datetime import datetime +from importlib import import_module + +import pytest +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import ( + Column, + create_engine, + insert, + inspect, + Integer, + MetaData, + select, + String, + Table, +) +from sqlalchemy.engine import Engine + +migration = import_module( + "superset.migrations.versions." + "2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices" +) + +TABLE_NAME = migration.TABLE_NAME # "slices" +INDEX_NAME = migration.INDEX_NAME # "ix_slices_deleted_at" + + +@pytest.fixture +def engine() -> Engine: + """In-memory SQLite seeded with a minimal pre-migration ``slices`` table. + + The real ``slices`` table has many columns; the migration only touches + ``deleted_at`` and its index, so only the columns that participate in + the test are seeded. + """ + engine = create_engine("sqlite:///:memory:") + md = MetaData() + Table( + TABLE_NAME, + md, + Column("id", Integer, primary_key=True), + Column("slice_name", String(250), nullable=False), + ) + md.create_all(engine) + return engine + + +def _columns(engine: Engine) -> set[str]: + return {col["name"] for col in inspect(engine).get_columns(TABLE_NAME)} + + +def _indexes(engine: Engine) -> set[str]: + return {ix["name"] for ix in inspect(engine).get_indexes(TABLE_NAME)} + + +def test_upgrade_adds_deleted_at_column_and_index(engine: Engine) -> None: + with engine.connect() as conn: + ctx = MigrationContext.configure(conn) + with Operations.context(ctx): + migration.upgrade() + + assert "deleted_at" in _columns(engine), "upgrade() must add the deleted_at column" + assert INDEX_NAME in _indexes(engine), ( + "upgrade() must create the supporting index on deleted_at" + ) + + +def test_downgrade_drops_deleted_at_column_and_index(engine: Engine) -> None: + with engine.connect() as conn: + ctx = MigrationContext.configure(conn) + with Operations.context(ctx): + migration.upgrade() + migration.downgrade() + + assert "deleted_at" not in _columns(engine), ( + "downgrade() must drop the deleted_at column" + ) + assert INDEX_NAME not in _indexes(engine), ( + "downgrade() must drop the supporting index" + ) + + +def test_downgrade_preserves_soft_deleted_row_data(engine: Engine) -> None: + """Pin the operator-facing contract from ``UPDATING.md``: rows that + were soft-deleted before the migration is reversed survive the + downgrade. The ``deleted_at`` column is gone, so those rows are + indistinguishable from live rows to any code path that no longer + applies the visibility filter — operators must decide on + hard-delete, restore, or rename BEFORE downgrading. See the + "Rollback note" in ``UPDATING.md``. + """ + with engine.connect() as conn: + ctx = MigrationContext.configure(conn) + with Operations.context(ctx): + migration.upgrade() + + slices = Table(TABLE_NAME, MetaData(), autoload_with=conn) + conn.execute( + insert(slices).values( + [ + { + "id": 1, + "slice_name": "live_chart", + "deleted_at": None, + }, + { + "id": 2, + "slice_name": "archived_chart", + "deleted_at": datetime(2026, 1, 1, 12, 0, 0), + }, + ] + ) + ) + + migration.downgrade() + + slices_after = Table(TABLE_NAME, MetaData(), autoload_with=conn) + rows = conn.execute( + select(slices_after).order_by(slices_after.c.id) + ).fetchall() + + assert [(r.id, r.slice_name) for r in rows] == [ + (1, "live_chart"), + (2, "archived_chart"), + ], ( + "downgrade() must not delete or mutate row data — soft-deleted " + "rows become indistinguishable from live rows but they remain" + ) + assert "deleted_at" not in { + c["name"] for c in inspect(engine).get_columns(TABLE_NAME) + } + + +def test_upgrade_is_idempotent(engine: Engine) -> None: + """The migration helpers (``add_columns``, ``create_index``) are + idempotent skip-if-exists; running ``upgrade()`` twice must not + raise. + """ + with engine.connect() as conn: + ctx = MigrationContext.configure(conn) + with Operations.context(ctx): + migration.upgrade() + migration.upgrade() + + +def test_downgrade_is_idempotent(engine: Engine) -> None: + """``drop_columns`` / ``drop_index`` are skip-if-not-exists; running + ``downgrade()`` twice must not raise. + """ + with engine.connect() as conn: + ctx = MigrationContext.configure(conn) + with Operations.context(ctx): + migration.upgrade() + migration.downgrade() + migration.downgrade()