From 78f613a25fb5452d4f608766bec6aa8171283266 Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Thu, 17 Feb 2022 23:02:34 +0530 Subject: [PATCH 01/15] HADOOP-18155. Refactor tests in TestFileUtil --- .../java/org/apache/hadoop/fs/FileUtil.java | 36 ++++- .../org/apache/hadoop/fs/TestFileUtil.java | 134 ++++++++++-------- 2 files changed, 109 insertions(+), 61 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java index 96d8a40512f0a..4af6590bbb94a 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java @@ -39,6 +39,7 @@ import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.LinkOption; +import java.nio.file.Paths; import java.util.ArrayList; import java.util.Enumeration; import java.util.List; @@ -992,6 +993,14 @@ private static void unpackEntries(TarArchiveInputStream tis, + " would create entry outside of " + outputDir); } + if (Shell.WINDOWS && (entry.isSymbolicLink() || entry.isLink())) { + String canonicalTargetPath = getCanonicalPath(entry.getLinkName(), outputDir); + if (!canonicalTargetPath.startsWith(targetDirPath)) { + throw new IOException( + "expanding " + entry.getName() + " would create entry outside of " + outputDir); + } + } + if (entry.isDirectory()) { File subDir = new File(outputDir, entry.getName()); if (!subDir.mkdirs() && !subDir.isDirectory()) { @@ -1007,10 +1016,12 @@ private static void unpackEntries(TarArchiveInputStream tis, } if (entry.isSymbolicLink()) { - // Create symbolic link relative to tar parent dir - Files.createSymbolicLink(FileSystems.getDefault() - .getPath(outputDir.getPath(), entry.getName()), - FileSystems.getDefault().getPath(entry.getLinkName())); + // Create symlink with canonical target path to ensure that we don't extract + // outside targetDirPath + String canonicalTargetPath = getCanonicalPath(entry.getLinkName(), outputDir); + Files.createSymbolicLink( + FileSystems.getDefault().getPath(outputDir.getPath(), entry.getName()), + FileSystems.getDefault().getPath(canonicalTargetPath)); return; } @@ -1022,7 +1033,8 @@ private static void unpackEntries(TarArchiveInputStream tis, } if (entry.isLink()) { - File src = new File(outputDir, entry.getLinkName()); + String canonicalTargetPath = getCanonicalPath(entry.getLinkName(), outputDir); + File src = new File(canonicalTargetPath); HardLink.createHardLink(src, outputFile); return; } @@ -1030,6 +1042,20 @@ private static void unpackEntries(TarArchiveInputStream tis, org.apache.commons.io.FileUtils.copyToFile(tis, outputFile); } + /** + * Gets the canonical path for the given path. + * + * @param path The path for which the canonical path needs to be computed. + * @param parentDir The parent directory to use if the path is a relative path. + * @return The canonical path of the given path. + */ + private static String getCanonicalPath(String path, File parentDir) throws IOException { + java.nio.file.Path targetPath = Paths.get(path); + return (targetPath.isAbsolute() ? + new File(path) : + new File(parentDir, path)).getCanonicalPath(); + } + /** * Class for creating hardlinks. * Supports Unix, WindXP. diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index e19900dfeacdb..fb7ff3e73259b 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -44,11 +44,13 @@ import java.nio.charset.StandardCharsets; import java.nio.file.FileSystems; import java.nio.file.Files; +import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.List; +import java.util.Objects; import java.util.jar.Attributes; import java.util.jar.JarFile; import java.util.jar.Manifest; @@ -160,7 +162,7 @@ public void setup() throws IOException { new File(del, FILE).createNewFile(); File tmpFile = new File(tmp, FILE); - tmpFile.createNewFile(); + assertTrue(tmpFile.createNewFile()); // create files new File(dir1, FILE).createNewFile(); @@ -173,7 +175,7 @@ public void setup() throws IOException { // create a symlink to dir File linkDir = new File(del, "tmpDir"); FileUtil.symLink(tmp.toString(), linkDir.toString()); - Assert.assertEquals(5, del.listFiles().length); + Assert.assertEquals(5, Objects.requireNonNull(del.listFiles()).length); // create files in partitioned directories createFile(partitioned, "part-r-00000", "foo"); @@ -200,13 +202,9 @@ public void tearDown() throws IOException { private File createFile(File directory, String name, String contents) throws IOException { File newFile = new File(directory, name); - PrintWriter pw = new PrintWriter(newFile); - try { + try (PrintWriter pw = new PrintWriter(newFile)) { pw.println(contents); } - finally { - pw.close(); - } return newFile; } @@ -218,11 +216,11 @@ public void testListFiles() throws IOException { //Test existing directory with no files case File newDir = new File(tmp.getPath(),"test"); - newDir.mkdir(); + assertTrue(newDir.mkdir()); Assert.assertTrue("Failed to create test dir", newDir.exists()); files = FileUtil.listFiles(newDir); Assert.assertEquals(0, files.length); - newDir.delete(); + assertTrue(newDir.delete()); Assert.assertFalse("Failed to delete test dir", newDir.exists()); //Test non-existing directory case, this throws @@ -244,11 +242,11 @@ public void testListAPI() throws IOException { //Test existing directory with no files case File newDir = new File(tmp.getPath(),"test"); - newDir.mkdir(); + assertTrue(newDir.mkdir()); Assert.assertTrue("Failed to create test dir", newDir.exists()); files = FileUtil.list(newDir); Assert.assertEquals("New directory unexpectedly contains files", 0, files.length); - newDir.delete(); + assertTrue(newDir.delete()); Assert.assertFalse("Failed to delete test dir", newDir.exists()); //Test non-existing directory case, this throws @@ -279,13 +277,13 @@ public void testFullyDelete() throws IOException { @Test (timeout = 30000) public void testFullyDeleteSymlinks() throws IOException { File link = new File(del, LINK); - Assert.assertEquals(5, del.list().length); + Assert.assertEquals(5, Objects.requireNonNull(del.list()).length); // Since tmpDir is symlink to tmp, fullyDelete(tmpDir) should not // delete contents of tmp. See setupDirs for details. boolean ret = FileUtil.fullyDelete(link); Assert.assertTrue(ret); Assert.assertFalse(link.exists()); - Assert.assertEquals(4, del.list().length); + Assert.assertEquals(4, Objects.requireNonNull(del.list()).length); validateTmpDir(); File linkDir = new File(del, "tmpDir"); @@ -294,7 +292,7 @@ public void testFullyDeleteSymlinks() throws IOException { ret = FileUtil.fullyDelete(linkDir); Assert.assertTrue(ret); Assert.assertFalse(linkDir.exists()); - Assert.assertEquals(3, del.list().length); + Assert.assertEquals(3, Objects.requireNonNull(del.list()).length); validateTmpDir(); } @@ -314,12 +312,12 @@ public void testFullyDeleteDanglingSymlinks() throws IOException { // dangling symlink to file File link = new File(del, LINK); - Assert.assertEquals(5, del.list().length); + Assert.assertEquals(5, Objects.requireNonNull(del.list()).length); // Even though 'y' is dangling symlink to file tmp/x, fullyDelete(y) // should delete 'y' properly. ret = FileUtil.fullyDelete(link); Assert.assertTrue(ret); - Assert.assertEquals(4, del.list().length); + Assert.assertEquals(4, Objects.requireNonNull(del.list()).length); // dangling symlink to directory File linkDir = new File(del, "tmpDir"); @@ -327,7 +325,7 @@ public void testFullyDeleteDanglingSymlinks() throws IOException { // delete tmpDir properly. ret = FileUtil.fullyDelete(linkDir); Assert.assertTrue(ret); - Assert.assertEquals(3, del.list().length); + Assert.assertEquals(3, Objects.requireNonNull(del.list()).length); } @Test (timeout = 30000) @@ -335,13 +333,13 @@ public void testFullyDeleteContents() throws IOException { boolean ret = FileUtil.fullyDeleteContents(del); Assert.assertTrue(ret); Assert.assertTrue(del.exists()); - Assert.assertEquals(0, del.listFiles().length); + Assert.assertEquals(0, Objects.requireNonNull(del.listFiles()).length); validateTmpDir(); } private void validateTmpDir() { Assert.assertTrue(tmp.exists()); - Assert.assertEquals(1, tmp.listFiles().length); + Assert.assertEquals(1, Objects.requireNonNull(tmp.listFiles()).length); Assert.assertTrue(new File(tmp, FILE).exists()); } @@ -463,7 +461,7 @@ public void testFailFullyDeleteDirSymlinks() throws IOException { // fail symlink deletion Assert.assertFalse(ret); Assert.assertTrue(linkDir.exists()); - Assert.assertEquals(5, del.list().length); + Assert.assertEquals(5, Objects.requireNonNull(del.list()).length); // tmp dir should exist validateTmpDir(); // simulate disk recovers and turns good @@ -472,7 +470,7 @@ public void testFailFullyDeleteDirSymlinks() throws IOException { // success symlink deletion Assert.assertTrue(ret); Assert.assertFalse(linkDir.exists()); - Assert.assertEquals(4, del.list().length); + Assert.assertEquals(4, Objects.requireNonNull(del.list()).length); // tmp dir should exist validateTmpDir(); } @@ -614,9 +612,8 @@ public void testGetDU() throws Exception { public void testUnTar() throws IOException { // make a simple tar: final File simpleTar = new File(del, FILE); - OutputStream os = new FileOutputStream(simpleTar); - TarOutputStream tos = new TarOutputStream(os); - try { + OutputStream os = new FileOutputStream(simpleTar); + try (TarOutputStream tos = new TarOutputStream(os)) { TarEntry te = new TarEntry("/bar/foo"); byte[] data = "some-content".getBytes("UTF-8"); te.setSize(data.length); @@ -625,8 +622,6 @@ public void testUnTar() throws IOException { tos.closeEntry(); tos.flush(); tos.finish(); - } finally { - tos.close(); } // successfully untar it into an existing dir: @@ -640,7 +635,7 @@ public void testUnTar() throws IOException { assertTrue(regularFile.exists()); try { FileUtil.unTar(simpleTar, regularFile); - assertTrue("An IOException expected.", false); + fail("An IOException expected."); } catch (IOException ioe) { // okay } @@ -1304,7 +1299,7 @@ public void setupCompareFs() { uri4 = new URI(uris4); uri5 = new URI(uris5); uri6 = new URI(uris6); - } catch (URISyntaxException use) { + } catch (URISyntaxException ignored) { } // Set up InetAddress inet1 = mock(InetAddress.class); @@ -1327,7 +1322,7 @@ public void setupCompareFs() { when(InetAddress.getByName(uris3)).thenReturn(inet3); when(InetAddress.getByName(uris4)).thenReturn(inet4); when(InetAddress.getByName(uris5)).thenReturn(inet5); - } catch (UnknownHostException ue) { + } catch (UnknownHostException ignored) { } fs1 = mock(FileSystem.class); @@ -1347,62 +1342,89 @@ public void setupCompareFs() { @Test public void testCompareFsNull() throws Exception { setupCompareFs(); - assertEquals(FileUtil.compareFs(null,fs1),false); - assertEquals(FileUtil.compareFs(fs1,null),false); + assertFalse(FileUtil.compareFs(null, fs1)); + assertFalse(FileUtil.compareFs(fs1, null)); } @Test public void testCompareFsDirectories() throws Exception { setupCompareFs(); - assertEquals(FileUtil.compareFs(fs1,fs1),true); - assertEquals(FileUtil.compareFs(fs1,fs2),false); - assertEquals(FileUtil.compareFs(fs1,fs5),false); - assertEquals(FileUtil.compareFs(fs3,fs4),true); - assertEquals(FileUtil.compareFs(fs1,fs6),false); + assertTrue(FileUtil.compareFs(fs1, fs1)); + assertFalse(FileUtil.compareFs(fs1, fs2)); + assertFalse(FileUtil.compareFs(fs1, fs5)); + assertTrue(FileUtil.compareFs(fs3, fs4)); + assertFalse(FileUtil.compareFs(fs1, fs6)); } @Test(timeout = 8000) public void testCreateSymbolicLinkUsingJava() throws IOException { final File simpleTar = new File(del, FILE); OutputStream os = new FileOutputStream(simpleTar); - TarArchiveOutputStream tos = new TarArchiveOutputStream(os); - File untarFile = null; - try { + try (TarArchiveOutputStream tos = new TarArchiveOutputStream(os)) { // Files to tar final String tmpDir = "tmp/test"; File tmpDir1 = new File(tmpDir, "dir1/"); File tmpDir2 = new File(tmpDir, "dir2/"); - // Delete the directories if they already exist - tmpDir1.mkdirs(); - tmpDir2.mkdirs(); + if (!tmpDir1.mkdirs()) { + throw new IOException(String.format("Unable to create directory %s", tmpDir1)); + } + if (!tmpDir2.mkdirs()) { + throw new IOException(String.format("Unable to create directory %s", tmpDir2)); + } - java.nio.file.Path symLink = FileSystems - .getDefault().getPath(tmpDir1.getPath() + "/sl"); + java.nio.file.Path symLink = Paths.get(tmpDir1.getPath(), "sl"); // Create Symbolic Link - Files.createSymbolicLink(symLink, - FileSystems.getDefault().getPath(tmpDir2.getPath())).toString(); + Files.createSymbolicLink(symLink, Paths.get(tmpDir2.getPath())); assertTrue(Files.isSymbolicLink(symLink.toAbsolutePath())); - // put entries in tar file + // Put entries in tar file putEntriesInTar(tos, tmpDir1.getParentFile()); tos.close(); - untarFile = new File(tmpDir, "2"); - // Untar using java + File untarFile = new File(tmpDir, "2"); + // Untar using Java FileUtil.unTarUsingJava(simpleTar, untarFile, false); // Check symbolic link and other directories are there in untar file assertTrue(Files.exists(untarFile.toPath())); - assertTrue(Files.exists(FileSystems.getDefault().getPath(untarFile - .getPath(), tmpDir))); - assertTrue(Files.isSymbolicLink(FileSystems.getDefault().getPath(untarFile - .getPath().toString(), symLink.toString()))); - + assertTrue(Files.exists(Paths.get(untarFile.getPath(), tmpDir))); + assertTrue(Files.isSymbolicLink(Paths.get(untarFile.getPath(), symLink.toString()))); } finally { FileUtils.deleteDirectory(new File("tmp")); - tos.close(); } + } + + @Test(expected = IOException.class) + public void testCreateArbitrarySymlinkUsingJava() throws IOException { + final File simpleTar = new File(del, FILE); + OutputStream os = new FileOutputStream(simpleTar); + + File rootDir = new File("tmp"); + try (TarArchiveOutputStream tos = new TarArchiveOutputStream(os)) { + // Create arbitrary dir + File arbitraryDir = new File(rootDir, "arbitrary-dir/"); + assertTrue(arbitraryDir.mkdirs()); + + // We will tar from the tar-root lineage + File tarRoot = new File(rootDir, "tar-root/"); + File dir1 = new File(tarRoot, "dir1/"); + assertTrue(dir1.mkdirs()); + // Create Symbolic Link to an arbitrary dir + java.nio.file.Path symLink = Paths.get(dir1.getPath(), "sl"); + Files.createSymbolicLink(symLink, arbitraryDir.toPath().toAbsolutePath()); + + // Put entries in tar file + putEntriesInTar(tos, tarRoot); + putEntriesInTar(tos, new File(symLink.toFile(), "dir-outside-tar-root/")); + tos.close(); + + // Untar using Java + File untarFile = new File(rootDir, "extracted"); + FileUtil.unTarUsingJava(simpleTar, untarFile, false); + } finally { + FileUtils.deleteDirectory(rootDir); + } } private void putEntriesInTar(TarArchiveOutputStream tos, File f) @@ -1419,7 +1441,7 @@ private void putEntriesInTar(TarArchiveOutputStream tos, File f) if (f.isDirectory()) { tos.putArchiveEntry(new TarArchiveEntry(f)); tos.closeArchiveEntry(); - for (File child : f.listFiles()) { + for (File child : Objects.requireNonNull(f.listFiles())) { putEntriesInTar(tos, child); } } From 0a848fb23896c11a00c0ba9df65f45c114daa5bc Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Tue, 8 Mar 2022 18:34:02 +0530 Subject: [PATCH 02/15] Handle long paths --- .../src/test/java/org/apache/hadoop/fs/TestFileUtil.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index fb7ff3e73259b..44293986b9a6e 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -1401,6 +1401,8 @@ public void testCreateArbitrarySymlinkUsingJava() throws IOException { File rootDir = new File("tmp"); try (TarArchiveOutputStream tos = new TarArchiveOutputStream(os)) { + tos.setLongFileMode(TarArchiveOutputStream.LONGFILE_GNU); + // Create arbitrary dir File arbitraryDir = new File(rootDir, "arbitrary-dir/"); assertTrue(arbitraryDir.mkdirs()); From 3916fca7b4c38f7f1c85ddc2cc30ff1acb9e92cd Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Tue, 8 Mar 2022 22:52:20 +0530 Subject: [PATCH 03/15] Check for link containment on all platforms --- .../src/main/java/org/apache/hadoop/fs/FileUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java index 4af6590bbb94a..b788c7ec6b664 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java @@ -993,7 +993,7 @@ private static void unpackEntries(TarArchiveInputStream tis, + " would create entry outside of " + outputDir); } - if (Shell.WINDOWS && (entry.isSymbolicLink() || entry.isLink())) { + if (entry.isSymbolicLink() || entry.isLink()) { String canonicalTargetPath = getCanonicalPath(entry.getLinkName(), outputDir); if (!canonicalTargetPath.startsWith(targetDirPath)) { throw new IOException( From dd67415f74a3a10245405bd91a2f14c1d452efca Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Wed, 9 Mar 2022 11:09:33 +0530 Subject: [PATCH 04/15] Use LambdaTestUtils * Using LambdaTestUtils for asserting expected exceptions. --- .../org/apache/hadoop/fs/TestFileUtil.java | 23 ++++++------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index 44293986b9a6e..e00a31401e86b 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -62,6 +62,7 @@ import org.apache.commons.io.FileUtils; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.test.GenericTestUtils; +import org.apache.hadoop.test.LambdaTestUtils; import org.apache.hadoop.util.StringUtils; import org.apache.tools.tar.TarEntry; import org.apache.tools.tar.TarOutputStream; @@ -607,9 +608,9 @@ public void testGetDU() throws Exception { FileUtil.chmod(partitioned.getAbsolutePath(), "0777", true/*recursive*/); } } - + @Test (timeout = 30000) - public void testUnTar() throws IOException { + public void testUnTar() throws Exception { // make a simple tar: final File simpleTar = new File(del, FILE); OutputStream os = new FileOutputStream(simpleTar); @@ -629,16 +630,11 @@ public void testUnTar() throws IOException { // check result: assertTrue(new File(tmp, "/bar/foo").exists()); assertEquals(12, new File(tmp, "/bar/foo").length()); - + final File regularFile = new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog"); regularFile.createNewFile(); assertTrue(regularFile.exists()); - try { - FileUtil.unTar(simpleTar, regularFile); - fail("An IOException expected."); - } catch (IOException ioe) { - // okay - } + LambdaTestUtils.intercept(IOException.class, () -> FileUtil.unTar(simpleTar, regularFile)); } @Test (timeout = 30000) @@ -698,7 +694,7 @@ public void testCreateLocalTempFile() throws IOException { } @Test (timeout = 30000) - public void testUnZip() throws IOException { + public void testUnZip() throws Exception { // make sa simple zip final File simpleZip = new File(del, FILE); OutputStream os = new FileOutputStream(simpleZip); @@ -725,12 +721,7 @@ public void testUnZip() throws IOException { final File regularFile = new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog"); regularFile.createNewFile(); assertTrue(regularFile.exists()); - try { - FileUtil.unZip(simpleZip, regularFile); - assertTrue("An IOException expected.", false); - } catch (IOException ioe) { - // okay - } + LambdaTestUtils.intercept(IOException.class, () -> FileUtil.unZip(simpleZip, regularFile)); } @Test (timeout = 30000) From 6df0d80a6043873cee922172360c18fb27524fe1 Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Wed, 9 Mar 2022 11:11:32 +0530 Subject: [PATCH 05/15] Remove redundant null check --- .../src/test/java/org/apache/hadoop/fs/TestFileUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index e00a31401e86b..6ab3ee414d119 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -1434,7 +1434,7 @@ private void putEntriesInTar(TarArchiveOutputStream tos, File f) if (f.isDirectory()) { tos.putArchiveEntry(new TarArchiveEntry(f)); tos.closeArchiveEntry(); - for (File child : Objects.requireNonNull(f.listFiles())) { + for (File child : f.listFiles()) { putEntriesInTar(tos, child); } } From 107678c7ee3612be4f378a4477895328bc3c458c Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Wed, 9 Mar 2022 11:24:50 +0530 Subject: [PATCH 06/15] Refactor assertion of del list length * Added TestFileUtil#assertDelListLength to assert that expected length of TestFileUtil#del --- .../org/apache/hadoop/fs/TestFileUtil.java | 27 +++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index 6ab3ee414d119..ee13da77150e8 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -66,6 +66,8 @@ import org.apache.hadoop.util.StringUtils; import org.apache.tools.tar.TarEntry; import org.apache.tools.tar.TarOutputStream; + +import org.assertj.core.api.Assertions; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -278,13 +280,13 @@ public void testFullyDelete() throws IOException { @Test (timeout = 30000) public void testFullyDeleteSymlinks() throws IOException { File link = new File(del, LINK); - Assert.assertEquals(5, Objects.requireNonNull(del.list()).length); + assertDelListLength(5); // Since tmpDir is symlink to tmp, fullyDelete(tmpDir) should not // delete contents of tmp. See setupDirs for details. boolean ret = FileUtil.fullyDelete(link); Assert.assertTrue(ret); Assert.assertFalse(link.exists()); - Assert.assertEquals(4, Objects.requireNonNull(del.list()).length); + assertDelListLength(4); validateTmpDir(); File linkDir = new File(del, "tmpDir"); @@ -293,7 +295,7 @@ public void testFullyDeleteSymlinks() throws IOException { ret = FileUtil.fullyDelete(linkDir); Assert.assertTrue(ret); Assert.assertFalse(linkDir.exists()); - Assert.assertEquals(3, Objects.requireNonNull(del.list()).length); + assertDelListLength(3); validateTmpDir(); } @@ -313,12 +315,12 @@ public void testFullyDeleteDanglingSymlinks() throws IOException { // dangling symlink to file File link = new File(del, LINK); - Assert.assertEquals(5, Objects.requireNonNull(del.list()).length); + assertDelListLength(5); // Even though 'y' is dangling symlink to file tmp/x, fullyDelete(y) // should delete 'y' properly. ret = FileUtil.fullyDelete(link); Assert.assertTrue(ret); - Assert.assertEquals(4, Objects.requireNonNull(del.list()).length); + assertDelListLength(4); // dangling symlink to directory File linkDir = new File(del, "tmpDir"); @@ -326,7 +328,7 @@ public void testFullyDeleteDanglingSymlinks() throws IOException { // delete tmpDir properly. ret = FileUtil.fullyDelete(linkDir); Assert.assertTrue(ret); - Assert.assertEquals(3, Objects.requireNonNull(del.list()).length); + assertDelListLength(3); } @Test (timeout = 30000) @@ -462,7 +464,7 @@ public void testFailFullyDeleteDirSymlinks() throws IOException { // fail symlink deletion Assert.assertFalse(ret); Assert.assertTrue(linkDir.exists()); - Assert.assertEquals(5, Objects.requireNonNull(del.list()).length); + assertDelListLength(5); // tmp dir should exist validateTmpDir(); // simulate disk recovers and turns good @@ -471,11 +473,20 @@ public void testFailFullyDeleteDirSymlinks() throws IOException { // success symlink deletion Assert.assertTrue(ret); Assert.assertFalse(linkDir.exists()); - Assert.assertEquals(4, Objects.requireNonNull(del.list()).length); + assertDelListLength(4); // tmp dir should exist validateTmpDir(); } + /** + * Asserts if the {@link TestFileUtil#del} meets the given expected length. + * + * @param expectedLength The expected length of the {@link TestFileUtil#del}. + */ + private void assertDelListLength(int expectedLength) { + Assertions.assertThat(del.list()).describedAs("del list").isNotNull().hasSize(expectedLength); + } + /** * Extend {@link File}. Same as {@link File} except for two things: (1) This * treats file1Name as a very special file which is not delete-able From 101c5c6276ec51698c34dfe7536b3904798920df Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Wed, 9 Mar 2022 11:54:17 +0530 Subject: [PATCH 07/15] Refactor createNewFile * Added TestFileUtil#createNewFileAndVerify that creates a new file and verifies if the operation was successful. --- .../org/apache/hadoop/fs/TestFileUtil.java | 68 +++++++++---------- 1 file changed, 31 insertions(+), 37 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index ee13da77150e8..4c8b89b9a3ae0 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -163,13 +163,12 @@ public void setup() throws IOException { FileUtils.forceMkdir(dir1); FileUtils.forceMkdir(dir2); - new File(del, FILE).createNewFile(); - File tmpFile = new File(tmp, FILE); - assertTrue(tmpFile.createNewFile()); + createNewFileAndVerify(new File(del, FILE)); + File tmpFile = createNewFileAndVerify(new File(tmp, FILE)); // create files - new File(dir1, FILE).createNewFile(); - new File(dir2, FILE).createNewFile(); + createNewFileAndVerify(new File(dir1, FILE)); + createNewFileAndVerify(new File(dir2, FILE)); // create a symlink to file File link = new File(del, LINK); @@ -367,15 +366,15 @@ private void validateTmpDir() { * @throws IOException */ private void setupDirsAndNonWritablePermissions() throws IOException { - new MyFile(del, FILE_1_NAME).createNewFile(); + createNewFileAndVerify(new MyFile(del, FILE_1_NAME)); // "file1" is non-deletable by default, see MyFile.delete(). xSubDir.mkdirs(); - file2.createNewFile(); + createNewFileAndVerify(file2); xSubSubDir.mkdirs(); - file22.createNewFile(); + createNewFileAndVerify(file22); revokePermissions(file22); revokePermissions(xSubSubDir); @@ -384,10 +383,9 @@ private void setupDirsAndNonWritablePermissions() throws IOException { revokePermissions(xSubDir); ySubDir.mkdirs(); - file3.createNewFile(); + createNewFileAndVerify(file3); - File tmpFile = new File(tmp, FILE); - tmpFile.createNewFile(); + File tmpFile = createNewFileAndVerify(new File(tmp, FILE)); FileUtil.symLink(tmpFile.toString(), zlink.toString()); } @@ -487,6 +485,11 @@ private void assertDelListLength(int expectedLength) { Assertions.assertThat(del.list()).describedAs("del list").isNotNull().hasSize(expectedLength); } + private File createNewFileAndVerify(File file) throws IOException { + assertTrue("Unable to create new file " + file, file.createNewFile()); + return file; + } + /** * Extend {@link File}. Same as {@link File} except for two things: (1) This * treats file1Name as a very special file which is not delete-able @@ -642,19 +645,15 @@ public void testUnTar() throws Exception { assertTrue(new File(tmp, "/bar/foo").exists()); assertEquals(12, new File(tmp, "/bar/foo").length()); - final File regularFile = new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog"); - regularFile.createNewFile(); - assertTrue(regularFile.exists()); + final File regularFile = + createNewFileAndVerify(new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog")); LambdaTestUtils.intercept(IOException.class, () -> FileUtil.unTar(simpleTar, regularFile)); } @Test (timeout = 30000) public void testReplaceFile() throws IOException { - final File srcFile = new File(tmp, "src"); - // src exists, and target does not exist: - srcFile.createNewFile(); - assertTrue(srcFile.exists()); + final File srcFile = createNewFileAndVerify(new File(tmp, "src")); final File targetFile = new File(tmp, "target"); assertTrue(!targetFile.exists()); FileUtil.replaceFile(srcFile, targetFile); @@ -662,20 +661,18 @@ public void testReplaceFile() throws IOException { assertTrue(targetFile.exists()); // src exists and target is a regular file: - srcFile.createNewFile(); + createNewFileAndVerify(srcFile); assertTrue(srcFile.exists()); FileUtil.replaceFile(srcFile, targetFile); assertTrue(!srcFile.exists()); assertTrue(targetFile.exists()); // src exists, and target is a non-empty directory: - srcFile.createNewFile(); + createNewFileAndVerify(srcFile); assertTrue(srcFile.exists()); targetFile.delete(); targetFile.mkdirs(); - File obstacle = new File(targetFile, "obstacle"); - obstacle.createNewFile(); - assertTrue(obstacle.exists()); + File obstacle = createNewFileAndVerify(new File(targetFile, "obstacle")); assertTrue(targetFile.exists() && targetFile.isDirectory()); try { FileUtil.replaceFile(srcFile, targetFile); @@ -728,10 +725,9 @@ public void testUnZip() throws Exception { // check result: assertTrue(new File(tmp, "foo").exists()); assertEquals(12, new File(tmp, "foo").length()); - - final File regularFile = new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog"); - regularFile.createNewFile(); - assertTrue(regularFile.exists()); + + final File regularFile = + createNewFileAndVerify(new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog")); LambdaTestUtils.intercept(IOException.class, () -> FileUtil.unZip(simpleZip, regularFile)); } @@ -888,8 +884,7 @@ public void testSymlink() throws Exception { */ @Test (timeout = 30000) public void testSymlinkRenameTo() throws Exception { - File file = new File(del, FILE); - file.createNewFile(); + File file = createNewFileAndVerify(new File(del, FILE)); File link = new File(del, "_link"); // create the symlink @@ -917,8 +912,7 @@ public void testSymlinkRenameTo() throws Exception { */ @Test (timeout = 30000) public void testSymlinkDelete() throws Exception { - File file = new File(del, FILE); - file.createNewFile(); + File file = createNewFileAndVerify(new File(del, FILE)); File link = new File(del, "_link"); // create the symlink @@ -1177,9 +1171,9 @@ public void testCreateJarWithClassPath() throws Exception { } // create non-jar files, which we expect to not be included in the classpath - Assert.assertTrue(new File(tmp, "text.txt").createNewFile()); - Assert.assertTrue(new File(tmp, "executable.exe").createNewFile()); - Assert.assertTrue(new File(tmp, "README").createNewFile()); + createNewFileAndVerify(new File(tmp, "text.txt")); + createNewFileAndVerify(new File(tmp, "executable.exe")); + createNewFileAndVerify(new File(tmp, "README")); // create classpath jar String wildcardPath = tmp.getCanonicalPath() + File.separator + "*"; @@ -1265,9 +1259,9 @@ public void testGetJarsInDirectory() throws Exception { } // create non-jar files, which we expect to not be included in the result - assertTrue(new File(tmp, "text.txt").createNewFile()); - assertTrue(new File(tmp, "executable.exe").createNewFile()); - assertTrue(new File(tmp, "README").createNewFile()); + createNewFileAndVerify(new File(tmp, "text.txt")); + createNewFileAndVerify(new File(tmp, "executable.exe")); + createNewFileAndVerify(new File(tmp, "README")); // pass in the directory String directory = tmp.getCanonicalPath(); From 3e726caa3630054b9c73e853f0f120400d09eb69 Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Wed, 9 Mar 2022 11:57:44 +0530 Subject: [PATCH 08/15] Add documentation for TestFileUtil#createNewFileAndVerify --- .../src/test/java/org/apache/hadoop/fs/TestFileUtil.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index 4c8b89b9a3ae0..5b4a6efb8b855 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -485,6 +485,13 @@ private void assertDelListLength(int expectedLength) { Assertions.assertThat(del.list()).describedAs("del list").isNotNull().hasSize(expectedLength); } + /** + * Creates a new file and verifies the result of the operation. + * + * @param file The {@link File} to create. + * @return The {@link File} instance that was passed. + * @throws IOException As per {@link File#createNewFile()}. + */ private File createNewFileAndVerify(File file) throws IOException { assertTrue("Unable to create new file " + file, file.createNewFile()); return file; From 987d8e393aa93177b36690e664d1b264ccf30028 Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Wed, 9 Mar 2022 12:05:49 +0530 Subject: [PATCH 09/15] Add TestFileUtil$Verify helper class * TestFileUtil$Verify helps in performing File operations and also verify them. --- .../org/apache/hadoop/fs/TestFileUtil.java | 69 ++++++++++--------- 1 file changed, 37 insertions(+), 32 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index 5b4a6efb8b855..a618f50581ee5 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -163,12 +163,12 @@ public void setup() throws IOException { FileUtils.forceMkdir(dir1); FileUtils.forceMkdir(dir2); - createNewFileAndVerify(new File(del, FILE)); - File tmpFile = createNewFileAndVerify(new File(tmp, FILE)); + Verify.createNewFile(new File(del, FILE)); + File tmpFile = Verify.createNewFile(new File(tmp, FILE)); // create files - createNewFileAndVerify(new File(dir1, FILE)); - createNewFileAndVerify(new File(dir2, FILE)); + Verify.createNewFile(new File(dir1, FILE)); + Verify.createNewFile(new File(dir2, FILE)); // create a symlink to file File link = new File(del, LINK); @@ -366,15 +366,15 @@ private void validateTmpDir() { * @throws IOException */ private void setupDirsAndNonWritablePermissions() throws IOException { - createNewFileAndVerify(new MyFile(del, FILE_1_NAME)); + Verify.createNewFile(new MyFile(del, FILE_1_NAME)); // "file1" is non-deletable by default, see MyFile.delete(). xSubDir.mkdirs(); - createNewFileAndVerify(file2); + Verify.createNewFile(file2); xSubSubDir.mkdirs(); - createNewFileAndVerify(file22); + Verify.createNewFile(file22); revokePermissions(file22); revokePermissions(xSubSubDir); @@ -383,9 +383,9 @@ private void setupDirsAndNonWritablePermissions() throws IOException { revokePermissions(xSubDir); ySubDir.mkdirs(); - createNewFileAndVerify(file3); + Verify.createNewFile(file3); - File tmpFile = createNewFileAndVerify(new File(tmp, FILE)); + File tmpFile = Verify.createNewFile(new File(tmp, FILE)); FileUtil.symLink(tmpFile.toString(), zlink.toString()); } @@ -486,17 +486,22 @@ private void assertDelListLength(int expectedLength) { } /** - * Creates a new file and verifies the result of the operation. - * - * @param file The {@link File} to create. - * @return The {@link File} instance that was passed. - * @throws IOException As per {@link File#createNewFile()}. + * Helper class to perform {@link File} operation and also verify them. */ - private File createNewFileAndVerify(File file) throws IOException { - assertTrue("Unable to create new file " + file, file.createNewFile()); - return file; + public static class Verify { + /** + * Creates a new file and verifies the result of the operation. + * + * @param file The {@link File} to create. + * @return The {@link File} instance that was passed. + * @throws IOException As per {@link File#createNewFile()}. + */ + public static File createNewFile(File file) throws IOException { + assertTrue("Unable to create new file " + file, file.createNewFile()); + return file; + } } - + /** * Extend {@link File}. Same as {@link File} except for two things: (1) This * treats file1Name as a very special file which is not delete-able @@ -653,14 +658,14 @@ public void testUnTar() throws Exception { assertEquals(12, new File(tmp, "/bar/foo").length()); final File regularFile = - createNewFileAndVerify(new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog")); + Verify.createNewFile(new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog")); LambdaTestUtils.intercept(IOException.class, () -> FileUtil.unTar(simpleTar, regularFile)); } @Test (timeout = 30000) public void testReplaceFile() throws IOException { // src exists, and target does not exist: - final File srcFile = createNewFileAndVerify(new File(tmp, "src")); + final File srcFile = Verify.createNewFile(new File(tmp, "src")); final File targetFile = new File(tmp, "target"); assertTrue(!targetFile.exists()); FileUtil.replaceFile(srcFile, targetFile); @@ -668,18 +673,18 @@ public void testReplaceFile() throws IOException { assertTrue(targetFile.exists()); // src exists and target is a regular file: - createNewFileAndVerify(srcFile); + Verify.createNewFile(srcFile); assertTrue(srcFile.exists()); FileUtil.replaceFile(srcFile, targetFile); assertTrue(!srcFile.exists()); assertTrue(targetFile.exists()); // src exists, and target is a non-empty directory: - createNewFileAndVerify(srcFile); + Verify.createNewFile(srcFile); assertTrue(srcFile.exists()); targetFile.delete(); targetFile.mkdirs(); - File obstacle = createNewFileAndVerify(new File(targetFile, "obstacle")); + File obstacle = Verify.createNewFile(new File(targetFile, "obstacle")); assertTrue(targetFile.exists() && targetFile.isDirectory()); try { FileUtil.replaceFile(srcFile, targetFile); @@ -734,7 +739,7 @@ public void testUnZip() throws Exception { assertEquals(12, new File(tmp, "foo").length()); final File regularFile = - createNewFileAndVerify(new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog")); + Verify.createNewFile(new File(tmp, "QuickBrownFoxJumpsOverTheLazyDog")); LambdaTestUtils.intercept(IOException.class, () -> FileUtil.unZip(simpleZip, regularFile)); } @@ -891,7 +896,7 @@ public void testSymlink() throws Exception { */ @Test (timeout = 30000) public void testSymlinkRenameTo() throws Exception { - File file = createNewFileAndVerify(new File(del, FILE)); + File file = Verify.createNewFile(new File(del, FILE)); File link = new File(del, "_link"); // create the symlink @@ -919,7 +924,7 @@ public void testSymlinkRenameTo() throws Exception { */ @Test (timeout = 30000) public void testSymlinkDelete() throws Exception { - File file = createNewFileAndVerify(new File(del, FILE)); + File file = Verify.createNewFile(new File(del, FILE)); File link = new File(del, "_link"); // create the symlink @@ -1178,9 +1183,9 @@ public void testCreateJarWithClassPath() throws Exception { } // create non-jar files, which we expect to not be included in the classpath - createNewFileAndVerify(new File(tmp, "text.txt")); - createNewFileAndVerify(new File(tmp, "executable.exe")); - createNewFileAndVerify(new File(tmp, "README")); + Verify.createNewFile(new File(tmp, "text.txt")); + Verify.createNewFile(new File(tmp, "executable.exe")); + Verify.createNewFile(new File(tmp, "README")); // create classpath jar String wildcardPath = tmp.getCanonicalPath() + File.separator + "*"; @@ -1266,9 +1271,9 @@ public void testGetJarsInDirectory() throws Exception { } // create non-jar files, which we expect to not be included in the result - createNewFileAndVerify(new File(tmp, "text.txt")); - createNewFileAndVerify(new File(tmp, "executable.exe")); - createNewFileAndVerify(new File(tmp, "README")); + Verify.createNewFile(new File(tmp, "text.txt")); + Verify.createNewFile(new File(tmp, "executable.exe")); + Verify.createNewFile(new File(tmp, "README")); // pass in the directory String directory = tmp.getCanonicalPath(); From 7398999d6addb954b24600197a3800545b241573 Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Wed, 9 Mar 2022 12:12:27 +0530 Subject: [PATCH 10/15] Add Verify#mkdirs --- .../org/apache/hadoop/fs/TestFileUtil.java | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index a618f50581ee5..5d67a6566c422 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -370,10 +370,10 @@ private void setupDirsAndNonWritablePermissions() throws IOException { // "file1" is non-deletable by default, see MyFile.delete(). - xSubDir.mkdirs(); + Verify.mkdirs(xSubDir); Verify.createNewFile(file2); - xSubSubDir.mkdirs(); + Verify.mkdirs(xSubSubDir); Verify.createNewFile(file22); revokePermissions(file22); @@ -382,7 +382,7 @@ private void setupDirsAndNonWritablePermissions() throws IOException { revokePermissions(file2); revokePermissions(xSubDir); - ySubDir.mkdirs(); + Verify.mkdirs(ySubDir); Verify.createNewFile(file3); File tmpFile = Verify.createNewFile(new File(tmp, FILE)); @@ -500,6 +500,17 @@ public static File createNewFile(File file) throws IOException { assertTrue("Unable to create new file " + file, file.createNewFile()); return file; } + + /** + * Invokes {@link File#mkdirs()} on the given {@link File} instance. + * + * @param file The file to call {@link File#mkdirs()} on. + * @return The result of {@link File#mkdirs()}. + */ + public static File mkdirs(File file) { + assertTrue("Unable to mkdirs for " + file, file.mkdirs()); + return file; + } } /** @@ -683,7 +694,7 @@ public void testReplaceFile() throws IOException { Verify.createNewFile(srcFile); assertTrue(srcFile.exists()); targetFile.delete(); - targetFile.mkdirs(); + Verify.mkdirs(targetFile); File obstacle = Verify.createNewFile(new File(targetFile, "obstacle")); assertTrue(targetFile.exists() && targetFile.isDirectory()); try { @@ -1373,12 +1384,8 @@ public void testCreateSymbolicLinkUsingJava() throws IOException { final String tmpDir = "tmp/test"; File tmpDir1 = new File(tmpDir, "dir1/"); File tmpDir2 = new File(tmpDir, "dir2/"); - if (!tmpDir1.mkdirs()) { - throw new IOException(String.format("Unable to create directory %s", tmpDir1)); - } - if (!tmpDir2.mkdirs()) { - throw new IOException(String.format("Unable to create directory %s", tmpDir2)); - } + Verify.mkdirs(tmpDir1); + Verify.mkdirs(tmpDir2); java.nio.file.Path symLink = Paths.get(tmpDir1.getPath(), "sl"); @@ -1413,12 +1420,12 @@ public void testCreateArbitrarySymlinkUsingJava() throws IOException { // Create arbitrary dir File arbitraryDir = new File(rootDir, "arbitrary-dir/"); - assertTrue(arbitraryDir.mkdirs()); + Verify.mkdirs(arbitraryDir); // We will tar from the tar-root lineage File tarRoot = new File(rootDir, "tar-root/"); File dir1 = new File(tarRoot, "dir1/"); - assertTrue(dir1.mkdirs()); + Verify.mkdirs(dir1); // Create Symbolic Link to an arbitrary dir java.nio.file.Path symLink = Paths.get(dir1.getPath(), "sl"); From 52815a0d37e2aa0997a6079912600e02db966af7 Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Wed, 9 Mar 2022 12:15:44 +0530 Subject: [PATCH 11/15] Add Verify#mkdir --- .../java/org/apache/hadoop/fs/TestFileUtil.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index 5d67a6566c422..7b8dc15fd8b93 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -218,7 +218,7 @@ public void testListFiles() throws IOException { //Test existing directory with no files case File newDir = new File(tmp.getPath(),"test"); - assertTrue(newDir.mkdir()); + Verify.mkdir(newDir); Assert.assertTrue("Failed to create test dir", newDir.exists()); files = FileUtil.listFiles(newDir); Assert.assertEquals(0, files.length); @@ -244,7 +244,7 @@ public void testListAPI() throws IOException { //Test existing directory with no files case File newDir = new File(tmp.getPath(),"test"); - assertTrue(newDir.mkdir()); + Verify.mkdir(newDir); Assert.assertTrue("Failed to create test dir", newDir.exists()); files = FileUtil.list(newDir); Assert.assertEquals("New directory unexpectedly contains files", 0, files.length); @@ -501,6 +501,17 @@ public static File createNewFile(File file) throws IOException { return file; } + /** + * Invokes {@link File#mkdir()} on the given {@link File} instance. + * + * @param file The file to call {@link File#mkdir()} on. + * @return The result of {@link File#mkdir()}. + */ + public static File mkdir(File file) { + assertTrue("Unable to mkdir for " + file, file.mkdir()); + return file; + } + /** * Invokes {@link File#mkdirs()} on the given {@link File} instance. * From 5792612b4deb341c2379bcb67337b2b16621a4b1 Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Wed, 9 Mar 2022 12:23:13 +0530 Subject: [PATCH 12/15] Add Verify#delete --- .../org/apache/hadoop/fs/TestFileUtil.java | 35 ++++++++++++------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index 7b8dc15fd8b93..3fdac3adb93fd 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -490,10 +490,10 @@ private void assertDelListLength(int expectedLength) { */ public static class Verify { /** - * Creates a new file and verifies the result of the operation. + * Invokes {@link File#createNewFile()} on the given {@link File} instance. * - * @param file The {@link File} to create. - * @return The {@link File} instance that was passed. + * @param file The file to call {@link File#createNewFile()} on. + * @return The result of {@link File#createNewFile()}. * @throws IOException As per {@link File#createNewFile()}. */ public static File createNewFile(File file) throws IOException { @@ -522,6 +522,17 @@ public static File mkdirs(File file) { assertTrue("Unable to mkdirs for " + file, file.mkdirs()); return file; } + + /** + * Invokes {@link File#delete()} on the given {@link File} instance. + * + * @param file The file to call {@link File#delete()} on. + * @return The result of {@link File#delete()}. + */ + public static File delete(File file) { + assertTrue("Unable to delete " + file, file.delete()); + return file; + } } /** @@ -704,7 +715,7 @@ public void testReplaceFile() throws IOException { // src exists, and target is a non-empty directory: Verify.createNewFile(srcFile); assertTrue(srcFile.exists()); - targetFile.delete(); + Verify.delete(targetFile); Verify.mkdirs(targetFile); File obstacle = Verify.createNewFile(new File(targetFile, "obstacle")); assertTrue(targetFile.exists() && targetFile.isDirectory()); @@ -730,8 +741,8 @@ public void testCreateLocalTempFile() throws IOException { assertTrue(tmp1.exists() && tmp2.exists()); assertTrue(tmp1.canWrite() && tmp2.canWrite()); assertTrue(tmp1.canRead() && tmp2.canRead()); - tmp1.delete(); - tmp2.delete(); + Verify.delete(tmp1); + Verify.delete(tmp2); assertTrue(!tmp1.exists() && !tmp2.exists()); } @@ -814,7 +825,7 @@ public void testCopy5() throws IOException { assertTrue(srcFile.exists()); // should not be deleted // copy regular file, delete src: - dest.delete(); + Verify.delete(dest); assertTrue(!dest.exists()); result = FileUtil.copy(fs, srcPath, dest, true, conf); assertTrue(result); @@ -824,7 +835,7 @@ public void testCopy5() throws IOException { assertTrue(!srcFile.exists()); // should be deleted // copy a dir: - dest.delete(); + Verify.delete(dest); assertTrue(!dest.exists()); srcPath = new Path(partitioned.toURI()); result = FileUtil.copy(fs, srcPath, dest, true, conf); @@ -985,12 +996,12 @@ public void testSymlinkLength() throws Exception { Assert.assertEquals(data.length, file.length()); Assert.assertEquals(data.length, link.length()); - file.delete(); + Verify.delete(file); Assert.assertFalse(file.exists()); Assert.assertEquals(0, link.length()); - link.delete(); + Verify.delete(link); Assert.assertFalse(link.exists()); } @@ -1057,7 +1068,7 @@ public void testSymlinkFileAlreadyExists() throws IOException { public void testSymlinkSameFile() throws IOException { File file = new File(del, FILE); - file.delete(); + Verify.delete(file); // Create a symbolic link // The operation should succeed @@ -1546,7 +1557,7 @@ public void testReadSymlinkWithAFileAsInput() throws IOException { String result = FileUtil.readLink(file); Assert.assertEquals("", result); - file.delete(); + Verify.delete(file); } /** From 69da55573f42185cf632d629f493c709e9988ea8 Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Wed, 9 Mar 2022 12:48:09 +0530 Subject: [PATCH 13/15] Add Verify#exists and Verify#noteExists --- .../org/apache/hadoop/fs/TestFileUtil.java | 120 +++++++++++------- 1 file changed, 73 insertions(+), 47 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index 3fdac3adb93fd..c61c86ffa2c36 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -266,7 +266,7 @@ public void testListAPI() throws IOException { public void testFullyDelete() throws IOException { boolean ret = FileUtil.fullyDelete(del); Assert.assertTrue(ret); - Assert.assertFalse(del.exists()); + Verify.notExists(del); validateTmpDir(); } @@ -284,7 +284,7 @@ public void testFullyDeleteSymlinks() throws IOException { // delete contents of tmp. See setupDirs for details. boolean ret = FileUtil.fullyDelete(link); Assert.assertTrue(ret); - Assert.assertFalse(link.exists()); + Verify.notExists(link); assertDelListLength(4); validateTmpDir(); @@ -293,7 +293,7 @@ public void testFullyDeleteSymlinks() throws IOException { // delete contents of tmp. See setupDirs for details. ret = FileUtil.fullyDelete(linkDir); Assert.assertTrue(ret); - Assert.assertFalse(linkDir.exists()); + Verify.notExists(linkDir); assertDelListLength(3); validateTmpDir(); } @@ -310,7 +310,7 @@ public void testFullyDeleteDanglingSymlinks() throws IOException { // to make y as a dangling link to file tmp/x boolean ret = FileUtil.fullyDelete(tmp); Assert.assertTrue(ret); - Assert.assertFalse(tmp.exists()); + Verify.notExists(tmp); // dangling symlink to file File link = new File(del, LINK); @@ -334,15 +334,15 @@ public void testFullyDeleteDanglingSymlinks() throws IOException { public void testFullyDeleteContents() throws IOException { boolean ret = FileUtil.fullyDeleteContents(del); Assert.assertTrue(ret); - Assert.assertTrue(del.exists()); + Verify.exists(del); Assert.assertEquals(0, Objects.requireNonNull(del.listFiles()).length); validateTmpDir(); } private void validateTmpDir() { - Assert.assertTrue(tmp.exists()); + Verify.exists(tmp); Assert.assertEquals(1, Objects.requireNonNull(tmp.listFiles()).length); - Assert.assertTrue(new File(tmp, FILE).exists()); + Verify.exists(new File(tmp, FILE)); } /** @@ -385,7 +385,8 @@ private void setupDirsAndNonWritablePermissions() throws IOException { Verify.mkdirs(ySubDir); Verify.createNewFile(file3); - File tmpFile = Verify.createNewFile(new File(tmp, FILE)); + File tmpFile = new File(tmp, FILE); + tmpFile.createNewFile(); FileUtil.symLink(tmpFile.toString(), zlink.toString()); } @@ -461,7 +462,7 @@ public void testFailFullyDeleteDirSymlinks() throws IOException { boolean ret = FileUtil.fullyDelete(linkDir); // fail symlink deletion Assert.assertFalse(ret); - Assert.assertTrue(linkDir.exists()); + Verify.exists(linkDir); assertDelListLength(5); // tmp dir should exist validateTmpDir(); @@ -470,7 +471,7 @@ public void testFailFullyDeleteDirSymlinks() throws IOException { ret = FileUtil.fullyDelete(linkDir); // success symlink deletion Assert.assertTrue(ret); - Assert.assertFalse(linkDir.exists()); + Verify.notExists(linkDir); assertDelListLength(4); // tmp dir should exist validateTmpDir(); @@ -533,6 +534,29 @@ public static File delete(File file) { assertTrue("Unable to delete " + file, file.delete()); return file; } + + /** + * Invokes {@link File#exists()} on the given {@link File} instance. + * + * @param file The file to call {@link File#exists()} on. + * @return The result of {@link File#exists()}. + */ + public static File exists(File file) { + assertTrue("Expected file " + file + " doesn't exist", file.exists()); + return file; + } + + /** + * Invokes {@link File#exists()} on the given {@link File} instance to check if the + * {@link File} doesn't exists. + * + * @param file The file to call {@link File#exists()} on. + * @return The negation of the result of {@link File#exists()}. + */ + public static File notExists(File file) { + assertFalse("Expected file " + file + " must not exist", file.exists()); + return file; + } } /** @@ -687,7 +711,7 @@ public void testUnTar() throws Exception { // successfully untar it into an existing dir: FileUtil.unTar(simpleTar, tmp); // check result: - assertTrue(new File(tmp, "/bar/foo").exists()); + Verify.exists(new File(tmp, "/bar/foo")); assertEquals(12, new File(tmp, "/bar/foo").length()); final File regularFile = @@ -700,21 +724,21 @@ public void testReplaceFile() throws IOException { // src exists, and target does not exist: final File srcFile = Verify.createNewFile(new File(tmp, "src")); final File targetFile = new File(tmp, "target"); - assertTrue(!targetFile.exists()); + Verify.notExists(targetFile); FileUtil.replaceFile(srcFile, targetFile); - assertTrue(!srcFile.exists()); - assertTrue(targetFile.exists()); + Verify.notExists(srcFile); + Verify.exists(targetFile); // src exists and target is a regular file: Verify.createNewFile(srcFile); - assertTrue(srcFile.exists()); + Verify.exists(srcFile); FileUtil.replaceFile(srcFile, targetFile); - assertTrue(!srcFile.exists()); - assertTrue(targetFile.exists()); + Verify.notExists(srcFile); + Verify.exists(targetFile); // src exists, and target is a non-empty directory: Verify.createNewFile(srcFile); - assertTrue(srcFile.exists()); + Verify.exists(srcFile); Verify.delete(targetFile); Verify.mkdirs(targetFile); File obstacle = Verify.createNewFile(new File(targetFile, "obstacle")); @@ -726,9 +750,9 @@ public void testReplaceFile() throws IOException { // okay } // check up the post-condition: nothing is deleted: - assertTrue(srcFile.exists()); + Verify.exists(srcFile); assertTrue(targetFile.exists() && targetFile.isDirectory()); - assertTrue(obstacle.exists()); + Verify.exists(obstacle); } @Test (timeout = 30000) @@ -768,7 +792,7 @@ public void testUnZip() throws Exception { // successfully unzip it into an existing dir: FileUtil.unZip(simpleZip, tmp); // check result: - assertTrue(new File(tmp, "foo").exists()); + Verify.exists(new File(tmp, "foo")); assertEquals(12, new File(tmp, "foo").length()); final File regularFile = @@ -819,24 +843,24 @@ public void testCopy5() throws IOException { final File dest = new File(del, "dest"); boolean result = FileUtil.copy(fs, srcPath, dest, false, conf); assertTrue(result); - assertTrue(dest.exists()); + Verify.exists(dest); assertEquals(content.getBytes().length + System.getProperty("line.separator").getBytes().length, dest.length()); - assertTrue(srcFile.exists()); // should not be deleted + Verify.exists(srcFile); // should not be deleted // copy regular file, delete src: Verify.delete(dest); - assertTrue(!dest.exists()); + Verify.notExists(dest); result = FileUtil.copy(fs, srcPath, dest, true, conf); assertTrue(result); - assertTrue(dest.exists()); + Verify.exists(dest); assertEquals(content.getBytes().length + System.getProperty("line.separator").getBytes().length, dest.length()); - assertTrue(!srcFile.exists()); // should be deleted + Verify.notExists(srcFile); // should be deleted // copy a dir: Verify.delete(dest); - assertTrue(!dest.exists()); + Verify.notExists(dest); srcPath = new Path(partitioned.toURI()); result = FileUtil.copy(fs, srcPath, dest, true, conf); assertTrue(result); @@ -848,7 +872,7 @@ public void testCopy5() throws IOException { assertEquals(3 + System.getProperty("line.separator").getBytes().length, f.length()); } - assertTrue(!partitioned.exists()); // should be deleted + Verify.notExists(partitioned); // should be deleted } @Test (timeout = 30000) @@ -929,14 +953,15 @@ public void testSymlink() throws Exception { */ @Test (timeout = 30000) public void testSymlinkRenameTo() throws Exception { - File file = Verify.createNewFile(new File(del, FILE)); + File file = new File(del, FILE); + file.createNewFile(); File link = new File(del, "_link"); // create the symlink FileUtil.symLink(file.getAbsolutePath(), link.getAbsolutePath()); - Assert.assertTrue(file.exists()); - Assert.assertTrue(link.exists()); + Verify.exists(file); + Verify.exists(link); File link2 = new File(del, "_link2"); @@ -946,10 +971,10 @@ public void testSymlinkRenameTo() throws Exception { // Make sure the file still exists // (NOTE: this would fail on Java6 on Windows if we didn't // copy the file in FileUtil#symlink) - Assert.assertTrue(file.exists()); + Verify.exists(file); - Assert.assertTrue(link2.exists()); - Assert.assertFalse(link.exists()); + Verify.exists(link2); + Verify.notExists(link); } /** @@ -957,19 +982,20 @@ public void testSymlinkRenameTo() throws Exception { */ @Test (timeout = 30000) public void testSymlinkDelete() throws Exception { - File file = Verify.createNewFile(new File(del, FILE)); + File file = new File(del, FILE); + file.createNewFile(); File link = new File(del, "_link"); // create the symlink FileUtil.symLink(file.getAbsolutePath(), link.getAbsolutePath()); - Assert.assertTrue(file.exists()); - Assert.assertTrue(link.exists()); + Verify.exists(file); + Verify.exists(link); // make sure that deleting a symlink works properly - Assert.assertTrue(link.delete()); - Assert.assertFalse(link.exists()); - Assert.assertTrue(file.exists()); + Verify.delete(link); + Verify.notExists(link); + Verify.exists(file); } /** @@ -997,12 +1023,12 @@ public void testSymlinkLength() throws Exception { Assert.assertEquals(data.length, link.length()); Verify.delete(file); - Assert.assertFalse(file.exists()); + Verify.notExists(file); Assert.assertEquals(0, link.length()); Verify.delete(link); - Assert.assertFalse(link.exists()); + Verify.notExists(link); } /** @@ -1141,21 +1167,21 @@ private void doUntarAndVerify(File tarFile, File untarDir) String parentDir = untarDir.getCanonicalPath() + Path.SEPARATOR + "name"; File testFile = new File(parentDir + Path.SEPARATOR + "version"); - Assert.assertTrue(testFile.exists()); + Verify.exists(testFile); Assert.assertTrue(testFile.length() == 0); String imageDir = parentDir + Path.SEPARATOR + "image"; testFile = new File(imageDir + Path.SEPARATOR + "fsimage"); - Assert.assertTrue(testFile.exists()); + Verify.exists(testFile); Assert.assertTrue(testFile.length() == 157); String currentDir = parentDir + Path.SEPARATOR + "current"; testFile = new File(currentDir + Path.SEPARATOR + "fsimage"); - Assert.assertTrue(testFile.exists()); + Verify.exists(testFile); Assert.assertTrue(testFile.length() == 4331); testFile = new File(currentDir + Path.SEPARATOR + "edits"); - Assert.assertTrue(testFile.exists()); + Verify.exists(testFile); Assert.assertTrue(testFile.length() == 1033); testFile = new File(currentDir + Path.SEPARATOR + "fstime"); - Assert.assertTrue(testFile.exists()); + Verify.exists(testFile); Assert.assertTrue(testFile.length() == 8); } From 2e841bbeb8170494cb7df9a3eefe9c9800ab197c Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Wed, 9 Mar 2022 22:48:16 +0530 Subject: [PATCH 14/15] Delete whitespace --- .../src/test/java/org/apache/hadoop/fs/TestFileUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index c61c86ffa2c36..a3fef24e9a8c0 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -558,7 +558,7 @@ public static File notExists(File file) { return file; } } - + /** * Extend {@link File}. Same as {@link File} except for two things: (1) This * treats file1Name as a very special file which is not delete-able From e9661229d3388eea8afd57012a19779f48d5257c Mon Sep 17 00:00:00 2001 From: Gautham Banasandra Date: Thu, 10 Mar 2022 10:52:05 +0530 Subject: [PATCH 15/15] Fix checkstyle issues --- .../src/test/java/org/apache/hadoop/fs/TestFileUtil.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java index a3fef24e9a8c0..29eafb9e4dac0 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/TestFileUtil.java @@ -42,7 +42,6 @@ import java.net.URL; import java.net.UnknownHostException; import java.nio.charset.StandardCharsets; -import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.Paths; import java.util.ArrayList; @@ -1472,11 +1471,11 @@ public void testCreateArbitrarySymlinkUsingJava() throws IOException { // We will tar from the tar-root lineage File tarRoot = new File(rootDir, "tar-root/"); - File dir1 = new File(tarRoot, "dir1/"); - Verify.mkdirs(dir1); + File symlinkRoot = new File(tarRoot, "dir1/"); + Verify.mkdirs(symlinkRoot); // Create Symbolic Link to an arbitrary dir - java.nio.file.Path symLink = Paths.get(dir1.getPath(), "sl"); + java.nio.file.Path symLink = Paths.get(symlinkRoot.getPath(), "sl"); Files.createSymbolicLink(symLink, arbitraryDir.toPath().toAbsolutePath()); // Put entries in tar file