Bug 796953 - [OS.File] writeAtomic should accept an option noOverwrite. r=Yoric
authorAndres Hernandez <andres@appcoast.com>
Tue, 09 Oct 2012 12:10:06 -0600
changeset 109860 268128a99ac91d12f4fad24f7d93d134ba44a1c1
parent 109859 ab04b3c23f0d06bc7875634a7476fcdc42fe4a58
child 109861 3bafdb575a2efcf893a73435f68ecf4076f799b5
push id23657
push useremorley@mozilla.com
push dateThu, 11 Oct 2012 13:47:00 +0000
treeherdermozilla-central@2fae8bd461da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersYoric
bugs796953
milestone19.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 796953 - [OS.File] writeAtomic should accept an option noOverwrite. r=Yoric
toolkit/components/osfile/osfile_async_front.jsm
toolkit/components/osfile/osfile_shared_front.jsm
toolkit/components/osfile/osfile_unix_allthreads.jsm
toolkit/components/osfile/osfile_unix_front.jsm
toolkit/components/osfile/osfile_win_allthreads.jsm
toolkit/components/osfile/osfile_win_front.jsm
toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
--- a/toolkit/components/osfile/osfile_async_front.jsm
+++ b/toolkit/components/osfile/osfile_async_front.jsm
@@ -630,16 +630,18 @@ File.read = function read(path, bytes) {
  *
  * @param {string} path The path of the file to modify.
  * @param {Typed Array | C pointer} buffer A buffer containing the bytes to write.
  * @param {*=} options Optionally, an object determining the behavior
  * of this function. This object may contain the following fields:
  * - {number} bytes The number of bytes to write. If unspecified,
  * |buffer.byteLength|. Required if |buffer| is a C pointer.
  * - {string} tmpPath The path at which to write the temporary file.
+ * - {bool} noOverwrite - If set, this function will fail if a file already
+ * exists at |path|. The |tmpPath| is not overwritten if |path| exist.
  *
  * @return {promise}
  * @resolves {number} The number of bytes actually written.
  */
 File.writeAtomic = function writeAtomic(path, buffer, options) {
   // Copy |options| to avoid modifying the original object
   options = clone(options || noOptions);
   // As options.tmpPath is a path, we need to encode it as |Type.path| message
--- a/toolkit/components/osfile/osfile_shared_front.jsm
+++ b/toolkit/components/osfile/osfile_shared_front.jsm
@@ -303,43 +303,68 @@ AbstractFile.read = function read(path, 
   try {
     return file.read(bytes);
   } finally {
     file.close();
   }
 };
 
 /**
+ * Find outs if a file exists.
+ *
+ * @param {string} path The path to the file.
+ *
+ * @return {bool} true if the file exists, false otherwise.
+ */
+AbstractFile.exists = function exists(path) {
+  try {
+    let file = exports.OS.File.open(path);
+    file.close();
+    return true;
+  } catch (x) {
+    return false;
+  }
+};
+
+/**
  * Write a file, atomically.
  *
  * By opposition to a regular |write|, this operation ensures that,
  * until the contents are fully written, the destination file is
  * not modified.
  *
  * Important note: In the current implementation, option |tmpPath|
  * is required. This requirement should disappear as part of bug 793660.
  *
  * @param {string} path The path of the file to modify.
  * @param {Typed Array | C pointer} buffer A buffer containing the bytes to write.
  * @param {*=} options Optionally, an object determining the behavior
  * of this function. This object may contain the following fields:
  * - {number} bytes The number of bytes to write. If unspecified,
  * |buffer.byteLength|. Required if |buffer| is a C pointer.
  * - {string} tmpPath The path at which to write the temporary file.
+ * - {bool} noOverwrite - If set, this function will fail if a file already
+ * exists at |path|. The |tmpPath| is not overwritten if |path| exist.
  *
  * @return {number} The number of bytes actually written.
  */
 AbstractFile.writeAtomic =
      function writeAtomic(path, buffer, options) {
   options = options || noOptions;
 
   let tmpPath = options.tmpPath;
   if (!tmpPath) {
     throw new TypeError("Expected option tmpPath");
   }
+
+  let noOverwrite = options.noOverwrite;
+  if (noOverwrite && OS.File.exists(path)) {
+    throw OS.File.Error.exists("writeAtomic");
+  }
+
   let tmpFile = OS.File.open(tmpPath, {write: true, truncate: true});
   let bytesWritten;
   try {
     bytesWritten = tmpFile.write(buffer, options);
     tmpFile.flush();
   } catch (x) {
     OS.File.remove(tmpPath);
     throw x;
--- a/toolkit/components/osfile/osfile_unix_allthreads.jsm
+++ b/toolkit/components/osfile/osfile_unix_allthreads.jsm
@@ -180,9 +180,13 @@ if (typeof Components != "undefined") {
     */
   Types.path = Types.cstring.withName("[in] path");
   Types.out_path = Types.out_cstring.withName("[out] path");
 
   // Special constructors that need to be defined on all threads
   OSError.closed = function closed(operation) {
     return new OSError(operation, OS.Constants.libc.EBADF);
   };
+
+  OSError.exists = function exists(operation) {
+    return new OSError(operation, OS.Constants.libc.EEXIST);
+  };
 })(this);
--- a/toolkit/components/osfile/osfile_unix_front.jsm
+++ b/toolkit/components/osfile/osfile_unix_front.jsm
@@ -823,16 +823,17 @@
          throw_on_negative("stat", UnixFile.lstat(path, gStatDataPtr));
        } else {
          throw_on_negative("stat", UnixFile.stat(path, gStatDataPtr));
        }
        return new File.Info(gStatData);
      };
 
      File.read = exports.OS.Shared.AbstractFile.read;
+     File.exists = exports.OS.Shared.AbstractFile.exists;
      File.writeAtomic = exports.OS.Shared.AbstractFile.writeAtomic;
 
      /**
       * Get/set the current directory.
       */
      Object.defineProperty(File, "curDir", {
          set: function(path) {
            throw_on_negative("curDir",
--- a/toolkit/components/osfile/osfile_win_allthreads.jsm
+++ b/toolkit/components/osfile/osfile_win_allthreads.jsm
@@ -189,9 +189,12 @@ if (typeof Components != "undefined") {
   Types.path = Types.wstring.withName("[in] path");
   Types.out_path = Types.out_wstring.withName("[out] path");
 
   // Special constructors that need to be defined on all threads
   OSError.closed = function closed(operation) {
     return new OSError(operation, exports.OS.Constants.Win.INVALID_HANDLE_VALUE);
   };
 
+  OSError.exists = function exists(operation) {
+    return new OSError(operation, exports.OS.Constants.Win.ERROR_FILE_EXISTS);
+  };
 })(this);
--- a/toolkit/components/osfile/osfile_win_front.jsm
+++ b/toolkit/components/osfile/osfile_win_front.jsm
@@ -817,16 +817,17 @@
        // Directories can be opened neither for reading(!) nor for writing
        winAccess: 0,
        // Directories can only be opened with backup semantics(!)
        winFlags: OS.Constants.Win.FILE_FLAG_BACKUP_SEMANTICS,
        winDisposition: OS.Constants.Win.OPEN_EXISTING
      };
 
      File.read = exports.OS.Shared.AbstractFile.read;
+     File.exists = exports.OS.Shared.AbstractFile.exists;
      File.writeAtomic = exports.OS.Shared.AbstractFile.writeAtomic;
 
      /**
       * Get/set the current directory.
       */
      Object.defineProperty(File, "curDir", {
          set: function(path) {
            throw_on_zero("set curDir",
--- a/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
+++ b/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ -515,16 +515,46 @@ let test_read_write_all = maketest(
     promise = promise.then(
       function compare_complete() {
         test.info("Compare complete");
         test.ok(!(new FileUtils.File(tmpPath).exists()), "Temporary file was removed");
       }
     );
     promise = ensureSuccess(promise, test);
 
+// Check that writeAtomic fails if noOverwrite is true and the destination file already exists!
+
+    promise = promise.then(
+      function check_with_noOverwrite() {
+        let view = new Uint8Array(contents.buffer, 10, 200);
+        options = {tmpPath: tmpPath, noOverwrite: true};
+        return OS.File.writeAtomic(pathDest, view, options);
+      }
+    );
+
+    promise = promise.then(
+      function onSuccess() {
+        test.fail("With noOverwrite, writeAtomic should have refused to overwrite file");
+      },
+      function onFailure(err) {
+        test.info("With noOverwrite, writeAtomic correctly failed");
+        test.ok(err instanceof OS.File.Error, "writeAtomic correctly failed with a file error");
+        test.ok(err.becauseExists, "writeAtomic file error confirmed that the file already exists");
+        return reference_compare_files(pathSource, pathDest, test);
+      }
+    );
+
+    promise = promise.then(
+      function compare_complete() {
+        test.info("With noOverwrite, writeAtomic correctly did not overwrite destination file");
+        test.ok(!(new FileUtils.File(tmpPath).exists()), "Temporary file was removed");
+      }
+    );
+    promise = ensureSuccess(promise, test);
+
 // Now write a subset
 
     let START = 10;
     let LENGTH = 100;
     promise = promise.then(
       function() {
         let view = new Uint8Array(contents.buffer, START, LENGTH);
         return OS.File.writeAtomic(pathDest, view, {tmpPath: tmpPath});
@@ -565,17 +595,19 @@ let test_read_write_all = maketest(
     );
 
     promise = promise.then(
       function onSuccess() {
         test.fail("Without a tmpPath, writeAtomic should have failed");
       },
       function onFailure() {
         test.ok("Without a tmpPath, writeAtomic has failed as expected");
-      });
+      }
+    );
+
     return promise;
   }
 );
 
 let test_position = maketest(
   "position",
   function position(test){
 
--- a/toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
+++ b/toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ -350,27 +350,41 @@ function test_readall_writeall_file()
   dest.setPosition(1234);
   dest.close();
 
   OS.File.writeAtomic(tmp_file_name, readResult,
     {tmpPath: tmp_file_name + ".tmp"});
   compare_files("test_readall_writeall_file (OS.File.readAll + writeAtomic 2)",
                 src_file_name, tmp_file_name);
 
+  // File.writeAtomic on top of existing file but without overwritten the file
+  exn = null;
+  try {
+    let view = new Uint8Array(readResult.buffer, 10, 200);
+    OS.File.writeAtomic(tmp_file_name, view,
+      { tmpPath: tmp_file_name + ".tmp", noOverwrite: true});
+  } catch (x) {
+    exn = x;
+  }
+  ok(exn && exn instanceof OS.File.Error && exn.becauseExists, "writeAtomic fails if file already exists with noOverwrite option");
+  // Check file was not overwritten.
+  compare_files("test_readall_writeall_file (OS.File.readAll + writeAtomic check file was not overwritten)",
+                src_file_name, tmp_file_name);
+
   // Ensure that File.writeAtomic fails if no temporary file name is provided
   // (FIXME: Remove this test as part of bug 793660)
 
   exn = null;
   try {
     OS.File.writeAtomic(tmp_file_name, readResult.buffer,
       {bytes: readResult.length});
   } catch (x) {
     exn = x;
   }
-  ok(!!exn && exn instanceof TypeError, "wrietAtomic fails if tmpPath is not provided");
+  ok(!!exn && exn instanceof TypeError, "writeAtomic fails if tmpPath is not provided");
 }
 
 /**
  * Test that copying a file using |copy| works.
  */
 function test_copy_existing_file()
 {
   let src_file_name = "chrome/toolkit/components/osfile/tests/mochi/worker_test_osfile_unix.js";