refactor(BA-3687): consolidate admin_repository into repository for session domain #7868
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
size:L
MinJuTur
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates the AdminSessionRepository into the main SessionRepository by removing the separate admin repository class and moving role-based ownership validation logic from the repository layer to the service layer. This follows the same architectural pattern established in the Image domain consolidation (PR #7860).
Key changes:
- Removed the entire
AdminSessionRepositoryclass and file - Added
get_session_by_id_for_status()method toSessionRepositorythat fetches sessions without ownership checks - Updated
SessionService.check_and_transit_status()to handle role-based authorization directly in the service layer - Added comprehensive test coverage for all user roles (SUPERADMIN, ADMIN, USER) with authorization scenarios
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ai/backend/manager/repositories/session/admin_repository.py | Removed entire admin-specific repository class (76 lines deleted) |
| src/ai/backend/manager/repositories/session/repository.py | Added get_session_by_id_for_status() method for ownership-agnostic session fetching |
| src/ai/backend/manager/repositories/session/repositories.py | Removed admin_repository instantiation and references |
| src/ai/backend/manager/services/session/service.py | Refactored check_and_transit_status() to handle role-based authorization in service layer; removed admin_session_repository dependency |
| src/ai/backend/manager/services/processors.py | Removed admin_session_repository from SessionService initialization |
| src/ai/backend/manager/services/session/README.md | Updated documentation to reflect single repository pattern with service-layer access control |
| tests/unit/manager/services/session/test_session_service.py | Added comprehensive test class with 5 test cases covering all role-based authorization scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+110
to
+124
| @session_repository_resilience.apply() | ||
| async def get_session_by_id_for_status( | ||
| self, | ||
| session_id: SessionId, | ||
| ) -> SessionRow: | ||
| """ | ||
| Get session by ID for status determination without ownership checks. | ||
| Used by service layer which handles ownership validation based on user role. | ||
| """ | ||
| async with self._db.begin_readonly_session() as db_sess: | ||
| query_stmt = sa.select(SessionRow).where(SessionRow.id == session_id) | ||
| session_row = await db_sess.scalar(query_stmt) | ||
| if session_row is None: | ||
| raise SessionNotFound(f"Session not found (id:{session_id})") | ||
| return session_row |
There was a problem hiding this comment.
The new method get_session_by_id_for_status doesn't load the session's kernels relationship, but the service layer calls session_row.to_dataclass() which requires kernels to be loaded (it accesses self.main_kernel.service_ports). This will cause a lazy loading error or fail at runtime.
The method should include .options(selectinload(SessionRow.kernels)) in the query, similar to the existing get_session_to_determine_status method on line 1163 of the SessionRow model.
Comment on lines
1408
to
1423
| log.warning( | ||
| f"You are not allowed to transit others's sessions status, skip (s:{session_id})" |
There was a problem hiding this comment.
There's a grammatical error in this log message. The possessive form should use an apostrophe before the 's'.
jopemachine
reviewed
Jan 9, 2026
jopemachine
reviewed
Jan 9, 2026
jopemachine
reviewed
Jan 9, 2026
…ository - Remove AdminSessionRepository class and file - Add get_session_by_id_for_status() to SessionRepository (replaces get_session_to_determine_status_force) - Move ownership check logic from repository to service layer - Update SessionService.check_and_transit_status() to handle role-based authorization - Update SessionServiceArgs and SessionRepositories to remove admin_repository - Add comprehensive tests for check_and_transit_status with all user roles - Update README.md to reflect the simplified repository structure 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Remove "(service handles authorization)" from comment as it's already obvious within the session service context. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replace hardcoded UUIDs with uuid4() generated random UUIDs in session service tests for better test isolation. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Role-based access control is not part of the session service layer. Updated the SessionRepository description to accurately reflect its purpose. Co-Authored-By: Claude Opus 4.5 <[email protected]>
MinJuTur
force-pushed
the
refactor/BA-3687-session
branch
from
0ec5762 to
58cdd25
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
AdminSessionRepositoryclass and its fileget_session_by_id_for_status()toSessionRepository(replacesget_session_to_determine_status_force)SessionService.check_and_transit_status()to handle role-based authorizationcheck_and_transit_statuswith all user roles (SUPERADMIN, ADMIN, USER)Related Issue
Changes
AdminSessionRepository, addedget_session_by_id_for_status()toSessionRepositorySessionServicenow handles ownership validation based on user roleTestCheckAndTransitStatusclass with 5 test cases covering all permission scenariosTest plan
pants lint)pants check)pants test tests/unit/manager/services/session/test_session_service.py)🤖 Generated with Claude Code