mikebridge commented on PR #44027: URL: https://github.com/apache/superset/pull/44027#issuecomment-5593521756
@aminghadersohi — requesting review on the conditional-write lock fix (sc-120014), the update-path sibling of #44015's restore fix and squarely in your versioning lane. The dataset PUT's `If-Match` path row-locked the entity with an id-only `SELECT ... FOR UPDATE` that discarded the values. On MySQL/InnoDB REPEATABLE READ, the transaction's read view is pinned by its first consistent read (the request's auth queries), so the entity the update command loaded after that lock still reflected the pre-lock snapshot — and the ORM, which only emits UPDATEs for attributes differing from the *loaded* values, could silently lose a concurrent commit that landed between the load and the lock. Fix: the lock is now a full-entity ORM read — `populate_existing()` + `with_for_update()` — so the locking read (exempt from the RR snapshot) writes current committed data into the identity-map object the command's `find_by_id` returns. Two hardenings from the adversarial review rounds worth knowing: - **Lifetime contract**: the helper *returns* the locked entity and the dataset PUT binds it for the rest of the request — the identity map references clean objects weakly, so a discarded result was collectible immediately, which would have silently reduced the fix to a production no-op (caught by the cross-model committer pass). Pinned both ways: held → `find_by_id` returns the very same refreshed instance; discarded → a weakref proves collection. - **Scoped honestly**: the refresh covers the entity row; the If-Match validator's own version-info reads (and lazy child-collection loads) have the same RR staleness one layer up and are tracked as a separate follow-up rather than folded in — the docstrings and the endpoint comment say exactly what is and isn't covered. Tests: 3 unit statement-shape pins (dropping either flag fails on every backend; mirrors #44015's refresh-flag guard) + 3 two-transaction integration tests (object-level staleness, so the interleave test fails pre-fix on every backend it runs on; SQLite skipped — a second writer connection can't model a concurrent request there). CI green including the full DB matrix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01JRLEJS4mUqKBoPjSjviKUW -- 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]
