[MODEL] Refactorings and cleanups for the multi-model search

Many thanks to @miguelff for the implementation!

* Rephrase documentation in README and code annotations
* Move the `Registry` class to the multimodel.rb file
* Clean up adapters/multiple.rb
* Tweak test descriptions and values

Closes elastic#345
Closes elastic#10
Closes elastic#30
Closes elastic#50
Closes elastic#129

Author: karmi

elasticsearch-model/README.md

@@ -172,7 +172,7 @@ response.results.first._source.title
 # => "Quick brown fox"
 ```
 
-Or if we want to perform the search across different models:
+To search across multiple models, use the module method:
 
 ```ruby
 Elasticsearch::Model.search 'fox dogs', [Article, Comment]
@@ -223,17 +223,8 @@ response.records.to_a
 
 The returned object is the genuine collection of model instances returned by your database,
 i.e. `ActiveRecord::Relation` for ActiveRecord, or `Mongoid::Criteria` in case of MongoDB.
-When the search is performed across different models, an Array of model instances is returned.
 
-```ruby
-Elasticsearch::Model.search('fox', [Article, Comment]).records
-# Article Load (0.3ms)  SELECT "articles".* FROM "articles" WHERE "articles"."id" IN (1)
-# Comment Load (0.2ms)  SELECT "comments".* FROM "comments" WHERE "comments"."id" IN (1,5)
-# => [#<Article id: 1, title: "Quick brown fox">, #<Comment id: 1, body: "I like foxes">,  #<Comment id: 5, body: "Michael J.Fox is my favorite actor">]
-```
-
-In the case of searching a single model, this allows you to
-chain other methods on top of search results, as you would normally do:
+This allows you to chain other methods on top of search results, as you would normally do:
 
 ```ruby
 response.records.where(title: 'Quick brown fox').to_a
@@ -268,6 +259,26 @@ response.records.each_with_hit { |record, hit| puts "* #{record.title}: #{hit._s
 # * Fast black dogs: 0.02250402
 ```
 
+It is possible to search across multiple models with the module method:
+
+```ruby
+Elasticsearch::Model.search('fox', [Article, Comment]).results.to_a.map(&:to_hash)
+# => [
+#      {"_index"=>"articles", "_type"=>"article", "_id"=>"1", "_score"=>0.35136628, "_source"=>...},
+#      {"_index"=>"comments", "_type"=>"comment", "_id"=>"1", "_score"=>0.35136628, "_source"=>...}
+#    ]
+
+Elasticsearch::Model.search('fox', [Article, Comment]).records.to_a
+# Article Load (0.3ms)  SELECT "articles".* FROM "articles" WHERE "articles"."id" IN (1)
+# Comment Load (0.2ms)  SELECT "comments".* FROM "comments" WHERE "comments"."id" IN (1,5)
+# => [#<Article id: 1, title: "Quick brown fox">, #<Comment id: 1, body: "Fox News">,  ...]
+```
+
+By default, all models which include the `Elasticsearch::Model` module are searched.
+
+NOTE: It is _not_ possible to chain other methods on top of the `records` object, since it
+      is a heterogenous collection, with models potentially backed by different databases.
+
 #### Pagination
 
 You can implement pagination with the `from` and `size` search parameters. However, search results

elasticsearch-model/lib/elasticsearch/model.rb

@@ -67,50 +67,6 @@ module Elasticsearch
   module Model
     METHODS = [:search, :mapping, :mappings, :settings, :index_name, :document_type, :import]
 
-
-    # Keeps a registry of the classes that include `Elasticsearch::Model`
-    #
-    class Registry
-
-      # Add the class of a model to the registry
-      #
-      def self.add(klass)
-        __instance.add(klass)
-      end
-
-      # List all the registered models
-      #
-      # @return [Class]
-      #
-      def self.all
-        __instance.models
-      end
-
-      # Returns the unique instance of the registry
-      #
-      # @api private
-      #
-      def self.__instance
-        @instance ||= new
-      end
-
-      def initialize
-        @models = []
-      end
-
-      # Adds a model to the registry
-      #
-      def add(klass)
-        @models << klass
-      end
-
-      # Gets a copy of the registered models
-      #
-      def models
-        @models.dup
-      end
-    end
-
     # Adds the `Elasticsearch::Model` functionality to the including class.
     #
     # * Creates the `__elasticsearch__` class and instance methods, pointing to the proxy object
@@ -199,20 +155,22 @@ def client=(client)
         @client = client
       end
 
-      # Search across models which include Elasticsearch::Model
+      # Search across multiple models
+      #
+      # By default, all models which include the `Elasticsearch::Model` module are searched
       #
       # @param query_or_payload [String,Hash,Object] The search request definition
       #                                              (string, JSON, Hash, or object responding to `to_hash`)
-      # @param models [Array] The list of Model objects to search
+      # @param models [Array] The Array of Model objects to search
       # @param options [Hash] Optional parameters to be passed to the Elasticsearch client
       #
       # @return [Elasticsearch::Model::Response::Response]
       #
-      # @example Search across specified models
+      # @example Search across specific models
       #
       #     Elasticsearch::Model.search('foo', [Author, Article])
       #
-      # @example Search across all models
+      # @example Search across all models which include the `Elasticsearch::Model` module
       #
       #     Elasticsearch::Model.search('foo')
       #

elasticsearch-model/lib/elasticsearch/model/adapters/multiple.rb

@@ -2,125 +2,106 @@ module Elasticsearch
   module Model
     module Adapter
 
-      # An adapter to be used for deserializing results from multiple models, retrieved through
-      # Elasticsearch::Model.search
+      # An adapter to be used for deserializing results from multiple models,
+      # retrieved through `Elasticsearch::Model.search`
       #
       # @see Elasticsearch::Model.search
       #
       module Multiple
-
         Adapter.register self, lambda { |klass| klass.is_a? Multimodel }
 
         module Records
-
-          # Returns an Array, which elements are the model instances represented
-          # by the search results.
-          #
-          # This means that if the models queried are a Mixture of ActiveRecord, Mongoid, or
-          # POROs, the elements contained in this array will also be instances of those models
-          #
-          # Ranking of results across multiple indexes is preserved, and queries made to the different
-          # model's datasources are minimal.
-          #
-          # Internally, it gets the results, as ranked by elasticsearch.
-          # Then results are grouped by _type
-          # Then the model corresponding to each _type is queried to retrieve the records
-          # Finally records are rearranged in the same way results were ranked.
+          # Returns a collection of model instances, possibly of different classes (ActiveRecord, Mongoid, ...)
           #
-          # @return [ElasticSearch::Model]
+          # @note The order of results in the Elasticsearch response is preserved
           #
           def records
-            @_records ||= begin
-              result = []
-              by_type = __records_by_type
-              __hits.each do |hit|
-                result << by_type[__type(hit)][hit[:_id]]
-              end
-              result.compact
+            records_by_type = __records_by_type
+
+            response.response["hits"]["hits"].map do |hit|
+              records_by_type[ __type_for_hit(hit) ][ hit[:_id] ]
             end
           end
 
-          # Returns the record representation of the results retrieved from Elasticsearch, grouped
-          # by model type
+          # Returns the collection of records grouped by class based on `_type`
           #
-          # @example
-          # {Series  =>
-          #   {"1"=> #<Series id: 1, series_name: "The Who S01", created_at: "2015-02-23 17:18:28">},
+          # Example:
           #
-          # "Title =>
-          #   {"1"=> #<Title id: 1, name: "Who Strikes Back", created_at: "2015-02-23 17:18:28">}}
+          # {
+          #   Foo  => {"1"=> #<Foo id: 1, title: "ABC"}, ...},
+          #   Bar  => {"1"=> #<Bar id: 1, name: "XYZ"}, ...}
+          # }
           #
           # @api private
           #
           def __records_by_type
-            array = __ids_by_type.map do |klass, ids|
-              records = __type_records(klass, ids)
-              ids = records.map(&:id).map(&:to_s)
-              [klass, Hash[ids.zip(records)]]
+            result = __ids_by_type.map do |klass, ids|
+              records = __records_for_klass(klass, ids)
+              ids     = records.map(&:id).map(&:to_s)
+              [ klass, Hash[ids.zip(records)] ]
             end
-            Hash[array]
+
+            Hash[result]
           end
 
-          # Returns the records for a specific type
+          # Returns the collection of records for a specific type based on passed `klass`
           #
           # @api private
           #
-          def __type_records(klass, ids)
-            if (adapter = Adapter.adapters[ActiveRecord]) && adapter.call(klass)
-              klass.where(klass.primary_key => ids)
-            elsif (adapter = Adapter.adapters[Mongoid]) && adapter.call(klass)
-              klass.where(:id.in => ids)
-            else
-              klass.find(ids)
+          def __records_for_klass(klass, ids)
+            adapter = __adapter_name_for_klass(klass)
+
+            case adapter
+              when Elasticsearch::Model::Adapter::ActiveRecord
+                klass.where(klass.primary_key => ids)
+              when Elasticsearch::Model::Adapter::Mongoid
+                klass.where(:id.in => ids)
+              else
+                klass.find(ids)
             end
           end
 
-
-          # @return A Hash containing for each type, the ids to retrieve
+          # Returns the record IDs grouped by class based on type `_type`
+          #
+          # Example:
           #
-          # @example {Series =>["1"], Title =>["1", "5"]}
+          #   { Foo => ["1"], Bar => ["1", "5"] }
           #
           # @api private
           #
           def __ids_by_type
             ids_by_type = {}
-            __hits.each do |hit|
-              type = __type(hit)
+
+            response.response["hits"]["hits"].each do |hit|
+              type = __type_for_hit(hit)
               ids_by_type[type] ||= []
               ids_by_type[type] << hit[:_id]
             end
             ids_by_type
           end
 
-          # Returns the class of the model associated to a certain hit
-          #
-          # A simple class-level memoization over the `_index` and `_type` properties of the hit is applied.
-          # Hence querying the Model Registry is done the minimal amount of times.
-          #
-          # Event though memoization happens at the class level, the side effect of a race condition will only be
-          # to iterate over models one extra time, so we can consider the method thread-safe, and don't include
-          # any Mutex.synchronize around the method implementaion
+          # Returns the class of the model corresponding to a specific `hit` in Elasticsearch results
           #
           # @see Elasticsearch::Model::Registry
           #
-          # @return Class
-          #
           # @api private
           #
-          def __type(hit)
+          def __type_for_hit(hit)
             @@__types ||= {}
-            @@__types[[hit[:_index], hit[:_type]].join("::")] ||= begin
-              Registry.all.detect { |model| model.index_name == hit[:_index] && model.document_type == hit[:_type] }
+
+            @@__types[ "#{hit[:_index]}::#{hit[:_type]}" ] ||= begin
+              Registry.all.detect do |model|
+                model.index_name == hit[:_index] && model.document_type == hit[:_type]
+              end
             end
           end
 
-
-          # Memoizes and returns the hits from the response
+          # Returns the adapter registered for a particular `klass` or `nil` if not available
           #
           # @api private
           #
-          def __hits
-            @__hits ||= response.response["hits"]["hits"]
+          def __adapter_name_for_klass(klass)
+            Adapter.adapters.select { |name, checker| checker.call(klass) }.keys.first
           end
         end
       end

elasticsearch-model/lib/elasticsearch/model/multimodel.rb

@@ -1,31 +1,71 @@
 module Elasticsearch
   module Model
 
-    # Wraps a series of models to be used when querying multiple indexes via Elasticsearch::Model.search
+    # Keeps a global registry of classes that include `Elasticsearch::Model`
+    #
+    class Registry
+      def initialize
+        @models = []
+      end
+
+      # Returns the unique instance of the registry (Singleton)
+      #
+      # @api private
+      #
+      def self.__instance
+        @instance ||= new
+      end
+
+      # Adds a model to the registry
+      #
+      def self.add(klass)
+        __instance.add(klass)
+      end
+
+      # Returns an Array of registered models
+      #
+      def self.all
+        __instance.models
+      end
+
+      # Adds a model to the registry
+      #
+      def add(klass)
+        @models << klass
+      end
+
+      # Returns a copy of the registered models
+      #
+      def models
+        @models.dup
+      end
+    end
+
+    # Wraps a collection of models when querying multiple indices
     #
     # @see Elasticsearch::Model.search
     #
     class Multimodel
-
       attr_reader :models
 
-      # @param models [Class] The list of models across which the search will be performed.
+      # @param models [Class] The list of models across which the search will be performed
+      #
       def initialize(*models)
         @models = models.flatten
         @models = Model::Registry.all if @models.empty?
       end
 
-      # Get the list of index names used for retrieving documents when doing a search across multiple models
+      # Get an Array of index names used for retrieving documents when doing a search across multiple models
       #
-      # @return [String] the list of index names used for retrieving documents
+      # @return [Array] the list of index names used for retrieving documents
       #
       def index_name
         models.map { |m| m.index_name }
       end
 
-      # Get the list of document types used for retrieving documents when doing a search across multiple models
+      # Get an Array of document types used for retrieving documents when doing a search across multiple models
       #
-      # @return [String] the list of document types used for retrieving documents
+      # @return [Array] the list of document types used for retrieving documents
       #
       def document_type
         models.map { |m| m.document_type }