diff --git a/analyzers/rspec/cs/S6960.html b/analyzers/rspec/cs/S6960.html new file mode 100644 index 00000000000..49b5da195eb --- /dev/null +++ b/analyzers/rspec/cs/S6960.html @@ -0,0 +1,237 @@ +

ASP.NET controllers should not have mixed responsibilities. +Following the Single Responsibility Principle (SRP), they should be kept +lean and focused +on a single, separate concern. In short, they should have a single reason to change.

+

The rule identifies different responsibilities by looking at groups of actions that use different sets of services defined in the controller.

+

Basic services that are typically required by most controllers are not considered:

+ +

The rule currently applies to ASP.NET Core only, and doesn’t cover ASP.NET MVC 4.x.

+

It also only takes into account web APIs controllers, i.e. the ones marked with the ApiController attribute. MVC controllers are +not in scope.

+

Why is this an issue?

+

Multiple issues can appear when the Single Responsibility Principle (SRP) is violated.

+

Harder to read and understand

+

A controller violating SRP is harder to read and understand since its Cognitive Complexity is generally above average (see +{rule:csharpsquid:S3776}).

+

For example, a controller MediaController that is in charge of both the "movies" and "photos" APIs would need to define all the +actions dealing with movies, alongside the ones dealing with photos, all defined in the same controller class.

+

The alternative is to define two controllers: a MovieController and a PhotoController, each in charge of a smaller +number of actions.

+

Harder to maintain and modify

+

Such complexity makes the controller harder to maintain and modify, slowing down new development and increasing the likelihood of bugs.

+

For example, a change in MediaController made for the movies APIs may inadvertently have an impact on the photos APIs as well. +Because the change was made in the context of movies, tests on photos may be overlooked, resulting in bugs in production.

+

That would not be likely to happen when two distinct controllers, MovieController and a PhotoController, are +defined.

+

Harder to test

+

The controller also becomes harder to test since the test suite would need to define a set of tests for each of the +responsibilities of the controller, resulting in a large and complex test suite.

+

For example, the MediaController introduced above would need to be tested on all movies-related actions, as well as on all +photos-related actions.

+

All those tests would be defined in the same test suite for MediaController, which would be affected by the same issues of +cognitive complexity as the controller under test by the suite.

+

Harder to reuse

+

A controller that has multiple responsibilities is less likely to be reusable. Lack of reuse can result in code duplication.

+

For example, when a new controller wants to derive from an existing one, it’s less probable that the new controller requires all the behaviors +exposed by the reused controller.

+

Rather, it’s much more common that the new controller only needs to reuse a fraction of the existing one. When reuse is not possible, the only +valid alternative is to duplicate part of the logic in the new controller.

+

Higher likelihood of performance issues

+

A controller that has multiple responsibilities may end up doing more than strictly necessary, resulting in a higher likelihood of +performance issues.

+

To understand why, it’s important to consider the difference between ASP.NET application vs non-ASP.NET applications.

+

In a non-ASP.NET application, controllers may be defined as Singletons via a Dependency Injection library.

+

In such a scenario, they would typically be instantiated only once, lazily or eagerly, at application startup.

+

In ASP.NET applications, however, the default is that controllers are instantiated as many times as the number of requests that are served by +the web server. Each instance of the controller would need to resolve services independently.

+

While service instantiation is typically handled at application startup, service resolution happens every time an +instance of controller needs to be built, for each service declared in the controller.

+

Whether the resolution is done via Dependency Injection, direct static access (in the case of a Singleton), or a Service Locator, the cost of resolution needs to be paid at every single +instantiation.

+

For example, the movies-related APIs of the MediaController mentioned above may require the instantiation of an +IStreamingService, typically done via dependency injection. Such a service may not be relevant +for photos-related APIs.

+

Similarly, some of the photos-related APIs may require the instantiation of an IRedEyeRemovalService, which may not work at all +with movies.

+

Having a single controller would force the developer to deal with both instantiations, even though a given instance of the controller may be +used only for photos, or only for movies.

+

More complex routing

+

A controller that deals with multiple concerns often has unnecessarily complex routing: the route template at controller level cannot factorize the +route identifying the concern, so the full route template needs to be defined at the action level.

+

For example, the MediaController would have an empty route (or equivalent, e.g. / or ~/) and the actions +would need to define themselves the movie or photo prefix, depending on the type of media they deal with.

+

On the other hand, MovieController and PhotoController can factorize the movie and photo +route respectively, so that the route on the action would only contain action-specific information.

+

What is the potential impact?

+

As the size and the responsibilities of the controller increase, the issues that come with such an increase will have a further impact on the +code.

+ +

Why MVC controllers are not in scope

+

Alongside attribute +routing, which is typical of web APIs, MVC controllers also come with [conventional routing].

+

In MVC, the file structure of controllers is important, since it drives conventional routing, which is specific to MVC, +as well as default view mapping.

+

For those reasons, splitting an MVC controller into smaller pieces may break core behaviors of the web application such as routing and views, +triggering a large refactor of the whole project.

+

How to fix it in ASP.NET Core

+

Split the controller into multiple controllers, each dealing with a single responsibility.

+

Code examples

+

Noncompliant code example

+
+[Route("media")]
+public class MediaController( // Noncompliant: This controller has multiple responsibilities and could be split into 2 smaller units.
+    // Used by all actions
+    ILogger<MediaController> logger,
+    // Movie-specific dependencies
+    IStreamingService streamingService, ISubtitlesService subtitlesService,
+    // Photo-specific dependencies
+    IRedEyeRemovalService redEyeRemovalService, IPhotoEnhancementService photoEnhancementService) : Controller
+{
+    [Route("movie/stream")]
+    public IActionResult MovieStream([FromQuery] StreamRequest request) // Belongs to responsibility #1.
+    {
+        logger.LogInformation("Requesting movie stream for {MovieId}", request.MovieId);
+        return File(streamingService.GetStream(request.MovieId), "video/mp4");
+    }
+
+    [Route("movie/subtitles")]
+    public IActionResult MovieSubtitles([FromQuery] SubtitlesRequest request) // Belongs to responsibility #1.
+    {
+        logger.LogInformation("Requesting movie subtitles for {MovieId}", request.MovieId);
+        return File(subtitlesService.GetSubtitles(request.MovieId, request.Language), "text/vtt");
+    }
+
+    [Route("photo/remove-red-eye")]
+    public IActionResult RemoveRedEye([FromQuery] RedEyeRemovalRequest request) // Belongs to responsibility #2.
+    {
+        logger.LogInformation("Removing red-eye from photo {PhotoId}", request.PhotoId);
+        return File(redEyeRemovalService.RemoveRedEye(request.PhotoId, request.Sensitivity), "image/jpeg");
+    }
+
+    [Route("photo/enhance")]
+    public IActionResult EnhancePhoto([FromQuery] PhotoEnhancementRequest request) // Belongs to responsibility #2.
+    {
+        logger.LogInformation("Enhancing photo {PhotoId}", request.PhotoId);
+        return File(photoEnhancementService.EnhancePhoto(request.PhotoId, request.ColorGrading), "image/jpeg");
+    }
+}
+
+

Compliant solution

+
+[Route("media/[controller]")]
+public class MovieController(
+    ILogger<MovieController> logger,
+    IStreamingService streamingService, ISubtitlesService subtitlesService) : Controller
+{
+    [Route("stream")]
+    public IActionResult MovieStream([FromQuery] StreamRequest request)
+    {
+        logger.LogInformation("Requesting movie stream for {MovieId}", request.MovieId);
+        return File(streamingService.GetStream(request.MovieId), "video/mp4");
+    }
+
+    [Route("subtitles")]
+    public IActionResult MovieSubtitles([FromQuery] SubtitlesRequest request)
+    {
+        logger.LogInformation("Requesting movie subtitles for {MovieId}", request.MovieId);
+        return File(subtitlesService.GetSubtitles(request.MovieId, request.Language), "text/vtt");
+    }
+}
+
+[Route("media/[controller]")]
+public class PhotoController(
+    ILogger<PhotoController> logger,
+    IRedEyeRemovalService redEyeRemovalService, IPhotoEnhancementService photoEnhancementService) : Controller
+{
+    [Route("remove-red-eye")]
+    public IActionResult RemoveRedEye([FromQuery] RedEyeRemovalRequest request)
+    {
+        logger.LogInformation("Removing red-eye from photo {PhotoId}", request.PhotoId);
+        return File(redEyeRemovalService.RemoveRedEye(request.PhotoId, request.Sensitivity), "image/jpeg");
+    }
+
+    [Route("enhance")]
+    public IActionResult EnhancePhoto([FromQuery] PhotoEnhancementRequest request)
+    {
+        logger.LogInformation("Enhancing photo {PhotoId}", request.PhotoId);
+        return File(photoEnhancementService.EnhancePhoto(request.PhotoId, request.ColorGrading), "image/jpeg");
+    }
+}
+
+

Resources

+

Documentation

+ +

Articles & blog posts

+ +

Conference presentations

+ +

Related rules

+ + diff --git a/analyzers/rspec/cs/S6960.json b/analyzers/rspec/cs/S6960.json new file mode 100644 index 00000000000..a959f65489b --- /dev/null +++ b/analyzers/rspec/cs/S6960.json @@ -0,0 +1,24 @@ +{ + "title": "Controllers should not have mixed responsibilities", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Linear", + "linearDesc": "responsibilities", + "linearFactor": "15min" + }, + "tags": [ + "asp.net" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-6960", + "sqKey": "S6960", + "scope": "Main", + "quickfix": "partial", + "code": { + "impacts": { + "MAINTAINABILITY": "HIGH" + }, + "attribute": "MODULAR" + } +} diff --git a/analyzers/rspec/cs/Sonar_way_profile.json b/analyzers/rspec/cs/Sonar_way_profile.json index 92612fae4ab..b4ff4ab92e1 100644 --- a/analyzers/rspec/cs/Sonar_way_profile.json +++ b/analyzers/rspec/cs/Sonar_way_profile.json @@ -316,6 +316,7 @@ "S6930", "S6931", "S6934", + "S6960", "S6961", "S6962", "S6964", diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveMixedResponsibilities.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveMixedResponsibilities.cs new file mode 100644 index 00000000000..a2aa25c2b66 --- /dev/null +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/ControllersHaveMixedResponsibilities.cs @@ -0,0 +1,203 @@ +/* + * 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.Collections.Concurrent; + +namespace SonarAnalyzer.Rules.CSharp; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class ControllersHaveMixedResponsibilities : SonarDiagnosticAnalyzer +{ + private const string DiagnosticId = "S6960"; + private const string MessageFormat = "This controller has multiple responsibilities and could be split into {0} smaller controllers."; + private const string UnspeakableIndexerName = "I"; // All indexers are considered part of the same group + + public enum MemberType + { + Service, + Action, + } + + private static readonly HashSet ExcludedWellKnownServices = + [ + "ILogger", + "IMediator", + "IMapper", + "IConfiguration", + "IBus", + "IMessageBus", + "IHttpClientFactory" + ]; + + private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterCompilationStartAction(compilationStartContext => + { + if (!compilationStartContext.Compilation.ReferencesControllers()) + { + return; + } + + compilationStartContext.RegisterSymbolStartAction(symbolStartContext => + { + var symbol = (INamedTypeSymbol)symbolStartContext.Symbol; + if (symbol.IsCoreApiController() + && symbol.BaseType.Is(KnownType.Microsoft_AspNetCore_Mvc_ControllerBase) + && !symbol.IsAbstract) + { + var relevantMembers = RelevantMembers(symbol); + + if (relevantMembers.Count < 2) + { + return; + } + + var dependencies = new ConcurrentStack(); + symbolStartContext.RegisterCodeBlockStartAction(PopulateDependencies(relevantMembers, dependencies)); + symbolStartContext.RegisterSymbolEndAction(CalculateAndReportOnResponsibilities(symbol, relevantMembers, dependencies)); + } + }, SymbolKind.NamedType); + }); + + private static Action> PopulateDependencies( + ImmutableDictionary relevantMembers, + ConcurrentStack dependencies) => + codeBlockStartContext => + { + if (BlockName(codeBlockStartContext.CodeBlock) is { } blockName) + { + codeBlockStartContext.RegisterNodeAction(c => + { + if (c.Node.GetName() is { } dependencyName && relevantMembers.ContainsKey(blockName) && relevantMembers.ContainsKey(dependencyName)) + { + dependencies.Push(new(blockName, dependencyName)); + } + }, SyntaxKind.IdentifierName); + } + }; + + private static Action CalculateAndReportOnResponsibilities( + INamedTypeSymbol controllerSymbol, + ImmutableDictionary relevantMembers, + ConcurrentStack dependencies) => + symbolEndContext => + { + if (ResponsibilityGroups(relevantMembers, dependencies) is { Count: > 1 } responsibilityGroups) + { + var secondaryLocations = SecondaryLocations(controllerSymbol, responsibilityGroups).ToList(); + foreach (var primaryLocation in IdentifierLocations(controllerSymbol)) + { + symbolEndContext.ReportIssue(Diagnostic.Create( + Rule, + primaryLocation, + secondaryLocations.ToAdditionalLocations(), + secondaryLocations.ToProperties(), + responsibilityGroups.Count)); + } + } + }; + + private static List> ResponsibilityGroups( + ImmutableDictionary relevantMembers, + ConcurrentStack dependencies) + { + var dependencySets = new DisjointSets(relevantMembers.Keys); + foreach (var dependency in dependencies) + { + dependencySets.Union(dependency.From, dependency.To); + } + + return dependencySets + .GetAllSets() + // Filter out sets of only actions or only services + .Where(x => x.Exists(x => relevantMembers[x] == MemberType.Service) && x.Exists(x => relevantMembers[x] == MemberType.Action)) + .ToList(); + } + + private static string BlockName(SyntaxNode block) => + block switch + { + AccessorDeclarationSyntax { Parent.Parent: PropertyDeclarationSyntax property } => property.GetName(), + AccessorDeclarationSyntax { Parent.Parent: IndexerDeclarationSyntax } => UnspeakableIndexerName, + ArrowExpressionClauseSyntax { Parent: PropertyDeclarationSyntax property } => property.GetName(), + ArrowExpressionClauseSyntax { Parent: IndexerDeclarationSyntax } => UnspeakableIndexerName, + MethodDeclarationSyntax method => method.GetName(), + PropertyDeclarationSyntax property => property.GetName(), + _ => null + }; + + private static ImmutableDictionary RelevantMembers(INamedTypeSymbol symbol) + { + var builder = ImmutableDictionary.CreateBuilder(); + foreach (var member in symbol.GetMembers().Where(x => !x.IsStatic)) + { + switch (member) + { + // Constructors are not considered because they have to be split anyway + // Accessors are not considered because they are part of properties, that are considered as a whole + case IMethodSymbol method when !method.IsConstructor() && method.MethodKind != MethodKind.StaticConstructor && method.AssociatedSymbol is not IPropertySymbol: + builder.Add(method.Name, MemberType.Action); + break; + // Indexers are treated as methods with an unspeakable name + case IPropertySymbol { IsIndexer: true }: + builder.Add(UnspeakableIndexerName, MemberType.Action); + break; + // Primary constructor parameters may or may not generate fields, and must be considered + case IMethodSymbol method when method.IsPrimaryConstructor(): + builder.AddRange(method.Parameters.Where(IsService).Select(x => new KeyValuePair(x.Name, MemberType.Service))); + break; + // Backing fields are excluded for auto-properties, since they are considered part of the property + case IFieldSymbol field when IsService(field) && !field.IsImplicitlyDeclared: + builder.Add(field.Name, MemberType.Service); + break; + case IPropertySymbol property when IsService(property): + builder.Add(property.Name, MemberType.Service); + break; + } + } + + return builder.ToImmutable(); + } + + private static bool IsService(ISymbol symbol) => + !ExcludedWellKnownServices.Contains(symbol.GetSymbolType().Name); + + private static IEnumerable SecondaryLocations(INamedTypeSymbol controllerSymbol, List> sets) + { + for (var setIndex = 0; setIndex < sets.Count; setIndex++) + { + foreach (var memberLocation in sets[setIndex].SelectMany(MemberLocations)) + { + yield return new SecondaryLocation(memberLocation, $"May belong to responsibility #{setIndex + 1}."); + } + } + + IEnumerable MemberLocations(string memberName) => + controllerSymbol.GetMembers(memberName).OfType().SelectMany(IdentifierLocations); + } + + private static IEnumerable IdentifierLocations(ISymbol symbol) where T : SyntaxNode => + symbol.DeclaringSyntaxReferences.Select(x => x.GetSyntax()).OfType().Select(x => x.GetIdentifier()?.GetLocation()); + + private record struct Dependency(string From, string To); +} diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/AspNetMvcHelper.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/AspNetMvcHelper.cs index aee781754ce..b2889416cdf 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/AspNetMvcHelper.cs +++ b/analyzers/src/SonarAnalyzer.Common/Helpers/AspNetMvcHelper.cs @@ -59,8 +59,7 @@ public static class AspNetMvcHelper && IsControllerType(methodSymbol.ContainingType); /// - /// Returns a value indicating whether the provided type symbol is a ASP.NET MVC - /// controller. + /// Whether the provided type symbol is a ASP.NET MVC controller. /// public static bool IsControllerType(this INamedTypeSymbol namedType) => namedType is not null @@ -69,6 +68,15 @@ public static class AspNetMvcHelper || namedType.GetAttributes(ControllerAttributeTypes).Any()) && !namedType.GetAttributes(NonControllerAttributeTypes).Any(); + /// + /// Whether the provided type symbol is an ASP.NET Core API controller. + /// Considers as API controllers also controllers deriving from ControllerBase but not Controller. + /// + public static bool IsCoreApiController(this INamedTypeSymbol namedType) => + namedType.IsControllerType() + && (namedType.GetAttributesWithInherited().Any(x => x.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ApiControllerAttribute)) + || (namedType.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ControllerBase) && !namedType.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_Controller))); + public static bool ReferencesControllers(this Compilation compilation) => compilation.GetTypeByMetadataName(KnownType.System_Web_Mvc_Controller) is not null || compilation.GetTypeByMetadataName(KnownType.Microsoft_AspNetCore_Mvc_Controller) is not null diff --git a/analyzers/tests/SonarAnalyzer.Test/Common/DisjointSetsTest.cs b/analyzers/tests/SonarAnalyzer.Test/Common/DisjointSetsTest.cs index e10193acd4b..25a22964018 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Common/DisjointSetsTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Common/DisjointSetsTest.cs @@ -1,4 +1,24 @@ -namespace SonarAnalyzer.Test.Common; +/* + * 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.Test.Common; [TestClass] public class DisjointSetsTest diff --git a/analyzers/tests/SonarAnalyzer.Test/Extensions/SyntaxNodeExtensionsTest.cs b/analyzers/tests/SonarAnalyzer.Test/Extensions/SyntaxNodeExtensionsTest.cs index 635ce395830..d549eadcbd2 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Extensions/SyntaxNodeExtensionsTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Extensions/SyntaxNodeExtensionsTest.cs @@ -98,7 +98,7 @@ public void ArrowExpressionBody_WithNotExpectedNode_ReturnsNull() [TestMethod] public void GetDeclarationTypeName_UnknownType() => #if DEBUG - Assert.ThrowsException(() => SyntaxNodeExtensionsCSharp.GetDeclarationTypeName(SyntaxFactory.Block()), "Unexpected type Block\r\nParameter name: kind"); + Assert.ThrowsException(() => SyntaxNodeExtensionsCSharp.GetDeclarationTypeName(SyntaxFactory.Block()), "Unexpected type Block\r\nParameter name: kind"); #else SyntaxNodeExtensionsCSharp.GetDeclarationTypeName(SyntaxFactory.Block()).Should().Be("type"); #endif diff --git a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs b/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs index 53283c1e09c..ba7bd4005c3 100644 --- a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs +++ b/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs @@ -6884,7 +6884,7 @@ internal static class RuleTypeMappingCS // ["S6957"], // ["S6958"], // ["S6959"], - // ["S6960"], + ["S6960"] = "CODE_SMELL", ["S6961"] = "CODE_SMELL", ["S6962"] = "CODE_SMELL", // ["S6963"], diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/AspNet/ControllersHaveMixedResponsibilitiesTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/AspNet/ControllersHaveMixedResponsibilitiesTest.cs new file mode 100644 index 00000000000..2fa9234d2fb --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/AspNet/ControllersHaveMixedResponsibilitiesTest.cs @@ -0,0 +1,51 @@ +/* + * 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 Microsoft.CodeAnalysis.CSharp; +using SonarAnalyzer.Rules.CSharp; + +namespace SonarAnalyzer.Test.Rules; + +#if NET + +[TestClass] +public class ControllersHaveMixedResponsibilitiesTest +{ + private readonly VerifierBuilder builder = + new VerifierBuilder().AddReferences(References).WithBasePath("AspNet"); + + private static IEnumerable References => + [ + AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcAbstractions, + AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcCore, + AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcViewFeatures, + CoreMetadataReference.SystemComponentModel, // For IServiceProvider + .. NuGetMetadataReference.MicrosoftExtensionsDependencyInjectionAbstractions("8.0.1"), // For IServiceProvider extensions + ]; + + [TestMethod] + public void ControllersHaveMixedResponsibilities_CS() => + builder + .AddPaths("ControllersHaveMixedResponsibilities.Latest.cs") + .WithLanguageVersion(LanguageVersion.Latest) + .Verify(); +} + +#endif diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveTooManyLinesTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveTooManyLinesTest.cs index debeb2775dc..6f662c35d2a 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveTooManyLinesTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/MethodsShouldNotHaveTooManyLinesTest.cs @@ -21,73 +21,73 @@ using CS = SonarAnalyzer.Rules.CSharp; using VB = SonarAnalyzer.Rules.VisualBasic; -namespace SonarAnalyzer.Test.Rules +namespace SonarAnalyzer.Test.Rules; + +[TestClass] +public class MethodsShouldNotHaveTooManyLinesTest { - [TestClass] - public class MethodsShouldNotHaveTooManyLinesTest - { - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_DefaultValues_CS() => - new VerifierBuilder().AddPaths("MethodsShouldNotHaveTooManyLines_DefaultValues.cs").Verify(); + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_DefaultValues_CS() => + new VerifierBuilder().AddPaths("MethodsShouldNotHaveTooManyLines_DefaultValues.cs").Verify(); - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_CustomValues_CS() => - CreateCSBuilder(2).AddPaths("MethodsShouldNotHaveTooManyLines_CustomValues.cs").Verify(); + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_CustomValues_CS() => + CreateCSBuilder(2).AddPaths("MethodsShouldNotHaveTooManyLines_CustomValues.cs").Verify(); #if NET - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_LocalFunctions() => - CreateCSBuilder(5).AddPaths("MethodsShouldNotHaveTooManyLines.LocalFunctions.cs").WithOptions(ParseOptionsHelper.FromCSharp8).Verify(); + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_LocalFunctions() => + CreateCSBuilder(5).AddPaths("MethodsShouldNotHaveTooManyLines.LocalFunctions.cs").WithOptions(ParseOptionsHelper.FromCSharp8).Verify(); - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_LocalFunctions_CSharp9() => - CreateCSBuilder(5).AddPaths("MethodsShouldNotHaveTooManyLines.LocalFunctions.CSharp9.cs").WithOptions(ParseOptionsHelper.FromCSharp9).Verify(); + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_LocalFunctions_CSharp9() => + CreateCSBuilder(5).AddPaths("MethodsShouldNotHaveTooManyLines.LocalFunctions.CSharp9.cs").WithOptions(ParseOptionsHelper.FromCSharp9).Verify(); - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_CustomValues_CSharp9() => - CreateCSBuilder(2).AddPaths("MethodsShouldNotHaveTooManyLines_CustomValues.CSharp9.cs").WithTopLevelStatements().Verify(); + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_CustomValues_CSharp9() => + CreateCSBuilder(2).AddPaths("MethodsShouldNotHaveTooManyLines_CustomValues.CSharp9.cs").WithTopLevelStatements().Verify(); - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_CustomValues_CSharp10() => - CreateCSBuilder(2).AddPaths("MethodsShouldNotHaveTooManyLines_CustomValues.CSharp10.cs").WithOptions(ParseOptionsHelper.FromCSharp10).Verify(); + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_CustomValues_CSharp10() => + CreateCSBuilder(2).AddPaths("MethodsShouldNotHaveTooManyLines_CustomValues.CSharp10.cs").WithOptions(ParseOptionsHelper.FromCSharp10).Verify(); - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_CSharp9_NoUsing() => - CreateCSBuilder(2).AddSnippet(@" + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_CSharp9_NoUsing() => + CreateCSBuilder(2).AddSnippet(@" int i = 1; i++; void LocalFunction() // Noncompliant {{This local function has 4 lines, which is greater than the 2 lines authorized.}} { - i++; - i++; - i++; - i++; +i++; +i++; +i++; +i++; }") - .WithOptions(ParseOptionsHelper.FromCSharp9) - .WithOutputKind(OutputKind.ConsoleApplication) - .Verify(); + .WithOptions(ParseOptionsHelper.FromCSharp9) + .WithOutputKind(OutputKind.ConsoleApplication) + .Verify(); - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_CSharp9_Valid() => - CreateCSBuilder(4).AddSnippet(@" + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_CSharp9_Valid() => + CreateCSBuilder(4).AddSnippet(@" int i = 1; i++; i++; i++; i++;") - .WithOptions(ParseOptionsHelper.FromCSharp9) - .WithOutputKind(OutputKind.ConsoleApplication) - .Verify(); + .WithOptions(ParseOptionsHelper.FromCSharp9) + .WithOutputKind(OutputKind.ConsoleApplication) + .Verify(); #endif - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_DoesntReportInTest_CS() => - new VerifierBuilder().AddPaths("MethodsShouldNotHaveTooManyLines_DefaultValues.cs") - .AddTestReference() - .VerifyNoIssueReported(); + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_DoesntReportInTest_CS() => + new VerifierBuilder().AddPaths("MethodsShouldNotHaveTooManyLines_DefaultValues.cs") + .AddTestReference() + .VerifyNoIssueReported(); - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_InvalidSyntax_CS() => - CreateCSBuilder(2).AddSnippet(@" + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_InvalidSyntax_CS() => + CreateCSBuilder(2).AddSnippet(@" public class Foo { public string () @@ -95,50 +95,49 @@ public string () return ""f""; } }") - .WithErrorBehavior(CompilationErrorBehavior.Ignore) - .Verify(); - - [DataTestMethod] - [DataRow(1)] - [DataRow(0)] - [DataRow(-1)] - public void MethodsShouldNotHaveTooManyLines_InvalidMaxThreshold_CS(int max) - { - var compilation = SolutionBuilder.CreateSolutionFromPath(@"TestCases\MethodsShouldNotHaveTooManyLines_CustomValues.cs") - .Compile(ParseOptionsHelper.CSharpLatest.ToArray()).Single(); - var errors = DiagnosticVerifier.AnalyzerExceptions(compilation, new CS.MethodsShouldNotHaveTooManyLines { Max = max }); - errors.Should().OnlyContain(x => x.GetMessage(null).Contains("Invalid rule parameter: maximum number of lines = ")).And.HaveCount(12); - } - - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_DefaultValues_VB() => - new VerifierBuilder().AddPaths("MethodsShouldNotHaveTooManyLines_DefaultValues.vb").Verify(); - - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_CustomValues_VB() => - new VerifierBuilder().AddAnalyzer(() => new VB.MethodsShouldNotHaveTooManyLines { Max = 2 }) - .AddPaths("MethodsShouldNotHaveTooManyLines_CustomValues.vb") - .Verify(); - - [TestMethod] - public void MethodsShouldNotHaveTooManyLines_DoesntReportInTest_VB() => - new VerifierBuilder().AddPaths("MethodsShouldNotHaveTooManyLines_DefaultValues.vb") - .AddTestReference() - .VerifyNoIssueReported(); - - [DataTestMethod] - [DataRow(1)] - [DataRow(0)] - [DataRow(-1)] - public void MethodsShouldNotHaveTooManyLines_InvalidMaxThreshold_VB(int max) - { - var compilation = SolutionBuilder.CreateSolutionFromPath(@"TestCases\MethodsShouldNotHaveTooManyLines_CustomValues.vb") - .Compile(ParseOptionsHelper.VisualBasicLatest.ToArray()).Single(); - var errors = DiagnosticVerifier.AnalyzerExceptions(compilation, new VB.MethodsShouldNotHaveTooManyLines { Max = max }); - errors.Should().OnlyContain(x => x.GetMessage(null).Contains("Invalid rule parameter: maximum number of lines = ")).And.HaveCount(7); - } - - private static VerifierBuilder CreateCSBuilder(int maxLines) => - new VerifierBuilder().AddAnalyzer(() => new CS.MethodsShouldNotHaveTooManyLines { Max = maxLines }); + .WithErrorBehavior(CompilationErrorBehavior.Ignore) + .Verify(); + + [DataTestMethod] + [DataRow(1)] + [DataRow(0)] + [DataRow(-1)] + public void MethodsShouldNotHaveTooManyLines_InvalidMaxThreshold_CS(int max) + { + var compilation = SolutionBuilder.CreateSolutionFromPath(@"TestCases\MethodsShouldNotHaveTooManyLines_CustomValues.cs") + .Compile(ParseOptionsHelper.CSharpLatest.ToArray()).Single(); + var errors = DiagnosticVerifier.AnalyzerExceptions(compilation, new CS.MethodsShouldNotHaveTooManyLines { Max = max }); + errors.Should().OnlyContain(x => x.GetMessage(null).Contains("Invalid rule parameter: maximum number of lines = ")).And.HaveCount(12); } + + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_DefaultValues_VB() => + new VerifierBuilder().AddPaths("MethodsShouldNotHaveTooManyLines_DefaultValues.vb").Verify(); + + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_CustomValues_VB() => + new VerifierBuilder().AddAnalyzer(() => new VB.MethodsShouldNotHaveTooManyLines { Max = 2 }) + .AddPaths("MethodsShouldNotHaveTooManyLines_CustomValues.vb") + .Verify(); + + [TestMethod] + public void MethodsShouldNotHaveTooManyLines_DoesntReportInTest_VB() => + new VerifierBuilder().AddPaths("MethodsShouldNotHaveTooManyLines_DefaultValues.vb") + .AddTestReference() + .VerifyNoIssueReported(); + + [DataTestMethod] + [DataRow(1)] + [DataRow(0)] + [DataRow(-1)] + public void MethodsShouldNotHaveTooManyLines_InvalidMaxThreshold_VB(int max) + { + var compilation = SolutionBuilder.CreateSolutionFromPath(@"TestCases\MethodsShouldNotHaveTooManyLines_CustomValues.vb") + .Compile(ParseOptionsHelper.VisualBasicLatest.ToArray()).Single(); + var errors = DiagnosticVerifier.AnalyzerExceptions(compilation, new VB.MethodsShouldNotHaveTooManyLines { Max = max }); + errors.Should().OnlyContain(x => x.GetMessage(null).Contains("Invalid rule parameter: maximum number of lines = ")).And.HaveCount(7); + } + + private static VerifierBuilder CreateCSBuilder(int maxLines) => + new VerifierBuilder().AddAnalyzer(() => new CS.MethodsShouldNotHaveTooManyLines { Max = maxLines }); } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/AspNet/ControllersHaveMixedResponsibilities.Latest.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/AspNet/ControllersHaveMixedResponsibilities.Latest.cs new file mode 100644 index 00000000000..513ae1e8aac --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/AspNet/ControllersHaveMixedResponsibilities.Latest.cs @@ -0,0 +1,1158 @@ +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.DependencyInjection; +using System; +using TestInstrumentation.ResponsibilitySpecificServices; +using TestInstrumentation.WellKnownInterfacesExcluded; +using TestInstrumentation; +using WithInjectionViaNormalConstructor; +using System.Runtime.CompilerServices; +using WithInjectionViaPrimaryConstructors.TwoActions; + +// Remark: secondary messages are asserted extensively, to ensure that grouping is done correctly and deterministically. + +namespace TestInstrumentation +{ + public interface IServiceWithAnAPI { void Use(); } + + public abstract class ServiceWithAnApi : IServiceWithAnAPI { public void Use() { } } + + // These interfaces have been kept to simulate the actual ones, from MediatR, AutoMapper, etc. + // For performance reasons, the analyzer ignores namespace and assembly, and only consider the name. + // While that may comes with some false positives, the likelihood of that happening is very low. + namespace WellKnownInterfacesExcluded + { + public interface ILogger : IServiceWithAnAPI { } // From Microsoft.Extensions.Logging + public interface IHttpClientFactory : IServiceWithAnAPI { } // From Microsoft.Extensions.Http + public interface IMediator : IServiceWithAnAPI { } // From MediatR + public interface IMapper : IServiceWithAnAPI { } // From AutoMapper + public interface IConfiguration : IServiceWithAnAPI { } // From Microsoft.Extensions.Configuration + public interface IBus : IServiceWithAnAPI { } // From MassTransit + public interface IMessageBus : IServiceWithAnAPI { } // From NServiceBus + } + + namespace WellKnownInterfacesNotExcluded + { + public interface IOption : IServiceWithAnAPI { } // From Microsoft.Extensions.Options + } + + namespace ResponsibilitySpecificServices + { + public interface IS1 : IServiceWithAnAPI { } + public interface IS2 : IServiceWithAnAPI { } + public interface IS3 : IServiceWithAnAPI { } + public interface IS4 : IServiceWithAnAPI { } + public interface IS5 : IServiceWithAnAPI { } + public interface IS6 : IServiceWithAnAPI { } + public interface IS7 : IServiceWithAnAPI { } + public interface IS8 : IServiceWithAnAPI { } + public interface IS9 : IServiceWithAnAPI { } + public interface IS10 : IServiceWithAnAPI { } + + public class Class1: ServiceWithAnApi, IS1 { } + public class Class2: ServiceWithAnApi, IS2 { } + public class Class3: ServiceWithAnApi, IS3 { } + public class Class4: ServiceWithAnApi, IS4 { } + + public class Struct1: IS1 { public void Use() { } } + public class Struct2: IS2 { public void Use() { } } + public class Struct3: IS3 { public void Use() { } } + public class Struct4: IS4 { public void Use() { } } + } +} + +namespace WithInjectionViaPrimaryConstructors +{ + using TestInstrumentation.ResponsibilitySpecificServices; + using TestInstrumentation.WellKnownInterfacesExcluded; + using TestInstrumentation.WellKnownInterfacesNotExcluded; + using TestInstrumentation; + + namespace AssertIssueLocationsAndMessage + { + // Noncompliant@+2 {{This controller has multiple responsibilities and could be split into 2 smaller controllers.}} + [ApiController] + public class TwoResponsibilities(IS1 s1, IS2 s2) : ControllerBase + { + public IActionResult A1() { s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + } + + [ApiController] + public class WithGenerics(IS1 s1, IS2 s2) : ControllerBase // Noncompliant + // ^^^^^^^^^^^^ + { + public IActionResult GenericAction() { s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + // ^^^^^^^^^^^^^ + public IActionResult NonGenericAction() { s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + // ^^^^^^^^^^^^^^^^ + } + + [ApiController] + public class @event(IS1 s1, IS2 s2) : ControllerBase // Noncompliant + // ^^^^^^ + { + public IActionResult @private() { s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + // ^^^^^^^^ + public IActionResult @public() { s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + // ^^^^^^^ + } + + [ApiController] + public class ThreeResponsibilities(IS1 s1, IS2 s2, IS3 s3) : ControllerBase // Noncompliant + // ^^^^^^^^^^^^^^^^^^^^^ + { + public IActionResult A1() { s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A3() { s3.Use(); return Ok(); } // Secondary {{May belong to responsibility #3.}} + } + } + + namespace WithIOptions + { + // Compliant@+2: 4 deps injected, all well-known => 4 singletons sets, merged into one + [ApiController] + public class WellKnownDepsController( + ILogger logger, IMediator mediator, IMapper mapper, IConfiguration configuration) : ControllerBase + { + public IActionResult A1() { logger.Use(); return Ok(); } + public IActionResult A2() { mediator.Use(); return Ok(); } + public IActionResult A3() { mapper.Use(); return Ok(); } + public IActionResult A4() { configuration.Use(); return Ok(); } + } + + // Noncompliant@+2: 4 different Option injected, that are not excluded + [ApiController] + public class FourDifferentOptionDepsController( + IOption o1, IOption o2, IOption o3, IOption o4) : ControllerBase + { + public IActionResult A1() { o1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { o2.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A3() { o3.Use(); return Ok(); } // Secondary {{May belong to responsibility #3.}} + public IActionResult A4() { o4.Use(); return Ok(); } // Secondary {{May belong to responsibility #4.}} + } + + // Compliant@+2: 5 different Option injected, used in couples to form a single responsibility + [ApiController] + public class FourDifferentOptionDepsUsedInCouplesController( + IOption o1, IOption o2, IOption o3, IOption o4, IOption o5) : ControllerBase + { + public IActionResult A1() { o1.Use(); o2.Use(); return Ok(); } + public IActionResult A2() { o2.Use(); o3.Use(); return Ok(); } + public IActionResult A3() { o3.Use(); o4.Use(); return Ok(); } + public IActionResult A4() { o4.Use(); o5.Use(); return Ok(); } + } + + // Noncompliant@+2: 3 Option deps injected, the rest are well-known dependencies (used as well as unused) + [ApiController] + public class ThreeOptionDepsController( + ILogger logger, IMediator mediator, IMapper mapper, IConfiguration configuration, + IOption o1, IOption o2, IOption o3) : ControllerBase + { + public IActionResult A1() { o1.Use(); logger.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { o2.Use(); mediator.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A3() { o3.Use(); configuration.Use(); return Ok(); } // Secondary {{May belong to responsibility #3.}} + public IActionResult A4() { logger.Use(); return Ok(); } + } + } + + namespace TwoActions + { + // Noncompliant@+2: 2 specific deps injected, each used in a separate responsibility, plus well-known dependencies + [ApiController] + public class TwoSeparateResponsibilitiesPlusSharedWellKnown( + ILogger logger, IMediator mediator, IMapper mapper, IConfiguration configuration, IBus bus, IMessageBus messageBus, + IS1 s1, IS2 s2) : ControllerBase + { + public IActionResult A1() { logger.Use(); mediator.Use(); bus.Use(); configuration.Use(); s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { mediator.Use(); mapper.Use(); bus.Use(); configuration.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + } + + // Noncompliant@+2: 4 specific deps injected, two for A1 and two for A2 + [ApiController] + public class FourSpecificDepsTwoForA1AndTwoForA2( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : ControllerBase + { + public IActionResult A1() { logger.Use(); s1.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { mediator.Use(); mapper.Use(); s3.Use(); s4.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + } + + // Compliant@+1: 4 specific deps injected, two for A1 and two for A2, in non-API controller derived from Controller + public class FourSpecificDepsTwoForA1AndTwoForA2NonApiFromController( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : Controller + { + public void A1() { logger.Use(); s1.Use(); s2.Use(); } + public void A2() { mediator.Use(); mapper.Use(); s3.Use(); s4.Use(); } + } + + // Noncompliant@+1: 4 specific deps injected, two for A1 and two for A2, in non-API controller derived from ControllerBase + public class FourSpecificDepsTwoForA1AndTwoForA2NonApiFromControllerBase( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : ControllerBase + { + public void A1() { logger.Use(); s1.Use(); s2.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { mediator.Use(); mapper.Use(); s3.Use(); s4.Use(); } // Secondary {{May belong to responsibility #2.}} + } + + // Compliant@+2: 4 specific deps injected, two for A1 and two for A2, in an API controller marked as NonController + [ApiController] + [NonController] public class FourSpecificDepsTwoForA1AndTwoForA2NoController( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : ControllerBase + { + public IActionResult A1() { logger.Use(); s1.Use(); s2.Use(); return Ok(); } + public IActionResult A2() { mediator.Use(); mapper.Use(); s3.Use(); s4.Use(); return Ok(); } + } + + // Noncompliant@+2: 4 specific deps injected, two for A1 and two for A2, in a PoCo controller with controller suffix + [ApiController] + public class FourSpecificDepsTwoForA1AndTwoForA2PoCoController( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : ControllerBase + { + public string A1() { logger.Use(); s1.Use(); s2.Use(); return "Ok"; } // Secondary {{May belong to responsibility #1.}} + public string A2() { mediator.Use(); mapper.Use(); s3.Use(); s4.Use(); return "Ok"; } // Secondary {{May belong to responsibility #2.}} + } + + // Compliant@+1: 4 specific deps injected, two for A1 and two for A2, in a PoCo controller without controller suffix + public class PoCoControllerWithoutControllerSuffix( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) + { + public string A1() { logger.Use(); s1.Use(); s2.Use(); return "Ok"; } + public string A2() { mediator.Use(); mapper.Use(); s3.Use(); s4.Use(); return "Ok"; } + } + + // Noncompliant@+3: 4 specific deps injected, two for A1 and two for A2, in a PoCo controller without controller suffix but with [Controller] attribute + [ApiController] + [Controller] + public class PoCoControllerWithoutControllerSuffixWithControllerAttribute( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : ControllerBase + { + public string A1() { logger.Use(); s1.Use(); s2.Use(); return "Ok"; } // Secondary {{May belong to responsibility #1.}} + public string A2() { mediator.Use(); mapper.Use(); s3.Use(); s4.Use(); return "Ok"; } // Secondary {{May belong to responsibility #2.}} + } + + // Noncompliant@+2: 4 specific deps injected, two for A1 and two for A2, with responsibilities in a different order + [ApiController] + public class FourSpecificDepsTwoForA1AndTwoForA2DifferentOrderOfResponsibilities( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : ControllerBase + { + public IActionResult A2() { logger.Use(); s1.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A1() { mediator.Use(); mapper.Use(); s3.Use(); s4.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + } + + // Noncompliant@+2: 4 specific deps injected, two for A1 and two for A2, with dependencies used in a different order + [ApiController] + public class FourSpecificDepsTwoForA1AndTwoForA2DifferentOrderOfDependencies( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : ControllerBase + { + public IActionResult A1() { logger.Use(); s2.Use(); s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { mediator.Use(); mapper.Use(); s3.Use(); s4.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + } + + // Noncompliant@+2: 4 specific deps injected, three for A1 and one for A2 + [ApiController] + public class FourSpecificDepsThreeForA1AndOneForA2( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS3 s4) : ControllerBase + { + public IActionResult A1() { logger.Use(); s1.Use(); s2.Use(); s3.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { mediator.Use(); mapper.Use(); s4.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + } + + // Compliant@+2: 4 specific deps injected, all for A1 and none for A2 + [ApiController] + public class FourSpecificDepsFourForA1AndNoneForA2( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS3 s4) : ControllerBase + { + public IActionResult A1() { logger.Use(); mediator.Use(); s1.Use(); s2.Use(); s3.Use(); s4.Use(); return Ok(); } + public IActionResult A2() { mediator.Use(); mapper.Use(); return Ok(); } + } + + // Compliant@+2: 4 specific deps injected, one in common between responsibility 1 and 2 + [ApiController] + public class ThreeSpecificDepsOneInCommonBetweenA1AndA2( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3) : ControllerBase + { + public IActionResult A1() { logger.Use(); mediator.Use(); s1.Use(); s2.Use(); return Ok(); } + public IActionResult A2() { mediator.Use(); mapper.Use(); s2.Use(); s3.Use(); return Ok(); } + } + } + + namespace ThreeActions + { + // Noncompliant@+2: 2 specific deps injected, each used in a separate responsibility + [ApiController] + public class ThreeResponsibilities( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2) : ControllerBase + { + public IActionResult A1() { logger.Use(); mediator.Use(); s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { mediator.Use(); mapper.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A3() { mapper.Use(); return Ok(); } + } + + // Noncompliant@+2: 3 specific deps injected, each used in a separate responsibility, possibly multiple times + [ApiController] + public class UpToThreeSpecificDepsController( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3) : ControllerBase + { + public IActionResult A1() { logger.Use(); s1.Use(); s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { logger.Use(); mapper.Use(); s2.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A3() { mediator.Use(); mapper.Use(); s3.Use(); s3.Use(); s3.Use(); return Ok(); } // Secondary {{May belong to responsibility #3.}} + } + + // Noncompliant@+2: 3 specific deps injected, each used in a separate responsibility + [ApiController] + public class ThreeResponsibilities2( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3) : ControllerBase + { + public IActionResult A1() { logger.Use(); mediator.Use(); s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { mediator.Use(); mapper.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A3() { s3.Use(); return Ok(); } // Secondary {{May belong to responsibility #3.}} + } + + // Noncompliant@+2: 3 specific deps injected, forming a chain and an isolated action + [ApiController] + public class ThreeSpecificDepsFormingAChainAndAnIsolatedAction( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3) : ControllerBase + { + // Chain: A1, A2 with s1 and s2 + public IActionResult A1() { logger.Use(); mediator.Use(); s1.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { mediator.Use(); mapper.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + // Isolated: A3 with s3 + public IActionResult A3() { s3.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + } + + // Noncompliant@+2: 4 specific deps injected, two for A1, one for A2, and one for A3 + [ApiController] + public class FourSpecificDepsTwoForA1OneForA2AndOneForA3( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : ControllerBase + { + public IActionResult A1() { logger.Use(); mediator.Use(); s1.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { mediator.Use(); mapper.Use(); s3.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A3() { s4.Use(); return Ok(); } // Secondary {{May belong to responsibility #3.}} + } + + // Noncompliant@+2: 4 specific deps injected, one for A1, one for A2, one for A3, one unused + [ApiController] + public class FourSpecificDepsOneForA1OneForA2OneForA3OneUnused( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : ControllerBase + { + public IActionResult A1() { logger.Use(); mediator.Use(); s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { mediator.Use(); mapper.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A3() { s3.Use(); return Ok(); } // Secondary {{May belong to responsibility #3.}} + } + + // Compliant@+2: 4 specific deps injected, forming a single 3-cycle + [ApiController] + public class FourSpecificDepsFormingACycle( + ILogger logger, IMediator mediator, IMapper mapper, + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : ControllerBase + { + // Cycle: A1, A2, A3 + public IActionResult A1() { logger.Use(); mediator.Use(); s1.Use(); s2.Use(); return Ok(); } + public IActionResult A2() { mediator.Use(); mapper.Use(); s2.Use(); s3.Use(); return Ok(); } + public IActionResult A3() { s3.Use(); s4.Use(); s1.Use(); return Ok(); } + } + } + + namespace SixActions + { + // Noncompliant@+2: 6 specific deps injected, forming 2 disconnected 3-cycles + [ApiController] + public class FourSpecificDepsFormingTwoDisconnectedCycles( + IS1 s1, IS2 s2, IS3 s3, IS4 s4, IS5 s5, IS6 s6) : ControllerBase + { + // Cycle 1: A1, A2, A3 + public IActionResult A1() { s1.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { s2.Use(); s3.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A3() { s3.Use(); s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + // Cycle 2: A4, A5, A6 (disconnected from cycle 1) + public IActionResult A4() { s4.Use(); s5.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A5() { s5.Use(); s6.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A6() { s6.Use(); s4.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + } + + // Compliant@+2: 5 specific deps injected, forming 2 connected 3-cycles + [ApiController] + public class FourSpecificDepsFormingTwoConnectedCycles( + IS1 s1, IS2 s2, IS3 s3, IS4 s4, IS5 s5) : ControllerBase + { + // Cycle 1: A1, A2, A3 + public IActionResult A1() { s1.Use(); s2.Use(); return Ok(); } + public IActionResult A2() { s2.Use(); s3.Use(); return Ok(); } + public IActionResult A3() { s3.Use(); s1.Use(); return Ok(); } + // Cycle 2: A4, A5, A6 (connected to cycle 1 via s1) + public IActionResult A4() { s1.Use(); s4.Use(); return Ok(); } + public IActionResult A5() { s4.Use(); s5.Use(); return Ok(); } + public IActionResult A6() { s5.Use(); s1.Use(); return Ok(); } + } + + // Compliant@+2: 4 specific deps injected, forming 2 3-cycles, connected by two dependencies (s1 and s2) + [ApiController] + public class FourSpecificDepsFormingTwoConnectedCycles2( + IS1 s1, IS2 s2, IS3 s3, IS4 s4) : ControllerBase + { + // Cycle 1: A1, A2, A3 + public IActionResult A1() { s1.Use(); s2.Use(); return Ok(); } + public IActionResult A2() { s2.Use(); s3.Use(); return Ok(); } + public IActionResult A3() { s3.Use(); s1.Use(); return Ok(); } + // Cycle 2: A4, A5, A6 + public IActionResult A4() { s1.Use(); s2.Use(); return Ok(); } + public IActionResult A5() { s2.Use(); s4.Use(); return Ok(); } + public IActionResult A6() { s4.Use(); s1.Use(); return Ok(); } + } + + // Compliant@+2: 4 specific deps injected, forming 2 3-cycles, connected by action invocations + [ApiController] + public class FourSpecificDepsFormingTwoConnectedCycles3( + IS1 s1, IS2 s2, IS3 s3, IS4 s4, IS5 s5, IS6 s6) : ControllerBase + { + // Cycle 1: A1, A2, A3 + public IActionResult A1() { s1.Use(); s2.Use(); return Ok(); } + public IActionResult A2() { s2.Use(); s3.Use(); return Ok(); } + public IActionResult A3() { s3.Use(); s1.Use(); return Ok(); } + // Cycle 2: A4, A5, A6, connected to cycle 1 via A1 invocation + public IActionResult A4() { A1(); s4.Use(); return Ok(); } + public IActionResult A5() { s5.Use(); s6.Use(); return Ok(); } + public IActionResult A6() { s6.Use(); s4.Use(); return Ok(); } + } + + // Noncompliant@+2: 6 specific deps injected, forming 3 disconnected 2-cycles + [ApiController] + public class FourSpecificDepsFormingThreeDisconnectedCycles( + IS1 s1, IS2 s2, IS3 s3, IS4 s4, IS5 s5, IS6 s6) : ControllerBase + { + // Cycle 1: A1, A2 + public IActionResult A1() { s1.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { s2.Use(); s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + // Cycle 2: A3, A4 + public IActionResult A3() { s3.Use(); s4.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A4() { s4.Use(); s3.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + // Cycle 3: A5, A6 + public IActionResult A5() { s5.Use(); s6.Use(); return Ok(); } // Secondary {{May belong to responsibility #3.}} + public IActionResult A6() { s6.Use(); s5.Use(); return Ok(); } // Secondary {{May belong to responsibility #3.}} + } + + // Noncompliant@+2: 6 specific deps injected, forming 2 connected 2-cycles and 1 disconnected 2-cycle + [ApiController] + public class FourSpecificDepsFormingTwoConnectedCyclesAndOneDisconnectedCycle( + IS1 s1, IS2 s2, IS3 s3, IS4 s4, IS5 s5, IS6 s6) : ControllerBase + { + // Cycle 1: A1, A2 + public IActionResult A1() { s1.Use(); s2.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A2() { s2.Use(); s1.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + // Cycle 2: A3, A4, connected to cycle 1 via A1 invocation + public IActionResult A3() { A1(); s3.Use(); s4.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + public IActionResult A4() { s4.Use(); s3.Use(); return Ok(); } // Secondary {{May belong to responsibility #1.}} + // Cycle 3: A5, A6 + public IActionResult A5() { s5.Use(); s6.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + public IActionResult A6() { s6.Use(); s5.Use(); return Ok(); } // Secondary {{May belong to responsibility #2.}} + } + + // Compliant@+2: 6 specific deps injected, forming 3 connected 2-cycles + [ApiController] + public class FourSpecificDepsFormingThreeConnectedCycles( + IS1 s1, IS2 s2, IS3 s3, IS4 s4, IS5 s5, IS6 s6) : ControllerBase + { + // Cycle 1: A1, A2 + public IActionResult A1() { s1.Use(); s2.Use(); return Ok(); } + public IActionResult A2() { s2.Use(); s1.Use(); return Ok(); } + // Cycle 2: A3, A4, connected to cycle 1 via A1 invocation + public IActionResult A3() { A1(); s3.Use(); s4.Use(); return Ok(); } + public IActionResult A4() { s4.Use(); s3.Use(); return Ok(); } + // Cycle 3: A5, A6, connected to cycle 1 via A2 invocation + public IActionResult A5() { A2(); s5.Use(); s6.Use(); return Ok(); } + public IActionResult A6() { s6.Use(); s5.Use(); return Ok(); } + } + + // Compliant@+2: 6 specific deps injected, forming 3 connected 2-cycles - transitivity of connection + [ApiController] + public class FourSpecificDepsFormingThreeConnectedCyclesTransitivity( + IS1 s1, IS2 s2, IS3 s3, IS4 s4, IS5 s5, IS6 s6) : ControllerBase + { + // Cycle 1: A1, A2 + public IActionResult A1() { s1.Use(); s2.Use(); return Ok(); } + public IActionResult A2() { s2.Use(); s1.Use(); return Ok(); } + // Cycle 2: A3, A4, connected to cycle 1 via A1 invocation + public IActionResult A3() { A1(); s3.Use(); s4.Use(); return Ok(); } + public IActionResult A4() { s4.Use(); s3.Use(); return Ok(); } + // Cycle 3: A5, A6, connected to cycle 1 via A2 invocation + public IActionResult A5() { A3(); s5.Use(); s6.Use(); return Ok(); } + public IActionResult A6() { s6.Use(); s5.Use(); return Ok(); } + } + } +} + +namespace WithInjectionViaNormalConstructor +{ + using TestInstrumentation.ResponsibilitySpecificServices; + using TestInstrumentation.WellKnownInterfacesExcluded; + using TestInstrumentation.WellKnownInterfacesNotExcluded; + using TestInstrumentation; + + [ApiController] + public class WithFields : ControllerBase // Noncompliant + { + private readonly IS1 s1; + private IS2 s2; + + public WithFields(IS1 s1, IS2 s2) { this.s1 = s1; this.s2 = s2; } + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #2.}} + } + + [ApiController] + public class WithDifferentVisibilities : ControllerBase // Noncompliant + { + private IS1 s1; + protected IS2 s2; + internal IS3 s3; + private protected IS4 s4; + protected internal IS5 s5; + public IS6 s6; + + public WithDifferentVisibilities(IS1 s1, IS2 s2, IS3 s3, IS4 s4, IS5 s5, IS6 s6) + { + this.s1 = s1; this.s2 = s2; this.s3 = s3; this.s4 = s4; this.s5 = s5; this.s6 = s6; + } + + private void A1() { s1.Use(); s2.Use(); } // Secondary {{May belong to responsibility #1.}} + protected void A2() { s2.Use(); s1.Use(); } // Secondary {{May belong to responsibility #1.}} + internal void A3() { s3.Use(); s4.Use(); } // Secondary {{May belong to responsibility #2.}} + private protected void A4() { s4.Use(); s3.Use(); } // Secondary {{May belong to responsibility #2.}} + protected internal void A5() { s5.Use(); s6.Use(); } // Secondary {{May belong to responsibility #3.}} + public void A6() { s6.Use(); s5.Use(); } // Secondary {{May belong to responsibility #3.}} + } + + [ApiController] + public class WithStaticFields : ControllerBase // Compliant, we don't take into account static fields and methods. + { + private static IS1 s1; + private static IS2 s2 = null; + + public WithStaticFields(IS1 s1, IS2 s2) { WithStaticFields.s1 = s1; WithStaticFields.s2 = s2; } + + static WithStaticFields() { s1 = S1.Instance; } + + public void A1() { s1.Use(); } + public void A2() { s2.Use(); } + + class S1 : IS1 { public void Use() { } public static IS1 Instance => new S1(); } + } + + [ApiController] + public class WithAutoProperties : ControllerBase // Noncompliant + { + public IS1 S1 { get; } + public IS2 S2 { get; set; } + protected IS3 S3 { get; init; } + private ILogger S4 { get; init; } // Well-known + + public WithAutoProperties(IS1 s1, IS2 s2, IS3 s3) { S1 = s1; S2 = s2; S3 = s3; } + + public void A1() { S1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { S2.Use(); } // Secondary {{May belong to responsibility #2.}} + public void A3() { S3.Use(); S2.Use(); } // Secondary {{May belong to responsibility #2.}} + public void A4() { S4.Use(); } // Only well-known service used => Ignored + } + + [ApiController] + public class WithFieldBackedProperties : ControllerBase // Noncompliant + { + private IS1 _s1; + private IS2 _s2; + + public IS1 S1 { get => _s1; } + public IS2 S2 { get => _s2; init => _s2 = value; } + + public WithFieldBackedProperties(IS1 s1, IS2 s2) { _s1 = s1; _s2 = s2; } + + public void A1() { S1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { S2.Use(); } // Secondary {{May belong to responsibility #2.}} + } + + [ApiController] + public class WithMixedStorageMechanismsAndPropertyDependency : ControllerBase // Compliant: single responsibility + { + // Property dependency: A3 -> S3 -> { _s3, _s1 } + private IS1 _s1; + private IS3 _s3; + + public IS2 S2 { get; set; } + public IS3 S3 { get => _s3; set { _s3 = value; _s1 = default; } } + + public WithMixedStorageMechanismsAndPropertyDependency(IS1 s1, IS2 s2, IS3 s3) { _s1 = s1; S2 = s2; _s3 = s3; } + + public void A1() { _s1.Use(); S2.Use(); } + public void A2() { S2.Use(); S3.Use(); } + public void A3() { S3.Use(); } + } + + [ApiController] + public class WithMixedStorageMechanismsAndPropertyDependencyTransitivity : ControllerBase + { + // Property dependency transitivity: A4 -> S4 -> { _s4, S3 } -> { _s4, _s3, S2 } + private IS1 _s1; + private IS3 _s3; + private IS4 _s4; + + public IS2 S2 { get; set; } + public IS3 S3 { get => _s3; set { _s3 = value; S2 = default; } } // Also resets S2 + public IS4 S4 { get => _s4; set { _s4 = value; S3 = default; } } // Also resets S3 + + public WithMixedStorageMechanismsAndPropertyDependencyTransitivity(IS1 s1, IS2 s2, IS3 s3, IS4 s4) + { _s1 = s1; S2 = s2; _s3 = s3; _s4 = s4; } + + public void A1() { _s1.Use(); S2.Use(); } + public void A2() { S2.Use(); S3.Use(); } + public void A3() { S3.Use(); _s1.Use(); } + public void A4() { S4.Use(); } + } + + [ApiController] + public class WithLambdaCapturingService : ControllerBase // Noncompliant: s1Provider and s2Provider explicitly wrap services + { + private readonly Func s1Provider; + private readonly Func s2Provider; + + public WithLambdaCapturingService(IS1 s1, IS2 s2) { s1Provider = () => s1; s2Provider = x => s2; } + + public void A1() { s1Provider().Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2Provider(42).Use(); } // Secondary {{May belong to responsibility #2.}} + } + + [ApiController] + public class WithNonPublicConstructor : ControllerBase // Noncompliant: ctor visibility is irrelevant + { + private readonly IS1 s1; + protected IS2 S2 { get; init; } + + private WithNonPublicConstructor(IS1 s1, IS2 s2) { this.s1 = s1; this.S2 = s2; } + + public void A1() { s1.Use(); s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { S2.Use(); } // Secondary {{May belong to responsibility #2.}} + } + + [ApiController] + public class WithCtorNotInitializingInjectedServices : ControllerBase // Noncompliant: initialization is irrelevant + { + private readonly IS1 s1; + internal IS2 s2; + + public WithCtorNotInitializingInjectedServices(IS1 s1, IS2 s2) { } + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #2.}} + } + + [ApiController] + public class WithServicesNotInjectedAtAll : ControllerBase // Noncompliant: ctor injection is irrelevant + { + private IS1 s1; + private IS2 s2; + + public WithServicesNotInjectedAtAll() { } + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #2.}} + } + + [ApiController] + public class WithServicesInitializedWithServiceProvider : ControllerBase // Noncompliant: service locator pattern is irrelevant + { + private readonly IS1 s1; + private readonly IS2 s2; + + public WithServicesInitializedWithServiceProvider(IServiceProvider serviceProvider) + { + s1 = serviceProvider.GetRequiredService(); + s2 = serviceProvider.GetRequiredService(); + } + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #2.}} + } + + [ApiController] + public class WithServicesInitializedWithSingletons : ControllerBase // Noncompliant: singleton pattern is irrelevant + { + private readonly IS1 s1 = S1.Instance; + private readonly IS2 s2 = S2.Instance; + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #2.}} + + class S1 : IS1 { public void Use() { } public static IS1 Instance => new S1(); } + class S2 : IS2 { public void Use() { } public static IS2 Instance => new S2(); } + } + + [ApiController] + public class WithServicesInitializedWithMixedStrategies : ControllerBase // Noncompliant + { + private readonly IS1 s1; + private readonly IS2 s2 = S2.Instance; + private readonly IS3 s3; + + public WithServicesInitializedWithMixedStrategies(IS1 s1, IServiceProvider serviceProvider) + { + this.s1 = s1; + s3 = serviceProvider.GetRequiredService(); + } + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #2.}} + public void A3() { s3.Use(); } // Secondary {{May belong to responsibility #3.}} + + class S2 : IS2 { public void Use() { } public static IS2 Instance => new S2(); } + } + + [ApiController] + public class WithAWellKnownInterfaceIncluded : ControllerBase // Noncompliant + { + private ILogger Logger { get; } + private readonly IS1 s1; + private readonly IS2 s2 = S2.Instance; + private readonly IS3 s3; + + public WithAWellKnownInterfaceIncluded(ILogger logger, IS1 s1, IServiceProvider serviceProvider) + { + Logger = logger; + this.s1 = s1; + s3 = serviceProvider.GetRequiredService(); + } + + public void A1() { Logger.Use(); s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { Logger.Use(); s2.Use(); } // Secondary {{May belong to responsibility #2.}} + public void A3() { Logger.Use(); s3.Use(); } // Secondary {{May belong to responsibility #3.}} + + class S2 : IS2 { public void Use() { } public static IS2 Instance => new S2(); } + } +} + +[ApiController] +public class WithHttpClientFactory : ControllerBase // Noncompliant +{ + private readonly IHttpClientFactory _httpClientFactory; // Well-known + private readonly IS1 s1; + private readonly IS2 s2; + + public void A1() { _httpClientFactory.Use(); s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { _httpClientFactory.Use(); s2.Use(); } // Secondary {{May belong to responsibility #2.}} + public void A3() { _httpClientFactory.Use(); } +} + +[ApiController] +public class WithUseInComplexBlocks : ControllerBase // Noncompliant +{ + IS1 s1; IS2 s2; IS3 s3; IS4 s4; IS5 s5; IS6 s6; IS7 s7; IS8 s8; IS9 s9; IS10 s10; + + public void If() // Secondary {{May belong to responsibility #2.}} + { + if (true) { s1.Use(); } else { if (false) { s2.Use(); } } + } + + public void Switch() // Secondary {{May belong to responsibility #2.}} + { + switch (0) { case 0: s2.Use(); break; case 1: s3.Use(); break; } + } + + public void For() // Secondary {{May belong to responsibility #2.}} + { + for (int i = 0; i < 1; i++) { s1.Use(); s3.Use(); } + } + + public void TryCatchFinally() // Secondary {{May belong to responsibility #3.}} + { + try { s4.Use(); } catch { s5.Use(); } finally { try { s6.Use(); } catch { s4.Use(); } } + } + + public void Using() // Secondary {{May belong to responsibility #3.}} + { + using (new ADisposable()) { s5.Use(); s7.Use(); } + } + + public void BlocksAndParentheses() // Secondary {{May belong to responsibility #1.}} + { + { { ((s8)).Use(); } } + } + + public void NestedLocalFunctions() // Secondary {{May belong to responsibility #1.}} + { + void LocalFunction() + { + void NestedLocalFunction() { s8.Use(); } + s9.Use(); + } + + LocalFunction(); + StaticLocalFunction(s10); + + static void StaticLocalFunction(IS10 s10) { s10.Use(); } + } + + class ADisposable : IDisposable { public void Dispose() { } } +} + +[ApiController] +public class WithMethodsDependingOnEachOther : ControllerBase // Noncompliant +{ + IS1 s1; IS2 s2; IS3 s3; IS4 s4; IS5 s5; IS6 s6; IS7 s7; + + // Chain: A2 to A1 + void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + void A2() { s2.Use(); A1(); } // Secondary {{May belong to responsibility #1.}} + void A3() { s2.Use(); } // Secondary {{May belong to responsibility #1.}} + // 1-cycle A4 => no service used => ignore + void A4() { A4(); } + // 2-cycle A5, A6 => no service used => ignore + void A5() { A6(); } + void A6() { A5(); } + // 3-cycle A7, A8, A9 => no service used => ignore + void A7() { A8(); } + void A8() { A9(); } + void A9() { A7(); } + // 3-cycle A10, A11, A12 with A13 depending on A12 via s3 + void A10() { A11(); } // Secondary {{May belong to responsibility #2.}} + void A11() { A12(); } // Secondary {{May belong to responsibility #2.}} + void A12() { A10(); s3.Use(); } // Secondary {{May belong to responsibility #2.}} + void A13() { s3.Use(); } // Secondary {{May belong to responsibility #2.}} + // 3-cycle A14, A15, A16 with chain A18 -> A15 via s4 + void A14() { A15(); s4.Use(); } // Secondary {{May belong to responsibility #3.}} + void A15() { A16(); } // Secondary {{May belong to responsibility #3.}} + void A16() { A14(); } // Secondary {{May belong to responsibility #3.}} + void A17() { s4.Use(); } // Secondary {{May belong to responsibility #3.}} + // Independent method => no service used => ignore + void A18() { } + // Independent method with its own service + void A19() { s5.Use(); } // Secondary {{May belong to responsibility #4.}} + // Two actions calling a third one + void A20() { A22(); } // Secondary {{May belong to responsibility #5.}} + void A21() { A22(); } // Secondary {{May belong to responsibility #5.}} + void A22() { s6.Use(); s7.Use(); } // Secondary {{May belong to responsibility #5.}} +} + +[ApiController] +public class WithServiceProvidersInjectionCoupled : ControllerBase // Compliant: s1Provider and s2Provider are known to provide services +{ + private readonly Func s1Provider; + private readonly Func s2Provider; + + public WithServiceProvidersInjectionCoupled(Func s1Provider, Func s2Provider) + { + this.s1Provider = s1Provider; + this.s2Provider = s2Provider; + } + + public void A1() { s1Provider().Use(); var s2 = s2Provider(42); s2.Use(); } + public void A2() { (s2Provider(42)).Use(); } +} + +[ApiController] +public class ApiController : ControllerBase { } + +public class DoesNotInheritDirectlyFromControllerBase(IS1 s1, IS2 s2) : ApiController // Compliant, we report only in classes that inherit directly from ControllerBase +{ + public IActionResult A1() { s1.Use(); return Ok(); } + public IActionResult A2() { s2.Use(); return Ok(); } +} + +public class InheritsFromController(IS1 s1, IS2 s2) : Controller // Compliant, we report only in classes that inherit directly from ControllerBase +{ + public IActionResult A1() { s1.Use(); return Ok(); } + public IActionResult A2() { s2.Use(); return Ok(); } +} + +public abstract class AbstractController (IS1 s1, IS2 s2) : ControllerBase // Compliant, we don't report on abstract controllers +{ + public IActionResult A1() { s1.Use(); return Ok(); } + public IActionResult A2() { s2.Use(); return Ok(); } + public abstract IActionResult A3(); +} + + +[ApiController] +public class WithServiceProvidersInjectionUsedInGroups : ControllerBase // Noncompliant +{ + private IS1 _s1; + private Func, IS5> _s5Provider; + + private Func S2Provider { get; } + private Func S3Provider => i => null; + private Func S4Provider { get; set; } = (s, c) => null; + private Func, IS5> S5Provider => _s5Provider; + + public void A1() { _s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { S2Provider().Use(); S3Provider(42).Use(); } // Secondary {{May belong to responsibility #3.}} + public void A3() { S3Provider(42).Use(); } // Secondary {{May belong to responsibility #3.}} + public void A4() { S4Provider("42", '4').Use(); _s5Provider(false, 3u, () => 42.0).Use(); } // Secondary {{May belong to responsibility #2.}} + public void A5() { _s5Provider(true, 3u, () => 42.0).Use(); } // Secondary {{May belong to responsibility #2.}} + public void A6() { S5Provider(true, 3u, () => 42.0).Use(); } // Secondary {{May belong to responsibility #2.}} +} + +[ApiController] +public class WithServiceWrappersInjection : ControllerBase // Compliant: no way to know whether s2Invoker wraps a service +{ + private readonly Func s1Provider; + private readonly Action s2Invoker; + + public WithServiceWrappersInjection(Func s1Provider, Action s2Invoker) + { + this.s1Provider = s1Provider; + this.s2Invoker = s2Invoker; + } + + public void A1() { s1Provider().Use(); s2Invoker(); } + public void A2() { s2Invoker(); } +} + +[ApiController] +public class WithLazyServicesInjection : ControllerBase // Noncompliant +{ + private readonly Lazy s1Lazy; + private readonly Lazy s2Lazy; + private readonly Lazy s3Lazy; + private readonly IS4 s4; + + public void A1() { s1Lazy.Value.Use(); s2Lazy.Value.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2Lazy.Value.Use(); s4.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A3() { s3Lazy.Value.Use(); } // Secondary {{May belong to responsibility #2.}} +} + +[ApiController] +public class WithMixOfLazyServicesAndServiceProviders : ControllerBase // Noncompliant +{ + private readonly Lazy s1Lazy; + private readonly Lazy s2Lazy; + private readonly IS3 s3; + private readonly IS4 s4; + private readonly Func s5Provider; + private readonly Func s6Provider; + + public void A1() { s1Lazy.Value.Use(); s3.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s1Lazy.Value.Use(); s2Lazy.Value.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A3() { s4.Use(); var s5 = s5Provider(); s5.Use(); } // Secondary {{May belong to responsibility #2.}} + public void A4() { s5Provider().Use(); var s6ProviderAlias = s6Provider(); s6ProviderAlias.Use(); } // Secondary {{May belong to responsibility #2.}} +} + +[ApiController] +public class WithIApi : ControllerBase // Compliant +{ + private readonly IApi _api; + + public WithIApi(IApi api) { _api = api; } + + public void A1() { _api.Use(); } + public void A2() { _api.Use(); } + + public interface IApi : IServiceWithAnAPI { } +} + +[ApiController] +public class WithUnusedServices : ControllerBase // Compliant +{ + private readonly IS1 s1; + private readonly IS2 s2; + private readonly IS3 s3; + private readonly IS4 s4; + + public void A1() { } + public void A2() { } + public void A3() { } + public void A4() { } +} + +[ApiController] +public class WithUnusedAndUsedServicesFormingTwoGroups : ControllerBase // Compliant +{ + // s2, s3 and s4 form a non-singleton, but no action use any of them => the set is filtered out + private readonly IS1 s1; + private readonly IS2 s2; + private readonly IS3 s3; + private readonly IS4 s4; + + public void A1() { s1.Use(); } +} + +[ApiController] +public class WithUnusedAndUsedServicesFormingThreeGroups : ControllerBase // Noncompliant {{This controller has multiple responsibilities and could be split into 3 smaller controllers.}} +{ + private readonly IS1 s1; + private readonly IS2 s2; + private readonly IS3 s3; + private readonly IS4 s4; + + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A3() { s3.Use(); } // Secondary {{May belong to responsibility #2.}} + public void A4() { s4.Use(); } // Secondary {{May belong to responsibility #3.}} +} + +[ApiController] +public class WithMethodOverloads : ControllerBase // Noncompliant +{ + private readonly IS1 s1; + private readonly IS2 s2; + private readonly IS3 s3; + private readonly IS4 s4; + private readonly IS5 s5; + private readonly IS6 s6; + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A1(int i) { s2.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A1(string s) { s3.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A1(int i, string s) { s4.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s5.Use(); } // Secondary {{May belong to responsibility #2.}} + public void A2(int i) { s6.Use(); } // Secondary {{May belong to responsibility #2.}} +} + +[ApiController] +public class WithIndexer : ControllerBase // Noncompliant +{ + private readonly IS1 s1; + private readonly IS2 s2; + private readonly IS3 s3; + + public int this[int i] { get { s1.Use(); return 42; } set { s2.Use(); } } // Clamp A1 and A2 together + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A3() { s3.Use(); } // Secondary {{May belong to responsibility #2.}} +} + +[ApiController] +public class WithIndexerOverloads : ControllerBase // Noncompliant +{ + private readonly IS1 s1; + private readonly IS2 s2; + private readonly IS3 s3; + private readonly IS4 s4; + private readonly IS5 s5; + + public int this[int i] { get { s1.Use(); return 42; } set { s2.Use(); } } // Clamp A1 and A2 together + public int this[long i] { get { s3.Use(); return 42; } set { s4.Use(); } } // Clamp A1 and A2 together + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A3() { s3.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A4() { s4.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A5() { s5.Use(); } // Secondary {{May belong to responsibility #2.}} +} + +[ApiController] +public class WithIndexerArrow : ControllerBase // Noncompliant +{ + private readonly IS1ReturningInt s1; + private readonly IS2ReturningInt s2; + private readonly IS3ReturningInt s3; + + public int this[int i] => 42 + s1.GetValue() + s2.GetValue(); // Clamp A1 and A2 together + + public void A1() { s1.GetValue(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.GetValue(); } // Secondary {{May belong to responsibility #1.}} + public void A3() { s3.GetValue(); } // Secondary {{May belong to responsibility #2.}} + + public interface IS1ReturningInt { int GetValue(); } + public interface IS2ReturningInt { int GetValue(); } + public interface IS3ReturningInt { int GetValue(); } +} + +[ApiController] +public class WithClassInjection : ControllerBase // Noncompliant +{ + private readonly Class1 s1; + private readonly Class2 s2; + private readonly Class3 s3; + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #2.}} + public void A3() { s3.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A4() { s3.Use(); s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A5() { } // No service used => ignore +} + +[ApiController] +public class WithStructInjection : ControllerBase // Noncompliant +{ + private readonly Struct1 s1; + private readonly Struct2 s2; + private readonly Struct3 s3; + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #2.}} + public void A3() { s3.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A4() { s3.Use(); s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A5() { } // No service used => ignore +} + +[ApiController] +public class WithMixOfInjectionTypes : ControllerBase // Noncompliant +{ + private readonly IS1 interface1; + private readonly IS2 interface2; + private readonly Class1 class1; + private readonly Class2 class2; + private readonly Class3 class3; // Unused + private readonly Struct1 struct1; + private readonly Struct2 struct2; + private readonly Struct3 struct3; // Unused + + private readonly Lazy lazy1; + private readonly Lazy lazy2; + private readonly Lazy lazy3; + private readonly Lazy lazy4; + private readonly Lazy lazy5; + private readonly Lazy> lazy6; + private readonly Lazy> lazy7; // Unused + + private readonly Func delegate1; + private readonly Func delegate2; + private readonly Func delegate3; + private readonly Func> delegate4; + private readonly Func> delegate5; // Unused + + public void A1() { interface1.Use(); class1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { interface2.Use(); class1.Use(); struct2.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A3() { class2.Use(); } // Secondary {{May belong to responsibility #4.}} + public void A4() { struct1.Use(); class2.Use(); _ = lazy4; } // Secondary {{May belong to responsibility #4.}} + public void A5() { struct2.Use(); lazy1.Value.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A6() { _ = lazy1.Value; lazy3.Value.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A7() { _ = lazy3.ToString(); } // Secondary {{May belong to responsibility #1.}} + public void A8() { lazy4.Value.Use(); _ = lazy5.IsValueCreated; } // Secondary {{May belong to responsibility #4.}} + public void A9() { _ = lazy6.GetHashCode(); delegate1().Use(); } // Secondary {{May belong to responsibility #2.}} + public void A10() { _ = delegate2(); delegate3.Invoke().Use(); } // Secondary {{May belong to responsibility #2.}} + public void A11() { _ = delegate1.Target; ((delegate3())).Use(); } // Secondary {{May belong to responsibility #2.}} + public void A12() { _ = delegate4(); } // Secondary {{May belong to responsibility #3.}} + public void A13() { } // No service used => ignored +} + +[ApiController] +public class WithDestructor : ControllerBase // Noncompliant +{ + private readonly IS1 s1; + private readonly IS2 s2; + + ~WithDestructor() { s1.Use(); s2.Use(); } + + public void A1() { s1.Use(); } // Secondary {{May belong to responsibility #1.}} + public void A2() { s2.Use(); } // Secondary {{May belong to responsibility #2.}} +} + +public class NotAControllerForCoverage +{ + private readonly IS1 s1; + private readonly IS2 s2; +} diff --git a/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/CoreMetadataReference.cs b/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/CoreMetadataReference.cs index d9e6a8bac33..7b31ffb8284 100644 --- a/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/CoreMetadataReference.cs +++ b/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/CoreMetadataReference.cs @@ -38,9 +38,10 @@ public static class CoreMetadataReference public static MetadataReference SystemCollectionsImmutable { get; } = CreateReference("System.Collections.Immutable.dll"); public static MetadataReference SystemCollectionsSpecialized { get; } = CreateReference("System.Collections.Specialized.dll"); public static MetadataReference SystemCollectionsNonGeneric { get; } = CreateReference("System.Collections.NonGeneric.dll"); - public static MetadataReference SystemConsole { get; } = CreateReference("System.Console.dll"); + public static MetadataReference SystemComponentModel { get; } = CreateReference("System.ComponentModel.dll"); public static MetadataReference SystemComponentModelPrimitives { get; } = CreateReference("System.ComponentModel.Primitives.dll"); public static MetadataReference SystemComponentModelTypeConverter { get; } = CreateReference("System.ComponentModel.TypeConverter.dll"); + public static MetadataReference SystemConsole { get; } = CreateReference("System.Console.dll"); public static MetadataReference SystemDataCommon { get; } = CreateReference("System.Data.Common.dll"); public static MetadataReference SystemDiagnosticsTraceSource { get; } = CreateReference("System.Diagnostics.TraceSource.dll"); public static MetadataReference SystemDiagnosticsProcess { get; } = CreateReference("System.Diagnostics.Process.dll");