Tarmil / FSharp.SystemTextJson

System.Text.Json extensions for F# types
MIT License
323 stars 44 forks source link

Map serialization with non-string keys (e.g. Guid keys) fails #152

Closed cmeeren closed 10 months ago

cmeeren commented 1 year ago

System.Text.Json supports many key types when deserializing to, say, Dictionary<_, _>.

However, when deserializing to Map<_, _>, I get a weird exception.

Repro:

let opts = JsonSerializerOptions()
opts.Converters.Add(JsonFSharpConverter())

// These work
JsonSerializer.Deserialize<Dictionary<string, string>>("{}", opts) |> ignore
JsonSerializer.Deserialize<Dictionary<int, string>>("{}", opts) |> ignore
JsonSerializer.Deserialize<Map<string, string>>("{}", opts) |> ignore

// This doesn't
JsonSerializer.Deserialize<Map<int, string>>("{}", opts) |> ignore

Exception:

System.Text.Json.JsonException: Failed to parse type Microsoft.FSharp.Collections.FSharpMap`2[[System.Int32, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]: expected JSON array, found StartObject
   at System.Text.Json.Serialization.Helpers.failf@12.Invoke(String x)
   at System.Text.Json.Serialization.Helpers.failExpecting[a](String expected, Utf8JsonReader& reader, Type ty)
   at System.Text.Json.Serialization.Helpers.expectAlreadyRead(JsonTokenType expectedTokenType, String expectedLabel, Utf8JsonReader& reader, Type ty)
   at System.Text.Json.Serialization.JsonMapConverter`2.Read(Utf8JsonReader& reader, Type _typeToConvert, JsonSerializerOptions options)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 utf8Json, JsonTypeInfo jsonTypeInfo, Nullable`1 actualByteCount)
   at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 json, JsonTypeInfo jsonTypeInfo)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
Tarmil commented 1 year ago

The error is not very clear indeed. What happens is that for all non-string key types, FSharp.SystemTextJson doesn't serialize as an object, but as an array with [key,value] items. So this will work:

JsonSerializer.Deserialize<Map<int, string>>("[]", opts) |> ignore

I am open to opt-in improvements in this area. A related issue is #67.

cmeeren commented 1 year ago

for all non-string key types, FSharp.SystemTextJson doesn't serialize as an object, but as an array with [key,value] items.

Out of interest, given that STJ supports many non-string key types, what is the rationale behind this behavior? Is it due to historical behavior that is kept due to backward compatibility, or are there good reasons to do this now, even for the key types supported by STJ? I'm asking because I find it surprising that the behavior for Map is different than the behavior for Dictionary, since they are the same type of thing. Additionally, I find it surprising that Map serializes differently when changing the key type, especially from string to a type that is still serialized as s JSON string anyway, such as Guid. I could easily see myself (in some circumstances) making that change without even testing it and expecting that it still serializes the same (i.e., as an object) with Guid keys as with string keys.

Tarmil commented 1 year ago

The discrepancy is mostly for historical reasons: at the time, STJ didn't support non-string keys at all. Support for non-string types as keys was added in .NET 5. And in a way, it is still partial: it only supports "primitive" types like integers and Guid.

I wanted to support all key types, as my priority at the time was to provide full round-trip for Bolero remoting, so I chose the array-of-arrays format. An alternative would be to do like FSharpLu.Json, which just serializes the F# key and puts that as the JSON key, but that quickly becomes an ugly mess of backslash escapes if your key type is in any way complex:

open Microsoft.FSharpLu.Json

Map [ ({| x = "1" |}, 2) ] |> Compact.serialize
// returns:
//   { "{ x = \"1\" }": 2 }
cmeeren commented 1 year ago

Thanks for the explanation.

I think it would be great to have something like .UseSystemTextJsonKeySerialization() (some alternative names are UseDefaultKeySerialization, UseStandardKeySerialization, and UsePrimitiveKeySerialization) in the options builder, which would remove the FSharp.SystemTextJson serialization logic for non-string key types and rely on System.Text.Json's built-in behavior. Hopefully that won't be too big of a change.

Due to my previous explanation of how surprising I find the current behavior, I also suggest making it the default in the next major version, though that is a separate consideration from supporting it in the first place.

Unfortunately I am unable to help out, due to being swamped both at work and at home for the moment. I have already worked around this, so it's no longer urgent for me.

MaybeSacred commented 1 year ago

Note that the behavior is based on the type; all you have to do is change the Property: Map<..., ...> to Property: IDictionary<..., ...> to get the expected behavior since Map implements IDictionary

Tarmil commented 10 months ago

This was added as .WithMapFormat(MapFormat.Object) in v1.2.