Suggestion: Let error String of fromJSON contain json path info.

[maralorn]

I was surprised to see that the quality of json parse errors in our program differed at different places. The discrepancy stemmed from using fromJSON and parseEither.

The reason is that parse ignores the json path when constructing the Result: (see const Error) here. The const drops info.

aeson/src/Data/Aeson/Types/Internal.hs

Lines 566 to 567 in 45d31f1

	parse :: (a -> Parser b) -> a -> Result b
	parse m v = runParser (m v) [] (const Error) Success

While parseEither adds the json path into the error message with onError path msg = Left (formatError path msg).

aeson/src/Data/Aeson/Types/Internal.hs

Lines 582 to 584 in 45d31f1

	parseEither :: (a -> Parser b) -> a -> Either String b
	parseEither m v = runParser (m v) [] onError Right
	where onError path msg = Left (formatError path msg)

This might very well be fully intentional, but again it took me by surprise. And my personal opinion is that the default should be the more verbose error.

I would thus suggest to either

• change the implementation of parse to also use formatError. (People can opt-in to the old behavior by using iparse and dropping the path.)
• Or to implement fromJSON in a way which uses formatError. E.g. by using iparse and applying formatError or on top of parseEither.

I see however that both suggestions might be contentious, so I am open to a discussion about this. If this idea is accept I would volunteer to implement the change.

[phadej]

There is https://hackage.haskell.org/package/aeson-2.2.3.0/docs/Data-Aeson-Types.html#v:iparseEither (and others) which gives the path

if you need the path, use those.

[phadej]

Oh I see the disrepancy:

• iparse and parse
• vs. iparseEither and parseEither.

I'll probably fix that by making parseEither ignore the path, and add parseEitherFormatPath (and maybe parseFormattingPath).

Note: parse with its current type-signature has no access to path info, it uses Result, not IResult. It might be possible to ditch Result (i.e. pathless parsing) completely. Then your suggestion would be possible, but that's more invasive change. Maybe it's the way to go though, i.e. embrace path-ful parsing.

[maralorn]

I am in favor of any solution to fix the discrepancy between parse and parseEither.

I would prefer the default msg to contain the formatPath but the other way is also fine.

Note: parse with its current type-signature has no access to path info, it uses Result, not IResult. It might be possible to ditch Result (i.e. pathless parsing) completely. Then your suggestion would be possible, but that's more invasive change. Maybe it's the way to go though, i.e. embrace path-ful parsing.

Couldn’t we simply change parse to do something like this? That would exactly mirror what parseEither does.

parse :: (a -> Parser b) -> a -> Result b 
parse m v = runParser (m v) [] onError Success
   where onError path msg = Error (formatError path msg) 

[phadej]

I see. I think the right solution is to remove Result and parse. All use cases can be converted to iparse, and then you can ignore the path or combine it with an actual message.

I don't want to have three variants, i.e.

• combine
• omit
• leave the option to the user

when the last one is enough.

Having two (i.e. leave the option to the user and either of two others) feels arbitrary.

[phadej]

In

eitherDecodeStrict :: (A.FromJSON a) => BS.ByteString -> Either String a
eitherDecodeStrict bs = unResult (toResultValue (bsToTokens bs)) Left $ \v bs' -> case A.ifromJSON v of
    A.ISuccess x
        | bsSpace bs' -> Right x
        | otherwise   -> Left String
"Trailing garbage"
    A.IError path msg -> Left $ formatError path msg

having formatError is somewhat justified, but users may as well ask for a variant which would not formatError and give them a path (and not be forced to reimplement that themselves).

Arguably, AesonException definitely should have path and msg separate. displayException may combine them, but it's otherwise better to not do work not asked for.

It's a mess.

[phadej]

Reusing AesonException as an error type (insttead of somewhat unclear (JSONPath, String) pair), we could have

eitherDecodeStrict :: (A.FromJSON a) => BS.ByteString -> Either AesonException a

and users may turn the error part into string with displayException if then need to.

Maybe that's a way to clean up API of the library. I have to think this through and settle in my head.

[maralorn]

• I agree that having parse and parseEither with isomorphic return types feels redundant and like your ideas for a cleanup.
• AesonException currently seems to be missing the JSONPath information. I guess it should be enriched if you want to use it more.
• In the end the one function that we would want to use in our codebase is parseEither (for non type class parsers) and parseEither parseJSON i.e. fromJSON but with return type Either. With path+error somehow in Left, probably just as String (or maybe Text while we are it?). But applying first displayException to it would be bearable. I am not saying this should be the bare primitive in aeson, but it is the function I would always go for and would probably define on top of aeson if it is missing from it.