playframework/play-ws

Unable to send requests with literal '+' character in URI query part

joschi opened this issue · 8 comments

joschi commented

I want to send a request to a remote HTTP service which has some strict encoding rules.

For example, HTTP requests to http://example.com/path?query+string are working, but requests to http://example.com/path?query%2Bstring are not working.

Unfortunately, Play WS always encodes the URL as http://example.com/path?query%2Bstring without the option to skip unnecessary encoding or to provide a raw query string.

Play WS Version

Play WS 2.6.20

$ sbt dependencyList|grep -E 'play-(ahc|ws)|asynchttp'
[info] com.typesafe.play:play-ahc-ws-standalone_2.12:1.1.10
[info] com.typesafe.play:play-ahc-ws_2.12:2.6.20
[info] com.typesafe.play:play-ws-standalone-json_2.12:1.1.10
[info] com.typesafe.play:play-ws-standalone-xml_2.12:1.1.10
[info] com.typesafe.play:play-ws-standalone_2.12:1.1.10
[info] com.typesafe.play:play-ws_2.12:2.6.20
[info] com.typesafe.play:shaded-asynchttpclient:1.1.10

API (Scala / Java / Neither / Both)

Java

Operating System

MacOS 10.13.6

$ uname -rspv
Darwin 17.7.0 Darwin Kernel Version 17.7.0: Thu Jun 21 22:53:14 PDT 2018; root:xnu-4570.71.2~1/RELEASE_X86_64 i386

JDK

java version "1.8.0_191"
Java(TM) SE Runtime Environment (build 1.8.0_191-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.191-b12, mixed mode)

Expected Behavior

  1. Create WSRequest with valid URL via WSClient#url(String), for example http://example.com/path?query+string.
  2. Execute HTTP request via WSRequest#get().
  3. HTTP request is being sent to the URL provided to WSRequest (http://example.com/path?query+string).

Actual Behavior

  1. Create WSRequest with valid URL via WSClient#url(String), for example http://example.com/path?query+string.
  2. Execute HTTP request via WSRequest#get().
  3. HTTP request is being sent to an incorrectly (or too eagerly) encoded URL (http://example.com/path?query%2Bstring).

Reproducible Test Case

import com.github.tomakehurst.wiremock.junit.WireMockRule;
import org.junit.Rule;
import org.junit.Test;
import play.libs.ws.WSClient;
import play.libs.ws.WSRequest;
import play.libs.ws.WSResponse;
import play.libs.ws.ahc.AhcWSClient;
import play.shaded.ahc.org.asynchttpclient.DefaultAsyncHttpClient;

import static com.github.tomakehurst.wiremock.client.WireMock.*;
import static org.junit.Assert.assertEquals;

public class WSRequestQueryParametersTest {
    @Rule
    public WireMockRule wireMockRule = new WireMockRule();

    @Test
    public void uriEncode() throws Exception {
        WSClient wsClient = new AhcWSClient(new DefaultAsyncHttpClient(), null);
        String requestUrl = wireMockRule.url("/path") + "?the+plus+must+remain";
        stubFor(get(urlEqualTo("/path?the+plus+must+remain")).willReturn(aResponse()
                .withStatus(200)
                .withBody("OK")));

        WSRequest wsRequest = wsClient.url(requestUrl);
        String url = wsRequest.getUrl();
        assertEquals("Request URL and WSRequest.Url should be identical", requestUrl, url);

        WSResponse wsResponse = wsRequest.get().toCompletableFuture().get();
        assertEquals(200, wsResponse.getStatus());

        verify(getRequestedFor(urlMatching("/path")).withQueryParam("the+plus+must+remain", equalTo("")));
    }
}
2018-10-26 15:31:57.070 +0200 [] [INFO] Logging initialized @1786ms to org.eclipse.jetty.util.log.Slf4jLog  
2018-10-26 15:31:57.248 +0200 [] [INFO] jetty-9.4.12.v20180830; built: 2018-08-30T13:59:14.071Z; git: 27208684755d94a92186989f695db2d7b21ebc51; jvm 1.8.0_181-b13  
2018-10-26 15:31:57.278 +0200 [] [INFO] Started o.e.j.s.ServletContextHandler@5b218417{/__admin,null,AVAILABLE}  
2018-10-26 15:31:57.281 +0200 [] [INFO] Started o.e.j.s.ServletContextHandler@413d1baf{/,null,AVAILABLE}  
2018-10-26 15:31:57.325 +0200 [] [INFO] Started NetworkTrafficServerConnector@31995969{HTTP/1.1,[http/1.1]}{0.0.0.0:8080}  
2018-10-26 15:31:57.326 +0200 [] [INFO] Started @2045ms  
2018-10-26 15:31:58.032 +0200 [] [INFO] RequestHandlerClass from context returned com.github.tomakehurst.wiremock.http.AdminRequestHandler. Normalized mapped under returned 'null'  
2018-10-26 15:31:58.379 +0200 [] [INFO] RequestHandlerClass from context returned com.github.tomakehurst.wiremock.http.StubRequestHandler. Normalized mapped under returned 'null'  
2018-10-26 15:31:58.395 +0200 [] [ERROR] 
                                               Request was not matched
                                               =======================

-----------------------------------------------------------------------------------------------------------------------
| Closest stub                                             | Request                                                  |
-----------------------------------------------------------------------------------------------------------------------
                                                           |
GET                                                        | GET
/path?the+plus+must+remain                                 | /path?the%2Bplus%2Bmust%2Bremain                    <<<<< URL does not match
                                                           |
                                                           |
-----------------------------------------------------------------------------------------------------------------------
  
2018-10-26 15:31:58.575 +0200 [] [INFO] Stopped NetworkTrafficServerConnector@31995969{HTTP/1.1,[http/1.1]}{0.0.0.0:8080}  
2018-10-26 15:31:58.577 +0200 [] [INFO] Stopped o.e.j.s.ServletContextHandler@413d1baf{/,null,UNAVAILABLE}  
2018-10-26 15:31:58.577 +0200 [] [INFO] Stopped o.e.j.s.ServletContextHandler@5b218417{/__admin,null,UNAVAILABLE}  

java.lang.AssertionError: 
Expected :200
Actual   :404
bpossolo commented

you're using the api incorrectly.
the url that you pass to WSClient.url() should only contain the path.
if you want to attach query params then you do that after like so:

WSRequest request = ws
  .url("/api/v1/foo")
  .addQueryParameter("name", "bob jenkins");

the addQueryParameter method will url encode the value
and the resultant url will be:

/api/v1/foo?name=bob%20jenkins

your server should be able to parse this just fine because, within a query string, + and %20 are semantically the same (they represent an encoded space)

in other words these two urls are to be considered effectively the same

/api/v1/foo?name=bob%20jenkins
/api/v1/foo?name=bob+jenkins

in both cases, the server must parse the query string then url decode the value to get the original bob jenkins

joschi commented

@bpossolo Thank you very much for your reply!

the url that you pass to WSClient.url() should only contain the path.

If this was the case, the documentation for that method has to be updated.
It currently says in WSClient#url(String):

Parameters:
url - the URL to request

And by "URL", I would expect it to accept a URL according to RFC 1738, section 3.3 or RFC 3986.

your server should be able to parse this just fine because, within a query string, + and %20 are semantically the same (they represent an encoded space)

You assume that I control the remote server, which is not the case. While %20 and + might both be used to encode a whitespace character, they're making for distinct URLs.

the addQueryParameter method will url encode the value

Yes, and that's the problem. It shouldn't encode "bob+jenkins" (which is perfectly fine) to "bob%2Bjenkins". It's not possible to create an HTTP request to a URL which contains a literal '+' character.

If it is possible, please show me how.

Here's the updated test with the changes you've recommended:

import com.github.tomakehurst.wiremock.junit.WireMockRule;
import org.junit.Rule;
import org.junit.Test;
import play.libs.ws.WSClient;
import play.libs.ws.WSRequest;
import play.libs.ws.WSResponse;
import play.libs.ws.ahc.AhcWSClient;
import play.shaded.ahc.org.asynchttpclient.DefaultAsyncHttpClient;

import static com.github.tomakehurst.wiremock.client.WireMock.*;
import static org.junit.Assert.assertEquals;

public class WSRequestQueryParametersTest {
    @Rule
    public WireMockRule wireMockRule = new WireMockRule();

    @Test
    public void uriEncode() throws Exception {
        WSClient wsClient = new AhcWSClient(new DefaultAsyncHttpClient(), null);
        String requestUrl = wireMockRule.url("/path");
        stubFor(get(urlEqualTo("/path?the+plus+must+remain")).willReturn(aResponse()
                .withStatus(200)
                .withBody("OK")));

        WSRequest wsRequest = wsClient.url(requestUrl)
                .addQueryParameter("the+plus+must+remain", "");
        String url = wsRequest.getUrl();
        assertEquals("Request URL and WSRequest.Url should be identical", requestUrl, url);

        WSResponse wsResponse = wsRequest.get().toCompletableFuture().get();
        assertEquals(200, wsResponse.getStatus());

        verify(getRequestedFor(urlMatching("/path")).withQueryParam("the+plus+must+remain", equalTo("")));
    }

    @Test
    public void uriEncode2() throws Exception {
        WSClient wsClient = new AhcWSClient(new DefaultAsyncHttpClient(), null);
        String requestUrl = wireMockRule.url("/path");
        stubFor(get(urlEqualTo("/path?the+plus+must+remain")).willReturn(aResponse()
                .withStatus(200)
                .withBody("OK")));

        WSRequest wsRequest = wsClient.url(requestUrl)
                .setQueryString("the+plus+must+remain");
        String url = wsRequest.getUrl();
        assertEquals("Request URL and WSRequest.Url should be identical", requestUrl, url);

        WSResponse wsResponse = wsRequest.get().toCompletableFuture().get();
        assertEquals(200, wsResponse.getStatus());

        verify(getRequestedFor(urlMatching("/path")).withQueryParam("the+plus+must+remain", equalTo("")));
    }
}

Failing output:

2018-10-30 09:06:05.630 +0100 [] [INFO] Logging initialized @1960ms to org.eclipse.jetty.util.log.Slf4jLog  
2018-10-30 09:06:05.805 +0100 [] [INFO] jetty-9.4.12.v20180830; built: 2018-08-30T13:59:14.071Z; git: 27208684755d94a92186989f695db2d7b21ebc51; jvm 1.8.0_181-b13  
2018-10-30 09:06:05.829 +0100 [] [INFO] Started o.e.j.s.ServletContextHandler@5b218417{/__admin,null,AVAILABLE}  
2018-10-30 09:06:05.831 +0100 [] [INFO] Started o.e.j.s.ServletContextHandler@413d1baf{/,null,AVAILABLE}  
2018-10-30 09:06:05.874 +0100 [] [INFO] Started NetworkTrafficServerConnector@3f53d667{HTTP/1.1,[http/1.1]}{0.0.0.0:8080}  
2018-10-30 09:06:05.874 +0100 [] [INFO] Started @2207ms  
2018-10-30 09:06:06.567 +0100 [] [INFO] RequestHandlerClass from context returned com.github.tomakehurst.wiremock.http.AdminRequestHandler. Normalized mapped under returned 'null'  
2018-10-30 09:06:06.951 +0100 [] [INFO] RequestHandlerClass from context returned com.github.tomakehurst.wiremock.http.StubRequestHandler. Normalized mapped under returned 'null'  
2018-10-30 09:06:06.976 +0100 [] [ERROR] 
                                               Request was not matched
                                               =======================

-----------------------------------------------------------------------------------------------------------------------
| Closest stub                                             | Request                                                  |
-----------------------------------------------------------------------------------------------------------------------
                                                           |
GET                                                        | GET
/path?the+plus+must+remain                                 | /path?the%2Bplus%2Bmust%2Bremain=                   <<<<< URL does not match
                                                           |
                                                           |
-----------------------------------------------------------------------------------------------------------------------
  
2018-10-30 09:06:07.149 +0100 [] [INFO] Stopped NetworkTrafficServerConnector@3f53d667{HTTP/1.1,[http/1.1]}{0.0.0.0:8080}  
2018-10-30 09:06:07.150 +0100 [] [INFO] Stopped o.e.j.s.ServletContextHandler@413d1baf{/,null,UNAVAILABLE}  
2018-10-30 09:06:07.151 +0100 [] [INFO] Stopped o.e.j.s.ServletContextHandler@5b218417{/__admin,null,UNAVAILABLE}  

java.lang.AssertionError: 
Expected :200
Actual   :404



2018-10-30 09:06:07.163 +0100 [] [INFO] jetty-9.4.12.v20180830; built: 2018-08-30T13:59:14.071Z; git: 27208684755d94a92186989f695db2d7b21ebc51; jvm 1.8.0_181-b13  
2018-10-30 09:06:07.164 +0100 [] [INFO] Started o.e.j.s.ServletContextHandler@cf65451{/__admin,null,AVAILABLE}  
2018-10-30 09:06:07.164 +0100 [] [INFO] Started o.e.j.s.ServletContextHandler@724f138e{/,null,AVAILABLE}  
2018-10-30 09:06:07.165 +0100 [] [INFO] Started NetworkTrafficServerConnector@37eeec90{HTTP/1.1,[http/1.1]}{0.0.0.0:8080}  
2018-10-30 09:06:07.165 +0100 [] [INFO] Started @3497ms  
2018-10-30 09:06:07.174 +0100 [] [INFO] RequestHandlerClass from context returned com.github.tomakehurst.wiremock.http.AdminRequestHandler. Normalized mapped under returned 'null'  
2018-10-30 09:06:07.207 +0100 [] [INFO] RequestHandlerClass from context returned com.github.tomakehurst.wiremock.http.StubRequestHandler. Normalized mapped under returned 'null'  
2018-10-30 09:06:07.208 +0100 [] [ERROR] 
                                               Request was not matched
                                               =======================

-----------------------------------------------------------------------------------------------------------------------
| Closest stub                                             | Request                                                  |
-----------------------------------------------------------------------------------------------------------------------
                                                           |
GET                                                        | GET
/path?the+plus+must+remain                                 | /path?the%2Bplus%2Bmust%2Bremain                    <<<<< URL does not match
                                                           |
                                                           |
-----------------------------------------------------------------------------------------------------------------------
  
2018-10-30 09:06:07.215 +0100 [] [INFO] Stopped NetworkTrafficServerConnector@37eeec90{HTTP/1.1,[http/1.1]}{0.0.0.0:8080}  
2018-10-30 09:06:07.216 +0100 [] [INFO] Stopped o.e.j.s.ServletContextHandler@724f138e{/,null,UNAVAILABLE}  
2018-10-30 09:06:07.216 +0100 [] [INFO] Stopped o.e.j.s.ServletContextHandler@cf65451{/__admin,null,UNAVAILABLE}  
2018-10-30 09:06:07.216 +0100 [] [WARN] QueuedThreadPool[qtp559050604]@21526f6c{STOPPING,8<=8<=10,i=0,q=4}[org.eclipse.jetty.util.thread.TryExecutor$$Lambda$2/1496949625@1a69561c] Couldn't stop Thread[qtp559050604-37,5,]  
2018-10-30 09:06:07.217 +0100 [] [WARN] QueuedThreadPool[qtp559050604]@21526f6c{STOPPING,8<=8<=10,i=0,q=4}[org.eclipse.jetty.util.thread.TryExecutor$$Lambda$2/1496949625@1a69561c] Couldn't stop Thread[qtp559050604-31,5,]  
2018-10-30 09:06:07.217 +0100 [] [WARN] QueuedThreadPool[qtp559050604]@21526f6c{STOPPING,8<=8<=10,i=0,q=4}[org.eclipse.jetty.util.thread.TryExecutor$$Lambda$2/1496949625@1a69561c] Couldn't stop Thread[qtp559050604-36,5,]  

java.lang.AssertionError: 
Expected :200
Actual   :404
bpossolo commented

I think you’re misunderstanding how URL encoding works.

The + character becomes %2B when url encoded.
addQueryParam() accepts unencoded values.
That’s the contract. You can’t pass in an already encoded value and expect it to magically know that you already encoded the value before passing it in.

Are you saying your server can’t understand %20 in a query string? Because if it doesn’t then the server is seriously broken.

Using a + instead of %20 is simply a convenience.

joschi commented

@bpossolo Thanks again for your swift reply!

I think you’re misunderstanding how URL encoding works.

Thanks, I think I understand it perfectly well. 😉

The + character becomes %2B when url encoded.

Correct. And my problem is that I want (or rather have) to use a literal '+' character in the HTTP request because the remote service otherwise cannot process the request.

addQueryParam() accepts unencoded values.

Correct. That's why I've asked how to provide raw or pre-encoded values.

Are you saying your server can’t understand %20 in a query string? Because if it doesn’t then the server is seriously broken.

Welcome to the Internet. This being said, a URL with literal '+' characters is still a valid URL, so if WSClient/WSRequest isn't able to build and send requests with that character, then they're broken (which is why I've opened this issue in the first place).

Is there a way to provide a pre-encoded query string via WSClient/WSRequest?
How could I send a GET request to the URL http://example.com/path?query=a+b?

joschi commented

The same issue still exists on Play WS 2.0.0-RC1.

I've created a repository with reproducible test cases for Play WS 1.1.12 and 2.0.0-RC1 at https://github.com/joschi/play-ws-issue-287.

joschi commented

I've added a successful test case using HttpURLConnection.

joschi commented

I've updated the repository to demonstrate the issue with Play WS 2.0.0.

bpossolo commented

StandaloneAhcWSRequest uses play.shaded.ahc.org.asynchttpclient.RequestBuilder to construct the final url that's requested:
https://github.com/playframework/play-ws/blob/master/play-ahc-ws-standalone/src/main/java/play/libs/ws/ahc/StandaloneAhcWSRequest.java#L412

RequestBuilder has multiple constructors (https://github.com/eclipse-ee4j/grizzly-ahc/blob/master/src/main/java/com/ning/http/client/RequestBuilder.java#L44 and https://github.com/eclipse-ee4j/grizzly-ahc/blob/master/src/main/java/com/ning/http/client/RequestBuilder.java#L48) which alter the encoding behaviour for the final uri.
So you'll need to enhance the StandaloneAhcWSRequest to support tweaking the underlying RequestBuilder.
You'll also need to change the higher level interfaces because those are the ones that you actually interact with in user-land.