Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@costin-zaharia-sonarsource
costin-zaharia-sonarsource committed Mar 22, 2024
1 parent 97d80f8 commit 3042fd3
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 66 deletions.
49 changes: 25 additions & 24 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/SpecifyRouteAttribute.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,42 +27,40 @@ public sealed class SpecifyRouteAttribute() : SonarDiagnosticAnalyzer<SyntaxKind
{
private const string DiagnosticId = "S6934";

private static readonly ImmutableArray<KnownType> RouteTemplateAttributes =
ImmutableArray.Create(KnownType.Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute, KnownType.Microsoft_AspNetCore_Mvc_RouteAttribute);
private static readonly ImmutableArray<KnownType> RouteTemplateAttributes = ImmutableArray.Create(
KnownType.Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute,
KnownType.Microsoft_AspNetCore_Mvc_RouteAttribute);

protected override string MessageFormat => "Specify the RouteAttribute when an HttpMethodAttribute is specified at an action level.";
protected override string MessageFormat => "Specify the RouteAttribute when an HttpMethodAttribute or RouteAttribute is specified at an action level.";
protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance;

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterCompilationStartAction(compilationStart =>
{
if (!UsesAttributeRouting(compilationStart.Compilation))
{
return;
}
compilationStart.RegisterSymbolStartAction(symbolStart =>
{
if (compilationStart.Compilation.GetTypeByMetadataName(KnownType.Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute) is null)
if (symbolStart.Symbol.GetAttributes().Any(x => x.AttributeClass.Is(KnownType.Microsoft_AspNetCore_Mvc_RouteAttribute)))
{
return;
}
compilationStart.RegisterSymbolStartAction(symbolStart =>
var secondaryLocations = new ConcurrentStack<Location>();
symbolStart.RegisterSyntaxNodeAction(nodeContext =>
{
if (symbolStart.Symbol.GetAttributes().Any(x => x.AttributeClass.Is(KnownType.Microsoft_AspNetCore_Mvc_RouteAttribute)))
var methodDeclaration = (MethodDeclarationSyntax)nodeContext.Node;
if (nodeContext.SemanticModel.GetDeclaredSymbol(methodDeclaration, nodeContext.Cancel) is { } method
&& method.IsControllerMethod()
&& method.GetAttributes().Any(x => !CanBeIgnored(x.GetAttributeRouteTemplate(RouteTemplateAttributes))))
{
return;
secondaryLocations.Push(methodDeclaration.Identifier.GetLocation());
}
var secondaryLocations = new ConcurrentStack<Location>();
symbolStart.RegisterSyntaxNodeAction(nodeContext =>
{
var node = (MethodDeclarationSyntax)nodeContext.Node;
if (nodeContext.SemanticModel.GetDeclaredSymbol(node, nodeContext.Cancel) is { } method
&& method.IsControllerMethod()
&& method.GetAttributes().Any(x =>
x.AttributeClass.DerivesFromAny(RouteTemplateAttributes)
&& x.TryGetAttributeValue<string>("template", out var template)
&& !CanBeIgnored(template)))
{
secondaryLocations.Push(node.Identifier.GetLocation());
}
}, SyntaxKind.MethodDeclaration);
symbolStart.RegisterSymbolEndAction(symbolEnd => ReportIssues(symbolEnd, symbolStart.Symbol, secondaryLocations));
}, SymbolKind.NamedType);
});
}, SyntaxKind.MethodDeclaration);
symbolStart.RegisterSymbolEndAction(symbolEnd => ReportIssues(symbolEnd, symbolStart.Symbol, secondaryLocations));
}, SymbolKind.NamedType);
});

private void ReportIssues(SonarSymbolReportingContext context, ISymbol symbol, ConcurrentStack<Location> secondaryLocations)
{
Expand All @@ -80,6 +78,9 @@ private void ReportIssues(SonarSymbolReportingContext context, ISymbol symbol, C
}
}

private static bool UsesAttributeRouting(Compilation compilation) =>
compilation.GetTypeByMetadataName(KnownType.Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute) is not null;

private static bool CanBeIgnored(string template) =>
string.IsNullOrWhiteSpace(template)
// See: https://learn.microsoft.com/en-us/aspnet/core/mvc/controllers/routing#combining-attribute-routes
Expand Down
6 changes: 6 additions & 0 deletions analyzers/src/SonarAnalyzer.Common/Extensions/AttributeDataExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ public static bool HasName(this AttributeData attribute, string name) =>
public static bool HasAnyName(this AttributeData attribute, params string[] names) =>
names.Any(x => attribute.HasName(x));

public static string GetAttributeRouteTemplate(this AttributeData attribute, ImmutableArray<KnownType> attributeTypes) =>
attribute.AttributeClass.DerivesFromAny(attributeTypes)
&& attribute.TryGetAttributeValue<string>("template", out var template)
? template
: null;

public static bool TryGetAttributeValue<T>(this AttributeData attribute, string valueName, out T value)
{
// named arguments take precedence over constructor arguments of the same name. For [Attr(valueName: false, valueName = true)] "true" is returned.
Expand Down
13 changes: 8 additions & 5 deletions analyzers/src/SonarAnalyzer.Common/Rules/RouteTemplateShouldNotStartWithSlashBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,19 @@

namespace SonarAnalyzer.Rules;

public abstract class RouteTemplateShouldNotStartWithSlashBase<TSyntaxKind> : SonarDiagnosticAnalyzer<TSyntaxKind>
public abstract class RouteTemplateShouldNotStartWithSlashBase<TSyntaxKind>() : SonarDiagnosticAnalyzer<TSyntaxKind>(DiagnosticId)
where TSyntaxKind : struct
{
private const string DiagnosticId = "S6931";
private const string MessageOnlyActions = "Change the paths of the actions of this controller to be relative and adapt the controller route accordingly.";
private const string MessageActionsAndController = "Change the paths of the actions of this controller to be relative and add a controller route with the common prefix.";

private static readonly ImmutableArray<KnownType> RouteTemplateAttributes = ImmutableArray.Create(
KnownType.Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute,
KnownType.Microsoft_AspNetCore_Mvc_RouteAttribute,
KnownType.System_Web_Mvc_RouteAttribute);

protected override string MessageFormat => "{0}";
protected RouteTemplateShouldNotStartWithSlashBase() : base(DiagnosticId) { }

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterCompilationStartAction(compilationStartContext =>
Expand Down Expand Up @@ -91,10 +95,9 @@ SyntaxNode ControllerDeclarationSyntax(INamedTypeSymbol symbol) =>
private static Dictionary<Location, string> RouteAttributeTemplateArguments(ImmutableArray<AttributeData> attributes)
{
var templates = new Dictionary<Location, string>();
var routeAttributes = attributes.Where(x => x.AttributeClass.IsAny(KnownType.RouteAttributes) || x.AttributeClass.DerivesFrom(KnownType.Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute));
foreach (var attribute in routeAttributes)
foreach (var attribute in attributes)
{
if (attribute.TryGetAttributeValue<string>("template", out var templateParameter))
if (attribute.GetAttributeRouteTemplate(RouteTemplateAttributes) is { } templateParameter)
{
templates.Add(attribute.ApplicationSyntaxReference.GetSyntax().GetLocation(), templateParameter);
}
Expand Down
62 changes: 27 additions & 35 deletions analyzers/tests/SonarAnalyzer.Test/Rules/SpecifyRouteAttributeTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,63 +31,55 @@ public class SpecifyRouteAttributeTest
.WithOptions(ParseOptionsHelper.FromCSharp12)
.AddReferences([
AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcCore,
AspNetCoreMetadataReference.MicrosoftAspNetCoreHttpAbstractions,
AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcViewFeatures,
AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcAbstractions,
AspNetCoreMetadataReference.MicrosoftAspNetCoreHttpFeatures,
AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcAbstractions
]);

[TestMethod]
public void SpecifyRouteAttribute_CS() =>
builder.AddPaths("SpecifyRouteAttribute.cs").Verify();
builder.AddPaths("SpecifyRouteAttribute.CSharp12.cs").Verify();

[TestMethod]
public void SpecifyRouteAttribute_PartialClasses_CS() =>
builder
.AddSnippet("""
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Routing;
using Microsoft.AspNetCore.Mvc;
public partial class HomeController : Controller // Noncompliant [first]
{
[HttpGet("Test")]
public IActionResult Index() => View(); // Secondary [first, second]
}
""")
public partial class HomeController : Controller // Noncompliant [first]
{
[HttpGet("Test")]
public IActionResult Index() => View(); // Secondary [first, second]
}
""")
.AddSnippet("""
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Routing;
using Microsoft.AspNetCore.Mvc;
public partial class HomeController : Controller // Noncompliant [second]
{
}
""")
public partial class HomeController : Controller { } // Noncompliant [second]
""")
.Verify();

[TestMethod]
public void SpecifyRouteAttribute_PartialClasses_OneGenerated_CS() =>
builder
.AddSnippet("""
// <auto-generated/>
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Routing;
// <auto-generated/>
using Microsoft.AspNetCore.Mvc;
public partial class HomeController : Controller
{
[HttpGet("Test")]
public IActionResult ActionInGeneratedCode() => View(); // Secondary
}
""")
public partial class HomeController : Controller
{
[HttpGet("Test")]
public IActionResult ActionInGeneratedCode() => View(); // Secondary
}
""")
.AddSnippet("""
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Routing;
using Microsoft.AspNetCore.Mvc;
public partial class HomeController : Controller // Noncompliant
{
[HttpGet("Test")]
public IActionResult Index() => View(); // Secondary
}
""")
public partial class HomeController : Controller // Noncompliant
{
[HttpGet("Test")]
public IActionResult Index() => View(); // Secondary
}
""")
.Verify();
}

Expand Down
4 changes: 2 additions & 2 deletions ...r.Test/TestCases/SpecifyRouteAttribute.cs → ...stCases/SpecifyRouteAttribute.CSharp12.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public class RouteTemplateIsNotSpecifiedController : Controller
public IActionResult Error() => View(); // Compliant
}

public class RouteTemplatesAreSpecifiedController : Controller // Noncompliant [controller] {{Specify the RouteAttribute when an HttpMethodAttribute is specified at an action level.}}
public class RouteTemplatesAreSpecifiedController : Controller // Noncompliant [controller] {{Specify the RouteAttribute when an HttpMethodAttribute or RouteAttribute is specified at an action level.}}
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
{
private const string ConstantRoute = "ConstantRoute";
Expand Down Expand Up @@ -175,4 +175,4 @@ public class NonController
public class DerivedController : Controller { }

[Controller]
public class Endpoint {}
public class Endpoint { }

0 comments on commit 3042fd3

Please sign in to comment.