Skip to content

Commit 4d5477f

Browse files
captainsafiacaptainsafia
committed
Address feedback from peer review
1 parent 3097baa commit 4d5477f

File tree

6 files changed

+120
-79
lines changed

6 files changed

+120
-79
lines changed

‎src/Http/Http.Extensions/gen/StaticRouteHandlerModel/Endpoint.cs

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@ public Endpoint(IInvocationOperation operation, WellKnownTypes wellKnownTypes, S
6868
switch (parameter.Source)
6969
{
7070
case EndpointParameterSource.BindAsync:
71-
IsAwaitable = true;
7271
switch (parameter.BindMethod)
7372
{
7473
case BindabilityMethod.IBindableFromHttpContext:
@@ -80,10 +79,8 @@ public Endpoint(IInvocationOperation operation, WellKnownTypes wellKnownTypes, S
8079
case EndpointParameterSource.JsonBody:
8180
case EndpointParameterSource.JsonBodyOrService:
8281
case EndpointParameterSource.FormBody:
83-
IsAwaitable = true;
8482
break;
8583
case EndpointParameterSource.AsParameters:
86-
IsAwaitable = parameter.EndpointParameters?.Any(p => p.Source is EndpointParameterSource.JsonBody or EndpointParameterSource.FormBody or EndpointParameterSource.JsonBodyOrService) ?? false;
8784
break;
8885
case EndpointParameterSource.Unknown:
8986
Diagnostics.Add(Diagnostic.Create(
@@ -100,14 +97,13 @@ public Endpoint(IInvocationOperation operation, WellKnownTypes wellKnownTypes, S
10097

10198
EmitterContext.HasEndpointParameterMetadataProvider = Parameters.Any(p => p.IsEndpointParameterMetadataProvider);
10299
EmitterContext.HasEndpointMetadataProvider = Response!.IsEndpointMetadataProvider || Parameters.Any(p => p.IsEndpointMetadataProvider || p.IsEndpointParameterMetadataProvider);
103-
EmitterContext.HasParsable = Parameters.Any(parameter => parameter.IsParsable || (parameter.Source == EndpointParameterSource.AsParameters && parameter.EndpointParameters.Any(p => p.IsParsable)));
104100
EmitterContext.RequiresLoggingHelper = !Parameters.All(parameter =>
105101
parameter.Source == EndpointParameterSource.SpecialType ||
106102
parameter is { IsArray: true, ElementType.SpecialType: SpecialType.System_String, Source: EndpointParameterSource.Query });
107103
}
108104

109105
public string HttpMethod { get; }
110-
public bool IsAwaitable { get; }
106+
public bool IsAwaitable { get; set; }
111107
public bool NeedsParameterArray { get; }
112108
public string? RoutePattern { get; }
113109
public EmitterContext EmitterContext { get; }

‎src/Http/Http.Extensions/gen/StaticRouteHandlerModel/EndpointParameter.cs

Lines changed: 42 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -18,33 +18,20 @@ namespace Microsoft.AspNetCore.Http.RequestDelegateGenerator.StaticRouteHandlerM
1818

1919
internal class EndpointParameter
2020
{
21-
public EndpointParameter(Endpoint endpoint, IParameterSymbol parameter, WellKnownTypes wellKnownTypes)
21+
public EndpointParameter(Endpoint endpoint, IParameterSymbol parameter, WellKnownTypes wellKnownTypes): this(endpoint, parameter.Type, parameter.Name, wellKnownTypes)
2222
{
23-
Type = parameter.Type;
24-
SymbolName = parameter.Name;
25-
LookupName = parameter.Name; // Default lookup name is same as parameter name (which is a valid C# identifier).
2623
Ordinal = parameter.Ordinal;
27-
Source = EndpointParameterSource.Unknown;
2824
IsOptional = parameter.IsOptional();
2925
DefaultValue = parameter.GetDefaultValueString();
30-
IsArray = TryGetArrayElementType(Type, out var elementType);
31-
ElementType = elementType;
32-
IsEndpointMetadataProvider = ImplementsIEndpointMetadataProvider(parameter, wellKnownTypes);
33-
IsEndpointParameterMetadataProvider = ImplementsIEndpointParameterMetadataProvider(parameter, wellKnownTypes);
3426
ProcessEndpointParameterSource(endpoint, parameter, parameter.GetAttributes(), wellKnownTypes);
3527
}
3628

37-
public EndpointParameter(Endpoint endpoint, IPropertySymbol property, IParameterSymbol? parameter, WellKnownTypes wellKnownTypes)
29+
private EndpointParameter(Endpoint endpoint, IPropertySymbol property, IParameterSymbol? parameter, WellKnownTypes wellKnownTypes) : this(endpoint, property.Type, property.Name, wellKnownTypes)
3830
{
39-
Type = property.Type;
40-
SymbolName = property.Name;
41-
LookupName = property.Name;
4231
Ordinal = parameter?.Ordinal ?? 0;
43-
Source = EndpointParameterSource.Unknown;
4432
IsOptional = property.IsOptional() || parameter?.IsOptional() == true;
4533
DefaultValue = parameter?.GetDefaultValueString() ?? "null";
46-
IsArray = TryGetArrayElementType(Type, out var elementType);
47-
ElementType = elementType;
34+
// Coalesce attributes on the property and attributes on the matching parameter
4835
var attributeBuilder = ImmutableArray.CreateBuilder<AttributeData>();
4936
attributeBuilder.AddRange(property.GetAttributes());
5037
if (parameter is not null)
@@ -54,31 +41,45 @@ public EndpointParameter(Endpoint endpoint, IPropertySymbol property, IParameter
5441
ProcessEndpointParameterSource(endpoint, property, attributeBuilder.ToImmutable(), wellKnownTypes);
5542
}
5643

44+
private EndpointParameter(Endpoint endpoint, ITypeSymbol typeSymbol, string parameterName, WellKnownTypes wellKnownTypes)
45+
{
46+
Type = typeSymbol;
47+
SymbolName = parameterName;
48+
LookupName = parameterName;
49+
Source = EndpointParameterSource.Unknown;
50+
IsArray = TryGetArrayElementType(typeSymbol, out var elementType);
51+
ElementType = elementType;
52+
IsEndpointMetadataProvider = ImplementsIEndpointMetadataProvider(typeSymbol, wellKnownTypes);
53+
IsEndpointParameterMetadataProvider = ImplementsIEndpointParameterMetadataProvider(typeSymbol, wellKnownTypes);
54+
endpoint.EmitterContext.HasEndpointParameterMetadataProvider = IsEndpointParameterMetadataProvider;
55+
}
56+
5757
private void ProcessEndpointParameterSource(Endpoint endpoint, ISymbol symbol, ImmutableArray<AttributeData> attributes, WellKnownTypes wellKnownTypes)
5858
{
59-
if (attributes.HasAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromRouteMetadata), out var fromRouteAttribute))
59+
if (attributes.TryGetAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromRouteMetadata), out var fromRouteAttribute))
6060
{
6161
Source = EndpointParameterSource.Route;
6262
LookupName = GetEscapedParameterName(fromRouteAttribute, symbol.Name);
6363
IsParsable = TryGetParsability(Type, wellKnownTypes, out var parsingBlockEmitter);
6464
ParsingBlockEmitter = parsingBlockEmitter;
6565
}
66-
else if (attributes.HasAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromQueryMetadata), out var fromQueryAttribute))
66+
else if (attributes.TryGetAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromQueryMetadata), out var fromQueryAttribute))
6767
{
6868
Source = EndpointParameterSource.Query;
6969
LookupName = GetEscapedParameterName(fromQueryAttribute, symbol.Name);
7070
IsParsable = TryGetParsability(Type, wellKnownTypes, out var parsingBlockEmitter);
7171
ParsingBlockEmitter = parsingBlockEmitter;
7272
}
73-
else if (attributes.HasAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromHeaderMetadata), out var fromHeaderAttribute))
73+
else if (attributes.TryGetAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromHeaderMetadata), out var fromHeaderAttribute))
7474
{
7575
Source = EndpointParameterSource.Header;
7676
LookupName = GetEscapedParameterName(fromHeaderAttribute, symbol.Name);
7777
IsParsable = TryGetParsability(Type, wellKnownTypes, out var parsingBlockEmitter);
7878
ParsingBlockEmitter = parsingBlockEmitter;
7979
}
80-
else if (attributes.HasAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromFormMetadata), out var fromFormAttribute))
80+
else if (attributes.TryGetAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromFormMetadata), out var fromFormAttribute))
8181
{
82+
endpoint.IsAwaitable = true;
8283
Source = EndpointParameterSource.FormBody;
8384
LookupName = GetEscapedParameterName(fromFormAttribute, symbol.Name);
8485
if (SymbolEqualityComparer.Default.Equals(Type, wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_IFormFileCollection)))
@@ -114,11 +115,12 @@ private void ProcessEndpointParameterSource(Endpoint endpoint, ISymbol symbol, I
114115
}
115116
else
116117
{
118+
endpoint.IsAwaitable = true;
117119
Source = EndpointParameterSource.JsonBody;
118120
}
119121
IsOptional = isOptional;
120122
}
121-
else if (attributes.HasAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromServiceMetadata)))
123+
else if (attributes.TryGetAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromServiceMetadata)))
122124
{
123125
Source = EndpointParameterSource.Service;
124126
}
@@ -151,24 +153,28 @@ Type is not INamedTypeSymbol namedTypeSymbol ||
151153
}
152154
else if (SymbolEqualityComparer.Default.Equals(Type, wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_IFormFileCollection)))
153155
{
156+
endpoint.IsAwaitable = true;
154157
Source = EndpointParameterSource.FormBody;
155158
LookupName = symbol.Name;
156159
AssigningCode = "httpContext.Request.Form.Files";
157160
}
158161
else if (SymbolEqualityComparer.Default.Equals(Type, wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_IFormFile)))
159162
{
163+
endpoint.IsAwaitable = true;
160164
Source = EndpointParameterSource.FormBody;
161165
LookupName = symbol.Name;
162166
AssigningCode = $"httpContext.Request.Form.Files[{SymbolDisplay.FormatLiteral(LookupName, true)}]";
163167
}
164168
else if (SymbolEqualityComparer.Default.Equals(Type, wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_IFormCollection)))
165169
{
170+
endpoint.IsAwaitable = true;
166171
Source = EndpointParameterSource.FormBody;
167172
LookupName = symbol.Name;
168173
AssigningCode = "httpContext.Request.Form";
169174
}
170175
else if (HasBindAsync(Type, wellKnownTypes, out var bindMethod))
171176
{
177+
endpoint.IsAwaitable = true;
172178
Source = EndpointParameterSource.BindAsync;
173179
BindMethod = bindMethod;
174180
}
@@ -189,20 +195,21 @@ Type is not INamedTypeSymbol namedTypeSymbol ||
189195
{
190196
Source = EndpointParameterSource.RouteOrQuery;
191197
IsParsable = true;
192-
endpoint.EmitterContext.HasParsable = true;
193198
ParsingBlockEmitter = parsingBlockEmitter;
194199
}
195200
else
196201
{
202+
endpoint.IsAwaitable = true;
197203
Source = EndpointParameterSource.JsonBodyOrService;
198204
}
205+
endpoint.EmitterContext.HasParsable |= IsParsable;
199206
}
200207

201-
private static bool ImplementsIEndpointMetadataProvider(IParameterSymbol parameter, WellKnownTypes wellKnownTypes)
202-
=> parameter.Type.Implements(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IEndpointMetadataProvider));
208+
private static bool ImplementsIEndpointMetadataProvider(ITypeSymbol type, WellKnownTypes wellKnownTypes)
209+
=> type.Implements(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IEndpointMetadataProvider));
203210

204-
private static bool ImplementsIEndpointParameterMetadataProvider(IParameterSymbol parameter, WellKnownTypes wellKnownTypes)
205-
=> parameter.Type.Implements(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IEndpointParameterMetadataProvider));
211+
private static bool ImplementsIEndpointParameterMetadataProvider(ITypeSymbol type, WellKnownTypes wellKnownTypes)
212+
=> type.Implements(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IEndpointParameterMetadataProvider));
206213

207214
private static bool ShouldDisableInferredBodyParameters(string httpMethod)
208215
{
@@ -220,11 +227,11 @@ private static bool ShouldDisableInferredBodyParameters(string httpMethod)
220227
public bool IsEndpointMetadataProvider { get; }
221228
public bool IsEndpointParameterMetadataProvider { get; }
222229
public string SymbolName { get; }
223-
public string LookupName { get; set; }
230+
public string LookupName { get; set; }
224231
public int Ordinal { get; }
225232
public bool IsOptional { get; set; }
226233
public bool IsArray { get; set; }
227-
public string DefaultValue { get; set; }
234+
public string DefaultValue { get; set; } = "null";
228235

229236
public EndpointParameterSource Source { get; set; }
230237

@@ -415,7 +422,7 @@ private static bool TryGetExplicitFromJsonBody(ISymbol typeSymbol,
415422
out bool isOptional)
416423
{
417424
isOptional = false;
418-
if (!attributes.HasAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromBodyMetadata), out var fromBodyAttribute))
425+
if (!attributes.TryGetAttributeImplementingInterface(wellKnownTypes.Get(WellKnownType.Microsoft_AspNetCore_Http_Metadata_IFromBodyMetadata), out var fromBodyAttribute))
419426
{
420427
return false;
421428
}
@@ -455,11 +462,14 @@ private static bool TryGetAsParametersConstructor(INamedTypeSymbol type, out boo
455462

456463
var constructors = type.Constructors.Where(constructor => constructor.DeclaredAccessibility == Accessibility.Public && !constructor.IsStatic);
457464
var numOfConstructors = constructors.Count();
465+
// When leveraging parameterless constructors, we want to ensure we only emit for writable
466+
// properties. We do not have this constraint if we are leveraging a parameterized constructor.
467+
var properties = type.GetMembers().OfType<IPropertySymbol>().Where(property => property.DeclaredAccessibility == Accessibility.Public);
468+
var writableProperties = properties.Where(property => !property.IsReadOnly);
458469

459470
if (numOfConstructors == 1)
460471
{
461472
var targetConstructor = constructors.SingleOrDefault();
462-
var properties = type.GetMembers().OfType<IPropertySymbol>().Where(property => property.DeclaredAccessibility == Accessibility.Public);
463473
var lookupTable = new Dictionary<ParameterLookupKey, IPropertySymbol>();
464474
foreach (var property in properties)
465475
{
@@ -472,7 +482,7 @@ private static bool TryGetAsParametersConstructor(INamedTypeSymbol type, out boo
472482
if (parameters.Length == 0)
473483
{
474484
isParameterlessConstructor = true;
475-
matchedParameters = properties.Select(property => new ConstructorParameter(property, null));
485+
matchedParameters = writableProperties.Select(property => new ConstructorParameter(property, null));
476486
return true;
477487
}
478488

@@ -497,7 +507,7 @@ private static bool TryGetAsParametersConstructor(INamedTypeSymbol type, out boo
497507
if (type.IsValueType)
498508
{
499509
isParameterlessConstructor = true;
500-
matchedParameters = type.GetMembers().OfType<IPropertySymbol>().Select(property => new ConstructorParameter(property, null));
510+
matchedParameters = writableProperties.Select(property => new ConstructorParameter(property, null));
501511
return true;
502512
}
503513

‎src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs

Lines changed: 37 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
using System.Text.Json.Serialization;
2222
using System.Text.Json.Serialization.Metadata;
2323
using Microsoft.AspNetCore.Builder;
24+
using Microsoft.AspNetCore.Http.Generators.Tests;
2425
using Microsoft.AspNetCore.Http;
2526
using Microsoft.AspNetCore.Http.Features;
2627
using Microsoft.AspNetCore.Http.HttpResults;
@@ -2750,42 +2751,42 @@ void TestAction([FromForm(Name = "foo")] IFormCollection formCollection)
27502751
Assert.Equal("Assigning a value to the IFromFormMetadata.Name property is not supported for parameters of type IFormCollection.", nse.Message);
27512752
}
27522753

2753-
// public static object[][] NullableFromParameterListActions
2754-
// {
2755-
// get
2756-
// {
2757-
// void TestParameterListRecordStruct([AsParameters] ParameterListRecordStruct? args)
2758-
// { }
2759-
//
2760-
// void TestParameterListRecordClass([AsParameters] ParameterListRecordClass? args)
2761-
// { }
2762-
//
2763-
// void TestParameterListStruct([AsParameters] ParameterListStruct? args)
2764-
// { }
2765-
//
2766-
// void TestParameterListClass([AsParameters] ParameterListClass? args)
2767-
// { }
2768-
//
2769-
// return new[]
2770-
// {
2771-
// new object[] { (Action<ParameterListRecordStruct?>)TestParameterListRecordStruct },
2772-
// new object[] { (Action<ParameterListRecordClass?>)TestParameterListRecordClass },
2773-
// new object[] { (Action<ParameterListStruct?>)TestParameterListStruct },
2774-
// new object[] { (Action<ParameterListClass?>)TestParameterListClass },
2775-
// };
2776-
// }
2777-
// }
2778-
//
2779-
// [Theory]
2780-
// [MemberData(nameof(NullableFromParameterListActions))]
2781-
// public void RequestDelegateThrowsWhenNullableParameterList(Delegate action)
2782-
// {
2783-
// var parameter = action.Method.GetParameters()[0];
2784-
// var httpContext = CreateHttpContext();
2785-
//
2786-
// var exception = Assert.Throws<InvalidOperationException>(() => RequestDelegateFactory.Create(action));
2787-
// Assert.Contains($"The nullable type '{TypeNameHelper.GetTypeDisplayName(parameter.ParameterType, fullName: false)}' is not supported, mark the parameter as non-nullable.", exception.Message);
2788-
// }
2754+
public static object[][] NullableFromParameterListActions
2755+
{
2756+
get
2757+
{
2758+
void TestParameterListRecordStruct([AsParameters] ParameterListRecordStruct? args)
2759+
{ }
2760+
2761+
void TestParameterListRecordClass([AsParameters] ParameterListRecordClass? args)
2762+
{ }
2763+
2764+
void TestParameterListStruct([AsParameters] ParameterListStruct? args)
2765+
{ }
2766+
2767+
void TestParameterListClass([AsParameters] ParameterListClass? args)
2768+
{ }
2769+
2770+
return new[]
2771+
{
2772+
new object[] { (Action<ParameterListRecordStruct?>)TestParameterListRecordStruct },
2773+
new object[] { (Action<ParameterListRecordClass?>)TestParameterListRecordClass },
2774+
new object[] { (Action<ParameterListStruct?>)TestParameterListStruct },
2775+
new object[] { (Action<ParameterListClass?>)TestParameterListClass },
2776+
};
2777+
}
2778+
}
2779+
2780+
[Theory]
2781+
[MemberData(nameof(NullableFromParameterListActions))]
2782+
public void RequestDelegateThrowsWhenNullableParameterList(Delegate action)
2783+
{
2784+
var parameter = action.Method.GetParameters()[0];
2785+
var httpContext = CreateHttpContext();
2786+
2787+
var exception = Assert.Throws<InvalidOperationException>(() => RequestDelegateFactory.Create(action));
2788+
Assert.Contains($"The nullable type '{TypeNameHelper.GetTypeDisplayName(parameter.ParameterType, fullName: false)}' is not supported, mark the parameter as non-nullable.", exception.Message);
2789+
}
27892790

27902791
private record struct SampleParameterList(int Foo);
27912792
private record struct AdditionalSampleParameterList(int Bar);

0 commit comments

Comments
 (0)