diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/__tests__/find-matching-branch.util.spec.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/__tests__/find-matching-branch.util.spec.ts new file mode 100644 index 0000000000..b8d7c1c766 --- /dev/null +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/__tests__/find-matching-branch.util.spec.ts @@ -0,0 +1,120 @@ +import { StepLogicalOperator, type StepFilterGroup } from 'twenty-shared/types'; +import { type StepIfElseBranch } from 'twenty-shared/workflow'; + +import { + WorkflowStepExecutorException, + WorkflowStepExecutorExceptionCode, +} from 'src/modules/workflow/workflow-executor/exceptions/workflow-step-executor.exception'; +import { + findMatchingBranch, + type ResolvedFilter, +} from 'src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/find-matching-branch.util'; + +describe('findMatchingBranch', () => { + const realGroup: StepFilterGroup = { + id: 'real-group', + logicalOperator: StepLogicalOperator.AND, + }; + + const matchingFilter = ( + stepFilterGroupId: string, + matches: boolean, + ): ResolvedFilter => + ({ + id: 'filter-1', + type: 'TEXT', + operand: 'IS', + stepFilterGroupId, + rightOperand: 'expected-value', + leftOperand: matches ? 'expected-value' : 'something-else', + }) as ResolvedFilter; + + it('should return the branch whose filter condition evaluates to true', () => { + const branches: StepIfElseBranch[] = [ + { id: 'branch-a', filterGroupId: 'real-group', nextStepIds: [] }, + ]; + + const matched = findMatchingBranch({ + branches, + stepFilterGroups: [realGroup], + resolvedFilters: [matchingFilter('real-group', true)], + }); + + expect(matched.id).toBe('branch-a'); + }); + + it('should return the trailing else branch when no conditional branch matches', () => { + const branches: StepIfElseBranch[] = [ + { id: 'branch-a', filterGroupId: 'real-group', nextStepIds: [] }, + { id: 'branch-else', nextStepIds: [] }, + ]; + + const matched = findMatchingBranch({ + branches, + stepFilterGroups: [realGroup], + resolvedFilters: [matchingFilter('real-group', false)], + }); + + expect(matched.id).toBe('branch-else'); + }); + + it('should throw INVALID_STEP_INPUT instead of silently matching a branch whose filterGroupId does not resolve to any stepFilterGroup', () => { + const branches: StepIfElseBranch[] = [ + { + id: 'branch-dangling', + filterGroupId: 'group-id-not-in-stepFilterGroups', + nextStepIds: [], + }, + { id: 'branch-real', filterGroupId: 'real-group', nextStepIds: [] }, + ]; + + expect(() => + findMatchingBranch({ + branches, + stepFilterGroups: [realGroup], + resolvedFilters: [matchingFilter('real-group', false)], + }), + ).toThrow( + expect.objectContaining({ + code: WorkflowStepExecutorExceptionCode.INVALID_STEP_INPUT, + }), + ); + }); + + it('should throw for a dangling branch even when an earlier branch already matches', () => { + const branches: StepIfElseBranch[] = [ + { id: 'branch-match', filterGroupId: 'real-group', nextStepIds: [] }, + { + id: 'branch-dangling', + filterGroupId: 'group-id-not-in-stepFilterGroups', + nextStepIds: [], + }, + ]; + + expect(() => + findMatchingBranch({ + branches, + stepFilterGroups: [realGroup], + resolvedFilters: [matchingFilter('real-group', true)], + }), + ).toThrow( + expect.objectContaining({ + code: WorkflowStepExecutorExceptionCode.INVALID_STEP_INPUT, + }), + ); + }); + + it('should throw when no branch matches and there is no else branch', () => { + const branches: StepIfElseBranch[] = [ + { id: 'branch-a', filterGroupId: 'real-group', nextStepIds: [] }, + ]; + + expect(() => + findMatchingBranch({ + branches, + stepFilterGroups: [realGroup], + resolvedFilters: [matchingFilter('real-group', false)], + }), + ).toThrow(WorkflowStepExecutorException); + }); +}); diff --git a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/find-matching-branch.util.ts b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/find-matching-branch.util.ts index 87f58616b9..fe0243a880 100644 --- a/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/find-matching-branch.util.ts +++ b/packages/twenty-server/src/modules/workflow/workflow-executor/workflow-actions/if-else/utils/find-matching-branch.util.ts @@ -46,6 +46,18 @@ export const findMatchingBranch = ({ stepFilterGroups: StepFilterGroup[]; resolvedFilters: ResolvedFilter[]; }): StepIfElseBranch => { + for (const branch of branches) { + if ( + isDefined(branch.filterGroupId) && + !stepFilterGroups.some((group) => group.id === branch.filterGroupId) + ) { + throw new WorkflowStepExecutorException( + `Branch "${branch.id}" references filter group "${branch.filterGroupId}", which does not exist`, + WorkflowStepExecutorExceptionCode.INVALID_STEP_INPUT, + ); + } + } + const matchingBranch = branches.find((branch) => { if (!isDefined(branch.filterGroupId)) { return true; diff --git a/packages/twenty-shared/src/workflow/validation/utils/__tests__/validate-workflow-graph.util.test.ts b/packages/twenty-shared/src/workflow/validation/utils/__tests__/validate-workflow-graph.util.test.ts index b43a170c02..92a60135b4 100644 --- a/packages/twenty-shared/src/workflow/validation/utils/__tests__/validate-workflow-graph.util.test.ts +++ b/packages/twenty-shared/src/workflow/validation/utils/__tests__/validate-workflow-graph.util.test.ts @@ -102,6 +102,78 @@ describe('validateWorkflowGraph', () => { expect(getCodes(workflow)).toContain('IF_ELSE_BRANCH_HAS_NO_NEXT_STEP'); }); + it('should flag an if-else branch whose filterGroupId does not exist', () => { + const workflow: ValidatableWorkflow = { + trigger: { type: 'MANUAL', nextStepIds: ['if'] }, + steps: [ + { + id: 'if', + type: WorkflowActionType.IF_ELSE, + settings: { + input: { + stepFilterGroups: [{ id: 'real-group' }], + branches: [ + { nextStepIds: ['end'], filterGroupId: 'ghost-group' }, + { nextStepIds: ['end'] }, + ], + }, + }, + }, + { id: 'end', type: 'CODE' }, + ], + }; + + expect(getCodes(workflow)).toContain('INVALID_STEP_PARAMS'); + }); + + it('should not flag an if-else branch whose filterGroupId exists', () => { + const workflow: ValidatableWorkflow = { + trigger: { type: 'MANUAL', nextStepIds: ['if'] }, + steps: [ + { + id: 'if', + type: WorkflowActionType.IF_ELSE, + settings: { + input: { + stepFilterGroups: [{ id: 'real-group' }], + branches: [ + { nextStepIds: ['end'], filterGroupId: 'real-group' }, + { nextStepIds: ['end'] }, + ], + }, + }, + }, + { id: 'end', type: 'CODE' }, + ], + }; + + expect(getCodes(workflow)).not.toContain('INVALID_STEP_PARAMS'); + }); + + it('should not throw when an if-else step has a non-array stepFilterGroups', () => { + const workflow: ValidatableWorkflow = { + trigger: { type: 'MANUAL', nextStepIds: ['if'] }, + steps: [ + { + id: 'if', + type: WorkflowActionType.IF_ELSE, + settings: { + input: { + stepFilterGroups: 'not-an-array', + branches: [ + { nextStepIds: ['end'], filterGroupId: 'ghost-group' }, + { nextStepIds: ['end'] }, + ], + }, + }, + }, + { id: 'end', type: 'CODE' }, + ], + }; + + expect(() => getCodes(workflow)).not.toThrow(); + }); + it('should flag an iterator with items but no loop body', () => { const workflow: ValidatableWorkflow = { trigger: { type: 'MANUAL', nextStepIds: ['iterator'] }, diff --git a/packages/twenty-shared/src/workflow/validation/utils/validate-workflow-graph.util.ts b/packages/twenty-shared/src/workflow/validation/utils/validate-workflow-graph.util.ts index 29b411e326..6647fa9db7 100644 --- a/packages/twenty-shared/src/workflow/validation/utils/validate-workflow-graph.util.ts +++ b/packages/twenty-shared/src/workflow/validation/utils/validate-workflow-graph.util.ts @@ -106,6 +106,14 @@ const validateBranchingStep = ( }); } + const stepFilterGroups = (input as Partial | undefined) + ?.stepFilterGroups; + const stepFilterGroupIds = new Set( + (Array.isArray(stepFilterGroups) ? stepFilterGroups : []) + .filter(isDefined) + .map((filterGroup) => filterGroup.id), + ); + for (const branch of branches) { const branchNextStepIds = branch?.nextStepIds; @@ -117,6 +125,18 @@ const validateBranchingStep = ( stepId: step.id, }); } + + if ( + isDefined(branch?.filterGroupId) && + !stepFilterGroupIds.has(branch.filterGroupId) + ) { + issues.push({ + severity: 'error', + code: 'INVALID_STEP_PARAMS', + message: `A branch of If/Else step "${step.name ?? step.id}" references filter group "${branch.filterGroupId}", which does not exist.`, + stepId: step.id, + }); + } } }