Use correct path for NewtonsoftJson ModelState errors

src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs

@@ -35,7 +35,7 @@ public static string CreateIndexModelName(string parentName, string index)
     }
 
     /// <summary>
-    /// Create an property model name with a prefix.
+    /// Create a property model name with a prefix.
     /// </summary>
     /// <param name="prefix">The prefix to use.</param>
     /// <param name="propertyName">The property name.</param>

src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs

@@ -1,22 +1,14 @@
-// Licensed to the .NET Foundation under one or more agreements.
+// Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-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;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
 using Microsoft.AspNetCore.Testing;
 using Microsoft.AspNetCore.WebUtilities;
-using Microsoft.Extensions.Logging.Testing;
-using Moq;
 using Newtonsoft.Json;
-using Xunit;
 
 namespace Microsoft.AspNetCore.Mvc.Formatters;
 
@@ -132,6 +124,34 @@ public async Task JsonFormatterReadsNonUtf8Content()
         Assert.True(httpContext.Request.Body.CanRead, "Verify that the request stream hasn't been disposed");
     }
 
+    [Fact]
+    public virtual async Task JsonFormatter_EscapedKeys()
+    {
+        var expectedKey = JsonFormatter_EscapedKeys_Expected;
+
+        // 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<IDictionary<string, short>>), httpContext);
+
+        // Act
+        var result = await formatter.ReadAsync(formatterContext);
+
+        // Assert
+        Assert.True(result.HasError);
+        Assert.Collection(
+            formatterContext.ModelState.OrderBy(k => k.Key),
+            kvp =>
+            {
+                Assert.Equal(expectedKey, kvp.Key);
+            });
+    }
+
     [Fact]
     public virtual async Task JsonFormatter_EscapedKeys_Bracket()
     {
@@ -160,12 +180,12 @@ public virtual async Task JsonFormatter_EscapedKeys_Bracket()
     }
 
     [Fact]
-    public virtual async Task JsonFormatter_EscapedKeys()
+    public virtual async Task JsonFormatter_EscapedKeys_SingleQuote()
     {
-        var expectedKey = JsonFormatter_EscapedKeys_Expected;
+        var expectedKey = JsonFormatter_EscapedKeys_SingleQuote_Expected;
 
         // Arrange
-        var content = "[{\"It\\\"s a key\": 1234556}]";
+        var content = "[{\"It's a key\": 1234556}]";
         var formatter = GetInputFormatter();
 
         var contentBytes = Encoding.UTF8.GetBytes(content);
@@ -471,6 +491,30 @@ public async Task ReadAsync_ComplexPoco()
             });
     }
 
+    [Fact]
+    public virtual async Task ReadAsync_NestedParseError()
+    {
+        // Arrange
+        var formatter = GetInputFormatter();
+        var content = @"{ ""b"": { ""c"": { ""d"": efg } } }";
+        var contentBytes = Encoding.UTF8.GetBytes(content);
+        var httpContext = GetHttpContext(contentBytes);
+
+        var formatterContext = CreateInputFormatterContext(typeof(A), httpContext);
+
+        // Act
+        var result = await formatter.ReadAsync(formatterContext);
+
+        // Assert
+        Assert.True(result.HasError, "Model should have had an error!");
+        Assert.Collection(
+            formatterContext.ModelState.OrderBy(k => k.Key),
+            kvp =>
+            {
+                Assert.Equal(ReadAsync_NestedParseError_Expected, kvp.Key);
+            });
+    }
+
     [Fact]
     public virtual async Task ReadAsync_RequiredAttribute()
     {
@@ -564,6 +608,8 @@ public async Task ReadAsync_WithEnableBufferingWorks_WithInputStreamAtOffset()
 
     internal abstract string JsonFormatter_EscapedKeys_Bracket_Expected { get; }
 
+    internal abstract string JsonFormatter_EscapedKeys_SingleQuote_Expected { get; }
+
     internal abstract string JsonFormatter_EscapedKeys_Expected { get; }
 
     internal abstract string ReadAsync_ArrayOfObjects_HasCorrectKey_Expected { get; }
@@ -576,6 +622,8 @@ public async Task ReadAsync_WithEnableBufferingWorks_WithInputStreamAtOffset()
 
     internal abstract string ReadAsync_ComplexPoco_Expected { get; }
 
+    internal abstract string ReadAsync_NestedParseError_Expected { get; }
+
     protected abstract TextInputFormatter GetInputFormatter(bool allowInputFormatterExceptionMessages = true);
 
     protected static HttpContext GetHttpContext(
@@ -637,6 +685,21 @@ protected sealed class ComplexModel
         public byte Small { get; set; }
     }
 
+    class A
+    {
+        public B B { get; set; }
+    }
+
+    class B 
+    {
+        public C C { get; set; }
+    }
+
+    class C
+    {
+        public string D { get; set; }
+    }
+
     private class VerifyDisposeFileBufferingReadStream : FileBufferingReadStream
     {
         public bool Disposed { get; private set; }

src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs

@@ -1,16 +1,11 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
-using System.Collections.Generic;
 using System.Globalization;
-using System.Linq;
 using System.Text;
 using System.Text.Json;
 using System.Text.Json.Serialization;
-using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
-using Xunit;
 
 namespace Microsoft.AspNetCore.Mvc.Formatters;
 
@@ -59,6 +54,12 @@ public override Task JsonFormatter_EscapedKeys_Bracket()
         return base.JsonFormatter_EscapedKeys_Bracket();
     }
 
+    [Fact]
+    public override Task JsonFormatter_EscapedKeys_SingleQuote()
+    {
+        return base.JsonFormatter_EscapedKeys_SingleQuote();
+    }
+
     [Fact]
     public async Task ReadAsync_SingleError()
     {
@@ -195,6 +196,8 @@ protected override TextInputFormatter GetInputFormatter(bool allowInputFormatter
 
     internal override string JsonFormatter_EscapedKeys_Bracket_Expected => "$[0]['It[s a key']";
 
+    internal override string JsonFormatter_EscapedKeys_SingleQuote_Expected => "$[0]['It's a key']";
+
     internal override string ReadAsync_ArrayOfObjects_HasCorrectKey_Expected => "$[2].Age";
 
     internal override string ReadAsync_InvalidArray_AddsOverflowErrorsToModelState_Expected => "$[2]";
@@ -203,6 +206,8 @@ protected override TextInputFormatter GetInputFormatter(bool allowInputFormatter
 
     internal override string ReadAsync_ComplexPoco_Expected => "$.Person.Numbers[2]";
 
+    internal override string ReadAsync_NestedParseError_Expected => "$.b.c.d";
+
     private class TypeWithBadConverters
     {
         [JsonConverter(typeof(DateTimeConverter))]

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

@@ -234,30 +234,81 @@ void ErrorHandler(object? sender, Newtonsoft.Json.Serialization.ErrorEventArgs e
         {
             successful = false;
 
-            // When ErrorContext.Path does not include ErrorContext.Member, add Member to form full path.
+            // The following addMember logic is intended to append the names of missing required properties to the
+            // ModelStateDictionary key. Normally, just the ModelName and ErrorContext.Path is used for this key,
+            // but ErrorContext.Path does not include the missing required property name like we want it to.
+            // For example, given the following class and input missing the required "Name" property:
+            //
+            // class Person
+            // {
+            //     [JsonProperty(Required = Required.Always)]
+            //     public string Name { get; set; }
+            // }
+            //
+            // We will see the following ErrorContext:
+            //
+            // Error   {"Required property 'Name' not found in JSON. Path 'Person'..."} System.Exception {Newtonsoft.Json.JsonSerializationException}
+            // Member  "Name"   object {string}
+            // Path    "Person" string
+            //
+            // So we update the path used for the ModelStateDictionary key to be "Person.Name" instead of just "Person".
+            // See https://github.com/aspnet/Mvc/issues/8509
             var path = eventArgs.ErrorContext.Path;
-            var member = eventArgs.ErrorContext.Member?.ToString();
-            var addMember = !string.IsNullOrEmpty(member);
+            var member = eventArgs.ErrorContext.Member as string;
+
+            // There are some deserialization exceptions that include the member in the path but not at the end.
+            // For example, given the following classes and invalid input like { "b": { "c": { "d": abc } } }:
+            //
+            // class A
+            // {
+            //     public B B { get; set; }
+            // }
+            // class B 
+            // {
+            //     public C C { get; set; }
+            // }
+            // class C 
+            // {
+            //     public string D { get; set; }
+            // }
+            //
+            // We will see the following ErrorContext:
+            //
+            // Error   {"Unexpected character encountered while parsing value: b. Path 'b.c.d'..."} System.Exception {Newtonsoft.Json.JsonReaderException}
+            // Member  "c"     object {string}
+            // Path    "b.c.d" string
+            //
+            // Notice that Member "c" is in the middle of the Path "b.c.d". The error handler gets invoked for each level of nesting.
+            // null, "b", "c" and "d" are each a Member in different ErrorContexts all reporting the same parsing error.
+            //
+            // The parsing error is reported as a JsonReaderException instead of as a JsonSerializationException like
+            // for missing required properties. We use the exception type to filter out these errors and keep the path used
+            // for the ModelStateDictionary key as "b.c.d" instead of "b.c.d.c"
+            // See https://github.com/dotnet/aspnetcore/issues/33451 
+            var addMember = !string.IsNullOrEmpty(member) && eventArgs.ErrorContext.Error is JsonSerializationException;
+
+            // There are still JsonSerilizationExceptions that set ErrorContext.Member but include it at the
+            // end of ErrorContext.Path already. The following logic attempts to filter these out.
             if (addMember)
             {
                 // Path.Member case (path.Length < member.Length) needs no further checks.
                 if (path.Length == member!.Length)
                 {
-                    // Add Member in Path.Memb case but not for Path.Path.
+                    // Add Member in Path.Member case but not for Path.Path.
                     addMember = !string.Equals(path, member, StringComparison.Ordinal);
                 }
                 else if (path.Length > member.Length)
                 {
-                    // Finally, check whether Path already ends with Member.
+                    // Finally, check whether Path already ends or starts with Member.
                     if (member[0] == '[')
                     {
                         addMember = !path.EndsWith(member, StringComparison.Ordinal);
                     }
                     else
                     {
-                        addMember = !path.EndsWith("." + member, StringComparison.Ordinal)
-                            && !path.EndsWith("['" + member + "']", StringComparison.Ordinal)
-                            && !path.EndsWith("[" + member + "]", StringComparison.Ordinal);
+                        addMember = !path.EndsWith($".{member}", StringComparison.Ordinal)
+                            && !path.EndsWith($"['{member}']", StringComparison.Ordinal)
+                            && !path.EndsWith($"[{member}]", StringComparison.Ordinal);
                     }
                 }
             }

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

@@ -1,13 +1,9 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
 using System.Buffers;
-using System.Collections.Generic;
 using System.Globalization;
-using System.Linq;
 using System.Text;
-using System.Threading.Tasks;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.WebUtilities;
@@ -18,7 +14,6 @@
 using Newtonsoft.Json;
 using Newtonsoft.Json.Converters;
 using Newtonsoft.Json.Serialization;
-using Xunit;
 
 namespace Microsoft.AspNetCore.Mvc.Formatters;
 
@@ -211,6 +206,12 @@ public override Task JsonFormatter_EscapedKeys_Bracket()
         return base.JsonFormatter_EscapedKeys_Bracket();
     }
 
+    [Fact(Skip = "https://github.com/dotnet/aspnetcore/issues/39069")]
+    public override Task JsonFormatter_EscapedKeys_SingleQuote()
+    {
+        return base.JsonFormatter_EscapedKeys_SingleQuote();
+    }
+
     [Theory]
     [InlineData(" ", true, true)]
     [InlineData(" ", false, false)]
@@ -523,14 +524,18 @@ private NewtonsoftJsonInputFormatter CreateFormatter(JsonSerializerSettings seri
 
     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 JsonFormatter_EscapedKeys_Bracket_Expected => "[0]['It[s a key']";
+
+    internal override string JsonFormatter_EscapedKeys_SingleQuote_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_NestedParseError_Expected => "b.c.d";
+
     internal override string ReadAsync_InvalidComplexArray_AddsOverflowErrorsToModelState_Expected => "names[1].Small";
 
     internal override string ReadAsync_InvalidArray_AddsOverflowErrorsToModelState_Expected => "[2]";