-
Notifications
You must be signed in to change notification settings - Fork 164
refactor(BA-3117, BA-3118): Introduce source-based structure in user resource policy #6907
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
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 user resource policy repository to introduce a source-based architecture pattern by adding a DB Source layer that handles database operations. The repository layer now delegates to this DB source, improving separation of concerns and maintainability.
Key changes:
- Introduced
UserResourcePolicyDBSourceto encapsulate database operations - Refactored
UserResourcePolicyRepositoryto delegate to the new DB source - Updated tests from mocked database tests to real database integration tests in the repository layer
- Replaced generic
ObjectNotFoundexception with domain-specificUserResourcePolicyNotFound
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
src/ai/backend/manager/repositories/user_resource_policy/db_source/db_source.py |
New DB source class implementing database operations for user resource policies |
src/ai/backend/manager/repositories/user_resource_policy/repository.py |
Refactored to delegate CRUD operations to the DB source layer |
src/ai/backend/manager/services/user_resource_policy/service.py |
Simplified to pass creator/modifier objects directly to repository |
src/ai/backend/manager/models/resource_policy.py |
Added from_creator classmethod to construct row from creator object |
src/ai/backend/common/exception.py |
Added domain-specific UserResourcePolicyNotFound exception |
src/ai/backend/common/metrics/metric.py |
Added USER_RESOURCE_POLICY_DB_SOURCE layer type for metrics |
tests/manager/repositories/user_resource_policy/test_user_resource_policy_repository.py |
Migrated from mock-based tests to real database integration tests |
tests/manager/services/user_resource_policy/test_user_resource_policy.py |
Updated assertions to verify creator/modifier objects are passed correctly |
tests/manager/integration/services/user_resource_policy/* |
Removed redundant integration tests that overlap with new repository tests |
docs/manager/graphql-reference/*.graphql |
GraphQL schema changes (unrelated to this PR) |
changes/6907.enhance.md |
Changelog entry for this enhancement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
src/ai/backend/manager/repositories/user_resource_policy/db_source/db_source.py
Outdated
Show resolved
Hide resolved
tests/manager/repositories/user_resource_policy/test_user_resource_policy_repository.py
Show resolved
Hide resolved
72b3d58 to
0c71376
Compare
89a4d72 to
2f04a01
Compare
| class UserResourcePolicyDBSource: | ||
| """ | ||
| Database source for user resource policy operations. | ||
| Handles all database operations for user resource policies. | ||
| """ |
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.
Why is serializable applied here? It seems like applying Read Committed this time would be worthwhile, don't you think?
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.
It seems like applying it in 1.4 would be limited... Let's see after it goes over 2.0 for now.
src/ai/backend/manager/repositories/user_resource_policy/db_source/db_source.py
Show resolved
Hide resolved
src/ai/backend/manager/repositories/user_resource_policy/db_source/db_source.py
Outdated
Show resolved
Hide resolved
52dd389 to
adadc90
Compare
Co-authored-by: octodog <mu001@lablup.com>
resolves #6888, #6889 (BA-3117, BA-3118)
Apply the DB Source to the Repository within the User resource policy. The DB Source communicates with the actual database, fetching data that meets specified conditions and returning DTOs or Entities to the repository. This enables the Repository layer to combine various Sources without needing to know the detailed implementation of the DB, thereby returning data suitable for the Service layer.
This PR includes the introduction of the DB Source and the resulting modifications to the test code.
As the Repository's current role is not significant, tests communicating with the actual database are conducted within the Repository tests.
The existing integration tests overlap with the newly written tests and are less aligned with Backend.AI's test code writing patterns, so we intend to remove them.
Checklist: (if applicable)
ai.backend.testdocsdirectory📚 Documentation preview 📚: https://sorna--6907.org.readthedocs.build/en/6907/
📚 Documentation preview 📚: https://sorna-ko--6907.org.readthedocs.build/ko/6907/