-
Notifications
You must be signed in to change notification settings - Fork 10.9k
Treat empty query strings as null for nullable value types #65816
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
b373704
a736080
8dfa9ef
818dead
4b6e9a2
84a80d5
d3e7a95
88ca720
767141c
0a07cea
4a78cf9
ab25d0c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
| var sourceCheckExpr = (Expression)(isNullableValueType | ||
| ? TempSourceStringIsNotNullOrEmptyExpr | ||
| : TempSourceStringNotNullExpr); | ||
|
|
||
| var ifNotNullTryParse = !parameter.HasDefaultValue | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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))); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -263,6 +263,90 @@ public async Task NullRouteParametersPrefersRouteOverQueryString() | |
| Assert.Equal(42, httpContext.Items["input"]); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task EmptyQueryStringValueTreatedAsNullForNullableBool() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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() | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already
isNotNullablelocal variable calculated early in this method that can be reused.