Skip to content

Commit

Permalink
New rule S6968 for C#: Actions that return a value should be annotate…
Browse files Browse the repository at this point in the history
…d with ProducesResponseTypeAttribute containing the return type (#9185)
  • Loading branch information
costin-zaharia-sonarsource committed Apr 24, 2024
1 parent 9e6e228 commit c83dc72
Show file tree
Hide file tree
Showing 20 changed files with 803 additions and 4 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ languages in [SonarQube](https://www.sonarsource.com/products/sonarqube), [Sonar

## Features

* [450+ C# rules](https://rules.sonarsource.com/csharp) and [210+ VB.​NET rules](https://rules.sonarsource.com/vbnet)
* [460+ C# rules](https://rules.sonarsource.com/csharp) and [210+ VB.​NET rules](https://rules.sonarsource.com/vbnet)
* Metrics (cognitive complexity, duplications, number of lines, etc.)
* Import of [test coverage reports](https://community.sonarsource.com/t/9871) from Visual Studio Code Coverage, dotCover, OpenCover, Coverlet, Altcover.
* Import of third-party Roslyn Analyzers results
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,8 @@
<ImplicitUsings>enable</ImplicitUsings>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Swashbuckle.AspNetCore.Swagger" Version="6.5.0" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using AspNetCore8.Models;
using Microsoft.AspNetCore.Mvc;

namespace AspNetCore8.Controllers;

[ApiController]
public class S6968Controller : ControllerBase
{
[HttpGet("foo")]
public IActionResult ReturnsOkWithValue() // Noncompliant
=> Ok(new Foo()); // Secondary
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
namespace AspNetCore8.Models;

public class Foo
{
public int Bar { get; set; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,24 @@
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/AspNetCore8/Controllers/S6930Controller.cs#L1",
"Location": "Line 1 Position 1-1"
},
{
"Id": "S1451",
"Message": "Add or update the header of this file.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/AspNetCore8/Controllers/S6968Controller.cs#L1",
"Location": "Line 1 Position 1-1"
},
{
"Id": "S1451",
"Message": "Add or update the header of this file.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/AspNetCore8/Models/ErrorViewModel.cs#L1",
"Location": "Line 1 Position 1-1"
},
{
"Id": "S1451",
"Message": "Add or update the header of this file.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/AspNetCore8/Models/Foo.cs#L1",
"Location": "Line 1 Position 1-1"
},
{
"Id": "S1451",
"Message": "Add or update the header of this file.",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"Issues": [
{
"Id": "S6934",
"Message": "Specify the RouteAttribute when an HttpMethodAttribute or RouteAttribute is specified at an action level.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/AspNetCore8/Controllers/S6968Controller.cs#L7",
"Location": "Line 7 Position 14-29"
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"Issues": [
{
"Id": "S6968",
"Message": "Annotate this method with ProducesResponseType containing the return type for successful responses.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/AspNetCore8/Controllers/S6968Controller.cs#L10",
"Location": "Line 10 Position 26-44"
}
]
}
98 changes: 98 additions & 0 deletions analyzers/rspec/cs/S6968.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
<p>In an <a href="https://learn.microsoft.com/en-us/aspnet/core">ASP.NET Core</a> <a href="https://en.wikipedia.org/wiki/Web_API">Web API</a>,
controller actions can optionally return a result value. If a controller action returns a value in the <a
href="https://en.wikipedia.org/wiki/Happy_path">happy path</a>, for example <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.controllerbase.ok#microsoft-aspnetcore-mvc-controllerbase-ok(system-object)">ControllerBase.Ok(Object)</a>,
annotating the action with one of the <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.producesresponsetypeattribute"><code>[ProducesResponseType]</code></a>
overloads that describe the type is recommended.</p>
<h2>Why is this an issue?</h2>
<p>If an ASP.NET Core Web API uses <a href="https://swagger.io/">Swagger</a>, the API documentation will be generated based on the input/output types
of the controller actions, as well as the attributes annotating the actions. If an action returns <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.iactionresult">IActionResult</a> or <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.iresult">IResult</a>, Swagger cannot infer the type of the response. From
the consumer’s perspective, this can be confusing and lead to unexpected results and bugs in the long run without the API provider’s awareness.</p>
<p>This rule raises an issue on a controller action when:</p>
<ul>
<li> The action returns a value in the happy path. This can be either:
<ul>
<li> <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200">200 OK</a> </li>
<li> <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/201">201 Created</a> </li>
<li> <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/202">202 Accepted</a> </li>
</ul> </li>
<li> There is no <code>[ProducesResponseType]</code> attribute containing the return type, either at controller or action level. </li>
<li> There is no <code>[SwaggerResponse]</code> attribute containing the return type, either at controller or action level. </li>
<li> The controller is annotated with the <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.apicontrollerattribute"><code>[ApiController]</code></a> attribute.
</li>
<li> The controller action returns either <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.iactionresult">IActionResult</a> or <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.iresult">IResult</a>. </li>
<li> The application has enabled the <a
href="https://learn.microsoft.com/en-us/aspnet/core/tutorials/getting-started-with-swashbuckle#add-and-configure-swagger-middleware">Swagger
middleware</a>. </li>
</ul>
<h2>How to fix it</h2>
<p>There are multiple ways to fix this issue:</p>
<ul>
<li> Annotate the action with <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.producesresponsetypeattribute"><code>[ProducesResponseType]</code></a>
containing the return type. </li>
<li> Annotate the action with <a
href="https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/master/README.md#enrich-response-metadata">SwaggerResponse Class</a> containing
the return type. </li>
<li> Return <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.actionresult-1">ActionResult&lt;TValue&gt;</a> instead of
<code>[IActionResult]</code> or <code>[IResult]</code>. </li>
<li> Return <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.httpresults.results-2">Results&lt;TResult1,
TResult2&gt;</a> instead of <code>[IActionResult]</code> or <code>[IResult]</code>. </li>
</ul>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
[HttpGet("foo")]
// Noncompliant: Annotate this method with ProducesResponseType containing the return type for succesful responses.
public IActionResult MagicNumber() =&gt; Ok(42);
</pre>
<pre data-diff-id="2" data-diff-type="noncompliant">
[HttpGet("foo")]
// Noncompliant: Use the ProducesResponseType overload containing the return type for succesful responses.
[ProducesResponseType(StatusCodes.Status200OK)]
public IActionResult MagicNumber() =&gt; Ok(42);
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
[HttpGet("foo")]
[ProducesResponseType&lt;int&gt;(StatusCodes.Status200OK)]
public IActionResult MagicNumber() =&gt; Ok(42);
</pre>
<pre data-diff-id="2" data-diff-type="compliant">
[HttpGet("foo")]
[ProducesResponseType(typeof(int), StatusCodes.Status200OK)]
public IActionResult MagicNumber() =&gt; Ok(42);
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> Wikipedia - <a href="https://en.wikipedia.org/wiki/Web_API">Web API</a> </li>
<li> Wikipedia - <a href="https://en.wikipedia.org/wiki/Happy_path">Happy path</a> </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core">ASP.NET Core</a> </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core/tutorials/getting-started-with-swashbuckle">Get started with
Swashbuckle and ASP.NET Core</a> </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.apicontrollerattribute">ApiControllerAttribute
Class</a> </li>
<li> Microsoft Learn - <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.producesresponsetypeattribute">ProducesResponseTypeAttribute Class</a>
</li>
<li> Microsoft Learn - <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.producesresponsetypeattribute-1">ProducesResponseTypeAttribute&lt;T&gt;
Class</a> </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.actionresult-1">ActionResult&lt;TValue&gt;
Class</a> </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.httpresults.results-2">Results&lt;TResult1,
TResult2&gt; Class</a> </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core/web-api/action-return-types#httpresults-type">HttpResults type</a>
</li>
<li> GitHub - <a href="https://github.com/domaindrivendev/Swashbuckle.AspNetCore/blob/master/README.md#enrich-response-metadata">SwaggerResponse
Class</a> </li>
<li> SmartBear - <a href="https://swagger.io/">Swagger</a> </li>
</ul>

23 changes: 23 additions & 0 deletions analyzers/rspec/cs/S6968.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"title": "Actions that return a value should be annotated with ProducesResponseTypeAttribute containing the return type",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
"tags": [
"asp.net"
],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-6968",
"sqKey": "S6968",
"scope": "Main",
"quickfix": "partial",
"code": {
"impacts": {
"MAINTAINABILITY": "HIGH"
},
"attribute": "CLEAR"
}
}
3 changes: 2 additions & 1 deletion analyzers/rspec/cs/Sonar_way_profile.json
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@
"S6961",
"S6962",
"S6965",
"S6966"
"S6966",
"S6968"
]
}
156 changes: 156 additions & 0 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/SwaggerActionReturnType.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2024 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Rules.CSharp;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class SwaggerActionReturnType : SonarDiagnosticAnalyzer
{
private const string DiagnosticId = "S6968";
private const string MessageFormat = "{0}";
private const string NoAttributeMessageFormat = "Annotate this method with ProducesResponseType containing the return type for successful responses.";
private const string NoTypeMessageFormat = "Use the ProducesResponseType overload containing the return type for successful responses.";

private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);
private static readonly ImmutableArray<KnownType> ControllerActionReturnTypes = ImmutableArray.Create(
KnownType.Microsoft_AspNetCore_Mvc_IActionResult,
KnownType.Microsoft_AspNetCore_Http_IResult);
private static HashSet<string> ActionResultMethods =>
[
"Ok",
"Created",
"CreatedAtAction",
"CreatedAtRoute",
"Accepted",
"AcceptedAtAction",
"AcceptedAtRoute"
];
private static HashSet<string> ResultMethods =>
[
"Ok",
"Created",
"CreatedAtRoute",
"Accepted",
"AcceptedAtRoute"
];

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

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterCompilationStartAction(compilationStart =>
{
if (!compilationStart.Compilation.Assembly.HasAttribute(KnownType.Microsoft_AspNetCore_Mvc_ApiConventionTypeAttribute)
&& compilationStart.Compilation.ReferencesAll(KnownAssembly.MicrosoftAspNetCoreMvcCore, KnownAssembly.SwashbuckleAspNetCoreSwagger))
{
compilationStart.RegisterSymbolStartAction(symbolStart =>
{
if (IsControllerCandidate(symbolStart.Symbol))
{
symbolStart.RegisterSyntaxNodeAction(nodeContext =>
{
var methodDeclaration = (MethodDeclarationSyntax)nodeContext.Node;
if (InvalidMethod(methodDeclaration, nodeContext) is { } method)
{
nodeContext.ReportIssue(Rule, methodDeclaration.Identifier, additionalLocations: method.ResponseInvocations, GetMessage(method.Symbol));
}
}, SyntaxKind.MethodDeclaration);
}
}, SymbolKind.NamedType);
}
});

private static InvalidMethodResult InvalidMethod(BaseMethodDeclarationSyntax methodDeclaration, SonarSyntaxNodeReportingContext nodeContext)
{
var responseInvocations = FindSuccessResponses(methodDeclaration, nodeContext.SemanticModel);
return responseInvocations.Length == 0
|| nodeContext.SemanticModel.GetDeclaredSymbol(methodDeclaration, nodeContext.Cancel) is not { } method
|| !method.IsControllerMethod()
|| !method.ReturnType.DerivesOrImplementsAny(ControllerActionReturnTypes)
|| method.GetAttributesWithInherited().Any(x => x.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ApiConventionMethodAttribute)
|| HasApiExplorerSettingsWithIgnoreApiTrue(x)
|| HasProducesResponseTypeAttributeWithReturnType(x))
? null
: new InvalidMethodResult(method, responseInvocations);
}

private static SyntaxNode[] FindSuccessResponses(SyntaxNode node, SemanticModel model)
{
return ActionResultInvocations().Concat(ObjectCreationInvocations()).Concat(ResultMethodsInvocations()).ToArray();

IEnumerable<SyntaxNode> ActionResultInvocations() =>
node.DescendantNodes()
.OfType<InvocationExpressionSyntax>()
.Where(x => ActionResultMethods.Contains(x.GetName())
&& x.ArgumentList.Arguments.Count > 0
&& model.GetSymbolInfo(x.Expression).Symbol is { } symbol
&& symbol.IsInType(KnownType.Microsoft_AspNetCore_Mvc_ControllerBase)
&& symbol.GetParameters().Any(parameter => parameter.HasAttribute(KnownType.Microsoft_AspNetCore_Mvc_Infrastructure_ActionResultObjectValueAttribute)));

IEnumerable<SyntaxNode> ObjectCreationInvocations() =>
node.DescendantNodes()
.OfType<ObjectCreationExpressionSyntax>()
.Where(x => x.GetName() == "ObjectResult"
&& x.ArgumentList?.Arguments.Count > 0
&& model.GetSymbolInfo(x.Type).Symbol.GetSymbolType().Is(KnownType.Microsoft_AspNetCore_Mvc_ObjectResult));

IEnumerable<SyntaxNode> ResultMethodsInvocations() =>
node.DescendantNodes()
.OfType<InvocationExpressionSyntax>()
.Where(x => ResultMethods.Contains(x.GetName())
&& x.ArgumentList.Arguments.Count > 0
&& model.GetSymbolInfo(x).Symbol.IsInType(KnownType.Microsoft_AspNetCore_Http_Results));
}

private static bool IsControllerCandidate(ISymbol symbol)
{
var hasApiControllerAttribute = false;
foreach (var attribute in symbol.GetAttributesWithInherited())
{
if (attribute.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ApiConventionTypeAttribute)
|| HasProducesResponseTypeAttributeWithReturnType(attribute)
|| HasApiExplorerSettingsWithIgnoreApiTrue(attribute))
{
return false;
}
hasApiControllerAttribute = hasApiControllerAttribute || attribute.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ApiControllerAttribute);
}
return hasApiControllerAttribute;
}

private static string GetMessage(ISymbol symbol) =>
symbol.GetAttributesWithInherited().Any(x => x.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ProducesResponseTypeAttribute))
? NoTypeMessageFormat
: NoAttributeMessageFormat;

private static bool HasProducesResponseTypeAttributeWithReturnType(AttributeData attribute) =>
attribute.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ProducesResponseTypeAttribute_T)
|| (attribute.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ProducesResponseTypeAttribute)
&& ContainsReturnType(attribute));

private static bool HasApiExplorerSettingsWithIgnoreApiTrue(AttributeData attribute) =>
attribute.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ApiExplorerSettingsAttribute)
&& attribute.NamedArguments.FirstOrDefault(x => x.Key == "IgnoreApi").Value.Value is true;

private static bool ContainsReturnType(AttributeData attribute) =>
!attribute.ConstructorArguments.FirstOrDefault(x => x.Type.Is(KnownType.System_Type)).IsNull
|| attribute.NamedArguments.FirstOrDefault(x => x.Key == "Type").Value.Value is not null;

private sealed record InvalidMethodResult(IMethodSymbol Symbol, SyntaxNode[] ResponseInvocations);
}
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ public abstract class SonarTreeReportingContextBase<TContext> : SonarReportingCo
public void ReportIssue(DiagnosticDescriptor rule, SyntaxToken locationToken, params object[] messageArgs) =>
ReportIssue(rule, locationToken.GetLocation(), messageArgs);

public void ReportIssue(DiagnosticDescriptor rule, SyntaxToken locationToken, IEnumerable<SyntaxNode> additionalLocations, params object[] messageArgs) =>
ReportIssueCore(Diagnostic.Create(rule, locationToken.GetLocation(), additionalLocations.Select(x => x.GetLocation()), messageArgs));

public void ReportIssue(DiagnosticDescriptor rule, Location location, params object[] messageArgs) =>
ReportIssueCore(Diagnostic.Create(rule, location, messageArgs));
}
Expand Down

0 comments on commit c83dc72

Please sign in to comment.