fs: add read(buffer[, options]) versions

doc/api/fs.md

@@ -417,6 +417,33 @@ Reads data from the file and stores that in the given buffer.
 If the file is not modified concurrently, the end-of-file is reached when the
 number of bytes read is zero.
 
+#### `filehandle.read(buffer[, options])`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+* `buffer` {Buffer|TypedArray|DataView} A buffer that will be filled with the
+  file data read.
+* `options` {Object}
+  * `offset` {integer} The location in the buffer at which to start filling.
+    **Default:** `0`
+  * `length` {integer} The number of bytes to read. **Default:**
+    `buffer.byteLength - offset`
+  * `position` {integer} The location where to begin reading data from the
+    file. If `null`, data will be read from the current file position, and
+    the position will be updated. If `position` is an integer, the current
+    file position will remain unchanged. **Default:**: `null`
+* Returns: {Promise} Fulfills upon success with an object with two properties:
+  * `bytesRead` {integer} The number of bytes read
+  * `buffer` {Buffer|TypedArray|DataView} A reference to the passed in `buffer`
+    argument.
+
+Reads data from the file and stores that in the given buffer.
+
+If the file is not modified concurrently, the end-of-file is reached when the
+number of bytes read is zero.
+
 #### `filehandle.readableWebStream()`
 
 <!-- YAML
@@ -3243,6 +3270,28 @@ Similar to the [`fs.read()`][] function, this version takes an optional
 `options` object. If no `options` object is specified, it will default with the
 above values.
 
+### `fs.read(fd, buffer[, options], callback)`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+* `fd` {integer}
+* `buffer` {Buffer|TypedArray|DataView} The buffer that the data will be
+  written to.
+* `options` {Object}
+  * `offset` {integer} **Default:** `0`
+  * `length` {integer} **Default:** `buffer.byteLength - offset`
+  * `position` {integer|bigint} **Default:** `null`
+* `callback` {Function}
+  * `err` {Error}
+  * `bytesRead` {integer}
+  * `buffer` {Buffer}
+
+Similar to the [`fs.read()`][] function, this version takes an optional
+`options` object. If no `options` object is specified, it will default with the
+above values.
+
 ### `fs.readdir(path[, options], callback)`
 
 <!-- YAML

lib/fs.js

@@ -136,6 +136,7 @@ const {
   validateEncoding,
   validateFunction,
   validateInteger,
+  validateObject,
   validateString,
 } = require('internal/validators');
 
@@ -595,7 +596,7 @@ function openSync(path, flags, mode) {
  * Reads file from the specified `fd` (file descriptor).
  * @param {number} fd
  * @param {Buffer | TypedArray | DataView} buffer
- * @param {number} offset
+ * @param {number} offsetOrOptions
  * @param {number} length
  * @param {number | bigint} position
  * @param {(
@@ -605,36 +606,42 @@ function openSync(path, flags, mode) {
  *   ) => any} callback
  * @returns {void}
  */
-function read(fd, buffer, offset, length, position, callback) {
+function read(fd, buffer, offsetOrOptions, length, position, callback) {
   fd = getValidatedFd(fd);
 
-  if (arguments.length <= 3) {
-    // Assume fs.read(fd, options, callback)
-    let options = ObjectCreate(null);
-    if (arguments.length < 3) {
+  let offset = offsetOrOptions;
+  let params = null;
+  if (arguments.length <= 4) {
+    if (arguments.length === 4) {
+      // This is fs.read(fd, buffer, options, callback)
+      validateObject(offsetOrOptions, 'options', { nullable: true });
+      callback = length;
+      params = offsetOrOptions;
+    } else if (arguments.length === 3) {
+      // This is fs.read(fd, bufferOrParams, callback)
+      if (!isArrayBufferView(buffer)) {
+        // This is fs.read(fd, params, callback)
+        params = buffer;
+        ({ buffer = Buffer.alloc(16384) } = params ?? ObjectCreate(null));
+      }
+      callback = offsetOrOptions;
+    } else {
       // This is fs.read(fd, callback)
-      // buffer will be the callback
       callback = buffer;
-    } else {
-      // This is fs.read(fd, {}, callback)
-      // buffer will be the options object
-      // offset is the callback
-      options = buffer;
-      callback = offset;
+      buffer = Buffer.alloc(16384);
     }
 
     ({
-      buffer = Buffer.alloc(16384),
       offset = 0,
       length = buffer.byteLength - offset,
       position = null
-    } = options);
+    } = params ?? ObjectCreate(null));
   }
 
   validateBuffer(buffer);
   callback = maybeCallback(callback);
 
-  if (offset == null) {
+  if (offset === false || offset === undefined) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);
@@ -683,26 +690,24 @@ ObjectDefineProperty(read, internalUtil.customPromisifyArgs,
  *   offset?: number;
  *   length?: number;
  *   position?: number | bigint;
- *   }} [offset]
+ *   }} [offsetOrOptions]
  * @returns {number}
  */
-function readSync(fd, buffer, offset, length, position) {
+function readSync(fd, buffer, offsetOrOptions, length, position) {
   fd = getValidatedFd(fd);
 
   validateBuffer(buffer);
 
-  if (arguments.length <= 3) {
-    // Assume fs.readSync(fd, buffer, options)
-    const options = offset || ObjectCreate(null);
-
+  let offset = offsetOrOptions;
+  if (typeof offset === 'object') {
     ({
       offset = 0,
       length = buffer.byteLength - offset,
       position = null
-    } = options);
+    } = offsetOrOptions ?? ObjectCreate(null));
   }
 
-  if (offset == null) {
+  if (offset === false || offset === undefined) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);

lib/internal/fs/promises.js

@@ -508,21 +508,31 @@ async function open(path, flags, mode) {
                                  flagsNumber, mode, kUsePromises));
 }
 
-async function read(handle, bufferOrOptions, offset, length, position) {
-  let buffer = bufferOrOptions;
+async function read(handle, bufferOrParams, offset, length, position) {
+  let buffer = bufferOrParams;
   if (!isArrayBufferView(buffer)) {
-    bufferOrOptions ??= ObjectCreate(null);
+    // This is fh.read(params)
+    bufferOrParams ??= ObjectCreate(null);
     ({
       buffer = Buffer.alloc(16384),
       offset = 0,
       length = buffer.byteLength - offset,
       position = null
-    } = bufferOrOptions);
+    } = bufferOrParams);
 
     validateBuffer(buffer);
   }
 
-  if (offset == null) {
+  if (typeof offset === 'object') {
+    // This is fh.read(buffer, options)
+    ({
+      offset = 0,
+      length = buffer.byteLength - offset,
+      position = null
+    } = offset ?? ObjectCreate(null));
+  }
+
+  if (offset === false || offset === undefined) {
     offset = 0;
   } else {
     validateInteger(offset, 'offset', 0);

test/parallel/test-fs-read-offset-null.js

@@ -15,28 +15,53 @@ const buf = Buffer.alloc(1);
 // Reading only one character, hence buffer of one byte is enough.
 
 // Test for callback API.
-fs.open(filepath, 'r', common.mustSucceed((fd) => {
-  fs.read(fd, { offset: null, buffer: buf },
-          common.mustSucceed((bytesRead, buffer) => {
-            // Test is done by making sure the first letter in buffer is
-            // same as first letter in file.
-            // 120 is the hex for ascii code of letter x.
-            assert.strictEqual(buffer[0], 120);
-            fs.close(fd, common.mustSucceed(() => {}));
-          }));
-}));
+fs.open(filepath, 'r', (_, fd) => {
+  assert.throws(
+    () => fs.read(fd, { offset: null, buffer: buf }, () => {}),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError',
+    }
+  );
+  assert.throws(
+    () => fs.read(fd, buf, { offset: null }, () => {}),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError',
+    }
+  );
+  fs.close(fd, common.mustSucceed(() => {}));
+});
 
 let filehandle = null;
 
 // Test for promise api
 (async () => {
   filehandle = await fsPromises.open(filepath, 'r');
-  const readObject = await filehandle.read(buf, null, buf.length);
+  assert.rejects(
+    async () => filehandle.read(buf, { offset: null }),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError',
+    }
+  );
+})()
+.then(common.mustCall())
+.finally(async () => {
+  if (filehandle)
+    await filehandle.close();
+});
+
+(async () => {
+  filehandle = await fsPromises.open(filepath, 'r');
+
+  // In this test, null is interpreted as default options, not null offset.
+  // 120 is the ascii code of letter x.
+  const readObject = await filehandle.read(buf, null);
   assert.strictEqual(readObject.buffer[0], 120);
 })()
 .then(common.mustCall())
 .finally(async () => {
-// Close the file handle if it is opened
   if (filehandle)
     await filehandle.close();
 });

test/parallel/test-fs-read-optional-params.js

@@ -9,28 +9,45 @@ const fd = fs.openSync(filepath, 'r');
 
 const expected = Buffer.from('xyz\n');
 const defaultBufferAsync = Buffer.alloc(16384);
-const bufferAsOption = Buffer.allocUnsafe(expected.length);
+const bufferAsOption = Buffer.allocUnsafe(expected.byteLength);
 
 // Test not passing in any options object
 fs.read(fd, common.mustCall((err, bytesRead, buffer) => {
-  assert.strictEqual(bytesRead, expected.length);
-  assert.deepStrictEqual(defaultBufferAsync.length, buffer.length);
+  assert.strictEqual(bytesRead, expected.byteLength);
+  assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength);
+}));
+fs.read(fd, bufferAsOption, { position: 0 }, common.mustCall((err, bytesRead, buffer) => {
+  assert.strictEqual(bytesRead, expected.byteLength);
+  assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
 }));
 
 // Test passing in an empty options object
 fs.read(fd, { position: 0 }, common.mustCall((err, bytesRead, buffer) => {
-  assert.strictEqual(bytesRead, expected.length);
-  assert.deepStrictEqual(defaultBufferAsync.length, buffer.length);
+  assert.strictEqual(bytesRead, expected.byteLength);
+  assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength);
+}));
+fs.read(fd, bufferAsOption, { position: 0 }, common.mustCall((err, bytesRead, buffer) => {
+  assert.strictEqual(bytesRead, expected.byteLength);
+  assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
 }));
 
 // Test passing in options
 fs.read(fd, {
   buffer: bufferAsOption,
   offset: 0,
-  length: bufferAsOption.length,
+  length: bufferAsOption.byteLength,
+  position: 0
+},
+        common.mustCall((err, bytesRead, buffer) => {
+          assert.strictEqual(bytesRead, expected.byteLength);
+          assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
+        }));
+fs.read(fd, bufferAsOption, {
+  offset: 0,
+  length: bufferAsOption.byteLength,
   position: 0
 },
         common.mustCall((err, bytesRead, buffer) => {
-          assert.strictEqual(bytesRead, expected.length);
-          assert.deepStrictEqual(bufferAsOption.length, buffer.length);
+          assert.strictEqual(bytesRead, expected.byteLength);
+          assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
         }));

test/parallel/test-fs-read-promises-optional-params.js

@@ -10,10 +10,18 @@ const fd = fs.openSync(filepath, 'r');
 
 const expected = Buffer.from('xyz\n');
 const defaultBufferAsync = Buffer.alloc(16384);
+const bufferAsOption = Buffer.allocUnsafe(expected.byteLength);
 
 read(fd, {})
   .then(function({ bytesRead, buffer }) {
-    assert.strictEqual(bytesRead, expected.length);
-    assert.deepStrictEqual(defaultBufferAsync.length, buffer.length);
+    assert.strictEqual(bytesRead, expected.byteLength);
+    assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength);
+  })
+  .then(common.mustCall());
+
+read(fd, bufferAsOption, { position: 0 })
+  .then(function({ bytesRead, buffer }) {
+    assert.strictEqual(bytesRead, expected.byteLength);
+    assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength);
   })
   .then(common.mustCall());

test/parallel/test-fs-readSync-optional-params.js

@@ -31,26 +31,16 @@ for (const options of [
   { length: expected.length, position: 0 },
   { offset: 0, length: expected.length, position: 0 },
 
-  { offset: null },
+  { offset: false },
   { position: null },
   { position: -1 },
   { position: 0n },
 
   // Test default params
   {},
   null,
-  undefined,
-
-  // Test if bad params are interpreted as default (not mandatory)
-  false,
-  true,
-  Infinity,
-  42n,
-  Symbol(),
 
   // Test even more malicious corner cases
-  '4'.repeat(expected.length),
-  new String('4444'),
   [4, 4, 4, 4],
 ]) {
   runTest(Buffer.allocUnsafe(expected.length), options);