Skip to content

Validate duplicate param names and input/global collisions with clear errors#743

Merged
YogeshPraj merged 2 commits into
microsoft:mainfrom
YogeshPraj:fix/duplicate-param-name-validation
Jun 11, 2026
Merged

Validate duplicate param names and input/global collisions with clear errors#743
YogeshPraj merged 2 commits into
microsoft:mainfrom
YogeshPraj:fix/duplicate-param-name-validation

Conversation

@YogeshPraj

Copy link
Copy Markdown
Contributor

Summary

Cleans up three cryptic "An item with the same key has already been added. Key: …" failures users hit when they accidentally introduce a name collision. Surfaced while triaging stale PR #402 — the underlying concerns it identified are real bugs even though that PR's proposed approach wasn't a fit.

The three cases

Case When Was Now
Two GlobalParams in the same workflow with the same Name Detected at AddWorkflow if eagerly used; otherwise at execution Dictionary.ToDictionary throws "An item with the same key has already been added. Key: dup" WorkflowsValidator throws RuleValidationException("Duplicate GlobalParam name 'dup'. GlobalParam names must be unique within a workflow.")
Two LocalParams on the same rule with the same Name Same Same cryptic error RuleValidator throws RuleValidationException("Duplicate LocalParam name 'dup'. …")
Caller passes RuleParameter("foo", …) against a workflow with GlobalParam("foo", …) At execution Same cryptic error from result-tree construction RulesEngine.AppendGlobals throws RuleException("GlobalParam name 'foo' collides with an input RuleParameter of the same name. Rename either the global or the input to disambiguate.") — flowed through the existing per-rule error-surfacing path so each rule's ExceptionMessage carries the explanation

Why this shape

  • Compile-time validation for duplicate workflow definitions (cases 1 and 2) — these are author errors and should be caught when the workflow is loaded, before execution starts. Fits cleanly into the existing FluentValidation pipeline (WorkflowsValidator / RuleValidator).
  • Run-time check for input/global collisions (case 3) — can only be detected when caller inputs are known. The check lives where the values are actually merged (AppendGlobals) and uses the same try/catch surfacing pattern other scoped-params errors already use in ExecuteAllRuleByWorkflow, so the behavior is consistent: each rule's ExceptionMessage explains the collision and IsSuccess = false.
  • Messages live in HelperFunctions/Constants.cs alongside the existing validator strings — same convention.

Not in this PR

The other claims in PR #402 either don't reproduce on current main (Dynamic.Core new (…) syntax works without AllowNewToEvaluateAnyType) or are design debates rather than bugs (chained-rule result visibility, PostActions timing).

Test plan

  • New test file DuplicateParamNameValidationTest.cs covering:
    • Duplicate GlobalParamsRuleValidationException with "duplicate" + offending name
    • Duplicate LocalParams → same
    • Caller input collides with global → per-rule ExceptionMessage contains "collide" + offending name; does NOT contain "same key has already been added"
    • Distinct names → still execute successfully (regression guard)
  • All 174 unit tests pass on net6.0 / net8.0 / net9.0 / net10.0
  • No public API surface change; behavior change is strictly more helpful errors on previously failing inputs

… errors

PR microsoft#402 surfaced two cases where the engine threw cryptic
"An item with the same key has already been added. Key: …" deep inside
RuleResultTree construction:

  1. Two GlobalParams in the same workflow sharing a name.
  2. Two LocalParams on the same rule sharing a name.
  3. A caller-supplied RuleParameter whose Name matches a workflow's
     GlobalParam name.

All three now surface clear, actionable errors:

- WorkflowsValidator catches case 1 at AddWorkflow time and throws
  RuleValidationException with "Duplicate GlobalParam name '<name>'".
- RuleValidator catches case 2 similarly with
  "Duplicate LocalParam name '<name>'".
- RulesEngine.AppendGlobals catches case 3 at execution time and throws
  RuleException with "GlobalParam name '<name>' collides with an input
  RuleParameter of the same name". The exception flows through the
  existing per-rule error-surfacing path in ExecuteAllRuleByWorkflow, so
  callers see the explanatory message on each rule's ExceptionMessage —
  the same pattern other scoped-params errors already use.

Constants in Constants.cs make the messages reusable and tested.

Regression test: DuplicateParamNameValidationTest covers all three
failure modes plus a sanity case where distinct names continue to work.

All 174 unit tests pass on net6 / net8 / net9 / net10.
@YogeshPraj YogeshPraj force-pushed the fix/duplicate-param-name-validation branch from 5fff2ef to fa2f73e Compare June 11, 2026 19:06
@YogeshPraj YogeshPraj merged commit e7c7313 into microsoft:main Jun 11, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants