fix(deps): add temporary SQLAlchemy 1.4 compatibility lane - #43263
fix(deps): add temporary SQLAlchemy 1.4 compatibility lane#43263aminghadersohi wants to merge 1 commit into
Conversation
github-actions
Bot
added
github_actions
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if transaction := connection.get_transaction(): | ||
| transaction.rollback() |
There was a problem hiding this comment.
Suggestion: In the invalidated-connection path, the failed SELECT 1 has already started a transaction under SQLAlchemy 2.x. Retrying connection.scalar(select(1)) before rolling back leaves the invalidated connection in an active transaction, so SQLAlchemy can raise PendingRollbackError instead of reconnecting. Roll back the existing transaction before the retry, then roll back any transaction created by the retry. [state/lifecycle]
Severity Level: Major ⚠️
- ❌ Stale pooled connections can fail health-check recovery.
- ⚠️ Requests requiring `db.engine` may receive connection errors.
- ⚠️ Transient database disconnects may become application-level failures.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/core.py
**Line:** 833:834
**Comment:**
*State Lifecycle: In the invalidated-connection path, the failed `SELECT 1` has already started a transaction under SQLAlchemy 2.x. Retrying `connection.scalar(select(1))` before rolling back leaves the invalidated connection in an active transaction, so SQLAlchemy can raise `PendingRollbackError` instead of reconnecting. Roll back the existing transaction before the retry, then roll back any transaction created by the retry.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| assert "FROM dbs LEFT OUTER JOIN ssh_tunnels" in sql | ||
| assert "'[my_db].(id:42)', '[my_other_db].(id:43)'" in sql | ||
| assert "dbs.database_name IN ('my_db', 'my_other_db', 'third_db')" in sql |
There was a problem hiding this comment.
Suggestion: The assertions only verify that each predicate appears somewhere in the SQL, not that the permission predicates are combined with the required OR grouping. A regression changing the authorization expression to AND, or otherwise altering its boolean grouping, would still pass all three assertions while returning an incorrect set of accessible databases. Assert the complete predicate structure or execute the query against representative rows. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Database listing authorization regressions remain undetected.
- ⚠️ Report APIs apply `DatabaseFilter` to database relations.
- ⚠️ Saved-query database selectors use the same filter.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/databases/filters_test.py
**Line:** 123:125
**Comment:**
*Incomplete Implementation: The assertions only verify that each predicate appears somewhere in the SQL, not that the permission predicates are combined with the required `OR` grouping. A regression changing the authorization expression to `AND`, or otherwise altering its boolean grouping, would still pass all three assertions while returning an incorrect set of accessible databases. Assert the complete predicate structure or execute the query against representative rows.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Code Review Agent Run #edce83Actionable 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43263 +/- ##
==========================================
- Coverage 66.65% 66.06% -0.59%
==========================================
Files 2874 2874
Lines 163781 163783 +2
Branches 37798 37800 +2
==========================================
- Hits 109165 108207 -958
- Misses 52477 53434 +957
- Partials 2139 2142 +3
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:
|
|
Closing this in favor of fixing forward on SQLAlchemy 2.0. We are keeping the SQLAlchemy 2 upgrade, updating and qualifying the downstream driver fleet, adding a machine-readable driver inventory/drift gate, and introducing real runtime coverage for materially used connectors rather than retaining the rollback path. The first Shell implementation is preset-io/superset-shell#4845, with follow-up work planned for Athena qualification, DataFusion restoration, customer-usage prioritization, and permanent connector canaries. |
1 participant
SUMMARY
Adds a temporary, tested SQLAlchemy 1.4 compatibility lane for downstreams such as Preset Shell without reverting the SQLAlchemy 2 upgrade from #42803.
The normal OSS lock files and install path remain on SQLAlchemy 2.0.51 with Flask-SQLAlchemy 3.1.1. Downstreams using the bridge must constrain both SQLAlchemy 1.4.54 and Flask-SQLAlchemy 2.5.1 together, using
requirements/sqlalchemy14.txtas the reference pair.Python package metadata cannot express correlated dependency alternatives ("SQLAlchemy 1.4 + Flask-SQLAlchemy 2.5" or "SQLAlchemy 2 + Flask-SQLAlchemy 3.1"). The widened bounds therefore expose the union needed by a downstream constraints file; arbitrary cross-pair combinations are not supported. Excluding Flask-SQLAlchemy 3.0.x, keeping the generated OSS requirements on the modern pair, and validating the exact legacy pair in CI avoids presenting an accidental third supported lane.
Runtime compatibility remains narrow:
The commonly tested
bigquery,druid,duckdb,fastmcp,gevent,gsheets,mysql,postgres,presto,prophet,trino, andthumbnailsextras are available in both lanes. The selecteddremio,exasol,firebird,redshift, andrisingwavedriver lines are explicitly documented as SQLAlchemy 2-only; their OSS defaults are not silently downgraded. Other extras are not covered by the legacy lane and require downstream validation.This is intended as a temporary migration bridge. Once Preset Shell and other known downstreams have moved to SQLAlchemy 2, remove the paired constraints, widened lower bounds, compatibility code, documentation, and legacy CI job together. #43260 remains the broader full-rollback alternative; this PR does not close it.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable; dependency, backend compatibility, and CI changes only.
TESTING INSTRUCTIONS
Regenerated dependency artifacts with repository tooling; the generated files were unchanged and continue to pin SQLAlchemy 2.0.51 / Flask-SQLAlchemy 3.1.1:
Validated dependencies and ran the CI-selected app initialization, ORM/session/engine, migration, SAVEPOINT, MCP isolation, DuckDB, and SQL Lab tests on both exact stacks:
Ran a full empty-SQLite
superset db upgradesuccessfully on both stacks.PRE_COMMIT_HOME=/tmp/pre-commit-cache pre-commit run # passedThe
sqlalchemy14-compatibilityCI job installs the existing development lock, replaces only the two packages fromrequirements/sqlalchemy14.txt, runsuv pip check, and executes the targeted suite. The existing required SA2 unit-test matrix is unchanged; its stable required anchor now also requires the compatibility lane.ADDITIONAL INFORMATION