From 9aa8ef35d805df0e2c0b635c62c31c0eb5fa2579 Mon Sep 17 00:00:00 2001
From: Elliotte Rusty Harold
Date: Thu, 21 Dec 2023 17:17:21 -0500
Subject: [PATCH] [IO-808] remove redundant checks; improve Javadoc (#533)
* remove redundant checks; improve Javadoc
* remove duplicate check
* update tests
* print failure message
* combin if statements to make PMD happy
* restore nested IOException
* remove unnecessary string concatenation
* fill in missing @throws tags
---
.../java/org/apache/commons/io/FileUtils.java | 121 ++++++++----------
.../commons/io/function/IOConsumer.java | 2 +-
...FileUtilsCopyDirectoryToDirectoryTest.java | 2 +-
.../io/FileUtilsDeleteDirectoryLinuxTest.java | 3 +-
4 files changed, 60 insertions(+), 68 deletions(-)
diff --git a/src/main/java/org/apache/commons/io/FileUtils.java b/src/main/java/org/apache/commons/io/FileUtils.java
index 363ab69f4b0..0c315204fec 100644
--- a/src/main/java/org/apache/commons/io/FileUtils.java
+++ b/src/main/java/org/apache/commons/io/FileUtils.java
@@ -293,6 +293,7 @@ public static String byteCountToDisplaySize(final Number size) {
* @throws NullPointerException if the given {@link File} is {@code null}.
* @throws NullPointerException if the given {@link Checksum} is {@code null}.
* @throws IllegalArgumentException if the given {@link File} is not a file.
+ * @throws FileNotFoundException if the file does not exist
* @throws IOException if an IO error occurs reading the file.
* @since 1.3
*/
@@ -377,8 +378,12 @@ public static boolean contentEquals(final File file1, final File file2) throws I
return true;
}
- checkFileExists(file1, "file1");
- checkFileExists(file2, "file2");
+ if (!file1.isFile()) {
+ throw new IllegalArgumentException("Parameter 'file1' is not a file: " + file1);
+ }
+ if (!file2.isFile()) {
+ throw new IllegalArgumentException("Parameter 'file2' is not a file: " + file2);
+ }
if (file1.length() != file2.length()) {
// lengths differ, cannot be equal
@@ -663,7 +668,7 @@ public static void copyDirectory(final File srcDir, final File destDir, final Fi
*/
public static void copyDirectory(final File srcDir, final File destDir, final FileFilter fileFilter, final boolean preserveFileDate,
final CopyOption... copyOptions) throws IOException {
- requireFileCopy(srcDir, destDir);
+ Objects.requireNonNull(destDir, "destination");
requireDirectoryExists(srcDir, "srcDir");
requireCanonicalPathsNotEquals(srcDir, destDir);
@@ -709,7 +714,7 @@ public static void copyDirectory(final File srcDir, final File destDir, final Fi
* @since 1.2
*/
public static void copyDirectoryToDirectory(final File sourceDir, final File destinationDir) throws IOException {
- requireDirectoryIfExists(sourceDir, "sourceDir");
+ Objects.requireNonNull(sourceDir, "sourceDir");
requireDirectoryIfExists(destinationDir, "destinationDir");
copyDirectory(sourceDir, new File(destinationDir, sourceDir.getName()), true);
}
@@ -795,11 +800,10 @@ public static void copyFile(final File srcFile, final File destFile, final boole
* @since 2.8.0
*/
public static void copyFile(final File srcFile, final File destFile, final boolean preserveFileDate, final CopyOption... copyOptions) throws IOException {
- requireFileCopy(srcFile, destFile);
+ Objects.requireNonNull(destFile, "destination");
checkFileExists(srcFile, "srcFile");
requireCanonicalPathsNotEquals(srcFile, destFile);
createParentDirectories(destFile);
- Objects.requireNonNull(destFile, "destFile");
if (destFile.exists()) {
checkFileExists(destFile, "destFile");
requireCanWrite(destFile, "destFile");
@@ -1249,7 +1253,7 @@ public static boolean deleteQuietly(final File file) {
*
* Edge cases:
*
- * - A {@code directory} must not be null: if null, throw IllegalArgumentException
+ * - A {@code directory} must not be null: if null, throw NullPointerException
* - A {@code directory} must be a directory: if not a directory, throw IllegalArgumentException
* - A directory does not contain itself: return false
* - A null child file is not contained in any parent: return false
@@ -1267,7 +1271,7 @@ public static boolean deleteQuietly(final File file) {
public static boolean directoryContains(final File directory, final File child) throws IOException {
requireDirectoryExists(directory, "directory");
- if (child == null || !directory.exists() || !child.exists()) {
+ if (child == null || !child.exists()) {
return false;
}
@@ -1328,14 +1332,15 @@ private static void doCopyDirectory(final File srcDir, final File destDir, final
*/
public static void forceDelete(final File file) throws IOException {
Objects.requireNonNull(file, "file");
+
final Counters.PathCounters deleteCounters;
try {
- deleteCounters = PathUtils.delete(file.toPath(), PathUtils.EMPTY_LINK_OPTION_ARRAY,
- StandardDeleteOption.OVERRIDE_READ_ONLY);
- } catch (final IOException e) {
- throw new IOException("Cannot delete file: " + file, e);
+ deleteCounters = PathUtils.delete(
+ file.toPath(), PathUtils.EMPTY_LINK_OPTION_ARRAY,
+ StandardDeleteOption.OVERRIDE_READ_ONLY);
+ } catch (final IOException ex) {
+ throw new IOException("Cannot delete file: " + file, ex);
}
-
if (deleteCounters.getFileCounter().get() < 1 && deleteCounters.getDirectoryCounter().get() < 1) {
// didn't find a file to delete.
throw new FileNotFoundException("File does not exist: " + file);
@@ -1533,6 +1538,7 @@ public static boolean isEmptyDirectory(final File directory) throws IOException
* @param chronoLocalDate the date reference.
* @return true if the {@link File} exists and has been modified after the given
* {@link ChronoLocalDate} at the current time.
+ * @throws UncheckedIOException if an I/O error occurs
* @throws NullPointerException if the file or local date is {@code null}.
* @since 2.8.0
*/
@@ -1554,6 +1560,7 @@ public static boolean isFileNewer(final File file, final ChronoLocalDate chronoL
* @param localTime the time reference.
* @return true if the {@link File} exists and has been modified after the given
* {@link ChronoLocalDate} at the given time.
+ * @throws UncheckedIOException if an I/O error occurs
* @throws NullPointerException if the file, local date or zone ID is {@code null}.
* @since 2.8.0
*/
@@ -1572,6 +1579,7 @@ public static boolean isFileNewer(final File file, final ChronoLocalDate chronoL
* @param offsetTime the time reference
* @return true if the {@link File} exists and has been modified after the given {@link ChronoLocalDate} at the given
* {@link OffsetTime}.
+ * @throws UncheckedIOException if an I/O error occurs
* @throws NullPointerException if the file, local date or zone ID is {@code null}
* @since 2.12.0
*/
@@ -1594,6 +1602,7 @@ public static boolean isFileNewer(final File file, final ChronoLocalDate chronoL
* @param chronoLocalDateTime the date reference.
* @return true if the {@link File} exists and has been modified after the given
* {@link ChronoLocalDateTime} at the system-default time zone.
+ * @throws UncheckedIOException if an I/O error occurs
* @throws NullPointerException if the file or local date time is {@code null}.
* @since 2.8.0
*/
@@ -1610,6 +1619,7 @@ public static boolean isFileNewer(final File file, final ChronoLocalDateTime>
* @param zoneId the time zone.
* @return true if the {@link File} exists and has been modified after the given
* {@link ChronoLocalDateTime} at the given {@link ZoneId}.
+ * @throws UncheckedIOException if an I/O error occurs
* @throws NullPointerException if the file, local date time or zone ID is {@code null}.
* @since 2.8.0
*/
@@ -1627,6 +1637,7 @@ public static boolean isFileNewer(final File file, final ChronoLocalDateTime>
* @return true if the {@link File} exists and has been modified after the given
* {@link ChronoZonedDateTime}.
* @throws NullPointerException if the file or zoned date time is {@code null}.
+ * @throws UncheckedIOException if an I/O error occurs
* @since 2.8.0
*/
public static boolean isFileNewer(final File file, final ChronoZonedDateTime> chronoZonedDateTime) {
@@ -1642,6 +1653,7 @@ public static boolean isFileNewer(final File file, final ChronoZonedDateTime>
* @param date the date reference.
* @return true if the {@link File} exists and has been modified
* after the given {@link Date}.
+ * @throws UncheckedIOException if an I/O error occurs
* @throws NullPointerException if the file or date is {@code null}.
*/
public static boolean isFileNewer(final File file, final Date date) {
@@ -1685,6 +1697,7 @@ public static boolean isFileNewer(final File file, final FileTime fileTime) thro
* @param instant the date reference.
* @return true if the {@link File} exists and has been modified after the given {@link Instant}.
* @throws NullPointerException if the file or instant is {@code null}.
+ * @throws UncheckedIOException if an I/O error occurs
* @since 2.8.0
*/
public static boolean isFileNewer(final File file, final Instant instant) {
@@ -1699,6 +1712,7 @@ public static boolean isFileNewer(final File file, final Instant instant) {
* @param timeMillis the time reference measured in milliseconds since the
* epoch (00:00:00 GMT, January 1, 1970).
* @return true if the {@link File} exists and has been modified after the given time reference.
+ * @throws UncheckedIOException if an I/O error occurs
* @throws NullPointerException if the file is {@code null}.
*/
public static boolean isFileNewer(final File file, final long timeMillis) {
@@ -1712,6 +1726,7 @@ public static boolean isFileNewer(final File file, final long timeMillis) {
* @param file the {@link File} of which the modification date must be compared
* @param offsetDateTime the date reference
* @return true if the {@link File} exists and has been modified before the given {@link OffsetDateTime}.
+ * @throws UncheckedIOException if an I/O error occurs
* @throws NullPointerException if the file or zoned date time is {@code null}
* @since 2.12.0
*/
@@ -1735,6 +1750,7 @@ public static boolean isFileNewer(final File file, final OffsetDateTime offsetDa
* @return true if the {@link File} exists and has been modified before the given
* {@link ChronoLocalDate} at the current time.
* @throws NullPointerException if the file or local date is {@code null}.
+ * @throws UncheckedIOException if an I/O error occurs
* @see ZoneId#systemDefault()
* @see LocalTime#now()
* @since 2.8.0
@@ -1757,6 +1773,7 @@ public static boolean isFileOlder(final File file, final ChronoLocalDate chronoL
* @param localTime the time reference.
* @return true if the {@link File} exists and has been modified before the
* given {@link ChronoLocalDate} at the specified time.
+ * @throws UncheckedIOException if an I/O error occurs
* @throws NullPointerException if the file, local date or local time is {@code null}.
* @see ZoneId#systemDefault()
* @since 2.8.0
@@ -1777,6 +1794,7 @@ public static boolean isFileOlder(final File file, final ChronoLocalDate chronoL
* @return true if the {@link File} exists and has been modified after the given {@link ChronoLocalDate} at the given
* {@link OffsetTime}.
* @throws NullPointerException if the file, local date or zone ID is {@code null}
+ * @throws UncheckedIOException if an I/O error occurs
* @since 2.12.0
*/
public static boolean isFileOlder(final File file, final ChronoLocalDate chronoLocalDate, final OffsetTime offsetTime) {
@@ -1799,6 +1817,7 @@ public static boolean isFileOlder(final File file, final ChronoLocalDate chronoL
* @return true if the {@link File} exists and has been modified before the given
* {@link ChronoLocalDateTime} at the system-default time zone.
* @throws NullPointerException if the file or local date time is {@code null}.
+ * @throws UncheckedIOException if an I/O error occurs
* @see ZoneId#systemDefault()
* @since 2.8.0
*/
@@ -1816,6 +1835,7 @@ public static boolean isFileOlder(final File file, final ChronoLocalDateTime>
* @return true if the {@link File} exists and has been modified before the given
* {@link ChronoLocalDateTime} at the given {@link ZoneId}.
* @throws NullPointerException if the file, local date time or zone ID is {@code null}.
+ * @throws UncheckedIOException if an I/O error occurs
* @since 2.8.0
*/
public static boolean isFileOlder(final File file, final ChronoLocalDateTime> chronoLocalDateTime, final ZoneId zoneId) {
@@ -1832,6 +1852,7 @@ public static boolean isFileOlder(final File file, final ChronoLocalDateTime>
* @return true if the {@link File} exists and has been modified before the given
* {@link ChronoZonedDateTime}.
* @throws NullPointerException if the file or zoned date time is {@code null}.
+ * @throws UncheckedIOException if an I/O error occurs
* @since 2.8.0
*/
public static boolean isFileOlder(final File file, final ChronoZonedDateTime> chronoZonedDateTime) {
@@ -1846,6 +1867,7 @@ public static boolean isFileOlder(final File file, final ChronoZonedDateTime>
* @param date the date reference.
* @return true if the {@link File} exists and has been modified before the given {@link Date}.
* @throws NullPointerException if the file or date is {@code null}.
+ * @throws UncheckedIOException if an I/O error occurs
*/
public static boolean isFileOlder(final File file, final Date date) {
Objects.requireNonNull(date, "date");
@@ -1859,7 +1881,8 @@ public static boolean isFileOlder(final File file, final Date date) {
* @param reference the {@link File} of which the modification date is used.
* @return true if the {@link File} exists and has been modified before the reference {@link File}.
* @throws NullPointerException if the file or reference file is {@code null}.
- * @throws IllegalArgumentException if the reference file doesn't exist.
+ * @throws FileNotFoundException if the reference file doesn't exist.
+ * @throws UncheckedIOException if an I/O error occurs
*/
public static boolean isFileOlder(final File file, final File reference) throws FileNotFoundException {
return Uncheck.get(() -> PathUtils.isOlder(file.toPath(), reference.toPath()));
@@ -1902,6 +1925,7 @@ public static boolean isFileOlder(final File file, final Instant instant) {
* epoch (00:00:00 GMT, January 1, 1970).
* @return true if the {@link File} exists and has been modified before the given time reference.
* @throws NullPointerException if the file is {@code null}.
+ * @throws UncheckedIOException if an I/O error occurs
*/
public static boolean isFileOlder(final File file, final long timeMillis) {
Objects.requireNonNull(file, "file");
@@ -2304,7 +2328,7 @@ private static File mkdirs(final File directory) throws IOException {
* @since 1.4
*/
public static void moveDirectory(final File srcDir, final File destDir) throws IOException {
- validateMoveParameters(srcDir, destDir);
+ Objects.requireNonNull(destDir, "destination");
requireDirectoryExists(srcDir, "srcDir");
requireAbsent(destDir, "destDir");
if (!srcDir.renameTo(destDir)) {
@@ -2326,7 +2350,7 @@ public static void moveDirectory(final File srcDir, final File destDir) throws I
* If {@code createDestDir} is true, creates all destination parent directories, including any necessary but non-existent parent directories.
*
*
- * @param source the file to be moved.
+ * @param source the directory to be moved.
* @param destDir the destination file.
* @param createDestDir If {@code true} create the destination directory, otherwise if {@code false} throw an
* IOException.
@@ -2391,7 +2415,7 @@ public static void moveFile(final File srcFile, final File destFile) throws IOEx
* @since 2.9.0
*/
public static void moveFile(final File srcFile, final File destFile, final CopyOption... copyOptions) throws IOException {
- validateMoveParameters(srcFile, destFile);
+ Objects.requireNonNull(destFile, "destination");
checkFileExists(srcFile, "srcFile");
requireAbsent(destFile, "destFile");
final boolean rename = srcFile.renameTo(destFile);
@@ -2406,15 +2430,15 @@ public static void moveFile(final File srcFile, final File destFile, final CopyO
}
/**
- * Moves a file to a directory.
+ * Moves a file into a directory.
*
* If {@code createDestDir} is true, creates all destination parent directories, including any necessary but non-existent parent directories.
*
*
* @param srcFile the file to be moved.
- * @param destDir the destination file.
- * @param createDestDir If {@code true} create the destination directory, otherwise if {@code false} throw an
- * IOException.
+ * @param destDir the directory to move the file into
+ * @param createDestDir if {@code true} create the destination directory. If {@code false} throw an
+ * IOException if the destination directory does not already exist.
* @throws NullPointerException if any of the given {@link File}s are {@code null}.
* @throws FileExistsException if the destination file exists.
* @throws FileNotFoundException if the source file does not exist.
@@ -2434,7 +2458,7 @@ public static void moveFileToDirectory(final File srcFile, final File destDir, f
}
/**
- * Moves a file or directory to a destination directory.
+ * Moves a file or directory into a destination directory.
*
* If {@code createDestDir} is true, creates all destination parent directories, including any necessary but non-existent parent directories.
*
@@ -2444,7 +2468,8 @@ public static void moveFileToDirectory(final File srcFile, final File destDir, f
*
* @param src the file or directory to be moved.
* @param destDir the destination directory.
- * @param createDestDir If {@code true} create the destination directory, otherwise if {@code false} throw an IOException.
+ * @param createDestDir if {@code true} create the destination directory. If {@code false} throw an
+ * IOException if the destination directory does not already exist.
* @throws NullPointerException if any of the given {@link File}s are {@code null}.
* @throws FileExistsException if the directory or file exists in the destination directory.
* @throws FileNotFoundException if the source file does not exist.
@@ -2555,7 +2580,9 @@ public static FileOutputStream openOutputStream(final File file) throws IOExcept
public static FileOutputStream openOutputStream(final File file, final boolean append) throws IOException {
Objects.requireNonNull(file, "file");
if (file.exists()) {
- checkFileExists(file, "file");
+ if (!file.isFile()) {
+ throw new IllegalArgumentException("Parameter 'file' is not a file: " + file);
+ }
requireCanWrite(file, "file");
} else {
createParentDirectories(file);
@@ -2711,7 +2738,6 @@ private static void requireCanonicalPathsNotEquals(final File file1, final File
* @throws IllegalArgumentException if the file is not writable.
*/
private static void requireCanWrite(final File file, final String name) {
- Objects.requireNonNull(file, "file");
if (!file.canWrite()) {
throw new IllegalArgumentException("File parameter '" + name + " is not writable: '" + file + "'");
}
@@ -2741,50 +2767,29 @@ private static void requireDirectoryExists(final File directory, final String na
*
* @param directory The {@link File} to check.
* @param name The parameter name to use in the exception message in case of null input.
- * @throws FileNotFoundException if the given {@link File} does not exist
* @throws NullPointerException if the given {@link File} is {@code null}.
* @throws IllegalArgumentException if the given {@link File} exists but is not a directory.
*/
private static void requireDirectoryIfExists(final File directory, final String name) throws FileNotFoundException {
Objects.requireNonNull(directory, name);
- if (directory.exists()) {
- requireDirectoryExists(directory, name);
- }
- }
-
- /**
- * Requires that the given {@link File} object, which may be a file or a directory,
- * points to an actually existing item in the file system,
- * and throws a {@link FileNotFoundException} if it doesn't.
- *
- * @param file The {@link File} to check.
- * @param fileParamName The parameter name to use in the exception message in case of {@code null} input.
- * @return the given file.
- * @throws NullPointerException if the given {@link File} is {@code null}.
- * @throws FileNotFoundException if the given {@link File} does not exist.
- */
- private static File checkFileObjectExists(final File file, final String fileParamName) throws FileNotFoundException {
- Objects.requireNonNull(file, fileParamName);
- if (!file.exists()) {
- throw new FileNotFoundException("File system element for parameter '" + fileParamName + "' does not exist: '" + file + "'");
+ if (directory.exists() && !directory.isDirectory()) {
+ throw new IllegalArgumentException("Parameter '" + name + "' is not a directory: '" + directory + "'");
}
- return file;
}
/**
- * Requires that the given {@link File} object,
+ * Requires that the given {@link File} object
* points to an actual file (not a directory) in the file system,
* and throws a {@link FileNotFoundException} if it doesn't.
* It throws an IllegalArgumentException if the object points to a directory.
*
* @param file The {@link File} to check.
* @param name The parameter name to use in the exception message.
- * @return the given file.
* @throws FileNotFoundException if the file does not exist
* @throws NullPointerException if the given {@link File} is {@code null}.
* @throws IllegalArgumentException if the given {@link File} is not a file.
*/
- private static File checkFileExists(final File file, final String name) throws FileNotFoundException {
+ private static void checkFileExists(final File file, final String name) throws FileNotFoundException {
Objects.requireNonNull(file, name);
if (!file.isFile()) {
if (file.exists()) {
@@ -2792,20 +2797,6 @@ private static File checkFileExists(final File file, final String name) throws F
}
throw new FileNotFoundException("Source '" + file + "' does not exist");
}
- return file;
- }
-
- /**
- * Requires parameter attributes for a file copy operation.
- *
- * @param source the source file
- * @param destination the destination
- * @throws NullPointerException if any of the given {@link File}s are {@code null}.
- * @throws FileNotFoundException if the source does not exist.
- */
- private static void requireFileCopy(final File source, final File destination) throws FileNotFoundException {
- checkFileObjectExists(source, "source");
- Objects.requireNonNull(destination, "destination");
}
/**
@@ -3520,7 +3511,7 @@ public static void writeStringToFile(final File file, final String data, final S
/**
* Instances should NOT be constructed in standard programming.
- * @deprecated Will be private in 3.0.
+ * @deprecated Will be private in 4.0.
*/
@Deprecated
public FileUtils() { //NOSONAR
diff --git a/src/main/java/org/apache/commons/io/function/IOConsumer.java b/src/main/java/org/apache/commons/io/function/IOConsumer.java
index e417178fc3e..e35ad7ef0e6 100644
--- a/src/main/java/org/apache/commons/io/function/IOConsumer.java
+++ b/src/main/java/org/apache/commons/io/function/IOConsumer.java
@@ -67,7 +67,7 @@ static void forAll(final IOConsumer action, final Stream stream) throw
}
/**
- * Performs an action for each element of the array gathering any exceptions.
+ * Performs an action for each element of the array, gathering any exceptions.
*
* @param action The action to apply to each input element.
* @param array The input to stream.
diff --git a/src/test/java/org/apache/commons/io/FileUtilsCopyDirectoryToDirectoryTest.java b/src/test/java/org/apache/commons/io/FileUtilsCopyDirectoryToDirectoryTest.java
index e4032f86924..cce762bd3f1 100644
--- a/src/test/java/org/apache/commons/io/FileUtilsCopyDirectoryToDirectoryTest.java
+++ b/src/test/java/org/apache/commons/io/FileUtilsCopyDirectoryToDirectoryTest.java
@@ -77,7 +77,7 @@ public void testCopyDirectoryToDirectoryThrowsIllegalExceptionWithCorrectMessage
try (TempFile srcDir = TempFile.create("notadirectory", null)) {
final File destDir = new File(temporaryFolder, "destinationDirectory");
destDir.mkdirs();
- final String expectedMessage = String.format("Parameter 'sourceDir' is not a directory: '%s'", srcDir);
+ final String expectedMessage = String.format("Parameter 'srcDir' is not a directory: '%s'", srcDir);
assertExceptionTypeAndMessage(srcDir.toFile(), destDir, IllegalArgumentException.class, expectedMessage);
}
}
diff --git a/src/test/java/org/apache/commons/io/FileUtilsDeleteDirectoryLinuxTest.java b/src/test/java/org/apache/commons/io/FileUtilsDeleteDirectoryLinuxTest.java
index 60600ff57d9..ad50f2f8170 100644
--- a/src/test/java/org/apache/commons/io/FileUtilsDeleteDirectoryLinuxTest.java
+++ b/src/test/java/org/apache/commons/io/FileUtilsDeleteDirectoryLinuxTest.java
@@ -88,7 +88,8 @@ public void testThrowsOnCannotDeleteFile() throws Exception {
try {
// deleteDirectory calls forceDelete
final IOExceptionList ioExceptionList = (IOExceptionList) assertThrows(IOException.class, () -> FileUtils.deleteDirectory(nested));
- assertTrue(ioExceptionList.getCause(0).getMessage().endsWith("Cannot delete file: " + file.getAbsolutePath()));
+ final String message = ioExceptionList.getCause(0).getMessage();
+ assertTrue(message.endsWith("Cannot delete file: " + file.getAbsolutePath()), message);
} finally {
chmod(nested, 755, false);
FileUtils.deleteDirectory(nested);