Decoders that accumulate errors #85

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
5 participants
@travisbrown
Member

travisbrown commented Nov 3, 2015

The implementation here is super rough and incomplete, but I want to see what people think about the general idea and API.

Current usage of Decoder is completely unchanged: if decoding fails, it will do so immediately, with a single error, and no further processing will be performed. The difference is that Decoder now has an accumulating method that will return a new kind of decoder that can accumulate errors in a ValidatedNel:

import cats._, cats.data._, cats.std.all._, cats.syntax.all._
import io.circe._, io.circe.generic.auto._, io.circe.jawn.parse

val Xor.Right(emptyJson) = parse("{}")

case class Foo(i: Int, s: String, c: Char)

val decodeFooAcc: AccumulatingDecoder[Foo] = Decoder[Foo].accumulating
val errors = decodeFooAcc(emptyJson.hcursor).fold(_.toList, _ => Nil)

And then:

scala> errors.foreach(error => println(error.getMessage))
Attempt to decode value on failed cursor: El(DownField(c),false)
Attempt to decode value on failed cursor: El(DownField(s),false)
Attempt to decode value on failed cursor: El(DownField(i),false)

The basic idea is that Decoder is monadic, returns its results in an Xor, and fails fast, while AccumulatingDecoder (which is not formally related to Decoder, although you can always create one from a Decoder with accumulating) is applicative, returns a Validated, and accumulates errors.

This means that if you build up a Decoder using applicative operations instead of monadic ones, you'll get an accumulating decoder from accumulating, but currently-supported usage (json.as[Foo], etc.) will never show a difference. For example:

import cats._, cats.data._, cats.std.all._, cats.syntax.all._
import io.circe._, io.circe.jawn.parse

val monadic = for {
  i <- Decoder.instance(_.get[Int]("i"))
  s <- Decoder.instance(_.get[String]("s"))
} yield (i, s)

val applicative = (
  Decoder.instance(_.get[Int]("i")) |@|
  Decoder.instance(_.get[String]("s"))
).tupled

Now monadic and applicative will always produce the same results, but monadic.accumulating and applicative.accumulating won't (the latter may contain multiple errors).

Is this a terrible idea? Does anyone have ideas about a better way to support error accumulation when needed while keeping the default monadic, fail-fast behavior?

@kaiserpelagic

This comment has been minimized.

Show comment
Hide comment
@kaiserpelagic

kaiserpelagic Nov 3, 2015

+1
This is a feature of Argonaut that I've always wanted, i.e. an Applicative that accumulates validation. The api design you propose looks fine to me.

+1
This is a feature of Argonaut that I've always wanted, i.e. an Applicative that accumulates validation. The api design you propose looks fine to me.

@mandubian

This comment has been minimized.

Show comment
Hide comment
@mandubian

mandubian Nov 3, 2015

Contributor

I feel like the default behavior is the applicative but monadic behavior can be cool sometimes to fail fast...

Here is a weird idea that may not work at all...

What if you built your Decoders as a lazy data structure using an Applicative semantics by default...
Applicative has this cool aspect that you know everything about it at compile time...
Then you can upgrade it from Applicative into Monad by re-compiling it...

ex:

decoderA |@| decoderB
=> Apply(decoderB, Map(decoderA, f: A => B => (A, B)) : Apply[(A, B)]

decoderA |@| decoderB |@| decoderC
=> Apply(decoderC, Map(Apply(decoderB, Map(decoderA, f: A => B => (A, B)))), g: (A, B) => C => Foo)

decoder.monadify
=> Flatmap(decoderA, a => Flatmap(decoderB, b => Flatmap(decoderC, c => g(f(a)(b))(c))))

A monad is an applicative but hasn't the same behavior...
If you have a Monad by default and want to downgrade it into Applicative, it's not really possible as it has a runtime aspect that you don't know at compile time (even if you know it in your code)...

Contributor

mandubian commented Nov 3, 2015

I feel like the default behavior is the applicative but monadic behavior can be cool sometimes to fail fast...

Here is a weird idea that may not work at all...

What if you built your Decoders as a lazy data structure using an Applicative semantics by default...
Applicative has this cool aspect that you know everything about it at compile time...
Then you can upgrade it from Applicative into Monad by re-compiling it...

ex:

decoderA |@| decoderB
=> Apply(decoderB, Map(decoderA, f: A => B => (A, B)) : Apply[(A, B)]

decoderA |@| decoderB |@| decoderC
=> Apply(decoderC, Map(Apply(decoderB, Map(decoderA, f: A => B => (A, B)))), g: (A, B) => C => Foo)

decoder.monadify
=> Flatmap(decoderA, a => Flatmap(decoderB, b => Flatmap(decoderC, c => g(f(a)(b))(c))))

A monad is an applicative but hasn't the same behavior...
If you have a Monad by default and want to downgrade it into Applicative, it's not really possible as it has a runtime aspect that you don't know at compile time (even if you know it in your code)...

@travisbrown

This comment has been minimized.

Show comment
Hide comment
@travisbrown

travisbrown Nov 3, 2015

Member

@mandubian It's just so handy to be able to use flatMap / for-comprehensions in cases where you don't care about accumulation.

Member

travisbrown commented Nov 3, 2015

@mandubian It's just so handy to be able to use flatMap / for-comprehensions in cases where you don't care about accumulation.

@mandubian

This comment has been minimized.

Show comment
Hide comment
@mandubian

mandubian Nov 3, 2015

Contributor

So you need to build it in a monadic flow keeping track that it was an applicative that has been monadified 👯

Contributor

mandubian commented Nov 3, 2015

So you need to build it in a monadic flow keeping track that it was an applicative that has been monadified 👯

@travisbrown

This comment has been minimized.

Show comment
Hide comment
@travisbrown

travisbrown Nov 3, 2015

Member

@mandubian I actually have a rough sketch of a flatMap macro that rewrites itself using ap wherever possible. 😄

In this case, though, I think there's a value in providing monadic binding even apart from the syntactic convenience.

Member

travisbrown commented Nov 3, 2015

@mandubian I actually have a rough sketch of a flatMap macro that rewrites itself using ap wherever possible. 😄

In this case, though, I think there's a value in providing monadic binding even apart from the syntactic convenience.

@julienrf

This comment has been minimized.

Show comment
Hide comment
@julienrf

julienrf Nov 3, 2015

Why not have both flatMap and ap in Decoder, so that you can use whatever style (monadic or applicative) you prefer?

julienrf commented Nov 3, 2015

Why not have both flatMap and ap in Decoder, so that you can use whatever style (monadic or applicative) you prefer?

@travisbrown

This comment has been minimized.

Show comment
Hide comment
@travisbrown

travisbrown Nov 3, 2015

Member

@julienrf That's currently what this change does, with apply returning an Xor[DecodingFailure, A] and decodeAccumulating returning a ValidationNel[DecodingFailure, A].

The problem is that the fact that decodeAccumulating returns different things depending on whether you construct your Decoder with ap or flatMap means that the monad instance for Decoder isn't exactly lawful. The idea behind AccumulatingDecoder is to minimize this unpleasantness as much as possible—AccumulatingDecoder is a perfectly legitimate non-monadic error-accumulating type.

Member

travisbrown commented Nov 3, 2015

@julienrf That's currently what this change does, with apply returning an Xor[DecodingFailure, A] and decodeAccumulating returning a ValidationNel[DecodingFailure, A].

The problem is that the fact that decodeAccumulating returns different things depending on whether you construct your Decoder with ap or flatMap means that the monad instance for Decoder isn't exactly lawful. The idea behind AccumulatingDecoder is to minimize this unpleasantness as much as possible—AccumulatingDecoder is a perfectly legitimate non-monadic error-accumulating type.

@julienrf

This comment has been minimized.

Show comment
Hide comment
@julienrf

julienrf Nov 4, 2015

Ok, I would say that I’d like to have juste one method, apply, whose the result type would be Xor[OneAnd[DecodingFailure], A] and errors would be accumulated if the decoder had been built with ap.

julienrf commented Nov 4, 2015

Ok, I would say that I’d like to have juste one method, apply, whose the result type would be Xor[OneAnd[DecodingFailure], A] and errors would be accumulated if the decoder had been built with ap.

@mandubian

This comment has been minimized.

Show comment
Hide comment
@mandubian

mandubian Nov 4, 2015

Contributor

but nothing prevents users from building with flatMap and in the middle of
it use a Decoder built with ap, right?

On Wed, Nov 4, 2015 at 9:26 AM, Julien Richard-Foy <notifications@github.com

wrote:

Ok, I would say that I’d like to have juste one method, apply, whose the
result type would be Xor[OneAnd[DecodingFailure], A] and errors would be
accumulated if the decoder had been built with ap.


Reply to this email directly or view it on GitHub
#85 (comment).

Contributor

mandubian commented Nov 4, 2015

but nothing prevents users from building with flatMap and in the middle of
it use a Decoder built with ap, right?

On Wed, Nov 4, 2015 at 9:26 AM, Julien Richard-Foy <notifications@github.com

wrote:

Ok, I would say that I’d like to have juste one method, apply, whose the
result type would be Xor[OneAnd[DecodingFailure], A] and errors would be
accumulated if the decoder had been built with ap.


Reply to this email directly or view it on GitHub
#85 (comment).

@julienrf

This comment has been minimized.

Show comment
Hide comment

julienrf commented Nov 4, 2015

No.

@travisbrown

This comment has been minimized.

Show comment
Hide comment
@travisbrown

travisbrown Nov 4, 2015

Member

@julienrf But suppose we're decoding List[Item]. If we're getting the JSON from a service, it's likely that we want fail-fast behavior, and that we want to stop processing as soon as something goes wrong. If the JSON is from something a human submitted through a form, wrote in a spreadsheet, etc., it's more likely that we want to accumulate errors so that we can display them all to the human at once.

If there's only one apply, then we have to choose between these two options when we write our generic List[A: Decoder] decoder.

Member

travisbrown commented Nov 4, 2015

@julienrf But suppose we're decoding List[Item]. If we're getting the JSON from a service, it's likely that we want fail-fast behavior, and that we want to stop processing as soon as something goes wrong. If the JSON is from something a human submitted through a form, wrote in a spreadsheet, etc., it's more likely that we want to accumulate errors so that we can display them all to the human at once.

If there's only one apply, then we have to choose between these two options when we write our generic List[A: Decoder] decoder.

WIP
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Dec 12, 2015

Current coverage is 72.81%

Merging #85 into master will decrease coverage by -2.37% as of bcea97f

Powered by Codecov. Updated on successful CI builds.

Current coverage is 72.81%

Merging #85 into master will decrease coverage by -2.37% as of bcea97f

Powered by Codecov. Updated on successful CI builds.

@travisbrown

This comment has been minimized.

Show comment
Hide comment
@travisbrown

travisbrown Dec 17, 2015

Member

Superseded by (and included in #139).

Member

travisbrown commented Dec 17, 2015

Superseded by (and included in #139).

crispywalrus pushed a commit to flyingwalrusllc/circe that referenced this pull request May 16, 2016

Merge pull request #85 from bobbyrauchenberg/issue59
Additional documentation on Applicative. fixes #59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment