Skip to content

Commit d6bc6c9

Browse files
author
Chris Martinez
committed
Improve logging for ambiguous matches. Related to #248
1 parent 07857f6 commit d6bc6c9

File tree

5 files changed

+230
-9
lines changed

5 files changed

+230
-9
lines changed

src/Microsoft.AspNetCore.Mvc.Versioning/Microsoft.AspNetCore.Mvc.Versioning.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
<ItemGroup>
1616
<ReleaseNotes Include="Fix 405 error message (#254)" />
17+
<ReleaseNotes Include="Improve logging for ambiguous actions" />
1718
</ItemGroup>
1819

1920
<ItemGroup>

src/Microsoft.AspNetCore.Mvc.Versioning/Routing/DefaultApiVersionRoutePolicy.cs

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
using Microsoft.AspNetCore.Http;
44
using Microsoft.AspNetCore.Http.Extensions;
55
using Microsoft.AspNetCore.Mvc.Abstractions;
6+
using Microsoft.AspNetCore.Mvc.Controllers;
67
using Microsoft.AspNetCore.Mvc.Infrastructure;
78
using Microsoft.AspNetCore.Mvc.Internal;
89
using Microsoft.AspNetCore.Routing;
@@ -12,6 +13,7 @@
1213
using System.Collections.Generic;
1314
using System.Diagnostics.Contracts;
1415
using System.Linq;
16+
using System.Text;
1517
using System.Threading.Tasks;
1618
using Versioning;
1719
using static ApiVersion;
@@ -194,7 +196,7 @@ protected virtual void OnMultipleMatches( RouteContext context, ActionSelectionR
194196
Arg.NotNull( selectionResult, nameof( selectionResult ) );
195197

196198
var matchingActions = selectionResult.MatchingActions.OrderBy( kvp => kvp.Key ).SelectMany( kvp => kvp.Value ).Distinct();
197-
var actionNames = Join( NewLine, matchingActions.Select( match => match.Action.DisplayName ) );
199+
var actionNames = Join( NewLine, matchingActions.Select( ExpandActionSignature ) );
198200

199201
Logger.AmbiguousActions( actionNames );
200202

@@ -203,6 +205,48 @@ protected virtual void OnMultipleMatches( RouteContext context, ActionSelectionR
203205
throw new AmbiguousActionException( message );
204206
}
205207

208+
static string ExpandActionSignature( ActionDescriptorMatch match )
209+
{
210+
Contract.Requires( match != null );
211+
Contract.Ensures( !IsNullOrEmpty( Contract.Result<string>() ) );
212+
213+
if ( !( match.Action is ControllerActionDescriptor action ) )
214+
{
215+
return match.Action.DisplayName;
216+
}
217+
218+
var signature = new StringBuilder();
219+
var controllerType = action.ControllerTypeInfo;
220+
221+
signature.Append( controllerType.GetTypeDisplayName() );
222+
signature.Append( '.' );
223+
signature.Append( action.MethodInfo.Name );
224+
signature.Append( '(' );
225+
226+
using ( var parameter = action.Parameters.GetEnumerator() )
227+
{
228+
if ( parameter.MoveNext() )
229+
{
230+
var parameterType = parameter.Current.ParameterType;
231+
232+
signature.Append( parameterType.GetTypeDisplayName( false ) );
233+
234+
while ( parameter.MoveNext() )
235+
{
236+
parameterType = parameter.Current.ParameterType;
237+
signature.Append( ", " );
238+
signature.Append( parameterType.GetTypeDisplayName( false ) );
239+
}
240+
}
241+
}
242+
243+
signature.Append( ") (" );
244+
signature.Append( controllerType.Assembly.GetName().Name );
245+
signature.Append( ')' );
246+
247+
return signature.ToString();
248+
}
249+
206250
RequestHandler ClientError( HttpContext httpContext, ActionSelectionResult selectionResult )
207251
{
208252
Contract.Requires( httpContext != null );

src/Shared/Shared.projitems

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,5 +12,6 @@
1212
<Compile Include="$(MSBuildThisFileDirectory)Arg.cs" />
1313
<Compile Include="$(MSBuildThisFileDirectory)SharedAssemblyInfo.cs" />
1414
<Compile Include="$(MSBuildThisFileDirectory)StringExtensions.cs" />
15+
<Compile Include="$(MSBuildThisFileDirectory)TypeNameHelper.cs" />
1516
</ItemGroup>
1617
</Project>

src/Shared/TypeNameHelper.cs

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
namespace Microsoft
2+
{
3+
using System;
4+
using System.Collections.Generic;
5+
using System.Diagnostics.Contracts;
6+
using System.Text;
7+
8+
// REF: https://raw.githubusercontent.com/aspnet/Common/dev/shared/Microsoft.Extensions.TypeNameHelper.Sources/TypeNameHelper.cs
9+
static class TypeNameHelper
10+
{
11+
static readonly Dictionary<Type, string> BuiltInTypeNames = new Dictionary<Type, string>
12+
{
13+
[typeof( void )] = "void",
14+
[typeof( bool )] = "bool",
15+
[typeof( byte )] = "byte",
16+
[typeof( char )] = "char",
17+
[typeof( decimal )] = "decimal",
18+
[typeof( double )] = "double",
19+
[typeof( float )] = "float",
20+
[typeof( int )] = "int",
21+
[typeof( long )] = "long",
22+
[typeof( object )] = "object",
23+
[typeof( sbyte )] = "sbyte",
24+
[typeof( short )] = "short",
25+
[typeof( string )] = "string",
26+
[typeof( uint )] = "uint",
27+
[typeof( ulong )] = "ulong",
28+
[typeof( ushort )] = "ushort",
29+
};
30+
31+
internal static string GetTypeDisplayName( object item, bool fullName = true ) => item?.GetType().GetTypeDisplayName( fullName );
32+
33+
/// <summary>
34+
/// Pretty print a type name.
35+
/// </summary>
36+
/// <param name="type">The <see cref="Type"/>.</param>
37+
/// <param name="fullName"><c>true</c> to print a fully qualified name.</param>
38+
/// <param name="includeGenericParameterNames"><c>true</c> to include generic parameter names.</param>
39+
/// <returns>The pretty printed type name.</returns>
40+
internal static string GetTypeDisplayName( this Type type, bool fullName = true, bool includeGenericParameterNames = false )
41+
{
42+
Contract.Requires( type != null );
43+
44+
var builder = new StringBuilder();
45+
ProcessType( builder, type, new DisplayNameOptions( fullName, includeGenericParameterNames ) );
46+
return builder.ToString();
47+
}
48+
49+
static void ProcessType( StringBuilder builder, Type type, DisplayNameOptions options )
50+
{
51+
Contract.Requires( builder != null );
52+
Contract.Requires( type != null );
53+
54+
if ( type.IsGenericType )
55+
{
56+
var genericArguments = type.GetGenericArguments();
57+
ProcessGenericType( builder, type, genericArguments, genericArguments.Length, options );
58+
}
59+
else if ( type.IsArray )
60+
{
61+
ProcessArrayType( builder, type, options );
62+
}
63+
else if ( BuiltInTypeNames.TryGetValue( type, out var builtInName ) )
64+
{
65+
builder.Append( builtInName );
66+
}
67+
else if ( type.IsGenericParameter )
68+
{
69+
if ( options.IncludeGenericParameterNames )
70+
{
71+
builder.Append( type.Name );
72+
}
73+
}
74+
else
75+
{
76+
builder.Append( options.FullName ? type.FullName : type.Name );
77+
}
78+
}
79+
80+
static void ProcessArrayType( StringBuilder builder, Type type, DisplayNameOptions options )
81+
{
82+
Contract.Requires( builder != null );
83+
Contract.Requires( type != null );
84+
85+
var innerType = type;
86+
87+
while ( innerType.IsArray )
88+
{
89+
innerType = innerType.GetElementType();
90+
}
91+
92+
ProcessType( builder, innerType, options );
93+
94+
while ( type.IsArray )
95+
{
96+
builder.Append( '[' );
97+
builder.Append( ',', type.GetArrayRank() - 1 );
98+
builder.Append( ']' );
99+
type = type.GetElementType();
100+
}
101+
}
102+
103+
static void ProcessGenericType( StringBuilder builder, Type type, Type[] genericArguments, int length, DisplayNameOptions options )
104+
{
105+
Contract.Requires( builder != null );
106+
Contract.Requires( type != null );
107+
Contract.Requires( genericArguments != null );
108+
Contract.Requires( length > 0 );
109+
110+
var offset = 0;
111+
112+
if ( type.IsNested )
113+
{
114+
offset = type.DeclaringType.GetGenericArguments().Length;
115+
}
116+
117+
if ( options.FullName )
118+
{
119+
if ( type.IsNested )
120+
{
121+
ProcessGenericType( builder, type.DeclaringType, genericArguments, offset, options );
122+
builder.Append( '+' );
123+
}
124+
else if ( !string.IsNullOrEmpty( type.Namespace ) )
125+
{
126+
builder.Append( type.Namespace );
127+
builder.Append( '.' );
128+
}
129+
}
130+
131+
var genericPartIndex = type.Name.IndexOf( '`' );
132+
133+
if ( genericPartIndex <= 0 )
134+
{
135+
builder.Append( type.Name );
136+
return;
137+
}
138+
139+
builder.Append( type.Name, 0, genericPartIndex );
140+
builder.Append( '<' );
141+
142+
for ( var i = offset; i < length; i++ )
143+
{
144+
ProcessType( builder, genericArguments[i], options );
145+
146+
if ( i + 1 == length )
147+
{
148+
continue;
149+
}
150+
151+
builder.Append( ',' );
152+
153+
if ( options.IncludeGenericParameterNames || !genericArguments[i + 1].IsGenericParameter )
154+
{
155+
builder.Append( ' ' );
156+
}
157+
}
158+
159+
builder.Append( '>' );
160+
}
161+
162+
struct DisplayNameOptions
163+
{
164+
internal DisplayNameOptions( bool fullName, bool includeGenericParameterNames )
165+
{
166+
FullName = fullName;
167+
IncludeGenericParameterNames = includeGenericParameterNames;
168+
}
169+
170+
public bool FullName { get; }
171+
172+
public bool IncludeGenericParameterNames { get; }
173+
}
174+
}
175+
}

test/Microsoft.AspNetCore.Mvc.Versioning.Tests/Versioning/ApiVersionActionSelectorTest.cs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -313,8 +313,8 @@ public void select_best_candidate_should_throw_exception_for_ambiguously_version
313313
{
314314
// arrange
315315
var message = $"Multiple actions matched. The following actions matched route data and had all constraints satisfied:{NewLine}{NewLine}" +
316-
$"Microsoft.AspNetCore.Mvc.Versioning.AttributeRoutedAmbiguous2Controller.Get (Microsoft.AspNetCore.Mvc.Versioning.Tests){NewLine}" +
317-
$"Microsoft.AspNetCore.Mvc.Versioning.AttributeRoutedAmbiguous3Controller.Get (Microsoft.AspNetCore.Mvc.Versioning.Tests)";
316+
$"Microsoft.AspNetCore.Mvc.Versioning.AttributeRoutedAmbiguous2Controller.Get() (Microsoft.AspNetCore.Mvc.Versioning.Tests){NewLine}" +
317+
$"Microsoft.AspNetCore.Mvc.Versioning.AttributeRoutedAmbiguous3Controller.Get() (Microsoft.AspNetCore.Mvc.Versioning.Tests)";
318318

319319
using ( var server = new WebServer( o => o.AssumeDefaultVersionWhenUnspecified = true ) )
320320
{
@@ -334,8 +334,8 @@ public void select_best_candidate_should_throw_exception_for_ambiguously_version
334334
Action<ApiVersioningOptions> versioningSetup = o => o.AssumeDefaultVersionWhenUnspecified = true;
335335
Action<IRouteBuilder> routesSetup = r => r.MapRoute( "default", "api/{controller}/{action=Get}/{id?}" );
336336
var message = $"Multiple actions matched. The following actions matched route data and had all constraints satisfied:{NewLine}{NewLine}" +
337-
$"Microsoft.AspNetCore.Mvc.Versioning.AmbiguousToo2Controller.Get (Microsoft.AspNetCore.Mvc.Versioning.Tests){NewLine}" +
338-
$"Microsoft.AspNetCore.Mvc.Versioning.AmbiguousTooController.Get (Microsoft.AspNetCore.Mvc.Versioning.Tests)";
337+
$"Microsoft.AspNetCore.Mvc.Versioning.AmbiguousToo2Controller.Get() (Microsoft.AspNetCore.Mvc.Versioning.Tests){NewLine}" +
338+
$"Microsoft.AspNetCore.Mvc.Versioning.AmbiguousTooController.Get() (Microsoft.AspNetCore.Mvc.Versioning.Tests)";
339339

340340
using ( var server = new WebServer( versioningSetup, routesSetup ) )
341341
{
@@ -353,8 +353,8 @@ public void select_best_candidate_should_throw_exception_for_ambiguous_neutral_a
353353
{
354354
// arrange
355355
var message = $"Multiple actions matched. The following actions matched route data and had all constraints satisfied:{NewLine}{NewLine}" +
356-
$"Microsoft.AspNetCore.Mvc.Versioning.AttributeRoutedAmbiguousNeutralController.Get (Microsoft.AspNetCore.Mvc.Versioning.Tests){NewLine}" +
357-
$"Microsoft.AspNetCore.Mvc.Versioning.AttributeRoutedAmbiguousController.Get (Microsoft.AspNetCore.Mvc.Versioning.Tests)";
356+
$"Microsoft.AspNetCore.Mvc.Versioning.AttributeRoutedAmbiguousNeutralController.Get() (Microsoft.AspNetCore.Mvc.Versioning.Tests){NewLine}" +
357+
$"Microsoft.AspNetCore.Mvc.Versioning.AttributeRoutedAmbiguousController.Get() (Microsoft.AspNetCore.Mvc.Versioning.Tests)";
358358

359359
using ( var server = new WebServer( o => o.AssumeDefaultVersionWhenUnspecified = true ) )
360360
{
@@ -374,8 +374,8 @@ public void select_best_candidate_should_throw_exception_for_ambiguous_neutral_a
374374
Action<ApiVersioningOptions> versioningSetup = o => o.AssumeDefaultVersionWhenUnspecified = true;
375375
Action<IRouteBuilder> routesSetup = r => r.MapRoute( "default", "api/{controller}/{action=Get}/{id?}" );
376376
var message = $"Multiple actions matched. The following actions matched route data and had all constraints satisfied:{NewLine}{NewLine}" +
377-
$"Microsoft.AspNetCore.Mvc.Versioning.AmbiguousNeutralController.Get (Microsoft.AspNetCore.Mvc.Versioning.Tests){NewLine}" +
378-
$"Microsoft.AspNetCore.Mvc.Versioning.AmbiguousController.Get (Microsoft.AspNetCore.Mvc.Versioning.Tests)";
377+
$"Microsoft.AspNetCore.Mvc.Versioning.AmbiguousNeutralController.Get() (Microsoft.AspNetCore.Mvc.Versioning.Tests){NewLine}" +
378+
$"Microsoft.AspNetCore.Mvc.Versioning.AmbiguousController.Get() (Microsoft.AspNetCore.Mvc.Versioning.Tests)";
379379

380380
using ( var server = new WebServer( versioningSetup, routesSetup ) )
381381
{

0 commit comments

Comments
 (0)