fix(versioning): pin shadow-row reads and restore to (id, uuid) - #42797
Conversation
Version reads and restore matched shadow rows on the entity's reusable integer id alone. A hard delete frees that id and the database may hand it to the next row inserted — guaranteed on SQLite ROWID tables, and reachable anywhere a sequence is reset. A successor created under a recycled id therefore inherited the deleted predecessor's version history, and a restore could write the predecessor's content over it. The shadow tables already carry `uuid`, so every read now pins both columns through a shared `_identity_filter`, and restore's target lookup carries the uuid alongside the id and transaction. A reused id matches nothing instead of matching a stranger. `current_live_version_uuid` was a quiet second instance: it accepted an entity_uuid but used it only to *derive* the client-visible version uuid, delegating the row lookup to the id-only `current_live_transaction_id`. It therefore produced an ETag built from the successor's uuid and the predecessor's transaction. It now threads the uuid into the lookup too. This mirrors `_identity_predicates` in the purge cascade — the identical bug class was found and fixed twice on the soft-delete side (`_purge_one` in apache#41549, the locked claim in apache#41550); the versioning read/restore paths never received the pattern. Five independent review lenses converged on it in the whole-system capstone. Callers: only `api_helpers.current_entity_version_info` reaches these functions, and it already resolves `entity_uuid` for its own use, so it just passes it along. The uuid parameter is optional and the filter falls back to id-only without it, so no caller is broken. Tests: `id_reuse_tests.py` recreates the scenario — create, capture, hard-delete, recreate under the freed id — and asserts the successor reports only its own history, that a deleted predecessor resolves no live transaction, and that restoring the successor to a predecessor's transaction refuses rather than overwrites. A fourth test guards the fixture itself by proving both entities' shadow rows really do coexist under the shared id, so the others cannot pass vacuously. The suite skips where the backend does not recycle the id. Control: reverting both sites to id-only fails all three behavioural tests (3 failed / 1 passed). Verified: 4/4 integration, 118/118 versioning unit, pre-commit green. Local test DB needed `superset db upgrade` first — it predated f3a8c1d2e9b7. Refs sc-115614; gates ENABLE_VERSIONING_CAPTURE enablement (sc-111918 stage 2b). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code Review Agent Run #d0e756Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42797 +/- ##
==========================================
- Coverage 65.79% 65.79% -0.01%
==========================================
Files 2842 2842
Lines 162127 162141 +14
Branches 37158 37160 +2
==========================================
+ Hits 106676 106681 +5
- Misses 53387 53394 +7
- Partials 2064 2066 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
CI's test-sqlite lane caught this: test_put_response_returns_old_and_new_ version_numbers asserted `old_version == 56` while the API reported 0. The test computed its expectation with an id-only count of shadow rows, which is the very query this branch fixes. The shared test database recycles integer ids across the suite, so that count included 56 rows belonging to hard-deleted predecessors — the dashboard under test has exactly one version of its own. The assertion was pinning the buggy behaviour, and the fix exposed it. Worth stating plainly: this is the defect reproducing unprompted in CI, on SQLite, without anyone constructing it. It is the strongest evidence available that the window is real rather than theoretical. Both entity suites' helpers and count-before computations now pin (id, uuid), matching what the production lookup does. Verified: 102 passed / 4 skipped across the versioning and chart-restore integration suites; pre-commit green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the previous commit, which fixed two sites and left the rest. CI's test-sqlite lane then failed at a third: test_restore_applies_scalar_field got a 404, because it selected its target row from an id-only query — picking up a hard-deleted predecessor's transaction — and the production fix correctly refused to restore an entity to a stranger's version. Every remaining `filter(ver_cls.id == ...)` in both entity suites now carries the uuid. `entity_uuid` was already in scope at each site; the lookups simply were not using it. The lesson is worth recording: fixing these one CI round at a time was slower and less honest than sweeping the file. The first pass changed the two sites the failure named, when the same pattern was visible at seven more in the same files. Verified: 8/8 dashboard restore, 102 passed / 4 skipped across chart restore and the versioning integration suite, pre-commit green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first pass pinned the ORM lookups (ver_cls.id) but missed the Core queries built on ver_tbl.c, so three shadow-row reads still matched on the reusable integer id alone: - list_versions: listed every row under the id, so the version-history panel showed a hard-deleted predecessor's versions as the successor's. - get_version: worse than a mismatch. version_num is counted through the pinned helper but applied to this query as an OFFSET, so an offset counted over one row set addressed a row in a wider one -- returning a predecessor's content labelled with the successor's version uuid. - factory's unchanged-value probe: compared a save against whatever open shadow row carried the id. Where an entity was deleted and recreated identically the values match, so a genuine edit reads as a no-op and is never captured. identity_filter only touches .id and .uuid, which both a version class and a table's .c collection expose, so one predicate now serves the ORM and Core paths and they cannot drift apart again. Renamed off the underscore now that it is shared across modules. The existing tests passed over all three because none of them called list_versions or get_version -- the two most user-visible read paths. Two tests now cover them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The main HIGH — MEDIUM — The API history, snapshot, ETag transaction, and restore queries are correctly pinned. Focused tests and pre-commit passed locally. The CodeAnt snapshot-query finding is resolved at HEAD; its concurrency concern is real but pre-existing rather than introduced here. Verdict: request changes until the baseline probe and transition regression are covered. |
|
Follow-up after implementation at
Verification: 13 focused tests pass; affected-file pre-commit passes; the full repository pre-commit suite passes with only |
|
Launch-review follow-up at
Verification: 13 focused tests pass; MyPy and affected-file pre-commit pass; full pre-commit passes apart from the documented unrelated frontend formatter skip. |
Code Review Agent Run #43ea00Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
@mikebridge LGTM. Already been through a proper shakedown (atomicity fix in the last commit, baseline probe pinned to uuid too) so nothing left for me to flag.
Flips the two versioning release defaults for general availability
(sc-111918 stage 2b, FR-001/002/008):
VERSION_HISTORY False -> True
ENABLE_VERSIONING_CAPTURE false -> true (env-var default)
Every save of a chart, dashboard, or dataset now writes version rows, and
the history panel appears on Explore and Dashboard pages.
The two flip together by design (FR-008). The panel reads what capture
writes, so panel-on with capture-off renders an empty "No history yet"
that misrepresents an edited entity as unchanged — the one combination
that is worse than either switch being off.
ENABLE_VERSIONING_CAPTURE is RETAINED permanently (FR-002) as the
operational kill-switch, not removed with the rollout toggles. Its
comment previously read as an instruction for this change ("activated by
flipping this default to on once validated in production"); it now
describes the switch's standing purpose instead.
Scope: versioning only. The soft-delete flips are apache#42800 — FR-009
permits the two shipping in different releases, and they answer to
different gates: a SIP-210 [VOTE] here, an internal determination there.
UPDATING.md gains the FR-006 entry: storage growth and the retention beat
task that bounds it, the PUT response-shape change (old_version_uuid /
new_version_uuid and ETag are now populated), and the note that unlike
the soft-delete toggle, turning capture off is a *clean* stop — existing
rows stay readable and no entity state is altered. A stale "(ships off)"
parenthetical further down is corrected.
docs/static/feature-flags.json regenerated by the docs-sync hook.
Depends on: apache#42797 (shadow-row reads pinned to (id, uuid)). Capture
on by default widens the id-reuse window that PR closes — purge frequency
rises as soft delete graduates, and recycled ids are deterministic on
SQLite — so it must land first.
Verified: 164/164 unit (versioning, config, initialization), 84/84 + 4
skipped versioning integration, pre-commit green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
SUMMARY
Version shadow-row reads and restore previously matched entities by reusable integer id alone. After a hard delete, a new entity can receive that id and accidentally inherit the predecessor history; restore could then write predecessor content over the successor.
The shadow tables already carry UUIDs. All entity-history operations now require and filter by the complete
(id, uuid)identity:versioning/queries.pyidentity_filterrequires both identifiers_get_version_count/current_version_numbercurrent_live_transaction_id/current_live_version_uuidlist_versionsresolve_version/get_versioncurrent_entity_version_inforestore_versionbaseline.shadow_row_countThe integer-id-only fallback was removed from the production helpers. Tests use explicit id-only control queries solely to prove the collision exists.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend correctness fix with no UI surface.
TESTING INSTRUCTIONS
Run:
The seven behavioral tests cover:
Also verified locally:
Result: 13 passed.
Affected-file pre-commit and the Python review pass. The full repository pre-commit suite passes with
oxfmt-frontendskipped because that all-files hook rewrites unrelated frontend files outside this Python-only PR.ADDITIONAL INFORMATION
Rollout relevance: this should land before broad
ENABLE_VERSIONING_CAPTUREenablement. Retention purges can make integer-id reuse more frequent, so history identity must be UUID-pinned before capture becomes the default.🤖 Generated with Claude Code