-
Notifications
You must be signed in to change notification settings - Fork 19.9k
refactor: port AppModelConfig #30919
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
refactor: port AppModelConfig #30919
Conversation
Summary of ChangesHello @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 focuses on modernizing the 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. 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
|
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 refactors the AppModelConfig model to use modern SQLAlchemy 2.0 syntax with Mapped and type hints, which is a great improvement for code clarity and maintainability. The corresponding changes in the test files to use the new dataclass-style constructor are also well-aligned with this refactoring. However, I've identified a few critical issues that need to be addressed. Firstly, in api/models/model.py, several columns in AppModelConfig are typed as nullable but are defined as non-nullable in the database mapping, which will lead to runtime errors. Secondly, in api/tests/test_containers_integration_tests/services/test_workflow_service.py, the tests are passing an unexpected tenant_id keyword argument when instantiating AppModelConfig, which will cause the tests to fail.
api/tests/test_containers_integration_tests/services/test_workflow_service.py
Outdated
Show resolved
Hide resolved
api/tests/test_containers_integration_tests/services/test_workflow_service.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 refactors the AppModelConfig model to use SQLAlchemy 2.0-style typed mappings by migrating from Base to TypeBase and adding Mapped type annotations to all columns. This is part of a larger effort to modernize the codebase's ORM usage.
Changes:
- Migrated
AppModelConfigfromBasetoTypeBasewith full type annotations - Updated test instantiation patterns from attribute assignment to constructor-based initialization
- Added explicit
default="simple"parameter toprompt_typefield
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| api/models/model.py | Converted AppModelConfig to TypeBase with Mapped type annotations for all columns |
| api/tests/test_containers_integration_tests/services/test_workflow_service.py | Refactored AppModelConfig instantiation in two test methods to use constructor pattern |
| api/tests/test_containers_integration_tests/services/test_app_dsl_service.py | Refactored AppModelConfig instantiation in test to use constructor pattern |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/tests/test_containers_integration_tests/services/test_workflow_service.py
Outdated
Show resolved
Hide resolved
api/tests/test_containers_integration_tests/services/test_workflow_service.py
Outdated
Show resolved
Hide resolved
|
/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 refactors the AppModelConfig to use SQLAlchemy's MappedAsDataclass, which is a great step towards more modern, type-safe code. The necessary changes to accommodate the new constructor have been correctly applied in most places. However, I found one critical issue where an AppModelConfig instance is created without a required app_id, which will lead to a runtime error. My review includes a specific suggestion to fix this.
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 refactors the AppModelConfig model to use modern SQLAlchemy 2.0 syntax with MappedAsDataclass. The changes correctly update the model definition and all instantiation sites to use the new dataclass-style __init__.
My main feedback is regarding a potential unintended schema change where many columns in AppModelConfig have been changed from non-nullable to nullable. Please see the specific comment for details. If this is intentional, it would be good to add a comment in the code or PR description explaining why. Otherwise, it should be fixed to avoid potential data integrity issues.
The rest of the changes in the service and test files are consistent with the model refactoring and look good.
Important
Fixes #<issue number>.Summary
part of #26788
Screenshots
Checklist
make lintandmake type-check(backend) andcd web && npx lint-staged(frontend) to appease the lint gods