refactor: use sessionmaker().begin() in web and mcp controllers#34281
Conversation
dosubot
bot
added
size:S
Pyrefly DiffNo changes detected. |
|
Hi @asukaminato0721 Do I need to addressing CI test failure? |
There was a problem hiding this comment.
Pull request overview
Refactors Web and MCP API controllers to use sessionmaker(...).begin() transaction scoping instead of constructing Session(...) objects with explicit commit(), aligning controller session usage with SQLAlchemy 2.0 session framing guidance.
Changes:
- Replaced
Session(db.engine, ...)context managers withsessionmaker(db.engine, ...).begin()in web and MCP controllers. - Removed an explicit
session.commit()path in web forgot-password reset flow, relying on the surroundingbegin()scope to commit. - Simplified MCP request handling session framing for end-user retrieval/creation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| api/controllers/web/wraps.py | Switches JWT decode DB lookups to sessionmaker(...).begin() with expire_on_commit=False. |
| api/controllers/web/forgot_password.py | Updates forgot-password flows to sessionmaker(...).begin() and removes explicit commit from password update helper. |
| api/controllers/web/conversation.py | Uses sessionmaker(db.engine).begin() for conversation list pagination query. |
| api/controllers/mcp/mcp.py | Moves MCP controller session usage to sessionmaker(...).begin() and refactors end-user retrieval/creation session scopes. |
Comments suppressed due to low confidence (1)
api/controllers/web/forgot_password.py:88
sessionmaker(db.engine).begin()will commit on exit, and with SQLAlchemy defaults that expires loaded ORM instances. Hereaccountis used after the context manager (passed toAccountService.send_reset_password_email), which can triggerDetachedInstanceErrorwhen accessingaccount.emailafter the commit/close. Either avoid committing for this read-only lookup (e.g., use a plain session context withoutbegin()), or setexpire_on_commit=False, or pass primitives (email/id) out of the session instead of the ORM instance.
with sessionmaker(db.engine).begin() as session:
account = AccountService.get_account_by_email_with_case_fallback(request_email, session=session)
token = None
if account is None:
raise AuthenticationFailedError()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
for the failed unit test, i suggest just port to container. |
|
@asukaminato0721 Shall I open more PR for migrating unit test to container or just address them in this PR? |
|
open more pr，keep each pr small to easy to review |
Pyrefly DiffNo changes detected. |
Desel72
force-pushed
the
refactor/use-sessionmaker-web-controllers
branch
from
e97d229 to
eb8e0af
Compare
dosubot
bot
added
size:M
Pyrefly DiffNo changes detected. |
Pyrefly DiffNo changes detected. |
Summary
Part of #24245.
Refactor web and MCP controllers to use
sessionmaker().begin()instead ofSession()with explicitcommit(), following SQLAlchemy best practices.Files changed
api/controllers/web/conversation.pyapi/controllers/web/forgot_password.pyapi/controllers/web/wraps.pyapi/controllers/mcp/mcp.py