refactor: make import/expression layer SQLAlchemy 2.0-compatible - #41179
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41179 +/- ##
==========================================
- Coverage 64.37% 64.37% -0.01%
==========================================
Files 2653 2653
Lines 145152 145157 +5
Branches 33491 33491
==========================================
Hits 93446 93446
- Misses 50014 50017 +3
- Partials 1692 1694 +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
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 |
|
Curious your take on this one @hy144328 - wasn't sure if this is already on your roadmap. |
d9e13b8 to
63acd9d
Compare
Code Review Agent Run #aa6c24Actionable Suggestions - 0Additional Suggestions - 1
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 AI Code Review powered by |
|
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). 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 |
@rusackas Here are my two cents:
These two were not on my roadmap because the unit tests did not flag them. #40273
Flask SQLAlchemy is also not on my radar yet but it looks good to me.
This one is actually on the roadmap. However, when I grep it, there are many more occurrences that are not covered by unit tests. This is not a problem. I have it on my roadmap, and I will probably crush this warning next.
This is actually on my radar since this is also part of official migration guide. https://docs.sqlalchemy.org/en/20/changelog/migration_20.html#migration-to-2-0-step-six-add-allow-unmapped-to-explicitly-typed-orm-models Long story short, I think this PR complements my roadmap well, and crushes multiple warnings that I have not documented yet. |
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>
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>
The connection check now uses engine.connect() + connection.execute() instead of the removed Engine.execute(), so the test must inject the failure on engine.connect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
76e5cee to
a48f832
Compare
|
Thanks @hy144328, that all tracks. These were just the warnings our unit tests happened to surface, so glad it slots into your roadmap rather than stepping on it. Go for the broader ORM sweep, and ping me if you want a second set of eyes on the |
Code Review Agent Run #8dace6Actionable Suggestions - 0Additional Suggestions - 1
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 AI Code Review powered by |
4 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