feat: bump SQLAlchemy to 2.0 and flask-sqlalchemy to 3.1.1 - #42803
Open
rusackas wants to merge 36 commits into
Open
feat: bump SQLAlchemy to 2.0 and flask-sqlalchemy to 3.1.1#42803rusackas wants to merge 36 commits into
rusackas wants to merge 36 commits into
Conversation
github-actions
Bot
added
preset-io
risk:db-migration
… MERGE) Investigation-only, cherry-picked from mikebridge's test/verify-sqlalchemy-continuum branch (backend-relevant hunks only, skipping unrelated frontend formatting drift). Bumps sqlalchemy to 2.0.51 and flask-sqlalchemy to 3.1.1 directly, rather than the FSA 3.0.5 intermediate step attempted in PR #42542. Notable fix beyond the version bumps: pessimistic_connection_handling (superset/utils/core.py) now calls connection.rollback() after its pool-checkout health-check SELECT. Under SQLAlchemy 2.0's autobegin behavior that SELECT implicitly opens a transaction it previously never closed, which is a plausible mechanism for the MySQL lock-wait-timeout symptom from the original #42542 break. Also updates two migration scripts (2018-07-26 add_implicit_tags, 2022-04-01 new_dataset_models_take_2) to Mapped[] typed relationship annotations -- these define their own standalone declarative models and were missed by discussion #40273's earlier unit-test-driven deprecation-warning sweep, since migration scripts aren't exercised by that suite. Removes the now-obsolete SQLALCHEMY_WARN_20 pytest.ini filterwarnings error lines, since sqlalchemy.exc.RemovedIn20Warning doesn't fire (or exist in the same form) once SQLAlchemy 2.0 is actually installed. Pushing to get a real test-sqlite/test-mysql CI signal, since local sqlite runs don't reliably reproduce the original break either way.
Three 2020-era migration scripts constructed MetaData(bind=bind), a kwarg removed outright in SQLAlchemy 2.0 (TypeError: MetaData.__init__() got an unexpected keyword argument 'bind'). Every actual usage already passes autoload_with=bind per-table, so the MetaData-level bind was redundant even before 2.0; dropped it. Also fixes row["key"] string-indexed access on a raw Core Row result in the same migration family -- SQLAlchemy 2.0's Row only supports positional/attribute access directly, string-key lookups need row._mapping["key"]. Verified both failure modes directly against a real sqlalchemy==2.0.51 install before making this change.
rusackas
force-pushed
the
experiment/sqla2-full-bump
branch
from
August 5, 2026 18:44
d209bf3 to
4f31f32
Compare
sqlalchemy.orm.load_only() requires real ORM-mapped attribute objects in SQLAlchemy 2.0 (ArgumentError: expected ORM mapped attribute for loader strategy argument) -- passing "id"/"uuid" as plain strings, as these two dynamically-generated import-mixin migrations did, no longer works. Both call sites already have the actual dynamically-built model class in scope (models["slices"]), so this just references its real .id/.uuid attributes instead of their string names.
rusackas
changed the title
Flask-SQLAlchemy 3.x scopes db.session by Flask app-context object identity (id(app_ctx._get_current_object())) instead of thread identity like 2.x. Pushing a redundant nested app_context() on a thread that already has one active (as these task-manager call sites did unconditionally) silently splits work across two distinct Session objects under 3.x, where 2.x transparently shared one. Verified locally against real flask-sqlalchemy==3.1.1 vs 2.5.1 installs: nested app_context() on one thread yields the same Session under 2.x, different Session objects under 3.x.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Root cause of the embedded-dashboard/task-framework test failures: Flask-SQLAlchemy 3.x scopes db.session by the identity of the active Flask app-context object (id(app_ctx)) instead of by thread/greenlet identity like 2.x. Superset's codebase and test fixtures widely (and often only implicitly, without an explicit commit()) assume a single shared session per thread across nested app.app_context() blocks - true under 2.x, false under 3.x once any code pushes a second context on the same thread (e.g. the AppContextTask Celery wrapper below, or test fixtures that each do their own `with app.app_context():`). Verified locally (real flask-sqlalchemy==3.1.1 + sqlalchemy==2.0.51, sqlite backend) against the exact tests failing in CI: restoring the 2.x scopefunc fixes tests/integration_tests/embedded/test_view.py (the original symptom from the first #42542 break), tasks/test_event_ handlers.py, tasks/test_timeout.py, and one of two tasks/test_ throttling.py failures. Also guard the Celery AppContextTask wrapper's app_context() push with has_app_context(), so eager-mode task execution (e.g. .apply() called from an existing request/test context) reuses the caller's session instead of unconditionally splitting into a second, blind one - the most impactful single instance of the pattern, since every task run goes through it. Two failures remain locally, both pre-existing and unrelated to this scoping issue (confirmed present in CI before this fix too): dashboards/soft_delete_tests.py's db.session.bind is None under FSA 3.x/SQLA 2.0, and a sqlite-specific check_same_thread cross-thread connection reuse in test_throttling.py's timer-thread path.
…ures Root cause of the ReportSchedule "is not persisted" InvalidRequestError and the cascaded StaleDataError/PendingRollbackError fan-out seen across charts/api_tests.py and dashboards/api_tests.py in CI: SQLAlchemy 2.0 removes the legacy cascade_backrefs behavior entirely. Under 1.4, constructing `ReportSchedule(chart=chart)` where `chart` was already a persistent, session-tracked object implicitly added the new ReportSchedule to the session too, via the Slice.report_schedules backref collection. That auto-cascade is gone in 2.0 (this is a documented, intentional SQLAlchemy 1.4->2.0 removal, not a bug) - an explicit db.session.add() is now required. Without it, `create_chart_with_report`/`create_dashboard_with_report` committed a ReportSchedule that was silently never persisted. The actual test bodies then found no report attached (delete succeeded with 200 instead of the expected 422 block), and fixture teardown's `db.session.delete(report_schedule)` blew up with "Instance ... is not persisted" since the object was transient all along. The dashboard variant's cascading StaleDataError on dashboard_slices was a downstream symptom of the same root cause (the "blocked" dashboard delete actually went through). Other ReportSchedule(...) construction sites in the test suite already call db.session.add() explicitly (deletion_retention/*, charts/ soft_delete_tests.py, databases/api_tests.py, reports/utils.py) or never persist the object at all (reports/alert_tests.py, reports/commands_tests.py) - these two fixtures were the only ones missing it. Production code is unaffected: BaseDAO.create() already calls db.session.add() explicitly, so the real create-report code path never relied on the removed cascade. Verified locally (real flask-sqlalchemy==3.1.1 + sqlalchemy==2.0.51, sqlite backend): tests/integration_tests/charts/api_tests.py (96/96) and tests/integration_tests/dashboards/api_tests.py (156/156) both pass clean after this fix, including the two previously-failing report-block tests and the dashboard_slices StaleDataError case.
…tive PG enum Root cause of the "type filter_type_enum does not exist" postgres failures across security/row_level_security_tests.py: no migration has ever created a native Postgres enum type named filter_type_enum. The 2020-09-15 e5ef6828ac4e migration that added this column only ever created a plain VARCHAR(255) - confirmed against a fresh `superset db upgrade` on postgres:17-alpine (`\d row_level_security_filters` shows filter_type as character varying, and `pg_type` has no filter_type_enum row). The ORM model's `Enum(..., name="filter_type_enum")` declaration was already mismatched with the real schema; that mismatch was harmless under SQLAlchemy 1.4. It stopped being harmless under SQLAlchemy 2.0: the new postgresql "insertmanyvalues" execution strategy renders every bound parameter with an explicit cast to its column type's DDL name, even for a single-row INSERT via ORM flush (e.g. `SELECT p0::VARCHAR, p1::TEXT, p2::filter_type_enum, ... ORDER BY sen_counter RETURNING ...`). Casting to a type name that was never actually created fails outright with psycopg2.errors.UndefinedObject, and poisons the session for the rest of that transaction (surfacing as PendingRollbackError on every subsequent statement in the same test). native_enum=False makes SQLAlchemy render this column as a plain VARCHAR with a CHECK constraint instead of a named Postgres enum type, which is what the physical schema has always actually been. Verified locally against a real postgres:17-alpine container running this branch's exact sqlalchemy==2.0.51/flask-sqlalchemy==3.1.1: before this fix, `test_model_view_rls_add_success` and friends failed with the exact UndefinedObject/PendingRollbackError chain seen in CI; after it, all 47 tests in security/row_level_security_tests.py pass.
…ion.bind Root cause: Flask-SQLAlchemy 2.x's SignallingSession.__init__ explicitly passed `bind=<default engine>` into the SQLAlchemy Session constructor. Flask-SQLAlchemy 3.x dropped that - it resolves the engine per-call via Session.get_bind() (consulting the app's registered engines) instead of fixing one at session-construction time, to support its reworked multi-engine/multi-bind model. db.session.bind is therefore always None under FSA 3.x; it was never a documented API, just an implementation detail of FSA 2.x's Session subclass. Confirmed locally: `db.session.bind` prints None while `db.session.get_bind()` correctly resolves the live Engine, against this branch's real flask-sqlalchemy==3.1.1/sqlalchemy==2.0.51 install. Fixes the "AttributeError: 'NoneType' object has no attribute 'dialect'" failures in dashboards/soft_delete_tests.py's partial-index dialect detection, and pre-emptively fixes the same pattern in versioning/id_reuse_tests.py (skipped under postgres/current in the CI run that surfaced this, so not in the failure log there, but broken by the same mechanism whenever it does run). Verified locally against both sqlite and a real postgres:17-alpine container: dashboards/soft_delete_tests.py and versioning/id_reuse_tests.py pass clean on both backends after this fix.
…ing() encodes them Root cause of the "mypass%25123" != "mypass%123" and "p%40ss%21word" != "p@ss!word" round-trip failures in test_database_password_encoding.py: Database.sqlalchemy_uri_decrypted manually percent-encoded the password with urllib.parse.quote() before handing it to URL.render_as_string(hide_password=False). Under SQLAlchemy 1.4, render_as_string() rendered URL.password as a literal value, so the manual pre-encoding was necessary. Under SQLAlchemy 2.0, render_as_string() always percent-encodes the password itself - the pre-encoded value gets encoded a second time (a literal "%" becomes "%25", which decodes back to "%25" instead of "%" on the next parse). Passing the raw password straight through and letting render_as_string() do the (now single) encoding fixes the round-trip. Confirmed the double-encoding mechanism directly against this branch's sqlalchemy==2.0.51: URL.set(password=<pre-encoded>).render_as_string() produces "mypass%2525123"; URL.set(password=<raw>).render_as_string() correctly produces "mypass%25123". Separately, model_tests.py::test_impersonate_user_trino (and the mysqlclient-only test_adjust_engine_params_mysql, exercised on real CI runners but skipped here where mysqlclient isn't importable) asserted on str(url) for URLs containing a password. SQLAlchemy 2.0 also changed URL.__str__() to hide the password by default (a deliberate hardening change - 1.4 rendered it in full); switched those assertions to render_as_string(hide_password=False) to compare against the real, unmasked URL the engine was actually constructed with, rather than relaxing what's being verified. Verified locally (sqlite): test_database_password_encoding.py (5/5) and model_tests.py (21 passed, 5 skipped - the mysqlclient-gated ones) both pass clean.
…o a new dashboard Same removed-cascade_backrefs pattern as the ReportSchedule test fixtures (see the "add missing db.session.add() in report-schedule test fixtures" commit), found in real production code this time: the legacy Explore "save as" -> "new dashboard" flow (superset/views/core.py, the new_dashboard_name branch of SliceAddView.save_or_overwrite_slice) constructs a brand-new, transient Dashboard, then does `dash.slices.append(slc)` where `slc` is already persistent. Appending a persistent Slice into a transient Dashboard's `slices` collection also populates the reverse `Slice.dashboards` backref - under SQLAlchemy 1.4 that implicitly cascaded the new Dashboard into the session; under 2.0 (cascade_backrefs removed) it no longer does, so `db.session.commit()` silently persisted nothing and the "new dashboard" the user asked for was never created. Confirmed directly: constructing a transient Dashboard, appending a persistent Slice to its `.slices`, and committing left `dash.id` as None (and logged "SAWarning: Object of type <Dashboard> not in session, add operation along 'Slice.dashboards' won't proceed" from Superset's own versioning listener's flush). Adding `db.session.add(dash)` before the append fixes it - verified the same probe then leaves `dash.id` populated after commit. No existing test exercises this specific new_dashboard_name path, so verified via a standalone repro script against this branch's real sqlalchemy==2.0.51/flask-sqlalchemy==3.1.1 rather than a test assertion; ran tests/integration_tests/core_tests.py in full (47 passed, 2 skipped) to confirm no regression to the adjacent saveas/ overwrite paths that do have coverage.
…and caches
Critical regression: SQLAlchemy 2.0 changed URL.__str__() to always
substitute "***" for the password rather than rendering it verbatim
(SQLAlchemy 1.4's str(URL) rendered the real value). Every
build_sqlalchemy_uri() implementation across the db_engine_specs
(base/Postgres+MySQL+etc, ClickHouse, Databricks x2, Snowflake,
Databend, Couchbase) built a URL with the user's real password and
returned str(url) - which is exactly the string
superset/databases/schemas.py's pre-load hook writes into
data["sqlalchemy_uri"] when a database is created or edited via the
parameterized connection form. Under SQLAlchemy 2.0 that stores the
literal password "***" instead of the real one, breaking every new
connection made that way. Switched all of these to
render_as_string(hide_password=False), which is the 2.0-native way to
get the real, unmasked URL string.
Two more instances of the same str(URL) regression in
superset/models/core.py, both with real functional impact:
- Database.set_sqlalchemy_uri() intentionally replaces the real
password with Superset's own PASSWORD_MASK sentinel
("X" * 10, not a secret) before storing self.sqlalchemy_uri, so a
later edit can compare conn.password != PASSWORD_MASK to detect
"the user didn't touch the password field, keep the existing one."
str(conn) under 2.0 was substituting its own "***" for that
sentinel, so the stored URI no longer round-tripped to
PASSWORD_MASK - it round-tripped to the meaningless literal "***",
breaking password-preservation on every database edit.
- The per-process SQLAlchemy engine cache (superset/models/core.py,
_ENGINE_CACHE) keys on str(sqlalchemy_url) specifically so that a
password rotation naturally invalidates the cached engine (the
module comment states this explicitly). Under 2.0, str(url) always
masks to the same "***" regardless of the real password, so
rotating a database's password would silently keep reusing the old,
now-wrong cached engine/connection pool for the life of the worker
process.
Also fixed a separate, unrelated 1.4->2.0 break in
superset/db_engine_specs/duckdb.py: two build_sqlalchemy_uri variants
called the raw URL(...) constructor, which SQLAlchemy 2.0 turned into
a strict NamedTuple requiring username/password/host/port to be
passed explicitly (they used to default to None). That raised
"URL.__new__() missing 4 required positional arguments" outright.
Switched both to URL.create(), which keeps those optional.
Test-side: model_tests.py/db_engine_specs test files that asserted
str(uri) == "<scheme>://user:realpassword@host/..." were relying on
the old unmasked str() behavior; switched them to
uri.render_as_string(hide_password=False) to keep verifying the real
underlying value rather than relaxing what's being checked.
Verified locally (sqlite): tests/integration_tests/db_engine_specs/
and tests/unit_tests/db_engine_specs/ - the password-masking and
duckdb URL() failures are gone (18 -> 12 remaining, unrelated:
mysqlclient not importable on this Mac, 5 bigquery test_fetch_data
failures, and a where_latest_partition literal-rendering cluster
across hive/presto/trino, tracked separately).
…QLAlchemy 2.0
Root cause of the where_latest_partition CompileError across hive/
presto/trino tests ("Could not render literal value '2023-05-01' with
datatype TIMESTAMP"): superset/models/sql_types/presto_sql_types.py's
TimeStamp/Date TypeDecorator subclasses only override process_bind_param,
which returns the *final* literal SQL text ("TIMESTAMP '2023-05-01'") -
Presto/Trino don't support parameter binding for these types, so
process_bind_param has always done double duty as the literal-rendering
hook too.
TypeDecorator.literal_processor()'s standard composition, when only
process_bind_param is overridden (no process_literal_param), pipes its
output through the *impl* type's own literal_processor - i.e. it takes
the string "TIMESTAMP '2023-05-01'" and still runs it through the real
TIMESTAMP type's literal processor, which expects an actual datetime
and calls .isoformat() on it. That blows up with a CompileError. This
composition happens for the process_bind_param fallback path
regardless of whether SQLAlchemy 1.4 or 2.0 - confirmed empirically
that overriding process_literal_param instead doesn't help either,
since TypeDecorator's own literal_processor() chains *both* paths
through the impl processor when the impl has one.
Fixed by overriding literal_processor() directly on both classes -
against TypeDecorator's own docstring advice ("should not implement
this method"), but correct here since process_bind_param already
returns final SQL text rather than a value for further impl
processing to convert.
Separately, hive_tests.py/presto_tests.py hardcoded the expected
compiled SQL for an empty-column select() with two spaces before the
newline ("SELECT \n"); SQLAlchemy 2.0 renders it with one ("SELECT
\n") - a cosmetic Core-compiler change, fixed the expected strings to
match.
Verified locally (sqlite): tests/integration_tests/db_engine_specs/
and tests/unit_tests/db_engine_specs/ where_latest_partition tests
(hive, presto, trino, both integration and unit) all pass.
…ll-bump # Conflicts: # superset/db_engine_specs/databend.py
…rges master currently has two unreconciled migration heads (4f145192b583, the pivot-table-percent-display/report-retry merge, and c4a1b8e2d739, the Databend secure->sslmode migration) - unrelated to this branch's SQLAlchemy 2.0 work, just two migrations that landed around the same time without a merge revision joining them. A fresh `superset db upgrade` fails outright with "Multiple head revisions are present" without this, which blocked local testing of the SQLAlchemy 2.0 fixes on this branch after merging master's tip. Standard trivial Alembic merge migration (empty upgrade/downgrade), generated via `superset db merge heads`.
SQLAlchemy 2.0 removed the subtransactions= parameter from Session.begin(); TestDatasource.setUp() called it unconditionally, failing every test in the class with "TypeError: scoped_session.begin() got an unexpected keyword argument 'subtransactions'" before the test body ever ran. Same fix as #42866 (open at the time of this commit, not yet merged to master despite this branch already having merged master's tip) - applying it directly here since it's blocking all further local verification of tests/integration_tests/datasource_tests.py and is independent of the rest of this branch's SQLAlchemy 2.0 work. Once #42866 merges to master, a future master-merge into this branch will no-op on this file. Verified locally (sqlite): all 31 tests in datasource_tests.py pass.
Root cause of TestDashboardDAO.test_copy_dashboard_copies_native_filters's "Instance '<Dashboard>' is not persisted" InvalidRequestError: copy_dashboard() builds the new Dashboard, calls db.session.add(dash), and returns it without ever flushing. Session.delete() on an object that was add()-ed but never flushed (state.key is still None) has always been invalid in SQLAlchemy - this isn't a 1.4->2.0 behavior change - but it was silently masked whenever the caller happened to run any other query afterward, since Session's default autoflush flushes all pending objects (including this one) before executing that query. The sibling test, test_copy_dashboard_duplicate_slices, passes only by accident: it queries db.session.query(Subject) right after copy_dashboard() returns (to look up the admin's Subject row for an assertion), which autoflushes `dash` as a side effect and gives it a real id before its own db.session.delete(dash) cleanup runs. test_copy_dashboard_copies_native_filters does no such incidental query - it asserts on dash.params_dict (pure Python, no DB access) and goes straight to db.session.delete(dash), so `dash` is still pending and delete() rejects it outright. Flushing explicitly at the end of copy_dashboard() (the DAO doesn't commit - that's the @transaction-decorated command layer's job, per superset/commands/dashboard/copy.py) makes the contract reliable: every caller gets back a Dashboard with a real, persisted id, not one that only works if they happen to touch the DB again afterward. Verified locally (sqlite): tests/integration_tests/dashboards/dao_tests.py (5/5) passes, including both copy_dashboard tests.
…get_table_metadata Root cause of test_get_invalid_table_table_metadata's 422-instead-of-200 on sqlite: SQLAlchemy 2.0's sqlite dialect raises NoSuchTableError from reflection (get_columns/get_pk_constraint/get_indexes/get_foreign_keys/ get_table_comment) for a table that doesn't exist. SQLAlchemy 1.4's sqlite dialect silently returned empty results instead - the API has always relied on that specifically for sqlite to answer with an empty-but-200 payload, which the test codifies explicitly (mysql and other backends already expect 422 for the same request, since their dialects already raised on missing tables pre-2.0 too - this is a sqlite-only regression). get_table_metadata() now catches NoSuchTableError, and only for sqlite falls back to the same empty-metadata shape the old dialect produced; every other backend re-raises unchanged, preserving the existing 422 behavior there. Also had to stop select_star() from re-triggering the same NoSuchTableError via its own internal database.get_columns() re-fetch (it re-fetches whenever `cols` is empty and either show_cols or latest_partition is requested) by passing latest_partition=False in the missing-table case - a missing table has no partitions to look up anyway. Verified locally (sqlite): test_get_invalid_table_table_metadata passes, and the full tests/integration_tests/databases/ suite (157 passed, 10 skipped, the 1 unrelated pre-existing count-mismatch failure was local DB-file reuse pollution from earlier ad-hoc runs, confirmed gone on a fresh DB) shows no regression.
Contributor
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 AI Code Review powered by |
rusackas
changed the title
6 tasks
…ll-bump # Conflicts: # pyproject.toml
… experiment/sqla2-full-bump
…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.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
# Conflicts: # UPDATING.md
hy144328
approved these changes
Aug 7, 2026
hy144328
left a comment
hy144328
left a comment
Contributor
There was a problem hiding this comment.
Mostly nits.
I was able to follow most changes but I am shaky on the Flask side of things, in particular:
- I have to read up on how app context has changed in Flask SQLAlchemy.
- I am pretty lost on the greenlet/thread nuances.
I will look into it in my own time what error/problem these two things actually fix.
Thanks!
| columns, | ||
| ) | ||
| query_result = str(result.compile(compile_kwargs={"literal_binds": True})) | ||
| assert "SELECT \nWHERE ds = '01-01-19' AND hour = 1" == query_result |
Contributor
There was a problem hiding this comment.
Perhaps it is worthwhile using an SQL prettifier here to make the test more robust unless you actually care for character-by-character equality.
Member
Author
There was a problem hiding this comment.
Exact match is deliberate here — the whitespace difference is the thing under test (2.0's select() with no columns renders one trailing space instead of two). A prettifier would normalize that away and stop catching a regression if 2.1 changes it again.
| indexes = get_indexes_metadata(database, table) | ||
| table_comment = database.get_table_comment(table) | ||
| except NoSuchTableError: | ||
| # SQLAlchemy 2.0's sqlite dialect raises NoSuchTableError from |
Contributor
There was a problem hiding this comment.
Sounds like SQLAlchemy 2 made the behavior for SQLite more consistent to the other databases.
Would it make sense to seize the opportunity of a Superset breaking change to remove the SQLite extra treatment in test as well as API?
Member
Author
There was a problem hiding this comment.
It's tempting, but that sqlite 200-with-empty-payload response is pre-existing behavior — test_get_invalid_table_table_metadata already branches on backend == "sqlite" and asserts the empty-but-200 shape, so it predates this PR and isn't something 2.0 introduced. This PR is just restoring that existing contract after 2.0's sqlite dialect started raising NoSuchTableError where 1.4 silently returned empty results. Unifying sqlite onto the 422 path would be a real API behavior change (and could break anyone relying on the current 200), so I'd rather keep that as a separate, deliberate follow-up than fold it into a dependency-bump PR.
| # the SELECT of a scalar value without a table is | ||
| # appropriately formatted for the backend | ||
| connection.scalar(select(1)) | ||
| connection.rollback() # pylint: disable=consider-using-transaction |
Contributor
There was a problem hiding this comment.
Out of curiosity, as mentioned, my unit tests all failed because during Superset initialization it was not possible to open a second transaction.
Is this the fix for this?
Member
Author
There was a problem hiding this comment.
Yes — see the commit message on a7d9bde: under 2.0's autobegin, the health-check SELECT opens a transaction it never used to, and that lingering transaction is what produced the lock-wait-timeout / can't-open-second-transaction symptom during init. rollback() closes it back out after the ping.
| "SQLite Database support for metadata databases will be " | ||
| "removed in a future version of Superset." | ||
| ) | ||
| # SQLAlchemy 2.0 changed the default poolclass for file-based SQLite |
Contributor
There was a problem hiding this comment.
Will there still be SQLite support in Superset 7?
If yes, I might have missed it, but I did not see a similar update in the production code, in addition to the test here.
Or is the responsibility delegated from a default to the admin?
Member
Author
There was a problem hiding this comment.
Production doesn't need a matching change because it doesn't hit this: the pool-reuse issue only bites check_same_thread=true URIs (what the test suite uses), and Superset's non-test default is check_same_thread=false, which was already safe under QueuePool. Can't speak to a hard removal date for SQLite support beyond the existing deprecation warning.
rusackas
pushed a commit
that referenced
this pull request
Aug 7, 2026
…my 2.0 core bump dremio, exasol, firebird, redshift, and risingwave each cut hard from a SQLAlchemy-1.4-only line to a SQLAlchemy-2.0-only line with no dual-compat release in between. pyproject.toml already documented each cap as "bump in lockstep with Superset's own SQLAlchemy 2.0 core bump (discussion #40273), not before" -- that bump has now landed (#42803), so this widens all five to their 2.0-compatible ranges: - dremio: sqlalchemy-dremio>=1.2.1,<3.0.5 -> >=3.0.5,<4 - exasol: sqlalchemy-exasol>=2.4.0,<6.0.0 -> >=6.0.0,<8.0 - firebird: sqlalchemy-firebird>=0.8.0,<2.0.0 -> >=2.2.0 - redshift: sqlalchemy-redshift>=0.8.1,<0.9 -> >=1.0.0 - risingwave: sqlalchemy-risingwave>=1.4.1,<2.0.0 -> >=2.0.0 None of these five are pinned in requirements/base.txt, requirements/development.txt, or any other lockfile -- they're purely optional per-connector extras -- so no lockfile regeneration is needed alongside this pyproject.toml change. This does not close out discussion #40273's driver survey entirely: aurora-data-api, d1, kusto, solr, and ocient remain blocked on their own upstream SQLAlchemy 2.0 support (or, for ocient, unverified compatibility), independent of Superset's own bump.
Contributor
Code Review Agent Run #5c0b5fActionable 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 AI Code Review powered by |
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>
Contributor
Code Review Agent Run #3cdca8Actionable 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 AI Code Review powered by |
rusackas
commented
Aug 8, 2026
Member
Author
There was a problem hiding this comment.
Minor, non-blocking follow-ups
superset/extensions/__init__.py'stry: from greenlet import getcurrent … except ImportError: from threading import get_ident—greenletis already a hardpyproject.tomldependency (>=3.5.4,<=3.5.4), so the except branch is unreachable in practice. Not wrong, just worth simplifying or commenting as defensive-only in a follow-up.- Bito's earlier suggestion about
superset/tasks/context.py's_deferred_flushusing an inlineif ... and not has_app_context()guard while the other three call sites in the same file use thenullcontext()pattern — still valid as a style-consistency nit, purely cosmetic (both are correct). - Also still-open from Bito: no dedicated unit test for the
has_app_context()branch itself (task-manager/task-context call sites when an app context is already active). Worth a small follow-up test PR — the behavior is exercised indirectly by the integration suite passing, but a direct unit test would pin it down explicitly. - Lower-confidence, worth a quick sanity check rather than a real concern:
pytest.inidrops allRemovedIn20Warningerror:lines, including three relationships not in discussion #40273's original checked-off list (ReportExecutionLog,ReportRecipients,SSHTunnelmerged-into-session warnings). Dropping them is correct regardless — that warning class doesn't exist once 2.0 is actually installed — but if any of those three ever get a similar "missing implicit cascade" bug as the ones already fixed here, it'd now only surface as silent data loss rather than a caught warning. GivenPython-Integration/Python-Unitare both fully green across three real-CI runs, I'd call this addressed in practice, just flagging for awareness.
rusackas
requested review from
dpgaspar,
hainenber,
nytai and
sha174n
as code owners
August 8, 2026 04:37
github-actions
Bot
added
the
github_actions
hainenber
reviewed
Aug 8, 2026
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>
Contributor
Code Review Agent Run #c9bc2cActionable 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 AI Code Review powered by |
…r SQLAlchemy 2.0 (#42891) Co-authored-by: Claude Code <noreply@anthropic.com>
Contributor
Code Review Agent Run #0828caActionable 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 AI Code Review powered by |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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