mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-11 16:09:39 +00:00
fix: skip triggers that are already in the destination
probeImportedPaths asked about scripts, flows, apps and resources but not triggers, so a retry replayed every trigger create into an API that rejects an existing path — reporting a failure for something already there, which is the wall the presence probe exists to remove. Triggers have no prefix-filtered list endpoint, so they cost one call per kind; the probe only asks when the project actually ships triggers. The presence set is now keyed by kind as well as path. The five kinds share one f/<folder>/ namespace, so a trigger and a script may both be called f/cal/sync, and a flat path set would let either one mask the other. Also drops expectedPaths, which was exported and tested but never called. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xg8vXUuHCH3aRkf91sfxjx
This commit is contained in:
co-authored by
Claude Opus 5
parent
f9ad196712
commit
52e502d043
@@ -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/<folder>/` 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),
|
||||
|
||||
@@ -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<string>) {
|
||||
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/<folder>/` 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 })
|
||||
})
|
||||
})
|
||||
@@ -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')
|
||||
|
||||
@@ -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({
|
||||
|
||||
@@ -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'
|
||||
])
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<Set<string>> {
|
||||
export async function probeImportedPaths(
|
||||
workspace: string,
|
||||
folder: string,
|
||||
opts?: { triggers?: boolean; hasEeLicense?: boolean }
|
||||
): Promise<Set<string>> {
|
||||
const pathStart = `f/${folder}/`
|
||||
const paths = new Set<string>()
|
||||
const collect = (rows: unknown) => {
|
||||
for (const r of (rows as { path?: string }[] | undefined) ?? []) if (r.path) paths.add(r.path)
|
||||
const found = new Set<string>()
|
||||
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<unknown>[] = [
|
||||
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
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user