diff --git a/src/RulesEngine/HelperFunctions/Constants.cs b/src/RulesEngine/HelperFunctions/Constants.cs index a6a3ae5b..b682c536 100644 --- a/src/RulesEngine/HelperFunctions/Constants.cs +++ b/src/RulesEngine/HelperFunctions/Constants.cs @@ -21,6 +21,9 @@ public static class Constants public const string LAMBDA_EXPRESSION_EXPRESSION_NULL_ERRMSG = "Expression cannot be null or empty when RuleExpressionType is LambdaExpression"; public const string LAMBDA_EXPRESSION_OPERATOR_ERRMSG = "Cannot use Operator field when RuleExpressionType is LambdaExpression"; public const string LAMBDA_EXPRESSION_RULES_ERRMSG = "Cannot use Rules field when RuleExpressionType is LambdaExpression"; + public const string DUPLICATE_GLOBAL_PARAM_NAME_ERRMSG = "Duplicate GlobalParam name '{0}'. GlobalParam names must be unique within a workflow."; + public const string DUPLICATE_LOCAL_PARAM_NAME_ERRMSG = "Duplicate LocalParam name '{0}'. LocalParam names must be unique within a rule."; + public const string GLOBAL_PARAM_INPUT_COLLISION_ERRMSG = "GlobalParam name '{0}' collides with an input RuleParameter of the same name. Rename either the global or the input to disambiguate."; } } diff --git a/src/RulesEngine/RulesEngine.cs b/src/RulesEngine/RulesEngine.cs index 391898ec..27bf8c08 100644 --- a/src/RulesEngine/RulesEngine.cs +++ b/src/RulesEngine/RulesEngine.cs @@ -190,6 +190,18 @@ private static RuleParameter[] AppendGlobals(RuleParameter[] ruleParameters, Fun } var inputs = ruleParameters.Select(c => c.Value).ToArray(); var evaluated = globalParamsDelegate(inputs); + + // Catch input/global name collisions here with a clear error instead of letting + // Helpers.ToResultTree's ToDictionary throw the cryptic "key already added" message. + var inputNames = new HashSet(ruleParameters.Select(c => c.Name), StringComparer.Ordinal); + foreach (var key in evaluated.Keys) + { + if (inputNames.Contains(key)) + { + throw new RuleException(string.Format(Constants.GLOBAL_PARAM_INPUT_COLLISION_ERRMSG, key)); + } + } + var globals = evaluated.Select(kv => new RuleParameter(kv.Key, kv.Value)); return ruleParameters.Concat(globals).ToArray(); } diff --git a/src/RulesEngine/Validators/RuleValidator.cs b/src/RulesEngine/Validators/RuleValidator.cs index 8ce43144..973222b2 100644 --- a/src/RulesEngine/Validators/RuleValidator.cs +++ b/src/RulesEngine/Validators/RuleValidator.cs @@ -33,6 +33,14 @@ public RuleValidator() }); }); RegisterExpressionTypeRules(); + + // Surface duplicate LocalParam names with a clear message instead of a cryptic + // "An item with the same key has already been added" failure later at execution. + RuleFor(c => c) + .Must(rule => WorkflowsValidator.FindDuplicateName(rule.LocalParams?.Select(p => p?.Name)) == null) + .WithMessage(rule => string.Format( + Constants.DUPLICATE_LOCAL_PARAM_NAME_ERRMSG, + WorkflowsValidator.FindDuplicateName(rule.LocalParams?.Select(p => p?.Name)))); } private void RegisterExpressionTypeRules() diff --git a/src/RulesEngine/Validators/WorkflowRulesValidator.cs b/src/RulesEngine/Validators/WorkflowRulesValidator.cs index 3c46d31b..35a1e59c 100644 --- a/src/RulesEngine/Validators/WorkflowRulesValidator.cs +++ b/src/RulesEngine/Validators/WorkflowRulesValidator.cs @@ -19,6 +19,26 @@ public WorkflowsValidator() var ruleValidator = new RuleValidator(); RuleForEach(c => c.Rules).SetValidator(ruleValidator); }); + + // Surface duplicate GlobalParam names with a clear message instead of a cryptic + // "An item with the same key has already been added" failure later at execution. + RuleFor(c => c) + .Must(workflow => FindDuplicateName(workflow.GlobalParams?.Select(p => p?.Name)) == null) + .WithMessage(workflow => string.Format( + Constants.DUPLICATE_GLOBAL_PARAM_NAME_ERRMSG, + FindDuplicateName(workflow.GlobalParams?.Select(p => p?.Name)))); + } + + internal static string FindDuplicateName(System.Collections.Generic.IEnumerable names) + { + if (names == null) return null; + var seen = new System.Collections.Generic.HashSet(System.StringComparer.Ordinal); + foreach (var name in names) + { + if (string.IsNullOrEmpty(name)) continue; + if (!seen.Add(name)) return name; + } + return null; } } } diff --git a/test/RulesEngine.UnitTest/DuplicateParamNameValidationTest.cs b/test/RulesEngine.UnitTest/DuplicateParamNameValidationTest.cs new file mode 100644 index 00000000..f1ee52ff --- /dev/null +++ b/test/RulesEngine.UnitTest/DuplicateParamNameValidationTest.cs @@ -0,0 +1,115 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using RulesEngine.Exceptions; +using RulesEngine.Models; +using System; +using System.Diagnostics.CodeAnalysis; +using System.Threading.Tasks; +using Xunit; + +namespace RulesEngine.UnitTest +{ + [ExcludeFromCodeCoverage] + public class DuplicateParamNameValidationTest + { + // Two GlobalParams with the same Name in the same workflow used to crash deep inside + // RuleResultTree construction with "An item with the same key has already been added. + // Key: dup". This change validates the workflow at AddWorkflow time and surfaces a clear + // error pointing at the offending name. + [Fact] + public void AddWorkflow_DuplicateGlobalParamNames_ThrowsClearValidationError() + { + var workflow = new Workflow + { + WorkflowName = "wf", + GlobalParams = new[] + { + new ScopedParam { Name = "dup", Expression = "1" }, + new ScopedParam { Name = "dup", Expression = "2" } + }, + Rules = new[] { new Rule { RuleName = "R", Expression = "dup == 1" } } + }; + + var engine = new RulesEngine(); + var ex = Assert.Throws(() => engine.AddWorkflow(workflow)); + Assert.Contains("dup", ex.Message); + Assert.Contains("duplicate", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void AddWorkflow_DuplicateLocalParamNames_ThrowsClearValidationError() + { + // Same protection for per-rule LocalParams. + var workflow = new Workflow + { + WorkflowName = "wf", + Rules = new[] { + new Rule { + RuleName = "R", + Expression = "dup == 1", + LocalParams = new[] + { + new ScopedParam { Name = "dup", Expression = "1" }, + new ScopedParam { Name = "dup", Expression = "2" } + } + } + } + }; + + var engine = new RulesEngine(); + var ex = Assert.Throws(() => engine.AddWorkflow(workflow)); + Assert.Contains("dup", ex.Message); + Assert.Contains("duplicate", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + // A caller-supplied RuleParameter whose Name collides with a workflow GlobalParam's Name + // used to surface as a cryptic "An item with the same key has already been added" deep + // inside result-tree construction. After this change the per-rule ExceptionMessage + // explicitly names the colliding identifier — same surfacing pattern as other + // scoped-params errors that ExecuteAllRuleByWorkflow already catches and reports per rule. + [Fact] + public async Task ExecuteAllRulesAsync_CallerInputCollidesWithGlobalParam_SurfacesClearCollisionError() + { + var workflow = new Workflow + { + WorkflowName = "wf", + GlobalParams = new[] + { + new ScopedParam { Name = "foo", Expression = "99" } + }, + Rules = new[] { new Rule { RuleName = "R", Expression = "foo == 99" } } + }; + var engine = new RulesEngine(new[] { workflow }); + + var results = await engine.ExecuteAllRulesAsync("wf", + new[] { RuleParameter.Create("foo", 42) }); + + Assert.False(results[0].IsSuccess); + Assert.Contains("foo", results[0].ExceptionMessage); + Assert.Contains("collide", results[0].ExceptionMessage, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("same key has already been added", results[0].ExceptionMessage); + } + + // Sanity: no collision when names are distinct (regression guard for the validation logic). + [Fact] + public async Task ExecuteAllRulesAsync_DistinctNames_StillWorks() + { + var workflow = new Workflow + { + WorkflowName = "wf", + GlobalParams = new[] + { + new ScopedParam { Name = "fromGlobal", Expression = "99" } + }, + Rules = new[] { new Rule { RuleName = "R", Expression = "fromInput == 42 && fromGlobal == 99" } } + }; + var engine = new RulesEngine(new[] { workflow }); + + var results = await engine.ExecuteAllRulesAsync("wf", + new[] { RuleParameter.Create("fromInput", 42) }); + + Assert.True(results[0].IsSuccess); + } + } +}