WORK IN PROGRESS: Akka-HTTP backend by @2m #205

ktoso · 2017-12-14T14:49:37Z

Opening a PR of @2m's work just so it's easier to comment and review :)

THIS IS NOT YET INTENDED TO BE MERGED.

Separate projects for the implmenetation and tests that are shared between the implementations.

ktoso · 2017-12-14T15:11:37Z

integration-tests/src/test/scala/play/api/libs/ws/WSClientSpec.scala

+    "throw an exception on invalid url" in {
+      withClient() { client =>
+        { client.url("localhost") } must throwAn[IllegalArgumentException]
+      }


I see, consistent with existing WS I guess?

I'm not sure play-ws has a consistent behavior between Java and Scala APIs though.

So, it is consistent that both versions throw an Exception:

In Scala:

java.lang.IllegalArgumentException: Invalid URL localhost at play.api.libs.ws.ahc.StandaloneAhcWSClient.validate(StandaloneAhcWSClient.scala:84) at play.api.libs.ws.ahc.StandaloneAhcWSClient.url(StandaloneAhcWSClient.scala:42) ... 39 elided Caused by: java.lang.IllegalArgumentException: localhost could not be parsed into a proper Uri, missing scheme at play.shaded.ahc.org.asynchttpclient.uri.Uri.create(Uri.java:40) at play.shaded.ahc.org.asynchttpclient.uri.Uri.create(Uri.java:32) at play.api.libs.ws.ahc.StandaloneAhcWSClient.validate(StandaloneAhcWSClient.scala:81) ... 40 more

And in Java:

java.lang.RuntimeException: java.net.MalformedURLException: no protocol: localhost at play.libs.ws.ahc.StandaloneAhcWSRequest.<init>(StandaloneAhcWSRequest.java:89) at play.libs.ws.ahc.StandaloneAhcWSClient.url(StandaloneAhcWSClient.java:60) ... 39 elided Caused by: java.net.MalformedURLException: no protocol: localhost at java.net.URL.<init>(URL.java:593) at java.net.URL.<init>(URL.java:490) at java.net.URL.<init>(URL.java:439) at play.libs.ws.ahc.StandaloneAhcWSRequest.<init>(StandaloneAhcWSRequest.java:75) ... 40 more

The URL parsing happens immediately, not in the Future, but the exact exception wasn't specified in code (no @throws in the scala trait, at least).

The tests are only shared between the different backends but are separate for Scala and Java APIs. So the functionality is encoded per API basis and I have modified akka-http backend implementation to follow the behavior of AHC backend.

ktoso · 2017-12-14T15:43:00Z

integration-tests/src/test/scala/play/api/libs/ws/akkahttp/AkkaHttpWSClientSpec.scala

@@ -0,0 +1,16 @@
+package play.api.libs.ws.akkahttp


Ahh, right. Added manually. Would be best to use sbt-header plugin in the future.

ktoso · 2017-12-14T15:43:08Z

integration-tests/src/test/scala/play/api/libs/ws/akkahttp/AkkaHttpWSClientSpec.scala

+      client.close()
+    }
+  }
+}


So you did pull off the sharing tests :)

Yea. Tests are shared between the backends, but are different between the Scala and Java APIs.

It might be possible to share the tests between the Scala and Java APIs as well, but it would need another abstraction layer. Maybe implemented using FreeMonad. :)

ktoso · 2017-12-14T15:51:37Z

integration-tests/src/test/scala/play/api/libs/ws/WSRequestFilterSpec.scala

+      withClient() { client =>
+        val callList = scala.collection.mutable.ArrayBuffer[Int]()
+        client.url(s"http://localhost:$testServerPort")
+          .withRequestFilter(new CallbackRequestFilter(callList, 1))


are we sure that's safe? (concurrency wise)

oh heck no, that's a implementation for the spec.

Yea. For the test its ok, since every tests has its own immutable request filter.

ktoso · 2017-12-14T15:55:21Z

integration-tests/src/test/scala/play/api/libs/ws/akkahttp/AkkaHttpWSRequestFilterSpec.scala

+
+class AkkaHttpWSRequestFilterSpec(implicit val executionEnv: ExecutionEnv) extends WSRequestFilterSpec {
+  def withClient()(block: StandaloneWSClient => Result): Result = {
+    val client = StandaloneAkkaHttpWSClient()


I see, so our goal is I guess to just have people change their impl class here and it should be source compatible.
I thought we could do it as just a configuration flag which backend is to be used hm

When used in Play framework, the client is injected using the API interface (implementation non-specific) like so:

class Application @Inject() (ws: WSClient) extends Controller {}

So to use the akka-http backend client one will have to change the dependency in the project. Something like

<<< libraryDependencies += ws >>> libraryDependencies += wsAkkaHttp

or similar.

ktoso · 2017-12-14T16:23:42Z

...-akka-http-ws-standalone/src/main/java/play/libs/ws/akkahttp/StandaloneAkkaHttpWSClient.java

+   */
+  @Override
+  public Object getUnderlying() {
+    return null;


should be Http.get(system)?

Yes, added.

ktoso · 2017-12-14T16:24:52Z

...-akka-http-ws-standalone/src/main/java/play/libs/ws/akkahttp/StandaloneAkkaHttpWSClient.java

+  @Override
+  public void close() throws IOException {
+
+  }


Could be httpshutdownAllConnectionPools()?

Yes, added.

ktoso · 2017-12-14T16:25:30Z

...-akka-http-ws-standalone/src/main/java/play/libs/ws/akkahttp/StandaloneAkkaHttpWSClient.java

+
+  private final ActorSystem sys;
+  private final Materializer mat;
+


Store the val http = Http() created from those and pass it into the StandaloneAkkaHttpWSRequest.
This way if someone calls the close here the right things would be closed

As Http is an extension, Http.get(sys).shutdownAllConnectionPools() in close is going to be the same as using the stored reference, right?

ktoso · 2017-12-14T16:25:40Z

...kka-http-ws-standalone/src/main/java/play/libs/ws/akkahttp/StandaloneAkkaHttpWSResponse.java

+   */
+  @Override
+  public Map<String, List<String>> getHeaders() {
+    return null;


Yea. :) I treat return null and ??? as fixme's currently. WIll not merge until any of these are left.

ktoso · 2017-12-14T16:26:35Z

...kka-http-ws-standalone/src/main/java/play/libs/ws/akkahttp/StandaloneAkkaHttpWSResponse.java

+import java.util.Map;
+import java.util.Optional;
+
+public final class StandaloneAkkaHttpWSResponse implements StandaloneWSResponse {


Seems a bit inversed? Thought this would wrap the Akka HTTP HttpResponse?

Yes. Wrapping Scala API did not fly. Now it wraps akka.http.javadsl.model.HttpResponse

ktoso · 2017-12-15T09:15:23Z

...tp-ws-standalone/src/main/scala/play/api/libs/ws/akkahttp/StandaloneAkkaHttpWSResponse.scala

+  /**
+   * Get the underlying response object.
+   */
+  override def underlying[T]: T = ???


return the response?

ktoso · 2017-12-15T09:15:44Z

...tp-ws-standalone/src/main/scala/play/api/libs/ws/akkahttp/StandaloneAkkaHttpWSResponse.scala

+
+final class StandaloneAkkaHttpWSResponse private (val response: HttpResponse)(implicit val mat: Materializer) extends StandaloneWSResponse {
+
+  final val UnmarshalTimeout = 1.second


could it be configurable somehow?

Configuration still needs to be tackled. Noted in the mega ticket.

ktoso · 2017-12-15T09:18:07Z

...tp-ws-standalone/src/main/scala/play/api/libs/ws/akkahttp/StandaloneAkkaHttpWSResponse.scala

+  /**
+   * The response status message.
+   */
+  override def statusText: String = ???


That's the reason field for us : scala> akka.http.scaladsl.model.StatusCodes.OK.reason

ktoso · 2017-12-15T09:26:57Z

...tp-ws-standalone/src/main/scala/play/api/libs/ws/akkahttp/StandaloneAkkaHttpWSResponse.scala

+   */
+  override def body: String = {
+    import akka.http.scaladsl.unmarshalling.PredefinedFromEntityUnmarshallers.stringUnmarshaller
+    Await.result(Unmarshal(response).to[String], UnmarshalTimeout)


I would rather suggest that we do a toStrict on the response, and store the strictified entity.
The current impl would blow up if we do r.body; r.body I think?

Yes, I agree, that will only work once. We first need to clarify the semantics whether these methods can be called several times and then figure out how to be able to make it work.

I think play people value convenience a lot.in this API... perhaps we can do some cheating with a CAS ing state machine here? like, here spin a future and await hehe other to drain? a hack, but it would get us to ship this WS impl. and once we do the smarter things in Akka http we could remove the hack here?

It could probably just be a lazy val that collects all the stuff. The question is in which way, so that the below methods would also work. That's what I meant with "clarify the semantics", i.e. whether it is allowed to call one or more of those body interpreting/accessing methods and whether it is allowed to call them only once or several times.

responded only seeing the first comment

Yea, current impl will blow up with such a testcase and needs to be fixed.

I am not sure about calling toStrict every time. If the user is using the client for streamed responses and is materializing stream from bodyAsSource only once then we should not waste resources with by calling toStrict internally. Take a look at 8d4a2e8 where I store strict response only when getBody or getBodyBytes is called.

ktoso · 2017-12-15T09:27:09Z

...tp-ws-standalone/src/main/scala/play/api/libs/ws/akkahttp/StandaloneAkkaHttpWSResponse.scala

+  /**
+   * @return the response as a source of bytes
+   */
+  override def bodyAsSource: Source[ByteString, _] = response.entity.dataBytes


ktoso · 2017-12-15T09:55:03Z

...ttp-ws-standalone/src/main/scala/play/api/libs/ws/akkahttp/StandaloneAkkaHttpWSRequest.scala

+  /**
+   * Execute this request
+   */
+  override def execute(): Future[Response] = {


Good to ask play team about semantics here.
Does execute mean strict and stream "streaming" as our default mode is?
If so, then we should rather implement the stream() and the execute() should be implemented in terms of it, by applying a toStrict()?

Does execute mean strict and stream "streaming" as our default mode is?

Yes. We should make this API clearer later.

Yes. That makes sense.

ktoso · 2017-12-15T10:10:25Z

...ttp-ws-standalone/src/main/scala/play/api/libs/ws/akkahttp/StandaloneAkkaHttpWSRequest.scala

+   * @return a future with the response for the PATCH request
+   */
+  override def patch[T: BodyWritable](body: T): Future[Response] =
+    withBody(body).execute("PATCH")


ktoso · 2017-12-15T10:10:55Z

...ttp-ws-standalone/src/main/scala/play/api/libs/ws/akkahttp/StandaloneAkkaHttpWSRequest.scala

+  /**
+   * Sets the proxy server to use in this request
+   */
+  override def withProxyServer(proxyServer: WSProxyServer): Self = ???


missing feature in our impl right? AFAIR we still only do https proxy but not any http?

Add fixme and link to ticket in akka-http

ktoso · 2017-12-15T10:17:27Z

...ttp-ws-standalone/src/main/scala/play/api/libs/ws/akkahttp/StandaloneAkkaHttpWSRequest.scala

+  /**
+   * Sets whether redirects (301, 302) should be followed automatically
+   */
+  override def withFollowRedirects(follow: Boolean): Self = ???


Missing Akka HTTP feature, please link to the ticket from here :)

And let's mark the ticket as "play integration" (we have such tag)

ignasi35 · 2017-12-15T10:36:38Z

project/Dependencies.scala

@@ -47,15 +47,20 @@ object Dependencies {

  val akkaVersion = "2.5.3"
  val akkaStreams = Seq("com.typesafe.akka" %% "akka-stream" % akkaVersion)
-  val akkaHttp = Seq("com.typesafe.akka" %% "akka-http" % "10.0.8")
+  val akkaHttp = Seq("com.typesafe.akka" %% "akka-http" % "10.0.11")
+  val akkaHttpCore = Seq("com.typesafe.akka" %% "akka-http" % "10.0.11")


Is this duplication intentional? (akkaHttp is used, at least once, in build.sbt but not in Dependencies)

If the duplication is intentional, what is the difference between akkaHttp and akkaHttpCore?

Sorry, messed this up. My idea was to make the new akka-http backend depend only on akka-http-core. But later I needed more (marshalling) from akka-http. Fixed in 108ee30

jrudolph · 2017-12-18T13:23:49Z

...akka-http-ws-standalone/src/main/java/play/libs/ws/akkahttp/StandaloneAkkaHttpWSRequest.java

+   */
+  @Override
+  public CompletionStage<? extends StandaloneWSResponse> get() {
+    return execute("GET");


Could we use the constant values here where we know it statically like HttpMethods.GET?

Done in f00d8b9

jrudolph · 2017-12-18T13:43:32Z

...akka-http-ws-standalone/src/main/java/play/libs/ws/akkahttp/StandaloneAkkaHttpWSRequest.java

+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+
+public final class StandaloneAkkaHttpWSRequest implements StandaloneWSRequest {


I think we talked about that already, but is there any good reason to implement these classes in Java? Keeping the interfaces written in Java might make sense but the implementation could still be done in Scala, right? If that's not possible I would still prefer an approach where the Java classes just delegate to the real implementation which could then be written in Scala.

This is not possible that easily:

This was an expirment i did in the past: https://github.com/schmitch/play-ws-akka
The biggest probem is that if something returns java.lang.Boolean instead of boolean or everything that returns a Future/CompletionStage I created a WrappedFuture to support that: https://github.com/schmitch/play-ws-akka/blob/master/play-ws-akka-standalone/src/main/scala/play/api/libs/ws/akkaws/WrappedFuture.scala Basically all overlapping types, might be problematic.

The delegate approach might be more feasible.

At least doing it this way uncovered some missing Java API methods in Akka Http.

is there any good reason to implement these classes in Java

Similar to what @2m said, the main reason is idiom -- if you're changing or adding to the API in Java but writing the tests in Scala, you often don't feel the pain of refactoring because you can leverage the Scala Collections API. It also helps for discovery, because Java users don't have to decipher Scala tests to see how the code should work, and writing documentation, because the tests and examples match up.

I was only talking about implementation. Interfaces and tests could and probably should be done in Java, I agree.

2m · 2017-12-18T15:04:43Z

Addressed review feedback. Will continue implementing missing features and adding tests further.

2m · 2017-12-20T15:25:29Z

This PR now contains everything, except the features listed in #207

I have also published a JAR from this PR for anyone that would like to try it out:

resolvers += Resolver.bintrayRepo("2m", "maven")
libraryDependencies += "com.typesafe.play" %% "play-akka-http-ws-standalone" % "1.0.0+62-7728dc21"

This PR does not touch the existing implementation, so after a review it can be safely merged and the work can continue in separate PRs.

PromanSEW · 2017-12-20T15:31:20Z

project/Dependencies.scala

@@ -47,7 +47,7 @@ object Dependencies {

  val akkaVersion = "2.5.3"


Yes, we could update it. But should probably be done in a separate PR.

2m · 2017-12-22T12:41:49Z

Added one more commit which adds a HTTPS test and also allows to use a custom HTTPS connection context.

This is ready to be reviewed and merged so the following work for implementing #207 can be done as separate PRs.

ktoso · 2017-12-22T12:43:32Z

Closing, Martynas will submit the same branch as not-WIP PR :)

2m · 2017-12-22T12:49:38Z

Follow up PR: #208

2m added 11 commits December 11, 2017 18:02

A prototype of Akka Http backend

04a343c

Separate projects for the implmenetation and tests that are shared between the implementations.

Cleanup integration tests

ca239d2

Implement request filters

a39487c

Add missing type annotations

62dcee1

Add test cases from Java api integration tests

2376860

All http methods

b2c7c6b

Authentication support

b7103af

Virtualhost support

d6b84aa

Request timeout support

71b1084

First part of the Java API implementation

d2053e4

Ported all scala integration tests over java implementation

146d19b

ktoso commented Dec 14, 2017

View reviewed changes

ktoso commented Dec 15, 2017

View reviewed changes

ignasi35 reviewed Dec 15, 2017

View reviewed changes

2m added 5 commits December 15, 2017 21:07

Java API implementation using javadsl Akka Http API

bc6d919

Port RequestFilter spec to Java backends

4c5776a

Refactor Scala API RequestFilter tests

1673b43

Add missing copyright headers

d27d7e4

Implement

be2d6c8

2m mentioned this pull request Dec 18, 2017

MEGA ticket for Akka Http backend related tickets #207

Closed

12 tasks

2m added 2 commits December 18, 2017 19:56

Mark unimplemented features as FIXMEs

4316dad

Implement get status text

ae8454d

jrudolph reviewed Dec 18, 2017

View reviewed changes

2m added 3 commits December 18, 2017 21:29

Save strict response if response is unmarshalled

8d4a2e8

Fix dependencies

108ee30

Use Http Method name constants

f00d8b9

2m added 14 commits December 19, 2017 15:11

Fix race condition in test setup

04aa224

Remove api converters from a failed previous attempt

16315ae

Fix Scala 2.11 compilation

cd056c9

Implement cookies

531e8ab

Complete request timeout support

58e4f70

Complete virtual host support

8ec6d3c

Implement auth getters

1c3094d

Implement getters for url, method, content type and body

e9df202

Fix content type spec

4b949bf

Typo fix

89e8c52

Implement content type getter on response

4ca4c1a

Port AHC unit tests as integration tests for all APIs and backends

507df12

Remove unnecessary dependency

8bb62c0

Remove unused imports

b6eb5a0

PromanSEW reviewed Dec 20, 2017

View reviewed changes

Https tests and context parameter

a1160df

ktoso closed this Dec 22, 2017


		private final ActorSystem sys;
		private final Materializer mat;


		final class StandaloneAkkaHttpWSResponse private (val response: HttpResponse)(implicit val mat: Materializer) extends StandaloneWSResponse {

		final val UnmarshalTimeout = 1.second

		@@ -47,7 +47,7 @@ object Dependencies {

		val akkaVersion = "2.5.3"

WORK IN PROGRESS: Akka-HTTP backend by @2m #205

WORK IN PROGRESS: Akka-HTTP backend by @2m #205

Conversation

ktoso commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2m Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2m Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmitch Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2m commented Dec 18, 2017

2m commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

2m commented Dec 22, 2017

ktoso commented Dec 22, 2017

2m commented Dec 22, 2017

2m Dec 18, 2017 •

edited

Loading

2m Dec 18, 2017 •

edited

Loading

schmitch Dec 18, 2017 •

edited

Loading