Redesign to match `find`

2 files changed, 100 insertions, 140 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileHelper.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileFinder.java
index d6faaf57262..57c643819dc 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileHelper.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileFinder.java
@@ -23,103 +23,63 @@ import java.util.stream.Stream;
 import static com.yahoo.vespa.hosted.node.admin.task.util.file.IOExceptionUtil.uncheck;
 
 /**
+ * Helper class to find and list or deleteRecursively files and directories. Follows the general syntax of command line
+ * tool `find`.
+ *
  * @author freva
  */
-public class FileHelper {
+public class FileFinder {
 
     private final Path basePath;
-    private Predicate<FileAttributes> fileFilter = attr -> false;
-    private Predicate<FileAttributes> directoryFilter = attr -> false;
-    private boolean includeBase = false;
-    private int maxDepth = 1;
+    private Predicate<FileAttributes> matcher;
+    private int maxDepth = Integer.MAX_VALUE;
 
-    public FileHelper(Path basePath) {
+    public FileFinder(Path basePath, Predicate<FileAttributes> initialMatcher) {
         this.basePath = basePath;
+        this.matcher = initialMatcher;
     }
 
-    /** Creates a FileHelper at the given basePath  */
-    public static FileHelper from(Path basePath) {
-        return new FileHelper(basePath);
+    /** Creates a FileFinder at the given basePath  */
+    public static FileFinder from(Path basePath) {
+        return new FileFinder(basePath, attrs -> true);
     }
 
-    /** Creates a FileHelper at give basePath that will match all files */
-    public static FileHelper streamFiles(Path basePath) {
-        return from(basePath).filterFile(attr -> true);
+    /** Creates a FileFinder at the given basePath that will match all files */
+    public static FileFinder files(Path basePath) {
+        return new FileFinder(basePath, FileAttributes::isRegularFile);
     }
 
 
-    /** Creates a FileHelper at give basePath that will match all directories */
-    public static FileHelper streamDirectories(Path basePath) {
-        return from(basePath).filterDirectory(attr -> true);
+    /** Creates a FileFinder at the given basePath that will match all directories */
+    public static FileFinder directories(Path basePath) {
+        return new FileFinder(basePath, FileAttributes::isDirectory);
     }
 
 
     /**
-     * Filter that will be used to match files under the base path. Files include everything that
-     * is not a directory (such as symbolic links)
-     */
-    public FileHelper filterFile(Predicate<FileAttributes> fileFilter) {
-        this.fileFilter = fileFilter;
-        return this;
-    }
-
-    /**
-     * Filter that will be used to match directories under the base path.
+     * Predicate that will be used to match files and directories under the base path.
      *
-     * NOTE: When a directory is matched, all of its sub-directories and files are also matched
+     * NOTE: Consequtive calls to this method are ANDed (this include the initial filter from
+     * {@link #files(Path)} or {@link #directories(Path)}.
      */
-    public FileHelper filterDirectory(Predicate<FileAttributes> directoryFilter) {
-        this.directoryFilter = directoryFilter;
+    public FileFinder match(Predicate<FileAttributes> matcher) {
+        this.matcher = this.matcher.and(matcher);
         return this;
     }
 
     /**
      * Maximum depth (relative to basePath) where contents should be matched with the given filters.
-     *
-     * Note: When using {@link #delete()}, elements beyond this depth will be deleted if they are inside
-     * a directory that matched before max depth. This behaves similarly to
-     * `find basePath -maxdepth maxDepth -exec rm -r "{}" \;`
+     * Default is unlimited.
      */
-    public FileHelper maxDepth(int maxDepth) {
+    public FileFinder maxDepth(int maxDepth) {
         this.maxDepth = maxDepth;
         return this;
     }
 
-    /** Whether the base path should also be considered (i.e. checked against the corresponding filter) */
-    public FileHelper includeBase(boolean includeBase) {
-        this.includeBase = includeBase;
-        return this;
-    }
-
-    /**
-     * Deletes all matching elements, ignores basePath
-     *
-     * @see #delete(boolean)
-     */
-    public int delete() {
-        return delete(false);
-    }
-
-    /**
-     * Deletes all matching elements
-     *
-     * @param deleteBase if true, will delete basePath aswell
-     * @return Number of deleted items
-     */
-    public int delete(boolean deleteBase) {
+    /** Recursively deletes all matching elements */
+    public int deleteRecursively() {
         int[] numDeletions = { 0 }; // :(
-        applyForEachToMatching(basePath,
-                deleteBase ? all() : fileFilter,
-                deleteBase ? all() : directoryFilter,
-                deleteBase ? 0 : maxDepth,
-                true,
-                deleteBase || includeBase,
-                attributes -> {
-                    if (deleteIfExists(attributes.path()))
-                        numDeletions[0]++;
-                }
-        );
-
+        forEach(attributes -> numDeletions[0] += deleteRecursively(attributes));
         return numDeletions[0];
     }
 
@@ -137,9 +97,9 @@ public class FileHelper {
         forEach(attributes -> action.accept(attributes.path()));
     }
 
-    /** Applies a given consumer to all the matching {@link FileHelper.FileAttributes} */
+    /** Applies a given consumer to all the matching {@link FileFinder.FileAttributes} */
     public void forEach(Consumer<FileAttributes> action) {
-        applyForEachToMatching(basePath, fileFilter, directoryFilter, maxDepth, false, includeBase, action);
+        applyForEachToMatching(basePath, matcher, maxDepth, action);
     }
     
 
@@ -148,14 +108,12 @@ public class FileHelper {
      * <em>depth-first</em>: The filter function is applied in pre-order (NLR), but the given
      * {@link Consumer} will be called in post-order (LRN).
      */
-    private void applyForEachToMatching(Path basePath, Predicate<FileAttributes> fileFilter, Predicate<FileAttributes> directoryFilter,
-                                        int maxMatchDepth, boolean matchAllInMatchingDirectories, boolean includeBase,
-                                        Consumer<FileAttributes> action) {
+    private void applyForEachToMatching(Path basePath, Predicate<FileAttributes> matcher,
+                                        int maxDepth, Consumer<FileAttributes> action) {
         try {
             // Only need to traverse as deep as we want to match, unless we want to match everything in directories
             // already matched
-            final int maxTraverseDepth = matchAllInMatchingDirectories ? Integer.MAX_VALUE : maxMatchDepth;
-            Files.walkFileTree(basePath, Collections.emptySet(), maxTraverseDepth, new SimpleFileVisitor<Path>() {
+            Files.walkFileTree(basePath, Collections.emptySet(), maxDepth, new SimpleFileVisitor<Path>() {
                 private final Stack<FileAttributes> matchingDirectoryStack = new Stack<>();
                 private int currentLevel = -1;
 
@@ -164,20 +122,7 @@ public class FileHelper {
                     currentLevel++;
 
                     FileAttributes attributes = new FileAttributes(dir, attrs);
-                    // If we are inside a directory that previously matched and we want to match anything
-                    // inside matching directories, add it to the matching stack and continue
-                    if (!matchingDirectoryStack.empty() && matchAllInMatchingDirectories) {
-                        matchingDirectoryStack.push(attributes);
-                        return FileVisitResult.CONTINUE;
-                    }
-
-                    boolean directoryMatches = directoryFilter.test(attributes);
-                    // If the directory does not match the filter and we are or beyond max matching depth, we can
-                    // skip the the entire subtree
-                    if (!directoryMatches && currentLevel >= maxMatchDepth)
-                        return FileVisitResult.SKIP_SUBTREE;
-
-                    if (directoryMatches)
+                    if (currentLevel > 0 && matcher.test(attributes))
                         matchingDirectoryStack.push(attributes);
 
                     return FileVisitResult.CONTINUE;
@@ -193,7 +138,7 @@ public class FileHelper {
                     }
 
                     FileAttributes attributes = new FileAttributes(file, attrs);
-                    if ((!matchingDirectoryStack.empty() && matchAllInMatchingDirectories) || fileFilter.test(attributes))
+                    if (matcher.test(attributes))
                         action.accept(attributes);
 
                     return FileVisitResult.CONTINUE;
@@ -201,11 +146,8 @@ public class FileHelper {
 
                 @Override
                 public FileVisitResult postVisitDirectory(Path dir, IOException exc) {
-                    if (!matchingDirectoryStack.isEmpty()) {
-                        FileAttributes attributes = matchingDirectoryStack.pop();
-                        if (currentLevel != 0 || includeBase)
-                            action.accept(attributes);
-                    }
+                    if (!matchingDirectoryStack.isEmpty())
+                        action.accept(matchingDirectoryStack.pop());
 
                     currentLevel--;
                     return FileVisitResult.CONTINUE;
@@ -218,6 +160,17 @@ public class FileHelper {
         }
     }
 
+    private static int deleteRecursively(FileAttributes fileAttributes) {
+        int numDeletions = 0;
+        if (fileAttributes.isDirectory()) {
+            numDeletions = FileFinder.from(fileAttributes.path())
+                    .match(all())
+                    .deleteRecursively();
+        }
+
+        return numDeletions + (deleteIfExists(fileAttributes.path()) ? 1 : 0);
+    }
+
 
     // Ideally, we would reuse the FileAttributes in this package, but unfortunately we only get
     // BasicFileAttributes and not PosixFileAttributes from FileVisitor
@@ -290,4 +243,9 @@ public class FileHelper {
     public static Path createDirectories(Path path) {
         return uncheck(() -> Files.createDirectories(path));
     }
+
+    public static int deleteRecursively(Path path) {
+        return deleteRecursively(
+                new FileAttributes(path, uncheck(() -> Files.readAttributes(path, BasicFileAttributes.class))));
+    }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileHelperTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileFinderTest.java
index f09ca9c18d3..a9d47f743e5 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileHelperTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/file/FileFinderTest.java
@@ -18,9 +18,7 @@ import java.nio.file.attribute.BasicFileAttributes;
 import java.nio.file.attribute.FileTime;
 import java.time.Duration;
 import java.time.Instant;
-import java.util.Arrays;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
@@ -39,7 +37,7 @@ import static org.mockito.Mockito.when;
  * @author freva
  */
 @RunWith(Enclosed.class)
-public class FileHelperTest {
+public class FileFinderTest {
 
     public static class GeneralLogicTests {
         @Rule
@@ -47,47 +45,43 @@ public class FileHelperTest {
 
         @Test
         public void all_files_non_recursive() {
-            assertFileHelper(FileHelper.from(testRoot())
-                            .filterFile(FileHelper.all()),
+            assertFileHelper(FileFinder.files(testRoot())
+                            .maxDepth(1),
 
                     of("file-1.json", "test.json", "test.txt"),
-                    of("test", "test/file.txt", "test/data.json", "test/subdir-1", "test/subdir-1/file", "test/subdir-2"));
+                    of("test", "test/file.txt", "test/data.json", "test/subdir-1", "test/subdir-1/test", "test/subdir-2"));
         }
 
         @Test
         public void all_files_recursive() {
-            assertFileHelper(FileHelper.from(testRoot())
-                            .filterFile(FileHelper.all())
-                            .maxDepth(3),
+            assertFileHelper(FileFinder.files(testRoot()),
 
-                    of("file-1.json", "test.json", "test.txt", "test/file.txt", "test/data.json", "test/subdir-1/file"),
+                    of("file-1.json", "test.json", "test.txt", "test/file.txt", "test/data.json", "test/subdir-1/test"),
                     of("test", "test/subdir-1", "test/subdir-2"));
         }
 
         @Test
         public void with_file_filter_recursive() {
-            assertFileHelper(FileHelper.from(testRoot())
-                            .filterFile(FileHelper.nameEndsWith(".json"))
-                            .maxDepth(3),
+            assertFileHelper(FileFinder.files(testRoot())
+                            .match(FileFinder.nameEndsWith(".json")),
 
                     of("file-1.json", "test.json", "test/data.json"),
-                    of("test.txt", "test", "test/file.txt", "test/subdir-1", "test/subdir-1/file", "test/subdir-2"));
+                    of("test.txt", "test", "test/file.txt", "test/subdir-1", "test/subdir-1/test", "test/subdir-2"));
         }
 
         @Test
         public void all_files_limited_depth() {
-            assertFileHelper(FileHelper.from(testRoot())
-                            .filterFile(FileHelper.all())
+            assertFileHelper(FileFinder.files(testRoot())
                             .maxDepth(2),
 
                     of("test.txt", "file-1.json", "test.json", "test/file.txt", "test/data.json"),
-                    of("test", "test/subdir-1", "test/subdir-1/file", "test/subdir-2"));
+                    of("test", "test/subdir-1", "test/subdir-1/test", "test/subdir-2"));
         }
 
         @Test
         public void directory_with_filter() {
-            assertFileHelper(FileHelper.from(testRoot())
-                            .filterDirectory(FileHelper.nameStartsWith("subdir"))
+            assertFileHelper(FileFinder.directories(testRoot())
+                            .match(FileFinder.nameStartsWith("subdir"))
                             .maxDepth(2),
 
                     of("test/subdir-1", "test/subdir-2"),
@@ -95,10 +89,18 @@ public class FileHelperTest {
         }
 
         @Test
+        public void match_file_and_directory_with_same_name() {
+            assertFileHelper(FileFinder.from(testRoot())
+                            .match(FileFinder.nameEndsWith("test")),
+
+                    of("test", "test/subdir-1/test"),
+                    of("file-1.json", "test.json", "test.txt"));
+        }
+
+        @Test
         public void all_contents() {
-            assertFileHelper(FileHelper.from(testRoot())
-                            .filterDirectory(FileHelper.all())
-                            .filterFile(FileHelper.all()),
+            assertFileHelper(FileFinder.from(testRoot())
+                            .maxDepth(1),
 
                     of("file-1.json", "test.json", "test.txt", "test"),
                     of());
@@ -108,7 +110,7 @@ public class FileHelperTest {
 
         @Test
         public void everything() {
-            FileHelper.from(testRoot()).delete(true);
+            FileFinder.deleteRecursively(testRoot());
 
             assertFalse(Files.exists(testRoot()));
         }
@@ -126,7 +128,7 @@ public class FileHelperTest {
             Files.createFile(root.resolve("test/data.json"));
 
             Files.createDirectories(root.resolve("test/subdir-1"));
-            Files.createFile(root.resolve("test/subdir-1/file"));
+            Files.createFile(root.resolve("test/subdir-1/test"));
 
             Files.createDirectories(root.resolve("test/subdir-2"));
         }
@@ -135,15 +137,15 @@ public class FileHelperTest {
             return folder.getRoot().toPath();
         }
 
-        private void assertFileHelper(FileHelper fileHelper, Set<String> expectedList, Set<String> expectedContentsAfterDelete) {
-            Set<String> actualList = fileHelper.stream()
-                    .map(FileHelper.FileAttributes::path)
+        private void assertFileHelper(FileFinder fileFinder, Set<String> expectedList, Set<String> expectedContentsAfterDelete) {
+            Set<String> actualList = fileFinder.stream()
+                    .map(FileFinder.FileAttributes::path)
                     .map(testRoot()::relativize)
                     .map(Path::toString)
                     .collect(Collectors.toSet());
             assertEquals(expectedList, actualList);
 
-            fileHelper.delete();
+            fileFinder.deleteRecursively();
             Set<String> actualContentsAfterDelete = recursivelyListContents(testRoot()).stream()
                     .map(testRoot()::relativize)
                     .map(Path::toString)
@@ -176,42 +178,42 @@ public class FileHelperTest {
         public void age_filter_test() {
             Path path = Paths.get("/my/fake/path");
             when(attributes.lastModifiedTime()).thenReturn(FileTime.from(Instant.now().minus(Duration.ofHours(1))));
-            FileHelper.FileAttributes fileAttributes = new FileHelper.FileAttributes(path, attributes);
+            FileFinder.FileAttributes fileAttributes = new FileFinder.FileAttributes(path, attributes);
 
-            assertFalse(FileHelper.olderThan(Duration.ofMinutes(61)).test(fileAttributes));
-            assertTrue(FileHelper.olderThan(Duration.ofMinutes(59)).test(fileAttributes));
+            assertFalse(FileFinder.olderThan(Duration.ofMinutes(61)).test(fileAttributes));
+            assertTrue(FileFinder.olderThan(Duration.ofMinutes(59)).test(fileAttributes));
 
-            assertTrue(FileHelper.youngerThan(Duration.ofMinutes(61)).test(fileAttributes));
-            assertFalse(FileHelper.youngerThan(Duration.ofMinutes(59)).test(fileAttributes));
+            assertTrue(FileFinder.youngerThan(Duration.ofMinutes(61)).test(fileAttributes));
+            assertFalse(FileFinder.youngerThan(Duration.ofMinutes(59)).test(fileAttributes));
         }
 
         @Test
         public void size_filters() {
             Path path = Paths.get("/my/fake/path");
             when(attributes.size()).thenReturn(100L);
-            FileHelper.FileAttributes fileAttributes = new FileHelper.FileAttributes(path, attributes);
+            FileFinder.FileAttributes fileAttributes = new FileFinder.FileAttributes(path, attributes);
 
-            assertFalse(FileHelper.largerThan(101).test(fileAttributes));
-            assertTrue(FileHelper.largerThan(99).test(fileAttributes));
+            assertFalse(FileFinder.largerThan(101).test(fileAttributes));
+            assertTrue(FileFinder.largerThan(99).test(fileAttributes));
 
-            assertTrue(FileHelper.smallerThan(101).test(fileAttributes));
-            assertFalse(FileHelper.smallerThan(99).test(fileAttributes));
+            assertTrue(FileFinder.smallerThan(101).test(fileAttributes));
+            assertFalse(FileFinder.smallerThan(99).test(fileAttributes));
         }
 
         @Test
         public void filename_filters() {
             Path path = Paths.get("/my/fake/path/some-12352-file.json");
-            FileHelper.FileAttributes fileAttributes = new FileHelper.FileAttributes(path, attributes);
+            FileFinder.FileAttributes fileAttributes = new FileFinder.FileAttributes(path, attributes);
 
-            assertTrue(FileHelper.nameStartsWith("some-").test(fileAttributes));
-            assertFalse(FileHelper.nameStartsWith("som-").test(fileAttributes));
+            assertTrue(FileFinder.nameStartsWith("some-").test(fileAttributes));
+            assertFalse(FileFinder.nameStartsWith("som-").test(fileAttributes));
 
-            assertTrue(FileHelper.nameEndsWith(".json").test(fileAttributes));
-            assertFalse(FileHelper.nameEndsWith("file").test(fileAttributes));
+            assertTrue(FileFinder.nameEndsWith(".json").test(fileAttributes));
+            assertFalse(FileFinder.nameEndsWith("file").test(fileAttributes));
 
-            assertTrue(FileHelper.nameMatches(Pattern.compile("some-[0-9]+-file.json")).test(fileAttributes));
-            assertTrue(FileHelper.nameMatches(Pattern.compile("^some-[0-9]+-file.json$")).test(fileAttributes));
-            assertFalse(FileHelper.nameMatches(Pattern.compile("some-[0-9]-file.json")).test(fileAttributes));
+            assertTrue(FileFinder.nameMatches(Pattern.compile("some-[0-9]+-file.json")).test(fileAttributes));
+            assertTrue(FileFinder.nameMatches(Pattern.compile("^some-[0-9]+-file.json$")).test(fileAttributes));
+            assertFalse(FileFinder.nameMatches(Pattern.compile("some-[0-9]-file.json")).test(fileAttributes));
         }
     }
 }