From 55089b8be35963824a5029d1bac42b2ddabc0fd5 Mon Sep 17 00:00:00 2001 From: Tushar Mathur Date: Wed, 26 Jan 2022 12:23:12 +0530 Subject: [PATCH 1/2] refactor: merge Request for Client and Server --- .../src/main/scala/zhttp/http/Request.scala | 4 +- .../src/main/scala/zhttp/service/Client.scala | 63 ++++---------- .../zhttp/service/EncodeClientParams.scala | 63 +++++++------- .../zhttp/http/EncodeClientRequestSpec.scala | 83 ------------------ .../scala/zhttp/http/EncodeRequestSpec.scala | 85 +++++++++++++++++++ .../zhttp/http/GetBodyAsStringSpec.scala | 30 +++---- .../test/scala/zhttp/internal/HttpGen.scala | 7 +- .../zhttp/internal/HttpRunnableSpec.scala | 11 +-- 8 files changed, 157 insertions(+), 189 deletions(-) delete mode 100644 zio-http/src/test/scala/zhttp/http/EncodeClientRequestSpec.scala create mode 100644 zio-http/src/test/scala/zhttp/http/EncodeRequestSpec.scala diff --git a/zio-http/src/main/scala/zhttp/http/Request.scala b/zio-http/src/main/scala/zhttp/http/Request.scala index a930ea5ed5..e78be8918b 100644 --- a/zio-http/src/main/scala/zhttp/http/Request.scala +++ b/zio-http/src/main/scala/zhttp/http/Request.scala @@ -95,8 +95,8 @@ object Request { method: Method = Method.GET, url: URL = URL.root, headers: Headers = Headers.empty, - remoteAddress: Option[InetAddress] = None, data: HttpData = HttpData.Empty, + remoteAddress: Option[InetAddress] = None, ): Request = { val m = method val u = url @@ -121,7 +121,7 @@ object Request { remoteAddress: Option[InetAddress], content: HttpData = HttpData.empty, ): UIO[Request] = - UIO(Request(method, url, headers, remoteAddress, content)) + UIO(Request(method, url, headers, content, remoteAddress)) /** * Lift request to TypedRequest with option to extract params diff --git a/zio-http/src/main/scala/zhttp/service/Client.scala b/zio-http/src/main/scala/zhttp/service/Client.scala index fb9afde801..140d7a338c 100644 --- a/zio-http/src/main/scala/zhttp/service/Client.scala +++ b/zio-http/src/main/scala/zhttp/service/Client.scala @@ -2,42 +2,38 @@ package zhttp.service import io.netty.bootstrap.Bootstrap import io.netty.buffer.{ByteBuf, ByteBufUtil} -import io.netty.channel.{ - Channel, - ChannelFactory => JChannelFactory, - ChannelHandlerContext, - EventLoopGroup => JEventLoopGroup, -} -import io.netty.handler.codec.http.HttpVersion +import io.netty.channel.{Channel, ChannelFactory => JChannelFactory, EventLoopGroup => JEventLoopGroup} +import io.netty.handler.codec.http.{FullHttpRequest, HttpVersion} import zhttp.http.URL.Location import zhttp.http._ import zhttp.http.headers.HeaderExtension import zhttp.service -import zhttp.service.Client.{ClientRequest, ClientResponse} +import zhttp.service.Client.ClientResponse import zhttp.service.client.ClientSSLHandler.ClientSSLOptions import zhttp.service.client.{ClientChannelInitializer, ClientInboundHandler} import zio.{Chunk, Promise, Task, ZIO} -import java.net.{InetAddress, InetSocketAddress} +import java.net.InetSocketAddress final case class Client(rtm: HttpRuntime[Any], cf: JChannelFactory[Channel], el: JEventLoopGroup) extends HttpMessageCodec { def request( - request: Client.ClientRequest, + request: Request, sslOption: ClientSSLOptions = ClientSSLOptions.DefaultSSL, ): Task[Client.ClientResponse] = for { promise <- Promise.make[Throwable, Client.ClientResponse] - _ <- Task(asyncRequest(request, promise, sslOption)).catchAll(cause => promise.fail(cause)) + jReq <- encodeClientParams(HttpVersion.HTTP_1_1, request) + _ <- Task(asyncRequest(request, jReq, promise, sslOption)).catchAll(cause => promise.fail(cause)) res <- promise.await } yield res private def asyncRequest( - req: ClientRequest, + req: Request, + jReq: FullHttpRequest, promise: Promise[Throwable, ClientResponse], sslOption: ClientSSLOptions, ): Unit = { - val jReq = encodeClientParams(HttpVersion.HTTP_1_1, req) try { val hand = ClientInboundHandler(rtm, jReq, promise) val host = req.url.host @@ -111,14 +107,14 @@ object Client { method: Method, url: URL, ): ZIO[EventLoopGroup with ChannelFactory, Throwable, ClientResponse] = - request(ClientRequest(method, url)) + request(Request(method, url)) def request( method: Method, url: URL, sslOptions: ClientSSLOptions, ): ZIO[EventLoopGroup with ChannelFactory, Throwable, ClientResponse] = - request(ClientRequest(method, url), sslOptions) + request(Request(method, url), sslOptions) def request( method: Method, @@ -126,7 +122,7 @@ object Client { headers: Headers, sslOptions: ClientSSLOptions, ): ZIO[EventLoopGroup with ChannelFactory, Throwable, ClientResponse] = - request(ClientRequest(method, url, headers), sslOptions) + request(Request(method, url, headers), sslOptions) def request( method: Method, @@ -134,48 +130,19 @@ object Client { headers: Headers, content: HttpData, ): ZIO[EventLoopGroup with ChannelFactory, Throwable, ClientResponse] = - request(ClientRequest(method, url, headers, content)) + request(Request(method, url, headers, content, None)) def request( - req: ClientRequest, + req: Request, ): ZIO[EventLoopGroup with ChannelFactory, Throwable, ClientResponse] = make.flatMap(_.request(req)) def request( - req: ClientRequest, + req: Request, sslOptions: ClientSSLOptions, ): ZIO[EventLoopGroup with ChannelFactory, Throwable, ClientResponse] = make.flatMap(_.request(req, sslOptions)) - final case class ClientRequest( - method: Method, - url: URL, - getHeaders: Headers = Headers.empty, - data: HttpData = HttpData.empty, - private val channelContext: ChannelHandlerContext = null, - ) extends HeaderExtension[ClientRequest] { self => - - def getBodyAsString: Option[String] = data match { - case HttpData.Text(text, _) => Some(text) - case HttpData.BinaryChunk(data) => Some(new String(data.toArray, HTTP_CHARSET)) - case HttpData.BinaryByteBuf(data) => Some(data.toString(HTTP_CHARSET)) - case _ => Option.empty - } - - def remoteAddress: Option[InetAddress] = { - if (channelContext != null && channelContext.channel().remoteAddress().isInstanceOf[InetSocketAddress]) - Some(channelContext.channel().remoteAddress().asInstanceOf[InetSocketAddress].getAddress) - else - None - } - - /** - * Updates the headers using the provided function - */ - override def updateHeaders(update: Headers => Headers): ClientRequest = - self.copy(getHeaders = update(self.getHeaders)) - } - final case class ClientResponse(status: Status, headers: Headers, private[zhttp] val buffer: ByteBuf) extends HeaderExtension[ClientResponse] { self => diff --git a/zio-http/src/main/scala/zhttp/service/EncodeClientParams.scala b/zio-http/src/main/scala/zhttp/service/EncodeClientParams.scala index eb9c935b3a..81c508a0d5 100644 --- a/zio-http/src/main/scala/zhttp/service/EncodeClientParams.scala +++ b/zio-http/src/main/scala/zhttp/service/EncodeClientParams.scala @@ -1,41 +1,42 @@ package zhttp.service -import io.netty.buffer.Unpooled import io.netty.handler.codec.http.{DefaultFullHttpRequest, FullHttpRequest, HttpHeaderNames, HttpVersion} -import zhttp.http.HTTP_CHARSET +import zhttp.http.Request +import zio.Task trait EncodeClientParams { /** * Converts client params to JFullHttpRequest */ - def encodeClientParams(jVersion: HttpVersion, req: Client.ClientRequest): FullHttpRequest = { - val method = req.method.asHttpMethod - val url = req.url - - // As per the spec, the path should contain only the relative path. - // Host and port information should be in the headers. - val path = url.relative.asString - - val content = req.getBodyAsString match { - case Some(text) => Unpooled.copiedBuffer(text, HTTP_CHARSET) - case None => Unpooled.EMPTY_BUFFER - } - - val encodedReqHeaders = req.getHeaders.encode - - val headers = url.host match { - case Some(value) => encodedReqHeaders.set(HttpHeaderNames.HOST, value) - case None => encodedReqHeaders - } - - val writerIndex = content.writerIndex() - if (writerIndex != 0) { - headers.set(HttpHeaderNames.CONTENT_LENGTH, writerIndex.toString()) - } - // TODO: we should also add a default user-agent req header as some APIs might reject requests without it. - val jReq = new DefaultFullHttpRequest(jVersion, method, path, content) - jReq.headers().set(headers) - - jReq + def encodeClientParams(jVersion: HttpVersion, req: Request): Task[FullHttpRequest] = req.getBodyAsByteBuf.map { + content => + val method = req.method.asHttpMethod + val url = req.url + + // As per the spec, the path should contain only the relative path. + // Host and port information should be in the headers. + val path = url.relative.asString + + // val content = req.getBodyAsString match { + // case Some(text) => Unpooled.copiedBuffer(text, HTTP_CHARSET) + // case None => Unpooled.EMPTY_BUFFER + // } + + val encodedReqHeaders = req.getHeaders.encode + + val headers = url.host match { + case Some(value) => encodedReqHeaders.set(HttpHeaderNames.HOST, value) + case None => encodedReqHeaders + } + + val writerIndex = content.writerIndex() + if (writerIndex != 0) { + headers.set(HttpHeaderNames.CONTENT_LENGTH, writerIndex.toString()) + } + // TODO: we should also add a default user-agent req header as some APIs might reject requests without it. + val jReq = new DefaultFullHttpRequest(jVersion, method, path, content) + jReq.headers().set(headers) + + jReq } } diff --git a/zio-http/src/test/scala/zhttp/http/EncodeClientRequestSpec.scala b/zio-http/src/test/scala/zhttp/http/EncodeClientRequestSpec.scala deleted file mode 100644 index 6d2ebc5b24..0000000000 --- a/zio-http/src/test/scala/zhttp/http/EncodeClientRequestSpec.scala +++ /dev/null @@ -1,83 +0,0 @@ -package zhttp.http - -import io.netty.handler.codec.http.{HttpHeaderNames, HttpVersion} -import zhttp.internal.HttpGen -import zhttp.service.{Client, EncodeClientParams} -import zio.random.Random -import zio.test.Assertion._ -import zio.test._ - -object EncodeClientRequestSpec extends DefaultRunnableSpec with EncodeClientParams { - - val anyClientParam: Gen[Random with Sized, Client.ClientRequest] = HttpGen.clientRequest( - HttpGen.httpData( - Gen.listOf(Gen.alphaNumericString), - ), - ) - - val clientParamWithAbsoluteUrl = HttpGen.clientRequest( - dataGen = HttpGen.httpData( - Gen.listOf(Gen.alphaNumericString), - ), - urlGen = HttpGen.genAbsoluteURL, - ) - - def clientParamWithFiniteData(size: Int): Gen[Random with Sized, Client.ClientRequest] = HttpGen.clientRequest( - for { - content <- Gen.alphaNumericStringBounded(size, size) - data <- Gen.fromIterable(List(HttpData.fromString(content))) - } yield data, - ) - - def spec = suite("EncodeClientParams") { - testM("method") { - check(anyClientParam) { params => - val req = encodeClientParams(HttpVersion.HTTP_1_1, params) - assert(req.method())(equalTo(params.method.asHttpMethod)) - } - } + - testM("method on HttpData.File") { - check(HttpGen.clientParamsForFileHttpData()) { params => - val req = encodeClientParams(HttpVersion.HTTP_1_1, params) - assert(req.method())(equalTo(params.method.asHttpMethod)) - } - } + - suite("uri") { - testM("uri") { - check(anyClientParam) { params => - val req = encodeClientParams(HttpVersion.HTTP_1_1, params) - assert(req.uri())(equalTo(params.url.relative.asString)) - } - } + - testM("uri on HttpData.File") { - check(HttpGen.clientParamsForFileHttpData()) { params => - val req = encodeClientParams(HttpVersion.HTTP_1_1, params) - assert(req.uri())(equalTo(params.url.relative.asString)) - } - } - } + - testM("content-length") { - check(clientParamWithFiniteData(5)) { params => - val req = encodeClientParams(HttpVersion.HTTP_1_1, params) - assert(req.headers().getInt(HttpHeaderNames.CONTENT_LENGTH).toLong)(equalTo(5L)) - } - } + - testM("host header") { - check(anyClientParam) { params => - val req = encodeClientParams(HttpVersion.HTTP_1_1, params) - val hostHeader = HttpHeaderNames.HOST - assert(Option(req.headers().get(hostHeader)))(equalTo(params.url.host)) - } - } + - testM("host header when absolute url") { - check(clientParamWithAbsoluteUrl) { params => - val req = encodeClientParams(HttpVersion.HTTP_1_1, params) - val reqHeaders = req.headers() - val hostHeader = HttpHeaderNames.HOST - - assert(reqHeaders.getAll(hostHeader).size)(equalTo(1)) && - assert(Option(reqHeaders.get(hostHeader)))(equalTo(params.url.host)) - } - } - } -} diff --git a/zio-http/src/test/scala/zhttp/http/EncodeRequestSpec.scala b/zio-http/src/test/scala/zhttp/http/EncodeRequestSpec.scala new file mode 100644 index 0000000000..d62176f5b6 --- /dev/null +++ b/zio-http/src/test/scala/zhttp/http/EncodeRequestSpec.scala @@ -0,0 +1,85 @@ +package zhttp.http + +import io.netty.handler.codec.http.{HttpHeaderNames, HttpVersion} +import zhttp.internal.HttpGen +import zhttp.service.EncodeClientParams +import zio.random.Random +import zio.test.Assertion._ +import zio.test._ + +object EncodeRequestSpec extends DefaultRunnableSpec with EncodeClientParams { + + val anyClientParam: Gen[Random with Sized, Request] = HttpGen.clientRequest( + HttpGen.httpData( + Gen.listOf(Gen.alphaNumericString), + ), + ) + + val clientParamWithAbsoluteUrl = HttpGen.clientRequest( + dataGen = HttpGen.httpData( + Gen.listOf(Gen.alphaNumericString), + ), + urlGen = HttpGen.genAbsoluteURL, + ) + + def clientParamWithFiniteData(size: Int): Gen[Random with Sized, Request] = HttpGen.clientRequest( + for { + content <- Gen.alphaNumericStringBounded(size, size) + data <- Gen.fromIterable(List(HttpData.fromString(content))) + } yield data, + ) + + def spec = suite("EncodeClientParams") { + testM("method") { + checkM(anyClientParam) { params => + val method = encodeClientParams(HttpVersion.HTTP_1_1, params).map(_.method()) + assertM(method)(equalTo(params.method.asHttpMethod)) + } + } + + testM("method on HttpData.File") { + checkM(HttpGen.clientParamsForFileHttpData()) { params => + val method = encodeClientParams(HttpVersion.HTTP_1_1, params).map(_.method()) + assertM(method)(equalTo(params.method.asHttpMethod)) + } + } + + suite("uri") { + testM("uri") { + checkM(anyClientParam) { params => + val uri = encodeClientParams(HttpVersion.HTTP_1_1, params).map(_.uri()) + assertM(uri)(equalTo(params.url.relative.asString)) + } + } + + testM("uri on HttpData.File") { + checkM(HttpGen.clientParamsForFileHttpData()) { params => + val uri = encodeClientParams(HttpVersion.HTTP_1_1, params).map(_.uri()) + assertM(uri)(equalTo(params.url.relative.asString)) + } + } + } + + testM("content-length") { + checkM(clientParamWithFiniteData(5)) { params => + val len = encodeClientParams(HttpVersion.HTTP_1_1, params).map( + _.headers().getInt(HttpHeaderNames.CONTENT_LENGTH).toLong, + ) + assertM(len)(equalTo(5L)) + } + } + + testM("host header") { + checkM(anyClientParam) { params => + val hostHeader = HttpHeaderNames.HOST + val headers = encodeClientParams(HttpVersion.HTTP_1_1, params).map(h => Option(h.headers().get(hostHeader))) + assertM(headers)(equalTo(params.url.host)) + } + } + + testM("host header when absolute url") { + checkM(clientParamWithAbsoluteUrl) { params => + val hostHeader = HttpHeaderNames.HOST + for { + reqHeaders <- encodeClientParams(HttpVersion.HTTP_1_1, params).map(_.headers()) + } yield assert(reqHeaders.getAll(hostHeader).size)(equalTo(1)) && assert(Option(reqHeaders.get(hostHeader)))( + equalTo(params.url.host), + ) + } + } + } +} diff --git a/zio-http/src/test/scala/zhttp/http/GetBodyAsStringSpec.scala b/zio-http/src/test/scala/zhttp/http/GetBodyAsStringSpec.scala index aad22542d1..bd3da3cc7e 100644 --- a/zio-http/src/test/scala/zhttp/http/GetBodyAsStringSpec.scala +++ b/zio-http/src/test/scala/zhttp/http/GetBodyAsStringSpec.scala @@ -1,7 +1,6 @@ package zhttp.http import io.netty.handler.codec.http.HttpHeaderNames -import zhttp.service.Client import zio.Chunk import zio.test.Assertion._ import zio.test._ @@ -16,27 +15,26 @@ object GetBodyAsStringSpec extends DefaultRunnableSpec { val charsetGen: Gen[Any, Charset] = Gen.fromIterable(List(UTF_8, UTF_16, UTF_16BE, UTF_16LE, US_ASCII, ISO_8859_1)) - check(charsetGen) { charset => - val encoded = Client - .ClientRequest( - Method.GET, - URL(Path("/")), - getHeaders = Headers(HttpHeaderNames.CONTENT_TYPE.toString, s"text/html; charset=$charset"), - data = HttpData.BinaryChunk(Chunk.fromArray("abc".getBytes())), - ) - .getBodyAsString - val actual = Option(new String(Chunk.fromArray("abc".getBytes(charset)).toArray, charset)) + checkM(charsetGen) { charset => + val encoded = Request( + Method.GET, + URL(Path("/")), + headers = Headers(HttpHeaderNames.CONTENT_TYPE.toString, s"text/html; charset=$charset"), + data = HttpData.BinaryChunk(Chunk.fromArray("abc".getBytes(charset))), + ).getBodyAsString - assert(actual)(equalTo(encoded)) + val expected = new String(Chunk.fromArray("abc".getBytes(charset)).toArray, charset) + + assertM(encoded)(equalTo(expected)) } } + - test("should map bytes to default utf-8 if no charset given") { + testM("should map bytes to default utf-8 if no charset given") { val data = Chunk.fromArray("abc".getBytes()) val content = HttpData.BinaryChunk(data) - val request = Client.ClientRequest(Method.GET, URL(Path("/")), data = content) + val request = Request(Method.GET, URL(Path("/")), data = content) val encoded = request.getBodyAsString - val actual = Option(new String(data.toArray, HTTP_CHARSET)) - assert(actual)(equalTo(encoded)) + val actual = new String(data.toArray, HTTP_CHARSET) + assertM(encoded)(equalTo(actual)) }, ) } diff --git a/zio-http/src/test/scala/zhttp/internal/HttpGen.scala b/zio-http/src/test/scala/zhttp/internal/HttpGen.scala index 996deb4d4a..5b06448608 100644 --- a/zio-http/src/test/scala/zhttp/internal/HttpGen.scala +++ b/zio-http/src/test/scala/zhttp/internal/HttpGen.scala @@ -3,7 +3,6 @@ package zhttp.internal import io.netty.buffer.Unpooled import zhttp.http.URL.Location import zhttp.http._ -import zhttp.service.Client.ClientRequest import zio.random.Random import zio.stream.ZStream import zio.test.{Gen, Sized} @@ -23,7 +22,7 @@ object HttpGen { url <- urlGen headers <- Gen.listOf(headerGen).map(Headers(_)) data <- dataGen - } yield ClientRequest(method, url, headers, data) + } yield Request(method, url, headers, data, None) def clientParamsForFileHttpData() = { for { @@ -31,7 +30,7 @@ object HttpGen { method <- HttpGen.method url <- HttpGen.url headers <- Gen.listOf(HttpGen.header).map(Headers(_)) - } yield ClientRequest(method, url, headers, HttpData.fromFile(file)) + } yield Request(method, url, headers, HttpData.fromFile(file), None) } def cookies: Gen[Random with Sized, Cookie] = for { @@ -119,7 +118,7 @@ object HttpGen { url <- HttpGen.url headers <- Gen.listOf(HttpGen.header).map(Headers(_)) data <- HttpGen.httpData(Gen.listOf(Gen.alphaNumericString)) - } yield Request(method, url, headers, None, data) + } yield Request(method, url, headers, data, None) def response[R](gContent: Gen[R, List[String]]): Gen[Random with Sized with R, Response] = { for { diff --git a/zio-http/src/test/scala/zhttp/internal/HttpRunnableSpec.scala b/zio-http/src/test/scala/zhttp/internal/HttpRunnableSpec.scala index 1642799a76..5718e794c4 100644 --- a/zio-http/src/test/scala/zhttp/internal/HttpRunnableSpec.scala +++ b/zio-http/src/test/scala/zhttp/internal/HttpRunnableSpec.scala @@ -21,7 +21,7 @@ import zio.{Has, Task, ZIO, ZManaged} */ abstract class HttpRunnableSpec extends DefaultRunnableSpec { self => - implicit class RunnableClientHttpSyntax[R, A](app: Http[R, Throwable, Client.ClientRequest, A]) { + implicit class RunnableClientHttpSyntax[R, A](app: Http[R, Throwable, Request, A]) { /** * Runs the deployed Http app by making a real http request to it. The method allows us to configure individual @@ -34,11 +34,12 @@ abstract class HttpRunnableSpec extends DefaultRunnableSpec { self => headers: Headers = Headers.empty, ): ZIO[R, Throwable, A] = app( - Client.ClientRequest( + Request( method, URL(path, Location.Absolute(Scheme.HTTP, "localhost", 0)), headers, HttpData.fromString(content), + None, ), ).catchAll { case Some(value) => ZIO.fail(value) @@ -58,7 +59,7 @@ abstract class HttpRunnableSpec extends DefaultRunnableSpec { self => for { port <- Http.fromZIO(DynamicServer.getPort) id <- Http.fromZIO(DynamicServer.deploy(app)) - response <- Http.fromFunctionZIO[Client.ClientRequest] { params => + response <- Http.fromFunctionZIO[Request] { params => Client.request( params .addHeader(DynamicServer.APP_ID, id) @@ -74,7 +75,7 @@ abstract class HttpRunnableSpec extends DefaultRunnableSpec { self => def deployWebSocket: HttpTestClient[SttpClient, client3.Response[Either[String, WebSocket[Task]]]] = for { id <- Http.fromZIO(DynamicServer.deploy(app)) res <- - Http.fromFunctionZIO[Client.ClientRequest](params => + Http.fromFunctionZIO[Request](params => for { port <- DynamicServer.getPort url = s"ws://localhost:$port${params.url.path.asString}" @@ -117,7 +118,7 @@ object HttpRunnableSpec { Http[ R with EventLoopGroup with ChannelFactory with DynamicServer with ServerChannelFactory, Throwable, - Client.ClientRequest, + Request, A, ] } From 2aa57cfe50d54c6e22863f8cff34fee24cac82a6 Mon Sep 17 00:00:00 2001 From: Tushar Mathur Date: Thu, 27 Jan 2022 19:50:11 +0530 Subject: [PATCH 2/2] remove commented code --- .../src/main/scala/zhttp/service/EncodeClientParams.scala | 5 ----- 1 file changed, 5 deletions(-) diff --git a/zio-http/src/main/scala/zhttp/service/EncodeClientParams.scala b/zio-http/src/main/scala/zhttp/service/EncodeClientParams.scala index b6964e6e02..ca5d4b4807 100644 --- a/zio-http/src/main/scala/zhttp/service/EncodeClientParams.scala +++ b/zio-http/src/main/scala/zhttp/service/EncodeClientParams.scala @@ -17,11 +17,6 @@ trait EncodeClientParams { // Host and port information should be in the headers. val path = url.relative.encode - // val content = req.getBodyAsString match { - // case Some(text) => Unpooled.copiedBuffer(text, HTTP_CHARSET) - // case None => Unpooled.EMPTY_BUFFER - // } - val encodedReqHeaders = req.getHeaders.encode val headers = url.host match {