WSRequest: Normalize URL #288

htmldoug · 2018-10-29T03:43:32Z

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:

StandaloneWSRequest.url path is encoded.
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 · 2018-11-18T00:18:10Z

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

htmldoug · 2018-11-29T07:50:15Z

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

htmldoug · 2018-12-09T00:37:54Z

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

wsargent · 2018-12-13T21:10:27Z

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.

play-ahc-ws-standalone/src/main/scala/play/api/libs/ws/ahc/StandaloneAhcWSClient.scala

wsargent · 2018-12-17T04:07:39Z

play-ahc-ws-standalone/src/main/scala/play/api/libs/ws/ahc/StandaloneAhcWSClient.scala

+  }
+
+  /**
+   * Builds an AHC [[Uri]] with all parts URL encoded.


Can we specify the RFC here used for the encoding?

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.

play-ahc-ws-standalone/src/main/scala/play/api/libs/ws/ahc/StandaloneAhcWSClient.scala

play-ahc-ws-standalone/src/test/scala/play/api/libs/ws/ahc/AhcWSRequestSpec.scala

htmldoug · 2018-12-17T05:06:28Z

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

htmldoug · 2019-01-22T02:44:08Z

play-ahc-ws-standalone/src/main/scala/play/api/libs/ws/ahc/StandaloneAhcWSClient.scala

+    } yield key -> value
+
+    req
+      .withUrl(urlNoQueryParams)


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().

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.

Done. Thoughts?

htmldoug · 2019-01-22T02:48:09Z

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 · 2019-01-31T21:08:03Z

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

htmldoug · 2019-04-07T23:48:17Z

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 · 2019-06-03T18:46:51Z

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 · 2019-06-03T19:44:39Z

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 · 2019-06-10T19:57:32Z

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

htmldoug force-pushed the uri-fixing branch from 64e02f9 to aef3546 Compare October 29, 2018 15:47

htmldoug force-pushed the uri-fixing branch from aef3546 to 587c5db Compare November 18, 2018 00:13

htmldoug changed the title ~~WIP: WSRequest: Normalize URL~~ WSRequest: Normalize URL Nov 18, 2018

wsargent self-requested a review December 13, 2018 21:10

wsargent reviewed Dec 17, 2018

View reviewed changes

wsargent suggested changes Dec 17, 2018

View reviewed changes

htmldoug force-pushed the uri-fixing branch 7 times, most recently from 7ae88ee to 8a8eb17 Compare January 22, 2019 02:26

htmldoug commented Jan 22, 2019

View reviewed changes

htmldoug force-pushed the uri-fixing branch from 8a8eb17 to 23cbbac Compare January 22, 2019 03:32

htmldoug mentioned this pull request Jan 22, 2019

Optimize StandaloneAhcWSRequest.add* methods. #301

Merged

7 tasks

htmldoug force-pushed the uri-fixing branch from dbef8b9 to 4a42bf5 Compare February 8, 2019 23:34

dwijnand added the status:backlog label Feb 14, 2019

WSRequest: Normalize URL

dbcc1c1

htmldoug force-pushed the uri-fixing branch from 5e62d35 to dbcc1c1 Compare June 3, 2019 19:17

htmldoug mentioned this pull request Jun 10, 2019

Document that uri throws. #353

Closed

7 tasks

htmldoug closed this Jun 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WSRequest: Normalize URL #288

WSRequest: Normalize URL #288

htmldoug commented Oct 29, 2018 •

edited

Loading

htmldoug commented Nov 18, 2018

htmldoug commented Nov 29, 2018

htmldoug commented Dec 9, 2018

wsargent commented Dec 13, 2018

wsargent left a comment

wsargent left a comment

wsargent Dec 17, 2018

htmldoug Jan 22, 2019 •

edited

Loading

htmldoug commented Dec 17, 2018

htmldoug Jan 22, 2019 •

edited

Loading

htmldoug Jan 23, 2019

htmldoug Jan 23, 2019

htmldoug commented Jan 22, 2019

htmldoug commented Jan 31, 2019

htmldoug commented Apr 7, 2019 •

edited

Loading

jeffmay commented Jun 3, 2019

htmldoug commented Jun 3, 2019 •

edited

Loading

htmldoug commented Jun 10, 2019

WSRequest: Normalize URL #288

WSRequest: Normalize URL #288

Conversation

htmldoug commented Oct 29, 2018 • edited Loading

Pull Request Checklist

Fixes

Purpose

Background Context

Caveats

htmldoug commented Nov 18, 2018

htmldoug commented Nov 29, 2018

htmldoug commented Dec 9, 2018

wsargent commented Dec 13, 2018

wsargent left a comment

Choose a reason for hiding this comment

wsargent left a comment

Choose a reason for hiding this comment

wsargent Dec 17, 2018

Choose a reason for hiding this comment

htmldoug Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

htmldoug commented Dec 17, 2018

htmldoug Jan 22, 2019 • edited Loading

Choose a reason for hiding this comment

htmldoug Jan 23, 2019

Choose a reason for hiding this comment

htmldoug Jan 23, 2019

Choose a reason for hiding this comment

htmldoug commented Jan 22, 2019

htmldoug commented Jan 31, 2019

htmldoug commented Apr 7, 2019 • edited Loading

jeffmay commented Jun 3, 2019

htmldoug commented Jun 3, 2019 • edited Loading

htmldoug commented Jun 10, 2019

htmldoug commented Oct 29, 2018 •

edited

Loading

htmldoug Jan 22, 2019 •

edited

Loading

htmldoug Jan 22, 2019 •

edited

Loading

htmldoug commented Apr 7, 2019 •

edited

Loading

htmldoug commented Jun 3, 2019 •

edited

Loading