From 29e5e664816f47feccab11cf9cf841afc61e5a0a Mon Sep 17 00:00:00 2001 From: Sidharth Vinod Date: Thu, 14 Sep 2023 15:20:21 +0530 Subject: [PATCH 1/5] refactor: Unify the edgeMarker adding logic --- .../src/dagre-wrapper/edgeMarker.spec.ts | 67 +++++++++++ .../mermaid/src/dagre-wrapper/edgeMarker.ts | 41 +++++++ packages/mermaid/src/dagre-wrapper/edges.js | 106 +----------------- .../flowchart/elk/flowRenderer-elk.js | 105 +---------------- 4 files changed, 115 insertions(+), 204 deletions(-) create mode 100644 packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts create mode 100644 packages/mermaid/src/dagre-wrapper/edgeMarker.ts diff --git a/packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts b/packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts new file mode 100644 index 0000000000..7a19c29518 --- /dev/null +++ b/packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts @@ -0,0 +1,67 @@ +import type { SVG } from '../diagram-api/types.js'; +import { addEdgeMarkers } from './edgeMarker.js'; + +describe('addEdgeMarker', () => { + const svgPath = { + attr: vitest.fn(), + } as unknown as SVG; + const url = 'http://example.com'; + const id = 'test'; + const diagramType = 'test'; + + it('should add markers for arrow_cross:arrow_point', () => { + const arrowTypeStart = 'arrow_cross'; + const arrowTypeEnd = 'arrow_point'; + addEdgeMarkers(svgPath, { arrowTypeStart, arrowTypeEnd }, url, id, diagramType); + expect(svgPath.attr).toHaveBeenCalledWith( + 'marker-start', + `url(${url}#${id}_${diagramType}-crossStart)` + ); + expect(svgPath.attr).toHaveBeenCalledWith( + 'marker-end', + `url(${url}#${id}_${diagramType}-pointEnd)` + ); + }); + + it('should add markers for aggregation:arrow_point', () => { + const arrowTypeStart = 'aggregation'; + const arrowTypeEnd = 'arrow_point'; + addEdgeMarkers(svgPath, { arrowTypeStart, arrowTypeEnd }, url, id, diagramType); + expect(svgPath.attr).toHaveBeenCalledWith( + 'marker-start', + `url(${url}#${id}_${diagramType}-aggregationStart)` + ); + expect(svgPath.attr).toHaveBeenCalledWith( + 'marker-end', + `url(${url}#${id}_${diagramType}-pointEnd)` + ); + }); + + it('should add markers for arrow_point:aggregation', () => { + const arrowTypeStart = 'arrow_point'; + const arrowTypeEnd = 'aggregation'; + addEdgeMarkers(svgPath, { arrowTypeStart, arrowTypeEnd }, url, id, diagramType); + expect(svgPath.attr).toHaveBeenCalledWith( + 'marker-start', + `url(${url}#${id}_${diagramType}-pointStart)` + ); + expect(svgPath.attr).toHaveBeenCalledWith( + 'marker-end', + `url(${url}#${id}_${diagramType}-aggregationEnd)` + ); + }); + + it('should add markers for aggregation:composition', () => { + const arrowTypeStart = 'aggregation'; + const arrowTypeEnd = 'composition'; + addEdgeMarkers(svgPath, { arrowTypeStart, arrowTypeEnd }, url, id, diagramType); + expect(svgPath.attr).toHaveBeenCalledWith( + 'marker-start', + `url(${url}#${id}_${diagramType}-aggregationStart)` + ); + expect(svgPath.attr).toHaveBeenCalledWith( + 'marker-end', + `url(${url}#${id}_${diagramType}-compositionEnd)` + ); + }); +}); diff --git a/packages/mermaid/src/dagre-wrapper/edgeMarker.ts b/packages/mermaid/src/dagre-wrapper/edgeMarker.ts new file mode 100644 index 0000000000..afce245d53 --- /dev/null +++ b/packages/mermaid/src/dagre-wrapper/edgeMarker.ts @@ -0,0 +1,41 @@ +import type { SVG } from '../diagram-api/types.js'; +import type { EdgeData } from '../types.js'; +/** + * Adds SVG markers to a path element based on the arrow types specified in the edge. + * + * @param svgPath - The SVG path element to add markers to. + * @param edge - The edge data object containing the arrow types. + * @param url - The URL of the SVG marker definitions. + * @param id - The ID prefix for the SVG marker definitions. + * @param diagramType - The type of diagram being rendered. + */ +export const addEdgeMarkers = ( + svgPath: SVG, + edge: Pick, + url: string, + id: string, + diagramType: string +) => { + if (edge.arrowTypeStart) { + addEdgeMarker(svgPath, 'start', edge.arrowTypeStart, url, id, diagramType); + } + if (edge.arrowTypeEnd) { + addEdgeMarker(svgPath, 'end', edge.arrowTypeEnd, url, id, diagramType); + } +}; + +const addEdgeMarker = ( + svgPath: SVG, + position: 'start' | 'end', + arrowType: string, + url: string, + id: string, + diagramType: string +) => { + if (arrowType.startsWith('arrow_')) { + arrowType = arrowType.replace('arrow_', ''); + } + + const suffix = position === 'start' ? 'Start' : 'End'; + svgPath.attr(`marker-${position}`, `url(${url}#${id}_${diagramType}-${arrowType}${suffix})`); +}; diff --git a/packages/mermaid/src/dagre-wrapper/edges.js b/packages/mermaid/src/dagre-wrapper/edges.js index 1b3e172c01..8ef30a5ada 100644 --- a/packages/mermaid/src/dagre-wrapper/edges.js +++ b/packages/mermaid/src/dagre-wrapper/edges.js @@ -6,6 +6,7 @@ import { getConfig } from '../config.js'; import utils from '../utils.js'; import { evaluate } from '../diagrams/common/common.js'; import { getLineFunctionsWithOffset } from '../utils/lineWithOffset.js'; +import { addEdgeMarker } from './edgeMarker.js'; let edgeLabels = {}; let terminalLabels = {}; @@ -506,108 +507,9 @@ export const insertEdge = function (elem, e, edge, clusterDb, diagramType, graph log.info('arrowTypeStart', edge.arrowTypeStart); log.info('arrowTypeEnd', edge.arrowTypeEnd); - switch (edge.arrowTypeStart) { - case 'arrow_cross': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-crossStart' + ')' - ); - break; - case 'arrow_point': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-pointStart' + ')' - ); - break; - case 'arrow_barb': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-barbStart' + ')' - ); - break; - case 'arrow_circle': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-circleStart' + ')' - ); - break; - case 'aggregation': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-aggregationStart' + ')' - ); - break; - case 'extension': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-extensionStart' + ')' - ); - break; - case 'composition': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-compositionStart' + ')' - ); - break; - case 'dependency': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-dependencyStart' + ')' - ); - break; - case 'lollipop': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-lollipopStart' + ')' - ); - break; - default: - } - switch (edge.arrowTypeEnd) { - case 'arrow_cross': - svgPath.attr('marker-end', 'url(' + url + '#' + id + '_' + diagramType + '-crossEnd' + ')'); - break; - case 'arrow_point': - svgPath.attr('marker-end', 'url(' + url + '#' + id + '_' + diagramType + '-pointEnd' + ')'); - break; - case 'arrow_barb': - svgPath.attr('marker-end', 'url(' + url + '#' + id + '_' + diagramType + '-barbEnd' + ')'); - break; - case 'arrow_circle': - svgPath.attr('marker-end', 'url(' + url + '#' + id + '_' + diagramType + '-circleEnd' + ')'); - break; - case 'aggregation': - svgPath.attr( - 'marker-end', - 'url(' + url + '#' + id + '_' + diagramType + '-aggregationEnd' + ')' - ); - break; - case 'extension': - svgPath.attr( - 'marker-end', - 'url(' + url + '#' + id + '_' + diagramType + '-extensionEnd' + ')' - ); - break; - case 'composition': - svgPath.attr( - 'marker-end', - 'url(' + url + '#' + id + '_' + diagramType + '-compositionEnd' + ')' - ); - break; - case 'dependency': - svgPath.attr( - 'marker-end', - 'url(' + url + '#' + id + '_' + diagramType + '-dependencyEnd' + ')' - ); - break; - case 'lollipop': - svgPath.attr( - 'marker-end', - 'url(' + url + '#' + id + '_' + diagramType + '-lollipopEnd' + ')' - ); - break; - default: - } + addEdgeMarker(svgPath, 'start', edge.arrowTypeStart, url, id, diagramType); + addEdgeMarker(svgPath, 'end', edge.arrowTypeEnd, url, id, diagramType); + let paths = {}; if (pointsHasChanged) { paths.updatedPath = points; diff --git a/packages/mermaid/src/diagrams/flowchart/elk/flowRenderer-elk.js b/packages/mermaid/src/diagrams/flowchart/elk/flowRenderer-elk.js index 737b492fb3..56fad15492 100644 --- a/packages/mermaid/src/diagrams/flowchart/elk/flowRenderer-elk.js +++ b/packages/mermaid/src/diagrams/flowchart/elk/flowRenderer-elk.js @@ -11,6 +11,7 @@ import common from '../../common/common.js'; import { interpolateToCurve, getStylesFromArray } from '../../../utils.js'; import ELK from 'elkjs/lib/elk.bundled.js'; import { getLineFunctionsWithOffset } from '../../../utils/lineWithOffset.js'; +import { addEdgeMarker } from '../../../dagre-wrapper/edgeMarker.js'; const elk = new ELK(); @@ -586,108 +587,8 @@ const addMarkersToEdge = function (svgPath, edgeData, diagramType, arrowMarkerAb } // look in edge data and decide which marker to use - switch (edgeData.arrowTypeStart) { - case 'arrow_cross': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-crossStart' + ')' - ); - break; - case 'arrow_point': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-pointStart' + ')' - ); - break; - case 'arrow_barb': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-barbStart' + ')' - ); - break; - case 'arrow_circle': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-circleStart' + ')' - ); - break; - case 'aggregation': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-aggregationStart' + ')' - ); - break; - case 'extension': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-extensionStart' + ')' - ); - break; - case 'composition': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-compositionStart' + ')' - ); - break; - case 'dependency': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-dependencyStart' + ')' - ); - break; - case 'lollipop': - svgPath.attr( - 'marker-start', - 'url(' + url + '#' + id + '_' + diagramType + '-lollipopStart' + ')' - ); - break; - default: - } - switch (edgeData.arrowTypeEnd) { - case 'arrow_cross': - svgPath.attr('marker-end', 'url(' + url + '#' + id + '_' + diagramType + '-crossEnd' + ')'); - break; - case 'arrow_point': - svgPath.attr('marker-end', 'url(' + url + '#' + id + '_' + diagramType + '-pointEnd' + ')'); - break; - case 'arrow_barb': - svgPath.attr('marker-end', 'url(' + url + '#' + id + '_' + diagramType + '-barbEnd' + ')'); - break; - case 'arrow_circle': - svgPath.attr('marker-end', 'url(' + url + '#' + id + '_' + diagramType + '-circleEnd' + ')'); - break; - case 'aggregation': - svgPath.attr( - 'marker-end', - 'url(' + url + '#' + id + '_' + diagramType + '-aggregationEnd' + ')' - ); - break; - case 'extension': - svgPath.attr( - 'marker-end', - 'url(' + url + '#' + id + '_' + diagramType + '-extensionEnd' + ')' - ); - break; - case 'composition': - svgPath.attr( - 'marker-end', - 'url(' + url + '#' + id + '_' + diagramType + '-compositionEnd' + ')' - ); - break; - case 'dependency': - svgPath.attr( - 'marker-end', - 'url(' + url + '#' + id + '_' + diagramType + '-dependencyEnd' + ')' - ); - break; - case 'lollipop': - svgPath.attr( - 'marker-end', - 'url(' + url + '#' + id + '_' + diagramType + '-lollipopEnd' + ')' - ); - break; - default: - } + addEdgeMarker(svgPath, 'start', edgeData.arrowTypeStart, url, id, diagramType); + addEdgeMarker(svgPath, 'end', edgeData.arrowTypeEnd, url, id, diagramType); }; /** From faa1fda7ba13f8571348f5bb60c812e3cda89f48 Mon Sep 17 00:00:00 2001 From: Sidharth Vinod Date: Thu, 14 Sep 2023 15:25:52 +0530 Subject: [PATCH 2/5] refactor: Unify the edgeMarker adding logic --- packages/mermaid/src/dagre-wrapper/edges.js | 5 ++--- .../mermaid/src/diagrams/flowchart/elk/flowRenderer-elk.js | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/mermaid/src/dagre-wrapper/edges.js b/packages/mermaid/src/dagre-wrapper/edges.js index 8ef30a5ada..3f4412fb73 100644 --- a/packages/mermaid/src/dagre-wrapper/edges.js +++ b/packages/mermaid/src/dagre-wrapper/edges.js @@ -6,7 +6,7 @@ import { getConfig } from '../config.js'; import utils from '../utils.js'; import { evaluate } from '../diagrams/common/common.js'; import { getLineFunctionsWithOffset } from '../utils/lineWithOffset.js'; -import { addEdgeMarker } from './edgeMarker.js'; +import { addEdgeMarkers } from './edgeMarker.js'; let edgeLabels = {}; let terminalLabels = {}; @@ -507,8 +507,7 @@ export const insertEdge = function (elem, e, edge, clusterDb, diagramType, graph log.info('arrowTypeStart', edge.arrowTypeStart); log.info('arrowTypeEnd', edge.arrowTypeEnd); - addEdgeMarker(svgPath, 'start', edge.arrowTypeStart, url, id, diagramType); - addEdgeMarker(svgPath, 'end', edge.arrowTypeEnd, url, id, diagramType); + addEdgeMarkers(svgPath, edge, url, id, diagramType); let paths = {}; if (pointsHasChanged) { diff --git a/packages/mermaid/src/diagrams/flowchart/elk/flowRenderer-elk.js b/packages/mermaid/src/diagrams/flowchart/elk/flowRenderer-elk.js index 56fad15492..844264a5fc 100644 --- a/packages/mermaid/src/diagrams/flowchart/elk/flowRenderer-elk.js +++ b/packages/mermaid/src/diagrams/flowchart/elk/flowRenderer-elk.js @@ -11,7 +11,7 @@ import common from '../../common/common.js'; import { interpolateToCurve, getStylesFromArray } from '../../../utils.js'; import ELK from 'elkjs/lib/elk.bundled.js'; import { getLineFunctionsWithOffset } from '../../../utils/lineWithOffset.js'; -import { addEdgeMarker } from '../../../dagre-wrapper/edgeMarker.js'; +import { addEdgeMarkers } from '../../../dagre-wrapper/edgeMarker.js'; const elk = new ELK(); @@ -587,8 +587,7 @@ const addMarkersToEdge = function (svgPath, edgeData, diagramType, arrowMarkerAb } // look in edge data and decide which marker to use - addEdgeMarker(svgPath, 'start', edgeData.arrowTypeStart, url, id, diagramType); - addEdgeMarker(svgPath, 'end', edgeData.arrowTypeEnd, url, id, diagramType); + addEdgeMarkers(svgPath, edgeData, url, id, diagramType); }; /** From 4952b13ad07ca832e63d4a2c9867d62e7de75c73 Mon Sep 17 00:00:00 2001 From: Sidharth Vinod Date: Thu, 9 Nov 2023 02:37:38 +0530 Subject: [PATCH 3/5] fix: Ignore unknown arrow type values Co-authored-by: Alois Klink --- .../mermaid/src/dagre-wrapper/edgeMarker.ts | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/packages/mermaid/src/dagre-wrapper/edgeMarker.ts b/packages/mermaid/src/dagre-wrapper/edgeMarker.ts index afce245d53..778a7708d8 100644 --- a/packages/mermaid/src/dagre-wrapper/edgeMarker.ts +++ b/packages/mermaid/src/dagre-wrapper/edgeMarker.ts @@ -1,4 +1,5 @@ import type { SVG } from '../diagram-api/types.js'; +import { log } from '../logger.js'; import type { EdgeData } from '../types.js'; /** * Adds SVG markers to a path element based on the arrow types specified in the edge. @@ -24,6 +25,18 @@ export const addEdgeMarkers = ( } }; +const arrowTypesMap = { + arrow_cross: 'cross', + arrow_point: 'point', + arrow_barb: 'barb', + arrow_circle: 'circle', + aggregation: 'aggregation', + extension: 'extension', + composition: 'composition', + dependency: 'dependency', + lollipop: 'lollipop', +} as const; + const addEdgeMarker = ( svgPath: SVG, position: 'start' | 'end', @@ -32,10 +45,13 @@ const addEdgeMarker = ( id: string, diagramType: string ) => { - if (arrowType.startsWith('arrow_')) { - arrowType = arrowType.replace('arrow_', ''); + const endMarkerType = arrowTypesMap[arrowType as keyof typeof arrowTypesMap]; + + if (!endMarkerType) { + log.warn(`Unknown arrow type: ${arrowType}`); + return; // unknown arrow type, ignore } const suffix = position === 'start' ? 'Start' : 'End'; - svgPath.attr(`marker-${position}`, `url(${url}#${id}_${diagramType}-${arrowType}${suffix})`); + svgPath.attr(`marker-${position}`, `url(${url}#${id}_${diagramType}-${endMarkerType}${suffix})`); }; From 8f572021aff0572e34f3cf4b66697d5515b4e4d9 Mon Sep 17 00:00:00 2001 From: Sidharth Vinod Date: Thu, 9 Nov 2023 02:38:48 +0530 Subject: [PATCH 4/5] chore: Add test for invalid marker --- packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts b/packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts index 7a19c29518..658e6d4f43 100644 --- a/packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts +++ b/packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts @@ -64,4 +64,11 @@ describe('addEdgeMarker', () => { `url(${url}#${id}_${diagramType}-compositionEnd)` ); }); + + it('should not add invalid markers', () => { + const arrowTypeStart = 'this is an invalid marker'; + const arrowTypeEnd = ') url(https://my-malicious-site.example)'; + addEdgeMarkers(svgPath, { arrowTypeStart, arrowTypeEnd }, url, id, diagramType); + expect(svgPath.attr).not.toHaveBeenCalled(); + }); }); From 09d9c31d53978e622540dd10ba6bc7869c01e42b Mon Sep 17 00:00:00 2001 From: Sidharth Vinod Date: Thu, 9 Nov 2023 11:36:45 +0530 Subject: [PATCH 5/5] chore: Reset mock in edgeMarker test --- packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts b/packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts index 658e6d4f43..6cfb59fab9 100644 --- a/packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts +++ b/packages/mermaid/src/dagre-wrapper/edgeMarker.spec.ts @@ -1,14 +1,19 @@ +import type { Mocked } from 'vitest'; import type { SVG } from '../diagram-api/types.js'; import { addEdgeMarkers } from './edgeMarker.js'; describe('addEdgeMarker', () => { const svgPath = { attr: vitest.fn(), - } as unknown as SVG; + } as unknown as Mocked; const url = 'http://example.com'; const id = 'test'; const diagramType = 'test'; + beforeEach(() => { + svgPath.attr.mockReset(); + }); + it('should add markers for arrow_cross:arrow_point', () => { const arrowTypeStart = 'arrow_cross'; const arrowTypeEnd = 'arrow_point';