Merge pull request #165 from Sphonic/improve-decode

improve DecodeRequest API to return Try instead of Option

.gitignore

@@ -2,4 +2,8 @@
 target/
 .idea/
 .idea_modules/
-.DS_STORE
+.DS_STORE
+.cache
+.settings
+.project
+.classpath

argonaut/src/main/scala/io/finch/argonaut/package.scala

@@ -19,26 +19,29 @@
  *
  * Contributor(s):
  * Ryan Plessner
+ * Jens Halm
  */
 
 package io.finch
 
 import _root_.argonaut.{EncodeJson, Json, Parse, DecodeJson}
 import io.finch.request.DecodeRequest
+import io.finch.request.RequestReaderError
 import io.finch.response.EncodeResponse
+import com.twitter.util.{Try, Throw, Return}
 
 package object argonaut {
 
   /**
    * @param decode The argonaut ''DecodeJson'' to use for decoding
    * @tparam A The type of data that the ''DecodeJson'' will decode into
-   * @return Create a Finch ''DecodeJson'' from an argonaut ''DecodeJson''
+   * @return Create a Finch ''DecodeRequest'' from an argonaut ''DecodeJson''
    */
   implicit def toArgonautDecode[A](implicit decode: DecodeJson[A]): DecodeRequest[A] = new DecodeRequest[A] {
-    override def apply(json: String): Option[A] = Parse.parseOption(json) match {
-      case None => None
-      case Some(blob: Json) => decode.decodeJson(blob).toOption
-    }
+    override def apply(json: String): Try[A] = Parse.decodeEither(json).fold(
+      error => Throw(new RequestReaderError(error)),
+      Return(_)
+    )
   }
 
   /**

argonaut/src/test/scala/io/finch/argonaut/ArgonautSpec.scala

@@ -4,7 +4,7 @@ import argonaut.Argonaut._
 import argonaut._
 import com.twitter.finagle.Service
 import com.twitter.finagle.httpx.Request
-import com.twitter.util.Await
+import com.twitter.util.{Await,Return}
 import io.finch._
 import io.finch.request.RequiredBody
 import io.finch.response.TurnIntoHttp
@@ -36,15 +36,15 @@ class ArgonautSpec extends FlatSpec with Matchers {
   val exampleUser = TestUser(42, "bob")
 
   "An ArgonautDecode" should "decode json string into a data structure" in {
-    toArgonautDecode(testUserCodec)(str) shouldEqual Option(exampleUser)
+    toArgonautDecode(testUserCodec)(str) shouldEqual Return(exampleUser)
   }
 
-  it should "return None if the string is not valid json" in {
-    toArgonautDecode(testUserCodec)(badJson) shouldEqual None
+  it should "fail if the string is not valid json" in {
+    toArgonautDecode(testUserCodec)(badJson).isThrow shouldEqual true
   }
 
-  it should "return None if the decoder could not decode the string into data" in {
-    toArgonautDecode(testUserCodec)(invalidStructure) shouldEqual None
+  it should "fail if the decoder could not decode the string into data" in {
+    toArgonautDecode(testUserCodec)(invalidStructure).isThrow shouldEqual true
   }
 
   it should "be compatible with finch-core's requests" in {
@@ -66,4 +66,4 @@ class ArgonautSpec extends FlatSpec with Matchers {
     val result = Await.result(service(exampleUser))
     result.getContentString() shouldEqual str
   }
-}
+}

core/src/main/scala/io/finch/request/package.scala

@@ -27,7 +27,7 @@
 package io.finch
 
 import com.twitter.finagle.httpx.Cookie
-import com.twitter.util.{Future, Return, Throw}
+import com.twitter.util.{Future, Return, Throw, Try}
 
 import scala.reflect.ClassTag
 
@@ -923,9 +923,13 @@ package object request {
    * and decodes it, according to an implicit decoder, into an ''Option''.
    */
   object OptionalBody {
-    def apply[A](implicit m: DecodeMagnet[A]): RequestReader[Option[A]] = for {
-      b <- OptionalStringBody
-    } yield b flatMap { m()(_) }
+    def apply[A](implicit m: DecodeMagnet[A]): RequestReader[Option[A]] = OptionalStringBody.flatMap {
+      case Some(body) => new RequestReader[Option[A]] {
+        def apply[Req](req: Req)(implicit ev: Req => HttpRequest) = Future.const(m()(body) map (Some(_)))
+        override def toString: String = "Optional body"
+      }
+      case None => ConstReader(Future.None)
+    }
 
     // TODO: Make it accept `Req` instead
     def apply[A](req: HttpRequest)(implicit m: DecodeMagnet[A]): Future[Option[A]] =
@@ -942,7 +946,7 @@ package object request {
         def apply[Req](req: Req)(implicit ev: Req => HttpRequest) = body.toFuture
         override def toString: String = "Required body"
       }
-      case None => ConstReader(BodyNotParsed.toFutureException)
+      case None => ConstReader(BodyNotFound.toFutureException)
     }
     // TODO: Make it accept `Req` instead
     def apply[A](req: HttpRequest)(implicit m: DecodeMagnet[A]): Future[A] = RequiredBody[A](m)(req)
@@ -994,14 +998,14 @@ package object request {
    * An abstraction that is responsible for decoding the request of type ''A''.
    */
   trait DecodeRequest[+A] {
-    def apply(req: String): Option[A]
+    def apply(req: String): Try[A]
   }
 
   /**
    * An abstraction that is responsible for decoding the request of general type.
    */
   trait DecodeAnyRequest {
-    def apply[A: ClassTag](req: String): Option[A]
+    def apply[A: ClassTag](req: String): Try[A]
   }
 
   /**
@@ -1025,7 +1029,7 @@ package object request {
   implicit def magnetFromAnyDecode[A](implicit d: DecodeAnyRequest, tag: ClassTag[A]): DecodeMagnet[A] =
     new DecodeMagnet[A] {
       def apply(): DecodeRequest[A] = new DecodeRequest[A] {
-        def apply(req: String): Option[A] = d(req)(tag)
+        def apply(req: String): Try[A] = d(req)(tag)
       }
     }
 

core/src/test/scala/io/finch/request/BodySpec.scala

@@ -25,7 +25,7 @@ package io.finch.request
 
 import com.twitter.finagle.httpx.Request
 import com.twitter.io.Buf.ByteArray
-import com.twitter.util.{Await, Future}
+import com.twitter.util.{Await, Future, Try}
 import io.finch.HttpRequest
 import org.jboss.netty.handler.codec.http.HttpHeaders
 import org.scalatest.{FlatSpec, Matchers}
@@ -101,8 +101,7 @@ class BodySpec extends FlatSpec with Matchers {
 
   "RequiredBody and OptionalBody" should "work with no request type available" in {
     implicit val decodeInt = new DecodeRequest[Int] {
-       def apply(req: String): Option[Int] =
-         try Some(req.toInt) catch { case _: NumberFormatException => None }
+       def apply(req: String): Try[Int] = Try(req.toInt)
     }
     val req = requestWithBody("123")
     val ri: RequestReader[Int] = RequiredBody[Int]
@@ -118,8 +117,7 @@ class BodySpec extends FlatSpec with Matchers {
 
   it should "work with custom request and its implicit view to HttpRequest" in {
     implicit val decodeDouble = new DecodeRequest[Double] { // custom encoder
-      def apply(req: String): Option[Double] =
-        try Some(req.toDouble) catch { case _: NumberFormatException => None }
+      def apply(req: String): Try[Double] = Try(req.toDouble)
     }
     case class CReq(http: HttpRequest) // custom request
     implicit val cReqEv = (req: CReq) => req.http // implicit view
@@ -135,6 +133,22 @@ class BodySpec extends FlatSpec with Matchers {
     Await.result(od(req)) shouldBe Some(42.0)
     Await.result(o) shouldBe Some(42.0)
   }
+
+  it should "fail if the decoding of the body fails" in {
+    implicit val decodeInt = new DecodeRequest[Int] {
+       def apply(req: String): Try[Int] = Try(req.toInt)
+    }
+    val req = requestWithBody("foo")
+    val ri: RequestReader[Int] = RequiredBody[Int]
+    val i: Future[Int] = RequiredBody[Int](req)
+    val oi: RequestReader[Option[Int]] = OptionalBody[Int]
+    val o: Future[Option[Int]] = OptionalBody[Int](req)
+
+    a [NumberFormatException] should be thrownBy Await.result(ri(req))
+    a [NumberFormatException] should be thrownBy Await.result(i)
+    a [NumberFormatException] should be thrownBy Await.result(oi(req))
+    a [NumberFormatException] should be thrownBy Await.result(o)
+  }
 
   private[this] def requestWithBody(body: String): HttpRequest = {
     requestWithBody(body.getBytes("UTF-8"))

core/src/test/scala/io/finch/request/DecodeSpec.scala

@@ -23,16 +23,18 @@
 package io.finch.request
 
 import org.scalatest.{Matchers, FlatSpec}
+import com.twitter.util.{Try,Return}
 import scala.math._
 
 class DecodeSpec extends FlatSpec with Matchers {
 
-  private def decode[A](json: String)(implicit d: DecodeRequest[A]): Option[A] = d(json)
+  private def decode[A](json: String)(implicit d: DecodeRequest[A]): Try[A] = d(json)
+
   "A DecodeJson" should "be accepted as implicit instance of superclass" in {
     implicit object BigDecimalJson extends DecodeRequest[BigDecimal] {
-      def apply(s: String): Option[BigDecimal] = Some(BigDecimal(s))
+      def apply(s: String): Try[BigDecimal] = Try(BigDecimal(s))
     }
 
-    decode[ScalaNumber]("12345.25") shouldBe Some(BigDecimal(12345.25))
+    decode[ScalaNumber]("12345.25") shouldBe Return(BigDecimal(12345.25))
   }
 }

docs.md

@@ -297,13 +297,14 @@ An HTTP body may be fetched from the HTTP request using the following readers:
 Finch supports pluggable request decoders. In fact, any type `A` my be read from the request body using either:
 `io.finch.request.RequiredBody[A]` or `io.finch.request.OptionalBody[A]` if there is an implicit value of type 
 `DecodeRequest[A]` available in the scope. The `DecodeRequest[A]` abstraction may be described as function 
-`String => Option[A]`. Thus, any decoders may be easily defined to use the functionality of body readers. Note, that 
+`String => Try[A]`. Thus, any decoders may be easily defined to use the functionality of body readers. Note, that 
 the body type (i.e., `Double`) should be always explicitly defined for both `RequiredBody` and `OptionalBody`.
 
 ```
 implicit val decodeDouble = new DecodeRequest[Double] {
-  def apply(s: String): Option[Double] =
-    try { Some(s.toDouble) } catch { case _: NumberFormatException => None }
+  
+  def apply(s: String): Try[Double] = Try { s.toDouble }
+  
 }
 val req: HttpRequest = ???
 val readDouble: RequestReader[Double] = RequiredBody[Double]

jackson/src/main/scala/io/finch/jackson/package.scala

@@ -25,16 +25,17 @@ package io.finch
 import com.fasterxml.jackson.databind.ObjectMapper
 import io.finch.request.DecodeAnyRequest
 import io.finch.response.EncodeAnyResponse
+import com.twitter.util.Try
 
 import scala.reflect.ClassTag
 
 package object jackson {
 
   implicit def decodeJackson(implicit mapper: ObjectMapper) = new DecodeAnyRequest {
-    def apply[A: ClassTag](s: String): Option[A] = try {
+    def apply[A: ClassTag](s: String): Try[A] = Try {
       val clazz = implicitly[ClassTag[A]].runtimeClass.asInstanceOf[Class[A]]
-      Some(mapper.readValue[A](s, clazz))
-    } catch { case _: Exception => None }
+      mapper.readValue[A](s, clazz)
+    }
   }
 
   implicit def encodeJackson(implicit mapper: ObjectMapper) = new EncodeAnyResponse {

jackson/src/test/scala/io/finch/jackson/JacksonSpec.scala

@@ -25,7 +25,7 @@ import com.fasterxml.jackson.databind.ObjectMapper
 import com.fasterxml.jackson.module.scala.DefaultScalaModule
 import com.twitter.finagle.httpx.Request
 import com.twitter.io.Buf.Utf8
-import com.twitter.util.{Await, Future}
+import com.twitter.util.{Await, Future, Return}
 import io.finch.request.{OptionalBody, RequiredBody, RequestReader}
 import io.finch.response.Ok
 import org.jboss.netty.handler.codec.http.HttpHeaders
@@ -42,7 +42,13 @@ class JacksonSpec extends FlatSpec with Matchers {
   it should "decode a case class from JSON" in {
     val json = "{\"bar\":\"bar\",\"baz\":20}"
     val decode = decodeJackson(objectMapper)
-    decode[Foo](json) shouldBe Some(Foo("bar", 20))
+    decode[Foo](json) shouldBe Return(Foo("bar", 20))
+  }
+
+  it should "fail given invalid JSON" in {
+    val json = "{\"bar\":\"bar\",\"baz\":20}"
+    val decode = decodeJackson(objectMapper)
+    decode[Foo]("{{{{").isThrow shouldBe true
   }
 
   it should "work w/o exceptions with ResponseBuilder" in {
@@ -56,7 +62,7 @@ class JacksonSpec extends FlatSpec with Matchers {
     val decode = decodeJackson(objectMapper)
 
     encode(list) shouldBe "[1,2,3]"
-    decode[List[Int]]("[1,2,3]") shouldBe Some(List(1, 2, 3))
+    decode[List[Int]]("[1,2,3]") shouldBe Return(List(1, 2, 3))
   }
 
   it should "work w/o exceptions with RequestReader" in {

jawn/src/main/scala/io/finch/jawn/pacakge.scala

@@ -27,6 +27,8 @@ import _root_.jawn.ast.{CanonicalRenderer, JValue}
 import _root_.jawn.{Facade, Parser}
 import io.finch.request.DecodeRequest
 import io.finch.response.EncodeResponse
+import com.twitter.util.{Try, Return, Throw}
+import scala.util.{Success, Failure}
 
 /**
  * This package provides support for use of the Jawn json parsing library in Finch.io.
@@ -40,7 +42,10 @@ package object jawn {
    *
    */
   implicit def toJawnDecode[J](implicit facade: Facade[J]): DecodeRequest[J] = new DecodeRequest[J] {
-    def apply(json: String): Option[J] = Parser.parseFromString(json).toOption
+    def apply(json: String): Try[J] = Parser.parseFromString(json) match {
+      case Success(value) => Return(value)
+      case Failure(error) => Throw(error)
+    }
   }
 
   /**

jawn/src/test/scala/io/finch/jawn/JawnSpec.scala

@@ -36,8 +36,12 @@ class JawnSpec extends FlatSpec with Matchers {
   "A DecodeJawn" should "parse valid json into its ast" in {
     toJawnDecode(facade)(str).foreach(v => v.shouldBe(jsVal))
   }
+
+  it should "fail given invalid JSON" in {
+    toJawnDecode(facade)("{{{{").isThrow shouldBe true
+  }
 
   "An EncodeJawn" should "render a valid JValue as a string" in {
     EncodeJawn(jsVal) == str
   }
-}
+}

json/src/main/scala/io/finch/json/package.scala

@@ -25,6 +25,8 @@ package io.finch
 
 import io.finch.request.DecodeRequest
 import io.finch.response.EncodeResponse
+import com.twitter.util.{Try, Throw, Return}
+import io.finch.request.BodyNotParsed
 
 package object json {
   implicit val encodeFinchJson = new EncodeResponse[Json] {
@@ -34,6 +36,7 @@ package object json {
   }
 
   implicit val decodeFinchJson = new DecodeRequest[Json] {
-    def apply(json: String): Option[Json] = Json.decode(json)
+    def apply(json: String): Try[Json] = Json.decode(json)
+      .fold[Try[Json]](Throw(BodyNotParsed))(Return(_)) // TODO - error detail is swallowed
   }
 }