Skip to content

Commit 5d87b5b

Browse files
committed
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
1 parent 6792609 commit 5d87b5b

File tree

6 files changed

+89
-188
lines changed

6 files changed

+89
-188
lines changed
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
// Copyright © Serilog Contributors
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
namespace Serilog.Expressions.Compilation;
16+
17+
class ExpressionValidationException : ArgumentException
18+
{
19+
public ExpressionValidationException(string message) : base(message)
20+
{
21+
}
22+
23+
public ExpressionValidationException(string message, Exception innerException) : base(message, innerException)
24+
{
25+
}
26+
}

src/Serilog.Expressions/Expressions/Compilation/Linq/LinqExpressionCompiler.cs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
using System.Reflection;
1717
using System.Runtime.InteropServices;
1818
using System.Text;
19+
using Serilog.Debugging;
1920
using Serilog.Events;
2021
using Serilog.Expressions.Ast;
2122
using Serilog.Expressions.Compilation.Transformations;
@@ -99,12 +100,24 @@ ExpressionBody Splice(Expression<Evaluatable> lambda)
99100
protected override ExpressionBody Transform(CallExpression call)
100101
{
101102
if (!_nameResolver.TryResolveFunctionName(call.OperatorName, out var m))
102-
throw new ArgumentException($"The function name `{call.OperatorName}` was not recognized.");
103+
throw new ExpressionValidationException($"The function name `{call.OperatorName}` was not recognized.");
103104

104105
var methodParameters = m.GetParameters()
105106
.Select(info => (pi: info, optional: info.GetCustomAttribute<OptionalAttribute>() != null))
106107
.ToList();
107108

109+
// Log warning for CI modifier usage on functions that don't support it
110+
// Note: We log a warning rather than throwing to maintain backward compatibility
111+
// Previously, invalid CI usage was silently ignored
112+
if (call.IgnoreCase)
113+
{
114+
var supportsStringComparison = methodParameters.Any(p => p.pi.ParameterType == typeof(StringComparison));
115+
if (!supportsStringComparison)
116+
{
117+
SelfLog.WriteLine($"The function `{call.OperatorName}` does not support case-insensitive operation; the 'ci' modifier will be ignored.");
118+
}
119+
}
120+
108121
var allowedParameters = methodParameters.Where(info => info.pi.ParameterType == typeof(LogEventPropertyValue)).ToList();
109122
var requiredParameterCount = allowedParameters.Count(info => !info.optional);
110123

src/Serilog.Expressions/Expressions/Compilation/Text/TextMatchingTransformer.cs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,7 @@ Expression TryCompileIndexOfMatch(bool ignoreCase, Expression corpus, Expression
6161
}
6262
catch (ArgumentException ex)
6363
{
64-
SelfLog.WriteLine($"Serilog.Expressions: Invalid regular expression in `IndexOfMatch()`: {ex.Message}");
65-
return new CallExpression(false, Operators.OpUndefined);
64+
throw new ExpressionValidationException($"Invalid regular expression in IndexOfMatch: {ex.Message}", ex);
6665
}
6766
}
6867

src/Serilog.Expressions/Expressions/Compilation/Validation/ExpressionValidator.cs

Lines changed: 0 additions & 144 deletions
This file was deleted.

src/Serilog.Expressions/Expressions/SerilogExpression.cs

Lines changed: 7 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
using System.Diagnostics.CodeAnalysis;
1616
using System.Globalization;
1717
using Serilog.Expressions.Compilation;
18-
using Serilog.Expressions.Compilation.Validation;
1918
using Serilog.Expressions.Parsing;
2019

2120
// ReSharper disable MemberCanBePrivate.Global
@@ -54,8 +53,8 @@ public static CompiledExpression Compile(string expression,
5453
/// <param name="result">A function that evaluates the expression in the context of a log event.</param>
5554
/// <param name="error">The reported error, if compilation was unsuccessful.</param>
5655
/// <returns>True if the function could be created; otherwise, false.</returns>
57-
/// <remarks>Regular expression syntax errors currently generate exceptions instead of producing friendly
58-
/// errors.</remarks>
56+
/// <remarks>Validation errors including invalid regular expressions and unknown function names are returned
57+
/// as friendly error messages. Invalid case-insensitive modifiers are ignored with warnings.</remarks>
5958
public static bool TryCompile(
6059
string expression,
6160
[MaybeNullWhen(false)] out CompiledExpression result,
@@ -76,8 +75,8 @@ public static bool TryCompile(
7675
/// <param name="result">A function that evaluates the expression in the context of a log event.</param>
7776
/// <param name="error">The reported error, if compilation was unsuccessful.</param>
7877
/// <returns>True if the function could be created; otherwise, false.</returns>
79-
/// <remarks>Regular expression syntax errors currently generate exceptions instead of producing friendly
80-
/// errors.</remarks>
78+
/// <remarks>Validation errors including invalid regular expressions and unknown function names are returned
79+
/// as friendly error messages. Invalid case-insensitive modifiers are ignored with warnings.</remarks>
8180
public static bool TryCompile(string expression,
8281
IFormatProvider? formatProvider,
8382
NameResolver nameResolver,
@@ -102,26 +101,16 @@ static bool TryCompileImpl(string expression,
102101
return false;
103102
}
104103

105-
var resolver = DefaultFunctionNameResolver.Build(nameResolver);
106-
107-
// Validate the expression before compilation
108-
if (!ExpressionValidator.Validate(root, resolver, out var validationError))
109-
{
110-
result = null;
111-
error = validationError ?? "Unknown validation error";
112-
return false;
113-
}
114-
115104
try
116105
{
117-
var evaluate = ExpressionCompiler.Compile(root, formatProvider, resolver);
106+
var evaluate = ExpressionCompiler.Compile(root, formatProvider, DefaultFunctionNameResolver.Build(nameResolver));
118107
result = evt => evaluate(new(evt));
119108
error = null;
120109
return true;
121110
}
122-
catch (ArgumentException ex)
111+
catch (ExpressionValidationException ex)
123112
{
124-
// Catch any remaining exceptions that weren't caught by validation
113+
// Catch validation errors from compilation
125114
result = null;
126115
error = ex.Message;
127116
return false;

test/Serilog.Expressions.Tests/ExpressionValidationTests.cs

Lines changed: 41 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -32,17 +32,17 @@ public void UnknownFunctionNamesAreReportedGracefully(string expression, string
3232
}
3333

3434
[Theory]
35-
[InlineData("Length(Name) ci", "The function `Length` does not support case-insensitive operation.")]
36-
[InlineData("Round(Value, 2) ci", "The function `Round` does not support case-insensitive operation.")]
37-
[InlineData("Now() ci", "The function `Now` does not support case-insensitive operation.")]
38-
[InlineData("TypeOf(Value) ci", "The function `TypeOf` does not support case-insensitive operation.")]
39-
[InlineData("IsDefined(Prop) ci", "The function `IsDefined` does not support case-insensitive operation.")]
40-
public void InvalidCiModifierUsageIsReported(string expression, string expectedError)
35+
[InlineData("Length(Name) ci")]
36+
[InlineData("Round(Value, 2) ci")]
37+
[InlineData("Now() ci")]
38+
[InlineData("TypeOf(Value) ci")]
39+
[InlineData("IsDefined(Prop) ci")]
40+
public void InvalidCiModifierUsageCompilesWithWarning(string expression)
4141
{
4242
var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
43-
Assert.False(result);
44-
Assert.Equal(expectedError, error);
45-
Assert.Null(compiled);
43+
Assert.True(result, $"Failed to compile: {error}");
44+
Assert.NotNull(compiled);
45+
Assert.Null(error);
4646
}
4747

4848
[Theory]
@@ -64,19 +64,17 @@ public void ValidCiModifierUsageCompiles(string expression)
6464
}
6565

6666
[Fact]
67-
public void MultipleErrorsAreCollectedAndReported()
67+
public void FirstErrorIsReportedInComplexExpressions()
6868
{
69-
var expression = "UnknownFunc() and IsMatch(Name, '[invalid') and Length(Value) ci";
69+
var expression = "UnknownFunc() and Length(Value) > 5";
7070
var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
7171

7272
Assert.False(result);
7373
Assert.Null(compiled);
7474

75-
// Should report all three errors
75+
// Should report the first error encountered
7676
Assert.Contains("UnknownFunc", error);
77-
Assert.Contains("Invalid regular expression", error);
78-
Assert.Contains("does not support case-insensitive", error);
79-
Assert.Contains("Multiple errors found", error);
77+
Assert.NotNull(error);
8078
}
8179

8280
[Fact]
@@ -113,8 +111,9 @@ public void CompileMethodStillThrowsForInvalidExpressions()
113111
Assert.Throws<ArgumentException>(() =>
114112
SerilogExpression.Compile("IsMatch(Name, '[invalid')"));
115113

116-
Assert.Throws<ArgumentException>(() =>
117-
SerilogExpression.Compile("Length(Name) ci"));
114+
// CI modifier on non-supporting functions compiles with warning
115+
var compiledWithCi = SerilogExpression.Compile("Length(Name) ci");
116+
Assert.NotNull(compiledWithCi);
118117

119118
Assert.Throws<ArgumentException>(() =>
120119
SerilogExpression.Compile("IndexOfMatch(Text, '(?<')"));
@@ -148,19 +147,38 @@ public void RegexTimeoutIsRespected()
148147
}
149148

150149
[Fact]
151-
public void ComplexExpressionsWithMixedIssues()
150+
public void ComplexExpressionsReportFirstError()
152151
{
153-
var expression = "(UnknownFunc1() or IsMatch(Name, '(invalid')) and NotRealFunc() ci";
152+
var expression = "UnknownFunc1() or Length(Value) > 5";
154153
var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
155154

156155
Assert.False(result);
157156
Assert.Null(compiled);
158157
Assert.NotNull(error);
159158

160-
// Should report multiple errors
161-
Assert.Contains("Multiple errors found", error);
159+
// Should report the first error encountered during compilation
162160
Assert.Contains("UnknownFunc1", error);
163-
Assert.Contains("NotRealFunc", error);
164-
Assert.Contains("Invalid regular expression", error);
161+
}
162+
163+
[Fact]
164+
public void BackwardCompatibilityPreservedForInvalidCiUsage()
165+
{
166+
// These previously compiled (CI was silently ignored)
167+
// They should still compile with the new changes
168+
var expressions = new[]
169+
{
170+
"undefined() ci",
171+
"null = undefined() ci",
172+
"Length(Name) ci",
173+
"Round(Value, 2) ci"
174+
};
175+
176+
foreach (var expr in expressions)
177+
{
178+
var result = SerilogExpression.TryCompile(expr, out var compiled, out var error);
179+
Assert.True(result, $"Breaking change detected: {expr} no longer compiles. Error: {error}");
180+
Assert.NotNull(compiled);
181+
Assert.Null(error);
182+
}
165183
}
166184
}

0 commit comments

Comments
 (0)