Add escape-hatch custom error constructor

[johncowie]

I've just upgraded my package-set and encountered the breaking changes from the new JsonDecodeError type. I'm not against having a more specific error type than String, but I find the available error constructors are too restrictive when writing my own instances of DecodeJson.

I propose adding an additional constructor to JsonDecodeError for supplying a custom error message, in the case that the errors produced in new DecodeJson instances aren't covered by the existing constructors.

[thomashoneyman]

Would you mind sharing an example? This error type is shared with purescript-codec-argonaut and it may be a good idea to extend it with a custom constructor.

[johncowie]

@thomashoneyman Yeah sure - a trivial example of the use case would be something like:

import Prelude
import Data.Argonaut.Decode (class DecodeJson, decodeJson)
import Data.Argonaut.Decode.Error (JsonDecodeError(..))
import Data.Either (Either(..))

newtype NonEmptyString = NonEmptyString String

instance decodeJsonNonEmptyString :: DecodeJson NonEmptyString where
  decodeJson json = do
    str <- decodeJson json
    case str of
      "" -> Left $ CustomError "String must not be empty"
      _ -> Right $ NonEmptyString str

Is that what you meant by an example?

[johncowie]

Btw if anyone has ideas for a better name for the constructor than CustomError, I'm very open to suggestions.

[thomashoneyman]

Yea, that’s all I meant! I’m trying to get a sense for what may not fit the existing type very well. For example, that NonEmptyString error would usually be something like:

TypeMismatch “NonEmptyString: String must be non-empty”

I’d like to walk through issues in codec-argonaut to see what other examples exist also. But it’s probably a good idea to include an escape hatch.

[johncowie]

At the moment if I used the TypeMismatch constructor I would end up with something like:

Expected value of type 'NonEmptyString: String must be non-empty.'"

when printing the error, which looks a bit odd. Alternatively I could just pass in the type name so it reads:

Expected value of type 'NonEmptyString'

but it's a shame to lose that error information. Appreciate this is very trivial example so not to difficult to figure out what might be the issue with a NonEmptyString. A better example might be something like a date type that you want to decode from a string (e.g. in the format DD/MM/YYYY) - in which case there are potentially a few different ways that the decoding could fail. It seems to me like the TypeMismatch error is really for a mismatch between JSON types (e.g. expected an Int but got an Object).

More philosophically I think that given the user has the freedom to define their own decoding instances, they should have some sort of option for tailoring their own error messages for their decoders if they choose to (as was possible before JsonDecodeError was introduced).

[thomashoneyman]

I agree that it's sensible that an escape hatch exists, and that TypeMismatch isn't quite the right thing for an error decoding a simple type into a richer one (like PreciseDate or something). Given that the type here is the same one used in purescript-codec-argonaut I'd like to discuss what an improvement might look like with @garyb, as he's been maintaining that library with this error type for much longer.

Either way, I checked with the package set and adding another constructor doesn't break anything. It shouldn't be a change that ripples out in the way that introducing the type in the first place was (few people, if any, are manually pattern-matching on this type). That makes it more palatable to make the change.

[johncowie]

Cool, yeah I must admit I'm a bit ignorant of the history of purescript-argonaut-codes & purescript-codec-argonaut but getting @garyb's input sounds good to me.

[garyb]

I'm torn - there are definitely times that I want to be a little more precise than just using a type mismatch or unexpected value error, but I'm also wary of adding it because it's open to being overused and losing the structural errors that this helps with in the first place.

I did consider using an open variant for the error, as that way it can be extended with additional meaningful constructors, but it adds quite a bit of complexity too. The variant approach seems a bit more like the correct thing to do, but I don't want the weight of it to be offputting either. 🤔

[srghma]

Just a note

the

TypeMismatch “NonEmptyString: String must be non-empty”

should be

Named "NonEmptyString" $ TypeMismatch “String must be non-empty”

[srghma]

is it really needed

why not MyJsonDecodeError = Orig JsonDecodeError | My String

[wclr]

I believe one more open but still potentially specific error type could be named ParsingFailure. But there still needs to be made a clarification on the difference between TypeMismatch, UnexpectedValue and ParsingFailure, because without it any error could be referred to any of these types.

I did consider using an open variant for the error, as that way it can be extended with additional meaningful constructors

Maybe this error type could be used for this.

[ntwilson]

Perhaps it would be more structured to add a ValidationFailed case instead of just CustomError? I often find myself wanting a better message after I've gotten the "raw" JSON value parsed, but when trying to build a value of some sort of richer type than what you can represent with just JSON. The examples given so far (making sure an array is non-empty, and a string is able to be parsed as a date) I think fall in the category of failures to validate the "raw" JSON array and string.

I generally prefer to do parsing and validation in a single step, which is why I run into this issue, but this all makes me wonder if it's even appropriate to have EncodeJson and DecodeJson instances for types whose values need validation? Sometimes, validation requires additional runtime information beyond just the serialized value, which precludes using the EncodeJson and DecodeJson classes anyway (e.g., say I serialize a Product via some UUID, but then to deserialize it I need to also have access to a lookup table of all Products by their ID, and finding a matching product by its ID sounds like a validation task to me). I would be content if it was decided that these classes and the JsonDecodeError type were only intended for serialization/deserialization, and then validation should be separate - and things like NonEmptyList or Date which require validation just don't make good instances of EncodeJson and DecodeJson. But it would make my life simpler if there were just a single error type that included both parse failures and validation failures 😉

[JordanMartinez]

How about using a Void pattern?

data JsonDecodeError' a
  = TypeMismatch String
  | UnexpectedValue J.Json
  | AtIndex Int JsonDecodeError
  | AtKey String JsonDecodeError
  | Named String JsonDecodeError
  | MissingValue
  | Custom a

type JsonDecodeError = JsonDecodeError' Void

printJsonDecodeError ∷ JsonDecodeError → String
printJsonDecodeError = printJsonDecodeError' absurd

printJsonDecodeError' :: forall a. (a -> String) -> JsonDecodeError' -> String
printJsonDecodeError' printCustom = case _
  ...
  Custom a -> printCustom a

[thomashoneyman]

@JordanMartinez I'm on board with your suggested solution here, if you'd like to go forward with it.