mikebridge commented on PR #41550:
URL: https://github.com/apache/superset/pull/41550#issuecomment-5142201629
## Capstone review — the complete soft-delete feature, with resolutions
Ahead of human review, a **10-lens capstone panel** was run over the
*entire* deletion feature as one system — the merged halves (soft-delete core,
restore, the #41549 retention engine) together with this PR — at `63419d4268`.
Lenses: clean-code, continuous-delivery, css, domain-driven-design, preset,
python, react, sqlalchemy, superset-committer, tidy-first. Seven of ten
verified claims empirically (test runs, migration round-trips, live probes).
The scope was deliberately cross-seam: this feature's worst historical bugs
lived in the composition between PRs, not in any single file.
**106 raw findings → ~70 after dedup: 1 BLOCKER, 2 HIGH, ~19 MEDIUM,
remainder LOW.** No lens found a blocker in the backend core — the locked-claim
purge design, write-ahead audit, and migration set were repeatedly verified
sound. The findings concentrated in the seams: frontend↔backend clock
conventions, merged↔unmerged caller assumptions, dialect↔dialect constraint
divergence, and copy↔behaviour honesty.
⚑N = found independently by N lenses. **Every "fixed" entry below carries a
verified control**: the regression test was confirmed to *fail* against the
pre-fix source and pass with the fix — a discipline adopted after earlier
rounds produced tests that passed either way.
### BLOCKER
| Finding | Resolution |
|---|---|
| **`allowHtml` silently dropped by `addToast`** — the restore toast's "View
chart →" link rendered as dead text in production; every test was green because
every test injected the flag downstream of the drop (react; verified
line-by-line) | **Fixed** `6ddca1349a` — flag carried through the payload + an
integration-shaped test through the real action creator. This also vindicates
an earlier external bug report of the dead link that a previous investigation
on this branch had wrongly refuted (it tested Interweave's attribute handling,
one layer below the drop). |
### HIGH
| Finding | Resolution |
|---|---|
| **⚑5 Timezone seam**: `deleted_at` is stamped naive-server-local
(deliberately, matching every audit column); the archive UI parsed it as UTC
and filtered with client-UTC cutoffs — ages and time-range windows shifted by
the server offset on any non-UTC deployment, invisible to UTC-based CI (CD,
clean-code, preset, ddd, committer) | **Fixed** `ec97bd4e04` — display is
server-humanized (`deleted_at_delta_humanized`, same pattern as
`changed_on_delta_humanized`); presets send day counts through new per-model
`<type>_deleted_recency` filters resolved with the server's clock. Also retires
two related LOWs: cutoffs frozen at page-mount, and absolute timestamps
persisted into `?filters=` URLs. |
| **PR process gate**: stale draft-tracker body, no template sections,
superseded title (committer, preset) | **Fixed** — body rewritten to the
template, retitled. Screenshots to be attached by the author. |
### MEDIUM — fixed on this branch
| Finding | Resolution |
|---|---|
| **Bulk "Archive" hard-deleted semantic views** behind "you can recover
them there" copy, friction removed, toast counting them as archived (react;
endpoint verified to have no soft-delete) | **Fixed** `20cde5a05d` — mixed
selections keep the full danger treatment (type-DELETE gate) and say plainly
which items die permanently; the toast counts the two fates separately. |
| **⚑2 Welcome-dashboard purge blocker was FK-dependent**: on Postgres/MySQL
an IntegrityError misreported as a policy block and retried forever; on SQLite
(FKs off) the dashboard purged *successfully*, stranding a broken homepage
pointer while the audit said `confirmed` (tidy-first, preset) | **Fixed**
`794dbda1d8` — explicit guard in `_validate_deletion_allowed` beside the
ReportSchedule check; dialect-independent, with a nameable reason. Control:
with the guard stripped, the SQLite test shows the dashboard genuinely
destroyed. |
| **⚑2 IntegrityError conflation**: raw driver text (SQL + bind parameters)
reached the client toast as the 422 reason, and a genuine cascade-coverage bug
was indistinguishable from an intentional block at INFO (preset, python) |
**Fixed** `794dbda1d8` — curated client message ("blocked by database
references"); constraint detail to the log at WARNING where a coverage gap is
diagnosable. |
| **Fail-open audience default**: `_scope_to_restore_audience` returned the
query unchanged for models without an `editors` relationship — latent, but the
next `SoftDeleteMixin` adopter would expose its archive to anyone with list
access (python) | **Fixed** `c91e08f5c7` — non-admins fail closed. The
docstring also stops overclaiming "mirrors `raise_for_editorship`":
`EXTRA_EDITORS_RESOLVER` and guest-role editorship have no SQL form, so the
enumeration is documented as narrower than the true audience, never wider. |
| **Unaudited REST purge**: the command's fail-open on audit failure was
licensed by "an operator is present at a shell", but this PR routes
`@protect()`-ed REST users through it without checking `record_id` (ddd) |
**Fixed** `c91e08f5c7` — `require_audit` flag, set by the REST caller: an
unwritable audit answers 422 with the entity untouched. The CLI keeps its
operator-trust fail-open. |
| **Chart filter duplicated the restore-audience rule** — a second,
semantically identical editors-EXISTS stacked on the base's; a future audience
change would land in one and charts would enforce the intersection (ddd) |
**Fixed** `c91e08f5c7` — override deleted; the existing editor-visibility tests
pass on the base rule alone, which is the proof the copy added nothing. |
| **sd_acl test fixtures leaked past a failed setup** — observed
empirically: one leaked slice made 28 unrelated chart tests fail (python;
introduced by this branch's own visibility tests) | **Fixed** `a3441c77c9` —
creation moved inside try/finally with None-seeded cleanup, plus a permanent
probe test that injects a failure at the exact old boundary and asserts nothing
survives. |
| **⚑4 Visibility-filter docstring contradicted its own gate** — claimed
relationship loads are skipped while the predicate 20 lines up deliberately
includes them; a "fix" toward the wrong docstring would reintroduce a lazy-load
leak (clean-code, preset, ddd, tidy-first) | **Fixed** `4eca699bd1` — the
listener's docstring now defers to the predicate. |
| **⚑2 Translation catalogue not regenerated** — none of the feature's ~30
frontend strings were in `messages.pot`; the archive surface shipped
untranslatable (committer, confirmed by grep) | **Fixed** `8c6dfca5f1` — real
`babel_update.sh` run; all strings extracted into the pot and every language
catalogue. Noted in the commit: `lazy_gettext` FAB filter names are outside the
pipeline's keyword list (long-standing, applies to pre-existing siblings too),
and the script's `msgcat` step silently no-ops on GNU gettext. |
| **No UPDATING.md entry** for the purge API and modal changes; the datasets
entry's "until a purge capability lands" now answerable (committer) | **Fixed**
`e3cb4e18ca` — full entry incl. the status contract, the semantic-view
carve-out, the database-deletion resolution pointer, and `purge_audit_log`'s
never-pruned-by-design posture. |
### MEDIUM — deliberate follow-ups (not blocking this PR)
- **⚑4 Audit growth / blocked-entity churn** (python, sqlalchemy, CD,
preset): blocked entities re-attempted every run, one immutable audit row per
day forever, no backoff, no audit-retention story. Wants a
skip-if-latest-blocked rule + a retention policy.
- **⚑3 Slug-reuse 500 on MariaDB / MySQL <8.0.13 / SQLite** (ddd,
sqlalchemy, committer): dialects keeping the full unique constraint let
validation (which sees only active rows) pass and die at flush.
sqlalchemy-review's sharpening: MariaDB is a supported production metadata DB,
so this deserves a visibility-bypassed pre-check → clean 422.
- **Missing beat-schedule startup warning** (CD): custom `CELERY_CONFIG`
deployments silently never purge; the sibling versioning task already ships the
exact warning pattern to generalize.
- **⚑2 Dry-run honesty** (CD, ddd): docs and the confirm-modal quote a
retention deadline that `SOFT_DELETE_PURGE_DRY_RUN=True` (the shipped default)
never enforces.
- **Flag ON→OFF slug-twin hazard** (CD): a new dashboard claims an archived
slug; flag-off resurrects the twin — two visible dashboards, one slug.
- **Audit-orchestration duplication** (clean-code): the ~60-line write-ahead
protocol is copied between the scheduled and force purge paths; extracting it
is also where `require_audit` naturally lands.
- **css accessibility cluster**: `:focus-within` reveal wins only by emotion
injection order; the keyboard-invisibility fix should move into TableCollection
(one line fixes every list page); sub-24px touch targets beside a no-confirm
Recover; hover-only reveal leaves this page's only affordances invisible on
touch devices.
### LOW (grouped; tracked as follow-ups)
**⚑6 bypass-release family** (session-scoped visibility bypass released only
on the happy path; no refcounting against the context-manager owner) · **⚑3
reconcile-semantics family** (>1h in-flight purge finalized `failed`
permanently; attribution edges) · **⚑3 favstar rows survive purge** (with a
SQLite recycled-rowid twist) · **orphan-sweep candidate asymmetry**
(`end_transaction_id` never considered for sweeping — a leak, not a correctness
break) · legacy pre-subjects join tables cleaned only by DB cascade · **⚑3
softDelete copy triplication** across the three list pages + `'%s item(s)'`
pluralization · window-default duplication · deprecated `utcnow()` in test
helpers · "Archived by" is `changed_by` (background maintenance can overwrite
it) · chart "Name" search is all-text · menu/header casing drift · assorted
per-lens LOWs.
### Validation of prior rounds
The lenses also re-covered ground fixed in the two earlier review rounds
posted above — the rison type-switch crash, the discarded 422 reason, the
cross-model UUID confusion, the restore race (whose identity+eligibility
predicate design was repeatedly *praised* rather than re-flagged), the CLI
`--type` handling — and none re-found them broken.
### Method notes, for calibration
- css and ddd could not load their skill definitions in subagents and
reviewed directly with the named discipline; python and sqlalchemy loaded
theirs manually. The remaining six ran as their named skills.
- The empirical runs were done against clean, freshly-migrated databases
after one lens traced local flakiness to a stale shared fixture DB (and another
traced *that* to the sd_acl leak fixed above).
- Backend suites at head: retention + soft-delete integration and unit
suites green; frontend: ArchivedList/DatasetList/MessageToasts suites green.
_This review and the fixes were generated by Claude (AI) on behalf of
@mikebridge._
--
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]
