Tarmil / FSharp.SystemTextJson

System.Text.Json extensions for F# types
MIT License
328 stars 43 forks source link

JsonException path, position, error message etc. incorrect for records #37

Open cmeeren opened 4 years ago

cmeeren commented 4 years ago

FSharp.SystemTextJson completely messes up the path and error messages when deserialization fails.

This is causing me some consternation, because I am creating an F# framework for JSON:API (using an immutable record-based document model) and want to return helpful deserialization messages to API clients, but that's not currently possible.

Repro:

DeserializationErrorTest.zip

open System.Text.Json
open System.Text.Json.Serialization

type ARecord = {
  X: int
}

type BRecord = {
  A: ARecord
}

type CRecord = {
  B: BRecord
}

type A () =
  member val X: int = 0 with get, set

type B () =
  member val A: A = Unchecked.defaultof<A> with get, set

type C () =
  member val B: B = Unchecked.defaultof<B> with get, set

[<EntryPoint>]
let main argv =

  let opts = JsonSerializerOptions()
  opts.Converters.Add(JsonFSharpConverter())
  let json = """{"B":{"A":{"X":"invalid"}}}"""

  // Correct path/message
  printfn "Deserializing normal classes\n"
  try JsonSerializer.Deserialize<C>(json, opts) |> ignore
  with ex -> printfn "%A" ex

  // Incorrect path/message
  printfn "\n\n\nDeserializing records\n"
  try JsonSerializer.Deserialize<CRecord>(json, opts) |> ignore
  with ex -> printfn "%A" ex

  0

Expected outcome:

Deserializing normal classes

System.Text.Json.JsonException: The JSON value could not be converted to System.Int32. Path: $.B.A.X | LineNumber: 0 | BytePositionInLine: 24.
 ---> System.InvalidOperationException: Cannot get the value of a token type 'String' as a number.
   <stack trace omitted>

Deserializing records

System.Text.Json.JsonException: The JSON value could not be converted to System.Int32. Path: $.B.A.X | LineNumber: 0 | BytePositionInLine: 24.
 ---> System.InvalidOperationException: Cannot get the value of a token type 'String' as a number.
   <stack trace omitted>

Actual outcome:

Expected outcome:

Deserializing normal classes

System.Text.Json.JsonException: The JSON value could not be converted to System.Int32. Path: $.B.A.X | LineNumber: 0 | BytePositionInLine: 24.
 ---> System.InvalidOperationException: Cannot get the value of a token type 'String' as a number.
   <stack trace omitted>

Deserializing records

System.Text.Json.JsonException: The JSON value could not be converted to System.Int32. Path: $ | LineNumber: 0 | BytePositionInLine: 9. Path: $ | LineNumber: 0 | BytePositionInLine: 5. Path: $ | LineNumber: 0 | BytePositionInLine: 5. Path: $ | LineNumber: 0 | BytePositionInLine: 5.
 ---> System.InvalidOperationException: Cannot get the value of a token type 'String' as a number.
   <stack trace omitted>
Tarmil commented 4 years ago

Ah, this is bad indeed. It looks like the path is handled by passing a string to JsonException, so this shouldn't be too difficult to fix.

Tarmil commented 4 years ago

So, as it turns out, this is not currently possible. Even if I catch the exception and raise a new one with a correct path, the exception gets mutated by System.Text.Json and its path is reset to "$" based on some internal properties of the reader. There is a proposed API to make such things extensible, and it seems that they want to have it for .NET 5, but not before.

cmeeren commented 4 years ago

Just to be clear, are you saying that with System.Text.Json it's not possible at all to get correct paths when implementing JSON converters for objects? (Except perhaps a hacky solution involving thread local storage or something, yuck...)

Tarmil commented 4 years ago

As far as I can tell from reading the source of S.T.J, that's right.

cmeeren commented 4 years ago

I have noticed that it's possible at least get the correct line number and byte position.

In terms of the code in this repo, instead of calling Deserialize here:

https://github.com/Tarmil/FSharp.SystemTextJson/blob/479c38162ffe51751b2aba1300b3531ea3d32f3f/src/FSharp.SystemTextJson/Record.fs#L87

Instead, call reader.Read() first, and then do this:

(options.GetConverter(p.Type) :?> JsonConverter<??>).Read(&reader, p.Type, options)

I'm not sure what you should place instead of ?? since you don't have the type statically, but I'm sure you can find a reflection-based way to call Read.

When deserialization fails, you get a message like this:

System.Text.Json.JsonException: The JSON value could not be converted to Program+C. Path: $ | LineNumber: 4 | BytePositionInLine: 20.
 ---> System.InvalidOperationException: Cannot get the value of a token type 'String' as a number.

The path is still incorrect, but the message is otherwise well formed, and the line number and byte position is correct and can be retrieved from the caught JsonException object, which is very useful information for API clients.

cmeeren commented 4 years ago

Actually, the LineNumber and BytePositionInLine properties of the JsonException are still incorrect, but at least the message is correct.

jannikbuschke commented 1 year ago

https://github.com/dotnet/runtime/issues/1562 seems to be implemented, any chance of looking into this again @tarmil? I could also have a look, but not sure if I am able to.