You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maven.apache.org by sj...@apache.org on 2025/02/02 20:31:53 UTC
(maven-clean-plugin) branch master updated: [MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate reason in case of failure
This is an automated email from the ASF dual-hosted git repository.
sjaranowski pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/maven-clean-plugin.git
The following commit(s) were added to refs/heads/master by this push:
new 282cf61 [MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate reason in case of failure
282cf61 is described below
commit 282cf6131d1c2c50bbb772d127df70c90e19ffc2
Author: Peter De Maeyer <pe...@gmail.com>
AuthorDate: Sat Nov 9 16:19:42 2024 +0100
[MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate reason in case of failure
---
pom.xml | 6 +
.../org/apache/maven/plugins/clean/Cleaner.java | 93 ++++++++------
.../apache/maven/plugins/clean/CleanerTest.java | 140 +++++++++++++++++++++
3 files changed, 199 insertions(+), 40 deletions(-)
diff --git a/pom.xml b/pom.xml
index adad51b..3dde41f 100644
--- a/pom.xml
+++ b/pom.xml
@@ -139,6 +139,12 @@ under the License.
<version>${guiceVersion}</version>
<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.slf4j</groupId>
<artifactId>slf4j-simple</artifactId>
diff --git a/src/main/java/org/apache/maven/plugins/clean/Cleaner.java b/src/main/java/org/apache/maven/plugins/clean/Cleaner.java
index 533a224..70d6832 100644
--- a/src/main/java/org/apache/maven/plugins/clean/Cleaner.java
+++ b/src/main/java/org/apache/maven/plugins/clean/Cleaner.java
@@ -74,11 +74,11 @@ class Cleaner {
/**
* Creates a new cleaner.
*
- * @param session The Maven session to be used.
- * @param log The logger to use, may be <code>null</code> to disable logging.
- * @param verbose Whether to perform verbose logging.
- * @param fastDir The explicit configured directory or to be deleted in fast mode.
- * @param fastMode The fast deletion mode.
+ * @param session the Maven session to be used
+ * @param log the logger to use, may be <code>null</code> to disable logging
+ * @param verbose whether to perform verbose logging
+ * @param fastDir the explicit configured directory or to be deleted in fast mode
+ * @param fastMode the fast deletion mode
*/
Cleaner(Session session, Log log, boolean verbose, Path fastDir, String fastMode) {
logDebug = (log == null || !log.isDebugEnabled()) ? null : logger(log::debug, log::debug);
@@ -111,14 +111,14 @@ class Cleaner {
/**
* Deletes the specified directories and its contents.
*
- * @param basedir The directory to delete, must not be <code>null</code>. Non-existing directories will be silently
- * ignored.
- * @param selector The selector used to determine what contents to delete, may be <code>null</code> to delete
- * everything.
- * @param followSymlinks Whether to follow symlinks.
- * @param failOnError Whether to abort with an exception in case a selected file/directory could not be deleted.
- * @param retryOnError Whether to undertake additional delete attempts in case the first attempt failed.
- * @throws IOException If a file/directory could not be deleted and <code>failOnError</code> is <code>true</code>.
+ * @param basedir the directory to delete, must not be <code>null</code>. Non-existing directories will be silently
+ * ignored
+ * @param selector the selector used to determine what contents to delete, may be <code>null</code> to delete
+ * everything
+ * @param followSymlinks whether to follow symlinks
+ * @param failOnError whether to abort with an exception in case a selected file/directory could not be deleted
+ * @param retryOnError whether to undertake additional delete attempts in case the first attempt failed
+ * @throws IOException if a file/directory could not be deleted and <code>failOnError</code> is <code>true</code>
*/
public void delete(
Path basedir, Selector selector, boolean followSymlinks, boolean failOnError, boolean retryOnError)
@@ -209,17 +209,17 @@ class Cleaner {
/**
* Deletes the specified file or directory.
*
- * @param file The file/directory to delete, must not be <code>null</code>. If <code>followSymlinks</code> is
- * <code>false</code>, it is assumed that the parent file is canonical.
- * @param pathname The relative pathname of the file, using {@link File#separatorChar}, must not be
- * <code>null</code>.
- * @param selector The selector used to determine what contents to delete, may be <code>null</code> to delete
- * everything.
- * @param followSymlinks Whether to follow symlinks.
- * @param failOnError Whether to abort with an exception in case a selected file/directory could not be deleted.
- * @param retryOnError Whether to undertake additional delete attempts in case the first attempt failed.
- * @return The result of the cleaning, never <code>null</code>.
- * @throws IOException If a file/directory could not be deleted and <code>failOnError</code> is <code>true</code>.
+ * @param file the file/directory to delete, must not be <code>null</code>. If <code>followSymlinks</code> is
+ * <code>false</code>, it is assumed that the parent file is canonical
+ * @param pathname the relative pathname of the file, using {@link File#separatorChar}, must not be
+ * <code>null</code>
+ * @param selector the selector used to determine what contents to delete, may be <code>null</code> to delete
+ * everything
+ * @param followSymlinks whether to follow symlinks
+ * @param failOnError whether to abort with an exception in case a selected file/directory could not be deleted
+ * @param retryOnError whether to undertake additional delete attempts in case the first attempt failed
+ * @return The result of the cleaning, never <code>null</code>
+ * @throws IOException if a file/directory could not be deleted and <code>failOnError</code> is <code>true</code>
*/
private Result delete(
Path file,
@@ -291,13 +291,19 @@ class Cleaner {
|| (attrs.isDirectory() && attrs.isOther());
}
+ /**
+ * Deletes the specified file or directory. If the path denotes a symlink, only the link is removed. Its target is
+ * left untouched.
+ *
+ * @param file the file/directory to delete, must not be <code>null</code>
+ * @param failOnError whether to abort with an exception if the file/directory could not be deleted
+ * @param retryOnError whether to undertake additional delete attempts if the first attempt failed
+ * @return <code>0</code> if the file was deleted, <code>1</code> otherwise
+ * @throws IOException if a file/directory could not be deleted and <code>failOnError</code> is <code>true</code>
+ */
private int delete(Path file, boolean failOnError, boolean retryOnError) throws IOException {
- try {
- Files.deleteIfExists(file);
- return 0;
- } catch (IOException e) {
- IOException exception = new IOException("Failed to delete " + file);
- exception.addSuppressed(e);
+ IOException failure = delete(file);
+ if (failure != null) {
if (retryOnError) {
if (ON_WINDOWS) {
@@ -309,24 +315,22 @@ class Cleaner {
for (int delay : delays) {
try {
Thread.sleep(delay);
- } catch (InterruptedException e2) {
- exception.addSuppressed(e2);
+ } catch (InterruptedException e) {
+ throw new IOException(e);
}
- try {
- Files.deleteIfExists(file);
- return 0;
- } catch (IOException e2) {
- exception.addSuppressed(e2);
+ failure = delete(file);
+ if (failure == null) {
+ break;
}
}
}
if (Files.exists(file)) {
if (failOnError) {
- throw new IOException("Failed to delete " + file, exception);
+ throw new IOException("Failed to delete " + file, failure);
} else {
if (logWarn != null) {
- logWarn.log("Failed to delete " + file, exception);
+ logWarn.log("Failed to delete " + file, failure);
}
return 1;
}
@@ -336,6 +340,15 @@ class Cleaner {
return 0;
}
+ private static IOException delete(Path file) {
+ try {
+ Files.deleteIfExists(file);
+ } catch (IOException e) {
+ return e;
+ }
+ return null;
+ }
+
private static class Result {
private int failures;
@@ -426,7 +439,7 @@ class Cleaner {
if (basedir == null) {
if (cleaner.session != null) {
SessionData data = cleaner.session.getData();
- Path lastDir = (Path) data.get(LAST_DIRECTORY_TO_DELETE);
+ Path lastDir = data.get(LAST_DIRECTORY_TO_DELETE);
if (lastDir != null) {
data.set(LAST_DIRECTORY_TO_DELETE, null);
return lastDir;
diff --git a/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java
new file mode 100644
index 0000000..798ee3a
--- /dev/null
+++ b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java
@@ -0,0 +1,140 @@
+/*
+ * 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.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
+import java.util.Set;
+
+import org.apache.maven.api.plugin.Log;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.condition.DisabledOnOs;
+import org.junit.jupiter.api.condition.OS;
+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.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 final Log log = mock(Log.class);
+
+ @Test
+ @DisabledOnOs(OS.WINDOWS)
+ void deleteSucceedsDeeply(@TempDir Path tempDir) throws Exception {
+ 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, null, false, true, false);
+ assertFalse(exists(basedir));
+ assertFalse(exists(file));
+ }
+
+ @Test
+ @DisabledOnOs(OS.WINDOWS)
+ void deleteFailsWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Exception {
+ 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, 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
+ @DisabledOnOs(OS.WINDOWS)
+ void deleteFailsAfterRetryWhenNoPermission(@TempDir Path tempDir) throws Exception {
+ 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, 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
+ @DisabledOnOs(OS.WINDOWS)
+ void deleteLogsWarningWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Exception {
+ 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, 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
+ @DisabledOnOs(OS.WINDOWS)
+ void deleteDoesNotLogAnythingWhenNoPermissionAndWarnDisabled(@TempDir Path tempDir) throws Exception {
+ 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, null, false, false, false));
+ verify(log, never()).warn(any(CharSequence.class), any(Throwable.class));
+ }
+}