Skip to content

Commit

Permalink
New rule S6965 for C#: You should use HttpAttribute in API controller…
Browse files Browse the repository at this point in the history
… actions
  • Loading branch information
gregory-paidis-sonarsource committed Apr 19, 2024
1 parent 02fcc9e commit 8bdc224
Show file tree
Hide file tree
Showing 9 changed files with 536 additions and 2 deletions.
77 changes: 77 additions & 0 deletions analyzers/rspec/cs/S6965.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<p>When building a <a href="https://learn.microsoft.com/en-us/aspnet/core/tutorials/first-web-api">REST API</a>, <a
href="https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/routing?view=aspnetcore-8.0#attribute-routing-with-http-verb-attributes">it’s
recommended</a> to annotate the controller actions with the available <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routing.httpmethodattribute">HTTP attributes</a> to be precise about what
your API supports.</p>
<h2>Why is this an issue?</h2>
<ul>
<li> <strong>Ambiguity</strong>: Without HttpAttributes, it’s unclear which HTTP methods an action method should respond to. This can lead to
confusion and make the code harder to understand and maintain. </li>
<li> <strong>Unsupported HTTP Methods</strong>: If an action is not annotated at all or is annotated only with the <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routeattribute">Route attribute</a>, it accepts all HTTP methods even if
they are not supported by that action, which leads to further confusion. </li>
<li> <strong>Problems with Swagger</strong>: <a
href="https://learn.microsoft.com/en-us/aspnet/core/tutorials/web-api-help-pages-using-swagger">Swagger</a> relies on HttpAttributes to generate
parts of the API documentation. These attributes are necessary for the generated documentation to be complete. </li>
<li> <strong>Route path conflicts</strong>: Without HttpAttributes, it’s possible to accidentally create action methods that respond to the same
route and HTTP method. This can lead to unexpected behavior and hard-to-diagnose bugs. </li>
<li> <strong>Lack of routing flexibility</strong>: The HTTP attributes allow you to define multiple action methods in the same controller that
respond to the same route but different HTTP methods. If you don’t use them, you might have limited flexibility when designing your API. </li>
</ul>
<h2>How to fix it</h2>
<p>You should annotate the controller actions with the available HttpMethod attributes. You can still use them in conjunction with the Route
attribute, in case there are multiple templates for one action and you need to <a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routeattribute.order?view=aspnetcore-8.0">set the order</a>. This allows
you to clearly define the HTTP methods each action method should respond to, while still being able to customize your routes.</p>
<h2>Exceptions</h2>
<p>This rule does not raise if the controller or the action is annotated with <code>[<a
href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.apiexplorersettingsattribute">ApiExplorerSettings</a>(IgnoreApi =
true)]</code> or <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.acceptverbsattribute"><code>AcceptsVerbs</code>
attribute</a>.</p>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
[Route("Customer")] // This route conflicts with GetCustomers action route
public async Task&lt;IResult&gt; ChangeCustomer([FromBody] CustomerData data) // Noncompliant
{
// ...
return Results.Ok();
}

[Route("Customer")] // This route conflicts with ChangeCustomer action route
public async Task&lt;string&gt; GetCustomers() // Noncompliant
{
return _customerRepository.GetAll();
}
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
[Route("Customer")]
[HttpPost]
public async Task&lt;IResult&gt; ChangeCustomer([FromBody] CustomerData data) // Compliant
{
// ...
return Results.Ok();
}

[HttpGet("Customer")]
public async Task&lt;string&gt; GetCustomers() // Compliant
{
return _customerRepository.GetAll();
}
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/routing">Routing to controller actions in ASP.NET
Core</a> </li>
<li> Microsoft Learn - <a
href="https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/routing#attribute-routing-with-http-verb-attributes">Attribute routing with Http
verb attributes</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/aspnet/core/web-api/handle-errors#exception-handler">ASP.NET Core Exception
handler</a> </li>
<li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routeattribute">RouteAttribute Class</a> </li>
</ul>

23 changes: 23 additions & 0 deletions analyzers/rspec/cs/S6965.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"title": "REST API actions should be annotated with an HTTP verb attribute",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
"tags": [
"asp.net"
],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-6965",
"sqKey": "S6965",
"scope": "Main",
"quickfix": "infeasible",
"code": {
"impacts": {
"MAINTAINABILITY": "MEDIUM"
},
"attribute": "CONVENTIONAL"
}
}
1 change: 1 addition & 0 deletions analyzers/rspec/cs/Sonar_way_profile.json
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@
"S6930",
"S6931",
"S6934",
"S6965",
"S6966"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* 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 AnnotateApiActionsWithHttpVerb : SonarDiagnosticAnalyzer
{
private const string DiagnosticId = "S6965";
private const string MessageFormat = "REST API controller actions should be annotated with the appropriate HTTP verb attribute.";

private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);

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

private static readonly ImmutableArray<KnownType> HttpMethodAttributes = ImmutableArray.Create(
KnownType.Microsoft_AspNetCore_Mvc_HttpGetAttribute,
KnownType.Microsoft_AspNetCore_Mvc_HttpPutAttribute,
KnownType.Microsoft_AspNetCore_Mvc_HttpPostAttribute,
KnownType.Microsoft_AspNetCore_Mvc_HttpDeleteAttribute,
KnownType.Microsoft_AspNetCore_Mvc_HttpPatchAttribute,
KnownType.Microsoft_AspNetCore_Mvc_HttpHeadAttribute,
KnownType.Microsoft_AspNetCore_Mvc_HttpOptionsAttribute,
KnownType.Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute,
KnownType.Microsoft_AspNetCore_Mvc_AcceptVerbsAttribute); // AcceptVerbs is treated as an exception

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterCompilationStartAction(compilationStartContext =>
{
if (!compilationStartContext.Compilation.ReferencesControllers())
{
return;
}
compilationStartContext.RegisterSymbolStartAction(symbolStartContext =>
{
var controllerSymbol = (INamedTypeSymbol)symbolStartContext.Symbol;
var controllerAttributes = controllerSymbol.GetAttributesWithInherited();
if (controllerSymbol.IsControllerType()
&& controllerAttributes.Any(x => x.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ApiControllerAttribute))
&& !IgnoresApiExplorer(controllerAttributes))
{
symbolStartContext.RegisterSyntaxNodeAction(c =>
{
var methodNode = (MethodDeclarationSyntax)c.Node;
var methodSymbol = c.SemanticModel.GetDeclaredSymbol(methodNode);
var methodAttributes = methodSymbol.GetAttributesWithInherited();
if (methodSymbol.IsControllerMethod()
&& !methodSymbol.IsAbstract
&& !methodAttributes.Any(x => x.AttributeClass.DerivesFromAny(HttpMethodAttributes))
&& !IgnoresApiExplorer(methodAttributes))
{
c.ReportIssue(Rule, methodNode.Identifier.GetLocation());
}
},
SyntaxKind.MethodDeclaration);
}
},
SymbolKind.NamedType);
});

// Tracks [ApiExplorerSettings(IgnoreApi = true)]
private bool IgnoresApiExplorer(IEnumerable<AttributeData> attributes) =>
attributes.FirstOrDefault(x => x.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_ApiExplorerSettingsAttribute)) is { } apiExplorerSettings
&& apiExplorerSettings.NamedArguments.FirstOrDefault(x => x.Key == "IgnoreApi").Value.Value is { } ignoreApi
&& ignoreApi.Equals(true);
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public static bool IsControllerType(this INamedTypeSymbol namedType) =>

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;
|| compilation.GetTypeByMetadataName(KnownType.Microsoft_AspNetCore_Mvc_Controller) is not null
|| compilation.GetTypeByMetadataName(KnownType.Microsoft_AspNetCore_Mvc_ControllerBase) is not null;
}
}
2 changes: 2 additions & 0 deletions analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public sealed partial class KnownType
public static readonly KnownType Microsoft_AspNetCore_Http_IHeaderDictionary = new("Microsoft.AspNetCore.Http.IHeaderDictionary");
public static readonly KnownType Microsoft_AspNetCore_Http_IRequestCookieCollection = new("Microsoft.AspNetCore.Http.IRequestCookieCollection");
public static readonly KnownType Microsoft_AspNetCore_Http_IResponseCookies = new("Microsoft.AspNetCore.Http.IResponseCookies");
public static readonly KnownType Microsoft_AspNetCore_Mvc_AcceptVerbsAttribute = new("Microsoft.AspNetCore.Mvc.AcceptVerbsAttribute");
public static readonly KnownType Microsoft_AspNetCore_Mvc_ApiControllerAttribute = new("Microsoft.AspNetCore.Mvc.ApiControllerAttribute");
public static readonly KnownType Microsoft_AspNetCore_Mvc_Controller = new("Microsoft.AspNetCore.Mvc.Controller");
public static readonly KnownType Microsoft_AspNetCore_Mvc_ControllerBase = new("Microsoft.AspNetCore.Mvc.ControllerBase");
Expand All @@ -75,6 +76,7 @@ public sealed partial class KnownType
public static readonly KnownType Microsoft_AspNetCore_Mvc_FromServicesAttribute = new("Microsoft.AspNetCore.Mvc.FromServicesAttribute");
public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpDeleteAttribute = new("Microsoft.AspNetCore.Mvc.HttpDeleteAttribute");
public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpGetAttribute = new("Microsoft.AspNetCore.Mvc.HttpGetAttribute");
public static readonly KnownType Microsoft_AspNetCore_Mvc_ApiExplorerSettingsAttribute = new("Microsoft.AspNetCore.Mvc.ApiExplorerSettingsAttribute");
public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpHeadAttribute = new("Microsoft.AspNetCore.Mvc.HttpHeadAttribute");
public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpOptionsAttribute = new("Microsoft.AspNetCore.Mvc.HttpOptionsAttribute");
public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpPatchAttribute = new("Microsoft.AspNetCore.Mvc.HttpPatchAttribute");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6889,8 +6889,8 @@ internal static class RuleTypeMappingCS
// ["S6962"],
// ["S6963"],
// ["S6964"],
// ["S6965"],
["S6966"] = "CODE_SMELL",
["S6965"] = "CODE_SMELL",
// ["S6967"],
// ["S6968"],
// ["S6969"],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* 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.
*/

#if NET
using SonarAnalyzer.Rules.CSharp;

namespace SonarAnalyzer.Test.Rules.AspNet;

[TestClass]
public class AnnotateApiActionsWithHttpVerbTest
{
private static readonly VerifierBuilder Builder = new VerifierBuilder<AnnotateApiActionsWithHttpVerb>()
.WithBasePath("AspNet")
.AddReferences(
[
AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcCore, // ControllerBase, ApiController, etc
AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcViewFeatures, // Controller
AspNetCoreMetadataReference.MicrosoftAspNetCoreHttpAbstractions, // StatusCodes
]);
[TestMethod]
public void AnnotateApiActionsWithHttpVerb_CS() =>
Builder
.AddPaths("AnnotateApiActionsWithHttpVerb.cs")
.Verify();
}
#endif
Loading

0 comments on commit 8bdc224

Please sign in to comment.