mikebridge commented on code in PR #40129: URL: https://github.com/apache/superset/pull/40129#discussion_r3510307878
########## 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() Review Comment: Following the surrounding file's conventions — local/test-scaffolding variables whose types are unambiguous from context. Declining as style. ########## 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} Review Comment: Following the surrounding file's conventions — local/test-scaffolding variables whose types are unambiguous from context. Declining as style. ########## 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" +) Review Comment: Following the surrounding file's conventions — local/test-scaffolding variables whose types are unambiguous from context. Declining as style. ########## superset/models/slice.py: ########## @@ -67,7 +71,7 @@ class Slice( # pylint: disable=too-many-public-methods - CoreChart, AuditMixinNullable, ImportExportMixin + CoreChart, SoftDeleteMixin, AuditMixinNullable, ImportExportMixin Review Comment: Factually out of date: `BaseDAO.delete` routes to `soft_delete()` **only when** `is_feature_enabled("SOFT_DELETE")` — the gate merged to master via #41166 (2026-07-01) and this branch is rebased onto it. With the flag off (the default), chart deletes remain permanent hard deletes; adding `SoftDeleteMixin` alone changes nothing. The UPDATING.md section and the delete/bulk_delete OpenAPI docstrings now state the flag gate explicitly. Declining. ########## superset/commands/chart/importers/v1/utils.py: ########## @@ -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"]): Review Comment: Following the surrounding file's conventions — local/test-scaffolding variables whose types are unambiguous from context. Declining as style. ########## 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 "" Review Comment: An empty `msgstr` is expected for newly-extracted strings — catalogs carry new ids untranslated until translators fill them in; untranslated strings fall back to source text. Declining. ########## superset/commands/report/exceptions.py: ########## @@ -225,6 +225,13 @@ class ReportScheduleExecuteUnexpectedError(CommandException): message = _("Report Schedule execution got an unexpected error.") +class ReportScheduleTargetChartDeletedError(CommandException): Review Comment: Deliberate, matching the documented precedent on `ReportScheduleExecutorNotFoundError` in this same file: this exception is raised inside the Celery report executor (not an HTTP route), where the status drives `get_logger_from_status` — 5xx marks the task FAILURE and keeps the broken report visible to ops task-state alerting, while 4xx would log a WARNING and hide it. A report that will fail every scheduled run until the chart is restored/re-pointed should surface as FAILURE. Declining. ########## 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 "" Review Comment: An empty `msgstr` is expected for newly-extracted strings — catalogs carry new ids untranslated until translators fill them in; untranslated strings fall back to source text. Declining. ########## superset/daos/dashboard.py: ########## @@ -276,11 +277,30 @@ def set_dash_metadata( if isinstance(value, dict) ] - current_slices = ( - db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all() - ) - - dashboard.slices = current_slices + # 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. + # + # The bypass must be session-scoped and cover the ASSIGNMENT, + # not just the resolution query: assigning to + # ``dashboard.slices`` makes the unit of work diff the new + # collection against the existing one, which it lazy-loads at + # that moment. A filtered baseline load would exclude the + # trashed member, so the diff would treat it as net-new and + # INSERT a ``dashboard_slices`` row that still exists (soft + # delete never removes junction rows) — an IntegrityError on + # the composite primary key on every save of a dashboard + # containing a trashed chart. + with skip_visibility_filter(db.session, Slice): + current_slices = ( + db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all() + ) Review Comment: Following the surrounding file's conventions — local/test-scaffolding variables whose types are unambiguous from context. Declining as style. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
