Skip to content

refactor: api/services/recommended_app_service.py to session#32162

Open
asukaminato0721 wants to merge 1 commit intolanggenius:mainfrom
asukaminato0721:24115-services/recommended_app_service.py
Open

refactor: api/services/recommended_app_service.py to session#32162
asukaminato0721 wants to merge 1 commit intolanggenius:mainfrom
asukaminato0721:24115-services/recommended_app_service.py

Conversation

@asukaminato0721
Copy link
Copy Markdown
Contributor

Important

  1. Make sure you have read our contribution guidelines
  2. Ensure there is an associated issue and you have been assigned to it
  3. Use the correct syntax to link this PR: Fixes #<issue number>.

Summary

part of #24115

Screenshots

Before After
... ...

Checklist

  • This change requires a documentation update, included: Dify Document
  • I understand that this PR may be closed in case there was no previous discussion or issues. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.
  • I ran make lint and make type-check (backend) and cd web && npx lint-staged (frontend) to appease the lint gods

Copilot AI review requested due to automatic review settings February 9, 2026 14:39
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. refactor labels Feb 9, 2026
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello @asukaminato0721, 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 recommended_app_service.py file to improve database session management by using session_factory and ensuring proper transaction handling. This change enhances the reliability and maintainability of database operations within the service.

Highlights

  • Database Session Management: This PR refactors the recommended_app_service.py to use session_factory for database session management instead of directly using db.session. This ensures that each database operation is performed within its own session.
  • Context Management: The code now uses with session_factory.create_session() as session: to create and manage database sessions, ensuring proper resource handling and transaction management.
  • Transaction Handling: The add_trial_app_record method now uses session.begin() to explicitly manage transactions, ensuring atomicity of database operations.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • api/services/recommended_app_service.py
    • Replaced direct db.session usage with session_factory for creating database sessions.
    • Implemented context management using with statement for database sessions.
    • Added explicit transaction management using session.begin() in add_trial_app_record.
Activity
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors RecommendedAppService to use the session_factory for database operations, which is a positive step towards better session management. However, I've identified a performance regression where a database session is created inside a loop, and a pre-existing race condition that should be addressed. My review includes suggestions to optimize the database queries for better performance and to fix the race condition for improved reliability.

Comment thread api/services/recommended_app_service.py
Comment thread api/services/recommended_app_service.py
Comment thread api/services/recommended_app_service.py
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors RecommendedAppService to use the project’s pure SQLAlchemy session_factory instead of Flask-SQLAlchemy’s db.session, aligning with the ongoing DB session migration effort (issue #24115).

Changes:

  • Replace db.session queries with session_factory.create_session() for trial-app lookups.
  • Wrap trial record mutation in a session.begin() transaction context.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread api/services/recommended_app_service.py
Comment thread api/services/recommended_app_service.py
Comment thread api/services/recommended_app_service.py
Comment thread api/services/recommended_app_service.py
Copilot AI review requested due to automatic review settings March 1, 2026 08:23
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 1, 2026

Pyrefly Diff

No changes detected.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 26 to +31
if FeatureService.get_system_features().enable_trial_app:
apps = result["recommended_apps"]
for app in apps:
app_id = app["app_id"]
trial_app_model = db.session.query(TrialApp).where(TrialApp.app_id == app_id).first()
if trial_app_model:
app["can_trial"] = True
else:
app["can_trial"] = False
with session_factory.create_session() as session:
trial_app_model = session.query(TrialApp).where(TrialApp.app_id == app_id).first()
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are existing unit tests for RecommendedAppService, but none exercise the enable_trial_app branches touched here (the session_factory lookups / can_trial flag behavior). Add tests that set FeatureService.get_system_features().enable_trial_app = True and mock session_factory.create_session() so the trial-app behavior is covered and protected during the db.session -> session_factory migration.

Copilot uses AI. Check for mistakes.
Comment on lines 28 to +32
for app in apps:
app_id = app["app_id"]
trial_app_model = db.session.query(TrialApp).where(TrialApp.app_id == app_id).first()
if trial_app_model:
app["can_trial"] = True
else:
app["can_trial"] = False
with session_factory.create_session() as session:
trial_app_model = session.query(TrialApp).where(TrialApp.app_id == app_id).first()
if trial_app_model:
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_recommended_apps_and_categories opens a new SQLAlchemy Session and runs a query for each app in the loop. This creates an N+1 query pattern and can put unnecessary pressure on the connection pool. Consider creating a single session for the whole block and fetching all matching TrialApp.app_id values in one IN (...) query, then setting can_trial via set membership.

Copilot uses AI. Check for mistakes.
Comment on lines 47 to +51
result: dict = retrieval_instance.get_recommend_app_detail(app_id)
if FeatureService.get_system_features().enable_trial_app:
app_id = result["id"]
trial_app_model = db.session.query(TrialApp).where(TrialApp.app_id == app_id).first()
if trial_app_model:
result["can_trial"] = True
else:
result["can_trial"] = False
with session_factory.create_session() as session:
trial_app_model = session.query(TrialApp).where(TrialApp.app_id == app_id).first()
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_recommend_app_detail can return None (e.g., remote retrieval non-200). When enable_trial_app is enabled, this code will raise a TypeError by subscripting result["id"]. Add a guard to return None early when result is falsy before using it in the trial-app lookup.

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 2, 2026

Pyrefly Diff

No changes detected.

@asukaminato0721 asukaminato0721 requested review from Copilot and crazywoola and removed request for Copilot March 13, 2026 13:25
@github-actions
Copy link
Copy Markdown
Contributor

Pyrefly Diff

No changes detected.

Copilot AI review requested due to automatic review settings March 18, 2026 00:22
@github-actions
Copy link
Copy Markdown
Contributor

Pyrefly Diff

No changes detected.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +50 to +54
with session_factory.create_session() as session:
trial_app_model = session.query(TrialApp).where(TrialApp.app_id == app_id).first()
if trial_app_model:
result["can_trial"] = True
else:
@asukaminato0721 asukaminato0721 force-pushed the 24115-services/recommended_app_service.py branch from 0bb07c6 to 262ec62 Compare March 27, 2026 23:24
@github-actions
Copy link
Copy Markdown
Contributor

Pyrefly Diff

No changes detected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants