diff --git a/src/Mvc/Mvc.Abstractions/ref/Microsoft.AspNetCore.Mvc.Abstractions.netcoreapp.cs b/src/Mvc/Mvc.Abstractions/ref/Microsoft.AspNetCore.Mvc.Abstractions.netcoreapp.cs index 3dc69d93e8..a13240eca5 100644 --- a/src/Mvc/Mvc.Abstractions/ref/Microsoft.AspNetCore.Mvc.Abstractions.netcoreapp.cs +++ b/src/Mvc/Mvc.Abstractions/ref/Microsoft.AspNetCore.Mvc.Abstractions.netcoreapp.cs @@ -823,6 +823,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding { public TooManyModelErrorsException(string message) { } } + public sealed partial class ValueProviderException : System.Exception + { + public ValueProviderException(string message) { } + public ValueProviderException(string message, System.Exception innerException) { } + } public partial class ValueProviderFactoryContext { public ValueProviderFactoryContext(Microsoft.AspNetCore.Mvc.ActionContext context) { } diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelStateDictionary.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelStateDictionary.cs index 40cb92c44c..53c475d199 100644 --- a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelStateDictionary.cs +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelStateDictionary.cs @@ -203,6 +203,13 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding throw new ArgumentNullException(nameof(exception)); } + if ((exception is InputFormatterException || exception is ValueProviderException) + && !string.IsNullOrEmpty(exception.Message)) + { + // InputFormatterException, ValueProviderException is a signal that the message is safe to expose to clients + return TryAddModelError(key, exception.Message); + } + if (ErrorCount >= MaxAllowedErrors - 1) { EnsureMaxErrorsReachedRecorded(); @@ -311,9 +318,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding return TryAddModelError(key, errorMessage); } - else if (exception is InputFormatterException && !string.IsNullOrEmpty(exception.Message)) + else if ((exception is InputFormatterException || exception is ValueProviderException) + && !string.IsNullOrEmpty(exception.Message)) { - // InputFormatterException is a signal that the message is safe to expose to clients + // InputFormatterException, ValueProviderException is a signal that the message is safe to expose to clients return TryAddModelError(key, exception.Message); } diff --git a/src/Mvc/Mvc.Abstractions/src/ModelBinding/ValueProviderException.cs b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ValueProviderException.cs new file mode 100644 index 0000000000..7607c72722 --- /dev/null +++ b/src/Mvc/Mvc.Abstractions/src/ModelBinding/ValueProviderException.cs @@ -0,0 +1,33 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; + +namespace Microsoft.AspNetCore.Mvc.ModelBinding +{ + /// + /// Exception thrown by when the input is unable to be read. + /// + public sealed class ValueProviderException : Exception + { + /// + /// Initializes a new instance of with the specified . + /// + /// The exception message. + public ValueProviderException(string message) + : base(message) + { + } + + /// + /// Initializes a new instance of with the specified and + /// inner exception that is the cause of this exception. + /// + /// The exception message. + /// The exception that is the cause of the current exception. + public ValueProviderException(string message, Exception innerException) + : base(message, innerException) + { + } + } +} diff --git a/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs b/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs index 2af7b47324..d5f13654f5 100644 --- a/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs +++ b/src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs @@ -1207,8 +1207,9 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding public void TryAddModelException_AddsErrorMessage_ForInputFormatterException() { // Arrange + var expectedMessage = "This is an InputFormatterException"; var dictionary = new ModelStateDictionary(); - var exception = new InputFormatterException("This is an InputFormatterException."); + var exception = new InputFormatterException(expectedMessage); // Act dictionary.TryAddModelException("key", exception); @@ -1217,7 +1218,25 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var entry = Assert.Single(dictionary); Assert.Equal("key", entry.Key); var error = Assert.Single(entry.Value.Errors); - Assert.Same(exception, error.Exception); + Assert.Equal(expectedMessage, error.ErrorMessage); + } + + [Fact] + public void TryAddModelException_AddsErrorMessage_ForValueProviderException() + { + // Arrange + var expectedMessage = "This is an ValueProviderException"; + var dictionary = new ModelStateDictionary(); + var exception = new ValueProviderException(expectedMessage); + + // Act + dictionary.TryAddModelException("key", exception); + + // Assert + var entry = Assert.Single(dictionary); + Assert.Equal("key", entry.Key); + var error = Assert.Single(entry.Value.Errors); + Assert.Equal(expectedMessage, error.ErrorMessage); } [Fact] @@ -1227,9 +1246,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var expectedMessage = "This is an InputFormatterException"; var dictionary = new ModelStateDictionary(); - var bindingMetadataProvider = new DefaultBindingMetadataProvider(); - var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider }); - var provider = new DefaultModelMetadataProvider(compositeProvider, new OptionsAccessor()); + var provider = new EmptyModelMetadataProvider(); var metadata = provider.GetMetadataForType(typeof(int)); // Act @@ -1242,6 +1259,26 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.Equal(expectedMessage, error.ErrorMessage); } + [Fact] + public void ModelStateDictionary_AddsErrorMessage_ForValueProviderException() + { + // Arrange + var expectedMessage = "This is an ValueProviderException"; + var dictionary = new ModelStateDictionary(); + + var provider = new EmptyModelMetadataProvider(); + var metadata = provider.GetMetadataForType(typeof(int)); + + // Act + dictionary.TryAddModelError("key", new ValueProviderException(expectedMessage), metadata); + + // Assert + var entry = Assert.Single(dictionary); + Assert.Equal("key", entry.Key); + var error = Assert.Single(entry.Value.Errors); + Assert.Equal(expectedMessage, error.ErrorMessage); + } + [Fact] public void ModelStateDictionary_ClearEntriesThatMatchWithKey_NonEmptyKey() { diff --git a/src/Mvc/Mvc.Core/src/ControllerBase.cs b/src/Mvc/Mvc.Core/src/ControllerBase.cs index dd83af24ad..90282b41a0 100644 --- a/src/Mvc/Mvc.Core/src/ControllerBase.cs +++ b/src/Mvc/Mvc.Core/src/ControllerBase.cs @@ -2452,7 +2452,12 @@ namespace Microsoft.AspNetCore.Mvc throw new ArgumentNullException(nameof(prefix)); } - var valueProvider = await CompositeValueProvider.CreateAsync(ControllerContext); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(ControllerContext, ControllerContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await TryUpdateModelAsync(model, prefix, valueProvider); } @@ -2526,7 +2531,12 @@ namespace Microsoft.AspNetCore.Mvc throw new ArgumentNullException(nameof(includeExpressions)); } - var valueProvider = await CompositeValueProvider.CreateAsync(ControllerContext); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(ControllerContext, ControllerContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await ModelBindingHelper.TryUpdateModelAsync( model, prefix, @@ -2565,7 +2575,12 @@ namespace Microsoft.AspNetCore.Mvc throw new ArgumentNullException(nameof(propertyFilter)); } - var valueProvider = await CompositeValueProvider.CreateAsync(ControllerContext); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(ControllerContext, ControllerContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await ModelBindingHelper.TryUpdateModelAsync( model, prefix, @@ -2693,7 +2708,12 @@ namespace Microsoft.AspNetCore.Mvc throw new ArgumentNullException(nameof(modelType)); } - var valueProvider = await CompositeValueProvider.CreateAsync(ControllerContext); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(ControllerContext, ControllerContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await ModelBindingHelper.TryUpdateModelAsync( model, modelType, diff --git a/src/Mvc/Mvc.Core/src/Controllers/ControllerBinderDelegateProvider.cs b/src/Mvc/Mvc.Core/src/Controllers/ControllerBinderDelegateProvider.cs index fcb0cd0d11..e7668e7b96 100644 --- a/src/Mvc/Mvc.Core/src/Controllers/ControllerBinderDelegateProvider.cs +++ b/src/Mvc/Mvc.Core/src/Controllers/ControllerBinderDelegateProvider.cs @@ -59,7 +59,12 @@ namespace Microsoft.AspNetCore.Mvc.Controllers async Task Bind(ControllerContext controllerContext, object controller, Dictionary arguments) { - var valueProvider = await CompositeValueProvider.CreateAsync(controllerContext); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(controllerContext, controllerContext.ValueProviderFactories); + if (!success) + { + return; + } + var parameters = actionDescriptor.Parameters; for (var i = 0; i < parameters.Count; i++) diff --git a/src/Mvc/Mvc.Core/src/Infrastructure/ProblemDetailsJsonConverter.cs b/src/Mvc/Mvc.Core/src/Infrastructure/ProblemDetailsJsonConverter.cs index 8b7e1f5576..267e25f758 100644 --- a/src/Mvc/Mvc.Core/src/Infrastructure/ProblemDetailsJsonConverter.cs +++ b/src/Mvc/Mvc.Core/src/Infrastructure/ProblemDetailsJsonConverter.cs @@ -20,7 +20,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure { var problemDetails = new ProblemDetails(); - if (!reader.Read()) + if (reader.TokenType != JsonTokenType.StartObject) { throw new JsonException(Resources.UnexpectedJsonEnd); } diff --git a/src/Mvc/Mvc.Core/src/Infrastructure/ValidationProblemDetailsJsonConverter.cs b/src/Mvc/Mvc.Core/src/Infrastructure/ValidationProblemDetailsJsonConverter.cs index d112ff7f18..0d24be6a83 100644 --- a/src/Mvc/Mvc.Core/src/Infrastructure/ValidationProblemDetailsJsonConverter.cs +++ b/src/Mvc/Mvc.Core/src/Infrastructure/ValidationProblemDetailsJsonConverter.cs @@ -18,7 +18,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure { var problemDetails = new ValidationProblemDetails(); - if (!reader.Read()) + if (reader.TokenType != JsonTokenType.StartObject) { throw new JsonException(Resources.UnexpectedJsonEnd); } diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/CompositeValueProvider.cs b/src/Mvc/Mvc.Core/src/ModelBinding/CompositeValueProvider.cs index eaf15df1be..b102fdb1c7 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/CompositeValueProvider.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/CompositeValueProvider.cs @@ -80,6 +80,22 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding return new CompositeValueProvider(valueProviderFactoryContext.ValueProviders); } + internal static async ValueTask<(bool success, CompositeValueProvider valueProvider)> TryCreateAsync( + ActionContext actionContext, + IList factories) + { + try + { + var valueProvider = await CreateAsync(actionContext, factories); + return (true, valueProvider); + } + catch (ValueProviderException exception) + { + actionContext.ModelState.TryAddModelException(key: string.Empty, exception); + return (false, null); + } + } + /// public virtual bool ContainsPrefix(string prefix) { diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/FormFileValueProviderFactory.cs b/src/Mvc/Mvc.Core/src/ModelBinding/FormFileValueProviderFactory.cs index 0f3ae8b8ff..df90de5039 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/FormFileValueProviderFactory.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/FormFileValueProviderFactory.cs @@ -2,8 +2,10 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.IO; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Core; namespace Microsoft.AspNetCore.Mvc.ModelBinding { @@ -32,10 +34,20 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding private static async Task AddValueProviderAsync(ValueProviderFactoryContext context, HttpRequest request) { - var formCollection = await request.ReadFormAsync(); - if (formCollection.Files.Count > 0) + IFormCollection form; + + try { - var valueProvider = new FormFileValueProvider(formCollection.Files); + form = await request.ReadFormAsync(); + } + catch (InvalidDataException ex) + { + throw new ValueProviderException(Resources.FormatFailedToReadRequestForm(ex.Message), ex); + } + + if (form.Files.Count > 0) + { + var valueProvider = new FormFileValueProvider(form.Files); context.ValueProviders.Add(valueProvider); } } diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/FormValueProviderFactory.cs b/src/Mvc/Mvc.Core/src/ModelBinding/FormValueProviderFactory.cs index f9febdef06..e7cbed8e11 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/FormValueProviderFactory.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/FormValueProviderFactory.cs @@ -3,7 +3,10 @@ using System; using System.Globalization; +using System.IO; using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Core; namespace Microsoft.AspNetCore.Mvc.ModelBinding { @@ -33,9 +36,20 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding private static async Task AddValueProviderAsync(ValueProviderFactoryContext context) { var request = context.ActionContext.HttpContext.Request; + IFormCollection form; + + try + { + form = await request.ReadFormAsync(); + } + catch (InvalidDataException ex) + { + throw new ValueProviderException(Resources.FormatFailedToReadRequestForm(ex.Message), ex); + } + var valueProvider = new FormValueProvider( BindingSource.Form, - await request.ReadFormAsync(), + form, CultureInfo.CurrentCulture); context.ValueProviders.Add(valueProvider); diff --git a/src/Mvc/Mvc.Core/src/Resources.resx b/src/Mvc/Mvc.Core/src/Resources.resx index 03647f8e23..273ac8531a 100644 --- a/src/Mvc/Mvc.Core/src/Resources.resx +++ b/src/Mvc/Mvc.Core/src/Resources.resx @@ -513,4 +513,7 @@ An error occured while processing your request. + + Failed to read the request form. {0} + \ No newline at end of file diff --git a/src/Mvc/Mvc.Core/test/ControllerBaseTest.cs b/src/Mvc/Mvc.Core/test/ControllerBaseTest.cs index 74e99b5942..32e9b998d7 100644 --- a/src/Mvc/Mvc.Core/test/ControllerBaseTest.cs +++ b/src/Mvc/Mvc.Core/test/ControllerBaseTest.cs @@ -2607,6 +2607,31 @@ namespace Microsoft.AspNetCore.Mvc.Core.Test Assert.NotEqual(0, binder.BindModelCount); } + [Fact] + public async Task TryUpdateModel_ReturnsFalse_IfValueProviderFactoryThrows() + { + // Arrange + var modelName = "mymodel"; + + var valueProviderFactory = new Mock(); + valueProviderFactory.Setup(f => f.CreateValueProviderAsync(It.IsAny())) + .Throws(new ValueProviderException("some error")); + + var controller = GetController(new StubModelBinder()); + controller.ControllerContext.ValueProviderFactories.Add(valueProviderFactory.Object); + var model = new MyModel(); + + // Act + var result = await controller.TryUpdateModelAsync(model, modelName); + + // Assert + Assert.False(result); + var modelState = Assert.Single(controller.ModelState); + Assert.Empty(modelState.Key); + var error = Assert.Single(modelState.Value.Errors); + Assert.Equal("some error", error.ErrorMessage); + } + [Fact] public async Task TryUpdateModel_PropertyFilterOverload_UsesPassedArguments() { @@ -3037,7 +3062,7 @@ namespace Microsoft.AspNetCore.Mvc.Core.Test }); } - private static ControllerBase GetController(IModelBinder binder, IValueProvider valueProvider) + private static ControllerBase GetController(IModelBinder binder, IValueProvider valueProvider = null) { var metadataProvider = new EmptyModelMetadataProvider(); var services = new ServiceCollection(); @@ -3056,11 +3081,11 @@ namespace Microsoft.AspNetCore.Mvc.Core.Test stringLocalizerFactory: null), }; - valueProvider = valueProvider ?? new SimpleValueProvider(); + valueProvider ??= new SimpleValueProvider(); var controllerContext = new ControllerContext() { HttpContext = httpContext, - ValueProviderFactories = new[] { new SimpleValueProviderFactory(valueProvider), }, + ValueProviderFactories = new List { new SimpleValueProviderFactory(valueProvider), }, }; var binderFactory = new Mock(); diff --git a/src/Mvc/Mvc.Core/test/Controllers/ControllerBinderDelegateProviderTest.cs b/src/Mvc/Mvc.Core/test/Controllers/ControllerBinderDelegateProviderTest.cs index a1bd64a817..97f879169e 100644 --- a/src/Mvc/Mvc.Core/test/Controllers/ControllerBinderDelegateProviderTest.cs +++ b/src/Mvc/Mvc.Core/test/Controllers/ControllerBinderDelegateProviderTest.cs @@ -1202,6 +1202,54 @@ namespace Microsoft.AspNetCore.Mvc.Controllers Assert.Equal(250.0, transferInfo.Amount); } + [Fact] + public async Task BinderDelegateRecordsErrorWhenValueProviderThrowsValueProviderException() + { + // Arrange + var actionDescriptor = new ControllerActionDescriptor() + { + BoundProperties = new List(), + Parameters = new[] { new ParameterDescriptor { Name = "name", ParameterType = typeof(string) } }, + }; + var modelMetadataProvider = new EmptyModelMetadataProvider(); + var modelBinderProvider = Mock.Of(); + var factory = TestModelBinderFactory.CreateDefault(modelBinderProvider); + var modelValidatorProvider = Mock.Of(); + var parameterBinder = new ParameterBinder( + new EmptyModelMetadataProvider(), + factory, + GetObjectValidator(modelMetadataProvider, modelValidatorProvider), + _optionsAccessor, + NullLoggerFactory.Instance); + + var valueProviderFactory = new Mock(); + valueProviderFactory.Setup(f => f.CreateValueProviderAsync(It.IsAny())) + .Throws(new ValueProviderException("Some error")); + + var controllerContext = GetControllerContext(actionDescriptor); + controllerContext.ValueProviderFactories.Add(valueProviderFactory.Object); + + var arguments = new Dictionary(StringComparer.Ordinal); + var modelState = controllerContext.ModelState; + + // Act + var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate( + parameterBinder, + factory, + TestModelMetadataProvider.CreateDefaultProvider(), + actionDescriptor, + _options); + + await binderDelegate(controllerContext, new TestController(), arguments); + + // Assert + var entry = Assert.Single(modelState); + Assert.Empty(entry.Key); + var error = Assert.Single(entry.Value.Errors); + + Assert.Equal("Some error", error.ErrorMessage); + } + private static ControllerContext GetControllerContext(ControllerActionDescriptor descriptor = null) { var services = new ServiceCollection(); diff --git a/src/Mvc/Mvc.Core/test/Infrastructure/ProblemDetailsConverterTest.cs b/src/Mvc/Mvc.Core/test/Infrastructure/ProblemDetailsJsonConverterTest.cs similarity index 80% rename from src/Mvc/Mvc.Core/test/Infrastructure/ProblemDetailsConverterTest.cs rename to src/Mvc/Mvc.Core/test/Infrastructure/ProblemDetailsJsonConverterTest.cs index aad71ca605..dd75501369 100644 --- a/src/Mvc/Mvc.Core/test/Infrastructure/ProblemDetailsConverterTest.cs +++ b/src/Mvc/Mvc.Core/test/Infrastructure/ProblemDetailsJsonConverterTest.cs @@ -8,7 +8,7 @@ using Xunit; namespace Microsoft.AspNetCore.Mvc.Infrastructure { - public class ProblemDetailsConverterTest + public class ProblemDetailsJsonConverterTest { private static JsonSerializerOptions JsonSerializerOptions => new JsonOptions().JsonSerializerOptions; @@ -41,6 +41,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure var json = $"{{\"type\":\"{type}\",\"title\":\"{title}\",\"status\":{status},\"detail\":\"{detail}\", \"instance\":\"{instance}\",\"traceId\":\"{traceId}\"}}"; var converter = new ProblemDetailsJsonConverter(); var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json)); + reader.Read(); // Act var problemDetails = converter.Read(ref reader, typeof(ProblemDetails), JsonSerializerOptions); @@ -59,6 +60,35 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure }); } + [Fact] + public void Read_UsingJsonSerializerWorks() + { + // Arrange + var type = "https://tools.ietf.org/html/rfc7231#section-6.5.4"; + var title = "Not found"; + var status = 404; + var detail = "Product not found"; + var instance = "http://example.com/products/14"; + var traceId = "|37dd3dd5-4a9619f953c40a16."; + var json = $"{{\"type\":\"{type}\",\"title\":\"{title}\",\"status\":{status},\"detail\":\"{detail}\", \"instance\":\"{instance}\",\"traceId\":\"{traceId}\"}}"; + + // Act + var problemDetails = JsonSerializer.Deserialize(json, JsonSerializerOptions); + + Assert.Equal(type, problemDetails.Type); + Assert.Equal(title, problemDetails.Title); + Assert.Equal(status, problemDetails.Status); + Assert.Equal(instance, problemDetails.Instance); + Assert.Equal(detail, problemDetails.Detail); + Assert.Collection( + problemDetails.Extensions, + kvp => + { + Assert.Equal("traceId", kvp.Key); + Assert.Equal(traceId, kvp.Value.ToString()); + }); + } + [Fact] public void Read_WithSomeMissingValues_Works() { @@ -70,6 +100,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure var json = $"{{\"type\":\"{type}\",\"title\":\"{title}\",\"status\":{status},\"traceId\":\"{traceId}\"}}"; var converter = new ProblemDetailsJsonConverter(); var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json)); + reader.Read(); // Act var problemDetails = converter.Read(ref reader, typeof(ProblemDetails), JsonSerializerOptions); diff --git a/src/Mvc/Mvc.Core/test/Infrastructure/ValidationProblemDetailsConverterTest.cs b/src/Mvc/Mvc.Core/test/Infrastructure/ValidationProblemDetailsJsonConverterTest.cs similarity index 70% rename from src/Mvc/Mvc.Core/test/Infrastructure/ValidationProblemDetailsConverterTest.cs rename to src/Mvc/Mvc.Core/test/Infrastructure/ValidationProblemDetailsJsonConverterTest.cs index f55da67a20..d81546315b 100644 --- a/src/Mvc/Mvc.Core/test/Infrastructure/ValidationProblemDetailsConverterTest.cs +++ b/src/Mvc/Mvc.Core/test/Infrastructure/ValidationProblemDetailsJsonConverterTest.cs @@ -9,7 +9,7 @@ using Xunit; namespace Microsoft.AspNetCore.Mvc.Infrastructure { - public class ValidationProblemDetailsConverterTest + public class ValidationProblemDetailsJsonConverterTest { private static JsonSerializerOptions JsonSerializerOptions => new JsonOptions().JsonSerializerOptions; @@ -27,6 +27,7 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure "\"errors\":{\"key0\":[\"error0\"],\"key1\":[\"error1\",\"error2\"]}}"; var converter = new ValidationProblemDetailsJsonConverter(); var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json)); + reader.Read(); // Act var problemDetails = converter.Read(ref reader, typeof(ValidationProblemDetails), JsonSerializerOptions); @@ -65,12 +66,14 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure var title = "Not found"; var status = 404; var traceId = "|37dd3dd5-4a9619f953c40a16."; - var json = $"{{\"type\":\"{type}\",\"title\":\"{title}\",\"status\":{status},\"traceId\":\"{traceId}\"}}"; - var converter = new ProblemDetailsJsonConverter(); + var json = $"{{\"type\":\"{type}\",\"title\":\"{title}\",\"status\":{status},\"traceId\":\"{traceId}\"," + + "\"errors\":{\"key0\":[\"error0\"],\"key1\":[\"error1\",\"error2\"]}}"; + var converter = new ValidationProblemDetailsJsonConverter(); var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json)); + reader.Read(); // Act - var problemDetails = converter.Read(ref reader, typeof(ProblemDetails), JsonSerializerOptions); + var problemDetails = converter.Read(ref reader, typeof(ValidationProblemDetails), JsonSerializerOptions); Assert.Equal(type, problemDetails.Type); Assert.Equal(title, problemDetails.Title); @@ -82,6 +85,56 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure Assert.Equal("traceId", kvp.Key); Assert.Equal(traceId, kvp.Value.ToString()); }); + Assert.Collection( + problemDetails.Errors.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("key0", kvp.Key); + Assert.Equal(new[] { "error0" }, kvp.Value); + }, + kvp => + { + Assert.Equal("key1", kvp.Key); + Assert.Equal(new[] { "error1", "error2" }, kvp.Value); + }); + } + + [Fact] + public void ReadUsingJsonSerializerWorks() + { + // Arrange + var type = "https://tools.ietf.org/html/rfc7231#section-6.5.4"; + var title = "Not found"; + var status = 404; + var traceId = "|37dd3dd5-4a9619f953c40a16."; + var json = $"{{\"type\":\"{type}\",\"title\":\"{title}\",\"status\":{status},\"traceId\":\"{traceId}\"," + + "\"errors\":{\"key0\":[\"error0\"],\"key1\":[\"error1\",\"error2\"]}}"; + + // Act + var problemDetails = JsonSerializer.Deserialize(json, JsonSerializerOptions); + + Assert.Equal(type, problemDetails.Type); + Assert.Equal(title, problemDetails.Title); + Assert.Equal(status, problemDetails.Status); + Assert.Collection( + problemDetails.Extensions, + kvp => + { + Assert.Equal("traceId", kvp.Key); + Assert.Equal(traceId, kvp.Value.ToString()); + }); + Assert.Collection( + problemDetails.Errors.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("key0", kvp.Key); + Assert.Equal(new[] { "error0" }, kvp.Value); + }, + kvp => + { + Assert.Equal("key1", kvp.Key); + Assert.Equal(new[] { "error1", "error2" }, kvp.Value); + }); } [Fact] diff --git a/src/Mvc/Mvc.RazorPages/src/Infrastructure/PageBinderFactory.cs b/src/Mvc/Mvc.RazorPages/src/Infrastructure/PageBinderFactory.cs index 574cf6e123..aab346896f 100644 --- a/src/Mvc/Mvc.RazorPages/src/Infrastructure/PageBinderFactory.cs +++ b/src/Mvc/Mvc.RazorPages/src/Infrastructure/PageBinderFactory.cs @@ -55,7 +55,12 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure async Task Bind(PageContext pageContext, object instance) { - var valueProvider = await CompositeValueProvider.CreateAsync(pageContext, pageContext.ValueProviderFactories); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(pageContext, pageContext.ValueProviderFactories); + if (!success) + { + return; + } + for (var i = 0; i < properties.Count; i++) { var property = properties[i]; @@ -131,7 +136,11 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure async Task Bind(PageContext pageContext, IDictionary arguments) { - var valueProvider = await CompositeValueProvider.CreateAsync(pageContext, pageContext.ValueProviderFactories); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(pageContext, pageContext.ValueProviderFactories); + if (!success) + { + return; + } for (var i = 0; i < parameterBindingInfo.Length; i++) { diff --git a/src/Mvc/Mvc.RazorPages/src/PageBase.cs b/src/Mvc/Mvc.RazorPages/src/PageBase.cs index 32f6810281..73b9fc03b2 100644 --- a/src/Mvc/Mvc.RazorPages/src/PageBase.cs +++ b/src/Mvc/Mvc.RazorPages/src/PageBase.cs @@ -1335,7 +1335,13 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages { throw new ArgumentNullException(nameof(prefix)); } - var valueProvider = await CompositeValueProvider.CreateAsync(PageContext, PageContext.ValueProviderFactories); + + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(PageContext, PageContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await TryUpdateModelAsync(model, prefix, valueProvider); } @@ -1407,7 +1413,12 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages throw new ArgumentNullException(nameof(includeExpressions)); } - var valueProvider = await CompositeValueProvider.CreateAsync(PageContext, PageContext.ValueProviderFactories); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(PageContext, PageContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await ModelBindingHelper.TryUpdateModelAsync( model, prefix, @@ -1445,7 +1456,12 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages throw new ArgumentNullException(nameof(propertyFilter)); } - var valueProvider = await CompositeValueProvider.CreateAsync(PageContext, PageContext.ValueProviderFactories); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(PageContext, PageContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await ModelBindingHelper.TryUpdateModelAsync( model, prefix, @@ -1570,7 +1586,12 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages throw new ArgumentNullException(nameof(modelType)); } - var valueProvider = await CompositeValueProvider.CreateAsync(PageContext, PageContext.ValueProviderFactories); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(PageContext, PageContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await ModelBindingHelper.TryUpdateModelAsync( model, modelType, diff --git a/src/Mvc/Mvc.RazorPages/src/PageModel.cs b/src/Mvc/Mvc.RazorPages/src/PageModel.cs index b3dbc5b9da..9821bb3a93 100644 --- a/src/Mvc/Mvc.RazorPages/src/PageModel.cs +++ b/src/Mvc/Mvc.RazorPages/src/PageModel.cs @@ -222,7 +222,12 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages throw new ArgumentNullException(nameof(name)); } - var valueProvider = await CompositeValueProvider.CreateAsync(PageContext, PageContext.ValueProviderFactories); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(PageContext, PageContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await TryUpdateModelAsync(model, name, valueProvider); } @@ -294,7 +299,12 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages throw new ArgumentNullException(nameof(includeExpressions)); } - var valueProvider = await CompositeValueProvider.CreateAsync(PageContext, PageContext.ValueProviderFactories); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(PageContext, PageContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await ModelBindingHelper.TryUpdateModelAsync( model, name, @@ -332,7 +342,12 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages throw new ArgumentNullException(nameof(propertyFilter)); } - var valueProvider = await CompositeValueProvider.CreateAsync(PageContext, PageContext.ValueProviderFactories); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(PageContext, PageContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await ModelBindingHelper.TryUpdateModelAsync( model, name, @@ -457,7 +472,12 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages throw new ArgumentNullException(nameof(modelType)); } - var valueProvider = await CompositeValueProvider.CreateAsync(PageContext, PageContext.ValueProviderFactories); + var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(PageContext, PageContext.ValueProviderFactories); + if (!success) + { + return false; + } + return await ModelBindingHelper.TryUpdateModelAsync( model, modelType, diff --git a/src/Mvc/Mvc.RazorPages/test/Infrastructure/PageBinderFactoryTest.cs b/src/Mvc/Mvc.RazorPages/test/Infrastructure/PageBinderFactoryTest.cs index 5458d7e339..ca9078bd72 100644 --- a/src/Mvc/Mvc.RazorPages/test/Infrastructure/PageBinderFactoryTest.cs +++ b/src/Mvc/Mvc.RazorPages/test/Infrastructure/PageBinderFactoryTest.cs @@ -709,6 +709,57 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Infrastructure }); } + [Fact] + public async Task FactoryRecordsErrorWhenValueProviderThrowsValueProviderException() + { + // Arrange + var type = typeof(PageModelWithExecutors); + var actionDescriptor = GetActionDescriptorWithHandlerMethod(type, nameof(PageModelWithExecutors.OnGet)); + + // Act + var parameterBinder = new TestParameterBinder(new Dictionary() + { + { "id", "value" }, + }); + + var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); + var modelBinderFactory = TestModelBinderFactory.CreateDefault(); + var factory = PageBinderFactory.CreateHandlerBinder( + parameterBinder, + modelMetadataProvider, + modelBinderFactory, + actionDescriptor, + actionDescriptor.HandlerMethods[0], + _options); + + var pageContext = GetPageContext(); + var page = new PageWithProperty + { + PageContext = pageContext, + }; + + var valueProviderFactory = new Mock(); + valueProviderFactory.Setup(f => f.CreateValueProviderAsync(It.IsAny())) + .Throws(new ValueProviderException("Some error")); + + pageContext.ValueProviderFactories.Add(valueProviderFactory.Object); + + var model = new PageModelWithExecutors(); + var arguments = new Dictionary(); + + // Act + await factory(page.PageContext, arguments); + + // Assert + var modelState = pageContext.ModelState; + var entry = Assert.Single(modelState); + Assert.Empty(entry.Key); + var error = Assert.Single(entry.Value.Errors); + + Assert.Equal("Some error", error.ErrorMessage); + } + + private static CompiledPageActionDescriptor GetActionDescriptorWithHandlerMethod(Type type, string method) { var handlerMethodInfo = type.GetMethod(method); diff --git a/src/Mvc/Mvc.RazorPages/test/PageModelTest.cs b/src/Mvc/Mvc.RazorPages/test/PageModelTest.cs index 762f22e5d1..3ec9ed5f6a 100644 --- a/src/Mvc/Mvc.RazorPages/test/PageModelTest.cs +++ b/src/Mvc/Mvc.RazorPages/test/PageModelTest.cs @@ -1739,6 +1739,35 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages Assert.Same(viewData, pageModel.ViewData); } + [Fact] + public async Task TryUpdateModel_ReturnsFalse_IfValueProviderFactoryThrows() + { + // Arrange + var valueProviderFactory = new Mock(); + valueProviderFactory.Setup(f => f.CreateValueProviderAsync(It.IsAny())) + .Throws(new ValueProviderException("some error")); + + var pageModel = new TestPageModel + { + PageContext = new PageContext + { + ValueProviderFactories = new[] { valueProviderFactory.Object }, + } + }; + + var model = new object(); + + // Act + var result = await pageModel.TryUpdateModelAsync(model); + + // Assert + Assert.False(result); + var modelState = Assert.Single(pageModel.ModelState); + Assert.Empty(modelState.Key); + var error = Assert.Single(modelState.Value.Errors); + Assert.Equal("some error", error.ErrorMessage); + } + [Fact] public void UrlHelperIsSet() { diff --git a/src/Mvc/Mvc.RazorPages/test/PageTest.cs b/src/Mvc/Mvc.RazorPages/test/PageTest.cs index 634986bdff..473fac2cde 100644 --- a/src/Mvc/Mvc.RazorPages/test/PageTest.cs +++ b/src/Mvc/Mvc.RazorPages/test/PageTest.cs @@ -1817,6 +1817,35 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages Assert.Same(viewData, result.ViewData); } + [Fact] + public async Task TryUpdateModel_ReturnsFalse_IfValueProviderFactoryThrows() + { + // Arrange + var valueProviderFactory = new Mock(); + valueProviderFactory.Setup(f => f.CreateValueProviderAsync(It.IsAny())) + .Throws(new ValueProviderException("some error")); + + var pageModel = new TestPage + { + PageContext = new PageContext + { + ValueProviderFactories = new[] { valueProviderFactory.Object }, + } + }; + + var model = new object(); + + // Act + var result = await pageModel.TryUpdateModelAsync(model); + + // Assert + Assert.False(result); + var modelState = Assert.Single(pageModel.ModelState); + Assert.Empty(modelState.Key); + var error = Assert.Single(modelState.Value.Errors); + Assert.Equal("some error", error.ErrorMessage); + } + public static IEnumerable RedirectTestData { get diff --git a/src/Mvc/test/Mvc.FunctionalTests/BasicTests.cs b/src/Mvc/test/Mvc.FunctionalTests/BasicTests.cs index 7477929732..7af4c54524 100644 --- a/src/Mvc/test/Mvc.FunctionalTests/BasicTests.cs +++ b/src/Mvc/test/Mvc.FunctionalTests/BasicTests.cs @@ -9,8 +9,7 @@ using System.Net.Http.Headers; using System.Reflection; using System.Threading.Tasks; using BasicWebSite.Models; -using Newtonsoft.Json; -using Newtonsoft.Json.Linq; +using System.Text.Json; using Xunit; namespace Microsoft.AspNetCore.Mvc.FunctionalTests @@ -444,7 +443,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests var response = await Client.GetStringAsync(url); // Assert - var result = JsonConvert.DeserializeObject(response); + var result = JsonSerializer.Deserialize(response, TestJsonSerializerOptionsProvider.Options); Assert.Equal(10, result.SampleInt); } @@ -458,7 +457,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests var response = await Client.GetStringAsync(url); // Assert - var result = JsonConvert.DeserializeObject(response); + var result = JsonSerializer.Deserialize(response, TestJsonSerializerOptionsProvider.Options); Assert.Equal(2, result.Length); } @@ -477,7 +476,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests { // Act var response = await Client.GetStringAsync("Home/GetAssemblyPartData"); - var assemblyParts = JsonConvert.DeserializeObject>(response); + var assemblyParts = JsonSerializer.Deserialize>(response, TestJsonSerializerOptionsProvider.Options); var expected = new[] { "BasicWebSite", @@ -548,7 +547,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests // Assert await response.AssertStatusCodeAsync(HttpStatusCode.OK); var content = await response.Content.ReadAsStringAsync(); - var data = JsonConvert.DeserializeObject(content); + var data = JsonSerializer.Deserialize(content, TestJsonSerializerOptionsProvider.Options); Assert.Equal("TestName", data.Name); Assert.Equal(10, data.Id); @@ -571,7 +570,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests // Assert await response.AssertStatusCodeAsync(HttpStatusCode.OK); var content = await response.Content.ReadAsStringAsync(); - var data = JsonConvert.DeserializeObject(content); + var data = JsonSerializer.Deserialize(content, TestJsonSerializerOptionsProvider.Options); Assert.Equal(10, data.Id); Assert.Null(data.IdFromRoute); @@ -593,7 +592,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests // Assert await response.AssertStatusCodeAsync(HttpStatusCode.OK); var content = await response.Content.ReadAsStringAsync(); - var data = JsonConvert.DeserializeObject(content); + var data = JsonSerializer.Deserialize(content, TestJsonSerializerOptionsProvider.Options); Assert.Null(data.BindNeverProperty); } @@ -628,6 +627,27 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.Equal("OnGetTestName", content); } + [Fact] + public async Task InvalidForm_ResultsInModelError() + { + // Arrange + var request = new HttpRequestMessage(HttpMethod.Post, "/Home/Product"); + request.Content = new MultipartFormDataContent(); + + var response = await Client.SendAsync(request); + + await response.AssertStatusCodeAsync(HttpStatusCode.BadRequest); + var content = await response.Content.ReadAsStringAsync(); + var problemDetails = JsonSerializer.Deserialize(content, TestJsonSerializerOptionsProvider.Options); + Assert.Collection( + problemDetails.Errors, + kvp => + { + Assert.Empty(kvp.Key); + Assert.Equal("Failed to read the request form. Form section has invalid Content-Disposition value: ", string.Join(" ", kvp.Value)); + }); + } + public class BindPropertyControllerData { public string Name { get; set; } diff --git a/src/Mvc/test/WebSites/BasicWebSite/Controllers/HomeController.cs b/src/Mvc/test/WebSites/BasicWebSite/Controllers/HomeController.cs index c2f1f402c2..802d738297 100644 --- a/src/Mvc/test/WebSites/BasicWebSite/Controllers/HomeController.cs +++ b/src/Mvc/test/WebSites/BasicWebSite/Controllers/HomeController.cs @@ -120,6 +120,11 @@ namespace BasicWebSite.Controllers [HttpPost] public IActionResult Product(Product product) { + if (!ModelState.IsValid) + { + return ValidationProblem(); + } + return RedirectToAction(); }