-
Notifications
You must be signed in to change notification settings - Fork 18.8k
refactor: port reqparse to BaseModel #28993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
8b7eaae to
ed40556
Compare
|
bug: so name is optional, but ConversationService.rename require name not optional |
|
/gemini review |
There was a problem hiding this 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 is a large-scale refactoring to replace reqparse with Pydantic's BaseModel for API payload validation. This is a great improvement for type safety, maintainability, and consistency across the codebase. The changes are well-executed in most places. I've found a couple of potential issues where the logic might have been unintentionally changed during the refactoring, and one place where the refactoring could be applied for better consistency. Overall, this is a solid refactoring that modernizes the API layer.
api/controllers/console/datasets/rag_pipeline/rag_pipeline_workflow.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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 migrates the codebase from Flask-RESTx's reqparse to Pydantic BaseModel for request validation. The refactoring replaces imperative request parsing with declarative schema definitions, improving type safety and validation consistency across all API endpoints. The changes affect service APIs, console APIs, internal APIs, and test files.
Key Changes:
- Created a new
controllers/common/schema.pymodule with helper functions to register Pydantic models with Flask-RESTX namespaces - Replaced all
reqparse.RequestParser()instances with PydanticBaseModelclasses - Updated test files to remove
reqparsedependencies and use Pydantic validation directly - Modified service method signatures to accept optional parameters with default values
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
api/controllers/common/schema.py |
New helper module for registering Pydantic models with Flask-RESTX |
api/services/hit_testing_service.py |
Added default None values to optional parameters |
api/controllers/service_api/dataset/segment.py |
Migrated segment operations from reqparse to Pydantic models |
api/controllers/service_api/dataset/metadata.py |
Migrated metadata operations from reqparse to Pydantic models |
api/controllers/service_api/dataset/document.py |
Migrated document operations from reqparse to Pydantic models |
api/controllers/service_api/dataset/dataset.py |
Migrated dataset CRUD and tag operations from reqparse to Pydantic models |
api/controllers/service_api/app/workflow.py |
Migrated workflow operations from reqparse to Pydantic models |
api/controllers/console/datasets/datasets.py |
Migrated console dataset operations from reqparse to Pydantic models |
| Multiple test files | Updated tests to remove reqparse and use Pydantic validation |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
/gemini review |
There was a problem hiding this 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 is an excellent and extensive refactoring, migrating request parsing from reqparse to Pydantic BaseModel across a large number of API endpoints. This is a significant improvement for code quality, making the validation logic more declarative, readable, and type-safe. The implementation is very consistent and well-executed.
I've identified a few minor issues, primarily related to the nuances of parsing query parameters with Pydantic, such as handling list-like parameters and optional UUIDs that might be passed as empty strings. I've also suggested an improvement for error handling in a decorator to make it more robust. Other than these points, the refactoring is solid. Great work on this large-scale improvement!
There was a problem hiding this 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 44 out of 44 changed files in this pull request and generated no new comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
918fa84 to
b1a2282
Compare
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
b1a2282 to
11de635
Compare
Important
Fixes #<issue number>.Summary
fix #27513
Screenshots
Checklist
dev/reformat(backend) andcd web && npx lint-staged(frontend) to appease the lint gods