refactor: engine enforce SQLAlchemy 2.0

[hy144328]

See #40273 .

SUMMARY

After all warnings about Engine transactions incompatible with the SQLAlchemy 2.0 API have been converted to errors, this PR goes a step further.
create_engine() is now called with future=True everywhere so that only the SQLAlchemy 2.0 API is allowed with Engine transactions.
Wherever we perform assertions on Engine mocks in unit tests, we have to temporarily modify those assertions with the future flag in order to pass.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Unit tests:

SQLALCHEMY_WARN_20=1 python3 -m pytest tests/unit_tests/

Integration tests via CI.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	ff9d04f
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a5fba4a0d3a6500088ee441
😎 Deploy Preview	https://deploy-preview-42277--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[codecov]

Codecov Report

❌ Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.11%. Comparing base (0dd06f3) to head (b0bb20f).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/cli/test_db.py	0.00%	2 Missing ⚠️
superset/models/core.py	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #42277      +/-   ##
==========================================
- Coverage   65.20%   65.11%   -0.10%     
==========================================
  Files        2774     2784      +10     
  Lines      156723   157382     +659     
  Branches    35771    35832      +61     
==========================================
+ Hits       102195   102479     +284     
- Misses      52560    52933     +373     
- Partials     1968     1970       +2     

Flag	Coverage Δ
hive	38.74% <25.00%> (+0.05%)	⬆️
mysql	57.66% <25.00%> (-0.13%)	⬇️
postgres	57.71% <25.00%> (-0.13%)	⬇️
presto	40.65% <25.00%> (+0.04%)	⬆️
python	59.09% <25.00%> (-0.14%)	⬇️
sqlite	57.32% <25.00%> (-0.12%)	⬇️
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[hy144328]

@rusackas This should be ready for review, too.
I am quite pleased that it caught a few cases in the integration tests where the code had regressed due to concurrent PRs, e.g. incorrect use of engine.connect(), which does not autocommit anymore.
They were not caught because the SQLALCHEMY_WARN_20 environment variable is only added to the unit test CI.
I wonder if this would be a good time now since we bumping versions has a real chance of breaking things.
Or in other words, a follow-up to #41915 ?

[bito-code-review]

Code Review Agent Run #2b63be

Actionable Suggestions - 0

Additional Suggestions - 3

• tests/unit_tests/security/exclude_users_filter_test.py - 1

  • Inconsistent engine configuration · Line 47-47
    The diff applies `future=True` to one test function (line 47) but three other functions in the same file still use `create_engine("sqlite://")` without this flag (lines 72, 108, 146). This creates divergent engine configurations that may cause inconsistent behavior across tests. The codebase convention (evidenced by 30+ usages) is to use `future=True` with SQLAlchemy 1.4 for future 2.0 compatibility.
• superset/cli/test_db.py - 2

  • Input dict mutation side effect · Line 283-284
    This function mutates `engine_kwargs` in-place, which is a side effect on the caller's object. If the caller retains a reference after calling this function, unexpected behavior could occur. Consider copying the dict first to keep the function's interface pure.
  • Semantic duplication with core.py · Line 283-284
    The same `if 'future' not in engine_kwargs: engine_kwargs['future'] = True` pattern exists in `superset/models/core.py:647-648`. Duplicating this logic creates maintenance risk if the default behavior needs to change.

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

• superset/migrations/versions/2018-06-13_10-20_4451805bbaa1_remove_double_percents.py - 2

  • Misleading line break · Line 91-93
  • Pending future=True deprecation · Line 84-84

Review Details

• Files reviewed - 28 · Commit Range: 84630f5..b0bb20f

  • scripts/benchmark_migration.py
  • superset/cli/test_db.py
  • superset/db_engine_specs/gsheets.py
  • superset/migrations/versions/2018-06-13_10-20_4451805bbaa1_remove_double_percents.py
  • superset/models/core.py
  • tests/integration_tests/dao/conftest.py
  • tests/integration_tests/fixtures/datasource.py
  • tests/integration_tests/migrations/composite_pk_association_tables__tests.py
  • tests/integration_tests/migrations/composite_pk_round_trip__tests.py
  • tests/unit_tests/charts/test_filters.py
  • tests/unit_tests/conftest.py
  • tests/unit_tests/connectors/sqla/models_test.py
  • tests/unit_tests/databases/filters_test.py
  • tests/unit_tests/db_engine_specs/test_gsheets.py
  • tests/unit_tests/db_engine_specs/test_sqlite.py
  • tests/unit_tests/extensions/test_sqlalchemy.py
  • tests/unit_tests/migrations/composite_pk_association_tables_test.py
  • tests/unit_tests/migrations/test_add_deleted_at_to_dashboards.py
  • tests/unit_tests/migrations/test_add_deleted_at_to_slices.py
  • tests/unit_tests/migrations/test_add_deleted_at_to_tables.py
  • tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py
  • tests/unit_tests/models/core_test.py
  • tests/unit_tests/models/helpers_test.py
  • tests/unit_tests/models/owner_association_tables_test.py
  • tests/unit_tests/security/exclude_users_filter_test.py
  • tests/unit_tests/utils/filters_test.py
  • tests/unit_tests/utils/pandas_sqlalchemy_compat_test.py
  • tests/unit_tests/versioning/test_listener.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[rusackas]

Thanks @hy144328, LGTM, approving.

Codeant's two threads on the duplicate future kwarg look resolved already in ff9d04f.

One nit, not blocking: exclude_users_filter_test.py only picked up future=True on one of its four create_engine calls, the other three are still bare. Worth a quick follow-up pass for consistency, no rush.

On your question, I think yes, SQLALCHEMY_WARN_20=1 on the integration matrix is worth doing as a follow-up to #41915. I'd rather it land as its own PR though, flipping that switch on postgres/mysql/sqlite/hive/presto will probably shake loose a few more stray engine.connect() spots that deserve their own look. Holler if you want a hand with either.

[Copilot]

Pull request overview

This PR tightens Superset’s SQLAlchemy usage toward the 2.0-style API by consistently enabling “future” engine behavior (future=True) on create_engine() calls, and updates unit/integration tests (and a couple of runtime utilities) to match the new engine initialization expectations.

Changes:

• Default future=True for database engine creation in Database._get_sqla_engine (and in the superset test-db CLI path when not explicitly provided).
• Update many unit/integration tests to create SQLAlchemy engines with future=True, and adjust mock assertions accordingly.
• Update a legacy migration and some test migration harnesses to use transaction-friendly connection patterns (engine.begin()).

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/unit_tests/versioning/test_listener.py	Uses future=True when creating the in-memory SQLite engine for lifecycle session tests.
tests/unit_tests/utils/pandas_sqlalchemy_compat_test.py	Creates test engine with future=True for pandas SQLAlchemy compatibility coverage.
tests/unit_tests/utils/filters_test.py	Compiles SQLAlchemy clauses using a future=True engine (and adjusts compile kwargs formatting).
tests/unit_tests/security/exclude_users_filter_test.py	Uses future=True for the in-memory engine used by the exclude-users filter tests.
tests/unit_tests/models/owner_association_tables_test.py	Uses future=True for the standalone SQLite engine used to validate constraints.
tests/unit_tests/models/helpers_test.py	Adds future=True to several create_engine calls used by helper/model tests.
tests/unit_tests/models/core_test.py	Updates create_engine mock assertions and test engine creation to include future=True.
tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py	Uses future=True for the in-memory migration test engine.
tests/unit_tests/migrations/test_add_deleted_at_to_tables.py	Uses future=True for the in-memory migration test engine.
tests/unit_tests/migrations/test_add_deleted_at_to_slices.py	Uses future=True for the in-memory migration test engine.
tests/unit_tests/migrations/test_add_deleted_at_to_dashboards.py	Uses future=True for the in-memory migration test engine.
tests/unit_tests/migrations/composite_pk_association_tables_test.py	Uses future=True for the in-memory engine that exercises composite PK migration logic.
tests/unit_tests/extensions/test_sqlalchemy.py	Uses future=True when creating superset:// engines in dialect tests.
tests/unit_tests/db_engine_specs/test_sqlite.py	Uses future=True with SQLite engine in engine spec tests.
tests/unit_tests/db_engine_specs/test_gsheets.py	Updates mocked create_engine expectations to include future=True.
tests/unit_tests/databases/filters_test.py	Uses future=True for SQLite engines used in database filter tests.
tests/unit_tests/connectors/sqla/models_test.py	Uses future=True for engines in connector model tests (including postgres URLs).
tests/unit_tests/conftest.py	Updates shared unit-test session fixture to use a future=True engine.
tests/unit_tests/charts/test_filters.py	Compiles captured SQLAlchemy clauses against a future=True engine.
tests/integration_tests/migrations/composite_pk_round_trip__tests.py	Switches migration runner to engine.begin() and uses future=True engines.
tests/integration_tests/migrations/composite_pk_association_tables__tests.py	Uses engine.begin() for Alembic context + future=True engine.
tests/integration_tests/fixtures/datasource.py	Uses future=True for the integration fixture engine created from app DB URI.
tests/integration_tests/dao/conftest.py	Adds future=True to the in-memory engine used by DAO integration tests.
superset/models/core.py	Defaults SQLAlchemy engine creation to future=True for database engines.
superset/migrations/versions/2018-06-13_10-20_4451805bbaa1_remove_double_percents.py	Uses future=True for migration-created engines and refactors session handling to context managers.
superset/db_engine_specs/gsheets.py	Adds future=True to the gsheets:// engine created during parameter validation.
superset/cli/test_db.py	Ensures future=True is set by default for the CLI dialect test unless overridden.
scripts/benchmark_migration.py	Uses future=True when creating engine for migration benchmarking reflections.

[rusackas]

I think the copilot findings are skippable here and can be addressed in follow-up touchups (just adding it to the tracker if warranted). Let's merge this thing! Thank you @hy144328 this is huge!