Add withUrl(String) #268

htmldoug · 2018-07-31T04:51:27Z

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? (see below)
Have you updated the documentation for both Scala and Java sections?
Have you added tests for any changed functionality?

Fixes

Fixes #264

Purpose

Adds support in the scala API for changing the url of the WSRequest.

Background Context

Like @gmethvin, I've encountered a couple use cases where I'd like to modify the URL of an existing WSRequest.

Fixing URIs
In my WSRequestFilters, I'd like to make use of WSRequest.uri, but it's unsafe (see StandaloneWSClient.url(String) permits inconsistent states with query params #267). Today, I have a WSRequestFilter to sanitize the entire request prior to subsequent WSRequestFilters touching WSRequest.uri. In order to strip out query params and any other garbage from the URL, my WSRequestFilter has to rebuild the whole request from scratch. Additionally, it has to require a WSClient to do the job (wsClient.url()), which I hope is the same WSClient that originally used to build the request. Ugh!
Changing hostnames
We're cursed with https://github.com/mesosphere/marathon-lb, which sometimes fails to route requests. I'd love to be able to have a WSRequestFilter I can use to bypass that layer and rewrite https://pray-marathon-lb-works.example.com/foo to https://10.0.0.25/foo.

htmldoug · 2018-07-31T05:01:13Z

I see MiMa failing (correctly for 1.0.0). Is master the right branch for 2.0.0?

[error] (play-ws-standalone / mimaReportBinaryIssues) play-ws-standalone: Binary compatibility check failed!

htmldoug · 2018-08-13T15:05:33Z

@gmethvin @marcospereira, you tagged #264 as help wanted. Is this what you had in mind? What are next steps?

gmethvin · 2018-08-13T17:57:51Z

I think it looks fine, but we should also update the Java API. It's OK to mutate the variable in the Java API since the API is mutable. Ideally we'd have an immutable API for Java as well but it's probably not worth the breaking change.

htmldoug · 2018-08-13T18:05:31Z

we should also update the Java API. It's OK to mutate the variable in the Java API since the API is mutable.

Thanks, I'll do that today and push to this PR.

htmldoug · 2018-08-13T18:07:09Z

Speaking of breaking changes, adding the withUrl method to the trait counts as a breaking change from MiMa's perspective. (Other implementations may not have implemented it.) Would we merge this with a failing CI build? Any validations I should should be doing locally besides sbt test?

htmldoug · 2018-08-13T21:12:44Z

Java API updated. sbt test passes.

htmldoug · 2018-08-15T17:41:22Z

@gmethvin @marcospereira how's it look now?

gmethvin · 2018-08-15T18:09:07Z

I think this can go in the next major release, where we'd allow a breaking change. So you can add a MiMa exclusion, but we just don't want to backport.

marcospereira · 2018-08-16T20:40:47Z

LGTM.

We just need to add the mima filters here:

play-ws/build.sbt

Line 38 in bc74282

mimaBinaryIssueFilters ++= Seq(

Thanks for helping here, @htmldoug.

htmldoug · 2018-08-16T23:31:09Z

Done. Passing build. Holler if you want me to squash.

htmldoug · 2018-08-23T18:42:14Z

Bump. What's the next step for this PR?

htmldoug · 2018-08-31T01:48:51Z

@marcospereira can we get this in for 2.0.0-M4?

htmldoug · 2018-08-31T01:50:32Z

Freshly squashed per checklist.

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

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

htmldoug · 2018-08-31T16:18:34Z

Thanks for the review, @cchantep. Fixed.

htmldoug · 2018-09-04T21:54:45Z

Would you like any other changes or can this be merged?

marcospereira

Two minor changes.

Besides that, LGTM and we can merge after the changes. :-)

Thank you, @htmldoug.

marcospereira · 2018-09-04T22:18:28Z

play-ahc-ws-standalone/src/main/java/play/libs/ws/ahc/StandaloneAhcWSRequest.java

@@ -225,6 +225,12 @@ public StandaloneAhcWSRequest setContentType(String contentType) {
        return getHeader(CONTENT_TYPE);
    }

+    @Override
+    public StandaloneWSRequest setUrl(String url) {


Return StandaloneAhcWSRequest instead.

marcospereira · 2018-09-04T22:18:42Z

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

@@ -183,6 +183,8 @@ case class StandaloneAhcWSRequest(
    withMethod(method).execute()
  }

+  override def withUrl(url: String): StandaloneWSRequest = copy(url = url)


Return Self instead.

htmldoug · 2018-09-04T22:52:01Z

Done and squashed.

marcospereira

Thank you, @htmldoug.

And thanks for the patience here too.

htmldoug changed the title ~~Add withUrl()~~ Add withUrl(String) Jul 31, 2018

htmldoug force-pushed the withUrl branch from d98f4ee to 66fdadb Compare August 13, 2018 21:08

htmldoug force-pushed the withUrl branch from c373419 to e41336d Compare August 31, 2018 01:49

cchantep suggested changes Aug 31, 2018

View reviewed changes

play-ahc-ws-standalone/src/test/scala/play/libs/ws/ahc/AhcWSRequestSpec.scala Outdated Show resolved Hide resolved

play-ahc-ws-standalone/src/test/scala/play/api/libs/ws/ahc/AhcWSRequestSpec.scala Outdated Show resolved Hide resolved

htmldoug force-pushed the withUrl branch 2 times, most recently from 7894c84 to 8361d9a Compare August 31, 2018 16:16

jeffmay approved these changes Sep 4, 2018

View reviewed changes

marcospereira reviewed Sep 4, 2018

View reviewed changes

Add withUrl()

8a2d043

htmldoug force-pushed the withUrl branch from 8361d9a to 8a2d043 Compare September 4, 2018 22:51

marcospereira approved these changes Sep 5, 2018

View reviewed changes

marcospereira merged commit 2a51699 into playframework:master Sep 5, 2018

htmldoug deleted the withUrl branch September 6, 2018 23:54

htmldoug mentioned this pull request Sep 7, 2018

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add withUrl(String) #268

Add withUrl(String) #268

htmldoug commented Jul 31, 2018 •

edited

Loading

htmldoug commented Jul 31, 2018 •

edited

Loading

htmldoug commented Aug 13, 2018

gmethvin commented Aug 13, 2018

htmldoug commented Aug 13, 2018

htmldoug commented Aug 13, 2018

htmldoug commented Aug 13, 2018

htmldoug commented Aug 15, 2018

gmethvin commented Aug 15, 2018

marcospereira commented Aug 16, 2018

htmldoug commented Aug 16, 2018

htmldoug commented Aug 23, 2018

htmldoug commented Aug 31, 2018

htmldoug commented Aug 31, 2018

htmldoug commented Aug 31, 2018

htmldoug commented Sep 4, 2018

marcospereira left a comment

marcospereira Sep 4, 2018

marcospereira Sep 4, 2018

htmldoug commented Sep 4, 2018

marcospereira left a comment

Add withUrl(String) #268

Add withUrl(String) #268

Conversation

htmldoug commented Jul 31, 2018 • edited Loading

Pull Request Checklist

Fixes

Purpose

Background Context

htmldoug commented Jul 31, 2018 • edited Loading

htmldoug commented Aug 13, 2018

gmethvin commented Aug 13, 2018

htmldoug commented Aug 13, 2018

htmldoug commented Aug 13, 2018

htmldoug commented Aug 13, 2018

htmldoug commented Aug 15, 2018

gmethvin commented Aug 15, 2018

marcospereira commented Aug 16, 2018

htmldoug commented Aug 16, 2018

htmldoug commented Aug 23, 2018

htmldoug commented Aug 31, 2018

htmldoug commented Aug 31, 2018

htmldoug commented Aug 31, 2018

htmldoug commented Sep 4, 2018

marcospereira left a comment

Choose a reason for hiding this comment

marcospereira Sep 4, 2018

Choose a reason for hiding this comment

marcospereira Sep 4, 2018

Choose a reason for hiding this comment

htmldoug commented Sep 4, 2018

marcospereira left a comment

Choose a reason for hiding this comment

htmldoug commented Jul 31, 2018 •

edited

Loading

htmldoug commented Jul 31, 2018 •

edited

Loading