-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1617,8 +1617,15 @@ function ReadStream(path, options) { | |
if (!(this instanceof ReadStream)) | ||
return new ReadStream(path, options); | ||
|
||
if (options === undefined) | ||
options = {}; | ||
else if (typeof options === 'string') | ||
options = { encoding: options }; | ||
else if (options === null || typeof options !== 'object') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that null throws, should we do this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your concern is If you concern about the backward compatible, we discussed here (#1845 (comment)) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly, now that we don't allow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Object.create is proposed from #635 . |
||
if (options.highWaterMark === undefined) | ||
options.highWaterMark = 64 * 1024; | ||
|
||
|
@@ -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); | ||
|
||
|
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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. passed. |
||
assert(chunk.equals(expected)); | ||
} | ||
}); | ||
assertStream.setDefaultEncoding(encoding); | ||
fs.createReadStream(example, encoding).pipe(assertStream); |
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'); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests should probably check for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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/); |
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/); |
There was a problem hiding this comment.
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 anObject.create(null)
call too.There was a problem hiding this comment.
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')
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 onnull
, since this is already a breaking change andnull
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.There was a problem hiding this comment.
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
)?There was a problem hiding this comment.
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 infalse
, for example, would throw.There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
andstring
. 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.