mikebridge commented on code in PR #44015:
URL: https://github.com/apache/superset/pull/44015#discussion_r4007083902
##########
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:
Good catch — fixed in 0e8c13aa6f: the lock now pins `(id, uuid, deleted_at
IS NULL)`, so a hard delete + integer-id reuse reads as absent and
`one_or_none()` → None surfaces the documented 404 instead of
`restore_version`'s `ValueError` → 500. Matches the `(id, uuid)` pinning
`restore_version` already applies to the version lookup. The unit guard now
asserts the uuid is part of the locking filter (dropping it fails 3 of 5
there), and the comment block records this as the fourth closed race.
##########
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:
Fixed in 0e8c13aa6f — the docstring now names the actual guard (the
`populate_existing().enable_eagerloads(False).filter_by(id=…, uuid=…,
deleted_at=None).with_for_update().one_or_none()` chain asserted by
`test_restore_version_concurrency.py`) and says explicitly that there is no
`refresh()` and why, so nobody restores the form the code calls buggy under
REPEATABLE READ.
##########
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:
Fixed in 0e8c13aa6f — both cleanups (the `deleted_at` reset here and the
`slice_name` reset in the concurrent-edit test) are now in `try/finally`,
issued over a separate connection after a `session.rollback()`, so they commit
regardless of what `run()` raises.
##########
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:
Agreed it should be deliberate rather than accidental — recorded in
0e8c13aa6f in the same comment block: restore is its own formulation because it
must also reload the locked row's content (`populate_existing`), assert the
active-row predicate, and pin the uuid, whereas `lock_entity_for_update` locks
`select(model.id)` by id and returns nothing; both lock the same PK row, so the
two paths still serialise against each other.
--
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]
