Skip to content

Commit 6ebf91f

Browse files
jhssilvajhssilva
committed
refactor(error-tracking): remove readFilePrefix from core/helpers/fs
Per review, readFilePrefix had a single call site (debugId.ts) so it didn't warrant living in the shared fs helpers. The byte-limited read is now implemented directly inside debugId.ts. Since debugId.ts's file read is no longer mockable through the shared addFixtureFiles fixture system, its tests (and the one sender.ts test that exercises real debug_id extraction) now write real temp files instead, matching the pattern already used by fs.test.ts's own (now-removed) readFilePrefix tests.
1 parent 52b5952 commit 6ebf91f

6 files changed

Lines changed: 69 additions & 81 deletions

File tree

‎packages/core/src/helpers/fs.test.ts‎

Lines changed: 1 addition & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,9 @@
33
// Copyright 2019-Present Datadog, Inc.
44

55
import { addFixtureFiles } from '@dd/tests/_jest/helpers/mocks';
6-
import os from 'os';
76
import path from 'path';
87

9-
import { checkFile, outputFileSync, readFilePrefix, rmSync } from './fs';
8+
import { checkFile } from './fs';
109

1110
jest.mock('fs/promises', () => {
1211
const original = jest.requireActual('fs/promises');
@@ -34,27 +33,3 @@ describe('checkFile', () => {
3433
expect(validity).toEqual(expected);
3534
});
3635
});
37-
38-
describe('readFilePrefix', () => {
39-
const tempDir = path.join(os.tmpdir(), 'dd-build-plugins-fs-test');
40-
41-
afterEach(() => {
42-
rmSync(tempDir);
43-
});
44-
45-
test('Should return the whole file when it is smaller than maxBytes.', async () => {
46-
const filePath = path.join(tempDir, 'small.js');
47-
outputFileSync(filePath, 'short content');
48-
49-
const content = await readFilePrefix(filePath, 1024);
50-
expect(content).toBe('short content');
51-
});
52-
53-
test('Should only return the first maxBytes of a file larger than maxBytes.', async () => {
54-
const filePath = path.join(tempDir, 'large.js');
55-
outputFileSync(filePath, 'a'.repeat(10_000));
56-
57-
const content = await readFilePrefix(filePath, 10);
58-
expect(content).toBe('a'.repeat(10));
59-
});
60-
});

‎packages/core/src/helpers/fs.ts‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -76,20 +76,6 @@ export const readFile = (filepath: string) => {
7676
return fsp.readFile(filepath, { encoding: 'utf-8' });
7777
};
7878

79-
// Read only the first `maxBytes` bytes of a file.
80-
// Useful when the data we need is guaranteed to live near the start of the file
81-
// and reading the whole (potentially large) file into memory would be wasteful.
82-
export const readFilePrefix = async (filepath: string, maxBytes: number): Promise<string> => {
83-
const fd = await fsp.open(filepath, 'r');
84-
try {
85-
const buffer = Buffer.alloc(maxBytes);
86-
const { bytesRead } = await fd.read(buffer, 0, maxBytes, 0);
87-
return buffer.toString('utf-8', 0, bytesRead);
88-
} finally {
89-
await fd.close();
90-
}
91-
};
92-
9379
export const readFileSync = (filepath: string) => {
9480
return fs.readFileSync(filepath, { encoding: 'utf-8' });
9581
};

‎packages/plugins/error-tracking/src/sourcemaps/debugId.test.ts‎

Lines changed: 32 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,43 +2,53 @@
22
// This product includes software developed at Datadog (https://www.datadoghq.com/).
33
// Copyright 2019-Present Datadog, Inc.
44

5-
import { addFixtureFiles } from '@dd/tests/_jest/helpers/mocks';
5+
import { outputFileSync, rmSync } from '@dd/core/helpers/fs';
6+
import os from 'os';
7+
import path from 'path';
68

79
import { extractDebugId } from './debugId';
810

9-
jest.mock('@dd/core/helpers/fs', () => {
10-
const original = jest.requireActual('@dd/core/helpers/fs');
11-
return {
12-
...original,
13-
readFilePrefix: jest.fn(),
14-
};
15-
});
16-
1711
describe('extractDebugId', () => {
1812
const debugId = '93fd4850-7b77-4f2e-9aa2-ba013e1a5027';
13+
const tempDir = path.join(os.tmpdir(), 'dd-build-plugins-debug-id-test');
14+
15+
afterEach(() => {
16+
rmSync(tempDir);
17+
});
1918

2019
test('Should extract the debug ID when the key is quoted (unminified JSON.stringify output)', async () => {
21-
addFixtureFiles({
22-
'/path/to/minified.min.js': `!function(){}({"service":"app","version":"1.0.0","ddDebugId":"${debugId}"},"DD_SOURCE_CODE_CONTEXT");`,
23-
});
24-
await expect(extractDebugId('/path/to/minified.min.js')).resolves.toBe(debugId);
20+
const filePath = path.join(tempDir, 'quoted.min.js');
21+
outputFileSync(
22+
filePath,
23+
`!function(){}({"service":"app","version":"1.0.0","ddDebugId":"${debugId}"},"DD_SOURCE_CODE_CONTEXT");`,
24+
);
25+
26+
await expect(extractDebugId(filePath)).resolves.toBe(debugId);
2527
});
2628

2729
test('Should extract the debug ID when the key is unquoted (minifiers strip quotes from valid identifier keys)', async () => {
28-
addFixtureFiles({
29-
'/path/to/minified.min.js': `!function(){}({service:"app",version:"1.0.0",ddDebugId:"${debugId}"},"DD_SOURCE_CODE_CONTEXT");`,
30-
});
31-
await expect(extractDebugId('/path/to/minified.min.js')).resolves.toBe(debugId);
30+
const filePath = path.join(tempDir, 'unquoted.min.js');
31+
outputFileSync(
32+
filePath,
33+
`!function(){}({service:"app",version:"1.0.0",ddDebugId:"${debugId}"},"DD_SOURCE_CODE_CONTEXT");`,
34+
);
35+
36+
await expect(extractDebugId(filePath)).resolves.toBe(debugId);
3237
});
3338

3439
test('Should return undefined when there is no debug ID in the content', async () => {
35-
addFixtureFiles({
36-
'/path/to/minified.min.js': `!function(){}({service:"app",version:"1.0.0"},"DD_SOURCE_CODE_CONTEXT");`,
37-
});
38-
await expect(extractDebugId('/path/to/minified.min.js')).resolves.toBeUndefined();
40+
const filePath = path.join(tempDir, 'no-debug-id.min.js');
41+
outputFileSync(
42+
filePath,
43+
`!function(){}({service:"app",version:"1.0.0"},"DD_SOURCE_CODE_CONTEXT");`,
44+
);
45+
46+
await expect(extractDebugId(filePath)).resolves.toBeUndefined();
3947
});
4048

4149
test('Should return undefined when the file cannot be read', async () => {
42-
await expect(extractDebugId('/path/to/missing.min.js')).resolves.toBeUndefined();
50+
const filePath = path.join(tempDir, 'missing.min.js');
51+
52+
await expect(extractDebugId(filePath)).resolves.toBeUndefined();
4353
});
4454
});

‎packages/plugins/error-tracking/src/sourcemaps/debugId.ts‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// This product includes software developed at Datadog (https://www.datadoghq.com/).
33
// Copyright 2019-Present Datadog, Inc.
44

5-
import { readFilePrefix } from '@dd/core/helpers/fs';
5+
import fsp from 'fs/promises';
66

77
// Matches the `ddDebugId:"<uuid>"` literal the RUM plugin injects into each chunk's own content
88
// (see packages/plugins/rum/src/getSourceCodeContextSnippet.ts). The key is quoted in source
@@ -24,14 +24,25 @@ const matchDebugId = (fileContent: string): string | undefined => {
2424
return DEBUG_ID_RX.exec(fileContent)?.[1];
2525
};
2626

27+
// Read only the first DEBUG_ID_SEARCH_PREFIX_BYTES bytes of the file, since that's all
28+
// we need to find the ddDebugId literal and reading the whole (potentially large)
29+
// minified bundle into memory would be wasteful.
30+
const readFilePrefix = async (filePath: string): Promise<string> => {
31+
const fd = await fsp.open(filePath, 'r');
32+
try {
33+
const buffer = Buffer.alloc(DEBUG_ID_SEARCH_PREFIX_BYTES);
34+
const { bytesRead } = await fd.read(buffer, 0, DEBUG_ID_SEARCH_PREFIX_BYTES, 0);
35+
return buffer.toString('utf-8', 0, bytesRead);
36+
} finally {
37+
await fd.close();
38+
}
39+
};
40+
2741
// Reads the minified file's own content and extracts the debug_id from it, instead of
2842
// trusting a filename as a coordination key with the RUM plugin — the bundler may still
2943
// rename the file after injection (e.g. webpack/rspack's realContentHash), but the content,
30-
// and the debug_id embedded in it, is unaffected. Only the file's prefix is read, since the
31-
// RUM plugin's injected snippet always lands near the start of the file.
44+
// and the debug_id embedded in it, is unaffected.
3245
export const extractDebugId = async (filePath: string): Promise<string | undefined> => {
33-
const fileContent = await readFilePrefix(filePath, DEBUG_ID_SEARCH_PREFIX_BYTES).catch(
34-
() => undefined,
35-
);
46+
const fileContent = await readFilePrefix(filePath).catch(() => undefined);
3647
return fileContent ? matchDebugId(fileContent) : undefined;
3748
};

‎packages/plugins/error-tracking/src/sourcemaps/sender.test.ts‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// This product includes software developed at Datadog (https://www.datadoghq.com/).
33
// Copyright 2019-Present Datadog, Inc.
44

5+
import { outputFileSync, rmSync } from '@dd/core/helpers/fs';
56
import { doRequest } from '@dd/core/helpers/request';
67
import {
78
getData,
@@ -21,6 +22,8 @@ import {
2122
getSourcemapsConfiguration,
2223
addFixtureFiles,
2324
} from '@dd/tests/_jest/helpers/mocks';
25+
import os from 'os';
26+
import path from 'path';
2427

2528
import * as payloadModule from './payload';
2629

@@ -30,7 +33,6 @@ jest.mock('@dd/core/helpers/fs', () => {
3033
...original,
3134
checkFile: jest.fn(),
3235
getFile: jest.fn(),
33-
readFilePrefix: jest.fn(),
3436
};
3537
});
3638

@@ -175,17 +177,27 @@ describe('Error Tracking Plugin Sourcemaps', () => {
175177
// renaming step (e.g. webpack/rspack's realContentHash) that happens after
176178
// the RUM plugin injects it.
177179
const debugId = '12345678-1234-4123-8123-123456789012';
180+
// Minifiers strip quotes from object keys that are valid identifiers, so the
181+
// real on-disk shape has an unquoted key, not `"ddDebugId":"..."`.
182+
const minifiedFileContent = `Some JS File with some content.(function(c,n){...})({ddDebugId:"${debugId}"},"DD_SOURCE_CODE_CONTEXT");`;
183+
// debugId.ts reads the minified file straight off disk (not through a mockable
184+
// fs helper), so it needs a real file on top of the virtual fixture used for
185+
// the checkFile validity checks below.
186+
const tempDir = path.join(os.tmpdir(), 'dd-build-plugins-sender-debug-id-test');
187+
const minifiedFilePath = path.join(tempDir, 'minified.min.js');
188+
outputFileSync(minifiedFilePath, minifiedFileContent);
189+
178190
addFixtureFiles({
179-
'/path/to/minified.min.js':
180-
// Minifiers strip quotes from object keys that are valid identifiers, so
181-
// the real on-disk shape has an unquoted key, not `"ddDebugId":"..."`.
182-
`Some JS File with some content.(function(c,n){...})({ddDebugId:"${debugId}"},"DD_SOURCE_CODE_CONTEXT");`,
191+
[minifiedFilePath]: minifiedFileContent,
183192
'/path/to/sourcemap.js.map': '{"version":3,"sources":["/path/to/minified.min.js"]}',
184193
});
185194

186195
const getPayloadSpy = jest.spyOn(payloadModule, 'getPayload');
187196

188-
const sourcemap = getSourcemapMock({ relativePath: 'path/to/minified.min.js' });
197+
const sourcemap = getSourcemapMock({
198+
minifiedFilePath,
199+
relativePath: 'path/to/minified.min.js',
200+
});
189201

190202
await sendSourcemaps(
191203
[sourcemap],
@@ -199,6 +211,7 @@ describe('Error Tracking Plugin Sourcemaps', () => {
199211
expect(debugIdArg).toBe(debugId);
200212

201213
getPayloadSpy.mockRestore();
214+
rmSync(tempDir);
202215
});
203216
});
204217

‎packages/tests/src/_jest/helpers/mocks.ts‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import {
88
getFile,
99
readFileSync,
1010
readFile,
11-
readFilePrefix,
1211
existsSync,
1312
outputFileSync,
1413
} from '@dd/core/helpers/fs';
@@ -481,7 +480,6 @@ const mockGetFile = jest.mocked(getFile);
481480
const mockCheckFile = jest.mocked(checkFile);
482481
const mockReadFileSync = jest.mocked(readFileSync);
483482
const mockReadFile = jest.mocked(readFile);
484-
const mockReadFilePrefix = jest.mocked(readFilePrefix);
485483
const mockExistsSync = jest.mocked(existsSync);
486484
const mockStat = jest.mocked(require('fs/promises').stat);
487485
const mockGlobSync = jest.mocked(require('glob').glob.sync);
@@ -537,11 +535,6 @@ export const addFixtureFiles = (files: Record<string, string>, buildRoot: string
537535
readFileImplementation(filePath),
538536
);
539537
}
540-
if (typeof mockReadFilePrefix.mockImplementation === 'function') {
541-
mockReadFilePrefix.mockImplementation(async (filePath: string, maxBytes: number) =>
542-
readFileImplementation(filePath).slice(0, maxBytes),
543-
);
544-
}
545538
if (typeof mockStat.mockImplementation === 'function') {
546539
mockStat.mockImplementation(async (filePath: PathLike) => {
547540
const resolvedPath = path.resolve(buildRoot, filePath.toString());

0 commit comments

Comments
 (0)