Skip to content

search_exists should be consistent with other *_exists methods #11204

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

Closed
honzakral opened this issue May 18, 2015 · 9 comments
Closed

search_exists should be consistent with other *_exists methods #11204

honzakral opened this issue May 18, 2015 · 9 comments
Assignees
Labels
:Core/Infra/REST API REST infrastructure and utilities discuss >enhancement

Comments

@honzakral
Copy link
Contributor

  • use HEAD request type (which also means to allow for POST when http client cannot send HEAD with body, as well as the source parameter)
  • the client API should only return true/false
  • the yaml tests need to be changed to reflect that
@nik9000
Copy link
Member

nik9000 commented May 18, 2015

Any reason not to have a HEAD request for a SEARCH default to search_exists? Maybe its just too weird do a HEAD with a body.

By the client API do you mean the Java api? All of them?

@honzakral
Copy link
Contributor Author

Unfortunately we cannot rely on HEAD working with body, the specs allow for it, but many clients and proxies don't so we need to be cautious and have a separate endpoint just to be sure.

I mean all of them, they should be consistent in this, but those will be updated once the java client is and the yaml test suite is changed to reflect that.

@javanna
Copy link
Member

javanna commented May 18, 2015

Search exists api is different compared to search. Search (as well as count) does allow you to set terminate_after, but it will still go to each of the shard in the context of the search request and execute the query there, the terminate_after is effectively per shard. Search exists returns the response after the first shard reports existing documents, instead of blocking for responses from all the shards. This is the main reason why this is a separate endpoint I think.

Supporting HEAD for consistency sounds good but this api is different compared to other exists* api, as it has to accept a body (which can be provided as query_string encoded source parameter too though to work around any potential proxy/client issue).

@javanna
Copy link
Member

javanna commented May 18, 2015

@honzakral I didn't notice at first your point around supporting POST too, makes perfect sense. +1

@s1monw
Copy link
Contributor

s1monw commented May 22, 2015

yeah lets do it - I think this is relatively simple to add. We have org.elasticsearch.rest.action.exists.RestExistsAction that is registered in org.elasticsearch.rest.action.search.RestSearchAction but only for GET and POST. If we register it for HEAD as well as omit the response it does when it is a HEAD request we are good to go?

For the response part I think we can just omit the response if we request.method() == RestRequest.Method.HEAD

@s1monw
Copy link
Contributor

s1monw commented May 22, 2015

I know @markharwood mentioned that @ESamir was looking for a low hanging fruit, this one could be one?

@clintongormley clintongormley added >enhancement :Core/Infra/REST API REST infrastructure and utilities labels Jun 8, 2015
@clintongormley clintongormley assigned nik9000 and unassigned ESamir Aug 5, 2015
@jasontedor jasontedor removed the help wanted adoptme label Aug 18, 2015
nik9000 added a commit to nik9000/elasticsearch that referenced this issue Sep 21, 2015
Registers the `_search/exists` api to receive HEAD requests. If it gets one
then it won't output the JSON response but will still do everything else.

Adds magic to the rest tests to ignore `catch: missing` for HEAD requests.
This behavior is checked instead with `- is_false: anystring`. This setup
makes the rest tests for `search/_exists` work for HEAD requests.

Also forces the request body over the source URL parameter for all HEAD
requests.

Closes elastic#11204
@javanna
Copy link
Member

javanna commented Oct 21, 2015

I marked this back for discussion. The search/exists api has been removed from master and deprecated in 2.1. We can now discuss whether we want _search to work with HEAD or not. It currently doesn't. Seems like a different endpoint than _search would still be required for the reasons stated by @honzakral above though. And it would be the first api that responds to HEAD but accepts a body, which many clients don't support.

@clintongormley
Copy link
Contributor

And it would be the first api that responds to HEAD but accepts a body, which many clients don't support.

If it is going to be part of the Elasticearch API, then it needs to be part of the REST tests, which means that all clients should be able to support it. This makes me lean towards: if you want to add a shortcut to a particular client then go for it, but it shouldn't be part of the official API.

@clintongormley
Copy link
Contributor

Search exists has been removed. Closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities discuss >enhancement
Projects
None yet
Development

No branches or pull requests

7 participants