Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors request parser definitions in several controller files by extracting them from within route handler methods to module-level scope and adding @api.expect() decorators to improve API documentation. The changes primarily affect plugin management, statistics, app management, and authentication endpoints.
Key Changes:
- Extracted request parsers from method-level to module-level scope
- Added
@api.expect(parser)decorators to document API request schemas - Added
apiimport to files where needed - Added missing
languageparameter documentation in setup endpoint
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/controllers/console/workspace/plugin.py | Extracted 18 request parsers to module scope and added @api.expect() decorators for plugin API endpoints |
| api/controllers/console/version.py | Moved version check parser to module scope and added @api.expect() decorator |
| api/controllers/console/setup.py | Added documentation for optional language parameter in setup API |
| api/controllers/console/datasets/rag_pipeline/datasource_auth.py | Extracted 7 datasource authentication parsers to module scope with @api.expect() decorators |
| api/controllers/console/app/statistic.py | Extracted shared statistics parser to module scope, reused across 7 endpoints |
| api/controllers/console/app/app_import.py | Moved app import parser to module scope and added @api.expect() decorator |
| api/controllers/console/app/app.py | Extracted app name parser to module scope and added @api.expect() decorator |
| api/controllers/console/app/annotation.py | Moved annotation update parser to module scope with @api.expect() decorator |
| api/controllers/console/app/agent.py | Extracted agent log parser to module scope and added @api.expect() decorator |
| api/controllers/console/app/advanced_prompt_template.py | Moved prompt template parser to module scope with @api.expect() decorator |
Comments suppressed due to low confidence (1)
api/controllers/console/datasets/rag_pipeline/datasource_auth.py:1
- The variable name
reqis used inconsistently withparserused elsewhere in the file. Use consistent naming across all request parsers.
from flask import make_response, redirect, request
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors request parsers across multiple API controllers by moving them to the module level for reuse. This is a good practice that improves code clarity and performance. The changes also add @api.expect decorators, which enhances API documentation.
However, I've found a critical issue in api/controllers/console/datasets/rag_pipeline/datasource_auth.py and api/controllers/console/workspace/plugin.py where parser variable names (parser, req) are reused at the module level. This will lead to incorrect argument parsing for some endpoints as only the last definition will be effective.
Additionally, a minor regression was found where help texts for parser arguments were removed in one file, which reduces the clarity of the API documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors request parsers in several controller files by moving them to the module level for reuse and adding @api.expect for Swagger documentation. While this is a good improvement for code clarity and maintainability, there is a critical issue in multiple files where parsers with the same name (parser, req, etc.) are defined at the module level. This causes later definitions to overwrite earlier ones, leading to incorrect request argument validation and parsing for most of the affected API endpoints. This needs to be fixed by giving each parser a unique name within its module.
api/controllers/console/datasets/rag_pipeline/datasource_auth.py
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request primarily refactors the code to move reqparse.RequestParser definitions to the module level. This is a positive change that improves performance and code clarity by avoiding parser re-creation on every request and reducing duplication. While the refactoring is generally well-executed, I've identified a critical issue in api/controllers/console/workspace/plugin.py where a parser variable req is redefined multiple times, which would lead to incorrect behavior. I've provided detailed comments and suggestions to fix this. Additionally, there's a minor performance improvement opportunity in api/controllers/console/workspace/account.py.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the code to define reqparse.RequestParser instances at the module level, which is a great improvement for performance and code clarity. It also adds @api.expect decorators, improving the API documentation.
The changes are well-executed across multiple files. I've found one minor inconsistency in api/controllers/console/workspace/account.py where a function is used to create a parser instead of a module-level variable, which I've commented on.
Please consider updating the pull request title and summary to better reflect the changes. A title like "Refactor: Move request parsers to module level and improve API docs" would be more descriptive.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a large-scale refactoring that significantly improves the code quality and API documentation. By moving reqparse.RequestParser definitions to the module level and using them with @api.expect, the code is now more consistent, readable, and maintainable. This change also fixes several inconsistencies where the API documentation did not match the actual implementation. The changes are well-executed across numerous files. This is an excellent contribution to the project's health. For future pull requests, I'd suggest a more descriptive title and a summary in the description to better reflect the changes made. For example, a title like "Refactor API request parsers for consistency and documentation" would be more informative. Overall, great work!
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring that moves reqparse.RequestParser definitions to the module level across numerous API controllers. This change greatly improves code clarity and maintainability by adhering to the DRY principle. It also fixes several inconsistencies between API documentation (@api.expect) and the actual argument parsing logic, which is a great improvement.
I've found a couple of minor typos in variable names and have a suggestion for making parser names more descriptive for better long-term maintainability. Overall, this is a solid and well-executed refactoring.
d0ec069 to
b30a42d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and beneficial refactoring across numerous API controllers. By moving reqparse.RequestParser definitions to the module level and adding @api.expect decorators, the code is now more performant, organized, and better documented. This change also fixes several inconsistencies where the API documentation did not match the implementation. I've identified a couple of minor areas for improvement where argument handling could be simplified to rely on the parser's defaults, making the code clearer and more robust. Overall, this is a great improvement to the codebase.
api/controllers/console/datasets/rag_pipeline/rag_pipeline_workflow.py
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a large-scale refactoring that moves reqparse.RequestParser definitions to the module level across numerous API controllers. This is a great improvement for code quality and maintainability, following the DRY principle by making parsers reusable and adding @api.expect decorators for better API documentation. The changes are consistent and well-executed. I've identified a couple of minor issues, primarily a potential unintended change in default parameter values and an opportunity to improve a parser's name for clarity.
api/controllers/console/datasets/rag_pipeline/rag_pipeline_workflow.py
Outdated
Show resolved
Hide resolved
api/controllers/console/datasets/rag_pipeline/rag_pipeline_workflow.py
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring effort. By moving the reqparse.RequestParser definitions to the module level and reusing them, you've greatly improved code reusability and maintainability, following the DRY principle. The addition of @api.expect decorators also enhances API documentation, which is a great plus. I've also noticed that this refactoring fixed a few inconsistencies and bugs, such as incorrect default values and redundant code. Overall, these changes improve the quality and clarity of the codebase. I have one suggestion for further consolidation in one of the files to reduce redundancy even more.
|
/gemini review |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…kflow.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…kflow.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
84e98a8 to
42bbd08
Compare
* fix(api): Trace Hierarchy, Span Status, and Broken Workflow for Arize & Phoenix Integration (langgenius#27937) Co-authored-by: crazywoola <100913391+crazywoola@users.noreply.github.com> * chore: add type-check to pre-commit (langgenius#28005) * fix document enable (langgenius#28081) * chore: not SaaS version can query long log time range (langgenius#28109) * When graph_engine worker run exception, keep the node_id for deep res… (langgenius#26205) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: -LAN- <laipz8200@outlook.com> * fix document index test (langgenius#28113) * chore: improve the user experience of not login into apps (langgenius#28120) * feat(api): Introduce `WorkflowResumptionContext` for pause state management (langgenius#28122) Certain metadata (including but not limited to `InvokeFrom`, `call_depth`, and `streaming`) is required when resuming a paused workflow. However, these fields are not part of `GraphRuntimeState` and were not saved in the previous implementation of `PauseStatePersistenceLayer`. This commit addresses this limitation by introducing a `WorkflowResumptionContext` model that wraps both the `*GenerateEntity` and `GraphRuntimeState`. This approach provides: - A structured container for all necessary resumption data - Better separation of concerns between execution state and persistence - Enhanced extensibility for future metadata additions - Clearer naming that distinguishes from `GraphRuntimeState` The `WorkflowResumptionContext` model makes extending the pause state easier while maintaining backward compatibility and proper version management for the entire execution state ecosystem. Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * add transform-datasource-credentials command online check (langgenius#28124) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Garfield Dai <dai.hai@foxmail.com> * feat: introduce trigger functionality (langgenius#27644) Signed-off-by: lyzno1 <yuanyouhuilyz@gmail.com> Co-authored-by: Stream <Stream_2@qq.com> Co-authored-by: lyzno1 <92089059+lyzno1@users.noreply.github.com> Co-authored-by: zhsama <torvalds@linux.do> Co-authored-by: Harry <xh001x@hotmail.com> Co-authored-by: lyzno1 <yuanyouhuilyz@gmail.com> Co-authored-by: yessenia <yessenia.contact@gmail.com> Co-authored-by: hjlarry <hjlarry@163.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: WTW0313 <twwu@dify.ai> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix: determine cpu cores determination in baseedpyright-check script on macos (langgenius#28058) * fix: variable assigner can't assign float number (langgenius#28068) * Add file type validation to paste upload (langgenius#28017) * test: create some hooks and utils test script, modified clipboard test script (langgenius#27928) * convert to TypeBase (langgenius#27935) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * chore: disable workflow logs auto-cleanup by default (langgenius#28136) This PR changes the default value of `WORKFLOW_LOG_CLEANUP_ENABLED` from `true` to `false` across all configuration files. ## Motivation Setting the default to `false` provides safer default behavior by: - Preventing unintended data loss for new installations - Giving users explicit control over when to enable log cleanup - Following the opt-in principle for data deletion features Users who need automatic cleanup can enable it by setting `WORKFLOW_LOG_CLEANUP_ENABLED=true` in their configuration. * fix: simplify graph structure validation in WorkflowService (langgenius#28146) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * fix: app's ai site text to speech api (langgenius#28091) * refactor(web): reuse the same edit-custom-collection-modal component, and fix the pop up error (langgenius#28003) * add doc (langgenius#28016) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * fix: inconsistent behaviour of zoom in button and shortcut (langgenius#27944) * consolve conficts * fix * fix --------- Signed-off-by: lyzno1 <yuanyouhuilyz@gmail.com> Co-authored-by: Ali Saleh <saleh.a@turing.com> Co-authored-by: crazywoola <100913391+crazywoola@users.noreply.github.com> Co-authored-by: lyzno1 <92089059+lyzno1@users.noreply.github.com> Co-authored-by: Jyong <76649700+JohnJyong@users.noreply.github.com> Co-authored-by: Joel <iamjoel007@gmail.com> Co-authored-by: 湛露先生 <zhanluxianshen@163.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: -LAN- <laipz8200@outlook.com> Co-authored-by: QuantumGhost <obelisk.reg+git@gmail.com> Co-authored-by: Garfield Dai <dai.hai@foxmail.com> Co-authored-by: Yeuoly <45712896+Yeuoly@users.noreply.github.com> Co-authored-by: Stream <Stream_2@qq.com> Co-authored-by: zhsama <torvalds@linux.do> Co-authored-by: Harry <xh001x@hotmail.com> Co-authored-by: lyzno1 <yuanyouhuilyz@gmail.com> Co-authored-by: yessenia <yessenia.contact@gmail.com> Co-authored-by: hjlarry <hjlarry@163.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Co-authored-by: WTW0313 <twwu@dify.ai> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Bowen Liang <liang.bowen.123@qq.com> Co-authored-by: Gen Sato <52241300+halogen22@users.noreply.github.com> Co-authored-by: Gritty_dev <101377478+codomposer@users.noreply.github.com> Co-authored-by: Asuka Minato <i@asukaminato.eu.org> Co-authored-by: mnasrautinno <m.nasr@dai.autinno.com> Co-authored-by: yangzheli <43645580+yangzheli@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Important
Fixes #<issue number>.Summary
part of #24421
Screenshots
Checklist
dev/reformat(backend) andcd web && npx lint-staged(frontend) to appease the lint gods