Keep the step ids a project already has

.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

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,
     }
   );
 

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';
 

packages/project/src/parse/from-app-state.ts

@@ -13,6 +13,8 @@ import getCredentialName from '../util/get-credential-name';
 export type fromAppStateConfig = Partial<l.WorkspaceConfig> & {
   format?: 'yaml' | 'json';
   alias?: string;
+
+  recordedIds?: RecordedIds;
 };
 
 export default (
@@ -22,6 +24,8 @@ export default (
 ) => {
   let stateJson = ensureJson<Provisioner.Project>(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,22 +123,120 @@ 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<string, string>;
+  steps: Record<string, Record<string, string>>;
+};
+
+// 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<string, string> = Object.create(null);
+  const steps: Record<string, Record<string, string>> = 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<string, string> = 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<string, string>,
+  fallback: string,
+  reserved: string[] = []
+): Record<string, string> => {
+  const byUuid: Record<string, string> = 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<string, string> = {}
+) => assignIds(workflows, recorded, 'workflow');
+
+export const resolveStepIds = (
+  jobs: Record<string, Provisioner.Job>,
+  triggers: Record<string, Provisioner.Trigger> = {},
+  recorded: Record<string, string> = {}
+): Record<string, string> => {
+  // 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<string, string> = {},
+  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: [],
     history: workflow.version_history ?? [],
     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;
       }, {});
     }