diff --git a/src/RulesEngine/RulesCache.cs b/src/RulesEngine/RulesCache.cs index 968d7b80..863c53be 100644 --- a/src/RulesEngine/RulesCache.cs +++ b/src/RulesEngine/RulesCache.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. +// Copyright (c) Microsoft Corporation. // Licensed under the MIT License. using RulesEngine.HelperFunctions; @@ -105,26 +105,53 @@ public void Clear() /// Workflows. /// Could not find injected Workflow: {wfname} public Workflow GetWorkflow(string workflowName) + { + return GetWorkflow(workflowName, null); + } + + private Workflow GetWorkflow(string workflowName, HashSet visited) { if (_workflow.TryGetValue(workflowName, out (Workflow rules, long tick) WorkflowsObj)) { var workflow = WorkflowsObj.rules; if (workflow.WorkflowsToInject?.Any() == true) { - if (workflow.Rules == null) + visited ??= new HashSet(StringComparer.Ordinal); + if (!visited.Add(workflowName)) { - workflow.Rules = new List(); + throw new InvalidOperationException($"Circular WorkflowsToInject reference detected involving workflow: {workflowName}"); } + + var combinedRules = workflow.Rules?.ToList() ?? new List(); foreach (string wfname in workflow.WorkflowsToInject) { - var injectedWorkflow = GetWorkflow(wfname); + if (visited.Contains(wfname)) + { + throw new InvalidOperationException($"Circular WorkflowsToInject reference detected involving workflow: {wfname}"); + } + + var injectedWorkflow = GetWorkflow(wfname, visited); if (injectedWorkflow == null) { throw new Exception($"Could not find injected Workflow: {wfname}"); } - workflow.Rules = workflow.Rules.Concat(injectedWorkflow.Rules).ToList(); + if (injectedWorkflow.Rules != null) + { + combinedRules.AddRange(injectedWorkflow.Rules); + } } + + visited.Remove(workflowName); + + return new Workflow + { + WorkflowName = workflow.WorkflowName, + RuleExpressionType = workflow.RuleExpressionType, + GlobalParams = workflow.GlobalParams, + WorkflowsToInject = workflow.WorkflowsToInject, + Rules = combinedRules + }; } return workflow; diff --git a/test/RulesEngine.UnitTest/WorkflowsToInjectMutationTest.cs b/test/RulesEngine.UnitTest/WorkflowsToInjectMutationTest.cs new file mode 100644 index 00000000..8d6648f4 --- /dev/null +++ b/test/RulesEngine.UnitTest/WorkflowsToInjectMutationTest.cs @@ -0,0 +1,90 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using RulesEngine.Models; +using System; +using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; +using System.Linq; +using System.Threading.Tasks; +using Xunit; + +namespace RulesEngine.UnitTest +{ + [Trait("Category", "Unit")] + [ExcludeFromCodeCoverage] + public class WorkflowsToInjectMutationTest + { + [Fact] + public async Task WorkflowsToInject_MultipleExecutionsAndInputSignatures_DoesNotMutateRulesOrThrowDuplicateKey() + { + var baseWorkflow = new Workflow + { + WorkflowName = "BaseWorkflow", + Rules = new List + { + new Rule + { + RuleName = "InjectedRule1", + Expression = "input1.Value > 0" + } + } + }; + + var childWorkflow = new Workflow + { + WorkflowName = "ChildWorkflow", + WorkflowsToInject = new List { "BaseWorkflow" }, + Rules = new List + { + new Rule + { + RuleName = "ChildRule1", + Expression = "input1.Value < 100" + } + } + }; + + var engine = new RulesEngine(new[] { baseWorkflow, childWorkflow }); + + var firstRun = await engine.ExecuteAllRulesAsync("ChildWorkflow", new RuleParameter("input1", new { Value = 10 })); + Assert.Equal(2, firstRun.Count); + Assert.All(firstRun, r => Assert.True(r.IsSuccess)); + + // Original workflow object's Rules collection should not have been mutated in-place. + Assert.Single(childWorkflow.Rules); + + // Second execution with a different input parameter signature triggers RegisterRule again. + // Previously, GetWorkflow mutated workflow.Rules on every call, duplicating InjectedRule1 + // and causing dictFunc.Add to throw ArgumentException ("An item with the same key has already been added"). + var secondRun = await engine.ExecuteAllRulesAsync("ChildWorkflow", + new RuleParameter("input1", new { Value = 10 }), + new RuleParameter("input2", new { Extra = "ok" })); + Assert.Equal(2, secondRun.Count); + Assert.All(secondRun, r => Assert.True(r.IsSuccess)); + } + + [Fact] + public async Task WorkflowsToInject_CircularReference_ThrowsInvalidOperationExceptionInsteadOfStackOverflow() + { + var wfA = new Workflow + { + WorkflowName = "WfA", + WorkflowsToInject = new List { "WfB" }, + Rules = new List { new Rule { RuleName = "R1", Expression = "true" } } + }; + + var wfB = new Workflow + { + WorkflowName = "WfB", + WorkflowsToInject = new List { "WfA" }, + Rules = new List { new Rule { RuleName = "R2", Expression = "true" } } + }; + + var engine = new RulesEngine(new[] { wfA, wfB }); + + await Assert.ThrowsAsync(async () => + await engine.ExecuteAllRulesAsync("WfA", new RuleParameter("input1", new { Value = 1 }))); + } + } +}