Skip to content

Commit

Permalink
[MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate…
Browse files Browse the repository at this point in the history
… reason in case of failure
  • Loading branch information
peterdemaeyer authored and slawekjaranowski committed Feb 2, 2025
1 parent 3cc5d60 commit 282cf61
Show file tree
Hide file tree
Showing 3 changed files with 199 additions and 40 deletions.
6 changes: 6 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
93 changes: 53 additions & 40 deletions src/main/java/org/apache/maven/plugins/clean/Cleaner.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -111,14 +111,14 @@ public void log(CharSequence message, Throwable t) {
/**
* 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)
Expand Down Expand Up @@ -209,17 +209,17 @@ private boolean fastDelete(Path baseDir) {
/**
* 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,
Expand Down Expand Up @@ -291,13 +291,19 @@ private boolean isSymbolicLink(Path path) throws IOException {
|| (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) {
Expand All @@ -309,24 +315,22 @@ private int delete(Path file, boolean failOnError, boolean retryOnError) throws
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;
}
Expand All @@ -336,6 +340,15 @@ private int delete(Path file, boolean failOnError, boolean retryOnError) throws
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;
Expand Down Expand Up @@ -426,7 +439,7 @@ synchronized Path pollNext() {
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;
Expand Down
140 changes: 140 additions & 0 deletions src/test/java/org/apache/maven/plugins/clean/CleanerTest.java
Original file line number Diff line number Diff line change
@@ -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));
}
}

0 comments on commit 282cf61

Please sign in to comment.