diff --git a/frontend/src/lib/components/workspaceSettings/projectInstall.ts b/frontend/src/lib/components/workspaceSettings/projectInstall.ts index b582c492b0..41a5da855c 100644 --- a/frontend/src/lib/components/workspaceSettings/projectInstall.ts +++ b/frontend/src/lib/components/workspaceSettings/projectInstall.ts @@ -263,6 +263,17 @@ export async function applyOneMigration( * reviewed) migrations. Each item's outcome is reported through `onResult`; * failures never abort the remaining items. */ +/** The kinds an import writes that carry a path and can therefore already be there. */ +export type ImportedKind = 'script' | 'flow' | 'app' | 'resource' | 'trigger' + +/** + * The key `alreadyPresent` is built and read with. Kind and path together, because the five + * kinds share one path namespace and a bare path cannot say which of them is already there. + */ +export function presenceKey(kind: ImportedKind, path: string): string { + return `${kind}:${path}` +} + export async function installProject(args: { workspace: string exportData: ProjectExport @@ -279,9 +290,13 @@ export async function installProject(args: { */ stopped?: () => boolean /** - * Paths already in the destination, retargeted — so a retry writes only what is missing - * instead of replaying the bundle into a wall of "already exists". Compared after - * retargeting, because that is what these items will actually be called. + * What is already in the destination, as `presenceKey` keys — so a retry writes only what + * is missing instead of replaying the bundle into a wall of "already exists". Built from + * retargeted paths, because that is what these items will actually be called. + * + * Keyed by kind and not by path alone: the five kinds share one `f//` namespace, so + * a trigger and a script may legitimately both be called `f/cal/sync`. A flat path set + * would let either one mask the other and silently skip an item that was never imported. * * Never a way to *replace* anything: an item that is there is left exactly as it is, * which is the same promise `updateIfExists: false` makes for a resource whose value @@ -317,8 +332,8 @@ export async function installProject(args: { * rather than dropped: the checklist has to account for every item the project ships, and * "already there" is a different thing from "imported". */ - const present = (path: string): boolean => { - if (!alreadyPresent?.has(path)) return false + const present = (kind: ImportedKind, path: string): boolean => { + if (!alreadyPresent?.has(presenceKey(kind, path))) return false onResult({ path, ok: true, skipped: true }) return true } @@ -363,7 +378,7 @@ export async function installProject(args: { for (const s of proj.scripts) { if (halted()) return - if (present(s.path)) continue + if (present('script', s.path)) continue // `$var:` is resolved in job args (flow inputs, schedule args, trigger config), // not in script source, so there is no variable arg to contain here. await checkedItem(s.path, extractScriptRefs(s.content ?? ''), undefined, () => @@ -372,12 +387,12 @@ export async function installProject(args: { } for (const f of proj.flows) { if (halted()) return - if (present(f.path)) continue + if (present('flow', f.path)) continue await checkedItem(f.path, extractFlowRefs(f.value), f.value, () => importFlow(workspace, f)) } for (const r of proj.resources) { if (halted()) return - if (present(r.path)) continue + if (present('resource', r.path)) continue await checked(r.path, () => importResourceStub(workspace, r)) } // Placeholders for the project's internal `$var:`/`$jsonvar:` refs (retargeted @@ -390,7 +405,7 @@ export async function installProject(args: { } for (const a of proj.apps) { if (halted()) return - if (present(a.path)) continue + if (present('app', a.path)) continue const isRaw = a.app_type === 'raw' const refs = isRaw ? extractRawAppRefs(a.value?.raw ?? '') : extractAppRefs(a.value) // Raw apps hold their runnables in the `value.raw` JSON string; parse it so the @@ -431,6 +446,7 @@ export async function installProject(args: { } for (const t of proj.triggers) { if (halted()) return + if (present('trigger', String(t.path))) continue const violation = guard(t.path, t.runnable_path) ?? triggerConfigViolation(t) await record( String(t.path), diff --git a/frontend/src/lib/components/workspaceSettings/projectInstallPresence.test.ts b/frontend/src/lib/components/workspaceSettings/projectInstallPresence.test.ts new file mode 100644 index 0000000000..e4f038e8f2 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/projectInstallPresence.test.ts @@ -0,0 +1,94 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +/** + * A retry must not re-write what the destination already has. The interesting kind is the + * trigger: it is the one item the import creates through an API that rejects an existing + * path outright, so replaying it turns "already there" into a reported failure. + */ + +const created = vi.hoisted(() => ({ triggers: [] as string[] })) + +vi.mock('$lib/gen', () => { + const nothing = vi.fn(async () => []) + return { + AppService: { createApp: vi.fn(), listApps: nothing }, + FlowService: { createFlow: vi.fn(), listFlows: nothing }, + FolderService: { createFolder: vi.fn() }, + ResourceService: { createResource: vi.fn(), listResource: nothing }, + ScriptService: { createScript: vi.fn(), listScripts: nothing }, + VariableService: { createVariable: vi.fn(), listVariable: nothing }, + WorkspaceService: { listDataTables: vi.fn(async () => []) } + } +}) + +vi.mock('../triggers/workspaceTriggersList', () => ({ + TRIGGER_KINDS: { schedule: { badge: 'Schedule', resourceField: undefined } }, + createWorkspaceTriggerDisabled: vi.fn(async (_ws: string, t: { path: string }) => { + created.triggers.push(t.path) + }), + triggerHandlerRefs: () => [] +})) + +import { installProject, presenceKey } from './projectInstall' + +const exportData = { + project: { slug: 'calendly', name: 'Calendly', summary: '', readme: null }, + scripts: [], + flows: [], + apps: [], + resources: [], + variables: [], + triggers: [ + { + kind: 'schedule', + path: 'f/calendly/nightly', + runnable_path: 'f/calendly/sync', + runnable_kind: 'script', + summary: null, + config: {} + } + ], + migrations: [] +} as any + +async function run(alreadyPresent?: Set) { + const results: any[] = [] + await installProject({ + workspace: 'w', + exportData, + folder: 'calendly', + migrations: [], + hasEeLicense: true, + alreadyPresent, + onResult: (r) => results.push(r) + }) + return results +} + +describe('installProject presence', () => { + beforeEach(() => { + created.triggers = [] + }) + + it('creates a trigger the destination does not have', async () => { + const results = await run(new Set()) + expect(created.triggers).toEqual(['f/calendly/nightly']) + expect(results).toContainEqual({ path: 'f/calendly/nightly', ok: true }) + }) + + // The regression: without this the retry calls the create API again, which rejects the + // existing path, and the row reads as a failure for something that is already there. + it('skips a trigger that is already there instead of re-creating it', async () => { + const results = await run(new Set([presenceKey('trigger', 'f/calendly/nightly')])) + expect(created.triggers).toEqual([]) + expect(results).toContainEqual({ path: 'f/calendly/nightly', ok: true, skipped: true }) + }) + + // Kinds share one `f//` namespace, so the key has to carry the kind: a script of + // the same name is not this trigger and must not stand in for it. + it('does not let another kind at the same path mask the trigger', async () => { + const results = await run(new Set([presenceKey('script', 'f/calendly/nightly')])) + expect(created.triggers).toEqual(['f/calendly/nightly']) + expect(results).toContainEqual({ path: 'f/calendly/nightly', ok: true }) + }) +}) diff --git a/frontend/src/lib/importWizard/abandon.test.ts b/frontend/src/lib/importWizard/abandon.test.ts index 66942a1edf..d899a17f64 100644 --- a/frontend/src/lib/importWizard/abandon.test.ts +++ b/frontend/src/lib/importWizard/abandon.test.ts @@ -40,7 +40,9 @@ vi.mock('$lib/components/workspaceSettings/projectInstall', () => ({ // returns on success. for (const path of ['a', 'b', 'c']) { if (args.stopped?.() === true) return - if (args.alreadyPresent?.has(path)) { + // Keyed exactly as the real `installProject` keys it, so this stand-in cannot drift + // into testing a contract the production code does not have. + if (args.alreadyPresent?.has(`script:${path}`)) { args.onResult({ path, ok: true, skipped: true }) continue } @@ -168,7 +170,7 @@ describe('retrying over what is already there', () => { }) it('writes nothing for a path the destination already holds', async () => { - present.paths = new Set(['a', 'b']) + present.paths = new Set(['script:a', 'script:b']) const run = new ImportExecution(PLAN, deps) await run.run() const byPath = new Map(run.itemResults.map((r) => [r.path, r])) @@ -178,7 +180,7 @@ describe('retrying over what is already there', () => { }) it('still accounts for every item, so the checklist stays complete', async () => { - present.paths = new Set(['a', 'b']) + present.paths = new Set(['script:a', 'script:b']) const run = new ImportExecution(PLAN, deps) await run.run() expect(run.itemResults.length).toBe(3) @@ -186,7 +188,7 @@ describe('retrying over what is already there', () => { }) it('says what it did rather than claiming to have imported all of it', async () => { - present.paths = new Set(['a', 'b']) + present.paths = new Set(['script:a', 'script:b']) const run = new ImportExecution(PLAN, deps) await run.run() const importRow = run.tasks.find((t) => t.key === 'import') diff --git a/frontend/src/lib/importWizard/execution.svelte.ts b/frontend/src/lib/importWizard/execution.svelte.ts index ff7e3171ad..7c062e28f7 100644 --- a/frontend/src/lib/importWizard/execution.svelte.ts +++ b/frontend/src/lib/importWizard/execution.svelte.ts @@ -363,7 +363,10 @@ export class ImportExecution { // holds some of these paths, and a run interrupted halfway is indistinguishable from // one that never started. On a workspace this run just created the answer is empty and // nothing is skipped. - const alreadyPresent = await probeImportedPaths(workspace, folder) + const alreadyPresent = await probeImportedPaths(workspace, folder, { + triggers: exportData.triggers.length > 0, + hasEeLicense: this.#deps.hasEeLicense + }) if (this.#abandoned) return try { await installProject({ diff --git a/frontend/src/lib/importWizard/probe.test.ts b/frontend/src/lib/importWizard/probe.test.ts index 47026639ed..1d66a0e14b 100644 --- a/frontend/src/lib/importWizard/probe.test.ts +++ b/frontend/src/lib/importWizard/probe.test.ts @@ -1,10 +1,10 @@ import { describe, expect, it } from 'vitest' -import { expectedPaths, expectedTables } from './probe' +import { expectedTables } from './probe' /** - * These two are inference over what the export happens to say, so they are the part of the - * probe that can be wrong while everything still type-checks. The network reads around them + * `expectedTables` is inference over what the export happens to say, so it is the part of the + * probe that can be wrong while everything still type-checks. The network reads around it * either answer or throw. */ @@ -52,33 +52,3 @@ COMMIT;` expect(expectedTables('')).toEqual([]) }) }) - -describe('expectedPaths', () => { - const EXPORT = { - project: { slug: 'calendly', name: 'Calendly', summary: '', readme: null }, - scripts: [{ path: 'f/calendly/book_slot' }], - flows: [], - apps: [{ path: 'f/calendly/booking' }], - resources: [{ path: 'f/calendly/smtp' }], - triggers: [], - migrations: [] - } as any - - it('lists what the import will write, across every kind', () => { - expect(expectedPaths(EXPORT, 'calendly')).toEqual([ - 'f/calendly/book_slot', - 'f/calendly/booking', - 'f/calendly/smtp' - ]) - }) - - // `installProject` retargets into the chosen folder, so the paths to look for are the - // retargeted ones — checking the export's own would look for stubs that never landed. - it('follows the folder the import was pointed at', () => { - expect(expectedPaths(EXPORT, 'elsewhere')).toEqual([ - 'f/elsewhere/book_slot', - 'f/elsewhere/booking', - 'f/elsewhere/smtp' - ]) - }) -}) diff --git a/frontend/src/lib/importWizard/probe.ts b/frontend/src/lib/importWizard/probe.ts index c3d76a304f..80e60e14e6 100644 --- a/frontend/src/lib/importWizard/probe.ts +++ b/frontend/src/lib/importWizard/probe.ts @@ -13,10 +13,12 @@ */ import { ResourceService, ScriptService, FlowService, AppService, WorkspaceService } from '$lib/gen' -import type { - ProjectExport, - ProjectMigration -} from '$lib/components/workspaceSettings/projectBundle' +import type { ProjectMigration } from '$lib/components/workspaceSettings/projectBundle' +import { + presenceKey, + type ImportedKind +} from '$lib/components/workspaceSettings/projectInstall' +import { listAllWorkspaceTriggers } from '$lib/components/triggers/workspaceTriggersList' /** * The tables a migration creates, as `schema.table`, read off its `CREATE TABLE` statements. @@ -36,15 +38,6 @@ export function expectedTables(sql: string): string[] { return [...new Set(out)] } -/** Every path the export will write, under the folder the import is targeting. */ -export function expectedPaths(exportData: ProjectExport, folder: string): string[] { - const from = exportData.project.slug - const rewrite = (p: string) => (folder === from ? p : p.replace(`f/${from}/`, `f/${folder}/`)) - return [...exportData.scripts, ...exportData.flows, ...exportData.apps, ...exportData.resources] - .map((i: any) => String(i.path)) - .map(rewrite) -} - export interface WorkspaceState { /** The user is a member of a workspace with this id. */ exists: boolean @@ -77,29 +70,52 @@ export async function probeWorkspace( } /** - * Which of the paths the import would write are already there. + * Which of the items the import would write are already there, as `presenceKey` keys. * - * Scoped by `pathStart` to the import's own folder, so this is four small reads rather than a - * workspace scan. Presence is not provenance — importing into an existing workspace that - * already held a path reads the same as having imported it — so callers use this to decide - * what is left to do, never to claim credit for what is there. + * Scoped by `pathStart` to the import's own folder, so the four path-bearing kinds are four + * small reads rather than a workspace scan. Presence is not provenance — importing into an + * existing workspace that already held a path reads the same as having imported it — so + * callers use this to decide what is left to do, never to claim credit for what is there. + * + * Triggers are asked for separately and only when the project ships some: they have no + * prefix-filtered list endpoint, so answering for them means one call per trigger kind, and a + * project without triggers should not pay for that. */ -export async function probeImportedPaths(workspace: string, folder: string): Promise> { +export async function probeImportedPaths( + workspace: string, + folder: string, + opts?: { triggers?: boolean; hasEeLicense?: boolean } +): Promise> { const pathStart = `f/${folder}/` - const paths = new Set() - const collect = (rows: unknown) => { - for (const r of (rows as { path?: string }[] | undefined) ?? []) if (r.path) paths.add(r.path) + const found = new Set() + const collect = (kind: ImportedKind) => (rows: unknown) => { + for (const r of (rows as { path?: string }[] | undefined) ?? []) { + if (r.path) found.add(presenceKey(kind, r.path)) + } } - const calls = [ - ScriptService.listScripts({ workspace, pathStart }).then(collect), - FlowService.listFlows({ workspace, pathStart }).then(collect), - AppService.listApps({ workspace, pathStart }).then(collect), - ResourceService.listResource({ workspace, pathStart }).then(collect) + const calls: Promise[] = [ + ScriptService.listScripts({ workspace, pathStart }).then(collect('script')), + FlowService.listFlows({ workspace, pathStart }).then(collect('flow')), + AppService.listApps({ workspace, pathStart }).then(collect('app')), + ResourceService.listResource({ workspace, pathStart }).then(collect('resource')) ] - // One kind failing should narrow the answer, not lose the other three: a missing path - // only ever means "still to do", which is the safe direction. + if (opts?.triggers) { + // `failedKinds` is deliberately ignored: a kind that could not be listed leaves its + // triggers out of the set, and a missing key only ever means "still to do". + calls.push( + listAllWorkspaceTriggers(workspace, { + includeEeOnly: opts.hasEeLicense === true + }).then(({ triggers }) => { + for (const t of triggers) { + if (t.path?.startsWith(pathStart)) found.add(presenceKey('trigger', t.path)) + } + }) + ) + } + // One kind failing should narrow the answer, not lose the others: a missing key only ever + // means "still to do", which is the safe direction. await Promise.allSettled(calls) - return paths + return found } /**