From 74b826e4a593b94d4aef255ac8944de89adef964 Mon Sep 17 00:00:00 2001 From: Rannes Date: Wed, 16 Oct 2024 11:42:51 +0700 Subject: [PATCH 1/7] feat: add BuildServiceProvider analyzer rule --- .../AgodaCustom/AG0043UnitTests.cs | 96 +++++++++++++++++ .../AG0043NoBuildServiceProvider.cs | 101 ++++++++++++++++++ .../CustomRulesResources.Designer.cs | 6 ++ .../AgodaCustom/CustomRulesResources.resx | 3 + 4 files changed, 206 insertions(+) create mode 100644 src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs create mode 100644 src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs diff --git a/src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs b/src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs new file mode 100644 index 0000000..3b80601 --- /dev/null +++ b/src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs @@ -0,0 +1,96 @@ +using System.Threading.Tasks; +using Agoda.Analyzers.AgodaCustom; +using Agoda.Analyzers.Test.Helpers; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Framework; +using Microsoft.Extensions.DependencyInjection; + +namespace Agoda.Analyzers.Test.AgodaCustom; + +[TestFixture] +class AG0043UnitTests : DiagnosticVerifier +{ + protected override DiagnosticAnalyzer DiagnosticAnalyzer => new AG0043NoBuildServiceProvider(); + + protected override string DiagnosticId => AG0043NoBuildServiceProvider.DIAGNOSTIC_ID; + + [Test] + public async Task BuildServiceProvider_Used_RaisesDiagnostic() + { + var services = new ServiceCollection(); + services.BuildServiceProvider(); + + var code = new CodeDescriptor + { + References = new[] + { + typeof(ServiceCollection).Assembly, + typeof(ServiceProvider).Assembly, + typeof(IServiceCollection).Assembly + }, + Code = @" + using Microsoft.Extensions.DependencyInjection; + + public class Program + { + public static void Main() + { + var services = new ServiceCollection(); + var provider = services.BuildServiceProvider(); + } + }" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(9, 53)); + } + + [Test] + public async Task OtherMethod_NoDiagnostic() + { + var code = new CodeDescriptor + { + References = new[] {typeof(ServiceCollection).Assembly}, + Code = @" + using Microsoft.Extensions.DependencyInjection; + + class TestClass + { + public void ConfigureServices() + { + var services = new ServiceCollection(); + services.AddSingleton(); + } + }" + }; + + await VerifyDiagnosticsAsync(code, EmptyDiagnosticResults); + } + + [Test] + public async Task BuildServiceProvider_WhenChaining_RaiseDiagnostic() + { + var code = new CodeDescriptor + { + References = new[] + { + typeof(ServiceCollection).Assembly, + typeof(ServiceProvider).Assembly, + typeof(IServiceCollection).Assembly + }, + Code = @" + using Microsoft.Extensions.DependencyInjection; + + public class Program + { + public static void Main() + { + var provider = new ServiceCollection() + .AddSingleton() + .BuildServiceProvider(); + } + }" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(10, 34)); + } +} \ No newline at end of file diff --git a/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs b/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs new file mode 100644 index 0000000..c38327b --- /dev/null +++ b/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs @@ -0,0 +1,101 @@ +using System.Collections.Immutable; +using System.Linq; +using Agoda.Analyzers.Helpers; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Agoda.Analyzers.AgodaCustom +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class AG0043NoBuildServiceProvider : DiagnosticAnalyzer + { + public const string DIAGNOSTIC_ID = "AG0043"; + + private static readonly LocalizableString Title = new LocalizableResourceString( + nameof(CustomRulesResources.AG0043Title), + CustomRulesResources.ResourceManager, + typeof(CustomRulesResources)); + + private static readonly LocalizableString MessageFormat = new LocalizableResourceString( + nameof(CustomRulesResources.AG0043Title), + CustomRulesResources.ResourceManager, + typeof(CustomRulesResources)); + + private static readonly LocalizableString Description + = DescriptionContentLoader.GetAnalyzerDescription(nameof(AG0043NoBuildServiceProvider)); + + private static readonly DiagnosticDescriptor Descriptor = new DiagnosticDescriptor( + DIAGNOSTIC_ID, + Title, + MessageFormat, + AnalyzerCategory.CustomQualityRules, + DiagnosticSeverity.Error, + AnalyzerConstants.EnabledByDefault, + Description, + "https://github.com/agoda-com/AgodaAnalyzers/issues/190", + WellKnownDiagnosticTags.EditAndContinue); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Descriptor); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.RegisterSyntaxNodeAction(AnalyzeNode, SyntaxKind.InvocationExpression); + } + + private static void AnalyzeNode(SyntaxNodeAnalysisContext context) + { + var invocationExpr = (InvocationExpressionSyntax)context.Node; + + // Check if it's a member access (e.g., something.BuildServiceProvider()) + if (!(invocationExpr.Expression is MemberAccessExpressionSyntax memberAccessExpr)) + return; + + // Check if the method name is BuildServiceProvider + if (memberAccessExpr.Name.Identifier.ValueText != "BuildServiceProvider") + return; + + // Get the method symbol + var methodSymbol = context.SemanticModel.GetSymbolInfo(invocationExpr).Symbol as IMethodSymbol; + if (methodSymbol == null) + return; + + // Verify it's from the correct namespace + if (!methodSymbol.ContainingNamespace.ToDisplayString() + .StartsWith("Microsoft.Extensions.DependencyInjection")) + return; + + // Get the containing type that defines the BuildServiceProvider method + var containingType = methodSymbol.ContainingType; + if (containingType == null) + return; + + // Check if the containing type is an extension method class for IServiceCollection + var isServiceCollectionExtension = containingType + .GetTypeMembers() + .SelectMany(t => t.GetMembers()) + .OfType() + .Any(m => m.Parameters.Length > 0 && + m.Parameters[0].Type.ToDisplayString() == + "Microsoft.Extensions.DependencyInjection.IServiceCollection"); + + // For extension methods, check the type of the expression being extended + var expressionType = context.SemanticModel.GetTypeInfo(memberAccessExpr.Expression).Type; + var isExpressionServiceCollection = expressionType != null && + (expressionType.AllInterfaces.Any(i => + i.ToDisplayString() == + "Microsoft.Extensions.DependencyInjection.IServiceCollection") || + expressionType.ToDisplayString() == + "Microsoft.Extensions.DependencyInjection.IServiceCollection"); + + if (isServiceCollectionExtension || isExpressionServiceCollection) + { + var diagnostic = Diagnostic.Create(Descriptor, memberAccessExpr.Name.GetLocation()); + context.ReportDiagnostic(diagnostic); + } + } + } +} \ No newline at end of file diff --git a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs index 905e367..b7dca7a 100644 --- a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs +++ b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs @@ -323,5 +323,11 @@ public static string AG0041Title { return ResourceManager.GetString("AG0041Title", resourceCulture); } } + + public static string AG0043Title { + get { + return ResourceManager.GetString("AG0043Title", resourceCulture); + } + } } } diff --git a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx index 1f99dcc..c2d3804 100644 --- a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx +++ b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx @@ -260,4 +260,7 @@ One exception is logging, where it can be useful to see the exact DC / cluster / You are using either an interpolated string or string concatenation in your logs, change these to the message template format to preserve structure in your logs + + Usage of BuildServiceProvider is likely to cause unexpected behaviour + \ No newline at end of file From 44c34b4e3b5ec87ba6b7299a44702bb036828b04 Mon Sep 17 00:00:00 2001 From: Joel Dickson <9032274+joeldickson@users.noreply.github.com> Date: Thu, 17 Oct 2024 12:59:02 +0700 Subject: [PATCH 2/7] Update AG0041LogTemplateAnalyzer.cs (#193) * Update AG0041LogTemplateAnalyzer.cs * Create AG0041.md --- doc/AG0041.md | 68 +++++++++++++++++++ .../AgodaCustom/AG0041LogTemplateAnalyzer.cs | 4 +- 2 files changed, 70 insertions(+), 2 deletions(-) create mode 100644 doc/AG0041.md diff --git a/doc/AG0041.md b/doc/AG0041.md new file mode 100644 index 0000000..577fe6a --- /dev/null +++ b/doc/AG0041.md @@ -0,0 +1,68 @@ +# AG0041: Use message templates in logging + +Structured logging should be employed instead of string interpolation or concatenation. + +## Why is this an issue? + +Logging is a critical aspect of any application, providing insights into its behavior and facilitating debugging. +However, not all logging approaches are created equal. Using string interpolation or concatenation for log messages is +a common pitfall that can lead to performance issues and hinder log analysis. + +Consider the following problematic code: + +```csharp +logger.Information($"User {userId} logged in at {DateTime.Now}"); // Noncompliant +logger.Information("Error occurred: " + errorMessage); // Noncompliant +``` + +Instead, you should use message templates: + +```csharp +logger.Information("User {UserId} logged in at {LoginTime}", userId, DateTime.Now); // Compliant +logger.Information("Error occurred: {ErrorMessage}", errorMessage); // Compliant +``` + +## Benefits of using message templates + +The use of message templates in logging offers several significant advantages over traditional string interpolation or +concatenation methods. Firstly, message templates can provide a notable performance boost, particularly in scenarios where +log messages aren't actually written due to log level settings. This efficiency is crucial in high-volume logging environments +where every millisecond counts. + +Structured logging, enabled by message templates, is another key benefit. It allows variables to be captured as separate entities +rather than being merged into a single string. This separation enables powerful log analysis capabilities, making it easier to search, +filter, and aggregate log data based on specific parameters. The ability to maintain consistent log formats across an application +is also greatly enhanced with message templates, leading to more uniform and easily interpretable logs. + +Message templates also facilitate semantic logging, allowing developers to attach additional metadata to log events. This extra +context can be invaluable when debugging complex issues or analyzing application behavior. Furthermore, modern logging frameworks such +as Serilog and Microsoft.Extensions.Logging are optimized for template-style logging. By using message templates, developers can +take full advantage of these frameworks' features and performance optimizations, ensuring their logging strategy is future-proof +and aligned with industry best practices. + +## More Info + +This analyzer checks for the use of string interpolation (`$"..."`) or string concatenation in logging method calls. It specifically targets methods of `ILogger` from both `Microsoft.Extensions.Logging` and `Serilog`. + +### Noncompliant Code Examples + +```csharp +_logger.LogInformation($"Processing order {orderId}"); +_logger.LogError("Error: " + exception.Message); +``` + +### Compliant Solution + +```csharp +_logger.LogInformation("Processing order {OrderId}", orderId); +_logger.LogError("Error: {ErrorMessage}", exception.Message); +``` + +### Exceptions + +This rule doesn't apply to non-logging methods or to logging methods that explicitly expect formatted strings. + +## Resources + +* [Structured logging with ILogger in .NET](https://docs.microsoft.com/en-us/aspnet/core/fundamentals/logging/?view=aspnetcore-5.0#log-message-template) +* [Serilog message templates](https://github.com/serilog/serilog/wiki/Writing-Log-Events#message-templates) diff --git a/src/Agoda.Analyzers/AgodaCustom/AG0041LogTemplateAnalyzer.cs b/src/Agoda.Analyzers/AgodaCustom/AG0041LogTemplateAnalyzer.cs index e5922aa..540a672 100644 --- a/src/Agoda.Analyzers/AgodaCustom/AG0041LogTemplateAnalyzer.cs +++ b/src/Agoda.Analyzers/AgodaCustom/AG0041LogTemplateAnalyzer.cs @@ -22,7 +22,7 @@ public class AG0041LogTemplateAnalyzer : DiagnosticAnalyzer Category, DiagnosticSeverity.Warning, isEnabledByDefault: true, - helpLinkUri: "https://github.com/agoda-com/AgodaAnalyzers/issues/183"); + helpLinkUri: "https://github.com/agoda-com/AgodaAnalyzers/blob/master/doc/AG0041.md"); public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); @@ -87,4 +87,4 @@ private static bool ContainsStringConcatenation(ExpressionSyntax expression) bes.Right.IsKind(SyntaxKind.StringLiteralExpression))); } } -} \ No newline at end of file +} From 9c6c8bf315300646c9c585c308f6704e1bfc8cc5 Mon Sep 17 00:00:00 2001 From: Rannes Date: Wed, 16 Oct 2024 11:42:51 +0700 Subject: [PATCH 3/7] feat: add BuildServiceProvider analyzer rule --- .../AgodaCustom/AG0043UnitTests.cs | 96 +++++++++++++++++ .../AG0043NoBuildServiceProvider.cs | 101 ++++++++++++++++++ .../CustomRulesResources.Designer.cs | 6 ++ .../AgodaCustom/CustomRulesResources.resx | 3 + 4 files changed, 206 insertions(+) create mode 100644 src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs create mode 100644 src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs diff --git a/src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs b/src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs new file mode 100644 index 0000000..3b80601 --- /dev/null +++ b/src/Agoda.Analyzers.Test/AgodaCustom/AG0043UnitTests.cs @@ -0,0 +1,96 @@ +using System.Threading.Tasks; +using Agoda.Analyzers.AgodaCustom; +using Agoda.Analyzers.Test.Helpers; +using Microsoft.CodeAnalysis.Diagnostics; +using NUnit.Framework; +using Microsoft.Extensions.DependencyInjection; + +namespace Agoda.Analyzers.Test.AgodaCustom; + +[TestFixture] +class AG0043UnitTests : DiagnosticVerifier +{ + protected override DiagnosticAnalyzer DiagnosticAnalyzer => new AG0043NoBuildServiceProvider(); + + protected override string DiagnosticId => AG0043NoBuildServiceProvider.DIAGNOSTIC_ID; + + [Test] + public async Task BuildServiceProvider_Used_RaisesDiagnostic() + { + var services = new ServiceCollection(); + services.BuildServiceProvider(); + + var code = new CodeDescriptor + { + References = new[] + { + typeof(ServiceCollection).Assembly, + typeof(ServiceProvider).Assembly, + typeof(IServiceCollection).Assembly + }, + Code = @" + using Microsoft.Extensions.DependencyInjection; + + public class Program + { + public static void Main() + { + var services = new ServiceCollection(); + var provider = services.BuildServiceProvider(); + } + }" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(9, 53)); + } + + [Test] + public async Task OtherMethod_NoDiagnostic() + { + var code = new CodeDescriptor + { + References = new[] {typeof(ServiceCollection).Assembly}, + Code = @" + using Microsoft.Extensions.DependencyInjection; + + class TestClass + { + public void ConfigureServices() + { + var services = new ServiceCollection(); + services.AddSingleton(); + } + }" + }; + + await VerifyDiagnosticsAsync(code, EmptyDiagnosticResults); + } + + [Test] + public async Task BuildServiceProvider_WhenChaining_RaiseDiagnostic() + { + var code = new CodeDescriptor + { + References = new[] + { + typeof(ServiceCollection).Assembly, + typeof(ServiceProvider).Assembly, + typeof(IServiceCollection).Assembly + }, + Code = @" + using Microsoft.Extensions.DependencyInjection; + + public class Program + { + public static void Main() + { + var provider = new ServiceCollection() + .AddSingleton() + .BuildServiceProvider(); + } + }" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(10, 34)); + } +} \ No newline at end of file diff --git a/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs b/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs new file mode 100644 index 0000000..c38327b --- /dev/null +++ b/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs @@ -0,0 +1,101 @@ +using System.Collections.Immutable; +using System.Linq; +using Agoda.Analyzers.Helpers; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Agoda.Analyzers.AgodaCustom +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class AG0043NoBuildServiceProvider : DiagnosticAnalyzer + { + public const string DIAGNOSTIC_ID = "AG0043"; + + private static readonly LocalizableString Title = new LocalizableResourceString( + nameof(CustomRulesResources.AG0043Title), + CustomRulesResources.ResourceManager, + typeof(CustomRulesResources)); + + private static readonly LocalizableString MessageFormat = new LocalizableResourceString( + nameof(CustomRulesResources.AG0043Title), + CustomRulesResources.ResourceManager, + typeof(CustomRulesResources)); + + private static readonly LocalizableString Description + = DescriptionContentLoader.GetAnalyzerDescription(nameof(AG0043NoBuildServiceProvider)); + + private static readonly DiagnosticDescriptor Descriptor = new DiagnosticDescriptor( + DIAGNOSTIC_ID, + Title, + MessageFormat, + AnalyzerCategory.CustomQualityRules, + DiagnosticSeverity.Error, + AnalyzerConstants.EnabledByDefault, + Description, + "https://github.com/agoda-com/AgodaAnalyzers/issues/190", + WellKnownDiagnosticTags.EditAndContinue); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Descriptor); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.RegisterSyntaxNodeAction(AnalyzeNode, SyntaxKind.InvocationExpression); + } + + private static void AnalyzeNode(SyntaxNodeAnalysisContext context) + { + var invocationExpr = (InvocationExpressionSyntax)context.Node; + + // Check if it's a member access (e.g., something.BuildServiceProvider()) + if (!(invocationExpr.Expression is MemberAccessExpressionSyntax memberAccessExpr)) + return; + + // Check if the method name is BuildServiceProvider + if (memberAccessExpr.Name.Identifier.ValueText != "BuildServiceProvider") + return; + + // Get the method symbol + var methodSymbol = context.SemanticModel.GetSymbolInfo(invocationExpr).Symbol as IMethodSymbol; + if (methodSymbol == null) + return; + + // Verify it's from the correct namespace + if (!methodSymbol.ContainingNamespace.ToDisplayString() + .StartsWith("Microsoft.Extensions.DependencyInjection")) + return; + + // Get the containing type that defines the BuildServiceProvider method + var containingType = methodSymbol.ContainingType; + if (containingType == null) + return; + + // Check if the containing type is an extension method class for IServiceCollection + var isServiceCollectionExtension = containingType + .GetTypeMembers() + .SelectMany(t => t.GetMembers()) + .OfType() + .Any(m => m.Parameters.Length > 0 && + m.Parameters[0].Type.ToDisplayString() == + "Microsoft.Extensions.DependencyInjection.IServiceCollection"); + + // For extension methods, check the type of the expression being extended + var expressionType = context.SemanticModel.GetTypeInfo(memberAccessExpr.Expression).Type; + var isExpressionServiceCollection = expressionType != null && + (expressionType.AllInterfaces.Any(i => + i.ToDisplayString() == + "Microsoft.Extensions.DependencyInjection.IServiceCollection") || + expressionType.ToDisplayString() == + "Microsoft.Extensions.DependencyInjection.IServiceCollection"); + + if (isServiceCollectionExtension || isExpressionServiceCollection) + { + var diagnostic = Diagnostic.Create(Descriptor, memberAccessExpr.Name.GetLocation()); + context.ReportDiagnostic(diagnostic); + } + } + } +} \ No newline at end of file diff --git a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs index 905e367..b7dca7a 100644 --- a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs +++ b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs @@ -323,5 +323,11 @@ public static string AG0041Title { return ResourceManager.GetString("AG0041Title", resourceCulture); } } + + public static string AG0043Title { + get { + return ResourceManager.GetString("AG0043Title", resourceCulture); + } + } } } diff --git a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx index 1f99dcc..c2d3804 100644 --- a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx +++ b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx @@ -260,4 +260,7 @@ One exception is logging, where it can be useful to see the exact DC / cluster / You are using either an interpolated string or string concatenation in your logs, change these to the message template format to preserve structure in your logs + + Usage of BuildServiceProvider is likely to cause unexpected behaviour + \ No newline at end of file From a40aaf7ab66457852f4571e322cf273f9b636754 Mon Sep 17 00:00:00 2001 From: Rannes Date: Thu, 17 Oct 2024 14:51:18 +0700 Subject: [PATCH 4/7] docs: update docs for the rule. --- doc/AG0043.md | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 doc/AG0043.md diff --git a/doc/AG0043.md b/doc/AG0043.md new file mode 100644 index 0000000..e17b151 --- /dev/null +++ b/doc/AG0043.md @@ -0,0 +1,88 @@ +# AG0043: BuildServiceProvider should not be used in production code + +## Problem Description + +Using `BuildServiceProvider()` in production code can lead to memory leaks and other issues because it creates a new container. This is especially problematic when called repeatedly, such as in request processing scenarios. + +## Rule Details + +This rule raises an issue when `BuildServiceProvider()` is called on `IServiceCollection` instances. + +### Noncompliant Code Example + +```csharp +public void ConfigureServices(IServiceCollection services) +{ + services.AddTransient(); + var serviceProvider = services.BuildServiceProvider(); // Noncompliant + var myService = serviceProvider.GetService(); +} +``` + +### Compliant Solution + +```csharp +public class Startup +{ + public void ConfigureServices(IServiceCollection services) + { + services.AddTransient(); + // Let ASP.NET Core build the service provider + } + + public void Configure(IApplicationBuilder app, IMyService myService) + { + // Services are injected by the framework + } +} +``` + +## Why is this an Issue? + +1. **Memory Leaks**: Each call to `BuildServiceProvider()` creates a new dependency injection container, which holds references to all registered services. If called repeatedly (e.g., during request processing), this leads to memory leaks. + +2. **Performance Impact**: Creating new service providers is computationally expensive and can impact application performance. + +3. **Singleton Duplication**: Multiple service providers can result in multiple instances of services that should be singletons. + +## Exceptions + +`BuildServiceProvider()` may be acceptable in the following scenarios: + +- Unit tests where a full DI container is needed +- Development-time configuration validation +- Tools and utilities that run outside the normal application lifecycle + +## How to Fix It + +1. In ASP.NET Core applications: + - Let the framework handle service provider creation + - Use constructor injection to obtain dependencies + - Use `IServiceScope` for creating scoped service providers when needed + - HttpContext and other services have method for `RequestServices.GetService` to get scoped services + +2. For configuration validation: + ```csharp + public void ConfigureServices(IServiceCollection services) + { + services.AddTransient(); + + // Only in development + if (Environment.IsDevelopment()) + { + // Validate service registration + var serviceProvider = services.BuildServiceProvider(validateScopes: true); + serviceProvider.Dispose(); + } + } + ``` + +## Benefits +- Prevents memory leaks +- Improves application performance +- Ensures proper singleton behavior +- Maintains proper service scoping + +## References +- [ASP.NET Core Dependency Injection Best Practices](https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection#service-lifetimes) +- [Dependency injection in ASP.NET Core](https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection) From 3f7e2608f52c40e9372d1e319ff62de0b6186c3c Mon Sep 17 00:00:00 2001 From: Rannes Date: Thu, 17 Oct 2024 14:51:43 +0700 Subject: [PATCH 5/7] chore: remove comments --- .../AgodaCustom/AG0043NoBuildServiceProvider.cs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs b/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs index c38327b..8653f89 100644 --- a/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs +++ b/src/Agoda.Analyzers/AgodaCustom/AG0043NoBuildServiceProvider.cs @@ -34,7 +34,7 @@ private static readonly LocalizableString Description DiagnosticSeverity.Error, AnalyzerConstants.EnabledByDefault, Description, - "https://github.com/agoda-com/AgodaAnalyzers/issues/190", + "https://github.com/agoda-com/AgodaAnalyzers/blob/master/doc/AG0043.md", WellKnownDiagnosticTags.EditAndContinue); public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Descriptor); @@ -50,30 +50,24 @@ private static void AnalyzeNode(SyntaxNodeAnalysisContext context) { var invocationExpr = (InvocationExpressionSyntax)context.Node; - // Check if it's a member access (e.g., something.BuildServiceProvider()) if (!(invocationExpr.Expression is MemberAccessExpressionSyntax memberAccessExpr)) return; - // Check if the method name is BuildServiceProvider if (memberAccessExpr.Name.Identifier.ValueText != "BuildServiceProvider") return; - // Get the method symbol var methodSymbol = context.SemanticModel.GetSymbolInfo(invocationExpr).Symbol as IMethodSymbol; if (methodSymbol == null) return; - // Verify it's from the correct namespace if (!methodSymbol.ContainingNamespace.ToDisplayString() .StartsWith("Microsoft.Extensions.DependencyInjection")) return; - // Get the containing type that defines the BuildServiceProvider method var containingType = methodSymbol.ContainingType; if (containingType == null) return; - // Check if the containing type is an extension method class for IServiceCollection var isServiceCollectionExtension = containingType .GetTypeMembers() .SelectMany(t => t.GetMembers()) @@ -82,7 +76,6 @@ private static void AnalyzeNode(SyntaxNodeAnalysisContext context) m.Parameters[0].Type.ToDisplayString() == "Microsoft.Extensions.DependencyInjection.IServiceCollection"); - // For extension methods, check the type of the expression being extended var expressionType = context.SemanticModel.GetTypeInfo(memberAccessExpr.Expression).Type; var isExpressionServiceCollection = expressionType != null && (expressionType.AllInterfaces.Any(i => From 2f25999ab51b7cefcc7ff6f33dc7b0c713c4e0b0 Mon Sep 17 00:00:00 2001 From: Joel Dickson <9032274+joeldickson@users.noreply.github.com> Date: Thu, 17 Oct 2024 15:56:46 +0700 Subject: [PATCH 6/7] Add examples with minimal API which is more common --- doc/AG0043.md | 61 +++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 47 insertions(+), 14 deletions(-) diff --git a/doc/AG0043.md b/doc/AG0043.md index e17b151..0e1861e 100644 --- a/doc/AG0043.md +++ b/doc/AG0043.md @@ -1,15 +1,14 @@ # AG0043: BuildServiceProvider should not be used in production code ## Problem Description - Using `BuildServiceProvider()` in production code can lead to memory leaks and other issues because it creates a new container. This is especially problematic when called repeatedly, such as in request processing scenarios. ## Rule Details - This rule raises an issue when `BuildServiceProvider()` is called on `IServiceCollection` instances. -### Noncompliant Code Example +### Noncompliant Code Examples +#### Traditional ASP.NET Core ```csharp public void ConfigureServices(IServiceCollection services) { @@ -19,8 +18,27 @@ public void ConfigureServices(IServiceCollection services) } ``` -### Compliant Solution +#### Minimal API +```csharp +var builder = WebApplication.CreateBuilder(args); + +builder.Services.AddTransient(); + +var app = builder.Build(); + +app.MapGet("/", () => +{ + var serviceProvider = app.Services.BuildServiceProvider(); // Noncompliant + var myService = serviceProvider.GetService(); + return myService.GetMessage(); +}); + +app.Run(); +``` + +### Compliant Solutions +#### Traditional ASP.NET Core ```csharp public class Startup { @@ -29,7 +47,6 @@ public class Startup services.AddTransient(); // Let ASP.NET Core build the service provider } - public void Configure(IApplicationBuilder app, IMyService myService) { // Services are injected by the framework @@ -37,36 +54,52 @@ public class Startup } ``` -## Why is this an Issue? +#### Minimal API +```csharp +var builder = WebApplication.CreateBuilder(args); -1. **Memory Leaks**: Each call to `BuildServiceProvider()` creates a new dependency injection container, which holds references to all registered services. If called repeatedly (e.g., during request processing), this leads to memory leaks. +builder.Services.AddTransient(); -2. **Performance Impact**: Creating new service providers is computationally expensive and can impact application performance. +var app = builder.Build(); +app.MapGet("/", (IMyService myService) => myService.GetMessage()); + +app.Run(); + +// Service interfaces and implementations +public interface IMyService +{ + string GetMessage(); +} + +public class MyService : IMyService +{ + public string GetMessage() => "Hello from MyService!"; +} +``` + +## Why is this an Issue? +1. **Memory Leaks**: Each call to `BuildServiceProvider()` creates a new dependency injection container, which holds references to all registered services. If called repeatedly (e.g., during request processing), this leads to memory leaks. +2. **Performance Impact**: Creating new service providers is computationally expensive and can impact application performance. 3. **Singleton Duplication**: Multiple service providers can result in multiple instances of services that should be singletons. ## Exceptions - `BuildServiceProvider()` may be acceptable in the following scenarios: - - Unit tests where a full DI container is needed - Development-time configuration validation - Tools and utilities that run outside the normal application lifecycle ## How to Fix It - 1. In ASP.NET Core applications: - Let the framework handle service provider creation - Use constructor injection to obtain dependencies - Use `IServiceScope` for creating scoped service providers when needed - - HttpContext and other services have method for `RequestServices.GetService` to get scoped services - + - `HttpContext` and other services have methods like `RequestServices.GetService()` to get scoped services 2. For configuration validation: ```csharp public void ConfigureServices(IServiceCollection services) { services.AddTransient(); - // Only in development if (Environment.IsDevelopment()) { From 17bb2c1a43ce64bdac39b73dcd6767f4e9a58f94 Mon Sep 17 00:00:00 2001 From: Joel Dickson <9032274+joeldickson@users.noreply.github.com> Date: Thu, 17 Oct 2024 15:59:30 +0700 Subject: [PATCH 7/7] Update error message with more explanation --- src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx index c2d3804..5771a6a 100644 --- a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx +++ b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx @@ -261,6 +261,6 @@ One exception is logging, where it can be useful to see the exact DC / cluster / You are using either an interpolated string or string concatenation in your logs, change these to the message template format to preserve structure in your logs - Usage of BuildServiceProvider is likely to cause unexpected behaviour + BuildServiceProvider detected in request processing code. This may cause memory leaks. Remove the BuildServiceProvider() call and let the framework manage the service provider lifecycle. - \ No newline at end of file +