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: remove coercion to string in writing methods #42796

Merged
merged 4 commits into from Sep 12, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion doc/api/deprecations.md
Expand Up @@ -3131,6 +3131,9 @@ resources and not the actual references.

<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/42796
description: End-of-Life.
- version: v18.0.0
pr-url: https://github.com/nodejs/node/pull/42607
description: Runtime deprecation.
Expand All @@ -3141,7 +3144,7 @@ changes:
description: Documentation-only deprecation.
-->

Type: Runtime
Type: End-of-Life

Implicit coercion of objects with own `toString` property, passed as second
parameter in [`fs.write()`][], [`fs.writeFile()`][], [`fs.appendFile()`][],
Expand Down
22 changes: 17 additions & 5 deletions doc/api/fs.md
Expand Up @@ -4534,6 +4534,10 @@ default with the above values.
<!-- YAML
added: v0.11.5
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/42796
description: Passing to the `string` parameter an object with an own
`toString` function is no longer supported.
- version: v17.8.0
pr-url: https://github.com/nodejs/node/pull/42149
description: Passing to the `string` parameter an object with an own
Expand All @@ -4560,16 +4564,16 @@ changes:
-->

* `fd` {integer}
* `string` {string|Object}
* `string` {string}
* `position` {integer|null} **Default:** `null`
* `encoding` {string} **Default:** `'utf8'`
* `callback` {Function}
* `err` {Error}
* `written` {integer}
* `string` {string}

Write `string` to the file specified by `fd`. If `string` is not a string, or an
object with an own `toString` function property, then an exception is thrown.
Write `string` to the file specified by `fd`. If `string` is not a string,
an exception is thrown.

`position` refers to the offset from the beginning of the file where this data
should be written. If `typeof position !== 'number'` the data will be written at
Expand Down Expand Up @@ -4602,6 +4606,10 @@ details.
<!-- YAML
added: v0.1.29
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/42796
description: Passing to the `string` parameter an object with an own
`toString` function is no longer supported.
- version: v18.0.0
pr-url: https://github.com/nodejs/node/pull/41678
description: Passing an invalid callback to the `callback` argument
Expand Down Expand Up @@ -4650,7 +4658,7 @@ changes:
-->

* `file` {string|Buffer|URL|integer} filename or file descriptor
* `data` {string|Buffer|TypedArray|DataView|Object}
* `data` {string|Buffer|TypedArray|DataView}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
Expand Down Expand Up @@ -5840,6 +5848,10 @@ this API: [`fs.utimes()`][].
<!-- YAML
added: v0.1.29
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/42796
description: Passing to the `data` parameter an object with an own
`toString` function is no longer supported.
- version: v17.8.0
pr-url: https://github.com/nodejs/node/pull/42149
description: Passing to the `data` parameter an object with an own
Expand All @@ -5865,7 +5877,7 @@ changes:
-->

* `file` {string|Buffer|URL|integer} filename or file descriptor
* `data` {string|Buffer|TypedArray|DataView|Object}
* `data` {string|Buffer|TypedArray|DataView}
* `options` {Object|string}
* `encoding` {string|null} **Default:** `'utf8'`
* `mode` {integer} **Default:** `0o666`
Expand Down
31 changes: 7 additions & 24 deletions lib/fs.js
Expand Up @@ -39,7 +39,6 @@ const {
ReflectApply,
SafeMap,
SafeSet,
String,
StringPrototypeCharCodeAt,
StringPrototypeIndexOf,
StringPrototypeSlice,
Expand Down Expand Up @@ -84,7 +83,6 @@ const { FSReqCallback } = binding;
const { toPathIfFileURL } = require('internal/url');
const {
customPromisifyArgs: kCustomPromisifyArgsSymbol,
deprecate,
kEmptyObject,
promisify: {
custom: kCustomPromisifiedSymbol,
Expand Down Expand Up @@ -123,7 +121,6 @@ const {
validateRmOptionsSync,
validateRmdirOptions,
validateStringAfterArrayBufferView,
validatePrimitiveStringAfterArrayBufferView,
warnOnNonPortableTemplate
} = require('internal/fs/utils');
const {
Expand Down Expand Up @@ -171,11 +168,6 @@ const isWindows = process.platform === 'win32';
const isOSX = process.platform === 'darwin';


const showStringCoercionDeprecation = deprecate(
() => {},
'Implicit coercion of objects with own toString property is deprecated.',
'DEP0162'
);
function showTruncateDeprecation() {
if (truncateWarn) {
process.emitWarning(
Expand Down Expand Up @@ -808,7 +800,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 {Buffer | TypedArray | DataView | string} buffer
* @param {number | object} [offsetOrOptions]
* @param {number} [length]
* @param {number | null} [position]
Expand Down Expand Up @@ -856,9 +848,6 @@ function write(fd, buffer, offsetOrOptions, length, position, callback) {
}

validateStringAfterArrayBufferView(buffer, 'buffer');
if (typeof buffer !== 'string') {
showStringCoercionDeprecation();
}

if (typeof position !== 'function') {
if (typeof offset === 'function') {
Expand All @@ -870,7 +859,7 @@ function write(fd, buffer, offsetOrOptions, length, position, callback) {
length = 'utf8';
}

const str = String(buffer);
const str = buffer;
validateEncoding(str, length);
callback = maybeCallback(position);

Expand Down Expand Up @@ -921,7 +910,7 @@ function writeSync(fd, buffer, offsetOrOptions, length, position) {
result = binding.writeBuffer(fd, buffer, offset, length, position,
undefined, ctx);
} else {
validatePrimitiveStringAfterArrayBufferView(buffer, 'buffer');
validateStringAfterArrayBufferView(buffer, 'buffer');
validateEncoding(buffer, length);

if (offset === undefined)
Expand Down Expand Up @@ -2149,7 +2138,7 @@ function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
/**
* Asynchronously writes data to the file.
* @param {string | Buffer | URL | number} path
* @param {string | Buffer | TypedArray | DataView | object} data
* @param {string | Buffer | TypedArray | DataView} data
* @param {{
* encoding?: string | null;
* mode?: number;
Expand All @@ -2166,10 +2155,7 @@ function writeFile(path, data, options, callback) {

if (!isArrayBufferView(data)) {
validateStringAfterArrayBufferView(data, 'data');
if (typeof data !== 'string') {
showStringCoercionDeprecation();
}
data = Buffer.from(String(data), options.encoding || 'utf8');
data = Buffer.from(data, options.encoding || 'utf8');
}

if (isFd(path)) {
Expand All @@ -2196,7 +2182,7 @@ function writeFile(path, data, options, callback) {
/**
* Synchronously writes data to the file.
* @param {string | Buffer | URL | number} path
* @param {string | Buffer | TypedArray | DataView | object} data
* @param {string | Buffer | TypedArray | DataView} data
* @param {{
* encoding?: string | null;
* mode?: number;
Expand All @@ -2209,10 +2195,7 @@ function writeFileSync(path, data, options) {

if (!isArrayBufferView(data)) {
validateStringAfterArrayBufferView(data, 'data');
if (typeof data !== 'string') {
showStringCoercionDeprecation();
}
data = Buffer.from(String(data), options.encoding || 'utf8');
data = Buffer.from(data, options.encoding || 'utf8');
}

const flag = options.flag || 'w';
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/fs/promises.js
Expand Up @@ -66,7 +66,7 @@ const {
validateOffsetLengthWrite,
validateRmOptions,
validateRmdirOptions,
validatePrimitiveStringAfterArrayBufferView,
validateStringAfterArrayBufferView,
warnOnNonPortableTemplate,
} = require('internal/fs/utils');
const { opendir } = require('internal/fs/dir');
Expand Down Expand Up @@ -604,7 +604,7 @@ async function write(handle, buffer, offsetOrOptions, length, position) {
return { bytesWritten, buffer };
}

validatePrimitiveStringAfterArrayBufferView(buffer, 'buffer');
validateStringAfterArrayBufferView(buffer, 'buffer');
validateEncoding(buffer, length);
const bytesWritten = (await binding.writeString(handle.fd, buffer, offset,
length, kUsePromises)) || 0;
Expand Down Expand Up @@ -834,7 +834,7 @@ async function writeFile(path, data, options) {
const flag = options.flag || 'w';

if (!isArrayBufferView(data) && !isCustomIterable(data)) {
validatePrimitiveStringAfterArrayBufferView(data, 'data');
validateStringAfterArrayBufferView(data, 'data');
data = Buffer.from(data, options.encoding || 'utf8');
}

Expand Down
23 changes: 0 additions & 23 deletions lib/internal/fs/utils.js
Expand Up @@ -14,7 +14,6 @@ const {
MathMin,
MathRound,
ObjectIs,
ObjectPrototypeHasOwnProperty,
ObjectSetPrototypeOf,
ReflectApply,
ReflectOwnKeys,
Expand Down Expand Up @@ -874,27 +873,6 @@ const getValidMode = hideStackFrames((mode, type) => {
});

const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
if (typeof buffer === 'string') {
return;
}

if (
typeof buffer === 'object' &&
buffer !== null &&
typeof buffer.toString === 'function' &&
ObjectPrototypeHasOwnProperty(buffer, 'toString')
) {
return;
}

throw new ERR_INVALID_ARG_TYPE(
name,
['string', 'Buffer', 'TypedArray', 'DataView'],
buffer
);
});

const validatePrimitiveStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
if (typeof buffer !== 'string') {
throw new ERR_INVALID_ARG_TYPE(
name,
Expand Down Expand Up @@ -958,6 +936,5 @@ module.exports = {
validateRmOptionsSync,
validateRmdirOptions,
validateStringAfterArrayBufferView,
validatePrimitiveStringAfterArrayBufferView,
warnOnNonPortableTemplate
};
30 changes: 15 additions & 15 deletions test/parallel/test-fs-write-file-sync.js
Expand Up @@ -102,20 +102,20 @@ tmpdir.refresh();
assert.strictEqual(content, 'hello world!');
}

// Test writeFileSync with an object with an own toString function
// Test writeFileSync with an invalid input
{
// Runtime deprecated by DEP0162
common.expectWarning('DeprecationWarning',
'Implicit coercion of objects with own toString property is deprecated.',
'DEP0162');
const file = path.join(tmpdir.path, 'testWriteFileSyncStringify.txt');
const data = {
toString() {
return 'hello world!';
}
};

fs.writeFileSync(file, data, { encoding: 'utf8', flag: 'a' });
const content = fs.readFileSync(file, { encoding: 'utf8' });
assert.strictEqual(content, String(data));
const file = path.join(tmpdir.path, 'testWriteFileSyncInvalid.txt');
for (const data of [
false, 5, {}, [], null, undefined, true, 5n, () => {}, Symbol(), new Map(),
new String('notPrimitive'),
{ [Symbol.toPrimitive]: (hint) => 'amObject' },
{ toString() { return 'amObject'; } },
Promise.resolve('amPromise'),
common.mustNotCall(),
]) {
assert.throws(
() => fs.writeFileSync(file, data, { encoding: 'utf8', flag: 'a' }),
{ code: 'ERR_INVALID_ARG_TYPE' }
);
}
}
25 changes: 5 additions & 20 deletions test/parallel/test-fs-write.js
Expand Up @@ -33,7 +33,6 @@ const fn = path.join(tmpdir.path, 'write.txt');
const fn2 = path.join(tmpdir.path, 'write2.txt');
const fn3 = path.join(tmpdir.path, 'write3.txt');
const fn4 = path.join(tmpdir.path, 'write4.txt');
const fn5 = path.join(tmpdir.path, 'write5.txt');
const expected = 'ümlaut.';
const constants = fs.constants;

Expand Down Expand Up @@ -127,23 +126,6 @@ fs.open(fn3, 'w', 0o644, common.mustSucceed((fd) => {
}));


// Test write with an object with an own toString function
// Runtime deprecated by DEP0162
common.expectWarning('DeprecationWarning',
'Implicit coercion of objects with own toString property is deprecated.',
'DEP0162');
fs.open(fn4, 'w', 0o644, common.mustSucceed((fd) => {
const done = common.mustSucceed((written) => {
assert.strictEqual(written, Buffer.byteLength(expected));
fs.closeSync(fd);
});

const data = {
toString() { return expected; }
};
fs.write(fd, data, done);
}));

[false, 'test', {}, [], null, undefined].forEach((i) => {
assert.throws(
() => fs.write(i, common.mustNotCall()),
Expand All @@ -162,9 +144,12 @@ fs.open(fn4, 'w', 0o644, common.mustSucceed((fd) => {
});

[
false, 5, {}, [], null, undefined,
false, 5, {}, [], null, undefined, true, 5n, () => {}, Symbol(), new Map(),
new String('notPrimitive'),
{ [Symbol.toPrimitive]: (hint) => 'amObject' },
{ toString() { return 'amObject'; } },
Promise.resolve('amPromise'),
common.mustNotCall(),
].forEach((data) => {
assert.throws(
() => fs.write(1, data, common.mustNotCall()),
Expand All @@ -184,7 +169,7 @@ fs.open(fn4, 'w', 0o644, common.mustSucceed((fd) => {

{
// Regression test for https://github.com/nodejs/node/issues/38168
const fd = fs.openSync(fn5, 'w');
const fd = fs.openSync(fn4, 'w');

assert.throws(
() => fs.writeSync(fd, 'abc', 0, 'hex'),
Expand Down