From bb94e0fa5e32b96b25e513028107d3169c6cf7c8 Mon Sep 17 00:00:00 2001 From: Mike Bridge Date: Tue, 26 May 2026 13:37:26 -0600 Subject: [PATCH] test(model): roll back uncommitted ds_col mutations in timestamp-expression tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``test_get_timestamp_expression`` and ``test_get_timestamp_expression_epoch`` mutate ``birth_names.ds_col.expression`` (and, in the ``_epoch`` variant, ``ds_col.python_date_format``) to exercise the ``get_timestamp_expression`` compile path. ``expression`` is restored at the end of each test; ``python_date_format`` is not. Neither test commits or rolls back, so the ``TableColumn`` row stays in ``session.dirty`` for the next autoflush. This is harmless today because nothing on master inspects ``TableColumn`` for dirty state at flush time. It becomes load-bearing the moment any code adds a SQLAlchemy listener that walks dirty rows — for example, the ``__versioned__`` instrumentation from the in-flight entity-versioning work (sc-103156 / PR #39603). With that instrumentation applied, the next autoflush captures the uncommitted mutation, writes a ``table_columns_version`` shadow row, and corrupts ``birth_names``' version timeline — cascading into 422s on downstream dataset-restore tests and empty RLS clauses on the virtual-dataset tests, but **only on Postgres** (SQLite's post-INSERT attribute-expire behaviour is laxer and hides the bug). Wrap each test body in ``try`` / ``finally`` with ``metadata_db.session.rollback()`` in the ``finally`` to discard the attribute history. Behaviour on vanilla master is unchanged; the fix just removes a latent footgun for anyone adding flush-time listeners to ``TableColumn``. Closes sc-107618. --- tests/integration_tests/model_tests.py | 89 ++++++++++++++++---------- 1 file changed, 54 insertions(+), 35 deletions(-) diff --git a/tests/integration_tests/model_tests.py b/tests/integration_tests/model_tests.py index 1da5e3d02102..f956286c315a 100644 --- a/tests/integration_tests/model_tests.py +++ b/tests/integration_tests/model_tests.py @@ -395,47 +395,66 @@ class TestSqlaTableModel(SupersetTestCase): def test_get_timestamp_expression(self): tbl = self.get_table(name="birth_names") ds_col = tbl.get_column("ds") - sqla_literal = ds_col.get_timestamp_expression(None) - assert str(sqla_literal.compile()) == "ds" - - sqla_literal = ds_col.get_timestamp_expression("P1D") - compiled = f"{sqla_literal.compile()}" - if tbl.database.backend == "mysql": - assert compiled == "DATE(ds)" - - prev_ds_expr = ds_col.expression - ds_col.expression = "DATE_ADD(ds, 1)" - sqla_literal = ds_col.get_timestamp_expression("P1D") - compiled = f"{sqla_literal.compile()}" - if tbl.database.backend == "mysql": - assert compiled == "DATE(DATE_ADD(ds, 1))" - ds_col.expression = prev_ds_expr + try: + sqla_literal = ds_col.get_timestamp_expression(None) + assert str(sqla_literal.compile()) == "ds" + + sqla_literal = ds_col.get_timestamp_expression("P1D") + compiled = f"{sqla_literal.compile()}" + if tbl.database.backend == "mysql": + assert compiled == "DATE(ds)" + + prev_ds_expr = ds_col.expression + ds_col.expression = "DATE_ADD(ds, 1)" + sqla_literal = ds_col.get_timestamp_expression("P1D") + compiled = f"{sqla_literal.compile()}" + if tbl.database.backend == "mysql": + assert compiled == "DATE(DATE_ADD(ds, 1))" + ds_col.expression = prev_ds_expr + finally: + # Discard the in-memory attribute history so the next session + # autoflush doesn't see this row as dirty. The test only + # exercises the in-memory compile path; any persisted write + # would be accidental. ``rollback`` rather than ``expire`` — + # the latter doesn't reliably clear SA's per-attribute history + # tracking for already-loaded objects. + metadata_db.session.rollback() @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_get_timestamp_expression_epoch(self): tbl = self.get_table(name="birth_names") ds_col = tbl.get_column("ds") - ds_col.expression = None - ds_col.python_date_format = "epoch_s" - sqla_literal = ds_col.get_timestamp_expression(None) - compiled = f"{sqla_literal.compile()}" - if tbl.database.backend == "mysql": - assert compiled == "from_unixtime(ds)" - - ds_col.python_date_format = "epoch_s" - sqla_literal = ds_col.get_timestamp_expression("P1D") - compiled = f"{sqla_literal.compile()}" - if tbl.database.backend == "mysql": - assert compiled == "DATE(from_unixtime(ds))" - - prev_ds_expr = ds_col.expression - ds_col.expression = "DATE_ADD(ds, 1)" - sqla_literal = ds_col.get_timestamp_expression("P1D") - compiled = f"{sqla_literal.compile()}" - if tbl.database.backend == "mysql": - assert compiled == "DATE(from_unixtime(DATE_ADD(ds, 1)))" - ds_col.expression = prev_ds_expr + try: + ds_col.expression = None + ds_col.python_date_format = "epoch_s" + sqla_literal = ds_col.get_timestamp_expression(None) + compiled = f"{sqla_literal.compile()}" + if tbl.database.backend == "mysql": + assert compiled == "from_unixtime(ds)" + + ds_col.python_date_format = "epoch_s" + sqla_literal = ds_col.get_timestamp_expression("P1D") + compiled = f"{sqla_literal.compile()}" + if tbl.database.backend == "mysql": + assert compiled == "DATE(from_unixtime(ds))" + + prev_ds_expr = ds_col.expression + ds_col.expression = "DATE_ADD(ds, 1)" + sqla_literal = ds_col.get_timestamp_expression("P1D") + compiled = f"{sqla_literal.compile()}" + if tbl.database.backend == "mysql": + assert compiled == "DATE(from_unixtime(DATE_ADD(ds, 1)))" + ds_col.expression = prev_ds_expr + finally: + # Discard the in-memory attribute history so the next session + # autoflush doesn't see this row as dirty — + # ``python_date_format`` isn't restored above and the test + # never commits, so the mutation would otherwise leak. + # ``rollback`` rather than ``expire`` — the latter doesn't + # reliably clear SA's per-attribute history tracking for + # already-loaded objects. + metadata_db.session.rollback() def query_with_expr_helper(self, is_timeseries, inner_join=True): tbl = self.get_table(name="birth_names")