Skip to content

Commit

Permalink
Add new rule AG0040 for detecting and converting logs to templates (#184
Browse files Browse the repository at this point in the history 
)

* Add new rule AG0040 for detecting and converting logs to templates

* fix tests

* fix numbering

* Update src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx

Co-authored-by: Szabó, Péter <me@peterszabo.dev>

---------

Co-authored-by: Joel Dickson <Joel.Dickson@agoda.com>
Co-authored-by: Joel Dickson <joeldickson@users.noreply.github.com>
Co-authored-by: Szabó, Péter <me@peterszabo.dev>
  • Loading branch information
@joeldickson @dicko2 @szaboopeeter
4 people authored Oct 13, 2024
1 parent 63f0bc8 commit 5fe7cb1
Show file tree
Hide file tree
Showing 10 changed files with 627 additions and 1 deletion.
2 changes: 2 additions & 0 deletions src/Agoda.Analyzers.Test/Agoda.Analyzers.Test.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,14 @@
<PackageReference Include="Microsoft.CodeAnalysis.Testing.Verifiers.NUnit" Version="1.1.1" />
<PackageReference Include="Microsoft.CodeAnalysis.Workspaces.Common" Version="4.5.0" />
<PackageReference Include="Microsoft.Composition" Version="1.0.27" />
<PackageReference Include="Microsoft.Extensions.Logging" Version="6.0.0" />
<PackageReference Include="Microsoft.Playwright" Version="1.47.0" />
<PackageReference Include="Microsoft.Web.Infrastructure" Version="1.0.0.0" />
<PackageReference Include="Newtonsoft.Json" Version="13.0.1" />
<PackageReference Include="NUnit" Version="3.13.3" />
<PackageReference Include="NUnit3TestAdapter" Version="4.4.2" />
<PackageReference Include="Selenium.WebDriver" Version="3.14.0" />
<PackageReference Include="Serilog" Version="4.0.1" />
<PackageReference Include="Shouldly" Version="3.0.0" />
<PackageReference Include="System.Collections" Version="4.3.0" />
<PackageReference Include="System.Collections.Immutable" Version="7.0.0" />
Expand Down
216 changes: 216 additions & 0 deletions src/Agoda.Analyzers.Test/CodeFixes/AgodaCustom/AG0041LogTemplateAnalyzerTests.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Threading;
using System.Threading.Tasks;
using Agoda.Analyzers.AgodaCustom;
using Microsoft.CodeAnalysis.CSharp.Testing;
using Microsoft.CodeAnalysis.Testing;
using Microsoft.CodeAnalysis.Testing.Verifiers;
using NUnit.Framework;

namespace Agoda.Analyzers.Test.AgodaCustom;

public class AG0041LogTemplateAnalyzerTests
{
private static IEnumerable<TestCaseData> TestCases()
{
yield return new TestCaseData(new TestCase
{
Usings = "using Microsoft.Extensions.Logging;",
SetupCode = "private readonly ILogger _logger;",
LogStatement = "_logger.LogInformation($\"User {name} is {age} years old\");",
ExpectedFix = "_logger.LogInformation(\"User {Name} is {Age} years old\", name, age);",
ExpectedDiagnostics = new[]
{
new DiagnosticResult(AG0041LogTemplateAnalyzer.Rule)
.WithSpan(13, 36, 13, 69)
.WithArguments("string interpolation")
}
}).SetName("ILogger with string interpolation");

yield return new TestCaseData(new TestCase
{
Usings = "using Serilog;",
SetupCode = "private readonly ILogger _logger;",
LogStatement = "_logger.Information(\"User \" + name + \" is \" + age + \" years old\");",
ExpectedFix = "_logger.Information(\"User {Name} is {Age} years old\", name, age);",
ExpectedDiagnostics = new[]
{
new DiagnosticResult(AG0041LogTemplateAnalyzer.Rule)
.WithSpan(13, 33, 13, 77)
.WithArguments("string concatenation")
}
}).SetName("Serilog with string concatenation");

yield return new TestCaseData(new TestCase
{
Usings = "using Microsoft.Extensions.Logging;",
SetupCode = "private readonly ILogger _logger;",
LogStatement = "_logger.LogWarning(\"User {Name} is {Age} years old\", name, age);",
ExpectedFix = "_logger.LogWarning(\"User {Name} is {Age} years old\", name, age);",
ExpectedDiagnostics = new DiagnosticResult[] { }
}).SetName("ILogger with correct usage (no diagnostics)");

yield return new TestCaseData(new TestCase
{
Usings = "using Microsoft.Extensions.Logging;",
SetupCode = "private readonly ILogger _logger;private Exception ex;",
LogStatement = "_logger.LogError($\"Error occurred: {ex.Message}\");",
ExpectedFix = "_logger.LogError(\"Error occurred: {Ex.Message}\", ex.Message);",
ExpectedDiagnostics = new[]
{
new DiagnosticResult(AG0041LogTemplateAnalyzer.Rule)
.WithSpan(13, 30, 13, 61)
.WithArguments("string interpolation")
}
}).SetName("ILogger with string interpolation - error logging");

yield return new TestCaseData(new TestCase
{
Usings = "using Serilog;",
SetupCode = "private readonly ILogger _logger;private string debugInfo;",
LogStatement = "_logger.Debug(\"Debug info: \" + debugInfo);",
ExpectedFix = "_logger.Debug(\"Debug info: {DebugInfo}\", debugInfo);",
ExpectedDiagnostics = new[]
{
new DiagnosticResult(AG0041LogTemplateAnalyzer.Rule)
.WithSpan(13, 27, 13, 53)
.WithArguments("string concatenation")
}
}).SetName("Serilog with string concatenation - debug logging");

yield return new TestCaseData(new TestCase
{
Usings = "using Microsoft.Extensions.Logging;",
SetupCode = "private readonly ILogger _logger;private int itemId;private int userId;",
LogStatement = "_logger.LogInformation(\"Processing item {ItemId} for user {UserId}\", itemId, userId);",
ExpectedFix = "_logger.LogInformation(\"Processing item {ItemId} for user {UserId}\", itemId, userId);",
ExpectedDiagnostics = new DiagnosticResult[] { }
}).SetName("ILogger with correct usage - multiple parameters");

yield return new TestCaseData(new TestCase
{
Usings = "using Serilog;",
SetupCode = "private readonly ILogger _logger;private string ip;",
LogStatement = "_logger.Information($\"Received request from {ip} at {DateTime.Now}\");",
ExpectedFix = "_logger.Information(\"Received request from {Ip} at {DateTime.Now}\", ip, DateTime.Now);",
ExpectedDiagnostics = new[]
{
new DiagnosticResult(AG0041LogTemplateAnalyzer.Rule)
.WithSpan(13, 33, 13, 80)
.WithArguments("string interpolation")
}
}).SetName("Serilog with string interpolation - complex expression");

yield return new TestCaseData(new TestCase
{
Usings = "using Microsoft.Extensions.Logging;\nusing Serilog;",
SetupCode =
"private readonly Microsoft.Extensions.Logging.ILogger _msLogger;\nprivate readonly Serilog.ILogger _seriLogger;private string info;",
LogStatement =
"_msLogger.LogInformation($\"MS: {info}\");\n _seriLogger.Information(\"Seri: \" + info);",
ExpectedFix =
"_msLogger.LogInformation(\"MS: {Info}\", info);\n _seriLogger.Information(\"Seri: {Info}\", info);",
ExpectedDiagnostics = new[]
{
new DiagnosticResult(AG0041LogTemplateAnalyzer.Rule)
.WithSpan(15, 38, 15, 51)
.WithArguments("string interpolation"),
new DiagnosticResult(AG0041LogTemplateAnalyzer.Rule)
.WithSpan(16, 37, 16, 52)
.WithArguments("string concatenation")
}
}).SetName("Multiple loggers - both with issues");

yield return new TestCaseData(new TestCase
{
Usings = "using Microsoft.Extensions.Logging;",
SetupCode = "private readonly ILogger _logger;private string username;private string ipAddress;",
LogStatement =
"_logger.LogInformation(\"User {Username} logged in from {IpAddress}\", username, ipAddress);",
ExpectedFix =
"_logger.LogInformation(\"User {Username} logged in from {IpAddress}\", username, ipAddress);",
ExpectedDiagnostics = new DiagnosticResult[] { }
}).SetName("ILogger with correct usage - multiple parameters with PascalCase");

yield return new TestCaseData(new TestCase
{
Usings = "using Serilog;",
SetupCode = "private readonly ILogger _logger;",
LogStatement = "_logger.Information(\"Status: {@Status}\", new { Code = 200, Message = \"OK\" });",
ExpectedFix = "_logger.Information(\"Status: {@Status}\", new { Code = 200, Message = \"OK\" });",
ExpectedDiagnostics = new DiagnosticResult[] { }
}).SetName("Serilog with correct usage - complex object logging");
}

[Test]
[TestCaseSource(nameof(TestCases))]
public async Task TestLogTemplateUsage(TestCase testCase)
{
var test = $@"
using System;
{testCase.Usings}
namespace TestNamespace
{{
public class TestClass
{{
{testCase.SetupCode}
public void TestMethod(string name, int age)
{{
{testCase.LogStatement}
}}
}}
}}";

var expected = $@"
using System;
{testCase.Usings}
namespace TestNamespace
{{
public class TestClass
{{
{testCase.SetupCode}
public void TestMethod(string name, int age)
{{
{testCase.ExpectedFix}
}}
}}
}}";

var codeFixTest = new CodeFixTest(test, expected, testCase.ExpectedDiagnostics);

await codeFixTest.RunAsync(CancellationToken.None);
}

private class CodeFixTest : CSharpCodeFixTest<AG0041LogTemplateAnalyzer, AG0041CodeFixProvider, NUnitVerifier>
{
public CodeFixTest(
string source,
string fixedSource,
IEnumerable<DiagnosticResult> expectedDiagnostics)
{
TestCode = source;
FixedCode = fixedSource;
ExpectedDiagnostics.AddRange(expectedDiagnostics);

ReferenceAssemblies = ReferenceAssemblies.Default
.AddPackages(ImmutableArray.Create(
new PackageIdentity("Microsoft.Extensions.Logging.Abstractions", "6.0.0"),
new PackageIdentity("Serilog", "2.10.0")
));
}
}

public class TestCase
{
public string Usings { get; set; }
public string SetupCode { get; set; }
public string LogStatement { get; set; }
public string ExpectedFix { get; set; }
public DiagnosticResult[] ExpectedDiagnostics { get; set; }
}
}
2 changes: 1 addition & 1 deletion src/Agoda.Analyzers.Test/Helpers/DiagnosticVerifier.Helper.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ private static Diagnostic[] SortDistinctDiagnostics(IEnumerable<Diagnostic> diag
/// <param name="filenames">The filenames or null if the default filename should be used</param>
/// <returns>A collection of <see cref="Diagnostic"/>s that surfaced in the source code, sorted by
/// <see cref="Diagnostic.Location"/>.</returns>
private Task<ImmutableArray<Diagnostic>> GetSortedDiagnosticsAsync(string[] sources, string language, ImmutableArray<DiagnosticAnalyzer> analyzers, CancellationToken cancellationToken, string[] filenames)
internal Task<ImmutableArray<Diagnostic>> GetSortedDiagnosticsAsync(string[] sources, string language, ImmutableArray<DiagnosticAnalyzer> analyzers, CancellationToken cancellationToken, string[] filenames)
{
return GetSortedDiagnosticsFromDocumentsAsync(analyzers, GetDocuments(sources, language, filenames), cancellationToken);
}
Expand Down
140 changes: 140 additions & 0 deletions src/Agoda.Analyzers/AgodaCustom/AG0041CodeFixProvider.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Composition;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;

namespace Agoda.Analyzers.AgodaCustom
{
[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(AG0041CodeFixProvider)), Shared]
public class AG0041CodeFixProvider : CodeFixProvider
{
private const string Title = "Use message template";

public sealed override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(AG0041LogTemplateAnalyzer.DiagnosticId);

public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;

public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
var diagnostic = context.Diagnostics.First();
var diagnosticSpan = diagnostic.Location.SourceSpan;

var invocation = root.FindToken(diagnosticSpan.Start).Parent.AncestorsAndSelf().OfType<InvocationExpressionSyntax>().First();

context.RegisterCodeFix(
CodeAction.Create(
title: Title,
createChangedDocument: c => ConvertToMessageTemplateAsync(context.Document, invocation, c),
equivalenceKey: Title),
diagnostic);
}
private async Task<Document> ConvertToMessageTemplateAsync(Document document, InvocationExpressionSyntax invocation, CancellationToken cancellationToken)
{
var semanticModel = await document.GetSemanticModelAsync(cancellationToken);
var root = await document.GetSyntaxRootAsync(cancellationToken);

var newInvocation = ConvertToMessageTemplate(invocation, semanticModel);

var newRoot = root.ReplaceNode(invocation, newInvocation);
return document.WithSyntaxRoot(newRoot);
}

private InvocationExpressionSyntax ConvertToMessageTemplate(InvocationExpressionSyntax invocation, SemanticModel semanticModel)
{
var arguments = invocation.ArgumentList.Arguments;
var firstArgument = arguments.First();

if (firstArgument.Expression is InterpolatedStringExpressionSyntax interpolatedString)
{
var (template, parameters) = ExtractFromInterpolatedString(interpolatedString);
return CreateNewInvocation(invocation, template, parameters);
}
else if (firstArgument.Expression is BinaryExpressionSyntax binaryExpression &&
binaryExpression.IsKind(SyntaxKind.AddExpression))
{
var (template, parameters) = ExtractFromConcatenation(binaryExpression);
return CreateNewInvocation(invocation, template, parameters);
}

// If we're here, the format is already correct, so we return the original invocation
return invocation;
}

private (string template, SeparatedSyntaxList<ArgumentSyntax> parameters) ExtractFromInterpolatedString(InterpolatedStringExpressionSyntax interpolatedString)
{
var template = "";
var parameters = new SeparatedSyntaxList<ArgumentSyntax>();

foreach (var content in interpolatedString.Contents)
{
if (content is InterpolatedStringTextSyntax text)
{
template += text.TextToken.ValueText;
}
else if (content is InterpolationSyntax interpolation)
{
var paramName = interpolation.Expression.ToString();
template += $"{{{char.ToUpper(paramName[0]) + paramName.Substring(1)}}}";
parameters = parameters.Add(SyntaxFactory.Argument(interpolation.Expression));
}
}

return (template, parameters);
}

private (string template, SeparatedSyntaxList<ArgumentSyntax> parameters) ExtractFromConcatenation(BinaryExpressionSyntax expression)
{
var template = "";
var parameters = new SeparatedSyntaxList<ArgumentSyntax>();

void ExtractPart(ExpressionSyntax expr)
{
if (expr is LiteralExpressionSyntax literal)
{
template += literal.Token.ValueText;
}
else
{
var paramName = expr.ToString();
template += $"{{{char.ToUpper(paramName[0]) + paramName.Substring(1)}}}";
parameters = parameters.Add(SyntaxFactory.Argument(expr));
}
}

void TraverseConcatenation(BinaryExpressionSyntax binaryExpr)
{
if (binaryExpr.Left is BinaryExpressionSyntax leftBinary && leftBinary.IsKind(SyntaxKind.AddExpression))
{
TraverseConcatenation(leftBinary);
}
else
{
ExtractPart(binaryExpr.Left);
}

ExtractPart(binaryExpr.Right);
}

TraverseConcatenation(expression);

return (template, parameters);
}

private InvocationExpressionSyntax CreateNewInvocation(InvocationExpressionSyntax originalInvocation, string template, SeparatedSyntaxList<ArgumentSyntax> parameters)
{
var newArguments = new SeparatedSyntaxList<ArgumentSyntax>()
.Add(SyntaxFactory.Argument(SyntaxFactory.LiteralExpression(SyntaxKind.StringLiteralExpression, SyntaxFactory.Literal(template))))
.AddRange(parameters);

return originalInvocation.WithArgumentList(SyntaxFactory.ArgumentList(newArguments));
}
}
}
Loading

0 comments on commit 5fe7cb1

Please sign in to comment.