Skip to content

Allowing empty FromBody parameters input based on nullability #39917

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

Merged
merged 20 commits into from
Feb 24, 2022
Merged
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
13 changes: 11 additions & 2 deletions src/Mvc/Mvc.Abstractions/src/ModelBinding/BindingInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.Mvc.Abstractions;

namespace Microsoft.AspNetCore.Mvc.ModelBinding;
Expand Down Expand Up @@ -246,8 +247,16 @@ public bool TryApplyBindingInfo(ModelMetadata modelMetadata)
PropertyFilterProvider = modelMetadata.PropertyFilterProvider;
}

// There isn't a ModelMetadata feature to configure AllowEmptyInputInBodyModelBinding,
// so nothing to infer from it.
// If the EmptyBody behavior is not configured will be inferred
// as Allow when the NullablityState == NullablityStateNull or HasDefaultValue
// https://github.com/dotnet/aspnetcore/issues/39754
if (EmptyBodyBehavior == EmptyBodyBehavior.Default &&
BindingSource == BindingSource.Body &&
(modelMetadata.NullabilityState == NullabilityState.Nullable || modelMetadata.IsNullableValueType || modelMetadata.HasDefaultValue))
{
isBindingInfoPresent = true;
EmptyBodyBehavior = EmptyBodyBehavior.Allow;
}

return isBindingInfoPresent;
}
Expand Down
7 changes: 7 additions & 0 deletions src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,12 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> BoundConstructorPrope
/// </summary>
internal virtual string? ValidationModelName { get; }

/// <summary>
/// Gets the value that indicates if the parameter has a default value set.
/// This is only available when <see cref="MetadataKind"/> is <see cref="ModelMetadataKind.Parameter"/> otherwise it will be false.
/// </summary>
internal bool HasDefaultValue { get; private set; }

/// <summary>
/// Throws if the ModelMetadata is for a record type with validation on properties.
/// </summary>
Expand Down Expand Up @@ -620,6 +626,7 @@ private void InitializeTypeInformation()
IsNullableValueType = Nullable.GetUnderlyingType(ModelType) != null;
IsReferenceOrNullableType = !ModelType.IsValueType || IsNullableValueType;
UnderlyingOrModelType = Nullable.GetUnderlyingType(ModelType) ?? ModelType;
HasDefaultValue = MetadataKind == ModelMetadataKind.Parameter && Identity.ParameterInfo!.HasDefaultValue;

var collectionType = ClosedGenericMatcher.ExtractGenericInterface(ModelType, typeof(ICollection<>));
IsCollectionType = collectionType != null;
Expand Down
83 changes: 82 additions & 1 deletion src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Licensed to the .NET Foundation under one or more agreements.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Mvc.ModelBinding.Binders;
Expand Down Expand Up @@ -44,6 +44,23 @@ public void GetBindingInfo_ReadsPropertyPredicateProvider()
Assert.Same(bindAttribute, bindingInfo.PropertyFilterProvider);
}

[Fact]
public void GetBindingInfo_ReadsEmptyBodyBehavior()
{
// Arrange
var attributes = new object[]
{
new FromBodyAttribute { EmptyBodyBehavior = EmptyBodyBehavior.Allow },
};

// Act
var bindingInfo = BindingInfo.GetBindingInfo(attributes);

// Assert
Assert.NotNull(bindingInfo);
Assert.Equal(EmptyBodyBehavior.Allow, bindingInfo.EmptyBodyBehavior);
}

[Fact]
public void GetBindingInfo_ReadsRequestPredicateProvider()
{
Expand Down Expand Up @@ -102,6 +119,26 @@ public void GetBindingInfo_WithAttributesAndModelMetadata_UsesValuesFromBindingI
Assert.Same("Test", bindingInfo.BinderModelName);
}

[Fact]
public void GetBindingInfo_WithAttributesAndModelMetadata_UsesEmptyBodyBehaviorFromBindingInfo_IfAttributesPresent()
{
// Arrange
var attributes = new object[]
{
new FromBodyAttribute() { EmptyBodyBehavior = EmptyBodyBehavior.Disallow }
};
var modelType = typeof(Guid?);
var provider = new TestModelMetadataProvider();
var modelMetadata = provider.GetMetadataForType(modelType);

// Act
var bindingInfo = BindingInfo.GetBindingInfo(attributes, modelMetadata);

// Assert
Assert.NotNull(bindingInfo);
Assert.Equal(EmptyBodyBehavior.Disallow, bindingInfo.EmptyBodyBehavior);
}

[Fact]
public void GetBindingInfo_WithAttributesAndModelMetadata_UsesBinderNameFromModelMetadata_WhenNotFoundViaAttributes()
{
Expand Down Expand Up @@ -205,4 +242,48 @@ public void GetBindingInfo_WithAttributesAndModelMetadata_UsesPropertyPredicateP
Assert.NotNull(bindingInfo);
Assert.Same(propertyFilterProvider, bindingInfo.PropertyFilterProvider);
}

[Fact]
public void GetBindingInfo_WithAttributesAndModelMetadata_UsesEmptyBodyFromModelMetadata_WhenNotFoundViaAttributes()
{
// Arrange
var attributes = new object[]
{
new ControllerAttribute(),
new BindNeverAttribute(),
new FromBodyAttribute(),
};
var modelType = typeof(Guid?);
var provider = new TestModelMetadataProvider();
var modelMetadata = provider.GetMetadataForType(modelType);

// Act
var bindingInfo = BindingInfo.GetBindingInfo(attributes, modelMetadata);

// Assert
Assert.NotNull(bindingInfo);
Assert.Equal(EmptyBodyBehavior.Allow, bindingInfo.EmptyBodyBehavior);
}

[Fact]
public void GetBindingInfo_WithAttributesAndModelMetadata_PreserveEmptyBodyDefault_WhenNotNullable()
{
// Arrange
var attributes = new object[]
{
new ControllerAttribute(),
new BindNeverAttribute(),
new FromBodyAttribute(),
};
var modelType = typeof(Guid);
var provider = new TestModelMetadataProvider();
var modelMetadata = provider.GetMetadataForType(modelType);

// Act
var bindingInfo = BindingInfo.GetBindingInfo(attributes, modelMetadata);

// Assert
Assert.NotNull(bindingInfo);
Assert.Equal(EmptyBodyBehavior.Default, bindingInfo.EmptyBodyBehavior);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.Mvc.ModelBinding;
using Microsoft.AspNetCore.Routing.Template;
using Microsoft.Extensions.DependencyInjection;
Expand Down Expand Up @@ -106,6 +107,12 @@ internal void InferParameterBindingSources(ActionModel action)
message += Environment.NewLine + parameters;
throw new InvalidOperationException(message);
}
else if (fromBodyParameters.Count == 1 &&
fromBodyParameters[0].BindingInfo!.EmptyBodyBehavior == EmptyBodyBehavior.Default &&
IsOptionalParameter(fromBodyParameters[0]))
{
fromBodyParameters[0].BindingInfo!.EmptyBodyBehavior = EmptyBodyBehavior.Allow;
}
}

// Internal for unit testing.
Expand Down Expand Up @@ -155,4 +162,25 @@ private bool IsComplexTypeParameter(ParameterModel parameter)

return metadata.IsComplexType;
}

private bool IsOptionalParameter(ParameterModel parameter)
{
if (parameter.ParameterInfo.HasDefaultValue)
{
return true;
}

if (_modelMetadataProvider is ModelMetadataProvider modelMetadataProvider)
{
var metadata = modelMetadataProvider.GetMetadataForParameter(parameter.ParameterInfo);
return metadata.NullabilityState == NullabilityState.Nullable || metadata.IsNullableValueType;
}
else
{
// Cannot be determine if the parameter is optional since the provider
// does not provides an option to getMetadata from the parameter info
// so, we will NOT treat the parameter as optional.
return false;
}
}
}
8 changes: 6 additions & 2 deletions src/Mvc/Mvc.Core/src/Formatters/InputFormatter.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Mvc.ApiExplorer;
using Microsoft.AspNetCore.Mvc.Core;

Expand Down Expand Up @@ -98,8 +99,11 @@ public virtual Task<InputFormatterResult> ReadAsync(InputFormatterContext contex
throw new ArgumentNullException(nameof(context));
}

var request = context.HttpContext.Request;
if (request.ContentLength == 0)
var canHaveBody = context.HttpContext.Features.Get<IHttpRequestBodyDetectionFeature>()?.CanHaveBody;
// In case the feature is not registered
canHaveBody ??= context.HttpContext.Request.ContentLength != 0;

if (canHaveBody is false)
{
if (context.TreatEmptyInputAsDefaultValue)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ public void InferParameterBindingSources_InfersSources()

var bindingInfo = parameter.BindingInfo;
Assert.NotNull(bindingInfo);
Assert.Equal(EmptyBodyBehavior.Default, bindingInfo.EmptyBodyBehavior);
Assert.Same(BindingSource.Body, bindingInfo.BindingSource);
},
parameter =>
Expand All @@ -121,6 +122,84 @@ public void InferParameterBindingSources_InfersSources()
});
}

[Fact]
public void InferParameterBindingSources_InfersSourcesFromRequiredComplexType()
{
// Arrange
var actionName = nameof(ParameterBindingController.RequiredComplexType);
var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider();
var convention = GetConvention(modelMetadataProvider);
var action = GetActionModel(typeof(ParameterBindingController), actionName, modelMetadataProvider);

// Act
convention.InferParameterBindingSources(action);

// Assert
Assert.Collection(
action.Parameters,
parameter =>
{
Assert.Equal("model", parameter.Name);

var bindingInfo = parameter.BindingInfo;
Assert.NotNull(bindingInfo);
Assert.Equal(EmptyBodyBehavior.Default, bindingInfo.EmptyBodyBehavior);
Assert.Same(BindingSource.Body, bindingInfo.BindingSource);
});
}

[Fact]
public void InferParameterBindingSources_InfersSourcesFromNullableComplexType()
{
// Arrange
var actionName = nameof(ParameterBindingController.NullableComplexType);
var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider();
var convention = GetConvention(modelMetadataProvider);
var action = GetActionModel(typeof(ParameterBindingController), actionName, modelMetadataProvider);

// Act
convention.InferParameterBindingSources(action);

// Assert
Assert.Collection(
action.Parameters,
parameter =>
{
Assert.Equal("model", parameter.Name);

var bindingInfo = parameter.BindingInfo;
Assert.NotNull(bindingInfo);
Assert.Equal(EmptyBodyBehavior.Allow, bindingInfo.EmptyBodyBehavior);
Assert.Same(BindingSource.Body, bindingInfo.BindingSource);
});
}

[Fact]
public void InferParameterBindingSources_InfersSourcesFromComplexTypeWithDefaultValue()
{
// Arrange
var actionName = nameof(ParameterBindingController.ComplexTypeWithDefaultValue);
var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider();
var convention = GetConvention(modelMetadataProvider);
var action = GetActionModel(typeof(ParameterBindingController), actionName, modelMetadataProvider);

// Act
convention.InferParameterBindingSources(action);

// Assert
Assert.Collection(
action.Parameters,
parameter =>
{
Assert.Equal("model", parameter.Name);

var bindingInfo = parameter.BindingInfo;
Assert.NotNull(bindingInfo);
Assert.Equal(EmptyBodyBehavior.Allow, bindingInfo.EmptyBodyBehavior);
Assert.Same(BindingSource.Body, bindingInfo.BindingSource);
});
}

[Fact]
public void Apply_PreservesBindingInfo_WhenInferringFor_ParameterWithModelBinder_AndExplicitName()
{
Expand Down Expand Up @@ -847,6 +926,17 @@ private class ParameterBindingController
[HttpPut("cancellation")]
public IActionResult ComplexTypeModelWithCancellationToken(TestModel model, CancellationToken cancellationToken) => null;

#nullable enable
[HttpPut("parameter-notnull")]
public IActionResult RequiredComplexType(TestModel model) => new OkResult();

[HttpPut("parameter-null")]
public IActionResult NullableComplexType(TestModel? model) => new OkResult();
#nullable restore

[HttpPut("parameter-with-default-value")]
public IActionResult ComplexTypeWithDefaultValue(TestModel model = null) => null;

[HttpGet("parameter-with-model-binder-attribute")]
public IActionResult ModelBinderAttribute([ModelBinder(Name = "top")] int value) => null;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,7 @@ protected static HttpContext GetHttpContext(
var httpContext = new DefaultHttpContext();
httpContext.Request.Body = requestStream;
httpContext.Request.ContentType = contentType;
httpContext.Request.ContentLength = requestStream.Length;

return httpContext;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -740,10 +740,13 @@ private static HttpContext GetHttpContext(
request.SetupGet(r => r.Headers).Returns(headers.Object);
request.SetupGet(f => f.Body).Returns(new MemoryStream(contentBytes));
request.SetupGet(f => f.ContentType).Returns(contentType);
request.SetupGet(f => f.ContentLength).Returns(contentBytes.Length);

var httpContext = new Mock<HttpContext>();
var features = new Mock<IFeatureCollection>();
httpContext.SetupGet(c => c.Request).Returns(request.Object);
httpContext.SetupGet(c => c.Request).Returns(request.Object);
httpContext.SetupGet(c => c.Features).Returns(features.Object);
return httpContext.Object;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -676,10 +676,12 @@ private static HttpContext GetHttpContext(
request.SetupGet(r => r.Headers).Returns(headers.Object);
request.SetupGet(f => f.Body).Returns(new MemoryStream(contentBytes));
request.SetupGet(f => f.ContentType).Returns(contentType);
request.SetupGet(f => f.ContentLength).Returns(contentBytes.Length);

var httpContext = new Mock<HttpContext>();
var features = new Mock<IFeatureCollection>();
httpContext.SetupGet(c => c.Request).Returns(request.Object);
httpContext.SetupGet(c => c.Request).Returns(request.Object);
httpContext.SetupGet(c => c.Features).Returns(features.Object);
return httpContext.Object;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -414,11 +414,14 @@ public async Task ReadAsync_RegistersFileStreamForDisposal()
new MvcOptions(),
new MvcNewtonsoftJsonOptions());
var httpContext = new Mock<HttpContext>();
var features = new Mock<IFeatureCollection>();
httpContext.SetupGet(c => c.Features).Returns(features.Object);
IDisposable registerForDispose = null;

var content = Encoding.UTF8.GetBytes("\"Hello world\"");
httpContext.Setup(h => h.Request.Body).Returns(new NonSeekableReadStream(content, allowSyncReads: false));
httpContext.Setup(h => h.Request.ContentType).Returns("application/json");
httpContext.Setup(h => h.Request.ContentLength).Returns(content.Length);
httpContext.Setup(h => h.Response.RegisterForDispose(It.IsAny<IDisposable>()))
.Callback((IDisposable disposable) => registerForDispose = disposable)
.Verifiable();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,10 +260,12 @@ private static HttpContext CreateHttpContext(
request.SetupGet(r => r.Headers).Returns(headers.Object);
request.SetupGet(f => f.Body).Returns(new MemoryStream(contentBytes));
request.SetupGet(f => f.ContentType).Returns(contentType);
request.SetupGet(f => f.ContentLength).Returns(contentBytes.Length);

var httpContext = new Mock<HttpContext>();
var features = new Mock<IFeatureCollection>();
httpContext.SetupGet(c => c.Request).Returns(request.Object);
httpContext.SetupGet(c => c.Request).Returns(request.Object);
httpContext.SetupGet(c => c.Features).Returns(features.Object);
return httpContext.Object;
}

Expand Down
Loading