Skip to content

Commit 962f55e

Browse files
halter73halter73
authored
Use correct path for NewtonsoftJson ModelState errors (#39058)
1 parent 00f7e1f commit 962f55e

File tree

5 files changed

+156
-14
lines changed

5 files changed

+156
-14
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ public static string CreateIndexModelName(string parentName, string index)
3535
}
3636

3737
/// <summary>
38-
/// Create an property model name with a prefix.
38+
/// Create a property model name with a prefix.
3939
/// </summary>
4040
/// <param name="prefix">The prefix to use.</param>
4141
/// <param name="propertyName">The property name.</param>

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

Lines changed: 75 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Licensed to the .NET Foundation under one or more agreements.
1+
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.ComponentModel.DataAnnotations;
@@ -124,6 +124,34 @@ public async Task JsonFormatterReadsNonUtf8Content()
124124
Assert.True(httpContext.Request.Body.CanRead, "Verify that the request stream hasn't been disposed");
125125
}
126126

127+
[Fact]
128+
public virtual async Task JsonFormatter_EscapedKeys()
129+
{
130+
var expectedKey = JsonFormatter_EscapedKeys_Expected;
131+
132+
// Arrange
133+
var content = "[{\"It\\\"s a key\": 1234556}]";
134+
var formatter = GetInputFormatter();
135+
136+
var contentBytes = Encoding.UTF8.GetBytes(content);
137+
var httpContext = GetHttpContext(contentBytes);
138+
139+
var formatterContext = CreateInputFormatterContext(
140+
typeof(IEnumerable<IDictionary<string, short>>), httpContext);
141+
142+
// Act
143+
var result = await formatter.ReadAsync(formatterContext);
144+
145+
// Assert
146+
Assert.True(result.HasError);
147+
Assert.Collection(
148+
formatterContext.ModelState.OrderBy(k => k.Key),
149+
kvp =>
150+
{
151+
Assert.Equal(expectedKey, kvp.Key);
152+
});
153+
}
154+
127155
[Fact]
128156
public virtual async Task JsonFormatter_EscapedKeys_Bracket()
129157
{
@@ -152,12 +180,12 @@ public virtual async Task JsonFormatter_EscapedKeys_Bracket()
152180
}
153181

154182
[Fact]
155-
public virtual async Task JsonFormatter_EscapedKeys()
183+
public virtual async Task JsonFormatter_EscapedKeys_SingleQuote()
156184
{
157-
var expectedKey = JsonFormatter_EscapedKeys_Expected;
185+
var expectedKey = JsonFormatter_EscapedKeys_SingleQuote_Expected;
158186

159187
// Arrange
160-
var content = "[{\"It\\\"s a key\": 1234556}]";
188+
var content = "[{\"It's a key\": 1234556}]";
161189
var formatter = GetInputFormatter();
162190

163191
var contentBytes = Encoding.UTF8.GetBytes(content);
@@ -463,6 +491,30 @@ public async Task ReadAsync_ComplexPoco()
463491
});
464492
}
465493

494+
[Fact]
495+
public virtual async Task ReadAsync_NestedParseError()
496+
{
497+
// Arrange
498+
var formatter = GetInputFormatter();
499+
var content = @"{ ""b"": { ""c"": { ""d"": efg } } }";
500+
var contentBytes = Encoding.UTF8.GetBytes(content);
501+
var httpContext = GetHttpContext(contentBytes);
502+
503+
var formatterContext = CreateInputFormatterContext(typeof(A), httpContext);
504+
505+
// Act
506+
var result = await formatter.ReadAsync(formatterContext);
507+
508+
// Assert
509+
Assert.True(result.HasError, "Model should have had an error!");
510+
Assert.Collection(
511+
formatterContext.ModelState.OrderBy(k => k.Key),
512+
kvp =>
513+
{
514+
Assert.Equal(ReadAsync_NestedParseError_Expected, kvp.Key);
515+
});
516+
}
517+
466518
[Fact]
467519
public virtual async Task ReadAsync_RequiredAttribute()
468520
{
@@ -556,6 +608,8 @@ public async Task ReadAsync_WithEnableBufferingWorks_WithInputStreamAtOffset()
556608

557609
internal abstract string JsonFormatter_EscapedKeys_Bracket_Expected { get; }
558610

611+
internal abstract string JsonFormatter_EscapedKeys_SingleQuote_Expected { get; }
612+
559613
internal abstract string JsonFormatter_EscapedKeys_Expected { get; }
560614

561615
internal abstract string ReadAsync_ArrayOfObjects_HasCorrectKey_Expected { get; }
@@ -568,6 +622,8 @@ public async Task ReadAsync_WithEnableBufferingWorks_WithInputStreamAtOffset()
568622

569623
internal abstract string ReadAsync_ComplexPoco_Expected { get; }
570624

625+
internal abstract string ReadAsync_NestedParseError_Expected { get; }
626+
571627
protected abstract TextInputFormatter GetInputFormatter(bool allowInputFormatterExceptionMessages = true);
572628

573629
protected static HttpContext GetHttpContext(
@@ -629,6 +685,21 @@ protected sealed class ComplexModel
629685
public byte Small { get; set; }
630686
}
631687

688+
class A
689+
{
690+
public B B { get; set; }
691+
}
692+
693+
class B
694+
{
695+
public C C { get; set; }
696+
}
697+
698+
class C
699+
{
700+
public string D { get; set; }
701+
}
702+
632703
private class VerifyDisposeFileBufferingReadStream : FileBufferingReadStream
633704
{
634705
public bool Disposed { get; private set; }

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ public override Task JsonFormatter_EscapedKeys_Bracket()
5454
return base.JsonFormatter_EscapedKeys_Bracket();
5555
}
5656

57+
[Fact]
58+
public override Task JsonFormatter_EscapedKeys_SingleQuote()
59+
{
60+
return base.JsonFormatter_EscapedKeys_SingleQuote();
61+
}
62+
5763
[Fact]
5864
public async Task ReadAsync_SingleError()
5965
{
@@ -190,6 +196,8 @@ protected override TextInputFormatter GetInputFormatter(bool allowInputFormatter
190196

191197
internal override string JsonFormatter_EscapedKeys_Bracket_Expected => "$[0]['It[s a key']";
192198

199+
internal override string JsonFormatter_EscapedKeys_SingleQuote_Expected => "$[0]['It's a key']";
200+
193201
internal override string ReadAsync_ArrayOfObjects_HasCorrectKey_Expected => "$[2].Age";
194202

195203
internal override string ReadAsync_InvalidArray_AddsOverflowErrorsToModelState_Expected => "$[2]";
@@ -198,6 +206,8 @@ protected override TextInputFormatter GetInputFormatter(bool allowInputFormatter
198206

199207
internal override string ReadAsync_ComplexPoco_Expected => "$.Person.Numbers[2]";
200208

209+
internal override string ReadAsync_NestedParseError_Expected => "$.b.c.d";
210+
201211
private class TypeWithBadConverters
202212
{
203213
[JsonConverter(typeof(DateTimeConverter))]

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

Lines changed: 59 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -237,30 +237,81 @@ void ErrorHandler(object? sender, Newtonsoft.Json.Serialization.ErrorEventArgs e
237237

238238
successful = false;
239239

240-
// When ErrorContext.Path does not include ErrorContext.Member, add Member to form full path.
240+
// The following addMember logic is intended to append the names of missing required properties to the
241+
// ModelStateDictionary key. Normally, just the ModelName and ErrorContext.Path is used for this key,
242+
// but ErrorContext.Path does not include the missing required property name like we want it to.
243+
// For example, given the following class and input missing the required "Name" property:
244+
//
245+
// class Person
246+
// {
247+
// [JsonProperty(Required = Required.Always)]
248+
// public string Name { get; set; }
249+
// }
250+
//
251+
// We will see the following ErrorContext:
252+
//
253+
// Error {"Required property 'Name' not found in JSON. Path 'Person'..."} System.Exception {Newtonsoft.Json.JsonSerializationException}
254+
// Member "Name" object {string}
255+
// Path "Person" string
256+
//
257+
// So we update the path used for the ModelStateDictionary key to be "Person.Name" instead of just "Person".
258+
// See https://github.com/aspnet/Mvc/issues/8509
241259
var path = eventArgs.ErrorContext.Path;
242-
var member = eventArgs.ErrorContext.Member?.ToString();
243-
var addMember = !string.IsNullOrEmpty(member);
260+
var member = eventArgs.ErrorContext.Member as string;
261+
262+
// There are some deserialization exceptions that include the member in the path but not at the end.
263+
// For example, given the following classes and invalid input like { "b": { "c": { "d": abc } } }:
264+
//
265+
// class A
266+
// {
267+
// public B B { get; set; }
268+
// }
269+
// class B
270+
// {
271+
// public C C { get; set; }
272+
// }
273+
// class C
274+
// {
275+
// public string D { get; set; }
276+
// }
277+
//
278+
// We will see the following ErrorContext:
279+
//
280+
// Error {"Unexpected character encountered while parsing value: b. Path 'b.c.d'..."} System.Exception {Newtonsoft.Json.JsonReaderException}
281+
// Member "c" object {string}
282+
// Path "b.c.d" string
283+
//
284+
// Notice that Member "c" is in the middle of the Path "b.c.d". The error handler gets invoked for each level of nesting.
285+
// null, "b", "c" and "d" are each a Member in different ErrorContexts all reporting the same parsing error.
286+
//
287+
// The parsing error is reported as a JsonReaderException instead of as a JsonSerializationException like
288+
// for missing required properties. We use the exception type to filter out these errors and keep the path used
289+
// for the ModelStateDictionary key as "b.c.d" instead of "b.c.d.c"
290+
// See https://github.com/dotnet/aspnetcore/issues/33451
291+
var addMember = !string.IsNullOrEmpty(member) && eventArgs.ErrorContext.Error is JsonSerializationException;
292+
293+
// There are still JsonSerilizationExceptions that set ErrorContext.Member but include it at the
294+
// end of ErrorContext.Path already. The following logic attempts to filter these out.
244295
if (addMember)
245296
{
246297
// Path.Member case (path.Length < member.Length) needs no further checks.
247298
if (path.Length == member!.Length)
248299
{
249-
// Add Member in Path.Memb case but not for Path.Path.
300+
// Add Member in Path.Member case but not for Path.Path.
250301
addMember = !string.Equals(path, member, StringComparison.Ordinal);
251302
}
252303
else if (path.Length > member.Length)
253304
{
254-
// Finally, check whether Path already ends with Member.
305+
// Finally, check whether Path already ends or starts with Member.
255306
if (member[0] == '[')
256307
{
257308
addMember = !path.EndsWith(member, StringComparison.Ordinal);
258309
}
259310
else
260311
{
261-
addMember = !path.EndsWith("." + member, StringComparison.Ordinal)
262-
&& !path.EndsWith("['" + member + "']", StringComparison.Ordinal)
263-
&& !path.EndsWith("[" + member + "]", StringComparison.Ordinal);
312+
addMember = !path.EndsWith($".{member}", StringComparison.Ordinal)
313+
&& !path.EndsWith($"['{member}']", StringComparison.Ordinal)
314+
&& !path.EndsWith($"[{member}]", StringComparison.Ordinal);
264315
}
265316
}
266317
}

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

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,12 @@ public override Task JsonFormatter_EscapedKeys_Bracket()
206206
return base.JsonFormatter_EscapedKeys_Bracket();
207207
}
208208

209+
[Fact(Skip = "https://github.com/dotnet/aspnetcore/issues/39069")]
210+
public override Task JsonFormatter_EscapedKeys_SingleQuote()
211+
{
212+
return base.JsonFormatter_EscapedKeys_SingleQuote();
213+
}
214+
209215
[Theory]
210216
[InlineData(" ", true, true)]
211217
[InlineData(" ", false, false)]
@@ -556,14 +562,18 @@ private NewtonsoftJsonInputFormatter CreateFormatter(JsonSerializerSettings seri
556562

557563
internal override string JsonFormatter_EscapedKeys_Expected => "[0]['It\"s a key']";
558564

559-
internal override string JsonFormatter_EscapedKeys_Bracket_Expected => "[0][\'It[s a key\']";
565+
internal override string JsonFormatter_EscapedKeys_Bracket_Expected => "[0]['It[s a key']";
566+
567+
internal override string JsonFormatter_EscapedKeys_SingleQuote_Expected => "[0]['It\\'s a key']";
560568

561569
internal override string ReadAsync_AddsModelValidationErrorsToModelState_Expected => "Age";
562570

563571
internal override string ReadAsync_ArrayOfObjects_HasCorrectKey_Expected => "[2].Age";
564572

565573
internal override string ReadAsync_ComplexPoco_Expected => "Person.Numbers[2]";
566574

575+
internal override string ReadAsync_NestedParseError_Expected => "b.c.d";
576+
567577
internal override string ReadAsync_InvalidComplexArray_AddsOverflowErrorsToModelState_Expected => "names[1].Small";
568578

569579
internal override string ReadAsync_InvalidArray_AddsOverflowErrorsToModelState_Expected => "[2]";

0 commit comments

Comments
 (0)