Thoth.Json icon indicating copy to clipboard operation
Thoth.Json copied to clipboard

Decoding Map<string,string> inside a record inside an erased discriminated union seems to produce an incorrect type - standard F# Map functions won't work on it

Open rmunn opened this issue 6 years ago • 13 comments

I have an API which includes a Map<string,string> field:

    type ProjectDetails = {
        identifier : string
        name : string
        description : string
        membership : Map<string,string>  // Keys are usernames, values are roles
    }

The membership field contains usernames and their corresponding roles in the project. E.g., this is what one project looks like going over the wire: {"identifier":"test-project","name": "Test Project","description":"Sample project for testing","membership":{"rmunn":"Manager","MangelMaxime":"Contributer"}}

When I decode that JSON into a ProjectDetails object on the Fable side of things, and log that to the console with console.log, I get {"rmunn":"Manager","MangelMaxime":"Contributer"}, so it's clearly not undefined. But when, later in my code, I do if project.membership |> Map.isEmpty then ..., I get an error in the Javascript console:

Uncaught TypeError: Cannot read property 'tag' of undefined
    at MapTreeModule$$$isEmpty (Map.js?1809:66)
    ...

Following the Map.js?1809:66 link takes me to the Map.js file of fable-library 2.4.11, which AFAICT is working properly. That line is part of the following compiled Javascript code:

export function MapTreeModule$$$isEmpty(m$$1) {
  if (m$$1.tag === 0) {
    return true;
  } else {
    return false;
  }
}

The m$$1.tag is causing the "Cannot read property 'tag' of undefined" error; in other words, isEmpty is being called with an undefined parameter. But just prior to that call, I have a console.log(project.membership) call, which is printing out something that looks a lot like a proper Javascript object (which is what I imagine an F# Map would convert to):

{"rmunn":"Manager","MangelMaxime":"Contributer"}

So the project.membership value is clearly not undefined here. I'm also getting very similar behavior (standard F# Map-module functions failing) with other Map functions such as Map.toSeq and so on; I won't create a separate issue for those because I think they all come from the same root cause. (And I've experienced very similar bugs with lists as well: when Thoth.Json auto-converts a string list, the standard F# List module functions fail; again, I won't create a separate issue for that one unless you ask me to, because I suspect that fixing the Map bug will fix the List bug as well).

BTW, the reason I'm submitting this here instead of as a bug against the Fable library is because I'm pretty confident that it's something in Thoth.Json that's causing this behavior: if, right before the offending Map.isEmpty call, I set the project membership to a hard-coded value with:

    let membership = Map.ofList ["rmunn", "Manager"; "MangelMaxime", "Contributer"]

then there are no errors and my page works as designed. But when the Map<string,string> instance is created by Thoth.Json decoding it (via an auto-decoder for the ProjectDetails type), I get all sorts of errors when I try to use functions from the standard Map module.

rmunn avatar Dec 10 '19 15:12 rmunn

Here's some further data to help track this down. I wrote the following code to see if there were any differences between the types created by Thoth.Json and the "standard" F# types as Fable translates them to Javascript:

let membershipViewInline (membership : Map<string,string>) =
    console.log("JSON-decoded membership:", membership, "of type", membership.GetType())
    let membership = Map.ofList ["rmunn", "Manager"; "foo", "Contributer"]
    console.log("Manually created membership:", membership, "of type", membership.GetType())

The function parameter comes from Thoth.Json decoding an API call with a membership: {username: role, username: role, ...} field, like the one I wrote about in my issue description. Then after logging that parameter to the console, I replace it with a manually-constructed Map<string, string> object. When I ran that code, I got this on the console:

JSON-decoded membership:
▼{manager1: "Manager", test: "Contributer", user1: "Contributer"}
 manager1: "Manager"
 test: "Contributer"
 user1: "Contributer"
▶__proto__: Object
of type
TypeInfo {fullname: "Microsoft.FSharp.Collections.FSharpMap`2", generics: Array(2), constructor: undefined, fields: undefined, cases: undefined, …}

Manually created membership:
▼Map_Map {comparer: {…}, tree: Map_MapTree}
▶comparer: {Compare: ƒ}
▶tree: Map_MapTree {tag: 2, name: "MapNode", fields: Array(5)}
 size: (...)
▶__proto__: SystemObject
of type
TypeInfo {fullname: "Microsoft.FSharp.Collections.FSharpMap`2", generics: Array(2), constructor: undefined, fields: undefined, cases: undefined, …}

Both of these had the same TypeInfo, but I'm particularly interested in the __proto__: the object coming from Thoth.Json had __proto__: Object, while the one coming from Fable's Map.ofList function had __proto: SystemObject. So however the Thoth.Json code is creating a Map<string,string>, it's apparently not following the same code path that Fable uses, and that's probably causing this issue somehow.

Library versions that might be relevant to debugging this:

  • Fable.Core: 3.1.3 (NuGet package)
  • fable-compiler: 2.4.11 (npm package)
  • Thoth.Json: 3.4.1 (NuGet package)
  • Thoth.Json.Net: 3.6 (NuGet package)

rmunn avatar Dec 11 '19 02:12 rmunn

I've created a minimal reproduction at https://github.com/rmunn/repro-json-parsing-failure — just clone the repo and run ./build.sh -t Run (or ./build.cmd -t Run if you're on Windows). Then open a developer console, click the big "Get Packages" button and watch the Javascript console. If it's successful, it should print two package versions to the console. If it fails, you should see an error "Unable to process the message: UserMsg PackagesLoaded [object Object] TypeError: Cannot read property 'tag' of undefined".

BTW, my repro targets .Net Core 3.0 to use the dotnet local tools, but if you have 2.2 installed, it should be pretty simple to retarget it to netcoreapp2.2 instead; you'll just have to run FAKE by hand instead of using my build.sh script.

rmunn avatar Dec 13 '19 08:12 rmunn

And here's the patch needed to make my repro succeed instead of failing:

diff --git a/src/Client/Client.fs b/src/Client/Client.fs
index e688b4a..0462bdb 100644
--- a/src/Client/Client.fs
+++ b/src/Client/Client.fs
@@ -26,10 +26,10 @@ type Msg =
     | Decrement
     | InitialCountLoaded of Counter
     | LoadPackages
-    | PackagesLoaded of JsonResult<Packages>
+    | PackagesLoaded of Packages
 
 let initialCounter () = Fetch.fetchAs<Counter> "/api/init"
-let loadPackages () = Fetch.fetchAs<JsonResult<Packages>> "/api/json"
+let loadPackages () = Fetch.fetchAs<Packages> "/api/json"
 
 // defines the initial state and initial command (= side-effect) of the application
 let init () : Model * Cmd<Msg> =
@@ -56,12 +56,11 @@ let update (msg : Msg) (currentModel : Model) : Model * Cmd<Msg> =
         let loadCmd =
             Cmd.OfPromise.perform loadPackages () PackagesLoaded
         currentModel, loadCmd
-    | _, PackagesLoaded jsonResult ->
-        unpackJsonResult currentModel jsonResult (fun packages ->
+    | _, PackagesLoaded packages ->
             console.log("Loaded packages: ", packages, " successfully")
             for pkg, ver in packages.devDependencies |> Map.toSeq do
                 console.log("Version of ", pkg, " is: ", ver)
-            currentModel, Cmd.none)
+            currentModel, Cmd.none
     | _ -> currentModel, Cmd.none
 
 
diff --git a/src/Server/Server.fs b/src/Server/Server.fs
index 51c1d01..7b5c9c6 100644
--- a/src/Server/Server.fs
+++ b/src/Server/Server.fs
@@ -38,7 +38,7 @@ let webApp = router {
                     "fable-compiler", "^2.4.6"
                     "fable-loader", "^2.1.8"
                 ]}
-            return! jsonSuccess packages next ctx
+            return! json packages next ctx
         })
 }
 

As you can see, the only difference between success and failure is that instead of sending a Packages data type via the API, I'm sending a JsonResult<Packages> data type. I'll create a new comment here to show you the relevant code so you don't have to dig through my repro to see it.

rmunn avatar Dec 13 '19 08:12 rmunn

Here's the relevant code for understanding the JsonResult type that I'm using:


```fsharp
// ***** Shared code in Shared.fs, imported by both client and server
type JsonSuccess<'a> = {
    ok : bool
    data : 'a
}

type JsonError = {
    ok : bool
    message : string
}



// ***** Client-side code in JsonHelpers.fs, loaded in Client.fs by "open JsonHelpers"
[<Erase>]
type JsonResult<'a> =
    | Success of JsonSuccess<'a>
    | Failure of JsonError



// ***** Server-side code in Server.fs:
let jsonError (msg : string) : HttpHandler = json { ok = false; message = msg }
let jsonSuccess data : HttpHandler = json { ok = true; data = data }
let jsonResult (result : Result<'a, string>) : HttpHandler =
    match result with
    | Ok data -> jsonSuccess data
    | Error msg -> jsonError msg

let webApp = router {
    get "/api/json" (fun next ctx ->
        task {
            let packages = {
                example = true
                devDependencies = Map.ofList [
                    "fable-compiler", "^2.4.6"
                    "fable-loader", "^2.1.8"
                ]}
            return! jsonSuccess packages next ctx
        })
}

let app = application {
    url ("http://0.0.0.0:" + port.ToString() + "/")
    use_router webApp
    memory_cache
    use_static publicPath
    use_json_serializer(Thoth.Json.Giraffe.ThothSerializer())
    use_gzip
}

As you can see, the JsonResult type isn't all that complex, but it's two or three levels deep of nesting: there's a DU wrapped around two different record types, and that nesting is probably where Thoth.Json is losing track of the Map type it should be creating and creating a standard JS object instead.

rmunn avatar Dec 13 '19 09:12 rmunn

Some things I'd check:

  • Is FetchAs actually using Thoth or just using an unsafe case from a plain JS object (which is probably what happens here because the internals changed between versions)
  • Are you using Thoth on the server side? You need to setup Giraffe accordingly.

Once you actually use Thoth on server and client stuff tends to work (or fail at serialization/deserialization time).

matthid avatar Dec 21 '19 19:12 matthid

  • Yes, FetchAs is actually using Thoth.Json. Here's the source: it opens the Thoth.Json namespace and then calls Decode.unsafeFromString on the HTTP response body. As far as I can tell from the Thoth.Json source, Map<string,string> should be auto-decoding correctly, and indeed it does auto-decode correctly when it's a field inside a record. The auto-decoding only fails to produce a Map instance when it's a field inside a record inside a discriminated union.
  • Yes, Thoth is in use on the Giraffe side, via use_json_serializer(Thoth.Json.Giraffe.ThothSerializer() in Saturn.

@matthid, when you mentioned the internals changing between versions: are you referring to internals and versions of Fable? Thoth? Whatever that change is that you're talking about, I'd like to take a look at it, because that might give me a clue as to where I should start.

rmunn avatar Dec 23 '19 20:12 rmunn

Ok, I can confirm that there is a problem when the map is inside a record inside a discriminated union.

Reproduction code using Thoth.Json tests modules.

module Tests.Encoders

open Thoth.Json
open Util.Testing
open System
open Tests.Types
open Fable.Core

type ProjectDetails =
    {
        Description : string
        Membership : Map<string,string>
    }

[<Erase>]
type JsonResultProjectDetails =
    | Success of ProjectDetails
    | Failure of exn

let tests : Test =
    testList "Thoth.Json.Encode" [

        testList "Repro" [

            testCase "test #1 - non empty map inside a record works" <| fun _ ->
                let data =
                    {
                        Description = "test-project"
                        Membership = Map.ofList [ "maxime", "user"; "rmunn", "admin" ]
                    }

                let json =
                    Encode.Auto.toString(4, data)

                let decoded = Decode.Auto.unsafeFromString(json)

                equal false (Map.isEmpty decoded.Membership)

            testCase "test #2 - empty map inside a record works" <| fun _ ->
                let data =
                    {
                        Description = "test-project"
                        Membership = Map.empty
                    }

                let json =
                    Encode.Auto.toString(4, data)

                let decoded = Decode.Auto.unsafeFromString(json)

                equal true (Map.isEmpty decoded.Membership)

            testCase "test #3 - non empty map inside a record inside a DU works" <| fun _ ->
                let data =
                    Success
                        {
                            Description = "test-project"
                            Membership = Map.ofList [ "maxime", "user"; "rmunn", "admin" ]
                        }

                let json =
                    Encode.Auto.toString(4, data)

                let decoded = Decode.Auto.unsafeFromString(json)

                match decoded with
                | Success jsonSuccess ->
                    equal false (Map.isEmpty jsonSuccess.Membership)

                | Failure _ ->
                    failwith "should not happen"


            testCase "test #4 - empty map inside a record inside a DU works" <| fun _ ->
                let data =
                    Success
                        {
                            Description = "test-project"
                            Membership = Map.empty
                        }

                let json =
                    Encode.Auto.toString(4, data)

                let decoded = Decode.Auto.unsafeFromString(json)

                match decoded with
                | Success jsonSuccess ->
                    equal true (Map.isEmpty jsonSuccess.Membership)

                | Failure _ ->
                    failwith "should not happen"

            testCase "test #5 - Map.count against decoded map inside a record works" <| fun _ ->
                let data =
                    {
                        Description = "test-project"
                        Membership = Map.ofList [ "maxime", "user"; "rmunn", "admin" ]
                    }

                let json =
                    Encode.Auto.toString(4, data)

                let decoded = Decode.Auto.unsafeFromString(json)

                equal 2 (Map.count decoded.Membership)

            testCase "test #6 - Map.count against decoded map inside a record inside a DU works" <| fun _ ->
                let data =
                    Success
                        {
                            Description = "test-project"
                            Membership = Map.ofList [ "maxime", "user"; "rmunn", "admin" ]
                        }

                let json =
                    Encode.Auto.toString(4, data)

                let decoded = Decode.Auto.unsafeFromString(json)

                match decoded with
                | Success jsonSuccess ->
                    equal 2 (Map.count jsonSuccess.Membership)

                | Failure _ ->
                    failwith "should not happen"

        ]

MangelMaxime avatar Jan 13 '20 10:01 MangelMaxime

When looking at the generated JSON it seems like when the "map is inside a record inside a DU" the JSON output is different from what is expected for a map.

test #1: {
    "Description": "test-project",
    "Membership": {
        "maxime": "user",
        "rmunn": "admin"
    }
}
      √ test #1 - non empty map inside a record works
test #2: {
    "Description": "test-project",
    "Membership": {}
}
      √ test #2 - empty map inside a record works
test #3: {
    "Description": "test-project",
    "Membership": {
        "comparer": {},
        "tree": [
            "MapNode",
            "rmunn",
            "admin",
            [
                "MapOne",
                "maxime",
                "user"
            ],
            "MapEmpty",
            2
        ]
    }
}
      √ test #3 - non empty map inside a record inside a DU works
test #4: {
    "Description": "test-project",
    "Membership": {
        "comparer": {},
        "tree": "MapEmpty"
    }
}
      1) test #4 - empty map inside a record inside a DU works
test #5: {
    "Description": "test-project",
    "Membership": {
        "maxime": "user",
        "rmunn": "admin"
    }
}
      √ test #5 - Map.count against decoded map inside a record works
test #6: {
    "Description": "test-project",
    "Membership": {
        "comparer": {},
        "tree": [
            "MapNode",
            "rmunn",
            "admin",
            [
                "MapOne",
                "maxime",
                "user"
            ],
            "MapEmpty",
            2
        ]
    }
}

So right now, I guess we first need to take a look at the Encode.Auto code.

MangelMaxime avatar Jan 13 '20 10:01 MangelMaxime

Also note that I've seen the same thing with lists, e.g. if you add a List<string> field to the ProjectDetails type it should exhibit similar decoding problems to Map<string,string>, decoding to a simple Javascript array instead of a Fable List type.

rmunn avatar Jan 16 '20 09:01 rmunn

The problem is caused by [<Erased>] attributes, if you remove it then the generated JSON is correct.

test #1: {
    "Description": "test-project",
    "Membership": {
        "maxime": "user",
        "rmunn": "admin"
    }
}
      √ test #1 - non empty map inside a record works
test #2: {
    "Description": "test-project",
    "Membership": {}
}
      √ test #2 - empty map inside a record works
test #3: [
    "Success",
    {
        "Description": "test-project",
        "Membership": {
            "maxime": "user",
            "rmunn": "admin"
        }
    }
]
      √ test #3 - non empty map inside a record inside a DU works
test #4: [
    "Success",
    {
        "Description": "test-project",
        "Membership": {}
    }
]
      √ test #4 - empty map inside a record inside a DU works
test #5: {
    "Description": "test-project",
    "Membership": {
        "maxime": "user",
        "rmunn": "admin"
    }
}
      √ test #5 - Map.count against decoded map inside a record works
test #6: [
    "Success",
    {
        "Description": "test-project",
        "Membership": {
            "maxime": "user",
            "rmunn": "admin"
        }
    }
]
      √ test #6 - Map.count against decoded map inside a record inside a DU works

MangelMaxime avatar Jan 20 '20 15:01 MangelMaxime

It seems like Erased union are considered System.Object in Fable reflection API and because we pass it as it is we didn't had an error reported.

Erased unions are not yet supported in Fable reflection API, I will take a look to add it. // TODO!!! Add reflection tests for interfaces, erased unions, // string enums, imported and replaced types

And I am also wondering why we added obj support out of the box inside of Thoth.Json. I mean, we can't guarantee the representation of the type, neither the decoding process. I will probably remove it and see if people were relying on it. For me, it looks like a problem/bug when considering Thoth.Json goals.

MangelMaxime avatar Jan 20 '20 15:01 MangelMaxime

I\m pretty sure we are one of the poor guys depending on obj, but afaik we use a custom Decoder/Encoder for it.

matthid avatar Jan 20 '20 15:01 matthid

Actually obj support is needed as it used to support null encoding/decoding.

When disabling it the only tests that are failing about null handling.

I think the issue is on Fable side and not Thoth.Json as it is Fable who doesn't provide the right information.

MangelMaxime avatar Jul 03 '20 21:07 MangelMaxime