refactor(BA-4092): Refactor VFolder purge API to Service-Repository pattern #8334
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:M
fregataa
force-pushed
the
refactor/BA-4092-vfolder-purge-action
branch
from
4e6287c to
c78861d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the VFolder purge API to follow the Action-Service-Repository pattern, replacing direct database access in the API handler with a properly layered implementation.
Changes:
- Introduces
PurgeVFolderActionandPurgeVFolderActionResultclasses for the action layer - Adds
purge()method toVFolderServicethat delegates to the repository - Implements
purge_vfolder()inVfolderRepositorywith status validation using thePurgerpattern - Registers
purge_vfolderprocessor inVFolderProcessors - Updates API handler to use the processor instead of direct database operations
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/ai/backend/manager/services/vfolder/actions/base.py |
Adds PurgeVFolderAction and PurgeVFolderActionResult action classes with appropriate permission types |
src/ai/backend/manager/services/vfolder/services/vfolder.py |
Implements purge() method in VFolderService that delegates to repository |
src/ai/backend/manager/services/vfolder/processors/vfolder.py |
Registers purge_vfolder processor with action monitors |
src/ai/backend/manager/repositories/vfolder/repository.py |
Implements purge_vfolder() with status validation and execute_purger usage |
src/ai/backend/manager/api/vfolder.py |
Refactors API handler to use processor, removes direct DB access and unused imports |
tests/unit/manager/services/vfolder/test_vfolder_service.py |
Adds comprehensive service layer tests verifying delegation and exception propagation |
tests/unit/manager/repositories/vfolder/test_vfolder_repository.py |
Adds comprehensive repository layer tests for various status scenarios |
tests/unit/manager/services/vfolder/BUILD |
Adds BUILD file for test discovery |
changes/8317.misc.md |
Documents the refactoring change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
430
to
431
| if vfolder_data is None: | ||
| raise VFolderNotFound(extra_data=str(vfolder_uuid)) |
There was a problem hiding this comment.
This null check is unreachable because get_by_id on line 429 raises VFolderNotFound when the vfolder doesn't exist - it never returns None. The exception is already raised by get_by_id, so this check is redundant and can be removed.
Suggested change
| if vfolder_data is None: | |
| raise VFolderNotFound(extra_data=str(vfolder_uuid)) |
jopemachine
reviewed
Jan 27, 2026
jopemachine
reviewed
Jan 27, 2026
tests/unit/manager/repositories/vfolder/test_vfolder_repository.py
Outdated
Show resolved
Hide resolved
jopemachine
reviewed
Jan 27, 2026
tests/unit/manager/repositories/vfolder/test_vfolder_repository.py
Outdated
Show resolved
Hide resolved
jopemachine
reviewed
Jan 27, 2026
jopemachine
reviewed
Jan 27, 2026
jopemachine
reviewed
Jan 27, 2026
| ) | ||
| ) | ||
| except VFolderInvalidParameter as e: | ||
| raise InvalidAPIParameters(str(e)) |
Member
There was a problem hiding this comment.
I'm just curious, why do we need to convert VFolderInvalidParameter to InvalidAPIParameters?
Member
Author
There was a problem hiding this comment.
I assume that we keep InvalidAPIParameters for error message compatibility
Member
There was a problem hiding this comment.
I think the error message could be updated.
Both errors are same HTTP response code, so I think we don't this need this conversion
| ) | ||
| ) | ||
| except VFolderInvalidParameter as e: | ||
| raise InvalidAPIParameters(str(e)) |
Comment on lines
646
to
653
| vfolder_id = uuid.uuid4() | ||
| await self._create_vfolder_in_db( | ||
| db_with_cleanup, | ||
| vfolder_id=vfolder_id, | ||
| domain_name=test_domain_name, | ||
| user_id=test_user, | ||
| status=status, | ||
| ) |
Collaborator
There was a problem hiding this comment.
Please put anything related to this situation in the fixture.
Member
Author
There was a problem hiding this comment.
it has to use a parameterized input (status). Is there any example to setup fixtures with parameters?
…itory pattern - Add PurgeVFolderAction and PurgeVFolderActionResult classes - Add purge() method to VFolderService with status validation - Add purge_vfolder() method to VfolderRepository using execute_purger - Register purge_vfolder processor in VFolderProcessors - Update API handler to use processor instead of direct DB access
…tory Move VFolderNotFound and VFolderInvalidParameter validation from VFolderService.purge() to VfolderRepository.purge_vfolder() for cleaner separation of concerns. - Repository now validates vfolder existence and purgable status - Service layer simplified to delegate to repository - Added unit tests for both service and repository layers
Add test_purge_vfolder_invalid_status to verify VFolderInvalidParameter is raised when attempting to purge vfolders with non-purgable status (READY, PERFORMING, CLONING, MOUNTED, DELETE_ONGOING, DELETE_ERROR).
Clarify why get_by_id() is called before execute_purger() - it's needed to validate vfolder status since execute_purger() only deletes by PK without checking status.
- Remove @pytest.mark.asyncio decorators (auto-detected by pytest) - Move all imports to top of file - Convert _create_vfolder_data helper to sample_vfolder_data fixture
- Merge purgable/non_purgable fixtures into single vfolder_in_db - Use indirect parametrization to pass status to fixture - Return only vfolder_id from fixture (status not needed)
…atus - Replace VFolderInvalidParameter with VFolderFilterStatusFailed in repository purge_vfolder() for consistency with existing error types - Remove try-except in API handler since VFolderFilterStatusFailed already inherits from HTTPBadRequest - Update tests accordingly
fregataa
force-pushed
the
refactor/BA-4092-vfolder-purge-action
branch
from
269c3ca to
a1a1529
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.
resolves #8317 (BA-4092)
Changes
Test Coverage
Service Layer (
VFolderService.purge())test_purge_vfolder_successPurgeVFolderActionResulttest_purge_vfolder_not_found_propagatesVFolderNotFoundexception from repositorytest_purge_vfolder_invalid_status_propagatesVFolderInvalidParameterexception from repositoryRepository Layer (
VfolderRepository.purge_vfolder())test_purge_vfolder_success[delete_complete]DELETE_COMPLETEstatus, verifies deletion from DBtest_purge_vfolder_success[delete_pending]DELETE_PENDINGstatus, verifies deletion from DBtest_purge_vfolder_not_foundVFolderNotFoundwhen vfolder doesn't existtest_purge_vfolder_invalid_status[ready]VFolderInvalidParameterforREADYstatustest_purge_vfolder_invalid_status[performing]VFolderInvalidParameterforPERFORMINGstatustest_purge_vfolder_invalid_status[cloning]VFolderInvalidParameterforCLONINGstatustest_purge_vfolder_invalid_status[mounted]VFolderInvalidParameterforMOUNTEDstatustest_purge_vfolder_invalid_status[delete_ongoing]VFolderInvalidParameterforDELETE_ONGOINGstatustest_purge_vfolder_invalid_status[delete_error]VFolderInvalidParameterforDELETE_ERRORstatusChecklist: (if applicable)
ai.backend.testdocsdirectory