Skip to content

Commit 55346e8

Browse files
author
Eric Wheeler
committed
safe-json: add streaming read support
Implement safeReadJson function to complement the existing safeWriteJson functionality: - Uses stream-json for efficient processing of large JSON files - Supports both full object reading and selective path extraction - Provides file locking to prevent concurrent access - Includes comprehensive error handling - Adds complete test coverage - Passthrough all exceptions This enables efficient and safe JSON reading operations throughout the codebase. Signed-off-by: Eric Wheeler <[email protected]>
1 parent 797f5fa commit 55346e8

File tree

4 files changed

+448
-16
lines changed

4 files changed

+448
-16
lines changed

.roo/rules/use-safeReadJson.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
# JSON File Reading Must Be Safe and Atomic
2+
3+
- You MUST use `safeReadJson(filePath: string, jsonPath?: string | string[]): Promise<any>` from `src/utils/safeReadJson.ts` instead of `fs.readFile` followed by `JSON.parse`
4+
- `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
5+
- Test files are exempt from this rule
Lines changed: 256 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,256 @@
1+
import { vi, describe, test, expect, beforeAll, afterAll, beforeEach, afterEach } from "vitest"
2+
import { safeReadJson } from "../safeReadJson"
3+
import { Readable } from "stream" // For typing mock stream
4+
5+
// First import the original modules to use their types
6+
import * as fsPromisesOriginal from "fs/promises"
7+
import * as fsOriginal from "fs"
8+
9+
// Set up mocks before imports
10+
vi.mock("proper-lockfile", () => ({
11+
lock: vi.fn(),
12+
check: vi.fn(),
13+
unlock: vi.fn(),
14+
}))
15+
16+
vi.mock("fs/promises", async () => {
17+
const actual = await vi.importActual<typeof import("fs/promises")>("fs/promises")
18+
return {
19+
...actual,
20+
writeFile: vi.fn(actual.writeFile),
21+
readFile: vi.fn(actual.readFile),
22+
access: vi.fn(actual.access),
23+
mkdir: vi.fn(actual.mkdir),
24+
mkdtemp: vi.fn(actual.mkdtemp),
25+
rm: vi.fn(actual.rm),
26+
}
27+
})
28+
29+
vi.mock("fs", async () => {
30+
const actualFs = await vi.importActual<typeof import("fs")>("fs")
31+
return {
32+
...actualFs,
33+
createReadStream: vi.fn((path: string, options?: any) => actualFs.createReadStream(path, options)),
34+
}
35+
})
36+
37+
// Now import the mocked versions
38+
import * as fs from "fs/promises"
39+
import * as fsSyncActual from "fs"
40+
import * as path from "path"
41+
import * as os from "os"
42+
import * as properLockfile from "proper-lockfile"
43+
44+
describe("safeReadJson", () => {
45+
let originalConsoleError: typeof console.error
46+
let tempTestDir: string = ""
47+
let currentTestFilePath = ""
48+
49+
beforeAll(() => {
50+
// Store original console.error
51+
originalConsoleError = console.error
52+
53+
// Replace with filtered version that suppresses output from the module
54+
console.error = function (...args) {
55+
// Check if call originated from safeReadJson.ts
56+
if (new Error().stack?.includes("safeReadJson.ts")) {
57+
// Suppress output but allow spy recording
58+
return
59+
}
60+
61+
// Pass through all other calls (from tests)
62+
return originalConsoleError.apply(console, args)
63+
}
64+
})
65+
66+
afterAll(() => {
67+
// Restore original behavior
68+
console.error = originalConsoleError
69+
})
70+
71+
vi.useRealTimers() // Use real timers for this test suite
72+
73+
beforeEach(async () => {
74+
// Create a unique temporary directory for each test
75+
const tempDirPrefix = path.join(os.tmpdir(), "safeReadJson-test-")
76+
tempTestDir = await fs.mkdtemp(tempDirPrefix)
77+
currentTestFilePath = path.join(tempTestDir, "test-data.json")
78+
})
79+
80+
afterEach(async () => {
81+
if (tempTestDir) {
82+
try {
83+
await fs.rm(tempTestDir, { recursive: true, force: true })
84+
} catch (err) {
85+
console.error("Failed to clean up temp directory", err)
86+
}
87+
tempTestDir = ""
88+
}
89+
90+
// Reset all mocks
91+
vi.resetAllMocks()
92+
})
93+
94+
// Helper function to write a JSON file for testing
95+
const writeJsonFile = async (filePath: string, data: any): Promise<void> => {
96+
await fs.writeFile(filePath, JSON.stringify(data), "utf8")
97+
}
98+
99+
// Success Scenarios
100+
test("should successfully read a JSON file", async () => {
101+
const testData = { message: "Hello, world!" }
102+
await writeJsonFile(currentTestFilePath, testData)
103+
104+
const result = await safeReadJson(currentTestFilePath)
105+
expect(result).toEqual(testData)
106+
})
107+
108+
test("should throw an error for a non-existent file", async () => {
109+
const nonExistentPath = path.join(tempTestDir, "non-existent.json")
110+
111+
await expect(safeReadJson(nonExistentPath)).rejects.toThrow(/ENOENT/)
112+
})
113+
114+
test("should read a specific path from a JSON file", async () => {
115+
const testData = {
116+
user: {
117+
name: "John",
118+
age: 30,
119+
address: {
120+
city: "New York",
121+
zip: "10001",
122+
},
123+
},
124+
settings: {
125+
theme: "dark",
126+
notifications: true,
127+
},
128+
}
129+
await writeJsonFile(currentTestFilePath, testData)
130+
131+
// Test reading a specific path
132+
const result = await safeReadJson(currentTestFilePath, "user.address.city")
133+
expect(result).toBe("New York")
134+
})
135+
136+
test("should read multiple paths from a JSON file", async () => {
137+
const testData = {
138+
user: {
139+
name: "John",
140+
age: 30,
141+
},
142+
settings: {
143+
theme: "dark",
144+
notifications: true,
145+
},
146+
}
147+
await writeJsonFile(currentTestFilePath, testData)
148+
149+
// Test reading multiple paths
150+
const result = await safeReadJson(currentTestFilePath, ["user.name", "settings.theme"])
151+
expect(result).toEqual(["John", "dark"])
152+
})
153+
154+
// Failure Scenarios
155+
test("should handle JSON parsing errors", async () => {
156+
// Write invalid JSON
157+
await fs.writeFile(currentTestFilePath, "{ invalid: json", "utf8")
158+
159+
await expect(safeReadJson(currentTestFilePath)).rejects.toThrow()
160+
})
161+
162+
test("should handle file access errors", async () => {
163+
const accessSpy = vi.spyOn(fs, "access")
164+
accessSpy.mockImplementationOnce(async () => {
165+
const err = new Error("Simulated EACCES Error") as NodeJS.ErrnoException
166+
err.code = "EACCES" // Simulate a permissions error
167+
throw err
168+
})
169+
170+
await expect(safeReadJson(currentTestFilePath)).rejects.toThrow("Simulated EACCES Error")
171+
172+
accessSpy.mockRestore()
173+
})
174+
175+
test("should handle stream errors", async () => {
176+
await writeJsonFile(currentTestFilePath, { test: "data" })
177+
178+
// Mock createReadStream to simulate a failure during streaming
179+
;(fsSyncActual.createReadStream as ReturnType<typeof vi.fn>).mockImplementationOnce(
180+
(_path: any, _options: any) => {
181+
const stream = new Readable({
182+
read() {
183+
this.emit("error", new Error("Simulated Stream Error"))
184+
},
185+
})
186+
return stream as fsSyncActual.ReadStream
187+
},
188+
)
189+
190+
await expect(safeReadJson(currentTestFilePath)).rejects.toThrow("Simulated Stream Error")
191+
})
192+
193+
test("should handle lock acquisition failures", async () => {
194+
await writeJsonFile(currentTestFilePath, { test: "data" })
195+
196+
// Mock proper-lockfile to simulate a lock acquisition failure
197+
const lockSpy = vi.spyOn(properLockfile, "lock").mockRejectedValueOnce(new Error("Failed to get lock"))
198+
199+
await expect(safeReadJson(currentTestFilePath)).rejects.toThrow("Failed to get lock")
200+
201+
expect(lockSpy).toHaveBeenCalledWith(expect.stringContaining(currentTestFilePath), expect.any(Object))
202+
203+
lockSpy.mockRestore()
204+
})
205+
206+
test("should release lock even if an error occurs during reading", async () => {
207+
await writeJsonFile(currentTestFilePath, { test: "data" })
208+
209+
// Mock createReadStream to simulate a failure during streaming
210+
;(fsSyncActual.createReadStream as ReturnType<typeof vi.fn>).mockImplementationOnce(
211+
(_path: any, _options: any) => {
212+
const stream = new Readable({
213+
read() {
214+
this.emit("error", new Error("Simulated Stream Error"))
215+
},
216+
})
217+
return stream as fsSyncActual.ReadStream
218+
},
219+
)
220+
221+
await expect(safeReadJson(currentTestFilePath)).rejects.toThrow("Simulated Stream Error")
222+
223+
// Lock should be released, meaning the .lock file should not exist
224+
const lockPath = `${path.resolve(currentTestFilePath)}.lock`
225+
await expect(fs.access(lockPath)).rejects.toThrow(expect.objectContaining({ code: "ENOENT" }))
226+
})
227+
228+
// Edge Cases
229+
test("should handle empty JSON files", async () => {
230+
await fs.writeFile(currentTestFilePath, "", "utf8")
231+
232+
await expect(safeReadJson(currentTestFilePath)).rejects.toThrow()
233+
})
234+
235+
test("should handle large JSON files", async () => {
236+
// Create a large JSON object
237+
const largeData: Record<string, number> = {}
238+
for (let i = 0; i < 10000; i++) {
239+
largeData[`key${i}`] = i
240+
}
241+
242+
await writeJsonFile(currentTestFilePath, largeData)
243+
244+
const result = await safeReadJson(currentTestFilePath)
245+
expect(result).toEqual(largeData)
246+
})
247+
248+
test("should handle path selection for non-existent paths", async () => {
249+
const testData = { user: { name: "John" } }
250+
await writeJsonFile(currentTestFilePath, testData)
251+
252+
// Test reading a non-existent path
253+
const result = await safeReadJson(currentTestFilePath, "user.address")
254+
expect(result).toBeUndefined()
255+
})
256+
})

0 commit comments

Comments
 (0)