From e69cca6461fc14eb44cabaa9b78ff6e07df07cde Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Fri, 14 Jun 2019 16:25:29 -0700 Subject: [PATCH] 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}