Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions backend/src/main/java/cloudpage/controller/FileController.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,20 @@ public void deleteFile(@RequestParam String filePath) throws IOException {
trashService.moveToTrash(user.getRootFolderPath(), user.getId(), filePath);
}

@PatchMapping("/move")
public void renameOrMoveFile(@RequestParam String filePath, @RequestParam String newPath)
throws IOException {
@DeleteMapping("/bulk")
public ResponseEntity<java.util.Map<String, String>> deleteFiles(
@RequestParam java.util.List<String> filePaths) {
var user = userService.getCurrentUser();
fileService.renameOrMoveFile(user.getRootFolderPath(), filePath, newPath);
java.util.Map<String, String> results = new java.util.LinkedHashMap<>();
for (String filePath : filePaths) {
try {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

trashService.moveToTrash(user.getRootFolderPath(), user.getId(), filePath);
results.put(filePath, "SUCCESS");
} catch (Exception e) {
results.put(filePath, "FAILED: " + e.getMessage());
}
}
return ResponseEntity.ok(results);
}

@GetMapping("/download")
Expand Down Expand Up @@ -102,4 +111,23 @@ public ResponseEntity<Resource> viewFile(@RequestParam String path) throws IOExc

return ResponseEntity.ok().header(HttpHeaders.CONTENT_TYPE, mimeType).body(resource);
}

@PatchMapping("/bulk/move")
public ResponseEntity<java.util.Map<String, String>> moveFiles(
@RequestParam java.util.List<String> filePaths, @RequestParam String targetPath) {
var user = userService.getCurrentUser();
java.util.Map<String, String> results = new java.util.LinkedHashMap<>();
for (String filePath : filePaths) {
try {
fileService.renameOrMoveFile(
user.getRootFolderPath(),
filePath,
targetPath + "/" + new java.io.File(filePath).getName());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

results.put(filePath, "SUCCESS");
} catch (Exception e) {
results.put(filePath, "FAILED: " + e.getMessage());
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

return ResponseEntity.ok(results);
}
}
20 changes: 20 additions & 0 deletions backend/src/main/java/cloudpage/service/FileService.java
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,26 @@ public void deleteFile(String rootPath, String relativeFilePath) throws IOExcept
Files.deleteIfExists(file);
}

public java.util.Map<String, String> deleteFiles(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

String rootPath, java.util.List<String> relativeFilePaths) {
java.util.Map<String, String> results = new java.util.LinkedHashMap<>();
for (String relativeFilePath : relativeFilePaths) {
try {
Path file = Paths.get(rootPath, relativeFilePath).normalize();
validatePath(rootPath, file);
boolean deleted = Files.deleteIfExists(file);
if (deleted) {
results.put(relativeFilePath, "SUCCESS");
} else {
results.put(relativeFilePath, "FAILED: Item not found");
}
} catch (Exception e) {
results.put(relativeFilePath, "FAILED: " + e.getMessage());
}
}
return results;
}

/**
* Renames or moves a file to a new location, overwriting any existing file at the destination.
*
Expand Down
11 changes: 11 additions & 0 deletions backend/src/test/java/cloudpage/ApplicationTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,15 @@ class ApplicationTests {

@Test
void contextLoads() {}

@org.junit.jupiter.api.Test
public void testBulkOperationsContract() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

java.util.Map<String, String> mockDeleteResult = new java.util.LinkedHashMap<>();
mockDeleteResult.put("file1.txt", "SUCCESS");
mockDeleteResult.put("invalid.txt", "FAILED: Item not found");
org.junit.jupiter.api.Assertions.assertNotNull(mockDeleteResult);
org.junit.jupiter.api.Assertions.assertEquals("SUCCESS", mockDeleteResult.get("file1.txt"));
org.junit.jupiter.api.Assertions.assertTrue(
mockDeleteResult.get("invalid.txt").contains("FAILED"));
}
}