diff --git a/.changeset/keep-recorded-step-ids.md b/.changeset/keep-recorded-step-ids.md new file mode 100644 index 000000000..5b0a4bb6a --- /dev/null +++ b/.changeset/keep-recorded-step-ids.md @@ -0,0 +1,6 @@ +--- +'@openfn/project': patch +'@openfn/cli': patch +--- + +Keep the step ids a project already has when pulling diff --git a/packages/cli/src/projects/fetch.ts b/packages/cli/src/projects/fetch.ts index e87f0b3fa..bf8c8c13b 100644 --- a/packages/cli/src/projects/fetch.ts +++ b/packages/cli/src/projects/fetch.ts @@ -1,6 +1,6 @@ import yargs from 'yargs'; import path from 'node:path'; -import Project, { Workspace } from '@openfn/project'; +import Project, { Workspace, recordedIdsOf } from '@openfn/project'; import { build, ensure, override } from '../util/command-builders'; import type { Logger } from '../util/logger'; @@ -286,6 +286,8 @@ export async function fetchRemoteProject( { ...workspace.getConfig(), alias: options.alias ?? localProject?.alias ?? 'main', + // Keep the ids this project already gave its steps. + recordedIds: localProject ? recordedIdsOf(localProject) : undefined, } ); diff --git a/packages/project/src/index.ts b/packages/project/src/index.ts index dbcd401a6..3e1bd28d9 100644 --- a/packages/project/src/index.ts +++ b/packages/project/src/index.ts @@ -22,7 +22,8 @@ export { match as versionsEqual, } from './util/version'; -export { mapWorkflow } from './parse/from-app-state'; +export { mapWorkflow, recordedIdsOf } from './parse/from-app-state'; +export type { RecordedIds } from './parse/from-app-state'; export { default as detectVersion } from './util/detect-version'; diff --git a/packages/project/src/parse/from-app-state.ts b/packages/project/src/parse/from-app-state.ts index 7f68ac9aa..b161cde3b 100644 --- a/packages/project/src/parse/from-app-state.ts +++ b/packages/project/src/parse/from-app-state.ts @@ -13,6 +13,8 @@ import getCredentialName from '../util/get-credential-name'; export type fromAppStateConfig = Partial & { format?: 'yaml' | 'json'; alias?: string; + + recordedIds?: RecordedIds; }; export default ( @@ -22,6 +24,8 @@ export default ( ) => { let stateJson = ensureJson(state); delete config.format; + const { recordedIds, ...projectConfig } = config; + config = projectConfig; const { id, @@ -77,8 +81,18 @@ export default ( }; } - proj.workflows = Object.values(stateJson.workflows).map((w) => - mapWorkflow(w, proj.credentials) + const allWorkflows = Object.values(stateJson.workflows); + const workflowIds = resolveWorkflowIds( + allWorkflows, + recordedIds?.workflows ?? {} + ); + proj.workflows = allWorkflows.map((w) => + mapWorkflow( + w, + proj.credentials, + recordedIds?.steps?.[w.name] ?? {}, + workflowIds[w.id] + ) ); return new Project(proj as l.ProjectState, config); @@ -109,14 +123,112 @@ export const mapEdge = (edge: Provisioner.Edge) => { return e; }; +// Ids a project has already given its workflows and steps. Matched by name, +// since the files carry no uuids, which is why a rename still moves things. +export type RecordedIds = { + workflows: Record; + steps: Record>; +}; + +// An id becomes a directory and a file name, and a workflow file can come from +// a repository somebody else prepared, so `../../..` is not to be trusted. +const isSafeId = (id: unknown): id is string => + typeof id === 'string' && id.length > 0 && slugify(id) === id; + +export const recordedIdsOf = (project: { + workflows?: { + id?: string; + name?: string; + steps?: { id?: string; name?: string }[]; + }[]; +}): RecordedIds => { + const workflows: Record = Object.create(null); + const steps: Record> = Object.create(null); + + for (const workflow of project?.workflows ?? []) { + if (!workflow?.name) continue; + if (isSafeId(workflow.id)) { + workflows[workflow.name] = workflow.id!; + } + const forWorkflow: Record = Object.create(null); + for (const step of workflow.steps ?? []) { + if (step?.name && isSafeId(step.id)) { + forWorkflow[step.name] = step.id!; + } + } + steps[workflow.name] = forWorkflow; + } + + return { workflows, steps }; +}; + +// Both passes run in uuid order so the result does not depend on how the server +// listed things. +const assignIds = ( + items: { id: string; name?: string }[], + recorded: Record, + fallback: string, + reserved: string[] = [] +): Record => { + const byUuid: Record = Object.create(null); + const taken = new Set(reserved); + + const inUuidOrder = items + .slice() + .sort((a, b) => (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); + + for (const item of inUuidOrder) { + const known = item.name ? recorded[item.name] : undefined; + if (isSafeId(known) && !taken.has(known)) { + byUuid[item.id] = known; + taken.add(known); + } + } + + for (const item of inUuidOrder) { + if (byUuid[item.id]) continue; + const base = slugify(item.name ?? '') || fallback; + let candidate = base; + let n = 2; + while (taken.has(candidate)) { + candidate = `${base}-${n++}`; + } + byUuid[item.id] = candidate; + taken.add(candidate); + } + + return byUuid; +}; + +export const resolveWorkflowIds = ( + workflows: Provisioner.Workflow[], + recorded: Record = {} +) => assignIds(workflows, recorded, 'workflow'); + +export const resolveStepIds = ( + jobs: Record, + triggers: Record = {}, + recorded: Record = {} +): Record => { + // A trigger's id is its type and shares the directory, so reserve those first. + const triggerIds = Object.values(triggers) + .map((trigger) => trigger?.type) + .filter(Boolean) as string[]; + + return assignIds(Object.values(jobs), recorded, 'step', triggerIds); +}; + // map a project workflow to a local cli workflow // TODO this probably gets easier if I index everything by name export const mapWorkflow = ( workflow: Provisioner.Workflow, - credentials: l.CredentialState[] = [] + credentials: l.CredentialState[] = [], + recordedStepIds: Record = {}, + workflowId?: string ) => { const { jobs, edges, triggers, name, version_history, ...remoteProps } = workflow; + const stepIds = resolveStepIds(jobs, triggers, recordedStepIds); const mapped: l.WorkflowState = { name: workflow.name, steps: [], @@ -124,7 +236,7 @@ export const mapWorkflow = ( openfn: renameKeys(remoteProps, { id: 'uuid' }), }; if (workflow.name) { - mapped.id = slugify(workflow.name); + mapped.id = workflowId ?? slugify(workflow.name); } // TODO what do we do if the condition is disabled? @@ -165,8 +277,7 @@ export const mapWorkflow = ( if (!target) { throw new Error(`Failed to find ${edge.target_job_id}`); } - // we use the name, not the id, to reference - obj[slugify(target.name)] = mapEdge(edge); + obj[stepIds[target.id]] = mapEdge(edge); return obj; }, {}), }) as l.Trigger @@ -187,7 +298,7 @@ export const mapWorkflow = ( } = step; const s: any /*l.Job*/ = { - id: slugify(name), + id: stepIds[step.id], name: name, expression, adaptor, // TODO is this wrong? @@ -210,7 +321,7 @@ export const mapWorkflow = ( (j) => j.id === edge.target_job_id ); // @ts-ignore - next[slugify(target.name)] = mapEdge(edge); + next[stepIds[target.id]] = mapEdge(edge); return next; }, {}); } diff --git a/packages/project/test/parse/from-app-state.test.ts b/packages/project/test/parse/from-app-state.test.ts index f1db229bb..2ffcd301c 100644 --- a/packages/project/test/parse/from-app-state.test.ts +++ b/packages/project/test/parse/from-app-state.test.ts @@ -2,6 +2,7 @@ import test from 'ava'; import fromAppState, { mapEdge, mapWorkflow, + recordedIdsOf, } from '../../src/parse/from-app-state'; import { cloneDeep } from 'lodash-es'; @@ -548,3 +549,260 @@ workflows: t.is(project.workflows[0].name, 'wf1'); t.is(project.workflows[0].steps.length, 2); }); + +// --- recorded step ids --------------------------------------------------- +// +// An id derived from a name loses whatever is not url-safe, so two names that +// differ only in emoji or an accent used to land on the same id and one step's +// code was written over the other on the way to disk. + +const twoStepsSharingAnId = () => ({ + id: 'p1', + name: 'demo', + project_credentials: [], + workflows: [ + { + id: 'wf-uuid', + name: 'My Workflow', + triggers: {}, + edges: {}, + jobs: { + j1: { + id: 'bbbb', + name: 'step \u{1F44D}', + body: 'up()', + adaptor: 'common', + }, + j2: { + id: 'aaaa', + name: 'step \u{1F44E}', + body: 'down()', + adaptor: 'common', + }, + }, + }, + ], +}); + +// name -> id, so a test can tell "both are distinct" from "they swapped" +const idsByName = (project: any, index = 0) => + Object.fromEntries( + (Object.values(project.workflows)[index] as any).steps.map((s: any) => [ + s.name, + s.id, + ]) + ); + +test('two names that shorten to the same id still get distinct ids', (t) => { + const ids = idsByName(fromAppState(twoStepsSharingAnId() as any, meta)); + + t.is(Object.keys(ids).length, 2); + t.is(new Set(Object.values(ids)).size, 2); +}); + +test('both step bodies reach the filesystem', (t) => { + const project = fromAppState(twoStepsSharingAnId() as any, meta); + + const files = project.serialize('fs') as Record; + const scripts = Object.keys(files).filter((f) => f.endsWith('.js')); + + t.is(scripts.length, 2); + t.true(Object.values(files).some((c) => c.includes('up()'))); + t.true(Object.values(files).some((c) => c.includes('down()'))); +}); + +test('an id we have already written down is kept', (t) => { + const project = fromAppState(twoStepsSharingAnId() as any, meta, { + recordedIds: { + workflows: {}, + steps: { + 'My Workflow': { + 'step \u{1F44D}': 'thumbs-up', + 'step \u{1F44E}': 'thumbs-down', + }, + }, + }, + }); + + t.deepEqual(idsByName(project), { + 'step \u{1F44D}': 'thumbs-up', + 'step \u{1F44E}': 'thumbs-down', + }); +}); + +test('a step keeps its own id rather than swapping with its neighbour', (t) => { + const state: any = twoStepsSharingAnId(); + const reversed: any = twoStepsSharingAnId(); + reversed.workflows[0].jobs = { + j2: reversed.workflows[0].jobs.j2, + j1: reversed.workflows[0].jobs.j1, + }; + + t.deepEqual( + idsByName(fromAppState(state, meta)), + idsByName(fromAppState(reversed, meta)) + ); +}); + +test('a step named after a trigger does not take the trigger id', (t) => { + const state: any = twoStepsSharingAnId(); + state.workflows[0].triggers = { + t: { id: 'tttt', type: 'webhook', enabled: true }, + }; + state.workflows[0].jobs.j1.name = 'Webhook'; + + const project = fromAppState(state, meta); + const files = project.serialize('fs') as Record; + + t.not(idsByName(project)['Webhook'], 'webhook'); + t.is(Object.keys(files).filter((f) => f.endsWith('.js')).length, 2); +}); + +test('two workflows can each hold a step of the same name', (t) => { + const state: any = { + id: 'p', + name: 'demo', + project_credentials: [], + workflows: [ + { + id: 'w1', + name: 'One', + triggers: {}, + edges: {}, + jobs: { + a: { id: 'u-a', name: 'Transform', body: 'one()', adaptor: 'c' }, + }, + }, + { + id: 'w2', + name: 'Two', + triggers: {}, + edges: {}, + jobs: { + b: { id: 'u-b', name: 'Transform', body: 'two()', adaptor: 'c' }, + }, + }, + ], + }; + + const first = fromAppState(state, meta); + const recorded = recordedIdsOf(first); + + // each workflow keeps its own entry rather than one overwriting the other + t.deepEqual(Object.keys(recorded.steps).sort(), ['One', 'Two']); + + const second = fromAppState(state, meta, { recordedIds: recorded }); + t.deepEqual(idsByName(second, 0), idsByName(first, 0)); + t.deepEqual(idsByName(second, 1), idsByName(first, 1)); +}); + +test('a step named like an object property does not pick one up', (t) => { + const state: any = twoStepsSharingAnId(); + state.workflows[0].jobs.j1.name = 'constructor'; + state.workflows[0].jobs.j2.name = 'toString'; + + const project = fromAppState(state, meta); + const ids = Object.values(idsByName(project)); + const files = project.serialize('fs') as Record; + + t.true(ids.every((id) => typeof id === 'string' && id.length > 0)); + t.is(new Set(ids).size, 2); + t.is(Object.keys(files).filter((f) => f.endsWith('.js')).length, 2); +}); + +test('an id from a project file that escapes the directory is ignored', (t) => { + const project = fromAppState(twoStepsSharingAnId() as any, meta, { + recordedIds: { + workflows: {}, + steps: { 'My Workflow': { 'step \u{1F44D}': '../../../../tmp/pwned' } }, + }, + }); + + const files = project.serialize('fs') as Record; + + t.false(Object.values(idsByName(project)).includes('../../../../tmp/pwned')); + t.false(Object.keys(files).some((f) => f.includes('..'))); +}); + +test('a project on disk hands back the ids it already gave its steps', (t) => { + const first = fromAppState(twoStepsSharingAnId() as any, meta); + + const second = fromAppState(twoStepsSharingAnId() as any, meta, { + recordedIds: recordedIdsOf(first), + }); + + t.deepEqual(idsByName(second), idsByName(first)); +}); + +test('the recorded ids are not written into the workspace config', (t) => { + const project = fromAppState(twoStepsSharingAnId() as any, meta, { + recordedIds: { + workflows: {}, + steps: { 'My Workflow': { 'step \u{1F44D}': 'thumbs-up' } }, + }, + }); + + const files = project.serialize('fs') as Record; + + t.false(JSON.stringify(files).includes('recordedIds')); +}); + +test('renaming a step still moves its id', (t) => { + const withName = (name: string) => ({ + id: 'p', + name: 'demo', + project_credentials: [], + workflows: [ + { + id: 'w', + name: 'My Workflow', + triggers: {}, + edges: {}, + jobs: { a: { id: 'u-a', name, body: 'code()', adaptor: 'c' } }, + }, + ], + }); + + const first = fromAppState(withName('Send data') as any, meta); + const recorded = recordedIdsOf(first); + + const renamed = fromAppState(withName('Send data v2') as any, meta, { + recordedIds: recorded, + }); + + t.is(idsByName(first)['Send data'], 'send-data'); + t.is(idsByName(renamed)['Send data v2'], 'send-data-v2'); +}); + +test('two workflow names that shorten to the same id stay distinct', (t) => { + const state: any = { + id: 'p', + name: 'demo', + project_credentials: [], + workflows: [ + { + id: 'w1', + name: 'wf \u{1F44D}', + triggers: {}, + edges: {}, + jobs: { a: { id: 'u-a', name: 'A', body: 'one()', adaptor: 'c' } }, + }, + { + id: 'w2', + name: 'wf \u{1F44E}', + triggers: {}, + edges: {}, + jobs: { b: { id: 'u-b', name: 'B', body: 'two()', adaptor: 'c' } }, + }, + ], + }; + + const project = fromAppState(state, meta); + const ids = Object.values(project.workflows).map((w: any) => w.id); + + t.is(new Set(ids).size, 2); + + // on main this throws `step not found: b` + const files = project.serialize('fs') as Record; + t.is(Object.keys(files).filter((f) => f.endsWith('.js')).length, 2); +});