mikebridge commented on PR #41076:
URL: https://github.com/apache/superset/pull/41076#issuecomment-4927469980
## Versioning base-infra — verified review notes (head `bf31b236`)
_Generated by a multi-lens review (committer / python / sqlalchemy / DDD
lenses) and then independently verified against the source — every finding
below was re-checked by reading the code at this head, and a few of the raw
findings were dropped or down-scoped in that pass (noted where relevant). Line
numbers are against `bf31b236`. SIP/process items are tracked separately and
excluded here._
**TL;DR** — nothing here blocks the dark launch. It ships inert behind
`ENABLE_VERSIONING_CAPTURE` (default off), the migration is
additive/reversible, and the security posture (per-object `raise_for_access` on
every read endpoint, fail-closed visibility filter, fail-open write path) is
solid. The items below are (A) capture-path correctness bugs to fix **before**
flipping the flag on, (B) small cleanups, and (C) forward-looking notes.
### A. Fix before enabling `ENABLE_VERSIONING_CAPTURE`
These are latent while capture is off, so they don't block merge — but they
bite the moment capture is enabled.
**`superset/versioning/diff.py:425-430` — duplicate natural keys silently
overwrite, dropping change records**
The by-key maps are built last-write-wins: two adhoc filters on the same
`subject` column (or two metrics sharing a `label`) collapse to one, so
editing/removing one emits a mangled single record and the other's change is
lost. The `_filter_key` docstring's promise that same-column filters stay
disambiguable can't hold once the collision drops one.
Would it be worth disambiguating on collision (secondary key, or fall back
to positional index only for colliding items)?
**`superset/versioning/diff.py:420-427` (+ listener catch at
`changes/listener.py:239`) — an unhashable `subject` raises `TypeError`, losing
that entity's scalar diff**
`_filter_key` returns `f.get("subject")` (typed `Key`, but it's `Any` from
user JSON). A list/dict subject flows unguarded into `from_by_key[<list>]` →
`TypeError: unhashable`. It's caught per-entity, so the blast radius is just
that one entity's scalar change records for the save (not the whole save) — but
the record loss is silent. A `str(...)` coercion (or a fallback in
`_effective_key` when the key isn't `str`/`int`/`None`) closes it.
**`superset/commands/importers/v1/utils.py:391-419` —
`_prime_versioning_unit_of_work`'s broad `except` gives false assurance**
The docstring promises it "never breaks an import," but the broad `except
Exception` only genuinely covers the ImportError (continuum-absent) case. If
`unit_of_work()` fails for any other reason while capture is enabled, the very
next `dashboard_slices.insert()` fires Continuum's `before_execute` with no UoW
and raises `KeyError` — the import dies anyway, now with a misleading
"proceeding without it" log. Narrowing to `except ImportError:` lets the real
failure surface at its source. (Narrow trigger — `unit_of_work` rarely fails —
but the false assurance is the issue.)
**`superset/versioning/activity/queries.py:444-489` — per-kind budget can
drop later chunks' records and mislabel the window as complete**
When one kind's entities span multiple id-chunks and fill the ~5000-row
budget, the loop `break`s and later chunks contribute zero rows — including
records newer than the truncation floor — yet `truncated=True` presents it as a
clean "older records omitted" cut. (Correction to the raw finding: the drop
order is arbitrary but **deterministic** — not `PYTHONHASHSEED`-dependent —
since these are `set[int]`. High bar: >500 entities of one kind **and** >5000
matching rows.) Applying the ceiling after concatenating + re-sorting all
chunks (and `sorted(entity_ids)` before chunking) makes any residual drop
deterministic and correctly labeled.
### B. Small cleanups (unconditional)
**`superset/versioning/diff.py:799-805` — `_meta_excluding_position` doesn't
exclude anything**
Name + docstring promise positional bits are stripped from `meta`; the body
just returns `dict(meta)` unchanged. It's correct-by-accident today (position
lives on `node["parents"]`, handled separately), but the next field added to
`meta` silently breaks the move-vs-edit contract the name advertises. Rename to
`_meta_copy` (+ honest docstring) or actually implement the strip.
**`Slice` → `chart` dispatch is spread across ~8 parallel structures keyed
on the class-name string**
`ENTITY_KIND_BY_CLASS_NAME` (`changes/table.py:86`), `TABLE_KIND_TO_API` /
`API_KIND_TO_TABLE` / `API_KIND_LABEL` / `USER_FACING_KIND` / `NOT_FOUND_EXC` /
`NAME_COLUMN` (`activity/kinds.py:73-115`), `_RAISE_FOR_ACCESS_KWARG`
(`api_helpers.py:136`), plus string-dispatch ladders in `changes/state.py`.
Adding a fourth versioned entity means editing all of them in lock-step with
nothing catching a miss. A single registry (one entry per entity,
constructor-checked for completeness) turns "did I update every table?" into a
type error. The DDD lens flagged this as the one durable must-fix as the
surface grows.
**`superset/daos/dataset.py:534-537` & `:602-605` — `_upsert_columns` /
`update_metrics` lack the `protected_keys` guard**
The sibling `_override_columns` (`:512-516`) protects `("id", "table_id")`
before `setattr`; these two loops set every payload key unguarded. Benign
today, but it's the exact footgun the sibling was hardened against — worth the
same guard for symmetry.
### C. Forward-looking (not this PR, but worth a note)
- **Shadow-table FKs to `version_transaction` have no `ondelete`, while
`version_changes.transaction_id` has `ondelete="CASCADE"`** (migration
`56cd24c07170`). When the retention task (stacked PR) deletes
`version_transaction` rows, it must delete referencing shadow rows first, in
order, or every prune cycle hits an FK violation on PG/MySQL. Either add
`ondelete="CASCADE"` to the six shadow FKs, or make the deletion-ordering
contract explicit where retention lands.
- **Migration idempotency is inconsistent** — `add_versioning_tables` uses
raw `create_table` (a partial failure on MySQL's non-transactional DDL isn't
re-runnable), while the index follow-up migration is inspector-guarded. Worth
matching the guarded pattern (or documenting the manual-cleanup caveat).
- **Per-save cost once capture is on** —
`SkipUnmodifiedPlugin._matches_previous_version` (`factory.py:294`) +
`_read_pre_state` add two full-row reads (incl. large `MediumText` columns) per
versioned save. `perf_validation_tests.py` is the right place to surface
numbers before the default flips.
### Checked and cleared
- `SqlaTable.__versioned__` excluding `editors` but not `viewers` — **not a
problem**: `SqlaTable` has no `viewers` relationship, so there's nothing to
exclude (unlike Dashboard/Slice).
- Import-time `make_versioned()` in `superset/extensions/__init__.py` — no
model classes are imported at that point, so it's the correct Continuum
ordering, not an import-safety regression.
Praise where due: the fail-open write path + dedicated capture-error
metrics, the thorough kill-switch (detaches Continuum's own listeners *and*
verifies), the deterministic UUIDv5 version identity, the pure/functional diff
engine with `ChangeRecord`/`Window` as clean value objects, and an unusually
honest `UPDATING.md` are all genuinely well done.
--
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]
