Skip to content

Commit 504f7f6

Browse files
pranavkmpranavkm
authored
Produce a ModelState error when reading the form throws (#14994)
* Introduce ValueProviderException analogous to InputFormatterException * Record ValueProviderException as a model state error * Fixup bug in reading ProblemDetails \ ValidationProblemDetails using the converter Fixes #10291
1 parent 30b31d7 commit 504f7f6

24 files changed

+538
-44
lines changed

‎src/Mvc/Mvc.Abstractions/ref/Microsoft.AspNetCore.Mvc.Abstractions.netcoreapp.cs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -823,6 +823,11 @@ public partial class TooManyModelErrorsException : System.Exception
823823
{
824824
public TooManyModelErrorsException(string message) { }
825825
}
826+
public sealed partial class ValueProviderException : System.Exception
827+
{
828+
public ValueProviderException(string message) { }
829+
public ValueProviderException(string message, System.Exception innerException) { }
830+
}
826831
public partial class ValueProviderFactoryContext
827832
{
828833
public ValueProviderFactoryContext(Microsoft.AspNetCore.Mvc.ActionContext context) { }

‎src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelStateDictionary.cs

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,13 @@ public bool TryAddModelException(string key, Exception exception)
203203
throw new ArgumentNullException(nameof(exception));
204204
}
205205

206+
if ((exception is InputFormatterException || exception is ValueProviderException)
207+
&& !string.IsNullOrEmpty(exception.Message))
208+
{
209+
// InputFormatterException, ValueProviderException is a signal that the message is safe to expose to clients
210+
return TryAddModelError(key, exception.Message);
211+
}
212+
206213
if (ErrorCount >= MaxAllowedErrors - 1)
207214
{
208215
EnsureMaxErrorsReachedRecorded();
@@ -311,9 +318,10 @@ public bool TryAddModelError(string key, Exception exception, ModelMetadata meta
311318

312319
return TryAddModelError(key, errorMessage);
313320
}
314-
else if (exception is InputFormatterException && !string.IsNullOrEmpty(exception.Message))
321+
else if ((exception is InputFormatterException || exception is ValueProviderException)
322+
&& !string.IsNullOrEmpty(exception.Message))
315323
{
316-
// InputFormatterException is a signal that the message is safe to expose to clients
324+
// InputFormatterException, ValueProviderException is a signal that the message is safe to expose to clients
317325
return TryAddModelError(key, exception.Message);
318326
}
319327

‎src/Mvc/Mvc.Abstractions/src/ModelBinding/ValueProviderException.cs

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System;
5+
6+
namespace Microsoft.AspNetCore.Mvc.ModelBinding
7+
{
8+
/// <summary>
9+
/// Exception thrown by <see cref="IValueProviderFactory"/> when the input is unable to be read.
10+
/// </summary>
11+
public sealed class ValueProviderException : Exception
12+
{
13+
/// <summary>
14+
/// Initializes a new instance of <see cref="ValueProviderException"/> with the specified <paramref name="message"/>.
15+
/// </summary>
16+
/// <param name="message">The exception message.</param>
17+
public ValueProviderException(string message)
18+
: base(message)
19+
{
20+
}
21+
22+
/// <summary>
23+
/// Initializes a new instance of <see cref="ValueProviderException"/> with the specified <paramref name="message"/> and
24+
/// inner exception that is the cause of this exception.
25+
/// </summary>
26+
/// <param name="message">The exception message.</param>
27+
/// <param name="innerException">The exception that is the cause of the current exception.</param>
28+
public ValueProviderException(string message, Exception innerException)
29+
: base(message, innerException)
30+
{
31+
}
32+
}
33+
}

‎src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs

Lines changed: 42 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1207,8 +1207,9 @@ public void ModelStateDictionary_NoErrorMessage_ForUnrecognizedException()
12071207
public void TryAddModelException_AddsErrorMessage_ForInputFormatterException()
12081208
{
12091209
// Arrange
1210+
var expectedMessage = "This is an InputFormatterException";
12101211
var dictionary = new ModelStateDictionary();
1211-
var exception = new InputFormatterException("This is an InputFormatterException.");
1212+
var exception = new InputFormatterException(expectedMessage);
12121213

12131214
// Act
12141215
dictionary.TryAddModelException("key", exception);
@@ -1217,7 +1218,25 @@ public void TryAddModelException_AddsErrorMessage_ForInputFormatterException()
12171218
var entry = Assert.Single(dictionary);
12181219
Assert.Equal("key", entry.Key);
12191220
var error = Assert.Single(entry.Value.Errors);
1220-
Assert.Same(exception, error.Exception);
1221+
Assert.Equal(expectedMessage, error.ErrorMessage);
1222+
}
1223+
1224+
[Fact]
1225+
public void TryAddModelException_AddsErrorMessage_ForValueProviderException()
1226+
{
1227+
// Arrange
1228+
var expectedMessage = "This is an ValueProviderException";
1229+
var dictionary = new ModelStateDictionary();
1230+
var exception = new ValueProviderException(expectedMessage);
1231+
1232+
// Act
1233+
dictionary.TryAddModelException("key", exception);
1234+
1235+
// Assert
1236+
var entry = Assert.Single(dictionary);
1237+
Assert.Equal("key", entry.Key);
1238+
var error = Assert.Single(entry.Value.Errors);
1239+
Assert.Equal(expectedMessage, error.ErrorMessage);
12211240
}
12221241

12231242
[Fact]
@@ -1227,9 +1246,7 @@ public void ModelStateDictionary_AddsErrorMessage_ForInputFormatterException()
12271246
var expectedMessage = "This is an InputFormatterException";
12281247
var dictionary = new ModelStateDictionary();
12291248

1230-
var bindingMetadataProvider = new DefaultBindingMetadataProvider();
1231-
var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider });
1232-
var provider = new DefaultModelMetadataProvider(compositeProvider, new OptionsAccessor());
1249+
var provider = new EmptyModelMetadataProvider();
12331250
var metadata = provider.GetMetadataForType(typeof(int));
12341251

12351252
// Act
@@ -1242,6 +1259,26 @@ public void ModelStateDictionary_AddsErrorMessage_ForInputFormatterException()
12421259
Assert.Equal(expectedMessage, error.ErrorMessage);
12431260
}
12441261

1262+
[Fact]
1263+
public void ModelStateDictionary_AddsErrorMessage_ForValueProviderException()
1264+
{
1265+
// Arrange
1266+
var expectedMessage = "This is an ValueProviderException";
1267+
var dictionary = new ModelStateDictionary();
1268+
1269+
var provider = new EmptyModelMetadataProvider();
1270+
var metadata = provider.GetMetadataForType(typeof(int));
1271+
1272+
// Act
1273+
dictionary.TryAddModelError("key", new ValueProviderException(expectedMessage), metadata);
1274+
1275+
// Assert
1276+
var entry = Assert.Single(dictionary);
1277+
Assert.Equal("key", entry.Key);
1278+
var error = Assert.Single(entry.Value.Errors);
1279+
Assert.Equal(expectedMessage, error.ErrorMessage);
1280+
}
1281+
12451282
[Fact]
12461283
public void ModelStateDictionary_ClearEntriesThatMatchWithKey_NonEmptyKey()
12471284
{

‎src/Mvc/Mvc.Core/src/ControllerBase.cs

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2452,7 +2452,12 @@ public virtual async Task<bool> TryUpdateModelAsync<TModel>(
24522452
throw new ArgumentNullException(nameof(prefix));
24532453
}
24542454

2455-
var valueProvider = await CompositeValueProvider.CreateAsync(ControllerContext);
2455+
var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(ControllerContext, ControllerContext.ValueProviderFactories);
2456+
if (!success)
2457+
{
2458+
return false;
2459+
}
2460+
24562461
return await TryUpdateModelAsync(model, prefix, valueProvider);
24572462
}
24582463

@@ -2526,7 +2531,12 @@ public async Task<bool> TryUpdateModelAsync<TModel>(
25262531
throw new ArgumentNullException(nameof(includeExpressions));
25272532
}
25282533

2529-
var valueProvider = await CompositeValueProvider.CreateAsync(ControllerContext);
2534+
var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(ControllerContext, ControllerContext.ValueProviderFactories);
2535+
if (!success)
2536+
{
2537+
return false;
2538+
}
2539+
25302540
return await ModelBindingHelper.TryUpdateModelAsync(
25312541
model,
25322542
prefix,
@@ -2565,7 +2575,12 @@ public async Task<bool> TryUpdateModelAsync<TModel>(
25652575
throw new ArgumentNullException(nameof(propertyFilter));
25662576
}
25672577

2568-
var valueProvider = await CompositeValueProvider.CreateAsync(ControllerContext);
2578+
var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(ControllerContext, ControllerContext.ValueProviderFactories);
2579+
if (!success)
2580+
{
2581+
return false;
2582+
}
2583+
25692584
return await ModelBindingHelper.TryUpdateModelAsync(
25702585
model,
25712586
prefix,
@@ -2693,7 +2708,12 @@ public virtual async Task<bool> TryUpdateModelAsync(
26932708
throw new ArgumentNullException(nameof(modelType));
26942709
}
26952710

2696-
var valueProvider = await CompositeValueProvider.CreateAsync(ControllerContext);
2711+
var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(ControllerContext, ControllerContext.ValueProviderFactories);
2712+
if (!success)
2713+
{
2714+
return false;
2715+
}
2716+
26972717
return await ModelBindingHelper.TryUpdateModelAsync(
26982718
model,
26992719
modelType,

‎src/Mvc/Mvc.Core/src/Controllers/ControllerBinderDelegateProvider.cs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,12 @@ public static ControllerBinderDelegate CreateBinderDelegate(
5959

6060
async Task Bind(ControllerContext controllerContext, object controller, Dictionary<string, object> arguments)
6161
{
62-
var valueProvider = await CompositeValueProvider.CreateAsync(controllerContext);
62+
var (success, valueProvider) = await CompositeValueProvider.TryCreateAsync(controllerContext, controllerContext.ValueProviderFactories);
63+
if (!success)
64+
{
65+
return;
66+
}
67+
6368
var parameters = actionDescriptor.Parameters;
6469

6570
for (var i = 0; i < parameters.Count; i++)

‎src/Mvc/Mvc.Core/src/Infrastructure/ProblemDetailsJsonConverter.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ public override ProblemDetails Read(ref Utf8JsonReader reader, Type typeToConver
2020
{
2121
var problemDetails = new ProblemDetails();
2222

23-
if (!reader.Read())
23+
if (reader.TokenType != JsonTokenType.StartObject)
2424
{
2525
throw new JsonException(Resources.UnexpectedJsonEnd);
2626
}

‎src/Mvc/Mvc.Core/src/Infrastructure/ValidationProblemDetailsJsonConverter.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ public override ValidationProblemDetails Read(ref Utf8JsonReader reader, Type ty
1818
{
1919
var problemDetails = new ValidationProblemDetails();
2020

21-
if (!reader.Read())
21+
if (reader.TokenType != JsonTokenType.StartObject)
2222
{
2323
throw new JsonException(Resources.UnexpectedJsonEnd);
2424
}

‎src/Mvc/Mvc.Core/src/ModelBinding/CompositeValueProvider.cs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,22 @@ public static async Task<CompositeValueProvider> CreateAsync(
8080
return new CompositeValueProvider(valueProviderFactoryContext.ValueProviders);
8181
}
8282

83+
internal static async ValueTask<(bool success, CompositeValueProvider valueProvider)> TryCreateAsync(
84+
ActionContext actionContext,
85+
IList<IValueProviderFactory> factories)
86+
{
87+
try
88+
{
89+
var valueProvider = await CreateAsync(actionContext, factories);
90+
return (true, valueProvider);
91+
}
92+
catch (ValueProviderException exception)
93+
{
94+
actionContext.ModelState.TryAddModelException(key: string.Empty, exception);
95+
return (false, null);
96+
}
97+
}
98+
8399
/// <inheritdoc />
84100
public virtual bool ContainsPrefix(string prefix)
85101
{

‎src/Mvc/Mvc.Core/src/ModelBinding/FormFileValueProviderFactory.cs

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
5+
using System.IO;
56
using System.Threading.Tasks;
67
using Microsoft.AspNetCore.Http;
8+
using Microsoft.AspNetCore.Mvc.Core;
79

810
namespace Microsoft.AspNetCore.Mvc.ModelBinding
911
{
@@ -32,10 +34,20 @@ public Task CreateValueProviderAsync(ValueProviderFactoryContext context)
3234

3335
private static async Task AddValueProviderAsync(ValueProviderFactoryContext context, HttpRequest request)
3436
{
35-
var formCollection = await request.ReadFormAsync();
36-
if (formCollection.Files.Count > 0)
37+
IFormCollection form;
38+
39+
try
40+
{
41+
form = await request.ReadFormAsync();
42+
}
43+
catch (InvalidDataException ex)
44+
{
45+
throw new ValueProviderException(Resources.FormatFailedToReadRequestForm(ex.Message), ex);
46+
}
47+
48+
if (form.Files.Count > 0)
3749
{
38-
var valueProvider = new FormFileValueProvider(formCollection.Files);
50+
var valueProvider = new FormFileValueProvider(form.Files);
3951
context.ValueProviders.Add(valueProvider);
4052
}
4153
}

‎src/Mvc/Mvc.Core/src/ModelBinding/FormValueProviderFactory.cs

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@
33

44
using System;
55
using System.Globalization;
6+
using System.IO;
67
using System.Threading.Tasks;
8+
using Microsoft.AspNetCore.Http;
9+
using Microsoft.AspNetCore.Mvc.Core;
710

811
namespace Microsoft.AspNetCore.Mvc.ModelBinding
912
{
@@ -33,9 +36,20 @@ public Task CreateValueProviderAsync(ValueProviderFactoryContext context)
3336
private static async Task AddValueProviderAsync(ValueProviderFactoryContext context)
3437
{
3538
var request = context.ActionContext.HttpContext.Request;
39+
IFormCollection form;
40+
41+
try
42+
{
43+
form = await request.ReadFormAsync();
44+
}
45+
catch (InvalidDataException ex)
46+
{
47+
throw new ValueProviderException(Resources.FormatFailedToReadRequestForm(ex.Message), ex);
48+
}
49+
3650
var valueProvider = new FormValueProvider(
3751
BindingSource.Form,
38-
await request.ReadFormAsync(),
52+
form,
3953
CultureInfo.CurrentCulture);
4054

4155
context.ValueProviders.Add(valueProvider);

‎src/Mvc/Mvc.Core/src/Resources.resx

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -513,4 +513,7 @@
513513
<data name="ApiConventions_Title_500" xml:space="preserve">
514514
<value>An error occured while processing your request.</value>
515515
</data>
516+
<data name="FailedToReadRequestForm" xml:space="preserve">
517+
<value>Failed to read the request form. {0}</value>
518+
</data>
516519
</root>

‎src/Mvc/Mvc.Core/test/ControllerBaseTest.cs

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2607,6 +2607,31 @@ public async Task TryUpdateModel_UsesModelValueProviderIfSpecified()
26072607
Assert.NotEqual(0, binder.BindModelCount);
26082608
}
26092609

2610+
[Fact]
2611+
public async Task TryUpdateModel_ReturnsFalse_IfValueProviderFactoryThrows()
2612+
{
2613+
// Arrange
2614+
var modelName = "mymodel";
2615+
2616+
var valueProviderFactory = new Mock<IValueProviderFactory>();
2617+
valueProviderFactory.Setup(f => f.CreateValueProviderAsync(It.IsAny<ValueProviderFactoryContext>()))
2618+
.Throws(new ValueProviderException("some error"));
2619+
2620+
var controller = GetController(new StubModelBinder());
2621+
controller.ControllerContext.ValueProviderFactories.Add(valueProviderFactory.Object);
2622+
var model = new MyModel();
2623+
2624+
// Act
2625+
var result = await controller.TryUpdateModelAsync(model, modelName);
2626+
2627+
// Assert
2628+
Assert.False(result);
2629+
var modelState = Assert.Single(controller.ModelState);
2630+
Assert.Empty(modelState.Key);
2631+
var error = Assert.Single(modelState.Value.Errors);
2632+
Assert.Equal("some error", error.ErrorMessage);
2633+
}
2634+
26102635
[Fact]
26112636
public async Task TryUpdateModel_PropertyFilterOverload_UsesPassedArguments()
26122637
{
@@ -3037,7 +3062,7 @@ public void RedirectToPage_WithPageName_Handler_AndRouteValues()
30373062
});
30383063
}
30393064

3040-
private static ControllerBase GetController(IModelBinder binder, IValueProvider valueProvider)
3065+
private static ControllerBase GetController(IModelBinder binder, IValueProvider valueProvider = null)
30413066
{
30423067
var metadataProvider = new EmptyModelMetadataProvider();
30433068
var services = new ServiceCollection();
@@ -3056,11 +3081,11 @@ private static ControllerBase GetController(IModelBinder binder, IValueProvider
30563081
stringLocalizerFactory: null),
30573082
};
30583083

3059-
valueProvider = valueProvider ?? new SimpleValueProvider();
3084+
valueProvider ??= new SimpleValueProvider();
30603085
var controllerContext = new ControllerContext()
30613086
{
30623087
HttpContext = httpContext,
3063-
ValueProviderFactories = new[] { new SimpleValueProviderFactory(valueProvider), },
3088+
ValueProviderFactories = new List<IValueProviderFactory> { new SimpleValueProviderFactory(valueProvider), },
30643089
};
30653090

30663091
var binderFactory = new Mock<IModelBinderFactory>();

0 commit comments

Comments
 (0)