Fix issues that are variants of those #1182 #1184
Conversation
| @@ -962,9 +962,15 @@ public static void PopulateObject(string value, object target, JsonSerializerSet | |||
| { | |||
| jsonSerializer.Populate(jsonReader, target); | |||
|
|
|||
| if (jsonReader.Read() && jsonReader.TokenType != JsonToken.Comment) | |||
| if (settings != null && settings.CheckAdditionalContent) | |||
There was a problem hiding this comment.
Adding this check because it seems that the current behaviour of throwing when there isn't a setting requesting it is, given that such a setting exists and is available to the user, a bug in itself. Maybe I'm misinterpreting the intent here.
| { | ||
| throw JsonReaderException.Create(reader, "Additional text found in JSON string after parsing content."); | ||
| if (reader.TokenType != JsonToken.Comment) |
There was a problem hiding this comment.
Should this also throw if settings.CommentHandling == CommentHandling.Load?
There was a problem hiding this comment.
No. That is specific to comments in arrays. It really just exists as a backwards compatibility toggle for older bad behavior.
There was a problem hiding this comment.
Ah. That makes that whole setting make more sense now :)
| { | ||
| if (jsonReader.TokenType != JsonToken.Comment) | ||
| { | ||
| throw new JsonSerializationException("Additional text found in JSON string after finishing deserializing object."); |
There was a problem hiding this comment.
Change this exception to use JsonSerializationException.Create(reader, "blah blah")
| { | ||
| throw JsonReaderException.Create(reader, "Additional text found in JSON string after parsing content."); | ||
| if (reader.TokenType != JsonToken.Comment) |
There was a problem hiding this comment.
No. That is specific to comments in arrays. It really just exists as a backwards compatibility toggle for older bad behavior.
| @@ -962,9 +962,15 @@ public static void PopulateObject(string value, object target, JsonSerializerSet | |||
| { | |||
| jsonSerializer.Populate(jsonReader, target); | |||
|
|
|||
| if (jsonReader.Read() && jsonReader.TokenType != JsonToken.Comment) | |||
| if (settings != null && settings.CheckAdditionalContent) | |||
|
Changes look good. I'd like tests of the new behavior - exceptions with more detail and checking for additional content - before merging. |
| { | ||
| throw JsonReaderException.Create(reader, "Additional text found in JSON string after parsing content."); | ||
| // Any content encountered here other than a comment will throw in the reader. |
There was a problem hiding this comment.
As became apparent. There's no way to actually detect reader.TokenType != JsonToken.Comment without catching the exception from the reader itself, and little point since the exception thrown is applicable.
Fix issues similar to #1182 in
JTokenand derived types.Include line information in exception.