[Feature]: Add backend API for bulk file and folder operations #92#94
[Feature]: Add backend API for bulk file and folder operations #92#94IrfanSyed0721 wants to merge 3 commits into
Conversation
DenizAltunkapan
left a comment
DenizAltunkapan
left a comment
There was a problem hiding this comment.
Thanks for the work here @IrfanSyed0721. The direction is good, the per-item success/failure map with an inner try-catch is exactly the right pattern for bulk operations. There are a few things I would like fixed before this can be merged. I left inline notes on each one.
| fileService.renameOrMoveFile(user.getRootFolderPath(), filePath, newPath); | ||
| java.util.Map<String, String> results = new java.util.LinkedHashMap<>(); | ||
| for (String filePath : filePaths) { | ||
| try { |
There was a problem hiding this comment.
@IrfanSyed0721 This replaces the existing single-file @PatchMapping("/move") route instead of adding the bulk one next to it. The old rename/move endpoint is now gone, which will break the frontend that still calls it. Please keep /move and add /bulk as a separate method.
| } catch (Exception e) { | ||
| results.put(filePath, "FAILED: " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
@IrfanSyed0721 The catch returns e.getMessage() straight to the client. On this kind of storage service that can leak internal paths or details. Please map failures to a safe, generic message instead of exposing the raw exception text.
| Files.deleteIfExists(file); | ||
| } | ||
|
|
||
| public java.util.Map<String, String> deleteFiles( |
There was a problem hiding this comment.
@IrfanSyed0721 This new deleteFiles method is never called anywhere. The bulk delete controller runs its own loop over trashService.moveToTrash instead, so we end up with two different delete paths and this one is dead code. Either wire the controller to this service method or drop it.
| fileService.renameOrMoveFile( | ||
| user.getRootFolderPath(), | ||
| filePath, | ||
| targetPath + "/" + new java.io.File(filePath).getName()); |
There was a problem hiding this comment.
@IrfanSyed0721 The target path is built by string concatenation (targetPath + "/" + ...) and only gets validated deep inside renameOrMoveFile. Please build it with Paths and rely on the existing validatePath so traversal cannot slip through. Also catch (Exception e) is too broad here, it hides real bugs as "FAILED", so narrow it to the IO case.
| void contextLoads() {} | ||
|
|
||
| @org.junit.jupiter.api.Test | ||
| public void testBulkOperationsContract() { |
There was a problem hiding this comment.
@IrfanSyed0721 This test builds a map by hand and then asserts on the values it just put in, so no production code runs. It always passes even if the endpoints are broken. Please turn it into a real test that calls the controller or service with a temp folder and checks the actual result. Minor: prefer the imported names over fully qualified ones like java.util.Map and org.junit.jupiter.api.Test to match the rest of the file.
|
Thanks for the feedback, Deniz. I am currently traveling in India and won't have access to work on this for the next two weeks. I'll dive right into these fixes as soon as I get back! |
|
@IrfanSyed0721 okay no problem |
2 participants
Summary
Implemented the backend API contract for bulk file and folder operations to support multi-select functionality.
Linked issue
Closes #92