fs: throw copyFileSync errors in JS

PR-URL: nodejs#18871 Refs: nodejs#18106 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MayaLekova · May 8, 2018 · b896bbc · b896bbc
1 parent 769cc2c
commit b896bbc
Show file tree

Hide file tree

Showing 4 changed files with 112 additions and 30 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -1859,7 +1859,7 @@ fs.copyFile = function(src, dest, flags, callback) {
     callback = flags;
     flags = 0;
   } else if (typeof callback !== 'function') {
-    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'callback', 'Function');
+    throw new errors.TypeError('ERR_INVALID_CALLBACK');
   }
 
   src = getPathFromURL(src);
@@ -1882,10 +1882,13 @@ fs.copyFileSync = function(src, dest, flags) {
   validatePath(src, 'src');
   validatePath(dest, 'dest');
 
+  const ctx = { path: src, dest };  // non-prefixed
+
   src = pathModule._makeLong(src);
   dest = pathModule._makeLong(dest);
   flags = flags | 0;
-  binding.copyFile(src, dest, flags);
+  binding.copyFile(src, dest, flags, undefined, ctx);
+  handleErrorFromBinding(ctx);
 };
 
 

diff --git a/src/node_file.cc b/src/node_file.cc
@@ -1232,21 +1232,28 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
 static void CopyFile(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
-  CHECK_GE(args.Length(), 3);
-  CHECK(args[2]->IsInt32());
+  const int argc = args.Length();
+  CHECK_GE(argc, 3);
 
   BufferValue src(env->isolate(), args[0]);
   CHECK_NE(*src, nullptr);
+
   BufferValue dest(env->isolate(), args[1]);
   CHECK_NE(*dest, nullptr);
-  int flags = args[2]->Int32Value();
+
+  CHECK(args[2]->IsInt32());
+  const int flags = args[2].As<Int32>()->Value();
 
   FSReqBase* req_wrap = GetReqWrap(env, args[3]);
-  if (req_wrap != nullptr) {
-    AsyncCall(env, req_wrap, args, "copyfile", UTF8, AfterNoArgs,
-              uv_fs_copyfile, *src, *dest, flags);
-  } else {
-    SYNC_DEST_CALL(copyfile, *src, *dest, *src, *dest, flags)
+  if (req_wrap != nullptr) {  // copyFile(src, dest, flags, req)
+    AsyncDestCall(env, req_wrap, args, "copyfile",
+                  *dest, dest.length(), UTF8, AfterNoArgs,
+                  uv_fs_copyfile, *src, *dest, flags);
+  } else {  // copyFile(src, dest, flags, undefined, ctx)
+    CHECK_EQ(argc, 5);
+    fs_req_wrap req_wrap;
+    SyncCall(env, args[4], &req_wrap, "copyfile",
+             uv_fs_copyfile, *src, *dest, flags);
   }
 }
 

diff --git a/test/parallel/test-fs-copyfile.js b/test/parallel/test-fs-copyfile.js
@@ -4,6 +4,7 @@ const fixtures = require('../common/fixtures');
 const tmpdir = require('../common/tmpdir');
 const assert = require('assert');
 const fs = require('fs');
+const uv = process.binding('uv');
 const path = require('path');
 const src = fixtures.path('a.js');
 const dest = path.join(tmpdir.path, 'copyfile.out');
@@ -37,15 +38,6 @@ verify(src, dest);
 fs.copyFileSync(src, dest, 0);
 verify(src, dest);
 
-// Throws if destination exists and the COPYFILE_EXCL flag is provided.
-assert.throws(() => {
-  fs.copyFileSync(src, dest, COPYFILE_EXCL);
-}, /^Error: EEXIST|ENOENT:.+, copyfile/);
-
-// Throws if the source does not exist.
-assert.throws(() => {
-  fs.copyFileSync(`${src}__does_not_exist`, dest, COPYFILE_EXCL);
-}, /^Error: ENOENT: no such file or directory, copyfile/);
 
 // Copies asynchronously.
 fs.unlinkSync(dest);
@@ -55,19 +47,30 @@ fs.copyFile(src, dest, common.mustCall((err) => {
 
   // Copy asynchronously with flags.
   fs.copyFile(src, dest, COPYFILE_EXCL, common.mustCall((err) => {
-    assert(
-      /^Error: EEXIST: file already exists, copyfile/.test(err.toString())
-    );
+    if (err.code === 'ENOENT') {  // Could be ENOENT or EEXIST
+      assert.strictEqual(err.message,
+                         'ENOENT: no such file or directory, copyfile ' +
+                         `'${src}' -> '${dest}'`);
+      assert.strictEqual(err.errno, uv.UV_ENOENT);
+      assert.strictEqual(err.code, 'ENOENT');
+      assert.strictEqual(err.syscall, 'copyfile');
+    } else {
+      assert.strictEqual(err.message,
+                         'EEXIST: file already exists, copyfile ' +
+                         `'${src}' -> '${dest}'`);
+      assert.strictEqual(err.errno, uv.UV_EEXIST);
+      assert.strictEqual(err.code, 'EEXIST');
+      assert.strictEqual(err.syscall, 'copyfile');
+    }
   }));
 }));
 
 // Throws if callback is not a function.
 common.expectsError(() => {
   fs.copyFile(src, dest, 0, 0);
 }, {
-  code: 'ERR_INVALID_ARG_TYPE',
-  type: TypeError,
-  message: 'The "callback" argument must be of type Function'
+  code: 'ERR_INVALID_CALLBACK',
+  type: TypeError
 });
 
 // Throws if the source path is not a string.
@@ -101,8 +104,3 @@ common.expectsError(() => {
     }
   );
 });
-
-// Errors if invalid flags are provided.
-assert.throws(() => {
-  fs.copyFileSync(src, dest, -1);
-}, /^Error: EINVAL: invalid argument, copyfile/);
diff --git a/test/parallel/test-fs-error-messages.js b/test/parallel/test-fs-error-messages.js
@@ -30,6 +30,7 @@ const existingFile = fixtures.path('exit.js');
 const existingFile2 = fixtures.path('create-file.js');
 const existingDir = fixtures.path('empty');
 const existingDir2 = fixtures.path('keys');
+const { COPYFILE_EXCL } = fs.constants;
 const uv = process.binding('uv');
 
 // Template tag function for escaping special characters in strings so that:
@@ -634,3 +635,76 @@ if (!common.isAIX) {
     validateError
   );
 }
+
+// copyFile with invalid flags
+{
+  const validateError = (err) => {
+    assert.strictEqual(err.message,
+                       'EINVAL: invalid argument, copyfile ' +
+                       `'${existingFile}' -> '${nonexistentFile}'`);
+    assert.strictEqual(err.errno, uv.UV_EINVAL);
+    assert.strictEqual(err.code, 'EINVAL');
+    assert.strictEqual(err.syscall, 'copyfile');
+    return true;
+  };
+
+  // TODO(joyeecheung): test fs.copyFile() when uv_fs_copyfile does not
+  // keep the loop open when the flags are invalid.
+  // See https://github.com/libuv/libuv/pull/1747
+
+  assert.throws(
+    () => fs.copyFileSync(existingFile, nonexistentFile, -1),
+    validateError
+  );
+}
+
+// copyFile: destination exists but the COPYFILE_EXCL flag is provided.
+{
+  const validateError = (err) => {
+    if (err.code === 'ENOENT') {  // Could be ENOENT or EEXIST
+      assert.strictEqual(err.message,
+                         'ENOENT: no such file or directory, copyfile ' +
+                         `'${existingFile}' -> '${existingFile2}'`);
+      assert.strictEqual(err.errno, uv.UV_ENOENT);
+      assert.strictEqual(err.code, 'ENOENT');
+      assert.strictEqual(err.syscall, 'copyfile');
+    } else {
+      assert.strictEqual(err.message,
+                         'EEXIST: file already exists, copyfile ' +
+                         `'${existingFile}' -> '${existingFile2}'`);
+      assert.strictEqual(err.errno, uv.UV_EEXIST);
+      assert.strictEqual(err.code, 'EEXIST');
+      assert.strictEqual(err.syscall, 'copyfile');
+    }
+    return true;
+  };
+
+  fs.copyFile(existingFile, existingFile2, COPYFILE_EXCL,
+              common.mustCall(validateError));
+
+  assert.throws(
+    () => fs.copyFileSync(existingFile, existingFile2, COPYFILE_EXCL),
+    validateError
+  );
+}
+
+// copyFile: the source does not exist.
+{
+  const validateError = (err) => {
+    assert.strictEqual(err.message,
+                       'ENOENT: no such file or directory, copyfile ' +
+                       `'${nonexistentFile}' -> '${existingFile2}'`);
+    assert.strictEqual(err.errno, uv.UV_ENOENT);
+    assert.strictEqual(err.code, 'ENOENT');
+    assert.strictEqual(err.syscall, 'copyfile');
+    return true;
+  };
+
+  fs.copyFile(nonexistentFile, existingFile2, COPYFILE_EXCL,
+              common.mustCall(validateError));
+
+  assert.throws(
+    () => fs.copyFileSync(nonexistentFile, existingFile2, COPYFILE_EXCL),
+    validateError
+  );
+}