mikebridge opened a new pull request, #42797: URL: https://github.com/apache/superset/pull/42797
### SUMMARY 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 entity 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`. Every read now pins both columns through a shared `_identity_filter`, and restore's target lookup carries the uuid alongside the id and transaction, so a reused id matches nothing rather than matching a stranger. Sites fixed: | Location | Was | |---|---| | `versioning/queries.py` `_get_version_count` | `ver_cls.id == entity_id` | | `versioning/queries.py` `current_live_transaction_id` | `ver_cls.id == entity_id` | | `versioning/queries.py` `list_versions` | `ver_cls.id == entity.id` | | `versioning/restore.py` target lookup | `ver_cls.id == entity.id` | **A fourth site not named in the ticket:** `current_live_version_uuid` already 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 as well. This mirrors `_identity_predicates` in the purge cascade. The identical bug class was found and fixed **twice** on the soft-delete side — `_purge_one`'s identity guard, and the locked claim composing `(id, uuid)` — but the versioning read/restore paths never received the pattern. Five independent review lenses converged on it in the whole-system capstone. **Compatibility:** the `entity_uuid` parameter is optional and `_identity_filter` falls back to id-only without it, so no existing caller breaks. In practice only `api_helpers.current_entity_version_info` reaches these functions, and it already resolves `entity_uuid` for its own use — it just passes it along now. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A — backend correctness fix, no UI surface. ### TESTING INSTRUCTIONS `pytest tests/integration_tests/versioning/id_reuse_tests.py` — four tests recreating the exact scenario: create → capture a second version → hard-delete → recreate under the freed id. 1. the successor reports only its own history (not the predecessor's) 2. a hard-deleted predecessor resolves **no** live transaction, where the id-only lookup would answer with the successor's 3. restoring the successor to a transaction belonging to the predecessor **refuses** rather than overwriting — the destructive half 4. a guard on the fixture itself, proving both entities' shadow rows genuinely coexist under the shared id, so the other three cannot pass vacuously The suite skips where the backend does not recycle the id; reuse is deterministic on SQLite ROWID tables, which is what makes this reproducible at all. **Control run:** reverting both sites to id-only matching fails all three behavioural tests (3 failed / 1 passed) — the passing one being the fixture guard, correctly indifferent to the fix. Also verified: 118/118 existing versioning unit tests, pre-commit green. One local-environment note for reviewers running this: the integration test database needs `superset db upgrade` if it predates `f3a8c1d2e9b7` (the reports-retry migration). ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Introduces new feature or API - [ ] Removes existing feature or API **Rollout relevance:** this gates broad `ENABLE_VERSIONING_CAPTURE` enablement. Purge frequency rises as soft-delete graduates (retention purges recycle ids on SQLite), so the window for this defect widens exactly as the features roll out — which is why it is worth landing before the capture default flips rather than after. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
