Fix ambiguous route detection for [Route] with HTTP methods (dotnet#4…

…7106)
Applesauce314 · Mar 9, 2023 · 114171c · 114171c
1 parent 34a16dc
commit 114171c
Show file tree

Hide file tree

Showing 2 changed files with 140 additions and 10 deletions.
diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Mvc/MvcAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Mvc/MvcAnalyzer.cs
@@ -66,6 +66,10 @@ private static void PopulateActionRoutes(SymbolAnalysisContext context, WellKnow
             if (member is IMethodSymbol methodSymbol &&
                 MvcDetector.IsAction(methodSymbol, wellKnownTypes))
             {
+                // [Route("xxx")] attributes don't have a HTTP method and instead use the HTTP methods of other attributes.
+                // For example, [HttpGet] + [HttpPost] + [Route("xxx")] means the route "xxx" is combined with the HTTP methods.
+                var unroutedHttpMethods = GetUnroutedMethodHttpMethods(wellKnownTypes, methodSymbol);
+
                 foreach (var attribute in methodSymbol.GetAttributes())
                 {
                     if (attribute.AttributeClass is null || !wellKnownTypes.IsType(attribute.AttributeClass, RouteAttributeTypes, out var match))
@@ -79,24 +83,52 @@ private static void PopulateActionRoutes(SymbolAnalysisContext context, WellKnow
                         continue;
                     }
 
-                    actionRoutes.Add(new ActionRoute(methodSymbol, routeUsage, GetHttpMethods(match.Value)));
+                    // [Route] uses unrouted HTTP verb attributes for its HTTP methods.
+                    var methods = match.Value is WellKnownType.Microsoft_AspNetCore_Mvc_RouteAttribute
+                        ? unroutedHttpMethods
+                        : ImmutableArray.Create(GetHttpMethod(match.Value)!);
+
+                    actionRoutes.Add(new ActionRoute(methodSymbol, routeUsage, methods));
                 }
             }
         }
     }
 
-    private static ImmutableArray<string> GetHttpMethods(WellKnownType match)
+    private static ImmutableArray<string> GetUnroutedMethodHttpMethods(WellKnownTypes wellKnownTypes, IMethodSymbol methodSymbol)
+    {
+        var httpMethodsBuilder = ImmutableArray.CreateBuilder<string>();
+        foreach (var attribute in methodSymbol.GetAttributes())
+        {
+            if (attribute.AttributeClass is null || !wellKnownTypes.IsType(attribute.AttributeClass, RouteAttributeTypes, out var match))
+            {
+                continue;
+            }
+            if (!attribute.ConstructorArguments.IsEmpty)
+            {
+                continue;
+            }
+
+            if (GetHttpMethod(match.Value) is { } method)
+            {
+                httpMethodsBuilder.Add(method);
+            }
+        }
+
+        return httpMethodsBuilder.ToImmutable();
+    }
+
+    private static string? GetHttpMethod(WellKnownType match)
     {
         return match switch
         {
-            WellKnownType.Microsoft_AspNetCore_Mvc_RouteAttribute => ImmutableArray<string>.Empty,// No HTTP method.
-            WellKnownType.Microsoft_AspNetCore_Mvc_HttpDeleteAttribute => ImmutableArray.Create("DELETE"),
-            WellKnownType.Microsoft_AspNetCore_Mvc_HttpGetAttribute => ImmutableArray.Create("GET"),
-            WellKnownType.Microsoft_AspNetCore_Mvc_HttpHeadAttribute => ImmutableArray.Create("HEAD"),
-            WellKnownType.Microsoft_AspNetCore_Mvc_HttpOptionsAttribute => ImmutableArray.Create("OPTIONS"),
-            WellKnownType.Microsoft_AspNetCore_Mvc_HttpPatchAttribute => ImmutableArray.Create("PATCH"),
-            WellKnownType.Microsoft_AspNetCore_Mvc_HttpPostAttribute => ImmutableArray.Create("POST"),
-            WellKnownType.Microsoft_AspNetCore_Mvc_HttpPutAttribute => ImmutableArray.Create("PUT"),
+            WellKnownType.Microsoft_AspNetCore_Mvc_RouteAttribute => null,// No HTTP method.
+            WellKnownType.Microsoft_AspNetCore_Mvc_HttpDeleteAttribute => "DELETE",
+            WellKnownType.Microsoft_AspNetCore_Mvc_HttpGetAttribute => "GET",
+            WellKnownType.Microsoft_AspNetCore_Mvc_HttpHeadAttribute => "HEAD",
+            WellKnownType.Microsoft_AspNetCore_Mvc_HttpOptionsAttribute => "OPTIONS",
+            WellKnownType.Microsoft_AspNetCore_Mvc_HttpPatchAttribute => "PATCH",
+            WellKnownType.Microsoft_AspNetCore_Mvc_HttpPostAttribute => "POST",
+            WellKnownType.Microsoft_AspNetCore_Mvc_HttpPutAttribute => "PUT",
             _ => throw new InvalidOperationException("Unexpected well known type:" + match),
         };
     }

diff --git a/src/Framework/AspNetCoreAnalyzers/test/Mvc/DetectAmbiguousActionRoutesTest.cs b/src/Framework/AspNetCoreAnalyzers/test/Mvc/DetectAmbiguousActionRoutesTest.cs
@@ -321,5 +321,103 @@ static void Main(string[] args)
         // Act & Assert
         await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics);
     }
+
+    [Fact]
+    public async Task DuplicateRoutes_HasHttpAttributes_NoDiagnostics()
+    {
+        // Arrange
+        var source = @"
+using Microsoft.AspNetCore.Builder;
+using Microsoft.AspNetCore.Mvc;
+public class MyController : Controller
+{
+    [HttpGet]
+    [Route(""Person"")]
+    public IActionResult Get() => Ok(""You GET me"");
+    
+    [HttpGet(""PersonGet"")]
+    [HttpPut]
+    [HttpPost]
+    [Route(""Person"")]
+    public IActionResult Put() => Ok(""You PUT me"");
+}
+internal class Program
+{
+    static void Main(string[] args)
+    {
+    }
+}
+";
+
+        // Act & Assert
+        await VerifyCS.VerifyAnalyzerAsync(source);
+    }
+
+    [Fact]
+    public async Task DuplicateRoutes_HasDuplicateHttpAttributes_HasDiagnostics()
+    {
+        // Arrange
+        var source = @"
+using Microsoft.AspNetCore.Builder;
+using Microsoft.AspNetCore.Mvc;
+public class MyController : Controller
+{
+    [HttpGet]
+    [Route({|#0:""Person""|})]
+    public IActionResult Get() => Ok(""You GET me"");
+    
+    [HttpGet]
+    [Route({|#1:""Person""|})]
+    public IActionResult Put() => Ok(""You PUT me"");
+}
+internal class Program
+{
+    static void Main(string[] args)
+    {
+    }
+}
+";
+
+        var expectedDiagnostics = new[] {
+            new DiagnosticResult(DiagnosticDescriptors.AmbiguousActionRoute).WithArguments("Person").WithLocation(0),
+            new DiagnosticResult(DiagnosticDescriptors.AmbiguousActionRoute).WithArguments("Person").WithLocation(1)
+        };
+
+        // Act & Assert
+        await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics);
+    }
+
+    [Fact]
+    public async Task DuplicateRoutes_RouteAndGetVsGet_HasDiagnostics()
+    {
+        // Arrange
+        var source = @"
+using Microsoft.AspNetCore.Builder;
+using Microsoft.AspNetCore.Mvc;
+public class MyController : Controller
+{
+    [HttpGet({|#0:""Person""|})]
+    public IActionResult Get() => Ok(""You GET me"");
+    
+    [HttpGet]
+    [Route({|#1:""Person""|})]
+    public IActionResult Put() => Ok(""You PUT me"");
+}
+internal class Program
+{
+    static void Main(string[] args)
+    {
+    }
+}
+";
+
+        var expectedDiagnostics = new[] {
+            new DiagnosticResult(DiagnosticDescriptors.AmbiguousActionRoute).WithArguments("Person").WithLocation(0),
+            new DiagnosticResult(DiagnosticDescriptors.AmbiguousActionRoute).WithArguments("Person").WithLocation(1)
+        };
+
+        // Act & Assert
+        await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics);
+    }
 }