Skip to content

refactor: port api/services/auth/api_key_auth_service.py api/services/datasource_provider_service.py api/services/saved_message_service.py api/services/web_conversation_service.py to session#32003

Open
asukaminato0721 wants to merge 4 commits intolanggenius:mainfrom
asukaminato0721:24115

Conversation

@asukaminato0721
Copy link
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 5, 2026 10:14
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. refactor labels Feb 5, 2026
@gemini-code-assist
Copy link
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 implements a significant refactoring of database session management within several core API services. The primary goal is to standardize how database sessions are acquired and managed, moving away from a global db.session to a more explicit, context-managed session_factory.create_session(). This change improves the robustness of database transactions and ensures proper session lifecycle management, contributing to a more maintainable and scalable codebase.

Highlights

  • Database Session Management Refactor: Multiple API service files have been refactored to transition from direct db.session usage to a new session_factory.create_session() context manager for database interactions.
  • Improved Transaction Handling: The refactoring introduces explicit session creation and transaction management using with session_factory.create_session() as session, session.begin():, enhancing reliability and consistency of database operations by ensuring proper commit/rollback.
  • Code Consistency: This change promotes a more uniform approach to database interactions across api_key_auth_service, datasource_provider_service, saved_message_service, and web_conversation_service, leading to a more maintainable codebase.

🧠 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/auth/api_key_auth_service.py
    • Updated imports to use session_factory instead of db.
    • Encapsulated all CRUD operations (get_provider_auth_list, create_provider_auth, get_auth_credentials, delete_provider_auth) within session_factory.create_session() context managers.
    • Removed manual db.session.commit() calls, relying on the context manager for transaction handling.
  • api/services/datasource_provider_service.py
    • Added session_factory import.
    • The remove_datasource_credentials method now uses session_factory.create_session() for its database interaction, ensuring proper session and transaction management.
  • api/services/saved_message_service.py
    • Replaced db import with session_factory.
    • Refactored pagination_by_last_id, save, and delete methods to utilize the new session_factory.create_session() pattern, centralizing session control.
  • api/services/web_conversation_service.py
    • Switched from db to session_factory import.
    • The pin and unpin methods now manage their database sessions using session_factory.create_session(), aligning with the new session management strategy.
Activity
  • The pull request is identified as "part of [Chore/Refactor] db.session.xxx -> with Session as session #24115", indicating it's a component of a larger task or initiative.
  • The author has confirmed that tests have been added for the introduced changes.
  • Documentation has been updated to reflect these changes.
  • Linting and type-checking checks have been successfully performed.
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
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 several service files to use the new session_factory for creating database sessions, replacing the direct use of db.session. This is a positive change that improves session management and consistency. The changes in api_key_auth_service.py, saved_message_service.py, and web_conversation_service.py are well-implemented. However, the refactoring for datasource_provider_service.py is incomplete, as only one method has been updated. For consistency, it would be beneficial to refactor the remaining methods in that file to use session_factory as well. I've also left a couple of minor suggestions to optimize read-only queries.

Copy link
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

This PR refactors four service files from using Flask-SQLAlchemy's db.session to pure SQLAlchemy's session_factory.create_session() pattern. This is part of a larger migration effort to transition the codebase from Flask-SQLAlchemy to pure SQLAlchemy for better flexibility and control over session management.

Changes:

  • Replaced db.session with session_factory.create_session() context managers
  • Added explicit transaction boundaries using session.begin() for write operations
  • Removed unused db import from api_key_auth_service.py

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File Description
api/services/auth/api_key_auth_service.py Refactored all methods to use session_factory; removed db import
api/services/datasource_provider_service.py Refactored remove_datasource_credentials method to use session_factory
api/services/saved_message_service.py Refactored all methods to use session_factory; removed db import
api/services/web_conversation_service.py Refactored pin and unpin methods to use session_factory; removed db import

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

Comment on lines +52 to +53
if saved_message:
return
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

The early return on line 53 occurs within a session.begin() transaction context. When exiting the context manager via return, an empty transaction is committed. While not incorrect, this is inefficient. Consider restructuring to avoid early returns within the transaction block, or move the transaction begin to after the early return check.

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +83
if not saved_message:
return
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

The early return on line 83 occurs within a session.begin() transaction context. When exiting the context manager via return, an empty transaction is committed. While not incorrect, this is inefficient. Consider restructuring to avoid early returns within the transaction block, or move the transaction begin to after the early return check.

Copilot uses AI. Check for mistakes.
Comment on lines +78 to +79
if pinned_conversation:
return
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

The early return on line 79 occurs within a session.begin() transaction context. When exiting the context manager via return, an empty transaction is committed. While not incorrect, this is inefficient. Consider restructuring to avoid early returns within the transaction block, or move the transaction begin to after the early return check.

Copilot uses AI. Check for mistakes.
Comment on lines +110 to +111
if not pinned_conversation:
return
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

The early return on line 111 occurs within a session.begin() transaction context. When exiting the context manager via return, an empty transaction is committed. While not incorrect, this is inefficient. Consider restructuring to avoid early returns within the transaction block, or move the transaction begin to after the early return check.

Copilot uses AI. Check for mistakes.
Comment on lines +39 to +55
with session_factory.create_session() as session:
data_source_api_key_bindings = (
session.query(DataSourceApiKeyAuthBinding)
.where(
DataSourceApiKeyAuthBinding.tenant_id == tenant_id,
DataSourceApiKeyAuthBinding.category == category,
DataSourceApiKeyAuthBinding.provider == provider,
DataSourceApiKeyAuthBinding.disabled.is_(False),
)
.first()
)
.first()
)
if not data_source_api_key_bindings:
return None
if not data_source_api_key_bindings.credentials:
return None
credentials = json.loads(data_source_api_key_bindings.credentials)
return credentials
if not data_source_api_key_bindings:
return None
if not data_source_api_key_bindings.credentials:
return None
credentials = json.loads(data_source_api_key_bindings.credentials)
return credentials
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

The data_source_api_key_bindings object accessed on lines 50-54 is detached from the session (since it's accessed after being queried within a session context that will exit). While the scalar attributes like credentials are already loaded and accessible, following the established pattern in the codebase (e.g., api/core/tools/workflow_as_tool/tool.py:226), the object should be explicitly expunged using session.expunge(data_source_api_key_bindings) before being used outside the session context. This makes it clear that the object is intended to be used as a detached object.

Copilot uses AI. Check for mistakes.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Copilot AI review requested due to automatic review settings February 5, 2026 15:13
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Copy link
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 4 out of 4 changed files in this pull request and generated 1 comment.


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

SavedMessage.app_id == app_model.id,
SavedMessage.created_by_role == ("account" if isinstance(user, Account) else "end_user"),
SavedMessage.created_by == user.id,
with session_factory.create_session() as session:
Copy link

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

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

The pagination_by_last_id method performs only read operations (querying SavedMessage), so it should not use session.begin(). The session.begin() context manager is only needed for write operations (insert, update, delete).

According to the codebase pattern (e.g., api/controllers/console/admin.py:118, api/controllers/console/admin.py:194), read-only queries should use: with session_factory.create_session() as session: without the session.begin() clause.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactor size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant