Skip to content

Commit df123f1

Browse files
captainsafiacaptainsafia
authored
Favor parameter name in route pattern when available (#41701)
* Favor parameter name in route pattern when available * Address feedback from peer review
1 parent 62ca84b commit df123f1

File tree

5 files changed

+82
-14
lines changed

5 files changed

+82
-14
lines changed

‎src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string
183183
var nullabilityContext = new NullabilityInfoContext();
184184
var nullability = nullabilityContext.Create(parameter);
185185
var isOptional = parameter.HasDefaultValue || nullability.ReadState != NullabilityState.NotNull || allowEmpty;
186-
var parameterDescriptor = CreateParameterDescriptor(parameter);
186+
var parameterDescriptor = CreateParameterDescriptor(parameter, pattern);
187187
var routeInfo = CreateParameterRouteInfo(pattern, parameter, isOptional);
188188

189189
return new ApiParameterDescription
@@ -199,13 +199,17 @@ private ApiDescription CreateApiDescription(RouteEndpoint routeEndpoint, string
199199
};
200200
}
201201

202-
private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo parameter)
203-
=> new EndpointParameterDescriptor
202+
private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo parameter, RoutePattern pattern)
203+
{
204+
var parameterName = parameter.Name ?? string.Empty;
205+
var name = pattern.GetParameter(parameterName)?.Name ?? parameterName;
206+
return new EndpointParameterDescriptor
204207
{
205-
Name = parameter.Name ?? string.Empty,
208+
Name = name,
206209
ParameterInfo = parameter,
207210
ParameterType = parameter.ParameterType,
208211
};
212+
}
209213

210214
private ApiParameterRouteInfo? CreateParameterRouteInfo(RoutePattern pattern, ParameterInfo parameter, bool isOptional)
211215
{
@@ -250,7 +254,9 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param
250254

251255
if (attributes.OfType<IFromRouteMetadata>().FirstOrDefault() is { } routeAttribute)
252256
{
253-
return (BindingSource.Path, routeAttribute.Name ?? parameter.Name ?? string.Empty, false, parameter.ParameterType);
257+
var parameterName = parameter.Name ?? string.Empty;
258+
var name = pattern.GetParameter(parameterName)?.Name ?? parameterName;
259+
return (BindingSource.Path, routeAttribute.Name ?? name, false, parameter.ParameterType);
254260
}
255261
else if (attributes.OfType<IFromQueryMetadata>().FirstOrDefault() is { } queryAttribute)
256262
{
@@ -285,9 +291,9 @@ private static ParameterDescriptor CreateParameterDescriptor(ParameterInfo param
285291
var displayType = !parameter.ParameterType.IsPrimitive && Nullable.GetUnderlyingType(parameter.ParameterType)?.IsPrimitive != true
286292
? typeof(string) : parameter.ParameterType;
287293
// Path vs query cannot be determined by RequestDelegateFactory at startup currently because of the layering, but can be done here.
288-
if (parameter.Name is { } name && pattern.GetParameter(name) is not null)
294+
if (parameter.Name is { } name && pattern.GetParameter(name) is { } routeParam)
289295
{
290-
return (BindingSource.Path, name, false, displayType);
296+
return (BindingSource.Path, routeParam.Name, false, displayType);
291297
}
292298
else
293299
{

‎src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -450,13 +450,13 @@ public void AddsMultipleParameters()
450450
[Fact]
451451
public void AddsMultipleParametersFromParametersAttribute()
452452
{
453-
static void AssertParameters(ApiDescription apiDescription)
453+
static void AssertParameters(ApiDescription apiDescription, string capturedName = "Foo")
454454
{
455455
Assert.Collection(
456456
apiDescription.ParameterDescriptions,
457457
param =>
458458
{
459-
Assert.Equal("Foo", param.Name);
459+
Assert.Equal(capturedName, param.Name);
460460
Assert.Equal(typeof(int), param.ModelMetadata.ModelType);
461461
Assert.Equal(BindingSource.Path, param.Source);
462462
Assert.True(param.IsRequired);
@@ -485,7 +485,7 @@ static void AssertParameters(ApiDescription apiDescription)
485485
AssertParameters(GetApiDescription(([AsParameters] ArgumentListRecord req) => { }));
486486
AssertParameters(GetApiDescription(([AsParameters] ArgumentListRecordStruct req) => { }));
487487
AssertParameters(GetApiDescription(([AsParameters] ArgumentListRecordWithoutPositionalParameters req) => { }));
488-
AssertParameters(GetApiDescription(([AsParameters] ArgumentListRecordWithoutAttributes req) => { }, "/{foo}"));
488+
AssertParameters(GetApiDescription(([AsParameters] ArgumentListRecordWithoutAttributes req) => { }, "/{foo}"), "foo");
489489
AssertParameters(GetApiDescription(([AsParameters] ArgumentListRecordWithoutAttributes req) => { }, "/{Foo}"));
490490
}
491491

@@ -1250,6 +1250,34 @@ public void HandlesEndpointWithDescriptionAndSummary_WithAttributes()
12501250
Assert.Equal("A summary", summaryMetadata.Summary);
12511251
}
12521252

1253+
[Theory]
1254+
[InlineData("/todos/{id}", "id")]
1255+
[InlineData("/todos/{Id}", "Id")]
1256+
[InlineData("/todos/{id:minlen(2)}", "id")]
1257+
public void FavorsParameterCasingInRoutePattern(string pattern, string expectedName)
1258+
{
1259+
var builder = CreateBuilder();
1260+
builder.MapGet(pattern, (int Id) => "");
1261+
1262+
var context = new ApiDescriptionProviderContext(Array.Empty<ActionDescriptor>());
1263+
1264+
var endpointDataSource = builder.DataSources.OfType<EndpointDataSource>().Single();
1265+
var hostEnvironment = new HostEnvironment
1266+
{
1267+
ApplicationName = nameof(EndpointMetadataApiDescriptionProviderTest)
1268+
};
1269+
var provider = CreateEndpointMetadataApiDescriptionProvider(endpointDataSource);
1270+
1271+
// Act
1272+
provider.OnProvidersExecuting(context);
1273+
1274+
// Assert
1275+
var apiDescription = Assert.Single(context.Results);
1276+
var parameter = Assert.Single(apiDescription.ParameterDescriptions);
1277+
Assert.Equal(expectedName, parameter.Name);
1278+
Assert.Equal(expectedName, parameter.ParameterDescriptor.Name);
1279+
}
1280+
12531281
private static IEnumerable<string> GetSortedMediaTypes(ApiResponseType apiResponseType)
12541282
{
12551283
return apiResponseType.ApiResponseFormats

‎src/OpenApi/src/OpenApiGenerator.cs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,11 @@ private List<OpenApiParameter> GetOpenApiParameters(MethodInfo methodInfo, Endpo
354354

355355
foreach (var parameter in parameters)
356356
{
357+
if (parameter.Name is null)
358+
{
359+
throw new InvalidOperationException($"Encountered a parameter of type '{parameter.ParameterType}' without a name. Parameters must have a name.");
360+
}
361+
357362
var (isBodyOrFormParameter, parameterLocation) = GetOpenApiParameterLocation(parameter, pattern, disableInferredBody);
358363

359364
// If the parameter isn't something that would be populated in RequestBody
@@ -367,9 +372,10 @@ private List<OpenApiParameter> GetOpenApiParameters(MethodInfo methodInfo, Endpo
367372
var nullabilityContext = new NullabilityInfoContext();
368373
var nullability = nullabilityContext.Create(parameter);
369374
var isOptional = parameter.HasDefaultValue || nullability.ReadState != NullabilityState.NotNull;
375+
var name = pattern.GetParameter(parameter.Name) is { } routeParameter ? routeParameter.Name : parameter.Name;
370376
var openApiParameter = new OpenApiParameter()
371377
{
372-
Name = parameter.Name,
378+
Name = name,
373379
In = parameterLocation,
374380
Content = GetOpenApiParameterContent(metadata),
375381
Schema = OpenApiSchemaGenerator.GetOpenApiSchema(parameter.ParameterType),

‎src/OpenApi/test/OpenApiGeneratorTests.cs

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4+
using System.Linq.Expressions;
45
using System.Reflection;
56
using System.Security.Claims;
67
using Microsoft.AspNetCore.Http;
@@ -49,6 +50,15 @@ public void UsesApplicationNameAsOperationTagsIfNoDeclaringType()
4950
Assert.Equal(declaringTypeName, tag.Name);
5051
}
5152

53+
[Fact]
54+
public void ThrowsInvalidOperationExceptionGivenUnnamedParameter()
55+
{
56+
var unnamedParameter = Expression.Parameter(typeof(int));
57+
var lambda = Expression.Lambda(Expression.Block(), unnamedParameter);
58+
var ex = Assert.Throws<InvalidOperationException>(() => GetOpenApiOperation(lambda.Compile()));
59+
Assert.Equal("Encountered a parameter of type 'System.Runtime.CompilerServices.Closure' without a name. Parameters must have a name.", ex.Message);
60+
}
61+
5262
[Fact]
5363
public void AddsRequestFormatFromMetadata()
5464
{
@@ -357,13 +367,13 @@ public void AddsMultipleParameters()
357367
[Fact]
358368
public void AddsMultipleParametersFromParametersAttribute()
359369
{
360-
static void AssertParameters(OpenApiOperation operation)
370+
static void AssertParameters(OpenApiOperation operation, string capturedName = "Foo")
361371
{
362372
Assert.Collection(
363373
operation.Parameters,
364374
param =>
365375
{
366-
Assert.Equal("Foo", param.Name);
376+
Assert.Equal(capturedName, param.Name);
367377
Assert.Equal("integer", param.Schema.Type);
368378
Assert.Equal(ParameterLocation.Path, param.In);
369379
Assert.True(param.Required);
@@ -391,7 +401,7 @@ static void AssertParameters(OpenApiOperation operation)
391401
AssertParameters(GetOpenApiOperation(([AsParameters] ArgumentListRecord req) => { }));
392402
AssertParameters(GetOpenApiOperation(([AsParameters] ArgumentListRecordStruct req) => { }));
393403
AssertParameters(GetOpenApiOperation(([AsParameters] ArgumentListRecordWithoutPositionalParameters req) => { }));
394-
AssertParameters(GetOpenApiOperation(([AsParameters] ArgumentListRecordWithoutAttributes req) => { }, "/{foo}"));
404+
AssertParameters(GetOpenApiOperation(([AsParameters] ArgumentListRecordWithoutAttributes req) => { }, "/{foo}"), "foo");
395405
AssertParameters(GetOpenApiOperation(([AsParameters] ArgumentListRecordWithoutAttributes req) => { }, "/{Foo}"));
396406
}
397407

@@ -787,6 +797,19 @@ public void HandlesEndpointWithMultipleResponses()
787797
Assert.Equal("object", content.Value.Schema.Type);
788798
Assert.Equal("200", response.Key);
789799
Assert.Equal("application/json", content.Key);
800+
801+
}
802+
803+
[Theory]
804+
[InlineData("/todos/{id}", "id")]
805+
[InlineData("/todos/{Id}", "Id")]
806+
[InlineData("/todos/{id:minlen(2)}", "id")]
807+
public void FavorsParameterCasingInRoutePattern(string pattern, string expectedName)
808+
{
809+
var operation = GetOpenApiOperation((int Id) => "", pattern);
810+
811+
var param = Assert.Single(operation.Parameters);
812+
Assert.Equal(expectedName, param.Name);
790813
}
791814

792815
private static OpenApiOperation GetOpenApiOperation(

‎src/Shared/PropertyAsParameterInfo.cs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,11 @@ public static ReadOnlySpan<ParameterInfo> Flatten(ParameterInfo[] parameters, Pa
7474

7575
for (var i = 0; i < parameters.Length; i++)
7676
{
77+
if (parameters[i].Name is null)
78+
{
79+
throw new InvalidOperationException($"Encountered a parameter of type '{parameters[i].ParameterType}' without a name. Parameters must have a name.");
80+
}
81+
7782
if (parameters[i].CustomAttributes.Any(a => a.AttributeType == typeof(AsParametersAttribute)))
7883
{
7984
// Initialize the list with all parameter already processed

0 commit comments

Comments
 (0)