Skip to content

Commit f258879

Browse files
authored
Merge pull request #136 from willibrandon/fix/eager-expression-validation
Fix #6: Add eager/non-throwing validation for expression syntax errors
2 parents 1b6a42d + 5d87b5b commit f258879

File tree

5 files changed

+255
-15
lines changed

5 files changed

+255
-15
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: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,21 @@ Expression TryCompileIndexOfMatch(bool ignoreCase, Expression corpus, Expression
5151
{
5252
if (regex is ConstantExpression { Constant: ScalarValue { Value: string s } })
5353
{
54-
var opts = RegexOptions.Compiled | RegexOptions.ExplicitCapture;
55-
if (ignoreCase)
56-
opts |= RegexOptions.IgnoreCase;
57-
var compiled = new Regex(s, opts, TimeSpan.FromMilliseconds(100));
58-
return new IndexOfMatchExpression(Transform(corpus), compiled);
54+
try
55+
{
56+
var opts = RegexOptions.Compiled | RegexOptions.ExplicitCapture;
57+
if (ignoreCase)
58+
opts |= RegexOptions.IgnoreCase;
59+
var compiled = new Regex(s, opts, TimeSpan.FromMilliseconds(100));
60+
return new IndexOfMatchExpression(Transform(corpus), compiled);
61+
}
62+
catch (ArgumentException ex)
63+
{
64+
throw new ExpressionValidationException($"Invalid regular expression in IndexOfMatch: {ex.Message}", ex);
65+
}
5966
}
6067

61-
SelfLog.WriteLine($"Serilog.Expressions: `IndexOfMatch()` requires a constant string regular expression argument; found ${regex}.");
68+
SelfLog.WriteLine($"Serilog.Expressions: `IndexOfMatch()` requires a constant string regular expression argument; found {regex}.");
6269
return new CallExpression(false, Operators.OpUndefined);
6370
}
6471
}

src/Serilog.Expressions/Expressions/SerilogExpression.cs

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,8 @@ public static CompiledExpression Compile(string expression,
5353
/// <param name="result">A function that evaluates the expression in the context of a log event.</param>
5454
/// <param name="error">The reported error, if compilation was unsuccessful.</param>
5555
/// <returns>True if the function could be created; otherwise, false.</returns>
56-
/// <remarks>Regular expression syntax errors currently generate exceptions instead of producing friendly
57-
/// 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>
5858
public static bool TryCompile(
5959
string expression,
6060
[MaybeNullWhen(false)] out CompiledExpression result,
@@ -75,8 +75,8 @@ public static bool TryCompile(
7575
/// <param name="result">A function that evaluates the expression in the context of a log event.</param>
7676
/// <param name="error">The reported error, if compilation was unsuccessful.</param>
7777
/// <returns>True if the function could be created; otherwise, false.</returns>
78-
/// <remarks>Regular expression syntax errors currently generate exceptions instead of producing friendly
79-
/// 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>
8080
public static bool TryCompile(string expression,
8181
IFormatProvider? formatProvider,
8282
NameResolver nameResolver,
@@ -101,10 +101,20 @@ static bool TryCompileImpl(string expression,
101101
return false;
102102
}
103103

104-
var evaluate = ExpressionCompiler.Compile(root, formatProvider, DefaultFunctionNameResolver.Build(nameResolver));
105-
result = evt => evaluate(new(evt));
106-
error = null;
107-
return true;
104+
try
105+
{
106+
var evaluate = ExpressionCompiler.Compile(root, formatProvider, DefaultFunctionNameResolver.Build(nameResolver));
107+
result = evt => evaluate(new(evt));
108+
error = null;
109+
return true;
110+
}
111+
catch (ExpressionValidationException ex)
112+
{
113+
// Catch validation errors from compilation
114+
result = null;
115+
error = ex.Message;
116+
return false;
117+
}
108118
}
109119

110120
/// <summary>
Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
using Xunit;
2+
3+
namespace Serilog.Expressions.Tests;
4+
5+
public class ExpressionValidationTests
6+
{
7+
[Theory]
8+
[InlineData("IsMatch(Name, '[invalid')", "Invalid regular expression")]
9+
[InlineData("IndexOfMatch(Text, '(?<')", "Invalid regular expression")]
10+
[InlineData("IsMatch(Name, '(?P<name>)')", "Invalid regular expression")]
11+
[InlineData("IsMatch(Name, '(unclosed')", "Invalid regular expression")]
12+
[InlineData("IndexOfMatch(Text, '*invalid')", "Invalid regular expression")]
13+
public void InvalidRegularExpressionsAreReportedGracefully(string expression, string expectedErrorFragment)
14+
{
15+
var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
16+
Assert.False(result);
17+
Assert.Contains(expectedErrorFragment, error);
18+
Assert.Null(compiled);
19+
}
20+
21+
[Theory]
22+
[InlineData("UnknownFunction()", "The function name `UnknownFunction` was not recognized.")]
23+
[InlineData("foo(1, 2, 3)", "The function name `foo` was not recognized.")]
24+
[InlineData("MyCustomFunc(Name)", "The function name `MyCustomFunc` was not recognized.")]
25+
[InlineData("notAFunction()", "The function name `notAFunction` was not recognized.")]
26+
public void UnknownFunctionNamesAreReportedGracefully(string expression, string expectedError)
27+
{
28+
var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
29+
Assert.False(result);
30+
Assert.Equal(expectedError, error);
31+
Assert.Null(compiled);
32+
}
33+
34+
[Theory]
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)
41+
{
42+
var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
43+
Assert.True(result, $"Failed to compile: {error}");
44+
Assert.NotNull(compiled);
45+
Assert.Null(error);
46+
}
47+
48+
[Theory]
49+
[InlineData("Contains(Name, 'test') ci")]
50+
[InlineData("StartsWith(Path, '/api') ci")]
51+
[InlineData("EndsWith(File, '.txt') ci")]
52+
[InlineData("IsMatch(Email, '@example') ci")]
53+
[InlineData("IndexOfMatch(Text, 'pattern') ci")]
54+
[InlineData("IndexOf(Name, 'x') ci")]
55+
[InlineData("Name = 'test' ci")]
56+
[InlineData("Name <> 'test' ci")]
57+
[InlineData("Name like '%test%' ci")]
58+
public void ValidCiModifierUsageCompiles(string expression)
59+
{
60+
var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
61+
Assert.True(result, $"Failed to compile: {error}");
62+
Assert.NotNull(compiled);
63+
Assert.Null(error);
64+
}
65+
66+
[Fact]
67+
public void FirstErrorIsReportedInComplexExpressions()
68+
{
69+
var expression = "UnknownFunc() and Length(Value) > 5";
70+
var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
71+
72+
Assert.False(result);
73+
Assert.Null(compiled);
74+
75+
// Should report the first error encountered
76+
Assert.Contains("UnknownFunc", error);
77+
Assert.NotNull(error);
78+
}
79+
80+
[Fact]
81+
public void ValidExpressionsStillCompileWithoutErrors()
82+
{
83+
var validExpressions = new[]
84+
{
85+
"IsMatch(Name, '^[A-Z]')",
86+
"IndexOfMatch(Text, '\\d+')",
87+
"Contains(Name, 'test') ci",
88+
"Length(Items) > 5",
89+
"Round(Value, 2)",
90+
"TypeOf(Data) = 'String'",
91+
"Name like '%test%'",
92+
"StartsWith(Path, '/') and EndsWith(Path, '.json')"
93+
};
94+
95+
foreach (var expr in validExpressions)
96+
{
97+
var result = SerilogExpression.TryCompile(expr, out var compiled, out var error);
98+
Assert.True(result, $"Failed to compile: {expr}. Error: {error}");
99+
Assert.NotNull(compiled);
100+
Assert.Null(error);
101+
}
102+
}
103+
104+
[Fact]
105+
public void CompileMethodStillThrowsForInvalidExpressions()
106+
{
107+
// Ensure Compile() method (not TryCompile) maintains throwing behavior for invalid expressions
108+
Assert.Throws<ArgumentException>(() =>
109+
SerilogExpression.Compile("UnknownFunction()"));
110+
111+
Assert.Throws<ArgumentException>(() =>
112+
SerilogExpression.Compile("IsMatch(Name, '[invalid')"));
113+
114+
// CI modifier on non-supporting functions compiles with warning
115+
var compiledWithCi = SerilogExpression.Compile("Length(Name) ci");
116+
Assert.NotNull(compiledWithCi);
117+
118+
Assert.Throws<ArgumentException>(() =>
119+
SerilogExpression.Compile("IndexOfMatch(Text, '(?<')"));
120+
}
121+
122+
[Theory]
123+
[InlineData("IsMatch(Name, Name)")] // Non-constant pattern
124+
[InlineData("IndexOfMatch(Text, Value)")] // Non-constant pattern
125+
public void NonConstantRegexPatternsHandledGracefully(string expression)
126+
{
127+
// These should compile but may log warnings (not errors)
128+
var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
129+
130+
// These compile successfully but return undefined at runtime
131+
Assert.True(result);
132+
Assert.NotNull(compiled);
133+
Assert.Null(error);
134+
}
135+
136+
[Fact]
137+
public void RegexTimeoutIsRespected()
138+
{
139+
// This regex should compile fine - timeout only matters at runtime
140+
var expression = @"IsMatch(Text, '(a+)+b')";
141+
142+
var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
143+
144+
Assert.True(result);
145+
Assert.NotNull(compiled);
146+
Assert.Null(error);
147+
}
148+
149+
[Fact]
150+
public void ComplexExpressionsReportFirstError()
151+
{
152+
var expression = "UnknownFunc1() or Length(Value) > 5";
153+
var result = SerilogExpression.TryCompile(expression, out var compiled, out var error);
154+
155+
Assert.False(result);
156+
Assert.Null(compiled);
157+
Assert.NotNull(error);
158+
159+
// Should report the first error encountered during compilation
160+
Assert.Contains("UnknownFunc1", 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+
}
183+
}
184+
}

0 commit comments

Comments
 (0)