Merge pull request #633 from finagle/vk/use-input-api

Use new Input API internally

benchmarks/src/main/scala/io/finch/benchmarks/BodyBenchmark.scala

@@ -1,21 +1,13 @@
 package io.finch.benchmarks
 
-import com.twitter.finagle.http.Request
 import com.twitter.io.Buf
 import io.finch._
 import org.openjdk.jmh.annotations.{Benchmark, Scope, State}
 
 @State(Scope.Benchmark)
 class BodyBenchmark extends FinchBenchmark {
 
-  val input = {
-    val r = Request()
-    val content = Buf.Utf8("x" * 1024)
-    r.content = content
-    r.contentLength = content.length.toLong
-
-    Input(r)
-  }
+  val input = Input.post("/").withBody(Buf.Utf8("x" * 1024))
 
   @Benchmark
   def stringOption: Option[String] = bodyOption(input).value.get

benchmarks/src/main/scala/io/finch/benchmarks/EndpointBenchmark.scala

@@ -1,6 +1,5 @@
 package io.finch.benchmarks
 
-import com.twitter.finagle.http.Request
 import com.twitter.util.Try
 import io.finch._
 import org.openjdk.jmh.annotations._
@@ -52,21 +51,21 @@ trait FooEndpointsAndRequests {
 
   val derivedFooReader: Endpoint[Foo] = Endpoint.derive[Foo].fromParams
 
-  val goodFooRequest: Input = Input(Request(
+  val goodFooRequest: Input = Input.get("/",
     "s" -> "Man hands on misery to man. It deepens like a coastal shelf.",
     "d" -> "0.234567",
     "i" -> "123456",
     "l" -> "1234567890",
     "b" -> "true"
-  ))
+  )
 
-  val badFooRequest: Input = Input(Request(
+  val badFooRequest: Input = Input.get("/",
     "s" -> "Man hands on misery to man. It deepens like a coastal shelf.",
     "d" -> "0.23h4567",
     "i" -> "123456",
     "l" -> "1234567890x",
     "b" -> "true"
-  ))
+  )
 
   val goodFooResult: Foo = Foo(
     "Man hands on misery to man. It deepens like a coastal shelf.",

benchmarks/src/main/scala/io/finch/benchmarks/ExtractorBenchmark.scala

@@ -1,16 +1,15 @@
 package io.finch.benchmarks
 
-import com.twitter.finagle.http.Request
 import io.finch._
 import org.openjdk.jmh.annotations._
 
 @State(Scope.Benchmark)
 class ExtractorBenchmark extends FinchBenchmark {
 
-  val empty: Input = Input(Request())
-  val fooBarBaz: Input = Input(Request("/foo/bar/baz"))
-  val tenTwenty: Input = Input(Request("/10/20"))
-  val trueFalse: Input = Input(Request("/true/false"))
+  val empty: Input = Input.get("/")
+  val fooBarBaz: Input = Input.get("/foo/bar/baz")
+  val tenTwenty: Input = Input.get("/10/20")
+  val trueFalse: Input = Input.get("/true/false")
 
   @Benchmark
   def stringSome: Option[String] = string(fooBarBaz).value

benchmarks/src/main/scala/io/finch/benchmarks/MatcherBenchmark.scala

@@ -1,15 +1,14 @@
 package io.finch.benchmarks
 
-import com.twitter.finagle.http.Request
 import io.finch._
 import org.openjdk.jmh.annotations._
 import shapeless.HNil
 
 @State(Scope.Benchmark)
 class MatcherBenchmark extends FinchBenchmark {
 
-  val empty: Input = Input(Request())
-  val fooBarBaz: Input = Input(Request("/foo/bar/baz"))
+  val empty: Input = Input.get("/")
+  val fooBarBaz: Input = Input.get("/foo/bar/baz")
 
   val foo: Endpoint0 = "foo"
 

core/src/main/scala/io/finch/Endpoint.scala

@@ -4,7 +4,6 @@ import scala.reflect.ClassTag
 
 import cats.Alternative
 import cats.data.NonEmptyList
-import cats.instances.list._
 import com.twitter.finagle.Service
 import com.twitter.finagle.http.{Cookie, Request, Response, Status}
 import com.twitter.util.{Future, Return, Throw, Try}
@@ -286,7 +285,7 @@ trait Endpoint[A] { self =>
 
     private[this] val safeEndpoint = self.handle(basicEndpointHandler)
 
-    def apply(req: Request): Future[Response] = safeEndpoint(Input(req)) match {
+    def apply(req: Request): Future[Response] = safeEndpoint(Input.request(req)) match {
       case Some((remainder, output)) if remainder.isEmpty =>
         output.map(oa => oa.toResponse[CT](req.version)).run
       case _ => Future.value(Response(req.version, Status.NotFound))

core/src/main/scala/io/finch/Input.scala

@@ -1,10 +1,11 @@
 package io.finch
 
-import java.nio.charset.Charset
-
 import cats.Eq
-import com.twitter.finagle.http.{Method, Request, RequestBuilder}
-import com.twitter.io.Buf
+import com.twitter.finagle.http.{MediaType, Method, Request}
+import com.twitter.finagle.netty3.ChannelBufferBuf
+import com.twitter.io.{Buf, ConcatBuf}
+import org.jboss.netty.handler.codec.http.{DefaultHttpRequest, HttpMethod, HttpRequest, HttpVersion}
+import org.jboss.netty.handler.codec.http.multipart.{DefaultHttpDataFactory, HttpPostRequestEncoder}
 
 /**
  * An input for [[Endpoint]].
@@ -14,56 +15,101 @@ final case class Input(request: Request, path: Seq[String]) {
   def drop(n: Int): Input = copy(path = path.drop(n))
   def isEmpty: Boolean = path.isEmpty
 
-  def withBody(buf: Buf, charset: Option[Charset] = None): Input = {
+  /**
+   * Overrides (mutates) the payload (`buf` and `contentType`) of this input and
+   * returns `this`.
+   *
+   * @note The `contentType` value is passed as an HTTP header so it might also
+   *       contain a charset separated by `;`.
+   */
+  def withBody(buf: Buf, contentType: Option[String] = None): Input = {
     request.content = buf
+
     request.contentLength = buf.length.toLong
-    charset.foreach(cs => request.charset = charset.get.displayName())
+    contentType.foreach(ct => request.contentType = ct)
+
     this
   }
 
+  /**
+   * Adds (mutates) new `headers` to this input and returns `this`.
+   */
   def withHeaders(headers: (String, String)*): Input = {
     headers.foreach { case (k, v) => request.headerMap.set(k, v) }
     this
   }
 
+  /**
+   * Overrides (mutates) the payload of this input to be `application/x-www-form-urlencoded`.
+   *
+   * @note In addition to media type, this will also set charset to UTF-8.
+   */
   def withForm(params: (String, String)*): Input = {
-    require(request.host.isDefined, "The host has to be defined")
-    require(params.nonEmpty, "Cannot create request without params")
-    val req = RequestBuilder()
-      .addFormElement(params: _*)
-      .url(request.host.get)
-      .buildFormPost()
-    request.method = req.method
-    request.content = req.content
-    req.contentLength.foreach(cl => request.contentLength = req.contentLength.get)
-    req.charset.foreach(cs => request.charset = req.charset.get)
-    this
+    // TODO: Figure out way to do that w/o Netty.
+    val dataFactory = new DefaultHttpDataFactory(false) // we don't use disk
+    val encoder = new HttpPostRequestEncoder(
+      dataFactory,
+      new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, request.path),
+      false
+    )
+
+    params.foreach {
+      case (k, v) => encoder.addBodyAttribute(k, v)
+    }
+
+    val req = encoder.finalizeRequest()
+
+    val content = if (req.isChunked) {
+      ConcatBuf(
+        Iterator.continually(encoder.nextChunk())
+        .takeWhile(c => !c.isLast)
+        .map(c => ChannelBufferBuf.Owned(c.getContent))
+        .toVector
+      )
+    } else ChannelBufferBuf.Owned(req.getContent)
+
+    withBody(content, Some(MediaType.WwwForm + ";charset=utf-8"))
   }
 }
 
 /**
  * Creates an input for [[Endpoint]] from [[Request]].
  */
 object Input {
-  def apply(req: Request): Input = Input(req, req.path.split("/").toList.drop(1))
 
   implicit val inputEq: Eq[Input] = Eq.fromUniversalEquals
 
-  def get(path: String, params: (String, String)*): Input = Input(Request(path, params: _*))
-  def put(path: String, params: (String, String)*): Input = {
-    val req = Request(path, params: _*)
-    req.method = Method.Put
-    Input(req)
-  }
-  def patch(path: String, params: (String, String)*): Input = {
-    val req = Request(path, params: _*)
-    req.method = Method.Patch
-    Input(req)
-  }
-  def delete(path: String, params: (String, String)*): Input = {
-    val req = Request(path, params: _*)
-    req.method = Method.Patch
-    Input(req)
-  }
-  def post(path: String): Input = Input(Request(Method.Post, path))
+  /**
+   * Creates an [[Input]] from a given [[Request]].
+   */
+  def request(req: Request): Input = Input(req, req.path.split("/").toList.drop(1))
+
+  /**
+   * Creates a `GET` input with a given query string (represented as `params`).
+   */
+  def get(path: String, params: (String, String)*): Input =
+    request(Request(Method.Get, Request.queryString(path, params: _*)))
+
+  /**
+   * Creates a `PUT` input with a given query string (represented as `params`).
+   */
+  def put(path: String, params: (String, String)*): Input =
+    request(Request(Method.Put, Request.queryString(path, params: _*)))
+
+  /**
+   * Creates a `PATCH` input with a given query string (represented as `params`).
+   */
+  def patch(path: String, params: (String, String)*): Input =
+    request(Request(Method.Patch, Request.queryString(path, params: _*)))
+
+  /**
+   * Creates a `DELETE` input with a given query string (represented as `params`).
+   */
+  def delete(path: String, params: (String, String)*): Input =
+    request(Request(Method.Delete, Request.queryString(path, params: _*)))
+
+  /**
+   * Creates a `POST` input with empty payload.
+   */
+  def post(path: String): Input = request(Request(Method.Post, path))
 }

core/src/test/scala/io/finch/BasicAuthSpec.scala

@@ -25,7 +25,7 @@ class BasicAuthSpec extends FinchSpec {
       req.authorization = encode(c.user, c.pass)
 
       val e = BasicAuth(realm)((u, p) => Future(u == c.user && p == c.pass))(Endpoint(Ok("foo")))
-      val i = Input(req)
+      val i = Input.request(req)
 
       e(i).output === Some(Ok("foo")) && {
         req.authorization = "secret"
@@ -37,18 +37,18 @@ class BasicAuthSpec extends FinchSpec {
   it should "reach the unprotected endpoint" in {
     val e = BasicAuth("realm")((_, _) => Future.False)("a") :+: ("b" :: Endpoint(Ok("foo")))
 
-    val protectedInput = Input(Request("/a"))
+    val protectedInput = Input.get("/a")
     e(protectedInput).remainder shouldBe Some(protectedInput.drop(1))
     e(protectedInput).output shouldBe Some(unauthorized("realm"))
 
-    val unprotectedInput = Input(Request("/b"))
+    val unprotectedInput = Input.get("/b")
     e(unprotectedInput).remainder shouldBe Some(unprotectedInput.drop(1))
     e(unprotectedInput).output.map(_.status) shouldBe Some(Status.Ok)
 
-    val notFound = Input(Request("/c"))
+    val notFound = Input.get("/c")
     e(notFound).remainder shouldBe None // 404
 
-    val notFoundPartialMatch = Input(Request("/a/b"))
+    val notFoundPartialMatch = Input.get("/a/b")
     e(notFoundPartialMatch).remainder shouldBe Some(notFoundPartialMatch.drop(1)) // 404
   }
 }

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

@@ -2,21 +2,13 @@ package io.finch
 
 import java.util.UUID
 
-import com.twitter.finagle.http.Request
 import com.twitter.io.Buf
 
 class BodySpec extends FinchSpec {
 
   behavior of "param*"
 
-  def withBody(b: String): Input = Input {
-    val req = Request()
-    val buf = Buf.Utf8(b)
-    req.content = buf
-    req.headerMap.put("Content-Length", buf.length.toString)
-
-    req
-  }
+  def withBody(b: String): Input = Input.post("/").withBody(Buf.Utf8(b))
 
   checkAll("Body[String]", EndpointLaws[String](bodyOption)(withBody).evaluating)
   checkAll("Body[Int]", EndpointLaws[Int](bodyOption)(withBody).evaluating)

core/src/test/scala/io/finch/EndpointSpec.scala

@@ -4,7 +4,7 @@ import java.util.UUID
 
 import cats.Applicative
 import cats.laws.discipline.AlternativeTests
-import com.twitter.finagle.http.{Request, Method, Cookie}
+import com.twitter.finagle.http.{Method, Cookie}
 import com.twitter.util.{Throw, Try, Future}
 
 class EndpointSpec extends FinchSpec {
@@ -224,13 +224,13 @@ class EndpointSpec extends FinchSpec {
   }
 
   it should "not split comma separated param values" in {
-    val i = Input(Request("/index?foo=a,b"))
+    val i = Input.get("/index", "foo" -> "a,b")
     val e = params("foo")
     e(i).value shouldBe Some(Seq("a,b"))
   }
 
   it should "throw NotPresent if an item is not found" in {
-    val i = Input(Request())
+    val i = Input.get("/")
 
     Seq(
       param("foo"), header("foo"), body, cookie("foo").map(_.value),

core/src/test/scala/io/finch/FinchSpec.scala

@@ -31,6 +31,7 @@ trait FinchSpec extends FlatSpec with Matchers with Checkers with AllInstances
   case class Headers(m: Map[String, String])
   case class Params(p: Map[String, String])
   case class Cookies(c: Seq[Cookie])
+  case class Path(p: String)
 
   case class OptionalNonEmptyString(o: Option[String])
 
@@ -112,15 +113,15 @@ trait FinchSpec extends FlatSpec with Matchers with Checkers with AllInstances
 
   def genVersion: Gen[Version] = Gen.oneOf(Version.Http10, Version.Http11)
 
-  def genPath: Gen[String] = for {
+  def genPath: Gen[Path] = for {
     n <- Gen.choose(0, 20)
     ss <- Gen.listOfN(n, Gen.oneOf(
       Gen.alphaStr.suchThat(_.nonEmpty),
       Gen.uuid.map(_.toString),
       Gen.posNum[Long].map(_.toString),
       Gen.oneOf(true, false).map(_.toString)
     ))
-  } yield "/" + ss.mkString("/")
+  } yield Path("/" + ss.mkString("/"))
 
   def genBuf: Gen[Buf] = for {
     s <- Arbitrary.arbitrary[String]
@@ -137,7 +138,7 @@ trait FinchSpec extends FlatSpec with Matchers with Checkers with AllInstances
       s <- genPath
       b <- genBuf
     } yield {
-      val r = Request(v, m, s)
+      val r = Request(v, m, s.p)
       r.content = b
       r.contentLength = b.length.toLong
       r.charset = "utf-8"
@@ -196,7 +197,7 @@ trait FinchSpec extends FlatSpec with Matchers with Checkers with AllInstances
   }
 
   implicit def arbitraryInput: Arbitrary[Input] =
-    Arbitrary(arbitraryRequest.arbitrary.map(Input.apply))
+    Arbitrary(arbitraryRequest.arbitrary.map(Input.request))
 
   implicit def arbitraryUUID: Arbitrary[UUID] = Arbitrary(Gen.uuid)
 
@@ -210,6 +211,8 @@ trait FinchSpec extends FlatSpec with Matchers with Checkers with AllInstances
 
   implicit def arbitraryParams: Arbitrary[Params] = Arbitrary(genParams)
 
+  implicit def arbitraryPath: Arbitrary[Path] = Arbitrary(genPath)
+
   implicit def arbitraryCharset: Arbitrary[Charset] = Arbitrary(genCharset)
 
   implicit def arbitraryBuf: Arbitrary[Buf] = Arbitrary(genBuf)