Fix to send empty body, and consume it #522
Conversation
| @@ -102,7 +103,7 @@ private[smithy4s] class SmithyHttp4sClientEndpointImpl[F[_], Op[_, _, _, _, _], | |||
| val baseRequest = Request[F](method, uri, headers = headers) | |||
| if (inputHasBody) { | |||
| baseRequest.withEntity(input) | |||
| } else baseRequest | |||
| } else baseRequest.withEntity(ByteVector.empty) | |||
There was a problem hiding this comment.
Should this not only apply to methods like PUT and POST?
There was a problem hiding this comment.
that's a good question, let me see what facilities exists here to distinguish what method allow or not bodies
There was a problem hiding this comment.
This is in ember:
private val NoPayloadMethods: Set[Method] =
Set(Method.GET, Method.DELETE, Method.CONNECT, Method.TRACE)Noticed that here
There was a problem hiding this comment.
it annoys me that I can't find a public equivalent for this and avoid copy/pasting that method Set out of ember :/
There was a problem hiding this comment.
.putHeaders(`Content-Length`.zero) might be another workaround. And I think this is ok for GET requests. IDK YMMV.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
happy to open it
There was a problem hiding this comment.
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.
modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sServerEndpoint.scala
Outdated
Show resolved
Hide resolved
| .toResource | ||
| .flatMap(port => | ||
| Port | ||
| .fromInt(port) |
There was a problem hiding this comment.
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)
modules/http4s/test/src-jvm-ce3/smithy4s/http4s/Http4sEmberPizzaClientSpec.scala
Outdated
Show resolved
Hide resolved
modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sServerEndpoint.scala
Outdated
Show resolved
Hide resolved
daddykotex
force-pushed
the
dfrancoeur/515-client-empty-test
branch
from
9b25f42
to
b699d7a
Compare
|
I've rebased on top of series/0.17 rather than trying to reconcile the two branches But I think the code in series/0.17 is somewhat problematic, in the sense, that my local githooks were failing for copyright notices missing on some generated files we should update the ci so that PR to 0.17 can trigger a CI run |
|
@daddykotex I've fixed it in my PR, you can apply the same change to |
|
I think I'll mark as draft and marge |
|
@daddykotex I think it's time to sync this and tag it ready |
modules/http4s/src/smithy4s/http4s/internals/SmithyHttp4sServerEndpoint.scala
Outdated
Show resolved
Hide resolved
Closes #515
I've decided to include a e2e test for ember because when fixed on both, client and server, the test is not flaky and I think its a long term win.
Equipped with http4s/http4s#4935 (comment), I implemented both the server fix and the client fix