From 3c4bccb2b344a031aa43566126ed7a79062e6df5 Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Thu, 28 Aug 2025 15:29:40 -0700 Subject: [PATCH 1/3] Fix #6: Add eager/non-throwing validation for expression syntax 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 --- .../Text/TextMatchingTransformer.cs | 18 +- .../Validation/ExpressionValidator.cs | 144 ++++++++++++++++ .../Expressions/SerilogExpression.cs | 29 +++- .../ExpressionValidationTests.cs | 163 ++++++++++++++++++ 4 files changed, 345 insertions(+), 9 deletions(-) create mode 100644 src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs create mode 100644 test/Serilog.Expressions.Tests/ExpressionValidationTests.cs diff --git a/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs b/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs index 9fe3ecb..e71528f 100644 --- 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 new file mode 100644 index 0000000..baeb337 --- /dev/null +++ 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 _errors = new(); + + // Functions that support case-insensitive operations (have StringComparison parameter) + static readonly HashSet 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}"); + } + } + } +} \ No newline at end of file diff --git a/src/Serilog.Expressions/Expressions/SerilogExpression.cs b/src/Serilog.Expressions/Expressions/SerilogExpression.cs index 9416c11..168d4c7 100644 --- 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; + } } /// diff --git a/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs b/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs new file mode 100644 index 0000000..0c865e4 --- /dev/null +++ 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)')", "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(() => + SerilogExpression.Compile("UnknownFunction()")); + + Assert.Throws(() => + SerilogExpression.Compile("IsMatch(Name, '[invalid')")); + + Assert.Throws(() => + 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); + } +} \ No newline at end of file From 6792609bf1be67a0d73322a9aa688f2afef2c2dd Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Thu, 28 Aug 2025 17:21:30 -0700 Subject: [PATCH 2/3] Fix string interpolation bug and rename variable - Fix ${regex} to {regex} in log message (was outputting literal text) - Rename CaseSensitiveFunctions to CaseInsensitiveCapableFunctions - Add test for IndexOfMatch with invalid regex in Compile() --- .../Expressions/Compilation/Text/TextMatchingTransformer.cs | 2 +- .../Expressions/Compilation/Validation/ExpressionValidator.cs | 4 ++-- test/Serilog.Expressions.Tests/ExpressionValidationTests.cs | 3 +++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs b/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs index e71528f..829729e 100644 --- a/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs +++ b/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs @@ -66,7 +66,7 @@ Expression TryCompileIndexOfMatch(bool ignoreCase, Expression corpus, Expression } } - SelfLog.WriteLine($"Serilog.Expressions: `IndexOfMatch()` requires a constant string regular expression argument; found ${regex}."); + SelfLog.WriteLine($"Serilog.Expressions: `IndexOfMatch()` requires a constant string regular expression argument; found {regex}."); return new CallExpression(false, Operators.OpUndefined); } } \ No newline at end of file diff --git a/src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs b/src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs index baeb337..edfe694 100644 --- a/src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs +++ b/src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs @@ -26,7 +26,7 @@ class ExpressionValidator : IdentityTransformer readonly List _errors = new(); // Functions that support case-insensitive operations (have StringComparison parameter) - static readonly HashSet CaseSensitiveFunctions = new(StringComparer.OrdinalIgnoreCase) + static readonly HashSet CaseInsensitiveCapableFunctions = new(StringComparer.OrdinalIgnoreCase) { // String operations Operators.OpContains, @@ -103,7 +103,7 @@ protected override Expression Transform(CallExpression call) } // Check for invalid CI modifier usage - if (call.IgnoreCase && !CaseSensitiveFunctions.Contains(call.OperatorName)) + if (call.IgnoreCase && !CaseInsensitiveCapableFunctions.Contains(call.OperatorName)) { _errors.Add($"The function `{call.OperatorName}` does not support case-insensitive operation."); } diff --git a/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs b/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs index 0c865e4..2844a3e 100644 --- a/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs +++ b/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs @@ -115,6 +115,9 @@ public void CompileMethodStillThrowsForInvalidExpressions() Assert.Throws(() => SerilogExpression.Compile("Length(Name) ci")); + + Assert.Throws(() => + SerilogExpression.Compile("IndexOfMatch(Text, '(?<')")); } [Theory] From 5d87b5b6ef12d226c27fda66550ceb9504a1ddad Mon Sep 17 00:00:00 2001 From: Brandon Williams Date: Fri, 5 Sep 2025 02:47:39 -0700 Subject: [PATCH 3/3] Add eager/non-throwing validation for expression syntax errors - 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 --- .../ExpressionValidationException.cs | 26 ++++ .../Linq/LinqExpressionCompiler.cs | 15 +- .../Text/TextMatchingTransformer.cs | 3 +- .../Validation/ExpressionValidator.cs | 144 ------------------ .../Expressions/SerilogExpression.cs | 25 +-- .../ExpressionValidationTests.cs | 64 +++++--- 6 files changed, 89 insertions(+), 188 deletions(-) create mode 100644 src/Serilog.Expressions/Expressions/Compilation/ExpressionValidationException.cs delete mode 100644 src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs diff --git a/src/Serilog.Expressions/Expressions/Compilation/ExpressionValidationException.cs b/src/Serilog.Expressions/Expressions/Compilation/ExpressionValidationException.cs new file mode 100644 index 0000000..4fb57ea --- /dev/null +++ b/src/Serilog.Expressions/Expressions/Compilation/ExpressionValidationException.cs @@ -0,0 +1,26 @@ +// 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. + +namespace Serilog.Expressions.Compilation; + +class ExpressionValidationException : ArgumentException +{ + public ExpressionValidationException(string message) : base(message) + { + } + + public ExpressionValidationException(string message, Exception innerException) : base(message, innerException) + { + } +} \ No newline at end of file diff --git a/src/Serilog.Expressions/Expressions/Compilation/Linq/LinqExpressionCompiler.cs b/src/Serilog.Expressions/Expressions/Compilation/Linq/LinqExpressionCompiler.cs index 2c6c99c..43e2b98 100644 --- a/src/Serilog.Expressions/Expressions/Compilation/Linq/LinqExpressionCompiler.cs +++ b/src/Serilog.Expressions/Expressions/Compilation/Linq/LinqExpressionCompiler.cs @@ -16,6 +16,7 @@ using System.Reflection; using System.Runtime.InteropServices; using System.Text; +using Serilog.Debugging; using Serilog.Events; using Serilog.Expressions.Ast; using Serilog.Expressions.Compilation.Transformations; @@ -99,12 +100,24 @@ ExpressionBody Splice(Expression lambda) protected override ExpressionBody Transform(CallExpression call) { if (!_nameResolver.TryResolveFunctionName(call.OperatorName, out var m)) - throw new ArgumentException($"The function name `{call.OperatorName}` was not recognized."); + throw new ExpressionValidationException($"The function name `{call.OperatorName}` was not recognized."); var methodParameters = m.GetParameters() .Select(info => (pi: info, optional: info.GetCustomAttribute() != null)) .ToList(); + // Log warning for CI modifier usage on functions that don't support it + // Note: We log a warning rather than throwing to maintain backward compatibility + // Previously, invalid CI usage was silently ignored + if (call.IgnoreCase) + { + var supportsStringComparison = methodParameters.Any(p => p.pi.ParameterType == typeof(StringComparison)); + if (!supportsStringComparison) + { + SelfLog.WriteLine($"The function `{call.OperatorName}` does not support case-insensitive operation; the 'ci' modifier will be ignored."); + } + } + var allowedParameters = methodParameters.Where(info => info.pi.ParameterType == typeof(LogEventPropertyValue)).ToList(); var requiredParameterCount = allowedParameters.Count(info => !info.optional); diff --git a/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs b/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs index 829729e..7392e1b 100644 --- a/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs +++ b/src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs @@ -61,8 +61,7 @@ Expression TryCompileIndexOfMatch(bool ignoreCase, Expression corpus, Expression } catch (ArgumentException ex) { - SelfLog.WriteLine($"Serilog.Expressions: Invalid regular expression in `IndexOfMatch()`: {ex.Message}"); - return new CallExpression(false, Operators.OpUndefined); + throw new ExpressionValidationException($"Invalid regular expression in IndexOfMatch: {ex.Message}", ex); } } diff --git a/src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs b/src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs deleted file mode 100644 index edfe694..0000000 --- a/src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs +++ /dev/null @@ -1,144 +0,0 @@ -// 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 _errors = new(); - - // Functions that support case-insensitive operations (have StringComparison parameter) - static readonly HashSet CaseInsensitiveCapableFunctions = 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 && !CaseInsensitiveCapableFunctions.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}"); - } - } - } -} \ No newline at end of file diff --git a/src/Serilog.Expressions/Expressions/SerilogExpression.cs b/src/Serilog.Expressions/Expressions/SerilogExpression.cs index 168d4c7..8233dbe 100644 --- a/src/Serilog.Expressions/Expressions/SerilogExpression.cs +++ b/src/Serilog.Expressions/Expressions/SerilogExpression.cs @@ -15,7 +15,6 @@ using System.Diagnostics.CodeAnalysis; using System.Globalization; using Serilog.Expressions.Compilation; -using Serilog.Expressions.Compilation.Validation; using Serilog.Expressions.Parsing; // ReSharper disable MemberCanBePrivate.Global @@ -54,8 +53,8 @@ public static CompiledExpression Compile(string expression, /// A function that evaluates the expression in the context of a log event. /// The reported error, if compilation was unsuccessful. /// True if the function could be created; otherwise, false. - /// Regular expression syntax errors currently generate exceptions instead of producing friendly - /// errors. + /// Validation errors including invalid regular expressions and unknown function names are returned + /// as friendly error messages. Invalid case-insensitive modifiers are ignored with warnings. public static bool TryCompile( string expression, [MaybeNullWhen(false)] out CompiledExpression result, @@ -76,8 +75,8 @@ public static bool TryCompile( /// A function that evaluates the expression in the context of a log event. /// The reported error, if compilation was unsuccessful. /// True if the function could be created; otherwise, false. - /// Regular expression syntax errors currently generate exceptions instead of producing friendly - /// errors. + /// Validation errors including invalid regular expressions and unknown function names are returned + /// as friendly error messages. Invalid case-insensitive modifiers are ignored with warnings. public static bool TryCompile(string expression, IFormatProvider? formatProvider, NameResolver nameResolver, @@ -102,26 +101,16 @@ static bool TryCompileImpl(string expression, return false; } - 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); + var evaluate = ExpressionCompiler.Compile(root, formatProvider, DefaultFunctionNameResolver.Build(nameResolver)); result = evt => evaluate(new(evt)); error = null; return true; } - catch (ArgumentException ex) + catch (ExpressionValidationException ex) { - // Catch any remaining exceptions that weren't caught by validation + // Catch validation errors from compilation result = null; error = ex.Message; return false; diff --git a/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs b/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs index 2844a3e..b7948dc 100644 --- a/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs +++ b/test/Serilog.Expressions.Tests/ExpressionValidationTests.cs @@ -32,17 +32,17 @@ public void UnknownFunctionNamesAreReportedGracefully(string expression, string } [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) + [InlineData("Length(Name) ci")] + [InlineData("Round(Value, 2) ci")] + [InlineData("Now() ci")] + [InlineData("TypeOf(Value) ci")] + [InlineData("IsDefined(Prop) ci")] + public void InvalidCiModifierUsageCompilesWithWarning(string expression) { var result = SerilogExpression.TryCompile(expression, out var compiled, out var error); - Assert.False(result); - Assert.Equal(expectedError, error); - Assert.Null(compiled); + Assert.True(result, $"Failed to compile: {error}"); + Assert.NotNull(compiled); + Assert.Null(error); } [Theory] @@ -64,19 +64,17 @@ public void ValidCiModifierUsageCompiles(string expression) } [Fact] - public void MultipleErrorsAreCollectedAndReported() + public void FirstErrorIsReportedInComplexExpressions() { - var expression = "UnknownFunc() and IsMatch(Name, '[invalid') and Length(Value) ci"; + var expression = "UnknownFunc() and Length(Value) > 5"; var result = SerilogExpression.TryCompile(expression, out var compiled, out var error); Assert.False(result); Assert.Null(compiled); - // Should report all three errors + // Should report the first error encountered Assert.Contains("UnknownFunc", error); - Assert.Contains("Invalid regular expression", error); - Assert.Contains("does not support case-insensitive", error); - Assert.Contains("Multiple errors found", error); + Assert.NotNull(error); } [Fact] @@ -113,8 +111,9 @@ public void CompileMethodStillThrowsForInvalidExpressions() Assert.Throws(() => SerilogExpression.Compile("IsMatch(Name, '[invalid')")); - Assert.Throws(() => - SerilogExpression.Compile("Length(Name) ci")); + // CI modifier on non-supporting functions compiles with warning + var compiledWithCi = SerilogExpression.Compile("Length(Name) ci"); + Assert.NotNull(compiledWithCi); Assert.Throws(() => SerilogExpression.Compile("IndexOfMatch(Text, '(?<')")); @@ -148,19 +147,38 @@ public void RegexTimeoutIsRespected() } [Fact] - public void ComplexExpressionsWithMixedIssues() + public void ComplexExpressionsReportFirstError() { - var expression = "(UnknownFunc1() or IsMatch(Name, '(invalid')) and NotRealFunc() ci"; + var expression = "UnknownFunc1() or Length(Value) > 5"; 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); + // Should report the first error encountered during compilation Assert.Contains("UnknownFunc1", error); - Assert.Contains("NotRealFunc", error); - Assert.Contains("Invalid regular expression", error); + } + + [Fact] + public void BackwardCompatibilityPreservedForInvalidCiUsage() + { + // These previously compiled (CI was silently ignored) + // They should still compile with the new changes + var expressions = new[] + { + "undefined() ci", + "null = undefined() ci", + "Length(Name) ci", + "Round(Value, 2) ci" + }; + + foreach (var expr in expressions) + { + var result = SerilogExpression.TryCompile(expr, out var compiled, out var error); + Assert.True(result, $"Breaking change detected: {expr} no longer compiles. Error: {error}"); + Assert.NotNull(compiled); + Assert.Null(error); + } } } \ No newline at end of file