diff --git a/.roo/rules/use-safeReadJson.md b/.roo/rules/use-safeReadJson.md new file mode 100644 index 0000000000..c5fdf23dfe --- /dev/null +++ b/.roo/rules/use-safeReadJson.md @@ -0,0 +1,33 @@ +# JSON File Reading Must Be Safe and Atomic + +- You MUST use `safeReadJson(filePath: string, jsonPath?: string | string[]): Promise` from `src/utils/safeReadJson.ts` to read JSON files +- `safeReadJson` provides atomic file access to local files with proper locking to prevent race conditions and uses `stream-json` to read JSON files without buffering to a string +- Test files are exempt from this rule + +## Correct Usage Example + +This pattern replaces all manual `fs` or `vscode.workspace.fs` reads. + +### ❌ Don't do this: + +```typescript +// Anti-patterns: string buffering wastes memory +const data = JSON.parse(await fs.readFile(filePath, 'utf8')); +const data = JSON.parse(await vscode.workspace.fs.readFile(fileUri)); + +// Anti-pattern: Unsafe existence check +if (await fileExists.. ) { /* then read */ } +``` + +### ✅ Use this unified pattern: + +```typescript +let data +try { + data = await safeReadJson(filePath) +} catch (error) { + if (error.code !== "ENOENT") { + // Handle at least ENOENT + } +} +``` diff --git a/src/utils/__tests__/safeReadJson.spec.ts b/src/utils/__tests__/safeReadJson.spec.ts new file mode 100644 index 0000000000..e14870fa00 --- /dev/null +++ b/src/utils/__tests__/safeReadJson.spec.ts @@ -0,0 +1,256 @@ +import { vi, describe, test, expect, beforeAll, afterAll, beforeEach, afterEach } from "vitest" +import { safeReadJson } from "../safeReadJson" +import { Readable } from "stream" // For typing mock stream + +// First import the original modules to use their types +import * as fsPromisesOriginal from "fs/promises" +import * as fsOriginal from "fs" + +// Set up mocks before imports +vi.mock("proper-lockfile", () => ({ + lock: vi.fn(), + check: vi.fn(), + unlock: vi.fn(), +})) + +vi.mock("fs/promises", async () => { + const actual = await vi.importActual("fs/promises") + return { + ...actual, + writeFile: vi.fn(actual.writeFile), + readFile: vi.fn(actual.readFile), + access: vi.fn(actual.access), + mkdir: vi.fn(actual.mkdir), + mkdtemp: vi.fn(actual.mkdtemp), + rm: vi.fn(actual.rm), + } +}) + +vi.mock("fs", async () => { + const actualFs = await vi.importActual("fs") + return { + ...actualFs, + createReadStream: vi.fn((path: string, options?: any) => actualFs.createReadStream(path, options)), + } +}) + +// Now import the mocked versions +import * as fs from "fs/promises" +import * as fsSyncActual from "fs" +import * as path from "path" +import * as os from "os" +import * as properLockfile from "proper-lockfile" + +describe("safeReadJson", () => { + let originalConsoleError: typeof console.error + let tempTestDir: string = "" + let currentTestFilePath = "" + + beforeAll(() => { + // Store original console.error + originalConsoleError = console.error + + // Replace with filtered version that suppresses output from the module + console.error = function (...args) { + // Check if call originated from safeReadJson.ts + if (new Error().stack?.includes("safeReadJson.ts")) { + // Suppress output but allow spy recording + return + } + + // Pass through all other calls (from tests) + return originalConsoleError.apply(console, args) + } + }) + + afterAll(() => { + // Restore original behavior + console.error = originalConsoleError + }) + + vi.useRealTimers() // Use real timers for this test suite + + beforeEach(async () => { + // Create a unique temporary directory for each test + const tempDirPrefix = path.join(os.tmpdir(), "safeReadJson-test-") + tempTestDir = await fs.mkdtemp(tempDirPrefix) + currentTestFilePath = path.join(tempTestDir, "test-data.json") + }) + + afterEach(async () => { + if (tempTestDir) { + try { + await fs.rm(tempTestDir, { recursive: true, force: true }) + } catch (err) { + console.error("Failed to clean up temp directory", err) + } + tempTestDir = "" + } + + // Reset all mocks + vi.resetAllMocks() + }) + + // Helper function to write a JSON file for testing + const writeJsonFile = async (filePath: string, data: any): Promise => { + await fs.writeFile(filePath, JSON.stringify(data), "utf8") + } + + // Success Scenarios + test("should successfully read a JSON file", async () => { + const testData = { message: "Hello, world!" } + await writeJsonFile(currentTestFilePath, testData) + + const result = await safeReadJson(currentTestFilePath) + expect(result).toEqual(testData) + }) + + test("should throw an error for a non-existent file", async () => { + const nonExistentPath = path.join(tempTestDir, "non-existent.json") + + await expect(safeReadJson(nonExistentPath)).rejects.toThrow(/ENOENT/) + }) + + test("should read a specific path from a JSON file", async () => { + const testData = { + user: { + name: "John", + age: 30, + address: { + city: "New York", + zip: "10001", + }, + }, + settings: { + theme: "dark", + notifications: true, + }, + } + await writeJsonFile(currentTestFilePath, testData) + + // Test reading a specific path + const result = await safeReadJson(currentTestFilePath, "user.address.city") + expect(result).toBe("New York") + }) + + test("should read multiple paths from a JSON file", async () => { + const testData = { + user: { + name: "John", + age: 30, + }, + settings: { + theme: "dark", + notifications: true, + }, + } + await writeJsonFile(currentTestFilePath, testData) + + // Test reading multiple paths + const result = await safeReadJson(currentTestFilePath, ["user.name", "settings.theme"]) + expect(result).toEqual(["John", "dark"]) + }) + + // Failure Scenarios + test("should handle JSON parsing errors", async () => { + // Write invalid JSON + await fs.writeFile(currentTestFilePath, "{ invalid: json", "utf8") + + await expect(safeReadJson(currentTestFilePath)).rejects.toThrow() + }) + + test("should handle file access errors", async () => { + const accessSpy = vi.spyOn(fs, "access") + accessSpy.mockImplementationOnce(async () => { + const err = new Error("Simulated EACCES Error") as NodeJS.ErrnoException + err.code = "EACCES" // Simulate a permissions error + throw err + }) + + await expect(safeReadJson(currentTestFilePath)).rejects.toThrow("Simulated EACCES Error") + + accessSpy.mockRestore() + }) + + test("should handle stream errors", async () => { + await writeJsonFile(currentTestFilePath, { test: "data" }) + + // Mock createReadStream to simulate a failure during streaming + ;(fsSyncActual.createReadStream as ReturnType).mockImplementationOnce( + (_path: any, _options: any) => { + const stream = new Readable({ + read() { + this.emit("error", new Error("Simulated Stream Error")) + }, + }) + return stream as fsSyncActual.ReadStream + }, + ) + + await expect(safeReadJson(currentTestFilePath)).rejects.toThrow("Simulated Stream Error") + }) + + test("should handle lock acquisition failures", async () => { + await writeJsonFile(currentTestFilePath, { test: "data" }) + + // Mock proper-lockfile to simulate a lock acquisition failure + const lockSpy = vi.spyOn(properLockfile, "lock").mockRejectedValueOnce(new Error("Failed to get lock")) + + await expect(safeReadJson(currentTestFilePath)).rejects.toThrow("Failed to get lock") + + expect(lockSpy).toHaveBeenCalledWith(expect.stringContaining(currentTestFilePath), expect.any(Object)) + + lockSpy.mockRestore() + }) + + test("should release lock even if an error occurs during reading", async () => { + await writeJsonFile(currentTestFilePath, { test: "data" }) + + // Mock createReadStream to simulate a failure during streaming + ;(fsSyncActual.createReadStream as ReturnType).mockImplementationOnce( + (_path: any, _options: any) => { + const stream = new Readable({ + read() { + this.emit("error", new Error("Simulated Stream Error")) + }, + }) + return stream as fsSyncActual.ReadStream + }, + ) + + await expect(safeReadJson(currentTestFilePath)).rejects.toThrow("Simulated Stream Error") + + // Lock should be released, meaning the .lock file should not exist + const lockPath = `${path.resolve(currentTestFilePath)}.lock` + await expect(fs.access(lockPath)).rejects.toThrow(expect.objectContaining({ code: "ENOENT" })) + }) + + // Edge Cases + test("should handle empty JSON files", async () => { + await fs.writeFile(currentTestFilePath, "", "utf8") + + await expect(safeReadJson(currentTestFilePath)).rejects.toThrow() + }) + + test("should handle large JSON files", async () => { + // Create a large JSON object + const largeData: Record = {} + for (let i = 0; i < 10000; i++) { + largeData[`key${i}`] = i + } + + await writeJsonFile(currentTestFilePath, largeData) + + const result = await safeReadJson(currentTestFilePath) + expect(result).toEqual(largeData) + }) + + test("should handle path selection for non-existent paths", async () => { + const testData = { user: { name: "John" } } + await writeJsonFile(currentTestFilePath, testData) + + // Test reading a non-existent path + const result = await safeReadJson(currentTestFilePath, "user.address") + expect(result).toBeUndefined() + }) +}) diff --git a/src/utils/safeReadJson.ts b/src/utils/safeReadJson.ts new file mode 100644 index 0000000000..80ca645fa7 --- /dev/null +++ b/src/utils/safeReadJson.ts @@ -0,0 +1,102 @@ +import * as fs from "fs/promises" +import * as fsSync from "fs" +import * as path from "path" +import * as Parser from "stream-json/Parser" +import * as Pick from "stream-json/filters/Pick" +import * as StreamValues from "stream-json/streamers/StreamValues" + +import { _acquireLock } from "./safeWriteJson" + +/** + * Safely reads JSON data from a file using streaming. + * - Uses 'proper-lockfile' for advisory locking to prevent concurrent access + * - Streams the file contents to efficiently handle large JSON files + * + * @param {string} filePath - The path to the file to read + * @returns {Promise} - The parsed JSON data + * + * @example + * // Read entire JSON file + * const data = await safeReadJson('config.json'); + */ +async function safeReadJson(filePath: string): Promise { + const absoluteFilePath = path.resolve(filePath) + let releaseLock = async () => {} // Initialized to a no-op + + try { + // Check if file exists + await fs.access(absoluteFilePath) + + // Acquire lock + try { + releaseLock = await _acquireLock(absoluteFilePath) + } catch (lockError) { + console.error(`Failed to acquire lock for reading ${absoluteFilePath}:`, lockError) + throw lockError + } + + // Stream and parse the file + return await _streamDataFromFile(absoluteFilePath) + } finally { + // Release the lock in the finally block + try { + await releaseLock() + } catch (unlockError) { + console.error(`Failed to release lock for ${absoluteFilePath}:`, unlockError) + } + } +} + +/** + * Helper function to stream JSON data from a file. + * @param sourcePath The path to read the stream from. + * @returns Promise The parsed JSON data. + */ +async function _streamDataFromFile(sourcePath: string): Promise { + // Create a readable stream from the file + const fileReadStream = fsSync.createReadStream(sourcePath, { encoding: "utf8" }) + + // Set up the pipeline components + const jsonParser = Parser.parser() + + // Create the base pipeline + let pipeline = fileReadStream.pipe(jsonParser) + + // Add value collection + const valueStreamer = StreamValues.streamValues() + pipeline = pipeline.pipe(valueStreamer) + + return new Promise((resolve, reject) => { + let errorOccurred = false + const result: any[] = [] + + const handleError = (streamName: string) => (err: unknown) => { + if (!errorOccurred) { + errorOccurred = true + if (!fileReadStream.destroyed) { + fileReadStream.destroy(err instanceof Error ? err : new Error(String(err))) + } + reject(err instanceof Error ? err : new Error(`${streamName} error: ${String(err)}`)) + } + } + + // Set up error handlers for all stream components + fileReadStream.on("error", handleError("FileReadStream")) + jsonParser.on("error", handleError("Parser")) + valueStreamer.on("error", handleError("StreamValues")) + + // Collect data + valueStreamer.on("data", (data: any) => { + result.push(data.value) + }) + + // Handle end of stream + valueStreamer.on("end", () => { + if (!errorOccurred) { + resolve(result.length === 1 ? result[0] : result) + } + }) + }) +} + +export { safeReadJson, _streamDataFromFile } diff --git a/src/utils/safeWriteJson.ts b/src/utils/safeWriteJson.ts index 719bbd7216..80863a80db 100644 --- a/src/utils/safeWriteJson.ts +++ b/src/utils/safeWriteJson.ts @@ -5,6 +5,34 @@ import * as lockfile from "proper-lockfile" import Disassembler from "stream-json/Disassembler" import Stringer from "stream-json/Stringer" +/** + * Acquires a lock on a file. + * + * @param {string} filePath - The path to the file to lock + * @param {lockfile.LockOptions} [options] - Optional lock options + * @returns {Promise<() => Promise>} - The lock release function + */ +export async function _acquireLock(filePath: string, options?: lockfile.LockOptions): Promise<() => Promise> { + const absoluteFilePath = path.resolve(filePath) + + return await lockfile.lock(absoluteFilePath, { + stale: 31000, // Stale after 31 seconds + update: 10000, // Update mtime every 10 seconds + realpath: false, // The file may not exist yet + retries: { + retries: 5, + factor: 2, + minTimeout: 100, + maxTimeout: 1000, + }, + onCompromised: (err) => { + console.error(`Lock at ${absoluteFilePath} was compromised:`, err) + throw err + }, + ...options, + }) +} + /** * Safely writes JSON data to a file. * - Creates parent directories if they don't exist @@ -39,22 +67,7 @@ async function safeWriteJson(filePath: string, data: any): Promise { // Acquire the lock before any file operations try { - releaseLock = await lockfile.lock(absoluteFilePath, { - stale: 31000, // Stale after 31 seconds - update: 10000, // Update mtime every 10 seconds to prevent staleness if operation is long - realpath: false, // the file may not exist yet, which is acceptable - retries: { - // Configuration for retrying lock acquisition - retries: 5, // Number of retries after the initial attempt - factor: 2, // Exponential backoff factor (e.g., 100ms, 200ms, 400ms, ...) - minTimeout: 100, // Minimum time to wait before the first retry (in ms) - maxTimeout: 1000, // Maximum time to wait for any single retry (in ms) - }, - onCompromised: (err) => { - console.error(`Lock at ${absoluteFilePath} was compromised:`, err) - throw err - }, - }) + releaseLock = await _acquireLock(absoluteFilePath) } catch (lockError) { // If lock acquisition fails, we throw immediately. // The releaseLock remains a no-op, so the finally block in the main file operations