Skip to content

HEAD support for the _search/exists api #13680

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
wants to merge 1 commit into from

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented 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 #11204

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
@nik9000 nik9000 added >enhancement :Search/Search Search-related issues that do not fall into other categories :Core/Infra/REST API REST infrastructure and utilities v2.1.0 v5.0.0-alpha1 labels Sep 21, 2015
@nik9000
Copy link
Member Author

nik9000 commented Sep 21, 2015

Ping @honzakral and maybe @areek for review.

The linked issue mentions some client API changes but I think what we have now makes sense as _search/exists is an action that hits many shards so the extra information returned in the Java api is fine.

@honzakral
Copy link
Contributor

I cannot speak for the code (LGTM but I am not a java person) but we should also change the yaml tests (https://github.com/nik9000/elasticsearch/tree/search_exists_head/rest-api-spec/src/main/resources/rest-api-spec/test/search_exists) to not check the body and instead behave as other exist_ apis (for example see https://github.com/nik9000/elasticsearch/tree/search_exists_head/rest-api-spec/src/main/resources/rest-api-spec/test/indices.exists).

Thanks!

@nik9000
Copy link
Member Author

nik9000 commented Sep 21, 2015

I cannot speak for the code (LGTM but I am not a java person) but we should also change the yaml tests (https://github.com/nik9000/elasticsearch/tree/search_exists_head/rest-api-spec/src/main/resources/rest-api-spec/test/search_exists) to not check the body and instead behave as other exist_ apis (for example see https://github.com/nik9000/elasticsearch/tree/search_exists_head/rest-api-spec/src/main/resources/rest-api-spec/test/indices.exists).

As I've got them right now the REST tests only look at the body if the request was GET or POST and then they still verify what they used to verify. If the request was HEAD they just check for the 404. I did it in a sneaky way though.

@nik9000
Copy link
Member Author

nik9000 commented Sep 21, 2015

https://github.com/nik9000/elasticsearch/blob/search_exists_head/rest-api-spec/src/main/resources/rest-api-spec/test/search_exists/10_basic.yaml#L26 this line will fail when HEAD is used.

Not after this patch it won't. Not in the Java version. That is what I mean by

This behavior is checked instead with - is_false: anystring.

The operative change is here.

@nik9000
Copy link
Member Author

nik9000 commented Sep 21, 2015

I'm fine with doing it in another way but this was the only way I could figure to keep the specs logicless and to test the body when there was one and the 404-ness when there wasn't.

@honzakral
Copy link
Contributor

According to the specs of the yaml test suite this should fail with HEAD: https://github.com/nik9000/elasticsearch/tree/search_exists_head/rest-api-spec/src/main/resources/rest-api-spec/test#is_true specifies that the key must exist for this assertion to pass.

@nik9000
Copy link
Member Author

nik9000 commented Sep 21, 2015

According to the specs of the yaml test suite this should fail with HEAD: https://github.com/nik9000/elasticsearch/tree/search_exists_head/rest-api-spec/src/main/resources/rest-api-spec/test#is_true specifies that the key must exist for this assertion to pass.

There are specs for this?! Nice! I had no idea. They are out of date though - is_true: '' is currently a special case and checks if the request was a 200. is_false: '' does a 404 check.

So what do you want:

  1. The tests ignore the GET and POST for this request and only ever do a HEAD.
  2. The tests ignore the HEAD and only ever do GET or POST.
  3. I change the specs to reflect that HEAD requests get all true for 200 and all false for 404.
  4. I just stop working on this entirely and wait for a decision on Remove search-exists API #13682.

@honzakral
Copy link
Contributor

I think 4 is the best option for now, if it doesn't work I'd just remove the is_true and is_false statements altogether - that way only the do commands will be checked which means the status code (one plain which means that the request must be asuccess (2XX) and the other one has catch: missing which means it asserts the status code to be 404, exactly as needed in both cases.

@nik9000
Copy link
Member Author

nik9000 commented Sep 21, 2015

I think 4 is the best option for now, if it doesn't work I'd just remove the is_true and is_false statements altogether - that way only the do commands will be checked which means the status code (one plain which means that the request must be asuccess (2XX) and the other one has catch: missing which means it asserts the status code to be 404, exactly as needed in both cases.

Everywhere else HEAD requests get an implicit ignore: missing.... Lets just ignore this until #13682 is decided.

@javanna
Copy link
Member

javanna commented Oct 31, 2015

Search exists has been removed from master. We are still discussing in #11204 whether we should add support for HEAD to _search, but that seems to complicate things quite a bit. We are also better documenting the search exists and how to utilize the search api to achieve the same (see ##14393). That said I think this PR could be closed, thanks for your work @nik9000 though!

@clintongormley
Copy link
Contributor

@javanna agreed!

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 >enhancement feedback_needed :Search/Search Search-related issues that do not fall into other categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants