From 784d84cf346c5c41679fa75d3da27119365e8f0d Mon Sep 17 00:00:00 2001
From: Livia Medeiros <74449973+LiviaMedeiros@users.noreply.github.com>
Date: Tue, 3 May 2022 02:01:27 +0800
Subject: [PATCH] fs: add `read(buffer[, options])` versions

This adds the following:
- `fs.read(fd, buffer[, options], callback)`.
- `filehandle.read(buffer[, options])`.

PR-URL: https://github.com/nodejs/node/pull/42768
Refs: https://github.com/nodejs/node/pull/42601
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
---
 doc/api/fs.md                                 | 49 +++++++++++++++++++
 lib/fs.js                                     | 37 ++++++++------
 lib/internal/fs/promises.js                   | 17 +++++--
 test/parallel/test-fs-read-offset-null.js     | 44 ++++++++++++-----
 test/parallel/test-fs-read-optional-params.js | 46 ++++++++---------
 .../test-fs-read-promises-optional-params.js  | 12 ++++-
 6 files changed, 151 insertions(+), 54 deletions(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 1422bee91fcb78..c27e1868351fd0 100644
--- a/doc/api/fs.md
+++ b/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
diff --git a/lib/fs.js b/lib/fs.js
index cf0879ce56d131..3b29f985e51a25 100644
--- a/lib/fs.js
+++ b/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 | null} position
  * @param {(
@@ -605,30 +606,36 @@ 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);
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 73c159550740b0..ea9482e69dc904 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -508,20 +508,29 @@ 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)
     ({
       buffer = Buffer.alloc(16384),
       offset = 0,
       length = buffer.byteLength - offset,
       position = null
-    } = bufferOrOptions);
+    } = bufferOrParams ?? ObjectCreate(null));
 
     validateBuffer(buffer);
   }
 
+  if (offset !== null && typeof offset === 'object') {
+    // This is fh.read(buffer, options)
+    ({
+      offset = 0,
+      length = buffer.byteLength - offset,
+      position = null
+    } = offset ?? ObjectCreate(null));
+  }
+
   if (offset == null) {
     offset = 0;
   } else {
diff --git a/test/parallel/test-fs-read-offset-null.js b/test/parallel/test-fs-read-offset-null.js
index 27e893f781b2ff..012c94e41e92c7 100644
--- a/test/parallel/test-fs-read-offset-null.js
+++ b/test/parallel/test-fs-read-offset-null.js
@@ -14,13 +14,22 @@ const filepath = fixtures.path('x.txt');
 const buf = Buffer.alloc(1);
 // Reading only one character, hence buffer of one byte is enough.
 
-// Test for callback API.
+// Tests are done by making sure the first letter in buffer is
+// same as first letter in file.
+// 120 is the ascii code of letter x.
+
+// Tests 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', common.mustSucceed((fd) => {
+  fs.read(fd, buf, { offset: null },
+          common.mustSucceed((bytesRead, buffer) => {
             assert.strictEqual(buffer[0], 120);
             fs.close(fd, common.mustSucceed(() => {}));
           }));
@@ -28,15 +37,28 @@ fs.open(filepath, 'r', common.mustSucceed((fd) => {
 
 let filehandle = null;
 
-// Test for promise api
+// Tests for promises api
+(async () => {
+  filehandle = await fsPromises.open(filepath, 'r');
+  const readObject = await filehandle.read(buf, { offset: null });
+  assert.strictEqual(readObject.buffer[0], 120);
+})()
+.finally(() => filehandle?.close())
+.then(common.mustCall());
+
+// Undocumented: omitted position works the same as position === null
 (async () => {
   filehandle = await fsPromises.open(filepath, 'r');
   const readObject = await filehandle.read(buf, null, buf.length);
   assert.strictEqual(readObject.buffer[0], 120);
 })()
-.then(common.mustCall())
-.finally(async () => {
-// Close the file handle if it is opened
-  if (filehandle)
-    await filehandle.close();
-});
+.finally(() => filehandle?.close())
+.then(common.mustCall());
+
+(async () => {
+  filehandle = await fsPromises.open(filepath, 'r');
+  const readObject = await filehandle.read(buf, null, buf.length, 0);
+  assert.strictEqual(readObject.buffer[0], 120);
+})()
+.finally(() => filehandle?.close())
+.then(common.mustCall());
diff --git a/test/parallel/test-fs-read-optional-params.js b/test/parallel/test-fs-read-optional-params.js
index aac5f51d0bb2db..2699850b0e9597 100644
--- a/test/parallel/test-fs-read-optional-params.js
+++ b/test/parallel/test-fs-read-optional-params.js
@@ -5,32 +5,34 @@ const fixtures = require('../common/fixtures');
 const fs = require('fs');
 const assert = require('assert');
 const filepath = fixtures.path('x.txt');
-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);
-}));
+function testValid(message, ...options) {
+  const paramsMsg = `${message} (as params)`;
+  const paramsFilehandle = fs.openSync(filepath, 'r');
+  fs.read(paramsFilehandle, ...options, common.mustSucceed((bytesRead, buffer) => {
+    assert.strictEqual(bytesRead, expected.byteLength, paramsMsg);
+    assert.deepStrictEqual(defaultBufferAsync.byteLength, buffer.byteLength, paramsMsg);
+    fs.closeSync(paramsFilehandle);
+  }));
 
-// 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);
-}));
+  const optionsMsg = `${message} (as options)`;
+  const optionsFilehandle = fs.openSync(filepath, 'r');
+  fs.read(optionsFilehandle, bufferAsOption, ...options, common.mustSucceed((bytesRead, buffer) => {
+    assert.strictEqual(bytesRead, expected.byteLength, optionsMsg);
+    assert.deepStrictEqual(bufferAsOption.byteLength, buffer.byteLength, optionsMsg);
+    fs.closeSync(optionsFilehandle);
+  }));
+}
 
-// Test passing in options
-fs.read(fd, {
-  buffer: bufferAsOption,
+testValid('Not passing in any object');
+testValid('Passing in a null', null);
+testValid('Passing in an empty object', {});
+testValid('Passing in an object', {
   offset: 0,
-  length: bufferAsOption.length,
-  position: 0
-},
-        common.mustCall((err, bytesRead, buffer) => {
-          assert.strictEqual(bytesRead, expected.length);
-          assert.deepStrictEqual(bufferAsOption.length, buffer.length);
-        }));
+  length: bufferAsOption.byteLength,
+  position: 0,
+});
diff --git a/test/parallel/test-fs-read-promises-optional-params.js b/test/parallel/test-fs-read-promises-optional-params.js
index 9d19eaa41151a8..48311e9df45c91 100644
--- a/test/parallel/test-fs-read-promises-optional-params.js
+++ b/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());