Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New rule S6674: Log message template should be syntactically correct #8818

Merged
merged 4 commits into from
Feb 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions analyzers/rspec/cs/S6674.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
<p>A <a href="https://messagetemplates.org/">message template</a> must conform to the specification. The rule raises an issue if the template string
violates the template string grammar.</p>
<h2>Why is this an issue?</h2>
<p>A message template needs to comply with a set of rules. <a
href="https://learn.microsoft.com/en-us/dotnet/core/extensions/logging-providers">Logging provider</a> parse the template and enrich log entries with
the information found in the template. An unparsable message template leads to corrupted log entries and might result in a loss of information in the
logs.</p>
<p>The rule covers the following logging frameworks:</p>
<ul>
<li> Nuget package - <a href="https://www.nuget.org/packages/Serilog">Serilog</a> </li>
<li> Nuget package - <a href="https://www.nuget.org/packages/NLog">Nlog</a> </li>
<li> Nuget package - <a href="https://www.nuget.org/packages/Microsoft.Extensions.Logging">Microsoft.Extensions.Logging</a> </li>
</ul>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
logger.LogError("Login failed for {User", user); // Noncompliant: Missing closing bracket
logger.LogError("Login failed for {{User}", user); // Noncompliant: Opening bracket is escaped
logger.LogError("Login failed for {&amp;User}", user); // Noncompliant: Only '@' and '$' are allowed as prefix
logger.LogError("Login failed for {User-Name}", user); // Noncompliant: Only chars, numbers, and underscore are allowed for placeholders
logger.LogDebug("Retry attempt {Cnt,r}", cnt); // Noncompliant: The alignment specifier must be numeric
logger.LogDebug("Retry attempt {Cnt:}", cnt); // Noncompliant: Empty format specifier is not allowed
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
logger.LogError("Login failed for {User}", user);
logger.LogError("Login failed for {{{User}}}", user);
logger.LogError("Login failed for {User}", user);
logger.LogError("Login failed for {User_Name}", user);
logger.LogDebug("Retry attempt {Cnt,-5}", cnt);
logger.LogDebug("Retry attempt {Cnt:000}", cnt);
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> Message Templates - <a href="https://messagetemplates.org/">Message template specification</a> </li>
<li> Microsoft Learn - <a
href="https://learn.microsoft.com/en-us/dotnet/core/extensions/logging?tabs=command-line#log-message-template-formatting">Log message template
formatting</a> </li>
<li> NLog - <a href="https://github.com/NLog/NLog/wiki/How-to-use-structured-logging">How to use structured logging</a> </li>
<li> Serilog - <a href="https://github.com/serilog/serilog/wiki/Structured-Data">Structured Data</a> </li>
<li> Serilog - <a
href="https://github.com/Suchiman/SerilogAnalyzer/blob/master/README.md#serilog002-message-template-syntax-verifier"><code>Serilog002</code>:
Message template syntax verifier</a> </li>
</ul>

15 changes: 15 additions & 0 deletions analyzers/rspec/cs/S6674.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"title": "Log message template should be syntactically correct",
"type": "BUG",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
"tags": ["logging"],
"defaultSeverity": "Critical",
"ruleSpecification": "RSPEC-6674",
"sqKey": "S6674",
"scope": "Main",
"quickfix": "infeasible"
}
1 change: 1 addition & 0 deletions analyzers/rspec/cs/Sonar_way_profile.json
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@
"S6640",
"S6667",
"S6668",
"S6674",
"S6677",
"S6678",
"S6797",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,5 @@ public interface IMessageTemplateCheck
{
DiagnosticDescriptor Rule { get; }

void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, Helpers.MessageTemplates.Placeholder[] placeholders);
void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, MessageTemplatesParser.Placeholder[] placeholders);
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@

using SonarAnalyzer.Rules.MessageTemplates;

using static Roslyn.Utilities.SonarAnalyzer.Shared.LoggingFrameworkMethods;

namespace SonarAnalyzer.Rules.CSharp;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
Expand All @@ -46,9 +44,9 @@ protected override void Initialize(SonarAnalysisContext context) =>
var invocation = (InvocationExpressionSyntax)c.Node;
var enabledChecks = Checks.Where(x => x.Rule.IsEnabled(c)).ToArray();
if (enabledChecks.Length > 0
&& TemplateArgument(invocation, c.SemanticModel) is { } argument
&& MessageTemplateExtractor.TemplateArgument(invocation, c.SemanticModel) is { } argument
&& HasValidExpression(argument)
&& Helpers.MessageTemplates.Parse(argument.Expression.ToString()) is { Success: true } result)
&& MessageTemplatesParser.Parse(argument.Expression.ToString()) is { Success: true } result)
{
foreach (var check in enabledChecks)
{
Expand All @@ -58,29 +56,6 @@ protected override void Initialize(SonarAnalysisContext context) =>
},
SyntaxKind.InvocationExpression);

private static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation, SemanticModel model) =>
TemplateArgument(invocation, model, KnownType.Microsoft_Extensions_Logging_LoggerExtensions, MicrosoftExtensionsLogging, "message")
?? TemplateArgument(invocation, model, KnownType.Serilog_Log, Serilog, "messageTemplate")
?? TemplateArgument(invocation, model, KnownType.Serilog_ILogger, Serilog, "messageTemplate", checkDerivedTypes: true)
?? TemplateArgument(invocation, model, KnownType.NLog_ILoggerExtensions, NLogLoggingMethods, "message")
?? TemplateArgument(invocation, model, KnownType.NLog_ILogger, NLogLoggingMethods, "message", checkDerivedTypes: true)
?? TemplateArgument(invocation, model, KnownType.NLog_ILoggerBase, NLogILoggerBase, "message", checkDerivedTypes: true);

private static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation,
SemanticModel model,
KnownType type,
ICollection<string> methods,
string template,
bool checkDerivedTypes = false) =>
methods.Contains(invocation.GetIdentifier().ToString())
&& model.GetSymbolInfo(invocation).Symbol is IMethodSymbol method
&& method.HasContainingType(type, checkDerivedTypes)
&& CSharpFacade.Instance.MethodParameterLookup(invocation, method) is { } lookup
&& lookup.TryGetSyntax(template, out var argumentsFound) // Fetch Argument.Expression with IParameterSymbol.Name == templateName
&& argumentsFound.Length == 1
? (ArgumentSyntax)argumentsFound[0].Parent
: null;

private static bool HasValidExpression(ArgumentSyntax argument) =>
argument.Expression.DescendantNodes().All(x => x.IsAnyKind(ValidTemplateKinds));
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2024 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using static Roslyn.Utilities.SonarAnalyzer.Shared.LoggingFrameworkMethods;

namespace SonarAnalyzer.Rules.MessageTemplates;

internal static class MessageTemplateExtractor
{
public static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation, SemanticModel model) =>
TemplateArgument(invocation, model, KnownType.Microsoft_Extensions_Logging_LoggerExtensions, MicrosoftExtensionsLogging, "message")
?? TemplateArgument(invocation, model, KnownType.Serilog_Log, Serilog, "messageTemplate")
?? TemplateArgument(invocation, model, KnownType.Serilog_ILogger, Serilog, "messageTemplate", checkDerivedTypes: true)
?? TemplateArgument(invocation, model, KnownType.NLog_ILoggerExtensions, NLogLoggingMethods, "message")
?? TemplateArgument(invocation, model, KnownType.NLog_ILogger, NLogLoggingMethods, "message", checkDerivedTypes: true)
?? TemplateArgument(invocation, model, KnownType.NLog_ILoggerBase, NLogILoggerBase, "message", checkDerivedTypes: true);

private static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation,
SemanticModel model,
KnownType type,
ICollection<string> methods,
string template,
bool checkDerivedTypes = false) =>
methods.Contains(invocation.GetIdentifier().ToString())
&& model.GetSymbolInfo(invocation).Symbol is IMethodSymbol method
&& method.HasContainingType(type, checkDerivedTypes)
&& CSharpFacade.Instance.MethodParameterLookup(invocation, method) is { } lookup
&& lookup.TryGetSyntax(template, out var argumentsFound) // Fetch Argument.Expression with IParameterSymbol.Name == templateName
&& argumentsFound.Length == 1
? (ArgumentSyntax)argumentsFound[0].Parent
: null;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2024 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Text.RegularExpressions;
using SonarAnalyzer.Rules.MessageTemplates;

namespace SonarAnalyzer.Rules.CSharp;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class MessageTemplatesShouldBeCorrect : SonarDiagnosticAnalyzer
{
private const string DiagnosticId = "S6674";
private const string MessageFormat = "Log message template {0}.";

private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(c =>
{
var invocation = (InvocationExpressionSyntax)c.Node;
if (MessageTemplateExtractor.TemplateArgument(invocation, c.SemanticModel) is { } argument
&& argument.Expression.IsKind(SyntaxKind.StringLiteralExpression)
&& TemplateValidator.ContainsErrors(argument.Expression.ToString(), out var errors))
{
var templateStart = argument.Expression.GetLocation().SourceSpan.Start;
foreach (var error in errors)
{
var location = Location.Create(c.Tree, new(templateStart + error.Start, error.Length));
c.ReportIssue(Diagnostic.Create(Rule, location, error.Message));
}
}
},
SyntaxKind.InvocationExpression);

private static class TemplateValidator
{
private const int EmptyPlaceholderSize = 2; // "{}"

private const string TextPattern = @"([^\{]|\{\{|\}\})+";
private const string HolePattern = @"{(?<Placeholder>[^\}]*)}";
private const string TemplatePattern = $"^({TextPattern}|{HolePattern})*$";
// This is similar to the regex used for MessageTemplatesAnalyzer, but it is far more permissive.
// The goal is to manually parse the placeholders, so that we can report more specific issues than just "malformed template".
private static readonly Regex TemplateRegex = new(TemplatePattern, RegexOptions.Compiled, TimeSpan.FromMilliseconds(300));
private static readonly Regex PlaceholderNameRegex = new("^[0-9a-zA-Z_]+$", RegexOptions.Compiled, RegexConstants.DefaultTimeout);
private static readonly Regex PlaceholderAlignmentRegex = new("^-?[0-9]+$", RegexOptions.Compiled, RegexConstants.DefaultTimeout);

public static bool ContainsErrors(string template, out List<ParsingError> errors)
{
var result = MessageTemplatesParser.Parse(template, TemplateRegex);
errors = result.Success
? result.Placeholders.Select(ParsePlaceholder).Where(x => x is not null).ToList()
: [new("should be syntactically correct", 0, template.Length)];
return errors.Count > 0;
}

private static ParsingError ParsePlaceholder(MessageTemplatesParser.Placeholder placeholder)
{
if (placeholder.Length == 0)
{
return new("should not contain empty placeholder", placeholder.Start - 1, EmptyPlaceholderSize);
}
var parts = Split(placeholder.Name);
return "🔥" switch
{
_ when !PlaceholderNameRegex.SafeIsMatch(parts.Name) =>
new($"placeholder '{parts.Name}' should only contain letters, numbers, and underscore", placeholder),

_ when parts.Alignment is not null && !PlaceholderAlignmentRegex.SafeIsMatch(parts.Alignment) =>
new($"placeholder '{parts.Name}' should have numeric alignment instead of '{parts.Alignment}'", placeholder),

_ when parts.Format == string.Empty =>
new($"placeholder '{parts.Name}' should not have empty format", placeholder),

_ => null,
};
}

// pattern is: name[,alignment][:format]
private static Parts Split(string placeholder)
{
string alignment = null;
string format = null;

var formatIndex = placeholder.IndexOf(':');
var alignmentIndex = placeholder.IndexOf(',');
if (formatIndex >= 0 && alignmentIndex > formatIndex)
{
// example {name:format,alignment}
// ^^^^^^^^^^^^^^^^ all of this is format, need to reset alignment
alignmentIndex = -1;
}

if (formatIndex == -1)
{
formatIndex = placeholder.Length;
}
else
{
format = placeholder.Substring(formatIndex + 1);
}
if (alignmentIndex == -1)
{
alignmentIndex = formatIndex;
}
else
{
alignment = placeholder.Substring(alignmentIndex + 1, formatIndex - alignmentIndex - 1);
}

var start = placeholder[0] is '@' or '$' ? 1 : 0; // skip prefix
var name = placeholder.Substring(start, alignmentIndex - start);
return new(name, alignment, format);
}

private sealed record Parts(string Name, string Alignment, string Format);

public sealed record ParsingError(string Message, int Start, int Length)
{
public ParsingError(string message, MessageTemplatesParser.Placeholder placeholder)
: this(message, placeholder.Start, placeholder.Length)
{ }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public sealed class NamedPlaceholdersShouldBeUnique : IMessageTemplateCheck

public DiagnosticDescriptor Rule => S6677;

public void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, Helpers.MessageTemplates.Placeholder[] placeholders)
public void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, MessageTemplatesParser.Placeholder[] placeholders)
{
var duplicatedGroups = placeholders
.Where(x => x.Name != "_" && !int.TryParse(x.Name, out _)) // exclude wildcard "_" and index placeholders like {42}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using static SonarAnalyzer.Helpers.MessageTemplates;
using static SonarAnalyzer.Helpers.MessageTemplatesParser;

namespace SonarAnalyzer.Rules.MessageTemplates;

Expand All @@ -31,7 +31,7 @@ public sealed class UsePascalCaseForNamedPlaceHolders : IMessageTemplateCheck

public DiagnosticDescriptor Rule => S6678;

public void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, Helpers.MessageTemplates.Placeholder[] placeholders)
public void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, Placeholder[] placeholders)
{
var nonPascalCasePlaceholders = placeholders.Where(x => char.IsLower(x.Name[0])).ToArray();
if (nonPascalCasePlaceholders.Length > 0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ namespace SonarAnalyzer.Helpers;
/// <summary>
/// Grammar can be found <a href="https://github.com/messagetemplates/grammar"> here</a>.
/// </summary>
public static class MessageTemplates
public static class MessageTemplatesParser
{
private const string NamePattern = "[0-9a-zA-Z_]+";
private const string PlaceholderPattern = $"(?<Placeholder>{NamePattern})";
Expand All @@ -42,9 +42,12 @@ public static class MessageTemplates
/// Matches and extracts placeholders from a template string.
/// For more info, see <a href="https://messagetemplates.org/">Message Templates</a>.
/// </summary>
public static ParseResult Parse(string template)
public static ParseResult Parse(string template) =>
Parse(template, TemplateRegex);

public static ParseResult Parse(string template, Regex regex)
{
if (TemplateRegex.SafeMatch(template) is { Success: true } match)
if (regex.SafeMatch(template) is { Success: true } match)
{
var placeholders = match.Groups["Placeholder"].Captures
.OfType<Capture>()
Expand Down
Loading