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

Support resumable serialization in NullableConverter<T> #65524

Conversation

eiriktsarpalis
Copy link
Member

Makes NullableConverter support resumable serialization, adapting an approach followed by the existing FSharpOptionConverter.

Fix #65522.

@ghost
Copy link

ghost commented Feb 17, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

Makes NullableConverter support resumable serialization, adapting an approach followed by the existing FSharpOptionConverter.

Fix #65522.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Text.Json

Milestone: 7.0.0

@@ -57,7 +57,7 @@ protected static JsonConverter<TElement> GetElementConverter(JsonTypeInfo elemen

protected static JsonConverter<TElement> GetElementConverter(ref WriteStack state)
{
JsonConverter<TElement> converter = (JsonConverter<TElement>)state.Current.DeclaredJsonPropertyInfo!.ConverterBase;
JsonConverter<TElement> converter = (JsonConverter<TElement>)state.Current.JsonPropertyInfo!.ConverterBase;
Copy link
Member Author

@eiriktsarpalis eiriktsarpalis Feb 17, 2022

Choose a reason for hiding this comment

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

This renames a property I should have renamed in #65224 and matches the naming convention by the equivalent property in ReadStackFrame.

@@ -227,7 +227,7 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
JsonTypeInfo originalJsonTypeInfo = state.Current.JsonTypeInfo;
#endif
state.Push();
Debug.Assert(TypeToConvert.IsAssignableFrom(state.Current.JsonTypeInfo.Type));
Debug.Assert(TypeToConvert == state.Current.JsonTypeInfo.Type);
Copy link
Member Author

Choose a reason for hiding this comment

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

Strengthens an assertion that should have been changed via #65224.

Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

LGTM with suggestion

@eiriktsarpalis eiriktsarpalis merged commit fb69200 into dotnet:main Feb 18, 2022
@eiriktsarpalis eiriktsarpalis deleted the nullableconverter-flow-serialization-state branch February 18, 2022 10:48
namespace System.Text.Json.Serialization.Converters
{
internal sealed class NullableConverter<T> : JsonConverter<T?> where T : struct
{
internal override ConverterStrategy ConverterStrategy { get; }
Copy link
Member

Choose a reason for hiding this comment

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

I assume the ConverterStrategy is determined by the value held by Nullable? I.e. it can be Value, Object, Collection.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right.

@@ -74,6 +74,29 @@ public async Task WriteNestedAsyncEnumerable_DTO<TElement>(IEnumerable<TElement>
Assert.Equal(1, asyncEnumerable.TotalDisposedEnumerators);
}

[Theory]
[MemberData(nameof(GetAsyncEnumerableSources))]
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a non-IAsyncEnumerable test added that can verify a larger, nullable POCO (as a value type)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nullable POCOs still work, however they don't flow the state. This becomes more evident in the case of IAE which simply fails serialization.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullableConverter does not flow serialization state
3 participants