WSRequest: Normalize URL

[htmldoug]

Pull Request Checklist

• Have you read through the contributor guidelines?
• Have you signed the Typesafe CLA?
• Have you squashed your commits?
• Have you added copyright headers to new files?
• Have you checked that both Scala and Java APIs are updated?
• Have you updated the documentation for both Scala and Java sections?
• Have you added tests for any changed functionality?

Fixes

Fixes #267

Purpose

Ensure that StandaloneWSRequest.uri does not return invalid values or throw, by normalizing the url: String when a StandaloneWSRequest is constructed. Same approach is used by the java implementation.

Background Context

This approach follows the "tolerant" recommendation, #267 (comment).

Ensures:

1. StandaloneWSRequest.url path is encoded.
2. Any query params present in the URL are moved to StandaloneWSRequest.queryString.

Implementation is based upon a WSRequestFilter-like decorator I've been using in production since June 2018. An advantage to fixing the request right away is that request.uri can be invoked safely sooner in the process.

Caveats

Normalization can still be bypassed by:

• StandaloneAhcWSClient.copy(url = ">^..^<", ...)
• new StandaloneAhcWSRequest(client, ">^..^<", ...)

[htmldoug]

@marcospereira I've removed "WIP", and I'm ready for a review now, please.

[htmldoug]

@gmethvin @wsargent ready for review. I see your fingerprints here from #80 and #189.

[htmldoug]

@marcospereira @gmethvin @wsargent can I get a review please?

[wsargent]

I'm free this weekend, I can take a look at it

[wsargent]

So I can see from the PR that there's some use of UriEncoder.FIXING and the path and the query parameters are touched, but it's not clear in the code what the overall effect is.

[wsargent]

I think this is useful but it leaves some ambiguities into what encoding means and what the end result is.

Tying things down to a particular RFC and providing examples would help, as would "boundary conditions" showing the normalize method will cover and what it won't. More unit tests showing failed URLs and URLs which can't be recovered would be good too.

[wsargent]

Can we specify the RFC here used for the encoding?

[htmldoug]

I'm not sure we can.

I assume UriEncoder.FIXING returns an org.asynchttpclient.uri.Uri capable of producing an RFC 3986 compliant String. It makes no guarantees in the javadoc, but the private static constant names in Utf8UrlEncoder mention RFC3986. I'm not sure we can go so far as to claim that the Uri result conforms to the RFC though. The result seems closer to it at least.

The goal is just to get it closer to returning a valid java.net.URI, which claims to represent RFC 2396, "aside from minor deviations".

I've updated the scaladoc of this method to say that it runs it through UriEncoder.FIXING, which is at least 100% accurate.

[htmldoug]

@wsargent thanks for the review. I'll update with RFC links, examples, and more tests.

[htmldoug]

One concern: withUrl() calls normalize() here which could lead to infinite mutual recursion if I missed a case. I'd prefer to call the StandaloneAhcWSRequest.copy() escape hatch here, but it requires access to the implicit Materializer which is not accessible.

Some ideas in order of preference (least -> most):

• Leave it as is and let users report cases we missed. Should happen pretty quickly, but ugh.
• Remove the check from withUrl(). That'd weaken the integrity of StandaloneAhcWSRequest, although it's already compromisable with copy() if you provide your own Materializer.
• Change visibility to StandaloneAhcWSRequest(...)(implicit private[ahc] val materializer: Materializer) and do the copy().

[htmldoug]

I'm going to go ahead and push up a second commit with the last approach. If you like it, I'll squash. Otherwise, I'll drop it.

[htmldoug]

Done. Thoughts?

[htmldoug]

Thanks again for the review and sorry for the long delay. Holidays derailed me, but https://blog.playframework.com/play-2-7-0-rc9-released/ was a nice kick in the pants.

I've addressed all comments, and added benchmarks and more tests. Ready for another review.

[htmldoug]

@marcospereira @wsargent do you guys need anything else from me for this to be mergeable?

[htmldoug]

We're starting on the play 2.7 upgrade now. Any chance of getting this merged, or should we just port this to a wrapper and wrap all of our WSClients to keep WSRequest.uri from throwing runtime exceptions? It sure would be nice to avoid that.

[jeffmay]

Any update on this? I would like to upgrade to Play 2.7 with this as well, and I have the same issue that Doug is trying to solve here.

[htmldoug]

It's been ready to go since #288 (comment). I've rebased onto latest master, although after master bumped to play 2.8.0-M1, I'm not sure if this will be available for play 2.7.

@wsargent do you plan to re-review or should I close the PR?

[htmldoug]

Closing due to lack of review. I'll leave #267 open. I've put up a much smaller #353 to document current behavior.