diff --git a/src/Framework/App.Ref/src/CompatibilitySuppressions.xml b/src/Framework/App.Ref/src/CompatibilitySuppressions.xml index e2ec12ef3b32..6aae8c381577 100644 --- a/src/Framework/App.Ref/src/CompatibilitySuppressions.xml +++ b/src/Framework/App.Ref/src/CompatibilitySuppressions.xml @@ -4,4 +4,8 @@ PKV004 net7.0 + + PKV004 + net8.0 + \ No newline at end of file diff --git a/src/Framework/App.Runtime/src/CompatibilitySuppressions.xml b/src/Framework/App.Runtime/src/CompatibilitySuppressions.xml index 056469f78b07..61fb1abb7e35 100644 --- a/src/Framework/App.Runtime/src/CompatibilitySuppressions.xml +++ b/src/Framework/App.Runtime/src/CompatibilitySuppressions.xml @@ -4,4 +4,8 @@ PKV0001 net7.0 + + PKV0001 + net8.0 + \ No newline at end of file diff --git a/src/Framework/AspNetCoreAnalyzers/samples/WebAppSample/Program.cs b/src/Framework/AspNetCoreAnalyzers/samples/WebAppSample/Program.cs index e71152eb39a6..869f6812d822 100644 --- a/src/Framework/AspNetCoreAnalyzers/samples/WebAppSample/Program.cs +++ b/src/Framework/AspNetCoreAnalyzers/samples/WebAppSample/Program.cs @@ -17,8 +17,8 @@ }); app.MapGet("/posts/{**rest}", (string rest) => $"Routing to {rest}"); -app.MapGet("/todos/{id}", (int id) => db.Todos.Find(id)); -app.MapGet("/todos/{text}", (string text) => db.Todos.Where(t => t.Text.Contains(text))); +app.MapGet("/todos/{id:int}", (int id) => db.Todos.Find(id)); +app.MapGet("/todos/{text:alpha}", (string text) => db.Todos.Where(t => t.Text.Contains(text))); app.MapGet("/posts/{slug:regex(^[a-z0-9_-]+$)}", (string slug) => { return $"Match slug: {Regex.IsMatch(slug, "[a-z0-9_-]+")}"; diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs index a0284e353f57..3b9acc42744a 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs @@ -178,4 +178,13 @@ internal static class DiagnosticDescriptors DiagnosticSeverity.Error, isEnabledByDefault: true, helpLinkUri: "https://aka.ms/aspnet/analyzers"); + + internal static readonly DiagnosticDescriptor AmbiguousRouteHandlerRoute = new( + "ASP0022", + "Route conflict detected between route handlers", + "Route '{0}' conflicts with another handler route. An HTTP request that matches multiple routes results in an ambiguous match error. Fix the conflict by changing the route's pattern, HTTP method, or route constraints.", + "Usage", + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + helpLinkUri: "https://aka.ms/aspnet/analyzers"); } diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/AmbiguousRoutePatternComparer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/AmbiguousRoutePatternComparer.cs new file mode 100644 index 000000000000..9f9cfb7ba99f --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/AmbiguousRoutePatternComparer.cs @@ -0,0 +1,158 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Linq; +using Microsoft.AspNetCore.Analyzers.Infrastructure.RoutePattern; + +namespace Microsoft.AspNetCore.Analyzers.Infrastructure; + +/// +/// This route pattern comparer checks to see if two route patterns match the same URL and create ambiguous match exceptions at runtime. +/// It doesn't check two routes exactly equal each other. For example, "/product/{id}" and "/product/{name}" aren't exactly equal but will match the same URL. +/// +internal sealed class AmbiguousRoutePatternComparer : IEqualityComparer +{ + public static AmbiguousRoutePatternComparer Instance { get; } = new(); + + public bool Equals(RoutePatternTree x, RoutePatternTree y) + { + if (x.Root.Parts.Length != y.Root.Parts.Length) + { + return false; + } + + for (var i = 0; i < x.Root.Parts.Length; i++) + { + var xPart = x.Root.Parts[i]; + var yPart = y.Root.Parts[i]; + + var equal = xPart switch + { + RoutePatternSegmentSeparatorNode _ => yPart is RoutePatternSegmentSeparatorNode, + RoutePatternSegmentNode xSegment => Equals(xSegment, yPart as RoutePatternSegmentNode), + _ => throw new InvalidOperationException($"Unexpected part type '{xPart.Kind}'."), + }; + + if (!equal) + { + return false; + } + } + + return true; + } + + private static bool Equals(RoutePatternSegmentNode x, RoutePatternSegmentNode? y) + { + if (y is null) + { + return false; + } + + if (x.Children.Length != y.Children.Length) + { + return false; + } + + for (var i = 0; i < x.Children.Length; i++) + { + var xChild = x.Children[i]; + var yChild = y.Children[i]; + + var equal = xChild switch + { + RoutePatternOptionalSeparatorNode _ => yChild is RoutePatternOptionalSeparatorNode, + RoutePatternReplacementNode xReplacement => yChild is RoutePatternReplacementNode yReplacement && IgnoreCaseEquals(xReplacement.TextToken.Value, yReplacement.TextToken.Value), + RoutePatternLiteralNode xLiteral => yChild is RoutePatternLiteralNode yLiteral && IgnoreCaseEquals(xLiteral.LiteralToken.Value, yLiteral.LiteralToken.Value), + RoutePatternParameterNode xParameter => Equals(xParameter, yChild as RoutePatternParameterNode), + _ => throw new InvalidOperationException($"Unexpected segment node type '{xChild.Kind}'."), + }; + + if (!equal) + { + return false; + } + } + + return true; + } + + private static bool IgnoreCaseEquals(object? value1, object? value2) + { + var s1 = value1 as string; + var s2 = value2 as string; + + if (s1 is null || s2 is null) + { + return false; + } + + return string.Equals(s1, s2, StringComparison.OrdinalIgnoreCase); + } + + private static bool Equals(RoutePatternParameterNode x, RoutePatternParameterNode? y) + { + if (y is null) + { + return false; + } + + // Only parameter policies differentiate between parameters. + var xParameterPolicies = x.ParameterParts.Where(p => p.Kind == RoutePatternKind.ParameterPolicy).OfType().ToList(); + var yParameterPolicies = y.ParameterParts.Where(p => p.Kind == RoutePatternKind.ParameterPolicy).OfType().ToList(); + + if (xParameterPolicies.Count != yParameterPolicies.Count) + { + return false; + } + + for (var i = 0; i < xParameterPolicies.Count; i++) + { + var xPolicy = xParameterPolicies[i]; + var yPolicy = yParameterPolicies[i]; + + if (!Equals(xPolicy, yPolicy)) + { + return false; + } + } + + return true; + } + + private static bool Equals(RoutePatternPolicyParameterPartNode x, RoutePatternPolicyParameterPartNode y) + { + if (x.PolicyFragments.Length != y.PolicyFragments.Length) + { + return false; + } + + for (var i = 0; i < x.PolicyFragments.Length; i++) + { + var xPart = x.PolicyFragments[i]; + var yPart = y.PolicyFragments[i]; + + var equal = xPart switch + { + RoutePatternPolicyFragment xFragment => yPart is RoutePatternPolicyFragment yFragment && Equals(xFragment.ArgumentToken.Value, yFragment.ArgumentToken.Value), + RoutePatternPolicyFragmentEscapedNode xFragmentEscaped => yPart is RoutePatternPolicyFragmentEscapedNode yFragmentEscaped && Equals(xFragmentEscaped.ArgumentToken.Value, yFragmentEscaped.ArgumentToken.Value), + _ => throw new InvalidOperationException($"Unexpected policy node type '{xPart.Kind}'."), + }; + + if (!equal) + { + return false; + } + } + + return true; + } + + public int GetHashCode(RoutePatternTree obj) + { + // TODO: Improve hash code calculation. This is rudimentary and will generate a lot of collisions. + return obj.Root.ChildCount; + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/IRoutePatternNodeVisitor.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/IRoutePatternNodeVisitor.cs index 7c72a8993efc..00ea72d130de 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/IRoutePatternNodeVisitor.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/IRoutePatternNodeVisitor.cs @@ -10,8 +10,8 @@ internal interface IRoutePatternNodeVisitor void Visit(RoutePatternReplacementNode node); void Visit(RoutePatternParameterNode node); void Visit(RoutePatternLiteralNode node); - void Visit(RoutePatternSegmentSeperatorNode node); - void Visit(RoutePatternOptionalSeperatorNode node); + void Visit(RoutePatternSegmentSeparatorNode node); + void Visit(RoutePatternOptionalSeparatorNode node); void Visit(RoutePatternCatchAllParameterPartNode node); void Visit(RoutePatternNameParameterPartNode node); void Visit(RoutePatternPolicyParameterPartNode node); diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternKind.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternKind.cs index b63777566b66..13591100e47e 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternKind.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternKind.cs @@ -9,7 +9,7 @@ internal enum RoutePatternKind EndOfFile, Segment, CompilationUnit, - Seperator, + Separator, Literal, Replacement, Parameter, diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternLexer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternLexer.cs index 76252318b5a0..236f16414f29 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternLexer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternLexer.cs @@ -102,7 +102,7 @@ private bool TextAt(int position, string val) if (ch.Value == '/') { - // Literal ends at a seperator or start of a parameter. + // Literal ends at a separator or start of a parameter. break; } else if (ch.Value == '{' && IsUnescapedChar(ref Position, '{')) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternNodes.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternNodes.cs index 0da6431b1d4c..a0aaa2d12174 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternNodes.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternNodes.cs @@ -43,11 +43,11 @@ public override void Accept(IRoutePatternNodeVisitor visitor) internal sealed class RoutePatternSegmentNode : RoutePatternRootPartNode { - public ImmutableArray Children { get; } + public ImmutableArray Children { get; } internal override int ChildCount => Children.Length; - public RoutePatternSegmentNode(ImmutableArray children) + public RoutePatternSegmentNode(ImmutableArray children) : base(RoutePatternKind.Segment) { Children = children; @@ -162,23 +162,23 @@ public override void Accept(IRoutePatternNodeVisitor visitor) => visitor.Visit(this); } -internal sealed class RoutePatternOptionalSeperatorNode : RoutePatternSegmentPartNode +internal sealed class RoutePatternOptionalSeparatorNode : RoutePatternSegmentPartNode { - public RoutePatternOptionalSeperatorNode(RoutePatternToken seperatorToken) - : base(RoutePatternKind.Seperator) + public RoutePatternOptionalSeparatorNode(RoutePatternToken separatorToken) + : base(RoutePatternKind.Separator) { - Debug.Assert(seperatorToken.Kind == RoutePatternKind.DotToken); - SeperatorToken = seperatorToken; + Debug.Assert(separatorToken.Kind == RoutePatternKind.DotToken); + SeparatorToken = separatorToken; } - public RoutePatternToken SeperatorToken { get; } + public RoutePatternToken SeparatorToken { get; } internal override int ChildCount => 1; internal override RoutePatternNodeOrToken ChildAt(int index) => index switch { - 0 => SeperatorToken, + 0 => SeparatorToken, _ => throw new InvalidOperationException(), }; @@ -186,23 +186,23 @@ public override void Accept(IRoutePatternNodeVisitor visitor) => visitor.Visit(this); } -internal sealed class RoutePatternSegmentSeperatorNode : RoutePatternRootPartNode +internal sealed class RoutePatternSegmentSeparatorNode : RoutePatternRootPartNode { - public RoutePatternSegmentSeperatorNode(RoutePatternToken seperatorToken) - : base(RoutePatternKind.Seperator) + public RoutePatternSegmentSeparatorNode(RoutePatternToken separatorToken) + : base(RoutePatternKind.Separator) { - Debug.Assert(seperatorToken.Kind == RoutePatternKind.SlashToken); - SeperatorToken = seperatorToken; + Debug.Assert(separatorToken.Kind == RoutePatternKind.SlashToken); + SeparatorToken = separatorToken; } - public RoutePatternToken SeperatorToken { get; } + public RoutePatternToken SeparatorToken { get; } internal override int ChildCount => 1; internal override RoutePatternNodeOrToken ChildAt(int index) => index switch { - 0 => SeperatorToken, + 0 => SeparatorToken, _ => throw new InvalidOperationException(), }; diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternParser.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternParser.cs index d38516559f2d..7155bd076547 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternParser.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternParser.cs @@ -171,7 +171,7 @@ private static void ValidateStart(RoutePatternCompilationUnit root, IList 2 && root.ChildAt(1).Node is var secondNode && - secondNode?.Kind == RoutePatternKind.Seperator) + secondNode?.Kind == RoutePatternKind.Separator) { return; } @@ -351,18 +351,18 @@ private static void ValidateParameterParts(RoutePatternCompilationUnit root, ILi private static void ValidateNoConsecutiveSeparators(RoutePatternCompilationUnit root, IList diagnostics) { - RoutePatternSegmentSeperatorNode? previousNode = null; + RoutePatternSegmentSeparatorNode? previousNode = null; foreach (var part in root) { - if (part.TryGetNode(RoutePatternKind.Seperator, out var seperatorNode)) + if (part.TryGetNode(RoutePatternKind.Separator, out var separatorNode)) { - var currentNode = (RoutePatternSegmentSeperatorNode)seperatorNode; + var currentNode = (RoutePatternSegmentSeparatorNode)separatorNode; if (previousNode != null) { diagnostics.Add( new EmbeddedDiagnostic( Resources.TemplateRoute_CannotHaveConsecutiveSeparators, - EmbeddedSyntaxHelpers.GetSpan(previousNode.SeperatorToken, currentNode.SeperatorToken))); + EmbeddedSyntaxHelpers.GetSpan(previousNode.SeparatorToken, currentNode.SeparatorToken))); } previousNode = currentNode; } @@ -421,13 +421,13 @@ private ImmutableArray ParseRootParts() private RoutePatternRootPartNode ParseRootPart() => _currentToken.Kind switch { - RoutePatternKind.SlashToken => ParseSegmentSeperator(), + RoutePatternKind.SlashToken => ParseSegmentSeparator(), _ => ParseSegment(), }; private RoutePatternSegmentNode ParseSegment() { - var result = ImmutableArray.CreateBuilder(); + var result = ImmutableArray.CreateBuilder(); while (_currentToken.Kind != RoutePatternKind.EndOfFile && _currentToken.Kind != RoutePatternKind.SlashToken) @@ -686,7 +686,7 @@ private RoutePatternPolicyParameterPartNode ParsePolicy() return new(colonToken, fragments.ToImmutable()); } - private RoutePatternSegmentSeperatorNode ParseSegmentSeperator() + private RoutePatternSegmentSeparatorNode ParseSegmentSeparator() => new(ConsumeCurrentToken()); private TextSpan GetTokenStartPositionSpan(RoutePatternToken token) diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/WellKnownTypes.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/WellKnownTypes.cs index 782eb4ba1ac9..dc249b601074 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/WellKnownTypes.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/WellKnownTypes.cs @@ -56,7 +56,13 @@ internal enum WellKnownType System_Threading_Tasks_ValueTask_T, System_Reflection_ParameterInfo, Microsoft_AspNetCore_Http_IBindableFromHttpContext_T, - System_IParsable_T + System_IParsable_T, + Microsoft_AspNetCore_Builder_AuthorizationEndpointConventionBuilderExtensions, + Microsoft_AspNetCore_Http_OpenApiRouteHandlerBuilderExtensions, + Microsoft_AspNetCore_Builder_CorsEndpointConventionBuilderExtensions, + Microsoft_Extensions_DependencyInjection_OutputCacheConventionBuilderExtensions, + Microsoft_AspNetCore_Builder_RateLimiterEndpointConventionBuilderExtensions, + Microsoft_AspNetCore_Builder_RoutingEndpointConventionBuilderExtensions, } internal sealed class WellKnownTypes @@ -108,7 +114,13 @@ internal sealed class WellKnownTypes "System.Threading.Tasks.ValueTask`1", "System.Reflection.ParameterInfo", "Microsoft.AspNetCore.Http.IBindableFromHttpContext`1", - "System.IParsable`1" + "System.IParsable`1", + "Microsoft.AspNetCore.Builder.AuthorizationEndpointConventionBuilderExtensions", + "Microsoft.AspNetCore.Http.OpenApiRouteHandlerBuilderExtensions", + "Microsoft.AspNetCore.Builder.CorsEndpointConventionBuilderExtensions", + "Microsoft.Extensions.DependencyInjection.OutputCacheConventionBuilderExtensions", + "Microsoft.AspNetCore.Builder.RateLimiterEndpointConventionBuilderExtensions", + "Microsoft.AspNetCore.Builder.RoutingEndpointConventionBuilderExtensions", }; public static WellKnownTypes GetOrCreate(Compilation compilation) => diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/Infrastructure/RouteUsageDetector.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/Infrastructure/RouteUsageDetector.cs index c61319f2e846..090d1012fb64 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/Infrastructure/RouteUsageDetector.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/Infrastructure/RouteUsageDetector.cs @@ -28,11 +28,13 @@ internal record struct ParameterSymbol(ISymbol Symbol, ISymbol? TopLevelSymbol = } internal readonly record struct RouteUsageContext( + SyntaxToken RouteToken, IMethodSymbol? MethodSymbol, SyntaxNode? MethodSyntax, RouteUsageType UsageType, ImmutableArray Parameters, - ImmutableArray ResolvedParameters) + ImmutableArray ResolvedParameters, + ImmutableArray HttpMethods) { public RoutePatternOptions RoutePatternOptions => UsageType switch { @@ -54,11 +56,13 @@ public static RouteUsageContext BuildContext(RouteOptions routeOptions, SyntaxTo if (routeOptions == RouteOptions.Component) { return new( + RouteToken: token, MethodSymbol: null, MethodSyntax: null, UsageType: RouteUsageType.Component, Parameters: ImmutableArray.Empty, - ResolvedParameters: ImmutableArray.Empty); + ResolvedParameters: ImmutableArray.Empty, + HttpMethods: default); } if (token.Parent is not LiteralExpressionSyntax) @@ -91,11 +95,13 @@ public static RouteUsageContext BuildContext(RouteOptions routeOptions, SyntaxTo var parameterSymbols = RoutePatternParametersDetector.GetParameterSymbols(mapMethodSymbol); var resolvedParameterSymbols = RoutePatternParametersDetector.ResolvedParameters(mapMethodSymbol, wellKnownTypes); return new( + RouteToken: token, MethodSymbol: mapMethodSymbol, MethodSyntax: mapMethodParts.Value.DelegateExpression, UsageType: RouteUsageType.MinimalApi, Parameters: parameterSymbols, - ResolvedParameters: resolvedParameterSymbols); + ResolvedParameters: resolvedParameterSymbols, + HttpMethods: CalculateHttpMethods(wellKnownTypes, mapMethodParts.Value.Method)); } else if (container.Parent.IsKind(SyntaxKind.AttributeArgument)) { @@ -113,24 +119,67 @@ public static RouteUsageContext BuildContext(RouteOptions routeOptions, SyntaxTo var parameterSymbols = RoutePatternParametersDetector.GetParameterSymbols(actionMethodSymbol); var resolvedParameterSymbols = RoutePatternParametersDetector.ResolvedParameters(actionMethodSymbol, wellKnownTypes); + + // TODO: Find HttpMethods for MVC actions. return new( + RouteToken: token, MethodSymbol: actionMethodSymbol, MethodSyntax: methodDeclarationSyntax, UsageType: RouteUsageType.MvcAction, Parameters: parameterSymbols, - ResolvedParameters: resolvedParameterSymbols); + ResolvedParameters: resolvedParameterSymbols, + HttpMethods: default); } else if (attributeParent is ClassDeclarationSyntax classDeclarationSyntax) { var classSymbol = semanticModel.GetDeclaredSymbol(classDeclarationSyntax, cancellationToken); var usageType = MvcDetector.IsController(classSymbol, wellKnownTypes) ? RouteUsageType.MvcController : RouteUsageType.Other; return new( + RouteToken: token, MethodSymbol: null, MethodSyntax: null, UsageType: usageType, Parameters: ImmutableArray.Empty, - ResolvedParameters: ImmutableArray.Empty); + ResolvedParameters: ImmutableArray.Empty, + HttpMethods: default); + } + } + + return default; + } + + private static ImmutableArray CalculateHttpMethods(WellKnownTypes wellKnownTypes, IMethodSymbol mapMethodSymbol) + { + if (SymbolEqualityComparer.Default.Equals(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Builder_EndpointRouteBuilderExtensions), mapMethodSymbol.ContainingType)) + { + var httpMethodsBuilder = ImmutableArray.CreateBuilder(); + // TODO: Support MapMethods. + switch (mapMethodSymbol.Name) + { + case "MapGet": + httpMethodsBuilder.Add("GET"); + break; + case "MapPost": + httpMethodsBuilder.Add("POST"); + break; + case "MapPut": + httpMethodsBuilder.Add("PUT"); + break; + case "MapDelete": + httpMethodsBuilder.Add("DELETE"); + break; + case "MapPatch": + httpMethodsBuilder.Add("PATCH"); + break; + case "Map": + // No HTTP methods. + break; + default: + // Unknown/unsupported method. + return default; } + + return httpMethodsBuilder.ToImmutable(); } return default; diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/RoutePatternClassifier.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/RoutePatternClassifier.cs index 932188453957..2d83b04a2053 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/RoutePatternClassifier.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/RoutePatternClassifier.cs @@ -77,12 +77,12 @@ public void Visit(RoutePatternLiteralNode node) // Nothing to highlight. } - public void Visit(RoutePatternSegmentSeperatorNode node) + public void Visit(RoutePatternSegmentSeparatorNode node) { // Nothing to highlight. } - public void Visit(RoutePatternOptionalSeperatorNode node) + public void Visit(RoutePatternOptionalSeparatorNode node) { // Nothing to highlight. } diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/DetectAmbiguousRoutes.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/DetectAmbiguousRoutes.cs new file mode 100644 index 000000000000..f76cf3fedd0d --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/DetectAmbiguousRoutes.cs @@ -0,0 +1,204 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Concurrent; +using System.Collections.Immutable; +using System.Diagnostics; +using System.Linq; +using Microsoft.AspNetCore.Analyzers.Infrastructure; +using Microsoft.AspNetCore.Analyzers.Infrastructure.RoutePattern; +using Microsoft.AspNetCore.App.Analyzers.Infrastructure; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace Microsoft.AspNetCore.Analyzers.RouteHandlers; + +public partial class RouteHandlerAnalyzer : DiagnosticAnalyzer +{ + private static void DetectAmbiguousRoutes(in OperationBlockAnalysisContext context, WellKnownTypes wellKnownTypes, ConcurrentDictionary mapOperations) + { + if (mapOperations.IsEmpty) + { + return; + } + + var groupedByParent = mapOperations + .Select(kvp => new { MapOperation = kvp.Key, ResolvedOperation = ResolveOperation(kvp.Key.Operation, wellKnownTypes) }) + .Where(u => u.ResolvedOperation != null && !u.MapOperation.RouteUsageModel.UsageContext.HttpMethods.IsDefault) + .GroupBy(u => new MapOperationGroupKey(u.MapOperation.Builder, u.ResolvedOperation!, u.MapOperation.RouteUsageModel.RoutePattern, u.MapOperation.RouteUsageModel.UsageContext.HttpMethods)); + + foreach (var ambigiousGroup in groupedByParent.Where(g => g.Count() >= 2)) + { + foreach (var ambigiousMapOperation in ambigiousGroup) + { + var model = ambigiousMapOperation.MapOperation.RouteUsageModel; + + context.ReportDiagnostic(Diagnostic.Create( + DiagnosticDescriptors.AmbiguousRouteHandlerRoute, + model.UsageContext.RouteToken.GetLocation(), + model.RoutePattern.Root.ToString())); + } + } + } + + private static IOperation? ResolveOperation(IOperation operation, WellKnownTypes wellKnownTypes) + { + // We want to group routes in a block together because we know they're being used together. + // There are some circumstances where we still don't want to use the route, either because it is only conditionally + // being called, or the IEndpointConventionBuilder returned from the method is being used. We can't accurately + // detect what extra endpoint metadata is being added to the routes. + // + // Don't use route endpoint if: + // - It's in a conditional statement. + // - It's in a coalesce statement. + // - It's has methods called on it. + // - It's assigned to a variable. + // - It's an argument to a method call, unless in a known safe method. + var current = operation; + if (current.Parent is IArgumentOperation { Parent: IInvocationOperation invocationOperation } && + IsAllowedEndpointBuilderMethod(invocationOperation, wellKnownTypes)) + { + return ResolveOperation(invocationOperation, wellKnownTypes); + } + + while (current != null) + { + if (current.Parent is IBlockOperation blockOperation) + { + return blockOperation; + } + else if (current.Parent is IConditionalOperation or + ICoalesceOperation or + IAssignmentOperation or + IArgumentOperation or + IInvocationOperation) + { + return current; + } + + current = current.Parent; + } + + return null; + } + + /// + /// Test the invocation operation. Safe methods are those that we know don't add metadata that impacts metadata. + /// + private static bool IsAllowedEndpointBuilderMethod(IInvocationOperation invocationOperation, WellKnownTypes wellKnownTypes) + { + var method = invocationOperation.TargetMethod; + + if (SymbolEqualityComparer.Default.Equals(method.ContainingType, wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Builder_RoutingEndpointConventionBuilderExtensions))) + { + return method.Name switch + { + "RequireHost" => false, // Adds IHostMetadata + "WithDisplayName" => true, + "WithMetadata" => false, // Can add anything + "WithName" => true, + "WithGroupName" => true, + _ => false + }; + } + else if (SymbolEqualityComparer.Default.Equals(method.ContainingType, wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Builder_AuthorizationEndpointConventionBuilderExtensions))) + { + return method.Name is "RequireAuthorization" or "AllowAnonymous"; + } + else if (SymbolEqualityComparer.Default.Equals(method.ContainingType, wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_OpenApiRouteHandlerBuilderExtensions))) + { + return method.Name switch + { + "Accepts" => false, // Adds IAcceptsMetadata + "ExcludeFromDescription" => true, + "Produces" => true, + "ProducesProblem" => true, + "ProducesValidationProblem" => true, + "WithDescription" => true, + "WithSummary" => true, + "WithTags" => true, + _ => false + }; + } + else if (SymbolEqualityComparer.Default.Equals(method.ContainingType, wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Builder_CorsEndpointConventionBuilderExtensions))) + { + return method.Name == "RequireCors"; + } + else if (SymbolEqualityComparer.Default.Equals(method.ContainingType, wellKnownTypes.Get(WellKnownType.Microsoft_Extensions_DependencyInjection_OutputCacheConventionBuilderExtensions))) + { + return method.Name == "CacheOutput"; + } + else if (SymbolEqualityComparer.Default.Equals(method.ContainingType, wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Builder_RateLimiterEndpointConventionBuilderExtensions))) + { + return method.Name is "RequireRateLimiting" or "DisableRateLimiting"; + } + + return false; + } + + private readonly struct MapOperationGroupKey : IEquatable + { + public IOperation? ParentOperation { get; } + public IOperation? Builder { get; } + public RoutePatternTree RoutePattern { get; } + public ImmutableArray HttpMethods { get; } + + public MapOperationGroupKey(IOperation? builder, IOperation parentOperation, RoutePatternTree routePattern, ImmutableArray httpMethods) + { + Debug.Assert(!httpMethods.IsDefault); + + ParentOperation = parentOperation; + Builder = builder; + RoutePattern = routePattern; + HttpMethods = httpMethods; + } + + public override bool Equals(object obj) + { + if (obj is MapOperationGroupKey key) + { + return Equals(key); + } + return false; + } + + public bool Equals(MapOperationGroupKey other) + { + return + ParentOperation != null && + Equals(ParentOperation, other.ParentOperation) && + Builder != null && + SymbolEqualityComparer.Default.Equals((Builder as ILocalReferenceOperation)?.Local, (other.Builder as ILocalReferenceOperation)?.Local) && + AmbiguousRoutePatternComparer.Instance.Equals(RoutePattern, other.RoutePattern) && + HasMatchingHttpMethods(HttpMethods, other.HttpMethods); + } + + private static bool HasMatchingHttpMethods(ImmutableArray httpMethods1, ImmutableArray httpMethods2) + { + if (httpMethods1.IsEmpty || httpMethods2.IsEmpty) + { + return true; + } + + foreach (var item1 in httpMethods1) + { + foreach (var item2 in httpMethods2) + { + if (item2 == item1) + { + return true; + } + } + } + + return false; + } + + public override int GetHashCode() + { + return (ParentOperation?.GetHashCode() ?? 0) ^ AmbiguousRoutePatternComparer.Instance.GetHashCode(RoutePattern); + } + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/RouteHandlerAnalyzer.cs b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/RouteHandlerAnalyzer.cs index 58d8fb8d621a..a763446ee7b0 100644 --- a/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/RouteHandlerAnalyzer.cs +++ b/src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/RouteHandlerAnalyzer.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Collections.Concurrent; using System.Collections.Immutable; using System.Linq; using Microsoft.AspNetCore.App.Analyzers.Infrastructure; @@ -16,13 +17,15 @@ namespace Microsoft.AspNetCore.Analyzers.RouteHandlers; public partial class RouteHandlerAnalyzer : DiagnosticAnalyzer { private const int DelegateParameterOrdinal = 2; + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create( DiagnosticDescriptors.DoNotUseModelBindingAttributesOnRouteHandlerParameters, DiagnosticDescriptors.DoNotReturnActionResultsFromRouteHandlers, DiagnosticDescriptors.DetectMisplacedLambdaAttribute, DiagnosticDescriptors.DetectMismatchedParameterOptionality, DiagnosticDescriptors.RouteParameterComplexTypeIsNotParsableOrBindable, - DiagnosticDescriptors.BindAsyncSignatureMustReturnValueTaskOfT + DiagnosticDescriptors.BindAsyncSignatureMustReturnValueTaskOfT, + DiagnosticDescriptors.AmbiguousRouteHandlerRoute ); public override void Initialize(AnalysisContext context) @@ -36,7 +39,30 @@ public override void Initialize(AnalysisContext context) var wellKnownTypes = WellKnownTypes.GetOrCreate(compilation); var routeUsageCache = RouteUsageCache.GetOrCreate(compilation); - context.RegisterOperationAction(context => + // We want ConcurrentHashSet here in case RegisterOperationAction runs in parallel. + // Since ConcurrentHashSet doesn't exist, use ConcurrentDictionary and ignore the value. + var concurrentQueue = new ConcurrentQueue>(); + context.RegisterOperationBlockStartAction(context => + { + // Pool and reuse lists for each block. + if (!concurrentQueue.TryDequeue(out var mapOperations)) + { + mapOperations = new ConcurrentDictionary(); + } + + context.RegisterOperationAction(c => DoOperationAnalysis(c, mapOperations), OperationKind.Invocation); + + context.RegisterOperationBlockEndAction(c => + { + DetectAmbiguousRoutes(c, wellKnownTypes, mapOperations); + + // Return to the pool. + mapOperations.Clear(); + concurrentQueue.Enqueue(mapOperations); + }); + }); + + void DoOperationAnalysis(OperationAnalysisContext context, ConcurrentDictionary mapOperations) { var invocation = (IInvocationOperation)context.Operation; var targetMethod = invocation.TargetMethod; @@ -71,6 +97,8 @@ public override void Initialize(AnalysisContext context) return; } + mapOperations.TryAdd(MapOperation.Create(invocation, routeUsage), value: default); + if (delegateCreation.Target.Kind == OperationKind.AnonymousFunction) { var lambda = (IAnonymousFunctionOperation)delegateCreation.Target; @@ -133,7 +161,7 @@ public override void Initialize(AnalysisContext context) } } - }, OperationKind.Invocation); + } }); } @@ -168,6 +196,46 @@ private static bool IsRouteHandlerInvocation( SymbolEqualityComparer.Default.Equals(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Builder_EndpointRouteBuilderExtensions), targetMethod.ContainingType) && invocation.Arguments.Length == 3 && targetMethod.Parameters.Length == 3 && - SymbolEqualityComparer.Default.Equals(wellKnownTypes.Get(WellKnownType.System_Delegate), targetMethod.Parameters[DelegateParameterOrdinal].Type); + IsCompatibleDelegateType(wellKnownTypes, targetMethod); + + static bool IsCompatibleDelegateType(WellKnownTypes wellKnownTypes, IMethodSymbol targetMethod) + { + var parmeterType = targetMethod.Parameters[DelegateParameterOrdinal].Type; + if (SymbolEqualityComparer.Default.Equals(wellKnownTypes.Get(WellKnownType.System_Delegate), parmeterType)) + { + return true; + } + if (SymbolEqualityComparer.Default.Equals(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_RequestDelegate), parmeterType)) + { + return true; + } + return false; + } + } + + private record struct MapOperation(IOperation? Builder, IInvocationOperation Operation, RouteUsageModel RouteUsageModel) + { + public static MapOperation Create(IInvocationOperation operation, RouteUsageModel routeUsageModel) + { + IOperation? builder = null; + + var builderArgument = operation.Arguments.SingleOrDefault(a => a.Parameter?.Ordinal == 0); + if (builderArgument != null) + { + builder = WalkDownConversion(builderArgument.Value); + } + + return new MapOperation(builder, operation, routeUsageModel); + } + + private static IOperation WalkDownConversion(IOperation operation) + { + while (operation is IConversionOperation conversionOperation) + { + operation = conversionOperation.Operand; + } + + return operation; + } } } diff --git a/src/Framework/AspNetCoreAnalyzers/test/Infrastructure/AmbiguousRoutePatternComparerTests.cs b/src/Framework/AspNetCoreAnalyzers/test/Infrastructure/AmbiguousRoutePatternComparerTests.cs new file mode 100644 index 000000000000..37edfaa53477 --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/test/Infrastructure/AmbiguousRoutePatternComparerTests.cs @@ -0,0 +1,103 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.AspNetCore.Analyzers.Infrastructure.RoutePattern; +using Microsoft.AspNetCore.Analyzers.Infrastructure.VirtualChars; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; + +namespace Microsoft.AspNetCore.Analyzers.Infrastructure; + +public partial class AmbiguousRoutePatternComparerTests +{ + [Fact] + public void Equals_RootMatching_True() + { + // Arrange + var route1 = ParseRoutePattern(@"""/"""); + var route2 = ParseRoutePattern(@"""/"""); + + // Act + var result = AmbiguousRoutePatternComparer.Instance.Equals(route1, route2); + + // Assert + Assert.True(result); + } + + [Theory] + [InlineData(@"""/a""", @"""/a""")] + [InlineData(@"""/a""", @"""/A""")] + [InlineData(@"""/{a}""", @"""/{b}""")] + [InlineData(@"""/{a}/{b}""", @"""/{b}/{c}""")] + [InlineData(@"""/{a:int}""", @"""/{b:int}""")] + [InlineData(@"""/{a:min(5)}""", @"""/{b:min(5)}""")] + [InlineData(@"""/{a}""", @"""/{a?}""")] + [InlineData(@"""/{a}""", @"""/{*a}""")] + [InlineData(@"""/{a}""", @"""/{**a}""")] + [InlineData(@"""/{a}""", @"""/{a=default}""")] + [InlineData(@"""/[controller]""", @"""/[controller]""")] + [InlineData(@"""/[controller]""", @"""/[CONTROLLER]""")] + public void Equals_Equivalent_True(string pattern1, string pattern2) + { + // Arrange + var route1 = ParseRoutePattern(pattern1); + var route2 = ParseRoutePattern(pattern2); + + // Act + var hashCode1 = AmbiguousRoutePatternComparer.Instance.GetHashCode(route1); + var hashCode2 = AmbiguousRoutePatternComparer.Instance.GetHashCode(route2); + var result = AmbiguousRoutePatternComparer.Instance.Equals(route1, route2); + + // Assert + Assert.True(result); + Assert.Equal(hashCode1, hashCode2); + } + + [Theory] + [InlineData(@"""/a""", @"""/""")] + [InlineData(@"""/{a}/b""", @"""/{b}/c""")] + [InlineData(@"""/{a:int}""", @"""/{b:long}""")] + [InlineData(@"""/{a:min(5)}""", @"""/{a:min(6)}""")] + [InlineData(@"""/[controller]""", @"""/[controller1]""")] + [InlineData(@"""/{a:min(5)}""", @"""/{a:MIN(5)}""")] + [InlineData(@"""/{a:regex(abc)}""", @"""/{a:regex(ABC)}""")] + public void Equals_NotEquivalent_False(string pattern1, string pattern2) + { + // Arrange + var route1 = ParseRoutePattern(pattern1); + var route2 = ParseRoutePattern(pattern2); + + // Act + var result = AmbiguousRoutePatternComparer.Instance.Equals(route1, route2); + + // Assert + Assert.False(result); + } + + private static SyntaxToken GetStringToken(string text) + { + const string statmentPrefix = "var v = "; + var statement = statmentPrefix + text; + var parsedStatement = SyntaxFactory.ParseStatement(statement); + var token = parsedStatement.DescendantTokens().ToArray()[3]; + Assert.True(token.IsKind(SyntaxKind.StringLiteralToken)); + return token; + } + + private static RoutePatternTree ParseRoutePattern(string text) + { + var token = GetStringToken(text); + var allChars = CSharpVirtualCharService.Instance.TryConvertToVirtualChars(token); + if (allChars.IsDefault) + { + Assert.Fail("Failed to convert text to token."); + } + + var tree = RoutePatternParser.TryParse(allChars, RoutePatternOptions.MvcAttributeRoute); + if (tree is null) + { + Assert.Fail("Failed to parse virtual chars to route pattern."); + } + return tree!; + } +} diff --git a/src/Framework/AspNetCoreAnalyzers/test/Microsoft.AspNetCore.App.Analyzers.Test.csproj b/src/Framework/AspNetCoreAnalyzers/test/Microsoft.AspNetCore.App.Analyzers.Test.csproj index cbd40c77a4d2..02dc9b4e3d49 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Microsoft.AspNetCore.App.Analyzers.Test.csproj +++ b/src/Framework/AspNetCoreAnalyzers/test/Microsoft.AspNetCore.App.Analyzers.Test.csproj @@ -19,6 +19,7 @@ + diff --git a/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_BasicTests.cs b/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_BasicTests.cs index 9fcc0488ce6b..829cac62dd8c 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_BasicTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_BasicTests.cs @@ -64,9 +64,9 @@ public void TestSlashSeperatedLiterals() hello - + / - + world @@ -92,9 +92,9 @@ public void TestDuplicateParameterNames() } - + / - + { @@ -129,9 +129,9 @@ public void TestSlashSeperatedSegments() } - + / - + { @@ -167,9 +167,9 @@ public void TestCatchAllParameterFollowedBySlash() } - + / - + @@ -195,9 +195,9 @@ public void TestCatchAllParameterNotLast() } - + / - + { @@ -1438,9 +1438,9 @@ public void TestTildeSlash() ~ - + / - + diff --git a/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ComponentsTests.cs b/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ComponentsTests.cs index 93f582d82375..4f15b0e3c242 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ComponentsTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ComponentsTests.cs @@ -27,9 +27,9 @@ public void Parse_MultipleOptionalParameters() } - + / - + { @@ -42,9 +42,9 @@ public void Parse_MultipleOptionalParameters() } - + / - + { @@ -102,17 +102,17 @@ public void Parse_MixedLiteralAndCatchAllParameter() awesome - + / - + wow - + / - + { @@ -143,9 +143,9 @@ public void Parse_MixedLiteralParameterAndCatchAllParameter() awesome - + / - + { @@ -155,9 +155,9 @@ public void Parse_MixedLiteralParameterAndCatchAllParameter() } - + / - + { @@ -197,17 +197,17 @@ public void InvalidTemplate_LiteralAfterOptionalParam() { Test(@"""/test/{a?}/test""", @" - + / - + test - + / - + { @@ -220,9 +220,9 @@ public void InvalidTemplate_LiteralAfterOptionalParam() } - + / - + test @@ -241,17 +241,17 @@ public void InvalidTemplate_NonOptionalParamAfterOptionalParam() { Test(@"""/test/{a?}/{b}""", @" - + / - + test - + / - + { @@ -264,9 +264,9 @@ public void InvalidTemplate_NonOptionalParamAfterOptionalParam() } - + / - + { @@ -290,17 +290,17 @@ public void InvalidTemplate_CatchAllParamWithMultipleAsterisks() { Test(@"""/test/{a}/{**b}""", @" - + / - + test - + / - + { @@ -310,9 +310,9 @@ public void InvalidTemplate_CatchAllParamWithMultipleAsterisks() } - + / - + { @@ -342,17 +342,17 @@ public void InvalidTemplate_CatchAllParamNotLast() { Test(@"""/test/{*a}/{b}""", @" - + / - + test - + / - + { @@ -365,9 +365,9 @@ public void InvalidTemplate_CatchAllParamNotLast() } - + / - + { @@ -394,17 +394,17 @@ public void InvalidTemplate_BadOptionalCharacterPosition() { Test(@"""/test/{a?bc}/{b}""", @" - + / - + test - + / - + { @@ -414,9 +414,9 @@ public void InvalidTemplate_BadOptionalCharacterPosition() } - + / - + { diff --git a/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ConformanceTests.cs b/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ConformanceTests.cs index eaa95e8d3354..698723b5dd5b 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ConformanceTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ConformanceTests.cs @@ -81,17 +81,17 @@ public void Parse_MultipleLiterals() cool - + / - + awesome - + / - + super @@ -117,9 +117,9 @@ public void Parse_MultipleParameters() } - + / - + { @@ -129,9 +129,9 @@ public void Parse_MultipleParameters() } - + / - + { @@ -452,9 +452,9 @@ public void Parse_ComplexSegment_OptionalParameterFollowingPeriod_MiddleSegment( } - + / - + { @@ -488,9 +488,9 @@ public void Parse_ComplexSegment_OptionalParameterFollowingPeriod_LastSegment() } - + / - + { @@ -537,9 +537,9 @@ public void Parse_ComplexSegment_OptionalParameterFollowingPeriod_PeriodAfterSla } - + / - + . diff --git a/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ReplacementTests.cs b/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ReplacementTests.cs index 7d17a613bf7a..c19c7e7b0131 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ReplacementTests.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/RouteEmbeddedLanguage/RoutePatternParserTests_ReplacementTests.cs @@ -161,9 +161,9 @@ public void TestMultipleReplacements() ] - + / - + [ @@ -229,9 +229,9 @@ public void TestMultipleTokenEscapes() [[-]][[ - + / - + [[controller]] @@ -255,9 +255,9 @@ public void TestReplacementContainingEscapedBackets() ] - + / - + [ @@ -283,9 +283,9 @@ public void TestReplacementContainingBraces() ] - + / - + [ @@ -293,9 +293,9 @@ public void TestReplacementContainingBraces() ] - + / - + { @@ -325,9 +325,9 @@ public void TestReplacementInEscapedBrackets() ] - + / - + [[ @@ -341,9 +341,9 @@ public void TestReplacementInEscapedBrackets() ]] - + / - + id @@ -367,9 +367,9 @@ public void TestReplacementInEscapedBrackets2() ] - + / - + [[[[ @@ -383,9 +383,9 @@ public void TestReplacementInEscapedBrackets2() ]]]] - + / - + id @@ -409,9 +409,9 @@ public void TestReplacementInEscapedBrackets3() ] - + / - + [[[[[[ @@ -425,9 +425,9 @@ public void TestReplacementInEscapedBrackets3() ]]]]]] - + / - + id @@ -451,9 +451,9 @@ public void TestReplacementInEscapedBrackets4() ] - + / - + [[[[ @@ -467,9 +467,9 @@ public void TestReplacementInEscapedBrackets4() ]]]]]] - + / - + id diff --git a/src/Framework/AspNetCoreAnalyzers/test/RouteHandlers/DetectAmbiguousMappedRoutesTest.cs b/src/Framework/AspNetCoreAnalyzers/test/RouteHandlers/DetectAmbiguousMappedRoutesTest.cs new file mode 100644 index 000000000000..79e45d504b5c --- /dev/null +++ b/src/Framework/AspNetCoreAnalyzers/test/RouteHandlers/DetectAmbiguousMappedRoutesTest.cs @@ -0,0 +1,346 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.CodeAnalysis.Testing; +using VerifyCS = Microsoft.AspNetCore.Analyzers.Verifiers.CSharpAnalyzerVerifier; + +namespace Microsoft.AspNetCore.Analyzers.RouteHandlers; + +public partial class DetectAmbiguousMappedRoutesTest +{ + [Fact] + public async Task DuplicateRoutes_SameHttpMethod_HasDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +app.MapGet({|#0:""/""|}, () => Hello()); +app.MapGet({|#1:""/""|}, () => Hello()); +void Hello() { } +"; + + var expectedDiagnostics = new[] { + new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0), + new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1) + }; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics); + } + + [Fact] + public async Task DuplicateRoutes_SameHttpMethod_HasRequestDelegate_HasDiagnostics() + { + // Arrange + var source = @" +using System.Threading.Tasks; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +var app = WebApplication.Create(); +app.MapGet({|#0:""/""|}, () => Hello()); +app.MapGet({|#1:""/""|}, (HttpContext context) => Task.CompletedTask); +void Hello() { } +"; + + var expectedDiagnostics = new[] { + new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0), + new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1) + }; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics); + } + + [Fact] + public async Task DuplicateRoutes_SameHttpMethod_InMethod_HasDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Routing; +var app = WebApplication.Create(); +void RegisterEndpoints(IEndpointRouteBuilder builder) +{ + builder.MapGet({|#0:""/""|}, () => Hello()); + builder.MapGet({|#1:""/""|}, () => Hello()); +} + +RegisterEndpoints(app); + +void Hello() { } +"; + + var expectedDiagnostics = new[] { + new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0), + new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1) + }; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics); + } + + [Fact] + public async Task DuplicateRoutes_TernaryStatement_NoDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +_ = (true) + ? app.MapGet(""/"", () => Hello()) + : app.MapGet(""/"", () => Hello()); +void Hello() { } +"; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task DuplicateRoutes_NullCoalescing_NoDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +_ = app.MapGet(""/"", () => Hello()) ?? app.MapGet(""/"", () => Hello()); +void Hello() { } +"; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task DuplicateRoutes_NullCoalescingAssignment_NoDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +var ep = app.MapPost(""/"", () => Hello()); +ep ??= app.MapPost(""/"", () => Hello()); +void Hello() { } +"; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task DuplicateRoutes_DifferentMethods_HasDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +app.MapGet(""/"", () => Hello()); +app.MapPost(""/"", () => Hello()); +void Hello() { } +"; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task DuplicateMapGetRoutes_InsideConditional_NoDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +if (true) +{ + app.MapGet(""/"", () => Hello()); +} +else +{ + app.MapGet(""/"", () => Hello()); +} +void Hello() { } +"; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task DuplicateRoutes_UnknownUsageOfEndConventionBuilderExtension_NoDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +app.MapGet(""/"", () => Hello()).DoSomething(); +app.MapGet(""/"", () => Hello()); +void Hello() { } + +internal static class Extensions +{ + public static void DoSomething(this IEndpointConventionBuilder builder) + { + builder.WithMetadata(new object()); + } +} +"; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task DuplicateRoutes_UnknownUsageOfEndConventionBuilder_NoDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +Extensions.DoSomething(app.MapGet(""/"", () => Hello())); +app.MapGet(""/"", () => Hello()); +void Hello() { } + +internal static class Extensions +{ + public static void DoSomething(IEndpointConventionBuilder builder) + { + builder.WithMetadata(new object()); + } +} +"; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task DuplicateRoutes_AddMethod_NoDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +app.MapGet(""/"", () => Hello()).Add(b => {}); +app.MapGet(""/"", () => Hello()); +void Hello() { } +"; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task DuplicateRoutes_AssignedToVariable_NoDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +_ = app.MapGet(""/"", () => Hello()); +app.MapGet(""/"", () => Hello()); +void Hello() { } +"; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task DuplicateRoutes_MultipleGroups_NoDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +var group1 = app.MapGroup(""/group1""); +var group2 = app.MapGroup(""/group2""); +group1.MapGet(""/"", () => Hello()); +group2.MapGet(""/"", () => Hello()); +void Hello() { } +"; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source); + } + + [Fact] + public async Task DuplicateRoutes_EndpointsOnGroup_HasDiagnostics() + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +var app = WebApplication.Create(); +var group1 = app.MapGroup(""/group1""); +group1.MapGet({|#0:""/""|}, () => Hello()); +group1.MapGet({|#1:""/""|}, () => Hello()); +var group2 = app.MapGroup(""/group2""); +group2.MapGet(""/"", () => Hello()); +void Hello() { } +"; + + // Act & Assert + var expectedDiagnostics = new[] { + new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0), + new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1) + }; + + await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics); + } + + [Theory] + [InlineData(@"RequireAuthorization()")] + [InlineData(@"AllowAnonymous()")] + [InlineData(@"Produces(statusCode:420)")] + [InlineData(@"WithDisplayName(""test!"")")] + [InlineData(@"WithName(""test!"")")] + [InlineData(@"RequireCors(""test!"")")] + [InlineData(@"CacheOutput(""test!"")")] + [InlineData(@"DisableRateLimiting()")] + [InlineData(@"RequireAuthorization().DisableRateLimiting()")] + public async Task DuplicateRoutes_AllowedBuilderExtensionMethods_HasDiagnostics(string method) + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; +var app = WebApplication.Create(); +app.MapGet({|#0:""/""|}, () => Hello())." + method + @"; +app.MapGet({|#1:""/""|}, () => Hello()); +void Hello() { } +"; + + var expectedDiagnostics = new[] { + new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0), + new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1) + }; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics); + } + + [Theory] + [InlineData(@"RequireHost(""test!"")")] + [InlineData(@"RequireHost(""test!"").DisableRateLimiting()")] + [InlineData(@"RequireAuthorization().RequireHost(""test!"")")] + public async Task DuplicateRoutes_UnknownBuilderExtensionMethods_NoDiagnostics(string method) + { + // Arrange + var source = @" +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing; +using Microsoft.Extensions.DependencyInjection; +var app = WebApplication.Create(); +app.MapGet({|#0:""/""|}, () => Hello())." + method + @"; +app.MapGet({|#1:""/""|}, () => Hello()); +void Hello() { } +"; + + // Act & Assert + await VerifyCS.VerifyAnalyzerAsync(source); + } +} + diff --git a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs index caac468fa0aa..374b7a59e5c7 100644 --- a/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs +++ b/src/Framework/AspNetCoreAnalyzers/test/Verifiers/CSharpAnalyzerVerifier.cs @@ -2,12 +2,14 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Immutable; +using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Testing; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp.Testing; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Testing; using Microsoft.CodeAnalysis.Testing.Verifiers; +using Microsoft.Extensions.DependencyInjection; namespace Microsoft.AspNetCore.Analyzers.Verifiers; @@ -60,6 +62,7 @@ internal static ReferenceAssemblies GetReferenceAssemblies() return net8Ref.AddAssemblies(ImmutableArray.Create( TrimAssemblyExtension(typeof(System.IO.Pipelines.PipeReader).Assembly.Location), + TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Authorization.IAuthorizeData).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Mvc.ModelBinding.IBinderTypeProviderMetadata).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Mvc.BindAttribute).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Hosting.WebHostBuilderExtensions).Assembly.Location), @@ -68,8 +71,11 @@ internal static ReferenceAssemblies GetReferenceAssemblies() TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Builder.ConfigureHostBuilder).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Builder.ConfigureWebHostBuilder).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Builder.EndpointRoutingApplicationBuilderExtensions).Assembly.Location), - TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Builder.EndpointRoutingApplicationBuilderExtensions).Assembly.Location), + TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Builder.RateLimiterEndpointConventionBuilderExtensions).Assembly.Location), + TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Builder.CorsEndpointConventionBuilderExtensions).Assembly.Location), + TrimAssemblyExtension(typeof(Microsoft.Extensions.DependencyInjection.OutputCacheConventionBuilderExtensions).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Builder.EndpointRouteBuilderExtensions).Assembly.Location), + TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Builder.AuthorizationEndpointConventionBuilderExtensions).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Routing.RouteData).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Components.ComponentBase).Assembly.Location), TrimAssemblyExtension(typeof(Microsoft.AspNetCore.Components.ParameterAttribute).Assembly.Location), diff --git a/src/Framework/Framework.slnf b/src/Framework/Framework.slnf index bcb23690e91a..34d7ee1c34ba 100644 --- a/src/Framework/Framework.slnf +++ b/src/Framework/Framework.slnf @@ -60,6 +60,7 @@ "src\\Middleware\\Localization.Routing\\src\\Microsoft.AspNetCore.Localization.Routing.csproj", "src\\Middleware\\Localization\\src\\Microsoft.AspNetCore.Localization.csproj", "src\\Middleware\\OutputCaching\\src\\Microsoft.AspNetCore.OutputCaching.csproj", + "src\\Middleware\\RateLimiting\\src\\Microsoft.AspNetCore.RateLimiting.csproj", "src\\Middleware\\ResponseCaching.Abstractions\\src\\Microsoft.AspNetCore.ResponseCaching.Abstractions.csproj", "src\\Middleware\\ResponseCaching\\src\\Microsoft.AspNetCore.ResponseCaching.csproj", "src\\Middleware\\ResponseCompression\\src\\Microsoft.AspNetCore.ResponseCompression.csproj",