Merge pull request #392

fix(21625): Prevent cycles in the Designer graphs * fix(21625): refactor the isValidConnection routine to detect self-con… * test(21625): add tests * refactor(21625): properly return a value * test(21625): fix test
hivemq · Apr 29, 2024 · 5752ba6 · 5752ba6
1 parent 2e700c3
commit 5752ba6
Show file tree

Hide file tree

Showing 4 changed files with 125 additions and 22 deletions.
diff --git a/hivemq-edge/src/frontend/src/components/Chakra/ShortcutRenderer.spec.cy.tsx b/hivemq-edge/src/frontend/src/components/Chakra/ShortcutRenderer.spec.cy.tsx
@@ -19,8 +19,10 @@ describe('ShortcutRenderer', () => {
 
   it('should render multiple shortcuts', () => {
     cy.mountWithProviders(<ShortcutRenderer hotkeys="CTRL+C,Meta+V,ESC" description="This is a description" />)
+    const cmd = Cypress.platform === 'darwin' ? 'Command' : 'Ctrl'
+    console.log('XXXXXX', cmd)
 
-    cy.get('[role="term"]').should('contain.text', 'CTRL + C , Command + V , ESC')
+    cy.get('[role="term"]').should('contain.text', `CTRL + C , ${cmd} + V , ESC`)
     cy.get('kbd').should('have.length', 5)
   })
 

diff --git a/hivemq-edge/src/frontend/src/extensions/datahub/components/pages/PolicyEditor.tsx b/hivemq-edge/src/frontend/src/extensions/datahub/components/pages/PolicyEditor.tsx
@@ -24,7 +24,10 @@ const PolicyEditor: FC = () => {
 
   const nodeTypes = useMemo(() => CustomNodeTypes, [])
 
-  const checkValidity = useCallback((connection: Connection) => isValidPolicyConnection(connection, nodes), [nodes])
+  const checkValidity = useCallback(
+    (connection: Connection) => isValidPolicyConnection(connection, nodes, edges),
+    [edges, nodes]
+  )
 
   const onDragOver = useCallback((event: React.DragEvent<HTMLElement> | undefined) => {
     if (event) {

diff --git a/hivemq-edge/src/frontend/src/extensions/datahub/utils/node.utils.spec.ts b/hivemq-edge/src/frontend/src/extensions/datahub/utils/node.utils.spec.ts
@@ -2,7 +2,7 @@ import { describe, expect } from 'vitest'
 import { DataHubNodeType, DataPolicyData, OperationData, SchemaType, StrategyType, ValidatorType } from '../types.ts'
 import { getNodeId, getNodePayload, isValidPolicyConnection } from './node.utils.ts'
 import { MOCK_JSONSCHEMA_SCHEMA } from '../__test-utils__/schema.mocks.ts'
-import { Node } from 'reactflow'
+import { Edge, Node } from 'reactflow'
 import { MOCK_DEFAULT_NODE } from '@/__test-utils__/react-flow/nodes.ts'
 
 describe('getNodeId', () => {
@@ -70,7 +70,7 @@ describe('getNodePayload', () => {
 describe('isValidPolicyConnection', () => {
   it('should not be a valid connection with an unknown source', async () => {
     expect(
-      isValidPolicyConnection({ source: 'source', target: 'target', sourceHandle: null, targetHandle: null }, [])
+      isValidPolicyConnection({ source: 'source', target: 'target', sourceHandle: null, targetHandle: null }, [], [])
     ).toBeFalsy()
   })
 
@@ -82,9 +82,11 @@ describe('isValidPolicyConnection', () => {
       position: { x: 0, y: 0 },
     }
     expect(
-      isValidPolicyConnection({ source: 'node-id', target: 'target', sourceHandle: null, targetHandle: null }, [
-        MOCK_NODE_DATA_POLICY,
-      ])
+      isValidPolicyConnection(
+        { source: 'node-id', target: 'target', sourceHandle: null, targetHandle: null },
+        [MOCK_NODE_DATA_POLICY],
+        []
+      )
     ).toBeFalsy()
   })
 
@@ -97,9 +99,11 @@ describe('isValidPolicyConnection', () => {
       position: { x: 0, y: 0 },
     }
     expect(
-      isValidPolicyConnection({ source: 'node-id', target: 'target', sourceHandle: null, targetHandle: null }, [
-        MOCK_NODE_DATA_POLICY,
-      ])
+      isValidPolicyConnection(
+        { source: 'node-id', target: 'target', sourceHandle: null, targetHandle: null },
+        [MOCK_NODE_DATA_POLICY],
+        []
+      )
     ).toBeFalsy()
   })
 
@@ -120,10 +124,11 @@ describe('isValidPolicyConnection', () => {
       position: { x: 0, y: 0 },
     }
     expect(
-      isValidPolicyConnection({ source: 'node-id', target: 'node-operation', sourceHandle: null, targetHandle: null }, [
-        MOCK_NODE_DATA_POLICY,
-        MOCK_NODE_OPERATION,
-      ])
+      isValidPolicyConnection(
+        { source: 'node-id', target: 'node-operation', sourceHandle: null, targetHandle: null },
+        [MOCK_NODE_DATA_POLICY, MOCK_NODE_OPERATION],
+        []
+      )
     ).toBeTruthy()
   })
 
@@ -151,7 +156,8 @@ describe('isValidPolicyConnection', () => {
           sourceHandle: null,
           targetHandle: OperationData.Handle.SCHEMA,
         },
-        [MOCK_NODE_DATA_POLICY, MOCK_NODE_OPERATION]
+        [MOCK_NODE_DATA_POLICY, MOCK_NODE_OPERATION],
+        []
       )
     ).toBeTruthy()
 
@@ -163,7 +169,75 @@ describe('isValidPolicyConnection', () => {
           sourceHandle: null,
           targetHandle: DataPolicyData.Handle.ON_SUCCESS,
         },
-        [MOCK_NODE_DATA_POLICY, MOCK_NODE_OPERATION]
+        [MOCK_NODE_DATA_POLICY, MOCK_NODE_OPERATION],
+        []
+      )
+    ).toBeFalsy()
+  })
+
+  it('should not be a valid connection if self-referencing', async () => {
+    const MOCK_NODE_OPERATION: Node = {
+      id: 'node-operation',
+      type: DataHubNodeType.OPERATION,
+      data: {},
+      ...MOCK_DEFAULT_NODE,
+      position: { x: 0, y: 0 },
+    }
+    expect(
+      isValidPolicyConnection(
+        {
+          source: 'node-operation',
+          target: 'node-operation',
+          sourceHandle: null,
+          targetHandle: null,
+        },
+        [MOCK_NODE_OPERATION],
+        []
+      )
+    ).toBeFalsy()
+  })
+
+  it('should not be a valid connection if creating a cycle', async () => {
+    const MOCK_NODE_OPERATION: Node = {
+      id: 'node-operation',
+      type: DataHubNodeType.OPERATION,
+      data: {},
+      ...MOCK_DEFAULT_NODE,
+      position: { x: 0, y: 0 },
+    }
+
+    const nodes: Node[] = [
+      MOCK_NODE_OPERATION,
+      { ...MOCK_NODE_OPERATION, id: 'node-operation-1' },
+      { ...MOCK_NODE_OPERATION, id: 'node-operation-2' },
+    ]
+    const edges: Edge[] = [
+      { id: '1', source: 'node-operation', target: 'node-operation-1', sourceHandle: null, targetHandle: null },
+      { id: '1', source: 'node-operation-1', target: 'node-operation-2', sourceHandle: null, targetHandle: null },
+    ]
+
+    expect(
+      isValidPolicyConnection(
+        {
+          source: 'node-operation-1',
+          target: 'node-operation',
+          sourceHandle: null,
+          targetHandle: null,
+        },
+        nodes,
+        edges
+      )
+    ).toBeFalsy()
+    expect(
+      isValidPolicyConnection(
+        {
+          source: 'node-operation-2',
+          target: 'node-operation',
+          sourceHandle: null,
+          targetHandle: null,
+        },
+        nodes,
+        edges
       )
     ).toBeFalsy()
   })

diff --git a/hivemq-edge/src/frontend/src/extensions/datahub/utils/node.utils.ts b/hivemq-edge/src/frontend/src/extensions/datahub/utils/node.utils.ts
@@ -1,4 +1,4 @@
-import { Connection, Node } from 'reactflow'
+import { Connection, Edge, getOutgoers, Node } from 'reactflow'
 import { v4 as uuidv4 } from 'uuid'
 import { MOCK_JSONSCHEMA_SCHEMA } from '../__test-utils__/schema.mocks.ts'
 
@@ -78,18 +78,42 @@ export const validConnections: ConnectionValidity = {
   [DataHubNodeType.FUNCTION]: [[DataHubNodeType.OPERATION, OperationData.Handle.FUNCTION]],
 }
 
-export const isValidPolicyConnection = (connection: Connection, nodes: Node[]) => {
+export const isValidPolicyConnection = (connection: Connection, nodes: Node[], edges: Edge[]) => {
   const source = nodes.find((node) => node.id === connection.source)
   const destination = nodes.find((node) => node.id === connection.target)
 
-  if (!source) {
+  const hasCycle = (node: Node, visited = new Set()) => {
+    if (visited.has(node.id)) return false
+
+    visited.add(node.id)
+
+    for (const outgoer of getOutgoers(node, nodes, edges)) {
+      if (outgoer.id === connection.source) return true
+      if (hasCycle(outgoer, visited)) return true
+    }
+    return false
+  }
+
+  if (!source || !destination) {
+    return false
+  }
+
+  if (!source.type) {
+    // node that are not Data Hub types are illegal
     return false
   }
-  const { type } = source
-  if (!type) {
+
+  if (destination.id === source.id) {
+    // self-connection are illegal
     return false
   }
-  const connectionValidators = validConnections[type]
+
+  if (hasCycle(destination)) {
+    // cycle are illegal
+    return false
+  }
+
+  const connectionValidators = validConnections[source.type]
   if (!connectionValidators) {
     return false
   }