[MODEL] Refactored the {Searching} module and the {Response} class to allow lazy loading

karmi · karmi · commit 50ddcb3cba87 · 2013-12-29T22:24:03.000+01:00
Previously, the Searching::ClassMethods#search method executed the search request immediately, and passed the Elasticsearch response as a Hash to the Response class initializer. This prevented for instance the common pattern of paginating search results in a Rails controller action: @articles = Article.search(params[:q]).page(params[:page]).records Here, the `search` method is re-initialized on each page load, immediately executing the search, and the `#page` call then performs another, separate request. With this patch, the Response class is initialized lazily, and the search request is performed only when `Response#response` is called (eg. via `Response#results`, `Response#records`, etc).
diff --git a/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb b/elasticsearch-model/lib/elasticsearch/model/adapters/active_record.rb
@@ -13,12 +13,12 @@ module Records
           # Returns an `ActiveRecord::Relation` instance
           #
           def records
-            sql_records = klass.where(id: @ids)
+            sql_records = klass.where(id: ids)
 
             # Re-order records based on the order from Elasticsearch hits
             # by redefining `to_a`, unless the user has called `order()`
             #
-            sql_records.instance_exec(response['hits']['hits']) do |hits|
+            sql_records.instance_exec(response.response['hits']['hits']) do |hits|
               define_singleton_method :to_a do
                 if defined?(::ActiveRecord) && ::ActiveRecord::VERSION::MAJOR >= 4
                   self.load
diff --git a/elasticsearch-model/lib/elasticsearch/model/adapters/mongoid.rb b/elasticsearch-model/lib/elasticsearch/model/adapters/mongoid.rb
@@ -16,9 +16,9 @@ module Records
           # Return a `Mongoid::Criteria` instance
           #
           def records
-            criteria = klass.where(:id.in => @ids)
+            criteria = klass.where(:id.in => ids)
 
-            criteria.instance_exec(response['hits']['hits']) do |hits|
+            criteria.instance_exec(response.response['hits']['hits']) do |hits|
               define_singleton_method :to_a do
                 self.entries.sort_by { |e| hits.index { |hit| hit['_id'].to_s == e.id.to_s } }
               end
diff --git a/elasticsearch-model/lib/elasticsearch/model/response.rb b/elasticsearch-model/lib/elasticsearch/model/response.rb
@@ -18,29 +18,52 @@ class Response
 
         forward :results, :each, :empty?, :size, :slice, :[], :to_ary
 
-        def initialize(klass, search, response)
+        def initialize(klass, search, options={})
           @klass     = klass
           @search    = search
-          @response  = response
-          @took      = response['took']
-          @timed_out = response['timed_out']
-          @shards    = Hashie::Mash.new(response['_shards'])
         end
 
-        # Return the collection of "hits" from Elasticsearch
+        # Returns the Elasticsearch response
         #
-        def results
+        # @return [Hash]
+        #
+        def response
           @response ||= search.execute!
-          @results  ||= Results.new(klass, response, nil, self)
         end
 
-        # Return the collection of records from the database
+        # Returns the collection of "hits" from Elasticsearch
+        #
+        # @return [Results]
+        #
+        def results
+          @results ||= Results.new(klass, self)
+        end
+
+        # Returns the collection of records from the database
+        #
+        # @return [Records]
         #
         def records
-          @response ||= search.execute!
-          @records  ||= Records.new(klass, response, results, self)
+          @records ||= Records.new(klass, self)
         end
 
+        # Returns the "took" time
+        #
+        def took
+          response['took']
+        end
+
+        # Returns whether the response timed out
+        #
+        def timed_out
+          response['timed_out']
+        end
+
+        # Returns the statistics on shards
+        #
+        def shards
+          Hashie::Mash.new(response['_shards'])
+        end
       end
     end
   end
diff --git a/elasticsearch-model/lib/elasticsearch/model/response/base.rb b/elasticsearch-model/lib/elasticsearch/model/response/base.rb
@@ -4,19 +4,15 @@ module Response
       # Common funtionality for classes in the {Elasticsearch::Model::Response} module
       #
       module Base
-        attr_reader :klass, :response, :response_object,
-                    :total, :max_score
+        attr_reader :klass, :response
 
         # @param klass    [Class] The name of the model class
         # @param response [Hash]  The full response returned from Elasticsearch client
         # @param results  [Results]  The collection of results
         #
-        def initialize(klass, response, results=nil, response_object=nil)
+        def initialize(klass, response, options={})
           @klass     = klass
-          @response_object = response_object
           @response  = response
-          @total     = response['hits']['total']
-          @max_score = response['hits']['max_score']
         end
 
         # @abstract Implement this method in specific class
@@ -31,6 +27,17 @@ def records
           raise NotImplemented, "Implement this method in #{klass}"
         end
 
+        # Returns the total number of hits
+        #
+        def total
+          response.response['hits']['total']
+        end
+
+        # Returns the max_score
+        #
+        def max_score
+          response.response['hits']['max_score']
+        end
       end
     end
   end
diff --git a/elasticsearch-model/lib/elasticsearch/model/response/pagination.rb b/elasticsearch-model/lib/elasticsearch/model/response/pagination.rb
@@ -20,8 +20,8 @@ def self.included(base)
             Elasticsearch::Model::Response::Results.__send__ :include, ::Kaminari::PageScopeMethods
             Elasticsearch::Model::Response::Records.__send__ :include, ::Kaminari::PageScopeMethods
 
-            Elasticsearch::Model::Response::Results.__send__ :forward, :response_object, :limit_value, :offset_value, :total_count
-            Elasticsearch::Model::Response::Records.__send__ :forward, :response_object, :limit_value, :offset_value, :total_count
+            Elasticsearch::Model::Response::Results.__send__ :forward, :response, :limit_value, :offset_value, :total_count
+            Elasticsearch::Model::Response::Records.__send__ :forward, :response, :limit_value, :offset_value, :total_count
 
             base.class_eval <<-RUBY, __FILE__, __LINE__ + 1
               # Define the `page` Kaminari method
diff --git a/elasticsearch-model/lib/elasticsearch/model/response/records.rb b/elasticsearch-model/lib/elasticsearch/model/response/records.rb
@@ -17,11 +17,8 @@ class Records
 
         # @see Base#initialize
         #
-        def initialize(klass, response, results=nil, response_object=nil)
+        def initialize(klass, response, options={})
           super
-          @response = response
-          @results  = results
-          @ids = response['hits']['hits'].map { |hit| hit['_id'] }
 
           # Include module provided by the adapter in the singleton class ("metaclass")
           #
@@ -32,16 +29,28 @@ def initialize(klass, response, results=nil, response_object=nil)
           self
         end
 
+        # Returns the hit IDs
+        #
+        def ids
+          response.response['hits']['hits'].map { |hit| hit['_id'] }
+        end
+
+        # Returns the {Results} collection
+        #
+        def results
+          response.results
+        end
+
         # Yields [record, hit] pairs to the block
         #
         def each_with_hit(&block)
-          records.zip(@results).each(&block)
+          records.zip(results).each(&block)
         end
 
         # Yields [record, hit] pairs and returns the result
         #
         def map_with_hit(&block)
-          records.zip(@results).map(&block)
+          records.zip(results).map(&block)
         end
 
         # Delegate methods to `@records`
diff --git a/elasticsearch-model/lib/elasticsearch/model/response/results.rb b/elasticsearch-model/lib/elasticsearch/model/response/results.rb
@@ -8,20 +8,22 @@ module Response
       #
       class Results
         include Base
-
-        attr_reader :results
-
         include Enumerable
 
         extend  Support::Forwardable
         forward :results, :each, :empty?, :size, :slice, :[], :to_a, :to_ary
 
         # @see Base#initialize
         #
-        def initialize(klass, response, results=nil, response_object=nil)
+        def initialize(klass, response, options={})
           super
+        end
+
+        # Returns the {Results} collection
+        #
+        def results
           # TODO: Configurable custom wrapper
-          @results   = response['hits']['hits'].map { |hit| Result.new(hit) }
+          @results  = response.response['hits']['hits'].map { |hit| Result.new(hit) }
         end
 
       end
diff --git a/elasticsearch-model/lib/elasticsearch/model/searching.rb b/elasticsearch-model/lib/elasticsearch/model/searching.rb
@@ -96,9 +96,8 @@ module ClassMethods
         #
         def search(query_or_payload, options={})
           search   = SearchRequest.new(self, query_or_payload, options={})
-          response = search.execute!
 
-          Response::Response.new(self, search, response)
+          Response::Response.new(self, search)
         end
 
       end
diff --git a/elasticsearch-model/test/integration/mongoid_basic_test.rb b/elasticsearch-model/test/integration/mongoid_basic_test.rb
@@ -47,6 +47,10 @@ def as_indexed_json(options={})
 
         context "Mongoid integration" do
           setup do
+            Elasticsearch::Model::Adapter.register \
+              Elasticsearch::Model::Adapter::Mongoid,
+              lambda { |klass| !!defined?(::Mongoid::Document) && klass.ancestors.include?(::Mongoid::Document) }
+
             MongoidArticle.__elasticsearch__.create_index! force: true
 
             MongoidArticle.delete_all
diff --git a/elasticsearch-model/test/unit/adapter_active_record_test.rb b/elasticsearch-model/test/unit/adapter_active_record_test.rb
@@ -3,11 +3,23 @@
 class Elasticsearch::Model::AdapterActiveRecordTest < Test::Unit::TestCase
   context "Adapter ActiveRecord module: " do
     class ::DummyClassForActiveRecord
+      RESPONSE = Struct.new('DummyActiveRecordResponse') do
+        def response
+          { 'hits' => {'hits' => [ {'_id' => 2}, {'_id' => 1} ]} }
+        end
+      end.new
+
       def response
-        { 'hits' => {'hits' => [ {'_id' => 2}, {'_id' => 1} ]} }
+        RESPONSE
+      end
+
+      def ids
+        [2, 1]
       end
     end
 
+    RESPONSE = { 'hits' => { 'total' => 123, 'max_score' => 456, 'hits' => [] } }
+
     setup do
       @records = [ stub(id: 1, inspect: '<Model-1>'), stub(id: 2, inspect: '<Model-2>') ]
       @records.stubs(:load).returns(true)
diff --git a/elasticsearch-model/test/unit/adapter_mongoid_test.rb b/elasticsearch-model/test/unit/adapter_mongoid_test.rb
@@ -3,8 +3,18 @@
 class Elasticsearch::Model::AdapterMongoidTest < Test::Unit::TestCase
   context "Adapter Mongoid module: " do
     class ::DummyClassForMongoid
+      RESPONSE = Struct.new('DummyMongoidResponse') do
+        def response
+          { 'hits' => {'hits' => [ {'_id' => 2}, {'_id' => 1} ]} }
+        end
+      end.new
+
       def response
-        { 'hits' => {'hits' => [ {'_id' => 2}, {'_id' => 1} ]} }
+        RESPONSE
+      end
+
+      def ids
+        [2, 1]
       end
     end
 
diff --git a/elasticsearch-model/test/unit/response_base_test.rb b/elasticsearch-model/test/unit/response_base_test.rb
@@ -1,26 +1,36 @@
 require 'test_helper'
 
-class Elasticsearch::Model::ResultTest < Test::Unit::TestCase
+class Elasticsearch::Model::BaseTest < Test::Unit::TestCase
   context "Response base module" do
-    class OriginClass; end
+    class OriginClass
+      def self.index_name;    'foo'; end
+      def self.document_type; 'bar'; end
+    end
 
     class DummyBaseClass
       include Elasticsearch::Model::Response::Base
     end
 
     RESPONSE = { 'hits' => { 'total' => 123, 'max_score' => 456, 'hits' => [] } }
 
+    setup do
+      @search   = Elasticsearch::Model::Searching::SearchRequest.new OriginClass, '*'
+      @response = Elasticsearch::Model::Response::Response.new OriginClass, @search
+      @search.stubs(:execute!).returns(RESPONSE)
+    end
+
     should "access klass, response, total and max_score" do
-      r = DummyBaseClass.new OriginClass, RESPONSE
+      r = DummyBaseClass.new OriginClass, @response
 
       assert_equal OriginClass, r.klass
-      assert_equal RESPONSE, r.response
+      assert_equal @response, r.response
+      assert_equal RESPONSE,  r.response.response
       assert_equal 123, r.total
       assert_equal 456, r.max_score
     end
 
     should "have abstract methods results and records" do
-      r = DummyBaseClass.new OriginClass, RESPONSE
+      r = DummyBaseClass.new OriginClass, @response
 
       assert_raise(Elasticsearch::Model::NotImplemented) { |e| r.results }
       assert_raise(Elasticsearch::Model::NotImplemented) { |e| r.records }
diff --git a/elasticsearch-model/test/unit/response_pagination_test.rb b/elasticsearch-model/test/unit/response_pagination_test.rb
@@ -95,10 +95,6 @@ def self.document_type; 'bar'; end
       end
 
       should "reset the variables" do
-        assert_not_nil @response.instance_variable_get(:@response)
-        assert_not_nil @response.instance_variable_get(:@records)
-        assert_not_nil @response.instance_variable_get(:@results)
-
         @response.limit(35)
 
         assert_nil @response.instance_variable_get(:@response)
@@ -119,10 +115,6 @@ def self.document_type; 'bar'; end
       end
 
       should "reset the variables" do
-        assert_not_nil @response.instance_variable_get(:@response)
-        assert_not_nil @response.instance_variable_get(:@records)
-        assert_not_nil @response.instance_variable_get(:@results)
-
         @response.offset(35)
 
         assert_nil @response.instance_variable_get(:@response)
@@ -133,6 +125,7 @@ def self.document_type; 'bar'; end
 
     context "total" do
       should "return the number of hits" do
+        @response.expects(:results).returns(mock('results', total: 100))
         assert_equal 100, @response.total_count
       end
     end
diff --git a/elasticsearch-model/test/unit/response_records_test.rb b/elasticsearch-model/test/unit/response_records_test.rb
@@ -11,19 +11,24 @@ def empty?;       ['FOO'].empty?;       end
       def foo;          'BAR';                end
     end
 
-    class DummyClass
+    class DummyModel
+      def self.index_name;    'foo'; end
+      def self.document_type; 'bar'; end
+
       def self.find(*args)
         DummyCollection.new
       end
     end
 
     RESPONSE = { 'hits' => { 'total' => 123, 'max_score' => 456, 'hits' => [{'foo' => 'bar'}] } }
-    RESULTS  = Elasticsearch::Model::Response::Results.new DummyClass, RESPONSE
+    RESULTS  = Elasticsearch::Model::Response::Results.new DummyModel, RESPONSE
 
     setup do
-      @records = Elasticsearch::Model::Response::Records.new DummyClass,
-                                                             RESPONSE,
-                                                             RESULTS
+      search   = Elasticsearch::Model::Searching::SearchRequest.new DummyModel, '*'
+      search.stubs(:execute!).returns RESPONSE
+
+      response = Elasticsearch::Model::Response::Response.new DummyModel, search
+      @records = Elasticsearch::Model::Response::Records.new DummyModel, response
     end
 
     should "access the records" do
@@ -68,12 +73,11 @@ def records_mixin
 
       should "delegate the records method to the adapter" do
         Elasticsearch::Model::Adapter.expects(:from_class)
-                                     .with(DummyClass)
+                                     .with(DummyModel)
                                      .returns(DummyAdapter)
 
-        @records = Elasticsearch::Model::Response::Records.new DummyClass,
-                                                               RESPONSE,
-                                                               RESULTS
+        @records = Elasticsearch::Model::Response::Records.new DummyModel,
+                                                               RESPONSE
 
         assert_equal ['FOOBAR'], @records.records
       end
diff --git a/elasticsearch-model/test/unit/response_results_test.rb b/elasticsearch-model/test/unit/response_results_test.rb
diff --git a/elasticsearch-model/test/unit/response_test.rb b/elasticsearch-model/test/unit/response_test.rb
diff --git a/elasticsearch-model/test/unit/searching_test.rb b/elasticsearch-model/test/unit/searching_test.rb

Original file line number	Diff line number	Diff line change
`@@ -13,12 +13,12 @@ module Records`
`13`	`13`	# Returns an `ActiveRecord::Relation` instance
`14`	`14`	`#`
`15`	`15`	`def records`
`16`		`- sql_records = klass.where(id: @ids)`
	`16`	`+ sql_records = klass.where(id: ids)`
`17`	`17`
`18`	`18`	`# Re-order records based on the order from Elasticsearch hits`
`19`	`19`	# by redefining `to_a`, unless the user has called `order()`
`20`	`20`	`#`
`21`		`- sql_records.instance_exec(response['hits']['hits']) do \|hits\|`
	`21`	`+ sql_records.instance_exec(response.response['hits']['hits']) do \|hits\|`
`22`	`22`	`define_singleton_method :to_a do`
`23`	`23`	`if defined?(::ActiveRecord) && ::ActiveRecord::VERSION::MAJOR >= 4`
`24`	`24`	`self.load`
Original file line number	Diff line number	Diff line change
`@@ -96,9 +96,8 @@ module ClassMethods`
`96`	`96`	`#`
`97`	`97`	`def search(query_or_payload, options={})`
`98`	`98`	`search = SearchRequest.new(self, query_or_payload, options={})`
`99`		`- response = search.execute!`
`100`	`99`
`101`		`- Response::Response.new(self, search, response)`
	`100`	`+ Response::Response.new(self, search)`
`102`	`101`	`end`
`103`	`102`
`104`	`103`	`end`