chore: session enforce sqlalchemy 2.0 - #42365
Conversation
Code Review Agent Run #c83a74Actionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42365 +/- ##
==========================================
+ Coverage 65.25% 65.34% +0.09%
==========================================
Files 2794 2799 +5
Lines 157396 158022 +626
Branches 35982 35982
==========================================
+ Hits 102706 103267 +561
- Misses 52713 52778 +65
Partials 1977 1977
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:
|
…list The pylint hook declares exclude: ^(...|superset/migrations/|...) but its inline script recomputes the file list from git diff, which bypassed that exclusion. Pylint's ignore=migrations setting in .pylintrc also does not apply to files passed explicitly on the command line, so touching migration files (which legitimately call session.commit()) tripped W9001 consider-using-transaction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Converting to draft for now. |
|
Thanks @hy144328! The pre-commit failure wasn't yours... the pylint hook was pulling migration files into its lint list even though they're meant to be excluded, and migrations legitimately call |
|
Not sure if this is the right approach... sorry if I'm meddling too much, just say the word. |
OK! Thanks for the quick reply! Saved me a ton of work here. This said, as I have just mentioned, I refactored one migration script yesterday in #42277 . Hope that this is not a problem. b0bb20f |
Not at all, I really appreciate it. Potentially saved me a ton of work here. Technically speaking, I would have been open to refactoring from explicit commit to context managers. I guess now is as good a time as ever since this PR already touches over 100 files, including the migration scripts. 😅 |
There was a problem hiding this comment.
LGTM! The diff is fully mechanical (all 203 changed lines are exact future=True twins of what they replace, no logic changes), and I poked at the scary part locally: on 1.4.54, a future session bound to a connection with an open transaction seems to commit and roll back just like a legacy one, so the downgrade() paths should behave the same as before.
One thing worth a glance, though not blocking. db.session(future=True) in the streaming export raises if a scoped session already exists on the thread. The current call path is fine since the generator runs after teardown, but it would bite if anyone ever wraps that response in stream_with_context. Bito's note about the redundant future=True in tags/models.py (the sessionmaker already sets it) is also legit if you feel like trimming it.
|
Capturing the follow-up work from the side discussion, since I'd rather keep this PR mechanical and easy to review than grow it. Adopting Converting the back catalog is optional and separable: the trailing-commit and try/commit/except/rollback shapes convert cleanly, but the loop/batch commits change failure semantics, so I'd leave those alone unless there's a concrete reason to touch one. Then there's deciding whether to lint migrations for real instead of excluding them. That would need a migration-appropriate message (there's no Happy to open issues for these if there's appetite. Thanks again for the PR! |
That is right. I thought about it when grepping |
I have to think about this one a little more (tomorrow). Where I do not quite follow: How is this related to setting As you described correctly, I was quite mechanical about this PR. I did not notice any existing transactions being affected by this. I was also not able to find |
|
Just need a codeowner stamp here due to all the migrations. Perhaps @betodealmeida, @eschutho, @michael-s-molina, @mistercrunch, and/or @sadpandajoe can take a looksee? |
Code Review Agent Run #03d2d6Actionable 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
See #40273 .
SUMMARY
Enforce SQLAlchemy ORM 2.0.
This includes setting
future=Trueon all calls tosessionmaker()andSession().In theory, this should also cover the session objects inside Flask Appbuilder because they are exposed to the Superset codebase as factory methods (SessionMaker objects are callable) that accept
futurearguments.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Unit tests:
ADDITIONAL INFORMATION