Merge pull request #4 from CarstenWickner/path-validation

CarstenWickner · web-flow · commit c4fabc752e28 · 2020-01-12T22:49:34.000+01:00
diff --git a/package.json b/package.json
@@ -33,10 +33,10 @@
     ],
     "devDependencies": {
         "@mdx-js/loader": "~1.5.1",
-        "@storybook/addon-actions": "~5.2.3",
-        "@storybook/addon-docs": "~5.2.3",
-        "@storybook/addon-options": "~5.2.3",
-        "@storybook/react": "~5.2.3",
+        "@storybook/addon-actions": "~5.3.1",
+        "@storybook/addon-docs": "~5.3.1",
+        "@storybook/addon-options": "~5.3.1",
+        "@storybook/react": "~5.3.1",
         "@types/enzyme": "~3.10.3",
         "@types/enzyme-adapter-react-16": "~1.0.5",
         "@types/jest": "24.0.23",
@@ -49,6 +49,7 @@
         "@wessberg/rollup-plugin-ts": "~1.1.73",
         "awesome-typescript-loader": "~5.2.1",
         "babel-loader": "~8.0.6",
+        "babel-preset-react-app": "~9.1.0",
         "coveralls": "~3.0.6",
         "css-loader": "~3.3.0",
         "enzyme": "~3.10.0",
diff --git a/src/component/renderDataUtils.ts b/src/component/renderDataUtils.ts
@@ -2,7 +2,7 @@ import { FlowContent, FlowGatewayDiverging, FlowModelerProps } from "../types/Fl
 import { GridCellData, ElementType, ConnectionType } from "../types/GridCellData";
 import { createElementTree } from "../model/modelUtils";
 import { FlowElement } from "../model/FlowElement";
-import { validatePaths } from "../model/pathValidationUtils";
+import { checkForCircularReference, validatePaths } from "../model/pathValidationUtils";
 
 const getColumnIndexAfter = (element: FlowElement): number => element.getColumnIndex() + (element.getFollowingElements().length > 1 ? 2 : 1);
 
@@ -122,8 +122,10 @@ export const buildRenderData = (
     flow: FlowModelerProps["flow"],
     verticalAlign: "top" | "bottom"
 ): { gridCellData: Array<GridCellData>; columnCount: number } => {
-    validatePaths(flow);
+    const { firstElementId, elements } = flow;
+    checkForCircularReference(firstElementId, elements);
     const treeRootElement = createElementTree(flow, verticalAlign);
+    validatePaths(treeRootElement);
     const result: Array<GridCellData> = [];
     // add single start element
     result.push({
@@ -132,7 +134,6 @@ export const buildRenderData = (
         rowEndIndex: 1 + treeRootElement.getRowCount(),
         type: ElementType.Start
     });
-    const { elements } = flow;
     collectGridCellData(treeRootElement, undefined, undefined, elements, 1, result);
     // for a more readable resulting html structure, sort the grid elements first from top to bottom and within each row from left to right
     result.sort(sortGridCellDataByPosition);
diff --git a/src/model/pathValidationUtils.ts b/src/model/pathValidationUtils.ts
@@ -1,68 +1,114 @@
+import { FlowElement } from "./FlowElement";
 import { isDivergingGateway } from "./modelUtils";
-import { FlowModelerProps, FlowContent, FlowGatewayDiverging } from "../types/FlowModelerProps";
 
-const recursivelyCollectPaths = (
+import { FlowModelerProps } from "../types/FlowModelerProps";
+
+/**
+ * Check the given input model for circular references, which are  specifically disallowed.
+ *
+ * @param {string} targetElementId - id of the element to check
+ * @param {object} elements - collection of all defined elements in the input model
+ * @param {Array.<string>} currentPath - ids of elements in front of target element
+ * @throws Error in case of a circular reference being present
+ */
+export const checkForCircularReference = (
     targetElementId: string,
-    elements: { [key: string]: FlowContent | FlowGatewayDiverging },
-    currentPath: Array<string>,
-    otherPaths: Array<Array<string>>
+    elements: FlowModelerProps["flow"]["elements"],
+    currentPath: Array<string> = []
 ): void => {
+    const targetElement = elements[targetElementId];
+    if (!targetElement) {
+        // path ends, i.e. there was no circular reference here
+        return;
+    }
     if (currentPath.includes(targetElementId)) {
         // no reason to go in circles, stop it right there
         throw new Error(`Circular reference to element: ${targetElementId}`);
     }
     currentPath.push(targetElementId);
-    const targetElement = elements[targetElementId];
-    if (!targetElement) {
-        // current path ends here, i.e. it should be added to the overall list
-        otherPaths.push(currentPath);
-    } else if (isDivergingGateway(targetElement)) {
-        // ensure that there are always at least two sub elements under a gateway to allow for respective End elements to be displayed
-        let subElements;
-        if (targetElement.nextElements.length > 1) {
-            subElements = targetElement.nextElements;
-        } else if (targetElement.nextElements.length === 1) {
-            subElements = [...targetElement.nextElements, {}];
-        } else {
-            subElements = [{}, {}];
-        }
-        subElements.forEach((next) => recursivelyCollectPaths(next.id, elements, currentPath.slice(0), otherPaths));
+    if (isDivergingGateway(targetElement)) {
+        // check on each sub-path after the targeted diverging gateway
+        targetElement.nextElements.forEach((next) => checkForCircularReference(next.id, elements, currentPath.slice(0)));
     } else {
-        recursivelyCollectPaths(targetElement.nextElementId, elements, currentPath, otherPaths);
+        // continue check after the targeted content element
+        checkForCircularReference(targetElement.nextElementId, elements, currentPath);
     }
 };
-/*
-const filterPathsWithDifferingStart = ([elementId, pathsIncludingElement]: [string, Array<Array<string>>]): boolean => {
-    if (pathsIncludingElement.length < 2) {
-        return false;
-    }
-    const elementIndex = pathsIncludingElement[0].indexOf(elementId);
-    if (pathsIncludingElement.some((path) => path.indexOf(elementId) !== elementIndex)) {
-        return true;
+
+/**
+ * Check whether the given two parents of the specified child element are neighbours.
+ *
+ * @param {FlowElement} child - element being referenced from multiple parents (thereby being preceded by an implicit converging gateway)
+ * @param {FlowElement} firstParent - leading specific preceding element from which the designated child is being referenced
+ * @param {FlowElement} secondParent - trailing specific preceding element from which the designated child is being referenced
+ * @returns {boolean} whether the implicit converging gateway is valid.
+ */
+const areParentsNeighbours = (child: FlowElement, firstParent: FlowElement, secondParent: FlowElement): boolean => {
+    // collect path to second element
+    const topPathToSecond: Array<FlowElement> = [child, secondParent];
+    let leadingParentOfSecond = secondParent;
+    while (leadingParentOfSecond.getPrecedingElements().length) {
+        // in case of converging gateway, always take the top element
+        leadingParentOfSecond = leadingParentOfSecond.getPrecedingElements()[0];
+        topPathToSecond.push(leadingParentOfSecond);
     }
-    const leadingPathPart = pathsIncludingElement[0].slice(0, elementIndex);
-    return pathsIncludingElement.some((path) => leadingPathPart.some((value, index) => value !== path[index]));
+    // iterate backwards over path to first element until finding a common parent (worst case: the root element)
+    const bottomPathToFirst: Array<FlowElement> = [child];
+    let firstBranch = firstParent;
+    do {
+        bottomPathToFirst.push(firstBranch);
+        if (topPathToSecond.indexOf(firstBranch) > -1) {
+            break;
+        }
+        const parents = firstBranch.getPrecedingElements();
+        firstBranch = parents[parents.length - 1];
+        // keep going, worst case till the single root element
+    } while (true);
+    const commonIndexInPathToSecond = topPathToSecond.indexOf(firstBranch);
+    const commonSiblings = firstBranch.getFollowingElements();
+    // check whether the two paths are neighbouring when branching off from their right-most common parent
+    const firstBranchIndex = commonSiblings.indexOf(bottomPathToFirst[bottomPathToFirst.length - 2]);
+    const secondBranchIndex = commonSiblings.indexOf(topPathToSecond[commonIndexInPathToSecond - 1]);
+    return firstBranchIndex + 1 === secondBranchIndex;
 };
-*/
 
-export const validatePaths = ({ firstElementId, elements }: FlowModelerProps["flow"]): void => {
-    const paths: Array<Array<string>> = [];
-    recursivelyCollectPaths(firstElementId, elements, [], paths);
-    /*
-    const elementsOnMultiplePaths = Object.keys(elements)
-        .map((elementId) => [elementId, paths.filter((path) => path.includes(elementId))] as [string, Array<Array<string>>])
-        .filter(filterPathsWithDifferingStart);
-    const getIndexOfPath = (path: Array<string>): number => paths.indexOf(path);
-    const invalidElements = elementsOnMultiplePaths.filter(([, pathsWithOverlap]) => {
-        const indexes = pathsWithOverlap.map(getIndexOfPath);
-        return indexes.length && indexes[0] + indexes.length != indexes[indexes.length - 1] + 1;
-    });
+/**
+ * Check whether the implicit converging gateway in front of the given element is valid, i.e. whether all connection pairs are direct neighbours.
+ *
+ * @param {FlowElement} convergingGateway - element being referenced from multiple parents (thereby being preceded by an implicit converging gateway)
+ * @returns {boolean} whether the implicit converging gateway is invalid (beware the negation!)
+ */
+const isInvalidConvergingGateway = (convergingGateway: FlowElement): boolean => {
+    const connectedElements = convergingGateway.getPrecedingElements();
+    return connectedElements
+        .slice(1)
+        .some((nextElement, previousIndex) => !areParentsNeighbours(convergingGateway, connectedElements[previousIndex], nextElement));
+};
+
+/**
+ * Validate that the parsed data model can be properly displayed. Ensuring that only directly neighbouring paths can link to the same element.
+ *
+ * @param {FlowElement} treeRootElement - root of the parsed data model to validate
+ * @throws Error in case of any (implicit) converging gateways connecting non-neighbouring paths
+ */
+export const validatePaths = (treeRootElement: FlowElement): void => {
+    // use Set to automatically filter out duplicates and thereby avoid checking the same gateway repeatedly
+    const convergingGateways = new Set<FlowElement>();
+    const collectConvergingGateways = (element: FlowElement): void => {
+        if (element.getId()) {
+            if (element.getPrecedingElements().length > 1) {
+                convergingGateways.add(element);
+            }
+            element.getFollowingElements().forEach(collectConvergingGateways);
+        }
+    };
+    collectConvergingGateways(treeRootElement);
+    const invalidElements = Array.from(convergingGateways).filter(isInvalidConvergingGateway);
     if (invalidElements.length) {
         throw new Error(
             `Multiple references only valid from neighbouring paths. Invalid references to: '${invalidElements
-                .map(([elementId]) => elementId)
+                .map((gateway) => gateway.getId())
                 .join("', '")}'`
         );
     }
-    */
 };
diff --git a/test/component/renderDataUtils.test.ts b/test/component/renderDataUtils.test.ts
@@ -724,7 +724,7 @@ describe("buildRenderData()", () => {
             );
         expect(execution).toThrowError("Circular reference to element: a");
     });
-    it.skip("throws error for multiple references on non-neighbouring paths", () => {
+    it("throws error for multiple references on non-neighbouring paths", () => {
         const execution = (): never =>
             buildRenderData(
                 {
diff --git a/test/model/pathValidationUtils.test.ts b/test/model/pathValidationUtils.test.ts
@@ -1,33 +1,99 @@
-import { validatePaths } from "../../src/model/pathValidationUtils";
+import { validatePaths, checkForCircularReference } from "../../src/model/pathValidationUtils";
+import { createElementTree } from "../../src/model/modelUtils";
+import { FlowModelerProps } from "../../src/types/FlowModelerProps";
 
-describe("validatePaths()", () => {
-    const ref = (nextId: string): { nextElementId: string } => ({ nextElementId: nextId });
+const ref = (nextId: string): { nextElementId: string } => ({ nextElementId: nextId });
+
+describe("checkForCircularReference()", () => {
     it.each`
-        testDescription            | flowElements
-        ${"direct self-reference"} | ${{ a: ref("a") }}
-        ${"nested one level"}      | ${{ a: ref("b"), b: ref("a") }}
-        ${"nested six levels"}     | ${{ a: ref("b"), b: ref("c"), c: ref("d"), d: ref("e"), e: ref("f"), f: ref("g"), g: ref("a") }}
+        testDescription                | flowElements
+        ${"direct self-reference"}     | ${{ a: ref("a") }}
+        ${"nested one level"}          | ${{ a: ref("b"), b: ref("a") }}
+        ${"nested six levels"}         | ${{ a: ref("b"), b: ref("c"), c: ref("d"), d: ref("e"), e: ref("f"), f: ref("g"), g: ref("a") }}
+        ${"behind diverging gateway"}  | ${{ a: { nextElements: [{ id: "b" }, { id: "c" }] }, b: {}, c: ref("d"), d: ref("a") }}
+        ${"behind converging gateway"} | ${{ a: { nextElements: [{ id: "b" }, { id: "c" }] }, b: ref("d"), c: ref("d"), d: ref("a") }}
     `("throws error for circular reference – $testDescription", ({ flowElements }) => {
-        const execution = (): never =>
-            validatePaths({
-                firstElementId: "a",
-                elements: flowElements
-            });
+        const execution = (): never => checkForCircularReference("a", flowElements);
         expect(execution).toThrowError("Circular reference to element: a");
     });
-    it.skip("throws error for multiple references on non-neighbouring paths", () => {
-        const execution = (): never =>
-            validatePaths({
-                firstElementId: "a",
-                elements: {
+    it("accepts non-circular references", () => {
+        const flowElements = {
+            a: ref("b"),
+            b: { nextElements: [{ id: "c" }, { id: "d" }, { id: "e" }] },
+            c: ref("f"),
+            d: { nextElements: [{ id: "d1" }, { id: "d2" }] },
+            d1: ref("f"),
+            d2: ref("f"),
+            e: ref("f"),
+            f: {}
+        };
+        const execution = (): never => checkForCircularReference("a", flowElements);
+        expect(execution).not.toThrowError();
+    });
+});
+
+describe("validatePaths()", () => {
+    describe.each`
+        testDescription                | firstElementId | additionalElements
+        ${"root"}                      | ${"a"}         | ${{}}
+        ${"behind content element"}    | ${"root"}      | ${{ root: ref("a") }}
+        ${"behind converging gateway"} | ${"root"}      | ${{ root: { nextElements: [{ id: "x" }, { id: "y" }] }, x: ref("a"), y: ref("a") }}
+    `(
+        "properly performs validation when gateway is $testDescription",
+        ({ firstElementId, additionalElements }: { firstElementId: string; additionalElements: FlowModelerProps["flow"]["elements"] }) => {
+            it("accepts link between diverging gateway and one of its children", () => {
+                const elements = {
+                    ...additionalElements,
+                    a: { nextElements: [{ id: "b" }, { id: "c" }] },
+                    b: ref("c"),
+                    c: {}
+                };
+                const treeRootElement = createElementTree({ firstElementId, elements }, "top");
+                const execution = (): never => validatePaths(treeRootElement);
+                expect(execution).not.toThrowError();
+            });
+            it("accepts link between neighbouring children of diverging gateway", () => {
+                const elements = {
+                    ...additionalElements,
+                    a: { nextElements: [{ id: "b" }, { id: "c" }] },
+                    b: ref("d"),
+                    c: ref("d"),
+                    d: {}
+                };
+                const treeRootElement = createElementTree({ firstElementId, elements }, "top");
+                const execution = (): never => validatePaths(treeRootElement);
+                expect(execution).not.toThrowError();
+            });
+            it("accepts links between multiple ancestors of nested diverging gateways", () => {
+                const elements = {
+                    ...additionalElements,
+                    a: { nextElements: [{ id: "b" }, { id: "c" }, { id: "d" }, { id: "e" }] },
+                    b: ref("bc"),
+                    c: ref("bc"),
+                    bc: ref("f"),
+                    d: ref("de"),
+                    e: ref("de"),
+                    de: ref("f"),
+                    f: {}
+                };
+                const treeRootElement = createElementTree({ firstElementId, elements }, "top");
+                const execution = (): never => validatePaths(treeRootElement);
+                expect(execution).not.toThrowError();
+            });
+            it("throws error for multiple references on non-neighbouring paths", () => {
+                const elements = {
+                    ...additionalElements,
                     a: { nextElements: [{ id: "b" }, { id: "c" }, { id: "d" }, { id: "e" }] },
                     b: ref("f"),
                     c: {},
                     d: ref("f"),
                     e: ref("b"),
                     f: {}
-                }
+                };
+                const treeRootElement = createElementTree({ firstElementId, elements }, "top");
+                const execution = (): never => validatePaths(treeRootElement);
+                expect(execution).toThrowError("Multiple references only valid from neighbouring paths. Invalid references to: 'b', 'f'");
             });
-        expect(execution).toThrowError("Multiple references only valid from neighbouring paths. Invalid references to: 'b', 'f'");
-    });
+        }
+    );
 });

Original file line number	Diff line number	Diff line change
`@@ -724,7 +724,7 @@ describe("buildRenderData()", () => {`
`724`	`724`	`);`
`725`	`725`	`expect(execution).toThrowError("Circular reference to element: a");`
`726`	`726`	`});`
`727`		`- it.skip("throws error for multiple references on non-neighbouring paths", () => {`
	`727`	`+ it("throws error for multiple references on non-neighbouring paths", () => {`
`728`	`728`	`const execution = (): never =>`
`729`	`729`	`buildRenderData(`
`730`	`730`	`{`