Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 9 additions & 2 deletions src/Http/Http.Extensions/src/RequestDelegateFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1820,9 +1820,16 @@ private static Expression BindParameterFromValue(ParameterInfo parameter, Expres
Expression.Assign(parameter.ParameterType.IsArray ? Expression.ArrayAccess(argument, index) : argument, Expression.Convert(parsedValue, targetParseType)),
failBlock));

// For nullable value types, treat empty strings as null (don't attempt to parse them).
// This matches controller behavior and avoids parse failures on ?param= (empty value).
var isNullableValueType = Nullable.GetUnderlyingType(parameter.ParameterType) != null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is already isNotNullable local variable calculated early in this method that can be reused.

var sourceCheckExpr = (Expression)(isNullableValueType
? TempSourceStringIsNotNullOrEmptyExpr
: TempSourceStringNotNullExpr);

var ifNotNullTryParse = !parameter.HasDefaultValue

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could ifNotNullTryParse be renamed so that it better reflects what this does?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please also make sure to update any relevant code comments.

? Expression.IfThen(TempSourceStringNotNullExpr, tryParseExpression)
: Expression.IfThenElse(TempSourceStringNotNullExpr, tryParseExpression,
? Expression.IfThen(sourceCheckExpr, tryParseExpression)
: Expression.IfThenElse(sourceCheckExpr, tryParseExpression,
Expression.Assign(argument,
CreateDefaultValueExpression(parameter.DefaultValue, parameter.ParameterType)));

Expand Down
84 changes: 84 additions & 0 deletions src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,90 @@ public async Task NullRouteParametersPrefersRouteOverQueryString()
Assert.Equal(42, httpContext.Items["input"]);
}

[Fact]
public async Task EmptyQueryStringValueTreatedAsNullForNullableBool()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are the tests here providing any coverage more than the other tests in RequestDelegateCreationTests.QueryParameters.cs?

I think in general, RequestDelegateFactory-specific tests are avoided in favor of the tests that ensure consistency between RequestDelegateFactory and RequestDelegateGenerator.

{
var httpContext = CreateHttpContext();

var factoryResult = RequestDelegateFactory.Create((bool? value, HttpContext httpContext) =>
{
httpContext.Items["value"] = value;
});

httpContext.Request.Query = new QueryCollection(new Dictionary<string, StringValues>
{
["value"] = "" // Empty string should be treated as null
});

var requestDelegate = factoryResult.RequestDelegate;
await requestDelegate(httpContext);

Assert.Null(httpContext.Items["value"]);
}

[Fact]
public async Task EmptyQueryStringValueTreatedAsNullForNullableInt()
{
var httpContext = CreateHttpContext();

var factoryResult = RequestDelegateFactory.Create((int? value, HttpContext httpContext) =>
{
httpContext.Items["value"] = value;
});

httpContext.Request.Query = new QueryCollection(new Dictionary<string, StringValues>
{
["value"] = "" // Empty string should be treated as null
});

var requestDelegate = factoryResult.RequestDelegate;
await requestDelegate(httpContext);

Assert.Null(httpContext.Items["value"]);
}

[Fact]
public async Task NonEmptyQueryStringValueParsedCorrectlyForNullableBool()
{
var httpContext = CreateHttpContext();

var factoryResult = RequestDelegateFactory.Create((bool? value, HttpContext httpContext) =>
{
httpContext.Items["value"] = value;
});

httpContext.Request.Query = new QueryCollection(new Dictionary<string, StringValues>
{
["value"] = "true"
});

var requestDelegate = factoryResult.RequestDelegate;
await requestDelegate(httpContext);

Assert.True((bool?)httpContext.Items["value"]);
}

[Fact]
public async Task NonEmptyQueryStringValueParsedCorrectlyForNullableInt()
{
var httpContext = CreateHttpContext();

var factoryResult = RequestDelegateFactory.Create((int? value, HttpContext httpContext) =>
{
httpContext.Items["value"] = value;
});

httpContext.Request.Query = new QueryCollection(new Dictionary<string, StringValues>
{
["value"] = "42"
});

var requestDelegate = factoryResult.RequestDelegate;
await requestDelegate(httpContext);

Assert.Equal(42, (int?)httpContext.Items["value"]);
}

[Fact]
public async Task CreatingDelegateWithInstanceMethodInfoCreatesInstancePerCall()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,4 +226,44 @@ public async Task MapAction_ExplicitQueryParam_NameTest(string name, string look
await endpoint.RequestDelegate(httpContext);
await VerifyResponseBodyAsync(httpContext, "test", 200);
}

[Fact]
public async Task MapAction_NullableBoolParam_WithEmptyQueryStringValue_SetsNull()
{
// Regression test for https://github.com/dotnet/aspnetcore/issues/65754
// ?myBool= should bind to null for bool? parameter, not throw.
var (results, compilation) = await RunGeneratorAsync("""
app.MapGet("/test", ([FromQuery] bool? myBool) => myBool.HasValue ? $"Value: {myBool.Value}" : "null");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we please have a test for a parameter that has default value?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When doing so, please validate the current behavior of Mvc as well.

""");
var endpoint = GetEndpointFromCompilation(compilation);

VerifyStaticEndpointModel(results, endpointModel =>
{
Assert.Equal("MapGet", endpointModel.HttpMethod);
var p = Assert.Single(endpointModel.Parameters);
Assert.Equal(EndpointParameterSource.Query, p.Source);
Assert.Equal("myBool", p.SymbolName);
});

var httpContext = CreateHttpContext();
httpContext.Request.QueryString = new QueryString("?myBool=");

await endpoint.RequestDelegate(httpContext);
await VerifyResponseBodyAsync(httpContext, "null");
}

[Fact]
public async Task MapAction_NullableIntParam_WithEmptyQueryStringValue_SetsNull()
{
var (results, compilation) = await RunGeneratorAsync("""
app.MapGet("/test", ([FromQuery] int? value) => value.HasValue ? $"{value.Value}" : "null");
""");
var endpoint = GetEndpointFromCompilation(compilation);

var httpContext = CreateHttpContext();
httpContext.Request.QueryString = new QueryString("?value=");

await endpoint.RequestDelegate(httpContext);
await VerifyResponseBodyAsync(httpContext, "null");
}
}
Loading