You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by vi...@apache.org on 2026/09/16 20:00:46 UTC
(superset) 17/34: fix(versioning): carry affected chart names in the rollup impact payload (#43838)
This is an automated email from the ASF dual-hosted git repository.
villebro pushed a commit to branch 7.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 2303b7e6c979546217700eddff3e003f4b88468e
Author: Mike Bridge <mi...@preset.io>
AuthorDate: Mon Sep 14 16:11:00 2026 -0600
fix(versioning): carry affected chart names in the rollup impact payload (#43838)
Co-authored-by: Claude Fable 5 <no...@anthropic.com>
Co-authored-by: Mike Bridge <mi...@ext.preset.io>
(cherry picked from commit 4ece071eb77bdd3156fc62e28b4decccaeab615f)
---
docs/static/resources/openapi.json | 20 +++
.../versionHistory/RelatedUpdateRow.test.tsx | 150 ++++++++++++++++++
.../features/versionHistory/RelatedUpdateRow.tsx | 103 +++++++++---
.../src/features/versionHistory/types.ts | 11 +-
superset/versioning/activity/impact.py | 152 +++++++++++-------
superset/versioning/activity/render.py | 35 +++--
superset/versioning/schemas.py | 47 +++++-
tests/unit_tests/versioning/test_activity.py | 175 ++++++++++++++++-----
8 files changed, 564 insertions(+), 129 deletions(-)
diff --git a/docs/static/resources/openapi.json b/docs/static/resources/openapi.json
index 5dc171cd475..63e24defb77 100644
--- a/docs/static/resources/openapi.json
+++ b/docs/static/resources/openapi.json
@@ -205,6 +205,13 @@
},
"ActivityImpact": {
"properties": {
+ "affected_charts": {
+ "description": "The affected sibling charts (id + name), sorted by name \u2014 the detail behind the ``charts`` count, rendered as the rollup entry's hover tooltip. Capped at 50 entries per record; ``charts`` always carries the full count.",
+ "items": {
+ "$ref": "#/components/schemas/ActivityImpactChart"
+ },
+ "type": "array"
+ },
"charts": {
"description": "Number of sibling charts on the path entity affected by the same related-record change at this transaction.",
"type": "integer"
@@ -212,6 +219,19 @@
},
"type": "object"
},
+ "ActivityImpactChart": {
+ "properties": {
+ "id": {
+ "description": "Chart id.",
+ "type": "integer"
+ },
+ "name": {
+ "description": "Chart name at the change's transaction \u2014 it may differ from the live name if the chart was renamed since.",
+ "type": "string"
+ }
+ },
+ "type": "object"
+ },
"ActivityRecord": {
"properties": {
"action_kind": {
diff --git a/superset-frontend/src/features/versionHistory/RelatedUpdateRow.test.tsx b/superset-frontend/src/features/versionHistory/RelatedUpdateRow.test.tsx
new file mode 100644
index 00000000000..6ae77f6b985
--- /dev/null
+++ b/superset-frontend/src/features/versionHistory/RelatedUpdateRow.test.tsx
@@ -0,0 +1,150 @@
+/**
+ * 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.
+ */
+import userEvent from '@testing-library/user-event';
+import { render, screen } from 'spec/helpers/testing-library';
+import type { ActivityRecord } from './types';
+import RelatedUpdateRow from './RelatedUpdateRow';
+
+const baseRecord: ActivityRecord = {
+ entity_kind: 'dataset',
+ entity_uuid: 'abc',
+ entity_name: 'Sales Transactions',
+ entity_deleted: false,
+ entity_deletion_state: null,
+ version_uuid: null,
+ source: 'related',
+ transaction_id: 100,
+ action_kind: null,
+ issued_at: '2026-09-03T10:00:00Z',
+ changed_by: null,
+ kind: 'metric',
+ operation: 'update',
+ path: ['metrics'],
+ from_value: 'a',
+ to_value: 'b',
+ summary: 'Dataset updated: Sales Transactions',
+ impact: {
+ charts: 2,
+ affected_charts: [
+ { id: 11, name: 'Alpha chart' },
+ { id: 12, name: 'Beta chart' },
+ ],
+ },
+};
+
+test('hovering the impact rollup headline lists the affected chart names', async () => {
+ render(<RelatedUpdateRow record={baseRecord} />);
+
+ expect(
+ screen.getByText(/Dataset used by 2 charts updated/),
+ ).toBeInTheDocument();
+
+ userEvent.hover(screen.getByText(/Dataset used by 2 charts updated/));
+
+ expect(await screen.findByText('Alpha chart')).toBeInTheDocument();
+ expect(screen.getByText('Beta chart')).toBeInTheDocument();
+});
+
+test('an impact chart with an empty name renders as Untitled in the tooltip', async () => {
+ const record: ActivityRecord = {
+ ...baseRecord,
+ impact: {
+ charts: 2,
+ affected_charts: [
+ { id: 11, name: '' },
+ { id: 12, name: 'Beta chart' },
+ ],
+ },
+ };
+ render(<RelatedUpdateRow record={record} />);
+
+ userEvent.hover(screen.getByText(/Dataset used by 2 charts updated/));
+
+ expect(await screen.findByText('Untitled')).toBeInTheDocument();
+ expect(screen.getByText('Beta chart')).toBeInTheDocument();
+});
+
+test('a capped impact list shows an overflow line for the remaining charts', async () => {
+ const record: ActivityRecord = {
+ ...baseRecord,
+ impact: {
+ charts: 5,
+ affected_charts: [
+ { id: 11, name: 'Alpha chart' },
+ { id: 12, name: 'Beta chart' },
+ { id: 13, name: 'Gamma chart' },
+ ],
+ },
+ };
+ render(<RelatedUpdateRow record={record} />);
+
+ userEvent.hover(screen.getByText(/Dataset used by 5 charts updated/));
+
+ expect(await screen.findByText('Alpha chart')).toBeInTheDocument();
+ expect(screen.getByText('…and 2 more charts')).toBeInTheDocument();
+});
+
+test('a single-chart impact still offers the names tooltip (pinned decision)', async () => {
+ // With exactly one affected chart the headline falls back to the server
+ // summary, but the hover detail remains available — the single name is
+ // still information the row cannot show inline.
+ const record: ActivityRecord = {
+ ...baseRecord,
+ impact: { charts: 1, affected_charts: [{ id: 11, name: 'Alpha chart' }] },
+ };
+ render(<RelatedUpdateRow record={record} />);
+
+ userEvent.hover(screen.getByText(/Dataset updated/));
+
+ expect(await screen.findByText('Alpha chart')).toBeInTheDocument();
+});
+
+test('a backend that emits the count but predates affected_charts gets a count-only tooltip', async () => {
+ // Older backends send `impact.charts` without `affected_charts`; the
+ // headline still says "used by N charts", so the hover must at least
+ // confirm the count rather than promise detail it cannot show.
+ const record: ActivityRecord = {
+ ...baseRecord,
+ impact: { charts: 4 },
+ };
+ render(<RelatedUpdateRow record={record} />);
+
+ userEvent.hover(screen.getByText(/Dataset used by 4 charts updated/));
+
+ expect(await screen.findByText('4 affected charts')).toBeInTheDocument();
+});
+
+test('a related record without impact names shows no tooltip', async () => {
+ const record: ActivityRecord = {
+ ...baseRecord,
+ impact: null,
+ summary: 'Dataset updated: Sales Transactions',
+ };
+ render(<RelatedUpdateRow record={record} />);
+
+ userEvent.hover(screen.getByText(/Dataset updated/));
+
+ // The tooltip mounts asynchronously; a synchronous negative query would
+ // pass even if one were about to appear. Wait out the mount window and
+ // require the lookup to come up empty.
+ await expect(
+ screen.findByRole('tooltip', {}, { timeout: 800 }),
+ ).rejects.toThrow();
+ expect(screen.queryByRole('tooltip')).not.toBeInTheDocument();
+});
diff --git a/superset-frontend/src/features/versionHistory/RelatedUpdateRow.tsx b/superset-frontend/src/features/versionHistory/RelatedUpdateRow.tsx
index e5417cfaea3..5f07ce2333e 100644
--- a/superset-frontend/src/features/versionHistory/RelatedUpdateRow.tsx
+++ b/superset-frontend/src/features/versionHistory/RelatedUpdateRow.tsx
@@ -16,8 +16,8 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ComponentType } from 'react';
-import { t } from '@apache-superset/core/translation';
+import { ComponentType, ReactNode } from 'react';
+import { t, tn } from '@apache-superset/core/translation';
import { styled } from '@apache-superset/core/theme';
import { Icons, Tooltip } from '@superset-ui/core/components';
import type { IconType } from '@superset-ui/core/components/Icons/types';
@@ -98,6 +98,12 @@ const Meta = styled.div`
`}
`;
+// Both hover tooltips in this component (rolled-up entity names, impact
+// chart names) render the same "list of names with an Untitled fallback"
+// shape; one helper keeps them visually identical.
+const tooltipNameList = (items: { key: number | string; name: string }[]) =>
+ items.map(({ key, name }) => <div key={key}>{name || t('Untitled')}</div>);
+
export interface RelatedUpdateRowProps {
record: ActivityRecord;
/** Entity names when several same-kind entities rolled into this row. */
@@ -114,6 +120,11 @@ export default function RelatedUpdateRow({
if (rollupEntityNames && rollupEntityNames.length > 1) {
// No single target to link to; a tooltip lists the rolled-up names.
+ // Deliberately NOT merged here: each rolled-up record's
+ // `impact.affected_charts`. This tooltip answers "which entities rolled
+ // up", and the per-record affected-chart detail is surfaced on
+ // single-record rows below. Unioning affected charts across the rolled-up
+ // records is a possible follow-up, not an oversight.
return (
<Row data-test="version-history-related-row">
<IconWrapper>
@@ -121,10 +132,12 @@ export default function RelatedUpdateRow({
</IconWrapper>
<Content>
<Tooltip
- title={rollupEntityNames.map((name, index) => (
- // eslint-disable-next-line react/no-array-index-key
- <div key={index}>{name || t('Untitled')}</div>
- ))}
+ title={tooltipNameList(
+ rollupEntityNames.map((name, index) => ({
+ key: `${index}-${name}`,
+ name,
+ })),
+ )}
>
<Headline>
{relatedRollupHeadline(
@@ -144,6 +157,48 @@ export default function RelatedUpdateRow({
const headline = relatedHeadline(record);
const entityName = entityDisplayName(record);
+ // The "Dataset used by N charts updated" phrasing summarizes siblings the
+ // row cannot name inline; the impact payload carries them for the hover
+ // detail (sc-119775), mirroring the rolled-up-names tooltip above.
+ const impactCount = record.impact?.charts ?? 0;
+ const impactCharts = record.impact?.affected_charts ?? [];
+ // The server caps the named refs while `charts` keeps the full count;
+ // surface the difference as an overflow line.
+ const impactOverflow = impactCount - impactCharts.length;
+ // Tooltip content: the named refs (plus overflow) when the backend supplied
+ // them; a count-only line when it emits `charts` but predates
+ // `affected_charts`, so the headline never advertises detail the hover
+ // cannot show; null when there is no impact at all, in which case no
+ // Tooltip is mounted — the common chart-related row stays as light as it
+ // was before the impact detail existed.
+ let impactTitle: ReactNode = null;
+ if (impactCharts.length > 0) {
+ impactTitle = [
+ ...tooltipNameList(
+ impactCharts.map(chart => ({
+ key: chart.id,
+ name: chart.name,
+ })),
+ ),
+ impactOverflow > 0 && (
+ <div key="impact-overflow">
+ {tn(
+ '…and %s more chart',
+ '…and %s more charts',
+ impactOverflow,
+ impactOverflow,
+ )}
+ </div>
+ ),
+ ];
+ } else if (impactCount > 0) {
+ impactTitle = tn(
+ '%s affected chart',
+ '%s affected charts',
+ impactCount,
+ impactCount,
+ );
+ }
const linkable = !record.entity_deleted && Boolean(onOpen);
// Both the server summary and the impact-aware phrasing end with the
// entity name; split it out so the name can render as a link. Records
@@ -153,26 +208,34 @@ export default function RelatedUpdateRow({
? headline.lastIndexOf(record.entity_name)
: -1;
+ const headlineNode = (
+ <Headline>
+ {nameIndex >= 0 ? (
+ <>
+ {headline.slice(0, nameIndex)}
+ <NameLink type="button" onClick={() => onOpen?.(record)}>
+ {entityName}
+ </NameLink>
+ {headline.slice(nameIndex + record.entity_name.length)}
+ </>
+ ) : (
+ headline
+ )}
+ {record.entity_deleted && ` (${t('deleted')})`}
+ </Headline>
+ );
+
return (
<Row data-test="version-history-related-row">
<IconWrapper>
<Icon iconSize="l" />
</IconWrapper>
<Content>
- <Headline>
- {nameIndex >= 0 ? (
- <>
- {headline.slice(0, nameIndex)}
- <NameLink type="button" onClick={() => onOpen?.(record)}>
- {entityName}
- </NameLink>
- {headline.slice(nameIndex + record.entity_name.length)}
- </>
- ) : (
- headline
- )}
- {record.entity_deleted && ` (${t('deleted')})`}
- </Headline>
+ {impactTitle !== null ? (
+ <Tooltip title={impactTitle}>{headlineNode}</Tooltip>
+ ) : (
+ headlineNode
+ )}
<Meta>
{formatAuthor(record.changed_by)} ·{' '}
{formatVersionDateTimeShort(record.issued_at)}
diff --git a/superset-frontend/src/features/versionHistory/types.ts b/superset-frontend/src/features/versionHistory/types.ts
index f37799c57cc..1ad17f83c43 100644
--- a/superset-frontend/src/features/versionHistory/types.ts
+++ b/superset-frontend/src/features/versionHistory/types.ts
@@ -58,7 +58,16 @@ export interface ActivityRecord {
from_value: unknown;
to_value: unknown;
summary: string;
- impact: { charts: number } | null;
+ impact: {
+ charts: number;
+ /**
+ * The affected charts (id + name at that transaction), sorted by name.
+ * Always present when `impact` is non-null on servers that emit it;
+ * optional only for responses from older backends that predate the
+ * field.
+ */
+ affected_charts?: { id: number; name: string }[];
+ } | null;
/**
* True only on records whose transaction is the entity's first tracked
* save (the first update after its retroactive baseline). Such saves can
diff --git a/superset/versioning/activity/impact.py b/superset/versioning/activity/impact.py
index f1c2b22b47e..3a636645fa7 100644
--- a/superset/versioning/activity/impact.py
+++ b/superset/versioning/activity/impact.py
@@ -17,31 +17,33 @@
"""Per-record impact computation for the activity DTO.
Only dashboard-path activity records pointing at a ``SqlaTable``
-related entity carry an ``impact`` field — the number of charts on
-the dashboard at that transaction that were pointing at the dataset.
-This module computes that count in a single batched query per
-request:
+related entity carry an ``impact`` field — the charts on the
+dashboard at that transaction that were pointing at the dataset, as
+a count plus per-chart id/name references. This module computes that
+payload in a single batched read per request:
* :func:`collect_impact_pairs` — pulls the distinct
- ``(dataset_id, transaction_id)`` pairs that need counts.
-* :func:`batch_chart_counts` — counts the matching charts without a
+ ``(dataset_id, transaction_id)`` pairs that need computing.
+* :func:`batch_chart_impacts` — collects 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`.
+ member-scoped ``slices_version`` scan supplies the chart→dataset window
+ and the chart's name-at-transaction; the two are combined per pair by
+ :func:`_collect_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).
+ chart references onto each record (returns ``None`` for
+ non-Dashboard paths or non-SqlaTable kinds, matching the
+ ``impact`` computation).
-Splitting the count batching from the pure projection keeps the SQL
-inside one function (the batched read) and the per-record decoration
-inside another (no DB).
+Splitting the batched read from the pure projection keeps the SQL
+inside one function and the per-record decoration inside another
+(no DB).
"""
from __future__ import annotations
from collections.abc import Mapping, Sequence
-from typing import Any
+from typing import Any, TypedDict
import sqlalchemy as sa
@@ -53,6 +55,15 @@ from superset.versioning.activity.kinds import (
Window,
)
from superset.versioning.baseline import OPERATION_DELETE
+from superset.versioning.schemas import IMPACT_AFFECTED_CHARTS_CAP
+
+
+class ChartRef(TypedDict):
+ """One affected chart in an ``impact`` payload: id plus name-at-transaction."""
+
+ id: int
+ name: str
+
# 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
@@ -79,28 +90,29 @@ def collect_impact_pairs(
}
-def batch_chart_counts(
+def batch_chart_impacts(
dashboard_id: int, pairs: set[tuple[int, int]]
-) -> dict[tuple[int, int], int]:
- """For every ``(dataset_id, target_tx)`` in *pairs*, count the
+) -> dict[tuple[int, int], list[ChartRef]]:
+ """For every ``(dataset_id, target_tx)`` in *pairs*, collect the
distinct charts that were both on *dashboard_id* and pointing at
- *dataset_id* at *target_tx*.
+ *dataset_id* at *target_tx*, as id + name-at-transaction references.
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
- correct.
+ on it would include 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) plus the chart's name at that
+ transaction. 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): [ChartRef, ...]}`` with each pair's
+ charts sorted by name; the name is the chart's name at that transaction (the
+ matched version row). Pairs with no matching charts are omitted so the
+ caller's ``.get(key)`` falsiness check is correct.
"""
if not pairs:
return {}
@@ -128,7 +140,7 @@ def batch_chart_counts(
if not attach_windows:
return {}
- # Chart→dataset validity from the slice parent shadow, whose
+ # Chart→dataset validity and name 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
@@ -161,6 +173,7 @@ def batch_chart_counts(
conditions.append(slices_tbl.c.datasource_id.in_(dataset_ids))
stmt = sa.select(
slices_tbl.c.id.label("slice_id"),
+ slices_tbl.c.slice_name,
slices_tbl.c.datasource_id,
slices_tbl.c.transaction_id.label("slice_start"),
slices_tbl.c.end_transaction_id.label("slice_end"),
@@ -171,30 +184,33 @@ def batch_chart_counts(
for dataset_id, target_tx in pairs:
pairs_by_dataset.setdefault(dataset_id, []).append(target_tx)
- return _count_attached_charts_at(attach_windows, slice_rows, pairs_by_dataset)
+ return _sorted_chart_refs(
+ _collect_attached_charts_at(attach_windows, slice_rows, pairs_by_dataset)
+ )
-def _count_attached_charts_at(
+def _collect_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``.
+) -> dict[tuple[int, int], dict[int, str]]:
+ """Pure combiner: for each ``(dataset_id, target_tx)``, collect the distinct
+ charts (``slice_id`` → name-at-transaction) 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
+ not collected. *slice_rows* are the chart→dataset parent-shadow rows
+ (``slice_id``, ``slice_name``, ``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; ``slice_name`` is the chart's name on that matched version row. Split
+ out of :func:`batch_chart_impacts` so this membership logic is unit-testable
without a live shadow-table fixture.
"""
- matches: dict[tuple[int, int], set[int]] = {}
+ matches: dict[tuple[int, int], dict[int, str]] = {}
for row in slice_rows:
windows = attach_windows.get(row["slice_id"])
if not windows:
@@ -206,27 +222,55 @@ def _count_attached_charts_at(
row["slice_end"] is None or row["slice_end"] > target_tx
)
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()}
+ matches.setdefault((ds_id, target_tx), {})[row["slice_id"]] = (
+ row["slice_name"] or ""
+ )
+ return matches
+
+
+def _sorted_chart_refs(
+ matches: dict[tuple[int, int], dict[int, str]],
+) -> dict[tuple[int, int], list[ChartRef]]:
+ """Order each pair's deduped charts by (casefolded name, id).
+
+ Pure function, split from the batched read so the ordering contract is
+ directly testable: names compare case-insensitively, ties break on id
+ for a deterministic wire order, and empty names sort first (they render
+ as an "Untitled" fallback downstream).
+ """
+ return {
+ pair: sorted(
+ (ChartRef(id=slice_id, name=name) for slice_id, name in charts.items()),
+ key=lambda chart: (chart["name"].casefold(), chart["id"]),
+ )
+ for pair, charts in matches.items()
+ }
def impact_for_record(
record: dict[str, Any],
path_kind: str,
- counts: dict[tuple[int, int], int],
-) -> dict[str, int] | None:
- """Synthesize the ``impact`` field for one record using the pre-
- fetched *counts* mapping. Pure function — no DB.
+ impacts: dict[tuple[int, int], list[ChartRef]],
+) -> dict[str, Any] | None:
+ """Synthesize the ``impact`` field for one record from *impacts*.
- For the ``impact`` computation: only
+ Pure function — no DB. For the ``impact`` computation: only
``path=Dashboard`` and ``related=SqlaTable`` shapes carry an
- impact; everything else returns ``None``.
+ impact; everything else returns ``None``. The payload keeps the
+ ``charts`` count and adds ``affected_charts`` — the affected charts
+ (id + name) that the count summarizes — so the rollup entry's
+ hover tooltip can list them. ``affected_charts`` is capped at
+ :data:`IMPACT_AFFECTED_CHARTS_CAP`; ``charts`` stays the full count so
+ the consumer can render an "and N more" overflow line.
"""
api_kind = TABLE_KIND_TO_API.get(record["entity_kind"])
if path_kind != "Dashboard" or api_kind != "SqlaTable":
return None
key = (record["entity_id"], record["transaction_id"])
- chart_count = counts.get(key, 0)
- if chart_count == 0:
+ charts = impacts.get(key) or []
+ if not charts:
return None
- return {"charts": chart_count}
+ return {
+ "charts": len(charts),
+ "affected_charts": charts[:IMPACT_AFFECTED_CHARTS_CAP],
+ }
diff --git a/superset/versioning/activity/render.py b/superset/versioning/activity/render.py
index 4c0e63c74bd..08900905213 100644
--- a/superset/versioning/activity/render.py
+++ b/superset/versioning/activity/render.py
@@ -20,13 +20,13 @@ After fetching + filtering, each record needs the synthesized fields
the API contract documents — ``entity_kind`` translated to the user-
facing form, ``entity_uuid``, ``entity_deleted`` /
``entity_deletion_state``, ``source`` (self vs. related),
-``summary`` (the headline), ``impact`` (chart-count for
+``summary`` (the headline), ``impact`` (affected-chart payload for
dashboard→dataset records), ``version_uuid``, ``changed_by``.
This module collects all those decorations:
* :func:`apply_record_decoration` — orchestrates the per-page additions in
- one pass: pulls tombstones + uuids + impact counts in batches, then
+ one pass: pulls tombstones + uuids + impact payloads in batches, then
walks records adding the synthesized fields and stripping the
internal-only columns the API contract doesn't expose.
* :func:`_lookup_entity_uuids` plus the historical UUID resolver — identify
@@ -46,7 +46,7 @@ import sqlalchemy as sa
from superset.extensions import db
from superset.versioning.activity.impact import (
- batch_chart_counts,
+ batch_chart_impacts,
collect_impact_pairs,
impact_for_record,
)
@@ -111,11 +111,10 @@ def apply_record_decoration(
tombstones = check_entity_tombstones(distinct)
live_uuids = _lookup_entity_uuids(distinct, tombstones)
historical_uuids = resolve_historical_entity_uuids(records)
- # Pre-compute impact counts for the whole page in one batch query
- # instead of one COUNT per related record (was N+1).
- impact_counts = batch_chart_counts(
- path_id, collect_impact_pairs(records, path_kind)
- )
+ # Pre-compute impact payloads (affected-chart ids + names) for the
+ # whole page in one batch query instead of one query per related
+ # record (was N+1).
+ impact_refs = batch_chart_impacts(path_id, collect_impact_pairs(records, path_kind))
for record in records:
api_kind = TABLE_KIND_TO_API.get(record["entity_kind"], "")
@@ -168,7 +167,7 @@ def apply_record_decoration(
record["impact"] = None
else:
record["summary"] = _build_summary(api_kind, record)
- record["impact"] = impact_for_record(record, path_kind, impact_counts)
+ record["impact"] = impact_for_record(record, path_kind, impact_refs)
if record["entity_deleted"]:
# Security: a tombstoned related entity has no live row, so
# the visibility filter cannot access-gate it (there is
@@ -192,6 +191,24 @@ def apply_record_decoration(
record["from_value"] = None
record["to_value"] = None
record["path"] = None
+ # ``impact`` is deliberately NOT redacted here — an explicit
+ # decision (review of #43838), recorded so this allowlist-by-
+ # omission is not mistaken for an oversight:
+ # * it names the charts on the *path* dashboard that pointed
+ # at the related entity at that transaction — the
+ # requester's own dashboard members, which
+ # ``raise_for_access`` already gated, and version history
+ # is itself edit-gated (sc-120001) — not the deleted entity
+ # or its editors, which is what this block withholds;
+ # * chart titles are visible to a dashboard's viewers today
+ # regardless of datasource access, and the historical
+ # members are the dashboard's own history, so naming them
+ # discloses nothing a requester entitled to the path entity
+ # cannot already see (the count was never redacted either);
+ # * "a dataset was deleted — which of my charts broke?" is the
+ # primary use of the impact detail, and it is exactly the
+ # tombstoned-related case, so redacting here would gut it.
+ # Not a SECURITY.md role/capability matrix row.
# Strip the internal-only columns the API contract doesn't expose.
for key in (
diff --git a/superset/versioning/schemas.py b/superset/versioning/schemas.py
index 1efcee0d704..1488888fa51 100644
--- a/superset/versioning/schemas.py
+++ b/superset/versioning/schemas.py
@@ -250,13 +250,38 @@ class ActivityChangedBySchema(Schema):
last_name = fields.String()
+# Each activity record's ``impact.affected_charts`` list is capped. The cap
+# bounds ONE record's payload (a per-tooltip number), not the page: it is
+# applied per record in ``impact_for_record``, so a page of 200 records can
+# still carry up to 200 × this many refs, and it only binds when a single
+# dataset feeds more than this many charts on one dashboard. ``impact.charts``
+# always carries the full count, so a consumer can render an "and N more"
+# overflow line. Defined with the contract so the field description below
+# quotes the same number the computation enforces.
+IMPACT_AFFECTED_CHARTS_CAP: int = 50
+
+
+class ActivityImpactChartSchema(Schema):
+ """One affected sibling chart inside an ``impact`` payload."""
+
+ id = fields.Integer(metadata={"description": "Chart id."})
+ name = fields.String(
+ metadata={
+ "description": (
+ "Chart name at the change's transaction — it may differ "
+ "from the live name if the chart was renamed since."
+ )
+ },
+ )
+
+
class ActivityImpactSchema(Schema):
- """Dependent-count summary attached to ``source='related'`` records.
+ """Dependent summary attached to ``source='related'`` records.
- Synthesized server-side at the time of the activity query — it counts
- siblings affected by the same upstream change at the same transaction
- (e.g., how many charts on the requested dashboard pointed at the
- dataset whose edit this record represents).
+ Synthesized server-side at the time of the activity query — it
+ identifies siblings affected by the same upstream change at the same
+ transaction (e.g., which charts on the requested dashboard pointed at
+ the dataset whose edit this record represents).
"""
charts = fields.Integer(
@@ -267,6 +292,18 @@ class ActivityImpactSchema(Schema):
)
},
)
+ affected_charts = fields.List(
+ fields.Nested(ActivityImpactChartSchema),
+ metadata={
+ "description": (
+ "The affected sibling charts (id + name), sorted by name — "
+ "the detail behind the ``charts`` count, rendered as the "
+ "rollup entry's hover tooltip. Capped at "
+ f"{IMPACT_AFFECTED_CHARTS_CAP} entries per record; "
+ "``charts`` always carries the full count."
+ )
+ },
+ )
class ActivityRecordSchema(Schema):
diff --git a/tests/unit_tests/versioning/test_activity.py b/tests/unit_tests/versioning/test_activity.py
index 21f9b6f55bd..09767e170d2 100644
--- a/tests/unit_tests/versioning/test_activity.py
+++ b/tests/unit_tests/versioning/test_activity.py
@@ -44,8 +44,9 @@ from superset.versioning.activity import (
Window,
)
from superset.versioning.activity.impact import (
- _count_attached_charts_at,
- batch_chart_counts,
+ _collect_attached_charts_at,
+ batch_chart_impacts,
+ ChartRef,
collect_impact_pairs,
impact_for_record,
)
@@ -219,7 +220,7 @@ def test_decoration_redacts_record_from_reused_entity_id() -> None:
return_value={("Slice", 7, 20): historical_uuid},
),
patch(
- "superset.versioning.activity.render.batch_chart_counts", return_value={}
+ "superset.versioning.activity.render.batch_chart_impacts", return_value={}
),
):
apply_record_decoration([record], "Dashboard", 1)
@@ -480,46 +481,131 @@ def test_changed_by_projects_only_display_fields() -> None:
# ---- impact_for_record (pure, post-batch) -------------------------------
-def test_impact_for_record_dashboard_path_dataset_related_uses_count() -> None:
+def test_impact_for_record_dashboard_path_dataset_related_uses_charts() -> None:
"""The only path/related shape that carries impact: ``Dashboard`` →
- ``SqlaTable``. The count comes from the pre-batched lookup."""
+ ``SqlaTable``. Count and names both come from the pre-batched lookup
+ (sc-119775: the tooltip needs the names, not just the count)."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- counts = {(5, 100): 3}
- assert impact_for_record(record, "Dashboard", counts) == {"charts": 3}
+ charts: list[ChartRef] = [
+ {"id": 11, "name": "Alpha"},
+ {"id": 12, "name": "Beta"},
+ {"id": 13, "name": "Gamma"},
+ ]
+ assert impact_for_record(record, "Dashboard", {(5, 100): charts}) == {
+ "charts": 3,
+ "affected_charts": charts,
+ }
-def test_impact_for_record_missing_count_yields_none() -> None:
+def test_impact_for_record_missing_pair_yields_none() -> None:
"""A pair the batch query didn't return (no matching siblings)
collapses to ``None`` rather than ``{"charts": 0}``."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
assert impact_for_record(record, "Dashboard", {}) is None
-def test_impact_for_record_zero_count_yields_none() -> None:
- """Explicit zero in the counts map is treated the same as missing —
- no impact field on the wire."""
+def test_impact_for_record_empty_charts_yields_none() -> None:
+ """An explicit empty list in the impacts map is treated the same as
+ missing — no impact field on the wire."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "Dashboard", {(5, 100): 0}) is None
+ assert impact_for_record(record, "Dashboard", {(5, 100): []}) is None
def test_impact_for_record_dashboard_path_chart_related_yields_none() -> None:
"""Dashboard → chart is a direct dependency; no further sibling
layer to count."""
record = {"entity_kind": "chart", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "Dashboard", {(5, 100): 999}) is None
+ charts: list[ChartRef] = [{"id": 9, "name": "X"}]
+ assert impact_for_record(record, "Dashboard", {(5, 100): charts}) is None
def test_impact_for_record_chart_path_with_dataset_related_yields_none() -> None:
"""Chart → dataset: the chart is itself the only dependent of the
dataset edit."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "Slice", {(5, 100): 999}) is None
+ charts: list[ChartRef] = [{"id": 9, "name": "X"}]
+ assert impact_for_record(record, "Slice", {(5, 100): charts}) is None
def test_impact_for_record_dataset_path_yields_none() -> None:
"""Datasets have no transitive layer (AV-004)."""
record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
- assert impact_for_record(record, "SqlaTable", {(5, 100): 999}) is None
+ charts: list[ChartRef] = [{"id": 9, "name": "X"}]
+ assert impact_for_record(record, "SqlaTable", {(5, 100): charts}) is None
+
+
+def test_sorted_chart_refs_orders_case_insensitively_with_id_tiebreak() -> None:
+ """The wire order is deterministic: casefolded name, then id; empty
+ names sort first (they render as an Untitled fallback)."""
+ from superset.versioning.activity.impact import _sorted_chart_refs
+
+ refs = _sorted_chart_refs({(5, 100): {3: "Beta", 2: "Alpha", 1: "alpha", 4: ""}})
+ assert refs == {
+ (5, 100): [
+ {"id": 4, "name": ""},
+ {"id": 1, "name": "alpha"},
+ {"id": 2, "name": "Alpha"},
+ {"id": 3, "name": "Beta"},
+ ]
+ }
+
+
+def test_impact_for_record_caps_affected_charts_but_not_the_count() -> None:
+ """A dataset feeding very many charts must not balloon the record: the
+ named refs are capped while ``charts`` keeps the full count."""
+ from superset.versioning.activity.impact import IMPACT_AFFECTED_CHARTS_CAP
+
+ record = {"entity_kind": "dataset", "entity_id": 5, "transaction_id": 100}
+ charts: list[ChartRef] = [
+ {"id": i, "name": f"chart {i:03d}"}
+ for i in range(IMPACT_AFFECTED_CHARTS_CAP + 10)
+ ]
+ result = impact_for_record(record, "Dashboard", {(5, 100): charts})
+ assert result is not None
+ assert result["charts"] == IMPACT_AFFECTED_CHARTS_CAP + 10
+ assert len(result["affected_charts"]) == IMPACT_AFFECTED_CHARTS_CAP
+ assert result["affected_charts"] == charts[:IMPACT_AFFECTED_CHARTS_CAP]
+
+
+def test_related_record_impact_payload_carries_affected_charts() -> None:
+ """End-to-end through record decoration: a dashboard-path dataset
+ record's wire ``impact`` carries the affected chart names alongside
+ the count (sc-119775 — the rollup tooltip's data source)."""
+ record = {
+ "entity_kind": "dataset",
+ "entity_id": 5,
+ "transaction_id": 100,
+ "kind": "metric",
+ "entity_name": "Sales",
+ "changed_by_id": None,
+ "from_value": "a",
+ "to_value": "b",
+ }
+ charts: list[ChartRef] = [
+ {"id": 11, "name": "Alpha"},
+ {"id": 12, "name": "Beta"},
+ ]
+ with (
+ patch(
+ "superset.versioning.activity.render.check_entity_tombstones",
+ return_value={("SqlaTable", 5): {"deleted": False, "deletion_state": None}},
+ ),
+ patch(
+ "superset.versioning.activity.render._lookup_entity_uuids",
+ return_value={},
+ ),
+ patch(
+ "superset.versioning.activity.render.resolve_historical_entity_uuids",
+ return_value={},
+ ),
+ patch(
+ "superset.versioning.activity.render.batch_chart_impacts",
+ return_value={(5, 100): charts},
+ ),
+ ):
+ apply_record_decoration([record], "Dashboard", 1)
+
+ assert record["impact"] == {"charts": 2, "affected_charts": charts}
# ---- collect_impact_pairs -----------------------------------------------
@@ -966,70 +1052,79 @@ def test_m2m_op_constants_match_continuum() -> None:
assert M2M_OP_DELETE == Operation.DELETE
-# ---- _count_attached_charts_at (batch_chart_counts membership) -----------
+# ---- _collect_attached_charts_at (batch_chart_impacts membership) ---------
def _slice_row(
- slice_id: int, datasource_id: int, start: int, end: int | None
+ slice_id: int,
+ datasource_id: int,
+ start: int,
+ end: int | None,
+ name: str = "chart",
) -> dict[str, Any]:
- """A chart→dataset parent-shadow row as batch_chart_counts fetches it."""
+ """A chart→dataset parent-shadow row as batch_chart_impacts fetches it."""
return {
"slice_id": slice_id,
+ "slice_name": name,
"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
+def test_collect_attached_charts_excludes_chart_removed_before_target() -> None:
+ """sc-119907: a chart attached@1 and removed@5 must NOT be collected 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
+ result = _collect_attached_charts_at(attach_windows, slice_rows, {100: [10]})
+ assert result == {} # pairs with no matching charts 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."""
+def test_collect_attached_charts_includes_chart_inside_its_window() -> None:
+ """The same chart IS collected at a target inside its attachment window,
+ carrying its name-at-transaction."""
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}
+ slice_rows = [_slice_row(7, 100, 1, None, name="Sales")]
+ result = _collect_attached_charts_at(attach_windows, slice_rows, {100: [3]})
+ assert result == {(100, 3): {7: "Sales"}}
-def test_count_attached_charts_requires_both_windows() -> None:
+def test_collect_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."""
+ chart→dataset window starts later) is not collected."""
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]}) == {}
+ assert _collect_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."""
+def test_collect_attached_charts_dedupes_within_pair() -> None:
+ """Multiple parent-shadow rows for the same slice collect 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
+ slice_rows = [
+ _slice_row(7, 100, 1, 4, name="Sales"),
+ _slice_row(7, 100, 4, None, name="Sales"),
+ ]
+ assert _collect_attached_charts_at(attach_windows, slice_rows, {100: [5]}) == {
+ (100, 5): {7: "Sales"}
}
-def test_count_attached_charts_ignores_slice_never_on_dashboard() -> None:
+def test_collect_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]}) == {}
+ assert _collect_attached_charts_at(attach_windows, slice_rows, {100: [3]}) == {}
-# ---- batch_chart_counts bind-variable floor (sc-119907) ------------------
+# ---- batch_chart_impacts bind-variable floor (sc-119907) -----------------
-def test_batch_chart_counts_stays_under_sqlite_bind_floor(app_context: None) -> None:
+def test_batch_chart_impacts_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
@@ -1063,7 +1158,7 @@ def test_batch_chart_counts_stays_under_sqlite_bind_floor(app_context: None) ->
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)
+ batch_chart_impacts(1, pairs)
assert captured, "expected at least one slice-scan statement"
for stmt in captured: