HADOOP-14630 Contract Tests to verify create, mkdirs and rename under a file is forbidden

Contributed by Steve Loughran.

Not all stores do complete validation here; in particular the S3A
Connector does not: checking up the entire directory tree to see if a path matches
is a file significantly slows things down.

This check does take place in S3A mkdirs(), which walks backwards up the list of
parent paths until it finds a directory (success) or a file (failure).
In practice production applications invariably create destination directories
before writing 1+ file into them -restricting check purely to the mkdirs()
call deliver significant speed up while implicitly including the checks.

Change-Id: I2c9df748e92b5655232e7d888d896f1868806eb0

Author: steveloughran

hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md

@@ -507,11 +507,11 @@ running out of memory as it calculates the partitions.
 
 Any FileSystem that does not actually break files into blocks SHOULD
 return a number for this that results in efficient processing.
-A FileSystem MAY make this user-configurable (the S3 and Swift filesystem clients do this).
+A FileSystem MAY make this user-configurable (the object store connectors usually do this).
 
 ###  `long getDefaultBlockSize(Path p)`
 
-Get the "default" block size for a path —that is, the block size to be used
+Get the "default" block size for a path --that is, the block size to be used
 when writing objects to a path in the filesystem.
 
 #### Preconditions
@@ -560,14 +560,21 @@ on the filesystem.
 
 ### `boolean mkdirs(Path p, FsPermission permission)`
 
-Create a directory and all its parents
+Create a directory and all its parents.
 
 #### Preconditions
 
 
+The path must either be a directory or not exist
+ 
      if exists(FS, p) and not isDir(FS, p) :
          raise [ParentNotDirectoryException, FileAlreadyExistsException, IOException]
 
+No ancestor may be a file
+
+    forall d = ancestors(FS, p) : 
+        if exists(FS, d) and not isDir(FS, d) :
+            raise [ParentNotDirectoryException, FileAlreadyExistsException, IOException]
 
 #### Postconditions
 
@@ -607,14 +614,20 @@ Writing to or overwriting a directory must fail.
 
     if isDir(FS, p) : raise {FileAlreadyExistsException, FileNotFoundException, IOException}
 
+No ancestor may be a file
+
+    forall d = ancestors(FS, p) : 
+        if exists(FS, d) and not isDir(FS, d) :
+            raise [ParentNotDirectoryException, FileAlreadyExistsException, IOException]
 
 FileSystems may reject the request for other
 reasons, such as the FS being read-only  (HDFS),
 the block size being below the minimum permitted (HDFS),
 the replication count being out of range (HDFS),
 quotas on namespace or filesystem being exceeded, reserved
 names, etc. All rejections SHOULD be `IOException` or a subclass thereof
-and MAY be a `RuntimeException` or subclass. For instance, HDFS may raise a `InvalidPathException`.
+and MAY be a `RuntimeException` or subclass.
+For instance, HDFS may raise an `InvalidPathException`.
 
 #### Postconditions
 

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractCreateTest.java

@@ -22,11 +22,11 @@
 import org.apache.hadoop.fs.FileAlreadyExistsException;
 import org.apache.hadoop.fs.FileStatus;
 import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.ParentNotDirectoryException;
 import org.apache.hadoop.fs.Path;
 import org.junit.Test;
-import org.junit.internal.AssumptionViolatedException;
+import org.junit.AssumptionViolatedException;
 
-import java.io.FileNotFoundException;
 import java.io.IOException;
 
 import static org.apache.hadoop.fs.contract.ContractTestUtils.dataset;
@@ -40,7 +40,7 @@
  * Test creating files, overwrite options etc.
  */
 public abstract class AbstractContractCreateTest extends
-                                                 AbstractFSContractTestBase {
+    AbstractFSContractTestBase {
 
   /**
    * How long to wait for a path to become visible.
@@ -113,7 +113,6 @@ private void testOverwriteExistingFile(boolean useBuilder) throws Throwable {
    * This test catches some eventual consistency problems that blobstores exhibit,
    * as we are implicitly verifying that updates are consistent. This
    * is why different file lengths and datasets are used
-   * @throws Throwable
    */
   @Test
   public void testOverwriteExistingFile() throws Throwable {
@@ -137,10 +136,6 @@ private void testOverwriteEmptyDirectory(boolean useBuilder)
     } catch (FileAlreadyExistsException expected) {
       //expected
       handleExpectedException(expected);
-    } catch (FileNotFoundException e) {
-      handleRelaxedException("overwriting a dir with a file ",
-                             "FileAlreadyExistsException",
-                             e);
     } catch (IOException e) {
       handleRelaxedException("overwriting a dir with a file ",
                              "FileAlreadyExistsException",
@@ -189,10 +184,6 @@ private void testOverwriteNonEmptyDirectory(boolean useBuilder)
     } catch (FileAlreadyExistsException expected) {
       //expected
       handleExpectedException(expected);
-    } catch (FileNotFoundException e) {
-      handleRelaxedException("overwriting a dir with a file ",
-                             "FileAlreadyExistsException",
-                             e);
     } catch (IOException e) {
       handleRelaxedException("overwriting a dir with a file ",
                              "FileAlreadyExistsException",
@@ -332,4 +323,117 @@ public void testCreateMakesParentDirs() throws Throwable {
     assertTrue("Grandparent directory does not appear to be a directory",
         fs.getFileStatus(grandparent).isDirectory());
   }
+
+  @Test
+  public void testCreateFileUnderFile() throws Throwable {
+    describe("Verify that it is forbidden to create file/file");
+    if (isSupported(CREATE_FILE_UNDER_FILE_ALLOWED)) {
+      // object store or some file systems: downgrade to a skip so that the
+      // failure is visible in test results
+      skip("This filesystem supports creating files under files");
+    }
+    Path grandparent = methodPath();
+    Path parent = new Path(grandparent, "parent");
+    expectCreateUnderFileFails(
+        "creating a file under a file",
+        grandparent,
+        parent);
+  }
+
+  @Test
+  public void testCreateUnderFileSubdir() throws Throwable {
+    describe("Verify that it is forbidden to create file/dir/file");
+    if (isSupported(CREATE_FILE_UNDER_FILE_ALLOWED)) {
+      // object store or some file systems: downgrade to a skip so that the
+      // failure is visible in test results
+      skip("This filesystem supports creating files under files");
+    }
+    Path grandparent = methodPath();
+    Path parent = new Path(grandparent, "parent");
+    Path child = new Path(parent, "child");
+    expectCreateUnderFileFails(
+        "creating a file under a subdirectory of a file",
+        grandparent,
+        child);
+  }
+
+
+  @Test
+  public void testMkdirUnderFile() throws Throwable {
+    describe("Verify that it is forbidden to create file/dir");
+    Path grandparent = methodPath();
+    Path parent = new Path(grandparent, "parent");
+    expectMkdirsUnderFileFails("mkdirs() under a file",
+        grandparent, parent);
+  }
+
+  @Test
+  public void testMkdirUnderFileSubdir() throws Throwable {
+    describe("Verify that it is forbidden to create file/dir/dir");
+    Path grandparent = methodPath();
+    Path parent = new Path(grandparent, "parent");
+    Path child = new Path(parent, "child");
+    expectMkdirsUnderFileFails("mkdirs() file/dir",
+        grandparent, child);
+
+    try {
+      // create the child
+      mkdirs(child);
+    } catch (FileAlreadyExistsException | ParentNotDirectoryException ex) {
+      // either of these may be raised.
+      handleExpectedException(ex);
+    } catch (IOException e) {
+      handleRelaxedException("creating a file under a subdirectory of a file ",
+          "FileAlreadyExistsException",
+          e);
+    }
+  }
+
+  /**
+   * Expect that touch() will fail because the parent is a file.
+   * @param action action for message
+   * @param file filename to create
+   * @param descendant path under file
+   * @throws Exception failure
+   */
+  protected void expectCreateUnderFileFails(String action,
+      Path file, Path descendant)
+      throws Exception {
+    createFile(file);
+    try {
+      // create the child
+      createFile(descendant);
+    } catch (FileAlreadyExistsException | ParentNotDirectoryException ex) {
+      //expected
+      handleExpectedException(ex);
+    } catch (IOException e) {
+      handleRelaxedException(action,
+          "ParentNotDirectoryException",
+          e);
+    }
+  }
+
+  protected void expectMkdirsUnderFileFails(String action,
+      Path file, Path descendant)
+      throws Exception {
+    createFile(file);
+    try {
+      // now mkdirs
+      mkdirs(descendant);
+    } catch (FileAlreadyExistsException | ParentNotDirectoryException ex) {
+      //expected
+      handleExpectedException(ex);
+    } catch (IOException e) {
+      handleRelaxedException(action,
+          "ParentNotDirectoryException",
+          e);
+    }
+  }
+
+  private void createFile(Path path) throws IOException {
+    byte[] data = dataset(256, 'a', 'z');
+    FileSystem fs = getFileSystem();
+    writeDataset(fs, path, data, data.length, 1024 * 1024,
+        true);
+  }
 }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractContractRenameTest.java

@@ -29,10 +29,10 @@
 import static org.apache.hadoop.fs.contract.ContractTestUtils.*;
 
 /**
- * Test creating files, overwrite options &c
+ * Test renaming files.
  */
 public abstract class AbstractContractRenameTest extends
-                                                 AbstractFSContractTestBase {
+    AbstractFSContractTestBase {
 
   @Test
   public void testRenameNewFileSameDir() throws Throwable {
@@ -83,7 +83,8 @@ public void testRenameNonexistentFile() throws Throwable {
           "FileNotFoundException",
           e);
     }
-    assertPathDoesNotExist("rename nonexistent file created a destination file", target);
+    assertPathDoesNotExist("rename nonexistent file created a destination file",
+        target);
   }
 
   /**
@@ -112,7 +113,7 @@ public void testRenameFileOverExistingFile() throws Throwable {
       // the filesystem supports rename(file, file2) by overwriting file2
 
       assertTrue("Rename returned false", renamed);
-        destUnchanged = false;
+      destUnchanged = false;
       } else {
         // rename is rejected by returning 'false' or throwing an exception
         if (renamed && !renameReturnsFalseOnRenameDestExists) {
@@ -129,12 +130,13 @@ public void testRenameFileOverExistingFile() throws Throwable {
     // verify that the destination file is as expected based on the expected
     // outcome
     verifyFileContents(getFileSystem(), destFile,
-        destUnchanged? destData: srcData);
+        destUnchanged ? destData: srcData);
   }
 
   @Test
   public void testRenameDirIntoExistingDir() throws Throwable {
-    describe("Verify renaming a dir into an existing dir puts it underneath"
+    describe("Verify renaming a dir into an existing dir puts it"
+        + " underneath"
              +" and leaves existing files alone");
     FileSystem fs = getFileSystem();
     String sourceSubdir = "source";
@@ -145,15 +147,15 @@ public void testRenameDirIntoExistingDir() throws Throwable {
     Path destDir = path("dest");
 
     Path destFilePath = new Path(destDir, "dest-512.txt");
-    byte[] destDateset = dataset(512, 'A', 'Z');
-    writeDataset(fs, destFilePath, destDateset, destDateset.length, 1024, false);
+    byte[] destData = dataset(512, 'A', 'Z');
+    writeDataset(fs, destFilePath, destData, destData.length, 1024, false);
     assertIsFile(destFilePath);
 
     boolean rename = rename(srcDir, destDir);
     Path renamedSrc = new Path(destDir, sourceSubdir);
     assertIsFile(destFilePath);
     assertIsDirectory(renamedSrc);
-    verifyFileContents(fs, destFilePath, destDateset);
+    verifyFileContents(fs, destFilePath, destData);
     assertTrue("rename returned false though the contents were copied", rename);
   }
 
@@ -285,4 +287,54 @@ private void validateAncestorsMoved(Path src, Path dst, String nestedPath)
     }
   }
 
+  @Test
+  public void testRenameFileUnderFile() throws Exception {
+    String action = "rename directly under file";
+    describe(action);
+    Path base = methodPath();
+    Path grandparent = new Path(base, "file");
+    expectRenameUnderFileFails(action,
+        grandparent,
+        new Path(base, "testRenameSrc"),
+        new Path(grandparent, "testRenameTarget"));
+  }
+
+  @Test
+  public void testRenameFileUnderFileSubdir() throws Exception {
+    String action = "rename directly under file/subdir";
+    describe(action);
+    Path base = methodPath();
+    Path grandparent = new Path(base, "file");
+    Path parent = new Path(grandparent, "parent");
+    expectRenameUnderFileFails(action,
+        grandparent,
+        new Path(base, "testRenameSrc"),
+        new Path(parent, "testRenameTarget"));
+  }
+
+  protected void expectRenameUnderFileFails(String action,
+      Path file, Path renameSrc, Path renameTarget)
+      throws Exception {
+    byte[] data = dataset(256, 'a', 'z');
+    FileSystem fs = getFileSystem();
+    writeDataset(fs, file, data, data.length, 1024 * 1024,
+        true);
+    writeDataset(fs, renameSrc, data, data.length, 1024 * 1024,
+        true);
+    String outcome;
+    boolean renamed;
+    try {
+      renamed = rename(renameSrc, renameTarget);
+      outcome = action + ": rename (" + renameSrc + ", " + renameTarget
+          + ")= " + renamed;
+    } catch (IOException e) {
+      // raw local raises an exception here
+      renamed = false;
+      outcome = "rename raised an exception: " + e;
+    }
+    assertPathDoesNotExist("after " + outcome, renameTarget);
+    assertFalse(outcome, renamed);
+    assertPathExists(action, renameSrc);
+  }
+
 }

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/AbstractFSContractTestBase.java

@@ -225,6 +225,15 @@ protected Path path(String filepath) throws IOException {
       new Path(getContract().getTestPath(), filepath));
   }
 
+  /**
+   * Get a path whose name ends with the name of this method.
+   * @return a path implicitly unique amongst all methods in this class
+   * @throws IOException IO problems
+   */
+  protected Path methodPath() throws IOException {
+    return path(methodName.getMethodName());
+  }
+
   /**
    * Take a simple path like "/something" and turn it into
    * a qualified path against the test FS.

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/fs/contract/ContractOptions.java

@@ -51,6 +51,15 @@ public interface ContractOptions {
    */
   String CREATE_VISIBILITY_DELAYED = "create-visibility-delayed";
 
+  /**
+   * Flag to indicate that it is possible to create a file under a file.
+   * This is a complete violation of the filesystem rules, but it is one
+   * which object stores have been known to do for performance
+   * <i>and because nobody has ever noticed.</i>
+   * {@value}
+   */
+  String CREATE_FILE_UNDER_FILE_ALLOWED = "create-file-under-file-allowed";
+
   /**
    * Is a filesystem case sensitive.
    * Some of the filesystems that say "no" here may mean

hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClient.java

@@ -1538,7 +1538,8 @@ public boolean rename(String src, String dst) throws IOException {
           DSQuotaExceededException.class,
           QuotaByStorageTypeExceededException.class,
           UnresolvedPathException.class,
-          SnapshotAccessControlException.class);
+          SnapshotAccessControlException.class,
+          ParentNotDirectoryException.class);
     }
   }