Akka-HTTP backend #208
Akka-HTTP backend #208
Conversation
Separate projects for the implmenetation and tests that are shared between the implementations.
|
Rebased on top of the current master to fix merge conflicts. |
| } | ||
| override def actorClass = classOf[InetAddressDnsResolver] | ||
| override def managerClass = classOf[SimpleDnsManager] | ||
| } |
| * | ||
| * @return the response body parsed as a String using the above algorithm. | ||
| */ | ||
| override def body: String = |
There was a problem hiding this comment.
I'm fine with this implementation but the comment is wrong because it's using UTF-8 all the time.
There was a problem hiding this comment.
The same applies to Java version.
| StandaloneAkkaHttpWSRequest(url) | ||
| } catch { | ||
| case ex: IllegalUriException => throw new IllegalArgumentException(ex.getMessage, ex) | ||
| case NonFatal(ex) => throw ex |
There was a problem hiding this comment.
Why do we need to catch and rethrow here?
|
|
||
| final class StandaloneAkkaHttpWSClient private ()( | ||
| implicit | ||
| val sys: ActorSystem, val mat: Materializer, val ctx: HttpsConnectionContext |
There was a problem hiding this comment.
Do we need the newline before this line?
| if (headers.containsKey(header.name())) { | ||
| headers.get(header.name()).add(header.value()); | ||
| } | ||
| else { |
There was a problem hiding this comment.
extra newline (we probably should set up a code formatter on this project...)
| */ | ||
| @Override | ||
| public Map<String, List<String>> getHeaders() { | ||
| final Map<String, List<String>> headers = new HashMap<>(); |
There was a problem hiding this comment.
Should this be a map with case-insensitive ordering?
| // Akka Http implementation of WS | ||
| //--------------------------------------------------------------- | ||
|
|
||
| lazy val `play-akka-http-ws-standalone` = project |
There was a problem hiding this comment.
I think it might be worth naming play-akka-http-ws-experimental, since this is still in development and we could possibly change APIs. WDYT @marcospereira?
There was a problem hiding this comment.
Agree.
It would be consistent with the way we changed from Netty to Akka HTTP on the server backend. It is also easier to set users expectations.
| */ | ||
| override def withHttpHeaders(headers: (String, String)*): Self = | ||
| copy(request = request.withHeaders(headers | ||
| .map { case (name, value) => HttpHeader.parse(name, value) } |
There was a problem hiding this comment.
It looks like this will just discard headers Akka HTTP considers invalid. Maybe we can warn instead?
| */ | ||
| override def headers: Map[String, Seq[String]] = | ||
| request.headers | ||
| .map(h => HttpHeader.parse(h.name().toLowerCase, h.value())) |
There was a problem hiding this comment.
Why are we parsing the headers again here?
| testOptions in Test := Seq(Tests.Argument(TestFrameworks.JUnit, "-a", "-v")) | ||
| ) | ||
| .settings( | ||
| // The scaladoc generation |
| @@ -0,0 +1,15 @@ | |||
| package akka.io | |||
There was a problem hiding this comment.
Any reason to have this inside akka.io package instead of play.api.ws?
| withClient() { client => | ||
| // FIXME configure ssl context with custom cert and enable SSL test for AHC | ||
| if (client.isInstanceOf[StandaloneAhcWSClient]) | ||
| success |
| } | ||
| } | ||
|
|
||
| "round trip XML" in { |
There was a problem hiding this comment.
Maybe also have a test for JSON?
| } | ||
| } | ||
|
|
||
| // FIXME this is different from Scala API but this is how AHC implementation is |
There was a problem hiding this comment.
This is a bug, but I think we can fix it after merging your PR.
| withClient() { client => | ||
| // FIXME configure ssl context with custom cert and enable SSL test for AHC | ||
| if (client.isInstanceOf[StandaloneAhcWSClient]) | ||
| success |
There was a problem hiding this comment.
Same as the Scala version. Should be pending.
| this.ctx = ctx; | ||
| } | ||
|
|
||
| /** |
| .equals(akka.http.scaladsl.model.ContentTypes.NoContentType()) ? null : response.entity().getContentType().toString(); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
No need to duplicate Java docs here since javadoc will do that already. The same applies to other methods and classes.
| final ResponseEntity strictEntity = response.entity() | ||
| .toStrict(UNMARSHAL_TIMEOUT.toMillis(), mat) | ||
| .toCompletableFuture() | ||
| .get(UNMARSHAL_TIMEOUT.toNanos(), TimeUnit.NANOSECONDS); |
There was a problem hiding this comment.
So this is also different from AHC implementation, right? I don't think we have a timeout when reading strict entities in AHC. Maybe we need a more descriptive exception message below to instruct users that they need to use getBodyAsSource.
|
Closing the akka-http backend related tickets for now. These will be handled in the future. |
This is a PR for a new Play WS backend based on Akka Http.
Currently it supports basic functionality. Missing bigger features for AHC backend parity are listed in #207
This PR adds a new project
play-akka-http-ws-standalonewhich is the Akka Http backend implementation. Tests covering the functionality have been added tointegration-testsproject and cover Akka Http and AHC backends for both Scala and Java APIs.You can also try the new backend by adding the following dependency to your project: