Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions calm-models/src/diff/diff-types.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type {
CalmControlDetailSchema,
CalmControlsSchema,
CalmFlowSchema,
CalmNodeSchema,
CalmRelationshipSchema,
} from '../types/index.js';
Expand All @@ -15,6 +16,11 @@ export interface RelationshipChange {
updated: CalmRelationshipSchema;
}

export interface FlowChange {
original: CalmFlowSchema;
updated: CalmFlowSchema;
}

export interface RenameMapping {
oldId: string;
newId: string;
Expand Down Expand Up @@ -105,7 +111,14 @@ export interface ControlDiffResult {
controlItemsModified: { [controlId: string]: ControlItemDiffResult };
}

export interface FlowDiffResult {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this is the only composed result interface here without a doc comment (AdrDiffResult/ControlDiffResult above both have one).

flowsAdded: CalmFlowSchema[];
flowsRemoved: CalmFlowSchema[];
flowsModified: FlowChange[];
flowsSame: CalmFlowSchema[];
}

/**
* Represents the result of diffing two CALM architecture instances: includes nodes, relationships, ADRs and controls. TODO: incorporate flows and metadata.
* Represents the result of diffing two CALM architecture instances: includes nodes, relationships, flows, ADRs and controls. TODO: incorporate metadata.
*/
export type ArchitectureDiffResult = NodesAndRelationshipsDiffResult & AdrDiffResult & ControlDiffResult;
export type ArchitectureDiffResult = NodesAndRelationshipsDiffResult & AdrDiffResult & ControlDiffResult & FlowDiffResult;
29 changes: 29 additions & 0 deletions calm-models/src/diff/diff.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,35 @@ describe('diff', () => {
});
});

describe('diffArchitectures - flow changes', () => {
const flow = {
'unique-id': 'write-flow',
name: 'Write flow',
description: 'Writes a record.',
transitions: [{ 'relationship-unique-id': 'gateway-to-payment', 'sequence-number': 1, description: 'Write.' }],
};

it('detects added, removed, modified, and unchanged flows', () => {
const base = { ...testArchitectures.baseArchitecture, flows: [flow] } as CalmArchitectureSchema;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we perhaps create new architectures within ./fixtures/diff-test-architectures.json? This is the current convention in these specs, rather than relying on the base architecture. Maybe we could update the base architecture to add flows: [flow] and then create a new architecture based on changed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that fits the existing specs better. I'll move these cases into diff-test-architectures.json, add the flow to the base fixture, and derive the changed case from it. Keeps the test setup consistent with the rest of the file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Aman's convention point β€” worth moving before merge so the flow test cases match the rest of the file.

const changed = {
...base,
flows: [
{ ...flow, transitions: [...flow.transitions, { 'relationship-unique-id': 'gateway-to-payment', 'sequence-number': 2, description: 'Confirm.' }] },
{ ...flow, 'unique-id': 'read-flow', name: 'Read flow' },
],
} as CalmArchitectureSchema;
const result = diffArchitectures(base, changed);
expect(result.flowsAdded.map((item) => item['unique-id'])).toEqual(['read-flow']);
expect(result.flowsRemoved).toEqual([]);
expect(result.flowsModified).toHaveLength(1);
expect(result.flowsModified[0].original['unique-id']).toBe('write-flow');
expect(result.flowsSame).toEqual([]);

const removed = diffArchitectures(base, { ...base, flows: [] });
expect(removed.flowsRemoved.map((item) => item['unique-id'])).toEqual(['write-flow']);
});
});

describe('diffArchitectures - comprehensive scenarios', () => {
const empty = (): CalmArchitectureSchema => ({
nodes: [],
Expand Down
28 changes: 27 additions & 1 deletion calm-models/src/diff/diff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type {
CalmControlDetailSchema,
CalmControlSchema,
CalmControlsSchema,
CalmFlowSchema,
CalmNodeSchema,
CalmRelationshipSchema,
} from '../types/index.js';
Expand All @@ -17,6 +18,8 @@ import type {
ArchitectureDiffResult,
ControlDiffResult,
ControlItemDiffResult,
FlowChange,
FlowDiffResult,
ChangeType,
} from './diff-types.js';

Expand Down Expand Up @@ -92,14 +95,37 @@ export function diffArchitectures(
const adrDiff = diffAdrs(archA.adrs ?? [], archB.adrs ?? []);

const controlsDiff = diffControls(archA.controls ?? {}, archB.controls ?? {});
const flowsDiff = diffFlows(archA.flows ?? [], archB.flows ?? []);

return {
...nodesAndRelationshipsDiff,
...adrDiff,
...controlsDiff
...controlsDiff,
...flowsDiff,
};
}

export function diffFlows(flowsA: CalmFlowSchema[], flowsB: CalmFlowSchema[]): FlowDiffResult {
const flowsByIdA = new Map(flowsA.map((flow) => [flow['unique-id'], flow]));
const flowsByIdB = new Map(flowsB.map((flow) => [flow['unique-id'], flow]));
const flowsAdded = [...flowsByIdB.values()].filter((flow) => !flowsByIdA.has(flow['unique-id']));
const flowsRemoved = [...flowsByIdA.values()].filter((flow) => !flowsByIdB.has(flow['unique-id']));
const flowsModified: FlowChange[] = [];
const flowsSame: CalmFlowSchema[] = [];

for (const [id, flowA] of flowsByIdA) {
const flowB = flowsByIdB.get(id);
if (!flowB) continue;
if (valuesEqual(flowA, flowB)) {
flowsSame.push(flowA);
} else {
flowsModified.push({ original: flowA, updated: flowB });
}
}

return { flowsAdded, flowsRemoved, flowsModified, flowsSame };
}

/**
* Core diff over the node/relationship arrays that both architectures and
* (normalised) patterns reduce to. Matching is by `unique-id`; items missing
Expand Down
3 changes: 3 additions & 0 deletions calm-models/src/diff/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ export {
diffAdrs,
diffArchitectures,
diffControls,
diffFlows,
diffNodesAndRelationships,
nodeStructureMatches,
relationshipStructureMatches,
Expand All @@ -26,6 +27,8 @@ export type {
AdrDiffResult,
ArchitectureDiffResult,
ControlDiffResult,
FlowChange,
FlowDiffResult,
NodeChange,
NodesAndRelationshipsDiffResult,
RelationshipChange,
Expand Down
18 changes: 15 additions & 3 deletions shared/src/commands/diff/diff-core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
diffTimelineAdjacent,
diffTimelineMoments,
type ArchitectureResolver,
type FlowDiffResult,
type MomentDiff,
type NodesAndRelationshipsDiffResult,
type TimelineInput,
Expand All @@ -15,13 +16,15 @@ export type DiffOutputFormat = 'json' | 'summary';

export type DiffDocumentType = 'architecture' | 'pattern';

type DiffResult = NodesAndRelationshipsDiffResult & Partial<FlowDiffResult>;
Comment thread
markscott-ms marked this conversation as resolved.

export interface DiffRunResult {
diff: NodesAndRelationshipsDiffResult;
diff: DiffResult;
formatted: string;
hasChanges: boolean;
}

export function hasChanges(diff: NodesAndRelationshipsDiffResult): boolean {
export function hasChanges(diff: DiffResult): boolean {
return (
diff.nodesAdded.length > 0 ||
diff.nodesRemoved.length > 0 ||
Expand All @@ -31,6 +34,9 @@ export function hasChanges(diff: NodesAndRelationshipsDiffResult): boolean {
diff.edgesRemoved.length > 0 ||
diff.edgesModified.length > 0 ||
diff.edgesRenamed.length > 0 ||
(diff.flowsAdded?.length ?? 0) > 0 ||
(diff.flowsRemoved?.length ?? 0) > 0 ||
(diff.flowsModified?.length ?? 0) > 0 ||
(diff.invalidItems?.nodes.length ?? 0) > 0 ||
(diff.invalidItems?.relationships.length ?? 0) > 0 ||
(diff.undiffableItems?.nodes.length ?? 0) > 0 ||
Expand All @@ -56,7 +62,7 @@ function edgeLabel(edge: CalmRelationshipSchema): string {
}

export function formatDiff(
diff: NodesAndRelationshipsDiffResult,
diff: DiffResult,
format: DiffOutputFormat,
documentType: DiffDocumentType = 'architecture',
): string {
Expand All @@ -74,6 +80,9 @@ export function formatDiff(
`Nodes: +${diff.nodesAdded.length} -${diff.nodesRemoved.length} ~${diff.nodesModified.length} ↔${diff.nodesRenamed.length} =${diff.nodesSame.length}`,
`Relationships: +${diff.edgesAdded.length} -${diff.edgesRemoved.length} ~${diff.edgesModified.length} ↔${diff.edgesRenamed.length} =${diff.edgesSame.length}`,
];
if (diff.flowsAdded) {
lines.push(`Flows: +${diff.flowsAdded.length} -${diff.flowsRemoved!.length} ~${diff.flowsModified!.length} =${diff.flowsSame!.length}`);
}
if (invalidNodes + invalidEdges > 0) {
lines.push(`Invalid items: ${invalidNodes} node(s) + ${invalidEdges} relationship(s) skipped (missing unique-id)`);
}
Expand All @@ -95,6 +104,9 @@ export function formatDiff(
list('Relationships removed:', diff.edgesRemoved.map(edgeLabel));
list('Relationships modified:', diff.edgesModified.map((e) => edgeLabel(e.original)));
list('Relationships renamed:', diff.edgesRenamed.map((r) => `${r.oldId} -> ${r.newId}`));
list('Flows added:', diff.flowsAdded?.map((flow) => flow['unique-id']) ?? []);
list('Flows removed:', diff.flowsRemoved?.map((flow) => flow['unique-id']) ?? []);
list('Flows modified:', diff.flowsModified?.map((flow) => flow.original['unique-id']) ?? []);
return lines.join('\n');
}

Expand Down
20 changes: 20 additions & 0 deletions shared/src/commands/diff/diff.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,9 @@ describe('hasChanges', () => {
['edgesRemoved'],
['edgesModified'],
['edgesRenamed'],
['flowsAdded'],
['flowsRemoved'],
['flowsModified'],
] as const)('returns true when %s has entries', (key) => {
const r: NodesAndRelationshipsDiffResult = { ...emptyResult, [key]: [{ placeholder: true }] as never };
expect(hasChanges(r)).toBe(true);
Expand Down Expand Up @@ -327,6 +330,23 @@ describe('diff core', () => {
expect(result.formatted).toContain('Nodes added:');
});

it('reports flow changes in the summary and hasChanges result', async () => {
const { diffDocuments } = await import('./diff-core');
const withFlow = {
...archA,
flows: [{
'unique-id': 'write-flow',
name: 'Write flow',
description: 'Writes a record.',
transitions: [],
}],
};
const result = diffDocuments(archA, withFlow, { format: 'summary' });
expect(result.hasChanges).toBe(true);
expect(result.formatted).toContain('Flows: +1 -0 ~0 =0');
expect(result.formatted).toContain('Flows added:\n - write-flow');
});

it('uses labels in the mismatch error instead of file paths', async () => {
const { diffDocuments } = await import('./diff-core');
expect(() => diffDocuments(archA, archB, { documentType: 'pattern', labels: ['left.json', 'right.json'] }))
Expand Down
Loading