Consume comments changes by WinCPP · Pull Request #35934 · dotnet/corefx

WinCPP · 2019-03-11T01:42:08Z

Changes to ValueSpan / ValueSequence to exclude comment delimiters as per point 2 below,

Would it make sense to strip the comment delimiters (\\, /* and */) and the line separators (\r, \r\n and \n) during parsing where the ValueSpan or ValueSequence for the JsonTokenType.Comment is prepared?

Yes, it would.

In that case, should I do the changes in PR #33393 or should I do them in this PR?

Let's stage it in smaller, clearer chunks, where the change is isolated. So, in this order:

Utf8JsonReader.cs - Add support for single line comments ending on \r, \r\n #33393 - Add support for single line comments ending on \r, \r\n
Open up a new PR - Change ValueSpan/ValueSequence to exclude the comment delimiters, effectively undoing parts of 4a2aa63 from Add JsonUtf8Reader (for ReadOnlySpan) along with unit tests #33216
This PR (Implement GetComment API #35705) to expose new GetComment API

What do you think of this approach?

Query: IIUC this line in JsonReaderHelper.CountNewLines needs to handle \r on its own as a line separator, and also ensure that \r\n are handled properly... ?

Unless this is blocking some of the work above, I would file a new issue for this and investigate/fix separately. Some questions we likely want to resolve first: What does Json.NET return for line number when comments end with just \r (for Windows vs Unix)? Do we double count line number \r\n or treat it as a single line increment?

cc @JamesNK

Originally posted by @ahsonkhan in #35705

WinCPP · 2019-03-16T18:04:16Z

@ahsonkhan I have checked in the fixes for review comments. Please check when you get a moment. Thanks!

WinCPP · 2019-03-28T13:19:03Z

@ahsonkhan appreciate your review comments on this one! Thanks!

ahsonkhan

Some feedback and questions. Apologies for the delayed review.

WinCPP · 2019-04-11T05:34:15Z

@ahsonkhan I have uploaded the entirely reworked logic. Please review referencing the original logic before this PR.

WinCPP · 2019-04-18T01:38:25Z

@ahsonkhan the code is ready for review with all the changes. Could you please review it. It also has fix for the existing issue that I mentioned in here.

ahsonkhan

Some more feedback/questions. Apologies for the late response.

WinCPP · 2019-05-04T19:25:53Z

@ahsonkhan I have fixed the latest review comment as well and resolved the code conflict. Please check.

ahsonkhan

Otherwise, LGTM.

ahsonkhan · 2019-05-06T03:37:06Z

I have fixed the latest review comment as well and resolved the code conflict. Please check.

@WinCPP, thanks for your patience and effort. Much appreciated.

We are almost at the finish line :)

I just have a nit and a leftover question around consistency when throwing particular exception messages.

WinCPP · 2019-05-16T05:31:58Z

Sorry I completely missed this communication. Will do it today. Thanks for reminder :-)

…

On Thu, May 16, 2019 at 8:25 AM Ahson Khan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs <#35934 (comment)>: > if (localBuffer.Length == 0) { if (IsLastSpan) { - ThrowHelper.ThrowJsonReaderException(ref this, ExceptionResource.ExpectedStartOfValueNotFound, JsonConstants.Slash); + ThrowHelper.ThrowJsonReaderException(ref this, ExceptionResource.UnexpectedEndOfDataWithinStartOfComment); @WinCPP <https://github.com/WinCPP> - let me know when you plan to address this final feedback + resolve the merge conflict. Thanks for your effort dealing with all the subtleties. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35934?email_source=notifications&email_token=ACEH6MX3T2DUXR7PC6LWBPLPVTEK5A5CNFSM4G46TBOKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBYZAB4I#discussion_r284522538>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACEH6MRQU3WGVFN2V5BVFSLPVTEK5ANCNFSM4G46TBOA> .

ahsonkhan

Returning "end of data" exception message for incorrect character within a comment is not correct.

ahsonkhan · 2019-05-17T02:47:04Z

                else
                {
-                    ThrowHelper.ThrowJsonReaderException(ref this, ExceptionResource.UnexpectedEndOfDataWithinStartOfComment);
+                    ThrowHelper.ThrowJsonReaderException(ref this, ExceptionResource.UnexpectedEndOfDataWhileReadingComment);


~~Use InvalidCharacterAtStartOfComment here. UnexpectedEndOfDataWhileReadingComment doesn't make sense for this case.~~

OR, better yet: since this is "Skip" and not "Consume", keep ExpectedStartOfValueNotFound here.

Skip in Multi-segment does the right thing. Single-segment should do the same:

corefx/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs

Line 2179 in 372fb7e

ThrowHelper.ThrowJsonReaderException(ref this, ExceptionResource.ExpectedStartOfValueNotFound, JsonConstants.Slash);

corefx/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs

Line 2186 in 372fb7e

ThrowHelper.ThrowJsonReaderException(ref this, ExceptionResource.ExpectedStartOfValueNotFound, JsonConstants.Slash);

corefx/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs

Line 2206 in 372fb7e

ThrowHelper.ThrowJsonReaderException(ref this, ExceptionResource.ExpectedStartOfValueNotFound, marker);

The value sent is JsonConstants.Slash but that could be an * as well, which we don't know yet because it is not there. Essentially, it could be / or * and we don't have placeholders for two characters in that exception. This is where all my thought process to have all those separate exception messages started.

So was the original exception that just emitted a / correct?

At line 2179 and 2186, the buffer length is already zero. We don't know the intention yet whether it was to follow with a / or * for single / multi line comment respectively. That is why I considered ExpectedStartOfComment as a wrong message. Because it expects a / (only). Why not *? We can't say anything if there is nothing!

At 2206, I did get confused because the exception resource name says ExpectedStartOfValueNotFound where as the actual message is <value> is invalid start of a value.. Perhaps, the exception resource name should be UnexpectedStartOfValue to align with the contents of exception message.

I see the confusion. Maybe I can try to help explain my perspective.

The value sent is JsonConstants.Slash but that could be an * as well, which we don't know yet because it is not there.

The reason why value sent is JsonConstants.Slash is because before calling Skip(), we already saw a slash (that's what got us into the Skip method). See:

corefx/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs

Lines 895 to 897 in 6f9570e

if (marker == JsonConstants.Slash)

{

if (SkipComment())

It doesn't matter if * follows next or /. Skip mode tries to ignore comments all together. So, the exception user sees is, "we saw / and then either we hit end of data or something invalid". / is not a valid start of value which is what we would expect (think of skip as if comments don't exist at all in JSON). That's why we have the ExpectedStartOfValueNotFound message.

That is why I considered ExpectedStartOfComment as a wrong message.

For "Skip" mode, I agree, we shouldn't return ExpectedStartOfComment. I am suggesting we return ExpectedStartOfValueNotFound. I don't think it makes sense for the user to see a comment related message when the user said "skip" them.

Let's say you have invalid JSON like this: {"property": /}. I expect an exception saying / is invalid start of a value. We should throw that regardless of whether the user was in "skip" or "disallow".

So was the original exception that just emitted a / correct?

In my opinion, yes.

Perhaps, the exception resource name should be UnexpectedStartOfValue to align with the contents of exception message.

Maybe, but I don't feel strongly about it. If you think it makes it clearer, I would be ok with renaming it.

@ahsonkhan I get the idea now. Thank you for the clarification. Didn't realize the subtlety in two usages of one exception message.

I have submitted the changes. Hope I have finally got them as wanted. Thanks for patience.

ahsonkhan · 2019-05-17T03:01:59Z

@WinCPP, here are all the cases we need to be consistent on.

Behavior of single-segment Skip/Consume should match the behavior of multi-segment Skip/Consume.

For invalid or incomplete data, Skip should throw ExpectedStartOfValueNotFound in all cases.

Consume should throw UnexpectedEndOfDataWhileReadingComment anywhere we hit end of data while reading. It should throw InvalidCharacterAtStartOfComment for invalid data.

To summarize:

Single-segment
a) Skip
- End of Data - ExpectedStartOfValueNotFound
  - <value>'{0}' is an invalid start of a value.</value>
- Invalid - ExpectedStartOfValueNotFound
  - <value>'{0}' is an invalid start of a value.</value>
b) Consume
- End of Data - UnexpectedEndOfDataWhileReadingComment
  - <value>Unexpected end of data while reading a comment.</value>
- Invalid - InvalidCharacterAtStartOfComment
  - <value>'{0}' is invalid after '/' at the beginning of the comment. Expected either '/' or '*'.</value>
Multi-segment
a) Skip
- End of Data - ExpectedStartOfValueNotFound
  - <value>'{0}' is an invalid start of a value.</value>
- Invalid - ExpectedStartOfValueNotFound
  - <value>'{0}' is an invalid start of a value.</value>
b) Consume
- End of Data - UnexpectedEndOfDataWhileReadingComment
  - <value>Unexpected end of data while reading a comment.</value>
- Invalid - InvalidCharacterAtStartOfComment
  - <value>'{0}' is invalid after '/' at the beginning of the comment. Expected either '/' or '*'.</value>

ahsonkhan · 2019-05-17T22:37:32Z

Let's stage it in smaller, clearer chunks, where the change is isolated

This PR (Implement GetComment API #35705) to expose new GetComment API

@WinCPP - thanks for the fix. Looks like the next step is #35705.

* Consume comments changes * Fixed issues in branch splitting * Use sequence positions. Elaborate comments. * Reworked algos, review comment fixes * Fixed review comments * minor comment fix * Standardize exception messages * Clean up exception messages Commit migrated from dotnet/corefx@d72d720

WinCPP changed the title ~~WIP: Consume comments changes~~ Consume comments changes Mar 11, 2019

ahsonkhan added the area-System.Text.Json label Mar 11, 2019

ahsonkhan added this to the 3.0 milestone Mar 11, 2019

ahsonkhan reviewed Mar 11, 2019

View reviewed changes

WinCPP force-pushed the extcom branch from 1bf3875 to d5164c4 Compare March 14, 2019 17:54

karelz assigned WinCPP, ahsonkhan and steveharter Mar 18, 2019

WinCPP mentioned this pull request Mar 18, 2019

Implement GetComment API #35705

Merged

ahsonkhan reviewed Mar 28, 2019

View reviewed changes

WinCPP force-pushed the extcom branch 2 times, most recently from b2bdff6 to aa6b47f Compare April 8, 2019 23:18

ahsonkhan reviewed Apr 24, 2019

View reviewed changes

WinCPP force-pushed the extcom branch from aa6b47f to 41eb718 Compare April 26, 2019 20:19

ahsonkhan reviewed May 6, 2019

View reviewed changes

Comment thread src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.MultiSegment.cs Outdated

ahsonkhan approved these changes May 6, 2019

View reviewed changes

ahsonkhan requested a review from bartonjs May 6, 2019 03:33

ahsonkhan reviewed May 6, 2019

View reviewed changes

Comment thread src/System.Text.Json/src/System/Text/Json/ThrowHelper.cs Outdated

ahsonkhan reviewed May 6, 2019

View reviewed changes

Comment thread src/System.Text.Json/tests/Utf8JsonReaderTests.MultiSegment.cs Outdated

WinCPP force-pushed the extcom branch from 8fcdc1a to 372fb7e Compare May 16, 2019 21:21

ahsonkhan reviewed May 17, 2019

View reviewed changes

Comment thread src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs Outdated

WinCPP added 8 commits May 18, 2019 01:37

Consume comments changes

d30ef1f

Fixed issues in branch splitting

95f0896

Use sequence positions. Elaborate comments.

586d929

Reworked algos, review comment fixes

4884ff1

Fixed review comments

03462b0

minor comment fix

6e859c3

Standardize exception messages

022a077

Clean up exception messages

15b2509

WinCPP force-pushed the extcom branch from 372fb7e to 15b2509 Compare May 17, 2019 20:17

ahsonkhan approved these changes May 17, 2019

View reviewed changes

ahsonkhan merged commit d72d720 into dotnet:master May 17, 2019

ahsonkhan mentioned this pull request May 20, 2019

SkipValidation when using the writer internally within the serializer. #37791

Merged

WinCPP deleted the extcom branch September 9, 2019 03:38

Conversation

WinCPP commented Mar 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WinCPP commented Mar 16, 2019

Uh oh!

WinCPP commented Mar 28, 2019

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WinCPP commented Apr 11, 2019

Uh oh!

WinCPP commented Apr 18, 2019

Uh oh!

ahsonkhan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WinCPP commented May 4, 2019

Uh oh!

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ahsonkhan commented May 6, 2019

Uh oh!

WinCPP commented May 16, 2019 via email

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahsonkhan May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan May 17, 2019

Choose a reason for hiding this comment

Uh oh!

WinCPP May 17, 2019

Choose a reason for hiding this comment

Uh oh!

WinCPP May 17, 2019

Choose a reason for hiding this comment

Uh oh!

ahsonkhan left a comment •

edited

Loading

ahsonkhan May 17, 2019 •

edited

Loading

ahsonkhan May 17, 2019 •

edited

Loading

ahsonkhan commented May 17, 2019 •

edited

Loading