Skip to content

Commit 7df092d

Browse files
joegoldman2JamesNK
joegoldman2
and
JamesNK
authored
ASP0022 - Fix false positive for groups without using intermediate variables (#56116)
Co-authored-by: James Newton-King <james@newtonking.com>
1 parent a3990b7 commit 7df092d

File tree

2 files changed

+212
-9
lines changed

2 files changed

+212
-9
lines changed

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

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,9 +173,42 @@ public bool Equals(MapOperationGroupKey other)
173173
ParentOperation != null &&
174174
Equals(ParentOperation, other.ParentOperation) &&
175175
Builder != null &&
176-
SymbolEqualityComparer.Default.Equals((Builder as ILocalReferenceOperation)?.Local, (other.Builder as ILocalReferenceOperation)?.Local) &&
176+
AreBuildersEqual(Builder, other.Builder) &&
177177
AmbiguousRoutePatternComparer.Instance.Equals(RoutePattern, other.RoutePattern) &&
178178
HasMatchingHttpMethods(HttpMethods, other.HttpMethods);
179+
180+
static bool AreBuildersEqual(IOperation builder, IOperation? other)
181+
{
182+
if (builder is ILocalReferenceOperation local && other is ILocalReferenceOperation otherLocal)
183+
{
184+
// The builders are both local variables.
185+
return SymbolEqualityComparer.Default.Equals(local.Local, otherLocal.Local);
186+
}
187+
188+
if (builder is IParameterReferenceOperation parameter && other is IParameterReferenceOperation otherParameter)
189+
{
190+
// The builders are both parameter variables.
191+
return SymbolEqualityComparer.Default.Equals(parameter.Parameter, otherParameter.Parameter);
192+
}
193+
194+
if (builder is IInvocationOperation invocation && other is IInvocationOperation otherInvocation)
195+
{
196+
if (invocation.TargetMethod.Name == "MapGroup" &&
197+
invocation.TargetMethod.Parameters.Length == 2 &&
198+
SymbolEqualityComparer.Default.Equals(invocation.TargetMethod, otherInvocation.TargetMethod) &&
199+
invocation.Arguments.Length == 2 &&
200+
otherInvocation.Arguments.Length == 2)
201+
{
202+
// The builders are both method calls. Special case checking known MapGroup method.
203+
// For example, two MapGroup calls with the same route are considered equal:
204+
// builder.MapGroup("/v1").MapGet("account")
205+
// builder.MapGroup("/v1").MapGet("account")
206+
return AreArgumentsEqual(invocation.TargetMethod, invocation.Arguments, otherInvocation.Arguments);
207+
}
208+
}
209+
210+
return false;
211+
}
179212
}
180213

181214
private static bool HasMatchingHttpMethods(ImmutableArray<string> httpMethods1, ImmutableArray<string> httpMethods2)
@@ -199,6 +232,66 @@ private static bool HasMatchingHttpMethods(ImmutableArray<string> httpMethods1,
199232
return false;
200233
}
201234

235+
private static bool AreArgumentsEqual(IMethodSymbol method, ImmutableArray<IArgumentOperation> arguments1, ImmutableArray<IArgumentOperation> arguments2)
236+
{
237+
for (var i = 0; i < method.Parameters.Length; i++)
238+
{
239+
var argument1 = GetParameterArgument(method.Parameters[i], arguments1);
240+
var argument2 = GetParameterArgument(method.Parameters[i], arguments2);
241+
242+
if (argument1 is ILocalReferenceOperation local && argument2 is ILocalReferenceOperation otherLocal)
243+
{
244+
if (!SymbolEqualityComparer.Default.Equals(local.Local, otherLocal.Local))
245+
{
246+
return false;
247+
}
248+
}
249+
else if (argument1 is IParameterReferenceOperation parameter && argument2 is IParameterReferenceOperation otherParameter)
250+
{
251+
if (!SymbolEqualityComparer.Default.Equals(parameter.Parameter, otherParameter.Parameter))
252+
{
253+
return false;
254+
}
255+
}
256+
else if (argument1 is ILiteralOperation literal && argument2 is ILiteralOperation otherLiteral)
257+
{
258+
if (!Equals(literal.ConstantValue, otherLiteral.ConstantValue))
259+
{
260+
return false;
261+
}
262+
}
263+
else
264+
{
265+
return false;
266+
}
267+
}
268+
269+
return true;
270+
271+
static IOperation? GetParameterArgument(IParameterSymbol parameter, ImmutableArray<IArgumentOperation> arguments)
272+
{
273+
for (var i = 0; i < arguments.Length; i++)
274+
{
275+
if (SymbolEqualityComparer.Default.Equals(arguments[i].Parameter, parameter))
276+
{
277+
return WalkDownConversion(arguments[i].Value);
278+
}
279+
}
280+
281+
return null;
282+
}
283+
}
284+
285+
private static IOperation WalkDownConversion(IOperation operation)
286+
{
287+
while (operation is IConversionOperation conversionOperation)
288+
{
289+
operation = conversionOperation.Operand;
290+
}
291+
292+
return operation;
293+
}
294+
202295
public override int GetHashCode()
203296
{
204297
return (ParentOperation?.GetHashCode() ?? 0) ^ AmbiguousRoutePatternComparer.Instance.GetHashCode(RoutePattern);

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

Lines changed: 118 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ public async Task DuplicateRoutes_SameHttpMethod_HasDiagnostics()
2020
void Hello() { }
2121
";
2222

23-
var expectedDiagnostics = new[] {
23+
var expectedDiagnostics = new[]
24+
{
2425
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0),
2526
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1)
2627
};
@@ -43,7 +44,8 @@ public async Task DuplicateRoutes_SameHttpMethod_HasRequestDelegate_HasDiagnosti
4344
void Hello() { }
4445
";
4546

46-
var expectedDiagnostics = new[] {
47+
var expectedDiagnostics = new[]
48+
{
4749
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0),
4850
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1)
4951
};
@@ -71,7 +73,8 @@ void RegisterEndpoints(IEndpointRouteBuilder builder)
7173
void Hello() { }
7274
";
7375

74-
var expectedDiagnostics = new[] {
76+
var expectedDiagnostics = new[]
77+
{
7578
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0),
7679
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1)
7780
};
@@ -140,7 +143,8 @@ public async Task DuplicateRoutes_InsideSwitchStatement_HasDiagnostics()
140143
void Hello() { }
141144
";
142145

143-
var expectedDiagnostics = new[] {
146+
var expectedDiagnostics = new[]
147+
{
144148
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0),
145149
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1)
146150
};
@@ -254,7 +258,8 @@ public async Task DuplicateMapGetRoutes_DuplicatesInsideConditional_NoDiagnostic
254258
void Hello() { }
255259
";
256260

257-
var expectedDiagnostics = new[] {
261+
var expectedDiagnostics = new[]
262+
{
258263
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0),
259264
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1)
260265
};
@@ -361,6 +366,110 @@ void Hello() { }
361366
await VerifyCS.VerifyAnalyzerAsync(source);
362367
}
363368

369+
[Fact]
370+
public async Task DuplicateRoutes_MultipleGroups_DirectInvocation_NoDiagnostics()
371+
{
372+
// Arrange
373+
var source = @"
374+
using Microsoft.AspNetCore.Builder;
375+
var app = WebApplication.Create();
376+
app.MapGroup(""/group1"").MapGet(""/"", () => { });
377+
app.MapGroup(""/group2"").MapGet(""/"", () => { });
378+
";
379+
380+
// Act & Assert
381+
await VerifyCS.VerifyAnalyzerAsync(source);
382+
}
383+
384+
[Fact]
385+
public async Task DuplicateRoutes_SingleGroup_DifferentBuilderVariable_DirectInvocation_NoDiagnostics()
386+
{
387+
// Arrange
388+
var source = @"
389+
using Microsoft.AspNetCore.Builder;
390+
var app1 = WebApplication.Create();
391+
var app2 = app1;
392+
app1.MapGroup(""/group1"").MapGet(""/"", () => { });
393+
app2.MapGroup(""/group1"").MapGet(""/"", () => { });
394+
";
395+
396+
// Act & Assert
397+
await VerifyCS.VerifyAnalyzerAsync(source);
398+
}
399+
400+
[Fact]
401+
public async Task DuplicateRoutes_SingleGroup_DirectInvocation_HasDiagnostics()
402+
{
403+
// Arrange
404+
var source = @"
405+
using Microsoft.AspNetCore.Builder;
406+
var app = WebApplication.Create();
407+
app.MapGroup(""/group1"").MapGet({|#0:""/""|}, () => { });
408+
app.MapGroup(""/group1"").MapGet({|#1:""/""|}, () => { });
409+
";
410+
411+
// Act & Assert
412+
var expectedDiagnostics = new[]
413+
{
414+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0),
415+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1)
416+
};
417+
418+
await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics);
419+
}
420+
421+
[Fact]
422+
public async Task DuplicateRoutes_SingleGroup_DirectInvocation_InMethod_HasDiagnostics()
423+
{
424+
// Arrange
425+
var source = @"
426+
using Microsoft.AspNetCore.Builder;
427+
using Microsoft.AspNetCore.Routing;
428+
var app = WebApplication.Create();
429+
void RegisterEndpoints(IEndpointRouteBuilder builder)
430+
{
431+
builder.MapGroup(""/group1"").MapGet({|#0:""/""|}, () => { });
432+
builder.MapGroup(""/group1"").MapGet({|#1:""/""|}, () => { });
433+
}
434+
435+
RegisterEndpoints(app);
436+
437+
void Hello() { }
438+
";
439+
440+
var expectedDiagnostics = new[]
441+
{
442+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0),
443+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1)
444+
};
445+
446+
// Act & Assert
447+
await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics);
448+
}
449+
450+
[Fact]
451+
public async Task DuplicateRoutes_SingleGroup_RoutePattern_DirectInvocation_HasDiagnostics()
452+
{
453+
// Arrange
454+
var source = @"
455+
using Microsoft.AspNetCore.Builder;
456+
using Microsoft.AspNetCore.Routing.Patterns;
457+
var routePattern = RoutePatternFactory.Parse(""/group1"");
458+
var app = WebApplication.Create();
459+
app.MapGroup(routePattern).MapGet({|#0:""/""|}, () => { });
460+
app.MapGroup(routePattern).MapGet({|#1:""/""|}, () => { });
461+
";
462+
463+
// Act & Assert
464+
var expectedDiagnostics = new[]
465+
{
466+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0),
467+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1)
468+
};
469+
470+
await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics);
471+
}
472+
364473
[Fact]
365474
public async Task DuplicateRoutes_EndpointsOnGroup_HasDiagnostics()
366475
{
@@ -377,7 +486,8 @@ void Hello() { }
377486
";
378487

379488
// Act & Assert
380-
var expectedDiagnostics = new[] {
489+
var expectedDiagnostics = new[]
490+
{
381491
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0),
382492
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1)
383493
};
@@ -409,7 +519,8 @@ public async Task DuplicateRoutes_AllowedBuilderExtensionMethods_HasDiagnostics(
409519
void Hello() { }
410520
";
411521

412-
var expectedDiagnostics = new[] {
522+
var expectedDiagnostics = new[]
523+
{
413524
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(0),
414525
new DiagnosticResult(DiagnosticDescriptors.AmbiguousRouteHandlerRoute).WithArguments("/").WithLocation(1)
415526
};
@@ -440,4 +551,3 @@ void Hello() { }
440551
await VerifyCS.VerifyAnalyzerAsync(source);
441552
}
442553
}
443-

0 commit comments

Comments
 (0)