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/07/14 18:10:18 UTC
(superset) branch master updated: fix(versioning): isolate action metadata writes (#41943)
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 e3a0d0e41ec fix(versioning): isolate action metadata writes (#41943)
e3a0d0e41ec is described below
commit e3a0d0e41ec4f3d2add3d5a62c1bae9c46413f65
Author: Mike Bridge <mi...@bridgecanada.com>
AuthorDate: Tue Jul 14 12:10:12 2026 -0600
fix(versioning): isolate action metadata writes (#41943)
Co-authored-by: Mike Bridge <mi...@ext.preset.io>
---
superset/versioning/changes/listener.py | 27 ++++---
.../versioning/transaction_correctness_tests.py | 84 ++++++++++++++++++++++
2 files changed, 102 insertions(+), 9 deletions(-)
diff --git a/superset/versioning/changes/listener.py b/superset/versioning/changes/listener.py
index 56854310640..d8875eae9f6 100644
--- a/superset/versioning/changes/listener.py
+++ b/superset/versioning/changes/listener.py
@@ -299,6 +299,17 @@ def _inject_action_meta_record(
logger.exception("version_changes: malformed ACTION_META_KEY payload")
+def _write_action_kind(
+ session: Session, tx_table: sa.Table, tx_id: int, action_kind: str
+) -> None:
+ """Write action metadata through the transaction's existing connection."""
+ session.connection().execute(
+ sa.update(tx_table)
+ .where(tx_table.c.id == tx_id)
+ .values(action_kind=action_kind)
+ )
+
+
def _stamp_action_kind_on_transaction(session: Session, tx_id: int) -> None:
"""Pop the per-tx action_kind from ``session.info`` and stamp it
onto the ``version_transaction`` row identified by *tx_id*.
@@ -312,10 +323,11 @@ def _stamp_action_kind_on_transaction(session: Session, tx_id: int) -> None:
The action_kind is popped (not just read) so a long-lived session
can't accidentally carry the value into the next transaction. A
- failed stamp is logged and swallowed — action_kind is a
- descriptive enrichment, not a correctness invariant; refusing to
- write change records because an UPDATE on a single column failed
- would punish the user save for an audit-log nicety.
+ failed stamp is rolled back to a SAVEPOINT, logged, and swallowed:
+ action_kind is descriptive enrichment, not a correctness invariant.
+ The SAVEPOINT is opened after the final flush, so a failed metadata
+ statement cannot poison the user transaction or disturb Continuum's
+ canonical shadows.
"""
# pylint: disable=import-outside-toplevel
from sqlalchemy_continuum import versioning_manager
@@ -325,11 +337,8 @@ def _stamp_action_kind_on_transaction(session: Session, tx_id: int) -> None:
return
tx_tbl = versioning_manager.transaction_cls.__table__
try:
- session.connection().execute(
- sa.update(tx_tbl)
- .where(tx_tbl.c.id == tx_id)
- .values(action_kind=action_kind)
- )
+ with session.connection().begin_nested():
+ _write_action_kind(session, tx_tbl, tx_id, action_kind)
except Exception: # pylint: disable=broad-except
logger.exception(
"version_changes: failed to stamp action_kind=%s on tx %s",
diff --git a/tests/integration_tests/versioning/transaction_correctness_tests.py b/tests/integration_tests/versioning/transaction_correctness_tests.py
index 9e55fae4c90..0cb3d9b1afa 100644
--- a/tests/integration_tests/versioning/transaction_correctness_tests.py
+++ b/tests/integration_tests/versioning/transaction_correctness_tests.py
@@ -16,6 +16,8 @@
# under the License.
"""Transaction-level correctness tests for entity version history."""
+from unittest.mock import patch
+
import pytest
import sqlalchemy as sa
@@ -23,6 +25,8 @@ from superset import db
from superset.connectors.sqla.models import SqlaTable
from superset.models.slice import Slice
from superset.utils import json
+from superset.versioning.changes import listener
+from superset.versioning.changes.listener import ACTION_KIND_KEY
from superset.versioning.changes.table import version_changes_table
from tests.integration_tests.base_tests import SupersetTestCase
from tests.integration_tests.fixtures.birth_names_dashboard import ( # noqa: F401
@@ -222,3 +226,83 @@ class TestVersionHistoryTransactionCorrectness(SupersetTestCase):
def test_nested_rollback_preserves_outer_initial_state(self) -> None:
"""Rolling back a SAVEPOINT keeps the outer transaction registry."""
self._assert_nested_transaction_preserves_outer_initial_state("rollback")
+
+ def test_action_kind_sql_failure_is_isolated_and_consumed(self) -> None:
+ """Failed descriptive metadata cannot poison or leak from the save."""
+ chart = self._chart("Girls")
+ chart_id = chart.id
+ boundary = db.session.scalar(
+ sa.text("SELECT coalesce(max(id), 0) FROM version_transaction")
+ )
+
+ def fail_action_kind(*_args: object, **_kwargs: object) -> None:
+ db.session.connection().execute(
+ sa.text("UPDATE __missing_action_kind_target__ SET value = 1")
+ )
+
+ try:
+ db.session.info[ACTION_KIND_KEY] = "restore"
+ chart.slice_name = "Girls survives action metadata failure"
+ with patch.object(listener, "_write_action_kind", fail_action_kind):
+ db.session.commit()
+
+ db.session.expire_all()
+ saved = db.session.get(Slice, chart_id)
+ assert saved is not None
+ assert saved.slice_name == "Girls survives action metadata failure"
+ assert ACTION_KIND_KEY not in db.session.info
+
+ first_rows = db.session.execute(
+ sa.text(
+ "SELECT sv.transaction_id, vt.action_kind "
+ "FROM slices_version sv "
+ "JOIN version_transaction vt ON vt.id = sv.transaction_id "
+ "WHERE sv.id = :id AND sv.operation_type = 1 "
+ "AND sv.slice_name = :name "
+ "AND sv.transaction_id > :boundary"
+ ),
+ {
+ "id": chart_id,
+ "name": "Girls survives action metadata failure",
+ "boundary": boundary,
+ },
+ ).all()
+ assert len(first_rows) == 1
+ assert first_rows[0].action_kind is None
+ failed_metadata_tx_id = first_rows[0].transaction_id
+
+ semantic_changes = _changes_for_chart(
+ chart_id, after_transaction_id=boundary
+ )
+ name_changes = [
+ row
+ for row in semantic_changes
+ if row["path"] == ["slice_name"]
+ and row["to_value"] == "Girls survives action metadata failure"
+ ]
+ assert len(name_changes) == 1
+ assert name_changes[0]["transaction_id"] == failed_metadata_tx_id
+
+ saved.slice_name = "Girls next transaction"
+ db.session.commit()
+ next_rows = db.session.execute(
+ sa.text(
+ "SELECT vt.action_kind FROM slices_version sv "
+ "JOIN version_transaction vt ON vt.id = sv.transaction_id "
+ "WHERE sv.id = :id AND sv.operation_type = 1 "
+ "AND sv.slice_name = :name "
+ "AND sv.transaction_id > :boundary"
+ ),
+ {
+ "id": chart_id,
+ "name": "Girls next transaction",
+ "boundary": failed_metadata_tx_id,
+ },
+ ).all()
+ assert len(next_rows) == 1
+ assert next_rows[0].action_kind is None
+ finally:
+ chart = db.session.get(Slice, chart_id)
+ assert chart is not None
+ chart.slice_name = "Girls"
+ db.session.commit()