Skip to content
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 public JsonElement.ParseValue() and TryParseValue() #43601

Merged
merged 2 commits into from
Oct 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/libraries/System.Text.Json/ref/System.Text.Json.cs
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public readonly partial struct JsonElement
[System.CLSCompliantAttribute(false)]
public ulong GetUInt64() { throw null; }
public override string? ToString() { throw null; }
public static System.Text.Json.JsonElement ParseValue(ref System.Text.Json.Utf8JsonReader reader) { throw null; }
public bool TryGetByte(out byte value) { throw null; }
public bool TryGetBytesFromBase64([System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] out byte[]? value) { throw null; }
public bool TryGetDateTime(out System.DateTime value) { throw null; }
Expand All @@ -92,6 +93,7 @@ public readonly partial struct JsonElement
public bool TryGetUInt32(out uint value) { throw null; }
[System.CLSCompliantAttribute(false)]
public bool TryGetUInt64(out ulong value) { throw null; }
public static bool TryParseValue(ref System.Text.Json.Utf8JsonReader reader, [System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] out System.Text.Json.JsonElement? element) { throw null; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonElement is a struct. What does NotNullWhen(true) attribute mean for value types like this?

I would have thought we wouldn't need it.

Suggested change
public static bool TryParseValue(ref System.Text.Json.Utf8JsonReader reader, [System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] out System.Text.Json.JsonElement? element) { throw null; }
public static bool TryParseValue(ref System.Text.Json.Utf8JsonReader reader, out System.Text.Json.JsonElement? element) { throw null; }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TryParseValue's out param returns Nullable<JsonElement> so we need the attribute. Using Nullable instead of default(JsonElement) prevents any possible misuse of default(JsonElement) and perhaps a bit less on the stack.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. That makes more sense now :) Thanks.

public bool ValueEquals(System.ReadOnlySpan<byte> utf8Text) { throw null; }
public bool ValueEquals(System.ReadOnlySpan<char> text) { throw null; }
public bool ValueEquals(string? text) { throw null; }
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Text.Json/src/System.Text.Json.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
<Compile Include="System\Text\Json\Document\JsonElement.ArrayEnumerator.cs" />
<Compile Include="System\Text\Json\Document\JsonElement.cs" />
<Compile Include="System\Text\Json\Document\JsonElement.ObjectEnumerator.cs" />
<Compile Include="System\Text\Json\Document\JsonElement.Parse.cs" />
<Compile Include="System\Text\Json\Document\JsonProperty.cs" />
<Compile Include="System\Text\Json\Document\JsonValueKind.cs" />
<Compile Include="System\Text\Json\JsonCommentHandling.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,8 @@ public static JsonDocument ParseValue(ref Utf8JsonReader reader)
bool ret = TryParseValue(ref reader, out JsonDocument? document, shouldThrow: true, useArrayPools: true);

Debug.Assert(ret, "TryParseValue returned false with shouldThrow: true.");
return document!;
Debug.Assert(document != null, "null document returned with shouldThrow: true.");
return document;
Comment on lines +340 to +341
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

}

internal static bool TryParseValue(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace System.Text.Json
{
public readonly partial struct JsonElement
{
/// <summary>
/// Parses one JSON value (including objects or arrays) from the provided reader.
/// </summary>
/// <param name="reader">The reader to read.</param>
/// <returns>
/// A JsonElement representing the value (and nested values) read from the reader.
/// </returns>
/// <remarks>
/// <para>
/// If the <see cref="Utf8JsonReader.TokenType"/> property of <paramref name="reader"/>
/// is <see cref="JsonTokenType.PropertyName"/> or <see cref="JsonTokenType.None"/>, the
/// reader will be advanced by one call to <see cref="Utf8JsonReader.Read"/> to determine
/// the start of the value.
/// </para>
///
/// <para>
/// Upon completion of this method <paramref name="reader"/> will be positioned at the
/// final token in the JSON value. If an exception is thrown the reader is reset to
/// the state it was in when the method was called.
/// </para>
///
/// <para>
/// This method makes a copy of the data the reader acted on, so there is no caller
/// requirement to maintain data integrity beyond the return of this method.
/// </para>
/// </remarks>
/// <exception cref="ArgumentException">
/// <paramref name="reader"/> is using unsupported options.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should either update the published docs to match this phrasing, or update this new phrasing to match the published one on JsonDocument.ParseValue:
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsondocument.parsevalue?view=netcore-3.1

FWIW, I prefer the phrasing in this PR over contains unsupported options.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm it's always been "contains unsupported options" on the Document side.

So the doc review changed to "is using unsupported options.", so it makes sense to change all copies of that sentence. cc @tdykstra

/// </exception>
/// <exception cref="ArgumentException">
/// The current <paramref name="reader"/> token does not start or represent a value.
/// </exception>
/// <exception cref="JsonException">
/// A value could not be read from the reader.
/// </exception>
public static JsonElement ParseValue(ref Utf8JsonReader reader)
{
bool ret = JsonDocument.TryParseValue(ref reader, out JsonDocument? document, shouldThrow: true, useArrayPools: false);

Debug.Assert(ret, "TryParseValue returned false with shouldThrow: true.");
Debug.Assert(document != null, "null document returned with shouldThrow: true.");
return document.RootElement;
}

/// <summary>
/// Attempts to parse one JSON value (including objects or arrays) from the provided reader.
/// </summary>
/// <param name="reader">The reader to read.</param>
/// <param name="element">Receives the parsed element.</param>
/// <returns>
/// <see langword="true"/> if a value was read and parsed into a JsonElement,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Matching https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsondocument.tryparsevalue?view=netcore-3.1

Suggested change
/// <see langword="true"/> if a value was read and parsed into a JsonElement,
/// <see langword="true"/> if a value was read and parsed into a JsonElement;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'll also update the JsonDocument since this element comment was copied from that.

/// <see langword="false"/> if the reader ran out of data while parsing.
/// All other situations result in an exception being thrown.
/// </returns>
/// <remarks>
/// <para>
/// If the <see cref="Utf8JsonReader.TokenType"/> property of <paramref name="reader"/>
/// is <see cref="JsonTokenType.PropertyName"/> or <see cref="JsonTokenType.None"/>, the
/// reader will be advanced by one call to <see cref="Utf8JsonReader.Read"/> to determine
/// the start of the value.
/// </para>
///
/// <para>
/// Upon completion of this method <paramref name="reader"/> will be positioned at the
/// final token in the JSON value. If an exception is thrown, or <see langword="false"/>
/// is returned, the reader is reset to the state it was in when the method was called.
/// </para>
///
/// <para>
/// This method makes a copy of the data the reader acted on, so there is no caller
/// requirement to maintain data integrity beyond the return of this method.
/// </para>
/// </remarks>
/// <exception cref="ArgumentException">
/// <paramref name="reader"/> is using unsupported options.
/// </exception>
/// <exception cref="ArgumentException">
/// The current <paramref name="reader"/> token does not start or represent a value.
/// </exception>
/// <exception cref="JsonException">
/// A value could not be read from the reader.
/// </exception>
public static bool TryParseValue(ref Utf8JsonReader reader, [NotNullWhen(true)] out JsonElement? element)
{
bool ret = JsonDocument.TryParseValue(ref reader, out JsonDocument? document, shouldThrow: false, useArrayPools: false);
element = document?.RootElement;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, only set element if ret is true.

Suggested change
element = document?.RootElement;
if (ret)
{
element = document?.RootElement;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing code is shorter.

Since the out param is nullable we still need to initialize element so it would be:

            if (JsonDocument.TryParseValue(...
            {
                element = document.RootElement;
                return true;
            }

            element = null;
            return false;

which is equivalent in functionality to the current code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed that part. Makes sense.

return ret;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,6 @@ internal JsonElement(JsonDocument parent, int idx)
_idx = idx;
}

// Currently used only as an optimization by the serializer, which does not want to
// return elements that are based on the <see cref="JsonDocument.Dispose"/> pattern.
internal static JsonElement ParseValue(ref Utf8JsonReader reader)
{
bool ret = JsonDocument.TryParseValue(ref reader, out JsonDocument? document, shouldThrow: true, useArrayPools: false);

Debug.Assert(ret != false, "Parse returned false with shouldThrow: true.");
Debug.Assert(document != null, "null document returned with shouldThrow: true.");
return document.RootElement;
}

[DebuggerBrowsable(DebuggerBrowsableState.Never)]
private JsonTokenType TokenType
{
Expand Down
142 changes: 142 additions & 0 deletions src/libraries/System.Text.Json/tests/JsonElementParseTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Xunit;
using System.Collections.Generic;

namespace System.Text.Json.Tests
{
public static class JsonElementParseTests
{
public static IEnumerable<object[]> ElementParseCases
{
get
{
yield return new object[] { "null", JsonValueKind.Null };
yield return new object[] { "true", JsonValueKind.True };
yield return new object[] { "false", JsonValueKind.False };
yield return new object[] { "\"MyString\"", JsonValueKind.String };
Comment on lines +17 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a number token and a string containing escaped characters.

yield return new object[] { "{}", JsonValueKind.Object };
yield return new object[] { "[]", JsonValueKind.Array };
}
}

[Theory]
[MemberData(nameof(ElementParseCases))]
public static void ParseValue(string json, JsonValueKind kind)
{
var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));

JsonElement? element = JsonElement.ParseValue(ref reader);
Assert.Equal(kind, element!.Value.ValueKind);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also assert the state of the reader after the call to ParseValue

}

[Theory]
[MemberData(nameof(ElementParseCases))]
public static void TryParseValue(string json, JsonValueKind kind)
{
var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));

bool success = JsonElement.TryParseValue(ref reader, out JsonElement? element);
Assert.True(success);
Assert.Equal(kind, element!.Value.ValueKind);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. Where is the reader now after the call which mutated it?

}

public static IEnumerable<object[]> ElementParsePartialDataCases
{
get
{
yield return new object[] { "\"MyString"};
yield return new object[] { "{" };
yield return new object[] { "[" };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add empty string an string with just whitespace.

}
}

[Theory]
[MemberData(nameof(ElementParsePartialDataCases))]
public static void ParseValuePartialDataFail(string json)
{
var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));

try
{
JsonElement.ParseValue(ref reader);
Assert.True(false, "Expected exception.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assert.True(false, "Expected exception.");
Assert.True(false, "Expected JsonException.");

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see another pattern in the Document tests that as an Exception variable and assigns that within the catch plus a couple Asserts for that variable, so I'll change to that which avoids the literal string.

}
catch (JsonException) { }
}

[Theory]
[MemberData(nameof(ElementParsePartialDataCases))]
public static void TryParseValuePartialDataFail(string json)
{
var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));

try
{
JsonElement.TryParseValue(ref reader, out JsonElement? element);
Assert.True(false, "Expected exception.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assert.True(false, "Expected exception.");
Assert.True(false, "Expected JsonException.");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test that the reader state doesn't change if TryParseValue returned false or threw (same with ParseValue), by first moving the reader to some nested object/state to test against.

Copy link
Member Author

@steveharter steveharter Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking to see if BytesConsumed is zero that should be sufficient -- I'll add that.

As mentioned in the PR description, this PR doesn't change the core logic, so it doesn't duplicate the bulk of the tests.

}
catch (JsonException) { }
}

[Theory]
[MemberData(nameof(ElementParsePartialDataCases))]
public static void ParseValueOutOfData(string json)
{
var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json), isFinalBlock: false, new JsonReaderState());

try
{
JsonElement.ParseValue(ref reader);
Assert.True(false, "Expected exception.");
}
catch (JsonException) { }
}

[Theory]
[MemberData(nameof(ElementParsePartialDataCases))]
public static void TryParseValueOutOfData(string json)
{
var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json), isFinalBlock: false, new JsonReaderState());
Assert.False(JsonElement.TryParseValue(ref reader, out JsonElement? element));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verify that JsonElement is still default/it's value kind is Undefined.

Additionally, maybe pass in an already initialized JsonElement and validate it doesn't change when TryParseValue returns false.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since a Nullable is returned, it will be null. I'll add a check for that.

}

public static IEnumerable<object[]> ElementParseInvalidDataCases
{
get
{
yield return new object[] { "nul" };
yield return new object[] { "{]" };
}
}

[Theory]
[MemberData(nameof(ElementParseInvalidDataCases))]
public static void ParseValueInvalidDataFail(string json)
{
var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));

try
{
JsonElement.ParseValue(ref reader);
Assert.True(false, "Expected exception.");
}
catch (JsonException) { }
}

[Theory]
[MemberData(nameof(ElementParseInvalidDataCases))]
public static void TryParseValueInvalidDataFail(string json)
{
var reader = new Utf8JsonReader(Encoding.UTF8.GetBytes(json));

try
{
JsonElement.TryParseValue(ref reader, out JsonElement? element);
Assert.True(false, "Expected exception.");
}
catch (JsonException) { }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
<Compile Include="JsonDateTimeTestData.cs" />
<Compile Include="JsonDocumentTests.cs" />
<Compile Include="JsonElementCloneTests.cs" />
<Compile Include="JsonElementParseTests.cs" />
<Compile Include="JsonElementWriteTests.cs" />
<Compile Include="JsonEncodedTextTests.cs" />
<Compile Include="JsonGuidTestData.cs" />
Expand Down