Skip to content

Complain if a route parameter is not used or cannot be bound. #35732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions eng/Dependencies.props
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ and are generated based on the last package release.
<LatestPackageReference Include="Serilog.Extensions.Logging" />
<LatestPackageReference Include="Serilog.Sinks.File" />
<LatestPackageReference Include="StackExchange.Redis" />
<LatestPackageReference Include="System.Memory" />
<LatestPackageReference Include="System.Reactive.Linq" />
<LatestPackageReference Include="xunit.abstractions" />
<LatestPackageReference Include="xunit.analyzers" />
Expand Down
1 change: 1 addition & 0 deletions eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@
<SerilogExtensionsLoggingVersion>1.4.0</SerilogExtensionsLoggingVersion>
<SerilogSinksFileVersion>4.0.0</SerilogSinksFileVersion>
<StackExchangeRedisVersion>2.2.4</StackExchangeRedisVersion>
<SystemMemoryVersion>4.5.4</SystemMemoryVersion>
<SystemReactiveLinqVersion>3.1.1</SystemReactiveLinqVersion>
<SwashbuckleAspNetCoreVersion>6.1.5</SwashbuckleAspNetCoreVersion>
<XunitAbstractionsVersion>2.0.3</XunitAbstractionsVersion>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ public partial class DelegateEndpointAnalyzer : DiagnosticAnalyzer
{
DiagnosticDescriptors.DoNotUseModelBindingAttributesOnDelegateEndpointParameters,
DiagnosticDescriptors.DoNotReturnActionResultsFromMapActions,
DiagnosticDescriptors.RouteValueIsUnused,
DiagnosticDescriptors.RouteParameterCannotBeBound,
});

public override void Initialize(AnalysisContext context)
Expand Down Expand Up @@ -54,6 +56,7 @@ public override void Initialize(AnalysisContext context)
var lambda = ((IAnonymousFunctionOperation)delegateCreation.Target);
DisallowMvcBindArgumentsOnParameters(in operationAnalysisContext, wellKnownTypes, invocation, lambda.Symbol);
DisallowReturningActionResultFromMapMethods(in operationAnalysisContext, wellKnownTypes, invocation, lambda);
RouteAttributeMismatch(in operationAnalysisContext, wellKnownTypes, invocation, lambda.Symbol);
}
else if (delegateCreation.Target.Kind == OperationKind.MethodReference)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,23 @@ internal static class DiagnosticDescriptors
DiagnosticSeverity.Warning,
isEnabledByDefault: true,
helpLinkUri: "https://aka.ms/aspnet/analyzers");

internal static readonly DiagnosticDescriptor RouteValueIsUnused = new(
"ASP0005",
"Route value is unused",
"The route value '{0}' does not get bound and can be removed",
"Usage",
DiagnosticSeverity.Warning,
isEnabledByDefault: true,
helpLinkUri: "https://aka.ms/minimal-action/analyzer");

internal static readonly DiagnosticDescriptor RouteParameterCannotBeBound = new(
"ASP0006",
"Route parameter is not bound",
"Route parameter does not have a corresponding route token and cannot be bound",
"Usage",
DiagnosticSeverity.Warning,
isEnabledByDefault: true,
helpLinkUri: "https://aka.ms/minimal-action/analyzer");
}
}
167 changes: 167 additions & 0 deletions src/Framework/Analyzer/src/DelegateEndpoints/RouteAttributeMismatch.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
// 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.Immutable;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;

namespace Microsoft.AspNetCore.Analyzers.DelegateEndpoints;

public partial class DelegateEndpointAnalyzer : DiagnosticAnalyzer
{
private static void RouteAttributeMismatch(
in OperationAnalysisContext context,
WellKnownTypes wellKnownTypes,
IInvocationOperation mapInvocation,
IMethodSymbol mapDelegate)
{
var value = mapInvocation.Arguments[1].Value;
if (value.ConstantValue is not { HasValue: true } constant ||
constant.Value is not string routeTemplate)
{
return;
}

var fromRouteParameters = GetFromRouteParameters(wellKnownTypes, mapDelegate.Parameters);

var enumerator = new RouteTokenEnumerator(routeTemplate);

while (enumerator.MoveNext())
{
var bound = false;
for (var i = fromRouteParameters.Length - 1; i >= 0; i--)
{
if (enumerator.Current.Equals(fromRouteParameters[i].RouteName.AsSpan(), StringComparison.Ordinal))
{
fromRouteParameters = fromRouteParameters.RemoveAt(i);
bound = true;
}
}

if (!bound)
{
// If we didn't bind to an explicit FromRoute parameter, look for
// an implicit one.
foreach (var parameter in mapDelegate.Parameters)
{
if (enumerator.Current.Equals(parameter.Name.AsSpan(), StringComparison.Ordinal))
{
bound = true;
}
}
}

if (!bound)
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.RouteValueIsUnused,
mapInvocation.Arguments[1].Syntax.GetLocation(),
enumerator.Current.ToString()));
}
}

// Report diagnostics for any FromRoute parameter that does is not represented in the route template.
foreach (var parameter in fromRouteParameters)
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.RouteParameterCannotBeBound,
parameter.Parameter.Locations.FirstOrDefault(),
enumerator.Current.ToString()));
}
}

private static ImmutableArray<(IParameterSymbol Parameter, string RouteName)> GetFromRouteParameters(
WellKnownTypes wellKnownTypes, ImmutableArray<IParameterSymbol> parameters)
{
var result = ImmutableArray<(IParameterSymbol, string)>.Empty;
foreach (var parameter in parameters)
{
var attribute = parameter.GetAttributes(wellKnownTypes.IFromRouteMetadata).FirstOrDefault();
if (attribute is not null)
{
var fromRouteName = parameter.Name;
var nameProperty = attribute.NamedArguments.FirstOrDefault(static f => f.Key == "Name");
if (nameProperty.Value is { IsNull: false, Type: { SpecialType: SpecialType.System_String } })
{
fromRouteName = (string)nameProperty.Value.Value;
}

result = result.Add((parameter, fromRouteName));
}
}

return result;
}

internal ref struct RouteTokenEnumerator
{
private ReadOnlySpan<char> _routeTemplate;

public RouteTokenEnumerator(string routeTemplateString)
{
_routeTemplate = routeTemplateString.AsSpan();
Current = default;
}

public ReadOnlySpan<char> Current { get; private set; }

public bool MoveNext()
{
if (_routeTemplate.IsEmpty)
{
return false;
}

findStartBrace:
var startIndex = _routeTemplate.IndexOf('{');
if (startIndex == -1)
{
return false;
}

if (startIndex < _routeTemplate.Length - 1 && _routeTemplate[startIndex + 1] == '{')
{
// Escaped sequence
_routeTemplate = _routeTemplate.Slice(startIndex + 1);
goto findStartBrace;
}

var tokenStart = startIndex + 1;

findEndBrace:
var endIndex = IndexOf(_routeTemplate, tokenStart, '}');
if (endIndex == -1)
{
return false;
}
if (endIndex < _routeTemplate.Length - 1 && _routeTemplate[endIndex + 1] == '}')
{
tokenStart = endIndex + 2;
goto findEndBrace;
}

var token = _routeTemplate.Slice(startIndex + 1, endIndex - startIndex - 1);
var qualifier = token.IndexOfAny(new[] { ':', '=' });
Current = qualifier == -1 ? token : token.Slice(0, qualifier);

_routeTemplate = _routeTemplate.Slice(endIndex + 1);
return true;
}

private static int IndexOf(ReadOnlySpan<char> span, int startIndex, char c)
{
for (var i = startIndex; i < span.Length; i++)
{
if (span[i] == c)
{
return i;
}
}

return -1;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ public static bool TryCreate(Compilation compilation, [NotNullWhen(true)] out We
return false;
}

const string IFromRouteMetadata = "Microsoft.AspNetCore.Http.Metadata.IFromRouteMetadata";
if (compilation.GetTypeByMetadataName(IFromRouteMetadata) is not { } iFromRouteMetadata)
{
return false;
}

wellKnownTypes = new WellKnownTypes
{
DelegateEndpointRouteBuilderExtensions = delegateEndpointRouteBuilderExtensions,
Expand All @@ -57,6 +63,7 @@ public static bool TryCreate(Compilation compilation, [NotNullWhen(true)] out We
IResult = iResult,
IActionResult = iActionResult,
IConvertToActionResult = iConvertToActionResult,
IFromRouteMetadata = iFromRouteMetadata,
};

return true;
Expand All @@ -68,4 +75,5 @@ public static bool TryCreate(Compilation compilation, [NotNullWhen(true)] out We
public INamedTypeSymbol IResult { get; private init; }
public INamedTypeSymbol IActionResult { get; private init; }
public INamedTypeSymbol IConvertToActionResult { get; private init; }
public INamedTypeSymbol IFromRouteMetadata { get; private init; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

<ItemGroup>
<Reference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" PrivateAssets="All" />

<Reference Include="System.Memory" />
<InternalsVisibleTo Include="Microsoft.AspNetCore.App.Analyzers.Test" />
</ItemGroup>

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Globalization;
using Microsoft.AspNetCore.Analyzer.Testing;
using Xunit;

namespace Microsoft.AspNetCore.Analyzers.DelegateEndpoints;

public class RouteAttributeMismatchTest
{
private TestDiagnosticAnalyzerRunner Runner { get; } = new(new DelegateEndpointAnalyzer());

[Theory]
[InlineData("{id}", new[] { "id" })]
[InlineData("{category}/product/{group}", new[] { "category", "group" })]
[InlineData("{category:int}/product/{group:range(10, 20)}?", new[] { "category", "group" })]
[InlineData("{person:int}/{ssn:regex(^\\d{{3}}-\\d{{2}}-\\d{{4}}$)}", new[] { "person", "ssn" })]
[InlineData("{area=Home}/{controller:required}/{id=0:int}", new[] { "area", "controller", "id" })]
public void RouteTokenizer_Works_ForSimpleRouteTemplates(string template, string[] expected)
{
// Arrange
var tokenizer = new DelegateEndpointAnalyzer.RouteTokenEnumerator(template);
var actual = new List<string>();

// Act
while (tokenizer.MoveNext())
{
actual.Add(tokenizer.Current.ToString());
}

// Assert
Assert.Equal(expected, actual);
}

[Fact]
public async Task MinimalAction_UnusedRouteValueProducesDiagnostics()
{
// Arrange
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
var webApp = WebApplication.Create();
webApp.MapPost(/*MM*/""/{id}"", () => {});
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
var diagnostic = Assert.Single(diagnostics);
Assert.Same(DiagnosticDescriptors.RouteValueIsUnused, diagnostic.Descriptor);
Assert.Equal($"The route value 'id' does not get bound and can be removed", diagnostic.GetMessage(CultureInfo.InvariantCulture));
AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
}

[Fact]
public async Task MinimalAction_SomeUnusedTokens_ProducesDiagnostics()
{
// Arrange
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
var webApp = WebApplication.Create();
webApp.MapPost(/*MM*/""/{id:int}/{location:alpha}"", (int id, string loc) => {});
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
var diagnostic = Assert.Single(diagnostics);
Assert.Same(DiagnosticDescriptors.RouteValueIsUnused, diagnostic.Descriptor);
Assert.Equal($"The route value 'location' does not get bound and can be removed", diagnostic.GetMessage(CultureInfo.InvariantCulture));
AnalyzerAssert.DiagnosticLocation(source.DefaultMarkerLocation, diagnostic.Location);
}

[Fact]
public async Task MinimalAction_FromRouteParameterWithMatchingToken_Works()
{
// Arrange
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
var webApp = WebApplication.Create();
webApp.MapPost(/*MM*/""/{id:int}"", ([FromRoute] int id, string loc) => {});
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
Assert.Empty(diagnostics);
}

[Fact]
public async Task MinimalAction_FromRouteParameterUsingName_Works()
{
// Arrange
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
var webApp = WebApplication.Create();
webApp.MapPost(/*MM*/""/{userId}"", ([FromRoute(Name = ""userId"")] int id) => {});
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
Assert.Empty(diagnostics);
}

[Fact]
public async Task MinimalAction_FromRouteParameterWithNoMatchingToken_ProducesDiagnostics()
{
// Arrange
var source = TestSource.Read(@"
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
var webApp = WebApplication.Create();
webApp.MapPost(/*MM1*/""/{userId:int}"", ([FromRoute] int /*MM2*/id, string loc) => {});
");
// Act
var diagnostics = await Runner.GetDiagnosticsAsync(source.Source);

// Assert
Assert.Collection(
diagnostics.OrderBy(d => d.Descriptor.Id),
diagnostic =>
{
Assert.Same(DiagnosticDescriptors.RouteValueIsUnused, diagnostic.Descriptor);
AnalyzerAssert.DiagnosticLocation(source.MarkerLocations["MM1"], diagnostic.Location);
},
diagnostic =>
{
Assert.Same(DiagnosticDescriptors.RouteParameterCannotBeBound, diagnostic.Descriptor);
AnalyzerAssert.DiagnosticLocation(source.MarkerLocations["MM2"], diagnostic.Location);
});
}
}