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

User Story: Developers using JsonSerializer want it to handle common F# types well #29812

Closed
terrajobst opened this issue Jun 7, 2019 · 24 comments · Fixed by #55108
Closed
Assignees
Labels
area-System.Text.Json Cost:S Work that requires one engineer up to 1 week json-functionality-doc Missing JSON specific functionality that needs documenting Priority:1 Work that is critical for the release, but we could probably ship without tracking This issue is tracking the completion of other related issues. User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@terrajobst
Copy link
Member

We should make sure that the JSON serializer can handle types / structures that commonly occur in F#, such as

  • Discriminated unions
  • Option types

/cc @steveharter @ahsonkhan

@terrajobst terrajobst changed the title Ensure JSON serializer can handle common F# types Ensure JSON serializer/deserializer can handle common F# types Jun 7, 2019
@cartermp
Copy link

cartermp commented Jun 7, 2019

Should be:

  • Discriminated Unions
  • Record types
  • Anonymous Record types (should be identical but we emit some slight special names)

@ahsonkhan
Copy link
Member

This requires careful design to make sure it is handled correctly to meet consumer expectations. We should flesh out the requirements of what the behavior should be, especially for cases like round-tripping, etc.

@bartelink
Copy link

bartelink commented Jun 14, 2019

Suggest mining https://github.com/jet/Jet.JsonNet.Converters and https://github.com/microsoft/fsharplu for commonly shimmed features - will be monitoring this issue as I would love rebase the test codebase for the former on top of this and/or dogfood it here. Related issue: https://github.com/jet/equinox/issues/79

@AArnott
Copy link
Contributor

AArnott commented Jul 30, 2019

Would the discriminated unions support be helpful in non F# cases as well? I'm particularly interested in cases of deserializing protocols such as JSON-RPC, where the incoming message type may be any of a few schemas and we need to distinguish them based on a couple of properties.

@bartelink
Copy link

Good question @AArnott - supporting management of a catchall case would also ideally be possible.
To call out one specific need - the json.net intrinsic F# support emits the fields as a sub-object alongside the case name; Jet.JsonNet.Converters and many common converters including Java ones can and do operate by representing the object as a single object node [including any marker/discriminator fields].

@Tarmil
Copy link

Tarmil commented Aug 8, 2019

While this is being decided, I've implemented an F# serializer as a separate library: https://github.com/Tarmil/FSharp.SystemTextJson

@hravnx
Copy link

hravnx commented Sep 12, 2019

While I understand that the F# specific types like DUs and Records are not supported yet, I've run into strange behaviour serializing a simple Map.

Given the following F# program (running in .Net Core 3 preview 9):

open System
open System.Text.Json


[<EntryPoint>]
let main _argv =

    let m1 = Map.empty<string, string> |> Map.add "answer" "42"
    let json1 = JsonSerializer.Serialize(m1)
    printfn "JSON1 %s" json1
    
    let m2 = Map.empty<string, Version> |> Map.add "version" (Version("10.0.1.1"))
    let json2 = JsonSerializer.Serialize(m2)
    printfn "JSON2 %s" json2

    0 

I expect the following output:

JSON1 {"answer":"42"}
JSON2 {"version":"Major":10,"Minor":0,"Build":1,"Revision":1,"MajorRevision":0,"MinorRevision":1}}

But instead I get this:

JSON1 {"answer":"42"}
Unhandled exception. System.InvalidCastException: Unable to cast object of type 'mkIEnumerator@431[System.String,System.Version]' to type 'System.Collections.IDictionaryEnumerator'.
 ...

So the first case worked, but not the second. Is this a known limitation or a bug?

@xperiandri
Copy link

@bartelink
Copy link

bartelink commented Sep 12, 2019

Version requires a converter even for for Json.net; suggest using a simple C# defined type to validate (I believe records don't work so be careful not to try that). Then extend to using non-record syntax in F# to define equivalent of the C# class etc. - going straight to a Map of versions is trying to do too many things at once. Also can we not pollute this thread with specific cases please - i.e. can this be moved to another issue? Another thing to simplify is to use Dictionary or |> dict in lieu of Map (thought the first case pretty much proves Map is fine, as one might expect as it implements IEnumerable)

@hravnx
Copy link

hravnx commented Sep 12, 2019

Map<string, string[]> is also broken. Map<string, Uri> works. But I'll stop "polluting" now. Bye.

@ScottHutchinson
Copy link

Here is an example where the JsonSerializer.Deserialize function fails to properly deserialize the value of an enum option. The value is correctly serialized as 1 (B), but is deserialized as A. The Newtonsoft.Json.JsonConvert.DeserializeObject function works properly (using the commented-out code). Also, Newtonsoft.Json does not require the [<CLIMutable>] attribute, but JsonSerializer does.

open System.IO

type TestType =
    | A = 0
    | B = 1

[<CLIMutable>]
type TestObjectB =
     {
         test : TestType option
     }

//let jsonSerializeToString obj = 
//    use writer = new StringWriter()
//    let ser =  new Newtonsoft.Json.JsonSerializer()
//    ser.Formatting <- Newtonsoft.Json.Formatting.Indented
//    ser.Serialize(writer, obj)
//    writer.ToString()

//let jsonDeserializeFromString str =
//    Newtonsoft.Json.JsonConvert.DeserializeObject<TestObjectB>(str)

open System.Text.Json

let jsonSerializeToString obj = 
    let mutable options = JsonSerializerOptions()
    options.WriteIndented <- true
    JsonSerializer.Serialize(obj, options)

let jsonDeserializeFromString (str: string) =
    JsonSerializer.Deserialize<TestObjectB>(str)

let Test obj = 
    let str = jsonSerializeToString obj
    let obj' = jsonDeserializeFromString str
    obj'

[<EntryPoint>]
let main argv =
    { test = Some TestType.B } |> Test |> ignore
    { test = None } |> Test |> ignore
    0

@ScottHutchinson
Copy link

^^
C:\Users\scott>dotnet --info
.NET Core SDK (reflecting any global.json):
Version: 3.0.100
Commit: 04339c3a26

Runtime Environment:
OS Name: Windows
OS Version: 10.0.18362
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\3.0.100\

Host (useful for support):
Version: 3.0.0
Commit: 7d57652f33

@cartermp
Copy link

cartermp commented Oct 7, 2020

Just as a quick update, various things do work.

This example now works in .NET 5:

open System
open System.Text.Json


[<EntryPoint>]
let main _argv =

    let m1 = Map.empty<string, string> |> Map.add "answer" "42"
    let json1 = JsonSerializer.Serialize(m1)
    printfn "JSON1 %s" json1
    
    let m2 = Map.empty<string, Version> |> Map.add "version" (Version("10.0.1.1"))
    let json2 = JsonSerializer.Serialize(m2)
    printfn "JSON2 %s" json2

    0 

F# records and anonymous records now appear to work (nested records example):

open System
open System.Text.Json

type Person = { Name: string; Age: int }
type Gaggle = { People: Person list; Name: string }

[<EntryPoint>]
let main _argv =

    let r1 =
        {
            People =
                [ { Name = "Phillip"; Age = Int32.MaxValue }
                  { Name = "Ryan Nowak"; Age = Int32.MinValue } ]
            Name = "Meme team"
        }

    let r2 =
        {|
            r1 with
                FavoriteMusic = "Dan Roth's Banjo Bonanza"
        |}

    printfn $"{JsonSerializer.Serialize r1}"
    printfn $"{JsonSerializer.Serialize r2}"

    0 

Yields:

{"People":[{"Name":"Phillip","Age":2147483647},{"Name":"Ryan Nowak","Age":-2147483648}],"Name":"Meme team"}
{"FavoriteMusic":"Dan Roth\u0027s Banjo Bonanza","Name":"Meme team","People":[{"Name":"Phillip","Age":2147483647},{"Name":"Ryan Nowak","Age":-2147483648}]}

But DUs are still missing. FSharp.SystemTextJson is still the way to serialize F# union types.

@cartermp
Copy link

cartermp commented Oct 7, 2020

@terrajobst @layomia happy to chat about what all would be the ideal format and what kind of data needs to be dealt with

@layomia
Copy link
Contributor

layomia commented Oct 20, 2020

@cartermp, thanks - I'll reach out to discuss.

@terrajobst terrajobst added Cost:S Work that requires one engineer up to 1 week User Story A single user-facing feature. Can be grouped under an epic. labels Nov 25, 2020
@marek-safar marek-safar added the tracking This issue is tracking the completion of other related issues. label Nov 27, 2020
@marek-safar marek-safar removed the User Story A single user-facing feature. Can be grouped under an epic. label Nov 27, 2020
@lambdakris
Copy link

Huh, I'm finding that I can't even deserialize lists of records, even with the CLIMutable attribute. This is with .NET 5.0.101.

The following snippet...

[<CLIMutable>]
type MenuItem = {
    FoodType: string
    Name: string
    Price: decimal
}

[<CLIMutable>]
type Menu = {
    Restaurant: string
    MenuItems: MenuItem list
}

let menu = JsonSerializer.Deserialize<Menu>(json)

throws...

Unhandled exception. System.NotSupportedException: The collection type 'Microsoft.FSharp.Collections.FSharpList`1[Program+MenuItem]' is abstract, an interface, or is read only, and could not be instantiated and populated. Path: $.MenuItems | LineNumber: 2 | BytePositionInLine: 14.
 ---> System.NotSupportedException: The collection type 'Microsoft.FSharp.Collections.FSharpList`1[Program+MenuItem]' is abstract, an interface, or is read only, and could not be instantiated and populated.
   --- End of inner exception stack trace ---
   at System.Text.Json.ThrowHelper.ThrowNotSupportedException(ReadStack& state, Utf8JsonReader& reader, NotSupportedException ex)
   at System.Text.Json.ThrowHelper.ThrowNotSupportedException_CannotPopulateCollection(Type type, Utf8JsonReader& reader, ReadStack& state)
   at System.Text.Json.Serialization.Converters.IEnumerableOfTConverter`2.CreateCollection(Utf8JsonReader& reader, ReadStack& state, JsonSerializerOptions options)
   at System.Text.Json.Serialization.Converters.IEnumerableDefaultConverter`2.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, TCollection& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.JsonPropertyInfo`1.ReadJsonAndSetMember(Object obj, ReadStack& state, Utf8JsonReader& reader)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   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.ReadCore[TValue](JsonConverter jsonConverter, Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
   at System.Text.Json.JsonSerializer.ReadCore[TValue](Utf8JsonReader& reader, Type returnType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, Type returnType, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
   at Program.requestMenu@71-1.Invoke(String _arg1) in C:\Users\cax9124\Code\Hobby\OloMenu\Program.fs:line 71
   at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvokeNoHijackCheck[a,b](AsyncActivation`1 ctxt, FSharpFunc`2 userCode, b result1) in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 404
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 104
--- End of stack trace from previous location ---
   at Microsoft.FSharp.Control.AsyncResult`1.Commit() in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 337
   at Microsoft.FSharp.Control.AsyncPrimitives.RunSynchronouslyInCurrentThread[a](CancellationToken cancellationToken, FSharpAsync`1 computation) in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 870
   at Microsoft.FSharp.Control.AsyncPrimitives.RunSynchronously[T](CancellationToken cancellationToken, FSharpAsync`1 computation, FSharpOption`1 timeout) in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 878
   at Microsoft.FSharp.Control.FSharpAsync.RunSynchronously[T](FSharpAsync`1 computation, FSharpOption`1 timeout, FSharpOption`1 cancellationToken) in F:\workspace\_work\1\s\src\fsharp\FSharp.Core\async.fs:line 1142
   at Program.main(String[] argv) in C:\Users\cax9124\Code\Hobby\OloMenu\Program.fs:line 77

@bartelink
Copy link

@lambdakris For now, list doesn't work, records do. You can use arrays or seq (internally, lists are Unions, which are not supported; also the fact that there's at least an allocation per node involved in representing it that way makes it a debatable to have out-of-the-box handling be added)

@layomia layomia changed the title Ensure JSON serializer/deserializer can handle common F# types User Story: Developers using JsonSerializer want it to handle common F# types well Jan 21, 2021
@layomia layomia added the User Story A single user-facing feature. Can be grouped under an epic. label Jan 21, 2021
@ericstj ericstj added the Priority:1 Work that is critical for the release, but we could probably ship without label Jan 22, 2021
@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 6.0.0 Feb 25, 2021
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented May 12, 2021

Should be:

  • Discriminated Unions
  • Record types
  • Anonymous Record types (should be identical but we emit some slight special names)

I would add the following types to the list:

  • Tuples (regular and struct tuples, serialized as JSON arrays).
  • F# lists and optional support.
  • F# Set and Map (NB deserialization is currently broken for both types).

Note that

  1. F# records (anonymous or not) serialization works OOTB in .NET 5.
  2. Tuple support will also impact C# users, since F# uses the same shared framework tuple types.
  3. Discriminated union support will likely require exposing a special attribute for customizing type discriminators.

@eiriktsarpalis eiriktsarpalis self-assigned this May 12, 2021
@xperiandri
Copy link

xperiandri commented May 12, 2021

Use https://github.com/Tarmil/FSharp.SystemTextJson/ as a reference. It covers almost all cases. Maybe not in the most perfect or easy-to-understand way...

@eiriktsarpalis
Copy link
Member

Concerning support for discriminated unions, it might be worth pointing out that there is no one canonical way to encode DUs in JSON. DUs are used in many different ways in F# code, including the following:

  • Single case DUs, e.g. type Email = Email of string, used to provide a type-safe wrapper for common values. A user might expect that Email("email") should serialize as its payload, "email".
  • Type-safe enums, e.g. type Suit = Heart | Spade | Diamond | Club. Users might reasonably expect that Spade should serialize as the union case identifier, "Spade".
  • Unions with arbitrary combinations of union cases and arities, e.g. type Shape = Point | Circle of radius:float | Rectangle of width:float * length:float. A value like Circle(42.) would require an encoding similar to { "shape" : "circle", "radius" : 42 }. Users should be able to specify the case discriminator property name ("shape") as well as the identifier for the union case (Circle mapping to "circle").

In light of the above, I'm increasingly starting to think that System.Text.Json should not be providing a default mechanism for serializing DUs. Users would still be able to use available custom converters that provide the union encoding that suits their use case or perhaps use unions in their data contracts in a way that bypasses the serialization layer altogether.

@deyanp
Copy link

deyanp commented Jul 12, 2021

@eiriktsarpalis we (and I think everybody else) use only option 2 (type-safe enums) at the edge ... and I think a performance-optimized (runtime reflection!) impl does 100% belong into System.Text.Json ...

Currently we are forced to use standard enums ... we tried implementing simple DU serialization/deserialization incl. caching of the values ... but there was sth. suboptimal (need to remember) ...

@bartelink
Copy link

@deyanp as an aside, tagging the typesafe enum type with this converter should work in the interim

I would of course also like to see an in the box professional impl of it of course - nothing would give me more pleasure than removing it from FsCodec (although the various conditional parsing helpers from the module TypeSafeEnum are not without merit)

As the FsCodec impl shows though, achieving a proper impl of that seemingly simple task is no entirely trivial even before you consider perf and has significant overlap with the work of doing a UnionConverter capable of handing stateful cases. Based on that I'd prefer to see any implementation work consider the broader set of use cases as Eirik has outlined; I definitely agree that any global default behavior is asking for trouble.

@jhf
Copy link

jhf commented Jul 15, 2021

Why not go for simple, but non-optimal translation, where a discriminated union is treated as a record where only one attribute is present, and where null is placed where no arguments are allowed for the constructor.
This would make it work, and for those who want a particular format, they can override it.

  • Single case DUs, ie. type Email = Email of string => {email="..."}
  • Type-safe enums, ie. type Suit = Heart | Spade | Diamond | Club => {spade=null}
  • Unions with arbitrary combinations of union cases and arities, ie. type Shape = Point | Circle of radius:float | Rectangle of width:float * length:float => {circle={radius=...}}

This would not be the most compact representation, but it would always work, as far as I can tell.

@bartelink
Copy link

bartelink commented Jul 15, 2021

@jhf see the linked issue for other proposals (and me attempting to explain why IMO you want to have a clean way to migrate all the way from nullary cases -> SCDU -> other arities of tuples -> records)

In my proposal, the renderings would be:

If using TypeSafeEnumConverter (which is basically a type safe StringEnumConverter), you'd opt into rendering and/or throwing if not mappable:

  • [<TypeSafeEnumConverter>]type Suit = Heart | Spade | Diamond | Club => "Spade"

The UnionConverter would enable:

  • [<UnionConverter>]type Suit = Heart | Spade | Diamond | Club => {"case": "spade"}
  • [<UnionConverter>]type Email = Email of addr: string => {"case": "email", "addr": "..."}
  • [<UnionConverter>]type Shape = Point | Circle of radius:float | Rectangle of width:float * length:float => {"case": "circle", "radius": ....}

(As mentioned in the other post, the "case" would be customizable via attributes, and unnamed Items in tuples would not be supported)

That leaves what do to with SCDUs to force them to be represented as their body without
a) turning them into a JSON object
b) having a [redundant] "case" field

I personally tend to use https://github.com/fsprojects/FSharp.UMX to side-step this (see also this very thorough blog article by @pblasucci)

If a pushed, my suggestion for a way to handle this, would be to have an opt-in converter could cover this case - [<InlineConverter>]type Email = Email of string => "a@example.org"

(While one could arguably offer that as a special case option on UnionConverter, I think that's fraught with problems - having a single canonical rendering per converter is pretty critical IMO)

@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json Cost:S Work that requires one engineer up to 1 week json-functionality-doc Missing JSON specific functionality that needs documenting Priority:1 Work that is critical for the release, but we could probably ship without tracking This issue is tracking the completion of other related issues. User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Development

Successfully merging a pull request may close this issue.