diff --git a/frontend/src/lib/components/ImportProjectStep.svelte b/frontend/src/lib/components/ImportProjectStep.svelte index 2aef08b6e7..5d50ceab20 100644 --- a/frontend/src/lib/components/ImportProjectStep.svelte +++ b/frontend/src/lib/components/ImportProjectStep.svelte @@ -14,6 +14,7 @@ import { ImportExecution, plannedTasks } from '$lib/importWizard/execution.svelte' import SetupChecklist, { type SetupStep } from '$lib/components/wizards/SetupChecklist.svelte' import { beforeNavigate, goto } from '$app/navigation' + import { untrack } from 'svelte' import { FOLDER_NAME_RE, planProblem, type ImportPlan } from '$lib/importWizard/plan' import type { ImportProjectSummary } from '$lib/components/ImportProjectCard.svelte' import { ArrowLeft, Download, Loader2 } from 'lucide-svelte' @@ -32,6 +33,13 @@ /** Hands the run to the page, which needs the export's data tables to know * whether a setup step follows this one. */ onExecution?: (execution: ImportExecution | undefined) => void + /** + * The run this step already made, handed back when it is remounted. Step 4 unmounts + * this component, so returning from it would otherwise arrive at a fresh step with no + * run — offering Import again over a bundle that is already in, and on a new + * workspace failing at create because the finished run cleared its parking. + */ + resume?: ImportExecution | undefined onBack: () => void } @@ -42,7 +50,8 @@ onFinish, onBack, setupPending = false, - onExecution + onExecution, + resume }: Props = $props() let folder = $state(plan.folder ?? plan.slug) @@ -137,7 +146,15 @@ // clearing it from an effect, so a previous run's outcome can never be shown // against another plan. The folder is deliberately not part of the tag: it is // pushed onto the existing run instead (see `start`). - let run = $state<{ key: string; execution: ImportExecution } | undefined>(undefined) + // Seeded from the handed-back run, under the same tag a fresh one would carry, so the + // `planKey` guard below still rejects it if the destination changed in between. + // `untrack`, because this is a mount-time snapshot on purpose: a later plan change must + // invalidate the run through that tag, not silently re-seed it. + let run = $state<{ key: string; execution: ImportExecution } | undefined>( + untrack(() => + resume ? { key: JSON.stringify(plan.destination) + plan.slug, execution: resume } : undefined + ) + ) const planKey = $derived(JSON.stringify(plan.destination) + plan.slug) const execution = $derived(run?.key === planKey ? run.execution : undefined) $effect(() => onExecution?.(execution)) diff --git a/frontend/src/lib/components/ImportSetupStep.svelte b/frontend/src/lib/components/ImportSetupStep.svelte index de31d2d6cb..548fad5875 100644 --- a/frontend/src/lib/components/ImportSetupStep.svelte +++ b/frontend/src/lib/components/ImportSetupStep.svelte @@ -18,8 +18,13 @@ import { registryCcCapableFor } from '$lib/components/oauthRegistry' import { resourceTypeDisplayName } from '$lib/components/resourceTypeDisplay' import { applyOneMigration } from '$lib/components/workspaceSettings/projectInstall' - import type { ProjectMigration } from '$lib/components/workspaceSettings/projectBundle' + import { + retargetProjectExport, + type ProjectExport, + type ProjectMigration + } from '$lib/components/workspaceSettings/projectBundle' import { sendUserToast } from '$lib/toast' + import { escapeHtml } from '$lib/utils' // The last step, and the only optional one: it exists when the project's data // tables are not configured in the destination. The import has already run — @@ -33,12 +38,16 @@ interface Props { workspace: string slug: string + /** The folder the import wrote into. The export names resources under the project's + * own slug and `installProject` retargets them, so reading the raw paths here would + * look for stubs that are not where they landed. */ + folder?: string onSkip: () => void onFinish: () => void onBack?: () => void } - let { workspace, slug, onSkip, onFinish, onBack }: Props = $props() + let { workspace, slug, folder, onSkip, onFinish, onBack }: Props = $props() type Row = { name: string @@ -133,10 +142,7 @@ `/api/w/${encodeURIComponent(workspace)}/hub/projects/${encodeURIComponent(slug)}/export` ) if (!res.ok) throw new Error(`the hub proxy answered ${res.status}`) - const exportData = (await res.json()) as { - migrations?: ProjectMigration[] - resources?: { path: string; resource_type: string }[] - } + const exportData = (await res.json()) as ProjectExport const enabled = (exportData.migrations ?? []).filter( (m) => m.enabled && (m.sql ?? '').trim() !== '' ) @@ -159,7 +165,17 @@ justSaved: false } }) - projectResources = exportData.resources ?? [] + // Retargeted the same way the import was, so these are where the stubs actually + // landed. `retargetProjectExport` is a no-op when the folder is the slug, which is + // every new-workspace import. + const target = folder?.trim() || slug + const retargeted = retargetProjectExport(exportData, exportData.project?.slug ?? slug, target) + // Contained for the same reason the import contains: a crafted export can name a + // path outside the folder, and offering that for editing would reach a resource + // this import was never allowed to create. + projectResources = (retargeted.resources ?? []) + .map((r) => ({ path: String(r.path), resource_type: String((r as any).resource_type) })) + .filter((r) => r.path.startsWith(`f/${target}/`)) await refreshBlanks() } catch (e: any) { loadError = e?.body ?? e?.message ?? String(e) @@ -247,12 +263,6 @@ void load() }) - /** - * Configure every named data table in one write, then run each one's migrations. - * The config is read back and merged rather than replaced: `editDataTableConfig` - * takes the whole settings object, so sending only ours would delete any the - * workspace already has. - */ /** * The data table now exists — run the migrations that were skipped for it during the * import, which is the whole reason this step waits for the configuration. @@ -277,6 +287,11 @@ row.status = 'failed' row.error = e?.body ?? e?.message ?? String(e) sendUserToast(`Could not run the migrations for ${name}: ${row.error}`, true) + // Rethrown, because this also runs as the wizard's last checklist step + // (`onFinishAlso`). Swallowing it there makes the wizard report a clean finish + // over a failed migration, and close — leaving the data table name taken and no + // way back to retry it. + throw e } finally { working = false } @@ -291,7 +306,12 @@ */ async function skip(): Promise { if (pendingTables.length > 0) { - const names = pendingTables.map((r) => r.name).join(', ') + // Escaped: `confirmationModal.ask` renders `children` through `createRawSnippet`, + // so this string is HTML, and the name is a `datatable_name` straight out of the + // hub export. A hub is not ours — `hub_base_url` is an instance setting and the + // wizard can be pointed at any of them — so a name carrying an event-bearing + // element would otherwise run script in this authenticated origin. + const names = pendingTables.map((r) => escapeHtml(r.name)).join(', ') const one = pendingTables.length === 1 const confirmed = await confirmationModal.ask({ title: 'The project will not run', diff --git a/frontend/src/lib/components/datatableSchemaSql.ts b/frontend/src/lib/components/datatableSchemaSql.ts index bcf868c04f..44ca2414cc 100644 --- a/frontend/src/lib/components/datatableSchemaSql.ts +++ b/frontend/src/lib/components/datatableSchemaSql.ts @@ -146,11 +146,6 @@ function resolveColumnType(c: TableEditorValuesColumn): { return { datatype: c.datatype, defaultValue: c.defaultValue } } -/** - * SQL for an added table, with the CREATE TABLE and the FK constraints split so - * callers creating several tables can emit every CREATE before any constraint — - * required for circular FKs, where no creation order satisfies inline FKs. - */ /** * The schema API reports a foreign key's target as a bare table name whenever it * lives in the same schema as the table declaring it. Emitting that verbatim @@ -172,6 +167,11 @@ function qualifyFkTarget( return targetTable } +/** + * SQL for an added table, with the CREATE TABLE and the FK constraints split so + * callers creating several tables can emit every CREATE before any constraint — + * required for circular FKs, where no creation order satisfies inline FKs. + */ export function generateAddedTableSql( change: TableDiff, sourceSchema: DatabaseSchema, diff --git a/frontend/src/lib/components/workspaceSettings/projectInstall.ts b/frontend/src/lib/components/workspaceSettings/projectInstall.ts index 6cae3d5e0e..994985947d 100644 --- a/frontend/src/lib/components/workspaceSettings/projectInstall.ts +++ b/frontend/src/lib/components/workspaceSettings/projectInstall.ts @@ -266,11 +266,26 @@ export async function installProject(args: { /** Called once, before the reviewed migrations are applied, when there are any. Lets a * caller show them as their own step rather than folding them into the item import. */ onMigrationsStart?: () => void + /** + * Asked before each write. Returning true stops the run where it is — the writes already + * made stay, the rest never start. Nothing here can cancel a request already in flight, + * so this is the granularity available without threading an `AbortSignal` through every + * service call: the import wizard uses it when the user confirms leaving mid-run. + */ + stopped?: () => boolean hasEeLicense: boolean onResult: (r: InstallResult) => void }): Promise { - const { workspace, exportData, folder, migrations, hasEeLicense, onResult, onMigrationsStart } = - args + const { + workspace, + exportData, + folder, + migrations, + hasEeLicense, + onResult, + onMigrationsStart, + stopped + } = args const record = (path: string, p: Promise): Promise => p.then( @@ -278,6 +293,9 @@ export async function installProject(args: { (e: any) => onResult({ path, ok: false, error: errorMessage(e) }) ) + /** Every write goes through here, so one check covers items, variables and migrations. */ + const halted = () => stopped?.() === true + try { await FolderService.createFolder({ workspace, requestBody: { name: folder } }) } catch {} @@ -317,6 +335,7 @@ export async function installProject(args: { } for (const s of proj.scripts) { + if (halted()) return // `$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, () => @@ -324,19 +343,23 @@ export async function installProject(args: { ) } for (const f of proj.flows) { + if (halted()) return await checkedItem(f.path, extractFlowRefs(f.value), f.value, () => importFlow(workspace, f)) } for (const r of proj.resources) { + if (halted()) return await checked(r.path, () => importResourceStub(workspace, r)) } // Placeholders for the project's internal `$var:`/`$jsonvar:` refs (retargeted // into this folder). External refs are rejected per-item, so only stub in-folder // ones; guard again in case an out-of-folder ref slipped through retargeting. for (const p of collectExportVarPaths(proj)) { + if (halted()) return if (!p.startsWith(prefix)) continue await record(`variable: ${p}`, importVariablePlaceholder(workspace, p)) } for (const a of proj.apps) { + if (halted()) return 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 @@ -376,6 +399,7 @@ export async function installProject(args: { return varContainmentViolation(cfg, folder) } for (const t of proj.triggers) { + if (halted()) return const violation = guard(t.path, t.runnable_path) ?? triggerConfigViolation(t) await record( String(t.path), @@ -397,8 +421,10 @@ export async function installProject(args: { } // Apply the reviewed data table migrations after items exist. + if (halted()) return if (migrations.length) onMigrationsStart?.() for (const m of migrations) { + if (halted()) return await record( `data table: ${m.datatable_name}`, applyOneMigration(workspace, exportData.project.slug, m) diff --git a/frontend/src/lib/importWizard/execution.svelte.ts b/frontend/src/lib/importWizard/execution.svelte.ts index 5c0cc2aea1..ecede98611 100644 --- a/frontend/src/lib/importWizard/execution.svelte.ts +++ b/frontend/src/lib/importWizard/execution.svelte.ts @@ -368,7 +368,10 @@ export class ImportExecution { migrations, hasEeLicense: this.#deps.hasEeLicense, onResult: (r) => (this.results = [...this.results, r]), - onMigrationsStart: () => this.#set('migrate', 'running') + onMigrationsStart: () => this.#set('migrate', 'running'), + // Checked before every write, so leaving mid-run stops the remaining items + // rather than only the phases. What already landed stays and is listed. + stopped: () => this.#abandoned }) } catch (e: any) { this.#set('import', 'failed', String(e)) diff --git a/frontend/src/lib/utils/workspaceId.test.ts b/frontend/src/lib/utils/workspaceId.test.ts index 3ed85b6b6f..bcbeb1b780 100644 --- a/frontend/src/lib/utils/workspaceId.test.ts +++ b/frontend/src/lib/utils/workspaceId.test.ts @@ -69,3 +69,20 @@ describe('toWorkspaceId', () => { expect(validateWorkspaceId(id)).toBeUndefined() }) }) + +describe('validateWorkspaceId — the reserved id', () => { + // `check_w_id_conflict` refuses it, and `existsWorkspace` reports it free, so without + // this the wizard walks the user to the last step before the create fails. + it('refuses `global`', () => { + expect(validateWorkspaceId('global')).toMatch(/not allowed/i) + }) + + it('refuses it as the effective id too', () => { + expect(validateWorkspaceId('wm-fork-x', 'global')).toMatch(/not allowed/i) + }) + + it('still accepts ids that merely contain it', () => { + expect(validateWorkspaceId('global-ops')).toBeUndefined() + expect(validateWorkspaceId('my-global')).toBeUndefined() + }) +}) diff --git a/frontend/src/lib/utils/workspaceId.ts b/frontend/src/lib/utils/workspaceId.ts index 1d7b989e13..b1bcbd9e1b 100644 --- a/frontend/src/lib/utils/workspaceId.ts +++ b/frontend/src/lib/utils/workspaceId.ts @@ -13,6 +13,9 @@ export const WORKSPACE_ID_MAX_LENGTH = 50 /** `validate_workspace_name` (windmill-common/src/workspaces.rs:246) refuses a longer name. */ export const WORKSPACE_NAME_MAX_LENGTH = 50 +/** `check_w_id_conflict` (windmill-api-workspaces/src/workspaces.rs:5111) rejects this id. */ +const RESERVED_WORKSPACE_ID = 'global' + /** * The reason `id` is not a usable workspace id, or undefined when it is. * @@ -24,6 +27,11 @@ export function validateWorkspaceId(id: string, effectiveId: string = id): strin if (!WORKSPACE_ID_RE.test(id)) { return 'ID can only contain letters, numbers and dashes and must not finish by a dash' } + // `check_w_id_conflict` refuses it outright, and `existsWorkspace` reports it free — + // so without this the wizard walks the user to the last step before the create fails. + if (id === RESERVED_WORKSPACE_ID || effectiveId === RESERVED_WORKSPACE_ID) { + return `'${RESERVED_WORKSPACE_ID}' is not allowed as a workspace ID` + } if (effectiveId.length > WORKSPACE_ID_MAX_LENGTH) { return `ID '${effectiveId}' is too long (${effectiveId.length} chars). Maximum is ${WORKSPACE_ID_MAX_LENGTH}.` } diff --git a/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte b/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte index 06cfd3c31b..99c0ab420d 100644 --- a/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte +++ b/frontend/src/routes/(root)/(logged)/projects/import/+page@(root).svelte @@ -446,7 +446,7 @@ onClick={() => expandCollapseAll?.()} title={allExpanded ? 'Collapse all' : 'Expand all'} startIcon={{ icon: allExpanded ? ChevronsDownUp : ChevronsUpDown }} - size="xs2" + unifiedSize="2xs" variant="default" > {allExpanded ? 'Collapse' : 'Expand'} @@ -482,7 +482,12 @@