Skip to content

Commit

Permalink
fix(Canvas): Duplicated edge IDs
Browse files Browse the repository at this point in the history 
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: #1897
  • Loading branch information
@lordrip
lordrip committed Jan 8, 2025
1 parent 8ce024a commit 4f65d05
Show file tree
Hide file tree
Showing 9 changed files with 49 additions and 28 deletions.
2 changes: 1 addition & 1 deletion packages/ui-tests/cypress/e2e/codeEditor/sourceCodeActions.cy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
4 changes: 3 additions & 1 deletion packages/ui-tests/cypress/e2e/designer/branchingFlows/branchingStepAddition.cy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
);
Expand All @@ -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',
);
Expand All @@ -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');
});
});
2 changes: 1 addition & 1 deletion packages/ui-tests/cypress/support/cypress.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ declare global {
selectRemoveGroup(groupName: string, nodeIndex?: number): Chainable<JQuery<Element>>;
performNodeAction(nodeName: string, action: ActionType, nodeIndex?: number): Chainable<JQuery<Element>>;
checkNodeExist(inputName: string, nodesCount: number): Chainable<JQuery<Element>>;
checkEdgeExists(sourceName: string, targetName: string): Chainable<JQuery<Element>>;
checkEdgeExists(scope: string, sourceName: string, targetName: string): Chainable<JQuery<Element>>;
deleteBranch(branchIndex: number): Chainable<JQuery<Element>>;
selectCamelRouteType(type: string, subType?: string): Chainable<JQuery<Element>>;
selectRuntimeVersion(type: string): Chainable<JQuery<Element>>;
Expand Down
4 changes: 2 additions & 2 deletions packages/ui-tests/cypress/support/next-commands/design.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
40 changes: 20 additions & 20 deletions packages/ui/src/components/Visualization/Canvas/__snapshots__/Canvas.test.tsx.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ exports[`Canvas Catalog button should NOT be present if \`CatalogModalContext\`
data-layer-id="default"
>
<g
data-id="route.from >>> route.from.steps.0.set-header"
data-id="route-8888|route.from >>> route.from.steps.0.set-header"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -210,7 +210,7 @@ exports[`Canvas Catalog button should NOT be present if \`CatalogModalContext\`
</g>
</g>
<g
data-id="route.from.steps.0.set-header >>> route.from.steps.1.choice"
data-id="route-8888|route.from.steps.0.set-header >>> route.from.steps.1.choice"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -279,7 +279,7 @@ exports[`Canvas Catalog button should NOT be present if \`CatalogModalContext\`
</g>
</g>
<g
data-id="route.from.steps.1.choice.otherwise.steps.0.to >>> route.from.steps.1.choice.otherwise.steps.1.to"
data-id="route-8888|route.from.steps.1.choice.otherwise.steps.0.to >>> route.from.steps.1.choice.otherwise.steps.1.to"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -348,7 +348,7 @@ exports[`Canvas Catalog button should NOT be present if \`CatalogModalContext\`
</g>
</g>
<g
data-id="route.from.steps.1.choice.otherwise.steps.1.to >>> route.from.steps.1.choice.otherwise.steps.2.log"
data-id="route-8888|route.from.steps.1.choice.otherwise.steps.1.to >>> route.from.steps.1.choice.otherwise.steps.2.log"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -417,7 +417,7 @@ exports[`Canvas Catalog button should NOT be present if \`CatalogModalContext\`
</g>
</g>
<g
data-id="route.from.steps.1.choice >>> route.from.steps.2.to"
data-id="route-8888|route.from.steps.1.choice >>> route.from.steps.2.to"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -925,7 +925,7 @@ exports[`Canvas Catalog button should be present if \`CatalogModalContext\` is p
data-layer-id="default"
>
<g
data-id="route.from >>> route.from.steps.0.set-header"
data-id="route-8888|route.from >>> route.from.steps.0.set-header"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -994,7 +994,7 @@ exports[`Canvas Catalog button should be present if \`CatalogModalContext\` is p
</g>
</g>
<g
data-id="route.from.steps.0.set-header >>> route.from.steps.1.choice"
data-id="route-8888|route.from.steps.0.set-header >>> route.from.steps.1.choice"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -1063,7 +1063,7 @@ exports[`Canvas Catalog button should be present if \`CatalogModalContext\` is p
</g>
</g>
<g
data-id="route.from.steps.1.choice.otherwise.steps.0.to >>> route.from.steps.1.choice.otherwise.steps.1.to"
data-id="route-8888|route.from.steps.1.choice.otherwise.steps.0.to >>> route.from.steps.1.choice.otherwise.steps.1.to"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -1132,7 +1132,7 @@ exports[`Canvas Catalog button should be present if \`CatalogModalContext\` is p
</g>
</g>
<g
data-id="route.from.steps.1.choice.otherwise.steps.1.to >>> route.from.steps.1.choice.otherwise.steps.2.log"
data-id="route-8888|route.from.steps.1.choice.otherwise.steps.1.to >>> route.from.steps.1.choice.otherwise.steps.2.log"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -1201,7 +1201,7 @@ exports[`Canvas Catalog button should be present if \`CatalogModalContext\` is p
</g>
</g>
<g
data-id="route.from.steps.1.choice >>> route.from.steps.2.to"
data-id="route-8888|route.from.steps.1.choice >>> route.from.steps.2.to"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -2521,7 +2521,7 @@ exports[`Canvas should render correctly 1`] = `
data-layer-id="default"
>
<g
data-id="route.from >>> route.from.steps.0.set-header"
data-id="route-8888|route.from >>> route.from.steps.0.set-header"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -2590,7 +2590,7 @@ exports[`Canvas should render correctly 1`] = `
</g>
</g>
<g
data-id="route.from.steps.0.set-header >>> route.from.steps.1.choice"
data-id="route-8888|route.from.steps.0.set-header >>> route.from.steps.1.choice"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -2659,7 +2659,7 @@ exports[`Canvas should render correctly 1`] = `
</g>
</g>
<g
data-id="route.from.steps.1.choice.otherwise.steps.0.to >>> route.from.steps.1.choice.otherwise.steps.1.to"
data-id="route-8888|route.from.steps.1.choice.otherwise.steps.0.to >>> route.from.steps.1.choice.otherwise.steps.1.to"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -2728,7 +2728,7 @@ exports[`Canvas should render correctly 1`] = `
</g>
</g>
<g
data-id="route.from.steps.1.choice.otherwise.steps.1.to >>> route.from.steps.1.choice.otherwise.steps.2.log"
data-id="route-8888|route.from.steps.1.choice.otherwise.steps.1.to >>> route.from.steps.1.choice.otherwise.steps.2.log"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -2797,7 +2797,7 @@ exports[`Canvas should render correctly 1`] = `
</g>
</g>
<g
data-id="route.from.steps.1.choice >>> route.from.steps.2.to"
data-id="route-8888|route.from.steps.1.choice >>> route.from.steps.2.to"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -3305,7 +3305,7 @@ exports[`Canvas should render correctly with more routes 1`] = `
data-layer-id="default"
>
<g
data-id="route.from >>> route.from.steps.0.set-header"
data-id="route-8888|route.from >>> route.from.steps.0.set-header"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -3374,7 +3374,7 @@ exports[`Canvas should render correctly with more routes 1`] = `
</g>
</g>
<g
data-id="route.from.steps.0.set-header >>> route.from.steps.1.choice"
data-id="route-8888|route.from.steps.0.set-header >>> route.from.steps.1.choice"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -3443,7 +3443,7 @@ exports[`Canvas should render correctly with more routes 1`] = `
</g>
</g>
<g
data-id="route.from.steps.1.choice.otherwise.steps.0.to >>> route.from.steps.1.choice.otherwise.steps.1.to"
data-id="route-8888|route.from.steps.1.choice.otherwise.steps.0.to >>> route.from.steps.1.choice.otherwise.steps.1.to"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -3512,7 +3512,7 @@ exports[`Canvas should render correctly with more routes 1`] = `
</g>
</g>
<g
data-id="route.from.steps.1.choice.otherwise.steps.1.to >>> route.from.steps.1.choice.otherwise.steps.2.log"
data-id="route-8888|route.from.steps.1.choice.otherwise.steps.1.to >>> route.from.steps.1.choice.otherwise.steps.2.log"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down Expand Up @@ -3581,7 +3581,7 @@ exports[`Canvas should render correctly with more routes 1`] = `
</g>
</g>
<g
data-id="route.from.steps.1.choice >>> route.from.steps.2.to"
data-id="route-8888|route.from.steps.1.choice >>> route.from.steps.2.to"
data-kind="edge"
data-type="edge"
style="z-index: 0;"
Expand Down
2 changes: 1 addition & 1 deletion packages/ui/src/components/Visualization/Canvas/__snapshots__/flow.service.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ exports[`FlowService getFlowDiagram should return nodes and edges for a multiple
[
{
"edgeStyle": "solid",
"id": "node >>> set-header",
"id": "test|node >>> set-header",
"source": "test|node",
"target": "test|set-header",
"type": "edge",
Expand Down
18 changes: 18 additions & 0 deletions packages/ui/src/components/Visualization/Canvas/flow.service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
});
1 change: 1 addition & 0 deletions packages/ui/src/components/Visualization/Canvas/flow.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}`;
});
Expand Down
4 changes: 2 additions & 2 deletions packages/ui/src/tests/__snapshots__/nodes-edges.test.ts.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit 4f65d05

Please sign in to comment.