Proposed design changes for optional decoders

[travisbrown]

This issue is closely related to #217 and #218, but there are a couple of unresolved underlying questions about what the "correct" behavior should be that I think deserve their own discussion.

Setup

Suppose we have a case class like this:

case class Foo(ab: Option[Int])

And we want to decode a document like this into Foo(Some(1)):

{ "a": { "b": 1 }}

Chains of operations

We can currently write a decoder like this:

import io.circe.Decoder

implicit val decodeFoo: Decoder[Foo] =
  Decoder.instance(_.downField("a").downField("b").as[Option[Int]].map(Foo(_)))

And confirm that it works on valid input:

scala> decode[Foo]("""{ "a": { "b": 1 }}""")
res0: cats.data.Xor[io.circe.Error,Foo] = Right(Foo(Some(1)))

If there's no b field in the inner JSON object we get a None, which I think is pretty reasonable:

scala> decode[Foo]("""{ "a": { "x": 1 }}""")
res1: cats.data.Xor[io.circe.Error,Foo] = Right(Foo(None))

If there is a b field but it's not an integral number, decoding fails (also reasonable to me):

scala> decode[Foo]("""{ "a": { "b": true }}""")
res3: cats.data.Xor[io.circe.Error,Foo] = Left(io.circe.DecodingFailure: Int: El(DownField(b),true),El(DownField(a),true))

If there's no a field in the outer object, we also get a None (somewhat more questionable to my eye, but still I think the right choice):

scala> decode[Foo]("""{ "x": { "b": 1 }}""")
res5: cats.data.Xor[io.circe.Error,Foo] = Right(Foo(None))

The next part is where it gets weird. Suppose we do have an a field, but its value isn't an object:

scala> decode[Foo]("""{ "a": true}""")
res6: cats.data.Xor[io.circe.Error,Foo] = Right(Foo(None))

Or that we don't have an a field at all (this is more or less what #217 reports):

scala> decode[Foo]("true")
res7: cats.data.Xor[io.circe.Error,Foo] = Right(Foo(None))

I think both of these cases should result in decoding failures.

Optional decoders and JSON arrays

A similar batch of issues comes up when our optional decoder uses positional selectors:

implicit val decodeFoo: Decoder[Foo] =
  Decoder.instance(_.downN(0).downN(1).as[Option[Int]].map(Foo(_)))

This is designed to decode JSON like the following as Foo(Some(1)) (it says "optionally decode the second element of the JSON array that's the first element of the focus as an Int"):

[[true, 1], true]

If there is no second element of the inner array, we get a None (fine by me):

scala> decode[Foo]("""[[true], true]""")
res10: cats.data.Xor[io.circe.Error,Foo] = Right(Foo(None))

But we also get a None if the first element isn't a JSON array at all (ugh):

scala> decode[Foo]("""[true, true]""")
res11: cats.data.Xor[io.circe.Error,Foo] = Right(Foo(None))

Proposal

My proposed solution is for optional decoders to consider two kinds of failure. The first kind of failure would be when a navigation operation is appropriate for the current JSON context (array or object), but still fails (either because the array doesn't contain enough elements or the object doesn't contain the key). These failures would result in successful decoding to None—even if they occur at the beginning of a chain of operations.

The second kind of failure is when the navigation operation isn't appropriate for the current JSON value—e.g. we try to downField("a") on a JSON number instead of a JSON object. This kind of mismatch would fail the decoding, not return a None.

This requires a little more bookkeeping and should probably be implemented by adding some information about operations at the CursorOp level, which means the fix for #217 would appear in 0.4.0 and probably wouldn't be backported to 0.3.

Does this sound like a reasonable proposal for the behavior of optional decoders? Is anyone uncomfortable with #217 not getting fixed in the 0.3 series?

[penland365]

This proposal feels 💯 to me. Personally, I would rather have these two types of failures and punt #217 to 0.4.0. Intuitively, a Decoder has two tasks:

1. Decode a Value
2. Move around a JSON object to Decode the next Value

That we should have two distinct Failure types to represent these seems correct to me. I would much prefer this be fleshed out rather than cutting the baby in half to force something in to 0.3.0, but that's just, like, my opinion man. I understand that for some users this may be an actual blocker.

👍

[longcao]

Chiming in here because GitHub reactions have yet to express enough weight for me yet with a 👍. I think you summarized it well with the dichotomy of failure, which is what I expected as user, where we return None for the lack of a presence (undefined, null) and hard fail if the types don't line up correctly.

[travisbrown]

Fixed by #231.