Skip to content

Conversation

willibrandon
Copy link

@willibrandon willibrandon commented Aug 28, 2025

Fixes #6

Implements eager validation for expression syntax errors, making them non-throwing in the TryCompile path.

Changes

  • Added ExpressionValidationException to handle validation errors internally
  • Invalid regular expressions in IsMatch()/IndexOfMatch() are detected during compilation
  • Unknown function names are reported gracefully instead of throwing
  • CI modifier usage logs warnings for unsupported functions (maintains backward compatibility)
  • Single validation error per exception

Testing

  • Added test suite in ExpressionValidationTests.cs
  • All existing tests pass
  • Backward compatibility maintained - Compile() still throws for invalid expressions

…x errors

- Add ExpressionValidator to validate expressions before compilation
- Validate regex patterns in IsMatch()/IndexOfMatch() without throwing
- Detect unknown function names gracefully
- Validate CI modifier usage on functions that support it
- Return validation errors via TryCompile() instead of throwing
- Add test suite for validation scenarios
- Fix ${regex} to {regex} in log message (was outputting literal text)
- Rename CaseSensitiveFunctions to CaseInsensitiveCapableFunctions
- Add test for IndexOfMatch with invalid regex in Compile()
@nblumhardt
Copy link
Member

Howdy! Thanks for checking this out.

Because NameResolver makes built-in functions pluggable, the set of ci capable operators isn't fixed, so I don't think listing them in the validator will work.

I ran through the current expression compiler implementation; since compilation failures really are on the error path, I'm coming around to thinking we should just lean on a custom ExpressionValidationException type, catch this where you have catch (ArgumentException ...) in TryCompileImpl(), and then work through the rest of the codebase to either throw this exception type where appropriate, or wrap exceptions from components we don't control.

From the outside, i.e. to the tests, I think the behavior would be almost identical.

I don't really like exceptions at the best of times :-) but skimming through the LinqExpressionCompiler type and a few others, the idea of maintaining a separate validator in parallel seems a bit daunting, so perhaps a blessed validation exception type is the lesser of the two evils?

@willibrandon
Copy link
Author

Thanks for the feedback! I understand the concerns about the pluggable NameResolver and parallel validation.

A couple questions before I rework this:

  1. For CI modifier validation - I can check at runtime if a resolved function supports StringComparison by inspecting the MethodInfo parameters, similar to how LinqExpressionCompiler already does. Should I implement this dynamic check, or skip CI validation entirely?

  2. Should I convert all existing ArgumentExceptions in the compiler to ExpressionValidationException, or just focus on the three cases from issue Eager/non-throwing checks for more expression syntax errors #6 (regex, unknown functions, invalid CI)?

  3. Do you want the ExpressionValidationException to support multiple errors like the current approach, or one error per exception?

Happy to rework this with the exception-based approach!

@nblumhardt
Copy link
Member

Thanks for taking another look.

  1. The NameResolver lookups only happen at compile time, so the cost is fairly trivial; by runtime (executing the expression) the whole thing's compiled via the LINQ expression tree and there won't be any further lookups. I think adding the check during name resolution seems worthwhile 👍

  2. Perhaps to keep things moving forward, we stick to the cases in Eager/non-throwing checks for more expression syntax errors #6, and leave the others for further work? Could be a lot of back-and-forth otherwise.

  3. I think since we can only get a single error out of TryCompile() (and out of ExpressionTemplate.TryParse()), carrying a single error on the exception type might be reasonable. The exception-driven approach makes collecting multiple errors more complicated and might not be worthwhile at this point. Since the exception type can remain internal (assuming we catch it internally in the APIs where it can be thrown) it would be possible to modify its interface to add multiple error support in the future, anyway.

- Replace pre-validation with exception-based approach
- Invalid regex and unknown functions now return friendly errors via TryCompile
- CI modifier on non-supporting functions logs warnings instead of throwing (backward compatibility)
- Use dynamic StringComparison parameter detection instead of hard-coded function lists
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.

Eager/non-throwing checks for more expression syntax errors
2 participants