feat: bump SQLAlchemy to 2.0 and flask-sqlalchemy to 3.1.1 - #42803
Conversation
d209bf3 to
4f31f32
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42803 +/- ##
==========================================
- Coverage 66.63% 66.42% -0.22%
==========================================
Files 2866 2863 -3
Lines 162576 162330 -246
Branches 37461 37370 -91
==========================================
- Hits 108340 107822 -518
- Misses 52146 52418 +272
Partials 2090 2090
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review Agent Run #64e76a
Actionable Suggestions - 1
-
superset-core/pyproject.toml - 1
- SQLAlchemy 2.0 Query.get() deprecation · Line 48-48
Additional Suggestions - 6
-
superset/tasks/manager.py - 3
-
Test coverage gap for app context · Line 262-262The code change is correct and follows the established pattern used in `superset/tasks/context.py`. However, the test suite lacks coverage for the `has_app_context()` behavior when the `app` parameter is provided and an app context already exists. Add a test that patches `has_app_context` to return True while passing a non-None `app` to verify the DB query executes without redundant context creation.
-
Test coverage gap for callback context · Line 465-465The pattern `if app and not has_app_context():` is consistent with existing usage in `superset/tasks/context.py` (lines 260, 479, 555, 643). Consider adding test coverage for the case where `app` is provided and `has_app_context()` returns True.
-
Test coverage gap for abort polling · Line 663-663The pattern `if app and not has_app_context():` is consistent with established codebase conventions. Add test coverage for the has_app_context() behavior path to ensure the abort listener correctly handles existing app contexts.
-
-
superset/tasks/context.py - 1
-
Inconsistent app context pattern · Line 260-263Timer callback `_deferred_flush` uses the old pattern (direct `with` statement) while the three other sites (lines 479, 555, 643) use the new `nullcontext()` pattern. For consistency, apply the same fix here.
-
-
superset/migrations/versions/2026-08-06_16-09_befa892fa3ad_merge_databend_sslmode_migration_with_.py - 2
-
Missing type annotation on upgrade · Line 30-31Add explicit `-> None` return type annotation to `upgrade()` per project typing standards. Also add a docstring explaining this is a merge migration (e.g., 'Merge migration combining databend sslmode and pivot-table-percent-display restore merges.')
-
Missing type annotation on downgrade · Line 34-35Add explicit `-> None` return type annotation to `downgrade()` per project typing standards. Also add a docstring explaining this merge migration's downgrade behavior.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
requirements/development.txt - 2
- CWE-703: SQLAlchemy 2.0 Breaking Change · Line 312-312
- CWE-703: SQLAlchemy 2.0 Breaking Change (duplicate) · Line 978-978
Review Details
-
Files reviewed - 42 · Commit Range:
a7d9bde..b4a1029- pyproject.toml
- pytest.ini
- requirements/base.txt
- requirements/development.txt
- superset-core/pyproject.toml
- superset/connectors/sqla/models.py
- superset/daos/dashboard.py
- superset/databases/utils.py
- superset/db_engine_specs/base.py
- superset/db_engine_specs/clickhouse.py
- superset/db_engine_specs/couchbase.py
- superset/db_engine_specs/databend.py
- superset/db_engine_specs/databricks.py
- superset/db_engine_specs/duckdb.py
- superset/db_engine_specs/snowflake.py
- superset/extensions/__init__.py
- superset/initialization/__init__.py
- superset/migrations/versions/2018-07-26_11-10_c82ee8a39623_add_implicit_tags.py
- superset/migrations/versions/2020-01-08_01-17_e96dbf2cfef0_datasource_cluster_fk.py
- superset/migrations/versions/2020-04-24_10-46_e557699a813e_add_tables_relation_to_row_level_.py
- superset/migrations/versions/2020-09-15_18-22_e5ef6828ac4e_add_rls_filter_type_and_grouping_key.py
- superset/migrations/versions/2020-09-28_17-57_b56500de1855_add_uuid_column_to_import_mixin.py
- superset/migrations/versions/2021-02-18_09-13_c501b7c653a3_add_missing_uuid_column.py
- superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py
- superset/migrations/versions/2026-08-06_16-09_befa892fa3ad_merge_databend_sslmode_migration_with_.py
- superset/models/core.py
- superset/models/sql_types/presto_sql_types.py
- superset/tasks/context.py
- superset/tasks/manager.py
- superset/utils/core.py
- superset/views/core.py
- tests/integration_tests/charts/api_tests.py
- tests/integration_tests/dashboards/api_tests.py
- tests/integration_tests/dashboards/soft_delete_tests.py
- tests/integration_tests/datasource_tests.py
- tests/integration_tests/db_engine_specs/hive_tests.py
- tests/integration_tests/db_engine_specs/presto_tests.py
- tests/integration_tests/model_tests.py
- tests/integration_tests/superset_test_config.py
- tests/integration_tests/versioning/id_reuse_tests.py
- tests/unit_tests/db_engine_specs/test_snowflake.py
- tests/unit_tests/db_engine_specs/test_trino.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
Code Review Agent Run #1edaf1Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Code Review Agent Run #98fd42Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
session.query(Model).get(id) at superset/commands/sql_lab/estimate.py was migrated to db.session.get(Database, id) in bc56ecb, but the integration-test mocks in tests/integration_tests/sql_lab/ still patched the old session.query().get() chain shape, so QueryEstimationCommand picked up an auto-generated MagicMock instead of the fixture and failed test-mysql/postgres/sqlite. Same mock-shape fix already applied to the unit-test sibling in the same commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…bump Notes the breaking dependency change for downstream consumers: custom db_engine_specs or extensions that touch SQLAlchemy internals directly should check the 1.4->2.0 migration guide, and the optional connector extras still capped below their own SQLAlchemy-2.0-only releases (either pending #42891 or blocked entirely on upstream) keep pulling 1.4-line dialect versions until their own caps move.
…ier merge d7cecc48bd55 and befa892fa3ad were both independently-created merge migrations reconciling the exact same two divergent parents (4f145192b583, c4a1b8e2d739) -- one from this branch's own earlier "merge divergent heads" commit, one from master's own concurrent fix (#42878). Master converged on d7cecc48bd55 as canonical; merging master into this branch left both in the same chain, producing two alembic heads and breaking every job that runs `superset db upgrade` (docker-build, test-postgres/mysql/sqlite, E2E, Presto/Hive, CLI tests). Drops the redundant befa892fa3ad, which nothing else references. Verified locally: `flask db heads` reports exactly one head (d7cecc48bd55, matching master), and a real `superset db upgrade` against a fresh sqlite db runs the full chain cleanly end to end.
The shillelagh ProgrammingError message embeds a version-specific sqlalche.me/e/<version>/... doc link; normalize it before comparing so the test doesn't need updating on every SQLAlchemy minor bump. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A prior rebase merge conflict resolution accidentally reverted the sqlglot minimum version bump from #42772, dropping the pyproject.toml constraint back to >=30.12.0. Restore >=30.14.0 to match master; the locked version (30.15.0) already satisfies this and is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r SQLAlchemy 2.0 (#42891) Co-authored-by: Claude Code <noreply@anthropic.com>
3134fed to
ffac9bd
Compare
Code Review Agent Run #0b6a38Actionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Only get_table_metadata's get_columns() call is guarded against NoSuchTableError now, so a NoSuchTableError raised later while reflecting foreign keys, indexes, or the table comment for a table that's confirmed to exist propagates normally instead of being misread as a missing table. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #5fecccActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
sha174n
left a comment
There was a problem hiding this comment.
Reviewed the SQLAlchemy 2.0 / flask-sqlalchemy 3.1.1 compatibility changes — the session scopefunc restoration, the query().get() -> session.get() swaps, the enum native_enum=False alignment, and the URL/password rendering changes all preserve existing behavior. LGTM.
…ll-bump # Conflicts: # requirements/base.txt # requirements/development.txt # superset/commands/sql_lab/estimate.py # tests/integration_tests/sql_lab/api_tests.py # tests/integration_tests/sql_lab/commands_tests.py # tests/unit_tests/commands/sql_lab/test_estimate.py
sniffio/sortedcontainers were pinned via trio, which is no longer a resolved dependency; sqlalchemy now pulls greenlet directly. Ran ./scripts/uv-pip-compile.sh to bring requirements/*.txt back in sync. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #1a89e9Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
Bumps SQLAlchemy from 1.4.54 to 2.0.51 and flask-sqlalchemy from <3.0 to 3.1.1, completing step 6 of discussion #40273's migration battle plan (steps 1-5, the deprecation-warning cleanup, already merged). Supersedes the flask-sqlalchemy 3.0.5 intermediate step attempted and reverted in #42542 by going straight to the real target version instead.
Started from a cherry-pick of @mikebridge's
test/verify-sqlalchemy-continuumbranch (backend-relevant hunks only), then root-caused and fixed everything real CI turned up on top of that, across several rounds:Core session-scoping bug (the actual cause of #42542's break): Flask-SQLAlchemy 3.x scopes
db.sessionby Flask app-context object identity instead of thread identity. Superset's code and test fixtures widely assume one shared session per thread across nestedapp.app_context()blocks. Fixed by restoring thread-scopeddb.sessionunder FSA 3.x and guarding the CeleryAppContextTaskwrapper'sapp_context()push withhas_app_context()so eager-mode task execution reuses the caller's session instead of silently splitting into a second one.Other SQLAlchemy 1.4→2.0 breaks fixed along the way:
MetaData(bind=)removed;load_only()needs real ORM attributes, not strings; rawRow["key"]string indexing removed (in older migration scripts)connection.rollback()added afterpessimistic_connection_handling's pool-checkout health-checkSELECT, since 2.0's autobegin now leaves that SELECT's transaction opendb.session.add()calls that used to work via implicit session tracking (report-schedule fixtures,DashboardDAO.copy_dashboard, Explore-save-to-new-dashboard)RowLevelSecurityFilter.filter_typeno longer rendered as a native PG enumdb.session.bindis nowNoneunder 2.0 in some contexts; switched toget_bind()URL.render_as_string()encodes itselfNullPooltoQueuePool, which could hand a pooled connection to a different thread than the one that opened it — reintroducing acheck_same_threadviolation in the GTF task framework's deferred-flush timer thread. PinnedNullPoolback for the test config specifically (production config'scheck_same_thread=falseis unaffected).session.query(Model).get(id)migrated tosession.get(Model, id)at the remaining 9 call sites (2.0 deprecation), plus matching test-mock updatesAlso updates two migration scripts (
2018-07-26_..._add_implicit_tags,2022-04-01_..._new_dataset_models_take_2) toMapped[]-typed relationship annotations, and removes the now-obsoleteSQLALCHEMY_WARN_20pytest.inifilter lines.Full context: discussion #40273, step 6.
Closes #39278 — that PR's approach (Flask 3.x via the flask-sqlalchemy 3.0.5 intermediate step, staying on SQLAlchemy 1.4) is superseded by this PR jumping straight to the real 2.0 target and fixing the underlying session-scoping bug that made the intermediate step unsafe in the first place.
TESTING INSTRUCTIONS
Verified with real CI (not just local):
Python-Integration(test-postgres/test-mysql/test-sqlite) andPython-Unit(11793 passed, 4 skipped, 2 xfailed) both green, along with the full remaining CI matrix.ADDITIONAL INFORMATION