Skip to content

Fix: Detect parsability when FromRoute attribute is used #45597

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

Merged
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
4 changes: 4 additions & 0 deletions src/Framework/App.Ref/src/CompatibilitySuppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,8 @@
<DiagnosticId>PKV004</DiagnosticId>
<Target>net7.0</Target>
</Suppression>
<Suppression>
<DiagnosticId>PKV004</DiagnosticId>
<Target>net8.0</Target>
</Suppression>
</Suppressions>
4 changes: 4 additions & 0 deletions src/Framework/App.Runtime/src/CompatibilitySuppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,8 @@
<DiagnosticId>PKV0001</DiagnosticId>
<Target>net7.0</Target>
</Suppression>
<Suppression>
<DiagnosticId>PKV0001</DiagnosticId>
<Target>net8.0</Target>
</Suppression>
</Suppressions>
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Immutable;
using Microsoft.AspNetCore.Analyzers.Infrastructure.EmbeddedSyntax;
using Microsoft.AspNetCore.Analyzers.Infrastructure.VirtualChars;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.AspNetCore.Analyzers.Infrastructure.RoutePattern;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@ public static bool HasAttribute(this ISymbol symbol, INamedTypeSymbol attributeT

public static bool HasAttributeImplementingInterface(this ISymbol symbol, INamedTypeSymbol interfaceType)
{
return HasAttributeImplementingInterface(symbol, interfaceType, out var _);
return symbol.HasAttributeImplementingInterface(interfaceType, out var _);
}

public static bool HasAttributeImplementingInterface(this ISymbol symbol, INamedTypeSymbol interfaceType, [NotNullWhen(true)] out AttributeData? matchedAttribute)
{
foreach (var attributeData in symbol.GetAttributes())
{
if (attributeData.AttributeClass is not null && Implements(attributeData.AttributeClass, interfaceType))
if (attributeData.AttributeClass is not null && attributeData.AttributeClass.Implements(interfaceType))
{
matchedAttribute = attributeData;
return true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ private static void DisallowNonParsableComplexTypesOnParameters(
location
)) { continue; }

// Match handler parameter against route parameters. If it is a route parameter it needs to be parsable/bindable in some fashion.
if (routeUsage.RoutePattern.TryGetRouteParameter(handlerDelegateParameter.Name, out var routeParameter))
if (IsRouteParameter(routeUsage, handlerDelegateParameter))
{
var parsability = ParsabilityHelper.GetParsability(parameterTypeSymbol, wellKnownTypes);
var bindability = ParsabilityHelper.GetBindability(parameterTypeSymbol, wellKnownTypes);
Expand All @@ -77,7 +76,7 @@ private static void DisallowNonParsableComplexTypesOnParameters(
context.ReportDiagnostic(Diagnostic.Create(
descriptor,
location,
routeParameter.Name,
handlerDelegateParameter.Name,
parameterTypeSymbol.Name
));
}
Expand All @@ -86,16 +85,22 @@ private static void DisallowNonParsableComplexTypesOnParameters(
}
}

static bool HasAttributeImplementingFromMetadataInterfaceType(IParameterSymbol parameter, WellKnownType fromMetadataInterfaceType, WellKnownTypes wellKnownTypes)
static bool IsRouteParameter(RouteUsageModel routeUsage, IParameterSymbol handlerDelegateParameter)
{
var fromMetadataInterfaceTypeSymbol = wellKnownTypes.Get(fromMetadataInterfaceType);
return parameter.GetAttributes().Any(ad => ad.AttributeClass.Implements(fromMetadataInterfaceTypeSymbol));
// This gets the ParameterSymbol (concept from RouteEmbeddedLanguage) regardless of whether it is in
// the route pattern or not. If it is and it has a custom [FromRoute(Name = "blah")] attribute then
// RouteParameterName and we'll be able to find it by looking it up in the route pattern (thus confirming
// that it is a route parameter).
var resolvedParameter = routeUsage.UsageContext.ResolvedParameters.FirstOrDefault(rp => rp.Symbol.Name == handlerDelegateParameter.Name);
var isRouteParameter = routeUsage.RoutePattern.TryGetRouteParameter(resolvedParameter.RouteParameterName, out var _);
return isRouteParameter;
}

static bool ReportFromAttributeDiagnostic(OperationAnalysisContext context, WellKnownType fromMetadataInterfaceType, WellKnownTypes wellKnownTypes, IParameterSymbol parameter, INamedTypeSymbol parameterTypeSymbol, Location location)
{
var fromMetadataInterfaceTypeSymbol = wellKnownTypes.Get(fromMetadataInterfaceType);
var parsability = ParsabilityHelper.GetParsability(parameterTypeSymbol, wellKnownTypes);
if (HasAttributeImplementingFromMetadataInterfaceType(parameter, fromMetadataInterfaceType, wellKnownTypes) && parsability != Parsability.Parsable)
if (parameter.HasAttributeImplementingInterface(fromMetadataInterfaceTypeSymbol) && parsability != Parsability.Parsable)
{
var descriptor = SelectDescriptor(parsability, Bindability.NotBindable);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,91 @@ public class Customer
await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostic);
}

[Fact]
public async Task Route_Parameter_withNameChanged_viaFromRoute_whenNotParsable_Fails()
{
// Arrange
var source = $$"""
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
var webApp = WebApplication.Create();
webApp.MapGet("/customers/{cust}", ({|#0:[FromRoute(Name = "cust")]Customer customer|}) => {});

public class Customer
{
}
""";

var expectedDiagnostic = new DiagnosticResult(DiagnosticDescriptors.RouteParameterComplexTypeIsNotParsableOrBindable)
.WithArguments("customer", "Customer")
.WithLocation(0);

// Act
await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostic);
}

[Fact]
public async Task Route_Parameter_withTwoReceivingHandlerParameters_Works()
{
// Arrange
var source = $$"""
using System;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
var webApp = WebApplication.Create();
webApp.MapGet("/customers/{cust}", ([FromRoute(Name = "cust")]Customer customer, [FromRoute(Name = "cust")]string customerKey) => {});

public class Customer
{
public static bool TryParse(string s, IFormatProvider provider, out Customer result)
{
result = new Customer();
return true;
}
}
""";

// Act
await VerifyCS.VerifyAnalyzerAsync(source);
}

[Fact]
public async Task Route_Parameter_withNameChanged_viaFromRoute_whenParsable_Works()
{
// Arrange
var source = $$"""
using System;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Mvc;
var webApp = WebApplication.Create();
webApp.MapGet("/customers/{cust}", ({|#0:[FromRoute(Name = "cust")]Customer customer|}) => {});

public class Customer : IParsable<Customer>
{
public static Customer Parse(string s, IFormatProvider provider)
{
if (TryParse(s, provider, out Customer customer))
{
return customer;
}
else
{
throw new ArgumentException(s);
}
}

public static bool TryParse(string s, IFormatProvider provider, out Customer result)
{
result = new Customer();
return true;
}
}
""";

// Act
await VerifyCS.VerifyAnalyzerAsync(source);
}

[Fact]
public async Task Route_Parameter_withBindAsyncMethodThatReturnsTask_of_T_Fails()
{
Expand Down Expand Up @@ -489,7 +574,7 @@ public class Customer
}

[Fact]
public async Task Handler_Parameter_withWellknownTypes_Works()
public async Task Handler_Parameter_withWellKnownTypes_Works()
{
// Arrange
var source = $$"""
Expand Down Expand Up @@ -538,6 +623,108 @@ public async Task Handler_Parameter_with_FromService_Attribute_Works()

public class MyService
{
}
""";

// Act
await VerifyCS.VerifyAnalyzerAsync(source);
}

[Fact]
public async Task Handler_Parameter_withServiceInterface_Works()
{
// Arrange
var source = $$"""
using System;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Builder;

var webApp = WebApplication.Create();
webApp.MapGet("/weatherforecast", (HttpContext context, IDownstreamWebApi downstreamWebApi) => {});

// This type doesn't need to be parsable because it should be assumed to be a service type.
public interface IDownstreamWebApi
{
}
""";

// Act
await VerifyCS.VerifyAnalyzerAsync(source);
}

[Fact]
public async Task Route_Parameter_withAbstractBaseType_Works()
{
// Arrange
var source = $$"""
using System;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Builder;

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.MapGet("/", () => "Hello World!");
app.MapGet("/{customer}", (BaseCustomer customer) => { });
app.Run();

public abstract class BaseCustomer : IParsable<BaseCustomer>
{
public static BaseCustomer Parse(string s, IFormatProvider? provider)
{
return new CommercialCustomer();
}

public static bool TryParse(string? s, IFormatProvider? provider, out BaseCustomer result)
{
result = new CommercialCustomer();
return true;
}
}

public class CommercialCustomer : BaseCustomer
{

}
""";

// Act
await VerifyCS.VerifyAnalyzerAsync(source);
}

[Fact]
public async Task Route_Parameter_withInterfaceType_Works()
{
// Arrange
var source = $$"""
using System;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Builder;

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.MapGet("/", () => "Hello World!");
app.MapGet("/{customer}", (ICustomer customer) => { });
app.Run();

public interface ICustomer
{
public static ICustomer Parse(string s, IFormatProvider? provider)
{
return new CommercialCustomer();
}

public static bool TryParse(string? s, IFormatProvider? provider, out ICustomer result)
{
result = new CommercialCustomer();
return true;
}
}

public class CommercialCustomer : ICustomer
{

}
""";

Expand Down