mikebridge commented on code in PR #42469: URL: https://github.com/apache/superset/pull/42469#discussion_r3673616665
########## tests/integration_tests/charts/version_restore_tests.py: ########## @@ -0,0 +1,454 @@ +# 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 (Slice) version restore. + +Covers POST /api/v1/chart/<uuid>/versions/<version_uuid>/restore: the +non-destructive revert applies the target snapshot, appends a new +version row, attributes the change to the restoring user, and returns +the documented 400/404 errors for malformed or unknown UUIDs. +""" + +from __future__ import annotations + +from typing import Any + +import pytest +from sqlalchemy_continuum import version_class + +from superset.extensions import db +from superset.models.slice import Slice +from superset.utils import json as _json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME +from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa: F401 + load_birth_names_dashboard_with_slices, + load_birth_names_data, +) + + +def _get_version_rows(chart: Slice) -> list[Any]: + ver_cls = version_class(Slice) + return ( + db.session.query(ver_cls) + .filter(ver_cls.id == chart.id) + .order_by(ver_cls.transaction_id.asc()) + .all() + ) + + +def _persist_fixture_state() -> None: + """Force fixture's pending INSERTs to commit in their own transaction. + + The birth_names fixture stages charts and the dashboard via session.add() + but does not commit. Without this, the test's first commit batches the + INSERTs and UPDATEs into the same Continuum transaction, causing the + existing version row to be updated in place instead of a new one being + created. + """ + db.session.commit() + + +class TestChartRestoreApi(SupersetTestCase): + """T037 — POST /api/v1/chart/<uuid>/versions/<version_uuid>/restore.""" + + @pytest.fixture(autouse=True) + def _load_data(self, load_birth_names_dashboard_with_slices): # noqa: PT004, F811 + pass + + def _restore(self, chart_uuid: str, version_uuid: str) -> Any: + return self.client.post( + f"/api/v1/chart/{chart_uuid}/versions/{version_uuid}/restore" + ) + + def _list(self, chart_uuid: str) -> Any: + return self.client.get(f"/api/v1/chart/{chart_uuid}/versions/") + + def test_restore_applies_scalar_field_from_target_version(self) -> None: + """Restoring version 0 puts the slice_name back to its pre-edit value + and appends a new version entry.""" + _persist_fixture_state() + chart: Slice = ( + db.session.query(Slice).filter(Slice.slice_name == "Girls").first() + ) + assert chart is not None + chart_uuid = str(chart.uuid) + original_name = chart.slice_name + + # Produce two additional saves so version history is 0/1/2. + chart.slice_name = "Girls v1" + db.session.commit() + chart.slice_name = "Girls v2" + db.session.commit() + + self.login(ADMIN_USERNAME) + rv_list = self._list(chart_uuid) + assert rv_list.status_code == 200 + listing = _json.loads(rv_list.data.decode("utf-8")) + initial_count = listing["count"] + assert initial_count >= 3 + target_uuid = listing["result"][0]["version_uuid"] + + # Restore to the first version (the original "Girls" name). + rv = self._restore(chart_uuid, target_uuid) + assert rv.status_code == 200, rv.data + + # Live state matches the restored snapshot. + db.session.expire_all() + chart = db.session.query(Slice).filter(Slice.uuid == chart.uuid).one() + assert chart.slice_name == original_name + + # A new version row was recorded (non-destructive). + rv_list2 = self._list(chart_uuid) + body = _json.loads(rv_list2.data.decode("utf-8")) + assert body["count"] == initial_count + 1 + + # Cleanup + chart.slice_name = original_name + db.session.commit() + + def test_restore_returns_404_for_unknown_uuid(self) -> None: + self.login(ADMIN_USERNAME) + rv = self._restore( + "00000000-0000-0000-0000-000000000000", + "00000000-0000-0000-0000-000000000001", + ) + assert rv.status_code == 404 + + def test_restore_returns_404_for_unknown_version_uuid(self) -> None: + _persist_fixture_state() + chart: Slice = ( + db.session.query(Slice).filter(Slice.slice_name == "Boys").first() + ) + assert chart is not None + self.login(ADMIN_USERNAME) + rv = self._restore(str(chart.uuid), "00000000-0000-0000-0000-000000000099") + assert rv.status_code == 404 + + def test_restore_returns_400_for_invalid_entity_uuid(self) -> None: + self.login(ADMIN_USERNAME) + rv = self._restore("not-a-uuid", "00000000-0000-0000-0000-000000000001") + assert rv.status_code == 400 + + def test_restore_returns_400_for_invalid_version_uuid(self) -> None: + _persist_fixture_state() + chart: Slice = ( + db.session.query(Slice).filter(Slice.slice_name == "Boys").first() + ) + assert chart is not None + self.login(ADMIN_USERNAME) + rv = self._restore(str(chart.uuid), "not-a-uuid") + assert rv.status_code == 400 + + def test_get_version_returns_historical_snapshot(self) -> None: + """GET /versions/<uuid>/ returns the chart's fields at that version + without modifying live state.""" + _persist_fixture_state() + chart: Slice = ( + db.session.query(Slice).filter(Slice.slice_name == "Girls").first() + ) + assert chart is not None + chart_uuid = str(chart.uuid) + original_name = chart.slice_name + + chart.slice_name = "Girls (v1)" + db.session.commit() + + self.login(ADMIN_USERNAME) + listing = _json.loads(self._list(chart_uuid).data.decode("utf-8")) + assert listing["count"] >= 2 + # The earliest entry should still hold the original slice_name. + first_version_uuid = listing["result"][0]["version_uuid"] + + rv = self.client.get( + f"/api/v1/chart/{chart_uuid}/versions/{first_version_uuid}/" + ) + assert rv.status_code == 200, rv.data + body = _json.loads(rv.data.decode("utf-8"))["result"] + assert body["slice_name"] == original_name + assert body["_version"]["version_uuid"] == first_version_uuid + assert body["_version"]["version_number"] == 0 + # Live row unchanged. + db.session.expire_all() + live = db.session.query(Slice).filter(Slice.uuid == chart.uuid).one() + assert live.slice_name == "Girls (v1)" + + # Cleanup + live.slice_name = original_name + db.session.commit() + + def test_get_version_returns_404_for_unknown_entity(self) -> None: + self.login(ADMIN_USERNAME) + rv = self.client.get( + "/api/v1/chart/00000000-0000-0000-0000-000000000000" + "/versions/00000000-0000-0000-0000-000000000001/" + ) + assert rv.status_code == 404 + + def test_get_version_returns_400_for_invalid_uuid(self) -> None: + self.login(ADMIN_USERNAME) + rv = self.client.get( + "/api/v1/chart/not-a-uuid/versions/00000000-0000-0000-0000-000000000001/" + ) + assert rv.status_code == 400 + + def test_restore_stamps_changed_by_with_restoring_user(self) -> None: + """After a restore, changed_by_fk on the live entity must point at + the restoring user (not at whoever authored the version being + restored). created_by_fk stays unchanged. The new version row + produced by the restore also carries the restoring user in its + changed_by metadata. + """ + from superset.daos.version import derive_version_uuid + + _persist_fixture_state() + self.login(ADMIN_USERNAME) + admin_id = self.get_user(ADMIN_USERNAME).id + chart: Slice = ( + db.session.query(Slice).filter(Slice.slice_name == "Girls").first() + ) + assert chart is not None + chart_id = chart.id + chart_uuid = str(chart.uuid) + entity_uuid = chart.uuid + original_name = chart.slice_name + original_created_by = chart.created_by_fk + before_changed_on = chart.changed_on + + # Produce a second version to restore to. + chart.slice_name = "Girls v1" + db.session.commit() + + ver_cls = version_class(Slice) + first_tx = ( + db.session.query(ver_cls.transaction_id) + .filter(ver_cls.id == chart_id) + .order_by(ver_cls.transaction_id.asc()) + .limit(1) + .scalar() + ) + assert first_tx is not None + target_uuid = str(derive_version_uuid(entity_uuid, first_tx)) + + rv = self.client.post( + f"/api/v1/chart/{chart_uuid}/versions/{target_uuid}/restore" + ) + assert rv.status_code == 200, rv.data + + db.session.expire_all() + chart = db.session.query(Slice).filter(Slice.id == chart_id).one() + + # Live entity checks. + assert chart.slice_name == original_name + assert chart.created_by_fk == original_created_by + assert chart.changed_by_fk == admin_id, ( + f"Expected changed_by_fk to be restoring user id={admin_id}, " + f"got {chart.changed_by_fk}" + ) + if before_changed_on is not None and chart.changed_on is not None: + assert chart.changed_on >= before_changed_on + + # The new version row produced by the restore must attribute the + # change to the restoring user. + rv_list = self.client.get(f"/api/v1/chart/{chart_uuid}/versions/") + assert rv_list.status_code == 200 + body = _json.loads(rv_list.data.decode("utf-8")) + latest_entry = body["result"][-1] + assert latest_entry["changed_by"] is not None, ( + "New version row should have a changed_by" + ) + assert latest_entry["changed_by"]["id"] == admin_id + + # Cleanup + chart.slice_name = original_name + db.session.commit() + + def test_put_response_returns_old_and_new_version_numbers(self) -> None: + """PUT /api/v1/chart/<id> response must include old_version and + new_version matching the list-versions ordering.""" + _persist_fixture_state() + chart: Slice = ( + db.session.query(Slice).filter(Slice.slice_name == "Girls").first() + ) + assert chart is not None + chart_id = chart.id + original_name = chart.slice_name + + ver_cls = version_class(Slice) + count_before = db.session.query(ver_cls).filter(ver_cls.id == chart_id).count() + expected_old = count_before - 1 if count_before > 0 else None + + self.login(ADMIN_USERNAME) + rv = self.client.put( + f"/api/v1/chart/{chart_id}", + json={"slice_name": "put-response-version-test"}, + ) + assert rv.status_code == 200, rv.data + body = _json.loads(rv.data.decode("utf-8")) + assert body["id"] == chart_id + assert body["old_version"] == expected_old + assert body["new_version"] is not None + assert "old_transaction_id" in body + assert "new_transaction_id" in body + if body["old_transaction_id"] is not None: + assert body["new_transaction_id"] != body["old_transaction_id"] + + # Cleanup + chart = db.session.query(Slice).filter(Slice.id == chart_id).one() + chart.slice_name = original_name + db.session.commit() + + def test_restore_denies_non_editor_with_write_permission(self) -> None: + """A user holding can_write on Chart but who is not an editor of + THIS chart gets 403 from the command's ``raise_for_editorship`` + branch — the interesting case route-level ``@protect()`` cannot + catch.""" + from superset.daos.version import derive_version_uuid + from tests.integration_tests.constants import ALPHA_USERNAME + + _persist_fixture_state() + chart: Slice = ( + db.session.query(Slice).filter(Slice.slice_name == "Girls").first() + ) + assert chart is not None + # Ensure alpha is not an editor of the fixture chart. + alpha = self.get_user(ALPHA_USERNAME) + assert alpha not in chart.editors + + ver_cls = version_class(Slice) + first_tx = ( + db.session.query(ver_cls.transaction_id) + .filter(ver_cls.id == chart.id) + .order_by(ver_cls.transaction_id.asc()) + .limit(1) + .scalar() + ) + assert first_tx is not None + target_uuid = str(derive_version_uuid(chart.uuid, first_tx)) + + self.login(ALPHA_USERNAME) + rv = self._restore(str(chart.uuid), target_uuid) + assert rv.status_code == 403, rv.data Review Comment: Taken in `c65d09f94e` — applied your suggestion verbatim. Agreed the asymmetry was the tell: the capture-disabled test already proved the entity was untouched, so the forbidden path should hold itself to the same bar. Done for the dashboard and dataset siblings too. ########## tests/integration_tests/dashboards/version_restore_tests.py: ########## @@ -0,0 +1,416 @@ +# 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 Dashboard version restore. + +Covers POST /api/v1/dashboard/<uuid>/versions/<version_uuid>/restore: +the non-destructive revert applies the target snapshot (including +reattaching a chart removed after that snapshot) and returns the +documented 404s for unknown entity/version UUIDs. +""" + +from __future__ import annotations + +from typing import Any + +import pytest +import sqlalchemy as sa +from sqlalchemy_continuum import version_class + +from superset.extensions import db +from superset.models.dashboard import Dashboard +from superset.utils import json as _json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME +from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa: F401 + load_birth_names_dashboard_with_slices, + load_birth_names_data, +) + + +def _get_version_rows(dashboard: Dashboard) -> list[Any]: + ver_cls = version_class(Dashboard) + return ( + db.session.query(ver_cls) + .filter(ver_cls.id == dashboard.id) + .order_by(ver_cls.transaction_id.asc()) + .all() + ) + + +def _persist_fixture_state() -> None: + """Force fixture's pending INSERTs to commit in their own transaction. + + The birth_names fixture stages charts and the dashboard via session.add() + but does not commit. Without this, the test's first commit batches the + INSERTs and UPDATEs into the same Continuum transaction, causing the + existing version row to be updated in place instead of a new one being + created. + """ + db.session.commit() + + +class TestDashboardRestoreApi(SupersetTestCase): + """T038 — POST /api/v1/dashboard/<uuid>/versions/<version_uuid>/restore.""" + + @pytest.fixture(autouse=True) + def _load_data(self, load_birth_names_dashboard_with_slices): # noqa: PT004, F811 + pass + + def _restore(self, dashboard_uuid: str, version_uuid: str) -> Any: + return self.client.post( + f"/api/v1/dashboard/{dashboard_uuid}/versions/{version_uuid}/restore" + ) + + def test_restore_applies_scalar_field(self) -> None: + """Restore a dashboard title edit.""" + from superset.daos.version import derive_version_uuid + + _persist_fixture_state() + dashboard: Dashboard = ( + db.session.query(Dashboard) + .filter(Dashboard.dashboard_title == "USA Births Names") + .first() + ) + assert dashboard is not None + dashboard_uuid = str(dashboard.uuid) + original_title = dashboard.dashboard_title + dashboard_id = dashboard.id + entity_uuid = dashboard.uuid + + # Make two more edits so we have a known non-trivial history to + # navigate: [initial, v1, v2]. + dashboard.dashboard_title = "USA Births Names v1" + db.session.commit() + dashboard.dashboard_title = "USA Births Names v2" + db.session.commit() + + ver_cls = version_class(Dashboard) + rows = ( + db.session.query( + ver_cls.transaction_id, + ver_cls.operation_type, + ver_cls.dashboard_title, + ver_cls.end_transaction_id, + ) + .filter(ver_cls.id == dashboard_id) + .order_by(ver_cls.transaction_id.asc()) + .all() + ) + # Find the version whose snapshot has the original title. + target_row = next( + (row for row in rows if row.dashboard_title == original_title), + None, + ) + assert target_row is not None, ( + f"Expected at least one version row with original title; rows={rows}" + ) + target_uuid = str(derive_version_uuid(entity_uuid, target_row.transaction_id)) + + self.login(ADMIN_USERNAME) + rv = self._restore(dashboard_uuid, target_uuid) + assert rv.status_code == 200, rv.data + + db.session.expire_all() + dashboard = ( + db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one() + ) + assert dashboard.dashboard_title == original_title, ( + f"Restore did not revert title; rows={rows}" + ) + + # Cleanup + dashboard.dashboard_title = original_title + db.session.commit() + + def test_restore_reattaches_chart_removed_after_snapshot(self) -> None: + """After the target snapshot is captured, detaching a chart and saving + must be undone by restore — the chart comes back on dashboard_slices.""" + from superset.daos.version import derive_version_uuid + + _persist_fixture_state() + dashboard: Dashboard = ( + db.session.query(Dashboard) + .filter(Dashboard.dashboard_title == "USA Births Names") + .first() + ) + assert dashboard is not None + dashboard_uuid = str(dashboard.uuid) + dashboard_id = dashboard.id + entity_uuid = dashboard.uuid + + original_slice_ids = sorted(s.id for s in dashboard.slices) + assert len(original_slice_ids) >= 2, ( + f"fixture expected to attach >= 2 charts; got {original_slice_ids}" + ) + slice_to_drop = dashboard.slices[0] + drop_id = slice_to_drop.id + + # Touch the dashboard so a snapshot row is captured at a known tx. + dashboard.dashboard_title = "USA Births Names — snapshot point" + db.session.commit() + + ver_cls = version_class(Dashboard) + target_tx = ( + db.session.query(ver_cls.transaction_id) + .filter(ver_cls.id == dashboard_id) + .order_by(ver_cls.transaction_id.desc()) + .limit(1) + .scalar() + ) + assert target_tx is not None + target_uuid = str(derive_version_uuid(entity_uuid, target_tx)) + + # Detach the chart and commit — moves history forward. + dashboard.slices.remove(slice_to_drop) + db.session.commit() + + db.session.expire_all() + dashboard = ( + db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one() + ) + live_ids = {s.id for s in dashboard.slices} + assert drop_id not in live_ids, "pre-restore: dropped chart should be detached" + + self.login(ADMIN_USERNAME) + rv = self._restore(dashboard_uuid, target_uuid) + assert rv.status_code == 200, rv.data + + db.session.expire_all() + dashboard = ( + db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one() + ) + restored_ids = sorted(s.id for s in dashboard.slices) + assert restored_ids == original_slice_ids, ( + f"restore did not re-attach chart: expected {original_slice_ids}, " + f"got {restored_ids}" + ) + + def test_restore_preserves_live_chart_content(self) -> None: + """Dashboard restore is membership-only: a member chart edited + AFTER the snapshot keeps its current content — charts are shared + entities with their own restore endpoint, so a dashboard restore + must never rewrite them to historical values.""" + from superset.daos.version import derive_version_uuid + from superset.models.slice import Slice + + _persist_fixture_state() + dashboard: Dashboard = ( + db.session.query(Dashboard) + .filter(Dashboard.dashboard_title == "USA Births Names") + .first() + ) + assert dashboard is not None + dashboard_id = dashboard.id + member = dashboard.slices[0] + member_id = member.id + original_chart_name = member.slice_name + original_title = dashboard.dashboard_title + + # Snapshot point: chart still has its original name. + dashboard.dashboard_title = "USA Births Names — content snapshot" + db.session.commit() + + ver_cls = version_class(Dashboard) + target_tx = ( + db.session.query(ver_cls.transaction_id) + .filter(ver_cls.id == dashboard_id) + .order_by(ver_cls.transaction_id.desc()) + .limit(1) + .scalar() + ) + target_uuid = str(derive_version_uuid(dashboard.uuid, target_tx)) + + # Edit the member chart AFTER the snapshot. + member = db.session.query(Slice).filter(Slice.id == member_id).one() + member.slice_name = "edited after snapshot" + db.session.commit() + + self.login(ADMIN_USERNAME) + rv = self._restore(str(dashboard.uuid), target_uuid) + assert rv.status_code == 200, rv.data + + db.session.expire_all() + member = db.session.query(Slice).filter(Slice.id == member_id).one() + assert member.slice_name == "edited after snapshot", ( + "dashboard restore must not rewrite live member chart content" + ) + + # Cleanup + member.slice_name = original_chart_name + dashboard = ( + db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one() + ) + dashboard.dashboard_title = original_title + db.session.commit() + + def test_restore_skips_member_chart_that_no_longer_exists(self) -> None: + """A snapshot member whose chart row has been hard-deleted stays + deleted: restore succeeds, reattaches the surviving members, does + NOT revive the deleted chart, and says so in the response.""" + from superset.daos.version import derive_version_uuid + from superset.models.slice import Slice + + _persist_fixture_state() + dashboard: Dashboard = ( + db.session.query(Dashboard) + .filter(Dashboard.dashboard_title == "USA Births Names") + .first() + ) + assert dashboard is not None + dashboard_id = dashboard.id + original_ids = sorted(s.id for s in dashboard.slices) + assert len(original_ids) >= 2 + victim = dashboard.slices[0] + victim_id = victim.id + + # Snapshot point: victim is a member. + dashboard.dashboard_title = "USA Births Names — skip snapshot" + db.session.commit() + + ver_cls = version_class(Dashboard) + target_tx = ( + db.session.query(ver_cls.transaction_id) + .filter(ver_cls.id == dashboard_id) + .order_by(ver_cls.transaction_id.desc()) + .limit(1) + .scalar() + ) + target_uuid = str(derive_version_uuid(dashboard.uuid, target_tx)) + + # Detach, then hard-delete the victim via raw SQL so no live row + # remains (bypasses the soft-delete listener deliberately — the + # scenario is a purged/legacy-deleted chart). + dashboard.slices.remove(victim) + db.session.commit() + for assoc in ("chart_editors", "chart_viewers"): + db.session.execute( + sa.text(f"DELETE FROM {assoc} WHERE chart_id = :sid"), # noqa: S608 + {"sid": victim_id}, + ) + db.session.execute( + sa.text("DELETE FROM slices WHERE id = :sid"), {"sid": victim_id} + ) + db.session.commit() + + self.login(ADMIN_USERNAME) + rv = self._restore(str(dashboard.uuid), target_uuid) + assert rv.status_code == 200, rv.data + message = _json.loads(rv.data.decode("utf-8")).get("message", "") + assert "no longer exist" in message, message + + db.session.expire_all() + dashboard = ( + db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one() + ) + restored_ids = sorted(s.id for s in dashboard.slices) + assert victim_id not in restored_ids, "deleted chart must stay deleted" + assert restored_ids == sorted(sid for sid in original_ids if sid != victim_id) + assert ( + db.session.query(Slice).filter(Slice.id == victim_id).one_or_none() is None + ), "restore must not revive a hard-deleted chart" + + def test_restore_denies_non_editor_with_write_permission(self) -> None: + """A user holding can_write on Dashboard but who is not an editor + of THIS dashboard gets 403 from the command's editorship check.""" + from superset.daos.version import derive_version_uuid + from tests.integration_tests.constants import ALPHA_USERNAME + + _persist_fixture_state() + dashboard: Dashboard = ( + db.session.query(Dashboard) + .filter(Dashboard.dashboard_title == "USA Births Names") + .first() + ) + assert dashboard is not None + alpha = self.get_user(ALPHA_USERNAME) + assert alpha not in dashboard.editors + + ver_cls = version_class(Dashboard) + first_tx = ( + db.session.query(ver_cls.transaction_id) + .filter(ver_cls.id == dashboard.id) + .order_by(ver_cls.transaction_id.asc()) + .limit(1) + .scalar() + ) + assert first_tx is not None + target_uuid = str(derive_version_uuid(dashboard.uuid, first_tx)) + + self.login(ALPHA_USERNAME) + rv = self._restore(str(dashboard.uuid), target_uuid) + assert rv.status_code == 403, rv.data Review Comment: Taken in `c65d09f94e`. ########## tests/integration_tests/datasets/version_restore_tests.py: ########## @@ -0,0 +1,505 @@ +# 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 Dataset (SqlaTable) version restore. + +Covers POST /api/v1/dataset/<uuid>/versions/<version_uuid>/restore: the +non-destructive revert applies the target snapshot, reverts child +column/metric edits (re-adding removed children and dropping added ones) +in a single transaction, denies callers without write permission, and +returns the documented 400/404 errors. +""" + +from __future__ import annotations + +from typing import Any + +import pytest +import sqlalchemy as sa +from sqlalchemy_continuum import version_class + +from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn +from superset.extensions import db +from superset.utils import json as _json +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME, GAMMA_USERNAME +from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa: F401 + load_birth_names_dashboard_with_slices, + load_birth_names_data, +) + + +def _get_table_column_version_rows(column: TableColumn) -> list[Any]: + ver_cls = version_class(TableColumn) + return ( + db.session.query(ver_cls) + .filter(ver_cls.id == column.id) + .order_by(ver_cls.transaction_id.asc()) + .all() + ) + + +def _get_sql_metric_version_rows(metric: SqlMetric) -> list[Any]: + ver_cls = version_class(SqlMetric) + return ( + db.session.query(ver_cls) + .filter(ver_cls.id == metric.id) + .order_by(ver_cls.transaction_id.asc()) + .all() + ) + + +def _get_table_version_rows(table: SqlaTable) -> list[Any]: + ver_cls = version_class(SqlaTable) + return ( + db.session.query(ver_cls) + .filter(ver_cls.id == table.id) + .order_by(ver_cls.transaction_id.asc()) + .all() + ) + + +def _persist_fixture_state() -> None: + """Force fixture's pending INSERTs to commit in their own transaction. + + The birth_names fixture stages charts and the dashboard via session.add() + but does not commit. Without this, the test's first commit batches the + INSERTs and UPDATEs into the same Continuum transaction, causing the + existing version row to be updated in place instead of a new one being + created. + """ + db.session.commit() + + +class TestDatasetRestoreApi(SupersetTestCase): + """T039 — POST /api/v1/dataset/<uuid>/versions/<version_uuid>/restore.""" + + @pytest.fixture(autouse=True) + def _load_data(self, load_birth_names_dashboard_with_slices): # noqa: PT004, F811 + pass + + def _restore(self, dataset_uuid: str, version_uuid: str) -> Any: + return self.client.post( + f"/api/v1/dataset/{dataset_uuid}/versions/{version_uuid}/restore" + ) + + def test_restore_applies_scalar_field(self) -> None: + """Restore a dataset's description edit.""" + from superset.daos.version import derive_version_uuid + + _persist_fixture_state() + table: SqlaTable = ( + db.session.query(SqlaTable) + .filter(SqlaTable.table_name == "birth_names") + .first() + ) + assert table is not None + table_uuid = str(table.uuid) + entity_uuid = table.uuid + table_id = table.id + original_description = table.description + + # Two more edits to produce a non-trivial history. + table.description = "restore-test v1" + db.session.commit() + table.description = "restore-test v2" + db.session.commit() + + ver_cls = version_class(SqlaTable) + rows = ( + db.session.query( + ver_cls.transaction_id, + ver_cls.operation_type, + ver_cls.description, + ) + .filter(ver_cls.id == table_id) + .order_by(ver_cls.transaction_id.asc()) + .all() + ) + target_row = next( + (row for row in rows if row.description == original_description), + None, + ) + assert target_row is not None, ( + f"No version with original description; rows={rows}" + ) + target_uuid = str(derive_version_uuid(entity_uuid, target_row.transaction_id)) + + self.login(ADMIN_USERNAME) + rv = self._restore(table_uuid, target_uuid) + assert rv.status_code == 200, rv.data + + db.session.expire_all() + table = db.session.query(SqlaTable).filter(SqlaTable.id == table_id).one() + assert table.description == original_description + + # Cleanup + table.description = original_description + db.session.commit() + + def test_restore_with_column_edits_reverts_columns(self) -> None: + """After editing a column's description, restoring an earlier version + reverts the column.""" + from superset.daos.version import derive_version_uuid + + _persist_fixture_state() + table: SqlaTable = ( + db.session.query(SqlaTable) + .filter(SqlaTable.table_name == "birth_names") + .first() + ) + assert table is not None + table_uuid = str(table.uuid) + entity_uuid = table.uuid + table_id = table.id + + col = table.columns[0] + col_name = col.column_name + original_col_description = col.description + + # Snapshot target version before our column edit. + ver_cls = version_class(SqlaTable) + last_tx = ( + db.session.query(ver_cls.transaction_id) + .filter(ver_cls.id == table_id) + .order_by(ver_cls.transaction_id.desc()) + .limit(1) + .scalar() + ) + assert last_tx is not None + target_uuid = str(derive_version_uuid(entity_uuid, last_tx)) + + col.description = "restore-test column edit" + db.session.commit() + + self.login(ADMIN_USERNAME) + rv = self._restore(table_uuid, target_uuid) + assert rv.status_code == 200, rv.data + + # JSON-snapshot restore reassigns child PKs, so look up by natural + # key (column_name) rather than the old id. + db.session.expire_all() + col = ( + db.session.query(TableColumn) + .filter(TableColumn.table_id == table_id) + .filter(TableColumn.column_name == col_name) + .one() + ) + assert col.description == original_col_description + + # Cleanup + col.description = original_col_description + db.session.commit() + + def test_restore_adds_back_removed_column_and_drops_added_one(self) -> None: + """After a snapshot is taken, removing an existing column and adding + a new one, restoring the snapshot must undo both operations.""" + from superset.daos.version import derive_version_uuid + + _persist_fixture_state() + table: SqlaTable = ( + db.session.query(SqlaTable) + .filter(SqlaTable.table_name == "birth_names") + .first() + ) + assert table is not None + table_id = table.id + table_uuid = str(table.uuid) + entity_uuid = table.uuid + + original_col_names = sorted(c.column_name for c in table.columns) + removed_name = table.columns[0].column_name + + # Capture a snapshot tx point by touching the dataset. + table.description = "snapshot before column-swap" + db.session.commit() + + ver_cls = version_class(SqlaTable) + target_tx = ( + db.session.query(ver_cls.transaction_id) + .filter(ver_cls.id == table_id) + .order_by(ver_cls.transaction_id.desc()) + .limit(1) + .scalar() + ) + assert target_tx is not None + target_uuid = str(derive_version_uuid(entity_uuid, target_tx)) + + # Remove a column, add a new one, commit (moves history forward). + db.session.delete(table.columns[0]) + db.session.add( + TableColumn( + table_id=table_id, + column_name="__restore_test_calc__", + expression="1", + ) + ) + db.session.commit() + + assert removed_name not in {c.column_name for c in table.columns} + assert "__restore_test_calc__" in {c.column_name for c in table.columns} + + self.login(ADMIN_USERNAME) + rv = self._restore(table_uuid, target_uuid) + assert rv.status_code == 200, rv.data + + db.session.expire_all() + table = db.session.query(SqlaTable).filter(SqlaTable.id == table_id).one() + restored_names = sorted(c.column_name for c in table.columns) + assert restored_names == original_col_names + + def test_restore_emits_full_child_diff_in_one_transaction(self) -> None: + """A restore that re-adds one column and drops another MUST write + *both* change records under the same transaction. Under the prior + per-relation flush loop the first flush emitted only the + easier-to-detect change (the modification of a surviving + column), the listener's tx-dedup guard then suppressed the + second pass, and the addition record was silently lost from + ``version_changes`` — the dropdown rendered the restore as an + empty "Baseline" entry. Locks in the single-flush restore + behavior in ``VersionDAO.restore_version``. + """ + from superset.daos.version import derive_version_uuid + from superset.versioning.changes import version_changes_table + + _persist_fixture_state() + table: SqlaTable = ( + db.session.query(SqlaTable) + .filter(SqlaTable.table_name == "birth_names") + .first() + ) + assert table is not None + table_id = table.id + table_uuid = str(table.uuid) + entity_uuid = table.uuid + removed_name = table.columns[0].column_name + added_name = "__restore_full_diff_test__" + + # Snapshot point captures the baseline. + table.description = "snapshot before full-diff column swap" + db.session.commit() + + ver_cls = version_class(SqlaTable) + target_tx = ( + db.session.query(ver_cls.transaction_id) + .filter(ver_cls.id == table_id) + .order_by(ver_cls.transaction_id.desc()) + .limit(1) + .scalar() + ) + assert target_tx is not None + target_uuid = str(derive_version_uuid(entity_uuid, target_tx)) + + db.session.delete(table.columns[0]) + db.session.add( + TableColumn(table_id=table_id, column_name=added_name, expression="1") + ) + db.session.commit() + + self.login(ADMIN_USERNAME) + rv = self._restore(table_uuid, target_uuid) + assert rv.status_code == 200, rv.data + db.session.expire_all() + + restore_tx = ( + db.session.query(ver_cls.transaction_id) + .filter(ver_cls.id == table_id) + .order_by(ver_cls.transaction_id.desc()) + .limit(1) + .scalar() + ) + rows = ( + db.session.connection() + .execute( + sa.select( + version_changes_table.c.kind, + version_changes_table.c.path, + ).where( + version_changes_table.c.transaction_id == restore_tx, + version_changes_table.c.entity_kind == "dataset", + version_changes_table.c.entity_id == table_id, + ) + ) + .all() + ) + paths = {tuple(row.path) for row in rows} + assert ("columns", added_name) in paths, ( + f"restore tx {restore_tx} did not emit removal record for " + f"the added-then-restored-away column {added_name!r}; " + f"observed paths={paths}" + ) + assert ("columns", removed_name) in paths, ( + f"restore tx {restore_tx} did not emit addition record for " + f"the deleted-then-restored column {removed_name!r}; " + f"observed paths={paths}" + ) + + def test_restore_returns_404_for_unknown_uuid(self) -> None: + self.login(ADMIN_USERNAME) + rv = self._restore( + "00000000-0000-0000-0000-000000000000", + "00000000-0000-0000-0000-000000000001", + ) + assert rv.status_code == 404 + + def test_restore_returns_404_for_unknown_version_uuid(self) -> None: + _persist_fixture_state() + table: SqlaTable = ( + db.session.query(SqlaTable) + .filter(SqlaTable.table_name == "birth_names") + .first() + ) + assert table is not None + self.login(ADMIN_USERNAME) + rv = self._restore(str(table.uuid), "00000000-0000-0000-0000-000000000099") + assert rv.status_code == 404 + + def test_restore_returns_400_for_invalid_entity_uuid(self) -> None: + self.login(ADMIN_USERNAME) + rv = self._restore("not-a-uuid", "00000000-0000-0000-0000-000000000001") + assert rv.status_code == 400 + + def test_restore_returns_400_for_invalid_version_uuid(self) -> None: + _persist_fixture_state() + table: SqlaTable = ( + db.session.query(SqlaTable) + .filter(SqlaTable.table_name == "birth_names") + .first() + ) + assert table is not None + self.login(ADMIN_USERNAME) + rv = self._restore(str(table.uuid), "not-a-uuid") + assert rv.status_code == 400 + + def test_get_version_returns_historical_snapshot_with_children(self) -> None: + """GET /versions/<uuid>/ on a dataset returns scalar fields and + reconstructed columns/metrics, without modifying live state.""" + from superset.daos.version import derive_version_uuid + + _persist_fixture_state() + table: SqlaTable = ( + db.session.query(SqlaTable) + .filter(SqlaTable.table_name == "birth_names") + .first() + ) + assert table is not None + table_id = table.id + table_uuid = str(table.uuid) + entity_uuid = table.uuid + original_description = table.description + original_col_names = sorted(c.column_name for c in table.columns) + + # Capture a snapshot point now; make a change after. + ver_cls = version_class(SqlaTable) + target_tx = ( + db.session.query(ver_cls.transaction_id) + .filter(ver_cls.id == table_id) + .order_by(ver_cls.transaction_id.desc()) + .limit(1) + .scalar() + ) + assert target_tx is not None + target_uuid = str(derive_version_uuid(entity_uuid, target_tx)) + + table.description = "edited after snapshot" + db.session.commit() + + self.login(ADMIN_USERNAME) + rv = self.client.get(f"/api/v1/dataset/{table_uuid}/versions/{target_uuid}/") + assert rv.status_code == 200, rv.data + body = _json.loads(rv.data.decode("utf-8"))["result"] + + # Scalar fields reflect the snapshot, not the live edit. + assert body["description"] == original_description + assert body["_version"]["version_uuid"] == target_uuid + + # Columns list matches original set. + snapshot_col_names = sorted(c["column_name"] for c in body["columns"]) + assert snapshot_col_names == original_col_names + + # Metrics reconstructed. + assert isinstance(body["metrics"], list) + assert all("metric_name" in m for m in body["metrics"]) + + # Live row remains in its edited state. + db.session.expire_all() + live = db.session.query(SqlaTable).filter(SqlaTable.id == table_id).one() + assert live.description == "edited after snapshot" + + # Cleanup + live.description = original_description + db.session.commit() + + def test_put_response_returns_old_and_new_version_numbers(self) -> None: + """PUT /api/v1/dataset/<id> should include old_version and new_version + fields that match the list-versions endpoint's version_number values.""" + _persist_fixture_state() + table: SqlaTable = ( + db.session.query(SqlaTable) + .filter(SqlaTable.table_name == "birth_names") + .first() + ) + assert table is not None + table_id = table.id + original_description = table.description + + ver_cls = version_class(SqlaTable) + count_before = db.session.query(ver_cls).filter(ver_cls.id == table_id).count() + expected_old = count_before - 1 if count_before > 0 else None + + self.login(ADMIN_USERNAME) + rv = self.client.put( + f"/api/v1/dataset/{table_id}", + json={"description": "version-number response test"}, + ) + assert rv.status_code == 200, rv.data + body = _json.loads(rv.data.decode("utf-8")) + assert body["id"] == table_id + assert "old_version" in body + assert "new_version" in body + assert "old_transaction_id" in body + assert "new_transaction_id" in body + assert body["old_version"] == expected_old + # new_version points to the live row post-commit. It is usually + # old_version + 1, but can equal old_version when retention pruning + # removed an older closed row in the same commit. + assert body["new_version"] is not None + assert body["new_version"] >= 0 + # Transaction ids are stable identifiers, so a successful update + # always produces a new_transaction_id distinct from the previous + # one (when old_transaction_id is known). + if body["old_transaction_id"] is not None: + assert body["new_transaction_id"] != body["old_transaction_id"] + + # Cleanup + table = db.session.query(SqlaTable).filter(SqlaTable.id == table_id).one() + table.description = original_description + db.session.commit() + + def test_restore_denies_without_write_permission(self) -> None: + """Gamma is read-only on Dataset — 403 on restore.""" + _persist_fixture_state() + table: SqlaTable = ( + db.session.query(SqlaTable) + .filter(SqlaTable.table_name == "birth_names") + .first() + ) + assert table is not None + table_uuid = str(table.uuid) + + self.login(GAMMA_USERNAME) + rv = self._restore(table_uuid, "00000000-0000-0000-0000-000000000001") + assert rv.status_code == 403 Review Comment: Taken in `c65d09f94e`. Agreed the bogus `version_uuid` makes a mutation implausible here, but it costs one line and stops the test from silently degrading if the fixture ever changes. -- 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]
