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

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

Open
cmeeren opened this issue Jan 25, 2020 · 7 comments
Open
Labels
bug Something isn't working

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Jan 25, 2020

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
Copy link
Owner

Tarmil commented Jan 25, 2020

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
Copy link
Owner

Tarmil commented Jan 25, 2020

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
Copy link
Contributor Author

cmeeren commented Jan 25, 2020

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
Copy link
Owner

Tarmil commented Jan 25, 2020

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

@cmeeren
Copy link
Contributor Author

cmeeren commented Jan 27, 2020

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:

fields.[i] <- JsonSerializer.Deserialize(&reader, p.Type, options)

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
Copy link
Contributor Author

cmeeren commented Feb 1, 2020

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

@jannikbuschke
Copy link

dotnet/runtime#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants