Decode.fromString can throw exceptions

[kentcb]

Hi,

I may be misunderstanding Decode.fromString, but the implementation and its unsafe counterpart seems to suggest that fromString should not throw, ever. However, I am able to get it to throw by giving it some bad data:

System.Reflection.TargetParameterCountException: 'Parameter count mismatch.'
This exception was originally thrown at this call stack:
    Thoth.Json.Net.Decode.makeUnion(Microsoft.FSharp.Collections.FSharpMap<string, Microsoft.FSharp.Core.FSharpRef<Thoth.Json.Net.BoxedDecoder>>, bool, System.Type, string, string, Newtonsoft.Json.Linq.JToken[])
    [email protected](string, Newtonsoft.Json.Linq.JToken)
    Thoth.Json.Net.Decode.DecoderCrate<T>.Decode(string, Newtonsoft.Json.Linq.JToken)
    Thoth.Json.Net.Decode.AutoModule.generateDecoder@1289<T>.Invoke(string, Newtonsoft.Json.Linq.JToken)
    Thoth.Json.Net.Decode.fromValue<T>(string, Microsoft.FSharp.Core.FSharpFunc<string, Microsoft.FSharp.Core.FSharpFunc<Newtonsoft.Json.Linq.JToken, Microsoft.FSharp.Core.FSharpResult<T, System.Tuple<string, Thoth.Json.Net.ErrorReason>>>>, Newtonsoft.Json.Linq.JToken)
    Thoth.Json.Net.Decode.fromString<T>(Microsoft.FSharp.Core.FSharpFunc<string, Microsoft.FSharp.Core.FSharpFunc<Newtonsoft.Json.Linq.JToken, Microsoft.FSharp.Core.FSharpResult<T, System.Tuple<string, Thoth.Json.Net.ErrorReason>>>>, string)

All I did to reproduce this is leave off a leading [ from my input string, which is being decoded into a single case DU. Is this a bug in Thoth, or something I'm doing wrong? Obviously I can simply catch this or all exceptions and assume the decoding failed, but I'm wondering why I need to do that when fromString already returns a Result<>.

[MangelMaxime]

Hello @kentcb,

Decode.fromString can fail if the exception is coming from one of the decoder.

For example, if you write a custom decoder and don't handle the exception at the decoder level, it will bubble to the top and it will be the user responsibility to handle it.

That's why most of the decoder use TryParse and not Parse

Thoth.Json.Net/src/Decode.fs

Lines 303 to 310 in 2a70134

	let timespan : Decoder<System.TimeSpan> =
	fun path token ->
	if token.Type = JTokenType.TimeSpan || token.Type = JTokenType.String then
	match System.TimeSpan.TryParse (Helpers.asString token, CultureInfo.InvariantCulture) with
	| true, x -> Ok x
	| _ -> (path, BadPrimitive("a timespan", token)) |> Error
	else
	(path, BadPrimitive("a timespan", token)) |> Error

Like that they can handle the error path and provide a good/better error message.

The only exception that is handled by Decode.fromString is this one Newtonsoft.Json.JsonReaderException

Thoth.Json.Net/src/Decode.fs

Lines 97 to 106 in 2a70134

	let fromString (decoder : Decoder<'T>) =
	fun value ->
	try
	use reader = new JsonTextReader(new StringReader(value), DateParseHandling = DateParseHandling.None)
	let json = Newtonsoft.Json.Linq.JValue.ReadFrom reader
	fromValue "$" decoder json
	with
	| :? Newtonsoft.Json.JsonReaderException as ex ->
	Error("Given an invalid JSON: " + ex.Message)

In theory, it should catch all the exception when passing an invalid JSON.

Can you please provide a reproduction code?

[kentcb]

Thanks @MangelMaxime. Here is a repro:

module Repro

open Xunit
open Thoth.Json.Net

type SomeUnion = SomeUnion of string
let someUnionDecoder = Decode.Auto.generateDecoder<SomeUnion> ()

[<Fact>]
let ``repro`` () =
    let good = """["SomeUnion","foo"]"""
    let goodDecode = Decode.fromString someUnionDecoder good
    let expectedGood = "foo" |> SomeUnion |> Ok
    Assert.Equal(goodDecode, expectedGood)

    let bad = """"SomeUnion","foo"]"""
    let badDecode = Decode.fromString someUnionDecoder bad
    let expectedBad = Error "Some kind of message"
    Assert.Equal(badDecode, expectedBad)

Instead of getting Error Some kind of message an exception is thrown:

System.Reflection.TargetParameterCountException : Parameter count mismatch.

  Stack Trace: 
    RuntimeMethodInfo.InvokeArgumentsCheck(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    Decode.makeUnion(FSharpMap`2 extra, Boolean isCamelCase, Type t, String name, String path, JToken[] values)
    [email protected](String path, JToken value)
    DecoderCrate`1.Decode(String path, JToken token)
    [email protected](String path, JToken token)
    Decode.fromValue[T](String path, FSharpFunc`2 decoder, JToken value)
    Decode.fromString[T](FSharpFunc`2 decoder, String value)
    Repro.repro() line 17

[MangelMaxime]

Hello @kentcb,

sorry for the delay I was on vacation and wanted to wrap things up on another OSS project of mine. But got sidetracked a bit 😅

So, the error you are having is because Newtonsoft seems to consider your JSON as valid...

When it is not:

And because of that, the reflection is trying to call the reflection API but with the wrong number of arguments as it can't extract the arguments from the JSON because it is invalid...

So for me the error is not in Thoth.Json.Net but in Newtonsoft. I opened an issue about that on their repo: JamesNK/Newtonsoft.Json#2585

I don't know how to capture the error in a clear way to give back a better message. Also, in future version, we will remove Newtonsoft as a dependency so this kind of cryptic error should not happen anymore.

[MangelMaxime]

Hello @kentcb,

it happen that this was due to how we consume Newtonsoft.

The way we used it, was we were asking it to consomme the JSON "token" per "token" and not as a whole.

It has been fixed in version 7.1.0