refactor: make import/expression layer SQLAlchemy 2.0-compatible#41179
refactor: make import/expression layer SQLAlchemy 2.0-compatible#41179rusackas wants to merge 4 commits into
Conversation
Forward-compatible groundwork for the SQLAlchemy 1.4 -> 2.0 bump. Every change here is backward-compatible with the current SQLAlchemy 1.4.54 / Flask-SQLAlchemy 2.5.1 baseline, so it does not regress anything while shrinking the eventual version bump. Removed-in-2.0 API fixes (these break on 2.0 but still work on 1.4): - `sqlalchemy.orm.eagerload` (removed alias of `joinedload`) in the security manager. - `sqlalchemy.sql.visitors.VisitableType` (removed) -> `sqlalchemy.types.TypeEngine` in dataset importer and mock_data type hints. - `flask_sqlalchemy.BaseQuery` (removed in FSA 3.x, required by SA 2.0) -> import from `flask_sqlalchemy.query.Query` with a fallback to the FSA 2.x path, in the query/saved-query filters and the report command tests. - `Engine.execute` / raw-string execution -> wrap startup health check in a `Connection` + `text()`, and the secrets re-encryptor in `text()` with a single bind dict and `row._mapping[...]` access. Prospective Annotated Declarative prep (no-ops on 1.4, ease the 2.0 flip): - `__allow_unmapped__` on the FAB declarative base so legacy 1.x annotations are tolerated during incremental migration. - `Mapped[...]` return annotations on `created_by_fk`/`changed_by_fk` and `BaseDatasource.slices`. The `select()` and `case()` syntax changes from the same effort already landed via #40276 / #40275, so they are intentionally excluded here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41179 +/- ##
==========================================
+ Coverage 63.73% 64.30% +0.57%
==========================================
Files 2651 2651
Lines 144737 144753 +16
Branches 33402 33402
==========================================
+ Hits 92242 93084 +842
+ Misses 50827 49999 -828
- Partials 1668 1670 +2
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:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #368617
Actionable Suggestions - 2
-
tests/integration_tests/reports/commands_tests.py - 1
- Import pattern duplicated · Line 39-44
-
superset/connectors/sqla/models.py - 1
- Incorrect type annotation for declared_attr · Line 338-338
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/queries/filters.py - 1
- Inconsistent Import Pattern · Line 19-24
Review Details
-
Files reviewed - 11 · Commit Range:
18a2a45..18a2a45- superset/__init__.py
- superset/commands/dataset/importers/v1/utils.py
- superset/connectors/sqla/models.py
- superset/initialization/__init__.py
- superset/models/helpers.py
- superset/queries/filters.py
- superset/queries/saved_queries/filters.py
- superset/security/manager.py
- superset/utils/encrypt.py
- superset/utils/mock_data.py
- tests/integration_tests/reports/commands_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ 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
AI Code Review powered by
The migrator now reads columns via row._mapping[...] and binds the UPDATE params as a single positional dict. Wrap the fixture rows in a _Row stand-in and read the bind dict positionally so the unit tests match the production interface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The re-encrypt path reads column values via row._mapping (SQLAlchemy 2.0 Row API), but the integration tests passed a plain dict, which has no _mapping, raising AttributeError. Build a genuine Row via a small helper that handles both the 1.4 and 2.0 Row constructor signatures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The migrator passes bind params positionally to conn.execute(stmt, params), so the assertion must read call_args.args[1] rather than call_args.kwargs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #d63e3eActionable 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 |
2 participants
SUMMARY
Forward-compatible groundwork for the SQLAlchemy 1.4 → 2.0 migration (companion to #40830 / #39735, and a sibling of the already-merged #40276 and the in-flight #40275). Every change here is backward-compatible with the current
sqlalchemy>=1.4,<2/flask-sqlalchemy 2.xbaseline, so it can land onmastertoday without bumping the version, while shrinking the eventual bump and de-risking it.Two categories:
1. Removed-in-2.0 API fixes — these break on 2.0 but still work on 1.4:
sqlalchemy.orm.eagerload(a removed alias ofjoinedload) →joinedload, in the security manager.sqlalchemy.sql.visitors.VisitableType(removed) →sqlalchemy.types.TypeEngine, in dataset-importer andmock_datatype hints.flask_sqlalchemy.BaseQuery(removed in Flask-SQLAlchemy 3.x, which SA 2.0 requires) → imported fromflask_sqlalchemy.query.Querywith a fallback to the FSA 2.x path, in the query / saved-query filters and the report command tests.Engine.execute/ raw-string execution → startup health check wrapped in aConnection+text(); secrets re-encryptor usestext()with a single bind dict androw._mapping[...]access.2. Prospective Annotated Declarative prep — no-ops on 1.4, but they ease the eventual 2.0 flip:
__allow_unmapped__on the FAB declarative base so legacy 1.x annotations are tolerated during incremental migration.Mapped[...]return annotations oncreated_by_fk/changed_by_fkandBaseDatasource.slices.The
select()andcase()syntax changes from the same effort are intentionally excluded here — they already landed via #40276 and are covered by #40275 respectively.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no user-facing changes.
TESTING INSTRUCTIONS
CI. Locally, all of these were verified against the current
SQLAlchemy 1.4.54/Flask-SQLAlchemy 2.5.1baseline (imports resolve,BaseQueryshim falls back correctly,pre-commit runis green including mypy/pylint/ruff). They should be a no-op behaviorally on 1.4 and unblock the 2.0 import/boot path.ADDITIONAL INFORMATION
🤖 Generated with Claude Code