StandaloneWSClient.url(String) permits inconsistent states with query params

Summary

Behavior of query parameters in wsClient.url(String) is not well defined and permits inconsistent states. For example, wsClient.url("http://postman-echo.com/get?pipe=|").execute() successfully sends the request as http://postman-echo.com/get?pipe=%7C", but wsRequest.queryString returns an empty map, and wsRequest.uri throws a URISyntaxException.

This makes methods uri and queryString hard to reason about and use, since they may differ from what is actually sent or throw exceptions.

Behavior Example

/**
  * Demonstrates inconsistencies in [[StandaloneAhcWSClient.url()]].
  */
object UrlTest extends App {

  implicit val actorSystem = ActorSystem()
  implicit val materializer = ActorMaterializer()

  val wsClient = StandaloneAhcWSClient()
  val wsRequest = wsClient.url("http://postman-echo.com/get?pipe=|")
  val wsResponse = Await.result(wsRequest.execute(), 1.minute)
  println("Response body: " + wsResponse.body) // success!

  println("Request Query params: " + wsRequest.queryString) // empty, but the server disagrees.

  try {
    println("URI: " + wsRequest.uri) // throws! and yet, AHC figured it out somehow.
  } catch {
    case t: URISyntaxException => t.printStackTrace(System.out)
  }
  finally {
    wsClient.close()
    materializer.shutdown()
    actorSystem.terminate()
  }
}

Prints:

Response body: {"args":{"pipe":"|"},"headers":{"host":"postman-echo.com","accept":"*/*","user-agent":"AHC/2.0","x-forwarded-port":"80","x-forwarded-proto":"http"},"url":"http://postman-echo.com/get?pipe=%7C"}
Request Query params: Map()
java.net.URISyntaxException: Illegal character in query at index 33: http://postman-echo.com/get?pipe=|
	at java.net.URI$Parser.fail(URI.java:2848)
	at java.net.URI$Parser.checkChars(URI.java:3021)
	at java.net.URI$Parser.parseHierarchical(URI.java:3111)
	at java.net.URI$Parser.parse(URI.java:3053)
	at java.net.URI.<init>(URI.java:588)
	at play.api.libs.ws.ahc.StandaloneAhcWSRequest.uri$lzycompute(StandaloneAhcWSRequest.scala:59)
	at play.api.libs.ws.ahc.StandaloneAhcWSRequest.uri(StandaloneAhcWSRequest.scala:52)
	at play.api.libs.UrlTest$.delayedEndpoint$play$api$libs$UrlTest$1(UrlTest.scala:29)
	at play.api.libs.UrlTest$delayedInit$body.apply(UrlTest.scala:16)
	at scala.Function0.apply$mcV$sp(Function0.scala:34)
	at scala.Function0.apply$mcV$sp$(Function0.scala:34)
	at scala.runtime.AbstractFunction0.apply$mcV$sp(AbstractFunction0.scala:12)
	at scala.App.$anonfun$main$1$adapted(App.scala:76)
	at scala.collection.immutable.List.foreach(List.scala:389)
	at scala.App.main(App.scala:76)
	at scala.App.main$(App.scala:74)
	at play.api.libs.UrlTest$.main(UrlTest.scala:16)
	at play.api.libs.UrlTest.main(UrlTest.scala)

Wireshark sees:

GET /get?pipe=%7C HTTP/1.1
Host: postman-echo.com
Accept: */*
User-Agent: AHC/2.0

Observations

The scaladoc is unclear. "url" implies https://tools.ietf.org/html/rfc1738, and yet "base URL" implies maybe not?

/**
 * Generates a request.  Throws IllegalArgumentException if the URL is invalid.
 *
 * @param url The base URL to make HTTP requests to.
 * @return a request
 */
@throws[IllegalArgumentException]
def url(url: String): StandaloneWSRequest

Tolerant or Strict?

I think we'd benefit from committing to one of these two approaches and documenting.

Tolerant

The behavior of org.asynchttpclient.util.UriEncoder.FIXING could be pulled up to StandaloneAhcWSClient.url(String) to create something sane, parsing query params, perhaps decoding the values, and putting them in the queryString field. From there, lazy val uri: URI can figure it out and queryString would reflect reality.

Downside is that StandaloneWSRequest.copy(url = "") exists and would still allow for inconsistent states. At least it'd be better.

Scaladoc would be updated to indicate that the URI parameter means https://tools.ietf.org/html/rfc1738 and tolerantly accepts query string values either encoded or in plaintext.

Strict

StandaloneAhcWSRequest would throw if query params are present in url, which could be renamed baseUrl: String, and defined as "everything up to the query params".

A consistency oriented implementation could add require(uri.getQuery == null) which would force parsing of the `java.net.URI. This is a bit heavy to do every method call of the builder.

Alternatively, we could move this check into StandaloneAhcWSClient.validate().

Scaladoc would be updated to indicate that query params MUST NOT be part of the "baseUrl".

Workarounds

In my current production Play 2.5 services, I'm defensively decorating WSClient to intercept the WSRequest prior to execution, and running it through UriEncoder.FIXING just as AHC does under the hood, and moving any query params into the WSRequest.queryString field.

It's kludgy without https://github.com/playframework/play-ws/issues/264. I ended up requiring a WSClient and just rebuilding the whole request from scratch. I'd love for this to be handled upfront by the library so I don't have to second guess every request.

Relates to https://github.com/playframework/playframework/issues/7444.

Read StandaloneAhcWSClient.url(String), StandaloneAhcWSRequest.uri and queryString, and StandaloneAhcWSClient.validate(), then compare their handling of query parameters with the documented base-URL contract. Done means choosing either the tolerant or strict behavior, making request execution, uri, and queryString consistent, and updating the StandaloneWSClient scaladoc.