fix(ext/node): fs.readFile, fs.readFileSync assert encoding (#30830)

Towards #29972

- Validates the encoding of `readFile`.
- Fixes the handling of `binary` encoding where previously it returns
Buffer. On Node.js, Buffer is only returned when the encoding is not
specified.
- Allows
[parallel/test-fs-read-file-assert-encoding.js](https://github.com/nodejs/node/blob/v24.2.0/test/parallel/test-fs-read-file-assert-encoding.js)
test to pass.

Author: Tango992

ext/node/polyfills/_fs/_fs_common.ts

@@ -16,6 +16,7 @@ import {
   TextEncodings,
 } from "ext:deno_node/_utils.ts";
 import { type Buffer } from "node:buffer";
+import { assertEncoding } from "ext:deno_node/internal/fs/utils.mjs";
 
 export type CallbackWithError = (err: ErrnoException | null) => void;
 
@@ -57,6 +58,21 @@ export function isFileOptions(
   );
 }
 
+export function getValidatedEncoding(
+  optOrCallback?:
+    | FileOptions
+    | WriteFileOptions
+    | ((...args: unknown[]) => unknown)
+    | Encodings
+    | null,
+): Encodings | null {
+  const encoding = getEncoding(optOrCallback);
+  if (encoding) {
+    assertEncoding(encoding);
+  }
+  return encoding;
+}
+
 export function getEncoding(
   optOrCallback?:
     | FileOptions

ext/node/polyfills/_fs/_fs_readFile.ts

@@ -6,33 +6,29 @@
 import {
   BinaryOptionsArgument,
   FileOptionsArgument,
-  getEncoding,
   getSignal,
+  getValidatedEncoding,
   TextOptionsArgument,
 } from "ext:deno_node/_fs/_fs_common.ts";
 import { Buffer } from "node:buffer";
 import { readAll, readAllSync } from "ext:deno_io/12_io.js";
 import { FileHandle } from "ext:deno_node/internal/fs/handle.ts";
 import { pathFromURL } from "ext:deno_web/00_infra.js";
-import {
-  BinaryEncodings,
-  Encodings,
-  TextEncodings,
-} from "ext:deno_node/_utils.ts";
+import { Encodings } from "ext:deno_node/_utils.ts";
 import { FsFile } from "ext:deno_fs/30_fs.js";
 import { denoErrorToNodeError } from "ext:deno_node/internal/errors.ts";
 
-function maybeDecode(data: Uint8Array, encoding: TextEncodings): string;
+function maybeDecode(data: Uint8Array, encoding: Encodings): string;
 function maybeDecode(
   data: Uint8Array,
-  encoding: BinaryEncodings | null,
+  encoding: null,
 ): Buffer;
 function maybeDecode(
   data: Uint8Array,
   encoding: Encodings | null,
 ): string | Buffer {
   const buffer = Buffer.from(data.buffer, data.byteOffset, data.byteLength);
-  if (encoding && encoding !== "binary") return buffer.toString(encoding);
+  if (encoding) return buffer.toString(encoding);
   return buffer;
 }
 
@@ -71,7 +67,7 @@ export function readFile(
     cb = callback;
   }
 
-  const encoding = getEncoding(optOrCallback);
+  const encoding = getValidatedEncoding(optOrCallback);
   const signal = getSignal(optOrCallback);
 
   let p: Promise<Uint8Array>;
@@ -87,12 +83,8 @@ export function readFile(
 
   if (cb) {
     p.then((data: Uint8Array) => {
-      if (encoding && encoding !== "binary") {
-        const text = maybeDecode(data, encoding);
-        return (cb as TextCallback)(null, text);
-      }
-      const buffer = maybeDecode(data, encoding);
-      (cb as BinaryCallback)(null, buffer);
+      const textOrBuffer = maybeDecode(data, encoding);
+      (cb as BinaryCallback)(null, textOrBuffer);
     }, (err) => cb && cb(denoErrorToNodeError(err, { path, syscall: "open" })));
   }
 }
@@ -134,11 +126,7 @@ export function readFileSync(
       throw denoErrorToNodeError(err, { path, syscall: "open" });
     }
   }
-  const encoding = getEncoding(opt);
-  if (encoding && encoding !== "binary") {
-    const text = maybeDecode(data, encoding);
-    return text;
-  }
-  const buffer = maybeDecode(data, encoding);
-  return buffer;
+  const encoding = getValidatedEncoding(opt);
+  const textOrBuffer = maybeDecode(data, encoding);
+  return textOrBuffer;
 }

tests/node_compat/config.toml

@@ -454,6 +454,7 @@
 "parallel/test-fs-promises-readfile-empty.js" = {}
 "parallel/test-fs-promisified.js" = {}
 "parallel/test-fs-read-empty-buffer.js" = {}
+"parallel/test-fs-read-file-assert-encoding.js" = {}
 "parallel/test-fs-read-file-sync-hostname.js" = {}
 "parallel/test-fs-read-stream-autoClose.js" = {}
 "parallel/test-fs-read-stream-double-close.js" = {}

tests/unit_node/_fs/_fs_readFile_test.ts

@@ -3,6 +3,7 @@ import { assertCallbackErrorUncaught } from "../_test_utils.ts";
 import { promises, readFile, readFileSync } from "node:fs";
 import * as path from "@std/path";
 import { assert, assertEquals, assertMatch } from "@std/assert";
+import { Buffer } from "node:buffer";
 
 const moduleDir = path.dirname(path.fromFileUrl(import.meta.url));
 const testData = path.resolve(moduleDir, "testdata", "hello.txt");
@@ -144,3 +145,45 @@ Deno.test("fs.readFileSync error message contains path + syscall", () => {
     }
   }
 });
+
+Deno.test("fs.readFile returns Buffer when encoding is not provided", async () => {
+  const data = await new Promise<Uint8Array>((res, rej) => {
+    readFile(testData, (err, data) => {
+      if (err) {
+        rej(err);
+      }
+      res(data as Uint8Array);
+    });
+  });
+
+  assert(data instanceof Uint8Array);
+  assertEquals(Buffer.isBuffer(data), true);
+  assertEquals(data.toString(), "hello world");
+});
+
+Deno.test("fs.readFile binary encoding returns string", async () => {
+  const data = await new Promise<string>((res, rej) => {
+    readFile(testData, { encoding: "binary" }, (err, data) => {
+      if (err) {
+        rej(err);
+      }
+      res(data as string);
+    });
+  });
+
+  assertEquals(typeof data, "string");
+  assertEquals(data, "hello world");
+});
+
+Deno.test("fs.readFileSync returns Buffer when encoding is not provided", () => {
+  const data = readFileSync(testData);
+  assert(data instanceof Uint8Array);
+  assertEquals(Buffer.isBuffer(data), true);
+  assertEquals(data.toString(), "hello world");
+});
+
+Deno.test("fs.readFileSync binary encoding returns string", () => {
+  const data = readFileSync(testData, { encoding: "binary" });
+  assertEquals(typeof data, "string");
+  assertEquals(data, "hello world");
+});