Fix WorkflowsToInject mutating cached Workflow.Rules in-place and guard against cycles

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()
         /// <returns>Workflows.</returns>
         /// <exception cref="Exception">Could not find injected Workflow: {wfname}</exception>
         public Workflow GetWorkflow(string workflowName)
+        {
+            return GetWorkflow(workflowName, null);
+        }
+
+        private Workflow GetWorkflow(string workflowName, HashSet<string> 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<string>(StringComparer.Ordinal);
+                    if (!visited.Add(workflowName))
                     {
-                        workflow.Rules = new List<Rule>();
+                        throw new InvalidOperationException($"Circular WorkflowsToInject reference detected involving workflow: {workflowName}");
                     }
+
+                    var combinedRules = workflow.Rules?.ToList() ?? new List<Rule>();
                     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;

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<Rule>
+                {
+                    new Rule
+                    {
+                        RuleName = "InjectedRule1",
+                        Expression = "input1.Value > 0"
+                    }
+                }
+            };
+
+            var childWorkflow = new Workflow
+            {
+                WorkflowName = "ChildWorkflow",
+                WorkflowsToInject = new List<string> { "BaseWorkflow" },
+                Rules = new List<Rule>
+                {
+                    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<string> { "WfB" },
+                Rules = new List<Rule> { new Rule { RuleName = "R1", Expression = "true" } }
+            };
+
+            var wfB = new Workflow
+            {
+                WorkflowName = "WfB",
+                WorkflowsToInject = new List<string> { "WfA" },
+                Rules = new List<Rule> { new Rule { RuleName = "R2", Expression = "true" } }
+            };
+
+            var engine = new RulesEngine(new[] { wfA, wfB });
+
+            await Assert.ThrowsAsync<InvalidOperationException>(async () =>
+                await engine.ExecuteAllRulesAsync("WfA", new RuleParameter("input1", new { Value = 1 })));
+        }
+    }
+}