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

REST high-level client: encode path parts #28663

Merged
merged 5 commits into from Feb 15, 2018

Conversation

Projects
None yet
4 participants
@javanna
Copy link
Member

commented Feb 13, 2018

The REST high-level client supports now encoding of path parts, so that for instance documents with valid ids, but containing characters that need to be encoded as part of urls (# etc.), are properly supported. We also make sure that each path part can contain / by encoding them properly too.

Closes #28625

REST high-level client: encode path parts
The REST high-level client supports now encoding of path parts, so that for instance documents with valid ids, but containing characters that need to be encoded as part of urls (`#` etc.), are properly supported. We also make sure that each path part can contain `/` by encoding them properly too.

Closes #28625
@tlrx
Copy link
Member

left a comment

This looks great, thanks for fixing this Luca. I just think that we can deal with the encoding of some parameters at the same time, what do you think?

//encode each part (e.g. index, type and id) separately before merging them into the path
URIBuilder uriBuilder = new URIBuilder().setPath(part);
//make sure that "/" in each part are properly encoded too
joiner.add(uriBuilder.build().getRawPath().replaceAll("/", "%2F"));

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 13, 2018

Member

Could we use a static encode() method for this? We'll have to reuse it in some URL parameters like routing or parent, right?

This comment has been minimized.

Copy link
@javanna

javanna Feb 13, 2018

Author Member

it's complicated :) querystring params should already be encoded by the rest low-level client. but the low-level client expects paths to be externally encoded, as they are provided altogether and we can't encode each part safely anymore. In the high-level client though, it makes sense to encode each part separately as they are provided separately. I think what we do here is all we need.

This comment has been minimized.

Copy link
@javanna

javanna Feb 13, 2018

Author Member

I will add tests for this.

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 13, 2018

Member

Of course, I just forgot that the low-level client already encodes parameters. Thanks.

@jasontedor jasontedor added v6.2.3 and removed v6.2.2 labels Feb 13, 2018

javanna added some commits Feb 14, 2018

@tlrx

tlrx approved these changes Feb 14, 2018

Copy link
Member

left a comment

LGTM as long as CI is happy

URIBuilder uriBuilder = new URIBuilder().setPath(part);
//make sure that "/" in each part are properly encoded too
joiner.add(uriBuilder.build().getRawPath().replaceAll("/", "%2F"));
//we prepend "/" to the path part to make this pate absolute, otherwise there can be issues with

This comment has been minimized.

Copy link
@tlrx

tlrx Feb 14, 2018

Member

pate -> path

Can we do something like
if (path.startsWith("/") == false) {
path = "/" + path
}

This comment has been minimized.

Copy link
@javanna

javanna Feb 14, 2018

Author Member

I don't think so. parts are not supposed to start with '/'. If they do, like a document with id /id, that / is part of the id and needs to be encoded, which we do manually. That first slash is added otherwise URI does crazy things in some cases, like trying to extract the scheme when an index name contains : (e.g. cluster:index). That doesn't happen if the url is absolute, meaning it starts with /. That is why we add it, yet when retrieving the encoded url we ignore it as we do substring(1). This stuff is extremely tricky, and buggy, and there isn't a clean way to do what we need in any of the available method utils that I know of/tried.

@javanna javanna added v5.6.9 and removed v5.6.8 labels Feb 15, 2018

@javanna javanna merged commit ebe5e8e into elastic:master Feb 15, 2018

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

javanna added a commit that referenced this pull request Feb 15, 2018

REST high-level client: encode path parts (#28663)
The REST high-level client supports now encoding of path parts, so that for instance documents with valid ids, but containing characters that need to be encoded as part of urls (`#` etc.), are properly supported. We also make sure that each path part can contain `/` by encoding them properly too.

Closes #28625
@javanna

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2018

This one is pending backport to 6.2 and 5.6 once 5.6.8 and 6.2.2 are out.

javanna added a commit to javanna/elasticsearch that referenced this pull request Feb 21, 2018

REST high-level client: encode path parts (elastic#28663)
The REST high-level client supports now encoding of path parts, so that for instance documents with valid ids, but containing characters that need to be encoded as part of urls (`#` etc.), are properly supported. We also make sure that each path part can contain `/` by encoding them properly too.

Closes elastic#28625

javanna added a commit that referenced this pull request Feb 21, 2018

REST high-level client: encode path parts (#28663)
The REST high-level client supports now encoding of path parts, so that for instance documents with valid ids, but containing characters that need to be encoded as part of urls (`#` etc.), are properly supported. We also make sure that each path part can contain `/` by encoding them properly too.

Closes #28625

@javanna javanna added the v6.2.3 label Feb 21, 2018

javanna added a commit that referenced this pull request Feb 22, 2018

REST high-level client: encode path parts (#28663)
The REST high-level client supports now encoding of path parts, so that for instance documents with valid ids, but containing characters that need to be encoded as part of urls (`#` etc.), are properly supported. We also make sure that each path part can contain `/` by encoding them properly too.

Closes #28625

@javanna javanna added the v5.6.9 label Feb 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.