optionalField Decoder

[ncthbrt]

When parsing user input it's important to be able to give good error messages. For example in a http PATCH request, the date_of_birth field might only accept a UTC conformant string when the field is present. If a user passes in a number, we need to return with a 400 BAD REQUEST.

When using the optional decoder in combination with the field decoder, if the field exists but the inner decoder fails, the final result will be None.

In the previous example { dateOfBirth: optional(field("date_of_birth", utcString)) } when fed with the payload { "date_of_birth": 1234 } would simply be { dateOfBirth: None }. The actual semantics that needed to be captured in this example was:

• { } becomes { dateOfBirth: None }
• { "date_of_birth": "Wed, 14 Feb 2018 10:22:19 GMT" } becomes { dateOfBirth: Some("Wed, 14 Feb 2018 10:22:19 GMT")
• { "date_of_birth": 123457 } becomes Json.Decoder.DecoderError("Expected a UTC string, got a number")

An optionalField decoder (or a decoder with a similar name) would solve this problem by returning None if the field is undefined, else would bubble up the error from inner decoder.

[glennsl]

Hmm, that's an interesting case. I can definitely see how this can be generally useful, but I think optionalField might be too specific.

I'd like to make errors more information-rich, but I'm not entirely sure how to best do so. Something like exception DecodeError(string, [>]) perhaps, so you could distinguish between DecodeError(_, `MissingField) and DecoderError(_, `IncompatibleType). but in your case you might instead want to distinguish between errors emitted by the given function from those emitted by nested functions. That's more difficult to generalize in a usable way.

In the meantime, could you do something like this instead?

exception Fatal(exn);

let fatal = decoder =>
  json => try decoder(json) {
    | DecodeError(_) as e => Fatal(e)
  };

...

{ dateOfBirth: optional(field("date_of_birth", fatal(utcString))) }

[ncthbrt]

That's a good approach. We took a more manual work around, but this seems much cleaner.

[RasmusKlett]

I think the definition of optional is very problematic. There are three distinct use cases where I would want to produce an option:

1. Return None if the field doesn't exist
2. Return None if the value of the field is null
3. Return None if the inner parsing fails for some reason

Personally I would expect a decoder named optional to be version 1, 2, or possibly a combination.
Version 3 is a very heavy-handed tool - you need to be extremely certain of which errors the inner decoder may throw, or you end up unexpectedly swallowing errors. I would expect such a decoder to have a name that stands out like failSilentlyOptional or decodeOrNone.
The current situation will definitely lead users of the library to swallow decoding errors by accident.

Apart from the above, I'm really enjoying using this library ;)

[RasmusKlett]

@glennsl Would you be open to a pull request switching optional to be strict, and adding a decodeOrNone implementing the current behaviour? This would be a slightly breaking change, I guess.

[ncthbrt]

@RasmusKlett Why not think of a new name? And mark the current one as deprecated?

[glennsl]

I do agree that the current situation is far from optimal, and that it will cause errors, but I think this needs to be thought through on a deeper level and be planned for a future 2.0 release. What do you mean by "switching optional to be strict", though?

Currently optional behaves like Elms Json.Decode.maybe, which I think is a reasonable choice in the short term since it's at least familiar to some.

[RasmusKlett]

@ncthbrt You're right, that's probably a better strategy.

@glennsl In my opinion the default optional decoder should match the JSON concept of a value which might be present. In some cases this is a field that might be null, in other cases it is a field which might be undefined. In no cases is it a field which might throw an error when decoding is attempted.

I guess my suggestion is to have three separate decoders for these cases, so the decoders can be more strict in what they accept, warning the developer earlier when something is not as expected.

[glennsl]

@RasmusKlett Sounds good in theory, but how would you implement it? optional doesn't operate on just fields, and field is just a generic decoder, it doesn't carry any information about it having anything to do with fields.

[RasmusKlett]

Warning: untested code ahead, I hope you get the point of it. Also I might be mixing Reason and OCaml syntax, sorry.

The most often used I think would be the null-as-None decoder:

let nullToNone = (decoder, json) =>
  json == Js.null ? None : decoder(json);

I guess to handle an undefined field, you would need a separate decoder:
optionalField : sting -> 'a decoder -> 'a option decoder
Which returns None if the field is undefined, and returns Some of the given decoder otherwise.

Lastly, we could have:
exceptionToNone : 'a decoder -> 'a option decoder
which swallows any errors during decoding, and turns them into None.

This can be composed into

1. Handling value which may be null: field("x" nullToNone(int))
2. Field which may be undefined, but always has a value when present: optionalField("x" int)
3. Field which may be undefined, and value may be null: optionalField("x", nullToNone(int))
4. Field which may be malformed, and we want to ignore decoding error: exceptionToNone(field("x", int))

In my opinion this doesn't add too much syntactic noise, and it allows a much more granular specification of what we want, reducing surprising behaviour in the future by throwing early.

[ncthbrt]

@RasmusKlett Why not:

  json == Js.null ? None : Some(decoder(json));

Seems like automatically wrapping in some would make it more composable

[glennsl]

nullToNone already exists, it's called nullable (edit: actually nullable returns Js.null('a), so you'd have to then convert it to an option, but basically the same thing). There's also nullAs which can be used with either or oneOf.

exceptionToNone is of course optional. I don't think exceptionToNone is a better name though, for several reasons: It should only catch specific exceptions, not all of them, as this name suggests, and it only describes the error path (as does nullToNone). Something like try would perhaps be good, but that's of course a reserved keyword.

optionalField is what @ncthbrt proposed, and is definitely an option, but as I said earlier I'd really like a more general solution if possible.

[RasmusKlett]

nullToNone already exists, it's called nullable (edit: actually nullable returns Js.null('a), so you'd have to then convert it to an option, but basically the same thing). There's also nullAs which can be used with either or oneOf.

Well yes, but I think returning Option instead of Js.null is crucial. After all, this library is for getting rid of the Js types, and getting nice OCaml types instead. Whether it is called nullable or nullToNone doesn't matter to me.

exceptionToNone is of course optional. I don't think exceptionToNone is a better name though, for several reasons: It should only catch specific exceptions, not all of them, as this name suggests, and it only describes the error path (as does nullToNone). Something like try would perhaps be good, but that's of course a reserved keyword.

Good point that it only catches DecodeError, that should also be expressed. But my main gripe is exactly that the name is optional, leading to overuse because it seems like the sensible thing to use for producing Option types. In my opinion the name should be verbose, to indicate its niche use. Maybe tryDecode or decodeOrNone?

optionalField is what @ncthbrt proposed, and is definitely an option, but as I said earlier I'd really like a more general solution if possible.

I'm unsure what direction you want to generalize it in? I think if you want to generalize checking for undefinedness, you will have to change the definition of field to make it more composable. I guess you could have field : string -> Js.Json.t, making my earlier examples something like:

1. Handling value which may be null: field("x") |> nulllToNone(int)
2. Field which may be undefined, but always has a value when present: field("x") |> undefinedToNone(int)
3. Field which may be undefined, and value may be null field("x") |> nullToNone(undefinedToNone(int))

I'm not sure if this is an improvement? In this way undefinedToNone is a normal decoder, and may be used for other undefined values. But field no longer produces a decoder.
On a side note, I think my code in example 3 in both cases actually returns a nested option, which should be flattened.

[glennsl]

Well yes, but I think returning Option instead of Js.null is crucial.

null does not necessarily have the same meaning as None. If it isn't distinguished, None could mean either null, undefined or field missing.

After all, this library is for getting rid of the Js types, and getting nice OCaml types instead.

No...? It's for decoding json into a usefully typed data structure. Whether that structure should include js types or not is a choice left to the consumer.

In my opinion the name should be verbose, to indicate its niche use. Maybe tryDecode or decodeOrNone?

I'm not a fan of redundant verbosity. That it's a decode operation is obvious from the context. decodeOrNone is also not accurate, as it wraps the success case too.

I'm unsure what direction you want to generalize it in?

As it is now, every decoder follows the same simple "shape". They either decode successfully or they fail with an exception. By "general solution" I mean one which preferably conforms to this shape, or alternatively changes the overall shape into something that better captures the properties we need, but still remains uniform. optionalField does not, as it can fail in several different ways.

I like field : string -> Js.Json.t -> Js.Json.t. It does kind of fit the existing shape (it just "decodes" to a Js.Json.t instead of something more specific), is more flexible, and there isn't any issues I can see immediately at least. It does break the current API, of course, but this along with more detailed exceptions could make for a good 2.0 API. I'd like to play with this a bit when I get some time.

[RasmusKlett]

@glennsl Well, I don't agree with a lot of your response. I guess I have different ideas for how a library like this should look. I'm glad you like the field idea; I'm gonna stop trying to impose my ideas on your library now ;)

[snird]

Hi, wanted to note here I just bumped my head against this issue too.
I generally didn't expect optional to swallow all underlying exceptions.
I too think that optional should be None only for undefined and null values, and for everything else throwing the exception.
But anyway, even if it is decided to keep it as it is, I think a documentation update to reflect more clearly what optional is doing is needed.