refactor: migrate db.session.query to select in infra layer#33694
Conversation
Summary of ChangesHello, 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 database interaction code across several core modules to align with SQLAlchemy 2.0's modern API. The changes involve updating query construction, delete operations, and update statements to use the newer Highlights
🧠 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. 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. Footnotes
|
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-03-18 15:24:32.189801172 +0000
+++ /tmp/pyrefly_pr.txt 2026-03-18 15:24:22.229825478 +0000
@@ -1,3 +1,11 @@
+ERROR Object of class `Result` has no attribute `rowcount` [missing-attribute]
+ --> commands/plugin.py:52:21
+ERROR Object of class `Result` has no attribute `rowcount` [missing-attribute]
+ --> commands/plugin.py:99:21
+ERROR Object of class `Result` has no attribute `rowcount` [missing-attribute]
+ --> commands/plugin.py:139:21
+ERROR Object of class `Result` has no attribute `rowcount` [missing-attribute]
+ --> commands/storage.py:744:27
ERROR `result` may be uninitialized [unbound-name]
--> controllers/console/app/annotation.py:126:16
ERROR Object of class `App` has no attribute `access_mode` [missing-attribute]
|
There was a problem hiding this comment.
Code Review
The pull request successfully migrates various SQLAlchemy 1.x query syntaxes to the SQLAlchemy 2.0 style across multiple files. This includes updating db.session.query().filter_by().delete() to db.session.execute(delete().where()), db.session.query().filter_by().all() to db.session.scalars(select().where()).all(), and db.session.query().filter_by().first() to db.session.scalar(select().where()). These changes are consistent with the SQLAlchemy 2.0 migration guide and improve code readability and maintainability by using explicit select, delete, and update statements. No critical or high-severity issues were found.
a21517c to
fff9482
Compare
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-03-18 16:33:16.538087374 +0000
+++ /tmp/pyrefly_pr.txt 2026-03-18 16:33:07.576042124 +0000
@@ -1,3 +1,11 @@
+ERROR Object of class `Result` has no attribute `rowcount` [missing-attribute]
+ --> commands/plugin.py:52:21
+ERROR Object of class `Result` has no attribute `rowcount` [missing-attribute]
+ --> commands/plugin.py:99:21
+ERROR Object of class `Result` has no attribute `rowcount` [missing-attribute]
+ --> commands/plugin.py:139:21
+ERROR Object of class `Result` has no attribute `rowcount` [missing-attribute]
+ --> commands/storage.py:744:27
ERROR `result` may be uninitialized [unbound-name]
--> controllers/console/app/annotation.py:126:16
ERROR Object of class `App` has no attribute `access_mode` [missing-attribute]
|
fff9482 to
5d9655d
Compare
Pyrefly DiffNo changes detected. |
Pyrefly DiffNo changes detected. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request successfully migrates various database query and update operations from the legacy SQLAlchemy 1.x API to the modern SQLAlchemy 2.0 style. This refactoring improves code clarity, leverages newer features, and aligns with current best practices for SQLAlchemy usage. The changes are consistently applied across multiple files, including api/commands/plugin.py, api/commands/storage.py, api/commands/system.py, api/commands/vector.py, api/events/event_handlers/create_document_index.py, api/events/event_handlers/update_app_dataset_join_when_app_model_config_updated.py, api/events/event_handlers/update_app_dataset_join_when_app_published_workflow_updated.py, api/extensions/ext_login.py, api/factories/file_factory.py, api/schedule/check_upgradable_plugin_task.py, api/schedule/clean_embedding_cache_task.py, api/schedule/clean_unused_datasets_task.py, api/schedule/create_tidb_serverless_task.py, and api/schedule/mail_clean_document_notify_task.py. The use of session.scalars() for fetching multiple entities, session.scalar() for single entities, and session.execute() for DML operations (DELETE, UPDATE) is correctly implemented. Additionally, the explicit inclusion of .limit(1) in queries expecting a single result enhances readability and intent.
|
@asukaminato0721 Please review again. |
|
Could you split this into smaller pr? |
How to implement none-cast code for "--- /tmp/pyrefly_base.txt 2026-03-18 16:33:16.538087374 +0000
Please give me your opinion. |
|
The pyrefly type errors are occurring because when you migrate from the legacy The fix: use from sqlalchemy.engine import CursorResult
from typing import cast
# Instead of:
result = session.execute(delete(Model).where(...))
rows_deleted = result.rowcount # Type error!
# Do this:
result = cast(CursorResult, session.execute(delete(Model).where(...)))
rows_deleted = result.rowcount # ✓ No errorThis pattern is already used throughout Dify's codebase. See examples in: The This is the standard type-safe pattern in Dify—the maintainer's concern about the cast part is unfounded since it's consistent with existing code. To reply, just mention @dosu. How did I do? Good | Irrelevant | Incorrect | Verbose | Hallucination | Report 🐛 | Other |
|
@asukaminato0721 Thanks for your review. |
|
@asukaminato0721 Thank you. |
Summary
db.session.query()calls to SQLAlchemy 2.0select()APIacross infrastructure modules:
schedule/,events/,commands/,extensions/, andfactories/.filter_by().delete()and.filter_by().update()withexecute(delete(...))andexecute(update(...)).limit(1)to non-primary-key lookups to preserve the implicitLIMIT 1behavior of the legacy.first()methodTest plan
ruff checkpasses on all modified filesfactories/,tasks/)db.session.queryorsession.queryremaining in modified filesCloses #22668 (partial)