-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add JsonElement.ValueSpan #77666
Comments
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsCurrently Utf8JsonReader exposes ValueSpan. It would be useful to have the same member on JsonElement. cc: @eiriktsarpalis @KrzysztofCwalina
|
While I like this suggestion in principle, there has been much discussion elsewhere about the level of abstraction expected from JsonElement, and whether this should be providing an encoded or decoded span, and (optionally) efficient conversion to There is a proposal in #74028 which addresses these issues for the |
One concern I have with this proposal is that similar to |
FWIW this was considered in the past and rejected, cf. #30207 (comment) cc @bartonjs |
A CopyString method, either escaping or not (maybe CopyString (unescaping) and CopyRawString (only transcoding, when needed)?), would be OK. The Span property is inappropriate, because the lifetime isn't obvious. ReadOnlySpan<byte> propValRaw = element.ValueSpan;
DoSomeStuff();
DoMoreStuff(propValRaw); If
The "I'll copy it to your buffer for you" method is more clear, and someone who needs the hyper-performance of inspecting it just has to drop to using the reader directly. |
Also, for what it's worth, I still think that anything that removes the possibility of UTF-8-or-UTF-16(-or-other) from JsonDocument/JsonElement is bad... which also favors the copy method over the property. |
Is this by virtue of the fact that cc @KrzysztofCwalina who had mentioned in the past that a copying approach would not meet their requirements. |
Utf8JsonReader.ValueSpan produces slices of a span (or sequence) you gave it. It's your buffer to corrupt. JsonElement's theoretical ValueSpan is over a buffer that is either from the array pool, or gcnew, but you can't know at the call site. The easy rule on spans is: Don't return a span that you weren't given. The relevant span is a detail from JsonDocument, not a thing JsonDocument was given, so it shouldn't be returned. The closest analogue here is Maybe there's an extremely convincing argument out there somewhere, but from a general framework approach (this point being about security/reliability) the property approach is a no-go. I don't see a path for JsonElement here other than a copy method; and for the scenario the answer is to use Utf8JsonReader. |
Could we expose this as TryGetValueSpan that would succeed only if the data is in the original buffer passed to Parse? |
A |
Although.... since it's an out, not a return, they could just use normal overloading... so maybe |
Not sure I like having a try method whose success also depends on how the object got constructed -- it is an intentional property encapsulated by the object and many components might not be able to control it. If we do end up doing it like that it should have a name that discourages general-purpose usage. |
Stream.Seek (and many other APIs in .NET) success depends on how the stream got created. |
I recently had to implement this internally for #103733. Here's what I ended up with: namespace System.Text.Json;
public partial struct JsonElement
{
+ public bool ValueIsEscaped { get; }
+ public ReadOnlySpan<byte> ValueSpan { get; }
}
public partial struct JsonProperty
{
public string Name { get; }
+ public bool NameIsEscaped { get; }
+ public ReadOnlySpan<byte> NameSpan { get; }
} While it doesn't address unescaping directly, this follows the API as exposed by |
After a lot of discussion that invented a JsonMarshal type, or adding new overloads of NameEquals/ValueEquals, we ended up deciding to not decide anything right now; we need a better understanding of the driving concerns. namespace System.Text.Json
{
public partial struct JsonElement
{
public bool ValueEquals(JsonElement stringElement);
}
public partial struct JsonProperty
{
public bool NameEquals(JsonProperty other);
}
}
namespace System.Runtime.InteropServices
{
// Very raw thoughts here, take with very large grains of salt.
public partial class JsonMarshal
{
public static bool TryGetRawValue(JsonElement element, out ReadOnlySpan<byte> rawValue);
public static bool TryGetStringValueSpan(JsonElement element, out ReadOnlySpan<byte> utf8ValueSpan);
public static bool TryGetStringValueSpan(JsonElement element, out ReadOnlySpan<char> valueSpan);
public static bool TryGetNameSpan(JsonProperty property, out ReadOnlySpan<byte> utf8ValueSpan);
public static bool TryGetNameSpan(JsonProperty property, out ReadOnlySpan<char> valueSpan);
}
} |
@KrzysztofCwalina @annelo-msft one question that came up while reviewing this is whether there is a need to extract the raw span in cases where the |
I'm either amused, or dismayed, that during the discussion we ended up saying almost word for word everything previously mentioned in this issue. (Including me starting with "don't return the span" and then later remembering "oh, right, and the theoretical UTF-16 future expansion") |
Having walked through everything again, I'm happy with |
If it only returns true in safe contexts, why hide it away in a marshal class? I think this would both be a source of usability problems and also not unblock valid uses cases that consume disposable documents. |
Fair, it could also live on JsonElement as the Try. I guess JsonMarshal was just in my head. |
@eiriktsarpalis, yeah, we often need the whole raw JSON of a complex object or array of values |
Is escaped JSON, comments or erratic indentation something that would be concerning in that case? |
And to follow up on that question, returning the raw JSON would typically involve including quotes in string values. Is that desirable? |
Following conversation with @annelo-msft we identified the key use case is offering a non-allocating variant of the namespace System.Runtime.InteropServices;
public partial class JsonMarshal
{
public static bool TryGetRawValue(JsonElement element, out ReadOnlySpan<byte> rawValue);
} Furthermore:
|
@eiriktsarpalis Please either put the proposal in the top post, or, at minimum, update the top post link to point to the item. Especially in "crunch time" full agenda meetings, we will start skipping anything (pushing it to the bottom of the day agenda) that is not in one of those two states. |
namespace System.Runtime.InteropServices;
public partial class JsonMarshal
{
public static ReadOnlySpan<byte> GetRawUtf8Value(JsonElement element);
} |
Updated API Proposal
Following conversation with @annelo-msft we identified the key use case is offering a non-allocating variant of the
JsonElement.GetRawText
method. As such we should probably go ahead with theJsonMarshal
design:Furthermore:
The text was updated successfully, but these errors were encountered: