This is an automated email from the ASF dual-hosted git repository.
rebenitez1802 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 8b9767ff263 fix(versioning): retryable error when a dashboard restore
races a chart purge (#45551)
8b9767ff263 is described below
commit 8b9767ff2630853f384de18756201c0f26071ad7
Author: Mike Bridge <[email protected]>
AuthorDate: Fri Oct 23 18:09:47 2026 -0600
fix(versioning): retryable error when a dashboard restore races a chart
purge (#45551)
Co-authored-by: Claude Opus 5.5 <[email protected]>
---
docs/docs/using-superset/version-history.mdx | 3 +
docs/static/resources/openapi.json | 15 ++
superset/commands/dashboard/restore_version.py | 42 ++++++
superset/dashboards/api.py | 12 ++
superset/translations/messages.pot | 3 +
superset/versioning/api_helpers.py | 8 +
superset/versioning/db_errors.py | 68 +++++++--
superset/versioning/restore.py | 84 ++++++++++-
.../dashboards/version_restore_tests.py | 168 +++++++++++++++++++++
tests/unit_tests/versioning/test_db_errors.py | 65 +++++++-
tests/unit_tests/versioning/test_restore.py | 167 +++++++++++++++++++-
11 files changed, 608 insertions(+), 27 deletions(-)
diff --git a/docs/docs/using-superset/version-history.mdx
b/docs/docs/using-superset/version-history.mdx
index ea2b4b06dee..9e9073aa90c 100644
--- a/docs/docs/using-superset/version-history.mdx
+++ b/docs/docs/using-superset/version-history.mdx
@@ -288,6 +288,9 @@ worth knowing:
version, so restoring one from the archive later puts it back on that
dashboard. An archived chart that was not in the version is unlinked, so
restoring it from the archive later does not put it back on this dashboard.
+ If a chart the restore is putting back or unlinking is permanently deleted
+ while the restore runs, nothing is changed and Superset asks you to try
+ again; the retry restores the version without that chart.
Restoring is offered to the same users who may edit the entity, and is withheld
from externally managed entities, whose source of truth lives outside Superset
diff --git a/docs/static/resources/openapi.json
b/docs/static/resources/openapi.json
index e6579ed648b..85f6d92351e 100644
--- a/docs/static/resources/openapi.json
+++ b/docs/static/resources/openapi.json
@@ -21419,6 +21419,21 @@
"404": {
"$ref": "#/components/responses/404"
},
+ "409": {
+ "content": {
+ "application/json": {
+ "schema": {
+ "properties": {
+ "message": {
+ "type": "string"
+ }
+ },
+ "type": "object"
+ }
+ }
+ },
+ "description": "A chart the restore was linking or unlinking was
permanently deleted while the restore ran. Nothing was changed; retry the same
request."
+ },
"422": {
"$ref": "#/components/responses/422"
}
diff --git a/superset/commands/dashboard/restore_version.py
b/superset/commands/dashboard/restore_version.py
index 07e1c546362..ca1285ab957 100644
--- a/superset/commands/dashboard/restore_version.py
+++ b/superset/commands/dashboard/restore_version.py
@@ -18,13 +18,23 @@
from __future__ import annotations
+import logging
+
+import sqlalchemy as sa
+from sqlalchemy.exc import SQLAlchemyError
+
from superset.commands.dashboard.exceptions import (
DashboardForbiddenError,
DashboardNotFoundError,
DashboardUpdateFailedError,
)
from superset.commands.version_restore import BaseRestoreVersionCommand
+from superset.extensions import db
from superset.models.dashboard import Dashboard
+from superset.models.slice import Slice
+from superset.versioning.restore import ChartPurgedDuringRestoreError,
RestoreResult
+
+logger: logging.Logger = logging.getLogger(__name__)
class RestoreDashboardVersionCommand(BaseRestoreVersionCommand):
@@ -43,3 +53,35 @@ class
RestoreDashboardVersionCommand(BaseRestoreVersionCommand):
not_found_exc = DashboardNotFoundError
forbidden_exc = DashboardForbiddenError
failed_exc = DashboardUpdateFailedError
+
+ def run(self) -> RestoreResult:
+ """Restore, confirming a reported chart-purge race before surfacing it.
+
+ The transaction has rolled back by the time the race reaches here,
+ so a fresh read sees the purge's commit. Only when one of the charts
+ whose membership the restore was changing is really gone is the
+ retryable :class:`ChartPurgedDuringRestoreError` raised. Otherwise,
+ or when the confirming read itself fails, the restore fails the
+ ordinary way.
+ """
+ try:
+ return super().run()
+ except ChartPurgedDuringRestoreError as ex:
+ slices: sa.Table = Slice.__table__
+ try:
+ remaining: set[int] = set(
+ db.session.execute(
+
sa.select(slices.c.id).where(slices.c.id.in_(ex.chart_ids))
+ ).scalars()
+ )
+ except SQLAlchemyError as read_ex:
+ raise self.failed_exc() from read_ex
+ if remaining.issuperset(ex.chart_ids):
+ raise self.failed_exc() from ex
+ logger.info(
+ "versioning: dashboard uuid=%s restore lost a race with a
purge "
+ "of chart(s) %s",
+ self._uuid,
+ sorted(set(ex.chart_ids) - remaining),
+ )
+ raise
diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py
index 28c74d170f4..e2335c2d868 100644
--- a/superset/dashboards/api.py
+++ b/superset/dashboards/api.py
@@ -3713,6 +3713,18 @@ class DashboardRestApi(
$ref: '#/components/responses/403'
404:
$ref: '#/components/responses/404'
+ 409:
+ description: >-
+ A chart the restore was linking or unlinking was permanently
+ deleted while the restore ran. Nothing was changed; retry the
+ same request.
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ message:
+ type: string
422:
$ref: '#/components/responses/422'
"""
diff --git a/superset/translations/messages.pot
b/superset/translations/messages.pot
index 8aad0b9bb2e..072e349f28c 100644
--- a/superset/translations/messages.pot
+++ b/superset/translations/messages.pot
@@ -899,6 +899,9 @@ msgstr ""
msgid "A candlestick chart shows opening, closing, high, and low values for
each period. It is commonly used for financial and other time-series range
analysis."
msgstr ""
+msgid "A chart on this dashboard or in this version was permanently deleted
while the dashboard was being restored, so nothing was changed. Please try
again."
+msgstr ""
+
msgid "A comma separated list of columns that should be parsed as dates"
msgstr ""
diff --git a/superset/versioning/api_helpers.py
b/superset/versioning/api_helpers.py
index e81e964289d..f50529a3489 100644
--- a/superset/versioning/api_helpers.py
+++ b/superset/versioning/api_helpers.py
@@ -506,6 +506,7 @@ def restore_version_endpoint(
# Deferred: restore.py pulls the model/versioning graph (same
# bootstrap-cycle rationale as this module's other local imports).
from superset.versioning.restore import (
+ ChartPurgedDuringRestoreError,
MissingDatasourceError,
PrunedChildHistoryError,
RecycledChildIdentityError,
@@ -530,6 +531,13 @@ def restore_version_endpoint(
# command's @transaction untouched (on_error re-raises
# non-SQLAlchemy exceptions as-is).
return api.response_422(message=str(ex))
+ except ChartPurgedDuringRestoreError as ex:
+ # A chart the restore was linking or unlinking was purged concurrently;
+ # the restore rolled back. Retrying is safe: the purge also removes the
+ # chart's membership history, so the retry no longer lists that chart
+ # as a snapshot member or in skipped_slice_ids. Return the retryable
+ # 409 rather than a 422. The exception's message is user-facing.
+ return api.response(409, message=str(ex))
except command_cls.failed_exc as ex:
logger.exception("Error restoring %s version", model_cls.__name__)
return api.response_422(message=str(ex))
diff --git a/superset/versioning/db_errors.py b/superset/versioning/db_errors.py
index c2572f89745..786e525f34b 100644
--- a/superset/versioning/db_errors.py
+++ b/superset/versioning/db_errors.py
@@ -53,6 +53,25 @@ _LOCK_CONTENTION_PHRASES: tuple[str, ...] = (
)
+def _driver_diagnostic(exc: BaseException | None) -> object | None:
+ """The driver error that classifies *exc*, or ``None`` when there is none.
+
+ A SQLAlchemy wrapper is classified on the driver error it preserves. A
+ wrapper that preserved none carries no diagnostic: its own text is the
+ rendered statement and bound parameters, which classification must never
+ read, so it is unclassifiable. A raw driver exception is its own
+ diagnostic.
+ """
+ if exc is None:
+ return None
+ orig: object = getattr(exc, "orig", None)
+ if orig is not None:
+ return orig
+ if isinstance(exc, DBAPIError):
+ return None
+ return exc
+
+
def is_lock_contention_error(exc: BaseException | None) -> bool:
"""Whether *exc* is a database deadlock / lock-wait failure.
@@ -77,22 +96,10 @@ def is_lock_contention_error(exc: BaseException | None) ->
bool:
with no ``__cause__``) and errors with empty driver args without
raising.
"""
- if exc is None:
+ diagnostic: object | None = _driver_diagnostic(exc)
+ if diagnostic is None:
return False
- orig: object = getattr(exc, "orig", None)
- if orig is None:
- if isinstance(exc, DBAPIError):
- # A wrapper whose driver error was not preserved carries no
- # diagnostic to classify -- and its own text is the
- # statement-and-parameters rendering this function must not
- # read. Unclassifiable is not retryable.
- return False
- # Not a wrapper: a raw driver exception is its own diagnostic.
- diagnostic: object = exc
- else:
- diagnostic = orig
-
args: tuple[object, ...] | None = getattr(diagnostic, "args", None)
if args and isinstance(args[0], int):
# MySQL-family errno: structured and therefore decisive.
@@ -109,6 +116,39 @@ def is_lock_contention_error(exc: BaseException | None) ->
bool:
return any(phrase in text for phrase in _LOCK_CONTENTION_PHRASES)
+#: MySQL/MariaDB errnos for a foreign key violation: adding a child row whose
+#: parent is missing (1452, and 1216 from older servers).
+_MYSQL_FOREIGN_KEY_VIOLATION: tuple[int, ...] = (1452, 1216)
+
+#: PostgreSQL SQLSTATE: foreign_key_violation.
+_PG_FOREIGN_KEY_VIOLATION: str = "23503"
+
+
+def is_foreign_key_violation(exc: BaseException | None) -> bool:
+ """Whether *exc* is a database foreign key violation.
+
+ Like :func:`is_lock_contention_error`, this reads the driver diagnostic,
+ never the SQLAlchemy wrapper's rendered statement and parameters, and a
+ structured MySQL errno or PostgreSQL SQLSTATE is decisive. Only SQLite,
+ which carries no code, is matched on its own message.
+ """
+ diagnostic: object | None = _driver_diagnostic(exc)
+ if diagnostic is None:
+ return False
+
+ args: tuple[object, ...] | None = getattr(diagnostic, "args", None)
+ if args and isinstance(args[0], int):
+ return args[0] in _MYSQL_FOREIGN_KEY_VIOLATION
+
+ sqlstate: str | None = getattr(diagnostic, "pgcode", None) or getattr(
+ diagnostic, "sqlstate", None
+ )
+ if sqlstate is not None:
+ return sqlstate == _PG_FOREIGN_KEY_VIOLATION
+
+ return "foreign key constraint failed" in str(diagnostic).lower()
+
+
#: PostgreSQL SQLSTATE for "relation does not exist" (undefined_table).
_PG_UNDEFINED_TABLE = "42P01"
diff --git a/superset/versioning/restore.py b/superset/versioning/restore.py
index d1e42f1f9c9..be4c9a30811 100644
--- a/superset/versioning/restore.py
+++ b/superset/versioning/restore.py
@@ -32,6 +32,7 @@ reported as skipped rather than revived or dangling.
from __future__ import annotations
import logging
+import re
from collections.abc import Sequence
from dataclasses import dataclass, field
from datetime import datetime
@@ -40,11 +41,14 @@ from uuid import UUID
import sqlalchemy as sa
from flask_babel import gettext as _
+from sqlalchemy.exc import IntegrityError
from sqlalchemy.orm.attributes import set_committed_value
+from sqlalchemy.orm.exc import StaleDataError
from sqlalchemy_continuum import version_class
from superset.extensions import db
from superset.versioning.baseline import OPERATION_DELETE, OPERATION_INSERT
+from superset.versioning.db_errors import is_foreign_key_violation
from superset.versioning.queries import find_active_by_uuid
from superset.versioning.utils import single_flush_scope
@@ -137,6 +141,30 @@ class RecycledChildIdentityError(Exception):
)
+class ChartPurgedDuringRestoreError(Exception):
+ """A chart whose dashboard membership the restore was changing was
+ permanently deleted before the restore could write the change.
+
+ The purge commits between the restore's read of the dashboard's charts
+ and its write. Reattaching the chart then fails on the foreign key, and
+ unlinking it finds its ``dashboard_slices`` row already gone; either way
+ the whole restore rolls back. Retrying is safe: the next attempt no
+ longer finds the chart. ``chart_ids`` are the charts whose membership the
+ restore was changing, so the caller can confirm, after the rollback,
+ that one of them is gone. The message is user-facing.
+ """
+
+ def __init__(self, chart_ids: Sequence[int]) -> None:
+ super().__init__(
+ _(
+ "A chart on this dashboard or in this version was permanently "
+ "deleted while the dashboard was being restored, so nothing
was "
+ "changed. Please try again."
+ )
+ )
+ self.chart_ids: list[int] = sorted(chart_ids)
+
+
class MissingDatasourceError(Exception):
"""The chart version's datasource no longer exists.
@@ -673,20 +701,25 @@ def restore_version(
chart_datasource: Any | None = _lock_chart_datasource(model_cls,
target_version)
skipped_slice_ids: list[int] = []
+ changed_slice_ids: list[int] = []
+ # Read before the flush: after a failed flush, touching an expired
+ # attribute raises PendingRollbackError instead of the original error.
+ entity_id: int = entity.id
try:
with single_flush_scope(db.session):
target_version.revert(relations=relations)
_sync_chart_perms(entity, chart_datasource)
if model_cls.__name__ == "Dashboard":
- skipped_slice_ids = _restore_dashboard_membership(
+ skipped_slice_ids, changed_slice_ids =
_restore_dashboard_membership(
entity, transaction_id
)
_stamp_audit_fields_for_restore(entity)
- except Exception:
+ except Exception as ex:
+ _raise_for_chart_purge_race(ex, changed_slice_ids)
logger.exception(
"Continuum revert() failed for %s id=%s tx=%s relations=%s",
model_cls.__name__,
- entity.id,
+ entity_id,
transaction_id,
relations,
)
@@ -703,7 +736,40 @@ def restore_version(
return RestoreResult(entity=entity, skipped_slice_ids=skipped_slice_ids)
-def _restore_dashboard_membership(dashboard: Any, transaction_id: int) ->
list[int]:
+#: ``dashboard_slices`` as a whole word, so ``dashboard_slices_version`` (the
+#: association's history table) does not match.
+_DASHBOARD_SLICES_PATTERN: re.Pattern[str] =
re.compile(r"\bdashboard_slices\b")
+
+
+def _raise_for_chart_purge_race(ex: Exception, changed_slice_ids: list[int])
-> None:
+ """Raise :class:`ChartPurgedDuringRestoreError` when *ex* is the
+ restore's write to ``dashboard_slices`` failing because a chart it was
+ linking or unlinking has been purged.
+
+ Linking a purged chart fails the row's foreign key; unlinking one finds
+ its row already deleted by the purge, so the delete matches no row.
+ Only failures of the ``dashboard_slices`` write itself are candidates,
+ and the caller still confirms, once the transaction has rolled back,
+ that one of the charts is gone. Any other failure returns, so it is
+ logged and re-raised as before.
+ """
+ if not changed_slice_ids:
+ return
+ if isinstance(ex, IntegrityError):
+ candidate: bool = is_foreign_key_violation(ex) and bool(
+ _DASHBOARD_SLICES_PATTERN.search(ex.statement or "")
+ )
+ elif isinstance(ex, StaleDataError):
+ candidate = _DASHBOARD_SLICES_PATTERN.search(str(ex)) is not None
+ else:
+ candidate = False
+ if candidate:
+ raise ChartPurgedDuringRestoreError(changed_slice_ids) from ex
+
+
+def _restore_dashboard_membership(
+ dashboard: Any, transaction_id: int
+) -> tuple[list[int], list[int]]:
"""Reset *dashboard*'s chart membership to what it was at
*transaction_id*, reattaching only charts that still exist.
@@ -719,8 +785,10 @@ def _restore_dashboard_membership(dashboard: Any,
transaction_id: int) -> list[i
``end_transaction_id`` the validity backfill does close (sc-119907).
Returns the ids of snapshot members that no longer exist and were
- skipped. Live charts' content is never touched — restoring a chart's
- content is the chart's own restore endpoint's job.
+ skipped, and the ids of charts whose membership the restore changes:
+ those it attaches and those it unlinks. Live charts' content is never
+ touched — restoring a chart's content is the chart's own restore
+ endpoint's job.
An archived (soft-deleted) chart still exists: it stays a recoverable
member, so its membership follows the snapshot like any other chart's.
@@ -779,6 +847,8 @@ def _restore_dashboard_membership(dashboard: Any,
transaction_id: int) -> list[i
dashboard.slices = live_slices
live_ids = {slc.id for slc in live_slices}
skipped = sorted(set(member_ids) - live_ids)
+ current_ids: set[int] = {slc.id for slc in current_slices}
+ changed: list[int] = sorted(live_ids ^ current_ids)
if skipped:
logger.warning(
"versioning: dashboard id=%s restore to tx=%s skipped %d "
@@ -788,7 +858,7 @@ def _restore_dashboard_membership(dashboard: Any,
transaction_id: int) -> list[i
len(skipped),
skipped,
)
- return skipped
+ return skipped, changed
def _stamp_audit_fields_for_restore(entity: Any) -> None:
diff --git a/tests/integration_tests/dashboards/version_restore_tests.py
b/tests/integration_tests/dashboards/version_restore_tests.py
index 3974d31c235..6fdda1cfadc 100644
--- a/tests/integration_tests/dashboards/version_restore_tests.py
+++ b/tests/integration_tests/dashboards/version_restore_tests.py
@@ -25,6 +25,7 @@ documented 404s for unknown entity/version UUIDs.
from __future__ import annotations
from typing import Any
+from unittest.mock import patch
from uuid import UUID
import pytest
@@ -858,3 +859,170 @@ class
TestDashboardRestoreLayoutMembership(SupersetTestCase, InsertChartMixin):
db.session.delete(table)
db.session.delete(database)
db.session.commit()
+
+
+class TestDashboardRestoreConcurrentPurge(SupersetTestCase, InsertChartMixin):
+ """A restore racing a purge of a chart it reattaches fails cleanly.
+
+ The purge commits on its own connection after the restore has read the
+ charts it will reattach and before it writes them, the window a real
+ concurrent purge can land in.
+ """
+
+ @parameterized.expand(
+ [
+ ("reattach_purged", False, True),
+ ("unlink_purged", True, True),
+ ("unrelated_foreign_key_failure", False, False),
+ ]
+ )
+ def test_restore_conflicts_with_concurrent_chart_purge(
+ self, case: str, unlink: bool, purge: bool
+ ) -> None:
+ """Purging a chart the restore links or unlinks makes the restore a
+ retryable 409. A simulated foreign key failure injected by the audit
+ stamp while no such chart is missing stays the ordinary 422, because
+ retrying would not help."""
+ import sqlite3
+
+ from sqlalchemy.exc import IntegrityError
+ from sqlalchemy.orm import Session
+
+ from superset.commands.deletion_retention.purge_cascade import (
+ cascade_hard_delete,
+ suppress_purge_association_versions,
+ )
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.models.core import Database
+ from superset.models.helpers import skip_visibility_filter
+ from superset.models.slice import Slice
+ from superset.versioning import restore as restore_module
+
+ admin: User = self.get_user(ADMIN_USERNAME)
+ dataset: SqlaTable = SqlaTable(
+ table_name="restore_concurrent_purge",
+ database=Database(
+ database_name="restore_concurrent_purge",
sqlalchemy_uri="sqlite://"
+ ),
+ )
+ db.session.add(dataset)
+ db.session.commit()
+ kept: Slice = self.insert_chart("restore-purge-kept", [admin.id],
dataset.id)
+ purged: Slice = self.insert_chart(
+ "restore-purge-purged", [admin.id], dataset.id
+ )
+ kept_id: int = kept.id
+ purged_id: int = purged.id
+ dashboard_id: int | None = None
+ try:
+ self.login(ADMIN_USERNAME)
+ dashboard_id = self.client.post(
+ "/api/v1/dashboard/", json={"dashboard_title": "restore-purge"}
+ ).json["id"]
+ dashboard: Dashboard = db.session.get(Dashboard, dashboard_id)
+ dashboard_uuid: str = str(dashboard.uuid)
+ # The restore to version 1 reattaches the purged chart, or with
+ # ``unlink`` removes it from the live dashboard.
+ layouts: list[list[int]] = [[kept_id, purged_id], [kept_id]]
+ if unlink:
+ layouts.reverse()
+ chart_ids: list[int]
+ for chart_ids in layouts:
+ rv: TestResponse = self.client.put(
+ f"/api/v1/dashboard/{dashboard_id}",
+ json={
+ "json_metadata": _json.dumps(
+ {"positions": _chart_layout(chart_ids)}
+ )
+ },
+ )
+ assert rv.status_code == 200, rv.data
+ versions: list[dict[str, Any]] = self.client.get(
+ f"/api/v1/dashboard/{dashboard_uuid}/versions/"
+ ).json["result"]
+ version_uuid: str = next(
+ version["version_uuid"]
+ for version in versions
+ if version["version_number"] == 1
+ )
+ # Only an archived chart can be purged.
+ rv = self.client.delete(f"/api/v1/chart/{purged_id}")
+ assert rv.status_code == 200, rv.data
+ db.session.expire_all()
+ position_before: str = db.session.get(Dashboard,
dashboard_id).position_json
+ version_rows_before: int = len(
+ _get_version_rows(db.session.get(Dashboard, dashboard_id))
+ )
+
+ stamp_audit_fields: Any =
restore_module._stamp_audit_fields_for_restore
+
+ def purge_then_stamp(entity: Any) -> None:
+ if not purge:
+ raise IntegrityError(
+ "INSERT INTO dashboard_slices",
+ None,
+ sqlite3.IntegrityError("FOREIGN KEY constraint
failed"),
+ )
+ other: Session
+ with Session(bind=db.engine) as other:
+ with skip_visibility_filter(other, Slice):
+ chart: Slice = other.get(Slice, purged_id)
+ with suppress_purge_association_versions(other):
+ assert cascade_hard_delete(
+ other, chart, enforce_window=False,
require_archived=True
+ ).purged
+ other.commit()
+ stamp_audit_fields(entity)
+
+ restore_url: str = (
+
f"/api/v1/dashboard/{dashboard_uuid}/versions/{version_uuid}/restore"
+ )
+ with patch.object(
+ restore_module,
+ "_stamp_audit_fields_for_restore",
+ side_effect=purge_then_stamp,
+ ):
+ rv = self.client.post(restore_url)
+ if purge:
+ assert rv.status_code == 409, rv.data
+ assert "try again" in rv.json["message"], rv.json
+ else:
+ assert rv.status_code == 422, rv.data
+
+ # Nothing was restored.
+ db.session.expire_all()
+ dashboard = db.session.get(Dashboard, dashboard_id)
+ assert dashboard.position_json == position_before
+ assert {s.id for s in dashboard.slices} == {kept_id}
+ assert len(_get_version_rows(dashboard)) == version_rows_before
+
+ # Retrying restores the version: its layout comes back, without
+ # the purged chart as a member. The purge also removed the
+ # chart's history, so the version no longer lists it at all.
+ if purge:
+ rv = self.client.post(restore_url)
+ assert rv.status_code == 200, rv.data
+ db.session.expire_all()
+ dashboard = db.session.get(Dashboard, dashboard_id)
+ assert dashboard.position_json != position_before
+ assert {
+ node["meta"]["chartId"]
+ for node in dashboard.position.values()
+ if isinstance(node, dict) and node.get("type") == "CHART"
+ } == set(layouts[0])
+ assert {s.id for s in dashboard.slices} == {kept_id}
+ finally:
+ db.session.rollback()
+ if dashboard_id is not None:
+ db.session.delete(db.session.get(Dashboard, dashboard_id))
+ chart_id: int
+ leftover: Slice | None
+ with skip_visibility_filter(db.session, Slice):
+ for chart_id in (kept_id, purged_id):
+ if (leftover := db.session.get(Slice, chart_id)) is not
None:
+ db.session.delete(leftover)
+ table: SqlaTable = db.session.get(SqlaTable, dataset.id)
+ database: Database = table.database
+ db.session.delete(table)
+ db.session.delete(database)
+ db.session.commit()
diff --git a/tests/unit_tests/versioning/test_db_errors.py
b/tests/unit_tests/versioning/test_db_errors.py
index 30eea341797..de6bdb170fd 100644
--- a/tests/unit_tests/versioning/test_db_errors.py
+++ b/tests/unit_tests/versioning/test_db_errors.py
@@ -22,9 +22,16 @@ metadata database."""
from typing import Any
import pytest
-from sqlalchemy.exc import DataError, OperationalError, ProgrammingError,
StatementError
+from sqlalchemy.exc import (
+ DataError,
+ IntegrityError,
+ OperationalError,
+ ProgrammingError,
+ StatementError,
+)
from superset.versioning.db_errors import (
+ is_foreign_key_violation,
is_lock_contention_error,
is_missing_table_error,
)
@@ -308,3 +315,59 @@ def test_lock_contention_negative(orig: BaseException |
None) -> None:
def test_lock_contention_none_input() -> None:
assert is_lock_contention_error(None) is False
+
+
+def _integrity_error(orig: BaseException | None) -> IntegrityError:
+ err: IntegrityError = IntegrityError("stmt", None, Exception("boom"))
+ err.orig = orig
+ return err
+
+
[email protected](
+ "orig",
+ [
+ _FakeLockDriverError(
+ args=(1452, "Cannot add or update a child row: a foreign key
fails")
+ ),
+ _FakeLockDriverError(args=(1216, "Cannot add or update a child row")),
+ _FakeLockDriverError(pgcode="23503"),
+ _FakeLockDriverError(sqlstate="23503"),
+ _FakeLockDriverError(message="FOREIGN KEY constraint failed"),
+ ],
+)
+def test_foreign_key_violation_positive(orig: _FakeLockDriverError) -> None:
+ assert is_foreign_key_violation(_integrity_error(orig)) is True
+
+
[email protected](
+ "orig",
+ [
+ # Unique and primary key violations are integrity errors too, but
+ # not a missing parent row.
+ _FakeLockDriverError(args=(1062, "Duplicate entry")),
+ _FakeLockDriverError(pgcode="23505"),
+ _FakeLockDriverError(message="UNIQUE constraint failed:
dashboard_slices.id"),
+ # A structured code is decisive even when the message says otherwise.
+ _FakeLockDriverError(
+ args=(1062, "foreign key constraint failed"), sqlstate="23000"
+ ),
+ _FakeLockDriverError(pgcode="23505", message="foreign key constraint
failed"),
+ None,
+ ],
+)
+def test_foreign_key_violation_negative(orig: BaseException | None) -> None:
+ assert is_foreign_key_violation(_integrity_error(orig)) is False
+
+
+def test_foreign_key_phrase_in_sql_is_not_a_violation() -> None:
+ """The wrapper's rendered statement is never read, so a statement that
+ merely mentions the phrase is not classified as a violation."""
+ err: IntegrityError = IntegrityError(
+ "SELECT 'foreign key constraint failed'", None, Exception()
+ )
+ err.orig = None
+ assert is_foreign_key_violation(err) is False
+
+
+def test_foreign_key_violation_none_input() -> None:
+ assert is_foreign_key_violation(None) is False
diff --git a/tests/unit_tests/versioning/test_restore.py
b/tests/unit_tests/versioning/test_restore.py
index 28384c6bf45..24ec2e69649 100644
--- a/tests/unit_tests/versioning/test_restore.py
+++ b/tests/unit_tests/versioning/test_restore.py
@@ -17,22 +17,33 @@
"""Unit-level coverage for the version-restore engine control flow.
The happy path is exercised end-to-end in the per-entity integration
-suites (``version_restore_tests.py``); here we pin the cheap, DB-free
-guard branches with mocks: unknown entity, missing target transaction,
-DELETE-row target, unregistered model, and the ``single_flush_scope``
-flush contract.
+suites (``version_restore_tests.py``); here we pin guard branches with mocks:
+unknown entity, missing target transaction, DELETE-row target, unregistered
+model, and the ``single_flush_scope`` flush contract. An isolated in-memory
+SQLAlchemy session covers logging after a failed flush.
"""
from __future__ import annotations
+import logging
+import sqlite3
from typing import Any
from unittest.mock import MagicMock, patch
from uuid import UUID
import pytest
+import sqlalchemy as sa
+from sqlalchemy.exc import IntegrityError, PendingRollbackError
+from sqlalchemy.orm import registry, Session
+from sqlalchemy.orm.exc import StaleDataError
from superset.versioning.baseline import OPERATION_DELETE, OPERATION_INSERT
-from superset.versioning.restore import _child_state_provable_at,
restore_version
+from superset.versioning.restore import (
+ _child_state_provable_at,
+ _raise_for_chart_purge_race,
+ ChartPurgedDuringRestoreError,
+ restore_version,
+)
from superset.versioning.utils import single_flush_scope
_UUID = UUID("00000000-0000-0000-0000-000000000000")
@@ -119,6 +130,78 @@ def test_restore_version_rejects_entity_uuid_mismatch(
)
+def test_restore_logs_original_error_after_failed_flush(
+ caplog: pytest.LogCaptureFixture,
+) -> None:
+ """Logging an expired entity must preserve the non-race flush failure."""
+ mapper_registry: registry = registry()
+ engine: sa.Engine = sa.create_engine("sqlite:///:memory:")
+
+ class Entity:
+ """A minimal persisted entity whose id expires when a flush fails."""
+
+ id: int
+ uuid: UUID = _UUID
+ name: str | None
+
+ table: sa.Table = sa.Table(
+ "restore_log_entity",
+ mapper_registry.metadata,
+ sa.Column("id", sa.Integer, primary_key=True),
+ sa.Column("name", sa.String, nullable=False),
+ )
+ mapper_registry.map_imperatively(Entity, table)
+ mapper_registry.metadata.create_all(engine)
+ try:
+ with Session(engine) as session:
+ entity: Entity = Entity()
+ entity.id = 7
+ entity.name = "before"
+ session.add(entity)
+ session.commit()
+ target: MagicMock = MagicMock()
+
+ def revert(*, relations: list[str]) -> None:
+ """Trigger a real NOT NULL violation in the final flush."""
+ entity.name = None
+
+ target.revert.side_effect = revert
+ with (
+ patch("superset.versioning.restore.db") as mock_db,
+ patch(
+ "superset.versioning.restore._find_target_version",
+ return_value=target,
+ ),
+ patch(
+
"superset.versioning.restore._restore_dashboard_membership",
+ return_value=([], []),
+ ),
+
patch("superset.versioning.restore._stamp_audit_fields_for_restore"),
+ ):
+ mock_db.session = session
+ with pytest.raises(IntegrityError) as failure:
+ restore_version(
+ MagicMock(__name__="Dashboard"), _UUID, 123,
entity=entity
+ )
+
+ assert sa.inspect(entity).expired
+ with pytest.raises(PendingRollbackError):
+ assert entity.id == 7
+ records: list[logging.LogRecord] = [
+ record
+ for record in caplog.records
+ if record.name == "superset.versioning.restore"
+ and record.getMessage().startswith("Continuum revert() failed")
+ ]
+ assert len(records) == 1
+ assert "Dashboard id=7 tx=123" in records[0].getMessage()
+ assert records[0].exc_info is not None
+ assert records[0].exc_info[1] is failure.value
+ finally:
+ mapper_registry.dispose()
+ engine.dispose()
+
+
def test_single_flush_scope_flushes_once_on_clean_exit() -> None:
session = MagicMock()
with single_flush_scope(session):
@@ -502,3 +585,77 @@ def test_restore_endpoint_maps_missing_datasource_to_422(
assert response == "resp-422"
api.response_422.assert_called_once_with(message=expected_message)
+
+
+def _foreign_key_failure(statement: str) -> IntegrityError:
+ return IntegrityError(
+ statement, None, sqlite3.IntegrityError("FOREIGN KEY constraint
failed")
+ )
+
+
[email protected](
+ "error",
+ [
+ pytest.param(
+ _foreign_key_failure("INSERT INTO dashboard_slices (dashboard_id)
..."),
+ id="link_foreign_key",
+ ),
+ pytest.param(
+ StaleDataError(
+ "DELETE statement on table 'dashboard_slices' expected to
delete "
+ "1 row(s); Only 0 were matched."
+ ),
+ id="unlink_stale",
+ ),
+ ],
+)
+def test_chart_purge_race_is_raised_for_dashboard_slices_writes(
+ error: Exception,
+) -> None:
+ with pytest.raises(ChartPurgedDuringRestoreError) as excinfo:
+ _raise_for_chart_purge_race(error, [3, 1])
+ assert excinfo.value.chart_ids == [1, 3]
+ assert excinfo.value.__cause__ is error
+
+
[email protected](
+ ("error", "changed_slice_ids"),
+ [
+ pytest.param(
+ _foreign_key_failure("INSERT INTO dashboard_slices (dashboard_id)
..."),
+ [],
+ id="no_membership_change",
+ ),
+ pytest.param(
+ _foreign_key_failure("UPDATE dashboards SET changed_by_fk=?"),
+ [1],
+ id="foreign_key_on_another_table",
+ ),
+ pytest.param(
+ _foreign_key_failure("INSERT INTO dashboard_slices_version
(slice_id)"),
+ [1],
+ id="history_table",
+ ),
+ pytest.param(
+ IntegrityError(
+ "INSERT INTO dashboard_slices (dashboard_id) ...",
+ None,
+ sqlite3.IntegrityError("UNIQUE constraint failed"),
+ ),
+ [1],
+ id="not_a_foreign_key_failure",
+ ),
+ pytest.param(
+ StaleDataError(
+ "UPDATE statement on table 'dashboards' expected to update 1
row(s)"
+ ),
+ [1],
+ id="stale_other_table",
+ ),
+ pytest.param(RuntimeError("boom"), [1], id="other_error"),
+ ],
+)
+def test_chart_purge_race_is_not_raised_for_other_failures(
+ error: Exception, changed_slice_ids: list[int]
+) -> None:
+ _raise_for_chart_purge_race(error, changed_slice_ids)
