Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
joschi opened this issue Oct 26, 2018 · 8 comments
Open

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

joschi opened this issue Oct 26, 2018 · 8 comments

Comments

@joschi
Copy link

joschi commented Oct 26, 2018

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
Copy link
Contributor

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
Copy link
Author

joschi commented Oct 30, 2018

@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
Copy link
Contributor

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
Copy link
Author

joschi commented Oct 30, 2018

@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 added a commit to joschi/play-ws-issue-287 that referenced this issue Nov 24, 2018
joschi added a commit to joschi/play-ws-issue-287 that referenced this issue Nov 24, 2018
@joschi
Copy link
Author

joschi commented Nov 24, 2018

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
Copy link
Author

joschi commented Nov 25, 2018

I've added a successful test case using HttpURLConnection.

joschi pushed a commit to joschi/play-ws-issue-287 that referenced this issue Jan 22, 2019
@joschi
Copy link
Author

joschi commented Jan 22, 2019

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

@bpossolo
Copy link
Contributor

bpossolo commented Jan 22, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants