Skip to content

Commit cb0af18

Browse files
sdkennedy2claude
andcommitted
fix(apps): pair static module specifiers with resolved IDs by unique specifier
`collectStaticModuleDependencies` zipped the resolved dependency IDs the bundler supplies against every static specifier found in the AST. Bundlers report one entry per *unique* dependency, so a module importing the same specifier twice yields more specifiers than resolved IDs and the pairing slips by one from that point on — attributing imports to the wrong dependency and dropping the last one entirely. Verified against a real Rollup build. Six specifiers, five resolved IDs, three mispaired: ./dup -> reexport.js (should be dup.js) ./reexport -> star.js (should be reexport.js) ./star -> zlast.js (should be star.js) ./zlast -> dropped These pairings map an action-catalog call back to the module it came from and feed importsByVariable / exportsByName / starExports, so a wrong pairing can attribute a connectionId to the wrong file or miss it. Since allowedConnectionIds is an allowlist, a miss is a functional break — and the build still succeeds, so it fails silently. Present today on Vite 6/7. Deduplicate specifiers by source string, preserving first-occurrence order. Known gap, documented rather than fixed: Rollup dedups its resolved-ID list by specifier string while Rolldown dedups by resolved module ID, both verified against real builds. They agree unless one module imports the same file through two different specifiers, which still mispairs under Rolldown. Removing that dependence needs pairing by path correspondence instead of by position, which is a larger change than this fix. Also adds an end-to-end test driving connection ID collection through a real vite.build() and asserting the exact collected IDs; it passes on master unchanged, so it characterises existing behaviour. Co-Authored-By: Claude <noreply@anthropic.com>
1 parent ab3b293 commit cb0af18

3 files changed

Lines changed: 294 additions & 4 deletions

File tree

packages/plugins/apps/src/backend/ast-parsing/module-graph.test.ts

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,4 +270,91 @@ describe('Backend Functions - module graph records', () => {
270270
FOR_OF_CONNECTIONS: { kind: 'unsupported', reason: 'mutated object binding' },
271271
});
272272
});
273+
274+
describe('static dependency pairing', () => {
275+
// The caller supplies resolved IDs from the bundler, which reports one
276+
// per *unique* dependency in first-occurrence order. Pairing them against
277+
// every specifier in the AST slips by one as soon as a specifier repeats,
278+
// which attributes an import to the wrong dependency.
279+
const cases = [
280+
{
281+
description: 'pair duplicate imports of the same specifier once',
282+
code: `
283+
import { a } from './dup.js';
284+
import { b } from './dup.js';
285+
import { c } from './other.js';
286+
`,
287+
staticDependencies: ['/project/src/dup.js', '/project/src/other.js'],
288+
expected: [
289+
{ source: './dup.js', resolvedId: '/project/src/dup.js' },
290+
{ source: './other.js', resolvedId: '/project/src/other.js' },
291+
],
292+
},
293+
{
294+
// Rollup keys its resolved-ID list by specifier string, so it
295+
// reports two entries here. Rolldown keys by resolved module ID
296+
// and reports one — verified against real builds of both — so
297+
// this shape still mispairs under Rolldown. Fixing that needs
298+
// pairing by path correspondence instead of position.
299+
description: 'keep distinct specifiers that resolve to the same module apart',
300+
code: `
301+
import { a } from './dup';
302+
import { b } from './dup.js';
303+
`,
304+
staticDependencies: ['/project/src/dup.js', '/project/src/dup.js'],
305+
expected: [
306+
{ source: './dup', resolvedId: '/project/src/dup.js' },
307+
{ source: './dup.js', resolvedId: '/project/src/dup.js' },
308+
],
309+
},
310+
{
311+
description: 'pair side-effect imports, re-exports and star exports in order',
312+
code: `
313+
import './side.js';
314+
import { a } from './dup.js';
315+
import { b } from './dup.js';
316+
export { c } from './reexport.js';
317+
export * from './star.js';
318+
import { z } from './zlast.js';
319+
`,
320+
staticDependencies: [
321+
'/project/src/side.js',
322+
'/project/src/dup.js',
323+
'/project/src/reexport.js',
324+
'/project/src/star.js',
325+
'/project/src/zlast.js',
326+
],
327+
expected: [
328+
{ source: './side.js', resolvedId: '/project/src/side.js' },
329+
{ source: './dup.js', resolvedId: '/project/src/dup.js' },
330+
{ source: './reexport.js', resolvedId: '/project/src/reexport.js' },
331+
{ source: './star.js', resolvedId: '/project/src/star.js' },
332+
{ source: './zlast.js', resolvedId: '/project/src/zlast.js' },
333+
],
334+
},
335+
];
336+
337+
test.each(cases)('Should $description', ({ code, staticDependencies, expected }) => {
338+
const record = createRecord(code, staticDependencies);
339+
340+
expect(record.staticDependencies).toEqual(expected);
341+
});
342+
343+
test('Should resolve import bindings against the corrected pairing', () => {
344+
const record = createRecord(
345+
`
346+
import { a } from './dup.js';
347+
import { b } from './dup.js';
348+
import { c } from './other.js';
349+
`,
350+
['/project/src/dup.js', '/project/src/other.js'],
351+
);
352+
353+
expect(bindingsByVariableName<ImportBinding>(record.importsByVariable)).toEqual({
354+
a: { kind: 'named', importedName: 'a', resolvedId: '/project/src/dup.js' },
355+
b: { kind: 'named', importedName: 'b', resolvedId: '/project/src/dup.js' },
356+
c: { kind: 'named', importedName: 'c', resolvedId: '/project/src/other.js' },
357+
});
358+
});
359+
});
273360
});

packages/plugins/apps/src/backend/ast-parsing/module-graph.ts

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,27 @@ export function createParsedModuleRecord(
178178
};
179179
}
180180

181+
/**
182+
* Pairs each static module specifier with the ID the bundler resolved it to.
183+
*
184+
* The two sequences are matched by position, which is only sound once the
185+
* specifiers are deduplicated: bundlers report one entry per *unique* dependency,
186+
* so a module importing `'./a'` twice yields two specifiers but one resolved ID,
187+
* and zipping the raw list slips by one from that point on — attributing an
188+
* import to the wrong dependency and dropping the last one entirely.
189+
*
190+
* Bundlers disagree on what "unique" means, and this is verified against real
191+
* builds rather than assumed: Rollup 4 keys by specifier string, so `'./a'` and
192+
* `'./a.js'` stay separate; Rolldown 1.1.5 keys by resolved module ID, so they
193+
* collapse into one entry. The two therefore agree exactly when no two
194+
* specifiers in a module resolve to the same file, which is the ordinary case.
195+
*
196+
* Known gap, deliberately not fixed here: under Rolldown a module that imports
197+
* one file through two different specifiers still mispairs, because dedup by
198+
* specifier yields more entries than the bundler reports. Pairing by path
199+
* correspondence rather than position would remove the dependence on either
200+
* bundler's dedup rule; that is a larger change than this fix.
201+
*/
181202
function collectStaticModuleDependencies(
182203
ast: Program,
183204
staticDependencyIds: string[],
@@ -190,20 +211,26 @@ function collectStaticModuleDependencies(
190211
}));
191212
}
192213

214+
/**
215+
* Unique static module specifiers, in first-occurrence order, matching how
216+
* bundlers order the resolved dependency IDs they report.
217+
*/
193218
function getStaticModuleSources(ast: Program): string[] {
194-
return ast.body.flatMap((node) => {
219+
const sources = new Set<string>();
220+
221+
for (const node of ast.body) {
195222
if (
196223
(node.type === 'ImportDeclaration' ||
197224
node.type === 'ExportNamedDeclaration' ||
198225
node.type === 'ExportAllDeclaration') &&
199226
node.source &&
200227
isStringLiteral(node.source)
201228
) {
202-
return [node.source.value];
229+
sources.add(node.source.value);
203230
}
231+
}
204232

205-
return [];
206-
});
233+
return [...sources];
207234
}
208235

209236
function collectImportBindings(
Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License.
2+
// This product includes software developed at Datadog (https://www.datadoghq.com/).
3+
// Copyright 2019-Present Datadog, Inc.
4+
5+
import { outputFile, rm } from '@dd/core/helpers/fs';
6+
import { mkdtemp, realpath } from 'fs/promises';
7+
import { tmpdir } from 'os';
8+
import path from 'path';
9+
import type { Plugin } from 'vite';
10+
import { build } from 'vite';
11+
12+
import { createBackendConnectionIdCollector } from './backend-connection-id-collector';
13+
import { getBaseBackendBuildConfig } from './build-config';
14+
15+
const ACTION_CATALOG_ID = '\0action-catalog-stub';
16+
17+
/**
18+
* Stands in for `@datadog/action-catalog`, which is not installed in this repo.
19+
* The collector only reads the import specifier written in app source, so a
20+
* virtual stub is indistinguishable from the real package for this purpose —
21+
* and its `\0` prefix means the collector skips the stub module itself.
22+
*/
23+
const actionCatalogStub = (): Plugin => ({
24+
name: 'action-catalog-stub',
25+
enforce: 'pre',
26+
resolveId(id) {
27+
return id.startsWith('@datadog/action-catalog') ? ACTION_CATALOG_ID : null;
28+
},
29+
load(id) {
30+
return id === ACTION_CATALOG_ID ? 'export function request() { return null; }' : null;
31+
},
32+
});
33+
34+
/**
35+
* Runs a real `vite.build()` over on-disk sources so the collector sees exactly
36+
* what a bundler hands it — post-`transform`, TypeScript already stripped.
37+
*
38+
* The unit tests feed the collector hand-written source, which cannot show
39+
* whether the constructs it statically matches on (the `@datadog/action-catalog`
40+
* import, the call site, the `connectionId` literal) actually survive Vite's
41+
* transform pipeline. That is the load-bearing assumption behind reading
42+
* `moduleInfo.code` instead of the bundler's AST, so it needs a real build.
43+
*/
44+
async function collectConnectionIds(files: Record<string, string>): Promise<string[]> {
45+
// `realpath` because macOS hands out `/var/...` temp dirs that Vite reports
46+
// as `/private/var/...`; the collector compares module IDs against the build
47+
// root as plain strings, so both sides have to agree.
48+
const createdRoot = await mkdtemp(path.join(tmpdir(), 'dd-apps-conn-ids-'));
49+
const root = await realpath(createdRoot);
50+
51+
try {
52+
for (const [relativePath, contents] of Object.entries(files)) {
53+
const absolutePath = path.join(root, relativePath);
54+
await outputFile(absolutePath, contents);
55+
}
56+
57+
const entryPath = path.join(root, 'entry.backend.ts');
58+
const collector = createBackendConnectionIdCollector(entryPath, root);
59+
const stub = actionCatalogStub();
60+
const baseConfig = getBaseBackendBuildConfig(root, {}, [collector.plugin, stub]);
61+
62+
await build({
63+
...baseConfig,
64+
build: {
65+
...baseConfig.build,
66+
write: false,
67+
rollupOptions: {
68+
...baseConfig.build.rollupOptions,
69+
input: { entry: entryPath },
70+
},
71+
},
72+
});
73+
74+
return collector.getAllowedConnectionIds();
75+
} finally {
76+
await rm(root);
77+
}
78+
}
79+
80+
describe('Backend Functions - connection ID collection through a real Vite build', () => {
81+
test('Should collect a connection ID written inline in the entry', async () => {
82+
const connectionIds = await collectConnectionIds({
83+
'entry.backend.ts': `
84+
import { request } from '@datadog/action-catalog/http/http';
85+
86+
export async function run(value: string): Promise<unknown> {
87+
return request({ connectionId: 'conn-inline', inputs: { value } });
88+
}
89+
`,
90+
});
91+
92+
expect(connectionIds).toEqual(['conn-inline']);
93+
});
94+
95+
test('Should collect connection IDs through transitive app-local modules', async () => {
96+
const connectionIds = await collectConnectionIds({
97+
'entry.backend.ts': `
98+
import { callDeep } from './helpers/deep';
99+
import { callNear } from './helpers/near';
100+
101+
export async function run(): Promise<unknown> {
102+
return Promise.all([callNear(), callDeep()]);
103+
}
104+
`,
105+
'helpers/near.ts': `
106+
import { request } from '@datadog/action-catalog/http/http';
107+
108+
export function callNear() {
109+
return request({ connectionId: 'conn-near', inputs: {} });
110+
}
111+
`,
112+
'helpers/deep.ts': `
113+
import { request } from '@datadog/action-catalog/http/http';
114+
115+
import { DEEP_ID } from './ids';
116+
117+
export function callDeep() {
118+
return request({ connectionId: DEEP_ID, inputs: {} });
119+
}
120+
`,
121+
'helpers/ids.ts': `
122+
export const DEEP_ID = 'conn-deep';
123+
`,
124+
});
125+
126+
expect(connectionIds).toEqual(['conn-deep', 'conn-near']);
127+
});
128+
129+
test('Should resolve a connection ID that TypeScript syntax wraps and re-exports', async () => {
130+
const connectionIds = await collectConnectionIds({
131+
'entry.backend.ts': `
132+
import { request } from '@datadog/action-catalog/http/http';
133+
134+
import { WRAPPED_ID } from './ids';
135+
136+
export async function run(): Promise<unknown> {
137+
return request({ connectionId: WRAPPED_ID, inputs: {} });
138+
}
139+
`,
140+
'ids.ts': `
141+
export { WRAPPED_ID } from './ids-source';
142+
`,
143+
'ids-source.ts': `
144+
export const WRAPPED_ID = 'conn-wrapped' as const;
145+
`,
146+
});
147+
148+
expect(connectionIds).toEqual(['conn-wrapped']);
149+
});
150+
151+
test('Should not attribute connection IDs to a module that only shares a specifier prefix', async () => {
152+
const connectionIds = await collectConnectionIds({
153+
'entry.backend.ts': `
154+
import { callUsed } from './helpers/used';
155+
import { callUsedToo } from './helpers/used.ts';
156+
157+
export async function run(): Promise<unknown> {
158+
return Promise.all([callUsed(), callUsedToo()]);
159+
}
160+
`,
161+
'helpers/used.ts': `
162+
import { request } from '@datadog/action-catalog/http/http';
163+
164+
export function callUsed() {
165+
return request({ connectionId: 'conn-used', inputs: {} });
166+
}
167+
168+
export function callUsedToo() {
169+
return request({ connectionId: 'conn-used-too', inputs: {} });
170+
}
171+
`,
172+
});
173+
174+
expect(connectionIds).toEqual(['conn-used', 'conn-used-too']);
175+
});
176+
});

0 commit comments

Comments
 (0)