From dc0c5de27d6db53f79d72b677f7fd1113251403b Mon Sep 17 00:00:00 2001 From: Jey Balachandran Date: Thu, 25 Jul 2013 22:28:05 -0700 Subject: [PATCH 1/2] Update __find_records_by_ids to implement per ORM. --- lib/tire/results/collection.rb | 50 ++++++++++++- test/unit/results_collection_test.rb | 103 +++++++++++++++++---------- 2 files changed, 116 insertions(+), 37 deletions(-) diff --git a/lib/tire/results/collection.rb b/lib/tire/results/collection.rb index 6fbb3ac2..565b8b4b 100644 --- a/lib/tire/results/collection.rb +++ b/lib/tire/results/collection.rb @@ -153,7 +153,55 @@ def __get_results_with_load(hits) end def __find_records_by_ids(klass, ids) - @options[:load] === true ? klass.find(ids) : klass.find(ids, @options[:load]) + Strategy.from_class(klass, @options).find(ids) + end + end + + # Importing strategies for common persistence frameworks (ActiveModel, Mongoid), as well as + # pagination libraries (WillPaginate, Kaminari), or a custom strategy. + module Strategy + def self.from_class(klass, options={}) + return const_get(options[:strategy]).new(klass, options) if options[:strategy] + + case + when defined?(::ActiveRecord) && klass.ancestors.include?(::ActiveRecord::Base) + ActiveRecord.new klass, options + else + Default.new klass, options + end + end + + module Base + attr_reader :klass, :options + + def initialize(klass, options={}) + @klass = klass + @options = options + end + end + + class ActiveRecord + include Base + + def find(ids) + if options[:load] === true + klass.where(:id => ids) + else + klass.where(:id => ids).all(options[:load]) + end + end + end + + class Default + include Base + + def find(ids) + if options[:load] === true + klass.find(ids) + else + klass.find(ids, options[:load]) + end + end end end diff --git a/test/unit/results_collection_test.rb b/test/unit/results_collection_test.rb index 9db6f391..d8fb87f5 100644 --- a/test/unit/results_collection_test.rb +++ b/test/unit/results_collection_test.rb @@ -258,26 +258,31 @@ class ResultsCollectionTest < Test::Unit::TestCase end end - should "yield the model instance and the raw hit" do - response = { 'hits' => { 'hits' => [ {'_id' => 1, '_score' => 0.5, '_type' => 'active_record_article'} ] } } - - ActiveRecord::Base.establish_connection( :adapter => 'sqlite3', :database => ":memory:" ) - ActiveRecord::Migration.verbose = false - ActiveRecord::Schema.define(:version => 1) { create_table(:active_record_articles) { |t| t.string(:title) } } - model = ActiveRecordArticle.new(:title => 'Test'); model.id = 1 - - ActiveRecordArticle.expects(:find).with([1]).returns([ model] ) - - Results::Collection.new(response, :load => true).each_with_hit do |result, hit| - assert_instance_of ActiveRecordArticle, result - assert_instance_of Hash, hit - assert_equal 'Test', result.title - assert_equal 0.5, hit['_score'] + { + Tire::Results::Strategy::Default => lambda { |model| ActiveRecordArticle.expects(:find).with([1]).returns([model]) }, + Tire::Results::Strategy::ActiveRecord => lambda { |model| ActiveRecordArticle.expects(:where).with(:id => [1]).returns([model]) } + }.each_pair do |strategy, expects| + should "yield the model instance and the raw hit for #{strategy}" do + Tire::Results::Strategy.stubs(:from_class).returns(strategy.new(ActiveRecordArticle, load: true)) + + response = { 'hits' => { 'hits' => [ {'_id' => 1, '_score' => 0.5, '_type' => 'active_record_article'} ] } } + + ActiveRecord::Base.establish_connection( :adapter => 'sqlite3', :database => ":memory:" ) + ActiveRecord::Migration.verbose = false + ActiveRecord::Schema.define(:version => 1) { create_table(:active_record_articles) { |t| t.string(:title) } } + model = ActiveRecordArticle.new(:title => 'Test'); model.id = 1 + + expects.call(model) + + Results::Collection.new(response, :load => true).each_with_hit do |result, hit| + assert_instance_of ActiveRecordArticle, result + assert_instance_of Hash, hit + assert_equal 'Test', result.title + assert_equal 0.5, hit['_score'] + end end - end - end context "while paginating results" do @@ -344,28 +349,54 @@ class ResultsCollectionTest < Test::Unit::TestCase ActiveRecordArticle.stubs(:inspect).returns("") end - should "load the records via model find method from database" do - ActiveRecordArticle.expects(:find).with([1,2,3]). - returns([ Results::Item.new(:id => 3), - Results::Item.new(:id => 1), - Results::Item.new(:id => 2) ]) - Results::Collection.new(@response, :load => true).results - end + { + Tire::Results::Strategy::Default => lambda { |ids, items| ActiveRecordArticle.expects(:find).with(ids).returns(items) }, + Tire::Results::Strategy::ActiveRecord => lambda { |ids, items| ActiveRecordArticle.expects(:where).with(:id => ids).returns(items) } + }.each_pair do |strategy, expects| + should "load the records via model find method from database for #{strategy}" do + Tire::Results::Strategy.stubs(:from_class).returns(strategy.new(ActiveRecordArticle, :load => true)) + + expects.call([1, 2, 3], [ + Results::Item.new(:id => 3), + Results::Item.new(:id => 1), + Results::Item.new(:id => 2) + ]) + + Results::Collection.new(@response, :load => true).results + end - should "pass the :load option Hash to model find metod" do - ActiveRecordArticle.expects(:find).with([1,2,3], :include => 'comments'). - returns([ Results::Item.new(:id => 3), - Results::Item.new(:id => 1), - Results::Item.new(:id => 2) ]) - Results::Collection.new(@response, :load => { :include => 'comments' }).results + should "preserve the order of records returned from search for #{strategy}" do + Tire::Results::Strategy.stubs(:from_class).returns(strategy.new(ActiveRecordArticle, :load => true)) + + expects.call([1, 2, 3], [ + Results::Item.new(:id => 3), + Results::Item.new(:id => 1), + Results::Item.new(:id => 2) + ]) + + assert_equal [1,2,3], Results::Collection.new(@response, :load => true).results.map(&:id) + end end - should "preserve the order of records returned from search" do - ActiveRecordArticle.expects(:find).with([1,2,3]). - returns([ Results::Item.new(:id => 3), - Results::Item.new(:id => 1), - Results::Item.new(:id => 2) ]) - assert_equal [1,2,3], Results::Collection.new(@response, :load => true).results.map(&:id) + { + Tire::Results::Strategy::Default => lambda { |ids, load_options, items| ActiveRecordArticle.expects(:find).with(ids, load_options).returns(items) }, + Tire::Results::Strategy::ActiveRecord => lambda { |ids, load_options, items| + relation = Struct.new(:all) + relation.expects(:all).with(load_options).returns(items) + ActiveRecordArticle.expects(:where).with(:id => ids).returns(relation) + } + }.each_pair do |strategy, expects| + should "pass the :load option Hash to model find method for #{strategy}" do + Tire::Results::Strategy.stubs(:from_class).returns(strategy.new(ActiveRecordArticle, :load => { :include => "comments" })) + + expects.call([1, 2, 3], { :include => "comments" }, [ + Results::Item.new(:id => 3), + Results::Item.new(:id => 1), + Results::Item.new(:id => 2) + ]) + + Results::Collection.new(@response, :load => { :include => 'comments' }).results + end end should "raise error when model class cannot be inferred from _type" do From a252a4a3973e2b169de9246c21db4b56af621db2 Mon Sep 17 00:00:00 2001 From: Jey Balachandran Date: Thu, 25 Jul 2013 22:33:13 -0700 Subject: [PATCH 2/2] Update to Ruby 1.8 Hash syntax. --- test/unit/results_collection_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/results_collection_test.rb b/test/unit/results_collection_test.rb index d8fb87f5..2c0e9bfc 100644 --- a/test/unit/results_collection_test.rb +++ b/test/unit/results_collection_test.rb @@ -263,7 +263,7 @@ class ResultsCollectionTest < Test::Unit::TestCase Tire::Results::Strategy::ActiveRecord => lambda { |model| ActiveRecordArticle.expects(:where).with(:id => [1]).returns([model]) } }.each_pair do |strategy, expects| should "yield the model instance and the raw hit for #{strategy}" do - Tire::Results::Strategy.stubs(:from_class).returns(strategy.new(ActiveRecordArticle, load: true)) + Tire::Results::Strategy.stubs(:from_class).returns(strategy.new(ActiveRecordArticle, :load => true)) response = { 'hits' => { 'hits' => [ {'_id' => 1, '_score' => 0.5, '_type' => 'active_record_article'} ] } }