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

Add query string parameter support #647

Merged

Conversation

mattzollinhofer
Copy link
Contributor

Per ES docs: search_type, request_cache, and the allow_partial_search_results
should not be in the body. They should be sent as query string parameters. (https://www.elastic.co/guide/en/elasticsearch/reference/6.4/search-request-body.html#_parameters_4)

My understanding is that chewy should act like this:

  $ IssuesIndex.request_cache(true).render
  # good (new)
  $ {:index=>["issues"], :type=>["issue"], :body=>{}, :request_cache=>true}
  # bad (old)
  $ {:index=>["issues"], :type=>["issue"], :body=>{:request_cache=>true}

I believe this resolves #586 and part of #642.

This is my first PR to this repo so please let me know if you'd like any changes. The second commit (Adding AllowPartialSearchResults parameter) is definitely optional as it supports an ES 6 feature. What do you think?

@mattzollinhofer mattzollinhofer force-pushed the add-query-string-parameter-support branch 4 times, most recently from 111f1d3 to af46a81 Compare August 26, 2018 02:35
Per ES docs: search_type, request_cache, and the allow_partial_search_results
should not be in the body. They should be sent as query string parameters. My
understanding is that chewy should act like this:

  $ IssuesIndex.request_cache(true).render
  # good (new)
  $ {:index=>["issues"], :type=>["issue"], :body=>{}, :request_cache=>true}
  # bad (old)
  $ {:index=>["issues"], :type=>["issue"], :body=>{:request_cache=>true}

I believe this resolves issue toptal#586 and part of issue toptal#642.

(https://www.elastic.co/guide/en/elasticsearch/reference/6.4/search-request-body.html#_parameters_4)
Just adding this for completeness. I didn't really test using this so
you can definitely remove this if it's not wanted. This support was
added in ES 6.3.

https://www.elastic.co/blog/this-week-in-elasticsearch-and-apache-lucene-2018-02-05
@mattzollinhofer mattzollinhofer force-pushed the add-query-string-parameter-support branch from af46a81 to 5f2d8f8 Compare August 26, 2018 02:54
@pyromaniac
Copy link
Contributor

Oh, I don't really understand this. https://github.com/elastic/elasticsearch-ruby/blob/master/elasticsearch-api supposed to handle this. But okay, let's merge it.

@pyromaniac pyromaniac merged commit 56af09f into toptal:master Aug 27, 2018
@pyromaniac
Copy link
Contributor

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

request_cache calls do not work
2 participants