fitzee commented on code in PR #44015:
URL: https://github.com/apache/superset/pull/44015#discussion_r3995855428
##########
superset/commands/version_restore.py:
##########
@@ -89,6 +89,44 @@ def _perform() -> RestoreResult:
def _do_restore(self) -> RestoreResult:
entity = self.validate()
+
+ # Re-read the live row under a FOR UPDATE lock, refreshing the
+ # in-memory entity (``populate_existing``) from the *current committed*
+ # state, and re-assert it is still active (``deleted_at IS NULL``). A
+ # single locking query closes three races opened between validate()'s
+ # unlocked read and the revert:
+ # * a concurrent content edit — a plain, non-locking read (bare
+ # refresh()) returns the transaction's first-read snapshot on
+ # MySQL/InnoDB REPEATABLE READ and would silently drop the edit
+ # from the revert UPDATE;
+ # * a concurrent hard delete — the row is gone, so the query returns
+ # None;
+ # * a concurrent soft delete — column loads (get()/refresh()) bypass
+ # the global active-row filter, so without the explicit
+ # ``deleted_at IS NULL`` predicate the revert would resurrect an
+ # archived entity and report success.
+ # A None result (hard- or soft-deleted) is surfaced as the documented
+ # 404 — not the transaction wrapper's generic 422, and not via a
+ # refresh() whose missing-row failure is a hard-to-catch
+ # InvalidRequestError. This is the pessimistic (serialise) half; the
+ # restore endpoint does not yet also honor an If-Match precondition to
+ # *detect* (rather than serialise) a concurrent edit — a follow-up.
+ entity = (
+ db.session.query(self.model_cls)
+ .populate_existing()
+ # Disable eager loaders before FOR UPDATE. A ``lazy="subquery"``
+ # relationship (e.g. ``Slice.table``) wraps the primary query into
+ # ``SELECT DISTINCT … FOR UPDATE`` to fetch its related rows, and
+ # Postgres rejects ``FOR UPDATE`` with ``DISTINCT``. We only need
the
+ # locked row's own columns here; relationships load lazily after.
+ .enable_eagerloads(False)
+ .filter_by(id=entity.id, deleted_at=None)
Review Comment:
**The lock re-read pins by `id` alone, not `(id, uuid)` — inconsistent with
`restore_version`'s own id-reuse defense, and yields a 500 instead of a 404.**
`restore.py:138-142` deliberately pins the version lookup to `(id, uuid)` with
the comment: *"a hard delete frees the integer id, so matching on it alone can
resolve a predecessor's version row and restore its content over the current
entity."* This locking query matches by `id` + `deleted_at` only.
Race: between `validate()` and this lock, the original entity is
hard-deleted and its integer id is reused by a new row (explicit-id insert /
seq reuse). This query locks that *new* row; `populate_existing()` swaps
`entity` to it (a different uuid). `resolve_version` still runs against the
original `self._uuid`, and `restore_version` then hits `entity.uuid !=
entity_uuid` and raises `ValueError` (restore.py:128). `ValueError` isn't a
`SQLAlchemyError`, so `on_error` re-raises it as-is → **500**, not the
documented 404. Adding `uuid=entity.uuid` to the `filter_by` closes the window
cleanly (row absent → `one_or_none()` → None → 404), and matches the pinning
the codebase already warns is required.
##########
tests/integration_tests/charts/version_restore_tests.py:
##########
@@ -162,6 +167,155 @@ def test_restore_refuses_externally_managed_chart(self)
-> None:
chart.slice_name = "Boys"
db.session.commit()
+ def test_restore_fully_overwrites_a_concurrently_committed_edit(self) ->
None:
+ """sc-115423: end-to-end, a restore fully overwrites an edit committed
+ by another connection — the concurrent value does not survive.
+
+ The dialect-independent regression guard for the fix is the unit test
+ asserting ``refresh(..., with_for_update=True)`` (dropping the flag
Review Comment:
**This docstring describes an implementation that wasn't shipped.** It calls
the dialect-independent guard "the unit test asserting `refresh(...,
with_for_update=True)`." But the shipped code uses
`db.session.query(...).populate_existing().enable_eagerloads(False).filter_by(id=…,
deleted_at=None).with_for_update().one_or_none()` — no `refresh()` — and the
unit test (`test_restore_version_concurrency.py`) asserts exactly that query
chain (`populate_existing` / `with_for_update` / `filter_by` / `one_or_none`),
not any `refresh()` call. A maintainer reading this would look for a
`refresh(..., with_for_update=True)` that doesn't exist, and might "restore"
the `refresh()` form the code comments explicitly call buggy under REPEATABLE
READ. Worth updating the docstring to name the actual guard.
##########
superset/commands/version_restore.py:
##########
@@ -89,6 +89,44 @@ def _perform() -> RestoreResult:
def _do_restore(self) -> RestoreResult:
entity = self.validate()
+
+ # Re-read the live row under a FOR UPDATE lock, refreshing the
+ # in-memory entity (``populate_existing``) from the *current committed*
+ # state, and re-assert it is still active (``deleted_at IS NULL``). A
+ # single locking query closes three races opened between validate()'s
+ # unlocked read and the revert:
+ # * a concurrent content edit — a plain, non-locking read (bare
+ # refresh()) returns the transaction's first-read snapshot on
+ # MySQL/InnoDB REPEATABLE READ and would silently drop the edit
+ # from the revert UPDATE;
+ # * a concurrent hard delete — the row is gone, so the query returns
+ # None;
+ # * a concurrent soft delete — column loads (get()/refresh()) bypass
+ # the global active-row filter, so without the explicit
+ # ``deleted_at IS NULL`` predicate the revert would resurrect an
+ # archived entity and report success.
+ # A None result (hard- or soft-deleted) is surfaced as the documented
+ # 404 — not the transaction wrapper's generic 422, and not via a
+ # refresh() whose missing-row failure is a hard-to-catch
+ # InvalidRequestError. This is the pessimistic (serialise) half; the
+ # restore endpoint does not yet also honor an If-Match precondition to
+ # *detect* (rather than serialise) a concurrent edit — a follow-up.
+ entity = (
+ db.session.query(self.model_cls)
Review Comment:
**Minor (maintainability):** this inline FOR UPDATE re-read is a second
row-lock formulation alongside
`versioning/api_helpers.py:lock_entity_for_update` (used by the
conditional-write PUT path). The two must serialize on the same row, yet lock
differently — PUT uses `select(model.id).where(id==…).with_for_update()`,
restore uses `query(model).filter_by(id=…,
deleted_at=None).…with_for_update()`. They do have different needs (restore
also refreshes row content via `populate_existing`, which the helper doesn't),
so they're not trivially interchangeable — but a future change to one's locking
semantics won't be reflected in the other. Low priority; flagging so the
divergence is a deliberate choice rather than an accident.
##########
tests/integration_tests/charts/version_restore_tests.py:
##########
@@ -162,6 +167,155 @@ def test_restore_refuses_externally_managed_chart(self)
-> None:
chart.slice_name = "Boys"
db.session.commit()
+ def test_restore_fully_overwrites_a_concurrently_committed_edit(self) ->
None:
+ """sc-115423: end-to-end, a restore fully overwrites an edit committed
+ by another connection — the concurrent value does not survive.
+
+ The dialect-independent regression guard for the fix is the unit test
+ asserting ``refresh(..., with_for_update=True)`` (dropping the flag
+ fails there on every backend). This test exercises the real command +
+ DB through the locking-refresh path and asserts the correct end state.
+ It reproduces the MySQL/InnoDB REPEATABLE-READ staleness the flag
+ guards against *only* when the restore shares this session's pre-edit
+ read view (no commit between the load below and the ``@transaction``
+ restore); where that holds, the pre-fix (plain-refresh) code leaves the
+ concurrent edit in place and this assertion fails. It is not relied on
+ as the sole MySQL guard for that reason.
+ """
+ _persist_fixture_state()
+ chart: Slice = (
+ db.session.query(Slice).filter(Slice.slice_name == "Boys").first()
+ )
+ assert chart is not None
+ chart_id = chart.id
+ chart_uuid = chart.uuid
+
+ # Edit + commit so there is a version whose value equals the *current*
+ # live value — that version is the restore target.
+ chart.slice_name = "Boys v1"
+ db.session.commit()
+
+ self.login(ADMIN_USERNAME)
+ listing = _json.loads(self._list(str(chart_uuid)).data.decode("utf-8"))
+ target = listing["result"][-1] # the latest version == "Boys v1"
+ target_uuid = UUID(target["version_uuid"])
+
+ # Load the chart into THIS session (establishing its read snapshot /
+ # identity map) BEFORE the concurrent edit; then commit an edit from a
+ # SEPARATE connection. This is the interleaving a non-locking refresh
+ # would miss.
+ loaded = db.session.query(Slice).filter(Slice.id == chart_id).one()
+ assert loaded.slice_name == "Boys v1" # loaded == restore target
+ with db.engine.begin() as conn:
+ conn.execute(
+ sa.text("UPDATE slices SET slice_name = :n WHERE id = :i"),
+ {"n": "edited by another connection", "i": chart_id},
+ )
+
+ RestoreChartVersionCommand(chart_uuid, target_uuid).run()
+
+ db.session.expire_all()
+ live = db.session.query(Slice).filter(Slice.id == chart_id).one()
+ assert live.slice_name == "Boys v1", (
+ f"restore did not fully overwrite the concurrent edit:
{live.slice_name!r}"
+ )
+
+ # Cleanup
+ live.slice_name = "Boys"
+ db.session.commit()
+
+ def test_restore_raises_not_found_when_hard_deleted_before_lock(self) ->
None:
+ """sc-115423: a concurrent hard delete committed between validate()'s
+ unlocked read and the FOR UPDATE lock must surface as the documented
+ 404 (``not_found_exc``), not the transaction wrapper's generic 422.
+
+ The race is injected deterministically: ``validate`` is patched to
+ return the live entity and, as its side effect, commit the delete from
+ a separate connection — exactly the window the locking re-read closes.
+ The pre-fix code (bare ``refresh()``) raised ``InvalidRequestError``
+ here, which ``on_error`` wrapped into ``failed_exc`` (422).
+ """
+ _persist_fixture_state()
+ chart: Slice = (
+ db.session.query(Slice).filter(Slice.slice_name == "Boys").first()
+ )
+ assert chart is not None
+ chart_id = chart.id
+ chart_uuid = chart.uuid
+
+ chart.slice_name = "Boys v1"
+ db.session.commit()
+ self.login(ADMIN_USERNAME)
+ listing = _json.loads(self._list(str(chart_uuid)).data.decode("utf-8"))
+ target_uuid = UUID(listing["result"][-1]["version_uuid"])
+ loaded = db.session.query(Slice).filter(Slice.id == chart_id).one()
+
+ def _hard_delete_then_return() -> Slice:
+ # Separate connection/transaction — a genuine second session. Drop
+ # the M2M attachment rows first to satisfy the dashboard_slices FK,
+ # then the live row.
+ with db.engine.begin() as conn:
+ conn.execute(
+ sa.text("DELETE FROM dashboard_slices WHERE slice_id =
:i"),
+ {"i": chart_id},
+ )
+ conn.execute(
+ sa.text("DELETE FROM slices WHERE id = :i"), {"i":
chart_id}
+ )
+ return loaded
+
+ cmd = RestoreChartVersionCommand(chart_uuid, target_uuid)
+ with patch.object(cmd, "validate",
side_effect=_hard_delete_then_return):
+ with pytest.raises(cmd.not_found_exc):
+ cmd.run()
+
+ def test_restore_refuses_when_soft_deleted_before_lock(self) -> None:
+ """sc-115423: a concurrent soft delete (``deleted_at`` set) committed
+ between validate() and the lock must refuse — not silently resurrect
+ the archived entity and report success.
+
+ Column loads (``get()``/``refresh()``) bypass the global active-row
+ filter, so the fix's explicit ``deleted_at IS NULL`` predicate on the
+ locking query is what makes the soft-deleted row read as absent
+ (``one_or_none()`` → None → ``not_found_exc``). Without it the revert
+ would run against the archived row.
+ """
+ _persist_fixture_state()
+ chart: Slice = (
+ db.session.query(Slice).filter(Slice.slice_name == "Boys").first()
+ )
+ assert chart is not None
+ chart_id = chart.id
+ chart_uuid = chart.uuid
+
+ chart.slice_name = "Boys v1"
+ db.session.commit()
+ self.login(ADMIN_USERNAME)
+ listing = _json.loads(self._list(str(chart_uuid)).data.decode("utf-8"))
+ target_uuid = UUID(listing["result"][-1]["version_uuid"])
+ loaded = db.session.query(Slice).filter(Slice.id == chart_id).one()
+
+ def _soft_delete_then_return() -> Slice:
+ with db.engine.begin() as conn:
+ conn.execute(
+ sa.text("UPDATE slices SET deleted_at = :ts WHERE id =
:i"),
+ {"ts": datetime.now(timezone.utc), "i": chart_id},
+ )
+ return loaded
+
+ cmd = RestoreChartVersionCommand(chart_uuid, target_uuid)
+ with patch.object(cmd, "validate",
side_effect=_soft_delete_then_return):
+ with pytest.raises(cmd.not_found_exc):
+ cmd.run()
+
+ # Cleanup — clear the archival flag so a shared/session-scoped row does
Review Comment:
**Soft-delete cleanup runs only if the command raises the expected
exception.** This `UPDATE slices SET deleted_at = NULL` sits *after* the `with
pytest.raises(cmd.not_found_exc)` block. If `cmd.run()` raises anything other
than `not_found_exc` (e.g. the id-reuse `ValueError`→500 path, or a lock
timeout), `pytest.raises` propagates it and this cleanup never executes —
leaving the shared/session-scoped `Boys` chart soft-deleted and breaking later
tests that query it. A `try/finally` or a fixture teardown (also for the
`slice_name` reset in the concurrent-edit test) would make the cleanup
unconditional.
--
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]
