From 4f65d0582f6b9ddbe2c2e51433d8318397a759a6 Mon Sep 17 00:00:00 2001 From: "Ricardo M." Date: Wed, 8 Jan 2025 12:11:10 +0000 Subject: [PATCH] fix(Canvas): Duplicated edge IDs Currently, edge IDs are not scoped to the owner flow, causing to be duplicated in case two flows use the same step combination. The fix is to scope the edge ID as well, so we avoid the duplication. fix: https://github.com/KaotoIO/kaoto/issues/1897 --- .../e2e/codeEditor/sourceCodeActions.cy.ts | 2 +- .../branchingStepAddition.cy.ts | 4 +- .../ui-tests/cypress/support/cypress.d.ts | 2 +- .../cypress/support/next-commands/design.ts | 4 +- .../Canvas/__snapshots__/Canvas.test.tsx.snap | 40 +++++++++---------- .../__snapshots__/flow.service.test.ts.snap | 2 +- .../Visualization/Canvas/flow.service.test.ts | 18 +++++++++ .../Visualization/Canvas/flow.service.ts | 1 + .../__snapshots__/nodes-edges.test.ts.snap | 4 +- 9 files changed, 49 insertions(+), 28 deletions(-) diff --git a/packages/ui-tests/cypress/e2e/codeEditor/sourceCodeActions.cy.ts b/packages/ui-tests/cypress/e2e/codeEditor/sourceCodeActions.cy.ts index 0450032f2..b10f76c5b 100644 --- a/packages/ui-tests/cypress/e2e/codeEditor/sourceCodeActions.cy.ts +++ b/packages/ui-tests/cypress/e2e/codeEditor/sourceCodeActions.cy.ts @@ -36,7 +36,7 @@ describe('Test source code editor', () => { cy.editorDeleteLine(12, 6); cy.openDesignPage(); // CHECK the kafka-sink step was removed - cy.checkNodeExist('kafka-sink', 0); + cy.checkNodeExist('integration|kafka-sink', 0); }); it('User edits step in the YAML', () => { diff --git a/packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts b/packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts index a2c2b80c4..328d2be8d 100644 --- a/packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts +++ b/packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts @@ -62,6 +62,7 @@ describe('Test for Branching actions from the canvas', () => { cy.checkNodeExist('activemq', 1); cy.checkEdgeExists( + 'eip-action', 'template.from.steps.1.choice.when.0.steps.1.setHeader', 'template.from.steps.1.choice.when.0.steps.2.to', ); @@ -77,6 +78,7 @@ describe('Test for Branching actions from the canvas', () => { cy.checkNodeExist('activemq', 1); cy.checkEdgeExists( + 'eip-action', 'template.from.steps.1.choice.when.0.steps.0.to', 'template.from.steps.1.choice.when.0.steps.1.to', ); @@ -91,6 +93,6 @@ describe('Test for Branching actions from the canvas', () => { cy.chooseFromCatalog('component', 'activemq'); cy.checkNodeExist('activemq', 1); - cy.checkEdgeExists('template.from.steps.2.to', 'template.from.steps.3.filter'); + cy.checkEdgeExists('eip-action', 'template.from.steps.2.to', 'template.from.steps.3.filter'); }); }); diff --git a/packages/ui-tests/cypress/support/cypress.d.ts b/packages/ui-tests/cypress/support/cypress.d.ts index e4f264895..4d6d62698 100644 --- a/packages/ui-tests/cypress/support/cypress.d.ts +++ b/packages/ui-tests/cypress/support/cypress.d.ts @@ -61,7 +61,7 @@ declare global { selectRemoveGroup(groupName: string, nodeIndex?: number): Chainable>; performNodeAction(nodeName: string, action: ActionType, nodeIndex?: number): Chainable>; checkNodeExist(inputName: string, nodesCount: number): Chainable>; - checkEdgeExists(sourceName: string, targetName: string): Chainable>; + checkEdgeExists(scope: string, sourceName: string, targetName: string): Chainable>; deleteBranch(branchIndex: number): Chainable>; selectCamelRouteType(type: string, subType?: string): Chainable>; selectRuntimeVersion(type: string): Chainable>; diff --git a/packages/ui-tests/cypress/support/next-commands/design.ts b/packages/ui-tests/cypress/support/next-commands/design.ts index ed39aea91..d650d3ed0 100644 --- a/packages/ui-tests/cypress/support/next-commands/design.ts +++ b/packages/ui-tests/cypress/support/next-commands/design.ts @@ -105,8 +105,8 @@ Cypress.Commands.add('checkNodeExist', (inputName, nodesCount) => { cy.get(`foreignObject[data-nodelabel="${inputName}"]`).should('have.length', nodesCount); }); -Cypress.Commands.add('checkEdgeExists', (sourceName: string, targetName: string) => { - const idPattern = `${sourceName} >>> ${targetName}`; +Cypress.Commands.add('checkEdgeExists', (scope: string, sourceName: string, targetName: string) => { + const idPattern = `${scope}|${sourceName} >>> ${targetName}`; // Check if an element with the matching id exists cy.get('g').should(($elements) => { // Use Cypress commands to check if any element matches the id pattern diff --git a/packages/ui/src/components/Visualization/Canvas/__snapshots__/Canvas.test.tsx.snap b/packages/ui/src/components/Visualization/Canvas/__snapshots__/Canvas.test.tsx.snap index c04160862..20131cfd0 100644 --- a/packages/ui/src/components/Visualization/Canvas/__snapshots__/Canvas.test.tsx.snap +++ b/packages/ui/src/components/Visualization/Canvas/__snapshots__/Canvas.test.tsx.snap @@ -141,7 +141,7 @@ exports[`Canvas Catalog button should NOT be present if \`CatalogModalContext\` data-layer-id="default" > >> set-header", + "id": "test|node >>> set-header", "source": "test|node", "target": "test|set-header", "type": "edge", diff --git a/packages/ui/src/components/Visualization/Canvas/flow.service.test.ts b/packages/ui/src/components/Visualization/Canvas/flow.service.test.ts index c48943215..3af1355e5 100644 --- a/packages/ui/src/components/Visualization/Canvas/flow.service.test.ts +++ b/packages/ui/src/components/Visualization/Canvas/flow.service.test.ts @@ -102,5 +102,23 @@ describe('FlowService', () => { expect(group.children).toEqual(['test|route.from', 'test|route.from.steps.0.placeholder']); expect(group.group).toBeTruthy(); }); + + it('should scope nodes & edges IDs', () => { + const routeNode = new CamelRouteVisualEntity({ + route: { id: 'route-8888', from: { uri: 'timer:clock', steps: [{ to: { uri: 'log' } }] } }, + }).toVizNode(); + + const { nodes, edges } = FlowService.getFlowDiagram('test', routeNode); + + expect(nodes).toHaveLength(3); + expect(nodes[0].id).toEqual('test|route.from'); + expect(nodes[1].id).toEqual('test|route.from.steps.0.to'); + expect(nodes[2].id).toEqual('test|route'); + + expect(edges).toHaveLength(1); + expect(edges[0].id).toEqual('test|route.from >>> route.from.steps.0.to'); + expect(edges[0].source).toEqual('test|route.from'); + expect(edges[0].target).toEqual('test|route.from.steps.0.to'); + }); }); }); diff --git a/packages/ui/src/components/Visualization/Canvas/flow.service.ts b/packages/ui/src/components/Visualization/Canvas/flow.service.ts index 5d954cfa1..5e156cbd3 100644 --- a/packages/ui/src/components/Visualization/Canvas/flow.service.ts +++ b/packages/ui/src/components/Visualization/Canvas/flow.service.ts @@ -21,6 +21,7 @@ export class FlowService { node.parentNode = node.parentNode ? `${scope}|${node.parentNode}` : undefined; }); this.edges.forEach((edge) => { + edge.id = `${scope}|${edge.id}`; edge.source = `${scope}|${edge.source}`; edge.target = `${scope}|${edge.target}`; }); diff --git a/packages/ui/src/tests/__snapshots__/nodes-edges.test.ts.snap b/packages/ui/src/tests/__snapshots__/nodes-edges.test.ts.snap index 0b7d46c35..edd73f015 100644 --- a/packages/ui/src/tests/__snapshots__/nodes-edges.test.ts.snap +++ b/packages/ui/src/tests/__snapshots__/nodes-edges.test.ts.snap @@ -4645,14 +4645,14 @@ exports[`Nodes and Edges should generate edges for steps with branches 2`] = ` [ { "edgeStyle": "solid", - "id": "route.from >>> route.from.steps.0.choice", + "id": "test|route.from >>> route.from.steps.0.choice", "source": "test|route.from", "target": "test|route.from.steps.0.choice", "type": "edge", }, { "edgeStyle": "solid", - "id": "route.from.steps.0.choice >>> route.from.steps.1.to", + "id": "test|route.from.steps.0.choice >>> route.from.steps.1.to", "source": "test|route.from.steps.0.choice", "target": "test|route.from.steps.1.to", "type": "edge",