This is an automated email from the ASF dual-hosted git repository.
rusackas 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 986b648f1ca fix(versioning): resolve dashboard M2M membership by
attach/detach windows, not end_transaction_id (#44010)
986b648f1ca is described below
commit 986b648f1ca2510584091f139a53b05279182bf0
Author: Mike Bridge <[email protected]>
AuthorDate: Tue Sep 8 18:59:04 2026 -0600
fix(versioning): resolve dashboard M2M membership by attach/detach windows,
not end_transaction_id (#44010)
Co-authored-by: Mike Bridge <[email protected]>
Co-authored-by: Claude Fable 5 <[email protected]>
---
superset/versioning/activity/impact.py | 152 ++++++++++++++-------
superset/versioning/activity/queries.py | 95 +------------
superset/versioning/activity/scope.py | 2 +-
superset/versioning/activity/windows.py | 66 +++++++++
superset/versioning/membership.py | 86 ++++++++++++
superset/versioning/restore.py | 50 ++++---
.../dashboards/version_restore_tests.py | 70 ++++++++++
tests/unit_tests/versioning/test_activity.py | 149 ++++++++++++++++++--
8 files changed, 487 insertions(+), 183 deletions(-)
diff --git a/superset/versioning/activity/impact.py
b/superset/versioning/activity/impact.py
index e4684c20b1a..f1c2b22b47e 100644
--- a/superset/versioning/activity/impact.py
+++ b/superset/versioning/activity/impact.py
@@ -24,9 +24,11 @@ request:
* :func:`collect_impact_pairs` — pulls the distinct
``(dataset_id, transaction_id)`` pairs that need counts.
-* :func:`batch_chart_counts` — one SQL query joining
- ``dashboard_slices_version`` and ``slices_version`` to count
- the matching charts validity-strategy-style.
+* :func:`batch_chart_counts` — counts the matching charts without a
+ join: dashboard membership comes from ``charts_attached_to_dashboard``'s
+ attach/detach windows over ``dashboard_slices_version``, and a
+ member-scoped ``slices_version`` scan supplies the chart→dataset window;
+ the two are combined per pair by :func:`_count_attached_charts_at`.
* :func:`impact_for_record` — pure projection from the pre-fetched
counts onto each record (returns ``None`` for non-Dashboard paths
or non-SqlaTable kinds, matching the ``impact`` computation).
@@ -38,6 +40,7 @@ inside another (no DB).
from __future__ import annotations
+from collections.abc import Mapping, Sequence
from typing import Any
import sqlalchemy as sa
@@ -47,7 +50,14 @@ from superset.versioning.activity.kinds import (
chunked_ids,
ENTITY_ID_CHUNK_SIZE,
TABLE_KIND_TO_API,
+ Window,
)
+from superset.versioning.baseline import OPERATION_DELETE
+
+# Headroom left below SQLite's 999 bind-variable floor for the handful of
scalar
+# binds in the slice-scan WHERE (datasource_type, operation_type, the two tx
+# bounds) once a member-id chunk and the dataset IN are accounted for.
+_SCALAR_BIND_HEADROOM = 20
def collect_impact_pairs(
@@ -76,12 +86,17 @@ def batch_chart_counts(
distinct charts that were both on *dashboard_id* and pointing at
*dataset_id* at *target_tx*.
- One SELECT against ``dashboard_slices_version`` ⨝ ``slices_version``,
- pulling the (slice, dataset, validity-window) state for every slice
- ever on the dashboard whose dataset matches one of the requested
- dataset_ids. The Python loop then applies the validity-strategy
- predicate per pair. Replaces the previous N+1 shape that fired one
- COUNT per related record.
+ No join: ``charts_attached_to_dashboard`` supplies each member chart's
+ ``[attach, detach)`` windows from the association shadow (Continuum never
+ closes an M2M shadow's ``end_transaction_id``, so a validity-window filter
+ on it would count a chart removed before ``target_tx`` — sc-119907), and a
+ member-scoped scan of ``slices_version`` supplies the chart→dataset window,
+ whose ``end_transaction_id`` the validity backfill *does* close, so the
+ ordinary validity predicate is right there. The Python loop counts a slice
+ for a pair when both an attachment window and its chart→dataset window
+ contain ``target_tx``. Replaces the previous N+1 shape that fired one COUNT
+ per related record, and the m2m⋈slices join whose M2M validity window was
+ the buggy naive filter.
Returns ``{(dataset_id, target_tx): count}``; pairs whose count
would be zero are omitted so the caller's ``.get(key, 0)`` is
@@ -94,73 +109,104 @@ def batch_chart_counts(
from sqlalchemy_continuum import version_class
from superset.models.slice import Slice
+ from superset.versioning.membership import charts_attached_to_dashboard
- metadata = version_class(Slice).__table__.metadata
- m2m_tbl = metadata.tables.get("dashboard_slices_version")
slices_tbl = version_class(Slice).__table__
- if m2m_tbl is None:
- return {}
dataset_ids: set[int] = {dataset_id for dataset_id, _ in pairs}
- # Bound both validity windows to the transaction range the page-set
- # needs. Both the attachment (m2m) and the chart→dataset (slice) window
- # must straddle a requested target_tx, so a row whose window starts
- # after the newest target, or closes at/before the oldest one, can
- # never contribute a match. Without this the join multiplies every
- # attachment row by the full slice version history — an unbounded cross
- # product on dashboards with long-lived, frequently-edited charts.
target_txs: set[int] = {target_tx for _, target_tx in pairs}
min_tx, max_tx = min(target_txs), max(target_txs)
- # Chunk the datasource_id IN-clause to stay under SQLite's bind-variable
- # floor (a dashboard pointing at very many datasets can exceed it).
- rows: list[Any] = []
- for chunk in chunked_ids(dataset_ids, ENTITY_ID_CHUNK_SIZE):
- stmt = sa.select(
- m2m_tbl.c.slice_id,
- slices_tbl.c.datasource_id,
- m2m_tbl.c.transaction_id.label("m2m_start"),
- m2m_tbl.c.end_transaction_id.label("m2m_end"),
- slices_tbl.c.transaction_id.label("slice_start"),
- slices_tbl.c.end_transaction_id.label("slice_end"),
- ).where(
- m2m_tbl.c.dashboard_id == dashboard_id,
- m2m_tbl.c.operation_type != 2,
- slices_tbl.c.id == m2m_tbl.c.slice_id,
- slices_tbl.c.datasource_id.in_(chunk),
+
+ # Attachment membership per slice. charts_attached_to_dashboard owns the
+ # association-shadow read and the attach/detach window pairing — the single
+ # place that must never filter the M2M shadow by end_transaction_id, which
+ # Continuum never closes (sc-119907). Reused here so restore.py, the
+ # activity relationship walk, and this rollup share one implementation.
+ attach_windows: dict[int, list[Window]] = {}
+ for slice_id, window in charts_attached_to_dashboard(dashboard_id):
+ attach_windows.setdefault(slice_id, []).append(window)
+ if not attach_windows:
+ return {}
+
+ # Chart→dataset validity from the slice parent shadow, whose
+ # end_transaction_id the validity backfill *does* close, so the ordinary
+ # half-open validity predicate is correct here. Bounded on the DB side to
+ # this dashboard's member charts (the attach_windows keys) and the
+ # transaction range; the member-id IN-clause is chunked to stay under
+ # SQLite's 999 bind-variable floor.
+ #
+ # The requested-dataset prune is applied on the DB side too, but only when
+ # the dataset set co-binds with a full member chunk under that floor — a
+ # member chunk (<= ENTITY_ID_CHUNK_SIZE) plus the dataset IN plus the few
+ # scalar binds must stay < 999. When there are too many requested datasets,
+ # the DB-side dataset predicate is dropped and the combiner filters
datasets
+ # in Python (it already keys on pairs_by_dataset), so a wide dashboard does
+ # not overflow the bind limit (sc-119907 review).
+ filter_datasets_in_sql = (
+ len(dataset_ids) <= 999 - ENTITY_ID_CHUNK_SIZE - _SCALAR_BIND_HEADROOM
+ )
+ slice_rows: list[Any] = []
+ for chunk in chunked_ids(set(attach_windows), ENTITY_ID_CHUNK_SIZE):
+ conditions = [
+ slices_tbl.c.id.in_(chunk),
slices_tbl.c.datasource_type == "table",
- slices_tbl.c.operation_type != 2,
- m2m_tbl.c.transaction_id <= max_tx,
- sa.or_(
- m2m_tbl.c.end_transaction_id.is_(None),
- m2m_tbl.c.end_transaction_id > min_tx,
- ),
+ slices_tbl.c.operation_type != OPERATION_DELETE,
slices_tbl.c.transaction_id <= max_tx,
sa.or_(
slices_tbl.c.end_transaction_id.is_(None),
slices_tbl.c.end_transaction_id > min_tx,
),
- )
- rows.extend(db.session.connection().execute(stmt).mappings().all())
+ ]
+ if filter_datasets_in_sql:
+ conditions.append(slices_tbl.c.datasource_id.in_(dataset_ids))
+ stmt = sa.select(
+ slices_tbl.c.id.label("slice_id"),
+ slices_tbl.c.datasource_id,
+ slices_tbl.c.transaction_id.label("slice_start"),
+ slices_tbl.c.end_transaction_id.label("slice_end"),
+ ).where(*conditions)
+
slice_rows.extend(db.session.connection().execute(stmt).mappings().all())
- # For each pair, collect the slice_ids whose two validity windows
- # both straddle target_tx. ``set`` dedupes within a pair.
- matches: dict[tuple[int, int], set[int]] = {}
pairs_by_dataset: dict[int, list[int]] = {}
for dataset_id, target_tx in pairs:
pairs_by_dataset.setdefault(dataset_id, []).append(target_tx)
- for row in rows:
+ return _count_attached_charts_at(attach_windows, slice_rows,
pairs_by_dataset)
+
+
+def _count_attached_charts_at(
+ attach_windows: dict[int, list[Window]],
+ slice_rows: Sequence[Mapping[str, Any]],
+ pairs_by_dataset: dict[int, list[int]],
+) -> dict[tuple[int, int], int]:
+ """Pure combiner: for each ``(dataset_id, target_tx)``, count the distinct
+ charts whose attachment window and chart→dataset window both contain
+ ``target_tx``.
+
+ *attach_windows* maps ``slice_id`` to its ``[attach, detach)`` episodes
+ (from the association shadow — see
+ :func:`~superset.versioning.activity.windows.attachment_windows`); a chart
+ removed before ``target_tx`` has no window containing it and is therefore
+ not counted. *slice_rows* are the chart→dataset parent-shadow rows
+ (``slice_id``, ``datasource_id``, ``slice_start``, ``slice_end``), whose
+ ``end_transaction_id`` (``slice_end``) the validity backfill does close, so
+ the half-open validity predicate is correct for them. Split out of
+ :func:`batch_chart_counts` so this membership logic is unit-testable
+ without a live shadow-table fixture.
+ """
+ matches: dict[tuple[int, int], set[int]] = {}
+ for row in slice_rows:
+ windows = attach_windows.get(row["slice_id"])
+ if not windows:
+ continue
ds_id = row["datasource_id"]
for target_tx in pairs_by_dataset.get(ds_id, ()):
- in_m2m = row["m2m_start"] <= target_tx and (
- row["m2m_end"] is None or row["m2m_end"] > target_tx
- )
+ in_attach = any(w.contains(target_tx) for w in windows)
in_slice = row["slice_start"] <= target_tx and (
row["slice_end"] is None or row["slice_end"] > target_tx
)
- if in_m2m and in_slice:
+ if in_attach and in_slice:
matches.setdefault((ds_id, target_tx),
set()).add(row["slice_id"])
-
return {pair: len(slice_ids) for pair, slice_ids in matches.items()}
diff --git a/superset/versioning/activity/queries.py
b/superset/versioning/activity/queries.py
index 97fdf67a754..f87cb6033bf 100644
--- a/superset/versioning/activity/queries.py
+++ b/superset/versioning/activity/queries.py
@@ -16,8 +16,10 @@
# under the License.
"""DB-touching helpers for the activity-view read path.
-All Phase A relationship walks (``charts_attached_to_dashboard``,
-``datasets_used_by_chart``, ``batch_datasets_used_by_charts``),
+The Phase A relationship walks (``datasets_used_by_chart``,
+``batch_datasets_used_by_charts``; the dashboard-membership walk
+``charts_attached_to_dashboard`` lives in
+:mod:`superset.versioning.membership`),
the Phase B change-record fetch (``fetch_change_records`` /
``_select_change_rows_for_kinds``), the name-denormalization helpers
(``_resolve_names_for_kind`` / ``apply_entity_name_denormalization``), the
@@ -39,7 +41,6 @@ from __future__ import annotations
import logging
from datetime import datetime
from heapq import heappush, heapreplace
-from itertools import groupby
from typing import Any
from uuid import UUID
@@ -119,94 +120,6 @@ def first_tracked_tx(
# ---- Phase A: relationship-traversal queries ------------------------------
-# ``operation_type`` values on a Continuum association shadow row
-# (sqlalchemy_continuum.operation.Operation): INSERT attaches, DELETE detaches.
-# UPDATE never occurs for a pure M2M association (there is nothing to update on
-# a (dashboard, slice) pair); if it ever appeared it is ignored — neither
-# opening nor closing a window — so an open attachment simply continues.
-# These mirror the library enum's numeric values;
``test_m2m_op_constants_match_
-# continuum`` pins them so a Continuum renumber fails loudly rather than
silently.
-_M2M_OP_INSERT = 0
-_M2M_OP_DELETE = 2
-
-
-def _attachment_windows(
- rows: list[tuple[int, int, int]],
-) -> list[tuple[int, Window]]:
- """Pair INSERT / DELETE association-version rows into ``[attach, detach)``
- windows, one per attachment episode.
-
- Each row is ``(slice_id, transaction_id, operation_type)``. Continuum
- **never closes** an association shadow row's ``end_transaction_id`` — its
- unit-of-work only *inserts* association versions
- (``create_association_versions``); the validity backfill that sets
- ``end_transaction_id`` runs for parent objects, not for M2M links. So the
- detach boundary lives on the DELETE row's ``transaction_id``, not on the
- attach row's ``end_transaction_id`` (which stays NULL for the association's
- whole life). An INSERT opens a window; the next DELETE closes it at its
- transaction id; an attachment with no following DELETE stays open (the
- chart is still on the dashboard). A DELETE at the same transaction as its
- open (add-and-remove in one save) yields no window — the chart was never
- on a committed dashboard state.
- """
- result: list[tuple[int, Window]] = []
- # operation_type is part of the sort key so that, within one transaction,
- # INSERT (0) sorts before DELETE (2): an add-and-remove in a single save is
- # then seen open-before-close and collapses to no window (the DELETE finds
- # ``tx == open_tx``, not ``>``). Do not drop it from the key.
- rows_sorted = sorted(rows, key=lambda r: (r[0], r[1], r[2]))
- for slice_id, group in groupby(rows_sorted, key=lambda r: r[0]):
- open_tx: int | None = None
- for _slice_id, tx, operation_type in group:
- if operation_type == _M2M_OP_DELETE:
- if open_tx is not None and tx > open_tx:
- result.append((slice_id, Window(open_tx, tx)))
- open_tx = None
- elif operation_type == _M2M_OP_INSERT and open_tx is None:
- open_tx = tx
- if open_tx is not None:
- result.append((slice_id, Window(open_tx, None)))
- return result
-
-
-def charts_attached_to_dashboard(dashboard_id: int) -> list[tuple[int,
Window]]:
- """Return ``(slice_id, window)`` for every chart that has ever been on
- *dashboard_id*, with each attachment episode's validity window in
- transaction-id space.
-
- Reads from ``dashboard_slices_version`` (Continuum's auto-generated M2M
- shadow) and pairs its INSERT/DELETE rows via :func:`_attachment_windows`,
- so a chart removed from the dashboard is bounded at the detach transaction
- rather than open-ended — otherwise the chart's edits made *after* removal
- would surface in the dashboard's related history.
- """
- # pylint: disable=import-outside-toplevel
- from sqlalchemy_continuum import version_class
-
- from superset.models.dashboard import Dashboard
-
- metadata = version_class(Dashboard).__table__.metadata
- m2m_tbl = metadata.tables.get("dashboard_slices_version")
- if m2m_tbl is None:
- return []
-
- rows = (
- db.session.connection()
- .execute(
- sa.select(
- m2m_tbl.c.slice_id,
- m2m_tbl.c.transaction_id,
- m2m_tbl.c.operation_type,
- ).where(
- m2m_tbl.c.dashboard_id == dashboard_id,
- m2m_tbl.c.slice_id.is_not(None),
- )
- )
- .all()
- )
- return _attachment_windows([(row[0], row[1], row[2]) for row in rows])
-
-
def datasets_used_by_chart(slice_id: int) -> list[tuple[int, Window]]:
"""Return ``(datasource_id, window)`` for every dataset that *slice_id*
has ever pointed at, with each association's validity window.
diff --git a/superset/versioning/activity/scope.py
b/superset/versioning/activity/scope.py
index bc1d7912b5a..1975105a1a4 100644
--- a/superset/versioning/activity/scope.py
+++ b/superset/versioning/activity/scope.py
@@ -35,13 +35,13 @@ from __future__ import annotations
from superset.versioning.activity.kinds import EntityWindows, Window
from superset.versioning.activity.queries import (
batch_datasets_used_by_charts,
- charts_attached_to_dashboard,
datasets_used_by_chart,
)
from superset.versioning.activity.windows import (
intersect_windows,
merge_entity_windows,
)
+from superset.versioning.membership import charts_attached_to_dashboard
def resolve_scope(
diff --git a/superset/versioning/activity/windows.py
b/superset/versioning/activity/windows.py
index b024636e8ff..a5dba64dcfc 100644
--- a/superset/versioning/activity/windows.py
+++ b/superset/versioning/activity/windows.py
@@ -29,10 +29,76 @@ means "open-ended (current)" and behaves like positive
infinity.
from __future__ import annotations
+from itertools import groupby
from typing import Any
from superset.versioning.activity.kinds import EntityWindows, Window
+# ``operation_type`` values on a Continuum association shadow row
+# (sqlalchemy_continuum.operation.Operation): INSERT attaches, DELETE detaches.
+# UPDATE never occurs for a pure M2M association (there is nothing to update on
+# a (dashboard, slice) pair); if it ever appeared it is ignored — neither
+# opening nor closing a window — so an open attachment simply continues.
+# These mirror the library enum's numeric values;
``test_m2m_op_constants_match_
+# continuum`` pins them so a Continuum renumber fails loudly rather than
silently.
+M2M_OP_INSERT = 0
+M2M_OP_DELETE = 2
+
+
+def attachment_windows(
+ rows: list[tuple[int, int, int]],
+) -> list[tuple[int, Window]]:
+ """Pair INSERT / DELETE association-version rows into ``[attach, detach)``
+ windows, one per attachment episode.
+
+ Each row is ``(assoc_id, transaction_id, operation_type)``. Continuum
+ **never closes** an association shadow row's ``end_transaction_id`` — its
+ unit-of-work only *inserts* association versions
+ (``create_association_versions``); the validity backfill that sets
+ ``end_transaction_id`` runs for parent objects, not for M2M links. So the
+ detach boundary lives on the DELETE row's ``transaction_id``, not on the
+ attach row's ``end_transaction_id`` (which stays NULL for the association's
+ whole life). An INSERT opens a window; the next DELETE closes it at its
+ transaction id; an attachment with no following DELETE stays open (the
+ association is still live). A DELETE at the same transaction as its open
+ (add-and-remove in one save) yields no window — the association was never
+ on a committed state. That last case is a deliberate divergence from
+ Continuum's own ``association_subquery`` reverter, which (selecting the
+ ``MAX(tx) <= T`` row and excluding only DELETEs) would treat such a pair as
+ a member; the never-committed reading is the safer one for restore.
+
+ This is the M2M-correct counterpart to
+ :func:`~superset.versioning.changes.shadow_queries.shadow_rows_valid_at`,
+ whose ``end_transaction_id`` validity filter is right for parent/child
+ shadows but silently re-includes a detached association.
+ """
+ result: list[tuple[int, Window]] = []
+ # operation_type is part of the sort key so that, within one transaction,
+ # INSERT (0) sorts before DELETE (2): an add-and-remove in a single save is
+ # then seen open-before-close and collapses to no window (the DELETE finds
+ # ``tx == open_tx``, not ``>``). Do not drop it from the key.
+ #
+ # Corollary / assumption: because INSERT is forced before DELETE within a
+ # transaction, this cannot represent a *remove-then-re-add* of the same
+ # association in one transaction (it would read the same as add-then-remove
+ # → no window). That relies on no write path emitting DELETE-then-INSERT
+ # for the same association within a single transaction — which holds today
+ # (a chart is detached or attached in a save, not both), so the case is
+ # latent, not live. Revisit this pairing if such a write path is added.
+ rows_sorted = sorted(rows, key=lambda r: (r[0], r[1], r[2]))
+ for assoc_id, group in groupby(rows_sorted, key=lambda r: r[0]):
+ open_tx: int | None = None
+ for _assoc_id, tx, operation_type in group:
+ if operation_type == M2M_OP_DELETE:
+ if open_tx is not None and tx > open_tx:
+ result.append((assoc_id, Window(open_tx, tx)))
+ open_tx = None
+ elif operation_type == M2M_OP_INSERT and open_tx is None:
+ open_tx = tx
+ if open_tx is not None:
+ result.append((assoc_id, Window(open_tx, None)))
+ return result
+
def intersect_windows(outer: Window, inner: Window) -> Window | None:
"""Intersect two half-open ``[start_tx, end_tx)`` windows.
diff --git a/superset/versioning/membership.py
b/superset/versioning/membership.py
new file mode 100644
index 00000000000..f7e37f3e570
--- /dev/null
+++ b/superset/versioning/membership.py
@@ -0,0 +1,86 @@
+# 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.
+"""M2M dashboard-membership queries, shared across the versioning surfaces.
+
+``charts_attached_to_dashboard`` reads the ``dashboard_slices_version``
+association shadow and pairs its INSERT/DELETE rows into ``[attach, detach)``
+windows. It must **never** filter that shadow by ``end_transaction_id``:
+Continuum never closes an M2M association's ``end_transaction_id`` (see
+:func:`~superset.versioning.activity.windows.attachment_windows`), so a
+validity filter would re-include a chart removed before the queried tx.
+
+This lives in a neutral module — not the activity read-path module
+(``activity/queries.py``) — so the restore write path and the impact rollup
+depend on it here rather than reaching up into the read path (sc-119907).
+"""
+
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
+import sqlalchemy as sa
+
+from superset.extensions import db
+
+if TYPE_CHECKING:
+ from superset.versioning.activity.kinds import Window
+
+
+def charts_attached_to_dashboard(dashboard_id: int) -> list[tuple[int,
Window]]:
+ """Return ``(slice_id, window)`` for every chart that has ever been on
+ *dashboard_id*, with each attachment episode's validity window in
+ transaction-id space.
+
+ Reads from ``dashboard_slices_version`` (Continuum's auto-generated M2M
+ shadow) and pairs its INSERT/DELETE rows via
+ :func:`~superset.versioning.activity.windows.attachment_windows`,
+ so a chart removed from the dashboard is bounded at the detach transaction
+ rather than open-ended — otherwise the chart's edits made *after* removal
+ would surface in the dashboard's related history.
+ """
+ # pylint: disable=import-outside-toplevel
+ # attachment_windows is imported lazily (not at module top) so that this
+ # module stays a leaf: importing it must not pull the activity package,
+ # whose read-path modules (scope.py) import charts_attached_to_dashboard
+ # back from here — a module-top import created a circular import that only
+ # surfaced at runtime, when a restore imported this module for the first
+ # time (sc-119907).
+ from sqlalchemy_continuum import version_class
+
+ from superset.models.dashboard import Dashboard
+ from superset.versioning.activity.windows import attachment_windows
+
+ metadata = version_class(Dashboard).__table__.metadata
+ m2m_tbl = metadata.tables.get("dashboard_slices_version")
+ if m2m_tbl is None:
+ return []
+
+ rows = (
+ db.session.connection()
+ .execute(
+ sa.select(
+ m2m_tbl.c.slice_id,
+ m2m_tbl.c.transaction_id,
+ m2m_tbl.c.operation_type,
+ ).where(
+ m2m_tbl.c.dashboard_id == dashboard_id,
+ m2m_tbl.c.slice_id.is_not(None),
+ )
+ )
+ .all()
+ )
+ return attachment_windows([(row[0], row[1], row[2]) for row in rows])
diff --git a/superset/versioning/restore.py b/superset/versioning/restore.py
index bf5a021e72d..b56bafe08c6 100644
--- a/superset/versioning/restore.py
+++ b/superset/versioning/restore.py
@@ -195,42 +195,40 @@ def _restore_dashboard_membership(dashboard: Any,
transaction_id: int) -> list[i
"""Reset *dashboard*'s chart membership to what it was at
*transaction_id*, reattaching only charts that still exist.
- Reads the validity-windowed ``dashboard_slices_version`` shadow
- (Continuum's auto-generated M2M table): a slice was a member at tx T
- iff a non-DELETE row has ``transaction_id <= T`` and an open or
- later-closing validity window.
+ Membership is derived from the ``dashboard_slices_version`` shadow
+ (Continuum's auto-generated M2M table) by pairing each slice's
+ INSERT/DELETE rows into ``[attach, detach)`` windows: a slice was a
+ member at tx T iff one of its attachment windows contains T. The
+ ``shadow_rows_valid_at`` validity filter must **not** be used here —
+ Continuum never closes an association shadow's ``end_transaction_id``,
+ so that filter would re-attach a chart that had been removed before T
+ (attached@1, removed@5, restore to tx10 → the chart wrongly returns).
+ ``shadow_rows_valid_at`` stays correct for parent/child shadows, whose
+ ``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.
"""
# pylint: disable=import-outside-toplevel
- # Local imports: models.slice transitively imports models.core, which
- # needs the initialised app — module-top import would recreate the
- # bootstrap cycle documented in changes/listener.py; shadow_queries is
- # imported lazily for the same reason (see queries.get_version).
+ # Local imports: models.slice transitively imports models.core, which needs
+ # the initialised app — a module-top import would recreate the bootstrap
+ # cycle documented in changes/listener.py. charts_attached_to_dashboard is
+ # imported lazily for the same reason: it pulls the window helpers, whose
+ # package transitively imports the versioning.changes listener graph, so a
+ # module-top import here would re-enter that same bootstrap cycle.
from superset.models.slice import Slice
- from superset.versioning.changes import shadow_rows_valid_at
+ from superset.versioning.membership import charts_attached_to_dashboard
- ver_cls = version_class(type(dashboard))
- m2m_tbl = ver_cls.__table__.metadata.tables.get("dashboard_slices_version")
- if m2m_tbl is None: # pragma: no cover — shadow tables always exist here
- return []
-
- # shadow_rows_valid_at owns the validity-window semantics (open or
- # later-closing window, non-DELETE) — the same predicate the version
- # snapshot's column/metric reconstruction uses.
+ # charts_attached_to_dashboard owns the association-shadow read and the
+ # attach/detach window pairing (the single place that must never filter the
+ # M2M shadow by end_transaction_id — Continuum never closes it). A slice
was
+ # a member at transaction_id iff one of its windows contains it
(sc-119907).
member_ids = sorted(
{
- row["slice_id"]
- for row in shadow_rows_valid_at(
- db.session,
- m2m_tbl,
- "dashboard_id",
- dashboard.id,
- transaction_id,
- )
- if row["slice_id"] is not None
+ slice_id
+ for slice_id, window in charts_attached_to_dashboard(dashboard.id)
+ if window.contains(transaction_id)
}
)
if not member_ids:
diff --git a/tests/integration_tests/dashboards/version_restore_tests.py
b/tests/integration_tests/dashboards/version_restore_tests.py
index cee290549ff..f372aa17eec 100644
--- a/tests/integration_tests/dashboards/version_restore_tests.py
+++ b/tests/integration_tests/dashboards/version_restore_tests.py
@@ -201,6 +201,76 @@ class TestDashboardRestoreApi(SupersetTestCase):
f"got {restored_ids}"
)
+ def test_restore_to_tx_after_removal_does_not_reattach_chart(self) -> None:
+ """The inverse of the re-attach case (sc-119907): restoring to a
+ snapshot captured *after* a chart was removed must NOT bring it back.
+
+ Continuum never closes an association shadow row's
+ ``end_transaction_id``, so the removed chart's INSERT row still looks
+ "valid at" any later tx under a naive validity filter — it would be
+ wrongly re-attached. Membership must be derived from paired
+ attach/detach windows: the window ``[attach, remove)`` does not contain
+ the post-removal target tx, so the chart stays off.
+ """
+ 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
+ assert entity_uuid is not None
+
+ 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
+
+ # Remove the chart and commit — the detach is recorded in history.
+ dashboard.slices.remove(slice_to_drop)
+ db.session.commit()
+
+ # Touch the dashboard AFTER the removal so the restore target snapshot
+ # post-dates the detach.
+ dashboard.dashboard_title = "USA Births Names — post-removal snapshot"
+ db.session.commit()
+
+ ver_cls = version_class(Dashboard)
+ target_tx = (
+ db.session.query(ver_cls.transaction_id)
+ .filter(ver_cls.id == dashboard_id, ver_cls.uuid == entity_uuid)
+ .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))
+
+ 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 drop_id not in restored_ids, (
+ "restore re-attached a chart that was removed before the target "
+ f"snapshot: {drop_id} in {restored_ids}"
+ )
+ survivors = sorted(set(original_slice_ids) - {drop_id})
+ assert restored_ids == survivors, (
+ f"expected the surviving members {survivors}, 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
diff --git a/tests/unit_tests/versioning/test_activity.py
b/tests/unit_tests/versioning/test_activity.py
index 98e91d1be41..21f9b6f55bd 100644
--- a/tests/unit_tests/versioning/test_activity.py
+++ b/tests/unit_tests/versioning/test_activity.py
@@ -44,6 +44,8 @@ from superset.versioning.activity import (
Window,
)
from superset.versioning.activity.impact import (
+ _count_attached_charts_at,
+ batch_chart_counts,
collect_impact_pairs,
impact_for_record,
)
@@ -54,9 +56,6 @@ from superset.versioning.activity.orchestrator import (
_MAX_PAGE_SIZE,
)
from superset.versioning.activity.queries import (
- _attachment_windows,
- _M2M_OP_DELETE,
- _M2M_OP_INSERT,
_merge_result_into_heap,
_record_sort_key,
BoundedRecordHeap,
@@ -68,7 +67,10 @@ from superset.versioning.activity.render import (
)
from superset.versioning.activity.scope import resolve_scope
from superset.versioning.activity.windows import (
+ attachment_windows,
intersect_windows,
+ M2M_OP_DELETE,
+ M2M_OP_INSERT,
merge_entity_windows,
row_within_any_window,
union_windows,
@@ -893,7 +895,7 @@ def test_build_summary_meta_headline_branches() -> None:
assert _build_summary("Dashboard", unknown) == "Dashboard updated"
-# ---- _attachment_windows -------------------------------------------------
+# ---- attachment_windows -------------------------------------------------
def test_attachment_windows_closes_at_detach() -> None:
@@ -903,7 +905,7 @@ def test_attachment_windows_closes_at_detach() -> None:
row's transaction id — an INSERT at t1 paired with a DELETE at t3 yields
the half-open window [t1, t3), so an edit at t2 (while attached) is
inside it and an edit at t4 (after removal) is not."""
- windows = _attachment_windows([(7, 1, 0), (7, 3, 2)]) # INSERT@1, DELETE@3
+ windows = attachment_windows([(7, 1, 0), (7, 3, 2)]) # INSERT@1, DELETE@3
assert windows == [(7, Window(1, 3))]
window = windows[0][1]
@@ -916,7 +918,7 @@ def test_attachment_windows_still_attached_is_open_ended()
-> None:
"""A chart with an INSERT and no following DELETE is still on the
dashboard, so its window is open-ended (end_tx = None) and every later
edit remains in scope."""
- assert _attachment_windows([(7, 5, 0)]) == [(7, Window(5, None))]
+ assert attachment_windows([(7, 5, 0)]) == [(7, Window(5, None))]
def test_attachment_windows_reattach_cycles_and_ordering() -> None:
@@ -924,20 +926,30 @@ def
test_attachment_windows_reattach_cycles_and_ordering() -> None:
is independent of the row order the query returns them in (the rows are
sorted by (slice_id, transaction_id) internally)."""
rows = [(7, 7, 2), (7, 1, 0), (7, 5, 0), (7, 3, 2)] # deliberately
shuffled
- assert _attachment_windows(rows) == [(7, Window(1, 3)), (7, Window(5, 7))]
+ assert attachment_windows(rows) == [(7, Window(1, 3)), (7, Window(5, 7))]
+
+
+def test_attachment_windows_reattach_leaves_the_last_episode_open() -> None:
+ """Attach@1, detach@5, re-attach@8 with no later detach: the first episode
+ is the closed window [1, 5) and the current attachment is open-ended
+ [8, None). An edit at tx 10 falls inside the live episode."""
+ windows = attachment_windows([(7, 1, 0), (7, 5, 2), (7, 8, 0)])
+ assert windows == [(7, Window(1, 5)), (7, Window(8, None))]
+ assert windows[1][1].contains(10)
+ assert not windows[0][1].contains(10)
def test_attachment_windows_add_and_remove_same_transaction() -> None:
"""Attaching and detaching in a single save (INSERT and DELETE at the
same transaction) leaves the chart on no committed dashboard state, so it
contributes no window (and no degenerate zero-width interval)."""
- assert _attachment_windows([(7, 2, 0), (7, 2, 2)]) == []
+ assert attachment_windows([(7, 2, 0), (7, 2, 2)]) == []
def test_attachment_windows_separates_slices() -> None:
"""Each slice gets its own independent windows."""
rows = [(7, 1, 0), (7, 3, 2), (9, 2, 0)]
- assert _attachment_windows(rows) == [
+ assert attachment_windows(rows) == [
(7, Window(1, 3)),
(9, Window(2, None)),
]
@@ -945,10 +957,123 @@ def test_attachment_windows_separates_slices() -> None:
def test_m2m_op_constants_match_continuum() -> None:
"""The association-shadow operation-type constants used by
- _attachment_windows are the numeric values of Continuum's Operation enum.
+ attachment_windows are the numeric values of Continuum's Operation enum.
Pin them so a library renumber fails here loudly rather than silently
mis-pairing attach/detach rows."""
from sqlalchemy_continuum import Operation
- assert _M2M_OP_INSERT == Operation.INSERT
- assert _M2M_OP_DELETE == Operation.DELETE
+ assert M2M_OP_INSERT == Operation.INSERT
+ assert M2M_OP_DELETE == Operation.DELETE
+
+
+# ---- _count_attached_charts_at (batch_chart_counts membership) -----------
+
+
+def _slice_row(
+ slice_id: int, datasource_id: int, start: int, end: int | None
+) -> dict[str, Any]:
+ """A chart→dataset parent-shadow row as batch_chart_counts fetches it."""
+ return {
+ "slice_id": slice_id,
+ "datasource_id": datasource_id,
+ "slice_start": start,
+ "slice_end": end,
+ }
+
+
+def test_count_attached_charts_excludes_chart_removed_before_target() -> None:
+ """sc-119907: a chart attached@1 and removed@5 must NOT be counted for a
+ dataset rollup at target_tx=10. Its attachment window [1, 5) does not
+ contain 10, even though its (never-closed) association shadow row would
+ pass a naive end_transaction_id validity filter."""
+ attach_windows = {7: [Window(1, 5)]}
+ slice_rows = [_slice_row(7, 100, 1, None)] # chart→dataset open the whole
time
+ result = _count_attached_charts_at(attach_windows, slice_rows, {100: [10]})
+ assert result == {} # zero-count pairs are omitted
+
+
+def test_count_attached_charts_counts_chart_inside_its_window() -> None:
+ """The same chart IS counted at a target inside its attachment window."""
+ attach_windows = {7: [Window(1, 5)]}
+ slice_rows = [_slice_row(7, 100, 1, None)]
+ result = _count_attached_charts_at(attach_windows, slice_rows, {100: [3]})
+ assert result == {(100, 3): 1}
+
+
+def test_count_attached_charts_requires_both_windows() -> None:
+ """A chart attached at target_tx but not yet pointing at the dataset (its
+ chart→dataset window starts later) is not counted."""
+ attach_windows = {7: [Window(1, None)]}
+ slice_rows = [_slice_row(7, 100, 8, None)] # points at dataset only from
tx8
+ assert _count_attached_charts_at(attach_windows, slice_rows, {100: [3]})
== {}
+
+
+def test_count_attached_charts_dedupes_within_pair() -> None:
+ """Multiple parent-shadow rows for the same slice count the slice once."""
+ attach_windows = {7: [Window(1, None)]}
+ slice_rows = [_slice_row(7, 100, 1, 4), _slice_row(7, 100, 4, None)]
+ assert _count_attached_charts_at(attach_windows, slice_rows, {100: [5]})
== {
+ (100, 5): 1
+ }
+
+
+def test_count_attached_charts_ignores_slice_never_on_dashboard() -> None:
+ """A slice pointing at the dataset but with no attachment window (never on
+ this dashboard) does not contribute — guards the no-join fetch that may
+ return slices from other dashboards sharing the dataset."""
+ attach_windows: dict[int, list[Window]] = {}
+ slice_rows = [_slice_row(7, 100, 1, None)]
+ assert _count_attached_charts_at(attach_windows, slice_rows, {100: [3]})
== {}
+
+
+# ---- batch_chart_counts bind-variable floor (sc-119907) ------------------
+
+
+def test_batch_chart_counts_stays_under_sqlite_bind_floor(app_context: None)
-> None:
+ """sc-119907: a wide dashboard (many member charts AND many requested
+ datasets) must not build a slice-scan statement that exceeds SQLite's 999
+ bind-variable floor. The member-id IN is chunked; the requested-dataset IN
+ is dropped to Python-side filtering when it would not co-bind under the
+ floor. Compiles every issued statement and asserts its bind count stays
+ safely under 999 — the pre-fix code (dataset IN always on) bound
+ 500 member + 600 dataset + scalars = ~1104 and would 500 on SQLite."""
+
+ from sqlalchemy.dialects import sqlite
+
+ member_windows = [(sid, Window(1, None)) for sid in range(1, 601)]
+ pairs = {(ds, 5) for ds in range(10_000, 10_600)} # 600 requested datasets
+ captured: list[Any] = []
+
+ class _FakeResult:
+ def mappings(self) -> "_FakeResult":
+ return self
+
+ def all(self) -> list[Any]:
+ return []
+
+ def _fake_execute(stmt: Any) -> "_FakeResult":
+ captured.append(stmt)
+ return _FakeResult()
+
+ with (
+ patch(
+ "superset.versioning.membership.charts_attached_to_dashboard",
+ return_value=member_windows,
+ ),
+ patch("superset.versioning.activity.impact.db") as mock_db,
+ ):
+ mock_db.session.connection.return_value.execute.side_effect =
_fake_execute
+ batch_chart_counts(1, pairs)
+
+ assert captured, "expected at least one slice-scan statement"
+ for stmt in captured:
+ # render_postcompile expands ``IN (...)`` (an expanding bind in
+ # SQLAlchemy 2.0) into one param per element, so the count reflects
+ # what actually hits SQLite — a plain compile would show one bind per
+ # IN and hide the overflow.
+ compiled = stmt.compile(
+ dialect=sqlite.dialect(),
+ compile_kwargs={"render_postcompile": True},
+ )
+ n_binds = len(compiled.params)
+ assert n_binds < 999, f"statement binds {n_binds} params (SQLite floor
999)"
