subset2 icon indicating copy to clipboard operation
subset2 copied to clipboard

Design change: throw exception on parsing when mandatory field is missing or its parsing has failed

Open unoexperto opened this issue 12 years ago • 1 comments

Hi Alexander!

I wanted to hear your opinion on design change. As I'm more and more using your library in production (on two projects now) I noticed repetitive issue. Consider following example.

There case class A that has field of type Option[B] that has number of fields on its own. For example:

case class User(id: ObjectId, fb_info: Option[UserFbInfo])

object User {
  private implicit object fromBson extends BsonFieldWithDoc[User] {
    lazy val Doc: DocParser[User] = oid("_id") ~
      get[UserFbInfo]("fb_info").opt map {
      case id ~ fb_info => User(id, fb_info)
    }
  }
}

case class UserFbInfo(id: Long, token : FbToken)

object UserFbInfo {
  implicit object fromBson extends BsonFieldWithDoc[UserFbInfo] {
    lazy val Doc: DocParser[UserFbInfo] =
      long("id") ~ get[FbToken]("token") map {
        case id ~ token => UserFbInfo(id, token)
      }
  }
}

If somethign goes wrong in parsing of UserFbInfo field User.fb_info becomes None. And since MongoDB is schemaless database it happens quite often in rapidly changing project. Typically it's one of these two cases:

  1. Non-optional field of UserFbInfo doesn't exist in Mongo document
  2. Developer made mistake in parser of FbTokenand forgot to specify correct type in case branch of partial function of Field[FbToken]. Unit was returned instead of FbToken.

It's very hard to catch bug because User.fb_info == None is perfectly legitimate use-case which indicates that user in my system hasn't attached his Facebook account to profile.

What if instead Subset would throw exception on parsing when mandatory field is missing ? Then at least I can catch it either in unit-test or production logs.

Thanks!

unoexperto avatar Aug 10 '13 01:08 unoexperto

.opt is a combinator, it may be applied to quite complex parser and everything this combinator knows is whether the underlying parser succeeded or failed.

But the I understand your problem, it's related to #7 . It's actually about Field not reporting errors and hence a parser cannot distinguish field non-existance from wrong type.

I've prepared a temporary workaround for this issue, please see the commit. However, it should evidently be addressed in more consistent fashion via Field error reporting.

alaz avatar Aug 10 '13 12:08 alaz