Skip to content

Commit

Permalink
fs: make params in writing methods optional
Browse files Browse the repository at this point in the history
This change allows passing objects as "named parameters":
- `fs.write(fd, buffer[, options], callback)`
- `fs.writeSync(fd, buffer[, options])`
- `filehandle.write(buffer[, options])`

Fixes: nodejs/node#41666

PR-URL: nodejs/node#42601
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
  • Loading branch information
LiviaMedeiros authored and guangwong committed Oct 10, 2022
1 parent a0e8fd1 commit e7dadc8
Show file tree
Hide file tree
Showing 6 changed files with 388 additions and 13 deletions.
67 changes: 63 additions & 4 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ added: v10.0.0
Change the file system timestamps of the object referenced by the {FileHandle}
then resolves the promise with no arguments upon success.
#### `filehandle.write(buffer[, offset[, length[, position]]])`
#### `filehandle.write(buffer, offset[, length[, position]])`
<!-- YAML
added: v10.0.0
Expand All @@ -574,7 +574,7 @@ changes:
* `buffer` {Buffer|TypedArray|DataView}
* `offset` {integer} The start position from within `buffer` where the data
to write begins. **Default:** `0`
to write begins.
* `length` {integer} The number of bytes from `buffer` to write. **Default:**
`buffer.byteLength - offset`
* `position` {integer|null} The offset from the beginning of the file where the
Expand All @@ -599,6 +599,25 @@ On Linux, positional writes do not work when the file is opened in append mode.
The kernel ignores the position argument and always appends the data to
the end of the file.
#### `filehandle.write(buffer[, options])`
<!-- YAML
added: REPLACEME
-->
* `buffer` {Buffer|TypedArray|DataView}
* `options` {Object}
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.byteLength - offset`
* `position` {integer} **Default:** `null`
* Returns: {Promise}
Write `buffer` to the file.
Similar to the above `filehandle.write` function, this version takes an
optional `options` object. If no `options` object is specified, it will
default with the above values.
#### `filehandle.write(string[, position[, encoding]])`
<!-- YAML
Expand Down Expand Up @@ -4144,7 +4163,7 @@ This happens when:
* the file is deleted, followed by a restore
* the file is renamed and then renamed a second time back to its original name
### `fs.write(fd, buffer[, offset[, length[, position]]], callback)`
### `fs.write(fd, buffer, offset[, length[, position]], callback)`
<!-- YAML
added: v0.0.2
Expand Down Expand Up @@ -4206,6 +4225,29 @@ On Linux, positional writes don't work when the file is opened in append mode.
The kernel ignores the position argument and always appends the data to
the end of the file.
### `fs.write(fd, buffer[, options], callback)`
<!-- YAML
added: REPLACEME
-->
* `fd` {integer}
* `buffer` {Buffer|TypedArray|DataView}
* `options` {Object}
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.byteLength - offset`
* `position` {integer} **Default:** `null`
* `callback` {Function}
* `err` {Error}
* `bytesWritten` {integer}
* `buffer` {Buffer|TypedArray|DataView}
Write `buffer` to the file specified by `fd`.
Similar to the above `fs.write` function, this version takes an
optional `options` object. If no `options` object is specified, it will
default with the above values.
### `fs.write(fd, string[, position[, encoding]], callback)`
<!-- YAML
Expand Down Expand Up @@ -5531,7 +5573,7 @@ for more details.
For detailed information, see the documentation of the asynchronous version of
this API: [`fs.writeFile()`][].
### `fs.writeSync(fd, buffer[, offset[, length[, position]]])`
### `fs.writeSync(fd, buffer, offset[, length[, position]])`
<!-- YAML
added: v0.1.21
Expand Down Expand Up @@ -5562,6 +5604,23 @@ changes:
For detailed information, see the documentation of the asynchronous version of
this API: [`fs.write(fd, buffer...)`][].
### `fs.writeSync(fd, buffer[, options])`
<!-- YAML
added: REPLACEME
-->
* `fd` {integer}
* `buffer` {Buffer|TypedArray|DataView}
* `options` {Object}
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.byteLength - offset`
* `position` {integer} **Default:** `null`
* Returns: {number} The number of bytes written.
For detailed information, see the documentation of the asynchronous version of
this API: [`fs.write(fd, buffer...)`][].
### `fs.writeSync(fd, string[, position[, encoding]])`
<!-- YAML
Expand Down
37 changes: 29 additions & 8 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,7 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {
({
offset = 0,
length = buffer.byteLength - offset,
position = null
position = null,
} = params ?? ObjectCreate(null));
}

Expand Down Expand Up @@ -701,7 +701,7 @@ function readSync(fd, buffer, offset, length, position) {
({
offset = 0,
length = buffer.byteLength - offset,
position = null
position = null,
} = options);
}

Expand Down Expand Up @@ -797,7 +797,7 @@ function readvSync(fd, buffers, position) {
* Writes `buffer` to the specified `fd` (file descriptor).
* @param {number} fd
* @param {Buffer | TypedArray | DataView | string | object} buffer
* @param {number} [offset]
* @param {number | object} [offsetOrOptions]
* @param {number} [length]
* @param {number | null} [position]
* @param {(
Expand All @@ -807,16 +807,26 @@ function readvSync(fd, buffers, position) {
* ) => any} callback
* @returns {void}
*/
function write(fd, buffer, offset, length, position, callback) {
function write(fd, buffer, offsetOrOptions, length, position, callback) {
function wrapper(err, written) {
// Retain a reference to buffer so that it can't be GC'ed too soon.
callback(err, written || 0, buffer);
}

fd = getValidatedFd(fd);

let offset = offsetOrOptions;
if (isArrayBufferView(buffer)) {
callback = maybeCallback(callback || position || length || offset);

if (typeof offset === 'object') {
({
offset = 0,
length = buffer.byteLength - offset,
position = null,
} = offsetOrOptions ?? ObjectCreate(null));
}

if (offset == null || typeof offset === 'function') {
offset = 0;
} else {
Expand Down Expand Up @@ -862,16 +872,27 @@ ObjectDefineProperty(write, internalUtil.customPromisifyArgs,
* specified `fd` (file descriptor).
* @param {number} fd
* @param {Buffer | TypedArray | DataView | string} buffer
* @param {number} [offset]
* @param {number} [length]
* @param {number | null} [position]
* @param {{
* offset?: number;
* length?: number;
* position?: number | null;
* }} [offsetOrOptions]
* @returns {number}
*/
function writeSync(fd, buffer, offset, length, position) {
function writeSync(fd, buffer, offsetOrOptions, length, position) {
fd = getValidatedFd(fd);
const ctx = {};
let result;

let offset = offsetOrOptions;
if (isArrayBufferView(buffer)) {
if (typeof offset === 'object') {
({
offset = 0,
length = buffer.byteLength - offset,
position = null
} = offsetOrOptions ?? ObjectCreate(null));
}
if (position === undefined)
position = null;
if (offset == null) {
Expand Down
11 changes: 10 additions & 1 deletion lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -517,11 +517,20 @@ async function readv(handle, buffers, position) {
return { bytesRead, buffers };
}

async function write(handle, buffer, offset, length, position) {
async function write(handle, buffer, offsetOrOptions, length, position) {
if (buffer?.byteLength === 0)
return { bytesWritten: 0, buffer };

let offset = offsetOrOptions;
if (isArrayBufferView(buffer)) {
if (typeof offset === 'object') {
({
offset = 0,
length = buffer.byteLength - offset,
position = null
} = offsetOrOptions ?? ObjectCreate(null));
}

if (offset == null) {
offset = 0;
} else {
Expand Down
95 changes: 95 additions & 0 deletions test/parallel/test-fs-promises-write-optional-params.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
'use strict';

const common = require('../common');

// This test ensures that filehandle.write accepts "named parameters" object
// and doesn't interpret objects as strings

const assert = require('assert');
const fsPromises = require('fs').promises;
const path = require('path');
const tmpdir = require('../common/tmpdir');

tmpdir.refresh();

const dest = path.resolve(tmpdir.path, 'tmp.txt');
const buffer = Buffer.from('zyx');

async function testInvalid(dest, expectedCode, ...params) {
let fh;
try {
fh = await fsPromises.open(dest, 'w+');
await assert.rejects(
fh.write(...params),
{ code: expectedCode });
} finally {
await fh?.close();
}
}

async function testValid(dest, buffer, options) {
let fh;
try {
fh = await fsPromises.open(dest, 'w+');
const writeResult = await fh.write(buffer, options);
const writeBufCopy = Uint8Array.prototype.slice.call(writeResult.buffer);

const readResult = await fh.read(buffer, options);
const readBufCopy = Uint8Array.prototype.slice.call(readResult.buffer);

assert.ok(writeResult.bytesWritten >= readResult.bytesRead);
if (options.length !== undefined && options.length !== null) {
assert.strictEqual(writeResult.bytesWritten, options.length);
}
if (options.offset === undefined || options.offset === 0) {
assert.deepStrictEqual(writeBufCopy, readBufCopy);
}
assert.deepStrictEqual(writeResult.buffer, readResult.buffer);
} finally {
await fh?.close();
}
}

(async () => {
// Test if first argument is not wrongly interpreted as ArrayBufferView|string
for (const badBuffer of [
undefined, null, true, 42, 42n, Symbol('42'), NaN, [], () => {},
Promise.resolve(new Uint8Array(1)),
{},
{ buffer: 'amNotParam' },
{ string: 'amNotParam' },
{ buffer: new Uint8Array(1).buffer },
new Date(),
new String('notPrimitive'),
{ toString() { return 'amObject'; } },
{ [Symbol.toPrimitive]: (hint) => 'amObject' },
]) {
await testInvalid(dest, 'ERR_INVALID_ARG_TYPE', badBuffer, {});
}

// First argument (buffer or string) is mandatory
await testInvalid(dest, 'ERR_INVALID_ARG_TYPE');

// Various invalid options
await testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { length: 5 });
await testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { offset: 5 });
await testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { length: 1, offset: 3 });
await testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { length: -1 });
await testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { offset: -1 });
await testInvalid(dest, 'ERR_INVALID_ARG_TYPE', buffer, { offset: false });
await testInvalid(dest, 'ERR_INVALID_ARG_TYPE', buffer, { offset: true });

// Test compatibility with filehandle.read counterpart
for (const options of [
{},
{ length: 1 },
{ position: 5 },
{ length: 1, position: 5 },
{ length: 1, position: -1, offset: 2 },
{ length: null },
{ position: null },
{ offset: 1 },
]) {
await testValid(dest, buffer, options);
}
})().then(common.mustCall());
Loading

0 comments on commit e7dadc8

Please sign in to comment.