-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Additional tests for System.Text.Json #32705
Changes from 5 commits
4b5f5c2
a4c5847
a8374d9
8090c69
1e8e33c
7a6e0cb
823f147
256de92
02ab96b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should seal the overrides in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch. We should update the docs with the new exceptions: 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): See other overloads that have exception docs as an example: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do, thanks for the pointers to the docs code and example. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
||
|
@@ -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); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,9 +12,10 @@ 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in 7a6e0cb |
||
await Assert.ThrowsAsync<ArgumentNullException>(async () => await JsonSerializer.DeserializeAsync(new MemoryStream(), (Type)null)); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -899,6 +899,60 @@ public static void TestSingleStringsMultiSegment() | |
} | ||
} | ||
|
||
[Fact] | ||
public static void TestMultiSegmentStringConversionToDateTime() | ||
{ | ||
string jsonString = "\"1997-07-16\""; | ||
string expectedString = "1997-07-16"; | ||
|
||
byte[] dataUtf8 = Encoding.UTF8.GetBytes(jsonString); | ||
|
||
ReadOnlySequence<byte> sequence = JsonTestHelper.CreateSegments(dataUtf8); | ||
|
||
var json = new Utf8JsonReader(sequence, isFinalBlock: false, state: default); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Consider looping through the entire sequence and creating slices of arbitrary lengths (and re-entering the reader with "partial data"). See other "single value" tests like: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tried something out with 823f147, let me know if that doesn't cover what you were thinking. |
||
while (json.Read()) | ||
{ | ||
if (json.TokenType == JsonTokenType.String) | ||
{ | ||
DateTime expected = DateTime.Parse(expectedString); | ||
|
||
Assert.True(json.TryGetDateTime(out DateTime actual)); | ||
Assert.Equal(expected, actual); | ||
|
||
Assert.Equal(expected, json.GetDateTime()); | ||
} | ||
} | ||
|
||
Assert.Equal(dataUtf8.Length, json.BytesConsumed); | ||
} | ||
|
||
[Fact] | ||
public static void TestMultiSegmentStringConversionToDateTimeOffset() | ||
{ | ||
string jsonString = "\"1997-07-16\""; | ||
string expectedString = "1997-07-16"; | ||
|
||
byte[] dataUtf8 = Encoding.UTF8.GetBytes(jsonString); | ||
|
||
ReadOnlySequence<byte> sequence = JsonTestHelper.CreateSegments(dataUtf8); | ||
|
||
var json = new Utf8JsonReader(sequence, isFinalBlock: false, state: default); | ||
while (json.Read()) | ||
{ | ||
if (json.TokenType == JsonTokenType.String) | ||
{ | ||
DateTimeOffset expected = DateTimeOffset.Parse(expectedString); | ||
|
||
Assert.True(json.TryGetDateTimeOffset(out DateTimeOffset actual)); | ||
Assert.Equal(expected, actual); | ||
|
||
Assert.Equal(expected, json.GetDateTimeOffset()); | ||
} | ||
} | ||
|
||
Assert.Equal(dataUtf8.Length, json.BytesConsumed); | ||
} | ||
|
||
private static void SpanSequenceStatesAreEqualInvalidJson(byte[] dataUtf8, ReadOnlySequence<byte> sequence, int maxDepth, JsonCommentHandling commentHandling) | ||
{ | ||
var stateSpan = new JsonReaderState(new JsonReaderOptions { CommentHandling = commentHandling, MaxDepth = maxDepth }); | ||
|
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 markedinternal
, 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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted in 256de92