Skip to content

Fixing minimal api header array binding #55803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 21 commits into from
Closed

Fixing minimal api header array binding #55803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
c9cbe68
Fixes
MattyLeslie May 15, 2024
1b42dd4
Merge branch 'dotnet:main' into fixingMinimalApiHeaderArrayBinding
MattyLeslie May 15, 2024
b5a2e38
Removing extra ';'
MattyLeslie May 15, 2024
79bdc5a
Merge branch 'dotnet:main' into fixingMinimalApiHeaderArrayBinding
MattyLeslie May 20, 2024
6158c14
Ensuring single header values and comma seperated header values are t…
MattyLeslie May 20, 2024
63dcea3
Test ammendments
MattyLeslie May 20, 2024
8e5f786
Adapting BindParameterFromExpression to handle nessisary cases
MattyLeslie May 20, 2024
39b025b
Merge branch 'fixingMinimalApiHeaderArrayBinding' of https://github.c…
MattyLeslie May 20, 2024
b52fad2
Removing trim method
MattyLeslie May 20, 2024
f415785
Adding comments
MattyLeslie May 20, 2024
aeb3354
Clean up
MattyLeslie May 20, 2024
5b31f50
Handling possible null occurances
MattyLeslie May 20, 2024
1e3ec2e
Removing extra lines
MattyLeslie May 20, 2024
b86969f
Merge branch 'dotnet:main' into fixingMinimalApiHeaderArrayBinding
MattyLeslie May 20, 2024
9c3f575
Improving performance and mimizing memory usage and allocations withi…
MattyLeslie May 20, 2024
c78f1a9
Update src/Http/Http.Extensions/src/RequestDelegateFactory.cs
MattyLeslie Jul 15, 2024
99ec84b
Update src/Http/Http.Extensions/src/RequestDelegateFactory.cs
MattyLeslie Jul 15, 2024
9db0e6f
Fixing indentations
MattyLeslie Jul 16, 2024
8bcad90
Fixing duplicate code and using MemoryAllocations.Split
MattyLeslie Jul 16, 2024
f81960c
Adding more test cases
MattyLeslie Jul 16, 2024
7f8bca7
Updating RequestDelegateGenerator
MattyLeslie Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/Http/Http.Extensions/gen/RequestDelegateGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
endpoint.EmitJsonPreparation(codeWriter);
endpoint.EmitRouteOrQueryResolver(codeWriter);
endpoint.EmitJsonBodyOrServiceResolver(codeWriter);
EmitHeaderValueParsing(codeWriter);

if (endpoint.NeedsParameterArray)
{
codeWriter.WriteLine("var parameters = del.Method.GetParameters();");
Expand Down Expand Up @@ -288,5 +290,25 @@ public void Initialize(IncrementalGeneratorInitializationContext context)

context.AddSource("GeneratedRouteBuilderExtensions.g.cs", code);
});

}
private static void EmitHeaderValueParsing(CodeWriter codeWriter)
{
codeWriter.WriteLine("var headerParameters = handler.Method.GetParameters().Where(p => p.GetCustomAttribute<FromHeaderAttribute>() != null).ToList();");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When emitting code at compile-time, we want to do the discovery of parameters like this via static analysis.

You'll observe that in other parts of the Request Delegate Generator (RDG) code, we generate an EndpointParameter object that we construct after statically analyzing the arguments then we use the information in that data model to emit the generated code. We don't need to re-compute things like the headerName dynamically like this.

You can find the pre-existing code where we emit the binding logic for header parameters here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the review, I will be working on this.

codeWriter.WriteLine("foreach (var param in headerParameters)");
codeWriter.StartBlock();
codeWriter.WriteLine("var headerAttribute = param.GetCustomAttribute<FromHeaderAttribute>();");
codeWriter.WriteLine("var headerName = headerAttribute?.Name ?? param.Name;");
codeWriter.WriteLine("var headerValue = httpContext.Request.Headers[headerName];");
codeWriter.WriteLine("if (param.ParameterType == typeof(string[]) || typeof(IEnumerable<string>).IsAssignableFrom(param.ParameterType))");
codeWriter.StartBlock();
codeWriter.WriteLine("var parsedValues = headerValue.SelectMany(v => v.Split(',')).Select(v => v.Trim()).ToArray();");
codeWriter.WriteLine("arguments[param.Position] = param.ParameterType == typeof(string[]) ? parsedValues : (object)parsedValues.AsEnumerable();");
codeWriter.EndBlock();
codeWriter.WriteLine("else if (param.ParameterType == typeof(string))");
codeWriter.StartBlock();
codeWriter.WriteLine("arguments[param.Position] = headerValue.ToString();");
codeWriter.EndBlock();
codeWriter.EndBlock();
}
}
77 changes: 60 additions & 17 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ private static RequestDelegateFactoryContext CreateFactoryContext(RequestDelegat
var serviceProvider = options?.ServiceProvider ?? options?.EndpointBuilder?.ApplicationServices ?? EmptyServiceProvider.Instance;
var endpointBuilder = options?.EndpointBuilder ?? new RdfEndpointBuilder(serviceProvider);
var jsonSerializerOptions = serviceProvider.GetService<IOptions<JsonOptions>>()?.Value.SerializerOptions ?? JsonOptions.DefaultSerializerOptions;
var formDataMapperOptions = new FormDataMapperOptions();;
var formDataMapperOptions = new FormDataMapperOptions();

var factoryContext = new RequestDelegateFactoryContext
{
Expand Down Expand Up @@ -758,7 +758,6 @@ private static Expression CreateArgument(ParameterInfo parameter, RequestDelegat
{
throw new NotSupportedException(
$"Assigning a value to the {nameof(IFromFormMetadata)}.{nameof(IFromFormMetadata.Name)} property is not supported for parameters of type {nameof(IFormCollection)}.");

}
return BindParameterFromFormCollection(parameter, factoryContext);
}
Expand Down Expand Up @@ -1628,6 +1627,7 @@ private static Expression BindParameterFromKeyedService(ParameterInfo parameter,

private static Expression BindParameterFromValue(ParameterInfo parameter, Expression valueExpression, RequestDelegateFactoryContext factoryContext, string source)
{

if (parameter.ParameterType == typeof(string) || parameter.ParameterType == typeof(string[])
|| parameter.ParameterType == typeof(StringValues) || parameter.ParameterType == typeof(StringValues?))
{
Expand Down Expand Up @@ -1854,10 +1854,10 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
}

private static Expression BindParameterFromExpression(
ParameterInfo parameter,
Expression valueExpression,
RequestDelegateFactoryContext factoryContext,
string source)
ParameterInfo parameter,
Expression valueExpression,
RequestDelegateFactoryContext factoryContext,
string source)
{
var nullability = factoryContext.NullabilityContext.Create(parameter);
var isOptional = IsOptionalParameter(parameter, factoryContext);
Expand All @@ -1868,15 +1868,27 @@ private static Expression BindParameterFromExpression(
var parameterNameConstant = Expression.Constant(parameter.Name);
var sourceConstant = Expression.Constant(source);

if (source == "header" && (parameter.ParameterType == typeof(string[]) || typeof(IEnumerable<string>).IsAssignableFrom(parameter.ParameterType)))
{
var stringValuesExpr = Expression.Convert(valueExpression, typeof(StringValues));
var toStringArrayMethod = typeof(StringValues).GetMethod(nameof(StringValues.ToArray))!;
var headerValuesArrayExpr = Expression.Call(stringValuesExpr, toStringArrayMethod);

var splitAndTrimMethod = typeof(RequestDelegateFactory).GetMethod(nameof(SplitAndTrim), System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic)!;
var splitAndTrimExpr = Expression.Call(splitAndTrimMethod, headerValuesArrayExpr);

valueExpression = Expression.Convert(splitAndTrimExpr, parameter.ParameterType);
}

if (!isOptional)
{
// The following is produced if the parameter is required:
//
// argument = value["param1"];
// argument = value;
// if (argument == null)
// {
// wasParamCheckFailure = true;
// Log.RequiredParameterNotProvided(httpContext, "TypeOfValue", "param1");
// Log.RequiredParameterNotProvided(httpContext, "TypeOfValue", "parameterName", "source");
// }
var checkRequiredStringParameterBlock = Expression.Block(
Expression.Assign(argument, valueExpression),
Expand All @@ -1890,8 +1902,6 @@ private static Expression BindParameterFromExpression(
)
);

// NOTE: when StringValues is used as a parameter, value["some_unpresent_parameter"] returns StringValue.Empty, and it's equivalent to (string?)null

factoryContext.ExtraLocals.Add(argument);
factoryContext.ParamCheckExpressions.Add(checkRequiredStringParameterBlock);
return argument;
Expand All @@ -1905,20 +1915,20 @@ private static Expression BindParameterFromExpression(
// when Nullable<StringValues> is used and the actual value is StringValues.Empty, we should pass in a Nullable<StringValues>
return Expression.Block(
Expression.Condition(Expression.Equal(valueExpression, Expression.Convert(Expression.Constant(StringValues.Empty), parameter.ParameterType)),
Expression.Convert(Expression.Constant(null), parameter.ParameterType),
valueExpression
)
);
Expression.Convert(Expression.Constant(null), parameter.ParameterType),
valueExpression
)
);
}
return valueExpression;
}

// The following is produced if the parameter is optional. Note that we convert the
// default value to the target ParameterType to address scenarios where the user is
// is setting null as the default value in a context where nullability is disabled.
// setting null as the default value in a context where nullability is disabled.
//
// param1_local = httpContext.RouteValue["param1"] ?? httpContext.Query["param1"];
// param1_local != null ? param1_local : Convert(null, Int32)
// param1_local = value;
// param1_local != null ? param1_local : Convert(defaultValue, ParameterType)
return Expression.Block(
Expression.Condition(Expression.NotEqual(valueExpression, Expression.Constant(null)),
valueExpression,
Expand Down Expand Up @@ -2846,6 +2856,39 @@ private static void FormatTrackedParameters(RequestDelegateFactoryContext factor
}
}

private static string[] SplitAndTrim(string[] values)
{
if (values == null || values.Length == 0)
{
return [];
}

var result = new List<string>();
Span<Range> parts = stackalloc Range[16];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem correct. If there are more than 16 values in a single header it will miss them. Maybe just revert to the previous code for now. We can look at improving the perf later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @BrennanConroy, I will revert that, I am going to mark this as draft as I'm still working on this.


foreach (var value in values)
{
if (string.IsNullOrWhiteSpace(value))
{
continue;
}

var valueSpan = value.AsSpan();
var length = valueSpan.Split(parts, ',');

for (int i = 0; i < length; i++)
{
var part = valueSpan[parts[i]].Trim();
if (!part.IsEmpty)
{
result.Add(new string(part));
}
}
}

return [.. result];
}

private sealed class RdfEndpointBuilder : EndpointBuilder
{
public RdfEndpointBuilder(IServiceProvider applicationServices)
Expand Down
71 changes: 71 additions & 0 deletions src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3150,6 +3150,77 @@ static void TestAction([AsParameters] ParameterListRequiredNullableStringFromDif
Assert.Null(httpContext.Items["RequiredHeaderParam"]);
}

[Fact]
public async Task RequestDelegate_ShouldHandleCommaSeparatedHeaderValuesAsString()
{
// Arrange
var httpContext = CreateHttpContext();
httpContext.Request.Headers["TestHeader"] = "Value1,Value2";

var resultFactory = RequestDelegateFactory.Create((HttpContext httpContext, [FromHeader(Name = "TestHeader")] string headerValue) =>
{
httpContext.Items["headerValue"] = headerValue;
});

var requestDelegate = resultFactory.RequestDelegate;

// Act
await requestDelegate(httpContext);

// Assert
Assert.True(httpContext.Items.ContainsKey("headerValue"));
Assert.Equal("Value1,Value2", httpContext.Items["headerValue"]);
}

[Fact]
public async Task RequestDelegate_ShouldHandleSingleHeaderValueAsStringArray()
{
// Arrange
var httpContext = CreateHttpContext();
httpContext.Request.Headers["TestHeader"] = "Value1";

var resultFactory = RequestDelegateFactory.Create((HttpContext httpContext, [FromHeader(Name = "TestHeader")] string[] headerValues) =>
{
httpContext.Items["headerValues"] = headerValues;
});

var requestDelegate = resultFactory.RequestDelegate;

// Act
await requestDelegate(httpContext);

// Assert
Assert.True(httpContext.Items.ContainsKey("headerValues"));
var headerValues = httpContext.Items["headerValues"] as string[];
Assert.NotNull(headerValues);
Assert.Single(headerValues);
Assert.Equal("Value1", headerValues[0]);
}

[Fact]
public async Task RequestDelegate_ShouldHandleCommaSeparatedHeaderValuesAsIEnumerable()
{
// Arrange
var httpContext = CreateHttpContext();
httpContext.Request.Headers["TestHeader"] = "Value1,Value2,Value3";

var resultFactory = RequestDelegateFactory.Create((HttpContext httpContext, [FromHeader(Name = "TestHeader")] IEnumerable<string> headerValues) =>
{
httpContext.Items["headerValues"] = headerValues;
});

var requestDelegate = resultFactory.RequestDelegate;

// Act
await requestDelegate(httpContext);

// Assert
Assert.True(httpContext.Items.ContainsKey("headerValues"));
var headerValues = httpContext.Items["headerValues"] as IEnumerable<string>;
Assert.NotNull(headerValues);
Assert.Equal(new[] { "Value1", "Value2", "Value3" }, headerValues);
}

#nullable disable
private class ParameterListMixedRequiredStringsFromDifferentSources
{
Expand Down
Loading