mikebridge opened a new pull request, #40451:
URL: https://github.com/apache/superset/pull/40451

   ### SUMMARY
   
   
`tests/integration_tests/model_tests.py::TestSqlaTableModel::test_get_timestamp_expression`
 and `::test_get_timestamp_expression_epoch` directly 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 nor 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 — concretely, the `__versioned__` 
instrumentation from the in-flight entity-versioning work 
([#39603](https://github.com/apache/superset/pull/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. The pollution cascades into:
   
   - 422 "Dataset could not be updated" on dataset restore tests
   - Empty RLS clauses on 
`TestRowLevelSecurity::test_rls_filter_applies_to_virtual_dataset` (+ 
`_with_join`, the upstream tests added in #36061)
   
   …but **only on Postgres**. SQLite's post-INSERT attribute-expire behaviour 
is laxer and hides the bug.
   
   Fix: wrap each test body in `try` / `finally` with 
`metadata_db.session.rollback()` in the `finally` to discard the dirty 
attribute history. Behaviour on master is unchanged; the fix removes a latent 
footgun for anyone adding flush-time listeners to `TableColumn`.
   
   ### TESTING INSTRUCTIONS
   
   The fix is hygiene-only on vanilla master — the two affected tests pass 
before and after.
   
   ```bash
   pytest 
tests/integration_tests/model_tests.py::TestSqlaTableModel::test_get_timestamp_expression
 \
          
tests/integration_tests/model_tests.py::TestSqlaTableModel::test_get_timestamp_expression_epoch
   ```
   
   To verify the latent bug this prevents, apply any SQLAlchemy `__versioned__` 
listener to `TableColumn` (e.g. via `sqlalchemy-continuum`) and run the full 
integration suite on Postgres. Before this fix: 3 downstream tests fail in CI's 
full-suite ordering. After: clean run.
   
   ### ADDITIONAL INFORMATION
   
   - [x] Has associated issue: Preset internal ticket sc-107618
   - [ ] Required feature flags:
   - [ ] Changes UI
   - [ ] Includes DB Migration
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API


-- 
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]

Reply via email to