You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/09/14 16:57:04 UTC
(superset) branch master updated: fix(dataset): refresh the entity under the conditional-write row lock (#44027)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 818857d3d98 fix(dataset): refresh the entity under the conditional-write row lock (#44027)
818857d3d98 is described below
commit 818857d3d98a9b08a5ddefe45bf504d2aebc35fb
Author: Mike Bridge <mi...@preset.io>
AuthorDate: Mon Sep 14 10:56:57 2026 -0600
fix(dataset): refresh the entity under the conditional-write row lock (#44027)
Co-authored-by: Claude Fable 5 <no...@anthropic.com>
---
superset/datasets/api.py | 10 +-
superset/versioning/api_helpers.py | 58 +++++-
.../versioning/conditional_write_lock_tests.py | 225 +++++++++++++++++++++
tests/unit_tests/versioning/test_lock_entity.py | 97 +++++++++
4 files changed, 384 insertions(+), 6 deletions(-)
diff --git a/superset/datasets/api.py b/superset/datasets/api.py
index f1d90184b64..9e2ae21a0ab 100644
--- a/superset/datasets/api.py
+++ b/superset/datasets/api.py
@@ -708,8 +708,16 @@ class DatasetRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi):
# the live version, the command writes, and the two must not interleave
# with another request's. Only a conditional save pays for the lock; an
# unconditional PUT behaves exactly as it did before the guard existed.
+ # (On MySQL REPEATABLE READ the version read below is still a plain
+ # consistent read and can predate the lock; see the caveats on
+ # lock_entity_for_update.)
if is_conditional_write():
- lock_entity_for_update(SqlaTable, pk)
+ # Hold the locked entity for the rest of the request: the
+ # session's identity map references clean objects weakly, so
+ # discarding the return value could let the refreshed object
+ # be collected and the command's find_by_id re-read a stale
+ # row on MySQL REPEATABLE READ (see lock_entity_for_update).
+ _locked_entity = lock_entity_for_update(SqlaTable, pk)
# Live version identifiers before the update (empty + query-free when
# ``ENABLE_VERSIONING_CAPTURE`` is off).
diff --git a/superset/versioning/api_helpers.py b/superset/versioning/api_helpers.py
index 6d01948dfee..dfe0e2d63a7 100644
--- a/superset/versioning/api_helpers.py
+++ b/superset/versioning/api_helpers.py
@@ -187,8 +187,10 @@ def entity_concurrency_token(
) or unversioned_entity_token(entity_uuid)
-def lock_entity_for_update(model_cls: type[Model], entity_id: int | None) -> None:
- """Row-lock *entity* so a conditional write's check and its update are atomic.
+def lock_entity_for_update(
+ model_cls: type[Model], entity_id: int | None
+) -> Model | None:
+ """Row-lock *entity*, refresh it to committed data, and return it.
``If-Match`` is verified against a read taken before the update command
runs. Without a lock two overlapping requests can both read the same live
@@ -197,16 +199,62 @@ def lock_entity_for_update(model_cls: type[Model], entity_id: int | None) -> Non
held until the command commits, because both run in the same scoped
session.
+ An id-only locking ``SELECT`` would serialise writers without fixing
+ what this transaction can *see*: InnoDB's default REPEATABLE READ pins
+ every consistent read to the snapshot established by the transaction's
+ first read (the request's auth queries), so an entity load issued after
+ such a lock still returns the pre-lock snapshot -- and the ORM, which
+ only emits UPDATEs for attributes that differ from the *loaded* values,
+ can silently discard a concurrent commit that landed before the lock.
+
+ The lock is therefore taken as a full-entity ORM read.
+ ``with_for_update()`` makes it a locking read, exempt from the
+ REPEATABLE READ snapshot, returning current committed data;
+ ``populate_existing()`` writes that data into the identity-map object,
+ which a later plain lookup (e.g. the update command's ``find_by_id``)
+ returns without re-hydrating from its own stale row. Call this before
+ the session's entity is modified: the refresh overwrites pending
+ attribute state, and the query's autoflush would flush earlier
+ mutations mid-request.
+
+ The caller MUST hold the returned entity for as long as the refreshed
+ state matters (through the update command's run). The identity map
+ references clean persistent objects weakly, so a discarded return
+ value can be garbage-collected immediately -- after which the
+ command's lookup re-hydrates a new object from a plain read, which on
+ MySQL REPEATABLE READ is the pre-lock snapshot again, silently
+ undoing the refresh.
+
+ The refresh covers the entity row itself. Lazy-loaded child
+ collections (columns, metrics) are still plain consistent reads
+ afterwards, as is the version-info read behind the ``If-Match``
+ comparison -- on MySQL REPEATABLE READ both can still observe the
+ pre-lock snapshot.
+
+ Postgres (READ COMMITTED) and SQLite are not exposed to the staleness,
+ and the refresh is harmless there. (Version restore has the same
+ staleness class; the matching locking-refresh fix is proposed in
+ apache/superset#44015.)
+
Renders no ``FOR UPDATE`` on SQLite, which serialises writers anyway.
+
+ Missing rows are not this function's concern: ``None`` is returned
+ (soft-deleted rows filter out like any ORM read), and existence keeps
+ being decided by the update command's own lookup (404 semantics
+ unchanged).
"""
try:
# The PUT route declares ``/<pk>`` (a string segment), so a non-numeric
# id must not raise a SQL cast error ahead of the command's 404.
entity_id = int(entity_id) # type: ignore[arg-type]
except (TypeError, ValueError):
- return
- db.session.execute(
- sa.select(model_cls.id).where(model_cls.id == entity_id).with_for_update()
+ return None
+ return (
+ db.session.query(model_cls)
+ .populate_existing()
+ .filter(model_cls.id == entity_id)
+ .with_for_update()
+ .one_or_none()
)
diff --git a/tests/integration_tests/versioning/conditional_write_lock_tests.py b/tests/integration_tests/versioning/conditional_write_lock_tests.py
new file mode 100644
index 00000000000..3482ce13fb1
--- /dev/null
+++ b/tests/integration_tests/versioning/conditional_write_lock_tests.py
@@ -0,0 +1,225 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""sc-120014: the conditional-write lock refreshes the entity it locks.
+
+Two-transaction behavioral proof for the ``lock_entity_for_update`` fix
+(mechanism documented on the helper itself). The staleness is
+*object-level* -- SQLAlchemy never refreshes an already-loaded entity from
+a later plain SELECT -- so the first test fails before the fix on every
+backend it runs on, not only on MySQL/InnoDB REPEATABLE READ where the
+underlying row read is stale too; the second test is a quiet-path control.
+SQLite is skipped: a second writer connection deadlocks against the open
+read transaction instead of modelling a concurrent request. The
+dialect-independent statement-shape pin lives in
+tests/unit_tests/versioning/test_lock_entity.py.
+
+Isolation note: CI pins the MySQL server to READ COMMITTED
+(.github/workflows/bashlib.sh) and PostgreSQL defaults to it, so the
+REPEATABLE READ staleness this fix targets is otherwise MASKED in CI —
+an RC re-read sees the concurrent commit even without the lock. The
+forced-RR test below closes that hole for the mutant that keeps
+``populate_existing()`` but drops ``with_for_update()``.
+"""
+
+import gc
+import weakref
+
+import pytest
+import sqlalchemy as sa
+
+from superset import db
+from superset.connectors.sqla.models import SqlaTable
+from superset.daos.dataset import DatasetDAO
+from superset.utils.core import override_user
+from superset.versioning.api_helpers import lock_entity_for_update
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa: F401
+ load_birth_names_dashboard_with_slices,
+ load_birth_names_data,
+)
+
+
+def _write_description_out_of_band(dataset_id: int, value: str | None) -> None:
+ """Commit a description change through a second connection.
+
+ A separate engine-level transaction stands in for the concurrent
+ request: it commits between this session's entity load and its lock,
+ exactly the window the fix closes. Core UPDATE (not ORM) so the write
+ is invisible to the scoped session until something re-reads the row.
+ """
+ with db.engine.begin() as conn:
+ conn.execute(
+ sa.update(SqlaTable.__table__)
+ .where(SqlaTable.__table__.c.id == dataset_id)
+ .values(description=value)
+ )
+
+
+@pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+class TestConditionalWriteLockRefresh(SupersetTestCase):
+ """The locking read must surface concurrently committed state."""
+
+ def _dataset(self) -> SqlaTable:
+ return (
+ db.session.query(SqlaTable)
+ .filter(SqlaTable.table_name == "birth_names")
+ .one()
+ )
+
+ def _skip_on_sqlite(self) -> None:
+ # SQLite serialises writers at the file level: the out-of-band
+ # engine transaction would deadlock against this session's open
+ # read transaction rather than model a concurrent request.
+ if db.session.get_bind().dialect.name == "sqlite":
+ pytest.skip("two-connection interleave is not expressible on SQLite")
+
+ def test_lock_refreshes_entity_to_concurrently_committed_state(self) -> None:
+ """A concurrent commit becomes visible once the lock is taken.
+
+ A commit landing between the entity load and the lock must show on
+ the loaded entity afterwards.
+
+ Pre-fix, the id-only lock left ``dataset.description`` at the value
+ loaded before the concurrent commit, and the update command's diff
+ ran against that stale state — the silent-lost-update window this
+ pins shut. The command path is exercised too: ``DatasetDAO.find_by_id``
+ must hand back the refreshed identity-map instance rather than
+ re-hydrating from a (possibly stale) plain read.
+ """
+ self._skip_on_sqlite()
+ dataset = self._dataset()
+ dataset_id = dataset.id
+ original = dataset.description
+ assert original != "committed by a concurrent request"
+
+ try:
+ _write_description_out_of_band(
+ dataset_id, "committed by a concurrent request"
+ )
+
+ lock_entity_for_update(SqlaTable, dataset_id)
+
+ assert dataset.description == "committed by a concurrent request"
+ # Pins the coupling the fix relies on: BaseDAO._query only
+ # re-hydrates under force_fetch, so the command's lookup returns
+ # the refreshed object. If DatasetDAO ever flips force_fetch,
+ # this catches the fix being silently undone. The DAO's base
+ # filters read g.user, which a direct test-process call must
+ # supply.
+ with override_user(self.get_user("admin")):
+ fetched = DatasetDAO.find_by_id(dataset_id)
+ assert fetched is dataset
+ assert fetched.description == "committed by a concurrent request"
+ finally:
+ # Release the row lock before restoring out-of-band.
+ db.session.rollback()
+ _write_description_out_of_band(dataset_id, original)
+
+ def test_locked_entity_must_be_held_by_the_caller(self) -> None:
+ """The helper returns the entity, and the caller must keep it alive.
+
+ Two halves, mirroring the production PUT path (which loads nothing
+ before the lock): held, the returned reference makes the command's
+ ``DatasetDAO.find_by_id`` hand back the very same refreshed
+ instance; discarded, the object is weakly referenced by the
+ identity map and is collectible -- after which find_by_id would
+ re-hydrate from a plain read (on MySQL REPEATABLE READ: the
+ pre-lock snapshot), silently undoing the fix. That is why
+ ``datasets/api.py`` binds the return value.
+ """
+ self._skip_on_sqlite()
+ dataset_id = (
+ db.session.query(SqlaTable.id)
+ .filter(SqlaTable.table_name == "birth_names")
+ .scalar()
+ )
+
+ try:
+ locked = lock_entity_for_update(SqlaTable, dataset_id)
+ assert locked is not None
+ with override_user(self.get_user("admin")):
+ fetched = DatasetDAO.find_by_id(dataset_id)
+ assert fetched is locked
+ finally:
+ db.session.rollback()
+
+ try:
+ still_held = lock_entity_for_update(SqlaTable, dataset_id)
+ tracker = weakref.ref(still_held)
+ del still_held, locked, fetched
+ gc.collect()
+ assert tracker() is None, (
+ "locked entity survived without a caller-held reference; "
+ "if SQLAlchemy starts pinning it, the hold-the-return "
+ "contract (and this test) can be retired"
+ )
+ finally:
+ db.session.rollback()
+
+ def test_lock_refresh_survives_forced_repeatable_read(self) -> None:
+ """Forced-RR proof: the locking read defeats the snapshot itself.
+
+ CI pins the MySQL server to READ COMMITTED (bashlib.sh) and PG
+ defaults to it, so without forcing REPEATABLE READ here, a mutant
+ that keeps populate_existing() but drops with_for_update() would
+ still pass every lane — an RC re-read sees the concurrent commit
+ anyway. Under forced RR the unlocked re-read is a consistent read
+ served from the pre-commit snapshot (stale), while the locking
+ read is exempt and returns committed data. MySQL-only: under
+ PostgreSQL's REPEATABLE READ a locking read of a concurrently
+ updated row raises a serialization failure instead of returning
+ it, which is a different (also safe) outcome.
+ """
+ if db.session.get_bind().dialect.name != "mysql":
+ pytest.skip("forced-RR locking-read semantics are MySQL-specific")
+ db.session.rollback()
+ db.session.connection(execution_options={"isolation_level": "REPEATABLE READ"})
+ dataset = self._dataset()
+ dataset_id = dataset.id
+ original = dataset.description
+ assert original != "committed under forced RR"
+
+ try:
+ _write_description_out_of_band(dataset_id, "committed under forced RR")
+
+ locked = lock_entity_for_update(SqlaTable, dataset_id)
+
+ assert locked is not None
+ assert dataset.description == "committed under forced RR"
+ finally:
+ db.session.rollback()
+ _write_description_out_of_band(dataset_id, original)
+
+ def test_lock_is_a_no_op_refresh_without_concurrent_writes(self) -> None:
+ """Quiet-path control: no interleaved commit, no observable change.
+
+ The refresh only surfaces state that actually changed underneath
+ the session; without a concurrent writer the entity is untouched
+ and nothing is dirtied.
+ """
+ self._skip_on_sqlite()
+ dataset = self._dataset()
+ dataset_id = dataset.id
+ original = dataset.description
+
+ try:
+ lock_entity_for_update(SqlaTable, dataset_id)
+
+ assert dataset.description == original
+ assert not db.session.dirty
+ finally:
+ db.session.rollback()
diff --git a/tests/unit_tests/versioning/test_lock_entity.py b/tests/unit_tests/versioning/test_lock_entity.py
new file mode 100644
index 00000000000..b8379cd0073
--- /dev/null
+++ b/tests/unit_tests/versioning/test_lock_entity.py
@@ -0,0 +1,97 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""sc-120014: the conditional-write row lock must also refresh the entity.
+
+Dialect-independent statement-shape guards for ``lock_entity_for_update``:
+the lock must be taken as a full-entity ORM read carrying BOTH
+``populate_existing()`` and ``with_for_update()`` -- dropping either flag
+fails here on every backend. Why both flags matter is documented on the
+helper itself; the behavioral two-transaction proof against a real database
+lives in tests/integration_tests/versioning/conditional_write_lock_tests.py.
+(The restore path has the same staleness class; a matching pin is proposed
+in apache/superset#44015.)
+
+These are deliberate shape pins on the exact query chain: an equivalent
+refactor (a 2.0-style ``select()``, reordered chaining) should update the
+pinned chain -- but never drop ``populate_existing`` or ``with_for_update``.
+"""
+
+from unittest.mock import MagicMock
+
+from pytest_mock import MockerFixture
+
+from superset.versioning.api_helpers import lock_entity_for_update
+
+
+def _mock_db(mocker: MockerFixture) -> MagicMock:
+ """Replace the module-level ``db`` with an explicit MagicMock, so
+ ``mock.patch`` builds a plain sync mock rather than choosing the
+ replacement type itself."""
+ db = MagicMock()
+ mocker.patch("superset.versioning.api_helpers.db", new=db)
+ return db
+
+
+def test_lock_is_a_populating_locking_read(mocker: MockerFixture) -> None:
+ """The lock is a populating, locking ORM read that RETURNS the entity.
+
+ The chain must carry populate_existing() AND with_for_update(), consume
+ the result via one_or_none() (no exception for a missing row -- 404
+ semantics stay with the update command's own lookup), and hand the
+ entity back so the caller can hold a strong reference -- the identity
+ map alone holds it only weakly.
+ """
+ db = _mock_db(mocker)
+ model_cls = MagicMock()
+
+ result = lock_entity_for_update(model_cls, 42)
+
+ db.session.query.assert_called_once_with(model_cls)
+ query = db.session.query.return_value
+ query.populate_existing.assert_called_once_with()
+ populated = query.populate_existing.return_value
+ # Arity-only on purpose: with a MagicMock model the criterion
+ # ``model_cls.id == 42`` is not introspectable, and a with-args assert
+ # would be trivially true. The right-row behavior is pinned by the
+ # integration tests on real backends.
+ populated.filter.assert_called_once()
+ filtered = populated.filter.return_value
+ filtered.with_for_update.assert_called_once_with()
+ filtered.with_for_update.return_value.one_or_none.assert_called_once_with()
+ assert result is filtered.with_for_update.return_value.one_or_none.return_value
+
+
+def test_non_numeric_id_skips_the_lock(mocker: MockerFixture) -> None:
+ """A non-numeric id never reaches SQL and returns None.
+
+ The PUT route declares /<pk> as a string segment; a cast error here
+ would pre-empt the command's 404.
+ """
+ db = _mock_db(mocker)
+
+ result = lock_entity_for_update(MagicMock(), "not-a-pk") # type: ignore[arg-type]
+
+ assert result is None
+ db.session.query.assert_not_called()
+
+
+def test_missing_id_skips_the_lock(mocker: MockerFixture) -> None:
+ db = _mock_db(mocker)
+
+ lock_entity_for_update(MagicMock(), None)
+
+ db.session.query.assert_not_called()