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

High Level Rest client should use POST when we have body #28326

Closed
dadoonet opened this issue Jan 22, 2018 · 20 comments
Closed

High Level Rest client should use POST when we have body #28326

dadoonet opened this issue Jan 22, 2018 · 20 comments
Assignees

Comments

@dadoonet
Copy link
Member

When elasticsearch is behind a Proxy and this Proxy does not support GET with body (which might be seen as a bug of the Proxy), people can't use our Java Rest Client as explained in https://discuss.elastic.co/t/es-cluster-via-proxy-ip-but-listener-timeout-after-waiting-for-30000-ms/116192/6

For example, the search Request here uses a GET method:

return new Request(HttpGet.METHOD_NAME, endpoint, params.getParams(), entity);

I think we should may be switch to POST instead anytime we might have a body.

Thoughts?

@javanna
Copy link
Member

javanna commented Jan 22, 2018

Makes sense. What I had in mind was to have an option that lets you choose between GET and POST, like other language clients do. Or switching to POST like you proposed.

@DaveCTurner
Copy link
Contributor

The use of bodies in HTTP GET requests is a little unusual. Although the spec is not crystal clear on the subject, it is possible to read it in a way that means the body of a GET request can and should be ignored: https://stackoverflow.com/a/983458

@javanna
Copy link
Member

javanna commented Jan 22, 2018

@DaveCTurner I don't think this is the right place to discuss what our API should support. Our API supports both, we always had a preference for GET with body but we decided (years ago) to support POST too for clients and proxies that don't support GET with body. In our clients, we should follow what the API exposes, hence we should allow to choose between POST and GET if the API allows both. This is something that I had wondered about when going for GET only but I was waiting for the first user to point out that GET causes trouble to them.

@DaveCTurner
Copy link
Contributor

Sorry, I didn't mean to open that debate. Supporting both on the API side seems fine. Just wanted to say that a proxy that doesn't pass the body through on a GET request shouldn't necessarily be considered buggy - it is more of a bug in the spec itself that this difference of opinion can exist. Using POST seems safer in situations that might involve a proxy: not only will it fix the OP's problem but it'll also avoid the possibility that the response is cached and quietly re-used in response to different queries against the same URL.

What'd be the disadvantage of completely switching to POST in the client as @dadoonet suggests?

@jasontedor
Copy link
Member

jasontedor commented Jan 22, 2018

Completely switching to POST for requests with bodies from the client seems like the right approach to me. Since we support both POST in all such cases there's no need for the additional complexity of a toggle?

@Mpdreamz
Copy link
Member

All low level clients either allow you to either pass any HTTP method or expose all the API + allowed http method combinations.

Defaulting to POST for high level clients and not making this configurable makes sense though, it's also the approach the high level c# client NEST takes.

In some cases NEST is smart enough to revert back to a GET automatically e.g if only the query on the querystring (?q=) is provided on _search or if the source common parameter is provided. This is not really standardized though.

@javanna
Copy link
Member

javanna commented Jan 22, 2018

Cool thanks for the feedback let's do POST only then. I prefer one less configuration knob too.

@javanna javanna self-assigned this Jan 23, 2018
@javanna javanna removed the help wanted adoptme label Jan 23, 2018
javanna added a commit to javanna/elasticsearch that referenced this issue Jan 23, 2018
…ch support request body

It has been pointed out that GET with body may cause problems to some proxies. We are then switching to POST the API that retrieve info and support a request body.

Closes elastic#28326
javanna added a commit that referenced this issue Jan 25, 2018
…ch support request body (#28342)

It has been pointed out that GET with body may cause problems to some proxies. We are then switching to POST the API that retrieve info and support a request body.

Closes #28326
javanna added a commit that referenced this issue Jan 25, 2018
…ch support request body (#28342)

It has been pointed out that GET with body may cause problems to some proxies. We are then switching to POST the API that retrieve info and support a request body.

Closes #28326
@asmita2201
Copy link

@javanna Please advise when would this be released?

@javanna
Copy link
Member

javanna commented Jan 31, 2018

@asmita2201 you can check the labels of the PR that closed this issue: #28342 . It will be released with 6.3.0. Unfortunately the change went in a tiny bit too late for 6.2 .

@javanna
Copy link
Member

javanna commented Jan 31, 2018

@asmita2201 I am now wondering if we should have merged this as a bug and backported to 6.2 branch (at least it would be released with 6.2.1) and maybe also to 5.6 (it would go out with 5.6.8). I take it that this is a blocker for you?

@asmita2201
Copy link

asmita2201 commented Jan 31, 2018 via email

@asmita2201
Copy link

@javanna - YEs please treat that as a blocker. We are unable to get the right JAR for this.

@javanna
Copy link
Member

javanna commented Feb 1, 2018

@asmita2201 6.3.0 is not released yet, you can get a snapshot from https://snapshots.elastic.co/maven/ . That said I wouldn't use a snapshot in production.

javanna added a commit that referenced this issue Feb 1, 2018
…ch support request body (#28342)

It has been pointed out that GET with body may cause problems to some proxies. We are then switching to POST the API that retrieve info and support a request body.

Closes #28326
@asmita2201
Copy link

@javanna can you advise how to access?
I get this page:

NoSuchKey
The specified key does not exist.
maven/
ED6094AC6235E7A6

WqwhJ5VYiGtlOkX45a8VCFtGHD+DNdXqSAgxuUqhH40UK4VlomzfA4oVaeLkoP9I2cfCHxlb8yU=

@javanna
Copy link
Member

javanna commented Feb 8, 2018

@asmita2201 we just realized that some recent snapshots are missing in our snapshots repo. We are fixing that as we speak, will let you know once it's fixed so you can try again.

@asmita2201
Copy link

@javanna could you please provide an ETA for this?

@javanna
Copy link
Member

javanna commented Feb 12, 2018

hi @asmita2201 the snapshot artifacts should be back up. Yet the URL the I gave you above needs to be configured in your maven build, it is not directly accessible from a browser.

javanna added a commit that referenced this issue Feb 12, 2018
…ch support request body (#28342)

It has been pointed out that GET with body may cause problems to some proxies. We are then switching to POST the API that retrieve info and support a request body.

Closes #28326
@asmita2201
Copy link

@javanna
I have tried using the maven build. The rest-high-level-cleint snapshot jar is getting downloaded. But one of the dependencies(org.elasticsearch:elasticsearch-secure-sm:jar:6.3.0-SNAPSHOT) needed by therest-high-level-cleint jar is not getting downloaded.

Failure to find org.elasticsearch:elasticsearch-secure-sm:jar:6.3.0-SNAPSHOT in http://s3.amazonaws.com/download.elasticsearch.org/lucenesnapshots/00142c9 was cached in the local repository, resolution will not be reattempted until the update interval of elastic-lucene-snapshots has elapsed or updates are forced -> [Help 1]
[ERROR]

@mgreau
Copy link
Member

mgreau commented Feb 13, 2018

Hi @asmita2201
Good catch. I have reproduced the problem and I opened #28658 to track and fix the problem.
Thanks

@asmita2201
Copy link

@mgreau @javanna
Thanks, we are able to get the JAR now. Can you confirm:
when would this officially be released?
what version of elastic supports this?

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

7 participants