Keep non-ASCII letters and punctuation in ids derived from names

.changeset/unique-slug-ids.md

@@ -0,0 +1,6 @@
+---
+'@openfn/project': minor
+'@openfn/cli': patch
+---
+
+Workflow, step and channel ids now keep non-ASCII letters and punctuation from their names, matching Lightning. Names with these characters get new ids, so their folders and resources.yaml keys change on the next pull.

packages/project/src/Project.ts

@@ -42,7 +42,7 @@ type CLIMeta = {
   version?: number;
   alias?: string | null;
   forked_from?: Record<string, string>;
-  // uuid of each workflow, by local id. Tracked but not used
+  // uuid of each workflow, by local id
   workflow_uuids?: Record<string, string>;
 };
 
@@ -54,7 +54,7 @@ export class Project {
   /** Human readable project name. This corresponds to the label in Lightning */
   name?: string;
 
-  /** Project id. Must be url safe. May be derived from the name. NOT a UUID */
+  /** Project id. Must be file safe. May be derived from the name. NOT a UUID */
   id: string;
 
   description?: string;

packages/project/src/merge/merge-project.ts

@@ -201,10 +201,10 @@ export function merge(
     baseMerge(target, source, ['channels'], assigns as any)
   );
 
-  // target channels with an id that the source dropped have been removed
-  const kept = new Set(source.channels?.map(channelKey));
+  // target channels whose id didn't match any source channel have been removed
+  const kept = new Set(merged.channels?.map((c) => c.id));
   merged.removedChannels = source.channels
-    ? (target.channels ?? []).filter((c) => c.id && !kept.has(channelKey(c)))
+    ? (target.channels ?? []).filter((c) => c.id && !kept.has(c.id))
     : [];
   return merged;
 }
@@ -225,8 +225,9 @@ export function mergeCollections(
   }));
 }
 
-// Source channels win, but keep the target's id on a key match. Matching on the
-// key rather than the name means renaming a channel keeps its id
+// Source channels win, but keep the target's id on a match. Like workflows,
+// a channel is matched by id (from the checkout file), then key, then name.
+// Matching on the id or key means renaming a channel keeps its id
 // If the source has no channels at all (no resources.yaml), keep the target's
 export function mergeChannels(
   source: ChannelState[] | undefined,
@@ -239,11 +240,25 @@ export function mergeChannels(
   if (!source) {
     return target;
   }
-  const targetByKey = new Map((target ?? []).map((c) => [channelKey(c), c]));
-  return source.map((c) => ({
-    ...c,
-    id: targetByKey.get(channelKey(c))?.id,
-  }));
+  const available = [...(target ?? [])];
+  const claim = (match?: ChannelState) => {
+    if (match) available.splice(available.indexOf(match), 1);
+    return match;
+  };
+  const matches = new Map<ChannelState, ChannelState | undefined>();
+  const passes = [
+    (s: ChannelState, t: ChannelState) => !!s.id && s.id === t.id,
+    (s: ChannelState, t: ChannelState) => channelKey(s) === channelKey(t),
+    (s: ChannelState, t: ChannelState) => s.name === t.name,
+  ];
+  for (const pass of passes) {
+    for (const s of source) {
+      if (!matches.get(s)) {
+        matches.set(s, claim(available.find((t) => pass(s, t))));
+      }
+    }
+  }
+  return source.map((c) => ({ ...c, id: matches.get(c)?.id }));
 }
 
 export const replaceCredentials = (

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

@@ -109,9 +109,35 @@ export default (
     return workflow;
   }) as unknown as l.WorkflowState[];
 
+  assertUniqueIds(
+    Object.values(stateJson.workflows).filter((w) => !w.delete),
+    'workflow names'
+  );
+
+  assertUniqueIds(channels ?? [], 'channel names');
+
   return new Project(proj as l.ProjectState, config);
 };
 
+// Ids are derived from names, so names that differ only by case or by
+// space vs hyphen (eg "Foo Bar" and "foo-bar") share an id and would overwrite
+// each other. Lightning's own export rejects these pairs too
+const assertUniqueIds = (items: Array<{ name: string }>, what: string) => {
+  const byId: Record<string, string[]> = {};
+  for (const { name } of items) {
+    (byId[slugify(name)] ??= []).push(name);
+  }
+  const dupes = Object.entries(byId).filter(([, names]) => names.length > 1);
+  if (dupes.length) {
+    const list = dupes
+      .map(([id, names]) => `${names.map((n) => `"${n}"`).join(', ')} -> ${id}`)
+      .join('\n  ');
+    throw new Error(
+      `These ${what} differ only by case or spaces vs hyphens, so they resolve to the same id:\n  ${list}\nRename one of them in Lightning so each has a unique id.`
+    );
+  }
+};
+
 // TODO maybe this is a util and moved out of this file
 export const mapEdge = (edge: Provisioner.Edge) => {
   const e: any = {
@@ -156,6 +182,11 @@ export const mapWorkflow = (
     mapped.id = slugify(workflow.name);
   }
 
+  assertUniqueIds(
+    Object.values(jobs).filter((j) => !j.delete),
+    `step names in workflow "${workflow.name}"`
+  );
+
   // TODO what do we do if the condition is disabled?
   // I don't think that's the same as edge condition false?
   Object.values(triggers).forEach((trigger: Provisioner.Trigger) => {

packages/project/src/parse/from-fs.ts

@@ -48,6 +48,9 @@ export const parseProject = async (options: FromFsConfig) => {
   const workflowUuids = isPlainObject(checkout.workflow_uuids)
     ? (checkout.workflow_uuids as Record<string, string>)
     : undefined;
+  const channelUuids = isPlainObject(checkout.channel_uuids)
+    ? (checkout.channel_uuids as Record<string, string>)
+    : {};
 
   const proj: any = {
     id: options.name ? slugify(options.name) : checkout.id,
@@ -57,6 +60,7 @@ export const parseProject = async (options: FromFsConfig) => {
       'alias',
       'forked_from',
       'workflow_uuids',
+      'channel_uuids',
       'collections',
     ]),
 
@@ -83,7 +87,10 @@ export const parseProject = async (options: FromFsConfig) => {
     });
   const parsed = resources ? yamlToJson(resources) : undefined;
   if (parsed?.channels) {
-    proj.channels = fromResourceChannels(parsed.channels);
+    // If the channel is tracked, we know its id
+    proj.channels = fromResourceChannels(parsed.channels).map((c) =>
+      channelUuids[c.key!] ? { ...c, id: channelUuids[c.key!] } : c
+    );
   }
   if (parsed && 'collections' in parsed) {
     // an empty key means no collections (not the same as no key!)

packages/project/src/util/config.ts

@@ -4,6 +4,7 @@ import path from 'node:path';
 import { pickBy, isNil } from 'lodash-es';
 import { yamlToJson, jsonToYaml } from './yaml';
 import Project from '../Project';
+import { channelKey } from './resources';
 
 // Recursively sort object keys so that config files serialize stably
 const sortKeys = (value: any): any => {
@@ -72,7 +73,6 @@ export const extractCheckout = (
   }
 
   // Track the uuid of each workflow, by local id
-  // This is recorded for future use and isn't read by anything yet
   const workflowUuids = source.workflows.reduce((obj: any, wf) => {
     if (wf.openfn?.uuid) {
       obj[wf.id] = wf.openfn.uuid;
@@ -83,6 +83,17 @@ export const extractCheckout = (
     project.workflow_uuids = workflowUuids;
   }
 
+  // And the id of each channel, by its key in resources.yaml
+  const channelUuids = (source.channels ?? []).reduce((obj: any, c) => {
+    if (c.id) {
+      obj[channelKey(c)] = c.id;
+    }
+    return obj;
+  }, {});
+  if (Object.keys(channelUuids).length) {
+    project.channel_uuids = channelUuids;
+  }
+
   return {
     path: getCheckoutPath(branch),
     content: jsonToYaml(sortKeys(project)),

packages/project/src/util/resources.ts

@@ -17,7 +17,7 @@ type ResourceChannels = Record<
   }
 >;
 
-// The id a channel is matched by on merge. Remote channels have no local key,
+// A channel's local id in resources.yaml. Remote channels have no local key,
 // so fall back to the name, slugified the same way as when it's written out
 export const channelKey = (c: l.ChannelState) => c.key ?? slugify(c.name);
 
@@ -37,12 +37,8 @@ export const toResourceChannels = (
       : c.destination_credential_id ?? undefined;
 
     // keep a local key if there is one, so renames survive a round trip
-    const base = channelKey(c);
-    let id = base;
-    for (let i = 2; id in result; i++) {
-      id = `${base}-${i}`;
-    }
-    result[id] = {
+    // (keys are unique: clashing names are rejected on pull)
+    result[channelKey(c)] = {
       name: c.name,
       destination_url: c.destination_url,
       enabled: c.enabled,

packages/project/src/util/slugify.ts

@@ -1,5 +1,10 @@
 export default function slugify(text: string) {
   return (
-    text?.replace(/\W/g, ' ').trim().replace(/\s+/g, '-').toLowerCase() ?? ''
+    text
+      ?.normalize('NFC')
+      .trim()
+      .toLowerCase()
+      .replace(/[\s/\\:*?"<>|]/g, '-')
+      .replace(/^\./, '-') ?? ''
   );
 }

packages/project/test/merge/merge-project.test.ts

@@ -282,6 +282,53 @@ test('replace mode: target channels missing from source are flagged as removed',
   t.deepEqual(result.removedChannels, [removed]);
 });
 
+test('replace mode: a channel is matched by its tracked id before its key', (t) => {
+  const wf = assignUUIDs({
+    steps: [
+      { id: 'x', name: 'X', adaptor: 'common', expression: 'fn(s => s)' },
+    ],
+  });
+  const channel = { destination_url: 'https://example.com', enabled: true };
+
+  // the key no longer matches the remote name (eg the slug rules changed),
+  // but the id from the checkout file does
+  const target = createProject(wf, 'a', {
+    channels: [{ ...channel, id: 'chan-1', name: 'My Channel (v2)' }],
+  });
+  const source = createProject(wf, 'b', {
+    channels: [
+      { ...channel, id: 'chan-1', key: 'my-channel-v2', name: 'Renamed' },
+    ],
+  });
+
+  const result = merge(source, target, { mode: REPLACE_MERGE });
+
+  t.is(result.channels![0].id, 'chan-1');
+  t.deepEqual(result.removedChannels, []);
+});
+
+test('replace mode: an untracked channel whose key changed is matched by name', (t) => {
+  const wf = assignUUIDs({
+    steps: [
+      { id: 'x', name: 'X', adaptor: 'common', expression: 'fn(s => s)' },
+    ],
+  });
+  const channel = { destination_url: 'https://example.com', enabled: true };
+
+  const target = createProject(wf, 'a', {
+    channels: [{ ...channel, id: 'chan-1', name: 'My Channel (v2)' }],
+  });
+  // key written by an older CLI, which stripped punctuation
+  const source = createProject(wf, 'b', {
+    channels: [{ ...channel, key: 'my-channel-v2', name: 'My Channel (v2)' }],
+  });
+
+  const result = merge(source, target, { mode: REPLACE_MERGE });
+
+  t.is(result.channels![0].id, 'chan-1');
+  t.deepEqual(result.removedChannels, []);
+});
+
 test('replace mode: renaming a channel keeps its id', (t) => {
   const wf = assignUUIDs({
     steps: [

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

@@ -107,6 +107,74 @@ test('should load a workflow flagged delete: true and flag it removed', (t) => {
   t.true(project.workflows[0].isRemoved());
 });
 
+test('should throw if two workflow names resolve to the same id', (t) => {
+  const s: any = cloneDeep(state);
+  s.workflows['cron-workflow'].name = 'webhook-workflow';
+
+  t.throws(() => fromAppState(s, meta), {
+    message: /"Webhook Workflow", "webhook-workflow" -> webhook-workflow/,
+  });
+});
+
+test('should not throw if a colliding workflow is flagged delete', (t) => {
+  const s: any = cloneDeep(state);
+  s.workflows['cron-workflow'].name = 'webhook-workflow';
+  s.workflows['cron-workflow'].delete = true;
+
+  t.notThrows(() => fromAppState(s, meta));
+});
+
+test('should throw if two step names resolve to the same id', (t) => {
+  const s: any = cloneDeep(state);
+  const wf = s.workflows['webhook-workflow'];
+  wf.jobs.other = {
+    ...wf.jobs['transform-data'],
+    id: 'x',
+    name: 'transform data',
+  };
+
+  t.throws(() => fromAppState(s, meta), {
+    message:
+      /in workflow "Webhook Workflow"[\s\S]*"Transform data", "transform data" -> transform-data/,
+  });
+});
+
+test('should keep non-ascii and punctuation in step and workflow ids', (t) => {
+  const s: any = cloneDeep(state);
+  const wf = s.workflows['webhook-workflow'];
+  wf.name = 'Flujo de atención (v2.0)';
+  wf.jobs['transform-data'].name = '患者確認';
+  wf.jobs.other = { ...wf.jobs['transform-data'], id: 'x', name: 'فحص المريض' };
+
+  const [workflow] = fromAppState(s, meta).workflows;
+  t.is(workflow.id, 'flujo-de-atención-(v2.0)');
+  t.deepEqual(
+    workflow.steps.map((s) => s.id),
+    ['webhook', '患者確認', 'فحص-المريض']
+  );
+  t.deepEqual(Object.keys(workflow.steps[0].next!), ['患者確認']);
+});
+
+test('should replace file-unsafe characters in ids', (t) => {
+  const s: any = cloneDeep(state);
+  s.workflows['webhook-workflow'].name = '..Q3/Q4 "Data:Sync"';
+
+  const [workflow] = fromAppState(s, meta).workflows;
+  t.is(workflow.id, '-.q3-q4--data-sync-');
+});
+
+test('should throw if two channel names resolve to the same id', (t) => {
+  const s: any = cloneDeep(state);
+  s.channels = [
+    { id: 'c1', name: 'My Channel', destination_url: 'https://a.com' },
+    { id: 'c2', name: 'my-channel', destination_url: 'https://b.com' },
+  ];
+
+  t.throws(() => fromAppState(s, meta), {
+    message: /"My Channel", "my-channel" -> my-channel/,
+  });
+});
+
 test('should load a job flagged delete: true and flag it removed', (t) => {
   const stateWithDeletedJob: any = cloneDeep(state);
   stateWithDeletedJob.workflows['webhook-workflow'].jobs[

packages/project/test/parse/from-fs.test.ts

@@ -565,3 +565,19 @@ test.serial('should prefer the file name to an id in the file', async (t) => {
   t.is(project.workflows.length, 1);
   t.is(project.workflows[0].id, 'my-workflow');
 });
+
+test.serial('should set the id on a tracked channel', async (t) => {
+  checkoutWithWorkflow({ channel_uuids: { 'my-channel': 'chan-1' } });
+  mockFile('/ws/resources.yaml', {
+    channels: {
+      'my-channel': { name: 'My Channel', destination_url: 'https://a.com' },
+      other: { name: 'Other', destination_url: 'https://b.com' },
+    },
+  });
+
+  const project = await parseProject({ root: '/ws' });
+
+  t.is(project.channels![0].id, 'chan-1');
+  t.falsy(project.channels![1].id);
+  t.falsy(project.openfn!.channel_uuids);
+});