fix: validate loop-scoped variables correctly in template validation #61
Conversation
Template validation now correctly handles variables inside foreach loops.
Previously, variables like {{Name}} inside {{#foreach Items}} were
incorrectly flagged as "Missing Variable" because validation only checked
the root data dictionary.
Changes:
- Add recursive ValidatePlaceholdersInScope() for loop-aware validation
- Aggregate properties from ALL items in collections (handles heterogeneous data)
- Add Warnings support to ValidationResult for empty collection cases
- Add 7 new tests for loop-scoped validation scenarios
Resolves customer feedback about unusable validation results.
Verify that validation correctly resolves a variable name when it exists both at global scope and inside loop items.
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical validation bug where variables inside {{#foreach}} loops were incorrectly flagged as missing. The fix introduces recursive validation with loop context awareness and adds a warnings system for non-blocking issues like empty collections.
Key Changes
- Introduced
ValidationWarningclass and warnings system for non-critical validation issues - Implemented recursive
ValidatePlaceholdersInScope()method that maintains loop context via a stack - Added property aggregation from collection items to validate loop-scoped variables correctly
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| ValidationResult.cs | Added Warnings property, ValidationWarning class, and ValidationWarningType enum to support non-blocking validation feedback |
| DocumentTemplateProcessor.cs | Refactored validation logic to recursively process loop scopes with proper variable resolution, including collection property aggregation |
| ValidationIntegrationTests.cs | Added 7 comprehensive tests covering loop-scoped validation, nested loops, empty collections, global variable shadowing, and heterogeneous collections |
| CLAUDE.md | Added development workflow documentation section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (collection is not IEnumerable enumerable) | ||
| { | ||
| return properties; | ||
| } |
There was a problem hiding this comment.
Strings implement IEnumerable, so if a collection value is a string, this method will iterate over its characters instead of treating it as an invalid collection. Add a check to exclude strings before checking for IEnumerable, or check for IEnumerable instead.
| catch (Exception ex) | ||
|
|
||
| // 4. Find placeholders in current scope (exclude nested loop content) | ||
| HashSet<string> placeholders = FindPlaceholdersInElements(elements, loopBlocks); |
There was a problem hiding this comment.
The FindPlaceholdersInElements call only excludes regular loop blocks but not table row loops. This could result in placeholders inside table row loops being validated twice: once recursively at line 474, and again at lines 489-506. This may cause duplicate error messages for missing variables. Consider collecting table loops and passing them to FindPlaceholdersInElements as well, or refactor to handle both loop types consistently.
|
|
||
| if (collection == null) | ||
| { | ||
| // Collection not found - it will be flagged as missing in the placeholder check |
There was a problem hiding this comment.
The comment states that if collection is null, "it will be flagged as missing in the placeholder check", but the code continues without flagging it immediately. If the collection name is itself a loop-scoped property (line 524 returns null), it won't be flagged as missing because it exists in the loop scope properties. Consider adding explicit handling for this case or clarifying the comment.
| // Collection not found - it will be flagged as missing in the placeholder check | |
| // Collection not found - flag it as missing and skip inner validation for this loop | |
| missingVariables.Add(loop.CollectionName); |
| } | ||
|
|
||
| // Nested property (e.g., "Address.City" - check if "Address" exists) | ||
| string rootProperty = placeholder.Split('.')[0]; |
There was a problem hiding this comment.
The Split method creates a new array on every call, which could be inefficient in a validation loop that processes many placeholders. Consider using IndexOf to find the first dot and substring extraction instead, or caching the split results if the same placeholder is checked multiple times.
| string rootProperty = placeholder.Split('.')[0]; | |
| int dotIndex = placeholder.IndexOf('.'); | |
| string rootProperty = dotIndex >= 0 ? placeholder.Substring(0, dotIndex) : placeholder; |
|
|
||
| foreach (string placeholder in allPlaceholders) | ||
| // Clear and re-populate placeholders with proper scoping | ||
| allPlaceholders.Clear(); |
There was a problem hiding this comment.
Clearing allPlaceholders here discards the placeholders collected in steps 1-4 (conditional blocks, loop blocks, table loops, and regular placeholders). While ValidatePlaceholdersInScope will re-add them, this approach duplicates work and could miss placeholders that were found in the earlier steps but not in the recursive validation. Consider refactoring to avoid clearing and re-populating, or ensure the recursive method finds all the same placeholders.
| // Re-add conditional variables (they were already extracted in step 1) | ||
| foreach (ConditionalBlock block in conditionalBlocks) | ||
| { | ||
| // Skip loop metadata placeholders (@index, @first, etc.) as they're generated at runtime | ||
| if (placeholder.StartsWith("@")) | ||
| string condition = block.ConditionExpression; | ||
| string[] parts = condition.Split(new[] { ' ', '(', ')', '!', '>', '<', '=', '&', '|' }, StringSplitOptions.RemoveEmptyEntries); | ||
| foreach (string part in parts) | ||
| { | ||
| continue; | ||
| if (part != "and" && part != "or" && part != "not" && | ||
| part != "eq" && part != "ne" && part != "lt" && part != "gt" && | ||
| part != "lte" && part != "gte" && !string.IsNullOrWhiteSpace(part)) | ||
| { | ||
| // Only add if it's not a string literal (enclosed in quotes) | ||
| string trimmed = part.Trim().Trim('"', '\''); | ||
| if (trimmed == part.Trim()) | ||
| { | ||
| allPlaceholders.Add(trimmed); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The conditional variable extraction logic here duplicates work already done in step 1 (lines around 240-270). This re-extraction is inefficient and increases maintenance burden. Consider storing the extracted conditional variables from step 1 and reusing them here, rather than re-parsing all conditional blocks.
| else | ||
| { | ||
| // POCO - get public properties | ||
| foreach (System.Reflection.PropertyInfo prop in item.GetType().GetProperties()) | ||
| { | ||
| properties.Add(prop.Name); | ||
| } | ||
| } |
There was a problem hiding this comment.
Using reflection (GetProperties) within a loop can be expensive, especially for collections with many items of the same type. Consider caching property information by type, or at minimum, detect when all items are the same type and only reflect once.
| foreach (string part in parts) | ||
| { | ||
| continue; | ||
| if (part != "and" && part != "or" && part != "not" && | ||
| part != "eq" && part != "ne" && part != "lt" && part != "gt" && | ||
| part != "lte" && part != "gte" && !string.IsNullOrWhiteSpace(part)) | ||
| { | ||
| // Only add if it's not a string literal (enclosed in quotes) | ||
| string trimmed = part.Trim().Trim('"', '\''); | ||
| if (trimmed == part.Trim()) | ||
| { | ||
| allPlaceholders.Add(trimmed); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (OpenXmlElement element in elements) | ||
| { | ||
| if (element is Table table) | ||
| { | ||
| try | ||
| { | ||
| IReadOnlyList<LoopBlock> tableLoops = LoopDetector.DetectTableRowLoops(table); | ||
| foreach (LoopBlock loop in tableLoops) | ||
| { | ||
| allPlaceholders.Add(loop.CollectionName); | ||
|
|
||
| object? collection = ResolveCollectionFromScope(loop.CollectionName, loopStack, data, resolver); | ||
|
|
||
| if (collection == null) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| // Skip relative path placeholders (e.g., .Name, .Address.City) as they're context-dependent | ||
| // These can only be resolved during processing when loop context exists | ||
| if (placeholder.StartsWith(".")) | ||
| HashSet<string> aggregatedProperties = AggregatePropertiesFromCollection(collection); | ||
|
|
||
| if (aggregatedProperties.Count == 0) | ||
| { | ||
| warnings.Add(ValidationWarning.Create( | ||
| ValidationWarningType.EmptyLoopCollection, | ||
| $"Collection '{loop.CollectionName}' is empty. Variables inside this loop could not be validated.")); | ||
| continue; | ||
| } | ||
|
|
||
| // Try to resolve the placeholder | ||
| if (!resolver.TryResolveValue(data, placeholder, out _)) | ||
| { | ||
| missingVariables.Add(placeholder); | ||
|
|
||
| // Add a validation error for missing variable | ||
| errors.Add(ValidationError.Create( | ||
| ValidationErrorType.MissingVariable, | ||
| $"Variable '{placeholder}' is referenced in the template but not provided in the data.")); | ||
| } | ||
| loopStack.Push((loop.CollectionName, aggregatedProperties)); | ||
| ValidatePlaceholdersInScope(loop.ContentElements, loopStack, data, allPlaceholders, missingVariables, warnings, errors, resolver); | ||
| loopStack.Pop(); | ||
| } | ||
| } | ||
| catch (InvalidOperationException) | ||
| { | ||
| // Table loop detection errors are captured elsewhere | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (ConditionalBlock block in conditionalBlocks) | ||
| { | ||
| // Skip loop metadata placeholders (@index, @first, etc.) as they're generated at runtime | ||
| if (placeholder.StartsWith("@")) | ||
| string condition = block.ConditionExpression; | ||
| string[] parts = condition.Split(new[] { ' ', '(', ')', '!', '>', '<', '=', '&', '|' }, StringSplitOptions.RemoveEmptyEntries); | ||
| foreach (string part in parts) | ||
| { | ||
| continue; | ||
| if (part != "and" && part != "or" && part != "not" && | ||
| part != "eq" && part != "ne" && part != "lt" && part != "gt" && | ||
| part != "lte" && part != "gte" && !string.IsNullOrWhiteSpace(part)) | ||
| { | ||
| // Only add if it's not a string literal (enclosed in quotes) | ||
| string trimmed = part.Trim().Trim('"', '\''); | ||
| if (trimmed == part.Trim()) | ||
| { | ||
| allPlaceholders.Add(trimmed); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
- Add WarnOnEmptyLoopCollections option to PlaceholderReplacementOptions (default: true) to control whether empty collections produce warnings - Add explanatory comment for nested property resolution limitation in static validation - Pass options through DocumentTemplateProcessor to TemplateValidator
- Fix string IEnumerable bug by excluding strings before iterating - Fix duplicate validation by collecting all loops (regular + table row) - Optimize Split with IndexOf for nested property checking - Cache reflection PropertyInfo by type for better performance - Remove unnecessary clearing and re-extraction of placeholders - Use excludeLiterals=true in initial conditional variable extraction - Apply LINQ improvements (OfType<T>, static operator keywords HashSet)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| private static readonly HashSet<string> _operatorKeywords = new HashSet<string>(StringComparer.Ordinal) | ||
| { | ||
| "and", "or", "not", "eq", "ne", "lt", "gt", "lte", "gte" |
There was a problem hiding this comment.
The operator keywords list includes "eq", "ne", "lt", "gt", "lte", and "gte", but based on ConditionalEvaluator.cs, the actual supported operators are "and", "or", "not", "=", "!=", ">", "<", ">=", and "<=". The keywords "eq", "ne", "lt", "gt", "lte", and "gte" don't appear to be supported operators in the conditional evaluation system. These should be removed from the _operatorKeywords set to accurately reflect the supported operator syntax.
| "and", "or", "not", "eq", "ne", "lt", "gt", "lte", "gte" | |
| "and", "or", "not" |
| if (excludeLiterals) | ||
| { | ||
| // Only add if it's not a string literal (enclosed in quotes) | ||
| string trimmed = part.Trim('"', '\''); | ||
| if (trimmed == part) | ||
| { | ||
| placeholders.Add(trimmed); | ||
| } |
There was a problem hiding this comment.
The string literal detection logic only handles ASCII quotes ('"' and ''') but doesn't account for typographic/curly quotes (U+201C, U+201D, U+2018, U+2019, etc.) that Word automatically converts. This means that in conditions like Status = "Active" (with curly quotes), the value "Active" (still wrapped in curly quotes after Trim) would be incorrectly added as a placeholder. Consider normalizing quotes before checking, similar to how ConditionalEvaluator.NormalizeQuotes() handles this.
| if (collection == null) | ||
| { | ||
| // Collection not found in global scope or is a loop-scoped property (nested collection). | ||
| // For loop-scoped properties, we skip validation since we can't resolve them statically. | ||
| // For truly missing collections, they will be flagged when validating placeholders | ||
| // if they appear as {{CollectionName}} somewhere in the template. | ||
| return; | ||
| } |
There was a problem hiding this comment.
When a loop collection cannot be resolved (collection == null), the code returns early without validating whether the collection exists in the data. This is a regression from the previous implementation which validated all collection names. A missing collection should be flagged as a validation error. The current logic only skips validation for loop-scoped nested collections (where the collection is a property of a parent loop item), but it should still flag truly missing global collections as errors.
| List<OpenXmlElement> elements = body.Elements<OpenXmlElement>().ToList(); | ||
|
|
||
| // 1. Validate conditionals and collect variables from conditions | ||
| IReadOnlyList<ConditionalBlock> conditionalBlocks = ValidateConditionals(elements, allPlaceholders, errors); |
There was a problem hiding this comment.
This assignment to conditionalBlocks is useless, since its value is never read.
| IReadOnlyList<ConditionalBlock> conditionalBlocks = ValidateConditionals(elements, allPlaceholders, errors); | |
| _ = ValidateConditionals(elements, allPlaceholders, errors); |
- Remove unsupported operator keywords (eq, ne, lt, gt, lte, gte) - Add NormalizeQuotes() to handle typographic quotes from Word - Flag missing global collections as validation errors (fix regression) - Replace unused conditionalBlocks variable with discard
Summary
{{#foreach}}loops{{Name}}inside{{#foreach Items}}were incorrectly flagged as "Missing Variable"Changes
Warningsproperty andValidationWarningclassValidatePlaceholdersInScope()with loop context awarenessTest plan