PR feedback

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<ILogger, Exception> _jsonInputFormatterException;

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;
+        }
     }
 }

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
+            Assert.Collection(
-            foreach(var modelState in formatterContext.ModelState)
+                formatterContext.ModelState.OrderBy(k => k),
-            {
+                kvp =>
-                foreach(var error in modelState.Value.Errors)
                 {
-                    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()

src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs

@@ -226,8 +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);
+                                && !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

src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs

@@ -235,23 +235,13 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             Assert.Equal(expectedMessage, modelError.ErrorMessage);
         }
 
-        [Theory]
+        [Fact]
-        [InlineData("[5,7,3]", 0)]
+        public async Task ReadAsync_AllowMultipleErrors()
-        [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 content = "[5, 'seven', 3, 'notnum']";
-            var serializerSettings = new JsonSerializerSettings
-            {
-                Error = delegate (object sender, ErrorEventArgs args)
-                {
-                    args.ErrorContext.Handled = true;
-                }
-            };
 
-            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)
+            Assert.Collection(
-            {
+                formatterContext.ModelState.OrderBy(k => k),
-                foreach (var error in modelState.Value.Errors)
+                kvp =>
                 {
-                    failTotal++;
+                    Assert.Equal("[1]", kvp.Key);
+                    var error = Assert.Single(kvp.Value.Errors);
                     Assert.StartsWith("Could not convert string to integer:", error.ErrorMessage);
-                }
+                },
-            }
+                kvp =>
-
+                {
-            Assert.Equal(failCount, failTotal);
+                    Assert.Equal("[3]", kvp.Key);
+                    var error = Assert.Single(kvp.Value.Errors);
+                    Assert.StartsWith("Could not convert string to integer:", error.ErrorMessage);
+                });
         }
 
         [Fact]

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}