Skip to content

Commit 5d0e3b0

Browse files
JamesNKJamesNK
authored
Fix duplicate route detection of actions with route parameters (#53545)
1 parent 481a513 commit 5d0e3b0

File tree

2 files changed

+307
-7
lines changed

2 files changed

+307
-7
lines changed

‎src/Framework/AspNetCoreAnalyzers/src/Analyzers/Mvc/DetectAmbiguousActionRoutes.cs

+29-7
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ public partial class MvcAnalyzer
2020
{
2121
private static void DetectAmbiguousActionRoutes(SymbolAnalysisContext context, WellKnownTypes wellKnownTypes, RoutePatternTree? controllerRoutePattern, List<ActionRoute> actionRoutes)
2222
{
23-
var controllerHasActionReplacement = controllerRoutePattern != null ? HasActionReplacementToken(controllerRoutePattern) : false;
23+
var controllerHasActionToken = controllerRoutePattern != null ? HasActionToken(controllerRoutePattern) : false;
2424

2525
// Ambiguous action route detection is conservative in what it detects to avoid false positives.
2626
//
@@ -33,7 +33,7 @@ private static void DetectAmbiguousActionRoutes(SymbolAnalysisContext context, W
3333
{
3434
// Group action routes together. When multiple match in a group, then report action routes to diagnostics.
3535
var groupedByParent = actionRoutes
36-
.GroupBy(ar => new ActionRouteGroupKey(ar.ActionSymbol, ar.RouteUsageModel.RoutePattern, ar.HttpMethods, controllerHasActionReplacement, wellKnownTypes));
36+
.GroupBy(ar => new ActionRouteGroupKey(ar.ActionSymbol, ar.RouteUsageModel.RoutePattern, ar.HttpMethods, controllerHasActionToken, wellKnownTypes));
3737

3838
foreach (var ambiguousGroup in groupedByParent.Where(g => g.Count() >= 2))
3939
{
@@ -48,7 +48,12 @@ private static void DetectAmbiguousActionRoutes(SymbolAnalysisContext context, W
4848
}
4949
}
5050

51-
private static bool HasActionReplacementToken(RoutePatternTree routePattern)
51+
/// <summary>
52+
/// Search route pattern for:
53+
/// 1. Action replacement tokens, [action]
54+
/// 2. Action parameter tokens, {action}
55+
/// </summary>
56+
private static bool HasActionToken(RoutePatternTree routePattern)
5257
{
5358
for (var i = 0; i < routePattern.Root.Parts.Length; i++)
5459
{
@@ -67,6 +72,23 @@ private static bool HasActionReplacementToken(RoutePatternTree routePattern)
6772
}
6873
}
6974
}
75+
else if (segment.Children[j] is RoutePatternParameterNode parameterNode)
76+
{
77+
for (var k = 0; k < parameterNode.ParameterParts.Length; k++)
78+
{
79+
if (parameterNode.ParameterParts[k] is RoutePatternNameParameterPartNode namePartNode)
80+
{
81+
if (!namePartNode.ParameterNameToken.IsMissing)
82+
{
83+
var name = namePartNode.ParameterNameToken.Value!.ToString();
84+
if (string.Equals(name, "action", StringComparison.OrdinalIgnoreCase))
85+
{
86+
return true;
87+
}
88+
}
89+
}
90+
}
91+
}
7092
}
7193
}
7294
}
@@ -80,10 +102,10 @@ private static bool HasActionReplacementToken(RoutePatternTree routePattern)
80102
public RoutePatternTree RoutePattern { get; }
81103
public ImmutableArray<string> HttpMethods { get; }
82104
public string ActionName { get; }
83-
public bool HasActionReplacement { get; }
105+
public bool HasActionToken { get; }
84106
private readonly WellKnownTypes _wellKnownTypes;
85107

86-
public ActionRouteGroupKey(IMethodSymbol actionSymbol, RoutePatternTree routePattern, ImmutableArray<string> httpMethods, bool controllerHasActionReplacement, WellKnownTypes wellKnownTypes)
108+
public ActionRouteGroupKey(IMethodSymbol actionSymbol, RoutePatternTree routePattern, ImmutableArray<string> httpMethods, bool controllerHasActionToken, WellKnownTypes wellKnownTypes)
87109
{
88110
Debug.Assert(!httpMethods.IsDefault);
89111

@@ -92,7 +114,7 @@ public ActionRouteGroupKey(IMethodSymbol actionSymbol, RoutePatternTree routePat
92114
HttpMethods = httpMethods;
93115
_wellKnownTypes = wellKnownTypes;
94116
ActionName = GetActionName(ActionSymbol, _wellKnownTypes);
95-
HasActionReplacement = controllerHasActionReplacement || HasActionReplacementToken(RoutePattern);
117+
HasActionToken = controllerHasActionToken || HasActionToken(RoutePattern);
96118
}
97119

98120
private static string GetActionName(IMethodSymbol actionSymbol, WellKnownTypes wellKnownTypes)
@@ -118,7 +140,7 @@ public bool Equals(ActionRouteGroupKey other)
118140
{
119141
return
120142
AmbiguousRoutePatternComparer.Instance.Equals(RoutePattern, other.RoutePattern) &&
121-
(!HasActionReplacement || string.Equals(ActionName, other.ActionName, StringComparison.OrdinalIgnoreCase)) &&
143+
(!HasActionToken || string.Equals(ActionName, other.ActionName, StringComparison.OrdinalIgnoreCase)) &&
122144
HasMatchingHttpMethods(HttpMethods, other.HttpMethods) &&
123145
CanMatchActions(_wellKnownTypes, ActionSymbol, other.ActionSymbol);
124146
}

‎src/Framework/AspNetCoreAnalyzers/test/Mvc/DetectAmbiguousActionRoutesTest.cs

+278
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,284 @@ static void Main(string[] args)
318318
await VerifyCS.VerifyAnalyzerAsync(source);
319319
}
320320

321+
[Fact]
322+
public async Task ActionRouteToken_DifferentActionNames_NoDiagnostics()
323+
{
324+
// Arrange
325+
var source = @"
326+
using Microsoft.AspNetCore.Builder;
327+
using Microsoft.AspNetCore.Mvc;
328+
public class WeatherForecastController : ControllerBase
329+
{
330+
[Route(""{action}"")]
331+
public object Get() => new object();
332+
333+
[Route(""{action}"")]
334+
public object Get1() => new object();
335+
}
336+
internal class Program
337+
{
338+
static void Main(string[] args)
339+
{
340+
}
341+
}
342+
";
343+
344+
// Act & Assert
345+
await VerifyCS.VerifyAnalyzerAsync(source);
346+
}
347+
348+
[Fact]
349+
public async Task ActionRouteToken_SameActionName_HasDiagnostics()
350+
{
351+
// Arrange
352+
var source = @"
353+
using Microsoft.AspNetCore.Builder;
354+
using Microsoft.AspNetCore.Mvc;
355+
public class WeatherForecastController : ControllerBase
356+
{
357+
[Route({|#0:""{action}""|})]
358+
public object Get() => new object();
359+
360+
[Route({|#1:""{action}""|})]
361+
public object Get(int i) => new object();
362+
}
363+
internal class Program
364+
{
365+
static void Main(string[] args)
366+
{
367+
}
368+
}
369+
";
370+
371+
var expectedDiagnostics = new[] {
372+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousActionRoute).WithArguments("{action}").WithLocation(0),
373+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousActionRoute).WithArguments("{action}").WithLocation(1)
374+
};
375+
376+
// Act & Assert
377+
await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics);
378+
}
379+
380+
[Fact]
381+
public async Task ActionRouteToken_ActionNameAttribute_HasDiagnostics()
382+
{
383+
// Arrange
384+
var source = @"
385+
using Microsoft.AspNetCore.Builder;
386+
using Microsoft.AspNetCore.Mvc;
387+
public class WeatherForecastController : ControllerBase
388+
{
389+
[Route({|#0:""{action}""|})]
390+
public object Get() => new object();
391+
392+
[Route({|#1:""{action}""|})]
393+
[ActionName(""get"")]
394+
public object Get1(int i) => new object();
395+
}
396+
internal class Program
397+
{
398+
static void Main(string[] args)
399+
{
400+
}
401+
}
402+
";
403+
404+
var expectedDiagnostics = new[] {
405+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousActionRoute).WithArguments("{action}").WithLocation(0),
406+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousActionRoute).WithArguments("{action}").WithLocation(1)
407+
};
408+
409+
// Act & Assert
410+
await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics);
411+
}
412+
413+
[Fact]
414+
public async Task ActionRouteToken_ActionNameAttributeNullValue_NoDiagnostics()
415+
{
416+
// Arrange
417+
var source = @"
418+
using Microsoft.AspNetCore.Builder;
419+
using Microsoft.AspNetCore.Mvc;
420+
public class WeatherForecastController : ControllerBase
421+
{
422+
[Route({|#0:""{action}""|})]
423+
public object Get() => new object();
424+
425+
[Route({|#1:""{action}""|})]
426+
[ActionName(null)]
427+
public object Get1(int i) => new object();
428+
}
429+
internal class Program
430+
{
431+
static void Main(string[] args)
432+
{
433+
}
434+
}
435+
";
436+
437+
// Act & Assert
438+
await VerifyCS.VerifyAnalyzerAsync(source);
439+
}
440+
441+
[Fact]
442+
public async Task ActionRouteToken_OnController_NoDiagnostics()
443+
{
444+
// Arrange
445+
var source = @"
446+
using Microsoft.AspNetCore.Builder;
447+
using Microsoft.AspNetCore.Mvc;
448+
[Route(""{controller}/{action}"")]
449+
public class WeatherForecastController : ControllerBase
450+
{
451+
[Route(""{i}"")]
452+
public object Get(int i) => new object();
453+
454+
[Route(""{i}"")]
455+
public object Get1(int i) => new object();
456+
}
457+
internal class Program
458+
{
459+
static void Main(string[] args)
460+
{
461+
}
462+
}
463+
";
464+
465+
// Act & Assert
466+
await VerifyCS.VerifyAnalyzerAsync(source);
467+
}
468+
469+
[Fact]
470+
public async Task ActionRouteToken_OnBaseController_NoDiagnostics()
471+
{
472+
// Arrange
473+
var source = @"
474+
using Microsoft.AspNetCore.Builder;
475+
using Microsoft.AspNetCore.Mvc;
476+
[Route(""{controller}/{action}"")]
477+
public class MyControllerBase : ControllerBase
478+
{
479+
}
480+
public class WeatherForecastController : MyControllerBase
481+
{
482+
[Route(""{i}"")]
483+
public object Get(int i) => new object();
484+
485+
[Route(""{i}"")]
486+
public object Get1(int i) => new object();
487+
}
488+
internal class Program
489+
{
490+
static void Main(string[] args)
491+
{
492+
}
493+
}
494+
";
495+
496+
// Act & Assert
497+
await VerifyCS.VerifyAnalyzerAsync(source);
498+
}
499+
500+
[Fact]
501+
public async Task ActionRouteToken_OnBaseControllerButOverridden_HasDiagnostics()
502+
{
503+
// Arrange
504+
var source = @"
505+
using Microsoft.AspNetCore.Builder;
506+
using Microsoft.AspNetCore.Mvc;
507+
[Route(""{controller}/{action}"")]
508+
public class MyControllerBase : ControllerBase
509+
{
510+
}
511+
[Route(""api"")]
512+
public class WeatherForecastController : MyControllerBase
513+
{
514+
[Route({|#0:""{i}""|})]
515+
public object Get(int i) => new object();
516+
517+
[Route({|#1:""{i}""|})]
518+
public object Get1(int i) => new object();
519+
}
520+
internal class Program
521+
{
522+
static void Main(string[] args)
523+
{
524+
}
525+
}
526+
";
527+
528+
var expectedDiagnostics = new[] {
529+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousActionRoute).WithArguments("{i}").WithLocation(0),
530+
new DiagnosticResult(DiagnosticDescriptors.AmbiguousActionRoute).WithArguments("{i}").WithLocation(1)
531+
};
532+
533+
// Act & Assert
534+
await VerifyCS.VerifyAnalyzerAsync(source, expectedDiagnostics);
535+
}
536+
537+
[Fact]
538+
public async Task ActionRouteToken_OnController_ActionName_NoDiagnostics()
539+
{
540+
// Arrange
541+
var source = @"
542+
using Microsoft.AspNetCore.Builder;
543+
using Microsoft.AspNetCore.Mvc;
544+
[Route(""{controller}/{action}"")]
545+
public class WeatherForecastController : ControllerBase
546+
{
547+
[Route(""{i}"")]
548+
public object Get(int i) => new object();
549+
550+
[Route(""{s}"")]
551+
[ActionName(name: ""getWithString"")]
552+
public object Get(string s) => new object();
553+
}
554+
internal class Program
555+
{
556+
static void Main(string[] args)
557+
{
558+
}
559+
}
560+
";
561+
562+
// Act & Assert
563+
await VerifyCS.VerifyAnalyzerAsync(source);
564+
}
565+
566+
[Fact]
567+
public async Task ActionRouteToken_OnController_ActionNameOnBase_NoDiagnostics()
568+
{
569+
// Arrange
570+
var source = @"
571+
using Microsoft.AspNetCore.Builder;
572+
using Microsoft.AspNetCore.Mvc;
573+
public abstract class MyControllerBase : ControllerBase
574+
{
575+
[ActionName(name: ""getWithString"")]
576+
public abstract object Get(string s);
577+
}
578+
[Route(""{controller}/{action}"")]
579+
public class WeatherForecastController : MyControllerBase
580+
{
581+
[Route(""{i}"")]
582+
public object Get(int i) => new object();
583+
584+
[Route(""{s}"")]
585+
public override object Get(string s) => new object();
586+
}
587+
internal class Program
588+
{
589+
static void Main(string[] args)
590+
{
591+
}
592+
}
593+
";
594+
595+
// Act & Assert
596+
await VerifyCS.VerifyAnalyzerAsync(source);
597+
}
598+
321599
[Fact]
322600
public async Task MixedRoutes_DifferentAction_HasDiagnostics()
323601
{

0 commit comments

Comments
 (0)