(De)serializing stacks with JsonSerializer should round-trip

[layomia]

The serializer currently doesn't roundtrip i.e serialize -> deserialize -> serialize does not result in the same state.

More discussion:

• https://github.com/dotnet/corefx/issues/40479#issuecomment-523586917
• Populate non-immutable collections directly on deserialize corefx#41482 (comment)

[layomia]

We'll leave the implementation in 3.0 and 3.1, where the stack contents are reversed on serialize, as is. This applies to generic stacks (Stack<T>, ConcurrentStack<T>), non-generic stacks (Stack), types that inherit from them, and ImmutableStack<T>.

In 5.0, we'll possibly make a breaking change where the stack contents are not reversed on serialize by default, which is the expected behavior, and ensures round-tripping. In this case, there'll be an option to instead use the behavior in 3.0/3.1.

[dbc2]

I am writing a JsonConverter<Stack<T>> that round-trips Stack<T> without reversing its order. See: this answer to How can I serialize a Stack to JSON using System.Text.Json without reversing the stack?:

public class StackConverter<TItem> : StackConverter<Stack<TItem>, TItem>
{
}

public class StackConverter<TStack, TItem> : JsonConverter<TStack> where TStack : Stack<TItem>, new()
{
    public override TStack Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        var list = JsonSerializer.Deserialize<List<TItem>>(ref reader, options);
        if (list == null)
            return null;
        var stack = typeToConvert == typeof(Stack<TItem>) ? (TStack)new Stack<TItem>(list.Count) : new TStack();
        for (int i = list.Count - 1; i >= 0; i--)
            stack.Push(list[i]);
        return stack;
    }

    public override void Write(Utf8JsonWriter writer, TStack value, JsonSerializerOptions options)
    {
        writer.WriteStartArray();
        foreach (var item in value)
            JsonSerializer.Serialize(writer, item, options);
        writer.WriteEndArray();
    }
}

My current version fixes the stack order on deserialization. If I want this to be compatible with 5.0 do I need to modify the code to reverse the items during serialization instead?

[ahsonkhan]

From @dbc2 in #1316:

If I have a Stack<T> for some T, and round-trip it to JSON using the new System.Text.Json.JsonSerializer, the order of the items in the stack will be reversed after deserialization.

Details as follows. I have a Stack<int> and push 3 values 1, 2, 3 onto it. I then serialize it to JSON using JsonSerializer, which results in

[3,2,1]

However, when I deserialize the JSON to a new stack, the integers in the stack are reversed, and a later assert that the stacks are sequentially equals fails:

var stack = new Stack<int>(new [] { 1, 2, 3 });

var json = JsonSerializer.Serialize(stack);

var stack2 = JsonSerializer.Deserialize<Stack<int>>(json);

var json2 = JsonSerializer.Serialize(stack2);

Console.WriteLine("Serialized {0}:", stack);
Console.WriteLine(json); // Prints [3,2,1]

Console.WriteLine("Round-tripped {0}:", stack);
Console.WriteLine(json2); // Prints [1,2,3]

Assert.IsTrue(stack.SequenceEqual(stack2)); // Fails
Assert.IsTrue(json == json2);               // Also fails

Demo fiddle here.

Some investigation shows that there is some code in CreateDerivedEnumerableInstance(ref ReadStack state, JsonPropertyInfo collectionPropertyInfo, IList sourceList) to create a stack from a deserialized list:

else if (instance is Stack<TDeclaredProperty> instanceOfStack)
{
    foreach (TDeclaredProperty item in sourceList)
    {
        instanceOfStack.Push(item);
    }

    return instanceOfStack;
}

However, it pushes them on in the wrong order, it needs to push them in reverse:

else if (instance is Stack<TDeclaredProperty> instanceOfStack)
{
    for (int i = sourceList.Count - 1; i >= 0; i--)
    {
        instanceOfStack.Push((TDeclaredProperty)sourceList[i]);
    }
    return instanceOfStack;
}

Since there is some code that attempts to correctly deserialize Stack<T> I believe this is an actual bug not an enhancement.

Workaround is to create a custom JsonConverter<Stack<T>> as shown in this answer to How can I serialize a Stack to JSON using System.Text.Json without reversing the stack?.

Incidentally, the code added to handle Stack<T>, while present in https://github.com/dotnet/corefx/blob/v3.1.0/src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoCommon.cs, seems not to be present in https://github.com/dotnet/runtime/blob/master/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoCommon.cs. So I don't know what would happen in the current development version.

[layomia]

So I don't know what would happen in the current development version.

@dbc2 - the behavior for master/5.0 is (currently) the same as 3.x.

[dbc]

@layomia please check your @ messages. The Real dbc does not work on this code.

[layomia]

We shouldn't do this. There's no standard on which side to reverse the items (serialization or deserialization) in order to roundtrip, so it is a non-starter as a breaking change candidate. The current behavior is compatible with Newtonsoft.Json behavior.

There's a work item to provide a sample converter showing how to roundtrip in the JSON docs which I think should suffice as a resolution for this issue: dotnet/docs#16690. Here's what this converter could look like - dotnet/docs#16225 (comment).