Skip to content

Conversation

@rodion-m
Copy link

@rodion-m rodion-m commented Oct 25, 2025

Fixes #64
Fixes #562

Key Features Implemented
✅ Collection Strategy: Collects ALL missing variables before throwing exception
✅ No Partial Output: Uses buffering to prevent partial renders on error
✅ Backward Compatible: StrictVariables defaults to false
✅ Deduplication: HashSet prevents duplicate missing variables
✅ Comprehensive: Tracks missing variables in all contexts (scopes, properties, nested access)
✅ Clear Error Messages: Exception message lists all missing variables
✅ Reusability: Clears tracking between renders for context reuse

Testing Coverage
The test suite includes 30+ tests covering:

Default behavior verification
Simple and complex variable access patterns
Property access on objects
Nested property access
Control structures (if, for, case)
Filters and assignments
Model fallback scenarios
Edge cases and error conditions

@rodion-m
Copy link
Author

@sebastienros Hi, could you please approve it? This feature is really required,

@sebastienros
Copy link
Owner

I am wondering if instead of checking in GetValue() we should instead have a special FluidValue.Undefined that would be returned. This way we would just record when it's used during rendering, it would register itself in the context, and after everything is done we can check the context.

That would improve two things:

  • Scope.GetValue would not need TemplateContext as a new argument, though we can think about it separately.
  • A template might want to check the variable exists (blank/empty) because it might or might not be defined in some cases as an expectation. If it's invoked and it doesn't exist, then that's an issue and the Undefined value usage would be detected. But if we check it and don't invoke it in any way then that would be fine, no exception.

Other concerns:

  • A personal one as I have a branch removing MemberAccessStrategy, so that will be more work for me when it's merge, but it's part of the game.
  • Buffering. It that is what we need to do then maybe we should be able to extend it in TemplateOptions and provide a custom BufferWriterFactory to provide the ones used for this purpose and the default implementation to do something sensible to prevent allocations. The current way allocates a lot, and in ways that it could be detrimental to existing perf. Obviously it's opt-in to that mitigates the issue, but the implication is not obvious from a user's perspective.

One more thing actually, what if the context had an event handler like we already have with TemplateOptions.AssignedDelegate Assigned { get; set; }, this way when the Undefined value is executed we can continue, and users can register this event and decide to throw when the template has been rendered. Then instead of catching an exception, and us needing to buffer, let then detect after rendering. If they don't want to render anything then up to them to buffer the result, or evaluate the template targeting a null-writer if they just want to check the values, for instance once per template. Users could even just decide to throw on the first undefined variable to make things simpler (optimistic approach).

If you don't see any major issue with these suggestions: event + delegate buffering to user then I think that should be the way to go. Less responsibility, and potentially more flexibility.

@rodion-m
Copy link
Author

@sebastienros Please take a look to the second commit. Did I get your idea right?

@sebastienros
Copy link
Owner

@rodion-m I updated your PR to my preferences, I hope you won't mind. It's simpler, all your tests are passing (though not using the exception directly) and added a few more. The main idea is to reuse Nil since it's actuall what it represents. And let users decide what should happen when a value is missing: throw, list unique variables, ...
The logic can be defined on the singleton TemplateOption or each TemplateContext.
If you are fine with the changes I'll merge.

@sebastienros sebastienros requested a review from Copilot November 1, 2025 22:09
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for handling undefined variables in Fluid templates by introducing an Undefined delegate that can be configured on TemplateOptions and TemplateContext. This allows developers to track missing variables during rendering, provide custom fallback values, or log undefined accesses for debugging purposes.

  • Adds UndefinedDelegate to TemplateOptions and TemplateContext for tracking undefined variable access
  • Updates ObjectValueBase and MemberExpression to invoke the Undefined delegate when variables cannot be resolved
  • Refactors code in ObjectValueBase to use switch expressions and removes unnecessary comments
  • Includes comprehensive documentation in README with three usage examples
  • Adds 37 test cases covering various scenarios of undefined variable handling

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
README.md Adds comprehensive documentation section explaining the Undefined delegate feature with three practical examples
Fluid/TemplateOptions.cs Introduces UndefinedDelegate type and Undefined property, reorders using statements
Fluid/TemplateContext.cs Adds Undefined property and initializes it from options, changes NilValue to EmptyValue for null values
Fluid/Values/ObjectValueBase.cs Updates member access logic to invoke Undefined delegate, refactors to switch expressions, reorders using statements
Fluid/Ast/MemberExpression.cs Adds Undefined delegate invocation for top-level undefined variables
Fluid/Scope.cs Reorders using statements
Fluid/Parser/FluidTemplate.cs Reorders using statements
Fluid.Tests/StrictVariableTests.cs Adds comprehensive test suite with 37 test cases validating undefined variable handling
Comments suppressed due to low confidence (1)

Fluid/Values/ObjectValueBase.cs:120

  • When target becomes null during nested property traversal and context.Undefined is set, the delegate is never invoked. The method should call context.Undefined.Invoke(string.Join(\".\", segments)) before returning NilValue.Instance to track this undefined access.
                if (target == null)
                {
                    return NilValue.Instance;
                }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

sebastienros and others added 3 commits November 1, 2025 15:16
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@sebastienros
Copy link
Owner

After chatting with Copilot I re-introduced undefined. I checked Ruby and Liquid and Nil represents both undefined and null, and my changed involved converting null values to empty which actually represents and empty string or array, so not something that is either undefined or null.

@rodion-m
Copy link
Author

rodion-m commented Nov 2, 2025

Hi @sebastienros ! Thank you for the improvements. I just didn't get why did we miss StrictVariables option. Initially, it was a goal to introduce this option that throws an exception in case of missing variables - for my opinion it's very useful to have such an option. I'm afraid that missing StrictVariables option will hurt API of this lib, cause it's a very convenient way to add a strong control to a template and we'll miss it. Thoughts?

@sebastienros
Copy link
Owner

It's about flexibility. Users won't have to catch an exception, unless thy want to (like you seem to), they can also ignore specific missing variables, or just decide to log them and still get the template running fine, and there is no buffering involved with any of these options.

In your case, here is the code you will have to to write to create exception:

var context = new TemplateContext();
context.Undefined = name => throw new StrictVariableException($"Undefined variable: {name}");
await template.RenderAsync(text, context);
}

It's not much more work, but users have more flexibility. You can decide to put the code in the TemplateOptions and reuse it too if you just want to throw as soon as one is not found. Or use a HashSet and the TemplateContext if you want to list then only once and clear it every time a template is rendered, and not exception will be involved, just check the list once the template is rendered.

@rodion-m
Copy link
Author

rodion-m commented Nov 3, 2025

Yeah, I got it. Anyway, what do you think if we add an option like ThrowOnUndefinedVariable that will just init Undefined delegate like you showed? I really believe that it's important to have such an option from a UX perspective.

If a user set both these options - we can throw an exception.

Or an alternative:

Just add a descendant StrictTemplateOptions without Undefined delegate all (it should be preset). This solution will be more type safe.

@sebastienros thoughts?

@sebastienros sebastienros changed the title #64: Ensure variables completeness with StrictVariables option Detect undefined variables Nov 3, 2025
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.

If a property is not provided, then its value is null / blank. Can we instead keep the 'placeholder'? Validate context model calls when rendering

3 participants