Skip to content

Commit 09b0db0

Browse files
mitchdennymitchdenny
authored
Fix: Detect parsability when FromRoute attribute is used (#45597)
* Add support for handling FromRoute attribute with route parameter renaming.
1 parent 97e26bd commit 09b0db0

File tree

6 files changed

+211
-10
lines changed

6 files changed

+211
-10
lines changed

‎src/Framework/App.Ref/src/CompatibilitySuppressions.xml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,8 @@
44
<DiagnosticId>PKV004</DiagnosticId>
55
<Target>net7.0</Target>
66
</Suppression>
7+
<Suppression>
8+
<DiagnosticId>PKV004</DiagnosticId>
9+
<Target>net8.0</Target>
10+
</Suppression>
711
</Suppressions>

‎src/Framework/App.Runtime/src/CompatibilitySuppressions.xml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,8 @@
44
<DiagnosticId>PKV0001</DiagnosticId>
55
<Target>net7.0</Target>
66
</Suppression>
7+
<Suppression>
8+
<DiagnosticId>PKV0001</DiagnosticId>
9+
<Target>net8.0</Target>
10+
</Suppression>
711
</Suppressions>

‎src/Framework/AspNetCoreAnalyzers/src/Analyzers/Infrastructure/RoutePattern/RoutePatternTree.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Collections.Immutable;
66
using Microsoft.AspNetCore.Analyzers.Infrastructure.EmbeddedSyntax;
77
using Microsoft.AspNetCore.Analyzers.Infrastructure.VirtualChars;
8+
using Microsoft.CodeAnalysis;
89
using Microsoft.CodeAnalysis.Text;
910

1011
namespace Microsoft.AspNetCore.Analyzers.Infrastructure.RoutePattern;

‎src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteEmbeddedLanguage/Infrastructure/SymbolExtensions.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,14 @@ public static bool HasAttribute(this ISymbol symbol, INamedTypeSymbol attributeT
2626

2727
public static bool HasAttributeImplementingInterface(this ISymbol symbol, INamedTypeSymbol interfaceType)
2828
{
29-
return HasAttributeImplementingInterface(symbol, interfaceType, out var _);
29+
return symbol.HasAttributeImplementingInterface(interfaceType, out var _);
3030
}
3131

3232
public static bool HasAttributeImplementingInterface(this ISymbol symbol, INamedTypeSymbol interfaceType, [NotNullWhen(true)] out AttributeData? matchedAttribute)
3333
{
3434
foreach (var attributeData in symbol.GetAttributes())
3535
{
36-
if (attributeData.AttributeClass is not null && Implements(attributeData.AttributeClass, interfaceType))
36+
if (attributeData.AttributeClass is not null && attributeData.AttributeClass.Implements(interfaceType))
3737
{
3838
matchedAttribute = attributeData;
3939
return true;

‎src/Framework/AspNetCoreAnalyzers/src/Analyzers/RouteHandlers/DisallowNonParsableComplexTypesOnParameters.cs

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,7 @@ private static void DisallowNonParsableComplexTypesOnParameters(
6464
location
6565
)) { continue; }
6666

67-
// Match handler parameter against route parameters. If it is a route parameter it needs to be parsable/bindable in some fashion.
68-
if (routeUsage.RoutePattern.TryGetRouteParameter(handlerDelegateParameter.Name, out var routeParameter))
67+
if (IsRouteParameter(routeUsage, handlerDelegateParameter))
6968
{
7069
var parsability = ParsabilityHelper.GetParsability(parameterTypeSymbol, wellKnownTypes);
7170
var bindability = ParsabilityHelper.GetBindability(parameterTypeSymbol, wellKnownTypes);
@@ -77,7 +76,7 @@ private static void DisallowNonParsableComplexTypesOnParameters(
7776
context.ReportDiagnostic(Diagnostic.Create(
7877
descriptor,
7978
location,
80-
routeParameter.Name,
79+
handlerDelegateParameter.Name,
8180
parameterTypeSymbol.Name
8281
));
8382
}
@@ -86,16 +85,22 @@ private static void DisallowNonParsableComplexTypesOnParameters(
8685
}
8786
}
8887

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

9599
static bool ReportFromAttributeDiagnostic(OperationAnalysisContext context, WellKnownType fromMetadataInterfaceType, WellKnownTypes wellKnownTypes, IParameterSymbol parameter, INamedTypeSymbol parameterTypeSymbol, Location location)
96100
{
101+
var fromMetadataInterfaceTypeSymbol = wellKnownTypes.Get(fromMetadataInterfaceType);
97102
var parsability = ParsabilityHelper.GetParsability(parameterTypeSymbol, wellKnownTypes);
98-
if (HasAttributeImplementingFromMetadataInterfaceType(parameter, fromMetadataInterfaceType, wellKnownTypes) && parsability != Parsability.Parsable)
103+
if (parameter.HasAttributeImplementingInterface(fromMetadataInterfaceTypeSymbol) && parsability != Parsability.Parsable)
99104
{
100105
var descriptor = SelectDescriptor(parsability, Bindability.NotBindable);
101106

‎src/Framework/AspNetCoreAnalyzers/test/RouteHandlers/DisallowNonParsableComplexTypesOnParametersTest.cs

Lines changed: 188 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,91 @@ public class Customer
203203
await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostic);
204204
}
205205

206+
[Fact]
207+
public async Task Route_Parameter_withNameChanged_viaFromRoute_whenNotParsable_Fails()
208+
{
209+
// Arrange
210+
var source = $$"""
211+
using Microsoft.AspNetCore.Builder;
212+
using Microsoft.AspNetCore.Mvc;
213+
var webApp = WebApplication.Create();
214+
webApp.MapGet("/customers/{cust}", ({|#0:[FromRoute(Name = "cust")]Customer customer|}) => {});
215+
216+
public class Customer
217+
{
218+
}
219+
""";
220+
221+
var expectedDiagnostic = new DiagnosticResult(DiagnosticDescriptors.RouteParameterComplexTypeIsNotParsableOrBindable)
222+
.WithArguments("customer", "Customer")
223+
.WithLocation(0);
224+
225+
// Act
226+
await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostic);
227+
}
228+
229+
[Fact]
230+
public async Task Route_Parameter_withTwoReceivingHandlerParameters_Works()
231+
{
232+
// Arrange
233+
var source = $$"""
234+
using System;
235+
using Microsoft.AspNetCore.Builder;
236+
using Microsoft.AspNetCore.Mvc;
237+
var webApp = WebApplication.Create();
238+
webApp.MapGet("/customers/{cust}", ([FromRoute(Name = "cust")]Customer customer, [FromRoute(Name = "cust")]string customerKey) => {});
239+
240+
public class Customer
241+
{
242+
public static bool TryParse(string s, IFormatProvider provider, out Customer result)
243+
{
244+
result = new Customer();
245+
return true;
246+
}
247+
}
248+
""";
249+
250+
// Act
251+
await VerifyCS.VerifyAnalyzerAsync(source);
252+
}
253+
254+
[Fact]
255+
public async Task Route_Parameter_withNameChanged_viaFromRoute_whenParsable_Works()
256+
{
257+
// Arrange
258+
var source = $$"""
259+
using System;
260+
using Microsoft.AspNetCore.Builder;
261+
using Microsoft.AspNetCore.Mvc;
262+
var webApp = WebApplication.Create();
263+
webApp.MapGet("/customers/{cust}", ({|#0:[FromRoute(Name = "cust")]Customer customer|}) => {});
264+
265+
public class Customer : IParsable<Customer>
266+
{
267+
public static Customer Parse(string s, IFormatProvider provider)
268+
{
269+
if (TryParse(s, provider, out Customer customer))
270+
{
271+
return customer;
272+
}
273+
else
274+
{
275+
throw new ArgumentException(s);
276+
}
277+
}
278+
279+
public static bool TryParse(string s, IFormatProvider provider, out Customer result)
280+
{
281+
result = new Customer();
282+
return true;
283+
}
284+
}
285+
""";
286+
287+
// Act
288+
await VerifyCS.VerifyAnalyzerAsync(source);
289+
}
290+
206291
[Fact]
207292
public async Task Route_Parameter_withBindAsyncMethodThatReturnsTask_of_T_Fails()
208293
{
@@ -489,7 +574,7 @@ public class Customer
489574
}
490575

491576
[Fact]
492-
public async Task Handler_Parameter_withWellknownTypes_Works()
577+
public async Task Handler_Parameter_withWellKnownTypes_Works()
493578
{
494579
// Arrange
495580
var source = $$"""
@@ -538,6 +623,108 @@ public async Task Handler_Parameter_with_FromService_Attribute_Works()
538623
539624
public class MyService
540625
{
626+
}
627+
""";
628+
629+
// Act
630+
await VerifyCS.VerifyAnalyzerAsync(source);
631+
}
632+
633+
[Fact]
634+
public async Task Handler_Parameter_withServiceInterface_Works()
635+
{
636+
// Arrange
637+
var source = $$"""
638+
using System;
639+
using Microsoft.AspNetCore.Http;
640+
using Microsoft.AspNetCore.Builder;
641+
642+
var webApp = WebApplication.Create();
643+
webApp.MapGet("/weatherforecast", (HttpContext context, IDownstreamWebApi downstreamWebApi) => {});
644+
645+
// This type doesn't need to be parsable because it should be assumed to be a service type.
646+
public interface IDownstreamWebApi
647+
{
648+
}
649+
""";
650+
651+
// Act
652+
await VerifyCS.VerifyAnalyzerAsync(source);
653+
}
654+
655+
[Fact]
656+
public async Task Route_Parameter_withAbstractBaseType_Works()
657+
{
658+
// Arrange
659+
var source = $$"""
660+
using System;
661+
using Microsoft.AspNetCore.Http;
662+
using Microsoft.AspNetCore.Builder;
663+
664+
var builder = WebApplication.CreateBuilder(args);
665+
var app = builder.Build();
666+
667+
app.MapGet("/", () => "Hello World!");
668+
app.MapGet("/{customer}", (BaseCustomer customer) => { });
669+
app.Run();
670+
671+
public abstract class BaseCustomer : IParsable<BaseCustomer>
672+
{
673+
public static BaseCustomer Parse(string s, IFormatProvider? provider)
674+
{
675+
return new CommercialCustomer();
676+
}
677+
678+
public static bool TryParse(string? s, IFormatProvider? provider, out BaseCustomer result)
679+
{
680+
result = new CommercialCustomer();
681+
return true;
682+
}
683+
}
684+
685+
public class CommercialCustomer : BaseCustomer
686+
{
687+
688+
}
689+
""";
690+
691+
// Act
692+
await VerifyCS.VerifyAnalyzerAsync(source);
693+
}
694+
695+
[Fact]
696+
public async Task Route_Parameter_withInterfaceType_Works()
697+
{
698+
// Arrange
699+
var source = $$"""
700+
using System;
701+
using Microsoft.AspNetCore.Http;
702+
using Microsoft.AspNetCore.Builder;
703+
704+
var builder = WebApplication.CreateBuilder(args);
705+
var app = builder.Build();
706+
707+
app.MapGet("/", () => "Hello World!");
708+
app.MapGet("/{customer}", (ICustomer customer) => { });
709+
app.Run();
710+
711+
public interface ICustomer
712+
{
713+
public static ICustomer Parse(string s, IFormatProvider? provider)
714+
{
715+
return new CommercialCustomer();
716+
}
717+
718+
public static bool TryParse(string? s, IFormatProvider? provider, out ICustomer result)
719+
{
720+
result = new CommercialCustomer();
721+
return true;
722+
}
723+
}
724+
725+
public class CommercialCustomer : ICustomer
726+
{
727+
541728
}
542729
""";
543730

0 commit comments

Comments
 (0)