diff --git a/cli/src/commands/sync/sync.ts b/cli/src/commands/sync/sync.ts index 38b9e87f84..138a346594 100644 --- a/cli/src/commands/sync/sync.ts +++ b/cli/src/commands/sync/sync.ts @@ -128,6 +128,8 @@ import { } from "../../utils/metadata.ts"; import { DoubleLinkedDependencyTree, + LocalScripts, + resolvePlaceholdersFromLocal, uploadScripts, } from "../../utils/dependency_tree.ts"; import { @@ -176,6 +178,7 @@ import { isDbtModulePath, isDbtGeneratedPath, isModuleEntryPoint, + scriptPathToRemotePath, getScriptBasePathFromModulePath, hasWrongFormatSuffix, DBT_DESCRIPTOR_NAME, @@ -3385,6 +3388,37 @@ async function addToChangedIfNotExists(p: string, tracker: ChangeTracker) { } } +/** + * Index the checkout's standalone scripts by the remote path a relative import + * resolves to, reusing the content the local/remote diff already read. + * + * Same classification as `addToChangedIfNotExists`: a flow or app inline script + * is not addressable as an import target, and a module bundle is addressed by + * its entry point. + */ +function localScriptsByRemotePath( + localMap: Record, +): LocalScripts { + const byRemotePath: LocalScripts = new Map(); + for (const [p, content] of Object.entries(localMap)) { + if (isScriptModulePath(p)) { + if (!isModuleEntryPoint(p)) continue; + } else if ( + !hasScriptExt(p) || + isDatatableMigrationPath(p) || + isFileResource(p) || + isFilesetResource(p) || + isFlowPath(p) || + isAppPath(p) || + isRawAppPath(p) + ) { + continue; + } + byRemotePath.set(scriptPathToRemotePath(p), { localPath: p, content }); + } + return byRemotePath; +} + export async function buildTracker(changes: Change[]) { const tracker: ChangeTracker = { scripts: [], @@ -5074,6 +5108,14 @@ export async function push( } if (autoRegenerate && tree) { + // Pass 1 only ever walks the change set, so anything imported through a + // module the push leaves alone is still a dead end here. + await resolvePlaceholdersFromLocal( + tree, + localScriptsByRemotePath(localMap), + opts.defaultTs, + ); + // Propagate staleness through imports + upload script content to // raw_script_temp so the dep job can resolve cross-folder relative imports // via temp_script_refs (instead of hitting 404s for not-yet-deployed diff --git a/cli/src/utils/dependency_tree.ts b/cli/src/utils/dependency_tree.ts index b0b75e659e..959a77b478 100644 --- a/cli/src/utils/dependency_tree.ts +++ b/cli/src/utils/dependency_tree.ts @@ -5,7 +5,7 @@ import { Workspace } from "../commands/workspace/workspace.ts"; import * as wmill from "../../gen/services.gen.ts"; import type { ScriptLang } from "../../gen/types.gen.ts"; -import { ScriptLanguage } from "./script_common.ts"; +import { ScriptLanguage, inferContentTypeFromFilePath } from "./script_common.ts"; import { filterWorkspaceDependencies, generateScriptHash, @@ -14,6 +14,65 @@ import { updateMetadataGlobalLock, } from "./metadata.ts"; import { generateHash } from "./utils.ts"; +import { extractRelativeImports } from "./relative_imports.ts"; + +/** A local script file, keyed in `LocalScripts` by its Windmill remote path. */ +export interface LocalScriptSource { + localPath: string; + content: string; +} +export type LocalScripts = Map; + +/** + * Give every import target that is only a placeholder its local content and its + * own imports, so the graph continues through it. A tree seeded from a subset of + * the checkout otherwise dead-ends at any module outside that subset — a + * re-export barrel needing no edit, typically — hiding what it re-exports from + * `getTempScriptRefs`, which then resolves it against the deployed copy. + */ +export async function resolvePlaceholdersFromLocal( + tree: DoubleLinkedDependencyTree, + localScripts: LocalScripts, + defaultTs: "bun" | "deno" | undefined +): Promise { + // A module resolved here can expose placeholders of its own (a barrel behind + // a barrel), so keep going until a round resolves nothing. + for (;;) { + let resolved = false; + for (const remotePath of tree.placeholderPaths()) { + const local = localScripts.get(remotePath); + if (!local) continue; + let language: ScriptLanguage; + try { + language = inferContentTypeFromFilePath(local.localPath, defaultTs); + } catch { + // A bare `.sql` names no dialect, so its imports cannot be read here. + continue; + } + const imports = await extractRelativeImports( + local.content, + remotePath, + language + ); + // Never directly stale: it is outside the change set, so nothing relocks + // it. It is here to carry edges, and to be uploaded if it differs from + // what is deployed. + await tree.addNode( + remotePath, + local.content, + language, + "", + imports, + "script", + remotePath, + local.localPath, + false + ); + resolved = true; + } + if (!resolved) break; + } +} /** * Diff local scripts against deployed versions, upload only those that differ. @@ -97,6 +156,9 @@ interface DependencyNode { originalPath: string; // Original path passed to handler (with extension for scripts) isRawApp?: boolean; // Only set for apps isDirectlyStale: boolean; // True if this item's content changed (vs transitively stale) + // True while the node exists only because something imports it, so it carries + // no content and no imports of its own. + isPlaceholder: boolean; } export class DoubleLinkedDependencyTree { @@ -130,9 +192,11 @@ export class DoubleLinkedDependencyTree { content: "", stalenessHash: "", language: "deno", metadata: "", imports: new Set(), importedBy: new Set(), itemType: "script", folder: "", originalPath: "", isDirectlyStale: false, + isPlaceholder: true, }); } const node = this.nodes.get(path)!; + node.isPlaceholder = false; node.content = content; node.stalenessHash = stalenessHash; node.language = language; @@ -155,7 +219,7 @@ export class DoubleLinkedDependencyTree { stalenessHash: "", language: depsInfo?.language ?? "deno", metadata: "", imports: new Set(), importedBy: new Set(), itemType: "dependencies", folder: "", originalPath: depsPath, - isDirectlyStale: !isUpToDate, + isDirectlyStale: !isUpToDate, isPlaceholder: false, }); } } @@ -169,6 +233,7 @@ export class DoubleLinkedDependencyTree { content: "", stalenessHash: "", language: "deno", metadata: "", imports: new Set(), importedBy: new Set(), itemType: "script", folder: "", originalPath: "", isDirectlyStale: false, + isPlaceholder: true, }); } this.nodes.get(importPath)!.importedBy.add(path); @@ -309,6 +374,18 @@ export class DoubleLinkedDependencyTree { return this.nodes.keys(); } + /** + * Paths that exist only as somebody's import target, so the traversal stops + * at them instead of continuing into what they themselves import. + */ + placeholderPaths(): string[] { + const result: string[] = []; + for (const [path, node] of this.nodes.entries()) { + if (node.isPlaceholder) result.push(path); + } + return result; + } + /** * Returns paths of all stale nodes (those with a staleReason). */ diff --git a/cli/test/dependency_tree_unit.test.ts b/cli/test/dependency_tree_unit.test.ts index 1a92bb9894..8e9c733c00 100644 --- a/cli/test/dependency_tree_unit.test.ts +++ b/cli/test/dependency_tree_unit.test.ts @@ -2,7 +2,10 @@ import { expect, test } from "bun:test"; import { mkdtemp, rm } from "node:fs/promises"; import os from "node:os"; import * as path from "node:path"; -import { DoubleLinkedDependencyTree } from "../src/utils/dependency_tree.ts"; +import { + DoubleLinkedDependencyTree, + resolvePlaceholdersFromLocal, +} from "../src/utils/dependency_tree.ts"; // addNode consults wmill-lock.yaml from cwd for workspace deps; run inside a // temp dir so the test never reads/writes the repo's own lock file. @@ -104,3 +107,62 @@ test("getAllTempScriptRefs is a superset of getTempScriptRefs for any node", asy }); }); }); + +// Two barrels deep so the fixpoint matters: resolving the first one is what +// puts the second in the tree, and only a further round reaches the leaf. +test("resolvePlaceholdersFromLocal walks the graph through unresolved barrels", async () => { + await withTempDir(async () => { + const tree = new DoubleLinkedDependencyTree(); + await tree.addNode( + "f/app/consumer", + `import { subtract } from "../barrel/index.ts"`, + "bun", + "", + ["f/barrel/index"], + "script", + "f/app/consumer", + "f/app/consumer.ts", + true, + ); + await tree.addNode( + "f/barrel/helper", + "export function subtract(a: number, b: number) { return a - b }", + "bun", + "", + [], + "script", + "f/barrel/helper", + "f/barrel/helper.ts", + true, + ); + // Neither barrel is in the change set, so both are bare import targets. + expect(tree.getTempScriptRefs("f/app/consumer")).toEqual({}); + + await resolvePlaceholdersFromLocal( + tree, + new Map([ + [ + "f/barrel/index", + { + localPath: "f/barrel/index.ts", + content: `export * from "./mid.ts"`, + }, + ], + [ + "f/barrel/mid", + { + localPath: "f/barrel/mid.ts", + content: `export * from "./helper.ts"`, + }, + ], + ]), + "bun", + ); + + // Only the leaf diverged from deployed, so only it was uploaded. + tree.setContentHash("f/barrel/helper", "hash_helper"); + expect(tree.getTempScriptRefs("f/app/consumer")).toEqual({ + "f/barrel/helper": "hash_helper", + }); + }); +}); diff --git a/cli/test/sync_push_auto_metadata_repro.test.ts b/cli/test/sync_push_auto_metadata_repro.test.ts index 4854198d64..38da094a37 100644 --- a/cli/test/sync_push_auto_metadata_repro.test.ts +++ b/cli/test/sync_push_auto_metadata_repro.test.ts @@ -268,3 +268,78 @@ test( }); }, ); + +// The importer and the leaf change; the barrel between them does not, so it is +// absent from the push's change set. See `resolvePlaceholdersFromLocal`. +test( + "sync push --auto-metadata succeeds when a changed leaf sits behind an unchanged barrel", + { timeout: 180000 }, + async () => { + await withTestBackend(async (backend, tempDir) => { + await writeFile(`${tempDir}/wmill.yaml`, wmillYaml); + + await createLocalScript( + tempDir, + "f/barrel", + "helper", + "bun", + `export function add(a: number, b: number) { return a + b; }\n`, + ); + await createLocalScript( + tempDir, + "f/barrel", + "index", + "bun", + `export * from "./helper.ts";\n`, + ); + await createLocalScript( + tempDir, + "f/app", + "consumer", + "bun", + `import { add } from "../barrel/index.ts"; +export async function main() { return add(1, 2); } +`, + ); + + const deploy = await backend.runCLICommand( + ["sync", "push", "--yes", "--auto-metadata"], + tempDir, + ); + if (deploy.code !== 0) { + console.log("STDOUT:", deploy.stdout); + console.log("STDERR:", deploy.stderr); + } + expect(deploy.code).toBe(0); + + // Add an export to the leaf and use it from the importer. The barrel + // re-exports it already, so it stays byte-identical and out of the push. + await writeFile( + `${tempDir}/f/barrel/helper.ts`, + `export function add(a: number, b: number) { return a + b; } +export function subtract(a: number, b: number) { return a - b; } +`, + ); + await writeFile( + `${tempDir}/f/app/consumer.ts`, + `import { subtract } from "../barrel/index.ts"; +export async function main() { return subtract(3, 1); } +`, + ); + + const result = await backend.runCLICommand( + ["sync", "push", "--yes", "--auto-metadata"], + tempDir, + ); + if (result.code !== 0) { + console.log("STDOUT:", result.stdout); + console.log("STDERR:", result.stderr); + } + expect(result.code).toBe(0); + + const combined = result.stdout + result.stderr; + expect(combined).not.toContain("No matching export"); + expect(combined).not.toContain("Failed to generate lockfile"); + }); + }, +);