fix(ext/node/fs): position argument not applied (#24009)

We didn't honour the `position` options of `fd.read` and `fd.write`
because we checked if the buffer is of type `Buffer` instead of just
`Uint8Array`. Node does the latter. In doing so I noticed that the file
handle id was written to a public property which it definitely shouldn't
be. This was probably a typo.

Fixes https://github.com/denoland/deno/issues/23707

ext/node/polyfills/internal/fs/handle.ts

@@ -28,27 +28,27 @@ export class FileHandle extends EventEmitter {
   #rid: number;
   constructor(rid: number) {
     super();
-    this.rid = rid;
+    this.#rid = rid;
   }
 
   get fd() {
-    return this.rid;
+    return this.#rid;
   }
 
   read(
-    buffer: Buffer,
+    buffer: Uint8Array,
     offset?: number,
     length?: number,
     position?: number | null,
   ): Promise<ReadResult>;
   read(options?: ReadOptions): Promise<ReadResult>;
   read(
-    bufferOrOpt: Buffer | ReadOptions,
+    bufferOrOpt: Uint8Array | ReadOptions,
     offset?: number,
     length?: number,
     position?: number | null,
   ): Promise<ReadResult> {
-    if (bufferOrOpt instanceof Buffer) {
+    if (bufferOrOpt instanceof Uint8Array) {
       return new Promise((resolve, reject) => {
         read(
           this.fd,
@@ -90,12 +90,12 @@ export class FileHandle extends EventEmitter {
     encoding: string,
   ): Promise<WriteResult>;
   write(
-    bufferOrStr: Buffer | string,
+    bufferOrStr: Uint8Array | string,
     offsetOrPosition: number,
     lengthOrEncoding: number | string,
     position?: number,
   ): Promise<WriteResult> {
-    if (bufferOrStr instanceof Buffer) {
+    if (bufferOrStr instanceof Uint8Array) {
       const buffer = bufferOrStr;
       const offset = offsetOrPosition;
       const length = lengthOrEncoding;

tests/unit_node/_fs/_fs_write_test.ts

@@ -49,3 +49,27 @@ Deno.test({
     assertEquals(decoder.decode(data), "hello");
   },
 });
+
+Deno.test({
+  name: "Data is padded if position > length",
+  async fn() {
+    const tempFile: string = Deno.makeTempFileSync();
+
+    using file = await Deno.open(tempFile, {
+      create: true,
+      write: true,
+      read: true,
+    });
+
+    const str = "hello world";
+    const buffer = Buffer.from(str);
+    const bytesWritten = writeSync(file.rid, buffer, 0, str.length, 4);
+
+    const data = Deno.readFileSync(tempFile);
+    Deno.removeSync(tempFile);
+
+    assertEquals(bytesWritten, str.length);
+    // Check if result is padded
+    assertEquals(decoder.decode(data), "\x00\x00\x00\x00hello world");
+  },
+});

tests/unit_node/fs_test.ts

@@ -15,7 +15,13 @@ import {
   statSync,
   writeFileSync,
 } from "node:fs";
-import { constants as fsPromiseConstants, cp } from "node:fs/promises";
+import {
+  constants as fsPromiseConstants,
+  cp,
+  FileHandle,
+  open,
+  writeFile,
+} from "node:fs/promises";
 import process from "node:process";
 import { pathToAbsoluteFileUrl } from "../unit/test_util.ts";
 
@@ -165,3 +171,33 @@ Deno.test(
     assertEquals(result, undefined);
   },
 );
+
+// Test for https://github.com/denoland/deno/issues/23707
+Deno.test(
+  "[node/fs/promises read] respect position argument",
+  async () => {
+    const file = mkdtempSync(join(tmpdir(), "foo-")) + "/test.bin";
+    await writeFile(file, "");
+
+    const res: number[] = [];
+    let fd: FileHandle | undefined;
+    try {
+      fd = await open(file, "r+");
+
+      for (let i = 0; i <= 5; i++) {
+        const buffer = new Uint8Array([i]);
+        await fd.write(buffer, 0, 1, i + 10);
+      }
+
+      for (let i = 10; i <= 15; i++) {
+        const buffer = new Uint8Array(1);
+        await fd.read(buffer, 0, 1, i);
+        res.push(Number(buffer.toString()));
+      }
+    } finally {
+      await fd?.close();
+    }
+
+    assertEquals(res, [0, 1, 2, 3, 4, 5]);
+  },
+);