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/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-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/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/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/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-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[ 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', + }); +});