New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error messages lost for recusive decoders #643

Closed
EdgeCaseBerg opened this Issue May 23, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@EdgeCaseBerg

EdgeCaseBerg commented May 23, 2017

When using emap to create a custom validation for a recusive decoder the error messages are lost for the parts of the object which are decoded recursively:

import io.circe._, io.circe.generic.semiauto._, io.circe.generic.decoding.DerivedDecoder, io.circe.generic.encoding.DerivedObjectEncoder

case class A(i: Int, l: List[A])

def validA(a: A): Boolean = {
    a.i > 0 && a.l.map(validA).foldLeft(true)(_ && _)
}

implicit val ae: Decoder[A] = deriveDecoder[A].emap { a =>
    if(!validA(a)) {
        Left("Bad!")
    } else {
        Right(a)
    }
}

val s = """{"i":1, "l": [{"i": -1, "l": []}, {"i": 2, "l": []}]}"""

parser.parse(s).right.get.as[A]

When running the last line of code you get

io.circe.Decoder.Result[A] = Left(DecodingFailure([A]List[A], List(DownArray, DownField(l))))

Which is a valid decoding failure, but the error message of [A]List[A] isn't what is expected. I would expect Bad! or something similar to DecodingFailure at .l[0]: Bad!

I'm using circe 0.7.0 for reference. And this issue was originally raised in the gitter channel

@EdgeCaseBerg

This comment has been minimized.

EdgeCaseBerg commented May 26, 2017

I came up with another example, not sure if it will help. But this one doesn't use emap and instead tries to be explicit about which decoder to use during the decoding of the children, but it still results in the same issue:

import io.circe._, io.circe.generic.semiauto._, io.circe.generic.decoding.DerivedDecoder, io.circe.generic.encoding.DerivedObjectEncoder

case class A(id: Int, children: List[A])

import cats.syntax.either._

implicit val decoder: Decoder[A] = Decoder.instance { hCursor =>
	val idDecodingResult = hCursor.downField("id").as[Int] match {
		case Right(id) if id > 0 => Right[DecodingFailure, Int](id)
		case Right(id) => Left(DecodingFailure("Id must be greater than 0", hCursor.history))
		case l => l
	}
	val childrenResult = hCursor.downField("children").as[List[A]](Decoder.decodeList(decoder))
	for {
		id <- idDecodingResult
		children <- childrenResult
	} yield A(id, children)
}

parser.parse("""{"id": 1, "children": [{"id" : 2, "children": []},{"id": -2, "children": []}]}""").right.get.as[A]

ends up with

res0: io.circe.Decoder.Result[A] = Left(DecodingFailure([A]List[A], List(MoveRight, DownArray, DownField(children))))

I did this one using circe 0.8.0 so it looks like it's not something that was specific to 0.7.0

@EdgeCaseBerg

This comment has been minimized.

EdgeCaseBerg commented May 26, 2017

It looks like that specific error message pops up from this line of code I think

@EdgeCaseBerg

This comment has been minimized.

EdgeCaseBerg commented May 29, 2017

I found a work around based on that line I mentioned above

import io.circe._, io.circe.generic.semiauto._, io.circe.generic.decoding.DerivedDecoder, io.circe.generic.encoding.DerivedObjectEncoder

case class A(id: Int, children: Vector[A])


implicit def vectorADecoder[A: Decoder]: Decoder[Vector[A]] = Decoder.decodeCanBuildFrom[A, Vector]

import cats.syntax.either._

implicit val decoder: Decoder[A] = Decoder.instance { hCursor =>
     val idDecodingResult = hCursor.downField("id").as[Int] match {
          case Right(id) if id > 0 => Right[DecodingFailure, Int](id)
          case Right(id) => Left(DecodingFailure("Id must be greater than 0", hCursor.history))
          case l => l
     }
     val childrenResult = hCursor.downField("children").as[Vector[A]]
     for {
          id <- idDecodingResult
          children <- childrenResult
     } yield A(id, children)
}

scala> parser.parse("""{"id": 1, "children": [{"id" : 2, "children": []},{"id": -2, "children": []}]}""").right.get.as[A]
res0: io.circe.Decoder.Result[A] = Left(DecodingFailure(Id must be greater than 0, List(MoveRight, DownArray, DownField(children))))

@EdgeCaseBerg

This comment has been minimized.

EdgeCaseBerg commented May 29, 2017

It seems like if you didn't want people to have to use this workaround, that maybe those default error messages should be removed? Why do they exist in the first place?

@travisbrown

This comment has been minimized.

Member

travisbrown commented May 29, 2017

@EdgeCaseBerg The decodeList and decodeVector instances are there for people who need to construct List or Vector instances by hand, as a convenience since decodeCanBuildFrom[Whatever, List] is kind of inconvenient. I don't remember exactly why they have an overridden error message—I believe it's just to provide more information than the default "Attempt to decode value on failed cursor". The fact that it's a problem in situations like this means the withErrorMessage should probably be removed, and I think we'd almost certainly accept a PR doing that.

In general the error message story in circe is still a work in progress (see e.g. #306, which has been open a long time). This is a relatively low-priority set of issues for me, since in most of my use cases having a useful path to the error is the most important thing, and there have been other issues I can work on that make circe work better for me personally. I'm always happy to work with anyone who wants to help speed up this effort, though.

@EdgeCaseBerg

This comment has been minimized.

EdgeCaseBerg commented May 30, 2017

The fact that it's a problem in situations like this means the withErrorMessage should probably be removed, and I think we'd almost certainly accept a PR doing that.

I can open one of those later today or this week when I'm not at work. I read #306 and will think on it for a bit. Not sure if I have any other thoughts to add to your initial suggestion about name and message fields. Besides maybe that an easily read path field would be useful, but I think we'd get that with the show on the failure anyway so maybe it's not needed.

Thanks!

cponomaryov added a commit to cponomaryov/circe that referenced this issue May 31, 2017

cponomaryov added a commit to cponomaryov/circe that referenced this issue May 31, 2017

travisbrown added a commit that referenced this issue Jun 1, 2017

Merge pull request #649 from cponomaryov/container-item-error-pass-th…
…rough

decodeList discards error messages from list items #596; Error messages lost for recusive decoders #643
@cponomaryov

This comment has been minimized.

Contributor

cponomaryov commented Jun 1, 2017

I think it is fixed by #649

@EdgeCaseBerg

This comment has been minimized.

EdgeCaseBerg commented Jun 1, 2017

@cponomaryov @travisbrown
So on the next minor release would this issue be marked closed? Just curious how to best stay informed about when I don't need to use my workaround in my code anymore and should update my dependencies versions

@travisbrown

This comment has been minimized.

Member

travisbrown commented Jun 1, 2017

@EdgeCaseBerg Normally I've been closing issues when the fix hits master. If it's really urgent I could backport this for an 0.8.1, but I'm guessing 0.9 is less than a week off and I wouldn't be able to get 0.8.1 out until Saturday anyway.

@travisbrown travisbrown closed this Jun 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment