Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: add fs.copyFile{Sync} #15034

Merged
merged 1 commit into from
Sep 8, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,88 @@ Returns an object containing commonly used constants for file system
operations. The specific constants currently defined are described in
[FS Constants][].

## fs.copyFile(src, dest[, flags], callback)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to see a code example in the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be one.

<!-- YAML
added: REPLACEME
-->

* `src` {string|Buffer|URL} source filename to copy
* `dest` {string|Buffer|URL} destination filename of the copy operation
* `flags` {number} modifiers for copy operation. **Default:** `0`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we certain this should be the default? I don't want to see bug reports later about how Node.js made it so that their files were all screwed up... and there is a non-zero chance of abusing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. cp defaults to overwriting, as does our own fs.rename().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm aware, just makes me rather cringy. Guess I'll grit my teeth and bear it.

* `callback` {Function}

Asynchronously copies `src` to `dest`. By default, `dest` is overwritten if it
already exists. No arguments other than a possible exception are given to the
callback function. Node.js makes no guarantees about the atomicity of the copy
operation. If an error occurs after the destination file has been opened for
writing, Node.js will attempt to remove the destination.

`flags` is an optional integer that specifies the behavior
of the copy operation. The only supported flag is `fs.constants.COPYFILE_EXCL`,
which causes the copy operation to fail if `dest` already exists.

Example:

```js
const fs = require('fs');

// destination.txt will be created or overwritten by default.
fs.copyFile('source.txt', 'destination.txt', (err) => {
if (err) throw err;
console.log('source.txt was copied to destination.txt');
});
```

If the third argument is a number, then it specifies `flags`, as shown in the
following example.

```js
const fs = require('fs');
const { COPYFILE_EXCL } = fs.constants;

// By using COPYFILE_EXCL, the operation will fail if destination.txt exists.
fs.copyFile('source.txt', 'destination.txt', COPYFILE_EXCL, callback);
```

## fs.copyFileSync(src, dest[, flags])
<!-- YAML
added: REPLACEME
-->

* `src` {string|Buffer|URL} source filename to copy
* `dest` {string|Buffer|URL} destination filename of the copy operation
* `flags` {number} modifiers for copy operation. **Default:** `0`

Synchronously copies `src` to `dest`. By default, `dest` is overwritten if it
already exists. Returns `undefined`. Node.js makes no guarantees about the
atomicity of the copy operation. If an error occurs after the destination file
has been opened for writing, Node.js will attempt to remove the destination.

`flags` is an optional integer that specifies the behavior
of the copy operation. The only supported flag is `fs.constants.COPYFILE_EXCL`,
which causes the copy operation to fail if `dest` already exists.

Example:

```js
const fs = require('fs');

// destination.txt will be created or overwritten by default.
fs.copyFileSync('source.txt', 'destination.txt');
console.log('source.txt was copied to destination.txt');
```

If the third argument is a number, then it specifies `flags`, as shown in the
following example.

```js
const fs = require('fs');
const { COPYFILE_EXCL } = fs.constants;

// By using COPYFILE_EXCL, the operation will fail if destination.txt exists.
fs.copyFileSync('source.txt', 'destination.txt', COPYFILE_EXCL);
```

## fs.createReadStream(path[, options])
<!-- YAML
added: v0.1.31
Expand Down
55 changes: 55 additions & 0 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -1884,6 +1884,61 @@ fs.mkdtempSync = function(prefix, options) {
};


// Define copyFile() flags.
Object.defineProperties(fs.constants, {
COPYFILE_EXCL: { enumerable: true, value: constants.UV_FS_COPYFILE_EXCL }
});


fs.copyFile = function(src, dest, flags, callback) {
if (typeof flags === 'function') {
callback = flags;
flags = 0;
} else if (typeof callback !== 'function') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'callback', 'function');
}

src = getPathFromURL(src);

if (handleError(src, callback))
return;

if (!nullCheck(src, callback))
return;

dest = getPathFromURL(dest);

if (handleError(dest, callback))
return;

if (!nullCheck(dest, callback))
return;

src = pathModule._makeLong(src);
dest = pathModule._makeLong(dest);
flags = flags | 0;
const req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
binding.copyFile(src, dest, flags, req);
};


fs.copyFileSync = function(src, dest, flags) {
src = getPathFromURL(src);
handleError(src);
nullCheck(src);

dest = getPathFromURL(dest);
handleError(dest);
nullCheck(dest);

src = pathModule._makeLong(src);
dest = pathModule._makeLong(dest);
flags = flags | 0;
binding.copyFile(src, dest, flags);
};


var pool;

function allocNewPool(poolSize) {
Expand Down
9 changes: 4 additions & 5 deletions src/node_constants.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1164,10 +1164,6 @@ void DefineSystemConstants(Local<Object> target) {
#endif
}

void DefineUVConstants(Local<Object> target) {
NODE_DEFINE_CONSTANT(target, UV_UDP_REUSEADDR);
}

void DefineCryptoConstants(Local<Object> target) {
#if HAVE_OPENSSL
NODE_DEFINE_STRING_CONSTANT(target,
Expand Down Expand Up @@ -1274,12 +1270,15 @@ void DefineConstants(v8::Isolate* isolate, Local<Object> target) {
DefineErrnoConstants(err_constants);
DefineWindowsErrorConstants(err_constants);
DefineSignalConstants(sig_constants);
DefineUVConstants(os_constants);
DefineSystemConstants(fs_constants);
DefineOpenSSLConstants(crypto_constants);
DefineCryptoConstants(crypto_constants);
DefineZlibConstants(zlib_constants);

// Define libuv constants.
NODE_DEFINE_CONSTANT(os_constants, UV_UDP_REUSEADDR);
NODE_DEFINE_CONSTANT(fs_constants, UV_FS_COPYFILE_EXCL);

os_constants->Set(OneByteString(isolate, "errno"), err_constants);
os_constants->Set(OneByteString(isolate, "signals"), sig_constants);
target->Set(OneByteString(isolate, "os"), os_constants);
Expand Down
26 changes: 26 additions & 0 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ void After(uv_fs_t *req) {
case UV_FS_FCHMOD:
case UV_FS_CHOWN:
case UV_FS_FCHOWN:
case UV_FS_COPYFILE:
// These, however, don't.
argc = 1;
break;
Expand Down Expand Up @@ -961,6 +962,30 @@ static void Open(const FunctionCallbackInfo<Value>& args) {
}


static void CopyFile(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

if (!args[0]->IsString())
return TYPE_ERROR("src must be a string");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are already being checked on the js side, perhaps just a CHECK in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JS layer checks don't actually verify this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh.. that's right. hmmm. we should definitely fix that.
would much prefer any new errors to go through the internal/errors path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. fs.js in general needs some TLC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll see if I can schedule some time next week to work on that. The risk, of course, is that fs is one of the most-monkey-patched modules out there and we risk breaking a lot of people by being too aggressive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fs is one of the most-monkey-patched modules out there and we risk breaking a lot of people by being too aggressive.

For new methods in fs it might then make sense to be more aggressive rather than not. Personally, the more assertions the better. Keeps things sharp.

if (!args[1]->IsString())
return TYPE_ERROR("dest must be a string");
if (!args[2]->IsInt32())
return TYPE_ERROR("flags must be an int");

BufferValue src(env->isolate(), args[0]);
ASSERT_PATH(src)
BufferValue dest(env->isolate(), args[1]);
ASSERT_PATH(dest)
int flags = args[2]->Int32Value();

if (args[3]->IsObject()) {
ASYNC_DEST_CALL(copyfile, args[3], *dest, UTF8, *src, *dest, flags)
} else {
SYNC_DEST_CALL(copyfile, *src, *dest, *src, *dest, flags)
}
}


// Wrapper for write(2).
//
// bytesWritten = write(fd, buffer, offset, length, position, callback)
Expand Down Expand Up @@ -1425,6 +1450,7 @@ void InitFs(Local<Object> target,
env->SetMethod(target, "writeBuffers", WriteBuffers);
env->SetMethod(target, "writeString", WriteString);
env->SetMethod(target, "realpath", RealPath);
env->SetMethod(target, "copyFile", CopyFile);

env->SetMethod(target, "chmod", Chmod);
env->SetMethod(target, "fchmod", FChmod);
Expand Down
104 changes: 104 additions & 0 deletions test/parallel/test-fs-copyfile.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const path = require('path');
const src = path.join(common.fixturesDir, 'a.js');
const dest = path.join(common.tmpDir, 'copyfile.out');
const { COPYFILE_EXCL, UV_FS_COPYFILE_EXCL } = fs.constants;

function verify(src, dest) {
const srcData = fs.readFileSync(src, 'utf8');
const srcStat = fs.statSync(src);
const destData = fs.readFileSync(dest, 'utf8');
const destStat = fs.statSync(dest);

assert.strictEqual(srcData, destData);
assert.strictEqual(srcStat.mode, destStat.mode);
assert.strictEqual(srcStat.size, destStat.size);
}

common.refreshTmpDir();

// Verify that flags are defined.
assert.strictEqual(typeof COPYFILE_EXCL, 'number');
assert.strictEqual(typeof UV_FS_COPYFILE_EXCL, 'number');
assert.strictEqual(COPYFILE_EXCL, UV_FS_COPYFILE_EXCL);

// Verify that files are overwritten when no flags are provided.
fs.writeFileSync(dest, '', 'utf8');
const result = fs.copyFileSync(src, dest);
assert.strictEqual(result, undefined);
verify(src, dest);

// Verify that files are overwritten with default flags.
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);
fs.copyFile(src, dest, common.mustCall((err) => {
assert.ifError(err);
verify(src, dest);

// Copy asynchronously with flags.
fs.copyFile(src, dest, COPYFILE_EXCL, common.mustCall((err) => {
assert(
/^Error: EEXIST: file already exists, copyfile/.test(err.toString())
);
}));
}));

// 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'
});

// Throws if the source path is not a string.
assert.throws(() => {
fs.copyFileSync(null, dest);
}, /^TypeError: src must be a string$/);

// Throws if the source path is an invalid path.
common.expectsError(() => {
fs.copyFileSync('\u0000', dest);
}, {
code: 'ERR_INVALID_ARG_TYPE',
type: Error,
message: 'The "path" argument must be of type string without null bytes.' +
' Received type string'
});

// Throws if the destination path is not a string.
assert.throws(() => {
fs.copyFileSync(src, null);
}, /^TypeError: dest must be a string$/);

// Throws if the destination path is an invalid path.
common.expectsError(() => {
fs.copyFileSync(src, '\u0000');
}, {
code: 'ERR_INVALID_ARG_TYPE',
type: Error,
message: 'The "path" argument must be of type string without null bytes.' +
' Received type string'
});

// Errors if invalid flags are provided.
assert.throws(() => {
fs.copyFileSync(src, dest, -1);
}, /^Error: EINVAL: invalid argument, copyfile/);