From f575677c9541e79e10c4fca5f1d9e46d1d72ccee Mon Sep 17 00:00:00 2001 From: Pranav K Date: Tue, 11 Jun 2019 10:40:42 -0700 Subject: [PATCH 1/8] Exception handling with SystemTextJsonInputFormatter --- .../SystemTextJsonInputFormatter.cs | 93 ++++++++++++++++++- .../Infrastructure/MvcCoreMvcOptionsSetup.cs | 2 +- .../src/NewtonsoftJsonLoggerExtensions.cs | 6 +- .../MvcSandbox/Controllers/HomeController.cs | 15 ++- 4 files changed, 106 insertions(+), 10 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs index 4b13a1b709..028c24894f 100644 --- a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs +++ b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs @@ -8,6 +8,8 @@ using System.Text.Json; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Formatters.Json; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.Extensions.Logging; namespace Microsoft.AspNetCore.Mvc.Formatters { @@ -16,13 +18,19 @@ namespace Microsoft.AspNetCore.Mvc.Formatters /// public class SystemTextJsonInputFormatter : TextInputFormatter, IInputFormatterExceptionPolicy { + private readonly ILogger _logger; + /// /// Initializes a new instance of . /// /// The . - public SystemTextJsonInputFormatter(JsonOptions options) + /// The . + public SystemTextJsonInputFormatter( + JsonOptions options, + ILogger logger) { SerializerOptions = options.JsonSerializerOptions; + _logger = logger; SupportedEncodings.Add(UTF8EncodingWithoutBOM); SupportedEncodings.Add(UTF16EncodingLittleEndian); @@ -67,6 +75,26 @@ namespace Microsoft.AspNetCore.Mvc.Formatters { model = await JsonSerializer.ReadAsync(inputStream, context.ModelType, SerializerOptions); } + catch (JsonException jsonException) + { + var path = jsonException.Path; + if (path.StartsWith("$.", StringComparison.Ordinal)) + { + path = path.Substring(2); + } + + // Handle path combinations such as ""+"Property", "Parent"+"Property", or "Parent"+"[12]". + var key = ModelNames.CreatePropertyModelName(context.ModelName, path); + + var formatterException = new InputFormatterException(jsonException.Message, jsonException); + + var metadata = GetPathMetadata(context.Metadata, path); + context.ModelState.TryAddModelError(key, formatterException, metadata); + + Log.JsonInputException(_logger, jsonException); + + return InputFormatterResult.Failure(); + } finally { if (inputStream is TranscodingReadStream transcoding) @@ -98,5 +126,68 @@ namespace Microsoft.AspNetCore.Mvc.Formatters return new TranscodingReadStream(httpContext.Request.Body, encoding); } + + // Keep in sync with NewtonsoftJsonInputFormatter.GetPatMetadata + private ModelMetadata GetPathMetadata(ModelMetadata metadata, string path) + { + var index = 0; + while (index >= 0 && index < path.Length) + { + if (path[index] == '[') + { + // At start of "[0]". + if (metadata.ElementMetadata == null) + { + // Odd case but don't throw just because ErrorContext had an odd-looking path. + break; + } + + metadata = metadata.ElementMetadata; + index = path.IndexOf(']', index); + } + else if (path[index] == '.' || path[index] == ']') + { + // Skip '.' in "prefix.property" or "[0].property" or ']' in "[0]". + index++; + } + else + { + // At start of "property", "property." or "property[0]". + var endIndex = path.IndexOfAny(new[] { '.', '[' }, index); + if (endIndex == -1) + { + endIndex = path.Length; + } + + var propertyName = path.Substring(index, endIndex - index); + if (metadata.Properties[propertyName] == null) + { + // Odd case but don't throw just because ErrorContext had an odd-looking path. + break; + } + + metadata = metadata.Properties[propertyName]; + index = endIndex; + } + } + + return metadata; + } + + private static class Log + { + private static readonly Action _jsonInputFormatterException; + + static Log() + { + _jsonInputFormatterException = LoggerMessage.Define( + LogLevel.Debug, + new EventId(1, "SystemTextJsonInputException"), + "JSON input formatter threw an exception."); + } + + public static void JsonInputException(ILogger logger, Exception exception) + => _jsonInputFormatterException(logger, exception); + } } } diff --git a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs index b94ff39557..79005a8389 100644 --- a/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs +++ b/src/Mvc/Mvc.Core/src/Infrastructure/MvcCoreMvcOptionsSetup.cs @@ -78,7 +78,7 @@ namespace Microsoft.AspNetCore.Mvc options.Filters.Add(new UnsupportedContentTypeFilter()); // Set up default input formatters. - options.InputFormatters.Add(new SystemTextJsonInputFormatter(_jsonOptions.Value)); + options.InputFormatters.Add(new SystemTextJsonInputFormatter(_jsonOptions.Value, _loggerFactory.CreateLogger())); // Set up default output formatters. options.OutputFormatters.Add(new HttpNoContentOutputFormatter()); diff --git a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonLoggerExtensions.cs b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonLoggerExtensions.cs index 14dcd92174..dfad01025e 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonLoggerExtensions.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonLoggerExtensions.cs @@ -8,13 +8,13 @@ namespace Microsoft.AspNetCore.Mvc.NewtonsoftJson { internal static class NewtonsoftJsonLoggerExtensions { - private static readonly Action _jsonInputFormatterCrashed; + private static readonly Action _jsonInputFormatterException; private static readonly Action _jsonResultExecuting; static NewtonsoftJsonLoggerExtensions() { - _jsonInputFormatterCrashed = LoggerMessage.Define( + _jsonInputFormatterException = LoggerMessage.Define( LogLevel.Debug, new EventId(1, "JsonInputException"), "JSON input formatter threw an exception."); @@ -27,7 +27,7 @@ namespace Microsoft.AspNetCore.Mvc.NewtonsoftJson public static void JsonInputException(this ILogger logger, Exception exception) { - _jsonInputFormatterCrashed(logger, exception); + _jsonInputFormatterException(logger, exception); } public static void JsonResultExecuting(this ILogger logger, object value) diff --git a/src/Mvc/samples/MvcSandbox/Controllers/HomeController.cs b/src/Mvc/samples/MvcSandbox/Controllers/HomeController.cs index 2aa4ff6829..751004d291 100644 --- a/src/Mvc/samples/MvcSandbox/Controllers/HomeController.cs +++ b/src/Mvc/samples/MvcSandbox/Controllers/HomeController.cs @@ -5,14 +5,19 @@ using Microsoft.AspNetCore.Mvc; namespace MvcSandbox.Controllers { + [ApiController] public class HomeController : Controller { - [ModelBinder] - public string Id { get; set; } - - public IActionResult Index() + [HttpPost("/")] + public IActionResult Index(Person person) { - return View(); + return Ok(person); } } + + public class Person + { + public int Id { get; set; } + } + } From 6bd4b87d2c7bd1ac16fd4929e9e983e5cdadf56c Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Thu, 13 Jun 2019 10:58:37 -0700 Subject: [PATCH 2/8] Additional tests --- .../SystemTextJsonInputFormatter.cs | 5 + .../Formatters/JsonInputFormatterTestBase.cs | 98 ++++++++++++++++--- .../SystemTextJsonInputFormatterTest.cs | 50 +++++++++- .../src/NewtonsoftJsonInputFormatter.cs | 3 +- .../test/NewtonsoftJsonInputFormatterTest.cs | 41 +++++++- .../MvcSandbox/Controllers/HomeController.cs | 15 +-- 6 files changed, 182 insertions(+), 30 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs index 028c24894f..15f59080d1 100644 --- a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs +++ b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs @@ -83,6 +83,11 @@ namespace Microsoft.AspNetCore.Mvc.Formatters path = path.Substring(2); } + if (path.StartsWith("$[", StringComparison.Ordinal)) + { + path = path.Substring(1); + } + // Handle path combinations such as ""+"Property", "Parent"+"Property", or "Parent"+"[12]". var key = ModelNames.CreatePropertyModelName(context.ModelName, path); diff --git a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs b/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs index e7190037e9..fe94632904 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs +++ b/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs @@ -3,16 +3,20 @@ using System; using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; using System.IO; using System.Text; +using System.Text.Json; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.Extensions.Logging.Testing; +using Newtonsoft.Json; using Xunit; namespace Microsoft.AspNetCore.Mvc.Formatters { - public abstract class JsonInputFormatterTestBase + public abstract class JsonInputFormatterTestBase : LoggedTest { [Theory] [InlineData("application/json", true)] @@ -199,6 +203,26 @@ namespace Microsoft.AspNetCore.Mvc.Formatters Assert.Equal(new int[] { 0, 23, 300 }, (IEnumerable)result.Model); } + [Fact] + public virtual async Task ReadAsync_ArrayOfObjects_HasCorrectKey() + { + // Arrange + var formatter = GetInputFormatter(); + + var content = "[{\"Age\": 5}, {\"Age\": 3}, {\"Age\": \"Cheese\"} ]"; + var contentBytes = Encoding.UTF8.GetBytes(content); + var httpContext = GetHttpContext(contentBytes); + + var formatterContext = CreateInputFormatterContext(typeof(List), httpContext); + + // Act + var result = await formatter.ReadAsync(formatterContext); + + // Assert + Assert.True(result.HasError, "Model should have had an error!"); + Assert.Single(formatterContext.ModelState["[2].Age"].Errors); + } + [Fact] public virtual async Task ReadAsync_AddsModelValidationErrorsToModelState() { @@ -215,10 +239,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters var result = await formatter.ReadAsync(formatterContext); // Assert - Assert.True(result.HasError); - Assert.Equal( - "Could not convert string to decimal: not-an-age. Path 'Age', line 1, position 44.", - formatterContext.ModelState["Age"].Errors[0].ErrorMessage); + Assert.True(result.HasError, "Model should have had an error!"); + Assert.Single(formatterContext.ModelState["Age"].Errors); } [Fact] @@ -227,19 +249,18 @@ namespace Microsoft.AspNetCore.Mvc.Formatters // Arrange var formatter = GetInputFormatter(); - var content = "[0, 23, 300]"; + var content = "[0, 23, 33767]"; var contentBytes = Encoding.UTF8.GetBytes(content); var httpContext = GetHttpContext(contentBytes); - var formatterContext = CreateInputFormatterContext(typeof(byte[]), httpContext); + var formatterContext = CreateInputFormatterContext(typeof(short[]), httpContext); // Act var result = await formatter.ReadAsync(formatterContext); // Assert - Assert.True(result.HasError); - Assert.Equal("The supplied value is invalid.", formatterContext.ModelState["[2]"].Errors[0].ErrorMessage); - Assert.Null(formatterContext.ModelState["[2]"].Errors[0].Exception); + Assert.True(result.HasError, "Model should have produced an error!"); + Assert.True(formatterContext.ModelState.ContainsKey("[2]"), "Should have contained key '[2]'"); } [Fact] @@ -259,9 +280,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters // Assert Assert.True(result.HasError); - Assert.Equal( - "Error converting value 300 to type 'System.Byte'. Path '[1].Small', line 1, position 69.", - formatterContext.ModelState["names[1].Small"].Errors[0].ErrorMessage); + Assert.Single(formatterContext.ModelState["names[1].Small"].Errors); } [Fact] @@ -318,6 +337,45 @@ namespace Microsoft.AspNetCore.Mvc.Formatters Assert.Null(result.Model); } + [Fact] + public async Task ReadAsync_ComplexPoco() + { + // Arrange + var formatter = GetInputFormatter(); + + var content = "{ \"Id\": 5, \"Person\": { \"Name\": \"name\", \"Numbers\": [3, 2, \"Hamburger\"]} }"; + var contentBytes = Encoding.UTF8.GetBytes(content); + var httpContext = GetHttpContext(contentBytes); + + var formatterContext = CreateInputFormatterContext(typeof(ComplexPoco), httpContext); + + // Act + var result = await formatter.ReadAsync(formatterContext); + + // Assert + Assert.True(result.HasError, "Model should have had an error!"); + Assert.Single(formatterContext.ModelState["Person.Numbers[2]"].Errors); + } + + [Fact] + public virtual async Task ReadAsync_RequiredAttribute() + { + // Arrange + var formatter = GetInputFormatter(); + var content = "{ \"Id\": 5, \"Person\": {\"Numbers\": [3]} }"; + var contentBytes = Encoding.UTF8.GetBytes(content); + var httpContext = GetHttpContext(contentBytes); + + var formatterContext = CreateInputFormatterContext(typeof(ComplexPoco), httpContext); + + // Act + var result = await formatter.ReadAsync(formatterContext); + + // Assert + Assert.True(result.HasError, "Model should have had an error!"); + Assert.Single(formatterContext.ModelState["Person.Name"].Errors); + } + protected abstract TextInputFormatter GetInputFormatter(); protected static HttpContext GetHttpContext( @@ -356,6 +414,20 @@ namespace Microsoft.AspNetCore.Mvc.Formatters treatEmptyInputAsDefaultValue: treatEmptyInputAsDefaultValue); } + protected sealed class ComplexPoco + { + public int Id { get; set; } + public Person Person{ get; set; } + } + + protected sealed class Person + { + [Required] + [JsonProperty(Required = Required.Always)] + public string Name { get; set; } + public IEnumerable Numbers { get; set; } + } + protected sealed class ComplexModel { public string Name { get; set; } diff --git a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs b/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs index 8cea7ff821..b4d33dad96 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs +++ b/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs @@ -1,40 +1,80 @@ // 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; +using System.Collections.Generic; +using System.Text; using System.Threading.Tasks; +using Microsoft.Extensions.Logging; using Xunit; namespace Microsoft.AspNetCore.Mvc.Formatters { public class SystemTextJsonInputFormatterTest : JsonInputFormatterTestBase { - [Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/8474")] + [Fact] public override Task ReadAsync_AddsModelValidationErrorsToModelState() { return base.ReadAsync_AddsModelValidationErrorsToModelState(); } - [Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/8474")] + [Fact] public override Task ReadAsync_InvalidArray_AddsOverflowErrorsToModelState() { return base.ReadAsync_InvalidArray_AddsOverflowErrorsToModelState(); } - [Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/8474")] + [Fact] public override Task ReadAsync_InvalidComplexArray_AddsOverflowErrorsToModelState() { return base.ReadAsync_InvalidComplexArray_AddsOverflowErrorsToModelState(); } - [Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/8474")] + [Fact] public override Task ReadAsync_UsesTryAddModelValidationErrorsToModelState() { return base.ReadAsync_UsesTryAddModelValidationErrorsToModelState(); } + [Fact(Skip = "https://github.com/dotnet/corefx/issues/38492")] + public override Task ReadAsync_RequiredAttribute() + { + // System.Text.Json does not yet support an equivalent of Required. + throw new NotImplementedException(); + } + + [Fact] + public async Task ReadAsync_SingleError() + { + // Arrange + var failTotal = 0; + var formatter = GetInputFormatter(); + + var content = "[5, 'seven', 3, notnum ]"; + var contentBytes = Encoding.UTF8.GetBytes(content); + var httpContext = GetHttpContext(contentBytes); + + var formatterContext = CreateInputFormatterContext(typeof(List), httpContext); + + // Act + await formatter.ReadAsync(formatterContext); + + // Assert + foreach(var modelState in formatterContext.ModelState) + { + foreach(var error in modelState.Value.Errors) + { + failTotal++; + Assert.StartsWith("''' is an invalid start of a value", error.ErrorMessage); + } + } + + Assert.Equal(1, failTotal); + } + protected override TextInputFormatter GetInputFormatter() { - return new SystemTextJsonInputFormatter(new JsonOptions()); + return new SystemTextJsonInputFormatter(new JsonOptions(), LoggerFactory.CreateLogger()); } } } diff --git a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs index 31a96631fc..ca30b611c5 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs @@ -226,7 +226,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters } else { - addMember = !path.EndsWith("." + member, StringComparison.Ordinal); + addMember = !path.EndsWith($".{member}", StringComparison.Ordinal) + && !path.EndsWith($"[{member}]", StringComparison.Ordinal); } } } diff --git a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs b/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs index f9d79a6f6d..8f773cba66 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs @@ -3,7 +3,7 @@ using System; using System.Buffers; -using System.IO; +using System.Collections.Generic; using System.Linq; using System.Text; using System.Threading.Tasks; @@ -235,6 +235,45 @@ namespace Microsoft.AspNetCore.Mvc.Formatters Assert.Equal(expectedMessage, modelError.ErrorMessage); } + [Theory] + [InlineData("[5,7,3]", 0)] + [InlineData("[5, 'seven', 3]", 1)] + [InlineData("[5, 'seven', 3, 'notnum']", 2)] + public async Task ReadAsync_AllowMultipleErrors(string content, int failCount) + { + // Arrange + var failTotal = 0; + var serializerSettings = new JsonSerializerSettings + { + Error = delegate (object sender, ErrorEventArgs args) + { + args.ErrorContext.Handled = true; + } + }; + + var formatter = CreateFormatter(serializerSettings: serializerSettings, allowInputFormatterExceptionMessages: true); + + var contentBytes = Encoding.UTF8.GetBytes(content); + var httpContext = GetHttpContext(contentBytes); + + var formatterContext = CreateInputFormatterContext(typeof(List), httpContext); + + // Act + var result = await formatter.ReadAsync(formatterContext); + + // Assert + foreach (var modelState in formatterContext.ModelState) + { + foreach (var error in modelState.Value.Errors) + { + failTotal++; + Assert.StartsWith("Could not convert string to integer:", error.ErrorMessage); + } + } + + Assert.Equal(failCount, failTotal); + } + [Fact] public async Task ReadAsync_DoNotAllowInputFormatterExceptionMessages_DoesNotWrapJsonInputExceptions() { diff --git a/src/Mvc/samples/MvcSandbox/Controllers/HomeController.cs b/src/Mvc/samples/MvcSandbox/Controllers/HomeController.cs index 751004d291..2aa4ff6829 100644 --- a/src/Mvc/samples/MvcSandbox/Controllers/HomeController.cs +++ b/src/Mvc/samples/MvcSandbox/Controllers/HomeController.cs @@ -5,19 +5,14 @@ using Microsoft.AspNetCore.Mvc; namespace MvcSandbox.Controllers { - [ApiController] public class HomeController : Controller { - [HttpPost("/")] - public IActionResult Index(Person person) + [ModelBinder] + public string Id { get; set; } + + public IActionResult Index() { - return Ok(person); + return View(); } } - - public class Person - { - public int Id { get; set; } - } - } From 6e2a617ed81bd44e11fb8c3e2943e4be0fe5968a Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Thu, 13 Jun 2019 12:01:28 -0700 Subject: [PATCH 3/8] Update ref package --- .../Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs | 2 +- src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs b/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs index bf33019b86..b270c45725 100644 --- a/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs +++ b/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs @@ -1876,7 +1876,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters } public partial class SystemTextJsonInputFormatter : Microsoft.AspNetCore.Mvc.Formatters.TextInputFormatter, Microsoft.AspNetCore.Mvc.Formatters.IInputFormatterExceptionPolicy { - public SystemTextJsonInputFormatter(Microsoft.AspNetCore.Mvc.JsonOptions options) { } + public SystemTextJsonInputFormatter(Microsoft.AspNetCore.Mvc.JsonOptions options, Microsoft.Extensions.Logging.ILogger logger) { } Microsoft.AspNetCore.Mvc.Formatters.InputFormatterExceptionPolicy Microsoft.AspNetCore.Mvc.Formatters.IInputFormatterExceptionPolicy.ExceptionPolicy { get { throw null; } } public System.Text.Json.JsonSerializerOptions SerializerOptions { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } [System.Diagnostics.DebuggerStepThroughAttribute] diff --git a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs index 15f59080d1..9c4bfe402d 100644 --- a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs +++ b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs @@ -132,7 +132,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters return new TranscodingReadStream(httpContext.Request.Body, encoding); } - // Keep in sync with NewtonsoftJsonInputFormatter.GetPatMetadata + // Keep in sync with NewtonsoftJsonInputFormatter.GetPathMetadata private ModelMetadata GetPathMetadata(ModelMetadata metadata, string path) { var index = 0; From e69cca6461fc14eb44cabaa9b78ff6e07df07cde Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Fri, 14 Jun 2019 16:25:29 -0700 Subject: [PATCH 4/8] PR feedback --- .../SystemTextJsonInputFormatter.cs | 49 +---------------- .../Mvc.Core/src/ModelBinding/ModelNames.cs | 46 ++++++++++++++++ .../SystemTextJsonInputFormatterTest.cs | 17 +++--- .../src/NewtonsoftJsonInputFormatter.cs | 52 ++----------------- .../test/NewtonsoftJsonInputFormatterTest.cs | 38 ++++++-------- src/Mvc/Mvc.sln | 15 ++++++ 6 files changed, 88 insertions(+), 129 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs index 9c4bfe402d..bb21bc64df 100644 --- a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs +++ b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs @@ -93,7 +93,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters var formatterException = new InputFormatterException(jsonException.Message, jsonException); - var metadata = GetPathMetadata(context.Metadata, path); + var metadata = ModelNames.GetPathMetadata(context.Metadata, path); context.ModelState.TryAddModelError(key, formatterException, metadata); Log.JsonInputException(_logger, jsonException); @@ -132,53 +132,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters return new TranscodingReadStream(httpContext.Request.Body, encoding); } - // Keep in sync with NewtonsoftJsonInputFormatter.GetPathMetadata - private ModelMetadata GetPathMetadata(ModelMetadata metadata, string path) - { - var index = 0; - while (index >= 0 && index < path.Length) - { - if (path[index] == '[') - { - // At start of "[0]". - if (metadata.ElementMetadata == null) - { - // Odd case but don't throw just because ErrorContext had an odd-looking path. - break; - } - - metadata = metadata.ElementMetadata; - index = path.IndexOf(']', index); - } - else if (path[index] == '.' || path[index] == ']') - { - // Skip '.' in "prefix.property" or "[0].property" or ']' in "[0]". - index++; - } - else - { - // At start of "property", "property." or "property[0]". - var endIndex = path.IndexOfAny(new[] { '.', '[' }, index); - if (endIndex == -1) - { - endIndex = path.Length; - } - - var propertyName = path.Substring(index, endIndex - index); - if (metadata.Properties[propertyName] == null) - { - // Odd case but don't throw just because ErrorContext had an odd-looking path. - break; - } - - metadata = metadata.Properties[propertyName]; - index = endIndex; - } - } - - return metadata; - } - private static class Log { private static readonly Action _jsonInputFormatterException; diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs b/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs index 185c8345e0..4f366e2a70 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs @@ -39,5 +39,51 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding return prefix + "." + propertyName; } + + public static ModelMetadata GetPathMetadata(ModelMetadata metadata, string path) + { + var index = 0; + while (index >= 0 && index < path.Length) + { + if (path[index] == '[') + { + // At start of "[0]". + if (metadata.ElementMetadata == null) + { + // Odd case but don't throw just because ErrorContext had an odd-looking path. + break; + } + + metadata = metadata.ElementMetadata; + index = path.IndexOf(']', index); + } + else if (path[index] == '.' || path[index] == ']') + { + // Skip '.' in "prefix.property" or "[0].property" or ']' in "[0]". + index++; + } + else + { + // At start of "property", "property." or "property[0]". + var endIndex = path.IndexOfAny(new[] { '.', '[' }, index); + if (endIndex == -1) + { + endIndex = path.Length; + } + + var propertyName = path.Substring(index, endIndex - index); + if (metadata.Properties[propertyName] == null) + { + // Odd case but don't throw just because ErrorContext had an odd-looking path. + break; + } + + metadata = metadata.Properties[propertyName]; + index = endIndex; + } + } + + return metadata; + } } } diff --git a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs b/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs index b4d33dad96..aa43db0221 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs +++ b/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Text; using System.Threading.Tasks; using Microsoft.Extensions.Logging; @@ -47,7 +48,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters public async Task ReadAsync_SingleError() { // Arrange - var failTotal = 0; var formatter = GetInputFormatter(); var content = "[5, 'seven', 3, notnum ]"; @@ -59,17 +59,14 @@ namespace Microsoft.AspNetCore.Mvc.Formatters // Act await formatter.ReadAsync(formatterContext); - // Assert - foreach(var modelState in formatterContext.ModelState) - { - foreach(var error in modelState.Value.Errors) + Assert.Collection( + formatterContext.ModelState.OrderBy(k => k), + kvp => { - failTotal++; + Assert.Equal("[1]", kvp.Key); + var error = Assert.Single(kvp.Value.Errors); Assert.StartsWith("''' is an invalid start of a value", error.ErrorMessage); - } - } - - Assert.Equal(1, failTotal); + }); } protected override TextInputFormatter GetInputFormatter() diff --git a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs index ca30b611c5..438f996032 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs @@ -226,8 +226,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters } else { - addMember = !path.EndsWith($".{member}", StringComparison.Ordinal) - && !path.EndsWith($"[{member}]", StringComparison.Ordinal); + addMember = !path.EndsWith("." + member, StringComparison.Ordinal) + && !path.EndsWith("[" + member + "]", StringComparison.Ordinal); } } } @@ -242,7 +242,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters exception = eventArgs.ErrorContext.Error; - var metadata = GetPathMetadata(context.Metadata, path); + var metadata = ModelNames.GetPathMetadata(context.Metadata, path); var modelStateException = WrapExceptionForModelState(exception); context.ModelState.TryAddModelError(key, modelStateException, metadata); @@ -300,52 +300,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters protected virtual void ReleaseJsonSerializer(JsonSerializer serializer) => _jsonSerializerPool.Return(serializer); - private ModelMetadata GetPathMetadata(ModelMetadata metadata, string path) - { - var index = 0; - while (index >= 0 && index < path.Length) - { - if (path[index] == '[') - { - // At start of "[0]". - if (metadata.ElementMetadata == null) - { - // Odd case but don't throw just because ErrorContext had an odd-looking path. - break; - } - - metadata = metadata.ElementMetadata; - index = path.IndexOf(']', index); - } - else if (path[index] == '.' || path[index] == ']') - { - // Skip '.' in "prefix.property" or "[0].property" or ']' in "[0]". - index++; - } - else - { - // At start of "property", "property." or "property[0]". - var endIndex = path.IndexOfAny(new[] { '.', '[' }, index); - if (endIndex == -1) - { - endIndex = path.Length; - } - - var propertyName = path.Substring(index, endIndex - index); - if (metadata.Properties[propertyName] == null) - { - // Odd case but don't throw just because ErrorContext had an odd-looking path. - break; - } - - metadata = metadata.Properties[propertyName]; - index = endIndex; - } - } - - return metadata; - } - private Exception WrapExceptionForModelState(Exception exception) { // In 2.0 and earlier we always gave a generic error message for errors that come from JSON.NET diff --git a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs b/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs index 8f773cba66..b1a2234a5d 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs @@ -235,23 +235,13 @@ namespace Microsoft.AspNetCore.Mvc.Formatters Assert.Equal(expectedMessage, modelError.ErrorMessage); } - [Theory] - [InlineData("[5,7,3]", 0)] - [InlineData("[5, 'seven', 3]", 1)] - [InlineData("[5, 'seven', 3, 'notnum']", 2)] - public async Task ReadAsync_AllowMultipleErrors(string content, int failCount) + [Fact] + public async Task ReadAsync_AllowMultipleErrors() { // Arrange - var failTotal = 0; - var serializerSettings = new JsonSerializerSettings - { - Error = delegate (object sender, ErrorEventArgs args) - { - args.ErrorContext.Handled = true; - } - }; + var content = "[5, 'seven', 3, 'notnum']"; - var formatter = CreateFormatter(serializerSettings: serializerSettings, allowInputFormatterExceptionMessages: true); + var formatter = CreateFormatter(allowInputFormatterExceptionMessages: true); var contentBytes = Encoding.UTF8.GetBytes(content); var httpContext = GetHttpContext(contentBytes); @@ -262,16 +252,20 @@ namespace Microsoft.AspNetCore.Mvc.Formatters var result = await formatter.ReadAsync(formatterContext); // Assert - foreach (var modelState in formatterContext.ModelState) - { - foreach (var error in modelState.Value.Errors) + Assert.Collection( + formatterContext.ModelState.OrderBy(k => k), + kvp => { - failTotal++; + Assert.Equal("[1]", kvp.Key); + var error = Assert.Single(kvp.Value.Errors); Assert.StartsWith("Could not convert string to integer:", error.ErrorMessage); - } - } - - Assert.Equal(failCount, failTotal); + }, + kvp => + { + Assert.Equal("[3]", kvp.Key); + var error = Assert.Single(kvp.Value.Errors); + Assert.StartsWith("Could not convert string to integer:", error.ErrorMessage); + }); } [Fact] diff --git a/src/Mvc/Mvc.sln b/src/Mvc/Mvc.sln index 2a8c20227b..dd3d0a4fd3 100644 --- a/src/Mvc/Mvc.sln +++ b/src/Mvc/Mvc.sln @@ -325,6 +325,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Extensions.ApiDescription.S EndProject Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.ApiDescription.Server", "Extensions.ApiDescription.Server\src\Microsoft.Extensions.ApiDescription.Server.csproj", "{D7CF2A1E-A29E-45AB-9C2A-CD6C3BAE54F2}" EndProject +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.AspNetCore.Metadata", "..\Http\Metadata\src\Microsoft.AspNetCore.Metadata.csproj", "{464195B3-022A-4D19-9104-8C66CC882D67}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -1841,6 +1843,18 @@ Global {D7CF2A1E-A29E-45AB-9C2A-CD6C3BAE54F2}.Release|Mixed Platforms.Build.0 = Release|Any CPU {D7CF2A1E-A29E-45AB-9C2A-CD6C3BAE54F2}.Release|x86.ActiveCfg = Release|Any CPU {D7CF2A1E-A29E-45AB-9C2A-CD6C3BAE54F2}.Release|x86.Build.0 = Release|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Debug|Any CPU.Build.0 = Debug|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Debug|Mixed Platforms.ActiveCfg = Debug|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Debug|Mixed Platforms.Build.0 = Debug|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Debug|x86.ActiveCfg = Debug|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Debug|x86.Build.0 = Debug|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Release|Any CPU.ActiveCfg = Release|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Release|Any CPU.Build.0 = Release|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Release|Mixed Platforms.ActiveCfg = Release|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Release|Mixed Platforms.Build.0 = Release|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Release|x86.ActiveCfg = Release|Any CPU + {464195B3-022A-4D19-9104-8C66CC882D67}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -1972,6 +1986,7 @@ Global {C6F3BCE6-1EFD-4360-932B-B98573E78926} = {45CE788D-4B69-4F83-981C-F43D8F15B0F1} {637119E8-5BBB-4FC7-A372-DAF38FF5EBD9} = {5FE3048A-E96B-44F8-A7C4-FC590D7E04B4} {D7CF2A1E-A29E-45AB-9C2A-CD6C3BAE54F2} = {C15AA245-9E54-4FD6-90FF-B46F47779C46} + {464195B3-022A-4D19-9104-8C66CC882D67} = {5FE3048A-E96B-44F8-A7C4-FC590D7E04B4} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {63D344F6-F86D-40E6-85B9-0AABBE338C4A} From c7b401ffd782d70d7a5285da92dc6fd36bd97f2f Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Wed, 19 Jun 2019 09:35:21 -0700 Subject: [PATCH 5/8] Test fixes and feedback --- .../SystemTextJsonInputFormatter.cs | 21 +++-- .../Formatters/JsonInputFormatterTestBase.cs | 77 ++++++++++++++++++- .../SystemTextJsonInputFormatterTest.cs | 2 + .../src/NewtonsoftJsonInputFormatter.cs | 1 + .../test/NewtonsoftJsonInputFormatterTest.cs | 4 +- 5 files changed, 97 insertions(+), 8 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs index bb21bc64df..d93260032f 100644 --- a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs +++ b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs @@ -118,6 +118,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters } else { + Log.JsonInputSuccess(_logger, context.ModelType); return InputFormatterResult.Success(model); } } @@ -134,18 +135,26 @@ namespace Microsoft.AspNetCore.Mvc.Formatters private static class Log { - private static readonly Action _jsonInputFormatterException; + private static readonly Action _jsonInputFormatterException; + private static readonly Action _jsonInputSuccess; static Log() { - _jsonInputFormatterException = LoggerMessage.Define( - LogLevel.Debug, - new EventId(1, "SystemTextJsonInputException"), - "JSON input formatter threw an exception."); + _jsonInputFormatterException = LoggerMessage.Define( + LogLevel.Debug, + new EventId(1, "SystemTextJsonInputException"), + "JSON input formatter threw an exception: {Message}"); + _jsonInputSuccess = LoggerMessage.Define( + LogLevel.Debug, + new EventId(2, "SystemTextJsonInputSuccess"), + "JSON input formatter succeeded, deserializing to type '{TypeName}'"); } public static void JsonInputException(ILogger logger, Exception exception) - => _jsonInputFormatterException(logger, exception); + => _jsonInputFormatterException(logger, exception.Message, exception); + + public static void JsonInputSuccess(ILogger logger, Type modelType) + => _jsonInputSuccess(logger, modelType.FullName, null); } } } diff --git a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs b/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs index fe94632904..af680361ea 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs +++ b/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using System.ComponentModel.DataAnnotations; using System.IO; +using System.Linq; using System.Text; using System.Text.Json; using System.Threading.Tasks; @@ -18,6 +19,14 @@ namespace Microsoft.AspNetCore.Mvc.Formatters { public abstract class JsonInputFormatterTestBase : LoggedTest { + internal enum Formatter + { + Newtonsoft, + SystemText + } + + internal abstract Formatter CurrentFormatter { get; } + [Theory] [InlineData("application/json", true)] [InlineData("application/*", false)] @@ -106,6 +115,67 @@ namespace Microsoft.AspNetCore.Mvc.Formatters Assert.Equal("abcd", stringValue); } + [Fact] + public async Task JsonFormatter_EscapedKeys_Bracket() + { + // Arrange + var content = "[{\"It[s a key\":1234556}]"; + var formatter = GetInputFormatter(); + + var contentBytes = Encoding.UTF8.GetBytes(content); + var httpContext = GetHttpContext(contentBytes); + + var formatterContext = CreateInputFormatterContext(typeof(IEnumerable>), httpContext); + + // Act + var result = await formatter.ReadAsync(formatterContext); + + // Assert + Assert.True(result.HasError); + Assert.Collection( + formatterContext.ModelState.OrderBy(k => k.Key), + kvp => + { + Assert.Equal("[0][\'It[s a key\']", kvp.Key); + }); + } + + [Fact] + public async Task JsonFormatter_EscapedKeys() + { + // Arrange + var content = "[{\"It\\\"s a key\": 1234556}]"; + var formatter = GetInputFormatter(); + + var contentBytes = Encoding.UTF8.GetBytes(content); + var httpContext = GetHttpContext(contentBytes); + + var formatterContext = CreateInputFormatterContext( + typeof(IEnumerable>), httpContext); + + // Act + var result = await formatter.ReadAsync(formatterContext); + + // Assert + Assert.True(result.HasError); + Assert.Collection( + formatterContext.ModelState.OrderBy(k => k.Key), + kvp => + { + switch(CurrentFormatter) + { + case Formatter.Newtonsoft: + Assert.Equal("[0]['It\"s a key']", kvp.Key); + break; + case Formatter.SystemText: + Assert.Equal("[0][\'It\\u0022s a key\']", kvp.Key); + break; + default: + throw new NotImplementedException(); + } + }); + } + [Fact] public virtual async Task JsonFormatterReadsDateTimeValue() { @@ -280,7 +350,12 @@ namespace Microsoft.AspNetCore.Mvc.Formatters // Assert Assert.True(result.HasError); - Assert.Single(formatterContext.ModelState["names[1].Small"].Errors); + Assert.Collection( + formatterContext.ModelState.OrderBy(k => k.Key), + kvp => { + Assert.Equal("names[1].Small", kvp.Key); + Assert.Single(kvp.Value.Errors); + }); } [Fact] diff --git a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs b/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs index aa43db0221..9ef9630a45 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs +++ b/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs @@ -13,6 +13,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters { public class SystemTextJsonInputFormatterTest : JsonInputFormatterTestBase { + internal override Formatter CurrentFormatter => Formatter.SystemText; + [Fact] public override Task ReadAsync_AddsModelValidationErrorsToModelState() { diff --git a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs index 438f996032..49a5581b87 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs @@ -227,6 +227,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters else { addMember = !path.EndsWith("." + member, StringComparison.Ordinal) + && !path.EndsWith("['" + member + "']", StringComparison.Ordinal) && !path.EndsWith("[" + member + "]", StringComparison.Ordinal); } } diff --git a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs b/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs index b1a2234a5d..fa29d0289f 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs @@ -24,6 +24,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters private static readonly ObjectPoolProvider _objectPoolProvider = new DefaultObjectPoolProvider(); private static readonly JsonSerializerSettings _serializerSettings = new JsonSerializerSettings(); + internal override Formatter CurrentFormatter => Formatter.Newtonsoft; + [Fact] public async Task Constructor_BuffersRequestBody_UsingDefaultOptions() { @@ -253,7 +255,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters // Assert Assert.Collection( - formatterContext.ModelState.OrderBy(k => k), + formatterContext.ModelState.OrderBy(k => k.Key), kvp => { Assert.Equal("[1]", kvp.Key); From aeaa0376d1572f57281b8fd13eb416ff1fe675da Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Wed, 19 Jun 2019 11:06:40 -0700 Subject: [PATCH 6/8] Update refs --- .../Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs b/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs index b270c45725..2af1c78d22 100644 --- a/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs +++ b/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs @@ -2320,6 +2320,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding public static string CreateIndexModelName(string parentName, int index) { throw null; } public static string CreateIndexModelName(string parentName, string index) { throw null; } public static string CreatePropertyModelName(string prefix, string propertyName) { throw null; } + public static Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata GetPathMetadata(Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata metadata, string path) { throw null; } } public abstract partial class ObjectModelValidator : Microsoft.AspNetCore.Mvc.ModelBinding.Validation.IObjectModelValidator { From 8f99ca3fad9634dfe07eb1d7e5c97978dd784fe3 Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Wed, 19 Jun 2019 15:08:47 -0700 Subject: [PATCH 7/8] Restructure tests --- .../SystemTextJsonInputFormatter.cs | 14 +--- .../Formatters/JsonInputFormatterTestBase.cs | 79 ++++++++++++------- .../SystemTextJsonInputFormatterTest.cs | 30 ++++++- .../test/NewtonsoftJsonInputFormatterTest.cs | 28 ++++++- 4 files changed, 106 insertions(+), 45 deletions(-) diff --git a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs index d93260032f..63dadc82b2 100644 --- a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs +++ b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs @@ -78,23 +78,11 @@ namespace Microsoft.AspNetCore.Mvc.Formatters catch (JsonException jsonException) { var path = jsonException.Path; - if (path.StartsWith("$.", StringComparison.Ordinal)) - { - path = path.Substring(2); - } - - if (path.StartsWith("$[", StringComparison.Ordinal)) - { - path = path.Substring(1); - } - - // Handle path combinations such as ""+"Property", "Parent"+"Property", or "Parent"+"[12]". - var key = ModelNames.CreatePropertyModelName(context.ModelName, path); var formatterException = new InputFormatterException(jsonException.Message, jsonException); var metadata = ModelNames.GetPathMetadata(context.Metadata, path); - context.ModelState.TryAddModelError(key, formatterException, metadata); + context.ModelState.TryAddModelError(path, formatterException, metadata); Log.JsonInputException(_logger, jsonException); diff --git a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs b/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs index af680361ea..fd76501888 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs +++ b/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs @@ -19,14 +19,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters { public abstract class JsonInputFormatterTestBase : LoggedTest { - internal enum Formatter - { - Newtonsoft, - SystemText - } - - internal abstract Formatter CurrentFormatter { get; } - [Theory] [InlineData("application/json", true)] [InlineData("application/*", false)] @@ -116,8 +108,10 @@ namespace Microsoft.AspNetCore.Mvc.Formatters } [Fact] - public async Task JsonFormatter_EscapedKeys_Bracket() + public virtual async Task JsonFormatter_EscapedKeys_Bracket() { + var expectedKey = JsonFormatter_EscapedKeys_Bracket_Expected; + // Arrange var content = "[{\"It[s a key\":1234556}]"; var formatter = GetInputFormatter(); @@ -136,13 +130,15 @@ namespace Microsoft.AspNetCore.Mvc.Formatters formatterContext.ModelState.OrderBy(k => k.Key), kvp => { - Assert.Equal("[0][\'It[s a key\']", kvp.Key); + Assert.Equal(expectedKey, kvp.Key); }); } [Fact] - public async Task JsonFormatter_EscapedKeys() + public virtual async Task JsonFormatter_EscapedKeys() { + var expectedKey = JsonFormatter_EscapedKeys_Expected; + // Arrange var content = "[{\"It\\\"s a key\": 1234556}]"; var formatter = GetInputFormatter(); @@ -162,17 +158,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters formatterContext.ModelState.OrderBy(k => k.Key), kvp => { - switch(CurrentFormatter) - { - case Formatter.Newtonsoft: - Assert.Equal("[0]['It\"s a key']", kvp.Key); - break; - case Formatter.SystemText: - Assert.Equal("[0][\'It\\u0022s a key\']", kvp.Key); - break; - default: - throw new NotImplementedException(); - } + Assert.Equal(expectedKey, kvp.Key); }); } @@ -285,12 +271,19 @@ namespace Microsoft.AspNetCore.Mvc.Formatters var formatterContext = CreateInputFormatterContext(typeof(List), httpContext); + var expectedKey = ReadAsync_ArrayOfObjects_HasCorrectKey_Expected; + // Act var result = await formatter.ReadAsync(formatterContext); // Assert Assert.True(result.HasError, "Model should have had an error!"); - Assert.Single(formatterContext.ModelState["[2].Age"].Errors); + Assert.Collection(formatterContext.ModelState.OrderBy(k => k.Key), + kvp => + { + Assert.Equal(expectedKey, kvp.Key); + Assert.Single(kvp.Value.Errors); + }); } [Fact] @@ -304,13 +297,19 @@ namespace Microsoft.AspNetCore.Mvc.Formatters var httpContext = GetHttpContext(contentBytes); var formatterContext = CreateInputFormatterContext(typeof(ComplexModel), httpContext); + var expectedKey = ReadAsync_AddsModelValidationErrorsToModelState_Expected; // Act var result = await formatter.ReadAsync(formatterContext); // Assert Assert.True(result.HasError, "Model should have had an error!"); - Assert.Single(formatterContext.ModelState["Age"].Errors); + Assert.Collection(formatterContext.ModelState.OrderBy(k => k.Key), + kvp => + { + Assert.Equal(expectedKey, kvp.Key); + Assert.Single(kvp.Value.Errors); + }); } [Fact] @@ -325,12 +324,17 @@ namespace Microsoft.AspNetCore.Mvc.Formatters var formatterContext = CreateInputFormatterContext(typeof(short[]), httpContext); + var expectedValue = ReadAsync_InvalidArray_AddsOverflowErrorsToModelState_Expected; + // Act var result = await formatter.ReadAsync(formatterContext); // Assert Assert.True(result.HasError, "Model should have produced an error!"); - Assert.True(formatterContext.ModelState.ContainsKey("[2]"), "Should have contained key '[2]'"); + Assert.Collection(formatterContext.ModelState.OrderBy(k => k.Key), + kvp => { + Assert.Equal(expectedValue, kvp.Key); + }); } [Fact] @@ -344,6 +348,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters var httpContext = GetHttpContext(contentBytes); var formatterContext = CreateInputFormatterContext(typeof(ComplexModel[]), httpContext, modelName: "names"); + var expectedKey = ReadAsync_InvalidComplexArray_AddsOverflowErrorsToModelState_Expected; // Act var result = await formatter.ReadAsync(formatterContext); @@ -353,7 +358,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters Assert.Collection( formatterContext.ModelState.OrderBy(k => k.Key), kvp => { - Assert.Equal("names[1].Small", kvp.Key); + Assert.Equal(expectedKey, kvp.Key); Assert.Single(kvp.Value.Errors); }); } @@ -424,12 +429,18 @@ namespace Microsoft.AspNetCore.Mvc.Formatters var formatterContext = CreateInputFormatterContext(typeof(ComplexPoco), httpContext); + var expectedKey = ReadAsync_ComplexPoco_Expected; + // Act var result = await formatter.ReadAsync(formatterContext); // Assert Assert.True(result.HasError, "Model should have had an error!"); - Assert.Single(formatterContext.ModelState["Person.Numbers[2]"].Errors); + Assert.Collection(formatterContext.ModelState.OrderBy(k => k.Key), + kvp => { + Assert.Equal(expectedKey, kvp.Key); + Assert.Single(kvp.Value.Errors); + }); } [Fact] @@ -451,6 +462,20 @@ namespace Microsoft.AspNetCore.Mvc.Formatters Assert.Single(formatterContext.ModelState["Person.Name"].Errors); } + internal abstract string JsonFormatter_EscapedKeys_Bracket_Expected { get; } + + internal abstract string JsonFormatter_EscapedKeys_Expected { get; } + + internal abstract string ReadAsync_ArrayOfObjects_HasCorrectKey_Expected { get; } + + internal abstract string ReadAsync_AddsModelValidationErrorsToModelState_Expected { get; } + + internal abstract string ReadAsync_InvalidArray_AddsOverflowErrorsToModelState_Expected { get; } + + internal abstract string ReadAsync_InvalidComplexArray_AddsOverflowErrorsToModelState_Expected { get; } + + internal abstract string ReadAsync_ComplexPoco_Expected { get; } + protected abstract TextInputFormatter GetInputFormatter(); protected static HttpContext GetHttpContext( diff --git a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs b/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs index 9ef9630a45..411725c7a9 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs +++ b/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs @@ -13,8 +13,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters { public class SystemTextJsonInputFormatterTest : JsonInputFormatterTestBase { - internal override Formatter CurrentFormatter => Formatter.SystemText; - [Fact] public override Task ReadAsync_AddsModelValidationErrorsToModelState() { @@ -46,6 +44,18 @@ namespace Microsoft.AspNetCore.Mvc.Formatters throw new NotImplementedException(); } + [Fact] + public override Task JsonFormatter_EscapedKeys() + { + return base.JsonFormatter_EscapedKeys(); + } + + [Fact] + public override Task JsonFormatter_EscapedKeys_Bracket() + { + return base.JsonFormatter_EscapedKeys_Bracket(); + } + [Fact] public async Task ReadAsync_SingleError() { @@ -65,7 +75,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters formatterContext.ModelState.OrderBy(k => k), kvp => { - Assert.Equal("[1]", kvp.Key); + Assert.Equal("$[1]", kvp.Key); var error = Assert.Single(kvp.Value.Errors); Assert.StartsWith("''' is an invalid start of a value", error.ErrorMessage); }); @@ -75,5 +85,19 @@ namespace Microsoft.AspNetCore.Mvc.Formatters { return new SystemTextJsonInputFormatter(new JsonOptions(), LoggerFactory.CreateLogger()); } + + internal override string ReadAsync_AddsModelValidationErrorsToModelState_Expected => "$.Age"; + + internal override string JsonFormatter_EscapedKeys_Expected => "$[0]['It\\u0022s a key']"; + + internal override string JsonFormatter_EscapedKeys_Bracket_Expected => "$[0]['It[s a key']"; + + internal override string ReadAsync_ArrayOfObjects_HasCorrectKey_Expected => "$[2].Age"; + + internal override string ReadAsync_InvalidArray_AddsOverflowErrorsToModelState_Expected => "$[2]"; + + internal override string ReadAsync_InvalidComplexArray_AddsOverflowErrorsToModelState_Expected => "$[1].Small"; + + internal override string ReadAsync_ComplexPoco_Expected => "$.Person.Numbers[2]"; } } diff --git a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs b/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs index fa29d0289f..d9b1406a05 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs @@ -24,8 +24,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters private static readonly ObjectPoolProvider _objectPoolProvider = new DefaultObjectPoolProvider(); private static readonly JsonSerializerSettings _serializerSettings = new JsonSerializerSettings(); - internal override Formatter CurrentFormatter => Formatter.Newtonsoft; - [Fact] public async Task Constructor_BuffersRequestBody_UsingDefaultOptions() { @@ -198,6 +196,18 @@ namespace Microsoft.AspNetCore.Mvc.Formatters Assert.Equal(settings.DateTimeZoneHandling, actual.DateTimeZoneHandling); } + [Fact] + public override Task JsonFormatter_EscapedKeys() + { + return base.JsonFormatter_EscapedKeys(); + } + + [Fact] + public override Task JsonFormatter_EscapedKeys_Bracket() + { + return base.JsonFormatter_EscapedKeys_Bracket(); + } + [Theory] [InlineData(" ", true, true)] [InlineData(" ", false, false)] @@ -360,6 +370,20 @@ namespace Microsoft.AspNetCore.Mvc.Formatters }); } + internal override string JsonFormatter_EscapedKeys_Expected => "[0]['It\"s a key']"; + + internal override string JsonFormatter_EscapedKeys_Bracket_Expected => "[0][\'It[s a key\']"; + + internal override string ReadAsync_AddsModelValidationErrorsToModelState_Expected => "Age"; + + internal override string ReadAsync_ArrayOfObjects_HasCorrectKey_Expected => "[2].Age"; + + internal override string ReadAsync_ComplexPoco_Expected => "Person.Numbers[2]"; + + internal override string ReadAsync_InvalidComplexArray_AddsOverflowErrorsToModelState_Expected => "names[1].Small"; + + internal override string ReadAsync_InvalidArray_AddsOverflowErrorsToModelState_Expected => "[2]"; + private class Location { public int Id { get; set; } From f6f1923e2bbecb71672aa7be79e525a34853f5e4 Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Wed, 19 Jun 2019 15:25:34 -0700 Subject: [PATCH 8/8] Cleanup --- ...osoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs | 1 - .../SystemTextJsonInputFormatter.cs | 3 +- .../Mvc.Core/src/ModelBinding/ModelNames.cs | 46 ------------------ .../src/NewtonsoftJsonInputFormatter.cs | 48 ++++++++++++++++++- 4 files changed, 48 insertions(+), 50 deletions(-) diff --git a/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs b/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs index 2af1c78d22..b270c45725 100644 --- a/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs +++ b/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs @@ -2320,7 +2320,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding public static string CreateIndexModelName(string parentName, int index) { throw null; } public static string CreateIndexModelName(string parentName, string index) { throw null; } public static string CreatePropertyModelName(string prefix, string propertyName) { throw null; } - public static Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata GetPathMetadata(Microsoft.AspNetCore.Mvc.ModelBinding.ModelMetadata metadata, string path) { throw null; } } public abstract partial class ObjectModelValidator : Microsoft.AspNetCore.Mvc.ModelBinding.Validation.IObjectModelValidator { diff --git a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs index 63dadc82b2..d584f47f82 100644 --- a/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs +++ b/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonInputFormatter.cs @@ -81,8 +81,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters var formatterException = new InputFormatterException(jsonException.Message, jsonException); - var metadata = ModelNames.GetPathMetadata(context.Metadata, path); - context.ModelState.TryAddModelError(path, formatterException, metadata); + context.ModelState.TryAddModelError(path, formatterException, context.Metadata); Log.JsonInputException(_logger, jsonException); diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs b/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs index 4f366e2a70..185c8345e0 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs @@ -39,51 +39,5 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding return prefix + "." + propertyName; } - - public static ModelMetadata GetPathMetadata(ModelMetadata metadata, string path) - { - var index = 0; - while (index >= 0 && index < path.Length) - { - if (path[index] == '[') - { - // At start of "[0]". - if (metadata.ElementMetadata == null) - { - // Odd case but don't throw just because ErrorContext had an odd-looking path. - break; - } - - metadata = metadata.ElementMetadata; - index = path.IndexOf(']', index); - } - else if (path[index] == '.' || path[index] == ']') - { - // Skip '.' in "prefix.property" or "[0].property" or ']' in "[0]". - index++; - } - else - { - // At start of "property", "property." or "property[0]". - var endIndex = path.IndexOfAny(new[] { '.', '[' }, index); - if (endIndex == -1) - { - endIndex = path.Length; - } - - var propertyName = path.Substring(index, endIndex - index); - if (metadata.Properties[propertyName] == null) - { - // Odd case but don't throw just because ErrorContext had an odd-looking path. - break; - } - - metadata = metadata.Properties[propertyName]; - index = endIndex; - } - } - - return metadata; - } } } diff --git a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs index 49a5581b87..8975f9f426 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs +++ b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs @@ -243,7 +243,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters exception = eventArgs.ErrorContext.Error; - var metadata = ModelNames.GetPathMetadata(context.Metadata, path); + var metadata = GetPathMetadata(context.Metadata, path); var modelStateException = WrapExceptionForModelState(exception); context.ModelState.TryAddModelError(key, modelStateException, metadata); @@ -301,6 +301,52 @@ namespace Microsoft.AspNetCore.Mvc.Formatters protected virtual void ReleaseJsonSerializer(JsonSerializer serializer) => _jsonSerializerPool.Return(serializer); + private ModelMetadata GetPathMetadata(ModelMetadata metadata, string path) + { + var index = 0; + while (index >= 0 && index < path.Length) + { + if (path[index] == '[') + { + // At start of "[0]". + if (metadata.ElementMetadata == null) + { + // Odd case but don't throw just because ErrorContext had an odd-looking path. + break; + } + + metadata = metadata.ElementMetadata; + index = path.IndexOf(']', index); + } + else if (path[index] == '.' || path[index] == ']') + { + // Skip '.' in "prefix.property" or "[0].property" or ']' in "[0]". + index++; + } + else + { + // At start of "property", "property." or "property[0]". + var endIndex = path.IndexOfAny(new[] { '.', '[' }, index); + if (endIndex == -1) + { + endIndex = path.Length; + } + + var propertyName = path.Substring(index, endIndex - index); + if (metadata.Properties[propertyName] == null) + { + // Odd case but don't throw just because ErrorContext had an odd-looking path. + break; + } + + metadata = metadata.Properties[propertyName]; + index = endIndex; + } + } + + return metadata; + } + private Exception WrapExceptionForModelState(Exception exception) { // In 2.0 and earlier we always gave a generic error message for errors that come from JSON.NET