Skip to content
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

Fix to send empty body, and consume it #522

Merged
merged 10 commits into from
Nov 12, 2022
4 changes: 3 additions & 1 deletion build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -605,7 +605,9 @@ lazy val http4s = projectMatrix
Dependencies.Http4s.client.value,
Dependencies.Alloy.core % Test,
Dependencies.Http4s.circe.value % Test,
Dependencies.Weaver.cats.value % Test
Dependencies.Weaver.cats.value % Test,
Dependencies.Http4s.emberClient.value % Test,
Dependencies.Http4s.emberServer.value % Test
)
},
moduleName := {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ import org.http4s.Request
import org.http4s.Response
import org.http4s.Uri
import org.http4s.client.Client
import scodec.bits.ByteVector
import smithy4s.kinds._
import smithy4s.http._
import smithy4s.schema.SchemaAlt
import smithy4s.kinds._

/**
* A construct that encapsulates interprets and a low-level
Expand Down Expand Up @@ -115,7 +116,7 @@ private[http4s] class SmithyHttp4sClientEndpointImpl[F[_], Op[_, _, _, _, _], I,
val baseRequest = Request[F](method, uri, headers = headers)
if (inputHasBody) {
baseRequest.withEntity(input)
} else baseRequest
} else baseRequest.withEntity(ByteVector.empty)
Copy link
Contributor

@ahjohannessen ahjohannessen Oct 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not only apply to methods like PUT and POST?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good question, let me see what facilities exists here to distinguish what method allow or not bodies

Copy link
Contributor

@ahjohannessen ahjohannessen Oct 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in ember:

  private val NoPayloadMethods: Set[Method] =
    Set(Method.GET, Method.DELETE, Method.CONNECT, Method.TRACE)

Noticed that here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it annoys me that I can't find a public equivalent for this and avoid copy/pasting that method Set out of ember :/

Copy link
Contributor

@armanbilge armanbilge Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.putHeaders(`Content-Length`.zero) might be another workaround. And I think this is ok for GET requests. IDK YMMV.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it annoys me that I can't find a public equivalent for this and avoid copy/pasting that method Set out of ember :/

Ah yeah, an issue or PR to fix this would be great. Seems like we just need to add a boolean to Method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy to open it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the discussions in the PR, I think this should be merged as in. I'm not sure there can be an always accurate way of telling whether a particular method should have a body or not.

Considering it works for our current use cases, we should move forward with it. If we get and issue later on that sending and empty body is problematic under certain situation, we should revisit.

}

private def outputFromResponse(response: Response[F]): F[O] =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ private[http4s] class SmithyHttp4sServerEndpointImpl[F[_], Op[_, _, _, _, _], I,
def run(pathParams: PathParams, request: Request[F]): F[Response[F]] = {
val run: F[O] = for {
metadata <- getMetadata(pathParams, request)
input <- extractInput.run((metadata, request))
input <- extractInput(metadata, request)
output <- (impl(endpoint.wrap(input)): F[O])
} yield output

Expand Down Expand Up @@ -132,26 +132,24 @@ private[http4s] class SmithyHttp4sServerEndpointImpl[F[_], Op[_, _, _, _, _], I,
errorTransformation(other).flatMap(F.raiseError)
}



// format: off
private val extractInput: (Metadata, Request[F]) ==> I = {
private val extractInput: (Metadata, Request[F]) => F[I] = {
inputMetadataDecoder.total match {
case Some(totalDecoder) =>
Kleisli(totalDecoder.decode(_: Metadata).liftTo[F]).local(_._1)
(metadata, request) =>
request.body.compile.drain *>
totalDecoder.decode(metadata).liftTo[F]
case None =>
// NB : only compiling the input codec if the data cannot be
// totally extracted from the metadata.
implicit val inputCodec = entityCompiler.compilePartialEntityDecoder(inputSchema, entityCache)
Kleisli { case (metadata, request) =>
implicit val inputCodec =
entityCompiler.compilePartialEntityDecoder(inputSchema, entityCache)
(metadata, request) =>
for {
metadataPartial <- inputMetadataDecoder.decode(metadata).liftTo[F]
bodyPartial <- request.as[BodyPartial[I]]
} yield metadataPartial.combine(bodyPartial)
}
}
}
// format: on

private def putHeaders(m: Message[F], headers: Headers) =
m.putHeaders(headers.headers)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright 2021-2022 Disney Streaming
*
* Licensed under the Tomorrow Open Source Technology License, Version 1.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://disneystreaming.github.io/TOST-1.0.txt
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package smithy4s.http4s

import cats.effect.IO
import cats.effect.Resource
import cats.effect.syntax.resource._
import cats.implicits._
import com.comcast.ip4s._
import com.comcast.ip4s.Port
import org.http4s.ember.client.EmberClientBuilder
import org.http4s.ember.server.EmberServerBuilder
import org.http4s.HttpApp
import org.http4s.Uri
import smithy4s.example._
import smithy4s.example.PizzaAdminService
import weaver._

object Http4sEmberPizzaClientSpec extends IOSuite {
type Res = PizzaAdminService[IO]

override def sharedResource: Resource[IO, Res] = {
SimpleRestJsonBuilder
.routes(dummyImpl)
.resource
.flatMap(r => retryResource(server(r.orNotFound)))
.flatMap { port => makeClient(port) }
}

def makeClient(port: Int): Resource[IO, PizzaAdminService[IO]] =
EmberClientBuilder.default[IO].build.flatMap { client =>
SimpleRestJsonBuilder(PizzaAdminService)
.client(client)
.uri(Uri.unsafeFromString(s"http://localhost:$port"))
.resource
}

def server(app: HttpApp[IO]): Resource[IO, Int] =
cats.effect.std.Random
.scalaUtilRandom[IO]
.flatMap(_.betweenInt(50000, 60000))
.toResource
.flatMap(port =>
Port
.fromInt(port)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haven't checked but wouldn't you be able to bind to 0 and get a random port? except in this case you'd have a guarantee that it's free (again, not sure if ember behaves like this)

.toRight(new Exception(s"Invalid port: $port"))
.liftTo[IO]
.toResource
)
.flatMap { port =>
EmberServerBuilder
.default[IO]
.withHost(host"localhost")
.withPort(port)
.withHttpApp(app)
.build
.map(_ => port.value)
}

test("empty body") { client =>
(client.book("name") *> client.book("name2")).as(success)
}

private val dummyImpl = new PizzaAdminService[IO]() {
// format: off
override def addMenuItem(restaurant: String, menuItem: MenuItem): IO[AddMenuItemResult] = IO.stub
override def getMenu(restaurant: String): IO[GetMenuResult] = IO.stub
override def version(): IO[VersionOutput] = IO.stub
override def health(query: Option[String]): IO[HealthResponse] = IO.pure(HealthResponse("good"))
override def headerEndpoint(uppercaseHeader: Option[String], capitalizedHeader: Option[String], lowercaseHeader: Option[String], mixedHeader: Option[String]): IO[HeaderEndpointData] = IO.stub
override def roundTrip(label: String, header: Option[String], query: Option[String], body: Option[String]): IO[RoundTripData] = IO.stub
override def getEnum(aa: TheEnum): IO[GetEnumOutput] = IO.stub
override def getIntEnum(aa: EnumResult): IO[GetIntEnumOutput] = IO.stub
override def customCode(code: Int): IO[CustomCodeOutput] = IO.stub
override def book(name: String, town: Option[String]): IO[BookOutput] = IO.pure(BookOutput("name"))
// format: on
}

def retryResource[A](
resource: Resource[IO, A],
max: Int = 10
): Resource[IO, A] =
if (max <= 0) resource
else resource.orElse(retryResource(resource, max - 1))
}
3 changes: 3 additions & 0 deletions modules/tests/src/smithy4s/tests/PizzaAdminServiceImpl.scala
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ object PizzaAdminServiceImpl {
class PizzaAdminServiceImpl(ref: Compat.Ref[IO, State])
extends PizzaAdminService[IO] {

def book(name: String, town: Option[String]): IO[BookOutput] =
IO.pure(BookOutput(message = s"Booked for $name"))

def getEnum(theEnum: TheEnum): IO[GetEnumOutput] =
IO.pure(GetEnumOutput(result = Some(theEnum.value)))

Expand Down
9 changes: 6 additions & 3 deletions modules/tests/src/smithy4s/tests/PizzaClientSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,16 @@ package smithy4s.tests
import cats.data.Chain
import cats.effect._
import cats.effect.std.UUIDGen
import cats.Show
import cats.syntax.all._
import io.circe.Json
import org.http4s.HttpApp
import org.http4s._
import org.http4s.circe._
import org.http4s.dsl.io._
import org.http4s.HttpApp
import org.typelevel.ci.CIString
import smithy4s.Timestamp
import smithy4s.example._
import cats.Show
import smithy4s.Timestamp
import weaver._

abstract class PizzaClientSpec extends IOSuite {
Expand Down Expand Up @@ -302,6 +302,9 @@ abstract class PizzaClientSpec extends IOSuite {
case request @ (GET -> Root / "custom-code" / IntVar(code)) =>
storeAndReturn(s"customCode$code", request)

case POST -> Root / "book" / _ =>
val body = Json.obj("message" -> Json.fromString("test"))
Ok(body)
}
.orNotFound
}
Expand Down
18 changes: 17 additions & 1 deletion sampleSpecs/pizza.smithy
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use alloy#simpleRestJson
service PizzaAdminService {
version: "1.0.0",
errors: [GenericServerError, GenericClientError],
operations: [AddMenuItem, GetMenu, Version, Health, HeaderEndpoint, RoundTrip, GetEnum, GetIntEnum, CustomCode]
operations: [AddMenuItem, GetMenu, Version, Health, HeaderEndpoint, RoundTrip, GetEnum, GetIntEnum, CustomCode, Book]
}

@http(method: "POST", uri: "/restaurant/{restaurant}/menu/item", code: 201)
Expand Down Expand Up @@ -301,3 +301,19 @@ structure CustomCodeOutput {
@httpResponseCode
code: Integer
}

@http(method: "POST", uri: "/book/{name}", code: 200)
operation Book {
input := {
@httpLabel
@required
name: String,

@httpQuery("town")
town: String
},
output := {
@required
message: String
}
}