rusackas commented on PR #40830: URL: https://github.com/apache/superset/pull/40830#issuecomment-4655095659
I dug into this SA 1.4 → 2.0 bump to see how far it can be made to land safely. Short version: **there is no longer a hard upstream blocker** (FAB 5.x already supports SA2), but this is a real multi-PR ORM migration and isn't fully landable in one shot. I've pushed a coherent, backward-compatible prep commit (`731d115`) and here's the full picture. ## No hard upstream dependency blocker Resolving the PR's constraints (`flask-appbuilder>=5.2.1`, `sqlalchemy>=2.0.50,<3`) pulls a clean, fully SA2-compatible set: - `flask-appbuilder 5.2.1` (requires `SQLAlchemy <3,>=1.4`) ✅ - `Flask-SQLAlchemy 3.1.1` ✅ (2.5.1 does **not** support SA2; 3.x does) - `marshmallow-sqlalchemy 1.5.0` ✅ (0.28.x pins `<2.0`; 1.x supports SA2) - `SQLAlchemy-Utils 0.42.1` ✅ So the historical "FAB pins SA<2" wall is gone. The work is application-code migration, not waiting on upstream. ## What I fixed and verified (pushed in `731d115`) All changes are written to work on **both** SA1.4/FSA2.x **and** SA2/FSA3, so they do **not** regress the current baseline (verified: the `tests/unit_tests/db_engine_specs/test_sqlite.py` suite still goes 47/47 on SA1.4 with these changes, and `dao/` + `models/` introduce zero new failures vs. master — the 2 `test_dttm_sql_literal` failures are pre-existing). Breaking-change categories addressed: - **Removed `eagerload`** (alias of `joinedload`) → `superset/security/manager.py`. - **Legacy `select([...])` / `case([...])` list-form** (hard `ArgumentError` in SA2) → `select(...)`/`case(...)` in `connectors/sqla/models.py`, `models/helpers.py`, `utils/core.py`, `extensions/metadb.py`, `common/tags.py`, `commands/importers/v1/utils.py`. - **`flask_sqlalchemy.BaseQuery` removed in FSA3** → `flask_sqlalchemy.query.Query` with a FSA2 fallback (`queries/filters.py`, `queries/saved_queries/filters.py`). - **`sqlalchemy.sql.visitors.VisitableType` removed** → `sqlalchemy.types.TypeEngine` (`utils/mock_data.py`, `commands/dataset/importers/v1/utils.py`). - **Annotated Declarative** (SA2 inspects type annotations on mapped classes): `@declared_attr` columns/relationships now need `Mapped[...]` return annotations — fixed `created_by_fk`/`changed_by_fk` and `BaseDatasource.slices`; set `__allow_unmapped__ = True` on the FAB declarative base in `superset/__init__.py` so remaining legacy 1.x annotations are tolerated during incremental migration. - **`Engine.execute` / raw-string execution removed**: wrapped the startup health check and the secrets-migrator queries in `text()`, used a `Connection`, switched `**kwargs` binds to a single dict, and moved `Row` string-key access to `row._mapping[...]` (`initialization/__init__.py`, `utils/encrypt.py`). - Bumped the in-repo `apache-superset-core` SQLAlchemy pin to `>=2.0.50,<3` (`superset-core/pyproject.toml`) — it was still pinned `<2.0`. With these, **the full app boots under SQLAlchemy 2.0.50** (`create_app()` succeeds, all models map). That was the first wall and it's cleared. ## The dominant remaining blocker (why this isn't fully landable yet) SA 2.0's **autobegin / connection-transaction semantics** break the app/test bootstrap. During `appbuilder.init_app` → FAB `_create_db()` → `inspect(engine).get_table_names()` then `Model.metadata.create_all(engine)`, SA2 raises: > `InvalidRequestError: This connection has already initialized a SQLAlchemy Transaction() object via begin() or autobegin; can't call begin() here unless rollback() or commit() is called first.` This errors **every** unit test that boots the app fixture (e.g. all of `tests/unit_tests/dao/*` error at setup; `test_sqlite.py` goes 47 passed → 47 errors). It reproduces specifically with the in-memory sqlite single-connection pool that the test harness uses. This is a genuine SA2 regression (confirmed against the pristine baseline) and needs deliberate session/engine-lifecycle work — likely committing/closing the session before FAB's `create_all`, or a small upstream FAB fix — not a mechanical edit. I deliberately did **not** force a speculative fix here. ## Remaining migration plan (by category, with refs) 1. **Resolve the autobegin/create_all transaction conflict** in the bootstrap (FAB `_create_db` path). This is the gating item; once green, the unit suite can be run end-to-end on SA2. 2. **Regenerate lockfiles**: `requirements/base.txt` and `requirements/development.txt` still pin `sqlalchemy==1.4.54`, `flask-sqlalchemy==2.5.1`, `marshmallow-sqlalchemy==1.4.0`. These must be re-`pip-compile`d from `pyproject.toml` (FSA must move to 3.x). The dependabot PR only touched `pyproject.toml`, so even the dependency layer is incomplete. 3. **`Engine.execute(<raw str>)` in db_engine_specs** (runtime breaks, db-specific, not on the core path): `db_engine_specs/doris.py:310`, `hive.py:237,263`, `databricks.py:557,561`, `impala.py:100`. Wrap in `text()` + use a `Connection`. 4. **`Query.get()` → `Session.get()`** (deprecated, still works, low risk): `security/manager.py:2082`, `cli/export_example.py:158`, `mcp_service/chart/preview_utils.py:84`, `commands/importers/v1/examples.py:71`, `commands/dataset/duplicate.py:58`, `commands/sql_lab/estimate.py:60`, `daos/dataset.py:417`. 5. **Migration files**: ~100 Alembic migrations import `declarative_base` from `sqlalchemy.ext.declarative` (still works, deprecation warning) and a handful use `MetaData(bind=...)` / `select([...])` (break only if that migration runs). Sweep when convenient. 6. **Full integration-test pass** + `Row`/result-API audit once 1–3 are done. I'll keep the pushed commit as the SA2-prep base since it's safe on the current baseline. Happy to take the autobegin fix next as a follow-up once we decide whether to patch it Superset-side or push it upstream to FAB. -- 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]
