aminghadersohi commented on code in PR #43838:
URL: https://github.com/apache/superset/pull/43838#discussion_r3931366905
##########
superset/versioning/schemas.py:
##########
@@ -267,6 +281,17 @@ class ActivityImpactSchema(Schema):
)
},
)
+ chart_names = fields.List(
Review Comment:
**Naming: `chart_names` holds objects, not names.**
The field is a list of `{id, name}`, and the description has to spell that
out ("The affected sibling charts (id + name)"). `charts` is already taken by
the count, but something like `affected_charts` or `chart_refs` would describe
the payload without the description having to correct the field name. Public
API field names are hard to change once shipped, so it's worth a moment now.
Purely a naming call — no behavioural concern.
Minor, same block: the literal "Capped at 50 entries" duplicates
`IMPACT_CHART_NAMES_CAP`; if the constant moves, this description goes stale
silently.
##########
superset/versioning/activity/impact.py:
##########
@@ -159,28 +179,56 @@ def batch_chart_counts(
row["slice_end"] is None or row["slice_end"] > target_tx
)
if in_m2m and in_slice:
- matches.setdefault((ds_id, target_tx),
set()).add(row["slice_id"])
+ matches.setdefault((ds_id, target_tx), {})[row["slice_id"]] = (
Review Comment:
**The m2m half of this predicate never closes, so charts detached from the
dashboard are still matched — and this PR promotes that from an inflated number
into a named, user-visible list.**
`row["m2m_end"]` is `dashboard_slices_version.end_transaction_id`. That's an
*association* shadow, and SQLAlchemy-Continuum never populates
`end_transaction_id` on association tables:
`UnitOfWork.update_version_validity` — the thing that closes a window — is
reached only from `process_operation`, i.e. for mapped parent objects.
Association rows go through `create_association_versions`, which does nothing
but `INSERT` with `transaction_id` set. A detach is recorded as a **separate
row with `operation_type = 2`**, not as a close of the original row, and the
`operation_type != 2` filter on line 144 removes exactly that row — leaving the
still-open `operation_type = 0` row to match forever.
I verified this rather than taking it on faith. Standalone probe (Continuum,
`strategy: validity`, in-memory SQLite): attach charts A+B at tx1 → detach B at
tx2 → rename A at tx3 → re-attach B at tx4.
```
== dash_slices_version ==
{'dashboard_id': 1, 'slice_id': 1, 'transaction_id': 1,
'end_transaction_id': None, 'operation_type': 0}
{'dashboard_id': 1, 'slice_id': 2, 'transaction_id': 1,
'end_transaction_id': None, 'operation_type': 0}
{'dashboard_id': 1, 'slice_id': 2, 'transaction_id': 2,
'end_transaction_id': None, 'operation_type': 2}
{'dashboard_id': 1, 'slice_id': 2, 'transaction_id': 4,
'end_transaction_id': None, 'operation_type': 0}
== slices_version ==
{'id': 1, 'slice_name': 'Chart A', 'transaction_id': 1,
'end_transaction_id': 3, 'operation_type': 0}
{'id': 2, 'slice_name': 'Chart B', 'transaction_id': 1,
'end_transaction_id': None, 'operation_type': 0}
{'id': 1, 'slice_name': 'Chart A renamed', 'transaction_id': 3,
'end_transaction_id': None, 'operation_type': 1}
```
Every m2m `end_transaction_id` is `NULL`, including for the chart that was
detached; the parent shadow's is correctly populated (`3`). So `row["m2m_end"]
is None or ...` on line 176 is vacuously true and the m2m window is unbounded
on the right.
**Failure scenario.** Chart "Q3 revenue" is on dashboard D pointing at
dataset X. It is removed from D at tx2. Someone edits a metric on X at tx5. The
rollup row for that edit counts the chart as affected, and with this PR
hovering now *displays* "Q3 revenue" as a chart the change affected — two
transactions after it stopped being on the dashboard.
The `in_slice` half is fine: `slices_version` is a parent shadow and its
windows really are closed, as the probe shows. Only the m2m half is affected.
**Scope / suggested handling.** This is pre-existing on master, not
introduced by you, and it's shared with at least
`activity/queries.py:charts_attached_to_dashboard`,
`changes/shadow_queries.py`, and `restore.py:_restore_dashboard_membership`,
which all apply the same validity predicate to this same M2M table. So I
wouldn't ask you to fix it inside this PR. But since #43837 is already
correcting exactly this in `charts_attached_to_dashboard`, it may be worth
making that a shared helper and pointing `impact.py` at it too, rather than
leaving this path on the unbounded predicate while the sibling path is fixed.
The predicate Continuum itself uses for associations is "latest non-DELETE row
per `(dashboard_id, slice_id)` with `transaction_id <= target`" — a
`max(transaction_id)` group-by rather than an end-window (see
`RelationshipBuilder.association_subquery`).
##########
superset/versioning/activity/impact.py:
##########
@@ -50,6 +52,19 @@
)
+class ChartRef(TypedDict):
+ """One affected chart in an ``impact`` payload: id plus
name-at-transaction."""
+
+ id: int
+ name: str
+
+
+# The wire ``chart_names`` list is capped so one dataset feeding very many
+# charts cannot balloon every related record on the page (page sizes reach
+# 200 records); ``charts`` always carries the full count.
+IMPACT_CHART_NAMES_CAP = 50
Review Comment:
**The cap is per record, so it doesn't bound the response the comment
reasons about.**
The comment argues from page size ("page sizes reach 200 records"), but the
cap is applied in `impact_for_record`, once per record, and
`charts[:IMPACT_CHART_NAMES_CAP]` produces a fresh list per record even when
several records share one `(dataset, tx)` pair. Worst case is 200 × 50 = 10,000
chart refs in one response — roughly 0.5-1 MB of JSON — not 50.
In practice the cap only binds when a single dataset feeds more than 50
charts *on one dashboard*, so the realistic bound is 200 × (charts on that
dashboard for that dataset), and 50 is a sensible per-tooltip number. Not
asking you to change the value. But the comment reads as if it bounds the page
when it bounds a record — either reword it, or add a page-level budget if you
want a hard ceiling.
##########
superset/versioning/activity/render.py:
##########
@@ -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)
Review Comment:
**`impact` isn't cleared by the tombstone redaction below, and it just
became a lot more descriptive.**
The block on lines 171-191 deliberately enumerates what a deleted related
entity must not disclose — `entity_name`, `summary`, `changed_by`,
`from_value`, `to_value`, `path` — but `record["impact"]`, set on this line, is
left as-is. Before this PR it was a bare count; now it carries chart ids and
names.
My own read is that this is **probably not** a boundary violation: `impact`
is non-null only when the related entity is a `SqlaTable`, and the charts it
names are members of the *path* dashboard, which the requester is already gated
to via `raise_for_access`. So it names the requester's own dashboard's charts,
not anything identifying the deleted dataset. The one edge is that the named
charts are those attached at the historical transaction, which may no longer be
on the dashboard.
Filing this as a question rather than a finding, per this repo's guidance: I
can't point at a role/capability row in `SECURITY.md` that it violates. But the
redaction block is an allowlist-by-omission, and the thing it omits is richer
than when the block was written — worth an explicit decision, and perhaps a
comment recording it, either way. (Independently raised by bito.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
