Permalink
Browse files

Attempt to simplify the eager loading code.

Took advantage of some ActiveSupport methods, since ActiveSupport is
already a dependancy of ActiveModel.
  • Loading branch information...
1 parent a3a696a commit 8e308301a58e2e07d7a6f860a75c4febb4626fa1 @dylanahsmith committed Nov 4, 2011
Showing with 20 additions and 34 deletions.
  1. +14 −28 lib/tire/results/collection.rb
  2. +6 −6 test/unit/results_collection_test.rb
@@ -40,46 +40,32 @@ def results
else
return [] if hits.empty?
- ranked_results = hits.map do |hit|
- type = hit['_type']
+ load_options = options[:load] === true ? {} : options[:load]
+
+ # Collect all ids of one type to perform one database query per type
+ records_by_type = {}
+ hits.group_by { |hit| hit['_type'] }.each do |type, hits|
raise NoMethodError, "You have tried to eager load the model instances, " +
"but Tire cannot find the model class because " +
"document has no _type property." unless type
- [type, hit['_id']]
- end
-
- # Collect all ids of one type to perform one database query per type
- ids_by_type = {}
- ranked_results.each do |type, id|
- ids_by_type[type] ||= []
- ids_by_type[type] <<= id
- end
- objects_by_type = {}
- ids_by_type.each do |type, ids|
+ ids = hits.map{ |hit| hit['_id'] }
begin
model = type.camelize.constantize
rescue NameError => e
- raise NameError, "You have tried to eager load the model instances, but " +
- "Tire cannot find the model class '#{type.camelize}' " +
- "based on _type '#{type}'.", e.backtrace
- end
-
- objects_by_id = {}
- load_options = options[:load] === true ? {} : options[:load]
- # Tolerate missing records
- model.where(model.primary_key.to_sym => ids).find(:all, load_options).each do |result|
- objects_by_id[result.id.to_s] = result
+ raise NameError, "Cannot find the model class '#{type.camelize}' " +
+ "based on _type '#{type}' during eager loading.", e.backtrace
end
- objects_by_type[type] = objects_by_id
+ records = model.where(model.primary_key => ids).all(load_options)
+ records_by_type[type] = records.index_by(&:id)
end
# Preserve original order from search results
- ranked_results.map! { |type, id| objects_by_type[type][id.to_s] }
- ranked_results.compact!
+ records = hits.map { |hit| records_by_type[hit['_type']][hit['_id'].to_i] }
+ records.compact!
# Remove missing records from the total
- @total -= hits.size - ranked_results.size
+ @total -= hits.size - records.size
- ranked_results
+ records
end
end
end
@@ -218,9 +218,9 @@ class ResultsCollectionTest < Test::Unit::TestCase
should "load the records via model find method from database" do
mock_relation = Object.new
- ActiveRecordArticle.expects(:where).with(:id => [1, 2, 3]).
+ ActiveRecordArticle.expects(:where).with('id' => [1, 2, 3]).
returns(mock_relation)
- mock_relation.expects(:find).with(:all, {}).
+ mock_relation.expects(:all).with({}).
returns([ Results::Item.new(:id => 3),
Results::Item.new(:id => 1),
Results::Item.new(:id => 2) ])
@@ -229,9 +229,9 @@ class ResultsCollectionTest < Test::Unit::TestCase
should "pass the :load option Hash to model find metod" do
mock_relation = Object.new
- ActiveRecordArticle.expects(:where).with(:id => [1, 2, 3]).
+ ActiveRecordArticle.expects(:where).with('id' => [1, 2, 3]).
returns(mock_relation)
- mock_relation.expects(:find).with(:all, { :include => 'comments' }).
+ mock_relation.expects(:all).with({ :include => 'comments' }).
returns([ Results::Item.new(:id => 3),
Results::Item.new(:id => 1),
Results::Item.new(:id => 2) ])
@@ -240,9 +240,9 @@ class ResultsCollectionTest < Test::Unit::TestCase
should "preserve the order of records returned from search" do
mock_relation = Object.new
- ActiveRecordArticle.expects(:where).with(:id => [1, 2, 3]).
+ ActiveRecordArticle.expects(:where).with('id' => [1, 2, 3]).
returns(mock_relation)
- mock_relation.expects(:find).with(:all, {}).
+ mock_relation.expects(:all).with({}).
returns([ Results::Item.new(:id => 3),
Results::Item.new(:id => 1),
Results::Item.new(:id => 2) ])

0 comments on commit 8e30830

Please sign in to comment.