refactor: admin api using session factory#29628
Conversation
dosubot
bot
added
the
size:L
Summary of ChangesHello @fatelei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the database session management within the admin API by introducing a dedicated Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a new session_factory module and extension to centralize and manage SQLAlchemy database sessions, replacing direct Session(db.engine) calls in api/controllers/console/admin.py with session_factory.create_session(). New files api/core/db/session_factory.py and api/extensions/ext_session_factory.py were added to define and initialize this factory, respectively. Additionally, a new unit test file, api/tests/unit_tests/controllers/console/test_admin.py, was added to cover the business logic of admin endpoints. Review comments indicate that the admin.py controller still incorrectly mixes database sessions, suggesting that all database operations within a method should use a single session from the new session_factory. The reviewer also proposes a refactoring of api/core/db/session_factory.py to simplify its design by encapsulating the session maker state within the SessionFactory class, thereby avoiding global variables and redundant module-level functions.
I am having trouble creating individual review comments. Click here to see my feedback.
api/controllers/console/admin.py (94)
This method incorrectly mixes database sessions. The app object is fetched using db.session (line 78), while recommended_app is fetched using a new session from session_factory. Then, db.session is used to add and commit changes (lines 111, 114, 128).
This will lead to errors or incorrect behavior:
- In the
if not recommended_app:block, you are adding an object (recommended_app) todb.sessionthat was not created in its scope. - In the
elseblock, you are modifyingrecommended_appwhich was fetched from a session that is now closed. These changes will be lost whendb.session.commit()is called.
All database operations within this method should use a single session from session_factory to ensure correctness. The entire method should be wrapped in a single with session_factory.create_session() as session: block.
api/core/db/session_factory.py (4-41)
The current implementation with module-level functions and a class wrapper with static methods is a bit redundant and uses a global variable. It can be simplified by encapsulating the session maker state within the SessionFactory class itself. This avoids using global and makes the code cleaner and more object-oriented. Note that this change will require updating api/extensions/ext_session_factory.py to call session_factory.configure().
class SessionFactory:
_session_maker: sessionmaker | None = None
def configure(self, engine: Engine, expire_on_commit: bool = False):
"""Configure the session factory"""
self._session_maker = sessionmaker(
bind=engine,
expire_on_commit=expire_on_commit
)
def get_session_maker(self) -> sessionmaker:
if self._session_maker is None:
raise RuntimeError("Session factory not configured. Call configure() first.")
return self._session_maker
def create_session(self) -> Session:
return self.get_session_maker()()
session_factory = SessionFactory()
fatelei
force-pushed
the
admin_api_using_session_factory
branch
from
b7c75fa to
b1bfae1
Compare
fatelei
force-pushed
the
admin_api_using_session_factory
branch
from
478d55c to
0a655d8
Compare
|
LGTM |
2 participants
Important
Fixes #<issue number>.Summary
#24245
admin api using session factory instead of Session
Screenshots
test in local
Checklist
dev/reformat(backend) andcd web && npx lint-staged(frontend) to appease the lint gods