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.

Sign up for GitHub

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

Jump to bottom

Additional tests for System.Text.Json #32705

Merged
merged 9 commits into from
Feb 29, 2020
Merged

Additional tests for System.Text.Json #32705

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4b5f5c2
added multi-segment DateTime tests
alanisaac Feb 23, 2020
a4c5847
added missing null argument test
alanisaac Feb 23, 2020
a8374d9
added null argument tests and missing check for SerializeAsync
alanisaac Feb 23, 2020
8090c69
removed unreachable defensive code block
alanisaac Feb 23, 2020
1e8e33c
sealed class type for dictionary converters and removed unreachable code
alanisaac Feb 23, 2020
7a6e0cb
added additional test permutation
alanisaac Feb 26, 2020
823f147
added slices to DateTime conversion tests
alanisaac Feb 26, 2020
256de92
Revert "sealed class type for dictionary converters and removed unrea…
alanisaac Feb 29, 2020
02ab96b
sealed class type for internal json converters
alanisaac Feb 29, 2020
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
46 changes: 14 additions & 32 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,45 +290,27 @@ internal bool TryWriteDataExtensionProperty(Utf8JsonWriter writer, T value, Json
bool success;
JsonDictionaryConverter<T> dictionaryConverter = (JsonDictionaryConverter<T>)this;

if (ClassType == ClassType.Value)
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if it is OK to remove this (it's possible that it is OK to do, but is surprising to me). What happens when the user defines and register their own JsonConverter<T> for dictionaries?

@steveharter, @layomia - please review this when you get a chance. I suspect this branch was needed for something (which means we need to provide the test case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I touched on this in Note 1 in the above comments, but I'm not sure it's possible. The line immediately above this one casts the current instance to a JsonDictionaryConverter<T> which is marked internal, so my assumption was users wouldn't be able to inherit it (unless there's something I'm not thinking of or aware of?)

I can certainly leave this in there if that's the call, but I think then testing and getting coverage on these lines would be difficult without introducing an InternalsVisibleTo relationship (or similar internals-testing-pattern) between the src and test project. That or adding an inherited class in the src project for the purposes of testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a bug with writing extension data if there is a custom converter for the extension property: #32903. This is the reason why the L291 is not problematic (InvalidCastException) - we throw an NRE up stack.

We'll likely need this branch when we fix that bug, so for this PR we should revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted in 256de92

{
Debug.Assert(!state.IsContinuation);

int originalPropertyDepth = writer.CurrentDepth;
bool isContinuation = state.IsContinuation;

// Ignore the naming policy for extension data.
state.Current.IgnoreDictionaryKeyPolicy = true;
state.Push();

success = dictionaryConverter.OnWriteResume(writer, value, options, ref state);
if (success)
{
VerifyWrite(originalPropertyDepth, writer);
}
}
else
if (!isContinuation)
{
bool isContinuation = state.IsContinuation;

state.Push();

if (!isContinuation)
{
Debug.Assert(state.Current.OriginalDepth == 0);
state.Current.OriginalDepth = writer.CurrentDepth;
}
Debug.Assert(state.Current.OriginalDepth == 0);
state.Current.OriginalDepth = writer.CurrentDepth;
}

// Ignore the naming policy for extension data.
state.Current.IgnoreDictionaryKeyPolicy = true;
// Ignore the naming policy for extension data.
state.Current.IgnoreDictionaryKeyPolicy = true;

success = dictionaryConverter.OnWriteResume(writer, value, options, ref state);
if (success)
{
VerifyWrite(state.Current.OriginalDepth, writer);
}

state.Pop(success);
success = dictionaryConverter.OnWriteResume(writer, value, options, ref state);
if (success)
{
VerifyWrite(state.Current.OriginalDepth, writer);
}

state.Pop(success);

return success;
}

Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonDictionaryConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace System.Text.Json.Serialization
/// </summary>
internal abstract class JsonDictionaryConverter<T> : JsonResumableConverter<T>
{
internal override ClassType ClassType => ClassType.Dictionary;
internal sealed override ClassType ClassType => ClassType.Dictionary;
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 seal the overrides in JsonCollectionConverter and JsonObjectConverter as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 02ab96b

protected internal abstract bool OnWriteResume(Utf8JsonWriter writer, T dictionary, JsonSerializerOptions options, ref WriteStack state);
}
}
8 changes: 3 additions & 5 deletions ...raries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ public static partial class JsonSerializer
/// <param name="cancellationToken">The <see cref="System.Threading.CancellationToken"/> which may be used to cancel the write operation.</param>
public static Task SerializeAsync<TValue>(Stream utf8Json, TValue value, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default)
{
if (utf8Json == null)
Copy link
Member

@ahsonkhan ahsonkhan Feb 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Nice catch. We should update the docs with the new exceptions:
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializer.serializeasync?view=netcore-3.1#System_Text_Json_JsonSerializer_SerializeAsync_System_IO_Stream_System_Object_System_Type_System_Text_Json_JsonSerializerOptions_System_Threading_CancellationToken_

If you get the chance, please add the exceptions this (and other SerializeAsync overloads) throw, here (if you are up for it, this would need a PR in the docs repo):
https://github.com/dotnet/dotnet-api-docs/blob/1e6b2e6052185c6744054acc10cd06e435a4fb8b/xml/System.Text.Json/JsonSerializer.xml#L546-L563

See other overloads that have exception docs as an example:
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializer.serialize?view=netcore-3.1#System_Text_Json_JsonSerializer_Serialize__1_System_Text_Json_Utf8JsonWriter___0_System_Text_Json_JsonSerializerOptions_

cc @carlossanlop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, thanks for the pointers to the docs code and example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issue dotnet/dotnet-api-docs#3961

throw new ArgumentNullException(nameof(utf8Json));

return WriteAsyncCore(utf8Json, value, typeof(TValue), options, cancellationToken);
}

Expand Down Expand Up @@ -65,11 +68,6 @@ private static async Task WriteAsyncCore(Stream utf8Json, object? value, Type in
return;
}

if (inputType == null)
{
inputType = value.GetType();
}

WriteStack state = default;
state.InitializeRoot(inputType, options, supportContinuation: true);

Expand Down
4 changes: 3 additions & 1 deletion src/libraries/System.Text.Json/tests/Serialization/Stream.ReadTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ namespace System.Text.Json.Serialization.Tests
public static partial class StreamTests
{
[Fact]
public static async Task NullArgumentFail()
public static async Task ReadNullArgumentFail()
{
await Assert.ThrowsAsync<ArgumentNullException>(async () => await JsonSerializer.DeserializeAsync<string>((Stream)null));
await Assert.ThrowsAsync<ArgumentNullException>(async () => await JsonSerializer.DeserializeAsync((Stream)null, (Type)null));
Copy link
Member

Choose a reason for hiding this comment

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

While we are doing this, let's add all permutations. For example: have the stream parameter be null, but the type parameter be non-null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 7a6e0cb

await Assert.ThrowsAsync<ArgumentNullException>(async () => await JsonSerializer.DeserializeAsync((Stream)null, typeof(string)));
await Assert.ThrowsAsync<ArgumentNullException>(async () => await JsonSerializer.DeserializeAsync(new MemoryStream(), (Type)null));
}

Expand Down
7 changes: 7 additions & 0 deletions src/libraries/System.Text.Json/tests/Serialization/Stream.WriteTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ namespace System.Text.Json.Serialization.Tests
{
public static partial class StreamTests
{
[Fact]
public static async Task WriteNullArgumentFail()
{
await Assert.ThrowsAsync<ArgumentNullException>(async () => await JsonSerializer.SerializeAsync((Stream)null, 1));
await Assert.ThrowsAsync<ArgumentNullException>(async () => await JsonSerializer.SerializeAsync((Stream)null, 1, typeof(int)));
}

[Fact]
public static async Task VerifyValueFail()
{
Expand Down
76 changes: 76 additions & 0 deletions src/libraries/System.Text.Json/tests/Utf8JsonReaderTests.MultiSegment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,6 +899,82 @@ public static void TestSingleStringsMultiSegment()
}
}

[Fact]
public static void TestMultiSegmentStringConversionToDateTime()
{
string jsonString = "\"1997-07-16\"";
string expectedString = "1997-07-16";
int expectedTokenLength = 10;
DateTime expectedDateTime = DateTime.Parse(expectedString);

byte[] utf8 = Encoding.UTF8.GetBytes(jsonString);

ReadOnlySequence<byte> sequence = JsonTestHelper.CreateSegments(utf8);

for (int j = 0; j < utf8.Length; j++)
{
var utf8JsonReader = new Utf8JsonReader(sequence.Slice(0, j), isFinalBlock: false, default);
ReadDateTimeHelper(ref utf8JsonReader, expectedDateTime, expectedTokenLength);

Assert.Equal(0, utf8JsonReader.TokenStartIndex);

long consumed = utf8JsonReader.BytesConsumed;
utf8JsonReader = new Utf8JsonReader(sequence.Slice(consumed), isFinalBlock: true, utf8JsonReader.CurrentState);
ReadDateTimeHelper(ref utf8JsonReader, expectedDateTime, expectedTokenLength);
}
}

private static void ReadDateTimeHelper(ref Utf8JsonReader jsonReader, DateTime expectedValue, long expectedTokenLength)
{
while (jsonReader.Read())
{
if (jsonReader.TokenType == JsonTokenType.String)
{
long tokenLength = jsonReader.HasValueSequence ? jsonReader.ValueSequence.Length : jsonReader.ValueSpan.Length;
Assert.Equal(expectedTokenLength, tokenLength);
Assert.Equal(expectedValue, jsonReader.GetDateTime());
Copy link
Member

Choose a reason for hiding this comment

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

nit: We missed the asserts for TryGetDateTime and TryGetDateTimeOffset APIs.

}
}
}

[Fact]
public static void TestMultiSegmentStringConversionToDateTimeOffset()
{
string jsonString = "\"1997-07-16\"";
string expectedString = "1997-07-16";
int expectedTokenLength = 10;
DateTimeOffset expectedDateTimeOffset = DateTimeOffset.Parse(expectedString);

byte[] utf8 = Encoding.UTF8.GetBytes(jsonString);

ReadOnlySequence<byte> sequence = JsonTestHelper.CreateSegments(utf8);

for (int j = 0; j < utf8.Length; j++)
{
var utf8JsonReader = new Utf8JsonReader(sequence.Slice(0, j), isFinalBlock: false, default);
ReadDateTimeOffsetHelper(ref utf8JsonReader, expectedDateTimeOffset, expectedTokenLength);

Assert.Equal(0, utf8JsonReader.TokenStartIndex);

long consumed = utf8JsonReader.BytesConsumed;
utf8JsonReader = new Utf8JsonReader(sequence.Slice(consumed), isFinalBlock: true, utf8JsonReader.CurrentState);
ReadDateTimeOffsetHelper(ref utf8JsonReader, expectedDateTimeOffset, expectedTokenLength);
}
}

private static void ReadDateTimeOffsetHelper(ref Utf8JsonReader jsonReader, DateTimeOffset expectedValue, long expectedTokenLength)
{
while (jsonReader.Read())
{
if (jsonReader.TokenType == JsonTokenType.String)
{
long tokenLength = jsonReader.HasValueSequence ? jsonReader.ValueSequence.Length : jsonReader.ValueSpan.Length;
Assert.Equal(expectedTokenLength, tokenLength);
Assert.Equal(expectedValue, jsonReader.GetDateTimeOffset());
}
}
}

private static void SpanSequenceStatesAreEqualInvalidJson(byte[] dataUtf8, ReadOnlySequence<byte> sequence, int maxDepth, JsonCommentHandling commentHandling)
{
var stateSpan = new JsonReaderState(new JsonReaderOptions { CommentHandling = commentHandling, MaxDepth = maxDepth });
Expand Down