Fix #6: Add eager/non-throwing validation for expression syntax errors

willibrandon · willibrandon · commit 3c4bccb2b344 · 2025-08-28T15:29:40.000-07:00
- 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
diff --git a/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs b/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs
@@ -51,11 +51,19 @@ Expression TryCompileIndexOfMatch(bool ignoreCase, Expression corpus, Expression
     {
         if (regex is ConstantExpression { Constant: ScalarValue { Value: string s } })
         {
-            var opts = RegexOptions.Compiled | RegexOptions.ExplicitCapture;
-            if (ignoreCase)
-                opts |= RegexOptions.IgnoreCase;
-            var compiled = new Regex(s, opts, TimeSpan.FromMilliseconds(100));
-            return new IndexOfMatchExpression(Transform(corpus), compiled);
+            try
+            {
+                var opts = RegexOptions.Compiled | RegexOptions.ExplicitCapture;
+                if (ignoreCase)
+                    opts |= RegexOptions.IgnoreCase;
+                var compiled = new Regex(s, opts, TimeSpan.FromMilliseconds(100));
+                return new IndexOfMatchExpression(Transform(corpus), compiled);
+            }
+            catch (ArgumentException ex)
+            {
+                SelfLog.WriteLine($"Serilog.Expressions: Invalid regular expression in `IndexOfMatch()`: {ex.Message}");
+                return new CallExpression(false, Operators.OpUndefined);
+            }
         }
 
         SelfLog.WriteLine($"Serilog.Expressions: `IndexOfMatch()` requires a constant string regular expression argument; found ${regex}.");
diff --git a/src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs b/src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs
@@ -0,0 +1,144 @@
+// Copyright © Serilog Contributors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+using System.Text;
+using System.Text.RegularExpressions;
+using Serilog.Events;
+using Serilog.Expressions.Ast;
+using Serilog.Expressions.Compilation.Transformations;
+
+namespace Serilog.Expressions.Compilation.Validation;
+
+class ExpressionValidator : IdentityTransformer
+{
+    readonly NameResolver _nameResolver;
+    readonly List<string> _errors = new();
+
+    // Functions that support case-insensitive operations (have StringComparison parameter)
+    static readonly HashSet<string> CaseSensitiveFunctions = new(StringComparer.OrdinalIgnoreCase)
+    {
+        // String operations
+        Operators.OpContains,
+        Operators.OpStartsWith,
+        Operators.OpEndsWith,
+        Operators.OpIndexOf,
+        Operators.OpLastIndexOf,
+        Operators.OpReplace,
+        
+        // Pattern matching
+        Operators.OpIndexOfMatch,
+        Operators.OpIsMatch,
+        Operators.IntermediateOpLike,
+        Operators.IntermediateOpNotLike,
+        
+        // Comparisons
+        Operators.RuntimeOpEqual,
+        Operators.RuntimeOpNotEqual,
+        Operators.RuntimeOpIn,
+        Operators.RuntimeOpNotIn,
+        
+        // Element access
+        Operators.OpElementAt,
+        
+        // Special functions
+        Operators.OpUndefined  // undefined() always accepts ci
+    };
+
+    ExpressionValidator(NameResolver nameResolver)
+    {
+        _nameResolver = nameResolver;
+    }
+
+    public static bool Validate(Expression expression, NameResolver nameResolver, out string? error)
+    {
+        var validator = new ExpressionValidator(nameResolver);
+        validator.Transform(expression);
+
+        if (validator._errors.Count == 0)
+        {
+            error = null;
+            return true;
+        }
+
+        if (validator._errors.Count == 1)
+        {
+            error = validator._errors[0];
+        }
+        else
+        {
+            var sb = new StringBuilder("Multiple errors found: ");
+            for (var i = 0; i < validator._errors.Count; i++)
+            {
+                if (i > 0)
+                    sb.Append("; ");
+                sb.Append(validator._errors[i]);
+            }
+            error = sb.ToString();
+        }
+
+        return false;
+    }
+
+    protected override Expression Transform(CallExpression call)
+    {
+        // Skip validation for intermediate operators (they get transformed later)
+        if (!call.OperatorName.StartsWith("_Internal_"))
+        {
+            // Check for unknown function names
+            if (!_nameResolver.TryResolveFunctionName(call.OperatorName, out _))
+            {
+                _errors.Add($"The function name `{call.OperatorName}` was not recognized.");
+            }
+        }
+
+        // Check for invalid CI modifier usage
+        if (call.IgnoreCase && !CaseSensitiveFunctions.Contains(call.OperatorName))
+        {
+            _errors.Add($"The function `{call.OperatorName}` does not support case-insensitive operation.");
+        }
+
+        // Validate regex patterns in IsMatch and IndexOfMatch
+        if (Operators.SameOperator(call.OperatorName, Operators.OpIsMatch) ||
+            Operators.SameOperator(call.OperatorName, Operators.OpIndexOfMatch))
+        {
+            ValidateRegexPattern(call);
+        }
+
+        return base.Transform(call);
+    }
+
+    void ValidateRegexPattern(CallExpression call)
+    {
+        if (call.Operands.Length != 2)
+            return;
+
+        var pattern = call.Operands[1];
+        if (pattern is ConstantExpression { Constant: ScalarValue { Value: string s } })
+        {
+            try
+            {
+                var opts = RegexOptions.Compiled | RegexOptions.ExplicitCapture;
+                if (call.IgnoreCase)
+                    opts |= RegexOptions.IgnoreCase;
+
+                // Try to compile the regex with timeout to catch invalid patterns
+                _ = new Regex(s, opts, TimeSpan.FromMilliseconds(100));
+            }
+            catch (ArgumentException ex)
+            {
+                _errors.Add($"Invalid regular expression in {call.OperatorName}: {ex.Message}");
+            }
+        }
+    }
+}
diff --git a/src/Serilog.Expressions/Expressions/SerilogExpression.cs b/src/Serilog.Expressions/Expressions/SerilogExpression.cs
@@ -15,6 +15,7 @@
 using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using Serilog.Expressions.Compilation;
+using Serilog.Expressions.Compilation.Validation;
 using Serilog.Expressions.Parsing;
 
 // ReSharper disable MemberCanBePrivate.Global
@@ -101,10 +102,30 @@ static bool TryCompileImpl(string expression,
             return false;
         }
 
-        var evaluate = ExpressionCompiler.Compile(root, formatProvider, DefaultFunctionNameResolver.Build(nameResolver));
-        result = evt => evaluate(new(evt));
-        error = null;
-        return true;
+        var resolver = DefaultFunctionNameResolver.Build(nameResolver);
+        
+        // Validate the expression before compilation
+        if (!ExpressionValidator.Validate(root, resolver, out var validationError))
+        {
+            result = null;
+            error = validationError ?? "Unknown validation error";
+            return false;
+        }
+
+        try
+        {
+            var evaluate = ExpressionCompiler.Compile(root, formatProvider, resolver);
+            result = evt => evaluate(new(evt));
+            error = null;
+            return true;
+        }
+        catch (ArgumentException ex)
+        {
+            // Catch any remaining exceptions that weren't caught by validation
+            result = null;
+            error = ex.Message;
+            return false;
+        }
     }
 
     /// <summary>
diff --git a/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs b/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs
@@ -0,0 +1,163 @@
+using Xunit;
+
+namespace Serilog.Expressions.Tests;
+
+public class ExpressionValidationTests
+{
+    [Theory]
+    [InlineData("IsMatch(Name, '[invalid')", "Invalid regular expression")]
+    [InlineData("IndexOfMatch(Text, '(?<')", "Invalid regular expression")]
+    [InlineData("IsMatch(Name, '(?P<name>)')", "Invalid regular expression")]
+    [InlineData("IsMatch(Name, '(unclosed')", "Invalid regular expression")]
+    [InlineData("IndexOfMatch(Text, '*invalid')", "Invalid regular expression")]
+    public void InvalidRegularExpressionsAreReportedGracefully(string expression, string expectedErrorFragment)
+    {
+        var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
+        Assert.False(result);
+        Assert.Contains(expectedErrorFragment, error);
+        Assert.Null(compiled);
+    }
+
+    [Theory]
+    [InlineData("UnknownFunction()", "The function name `UnknownFunction` was not recognized.")]
+    [InlineData("foo(1, 2, 3)", "The function name `foo` was not recognized.")]
+    [InlineData("MyCustomFunc(Name)", "The function name `MyCustomFunc` was not recognized.")]
+    [InlineData("notAFunction()", "The function name `notAFunction` was not recognized.")]
+    public void UnknownFunctionNamesAreReportedGracefully(string expression, string expectedError)
+    {
+        var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
+        Assert.False(result);
+        Assert.Equal(expectedError, error);
+        Assert.Null(compiled);
+    }
+
+    [Theory]
+    [InlineData("Length(Name) ci", "The function `Length` does not support case-insensitive operation.")]
+    [InlineData("Round(Value, 2) ci", "The function `Round` does not support case-insensitive operation.")]
+    [InlineData("Now() ci", "The function `Now` does not support case-insensitive operation.")]
+    [InlineData("TypeOf(Value) ci", "The function `TypeOf` does not support case-insensitive operation.")]
+    [InlineData("IsDefined(Prop) ci", "The function `IsDefined` does not support case-insensitive operation.")]
+    public void InvalidCiModifierUsageIsReported(string expression, string expectedError)
+    {
+        var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
+        Assert.False(result);
+        Assert.Equal(expectedError, error);
+        Assert.Null(compiled);
+    }
+
+    [Theory]
+    [InlineData("Contains(Name, 'test') ci")]
+    [InlineData("StartsWith(Path, '/api') ci")]
+    [InlineData("EndsWith(File, '.txt') ci")]
+    [InlineData("IsMatch(Email, '@example') ci")]
+    [InlineData("IndexOfMatch(Text, 'pattern') ci")]
+    [InlineData("IndexOf(Name, 'x') ci")]
+    [InlineData("Name = 'test' ci")]
+    [InlineData("Name <> 'test' ci")]
+    [InlineData("Name like '%test%' ci")]
+    public void ValidCiModifierUsageCompiles(string expression)
+    {
+        var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
+        Assert.True(result, $"Failed to compile: {error}");
+        Assert.NotNull(compiled);
+        Assert.Null(error);
+    }
+
+    [Fact]
+    public void MultipleErrorsAreCollectedAndReported()
+    {
+        var expression = "UnknownFunc() and IsMatch(Name, '[invalid') and Length(Value) ci";
+        var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
+        
+        Assert.False(result);
+        Assert.Null(compiled);
+        
+        // Should report all three errors
+        Assert.Contains("UnknownFunc", error);
+        Assert.Contains("Invalid regular expression", error);
+        Assert.Contains("does not support case-insensitive", error);
+        Assert.Contains("Multiple errors found", error);
+    }
+
+    [Fact]
+    public void ValidExpressionsStillCompileWithoutErrors()
+    {
+        var validExpressions = new[]
+        {
+            "IsMatch(Name, '^[A-Z]')",
+            "IndexOfMatch(Text, '\\d+')",
+            "Contains(Name, 'test') ci",
+            "Length(Items) > 5",
+            "Round(Value, 2)",
+            "TypeOf(Data) = 'String'",
+            "Name like '%test%'",
+            "StartsWith(Path, '/') and EndsWith(Path, '.json')"
+        };
+        
+        foreach (var expr in validExpressions)
+        {
+            var result = SerilogExpression.TryCompile(expr, out var compiled, out var error);
+            Assert.True(result, $"Failed to compile: {expr}. Error: {error}");
+            Assert.NotNull(compiled);
+            Assert.Null(error);
+        }
+    }
+
+    [Fact]
+    public void CompileMethodStillThrowsForInvalidExpressions()
+    {
+        // Ensure Compile() method (not TryCompile) maintains throwing behavior for invalid expressions
+        Assert.Throws<ArgumentException>(() => 
+            SerilogExpression.Compile("UnknownFunction()"));
+        
+        Assert.Throws<ArgumentException>(() => 
+            SerilogExpression.Compile("IsMatch(Name, '[invalid')"));
+        
+        Assert.Throws<ArgumentException>(() => 
+            SerilogExpression.Compile("Length(Name) ci"));
+    }
+
+    [Theory]
+    [InlineData("IsMatch(Name, Name)")] // Non-constant pattern
+    [InlineData("IndexOfMatch(Text, Value)")] // Non-constant pattern
+    public void NonConstantRegexPatternsHandledGracefully(string expression)
+    {
+        // These should compile but may log warnings (not errors)
+        var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
+        
+        // These compile successfully but return undefined at runtime
+        Assert.True(result);
+        Assert.NotNull(compiled);
+        Assert.Null(error);
+    }
+
+    [Fact]
+    public void RegexTimeoutIsRespected()
+    {
+        // This regex should compile fine - timeout only matters at runtime
+        var expression = @"IsMatch(Text, '(a+)+b')";
+        
+        var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
+        
+        Assert.True(result);
+        Assert.NotNull(compiled);
+        Assert.Null(error);
+    }
+
+    [Fact]
+    public void ComplexExpressionsWithMixedIssues()
+    {
+        var expression = "(UnknownFunc1() or IsMatch(Name, '(invalid')) and NotRealFunc() ci";
+        var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
+        
+        Assert.False(result);
+        Assert.Null(compiled);
+        Assert.NotNull(error);
+        
+        // Should report multiple errors
+        Assert.Contains("Multiple errors found", error);
+        Assert.Contains("UnknownFunc1", error);
+        Assert.Contains("NotRealFunc", error);
+        Assert.Contains("Invalid regular expression", error);
+    }
+}
