From 4eb45b884d9bd1f13979047750ad680275f4a348 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 16 Feb 2018 13:25:21 +0800 Subject: [PATCH] fs: throw copyFileSync errors in JS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-URL: https://github.com/nodejs/node/pull/18871 Refs: https://github.com/nodejs/node/issues/18106 Reviewed-By: Michaël Zasso Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- lib/fs.js | 7 ++- src/node_file.cc | 23 +++++--- test/parallel/test-fs-copyfile.js | 38 ++++++------- test/parallel/test-fs-error-messages.js | 74 +++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 30 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 99d63476907985..510369e44fd4af 100644 --- 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 index d73b5450daba32..00fe1f178bedc3 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1232,21 +1232,28 @@ static void OpenFileHandle(const FunctionCallbackInfo& args) { static void CopyFile(const FunctionCallbackInfo& 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()->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 index 8b910ba046ec48..21e0838148b5ef 100644 --- 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,9 +47,21 @@ 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'); + } })); })); @@ -65,9 +69,8 @@ fs.copyFile(src, dest, common.mustCall((err) => { 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 index 29056e8b669e07..6718e6fe660803 100644 --- 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 + ); +}