Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: remove extra parsing of lane elements which are not in the BPMN spec #2805

Merged
merged 5 commits into from
Aug 24, 2023
Merged

fix: remove extra parsing of lane elements which are not in the BPMN spec #2805

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
98b5a06
- remove parsing of process[ShapeBpmnElementKind.LANE]
csouchet Aug 21, 2023
ee7e9ca
Fix other tests & code
csouchet Aug 21, 2023
b1e2366
Merge branch 'master' into 2188-Remove_extra_parsing_of_lane_elements
csouchet Aug 22, 2023
96d31d7
rename getBpmnElementNewValue function and simplify the reading of th…
csouchet Aug 23, 2023
1ed1609
Merge branch 'master' into 2188-Remove_extra_parsing_of_lane_elements
csouchet Aug 24, 2023
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
6 changes: 4 additions & 2 deletions src/component/parser/json/converter/EventDefinitionConverter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import type { TEventDefinition } from '../../../../model/bpmn/json/baseElement/r
import type { ConvertedElements } from './utils';
import { ensureIsArray } from '../../../helpers/array-utils';

type EventDefinitions = string | TEventDefinition | (string | TEventDefinition)[];

/**
* @internal
*/
Expand All @@ -28,8 +30,8 @@ export default class EventDefinitionConverter {

deserialize(definitions: TDefinitions): void {
eventDefinitionKinds.forEach(eventDefinitionKind => {
// sometimes eventDefinition is simple and therefore it is parsed as empty string "", in that case eventDefinition will be converted to an empty object
const eventDefinitions: string | TEventDefinition | (string | TEventDefinition)[] = definitions[eventDefinitionKind + 'EventDefinition'];
// sometimes eventDefinition is simple, and therefore it is parsed as empty string "", in that case eventDefinition will be converted to an empty object
const eventDefinitions = definitions[(eventDefinitionKind + 'EventDefinition') as keyof TDefinitions] as EventDefinitions;
Copy link
Member

Choose a reason for hiding this comment

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

question: It is fine to introduce the new type as the code is now easier to read IMHO.
However, I don't understand why switching the syntax to "as"? I suggest to keep the existing it and to set an eslint rules to enforce this as part of #2742.

These are clearly extra changes. During my review, I had to wonder if this was part of the correction. So I guess if I or anyone else reads this in the future, the same question will arise.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do it, because is not working with the last syntax due to the remove of [key:string]:any 🙂

ensureIsArray<TEventDefinition>(eventDefinitions, true).forEach(eventDefinition =>
this.convertedElements.registerEventDefinitionsOfDefinition(eventDefinition.id, eventDefinitionKind),
);
Expand Down
5 changes: 2 additions & 3 deletions src/component/parser/json/converter/ProcessConverter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ const computeSubProcessKind = (processedSemanticType: BpmnSemanticType, bpmnElem
}
};

const orderedFlowNodeBpmnTypes: BpmnSemanticType[] = ['adHocSubProcess', 'transaction'] // specific management for adhoc and transaction sub-processes which are handled with a dedicated ShapeBpmnSubProcessKind
const orderedFlowNodeBpmnTypes: BpmnSemanticType[] = (['adHocSubProcess', 'transaction'] as BpmnSemanticType[]) // specific management for adhoc and transaction sub-processes which are handled with a dedicated ShapeBpmnSubProcessKind
Copy link
Member

Choose a reason for hiding this comment

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

question: Same question as in EventDefinitionConverter.ts about switching the syntax to "as".
We already have this discussion when I create the PR about this code, and we concluded that this was extra typing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same answer, TypeScript send an error due to the removing of [key:string]:any 🙂

// process boundary events afterward as we need its parent activity to be available when building it
.concat(ShapeUtil.flowNodeKinds().filter(kind => kind != ShapeBpmnElementKind.EVENT_BOUNDARY))
.concat(ShapeUtil.flowNodeKinds().filter(kind => kind != ShapeBpmnElementKind.EVENT_BOUNDARY) as BpmnSemanticType[])
.concat([ShapeBpmnElementKind.EVENT_BOUNDARY]);

function getShapeBpmnElementKind(bpmnSemanticType: BpmnSemanticType): ShapeBpmnElementKind {
Expand Down Expand Up @@ -152,7 +152,6 @@ export default class ProcessConverter {
// flow nodes
orderedFlowNodeBpmnTypes.forEach(bpmnType => this.buildFlowNodeBpmnElements(process[bpmnType], getShapeBpmnElementKind(bpmnType), parentId, process.id, bpmnType));
// containers
this.buildLaneBpmnElements(process[ShapeBpmnElementKind.LANE], parentId, process.id);
this.buildLaneSetBpmnElements(process.laneSet, parentId, process.id);

// flows
Expand Down
3 changes: 0 additions & 3 deletions src/model/bpmn/json/BPMN20.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,6 @@ export interface TDefinitions {
typeLanguage?: string; // default="http://www.w3.org/2001/XMLSchema"
exporter?: string;
exporterVersion?: string;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
[key: string]: any;
}

export interface TImport {
Expand Down
3 changes: 0 additions & 3 deletions src/model/bpmn/json/baseElement/rootElement/rootElement.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,6 @@ export interface TProcess extends TCallableElement {
isClosed?: boolean; // default="false"
isExecutable?: boolean;
definitionalCollaborationRef?: string;

// eslint-disable-next-line @typescript-eslint/no-explicit-any
[key: string]: any;
}

export enum tProcessType {
Expand Down
2 changes: 1 addition & 1 deletion test/unit/component/parser/json/BpmnJsonParser.flowNode.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe.each([
['parallelGateway', ShapeBpmnElementKind.GATEWAY_PARALLEL],
['eventBasedGateway', ShapeBpmnElementKind.GATEWAY_EVENT_BASED],
['complexGateway', ShapeBpmnElementKind.GATEWAY_COMPLEX],
])('parse bpmn as json for %s', (bpmnKind: string, expectedShapeBpmnElementKind: ShapeBpmnElementKind) => {
] as [keyof TProcess, ShapeBpmnElementKind][])('parse bpmn as json for %s', (bpmnKind: keyof TProcess, expectedShapeBpmnElementKind: ShapeBpmnElementKind) => {
const processWithFlowNodeAsObject: TProcess = {
[bpmnKind]: {
id: `${bpmnKind}_id_0`,
Expand Down
12 changes: 5 additions & 7 deletions test/unit/component/parser/json/BpmnJsonParser.lane.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,7 @@ describe('parse bpmn as json for lane', () => {
const json: BpmnJsonModel = {
definitions: {
targetNamespace: '',
process: {
lane: { id: 'Lane_12u5n6x' },
},
process: { laneSet: { lane: { id: 'Lane_12u5n6x' } } },
BPMNDiagram: {
BPMNPlane: {
BPMNShape: {
Expand Down Expand Up @@ -64,7 +62,7 @@ describe('parse bpmn as json for lane', () => {
definitions: {
targetNamespace: '',
process: {
lane: { id: 'Lane_12u5n6x', flowNodeRef: 'event_id_0' },
laneSet: { lane: { id: 'Lane_12u5n6x', flowNodeRef: 'event_id_0' } },
startEvent: { id: 'event_id_0' },
},
BPMNDiagram: {
Expand Down Expand Up @@ -113,7 +111,7 @@ describe('parse bpmn as json for lane', () => {
definitions: {
targetNamespace: '',
process: {
lane: { id: 'Lane_12u5n6x', flowNodeRef: 'event_id_0' },
laneSet: { lane: { id: 'Lane_12u5n6x', flowNodeRef: 'event_id_0' } },
},
BPMNDiagram: {
BPMNPlane: {
Expand Down Expand Up @@ -471,7 +469,7 @@ describe('parse bpmn as json for lane', () => {
definitions: {
targetNamespace: '',
process: {
lane: { id: 'Lane_12u5n6x' },
laneSet: { lane: { id: 'Lane_12u5n6x' } },
},
BPMNDiagram: {
BPMNPlane: {
Expand Down Expand Up @@ -509,7 +507,7 @@ describe('parse bpmn as json for lane', () => {
definitions: {
targetNamespace: '',
process: {
lane: { id: 'Lane_12u5n6x' },
laneSet: { lane: { id: 'Lane_12u5n6x' } },
},
BPMNDiagram: {
BPMNPlane: {
Expand Down
3 changes: 2 additions & 1 deletion test/unit/component/parser/json/BpmnJsonParser.marker.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { parseJsonAndExpectOnlyFlowNodes } from '../../../helpers/JsonTestUtils'
import { getExpectedMarkers, verifyShape } from '../../../helpers/bpmn-model-expect';

import type { BpmnJsonModel } from '@lib/model/bpmn/json/BPMN20';
import type { TProcess } from '@lib/model/bpmn/json/baseElement/rootElement/rootElement';
import type { TMultiInstanceLoopCharacteristics, TStandardLoopCharacteristics } from '@lib/model/bpmn/json/baseElement/loopCharacteristics';
import { ShapeBpmnCallActivityKind, ShapeBpmnElementKind, ShapeBpmnMarkerKind } from '@lib/model/bpmn/internal';

Expand All @@ -34,7 +35,7 @@ describe.each([
['manualTask', ShapeBpmnElementKind.TASK_MANUAL],
['scriptTask', ShapeBpmnElementKind.TASK_SCRIPT],
['businessRuleTask', ShapeBpmnElementKind.TASK_BUSINESS_RULE],
])(`parse bpmn as json for '%s'`, (bpmnSemanticType: string, expectedShapeBpmnElementKind: ShapeBpmnElementKind) => {
] as [keyof TProcess, ShapeBpmnElementKind][])(`parse bpmn as json for '%s'`, (bpmnSemanticType: keyof TProcess, expectedShapeBpmnElementKind: ShapeBpmnElementKind) => {
describe.each([
['standardLoopCharacteristics', ShapeBpmnMarkerKind.LOOP],
['multiInstanceLoopCharacteristics', ShapeBpmnMarkerKind.MULTI_INSTANCE_PARALLEL],
Expand Down
62 changes: 27 additions & 35 deletions test/unit/helpers/JsonBuilder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import type { TBaseElement, TLane, TLaneSet, TMessageFlow } from '@lib/model/bpm
import type { TFlowElement } from '@lib/model/bpmn/json/baseElement/flowElement';
import type { TFlowNode } from '@lib/model/bpmn/json/baseElement/flowElement';
import type { TBoundaryEvent, TCatchEvent, TThrowEvent } from '@lib/model/bpmn/json/baseElement/flowNode/event';
import type { TParticipant } from '@lib/model/bpmn/json/baseElement/participant';
import type { TCollaboration } from '@lib/model/bpmn/json/baseElement/rootElement/collaboration';
import type { TEventDefinition } from '@lib/model/bpmn/json/baseElement/rootElement/eventDefinition';
import type { TProcess } from '@lib/model/bpmn/json/baseElement/rootElement/rootElement';
Expand Down Expand Up @@ -290,17 +289,13 @@ function addParticipantProcessAndElements(processParameter: BuildProcessParamete
if (processParameter.withParticipant) {
addParticipant(id, jsonModel);
}
updateBpmnElement(
jsonModel.definitions.process as TProcess | TProcess[],
{ id: processParameter.withParticipant ? `process_${id}` : id },
(value: TProcess | TProcess[]) => (jsonModel.definitions.process = value),
);
jsonModel.definitions.process = getBpmnElementNewValue(jsonModel.definitions.process as TProcess | TProcess[], { id: processParameter.withParticipant ? `process_${id}` : id });
addElementsOnProcess(processParameter, jsonModel, index);
}

function addParticipant(id: string, jsonModel: BpmnJsonModel): void {
const collaboration: TCollaboration = getElementOfArray<TProcess>(jsonModel.definitions.collaboration as TCollaboration);
updateBpmnElement(collaboration.participant, { id: id, processRef: `process_${id}` }, (value: TParticipant | TParticipant[]) => (collaboration.participant = value));
collaboration.participant = getBpmnElementNewValue(collaboration.participant, { id: id, processRef: `process_${id}` });

const shape = {
id: `shape_${id}`,
Expand All @@ -314,7 +309,7 @@ function addMessageFlow(messageFlowParameter: BuildMessageFlowParameter, jsonMod
const messageFlow: TMessageFlow = messageFlowParameter;

const collaboration: TCollaboration = getElementOfArray<TCollaboration>(jsonModel.definitions.collaboration as TCollaboration);
updateBpmnElement(collaboration.messageFlow, messageFlow, (value: TMessageFlow | TMessageFlow[]) => (collaboration.messageFlow = value));
collaboration.messageFlow = getBpmnElementNewValue(collaboration.messageFlow, messageFlow);

addEdge(jsonModel, {
bpmnElement: messageFlow.id,
Expand All @@ -332,7 +327,7 @@ const addGlobalTask = ({ id, bpmnKind = 'globalTask', ...rest }: BuildGlobalTask
};

const definitions = jsonModel.definitions;
updateBpmnElement(definitions[bpmnKind], globalTask, (value: TGlobalTask | TGlobalTask[]) => (definitions[bpmnKind] = value));
definitions[bpmnKind] = getBpmnElementNewValue(definitions[bpmnKind], globalTask);
};

function addElementsOnProcess(processParameter: BuildProcessParameter, json: BpmnJsonModel, processIndex: number): void {
Expand Down Expand Up @@ -423,15 +418,16 @@ function getElementOfArray<T>(object: T | T[], index = 0): T {
}
}

function updateBpmnElement<T extends TBaseElement | DiagramElement>(parentElement: T | T[], childElement: T, setValue: (value: T | T[]) => void): void {
if (parentElement) {
if (!Array.isArray(parentElement)) {
setValue([parentElement, childElement]);
function getBpmnElementNewValue<T extends TBaseElement | DiagramElement>(currentValue: T | T[], elementToAdd: T): T | T[] {
csouchet marked this conversation as resolved.
Show resolved Hide resolved
if (currentValue) {
if (!Array.isArray(currentValue)) {
return [currentValue, elementToAdd];
} else {
parentElement.push(childElement);
currentValue.push(elementToAdd);
return currentValue;
}
} else {
setValue(childElement);
return elementToAdd;
}
}

Expand All @@ -443,8 +439,9 @@ function addLane(jsonModel: BpmnJsonModel, { id, name, ...rest }: BuildLaneParam
if (name) {
lane.name = name;
}

const laneSet = getElementOfArray<TProcess>(jsonModel.definitions.process as TProcess | TProcess[], processIndex).laneSet as TLaneSet;
updateBpmnElement(laneSet.lane, lane, (value: TLane | TLane[]) => (laneSet.lane = value));
laneSet.lane = getBpmnElementNewValue(laneSet.lane, lane);

const shape = {
id: `shape_${lane.id}`,
Expand All @@ -470,39 +467,34 @@ function addProcessElementWithEdge(jsonModel: BpmnJsonModel, bpmnKind: keyof TPr
});
}

function addProcessElement(jsonModel: BpmnJsonModel, bpmnKind: keyof TProcess, { id, index, processIndex, ...rest }: BuildProcessElementParameter): TFlowNode {
const processElement: TFlowNode | TArtifact = {
function addProcessElement(jsonModel: BpmnJsonModel, bpmnKind: keyof TProcess, { id, index, processIndex, ...rest }: BuildProcessElementParameter): TFlowNode | TArtifact {
const processElement: TFlowElement | TArtifact = {
id: id ? id : `${bpmnKind}_id_${processIndex}_${index}`,
...rest,
};

const process: TProcess = getElementOfArray<TProcess>(jsonModel.definitions.process as TProcess | TProcess[], processIndex);
updateBpmnElement(process[bpmnKind], processElement, (value: TFlowNode | TFlowNode[] | TArtifact | TArtifact[]) => (process[bpmnKind] = value));
(process[bpmnKind] as TFlowElement | TArtifact | TFlowNode[] | TArtifact[]) = getBpmnElementNewValue<TFlowElement | TArtifact>(
process[bpmnKind] as TFlowElement | TArtifact | TFlowNode[] | TArtifact[],
processElement,
);
return processElement;
}

function addShape(jsonModel: BpmnJsonModel, shape: BPMNShape): void {
const bpmnPlane: BPMNPlane = getElementOfArray(jsonModel.definitions.BPMNDiagram).BPMNPlane;
updateBpmnElement(
bpmnPlane.BPMNShape,
{
id: `shape_${shape.bpmnElement}`,
...shape,
},
(value: BPMNShape | BPMNShape[]) => (bpmnPlane.BPMNShape = value),
);
bpmnPlane.BPMNShape = getBpmnElementNewValue(bpmnPlane.BPMNShape, {
id: `shape_${shape.bpmnElement}`,
...shape,
});
}

function addEdge(jsonModel: BpmnJsonModel, edge: BPMNEdge): void {
const bpmnPlane: BPMNPlane = getElementOfArray(jsonModel.definitions.BPMNDiagram).BPMNPlane;
updateBpmnElement(
bpmnPlane.BPMNEdge,
{
id: `edge_${edge.bpmnElement}`,
...edge,
},
(value: BPMNEdge | BPMNEdge[]) => (bpmnPlane.BPMNEdge = value),
);
bpmnPlane.BPMNEdge = getBpmnElementNewValue(bpmnPlane.BPMNEdge, {
id: `edge_${edge.bpmnElement}`,
...edge,
});
}

function addEventDefinitions(
Expand Down