From a3d7890878bd688073a93ce20237cd538625a1df Mon Sep 17 00:00:00 2001 From: Thomas Bouffard <27200110+tbouffard@users.noreply.github.com> Date: Mon, 15 Feb 2021 20:30:59 +0100 Subject: [PATCH] [FIX] Don't store 'edge without bpmn element' in the BpmnModel This is invalid and was not homogeneous with BPMN Shape that are not stored when we don't get the bpmn element. In addition, this can produced errors in client code as the bpmn element is not supposed to be `undefined`. A dedicated test has been added to manage this use case and parsing tests that were relying on this 'hole' have been updated to use valid edge BPMN sources (mainly tests about edge labels). --- .../parser/json/converter/DiagramConverter.ts | 35 ++++++---- .../parser/json/BpmnJsonParser.edge.test.ts | 49 +++++++++++++ .../json/BpmnJsonParser.label.bounds.test.ts | 18 ++++- .../json/BpmnJsonParser.label.font.test.ts | 69 ++++++++++++++++--- .../parser/json/BpmnJsonParser.label.test.ts | 27 +++++++- 5 files changed, 171 insertions(+), 27 deletions(-) create mode 100644 test/unit/component/parser/json/BpmnJsonParser.edge.test.ts diff --git a/src/component/parser/json/converter/DiagramConverter.ts b/src/component/parser/json/converter/DiagramConverter.ts index 280b54f8e1..27b9c6619e 100644 --- a/src/component/parser/json/converter/DiagramConverter.ts +++ b/src/component/parser/json/converter/DiagramConverter.ts @@ -141,19 +141,28 @@ export default class DiagramConverter { } private deserializeEdges(edges: BPMNEdge | BPMNEdge[]): Edge[] { - return ensureIsArray(edges).map(edge => { - const flow = - this.convertedElements.findSequenceFlow(edge.bpmnElement) || - this.convertedElements.findMessageFlow(edge.bpmnElement) || - this.convertedElements.findAssociationFlow(edge.bpmnElement); - const waypoints = this.deserializeWaypoints(edge.waypoint); - const label = this.deserializeLabel(edge.BPMNLabel, edge.id); - - // TODO Remove messageVisibleKind conversion type when we merge/simplify internal model with BPMN json model - const messageVisibleKind = edge.messageVisibleKind ? ((edge.messageVisibleKind as unknown) as MessageVisibleKind) : MessageVisibleKind.NONE; - - return new Edge(edge.id, flow, waypoints, label, messageVisibleKind); - }); + return ensureIsArray(edges) + .map(edge => { + const flow = + this.convertedElements.findSequenceFlow(edge.bpmnElement) || + this.convertedElements.findMessageFlow(edge.bpmnElement) || + this.convertedElements.findAssociationFlow(edge.bpmnElement); + + if (!flow) { + // TODO error management + console.warn('Edge json deserialization: unable to find bpmn element with id %s', edge.bpmnElement); + return; + } + + const waypoints = this.deserializeWaypoints(edge.waypoint); + const label = this.deserializeLabel(edge.BPMNLabel, edge.id); + + // TODO Remove messageVisibleKind conversion type when we merge/simplify internal model with BPMN json model + const messageVisibleKind = edge.messageVisibleKind ? ((edge.messageVisibleKind as unknown) as MessageVisibleKind) : MessageVisibleKind.NONE; + + return new Edge(edge.id, flow, waypoints, label, messageVisibleKind); + }) + .filter(edge => edge); } private deserializeWaypoints(waypoints: Point[]): Waypoint[] { diff --git a/test/unit/component/parser/json/BpmnJsonParser.edge.test.ts b/test/unit/component/parser/json/BpmnJsonParser.edge.test.ts new file mode 100644 index 0000000000..5932c18841 --- /dev/null +++ b/test/unit/component/parser/json/BpmnJsonParser.edge.test.ts @@ -0,0 +1,49 @@ +/** + * Copyright 2021 Bonitasoft S.A. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { parseJsonAndExpectOnlyEdges } from './JsonTestUtils'; + +describe('parse bpmn as json for edges', () => { + jest.spyOn(console, 'warn'); + + afterEach(() => { + jest.clearAllMocks(); + }); + + // this also covers unsupported bpmn element types that are then not retrieved during the parsing + it('should not convert as Edge without related BPMN element', () => { + console.warn = jest.fn(); + const json = { + definitions: { + targetNamespace: '', + process: '', + BPMNDiagram: { + id: 'BpmnDiagram_1', + BPMNPlane: { + id: 'BpmnPlane_1', + BPMNEdge: { + id: 'BPMNEdge_id_0', + bpmnElement: 'edge-bpmnElement-unknown', + waypoint: [{ x: 10, y: 10 }], + }, + }, + }, + }, + }; + + parseJsonAndExpectOnlyEdges(json, 0); + expect(console.warn).toHaveBeenCalledWith('Edge json deserialization: unable to find bpmn element with id %s', 'edge-bpmnElement-unknown'); + }); +}); diff --git a/test/unit/component/parser/json/BpmnJsonParser.label.bounds.test.ts b/test/unit/component/parser/json/BpmnJsonParser.label.bounds.test.ts index 074fc7fa5e..fcd92eea23 100644 --- a/test/unit/component/parser/json/BpmnJsonParser.label.bounds.test.ts +++ b/test/unit/component/parser/json/BpmnJsonParser.label.bounds.test.ts @@ -100,13 +100,20 @@ describe('parse bpmn as json for label bounds', () => { const json = { definitions: { targetNamespace: '', - process: '', + process: { + sequenceFlow: { + id: 'sequenceFlow_id_0', + sourceRef: 'sourceRef_0', + targetRef: 'targetRef_0', + }, + }, BPMNDiagram: { id: 'BpmnDiagram_1', BPMNPlane: { id: 'BpmnPlane_1', BPMNEdge: { id: 'BPMNEdge_id_0', + bpmnElement: 'sequenceFlow_id_0', waypoint: [{ x: 10, y: 10 }], BPMNLabel: { id: 'label_id', @@ -127,13 +134,20 @@ describe('parse bpmn as json for label bounds', () => { const json = { definitions: { targetNamespace: '', - process: '', + process: { + sequenceFlow: { + id: 'sequenceFlow_id_0', + sourceRef: 'sourceRef_0', + targetRef: 'targetRef_0', + }, + }, BPMNDiagram: { id: 'BpmnDiagram_1', BPMNPlane: { id: 'BpmnPlane_1', BPMNEdge: { id: 'BPMNEdge_id_0', + bpmnElement: 'sequenceFlow_id_0', waypoint: [{ x: 10, y: 10 }], BPMNLabel: { id: 'label_id', diff --git a/test/unit/component/parser/json/BpmnJsonParser.label.font.test.ts b/test/unit/component/parser/json/BpmnJsonParser.label.font.test.ts index 347ea407bf..8651d7a745 100644 --- a/test/unit/component/parser/json/BpmnJsonParser.label.font.test.ts +++ b/test/unit/component/parser/json/BpmnJsonParser.label.font.test.ts @@ -82,17 +82,24 @@ describe('parse bpmn as json for label font', () => { }, ); - it("should convert as Edge with Font, when a BPMNEdge (who references a BPMNLabelStyle with font) is an attribute (as object) of 'BPMNPlane' (as object)", () => { + it("should convert as Edge with Font, when a BPMNEdge (which references a BPMNLabelStyle with font) is an attribute (as object) of 'BPMNPlane' (as object)", () => { const json = { definitions: { targetNamespace: '', - process: '', + process: { + sequenceFlow: { + id: 'sequenceFlow_id_0', + sourceRef: 'sourceRef', + targetRef: 'targetRef', + }, + }, BPMNDiagram: { id: 'BpmnDiagram_1', BPMNPlane: { id: 'BpmnPlane_1', BPMNEdge: { id: 'BPMNEdge_id_0', + bpmnElement: 'sequenceFlow_id_0', waypoint: [{ x: 10, y: 10 }], BPMNLabel: { id: 'label_id', @@ -174,7 +181,20 @@ describe('parse bpmn as json for label font', () => { const json = { definitions: { targetNamespace: '', - process: '', + process: { + sequenceFlow: [ + { + id: 'sequenceFlow_id_0', + sourceRef: 'sourceRef_0', + targetRef: 'targetRef_0', + }, + { + id: 'sequenceFlow_id_1', + sourceRef: 'sourceRef_1', + targetRef: 'targetRef_1', + }, + ], + }, BPMNDiagram: { id: 'BpmnDiagram_1', BPMNPlane: { @@ -182,6 +202,7 @@ describe('parse bpmn as json for label font', () => { BPMNEdge: [ { id: 'BPMNEdge_id_0', + bpmnElement: 'sequenceFlow_id_0', waypoint: [{ x: 10, y: 10 }], BPMNLabel: { labelStyle: 'style_id_1', @@ -189,6 +210,7 @@ describe('parse bpmn as json for label font', () => { }, { id: 'BPMNEdge_id_1', + bpmnElement: 'sequenceFlow_id_1', waypoint: [{ x: 10, y: 10 }], BPMNLabel: { labelStyle: 'style_id_1', @@ -280,11 +302,24 @@ describe('parse bpmn as json for label font', () => { expect(model.flowNodes[1].label).toBeUndefined(); }); - it("should convert as Edge[] without Font, when BPMNEdges (who reference a BPMNLabelStyle) are an attribute (as array) of 'BPMNPlane' (as object) & BPMNLabelStyle (with font with/without all attributes) is an attribute (as array) of 'BPMNDiagram' (as object)", () => { + it("should convert as Edge[] without Font, when BPMNEdges (which reference a BPMNLabelStyle) are an attribute (as array) of 'BPMNPlane' (as object) & BPMNLabelStyle (with font with/without all attributes) is an attribute (as array) of 'BPMNDiagram' (as object)", () => { const json = { definitions: { targetNamespace: '', - process: '', + process: { + sequenceFlow: [ + { + id: 'sequenceFlow_id_0', + sourceRef: 'sourceRef_0', + targetRef: 'targetRef_0', + }, + { + id: 'sequenceFlow_id_1', + sourceRef: 'sourceRef_1', + targetRef: 'targetRef_1', + }, + ], + }, BPMNDiagram: { id: 'BpmnDiagram_1', BPMNPlane: { @@ -292,6 +327,7 @@ describe('parse bpmn as json for label font', () => { BPMNEdge: [ { id: 'BPMNEdge_id_0', + bpmnElement: 'sequenceFlow_id_0', waypoint: [{ x: 10, y: 10 }], BPMNLabel: { id: 'label_id_1', @@ -300,6 +336,7 @@ describe('parse bpmn as json for label font', () => { }, { id: 'BPMNEdge_id_1', + bpmnElement: 'sequenceFlow_id_1', waypoint: [{ x: 10, y: 10 }], BPMNLabel: { id: 'label_id_2', @@ -372,17 +409,24 @@ describe('parse bpmn as json for label font', () => { expect(model.flowNodes[0].label).toBeUndefined(); }); - it("should convert as Edge without Font, when a BPMNEdge (who references a BPMNLabelStyle without font) is an attribute (as object) of 'BPMNPlane' (as object)", () => { + it("should convert as Edge without Font, when a BPMNEdge (which references a BPMNLabelStyle without font) is an attribute (as object) of 'BPMNPlane' (as object)", () => { const json = { definitions: { targetNamespace: '', - process: '', + process: { + sequenceFlow: { + id: 'sequenceFlow_id_0', + sourceRef: 'sourceRef_0', + targetRef: 'targetRef_0', + }, + }, BPMNDiagram: { id: 'BpmnDiagram_1', BPMNPlane: { id: 'BpmnPlane_1', BPMNEdge: { id: 'BPMNEdge_id_0', + bpmnElement: 'sequenceFlow_id_0', waypoint: [{ x: 10, y: 10 }], BPMNLabel: { id: 'label_id', @@ -437,18 +481,25 @@ describe('parse bpmn as json for label font', () => { expect(console.warn).toHaveBeenCalledWith('Unable to assign font from style %s to shape/edge %s', 'non-existing_style_id', 'BPMNShape_id_0'); }); - it("should convert as Edge without Font, when a BPMNEdge (who references a non-existing BPMNLabelStyle) is an attribute (as object) of 'BPMNPlane' (as object)", () => { + it("should convert as Edge without Font, when a BPMNEdge (which references a non-existing BPMNLabelStyle) is an attribute (as object) of 'BPMNPlane' (as object)", () => { console.warn = jest.fn(); const json = { definitions: { targetNamespace: '', - process: '', + process: { + sequenceFlow: { + id: 'sequenceFlow_id_0', + sourceRef: 'sourceRef_0', + targetRef: 'targetRef_0', + }, + }, BPMNDiagram: { id: 'BpmnDiagram_1', BPMNPlane: { id: 'BpmnPlane_1', BPMNEdge: { id: 'BPMNEdge_id_0', + bpmnElement: 'sequenceFlow_id_0', waypoint: [{ x: 10, y: 10 }], BPMNLabel: { id: 'label_id', diff --git a/test/unit/component/parser/json/BpmnJsonParser.label.test.ts b/test/unit/component/parser/json/BpmnJsonParser.label.test.ts index 027b301a57..88d3e534ac 100644 --- a/test/unit/component/parser/json/BpmnJsonParser.label.test.ts +++ b/test/unit/component/parser/json/BpmnJsonParser.label.test.ts @@ -67,13 +67,20 @@ describe('parse bpmn as json for label font', () => { const json = { definitions: { targetNamespace: '', - process: '', + process: { + sequenceFlow: { + id: 'sequenceFlow_id_0', + sourceRef: 'sourceRef_0', + targetRef: 'targetRef_0', + }, + }, BPMNDiagram: { id: 'BpmnDiagram_1', BPMNPlane: { id: 'BpmnPlane_1', BPMNEdge: { id: 'BPMNEdge_id_0', + bpmnElement: 'sequenceFlow_id_0', waypoint: [{ x: 10, y: 10 }], BPMNLabel: '', }, @@ -123,13 +130,20 @@ describe('parse bpmn as json for label font', () => { const json = { definitions: { targetNamespace: '', - process: '', + process: { + sequenceFlow: { + id: 'sequenceFlow_id_0', + sourceRef: 'sourceRef_0', + targetRef: 'targetRef_0', + }, + }, BPMNDiagram: { id: 'BpmnDiagram_1', BPMNPlane: { id: 'BpmnPlane_1', BPMNEdge: { id: 'BPMNEdge_id_0', + bpmnElement: 'sequenceFlow_id_0', waypoint: [{ x: 10, y: 10 }], BPMNLabel: { id: '', @@ -178,13 +192,20 @@ describe('parse bpmn as json for label font', () => { const json = { definitions: { targetNamespace: '', - process: '', + process: { + sequenceFlow: { + id: 'sequenceFlow_id_0', + sourceRef: 'sourceRef_0', + targetRef: 'targetRef_0', + }, + }, BPMNDiagram: { id: 'BpmnDiagram_1', BPMNPlane: { id: 'BpmnPlane_1', BPMNEdge: { id: 'BPMNEdge_id_0', + bpmnElement: 'sequenceFlow_id_0', waypoint: [{ x: 10, y: 10 }], }, },