From 8f9906aae2414be1d5fdf44b14fece1d884ac9da Mon Sep 17 00:00:00 2001 From: Frank Midigo Date: Thu, 29 Oct 2026 18:04:05 +0300 Subject: [PATCH 1/2] Keep non-ASCII letters and punctuation in ids slugify replaced every non-word character with a hyphen, so names in other scripts lost their letters and their ids didn't match Lightning's. It now only replaces characters that aren't safe in file names. Names that differ only by case or by space vs hyphen still give the same id. A pull now rejects them, as Lightning's export does, so the resources.yaml writer no longer adds -2 suffixes to channel keys. --- .changeset/unique-slug-ids.md | 6 ++ packages/project/src/parse/from-app-state.ts | 31 +++++++++ packages/project/src/util/resources.ts | 10 +-- packages/project/src/util/slugify.ts | 7 +- .../project/test/parse/from-app-state.test.ts | 68 +++++++++++++++++++ 5 files changed, 114 insertions(+), 8 deletions(-) create mode 100644 .changeset/unique-slug-ids.md diff --git a/.changeset/unique-slug-ids.md b/.changeset/unique-slug-ids.md new file mode 100644 index 000000000..826370a1a --- /dev/null +++ b/.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. diff --git a/packages/project/src/parse/from-app-state.ts b/packages/project/src/parse/from-app-state.ts index 9b4c8e2b8..36aba73d5 100644 --- a/packages/project/src/parse/from-app-state.ts +++ b/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 = {}; + 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) => { diff --git a/packages/project/src/util/resources.ts b/packages/project/src/util/resources.ts index 6f21905a3..6655cefbf 100644 --- a/packages/project/src/util/resources.ts +++ b/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, diff --git a/packages/project/src/util/slugify.ts b/packages/project/src/util/slugify.ts index 4c28a71c8..5af538a13 100644 --- a/packages/project/src/util/slugify.ts +++ b/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(/^\./, '-') ?? '' ); } diff --git a/packages/project/test/parse/from-app-state.test.ts b/packages/project/test/parse/from-app-state.test.ts index 9876af3ff..14a2fe6cd 100644 --- a/packages/project/test/parse/from-app-state.test.ts +++ b/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[ From 074c5fb250cca5106ba840e99f64be46d33b750f Mon Sep 17 00:00:00 2001 From: Frank Midigo Date: Thu, 29 Oct 2026 18:04:05 +0300 Subject: [PATCH 2/2] Track channel ids in the checkout file The new slug rules change some resources.yaml keys, and merging only matched channels by key, so a channel whose key changed lost its id and was treated as removed. The checkout file now records channel_uuids by key, as it already does for workflows. A merge matches channels by id first, then by key, then by name. --- packages/project/src/Project.ts | 4 +- packages/project/src/merge/merge-project.ts | 35 ++++++++++---- packages/project/src/parse/from-fs.ts | 9 +++- packages/project/src/util/config.ts | 13 ++++- .../project/test/merge/merge-project.test.ts | 47 +++++++++++++++++++ packages/project/test/parse/from-fs.test.ts | 16 +++++++ packages/project/test/util/config.test.ts | 19 ++++++++ 7 files changed, 129 insertions(+), 14 deletions(-) diff --git a/packages/project/src/Project.ts b/packages/project/src/Project.ts index e6040f5be..44dcee47c 100644 --- a/packages/project/src/Project.ts +++ b/packages/project/src/Project.ts @@ -42,7 +42,7 @@ type CLIMeta = { version?: number; alias?: string | null; forked_from?: Record; - // uuid of each workflow, by local id. Tracked but not used + // uuid of each workflow, by local id workflow_uuids?: Record; }; @@ -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; diff --git a/packages/project/src/merge/merge-project.ts b/packages/project/src/merge/merge-project.ts index 566049bf4..cf2eea399 100644 --- a/packages/project/src/merge/merge-project.ts +++ b/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(); + 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 = ( diff --git a/packages/project/src/parse/from-fs.ts b/packages/project/src/parse/from-fs.ts index 6cd8c7ecc..28cdbcb9e 100644 --- a/packages/project/src/parse/from-fs.ts +++ b/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) : undefined; + const channelUuids = isPlainObject(checkout.channel_uuids) + ? (checkout.channel_uuids as Record) + : {}; 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!) diff --git a/packages/project/src/util/config.ts b/packages/project/src/util/config.ts index d2a9f1c97..e7b33815b 100644 --- a/packages/project/src/util/config.ts +++ b/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)), diff --git a/packages/project/test/merge/merge-project.test.ts b/packages/project/test/merge/merge-project.test.ts index 8671afeb1..439b9b81a 100644 --- a/packages/project/test/merge/merge-project.test.ts +++ b/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: [ diff --git a/packages/project/test/parse/from-fs.test.ts b/packages/project/test/parse/from-fs.test.ts index 3d16c47f2..7572dd296 100644 --- a/packages/project/test/parse/from-fs.test.ts +++ b/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); +}); diff --git a/packages/project/test/util/config.test.ts b/packages/project/test/util/config.test.ts index 63434d79e..b8fb37286 100644 --- a/packages/project/test/util/config.test.ts +++ b/packages/project/test/util/config.test.ts @@ -369,3 +369,22 @@ test("checkout file excludes workflow_uuids if there aren't any", (t) => { const result = extractCheckout(proj); t.false(result.content.includes('workflow_uuids')); }); + +test('generate checkout file with channel_uuids, keyed by resources key', (t) => { + const proj = new Project({ + id: 'my-project', + channels: [ + // remote channel: keyed by its slugified name + { id: 'chan-1', name: 'My Channel (v2)' } as any, + // local channel: keeps its key + { id: 'chan-2', key: 'local', name: 'Other' } as any, + // not yet deployed + { key: 'new', name: 'New' } as any, + ], + }); + const result = extractCheckout(proj); + t.deepEqual(yamlToJson(result.content).channel_uuids, { + 'my-channel-(v2)': 'chan-1', + local: 'chan-2', + }); +});