-
Notifications
You must be signed in to change notification settings - Fork 228
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
New rule S6673: Log message template placeholders should be in the ri…
…ght order (#8859)
- Loading branch information
1 parent
52341f9
commit 3cf7728
Showing
16 changed files
with
542 additions
and
14 deletions.
There are no files selected for viewing
10 changes: 10 additions & 0 deletions
10
...zers/its/expected/ManuallyAddedNoncompliantIssues.CS/S103-IntentionalFindings-net8.0.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "Issues": [ | ||
| { | ||
| "Id": "S103", | ||
| "Message": "Split this 219 characters long line (which is greater than 200 authorized).", | ||
| "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6673.cs#L9", | ||
| "Location": "Line 9 Position 1-220" | ||
| } | ||
| ] | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
10 changes: 10 additions & 0 deletions
10
...ers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6673-IntentionalFindings-net8.0.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| { | ||
| "Issues": [ | ||
| { | ||
| "Id": "S6673", | ||
| "Message": "Template placeholders should be in the right order: placeholder \u0027Arg\u0027 does not match with argument \u0027anotherArg\u0027.", | ||
| "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6673.cs#L9", | ||
| "Location": "Line 9 Position 42-45" | ||
| } | ||
| ] | ||
| } |
11 changes: 11 additions & 0 deletions
11
analyzers/its/sources/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6673.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| using System; | ||
| using Microsoft.Extensions.Logging; | ||
|
|
||
| namespace IntentionalFindings | ||
| { | ||
| public class S6673 | ||
| { | ||
| private void Log(ILogger logger, int arg, int anotherArg) => | ||
| logger.LogInformation("Arg: {Arg} {AnotherArg}", anotherArg, arg); // Noncompliant (S6673) {{Template placeholders should be in the right order: placeholder 'Arg' does not match with argument 'anotherArg'.}} | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| <p>The positions of arguments in a logging call should match the positions of their <a href="https://messagetemplates.org">message template</a> | ||
| placeholders.</p> | ||
| <h2>Why is this an issue?</h2> | ||
| <p>The placeholders of a <a href="https://messagetemplates.org">message template</a> are defined by their name and their position. Log methods specify | ||
| the values for the placeholder at runtime by passing them in a <a | ||
| href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/params">params array</a>:</p> | ||
| <pre data-diff-id="1" data-diff-type="compliant"> | ||
| logger.LogError("{First} placeholder and {Second} one.", first, second); | ||
| </pre> | ||
| <p>This rule raises an issue if the position of an argument does not match the position of the corresponding placeholder:</p> | ||
| <pre data-diff-id="1" data-diff-type="noncompliant"> | ||
| // 'first' and 'second' are swapped | ||
| logger.LogError("{First} placeholder and {Second} one.", second, first); | ||
| // ^^^^^^ ^^^^^ | ||
| </pre> | ||
| <h3>What is the potential impact?</h3> | ||
| <p>Logging providers use placeholder names to create key/value pairs in the log entry. The key corresponds to the placeholder and the value is the | ||
| argument passed in the log call.</p> | ||
| <p>If the positions of the placeholder and the argument do not match, the value is associated with the wrong key. This corrupts the logs entry and | ||
| makes log analytics unreliable.</p> | ||
| <h2>How to fix it</h2> | ||
| <p>Make sure that the placeholder positions and the argument positions match. Use local variables, fields, or properties for the arguments and name | ||
| the placeholders accordingly.</p> | ||
| <h3>Code examples</h3> | ||
| <h4>Noncompliant code example</h4> | ||
| <p>'path' and 'fileName' are swapped and therefore assigned to the wrong placeholders.</p> | ||
| <pre data-diff-id="2" data-diff-type="noncompliant"> | ||
| logger.LogError("File {FileName} not found in folder {Path}", path, fileName); | ||
| // ^^^^ ^^^^^^^^ | ||
| </pre> | ||
| <h4>Compliant solution</h4> | ||
| <p>Swap the arguments.</p> | ||
| <pre data-diff-id="2" data-diff-type="compliant"> | ||
| logger.LogError("File {FileName} not found in folder {Path}", fileName, path); | ||
| </pre> | ||
| <h4>Noncompliant code example</h4> | ||
| <p>'Name' is detected but 'Folder' is not. The placeholder’s name should correspond to the name from the argument.</p> | ||
| <pre data-diff-id="3" data-diff-type="noncompliant"> | ||
| logger.LogError("File {Name} not found in folder {Folder}", file.DirectoryName, file.Name); | ||
| // ^^^^ | ||
| </pre> | ||
| <h4>Compliant solution</h4> | ||
| <p>Swap the arguments and rename the placeholder to 'DirectoryName'.</p> | ||
| <pre data-diff-id="3" data-diff-type="compliant"> | ||
| logger.LogError("File {Name} not found in folder {DirectoryName}", file.Name, file.DirectoryName); | ||
| </pre> | ||
| <h4>Noncompliant code example</h4> | ||
| <p>Not detected: A name for the arguments can not be inferred. Use locals to support detection.</p> | ||
| <pre data-diff-id="4" data-diff-type="noncompliant"> | ||
| logger.LogError("Sum is {Sum} and product is {Product}", x * y, x + y); // Not detected | ||
| </pre> | ||
| <h4>Compliant solution</h4> | ||
| <p>Help detection by using arguments with a name.</p> | ||
| <pre data-diff-id="4" data-diff-type="compliant"> | ||
| var sum = x + y; | ||
| var product = x * y; | ||
| logger.LogError("Sum is {Sum} and product is {Product}", sum, product); | ||
| </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">Log message | ||
| template</a> </li> | ||
| <li> Serilog - <a href="https://github.com/serilog/serilog/wiki/Structured-Data">Structured Data</a> </li> | ||
| <li> NLog - <a href="https://github.com/NLog/NLog/wiki/How-to-use-structured-logging">How to use structured logging</a> </li> | ||
| </ul> | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| { | ||
| "title": "Log message template placeholders should be in the right order", | ||
| "type": "CODE_SMELL", | ||
| "status": "ready", | ||
| "remediation": { | ||
| "func": "Constant\/Issue", | ||
| "constantCost": "5min" | ||
| }, | ||
| "tags": [ | ||
| "logging" | ||
| ], | ||
| "defaultSeverity": "Major", | ||
| "ruleSpecification": "RSPEC-6673", | ||
| "sqKey": "S6673", | ||
| "scope": "Main", | ||
| "quickfix": "infeasible" | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
102 changes: 102 additions & 0 deletions
102
...SonarAnalyzer.CSharp/Rules/MessageTemplates/LoggingTemplatePlaceHoldersShouldBeInOrder.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| /* | ||
| * 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. | ||
| */ | ||
|
|
||
| namespace SonarAnalyzer.Rules.MessageTemplates; | ||
|
|
||
| public sealed class LoggingTemplatePlaceHoldersShouldBeInOrder : IMessageTemplateCheck | ||
| { | ||
| private const string DiagnosticId = "S6673"; | ||
| private const string MessageFormat = "Template placeholders should be in the right order: placeholder '{0}' does not match with argument '{1}'."; | ||
|
|
||
| internal static readonly DiagnosticDescriptor S6673 = DescriptorFactory.Create(DiagnosticId, MessageFormat); | ||
|
|
||
| public DiagnosticDescriptor Rule => S6673; | ||
|
|
||
| public void Execute(SonarSyntaxNodeReportingContext context, InvocationExpressionSyntax invocation, ArgumentSyntax templateArgument, MessageTemplatesParser.Placeholder[] placeholders) | ||
| { | ||
| var methodSymbol = (IMethodSymbol)context.SemanticModel.GetSymbolInfo(invocation).Symbol; | ||
| var placeholderValues = PlaceholderValues(invocation, methodSymbol).ToImmutableArray(); | ||
| for (var i = 0; i < placeholders.Length; i++) | ||
| { | ||
| var placeholder = placeholders[i]; | ||
| if (placeholder.Name != "_" | ||
| && !int.TryParse(placeholder.Name, out _) | ||
| && Array.FindIndex(placeholders, x => x.Name == placeholder.Name) == i // don't raise for duplicate placeholders | ||
| && OutOfOrderPlaceholderValue(placeholder, i, placeholderValues) is { } outOfOrderArgument) | ||
| { | ||
| var templateStart = templateArgument.Expression.GetLocation().SourceSpan.Start; | ||
| var primaryLocation = Location.Create(context.Tree, new(templateStart + placeholder.Start, placeholder.Length)); | ||
| var secondaryLocation = outOfOrderArgument.GetLocation(); | ||
| context.ReportIssue(Diagnostic.Create(Rule, primaryLocation, [secondaryLocation], placeholder.Name, outOfOrderArgument.ToString())); | ||
| return; // only raise on the first out-of-order placeholder to make the rule less noisy | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static IEnumerable<SyntaxNode> PlaceholderValues(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol) | ||
| { | ||
| var parameters = methodSymbol.Parameters.Where(x => x.Name == "args" | ||
| || x.Name.StartsWith("argument") | ||
| || x.Name.StartsWith("propertyValue")) | ||
| .ToArray(); | ||
| if (parameters.Length == 0) | ||
| { | ||
| yield break; | ||
| } | ||
| var parameterLookup = CSharpFacade.Instance.MethodParameterLookup(invocation, methodSymbol); | ||
| foreach (var parameter in parameters) | ||
| { | ||
| if (parameterLookup.TryGetSyntax(parameter, out var expressions)) | ||
| { | ||
| foreach (var item in expressions) | ||
| { | ||
| yield return item; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static SyntaxNode OutOfOrderPlaceholderValue(MessageTemplatesParser.Placeholder placeholder, int placeholderIndex, ImmutableArray<SyntaxNode> placeholderValues) | ||
| { | ||
| if (placeholderIndex < placeholderValues.Length && MatchesName(placeholder.Name, placeholderValues[placeholderIndex])) | ||
| { | ||
| return null; | ||
| } | ||
| else | ||
| { | ||
| for (var i = 0; i < placeholderValues.Length; i++) | ||
| { | ||
| if (i != placeholderIndex && MatchesName(placeholder.Name, placeholderValues[i])) | ||
| { | ||
| return placeholderValues[placeholderIndex]; | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private static bool MatchesName(string placeholderName, SyntaxNode placeholderValue) => | ||
| placeholderValue switch | ||
| { | ||
| MemberAccessExpressionSyntax memberAccess => MatchesName(placeholderName, memberAccess.Name) || MatchesName(placeholderName, memberAccess.Expression), | ||
| ObjectCreationExpressionSyntax => false, | ||
| _ => new string(placeholderName.Where(char.IsLetterOrDigit).ToArray()).Equals(placeholderValue.ToString(), StringComparison.OrdinalIgnoreCase) | ||
| }; | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.