mikebridge opened a new pull request, #44027: URL: https://github.com/apache/superset/pull/44027
### SUMMARY The dataset PUT's conditional-write path (`If-Match` saves) serialises check-and-write by row-locking the entity first (`lock_entity_for_update`, called from `superset/datasets/api.py`). But the lock was an id-only `SELECT id ... FOR UPDATE` that discarded the row's values — it serialised writers without refreshing what the transaction can *see*: - On MySQL/InnoDB default **REPEATABLE READ**, every consistent read in a transaction is pinned to the read view established by its first read (the request's auth queries run well before the lock). So the entity loaded after that id-only lock still reflected the **pre-lock snapshot**, and the ORM — which only emits `UPDATE`s for attributes that differ from the *loaded* values — could silently lose a concurrent commit that landed between the initial load and the lock. This is the same class as the version-restore partial-write fixed in #44015, on the ordinary update path. - The staleness is also **object-level on every backend**: SQLAlchemy never refreshes an already-loaded entity from a later plain `SELECT`, so even where the row read is fresh (Postgres READ COMMITTED), the identity-map object the update command operates on stayed stale. **Fix** (mirroring #44015's locking-refresh): take the lock as a full-entity ORM read — `db.session.query(model).populate_existing().filter(id).with_for_update().one_or_none()`. A locking read is exempt from the REPEATABLE READ snapshot and returns current committed data; `populate_existing()` writes it into the identity-map object, which the update command's later `find_by_id` returns without re-hydrating from its own (possibly stale) row. Missing-row semantics are unchanged: the result is discarded and existence keeps being decided by the command's own lookup (404 as before). Chart/dashboard PUTs have no conditional-write lock path, so the fix is dataset-scoped at the single shared helper. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A — concurrency fix with no UI or response-shape change. ### TESTING INSTRUCTIONS - `pytest tests/unit_tests/versioning/test_lock_entity.py` — 3 tests: the dialect-independent regression guard pins the statement shape (`populate_existing()` **and** `with_for_update()` both chained, result consumed via `one_or_none()`; dropping either fails on every backend — the analogue of #44015's `refresh(..., with_for_update=True)` pin), plus the non-numeric/None id early-outs that keep the `/<pk>` string route from raising a SQL cast error. Reverting the helper flips the shape test. - `pytest tests/integration_tests/versioning/conditional_write_lock_tests.py` — two-transaction behavioral proof: a second engine-level connection commits a change between this session's entity load and the lock; after `lock_entity_for_update` the loaded entity must show the concurrently committed value (fails pre-fix on every backend, since the staleness is object-level), plus a quiet-path control asserting the refresh is a no-op (and dirties nothing) when nothing changed underneath. Skipped on SQLite, where a second writer connection deadlocks against the open read transaction instead of modelling a concurrent request — CI's Postgres/MySQL runs execute them (local env here is SQLite-backed, so these two are CI-verified). - Manual (MySQL): open a dataset edit in one tab; from a second session commit a change to another field; save the first tab with `If-Match`. Before the fix the first save's flush could quietly discard the concurrent change on MySQL; after, the command diffs against committed state. ### 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)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API 🤖 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]
