-
Notifications
You must be signed in to change notification settings - Fork 18.5k
more typed orm #28519
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?
more typed orm #28519
Conversation
|
/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.
Pull request overview
This PR continues the migration of ORM models from Base to TypeBase (which uses MappedAsDataclass) to improve type safety. The changes involve converting several model classes and updating their instantiation patterns in tests.
Key changes:
- Migrated 7 models from
BasetoTypeBase:LoadBalancingModelConfig,ProviderCredential,ProviderModelCredential,TriggerOAuthTenantClient,OAuthProviderApp,MessageFile,AppAnnotationSetting,DatasetCollectionBinding, andDatasetAutoDisableLog - Added proper type annotations and
init=Falseto auto-generated fields (id, created_at, updated_at) - Updated test instantiation patterns to set auto-generated fields after object creation
- Removed custom
__init__method fromMessageFilemodel
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| api/models/provider.py | Migrated LoadBalancingModelConfig, ProviderCredential, and ProviderModelCredential to TypeBase with proper type annotations and init=False for auto-generated fields |
| api/models/trigger.py | Migrated TriggerOAuthTenantClient to TypeBase with updated field definitions |
| api/models/model.py | Migrated OAuthProviderApp, MessageFile, and AppAnnotationSetting to TypeBase, removed custom MessageFile.init and unused FileType import |
| api/models/dataset.py | Migrated DatasetCollectionBinding and DatasetAutoDisableLog to TypeBase with proper type annotations |
| api/tests/unit_tests/core/test_provider_manager.py | Updated LoadBalancingModelConfig instantiation to set id after creation instead of in constructor |
| api/tests/test_containers_integration_tests/tasks/test_add_document_to_index_task.py | Updated DatasetAutoDisableLog instantiation to set id after creation |
| api/tests/test_containers_integration_tests/services/test_annotation_service.py | Updated DatasetCollectionBinding and AppAnnotationSetting instantiation patterns to use constructor for required fields and set id afterwards |
| api/tests/test_containers_integration_tests/services/test_agent_service.py | Updated MessageFile instantiation to use constructor without custom init method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/tests/test_containers_integration_tests/services/test_agent_service.py
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.
Code Review
This pull request focuses on migrating several SQLAlchemy ORM models to use MappedAsDataclass for better type support. The changes are generally well-executed, introducing explicit Mapped types and init=False for database-managed fields. However, I've identified a few inconsistencies regarding constructor argument defaults. Some fields that were previously optional have become required, which could be a breaking change. My review includes suggestions to add Python-side defaults and reorder fields where necessary to maintain backward compatibility and improve the developer experience when instantiating these models.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/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 continues the migration of SQLAlchemy ORM models to a more modern, typed style using MappedAsDataclass. The changes are applied across several models in dataset.py, model.py, provider.py, and trigger.py, with corresponding test files updated to reflect the new dataclass-style initializers. This is a valuable refactoring that improves type safety and code clarity. I've identified a couple of instances where the change in model definition leads to a change in the constructor's signature, making previously optional arguments required. I've left specific comments with suggestions to address these potential breaking changes.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/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 great refactoring effort to modernize the SQLAlchemy models by using TypeBase and Mapped for better type safety. The changes are consistent across the models and the corresponding test files. I've found a couple of minor redundancies in the test code where IDs are being set manually for fields that have a default factory. Removing these manual assignments would make the tests cleaner and rely more on the ORM's features.
api/tests/test_containers_integration_tests/services/test_annotation_service.py
Show resolved
Hide resolved
api/tests/test_containers_integration_tests/tasks/test_add_document_to_index_task.py
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
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/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 does a great job of modernizing the SQLAlchemy ORM models by introducing more explicit typing with Mapped and migrating to a dataclass-based TypeBase. The changes are consistent across mãeodels and correctly apply init=False for database-generated columns and default/default_factory for others, which enhances the models' usability. The test files are also diligently updated to reflect these changes. I've identified a couple of minor opportunities to further improve consistency by adding Python-side defaults to match server_default values.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Important
Fixes #<issue number>.Summary
part of #23579
Screenshots
Checklist
dev/reformat(backend) andcd web && npx lint-staged(frontend) to appease the lint gods