From cf63312cda49d2a31b443666bac61c5811462311 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Thu, 29 Aug 2019 16:20:59 -0400 Subject: [PATCH] Fixing bad behavior in IOUtil.deletePaths * The method IOUtil.deletePaths(Iterable) had pathological. * A Path is an Iterable which iterates over the subpaths in the Path. Therefore, this version was binding instead of the varargs version that takes Path... Then the method would try to delete all of the subpaths in the path. * This is obviously bad behavior. Fixed by adding an explicit check to see if it's called with a single Path. * Extracted a new method deletePath which is used for deleting a single path. * Fixes #https://github.com/samtools/htsjdk/issues/1414 --- .../htsjdk/samtools/util/DiskBackedQueue.java | 2 +- .../java/htsjdk/samtools/util/IOUtil.java | 31 ++++++++++++++----- .../java/htsjdk/samtools/util/IOUtilTest.java | 27 +++++++++++----- 3 files changed, 45 insertions(+), 15 deletions(-) diff --git a/src/main/java/htsjdk/samtools/util/DiskBackedQueue.java b/src/main/java/htsjdk/samtools/util/DiskBackedQueue.java index a3c7ff2801..b353fb132f 100644 --- a/src/main/java/htsjdk/samtools/util/DiskBackedQueue.java +++ b/src/main/java/htsjdk/samtools/util/DiskBackedQueue.java @@ -339,7 +339,7 @@ private E readFileRecord (final Path file) { private void closeIOResources() { CloserUtil.close(this.outputStream); CloserUtil.close(this.inputStream); - if (this.diskRecords != null) IOUtil.deletePaths(this.diskRecords); + if (this.diskRecords != null) IOUtil.deletePath(this.diskRecords); } /** diff --git a/src/main/java/htsjdk/samtools/util/IOUtil.java b/src/main/java/htsjdk/samtools/util/IOUtil.java index d32b7f9d40..f98c2a2b2c 100755 --- a/src/main/java/htsjdk/samtools/util/IOUtil.java +++ b/src/main/java/htsjdk/samtools/util/IOUtil.java @@ -303,16 +303,33 @@ public static void deleteFiles(final Iterable files) { } public static void deletePaths(final Path... paths) { - deletePaths(Arrays.asList(paths)); + for(Path path: paths){ + deletePath(path); + } } + /** + * Iterate through Paths and delete each one. + * Note: Path is itself an Iterable. This method special cases that and deletes the single Path rather than + * Iterating the Path for targets to delete. + * @param paths an iterable of Paths to delete + */ public static void deletePaths(final Iterable paths) { - for (final Path p : paths) { - try { - Files.delete(p); - } catch (IOException e) { - System.err.println("Could not delete file " + p); - } + //Path is itself an Iterable which causes very confusing behavior if we don't explicitly check here. + if( paths instanceof Path){ + deletePath((Path)paths); + } + paths.forEach(IOUtil::deletePath); + } + + /** + * Attempt to delete a single path and log an error if it is not deleted. + */ + public static void deletePath(Path path){ + try { + Files.delete(path); + } catch (IOException e) { + System.err.println("Could not delete file " + path); } } diff --git a/src/test/java/htsjdk/samtools/util/IOUtilTest.java b/src/test/java/htsjdk/samtools/util/IOUtilTest.java index 8bdfd0cca9..9b3b60ff34 100644 --- a/src/test/java/htsjdk/samtools/util/IOUtilTest.java +++ b/src/test/java/htsjdk/samtools/util/IOUtilTest.java @@ -35,7 +35,6 @@ import java.nio.file.Paths; import java.nio.file.spi.FileSystemProvider; -import htsjdk.samtools.BamFileIoUtils; import htsjdk.samtools.SAMException; import org.testng.Assert; import org.testng.annotations.AfterClass; @@ -44,15 +43,12 @@ import org.testng.annotations.Test; import java.lang.IllegalArgumentException; -import java.nio.file.Path; -import java.nio.file.Files; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.stream.Collectors; import java.util.Random; -import java.util.stream.Stream; import java.util.zip.GZIPOutputStream; @@ -318,13 +314,29 @@ public void testGetDefaultTmpDirPath() throws Exception { public void testDeletePathLocal(final List fileNames) throws Exception { final File tmpDir = IOUtil.createTempDir("testDeletePath", ""); final List paths = createLocalFiles(tmpDir, fileNames); - testDeletePath(paths); + testDeletePaths(paths); + } + + @Test + public void testDeleteSinglePath() throws Exception { + final Path toDelete = Files.createTempFile("file",".bad"); + Assert.assertTrue(Files.exists(toDelete)); + IOUtil.deletePath(toDelete); + Assert.assertFalse(Files.exists(toDelete)); + } + + @Test + public void testDeleteSingleWithDeletePaths() throws Exception { + final Path toDelete = Files.createTempFile("file",".bad"); + Assert.assertTrue(Files.exists(toDelete)); + IOUtil.deletePaths(toDelete); + Assert.assertFalse(Files.exists(toDelete)); } @Test(dataProvider = "fileNamesForDelete") public void testDeletePathJims(final List fileNames) throws Exception { final List paths = createJimfsFiles("testDeletePath", fileNames); - testDeletePath(paths); + testDeletePaths(paths); } @Test(dataProvider = "fileNamesForDelete") @@ -340,7 +352,8 @@ public void testDeleteArrayPathJims(final List fileNames) throws Excepti testDeletePathArray(paths); } - private static void testDeletePath(final List paths) { + + private static void testDeletePaths(final List paths) { paths.forEach(p -> Assert.assertTrue(Files.exists(p))); IOUtil.deletePaths(paths); paths.forEach(p -> Assert.assertFalse(Files.exists(p)));