CR feedback: Let input formatters return "no value" results if they w…

…ant (not purely triggered by request content length)

src/Microsoft.AspNetCore.Mvc.Abstractions/Formatters/InputFormatterContext.cs

@@ -36,6 +36,36 @@ public InputFormatterContext(
             ModelStateDictionary modelState,
             ModelMetadata metadata,
             Func<Stream, Encoding, TextReader> readerFactory)
+            : this(httpContext, modelName, modelState, metadata, readerFactory, allowEmptyInput: false)
+        {
+        }
+
+        /// <summary>
+        /// Creates a new instance of <see cref="InputFormatterContext"/>.
+        /// </summary>
+        /// <param name="httpContext">
+        /// The <see cref="Http.HttpContext"/> for the current operation.
+        /// </param>
+        /// <param name="modelName">The name of the model.</param>
+        /// <param name="modelState">
+        /// The <see cref="ModelStateDictionary"/> for recording errors.
+        /// </param>
+        /// <param name="metadata">
+        /// The <see cref="ModelMetadata"/> of the model to deserialize.
+        /// </param>
+        /// <param name="readerFactory">
+        /// A delegate which can create a <see cref="TextReader"/> for the request body.
+        /// </param>
+        /// <param name="allowEmptyInput">
+        /// A value for <see cref="AllowEmptyInput"/>.
+        /// </param>
+        public InputFormatterContext(
+            HttpContext httpContext,
+            string modelName,
+            ModelStateDictionary modelState,
+            ModelMetadata metadata,
+            Func<Stream, Encoding, TextReader> readerFactory,
+            bool allowEmptyInput)
         {
             if (httpContext == null)
             {
@@ -67,9 +97,19 @@ public InputFormatterContext(
             ModelState = modelState;
             Metadata = metadata;
             ReaderFactory = readerFactory;
+            AllowEmptyInput = allowEmptyInput;
             ModelType = metadata.ModelType;
         }
 
+        /// <summary>
+        /// Gets a flag to indicate whether the input formatter should allow no value to be provided.
+        /// If <see langword="false"/>, the input formatter should handle empty input by returning
+        /// <see cref="InputFormatterResult.NoValueAsync()"/>. If <see langword="false"/>, the input
+        /// formatter should handle empty input by returning the default value for the type
+        /// <see cref="ModelType"/>.
+        /// </summary>
+        public bool AllowEmptyInput { get; }
+
         /// <summary>
         /// Gets the <see cref="Http.HttpContext"/> associated with the current operation.
         /// </summary>

src/Microsoft.AspNetCore.Mvc.Abstractions/Formatters/InputFormatterResult.cs

@@ -10,24 +10,32 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
     /// </summary>
     public class InputFormatterResult
     {
-        private static readonly InputFormatterResult _failure = new InputFormatterResult();
+        private static readonly InputFormatterResult _failure = new InputFormatterResult(hasError: true);
+        private static readonly InputFormatterResult _noValue = new InputFormatterResult(hasError: false);
         private static readonly Task<InputFormatterResult> _failureAsync = Task.FromResult(_failure);
+        private static readonly Task<InputFormatterResult> _noValueAsync = Task.FromResult(_noValue);
 
-        private InputFormatterResult()
+        private InputFormatterResult(bool hasError)
         {
-            HasError = true;
+            HasError = hasError;
         }
 
         private InputFormatterResult(object model)
         {
             Model = model;
+            IsModelSet = true;
         }
 
         /// <summary>
         /// Gets an indication whether the <see cref="IInputFormatter.ReadAsync"/> operation had an error.
         /// </summary>
         public bool HasError { get; }
 
+        /// <summary>
+        /// Gets an indication whether a value for the <see cref="Model"/> property was supplied.
+        /// </summary>
+        public bool IsModelSet { get; }
+
         /// <summary>
         /// Gets the deserialized <see cref="object"/>.
         /// </summary>
@@ -89,5 +97,31 @@ public static Task<InputFormatterResult> SuccessAsync(object model)
         {
             return Task.FromResult(Success(model));
         }
+
+        /// <summary>
+        /// Returns an <see cref="InputFormatterResult"/> indicating the <see cref="IInputFormatter.ReadAsync"/>
+        /// operation produced no value.
+        /// </summary>
+        /// <returns>
+        /// An <see cref="InputFormatterResult"/> indicating the <see cref="IInputFormatter.ReadAsync"/>
+        /// operation produced no value.
+        /// </returns>
+        public static InputFormatterResult NoValue()
+        {
+            return _noValue;
+        }
+
+        /// <summary>
+        /// Returns a <see cref="Task"/> that on completion provides an <see cref="InputFormatterResult"/> indicating
+        /// the <see cref="IInputFormatter.ReadAsync"/> operation produced no value.
+        /// </summary>
+        /// <returns>
+        /// A <see cref="Task"/> that on completion provides an <see cref="InputFormatterResult"/> indicating the
+        /// <see cref="IInputFormatter.ReadAsync"/> operation produced no value.
+        /// </returns>
+        public static Task<InputFormatterResult> NoValueAsync()
+        {
+            return _noValueAsync;
+        }
     }
 }

src/Microsoft.AspNetCore.Mvc.Core/Formatters/InputFormatter.cs

@@ -105,7 +105,14 @@ public virtual Task<InputFormatterResult> ReadAsync(InputFormatterContext contex
             var request = context.HttpContext.Request;
             if (request.ContentLength == 0)
             {
-                return InputFormatterResult.SuccessAsync(GetDefaultValueForType(context.ModelType));
+                if (context.AllowEmptyInput)
+                {
+                    return InputFormatterResult.SuccessAsync(GetDefaultValueForType(context.ModelType));
+                }
+                else
+                {
+                    return InputFormatterResult.NoValueAsync();
+                }
             }
 
             return ReadRequestBodyAsync(context);

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/BodyModelBinder.cs

@@ -20,9 +20,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
     public class BodyModelBinder : IModelBinder
     {
         private readonly IList<IInputFormatter> _formatters;
-        private readonly MvcOptions _options;
         private readonly Func<Stream, Encoding, TextReader> _readerFactory;
         private readonly ILogger _logger;
+        private readonly MvcOptions _options;
 
         /// <summary>
         /// Creates a new <see cref="BodyModelBinder"/>.
@@ -114,12 +114,15 @@ public async Task BindModelAsync(ModelBindingContext bindingContext)
 
             var httpContext = bindingContext.HttpContext;
 
+            var allowEmptyInput = _options?.AllowEmptyInputInInputFormatter == true;
+
             var formatterContext = new InputFormatterContext(
                 httpContext,
                 modelBindingKey,
                 bindingContext.ModelState,
                 bindingContext.ModelMetadata,
-                _readerFactory);
+                _readerFactory,
+                allowEmptyInput);
 
             var formatter = (IInputFormatter)null;
             for (var i = 0; i < _formatters.Count; i++)
@@ -156,10 +159,17 @@ public async Task BindModelAsync(ModelBindingContext bindingContext)
                     // Formatter encountered an error. Do not use the model it returned.
                     return;
                 }
-
-                if (model == null && _options?.AllowEmptyInputInInputFormatter != true)
+
+                if (result.IsModelSet)
+                {
+                    bindingContext.Result = ModelBindingResult.Success(model);
+                }
+                else
                 {
-                    // Body model binding is required, but no value was supplied.
+                    // If the input formatter gives a "no value" result, that's always a validation error.
+                    // If instead the input formatter wants to treat the input as optional, it must do so by
+                    // returning InputFormatterResult.Success(defaultForModelType), because input formatters
+                    // are responsible for choosing a default value for the model type.
                     bindingContext.Result = ModelBindingResult.Failed();
 
                     var message = bindingContext
@@ -168,10 +178,6 @@ public async Task BindModelAsync(ModelBindingContext bindingContext)
                         .MissingRequestBodyRequiredValueAccessor();
                     bindingContext.ModelState.AddModelError(modelBindingKey, message);
                 }
-                else
-                {
-                    bindingContext.Result = ModelBindingResult.Success(model);
-                }
 
                 return;
             }

src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonInputFormatter.cs

@@ -158,7 +158,18 @@ public override Task<InputFormatterResult> ReadRequestBodyAsync(
 
                     if (successful)
                     {
-                        return InputFormatterResult.SuccessAsync(model);
+                        if (model == null && !context.AllowEmptyInput)
+                        {
+                            // Some nonempty inputs might deserialize as null, for example whitespace,
+                            // or the JSON-encoded value "null". The upstream BodyModelBinder needs to
+                            // be notified that we don't regard this as a real input so it can register
+                            // a validation error.
+                            return InputFormatterResult.NoValueAsync();
+                        }
+                        else
+                        {
+                            return InputFormatterResult.SuccessAsync(model);
+                        }
                     }
 
                     return InputFormatterResult.FailureAsync();

test/Microsoft.AspNetCore.Mvc.Core.Test/Formatters/InputFormatterTest.cs

@@ -410,6 +410,31 @@ public void GetSupportedContentTypes_ThrowsInvalidOperationException_IfMediaType
                 () => formatter.GetSupportedContentTypes("application/json", typeof(object)));
         }
 
+        [Theory]
+        [InlineData(true, true)]
+        [InlineData(false, false)]
+        public async Task ReadAsync_WithEmptyRequest_ReturnsNoValueResultWhenExpected(bool allowEmptyInputValue, bool expectedIsModelSet)
+        {
+            // Arrange
+            var formatter = new TestFormatter();
+            var context = new InputFormatterContext(
+                new DefaultHttpContext(),
+                "",
+                new ModelStateDictionary(),
+                new EmptyModelMetadataProvider().GetMetadataForType(typeof(object)),
+                (s, e) => new StreamReader(s, e),
+                allowEmptyInputValue);
+            context.HttpContext.Request.ContentLength = 0;
+
+            // Act
+            var result = await formatter.ReadAsync(context);
+
+            // Assert
+            Assert.False(result.HasError);
+            Assert.Null(result.Model);
+            Assert.Equal(expectedIsModelSet, result.IsModelSet);
+        }
+
         private class BadConfigurationFormatter : InputFormatter
         {
             public override Task<InputFormatterResult> ReadRequestBodyAsync(InputFormatterContext context)

test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/BodyModelBinderTests.cs

@@ -115,17 +115,15 @@ public async Task BindModel_IsGreedy()
             Assert.False(bindingContext.Result.IsModelSet);
         }
 
-        [Theory]
-        [InlineData(false, true)]
-        [InlineData(true, false)]
-        public async Task BindModel_NoInput_SetsModelStateErrorWhenExpected(bool allowEmptyInputSetting, bool expectValidationError)
+        [Fact]
+        public async Task BindModel_NoValueResult_SetsModelStateError()
         {
             // Arrange
             var mockInputFormatter = new Mock<IInputFormatter>();
             mockInputFormatter.Setup(f => f.CanRead(It.IsAny<InputFormatterContext>()))
                 .Returns(true);
             mockInputFormatter.Setup(o => o.ReadAsync(It.IsAny<InputFormatterContext>()))
-                              .Returns(InputFormatterResult.SuccessAsync(null));
+                              .Returns(InputFormatterResult.NoValueAsync());
             var inputFormatter = mockInputFormatter.Object;
 
             var provider = new TestModelMetadataProvider();
@@ -136,29 +134,53 @@ public async Task BindModel_NoInput_SetsModelStateErrorWhenExpected(bool allowEm
                 metadataProvider: provider);
             bindingContext.BinderModelName = "custom";
 
-            var binder = CreateBinder(new[] { inputFormatter }, allowEmptyInputSetting);
+            var binder = CreateBinder(new[] { inputFormatter });
 
             // Act
             await binder.BindModelAsync(bindingContext);
 
             // Assert
-            var expectIsModelSet = !expectValidationError; // If we allow null as a valid input, then IsModelSet==true in that case
-            Assert.Equal(expectIsModelSet, bindingContext.Result.IsModelSet);
             Assert.Null(bindingContext.Result.Model);
+            Assert.False(bindingContext.Result.IsModelSet);
+            Assert.False(bindingContext.ModelState.IsValid);
 
-            if (expectValidationError)
-            {
-                // Key is the bindermodelname because this was a top-level binding.
-                Assert.False(bindingContext.ModelState.IsValid);
-                var entry = Assert.Single(bindingContext.ModelState);
-                Assert.Equal("custom", entry.Key);
-                Assert.Single(entry.Value.Errors);
-            }
-            else
-            {
-                Assert.True(bindingContext.ModelState.IsValid);
-                Assert.Empty(bindingContext.ModelState);
-            }
+            // Key is the bindermodelname because this was a top-level binding.
+            var entry = Assert.Single(bindingContext.ModelState);
+            Assert.Equal("custom", entry.Key);
+            Assert.Single(entry.Value.Errors);
+        }
+
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public async Task BindModel_PassesAllowEmptyInputOptionViaContext(bool allowEmptyInputValue)
+        {
+            // Arrange
+            var mockInputFormatter = new Mock<IInputFormatter>();
+            mockInputFormatter.Setup(f => f.CanRead(It.IsAny<InputFormatterContext>()))
+                .Returns(true);
+            mockInputFormatter.Setup(o => o.ReadAsync(It.IsAny<InputFormatterContext>()))
+                              .Returns(InputFormatterResult.NoValueAsync())
+                              .Verifiable();
+            var inputFormatter = mockInputFormatter.Object;
+
+            var provider = new TestModelMetadataProvider();
+            provider.ForType<Person>().BindingDetails(d => d.BindingSource = BindingSource.Body);
+
+            var bindingContext = GetBindingContext(
+                typeof(Person),
+                metadataProvider: provider);
+            bindingContext.BinderModelName = "custom";
+
+            var binder = CreateBinder(new[] { inputFormatter }, allowEmptyInputValue);
+
+            // Act
+            await binder.BindModelAsync(bindingContext);
+
+            // Assert
+            mockInputFormatter.Verify(formatter => formatter.ReadAsync(
+                It.Is<InputFormatterContext>(ctx => ctx.AllowEmptyInput == allowEmptyInputValue)),
+                Times.Once);
         }
 
         [Fact]

test/Microsoft.AspNetCore.Mvc.Formatters.Json.Test/JsonInputFormatterTest.cs

@@ -342,6 +342,40 @@ public async Task ReadAsync_UsesTryAddModelValidationErrorsToModelState()
             Assert.IsType<TooManyModelErrorsException>(error.Exception);
         }
 
+        [Theory]
+        [InlineData("null", true, true)]
+        [InlineData("null", false, false)]
+        [InlineData(" ", true, true)]
+        [InlineData(" ", false, false)]
+        public async Task ReadAsync_WithInputThatDeserializesToNull_SetsModelOnlyIfAllowingEmptyInput(string content, bool allowEmptyInput, bool expectedIsModelSet)
+        {
+            // Arrange
+            var logger = GetLogger();
+            var formatter =
+                new JsonInputFormatter(logger, _serializerSettings, ArrayPool<char>.Shared, _objectPoolProvider);
+            var contentBytes = Encoding.UTF8.GetBytes(content);
+
+            var modelState = new ModelStateDictionary();
+            var httpContext = GetHttpContext(contentBytes);
+            var provider = new EmptyModelMetadataProvider();
+            var metadata = provider.GetMetadataForType(typeof(object));
+            var context = new InputFormatterContext(
+                httpContext,
+                modelName: string.Empty,
+                modelState: modelState,
+                metadata: metadata,
+                readerFactory: new TestHttpRequestStreamReaderFactory().CreateReader,
+                allowEmptyInput: allowEmptyInput);
+
+            // Act
+            var result = await formatter.ReadAsync(context);
+
+            // Assert
+            Assert.False(result.HasError);
+            Assert.Equal(expectedIsModelSet, result.IsModelSet);
+            Assert.Null(result.Model);
+        }
+
         [Fact]
         public void Constructor_UsesSerializerSettings()
         {