toptal / chewy

High-level Elasticsearch Ruby framework based on the official elasticsearch-ruby client
MIT License
1.89k stars 368 forks source link

Update ES query in Chewy::Stash::Journal.for to support ES < 2 #742

Closed Bhacaz closed 3 years ago

Bhacaz commented 3 years ago

Hi, I know that change can seems no necessary, but the gem did a really good job supporting old version of ElasticSearch. However I found that query while testing an old app and it takes for granted that the search_class is Chewy::Search::Request and it doesn't work with the old Chewy::Query.

Here the error I got before the change:

NoMethodError: undefined method `or' for #<Chewy::Stash::Journal::Query:0x00007fc105a7ee78>

I thing the new implementation looks a bit heavy and doesn’t use must of the DSLs, but it works for ES < 2 and newer.


Step to reproduce:

require 'chewy'
Chewy.search_class = Chewy::Query
class D; end
class DummiesIndex < Chewy::Index
  define_type D do
  end
end
Chewy::Stash::Journal.for(DummiesIndex)

Error:

Traceback (most recent call last):
        7: from /Users/bhacaz/.rbenv/versions/2.6.6/bin/irb:23:in `<main>'
        6: from /Users/bhacaz/.rbenv/versions/2.6.6/bin/irb:23:in `load'
        5: from /Users/bhacaz/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        4: from (irb):29
        3: from /Users/bhacaz/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/chewy-5.2.0/lib/chewy/stash.rb:49:in `for'
        2: from /Users/bhacaz/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/chewy-5.2.0/lib/chewy/stash.rb:49:in `each'
        1: from /Users/bhacaz/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/chewy-5.2.0/lib/chewy/stash.rb:50:in `block in for'
NoMethodError (undefined method `or' for #<Chewy::Stash::Journal::Query:0x00007fc8a82b1808>)
rabotyaga commented 3 years ago

@Bhacaz Thanks! Could you please describe how to reproduce the error?

Bhacaz commented 3 years ago

Sure, I updated the PR description.

rabotyaga commented 3 years ago

As far as I can see, there are much more things to fail with the legacy DSL, for example, Chewy::Stash::Journal.clean. Am I missing something?

Bhacaz commented 3 years ago

@rabotyaga

I'm aware that many thing doesn't work with the the legacy DSL and old version of ElasticSearch. My objectif with this PR is only that since Chewy offer the option to use the legacy DSL, a build-in feature should not raise an error method_missing when it's configured that way.

The reason I proposed this change, is that Chewy offer the possibility to configure which DSL indexes class will used. So every class who inherited from Chewy::Index will use the configured DSL, the old Chewy::Query or the default Chewy::Search::Request.

I far as I know, there is two classes that inherited from Chewy::Index in Chewy, Chewy::Stash::Specification and Chewy::Stash::Journal. And Chewy::Stash::Journal use his Chewy::Search::ClassMethods#search_class. Sadly Chewy::Stash::Journal.for called Chewy::Search::Request#or which it's not defined in the old DSL.

I refactored the method to only use filter which is defined in both DSL. I looked around and as far as I can see it is the only place that may cause that type of error.

rabotyaga commented 3 years ago

I meant - does this fix changes something in terms of legacy DSL usage? Is this place is the only one, that prevents legacy DSL usage? Looks like not, and then we will have to chase the goal of making legacy DSL working, while our goal right now is quite the opposite - bring support for ES6 and then ES7.

Bhacaz commented 3 years ago

does this fix changes something in terms of legacy DSL usage?

Yes, if someone use the configuration Chewy.search_class = Chewy::Query he cannot use the Journialing feature.

Is this place is the only one, that prevents legacy DSL usage?

Yes, Chewy::Stash::Specification and Chewy::Stash::Journal are the only index classes created inside Chewy, so it limite the research.


while our goal right now is quite the opposite - bring support for ES6 and then ES7.

I totally understand and I look forward for it. I don't want anybody to waste energy on old features. Be free to simply reject the PR I will totally understand. 😃

rabotyaga commented 3 years ago

@Bhacaz Let's close this by now, as it doesn't render Stash completely working with the legacy DSL (try the following patch and run the specs

--- a/spec/chewy/stash_spec.rb
+++ b/spec/chewy/stash_spec.rb
@@ -5,6 +5,7 @@ describe Chewy::Stash::Journal, :orm do
     response['deleted'] || response['_indices']['_all']['deleted']
   end

+  before { Chewy.search_class = Chewy::Query }
   before { Chewy.massacre }

   before do

) and, at the same time, introduces some crutches. Thanks a lot for your effort!