Add Base64 APIs to Utf8JsonReader, Utf8JsonWriter, and JsonElement#38048
Add Base64 APIs to Utf8JsonReader, Utf8JsonWriter, and JsonElement#38048ahsonkhan merged 11 commits intodotnet:masterfrom
Conversation
| byte[] unescapedArray = null; | ||
|
|
||
| Span<byte> utf8Unescaped = utf8Source.Length <= JsonConstants.StackallocThreshold ? | ||
| stackalloc byte[utf8Source.Length] : |
There was a problem hiding this comment.
Should this be stackalloced to JsonConstants.StackallocThreshold and then sliced*? I have #37973 (comment) in mind.
* Slicing is done in L24 anyway, so no need to slice after the stackallocation
There was a problem hiding this comment.
I would like to validate first/see the difference before making the change. Also, I would like to do so all up, rather than piecemeal (in a separate PR), since the current pattern is used everywhere in S.T.Json atm, and for now I want to remain consistent.
|
|
||
| if (propertyArray != null) | ||
| { | ||
| ArrayPool<char>.Shared.Return(propertyArray); |
There was a problem hiding this comment.
Here the array is not cleared on return. Though I don't know if this is critical.
There was a problem hiding this comment.
This is consistent with the other "escape" scratch space for the existing writer APIs. If we should clear, I would do this separately for all of them.
| } | ||
|
|
||
| /// <summary> | ||
| /// Attempts to represent the current JSON string as bytes assuming it is <see cref="Base64"/> encoded. |
There was a problem hiding this comment.
I don't think "Attempts" is appropriate. The return value of false should explain the failure case.
ALso are we supposed to indent by two spaces? If so, it seems like there will be a lot of 'nit' comments going forward.
There was a problem hiding this comment.
Both of these match the style of the rest of JsonElement's doc comments.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s), but failed to run 1 pipeline(s). |
|
/azp run corefx-ci |
|
Azure Pipelines failed to run 1 pipeline(s). |
|
Please let me know if you have any other critical feedback. I would like to merge this tonight, if possible. |
| Assert.Throws<FormatException>(() => root.GetUInt64()); | ||
|
|
||
| Assert.Throws<InvalidOperationException>(() => root.GetString()); | ||
| Assert.Throws<InvalidOperationException>(() => root.GetBytesFromBase64()); |
There was a problem hiding this comment.
About half of the "invalid state" test blocks are missing TryGetBytesFromBase64
…otnet/corefx#38048) * Add Utf8JsonWriter Base64 APIs. * Add Utf8JsonReader Base64 APIs. * Add JsonElement Base64 APIs. * Update API shape based on review. * Auto-generate the ref assembly. * Address PR feedback so far. * Add escaping step and update length counters accordingly. * Add JsonWriter API tests. * Add JsonReader and JsonElement tests. Commit migrated from dotnet/corefx@82408cd
4 participants
Fixes https://github.com/dotnet/corefx/issues/36974
TODO: