Rest client does not url-encode the path

[yrodiere]

Elasticsearch version: 5.4.0

Plugins installed: N/A

JVM version (java -version): 1.8.0_121

OS version (uname -a if on a Unix-like system): Fedora 25

Description of the problem including expected versus actual behavior:

When calling any of the "performRequest" method of RestClient, the "endpoint" (= path) parameter is not url-encoded. As a result, non-ascii parameters may appear in the resulting request.

One could expect the path to be encoded, because the query parameters, passed as a map, are encoded by RestClient. I'd expect the client to either handle all of the encoding, or none of it.

Steps to reproduce:

1. Call `client.performRequest("GET", "foo$bar")
2. See how the produced request still includes the "$" character, instead of "%24".

Possible explanation of the issue:

The reason why path is not encoded can be found in RestClient#buildUri

           // ...
            URIBuilder uriBuilder = new URIBuilder(fullPath);
            for (Map.Entry<String, String> param : params.entrySet()) {
                uriBuilder.addParameter(param.getKey(), param.getValue());
            }
            return uriBuilder.build();

The path is passed to the URIBuilder constructor, but the constructor expects a valid (encoded) String representation of an URI, not a non-encoded path.
This can be confirmed by diving deeper into the implementation: URIBuilder(fullPath) first calls new URI(fullPath), which calls parse(false), which sets the path attribute. Then the constructor calls digestURI with the resulting URI, which sets encodedPath (!) to URI.getRawPath(), which simply returns the value of the path attribute that was set earlier, which is not an encoded value...

A simple solution would be to use the default constructor of URIBuilder, and just call setPath(String):

           // ...
            URIBuilder uriBuilder = new URIBuilder();
            uriBuilder.setPath(fullPath);
            for (Map.Entry<String, String> param : params.entrySet()) {
                uriBuilder.addParameter(param.getKey(), param.getValue());
            }
            return uriBuilder.build();

[clintongormley]

I think performRequest is too late to encode the path, eg imagine you're using PUT {index}/{type}/{id} and your id is foo/bar. You can't encode the full path correctly at that stage. Instead, the path needs to be built by concatenating encoded sub-parts.

[Sanne]

I work on the same team as @yrodiere . Just want to point out that I'd prefer the Rest client to not encode things so that we can pre-encode and reuse the strings pointing to frequently accessed URLs.

So we'd be happy to keep things as they are but it would be great to know if we can rely on this in the foreseeable future?

Thanks!

[clintongormley]

@javanna can confirm this

[javanna]

I confirm this is by design. I opened #25193 to add a test for this behaviour just to make sure that we see a failure if we ever try to change it.

[Sanne]

Thanks for that @javanna . I agree, seems sensible design.