diff --git a/frontend/src/lib/importWizard/abandon.test.ts b/frontend/src/lib/importWizard/abandon.test.ts index 2ea9062e4c..ab9e3b62d4 100644 --- a/frontend/src/lib/importWizard/abandon.test.ts +++ b/frontend/src/lib/importWizard/abandon.test.ts @@ -3,8 +3,17 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' // Everything the executor reaches over the network, stubbed. The two behaviours under test // are decisions it makes around those calls, not the calls themselves. vi.mock('$lib/gen', () => ({ - WorkspaceService: { createWorkspace: vi.fn(), listDataTables: vi.fn(async () => []) }, - UserService: { whoami: vi.fn(async () => ({ username: 'u' })) } + WorkspaceService: { + createWorkspace: vi.fn(), + listDataTables: vi.fn(async () => []), + // No workspace of ours at that id: the existing-workspace plan these tests use never + // reaches the create, and an empty list is the honest answer for a fresh instance. + listWorkspaces: vi.fn(async () => []) + }, + UserService: { + whoami: vi.fn(async () => ({ username: 'u' })), + globalWhoami: vi.fn(async () => ({ email: 'u@example.com' })) + } })) vi.mock('$lib/storeUtils', () => ({ switchWorkspace: vi.fn() })) vi.mock('$lib/user', () => ({ getUserExt: vi.fn(async () => ({ username: 'u' })) })) @@ -57,7 +66,6 @@ vi.stubGlobal( ) import { ImportExecution } from './execution.svelte' -import { clearParkedImport, parkImport, resumableImport } from './parking' const PLAN = { slug: 'calendly', destination: { kind: 'existing' as const, workspaceId: 'ws-a' } } const deps = { reviewMigrations: async () => [], hasEeLicense: false } @@ -99,7 +107,6 @@ describe('planTag', () => { describe('abandoning mid-import', () => { beforeEach(() => { - clearParkedImport() hooks.afterFirstItem = undefined hooks.afterMigrationsStart = undefined }) @@ -121,14 +128,6 @@ describe('abandoning mid-import', () => { expect(run.itemResults.length).toBe(3) }) - it('keeps the parked workspace so the link the leave dialog promises still resumes', async () => { - parkImport({ slug: 'calendly', workspaceId: 'ws-a' }) - const run = new ImportExecution(PLAN, deps) - hooks.afterFirstItem = () => run.abandon() - await run.run() - expect(resumableImport('calendly', 'ws-a')).toBe(true) - }) - it('stops the migrate row spinning when it is abandoned mid-migration', async () => { const run = new ImportExecution(PLAN, depsWithMigration) // After `onMigrationsStart`, which is where the row is actually set to running — @@ -143,11 +142,4 @@ describe('abandoning mid-import', () => { expect(migrate?.status).not.toBe('running') expect(run.done).toBe(false) }) - - it('clears it on a clean finish, so a later import reaches its own create', async () => { - parkImport({ slug: 'calendly', workspaceId: 'ws-a' }) - const run = new ImportExecution(PLAN, deps) - await run.run() - expect(resumableImport('calendly', 'ws-a')).toBe(false) - }) }) diff --git a/frontend/src/lib/importWizard/execution.svelte.ts b/frontend/src/lib/importWizard/execution.svelte.ts index 2017c08b27..503695a18b 100644 --- a/frontend/src/lib/importWizard/execution.svelte.ts +++ b/frontend/src/lib/importWizard/execution.svelte.ts @@ -1,4 +1,4 @@ -import { WorkspaceService } from '$lib/gen' +import { UserService, WorkspaceService } from '$lib/gen' import { switchWorkspace } from '$lib/storeUtils' import { userStore, workspaceStore } from '$lib/stores' import { getUserExt } from '$lib/user' @@ -13,7 +13,7 @@ import type { ProjectMigration } from '$lib/components/workspaceSettings/projectBundle' import { planWorkspaceId, type ImportPlan } from './plan' -import { clearParkedImport, parkImport, resumableImport } from './parking' +import { probeWorkspace } from './probe' /** * The only thing in the wizard that changes anything. It takes a finished plan and @@ -143,19 +143,9 @@ export class ImportExecution { // `$workspaceStore` already holds the workspace being entered. #priorWorkspace = get(workspaceStore) - /** - * A workspace this plan created before the page was reloaded. Only ever true for a run - * whose create already succeeded: `resumableImport` requires the parked project *and* - * workspace to be this plan's, so an entry left by another import cannot make this run - * skip a create it has not done. - */ - #resumed: boolean - constructor(plan: ImportPlan, deps: ExecutionDeps) { this.#plan = plan this.#deps = deps - const d = plan.destination - this.#resumed = d?.kind === 'new' && resumableImport(plan.slug, d.id) this.tasks = this.#initialTasks() } @@ -176,13 +166,12 @@ export class ImportExecution { /** * True once this run created a workspace — the only case where deleting is ours to offer. * - * Deliberately not satisfied by `#resumed`. A parked entry is enough to skip a create, - * because entering the wrong workspace is recoverable; it is not enough to delete one, - * because that is not. Verifying would need a discriminator to compare the live - * workspace against, and a workspace has none — no `created_at`, nothing that moves - * when someone else writes — so a parked id could name a workspace another admin made - * at that id after ours was removed. A resumed run therefore finishes the import and - * leaves the undo to the run that actually did the creating. + * Deliberately not satisfied by having *adopted* one. `workspace.owner` is enough to know + * a create can be skipped — the id is one this user made — but not enough to offer to + * delete it, because `owner` is an identity, not a run: a second import by the same person + * into the same id looks identical. Skipping a create wrongly is recoverable; deleting a + * workspace is not, so an adopted run finishes the import and leaves the undo to the run + * that did the creating. */ get createdWorkspace(): boolean { return this.#workspaceCreated @@ -216,9 +205,9 @@ export class ImportExecution { * in flight — `installProject` takes no signal — so this stops the run at the next phase * boundary instead, which is as far as "stops where it is" can honestly go. * - * Its other job is to keep the parked workspace: a run that clears parking on its way out - * would make the link the user was told to come back to create the workspace a second - * time and fail with "already exists". + * The workspace it created stays, and the run stays resumable: coming back to the link + * re-probes the instance, finds the workspace, and carries on rather than trying to create + * it a second time. */ #abandoned = false @@ -276,11 +265,14 @@ export class ImportExecution { await this.#adoptUser(d.workspaceId) return d.workspaceId } - // Keyed on the workspace existing rather than on the task being green: a retry - // after entering it failed must not run the create again, which would only - // report the id as taken by the workspace this run just made. `#resumed` covers - // the same ground across a reload, where the field starts false again. - if (!this.#workspaceCreated && !this.#resumed) { + // Asked of the instance, not remembered. A retry after entering it failed must not + // run the create again — that would only report the id as taken by the workspace this + // run just made — and after a reload the field is false again while the workspace is + // still there. `ours` is what makes adopting it safe: an id that exists but belongs to + // someone else is not this run's work, and importing into it would be importing into + // a stranger's workspace. + const already = await probeWorkspace(d.id, await this.#email()) + if (!this.#workspaceCreated && !(already.exists && already.ours)) { this.#set('create', 'running') try { await WorkspaceService.createWorkspace({ @@ -293,9 +285,6 @@ export class ImportExecution { return undefined } this.#workspaceCreated = true - // From here a reload can no longer tell that this id is ours, so record it - // before anything else can fail. - parkImport({ slug: this.#plan.slug, workspaceId: d.id }) } try { await enterNewWorkspace(d.id) @@ -426,11 +415,6 @@ export class ImportExecution { // A partial import is finished, not broken: the items that landed are real, // and the failures are listed. Only a hard stop leaves `done` false. this.done = true - // Nothing left to resume. A later import of the same project must reach its - // create rather than adopt this one — unless the user left mid-run, in which case - // the workspace this created is exactly what the link they were told to return to - // has to find. - if (!this.#abandoned) clearParkedImport() if (failed > 0) this.error = `${failed} item${failed === 1 ? '' : 's'} failed to import.` } @@ -451,6 +435,21 @@ export class ImportExecution { } } + /** + * Who we are, for the ownership check. `$userStore` is workspace-scoped and this page is + * reparented out of `(logged)`, so it is unset until a run adopts one — `globalWhoami` is + * the identity that exists before any workspace does. + */ + async #email(): Promise { + const known = get(userStore)?.email + if (known) return known + try { + return (await UserService.globalWhoami()).email + } catch { + return undefined + } + } + /** Undoes the one thing this run created, when the user asks for it. */ async deleteCreatedWorkspace(): Promise { const d = this.#plan.destination @@ -463,8 +462,8 @@ export class ImportExecution { switchWorkspace(this.#priorWorkspace) await refreshWorkspaceList() this.#workspaceCreated = false - // The id is free again, so a retry has to create it rather than adopt it. - clearParkedImport() + // The id is free again; the next `#ensureWorkspace` asks the instance and finds it + // gone, so a retry creates rather than adopts. this.#set('create', 'pending') this.done = false this.results = [] diff --git a/frontend/src/lib/importWizard/parking.test.ts b/frontend/src/lib/importWizard/parking.test.ts deleted file mode 100644 index 8b57ac20c8..0000000000 --- a/frontend/src/lib/importWizard/parking.test.ts +++ /dev/null @@ -1,39 +0,0 @@ -import { beforeEach, describe, expect, it } from 'vitest' - -import { clearParkedImport, parkImport, readParkedImport, resumableImport } from './parking' - -// A run that skips its create when it should not have imports a project into a workspace -// somebody else owns, so the match has to be exact and a damaged entry has to read as -// nothing parked rather than as a partial match. - -describe('resumableImport', () => { - beforeEach(() => clearParkedImport()) - - it('resumes the run that parked it', () => { - parkImport({ slug: 'calendly', workspaceId: 'calendly-7' }) - expect(resumableImport('calendly', 'calendly-7')).toBe(true) - }) - - it('does not resume another project parked at the same workspace', () => { - parkImport({ slug: 'calendly', workspaceId: 'calendly-7' }) - expect(resumableImport('bitly', 'calendly-7')).toBe(false) - }) - - it('does not resume the same project aimed at another workspace', () => { - parkImport({ slug: 'calendly', workspaceId: 'calendly-7' }) - expect(resumableImport('calendly', 'calendly-8')).toBe(false) - }) - - it('does not resume once cleared', () => { - parkImport({ slug: 'calendly', workspaceId: 'calendly-7' }) - clearParkedImport() - expect(resumableImport('calendly', 'calendly-7')).toBe(false) - }) - - it('reads a damaged entry as nothing parked', () => { - sessionStorage.setItem('import_wizard_parked', '{"slug":"calendly"') - expect(readParkedImport()).toBeUndefined() - sessionStorage.setItem('import_wizard_parked', '{"slug":"calendly"}') - expect(readParkedImport()).toBeUndefined() - }) -}) diff --git a/frontend/src/lib/importWizard/parking.ts b/frontend/src/lib/importWizard/parking.ts deleted file mode 100644 index 330d32e41d..0000000000 --- a/frontend/src/lib/importWizard/parking.ts +++ /dev/null @@ -1,62 +0,0 @@ -/** - * What an import run has already created, kept across a reload. - * - * The plan lives in the URL (`./plan`) and everything the first two steps decide belongs - * there — that is what makes the back button and shareable links work. This file is for the - * one thing the URL cannot hold: a fact the run produced rather than the user, which the - * next page load has no other way to learn. Creating the workspace is currently the only - * one. Anything a user chose goes in the plan, not here. - * - * Kept out of the executor so a caller can ask what is parked without pulling the run in. - */ - -const PARKED_KEY = 'import_wizard_parked' - -export type ParkedImport = { - /** The project this run is importing. */ - slug: string - /** The workspace the run created, which a resumed run must not try to create again. */ - workspaceId: string -} - -export function parkImport(parked: ParkedImport): void { - try { - sessionStorage.setItem(PARKED_KEY, JSON.stringify(parked)) - } catch { - // Storage disabled or full. The run continues; only the resume is lost. - } -} - -export function clearParkedImport(): void { - try { - sessionStorage.removeItem(PARKED_KEY) - } catch {} -} - -/** - * The parked run, when it is the one being asked about. Both fields have to match: an entry - * left by another project would otherwise make this run skip a create it has not done, and - * enter a workspace that belongs to a different import. - */ -export function resumableImport(slug: string, workspaceId: string): boolean { - const parked = readParkedImport() - return parked?.slug === slug && parked?.workspaceId === workspaceId -} - -export function readParkedImport(): ParkedImport | undefined { - let raw: string | null = null - try { - raw = sessionStorage.getItem(PARKED_KEY) - } catch { - return undefined - } - if (!raw) return undefined - try { - const parsed = JSON.parse(raw) - return typeof parsed?.slug === 'string' && typeof parsed?.workspaceId === 'string' - ? { slug: parsed.slug, workspaceId: parsed.workspaceId } - : undefined - } catch { - return undefined - } -} diff --git a/frontend/src/lib/importWizard/probe.test.ts b/frontend/src/lib/importWizard/probe.test.ts new file mode 100644 index 0000000000..47026639ed --- /dev/null +++ b/frontend/src/lib/importWizard/probe.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, it } from 'vitest' + +import { expectedPaths, 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 + * either answer or throw. + */ + +describe('expectedTables', () => { + // The shape `datatableSchemaSql.ts` emits, and what every published project carries. + const REAL = `BEGIN; +CREATE SCHEMA IF NOT EXISTS "calendly"; +CREATE TABLE IF NOT EXISTS "calendly"."config" ( + "id" SERIAL NOT NULL, + "host_name" text NOT NULL DEFAULT ''::text +); +CREATE TABLE IF NOT EXISTS "calendly"."bookings" ( "id" SERIAL NOT NULL ); +COMMIT;` + + it('reads every table a migration creates', () => { + expect(expectedTables(REAL)).toEqual(['calendly.config', 'calendly.bookings']) + }) + + it('does not mistake the schema for a table', () => { + expect(expectedTables(REAL)).not.toContain('calendly') + }) + + it('reads the form without IF NOT EXISTS', () => { + expect(expectedTables('CREATE TABLE "bitly"."links" (id int)')).toEqual(['bitly.links']) + }) + + it('is case- and whitespace-insensitive the way SQL is', () => { + expect(expectedTables('create table\n "a" . "b" (x int)')).toEqual(['a.b']) + }) + + it('reports each table once, however many times it is named', () => { + const sql = 'CREATE TABLE "a"."b" (x int); CREATE TABLE IF NOT EXISTS "a"."b" (x int);' + expect(expectedTables(sql)).toEqual(['a.b']) + }) + + /** + * The answer that keeps a caller honest. An unquoted or unqualified `CREATE TABLE` is + * something this cannot resolve — the schema would come from `search_path` at run time — + * so it reads as nothing expected, and the caller treats that as "cannot tell" rather than + * as "no tables, so the migration must have run". + */ + it('claims nothing about SQL it cannot resolve', () => { + expect(expectedTables('CREATE TABLE links (id int)')).toEqual([]) + expect(expectedTables('CREATE TABLE bitly.links (id int)')).toEqual([]) + 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 new file mode 100644 index 0000000000..c3d76a304f --- /dev/null +++ b/frontend/src/lib/importWizard/probe.ts @@ -0,0 +1,136 @@ +/** + * What is already true in the destination, read from the destination itself. + * + * The wizard used to remember what a run had done — a note in `sessionStorage` saying "this + * run created workspace X" — which a reload could outlive but a stale entry could also + * outlive the workspace it named. Everything here is asked of the instance instead, so there + * is nothing to go stale and nothing to clear: the plan in the URL says what should exist, + * and these functions say what does. + * + * The one thing the instance cannot answer is *which tables a migration was supposed to + * create*. That is inferred from the SQL the project ships (`expectedTables`), because the + * export states only what to run, never what running it should produce. + */ + +import { ResourceService, ScriptService, FlowService, AppService, WorkspaceService } from '$lib/gen' +import type { + ProjectExport, + ProjectMigration +} from '$lib/components/workspaceSettings/projectBundle' + +/** + * The tables a migration creates, as `schema.table`, read off its `CREATE TABLE` statements. + * + * Inference, not a contract: the export ships SQL and nothing else, so this is the only way + * to check a migration's work without a record of it having run. It deliberately reads only + * the shape this project's generator emits (`datatableSchemaSql.ts` always writes the + * schema-qualified, quoted form) — anything hand-edited into a different shape simply reads + * as no expected tables, which makes the caller fall back to "cannot tell" rather than to a + * confident wrong answer. + */ +export function expectedTables(sql: string): string[] { + const out: string[] = [] + const re = /CREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?"([^"]+)"\s*\.\s*"([^"]+)"/gi + let m: RegExpExecArray | null + while ((m = re.exec(sql)) !== null) out.push(`${m[1]}.${m[2]}`) + 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 + /** …and it is one they own, so a run of theirs is what made it. */ + ours: boolean +} + +/** + * Whether the destination workspace is there, and whether it is the caller's. + * + * `listWorkspaces` answers both in one call: it returns only workspaces the caller is a + * member of, each carrying the `owner` email set at creation. Ownership is what makes + * skipping a create safe — an id that exists but belongs to someone else is not a workspace + * this run made, and importing into it would be importing into a stranger's. + */ +export async function probeWorkspace( + id: string, + email: string | undefined +): Promise { + try { + const mine = await WorkspaceService.listWorkspaces() + const found = mine.find((w) => w.id === id) + if (!found) return { exists: false, ours: false } + return { exists: true, ours: !!email && found.owner === email } + } catch { + // Cannot tell. Reported as absent so the caller creates rather than adopts: a failed + // create is a clear error, adopting the wrong workspace is a silent one. + return { exists: false, ours: false } + } +} + +/** + * Which of the paths the import would write are already there. + * + * 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. + */ +export async function probeImportedPaths(workspace: string, folder: string): 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 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) + ] + // 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. + await Promise.allSettled(calls) + return paths +} + +/** + * Whether a migration's tables are in the data table it targets. + * + * The ground truth for "did this run", and the only one that covers both paths + * `applyOneMigration` takes: it records a migration when the data table has migrations + * enabled, and otherwise runs the SQL once as a job that nothing remembers. The tables + * outlive both. + * + * `undefined` means the question could not be answered — the schema was unreadable, or the + * SQL named no tables this can recognise. Distinct from `false`, because "not there" invites + * a caller to run the migration and "cannot tell" does not. + */ +export async function probeMigrationApplied( + workspace: string, + migration: ProjectMigration +): Promise { + const wanted = expectedTables(migration.sql ?? '') + if (wanted.length === 0) return undefined + try { + const schema = (await WorkspaceService.getDatatableFullSchema({ + workspace, + requestBody: { source: `datatable://${migration.datatable_name}` } + })) as Record> + const present = new Set() + for (const [schemaName, tables] of Object.entries(schema ?? {})) { + for (const table of Object.keys(tables ?? {})) present.add(`${schemaName}.${table}`) + } + return wanted.every((t) => present.has(t)) + } catch { + return undefined + } +}