Add Utf8JsonWriter.WriteRawValue(System.Buffers.ReadOnlySequence).

[lateapexearlyspeed]

Fix #68223

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fix #68223

Author:	lateapexearlyspeed
Assignees:	-
Labels:	area-System.Text.Json, new-api-needs-documentation
Milestone:	-

[eiriktsarpalis]

I know this check has been taken from the ReadOnlySpan overload, but I struggle to understand what it means. Why only check for int.MaxValue lengths specifically?

For the case of ReadOnlySpan, it will almost always be false (as managed buffers are bounded by Array.MaxLength which in turn is less than int.MaxValue). For the case of ReadOnlySequence the Length property is long so any value > int.MaxValue will happily pass this check.

[eiriktsarpalis]

cc @layomia

[lateapexearlyspeed]

Fixed.
For ReadOnlySequence overload side, it is my oversight when I thought they are similar codes at first sight, sorry about that ...
For ReadOnlySpan overload side, I guess this int length cannot be int.MaxValue because subsequent code may add ','.

[eiriktsarpalis]

I guess I'm wondering if the check in the ReadOnlySpan overload is correct and whether we should be fixing it.

[lateapexearlyspeed]

Ok, just confirm, we will see @layomia 's reply about that check.

[layomia]

I know this check has been taken from the ReadOnlySpan overload

It wasn't.

Update the assertion in the core methods to say len <= int.MaxValue.

I recommend we leave as-is. We require the the input length is than int.MaxValue so we can add a comma if required. The comment talks about validation in the Utf16 based entry points (the info about integer division) but should also state that the ROSpan/Sequence entry points need to adhere to this to.

The alternative would be to throw only if the comma is required but I think the current logic is cleaner.

[layomia]

Add unit tests covering these boundary conditions

I believe these are covered here but it's still worth some thought

runtime/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Utf8JsonWriterTests.WriteRaw.cs

Line 478 in af86947

public static void WriteRawUtf16LengthGreaterThanMax(int len)

[lateapexearlyspeed]

For new ROSeq, there was also test case added to cover boundary conditions.

[eiriktsarpalis]

It wasn't.
I recommend we leave as-is. We require the the input length is than int.MaxValue so we can add a comma if required. The comment talks about validation in the Utf16 based entry points (the info about integer division) but should also state that the ROSpan/Sequence entry points need to adhere to this to.

I'm not sure I follow. Even you are able to add one comma at the end, that isn't necessary to produce a complete JSON document.

My concern is that in 99.9% of use cases, this check isn't particularly meaningful since both the source and Utf8JsonWriter destination buffers are bounded by Array.MaxLength which is substantially less than int.MaxValue. You could, in theory, construct an unmanaged span of length int.MaxValue, but if you do that then your code probably has bigger problems than not being able to append one character at the end.

In any case, I probably agree we should not try to change this in the context of this PR.

[layomia]

Utf8JsonWriter destination buffers are bounded by Array.MaxLength which is substantially less than int.MaxValue.

Yeah that's right - context I didn't keep in mind when thinking about this.

we should not try to change this in the context of this PR.

👍 yeah fine to address in a follow up.

[ghost]

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

[eiriktsarpalis]

LGTM, thanks! I've rebased your changes on top of the latest main, will merge once CI completes.

[eiriktsarpalis]

Thank you for your contribution @lateapexearlyspeed!