[MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate…

… reason in case of failure (#60)

* MCLEAN-124 Leverage Files.delete(Path) API to provide more accurate reason in case of failure

* Use Mockito + fix unit tests for Windows

* Renamed variable error -> failure

* Removed the resetting of permissions in tests, relying on JUnit @tempdir to take care of permissions issues when clearing the temporary directory

* Fixed typo a -> an

* Simplified the setting of permissions in tests

pom.xml

@@ -108,6 +108,12 @@ under the License.
       <artifactId>junit-jupiter-api</artifactId>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>org.mockito</groupId>
+      <artifactId>mockito-core</artifactId>
+      <version>4.11.0</version>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>org.codehaus.plexus</groupId>
       <artifactId>plexus-testing</artifactId>

src/main/java/org/apache/maven/plugins/clean/Cleaner.java

@@ -273,7 +273,8 @@ private boolean isSymbolicLink(Path path) throws IOException {
      * @throws IOException If a file/directory could not be deleted and <code>failOnError</code> is <code>true</code>.
      */
     private int delete(File file, boolean failOnError, boolean retryOnError) throws IOException {
-        if (!file.delete()) {
+        IOException failure = delete(file);
+        if (failure != null) {
             boolean deleted = false;
 
             if (retryOnError) {
@@ -289,18 +290,18 @@ private int delete(File file, boolean failOnError, boolean retryOnError) throws
                     } catch (InterruptedException e) {
                         // ignore
                     }
-                    deleted = file.delete() || !file.exists();
+                    deleted = delete(file) == null || !file.exists();
                 }
             } else {
                 deleted = !file.exists();
             }
 
             if (!deleted) {
                 if (failOnError) {
-                    throw new IOException("Failed to delete " + file);
+                    throw new IOException("Failed to delete " + file, failure);
                 } else {
                     if (log.isWarnEnabled()) {
-                        log.warn("Failed to delete " + file);
+                        log.warn("Failed to delete " + file, failure);
                     }
                     return 1;
                 }
@@ -310,6 +311,15 @@ private int delete(File file, boolean failOnError, boolean retryOnError) throws
         return 0;
     }
 
+    private static IOException delete(File file) {
+        try {
+            Files.delete(file.toPath());
+        } catch (IOException e) {
+            return e;
+        }
+        return null;
+    }
+
     private static class Result {
 
         private int failures;

src/test/java/org/apache/maven/plugins/clean/CleanerTest.java

@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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.apache.maven.plugins.clean;
+
+import java.io.IOException;
+import java.nio.file.AccessDeniedException;
+import java.nio.file.DirectoryNotEmptyException;
+import java.nio.file.FileSystems;
+import java.nio.file.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.util.Set;
+
+import org.apache.maven.plugin.logging.Log;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.mockito.ArgumentCaptor;
+import org.mockito.InOrder;
+
+import static java.nio.file.Files.createDirectory;
+import static java.nio.file.Files.createFile;
+import static java.nio.file.Files.exists;
+import static java.nio.file.Files.setPosixFilePermissions;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.inOrder;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+class CleanerTest {
+
+    private static final boolean POSIX_COMPLIANT =
+            FileSystems.getDefault().supportedFileAttributeViews().contains("posix");
+
+    private final Log log = mock();
+
+    @Test
+    void deleteSucceedsDeeply(@TempDir Path tempDir) throws Exception {
+        assumeTrue(POSIX_COMPLIANT);
+        final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath();
+        final Path file = createFile(basedir.resolve("file"));
+        final Cleaner cleaner = new Cleaner(null, log, false, null, null);
+        cleaner.delete(basedir.toFile(), null, false, true, false);
+        assertFalse(exists(basedir));
+        assertFalse(exists(file));
+    }
+
+    @Test
+    void deleteFailsWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Exception {
+        assumeTrue(POSIX_COMPLIANT);
+        when(log.isWarnEnabled()).thenReturn(true);
+        final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath();
+        createFile(basedir.resolve("file"));
+        // Remove the executable flag to prevent directory listing, which will result in a DirectoryNotEmptyException.
+        final Set<PosixFilePermission> permissions = PosixFilePermissions.fromString("rw-rw-r--");
+        setPosixFilePermissions(basedir, permissions);
+        final Cleaner cleaner = new Cleaner(null, log, false, null, null);
+        final IOException exception =
+                assertThrows(IOException.class, () -> cleaner.delete(basedir.toFile(), null, false, true, false));
+        verify(log, never()).warn(any(CharSequence.class), any(Throwable.class));
+        assertEquals("Failed to delete " + basedir, exception.getMessage());
+        final DirectoryNotEmptyException cause =
+                assertInstanceOf(DirectoryNotEmptyException.class, exception.getCause());
+        assertEquals(basedir.toString(), cause.getMessage());
+    }
+
+    @Test
+    void deleteFailsAfterRetryWhenNoPermission(@TempDir Path tempDir) throws Exception {
+        assumeTrue(POSIX_COMPLIANT);
+        final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath();
+        createFile(basedir.resolve("file"));
+        // Remove the executable flag to prevent directory listing, which will result in a DirectoryNotEmptyException.
+        final Set<PosixFilePermission> permissions = PosixFilePermissions.fromString("rw-rw-r--");
+        setPosixFilePermissions(basedir, permissions);
+        final Cleaner cleaner = new Cleaner(null, log, false, null, null);
+        final IOException exception =
+                assertThrows(IOException.class, () -> cleaner.delete(basedir.toFile(), null, false, true, true));
+        assertEquals("Failed to delete " + basedir, exception.getMessage());
+        final DirectoryNotEmptyException cause =
+                assertInstanceOf(DirectoryNotEmptyException.class, exception.getCause());
+        assertEquals(basedir.toString(), cause.getMessage());
+    }
+
+    @Test
+    void deleteLogsWarningWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Exception {
+        assumeTrue(POSIX_COMPLIANT);
+        when(log.isWarnEnabled()).thenReturn(true);
+        final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath();
+        final Path file = createFile(basedir.resolve("file"));
+        // Remove the writable flag to prevent deletion of the file, which will result in an AccessDeniedException.
+        final Set<PosixFilePermission> permissions = PosixFilePermissions.fromString("r-xr-xr-x");
+        setPosixFilePermissions(basedir, permissions);
+        final Cleaner cleaner = new Cleaner(null, log, false, null, null);
+        assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false));
+        verify(log, times(2)).warn(any(CharSequence.class), any(Throwable.class));
+        InOrder inOrder = inOrder(log);
+        ArgumentCaptor<AccessDeniedException> cause1 = ArgumentCaptor.forClass(AccessDeniedException.class);
+        inOrder.verify(log).warn(eq("Failed to delete " + file), cause1.capture());
+        assertEquals(file.toString(), cause1.getValue().getMessage());
+        ArgumentCaptor<DirectoryNotEmptyException> cause2 = ArgumentCaptor.forClass(DirectoryNotEmptyException.class);
+        inOrder.verify(log).warn(eq("Failed to delete " + basedir), cause2.capture());
+        assertEquals(basedir.toString(), cause2.getValue().getMessage());
+    }
+
+    @Test
+    void deleteDoesNotLogAnythingWhenNoPermissionAndWarnDisabled(@TempDir Path tempDir) throws Exception {
+        assumeTrue(POSIX_COMPLIANT);
+        when(log.isWarnEnabled()).thenReturn(false);
+        final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath();
+        createFile(basedir.resolve("file"));
+        // Remove the writable flag to prevent deletion of the file, which will result in an AccessDeniedException.
+        final Set<PosixFilePermission> permissions = PosixFilePermissions.fromString("r-xr-xr-x");
+        setPosixFilePermissions(basedir, permissions);
+        final Cleaner cleaner = new Cleaner(null, log, false, null, null);
+        assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false));
+        verify(log, never()).warn(any(CharSequence.class), any(Throwable.class));
+    }
+}