Change JsonException.Path to be a json-based path #37938

steveharter · 2019-05-24T21:06:26Z

Fixes https://github.com/dotnet/corefx/issues/37768

Currently we escape the property names in the Path.

We need to be clear on what is reported on invalid array element. Do we return the invalid array element, or the "last known good" element. For example does [1,X report a Path of $[0] (last known good) or $[1] (invalid element even though it's not a valid indexer). Currently the code returns $[1] for this case. UPDATE: this behavior has been approved.

The path is not followed into calls into JsonDocument which occurs during deserialization of type(object) and for the overflow[DataExtensionProperty]. The current plan is to create another issue to address this by first adding support within JsonDocument to support Path (which would work when not called by the serializer), and then have the serializer catch the appropriate JsonReaderException thrown by JsonDocument and prepend the exception's Path with the Path that existed before the call to JsonDocument and then re-throw as JsonException. UPDATE: see issue https://github.com/dotnet/corefx/issues/38074

JamesNK

Overall I think this looks good. Would like some more tests.

I haven't looked closely at the implementation.

src/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs

JamesNK · 2019-05-24T21:15:51Z

src/System.Text.Json/tests/Serialization/ExceptionTests.cs

+        {
+            try
+            {
+                JsonSerializer.Parse<Dictionary<string, int>>(@"{""Key1"":1, ""Key2"":bad}");


What happens if the JSON is @"{""Key1"":1, ""Key2"

What happens if the JSON is @"{""Key1"":1, ""Key2"""

Since there is no : delimiter, it would be $.Key1

I'll add a test.

The path should be $. The error didn't happen at $.Key1. Key1 just happened to be the last property read. The error happened on attempting to read a the next name/value from $.

Situations like this is why I kept things simple in Json.NET and only included the known parent segments.

The current Path semantics follow the reader semantics. If we want different semantics we'd likely have to change the reader to not accept a space for the delimiter cc @ahsonkhan

An error didn't happen parsing $.Key1. Key1 is done. It's finished and in the past. The state should have been popped off the stack when its value (1) was finished deserializing. The new error happened when parsing a parameter on the root object: $. I don't know what the reader semantics have to do with it.

Reading through the JSON, and what the path would be if an error was thrown:

Read: {
Path: $

Read: Key1
Path: $ (because you haven't finished reading Key1)

Read: 1
Path: $.Key1

Read: Key2
Path: $ (Key1 was popped off the stack once its value was read. Now you start reading the next property on the root object)

The serializer currently does not track state separate from the reader. So if the reader says a property name or array element is "read" successfully then the serializer treats that as part of the path. Let's discuss in next meeting.

JamesNK · 2019-05-24T21:16:32Z

src/System.Text.Json/tests/Serialization/ExceptionTests.cs

+        {
+            try
+            {
+                JsonSerializer.Parse<int[]>(@"[1, bad]");


What happens if the JSON is @"[1"

What happens if the JSON is @"[1 /* comment starts but doesn't end"

The semantics are currently determined by the reader on what it treats as delimiters or separators for a given unit.

For [1 it would be $[0]
For [1 /* comment it would be $[1]

For [1 /* comment it would be $[1]

$[1] isn't correct. There is no second item in [1 /* comment.

The index shouldn't be increased unless a comma is read.

Situations like this is why I kept things simple in Json.NET and only included the known parent segments.

Same response as above @ahsonkhan

@steveharter, can you please file an issue to track this

https://github.com/dotnet/corefx/issues/39478

src/System.Text.Json/tests/Serialization/Value.ReadTests.cs

src/System.Text.Json/tests/Serialization/ExceptionTests.cs

JamesNK · 2019-05-24T21:20:19Z

src/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs

@@ -10,7 +11,9 @@ namespace System.Text.Json.Serialization
    [DebuggerDisplay("Current: ClassType.{Current.JsonClassInfo.ClassType}, {Current.JsonClassInfo.Type.Name}")]
    internal struct ReadStack
    {
-        // A fields is used instead of a property to avoid value semantics.
+        private static readonly char[] SpecialCharacters = { '.', ' ', '\'', '/', '"', '[', ']', '(', ')', '\t', '\n', '\r', '\f', '\b', '\\', '\u0085', '\u2028', '\u2029' };


Is this the same as Json.NET?

Yes verbatim pending review. cc @ahsonkhan

'\u0085', '\u2028', '\u2029' might be concerning, but I am not sure if it this issue applies here: https://github.com/dotnet/corefx/issues/33321

cc @GrabYourPitchforks, @bartonjs

ahsonkhan

What's the performance regression in both serializer/deserializer due to this change? Can you measure and share the results?
https://github.com/dotnet/performance/blob/3439a62de1e17c5daa99086bdd6cda31384ade91/src/benchmarks/micro/corefx/System.Text.Json/Serializer/ReadJson.cs#L11
https://github.com/dotnet/performance/blob/3439a62de1e17c5daa99086bdd6cda31384ade91/src/benchmarks/micro/corefx/System.Text.Json/Serializer/WriteJson.cs#L11

...ystem.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs

JamesNK · 2019-05-25T00:59:59Z

@pranavkm

steveharter · 2019-05-30T19:58:06Z

@ahsonkhan I previously measured benchmarks -- with case-sensitive the results didn't change (within margin of error). I re-ran locally with threshold of 1% and results are consistent with that:

better: 9, geomean: 1.250
worse: 6, geomean: 1.183
total diff: 15

| Slower                                                                           | diff/base | Base Median (ns) | Diff Median (ns) | Modality  |
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| ---------- |
| System.Text.Json.Serialization.Tests.WriteJson<LoginViewModel>.SerializeToString |      1.43 |           612.14 |           876.19 |           |
| System.Text.Json.Serialization.Tests.ReadJson<IndexViewModel>.DeserializeFromStr |      1.19 |         49452.33 |         58921.34 | bimodal   |
| System.Text.Json.Serialization.Tests.WriteJson<MyEventsListerViewModel>.Serializ |      1.18 |        698441.79 |        826713.44 | bimodal   |
| System.Text.Json.Serialization.Tests.WriteJson<MyEventsListerViewModel>.Serializ |      1.16 |        745253.35 |        868183.78 |           |
| System.Text.Json.Serialization.Tests.WriteJson<MyEventsListerViewModel>.Serializ |      1.11 |        725348.65 |        802737.53 | multimodal|
| System.Text.Json.Serialization.Tests.WriteJson<IndexViewModel>.SerializeToStream |      1.05 |         34075.94 |         35865.59 |           |

| Faster                                                                           | base/diff | Base Median (ns) | Diff Median (ns) | Modality  |
| -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| ---------- |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromStr |      1.45 |          1192.24 |           821.82 | bimodal   |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromStr |      1.36 |          1571.40 |          1151.44 | several?  |
| System.Text.Json.Serialization.Tests.ReadJson<Location>.DeserializeFromStream    |      1.31 |          3423.60 |          2623.12 | several?  |
| System.Text.Json.Serialization.Tests.ReadJson<LoginViewModel>.DeserializeFromUtf |      1.29 |          1018.41 |           791.12 | bimodal   |
| System.Text.Json.Serialization.Tests.WriteJson<IndexViewModel>.SerializeToString |      1.20 |         45131.60 |         37657.88 | multimodal|
| System.Text.Json.Serialization.Tests.ReadJson<Location>.DeserializeFromString    |      1.18 |          2649.39 |          2249.76 |           |
| System.Text.Json.Serialization.Tests.ReadJson<MyEventsListerViewModel>.Deseriali |      1.18 |        609012.88 |        517570.93 | bimodal   |
| System.Text.Json.Serialization.Tests.WriteJson<LoginViewModel>.SerializeToUtf8By |      1.17 |           651.04 |           554.58 | bimodal   |
| System.Text.Json.Serialization.Tests.ReadJson<Location>.DeserializeFromUtf8Bytes |      1.15 |          2239.36 |          1945.41 | bimodal   |

steveharter · 2019-05-30T21:37:39Z

CI failure on Linux arm64-Release caused by unrelated System.Runtime.Serialization.Formatters.Tests

JamesNK · 2019-05-30T21:46:28Z

#37938 (comment)

My feedback??

Commit migrated from dotnet/corefx@02f649e

Change JsonException.Path to be a json-based path

fd60eda

steveharter added the area-System.Text.Json label May 24, 2019

steveharter requested review from JamesNK, rynowak and ahsonkhan May 24, 2019 21:06

steveharter self-assigned this May 24, 2019

JamesNK reviewed May 24, 2019

View reviewed changes

ahsonkhan reviewed May 24, 2019

View reviewed changes

...ystem.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs Outdated Show resolved Hide resolved

steveharter added 3 commits May 30, 2019 14:10

Review feedback

a598ec6

Merge branch 'master' of https://github.com/dotnet/corefx into JsonPath

ffb3b7f

Additional merge changes

d78a45f

Merge branch 'master' of https://github.com/dotnet/corefx into JsonPath

85556fc

steveharter merged commit 02f649e into dotnet:master May 30, 2019

steveharter deleted the JsonPath branch May 30, 2019 21:44

karelz added this to the 3.0 milestone Jul 16, 2019

This was referenced Feb 1, 2020

Add JSONPath to the exception thrown by JsonDocument while parsing invalid JSON dotnet/runtime#29715

Closed

Improve JsonException.Path accuracy dotnet/runtime#30259

Closed

layomia mentioned this pull request Apr 7, 2020

Improve consistency of JSONPath in JsonException dotnet/runtime#31955

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Change JsonException.Path to be a json-based path (dotnet/corefx#37938)

c4a989d

Commit migrated from dotnet/corefx@02f649e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change JsonException.Path to be a json-based path #37938

Change JsonException.Path to be a json-based path #37938

steveharter commented May 24, 2019 •

edited

Loading

JamesNK left a comment

JamesNK May 24, 2019

JamesNK May 24, 2019 •

edited

Loading

steveharter May 30, 2019

JamesNK May 30, 2019 •

edited

Loading

JamesNK May 30, 2019

steveharter May 30, 2019

JamesNK May 31, 2019 •

edited

Loading

steveharter May 31, 2019

JamesNK May 24, 2019

JamesNK May 24, 2019 •

edited

Loading

steveharter May 30, 2019

JamesNK May 30, 2019

JamesNK May 30, 2019

steveharter May 30, 2019

ahsonkhan Jun 22, 2019 •

edited

Loading

steveharter Jul 15, 2019

JamesNK May 24, 2019

steveharter May 30, 2019

ahsonkhan May 30, 2019

ahsonkhan left a comment

JamesNK commented May 25, 2019

steveharter commented May 30, 2019

steveharter commented May 30, 2019

JamesNK commented May 30, 2019

Change JsonException.Path to be a json-based path #37938

Change JsonException.Path to be a json-based path #37938

Conversation

steveharter commented May 24, 2019 • edited Loading

JamesNK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK May 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK May 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jun 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan left a comment

Choose a reason for hiding this comment

JamesNK commented May 25, 2019

steveharter commented May 30, 2019

steveharter commented May 30, 2019

JamesNK commented May 30, 2019

steveharter commented May 24, 2019 •

edited

Loading

JamesNK May 24, 2019 •

edited

Loading

JamesNK May 30, 2019 •

edited

Loading

JamesNK May 31, 2019 •

edited

Loading

JamesNK May 24, 2019 •

edited

Loading

ahsonkhan Jun 22, 2019 •

edited

Loading