fsprojects / Fleece

Json mapper for F#
http://fsprojects.github.io/Fleece
Apache License 2.0
199 stars 31 forks source link

add OfJson and ToJson instances for JsonObject and JsonValue #71

Closed mt-caret closed 4 years ago

mt-caret commented 4 years ago

Addresses https://github.com/mausch/Fleece/issues/70.

@gusty I have a background in FP (OCaml, Haskell, and the like), but I'm a novice in F#, so I'm not 100% confident if this works (at least it seems to compile).

gusty commented 4 years ago

Actually, there is already a failing test:

From JSON/nested item: Failed: 
item
Expected: Ok (NestedItem {Id = 1;
                Brand = "Sony";
                Availability = Some "1 week";})
Actual: Error
  (JsonTypeMismatch
     (System.Tuple`2[System.String,FSharp.Data.JsonValue][],
      {
  "brand": "Sony",
  "availability": "1 week"
},Array,Object))
 (00:00:00.3021316)
todo: {
  "a": true,
  "b": 200,
  "c": "a"
} ~ {
  "a": true,
  "b": 200,
  "c": null
}
63 tests run: 62 passed, 0 ignored, 1 failed, 0 errored (00:00:17.8344018)

and this is coming from the FSharp.Data flavor

I suspect this is due to the fact I mentioned initially, that in FSharp.Data it's an alias:

type JsonObject = (string * JsonValue) []

But let's try to make sense of this and evaluate possible workarounds.

mt-caret commented 4 years ago

This seems to be an odd error, since the JsonValue OfJson instance is triggering the test failure (all tests pass if that line is commented out).

gusty commented 4 years ago

I'll have a look at this and see how can I help to unblock you.

gusty commented 4 years ago

It's funny that in Debug mode it works. The problem shows only when compiled in Release mode which means that there is a difference in overload resolution, however I wouldn't be surprised if that's the case.

gusty commented 4 years ago

Let's add some details of the problem:

As I said, the root problem is the type alias in FSharp.Data.

We can take the failing code and break it down:

let (Ok jsonv) = fst jsonValueToTextCodec """{"id": 1, "blah": {"brand": "Sony", "availability": "1 week"}}""" ;;

val jsonv : FSharp.Data.JsonValue =
  {
  "id": 1,
  "blah": {
    "brand": "Sony",
    "availability": "1 week"
  }
}

So that's our jsonValue, now if we do ofJson

> let actual: NestedItem ParseResult = ofJson jsonv ;;

[<Struct>]
val actual : NestedItem ParseResult =
  Error
    (JsonTypeMismatch
       (System.Tuple`2[System.String,FSharp.Data.JsonValue][],
        {
  "brand": "Sony",
  "availability": "1 week"
}, Array, Object))

> Result.mapError string actual ;;

[<Struct>]
val it : Result<NestedItem,string> =
  Error
    "Array expected but got Object while decoding {
  "brand": "Sony",
  "availability": "1 week"
} as System.Tuple`2[System.String,FSharp.Data.JsonValue][]"

This comes from this overload https://github.com/mt-caret/Fleece/blob/9f9d85bf395ca19ea099a8868402e5a985642e0c/Fleece/Fleece.fs#L713

instead of this one https://github.com/mt-caret/Fleece/blob/9f9d85bf395ca19ea099a8868402e5a985642e0c/Fleece/Fleece.fs#L762

Why ?

Both of them match as both expect the same result type, one expects array<'t> and the other one a JsonObject but in FSharpData a JsonObject is actually an array<string,JsonValue> so the resolution can go either to array<string,JsonValue> or the more generic array<'t>.

I would expect to go to the more generic array<'t> as the other one has a Default1 marker, but as I said the funny thing is that in Debug mode it goes the more specific one, despite the marker.

Possible solutions:

But I would suggest to leave JsonObject de/serialization overloads out from this PR and work in a separate PR, as doing so this can be merged asap.

gusty commented 4 years ago

Still, one thing that it's not entirely clear to me, is why the fact of adding the OfJson for JsonValue makes this difference, as JsonValue itself is not an alias to a primitive type.

It probably has to do with some dark rules in overload resolution.

Now, what I'm thinking is that probably, even without this PR there might be some conflicting cases, because the main issue seems to be aliasing JsonObject with (string * JsonValue) [] which has a different representation.

So, (string * JsonValue) [] is represented as a JArray but if we consider it a JsonObject it should be JObject instead.

For all the above, a better solution would be to change the type alias, to a primitive type that serialize to JObject, like Map<string,JsonValue.

Maybe I should open a separate issue for that. I'm sure that a side effect of solving that is that this PR will work.

gusty commented 4 years ago

@mt-caret this is ready to go to master and a release would follow shortly.

I just wonder if you're happy with the changes.

Can you possibly test it with your code?