diff --git a/src/main/java/org/commonjava/service/storage/controller/StorageController.java b/src/main/java/org/commonjava/service/storage/controller/StorageController.java index d51ada1..8291c6d 100644 --- a/src/main/java/org/commonjava/service/storage/controller/StorageController.java +++ b/src/main/java/org/commonjava/service/storage/controller/StorageController.java @@ -46,6 +46,10 @@ import static org.apache.commons.lang3.StringUtils.isNotBlank; import static org.commonjava.service.storage.util.Utils.getDuration; import static org.commonjava.service.storage.util.Utils.sort; +import static org.commonjava.service.storage.util.Utils.depth; +import static org.commonjava.service.storage.util.Utils.getParentPath; +import static org.commonjava.service.storage.util.Utils.getAllCandidates; +import static org.commonjava.service.storage.util.Utils.normalizeFolderPath; import static org.commonjava.storage.pathmapped.util.PathMapUtils.ROOT_DIR; @Startup @@ -269,6 +273,77 @@ public void purgeEmptyFilesystems() ret.forEach( filesystem -> fileManager.purgeFilesystem( filesystem )); } + /** + * Cleans up (deletes) the given empty folders and, if possible, their parent folders up to the root. + *

+ * Optimization details: + *

+ * This approach avoids redundant deletion attempts and is efficient for overlapping directory trees. + */ + public BatchDeleteResult cleanupEmptyFolders(String filesystem, Collection paths) { + Set allCandidates = getAllCandidates(paths); + // Sort by depth, deepest first + List sortedCandidates = new ArrayList<>(allCandidates); + sortedCandidates.sort((a, b) -> Integer.compare(depth(b), depth(a))); + logger.debug("Sorted candidate folders for cleanup (deepest first): {}", sortedCandidates); + + Set succeeded = new HashSet<>(); + Set failed = new HashSet<>(); + Set processed = new HashSet<>(); + for (String folder : sortedCandidates) { + if (processed.contains(folder)) { + continue; + } + processed.add(folder); + try { + if (!fileManager.isDirectory(filesystem, folder)) { + logger.debug("Path is not a directory or does not exist, skipping: {} in filesystem: {}", folder, filesystem); + continue; + } + String[] contents = fileManager.list(filesystem, folder); + if (contents == null || contents.length == 0) { + boolean deleted = fileManager.delete(filesystem, normalizeFolderPath(folder)); + if (deleted) { + succeeded.add(folder); + logger.debug("Folder deleted: {} in filesystem: {}", folder, filesystem); + } else { + failed.add(folder); + } + } else { + logger.debug("Folder not empty, skipping cleanup: {} in filesystem: {}, contents: {}", folder, filesystem, Arrays.toString(contents)); + // Mark this folder and all its ancestors as processed + markAncestorsProcessed(folder, processed); + } + } catch (Exception e) { + logger.warn("Failed to clean up folder: {} in filesystem: {}. Error: {}", folder, filesystem, e.getMessage()); + failed.add(folder); + markAncestorsProcessed(folder, processed); + } + } + logger.info("Cleanup empty folders result: succeeded={}, failed={}", succeeded, failed); + BatchDeleteResult result = new BatchDeleteResult(); + result.setFilesystem(filesystem); + result.setSucceeded(succeeded); + result.setFailed(failed); + return result; + } + + // Mark the given folder and all its ancestors as processed + private void markAncestorsProcessed(String folder, Set processed) { + String current = folder; + while (current != null && !current.isEmpty() && !current.equals("/")) { + processed.add(current); + current = getParentPath(current); + } + } + /** * By default, the pathmap storage will override existing paths. Here we must check: * 1. all paths exist in source diff --git a/src/main/java/org/commonjava/service/storage/jaxrs/StorageMaintResource.java b/src/main/java/org/commonjava/service/storage/jaxrs/StorageMaintResource.java index 55f2359..63a0900 100644 --- a/src/main/java/org/commonjava/service/storage/jaxrs/StorageMaintResource.java +++ b/src/main/java/org/commonjava/service/storage/jaxrs/StorageMaintResource.java @@ -20,6 +20,7 @@ import org.commonjava.service.storage.dto.BatchDeleteResult; import org.commonjava.service.storage.util.ResponseHelper; import org.commonjava.storage.pathmapped.model.Filesystem; +import org.commonjava.service.storage.dto.BatchDeleteRequest; import org.eclipse.microprofile.openapi.annotations.Operation; import org.eclipse.microprofile.openapi.annotations.responses.APIResponse; import org.eclipse.microprofile.openapi.annotations.responses.APIResponses; @@ -94,4 +95,26 @@ public Response purgeFilesystem( final @PathParam( "filesystem" ) String filesys logger.debug( "Purge filesystem result: {}", result ); return responseHelper.formatOkResponseWithJsonEntity( result ); } + + /** + * Cleans up multiple empty folders in a filesystem. + * + * @param request BatchDeleteRequest containing the filesystem and the set of folder paths to attempt to clean up. + * Only empty folders will be deleted; non-empty folders will be skipped. + * If a parent folder becomes empty as a result, it will also be deleted recursively up to the root. + */ + @Operation( summary = "Cleanup multiple empty folders in a filesystem. Always returns 200 OK for easier client handling; " + + "failures are reported in the result object." ) + @APIResponses( { @APIResponse( responseCode = "200", + description = "Cleanup done (some folders may have failed, see result object)." ) } ) + @Consumes( APPLICATION_JSON ) + @Produces( APPLICATION_JSON ) + @DELETE + @Path( "folders/empty" ) + public Response cleanupEmptyFolders( BatchDeleteRequest request ) + { + logger.info( "Cleanup empty folders: filesystem={}, paths={}", request.getFilesystem(), request.getPaths() ); + BatchDeleteResult result = controller.cleanupEmptyFolders( request.getFilesystem(), request.getPaths() ); + return Response.ok(result).build(); + } } diff --git a/src/main/java/org/commonjava/service/storage/util/Utils.java b/src/main/java/org/commonjava/service/storage/util/Utils.java index 1b32200..979f09c 100644 --- a/src/main/java/org/commonjava/service/storage/util/Utils.java +++ b/src/main/java/org/commonjava/service/storage/util/Utils.java @@ -19,6 +19,9 @@ import java.util.Collections; import java.util.LinkedList; import java.util.List; +import java.util.Set; +import java.util.HashSet; +import java.util.Collection; public class Utils { public static Duration getDuration( String timeout ) @@ -59,4 +62,68 @@ public static String[] sort(String[] list) { Collections.sort(ret); return ret.toArray(new String[0]); } + + /** + * Returns the depth (number of slashes) in the path, ignoring root. + */ + public static int depth(String path) { + if (path == null || path.isEmpty() || path.equals("/")) { + return 0; + } + String normalized = path.endsWith("/") && path.length() > 1 ? path.substring(0, path.length() - 1) : path; + int depth = 0; + for (char c : normalized.toCharArray()) { + if (c == '/') depth++; + } + return depth; + } + + /** + * Returns the parent path of a given path, or null if at root. + */ + public static String getParentPath(String path) { + if (path == null || path.isEmpty() || path.equals("/")) { + return null; + } + String normalized = path.endsWith("/") && path.length() > 1 ? path.substring(0, path.length() - 1) : path; + int lastSlashIndex = normalized.lastIndexOf('/'); + if (lastSlashIndex == -1) { + return null; + } + return normalized.substring(0, lastSlashIndex); + } + + /** + * Given a collection of folder paths, returns a set of all those folders and their ancestors, + * up to but not including root. + */ + public static Set getAllCandidates(Collection paths) { + Set allCandidates = new HashSet<>(); + if (paths != null) { + for (String path : paths) { + String current = path; + while (current != null && !current.isEmpty() && !current.equals("/")) { + allCandidates.add(current); + current = getParentPath(current); + } + } + } + return allCandidates; + } + + /** + * Normalize a folder path for deletion: ensures a trailing slash (except for root). + * This helps avoid issues with path handling in the underlying storage system. + */ + public static String normalizeFolderPath(String path) { + if (path == null || path.isEmpty() || "/".equals(path)) { + return "/"; + } + // Add trailing slashes + String normalized = path; + while (!normalized.endsWith("/")) { + normalized = path + "/"; + } + return normalized; + } } diff --git a/src/test/java/org/commonjava/service/storage/StorageControllerIT.java b/src/test/java/org/commonjava/service/storage/StorageControllerIT.java index bda2818..8c5eac5 100644 --- a/src/test/java/org/commonjava/service/storage/StorageControllerIT.java +++ b/src/test/java/org/commonjava/service/storage/StorageControllerIT.java @@ -19,6 +19,7 @@ import org.commonjava.service.storage.controller.StorageController; import org.commonjava.service.storage.dto.BatchCleanupResult; import org.commonjava.service.storage.dto.FileInfoObj; +import org.commonjava.service.storage.dto.BatchDeleteResult; import org.junit.jupiter.api.Test; import jakarta.inject.Inject; @@ -27,6 +28,7 @@ import java.util.Collection; import java.util.HashSet; import java.util.Set; +import java.io.OutputStream; import static org.junit.jupiter.api.Assertions.*; @@ -80,4 +82,86 @@ public void testCleanup() assertNull( result ); } + /** + * Test cleanupEmptyFolders for: + * - Deleting empty folders (should succeed) + * - Not deleting non-empty folders (should remain) + * - Recursive parent cleanup (parents deleted if they become empty) + * - Overlapping folder trees (shared parents handled efficiently) + * - Correct reporting in BatchDeleteResult (succeeded/failed) + * - Actual state of the storage after cleanup + */ + @Test + public void testCleanupEmptyFolders_recursiveAndOverlapping() throws Exception { + // Setup: create a nested directory structure + // Structure: + // root/ + // a/ + // b/ (empty) + // c/ (contains file) + // d/ (empty) + // e/f/g/ (empty) + String root = "test-root"; + String a = root + "/a"; + String b = a + "/b"; + String c = a + "/c"; + String d = root + "/d"; + String e = root + "/e"; + String f = e + "/f"; + String g = f + "/g"; + String fileInC = c + "/file.txt"; + + // Create directories (by writing and deleting a dummy file) + createEmptyDirectory(filesystem, b); + createEmptyDirectory(filesystem, c); + createEmptyDirectory(filesystem, d); + createEmptyDirectory(filesystem, g); + // Add a file to c (so c and a are not empty) + try (OutputStream out = fileManager.openOutputStream(filesystem, fileInC)) { + out.write("data".getBytes()); + } + + // Sanity check: all directories exist + assertTrue(fileManager.isDirectory(filesystem, b)); + assertTrue(fileManager.isDirectory(filesystem, c)); + assertTrue(fileManager.isDirectory(filesystem, d)); + assertTrue(fileManager.isDirectory(filesystem, g)); + assertTrue(fileManager.isDirectory(filesystem, f)); + assertTrue(fileManager.isDirectory(filesystem, e)); + assertTrue(fileManager.isDirectory(filesystem, a)); + assertTrue(fileManager.isDirectory(filesystem, root)); + + // Call cleanupEmptyFolders on [b, d, g] + Set toCleanup = new HashSet<>(); + toCleanup.add(b); // should delete b, then a (if a becomes empty) + toCleanup.add(d); // should delete d + toCleanup.add(g); // should delete g, f, e (if they become empty) + BatchDeleteResult result = controller.cleanupEmptyFolders(filesystem, toCleanup); + + // Check results + // b, d, g, f, e should be deleted (a and root remain because c is not empty) + assertTrue(result.getSucceeded().contains(b)); + assertTrue(result.getSucceeded().contains(d)); + assertTrue(result.getSucceeded().contains(g)); + assertTrue(result.getSucceeded().contains(f)); + assertTrue(result.getSucceeded().contains(e)); + // a, c, root should not be deleted + assertFalse(result.getSucceeded().contains(a)); + assertFalse(result.getSucceeded().contains(c)); + assertFalse(result.getSucceeded().contains(root)); + // No failures expected + assertTrue(result.getFailed().isEmpty()); + // Check actual state + assertFalse(fileManager.isDirectory(filesystem, b)); + assertFalse(fileManager.isDirectory(filesystem, d)); + assertFalse(fileManager.isDirectory(filesystem, g)); + assertFalse(fileManager.isDirectory(filesystem, f)); + assertFalse(fileManager.isDirectory(filesystem, e)); + assertTrue(fileManager.isDirectory(filesystem, a)); + assertTrue(fileManager.isDirectory(filesystem, c)); + assertTrue(fileManager.isDirectory(filesystem, root)); + // File in c should still exist + assertTrue(fileManager.exists(filesystem, fileInC)); + } + } diff --git a/src/test/java/org/commonjava/service/storage/StorageIT.java b/src/test/java/org/commonjava/service/storage/StorageIT.java index b8ecd6d..79b9ea5 100644 --- a/src/test/java/org/commonjava/service/storage/StorageIT.java +++ b/src/test/java/org/commonjava/service/storage/StorageIT.java @@ -85,6 +85,14 @@ protected void prepareFile( InputStream in, String filesystem, String path ) thr } } + protected void createEmptyDirectory(String filesystem, String dir) throws Exception { + String dummyFile = dir + "/.keep"; + try (OutputStream out = fileManager.openOutputStream(filesystem, dummyFile)) { + out.write(0); + } + fileManager.delete(filesystem, dummyFile); + } + /** * Override this if your test case don't need to prepare the PATH * @return diff --git a/src/test/java/org/commonjava/service/storage/StorageMaintResourceIT.java b/src/test/java/org/commonjava/service/storage/StorageMaintResourceIT.java new file mode 100644 index 0000000..49d78b9 --- /dev/null +++ b/src/test/java/org/commonjava/service/storage/StorageMaintResourceIT.java @@ -0,0 +1,106 @@ +/* + * Copyright (C) 2021 Red Hat, Inc. (https://github.com/Commonjava/service-parent) + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.commonjava.service.storage; + +import io.quarkus.test.junit.QuarkusTest; +import org.commonjava.service.storage.dto.BatchDeleteRequest; +import org.commonjava.service.storage.dto.BatchDeleteResult; +import org.junit.jupiter.api.Test; + +import jakarta.ws.rs.core.MediaType; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.*; +import static io.restassured.RestAssured.given; + +@QuarkusTest +public class StorageMaintResourceIT extends StorageIT { + + @Override + protected boolean isPrepareFile() { + return false; + } + + /** + * Integration test for the cleanupEmptyFolders REST endpoint. + *

+ * Folder structure used in this test: + * 

+     * test-root-http/
+     * ├── a/
+     * │   ├── b/   (not empty, contains file.txt)
+     * │   └── file.txt (so a is not empty)
+     * └── d/     (empty)
+     *
+ *
    + *
  • Sets up a nested directory structure with both empty and non-empty folders.
    • + *
  • Sends a DELETE request to /api/storage/maint/folders/empty with a batch of folders.
    • + *
  • Asserts that only empty folders are deleted and non-empty folders remain.
    • + *
  • Verifies the HTTP response and the BatchDeleteResult content.
    • + *
+ */ + @Test + public void testCleanupEmptyFolders_HttpLevel() { + // Setup: create a nested directory structure as in the controller test + String root = "test-root-http"; + String a = root + "/a"; + String b = a + "/b"; + String d = root + "/d"; + String fileInB = b + "/file.txt"; + String fileInA = a + "/file.txt"; + + // Create directories (by writing and deleting a dummy file) + try { + createEmptyDirectory(filesystem, b); + createEmptyDirectory(filesystem, d); + // Add a file to b (so b is not empty) + try (java.io.OutputStream out = fileManager.openOutputStream(filesystem, fileInB)) { + out.write("data".getBytes()); + } + // Add a file to a (so a is not empty) + try (java.io.OutputStream out = fileManager.openOutputStream(filesystem, fileInA)) { + out.write("data".getBytes()); + } + } catch (Exception e) { + throw new RuntimeException(e); + } + + // Prepare request: try to clean up b and d + BatchDeleteRequest request = new BatchDeleteRequest(); + request.setFilesystem(filesystem); + Set paths = new HashSet<>(Arrays.asList(b, d)); + request.setPaths(paths); + + // Send DELETE request to the API + BatchDeleteResult result = + given() + .contentType(MediaType.APPLICATION_JSON) + .body(request) + .when() + .delete("/api/storage/maint/folders/empty") + .then() + .statusCode(200) + .extract().as(BatchDeleteResult.class); + + // d should be deleted (empty), b should not (not empty) + assertThat(result.getSucceeded(), hasItem(d)); + assertThat(result.getSucceeded(), not(hasItem(b))); + assertThat(result.getFailed(), not(hasItem(d))); + } +} \ No newline at end of file diff --git a/src/test/java/org/commonjava/service/storage/StorageResourceIT.java b/src/test/java/org/commonjava/service/storage/StorageResourceIT.java index 81b6ac1..02eb247 100644 --- a/src/test/java/org/commonjava/service/storage/StorageResourceIT.java +++ b/src/test/java/org/commonjava/service/storage/StorageResourceIT.java @@ -28,7 +28,6 @@ import org.junit.jupiter.api.Test; import java.io.ByteArrayInputStream; -import java.io.IOException; import java.io.InputStream; import java.util.Arrays; import java.util.HashSet; diff --git a/src/test/java/org/commonjava/service/storage/util/UtilsTest.java b/src/test/java/org/commonjava/service/storage/util/UtilsTest.java index e209936..9a7a83b 100644 --- a/src/test/java/org/commonjava/service/storage/util/UtilsTest.java +++ b/src/test/java/org/commonjava/service/storage/util/UtilsTest.java @@ -20,7 +20,11 @@ import java.time.Duration; import static org.commonjava.service.storage.util.Utils.getDuration; +import static org.commonjava.service.storage.util.Utils.getAllCandidates; import static org.junit.jupiter.api.Assertions.assertEquals; +import java.util.Set; +import java.util.HashSet; +import java.util.Arrays; public class UtilsTest { @@ -40,4 +44,19 @@ public void getDurationTest() assertEquals( d1, d2 ); } + /* + * Test getAllCandidates for: + * - Collecting all input folders and their ancestors + * - Excluding root + */ + @Test + public void testGetAllCandidates() { + Set input = new HashSet<>(Arrays.asList("a/b/c", "a/d")); + Set expected = new HashSet<>(Arrays.asList( + "a/b/c", "a/b", "a", "a/d" + )); + Set result = getAllCandidates(input); + assertEquals(expected, result); + } + }