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: check argument type on createStream method #1845

Closed
wants to merge 1 commit into from
Closed
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
6 changes: 4 additions & 2 deletions doc/api/fs.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -786,7 +786,7 @@ on Unix systems, it never was.

Returns a new ReadStream object (See `Readable Stream`).

`options` is an object with the following defaults:
`options` is an object or string with the following defaults:

{ flags: 'r',
encoding: null,
Expand All @@ -812,6 +812,7 @@ An example to read the last 10 bytes of a file which is 100 bytes long:

fs.createReadStream('sample.txt', {start: 90, end: 99});

If `options` is a string, then it specifies the encoding.

## Class: fs.ReadStream

Expand All @@ -828,7 +829,7 @@ Emitted when the ReadStream's file is opened.

Returns a new WriteStream object (See `Writable Stream`).

`options` is an object with the following defaults:
`options` is an object or string with the following defaults:

{ flags: 'w',
encoding: null,
Expand All @@ -844,6 +845,7 @@ Like `ReadStream` above, if `fd` is specified, `WriteStream` will ignore the
`path` argument and will use the specified file descriptor. This means that no
`open` event will be emitted.

If `options` is a string, then it specifies the encoding.

## Class: fs.WriteStream

Expand Down
18 changes: 16 additions & 2 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -1617,8 +1617,15 @@ function ReadStream(path, options) {
if (!(this instanceof ReadStream))
return new ReadStream(path, options);

if (options === undefined)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps use loose equality here to grab null also? It'll avoid an Object.create(null) call too.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to change the check on line 1624 to else if (options === null || typeof options !== 'object')

Copy link
Member Author

Choose a reason for hiding this comment

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

latest node and io.js accept null on fs.createReadStream.
current pull request does not change the behavior on fs.createReadStream, accepts string and throws proper exception.

if we don't accept null, this api may break the compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

Current versions accept null and any other falsey value so this is still a breaking change. If this were to land, it would make sense to throw on null, since this is already a breaking change and null is not a useful value to support here. This needs to be given a lot of consideration, especially after the events of this past weekend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this already a breaking change (disregarding null)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because fs.createReadStream() currently allows any falsey value (false, NaN, etc.). Under this PR, passing in false, for example, would throw.

Copy link
Contributor

Choose a reason for hiding this comment

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

options = options || {} used to allow other falsy values as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

hm. OK. I will disallow null.

our current spec is to allow object only.
And under this PR, we can use string as an encoding.

So, this function allows object and string. the other types are not allowed.
If we found disallowed type, we should throw TypeError.

But null is unclear type.
I think null and undefined are not easy to use properly for beginner.
So my first impression, we should allow null just like undefined.
But I will follow @cjihrig suggestion. his spec is clear.

options = {};
else if (typeof options === 'string')
options = { encoding: options };
else if (options === null || typeof options !== 'object')
Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

throw new TypeError('options must be a string or an object');

// a little bit bigger buffer and water marks by default
options = Object.create(options || {});
options = Object.create(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that null throws, should we do this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Your concern is Object.create(null); throws null exception?
Object.create(null) does not throw null exception, and I prevent null on line 1624.

If you concern about the backward compatible, we discussed here (#1845 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, now that we don't allow null, why do we need Object.create? options will be an object already.

Copy link
Member Author

Choose a reason for hiding this comment

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

Object.create is proposed from #635 .
According to the PR, Object.create let options inheritable properly.

if (options.highWaterMark === undefined)
options.highWaterMark = 64 * 1024;

Expand Down Expand Up @@ -1783,7 +1790,14 @@ function WriteStream(path, options) {
if (!(this instanceof WriteStream))
return new WriteStream(path, options);

options = options || {};
if (options === undefined)
options = {};
else if (typeof options === 'string')
options = { encoding: options };
else if (options === null || typeof options !== 'object')
throw new TypeError('options must be a string or an object');

options = Object.create(options);

Writable.call(this, options);

Expand Down
17 changes: 17 additions & 0 deletions test/parallel/test-fs-read-stream-encoding.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const path = require('path');
const stream = require('stream');
const encoding = 'base64';

const example = path.join(common.fixturesDir, 'x.txt');
const assertStream = new stream.Writable({
write: function(chunk, enc, next) {
const expected = new Buffer('xyz');
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to the '\n' at the end of the file?

Copy link
Member Author

Choose a reason for hiding this comment

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

passed.
I guess if I choose 'utf8' as encoding type, I should change 'xyz\n\n';

assert(chunk.equals(expected));
}
});
assertStream.setDefaultEncoding(encoding);
fs.createReadStream(example, encoding).pipe(assertStream);
33 changes: 33 additions & 0 deletions test/parallel/test-fs-read-stream-throw-type-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const path = require('path');

Copy link
Contributor

Choose a reason for hiding this comment

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

These tests should probably check for null. It might be a good idea to also verify that string, object, and undefined do not throw.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. that is good. I added some tests.

const example = path.join(common.fixturesDir, 'x.txt');

assert.doesNotThrow(function() {
fs.createReadStream(example, undefined);
});
assert.doesNotThrow(function() {
fs.createReadStream(example, 'utf8');
});
assert.doesNotThrow(function() {
fs.createReadStream(example, {encoding: 'utf8'});
});

assert.throws(function() {
fs.createReadStream(example, null);
}, /options must be a string or an object/);
assert.throws(function() {
fs.createReadStream(example, 123);
}, /options must be a string or an object/);
assert.throws(function() {
fs.createReadStream(example, 0);
}, /options must be a string or an object/);
assert.throws(function() {
fs.createReadStream(example, true);
}, /options must be a string or an object/);
assert.throws(function() {
fs.createReadStream(example, false);
}, /options must be a string or an object/);
33 changes: 33 additions & 0 deletions test/parallel/test-fs-write-stream-throw-type-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const path = require('path');

const example = path.join(common.tmpDir, 'dummy');

assert.doesNotThrow(function() {
fs.createWriteStream(example, undefined);
});
assert.doesNotThrow(function() {
fs.createWriteStream(example, 'utf8');
});
assert.doesNotThrow(function() {
fs.createWriteStream(example, {encoding: 'utf8'});
});

assert.throws(function() {
fs.createWriteStream(example, null);
}, /options must be a string or an object/);
assert.throws(function() {
fs.createWriteStream(example, 123);
}, /options must be a string or an object/);
assert.throws(function() {
fs.createWriteStream(example, 0);
}, /options must be a string or an object/);
assert.throws(function() {
fs.createWriteStream(example, true);
}, /options must be a string or an object/);
assert.throws(function() {
fs.createWriteStream(example, false);
}, /options must be a string or an object/);