diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs b/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs index f9878868e712..8e6afac46e68 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/ModelNames.cs @@ -35,7 +35,7 @@ public static string CreateIndexModelName(string parentName, string index) } /// - /// Create an property model name with a prefix. + /// Create a property model name with a prefix. /// /// The prefix to use. /// The property name. diff --git a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs b/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs index dfedd751c852..40aba81381d5 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/JsonInputFormatterTestBase.cs +++ b/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>), 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; } diff --git a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs b/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs index ee6ed0383b68..6bcd1576f9d3 100644 --- a/src/Mvc/Mvc.Core/test/Formatters/SystemTextJsonInputFormatterTest.cs +++ b/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))] diff --git a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs b/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs index e4f24e2b4ae0..facd481de038 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonInputFormatter.cs +++ b/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); } } } diff --git a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs b/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs index a180a4e07532..90a49d946f1a 100644 --- a/src/Mvc/Mvc.NewtonsoftJson/test/NewtonsoftJsonInputFormatterTest.cs +++ b/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,7 +524,9 @@ 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"; @@ -531,6 +534,8 @@ private NewtonsoftJsonInputFormatter CreateFormatter(JsonSerializerSettings seri 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]";