HADOOP-7223. FileContext createFlag combinations are not clearly defined. Contributed by Suresh Srinivas.

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1091613 13f79535-47bb-0310-9956-ffa450edef68

CHANGES.txt

@@ -84,6 +84,9 @@ Trunk (unreleased changes)
 
     HADOOP-7202. Improve shell Command base class.  (Daryn Sharp via szetszwo)
 
+    HADOOP-7223. FileContext createFlag combinations are not clearly defined.
+    (suresh)
+
   OPTIMIZATIONS
 
   BUG FIXES

src/java/org/apache/hadoop/fs/ChecksumFs.java

@@ -337,8 +337,9 @@ public abstract class ChecksumFs extends FilterFs {
       int bytesPerSum = fs.getBytesPerSum();
       int sumBufferSize = fs.getSumBufferSize(bytesPerSum, bufferSize);
       this.sums = fs.getRawFs().createInternal(fs.getChecksumFile(file),
-          EnumSet.of(CreateFlag.OVERWRITE), absolutePermission, sumBufferSize,
+          EnumSet.of(CreateFlag.CREATE, CreateFlag.OVERWRITE),
-          replication,  blockSize,  progress, bytesPerChecksum,  createParent);
+          absolutePermission, sumBufferSize, replication, blockSize, progress,
+          bytesPerChecksum, createParent);
       sums.write(CHECKSUM_VERSION, 0, CHECKSUM_VERSION.length);
       sums.writeInt(bytesPerSum);
     }

src/java/org/apache/hadoop/fs/CreateFlag.java

@@ -17,6 +17,11 @@
  */
 package org.apache.hadoop.fs;
 
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.util.EnumSet;
+
+import org.apache.hadoop.HadoopIllegalArgumentException;
 import org.apache.hadoop.classification.InterfaceAudience;
 import org.apache.hadoop.classification.InterfaceStability;
 
@@ -25,41 +30,50 @@ import org.apache.hadoop.classification.InterfaceStability;
  * <code>
  * EnumSet.of(CreateFlag.CREATE, CreateFlag.APPEND)
  * <code>
- * and pass it to {@link org.apache.hadoop.fs.FileSystem #create(Path f, FsPermission permission,
- * EnumSet<CreateFlag> flag, int bufferSize, short replication, long blockSize,
- * Progressable progress)}.
- * 
  * <p>
- * Combine {@link #OVERWRITE} with either {@link #CREATE} 
+ * 
- * or {@link #APPEND} does the same as only use 
+ * Use the CreateFlag as follows:
- * {@link #OVERWRITE}. <br>
- * Combine {@link #CREATE} with {@link #APPEND} has the semantic:
  * <ol>
- * <li> create the file if it does not exist;
+ * <li> CREATE - to create a file if it does not exist, 
- * <li> append the file if it already exists.
+ * else throw FileAlreadyExists.</li>
+ * <li> APPEND - to append to a file if it exists, 
+ * else throw FileNotFoundException.</li>
+ * <li> OVERWRITE - to truncate a file if it exists, 
+ * else throw FileNotFoundException.</li>
+ * <li> CREATE|APPEND - to create a file if it does not exist, 
+ * else append to an existing file.</li>
+ * <li> CREATE|OVERWRITE - to create a file if it does not exist, 
+ * else overwrite an existing file.</li>
+ * </ol>
+ * 
+ * Following combination is not valid and will result in 
+ * {@link HadoopIllegalArgumentException}:
+ * <ol>
+ * <li> APPEND|OVERWRITE</li>
+ * <li> CREATE|APPEND|OVERWRITE</li>
  * </ol>
  *****************************************************************/
 @InterfaceAudience.Public
-@InterfaceStability.Stable
+@InterfaceStability.Evolving
 public enum CreateFlag {
 
   /**
-   * create the file if it does not exist, and throw an IOException if it
+   * Create a file. See javadoc for more description
    * already exists
    */
   CREATE((short) 0x01),
 
   /**
-   * create the file if it does not exist, if it exists, overwrite it.
+   * Truncate/overwrite a file. Same as POSIX O_TRUNC. See javadoc for description.
    */
   OVERWRITE((short) 0x02),
 
   /**
-   * append to a file, and throw an IOException if it does not exist
+   * Append to a file. See javadoc for more description.
    */
   APPEND((short) 0x04);
 
-  private short mode;
+  private final short mode;
 
   private CreateFlag(short mode) {
     this.mode = mode;
@@ -68,4 +82,38 @@ public enum CreateFlag {
   short getMode() {
     return mode;
   }
+  
+  /**
+   * Validate the CreateFlag for create operation
+   * @param path Object representing the path; usually String or {@link Path}
+   * @param pathExists pass true if the path exists in the file system
+   * @param flag set of CreateFlag
+   * @throws IOException on error
+   * @throws HadoopIllegalArgumentException if the CreateFlag is invalid
+   */
+  public static void validate(Object path, boolean pathExists,
+      EnumSet<CreateFlag> flag) throws IOException {
+    if (flag == null || flag.isEmpty()) {
+      throw new HadoopIllegalArgumentException(flag
+          + " does not specify any options");
+    }
+    final boolean append = flag.contains(APPEND);
+    final boolean overwrite = flag.contains(OVERWRITE);
+    
+    // Both append and overwrite is an error
+    if (append && overwrite) {
+      throw new HadoopIllegalArgumentException(
+          flag + "Both append and overwrite options cannot be enabled.");
+    }
+    if (pathExists) {
+      if (!(append || overwrite)) {
+        throw new FileAlreadyExistsException("File already exists: "
+            + path.toString()
+            + ". Append or overwrite option must be specified in " + flag);
+      }
+    } else if (!flag.contains(CREATE)) {
+      throw new FileNotFoundException("Non existing file: " + path.toString()
+          + ". Create option is not specified in " + flag);
+    }
+  }
 }

src/java/org/apache/hadoop/fs/FileContext.java

@@ -509,7 +509,7 @@ public final class FileContext {
    * writing into the file.
    * 
    * @param f the file name to open
-   * @param createFlag gives the semantics of create: overwrite, append etc.
+   * @param createFlag gives the semantics of create; see {@link CreateFlag}
    * @param opts file creation options; see {@link Options.CreateOpts}.
    *          <ul>
    *          <li>Progress - to report progress on the operation - default null
@@ -2055,7 +2055,10 @@ public final class FileContext {
         OutputStream out = null;
         try {
           in = open(qSrc);
-          out = create(qDst, EnumSet.of(CreateFlag.OVERWRITE));
+          EnumSet<CreateFlag> createFlag = overwrite ? EnumSet.of(
+              CreateFlag.CREATE, CreateFlag.OVERWRITE) : 
+                EnumSet.of(CreateFlag.CREATE);
+          out = create(qDst, createFlag);
           IOUtils.copyBytes(in, out, conf, true);
         } catch (IOException e) {
           IOUtils.closeStream(out);

src/java/org/apache/hadoop/fs/FileSystem.java

@@ -700,23 +700,20 @@ public abstract class FileSystem extends Configured implements Closeable {
      short replication, long blockSize, Progressable progress,
      int bytesPerChecksum) throws IOException {
 
+    boolean pathExists = exists(f);
+    CreateFlag.validate(f, pathExists, flag);
+    
     // Default impl  assumes that permissions do not matter and 
     // nor does the bytesPerChecksum  hence
     // calling the regular create is good enough.
     // FSs that implement permissions should override this.
 
-    if (exists(f)) {
+    if (pathExists && flag.contains(CreateFlag.APPEND)) {
-      if (flag.contains(CreateFlag.APPEND)) {
       return append(f, bufferSize, progress);
-      } else if (!flag.contains(CreateFlag.OVERWRITE)) {
-        throw new IOException("File already exists: " + f);
-      }
-    } else {
-      if (flag.contains(CreateFlag.APPEND) && !flag.contains(CreateFlag.CREATE))
-        throw new IOException("File already exists: " + f.toString());
     }
     
-    return this.create(f, absolutePermission, flag.contains(CreateFlag.OVERWRITE), bufferSize, replication,
+    return this.create(f, absolutePermission,
+        flag.contains(CreateFlag.OVERWRITE), bufferSize, replication,
         blockSize, progress);
   }
   

src/java/org/apache/hadoop/fs/RawLocalFileSystem.java

@@ -264,28 +264,6 @@ public class RawLocalFileSystem extends FileSystem {
     return out;
   }
   
-
-  @Override
-  protected FSDataOutputStream primitiveCreate(Path f,
-      FsPermission absolutePermission, EnumSet<CreateFlag> flag,
-      int bufferSize, short replication, long blockSize, Progressable progress,
-      int bytesPerChecksum) throws IOException {
-    
-    if(flag.contains(CreateFlag.APPEND)){
-      if (!exists(f)){
-        if(flag.contains(CreateFlag.CREATE)) {
-          return create(f, false, bufferSize, replication, blockSize, null);
-        }
-      }
-      return append(f, bufferSize, null);
-    }
- 
-    FSDataOutputStream out = create(f, flag.contains(CreateFlag.OVERWRITE),
-                                 bufferSize, replication, blockSize, progress);
-    setPermission(f, absolutePermission);
-    return out;
-  }
-
   public boolean rename(Path src, Path dst) throws IOException {
     if (pathToFile(src).renameTo(pathToFile(dst))) {
       return true;

src/test/core/org/apache/hadoop/fs/FileContextMainOperationsBaseTest.java

@@ -21,8 +21,8 @@ package org.apache.hadoop.fs;
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.util.EnumSet;
-import java.util.Iterator;
 
+import org.apache.hadoop.HadoopIllegalArgumentException;
 import org.apache.hadoop.fs.Options.CreateOpts;
 import org.apache.hadoop.fs.Options.Rename;
 import org.apache.hadoop.fs.permission.FsPermission;
@@ -32,6 +32,7 @@ import org.junit.Before;
 import org.junit.Test;
 
 import static org.apache.hadoop.fs.FileContextTestHelper.*;
+import static org.apache.hadoop.fs.CreateFlag.*;
 
 /**
  * <p>
@@ -155,7 +156,7 @@ public abstract class FileContextMainOperationsBaseTest  {
     
     // Now open a file relative to the wd we just set above.
     Path absolutePath = new Path(absoluteDir, "foo");
-    fc.create(absolutePath, EnumSet.of(CreateFlag.CREATE)).close();
+    fc.create(absolutePath, EnumSet.of(CREATE)).close();
     fc.open(new Path("foo")).close();
     
     
@@ -645,7 +646,7 @@ public abstract class FileContextMainOperationsBaseTest  {
     
     fc.mkdir(path.getParent(), FsPermission.getDefault(), true);
 
-    FSDataOutputStream out = fc.create(path, EnumSet.of(CreateFlag.CREATE),
+    FSDataOutputStream out = fc.create(path, EnumSet.of(CREATE),
         CreateOpts.repFac((short) 1), CreateOpts
             .blockSize(getDefaultBlockSize()));
     out.write(data, 0, len);
@@ -670,31 +671,93 @@ public abstract class FileContextMainOperationsBaseTest  {
 
   }
   
-  @Test
+  @Test(expected=HadoopIllegalArgumentException.class)
-  public void testOverwrite() throws IOException {
+  public void testNullCreateFlag() throws IOException {
-    Path path = getTestRootPath(fc, "test/hadoop/file");
+    Path p = getTestRootPath(fc, "test/file");
-    
+    fc.create(p, null);
-    fc.mkdir(path.getParent(), FsPermission.getDefault(), true);
+    Assert.fail("Excepted exception not thrown");
-
-    createFile(path);
-    
-    Assert.assertTrue("Exists", exists(fc, path));
-    Assert.assertEquals("Length", data.length, fc.getFileStatus(path).getLen());
-    
-    try {
-      fc.create(path, EnumSet.of(CreateFlag.CREATE));
-      Assert.fail("Should throw IOException.");
-    } catch (IOException e) {
-      // Expected
   }
   
-    FSDataOutputStream out = fc.create(path,EnumSet.of(CreateFlag.OVERWRITE));
+  @Test(expected=HadoopIllegalArgumentException.class)
+  public void testEmptyCreateFlag() throws IOException {
+    Path p = getTestRootPath(fc, "test/file");
+    fc.create(p, EnumSet.noneOf(CreateFlag.class));
+    Assert.fail("Excepted exception not thrown");
+  }
+  
+  @Test(expected=FileAlreadyExistsException.class)
+  public void testCreateFlagCreateExistingFile() throws IOException {
+    Path p = getTestRootPath(fc, "test/file");
+    createFile(p);
+    fc.create(p, EnumSet.of(CREATE));
+    Assert.fail("Excepted exception not thrown");
+  }
+  
+  @Test(expected=FileNotFoundException.class)
+  public void testCreateFlagOverwriteNonExistingFile() throws IOException {
+    Path p = getTestRootPath(fc, "test/nonExistent");
+    fc.create(p, EnumSet.of(OVERWRITE));
+    Assert.fail("Excepted exception not thrown");
+  }
+  
+  @Test
+  public void testCreateFlagOverwriteExistingFile() throws IOException {
+    Path p = getTestRootPath(fc, "test/file");
+    createFile(p);
+    FSDataOutputStream out = fc.create(p, EnumSet.of(OVERWRITE));
+    writeData(fc, p, out, data, data.length);
+  }
+  
+  @Test(expected=FileNotFoundException.class)
+  public void testCreateFlagAppendNonExistingFile() throws IOException {
+    Path p = getTestRootPath(fc, "test/nonExistent");
+    fc.create(p, EnumSet.of(APPEND));
+    Assert.fail("Excepted exception not thrown");
+  }
+  
+  @Test
+  public void testCreateFlagAppendExistingFile() throws IOException {
+    Path p = getTestRootPath(fc, "test/file");
+    createFile(p);
+    FSDataOutputStream out = fc.create(p, EnumSet.of(APPEND));
+    writeData(fc, p, out, data, 2 * data.length);
+  }
+  
+  @Test
+  public void testCreateFlagCreateAppendNonExistingFile() throws IOException {
+    Path p = getTestRootPath(fc, "test/nonExistent");
+    FSDataOutputStream out = fc.create(p, EnumSet.of(CREATE, APPEND));
+    writeData(fc, p, out, data, data.length);
+  }
+  
+  @Test
+  public void testCreateFlagCreateAppendExistingFile() throws IOException {
+    Path p = getTestRootPath(fc, "test/nonExistent");
+    createFile(p);
+    FSDataOutputStream out = fc.create(p, EnumSet.of(CREATE, APPEND));
+    writeData(fc, p, out, data, 2*data.length);
+  }
+  
+  @Test(expected=HadoopIllegalArgumentException.class)
+  public void testCreateFlagAppendOverwrite() throws IOException {
+    Path p = getTestRootPath(fc, "test/nonExistent");
+    fc.create(p, EnumSet.of(APPEND, OVERWRITE));
+    Assert.fail("Excepted exception not thrown");
+  }
+  
+  @Test(expected=HadoopIllegalArgumentException.class)
+  public void testCreateFlagAppendCreateOverwrite() throws IOException {
+    Path p = getTestRootPath(fc, "test/nonExistent");
+    fc.create(p, EnumSet.of(CREATE, APPEND, OVERWRITE));
+    Assert.fail("Excepted exception not thrown");
+  }
+  
+  private static void writeData(FileContext fc, Path p, FSDataOutputStream out,
+      byte[] data, long expectedLen) throws IOException {
     out.write(data, 0, data.length);
     out.close();
-    
+    Assert.assertTrue("Exists", exists(fc, p));
-    Assert.assertTrue("Exists", exists(fc, path));
+    Assert.assertEquals("Length", expectedLen, fc.getFileStatus(p).getLen());
-    Assert.assertEquals("Length", data.length, fc.getFileStatus(path).getLen());
-    
   }
   
   @Test
@@ -1057,7 +1120,7 @@ public abstract class FileContextMainOperationsBaseTest  {
     //HADOOP-4760 according to Closeable#close() closing already-closed 
     //streams should have no effect. 
     Path src = getTestRootPath(fc, "test/hadoop/file");
-    FSDataOutputStream out = fc.create(src, EnumSet.of(CreateFlag.CREATE),
+    FSDataOutputStream out = fc.create(src, EnumSet.of(CREATE),
             Options.CreateOpts.createParent());
     
     out.writeChar('H'); //write some data
@@ -1091,7 +1154,7 @@ public abstract class FileContextMainOperationsBaseTest  {
   }
   
   protected void createFile(Path path) throws IOException {
-    FSDataOutputStream out = fc.create(path, EnumSet.of(CreateFlag.CREATE),
+    FSDataOutputStream out = fc.create(path, EnumSet.of(CREATE),
         Options.CreateOpts.createParent());
     out.write(data, 0, data.length);
     out.close();