Skip to content

Commit 26440d9

Browse files
author
Eric Wheeler
committed
refactor: replace JSON.stringify with safeWriteJson for file operations
Replace all non-test instances of JSON.stringify used for writing to JSON files with safeWriteJson to ensure safer file operations with proper locking, error handling, and atomic writes. - Updated src/services/mcp/McpHub.ts - Updated src/services/code-index/cache-manager.ts - Updated src/api/providers/fetchers/modelEndpointCache.ts - Updated src/api/providers/fetchers/modelCache.ts - Updated tests to match the new implementation Signed-off-by: Eric Wheeler <[email protected]>
1 parent 7edda69 commit 26440d9

File tree

5 files changed

+24
-20
lines changed

5 files changed

+24
-20
lines changed

src/api/providers/fetchers/modelCache.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import * as path from "path"
22
import fs from "fs/promises"
33

44
import NodeCache from "node-cache"
5+
import { safeWriteJson } from "../../../utils/safeWriteJson"
56

67
import { ContextProxy } from "../../../core/config/ContextProxy"
78
import { getCacheDirectoryPath } from "../../../utils/storage"
@@ -19,7 +20,7 @@ const memoryCache = new NodeCache({ stdTTL: 5 * 60, checkperiod: 5 * 60 })
1920
async function writeModels(router: RouterName, data: ModelRecord) {
2021
const filename = `${router}_models.json`
2122
const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath)
22-
await fs.writeFile(path.join(cacheDir, filename), JSON.stringify(data))
23+
await safeWriteJson(path.join(cacheDir, filename), data)
2324
}
2425

2526
async function readModels(router: RouterName): Promise<ModelRecord | undefined> {

src/api/providers/fetchers/modelEndpointCache.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import * as path from "path"
22
import fs from "fs/promises"
33

44
import NodeCache from "node-cache"
5+
import { safeWriteJson } from "../../../utils/safeWriteJson"
56
import sanitize from "sanitize-filename"
67

78
import { ContextProxy } from "../../../core/config/ContextProxy"
@@ -18,7 +19,7 @@ const getCacheKey = (router: RouterName, modelId: string) => sanitize(`${router}
1819
async function writeModelEndpoints(key: string, data: ModelRecord) {
1920
const filename = `${key}_endpoints.json`
2021
const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath)
21-
await fs.writeFile(path.join(cacheDir, filename), JSON.stringify(data, null, 2))
22+
await safeWriteJson(path.join(cacheDir, filename), data)
2223
}
2324

2425
async function readModelEndpoints(key: string): Promise<ModelRecord | undefined> {

src/services/code-index/__tests__/cache-manager.test.ts

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import * as vscode from "vscode"
22
import { createHash } from "crypto"
33
import debounce from "lodash.debounce"
44
import { CacheManager } from "../cache-manager"
5+
import { safeWriteJson } from "../../../utils/safeWriteJson"
56

67
// Mock vscode
78
jest.mock("vscode", () => ({
@@ -11,12 +12,16 @@ jest.mock("vscode", () => ({
1112
workspace: {
1213
fs: {
1314
readFile: jest.fn(),
14-
writeFile: jest.fn(),
1515
delete: jest.fn(),
1616
},
1717
},
1818
}))
1919

20+
// Mock safeWriteJson
21+
jest.mock("../../../utils/safeWriteJson", () => ({
22+
safeWriteJson: jest.fn(),
23+
}))
24+
2025
// Mock debounce to execute immediately
2126
jest.mock("lodash.debounce", () => jest.fn((fn) => fn))
2227

@@ -88,7 +93,7 @@ describe("CacheManager", () => {
8893
cacheManager.updateHash(filePath, hash)
8994

9095
expect(cacheManager.getHash(filePath)).toBe(hash)
91-
expect(vscode.workspace.fs.writeFile).toHaveBeenCalled()
96+
expect(safeWriteJson).toHaveBeenCalled()
9297
})
9398

9499
it("should delete hash and trigger save", () => {
@@ -99,7 +104,7 @@ describe("CacheManager", () => {
99104
cacheManager.deleteHash(filePath)
100105

101106
expect(cacheManager.getHash(filePath)).toBeUndefined()
102-
expect(vscode.workspace.fs.writeFile).toHaveBeenCalled()
107+
expect(safeWriteJson).toHaveBeenCalled()
103108
})
104109

105110
it("should return shallow copy of hashes", () => {
@@ -124,18 +129,14 @@ describe("CacheManager", () => {
124129

125130
cacheManager.updateHash(filePath, hash)
126131

127-
expect(vscode.workspace.fs.writeFile).toHaveBeenCalledWith(mockCachePath, expect.any(Uint8Array))
132+
expect(safeWriteJson).toHaveBeenCalledWith(mockCachePath.fsPath, { [filePath]: hash })
128133

129-
// Verify the saved data
130-
const savedData = JSON.parse(
131-
Buffer.from((vscode.workspace.fs.writeFile as jest.Mock).mock.calls[0][1]).toString(),
132-
)
133-
expect(savedData).toEqual({ [filePath]: hash })
134+
// No need to parse the data since safeWriteJson receives the object directly
134135
})
135136

136137
it("should handle save errors gracefully", async () => {
137138
const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation()
138-
;(vscode.workspace.fs.writeFile as jest.Mock).mockRejectedValue(new Error("Save failed"))
139+
;(safeWriteJson as jest.Mock).mockRejectedValue(new Error("Save failed"))
139140

140141
cacheManager.updateHash("test.ts", "hash")
141142

@@ -152,19 +153,19 @@ describe("CacheManager", () => {
152153
it("should clear cache file and reset state", async () => {
153154
cacheManager.updateHash("test.ts", "hash")
154155

155-
// Reset the mock to ensure writeFile succeeds for clearCacheFile
156-
;(vscode.workspace.fs.writeFile as jest.Mock).mockClear()
157-
;(vscode.workspace.fs.writeFile as jest.Mock).mockResolvedValue(undefined)
156+
// Reset the mock to ensure safeWriteJson succeeds for clearCacheFile
157+
;(safeWriteJson as jest.Mock).mockClear()
158+
;(safeWriteJson as jest.Mock).mockResolvedValue(undefined)
158159

159160
await cacheManager.clearCacheFile()
160161

161-
expect(vscode.workspace.fs.writeFile).toHaveBeenCalledWith(mockCachePath, Buffer.from("{}"))
162+
expect(safeWriteJson).toHaveBeenCalledWith(mockCachePath.fsPath, {})
162163
expect(cacheManager.getAllHashes()).toEqual({})
163164
})
164165

165166
it("should handle clear errors gracefully", async () => {
166167
const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation()
167-
;(vscode.workspace.fs.writeFile as jest.Mock).mockRejectedValue(new Error("Save failed"))
168+
;(safeWriteJson as jest.Mock).mockRejectedValue(new Error("Save failed"))
168169

169170
await cacheManager.clearCacheFile()
170171

src/services/code-index/cache-manager.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import * as vscode from "vscode"
22
import { createHash } from "crypto"
33
import { ICacheManager } from "./interfaces/cache"
44
import debounce from "lodash.debounce"
5+
import { safeWriteJson } from "../../utils/safeWriteJson"
56

67
/**
78
* Manages the cache for code indexing
@@ -46,7 +47,7 @@ export class CacheManager implements ICacheManager {
4647
*/
4748
private async _performSave(): Promise<void> {
4849
try {
49-
await vscode.workspace.fs.writeFile(this.cachePath, Buffer.from(JSON.stringify(this.fileHashes, null, 2)))
50+
await safeWriteJson(this.cachePath.fsPath, this.fileHashes)
5051
} catch (error) {
5152
console.error("Failed to save cache:", error)
5253
}
@@ -57,7 +58,7 @@ export class CacheManager implements ICacheManager {
5758
*/
5859
async clearCacheFile(): Promise<void> {
5960
try {
60-
await vscode.workspace.fs.writeFile(this.cachePath, Buffer.from("{}"))
61+
await safeWriteJson(this.cachePath.fsPath, {})
6162
this.fileHashes = {}
6263
} catch (error) {
6364
console.error("Failed to clear cache file:", error, this.cachePath)

src/services/mcp/McpHub.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1135,7 +1135,7 @@ export class McpHub {
11351135
mcpServers: config.mcpServers,
11361136
}
11371137

1138-
await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2))
1138+
await safeWriteJson(configPath, updatedConfig)
11391139
}
11401140

11411141
public async updateServerTimeout(

0 commit comments

Comments
 (0)