Adding excerpts/sphinx_attributes/matching_fields to all ActiveRecord…

… objects, and setting them as part of search results population. This means you can now serialise searches, and may even speed up search requests a little as well (now that we're not adding up to three instance methods on-the-fly to each search result).

lib/thinking_sphinx/active_record.rb

@@ -322,6 +322,10 @@ def eldest_indexed_ancestor
       end
     end
 
+    attr_accessor :excerpts
+    attr_accessor :sphinx_attributes
+    attr_accessor :matching_fields
+
     def in_index?(suffix)
       self.class.search_for_id self.sphinx_document_id, sphinx_index_name(suffix)
     end

lib/thinking_sphinx/search.rb

@@ -346,43 +346,32 @@ def populate
 
     def add_excerpter
       each do |object|
-        next if object.respond_to?(:excerpts)
+        next if object.nil?
 
-        excerpter = ThinkingSphinx::Excerpter.new self, object
-        block = lambda { excerpter }
-
-        object.singleton_class.instance_eval do
-          define_method(:excerpts, &block)
-        end
+        object.excerpts = ThinkingSphinx::Excerpter.new self, object
       end
     end
 
     def add_sphinx_attributes
       each do |object|
-        next if object.nil? || object.respond_to?(:sphinx_attributes)
+        next if object.nil?
 
         match = match_hash object
         next if match.nil?
 
-        object.singleton_class.instance_eval do
-          define_method(:sphinx_attributes) { match[:attributes] }
-        end
+        object.sphinx_attributes = match[:attributes]
       end
     end
 
     def add_matching_fields
       each do |object|
-        next if object.nil? || object.respond_to?(:matching_fields)
+        next if object.nil?
 
         match = match_hash object
         next if match.nil?
-        fields = ThinkingSphinx::Search.matching_fields(
+        object.matching_fields = ThinkingSphinx::Search.matching_fields(
           @results[:fields], match[:weight]
         )
-
-        object.singleton_class.instance_eval do
-          define_method(:matching_fields) { fields }
-        end
       end
     end
 

spec/thinking_sphinx/facet_search_spec.rb

@@ -1,109 +1,49 @@
 require 'spec_helper'
 
 describe ThinkingSphinx::FacetSearch do
+  let(:search) { stub('search', :append_to => nil, :empty? => true) }
+  let(:config) { ThinkingSphinx::Configuration.instance }
+  let(:client) { stub('client', :run => []) }
+
+  before :each do
+    config.stub!(:client => client)
+  end
+
   describe 'populate' do
-    it "should make separate Sphinx queries for each facet" do
-      ThinkingSphinx.should_receive(:search).with(
-        hash_including(:group_by => 'city_facet')
-      ).and_return([])
-      ThinkingSphinx.should_receive(:search).with(
-        hash_including(:group_by => 'state_facet')
-      ).and_return([])
-      ThinkingSphinx.should_receive(:search).with(
-        hash_including(:group_by => 'birthday')
-      ).and_return([])
-
-      ThinkingSphinx::FacetSearch.new(:classes => [Person])
+    before :each do
+      config.configuration.searchd.max_matches = 10_000
     end
-
+        
     it "should request all shared facets in a multi-model request by default" do
-      ThinkingSphinx.stub!(:search => [])
+      ThinkingSphinx.stub!(:search => search)
       ThinkingSphinx::FacetSearch.new.facet_names.should == ['class_crc']
     end
 
     it "should request all facets in a multi-model request if specified" do
-      ThinkingSphinx.stub!(:search => [])
+      ThinkingSphinx.stub!(:search => search)
       ThinkingSphinx::FacetSearch.new(
         :all_facets => true
       ).facet_names.should == [
         'class_crc', 'city_facet', 'state_facet', 'birthday'
       ]
     end
 
-    describe ':facets option' do
-      it "should limit facets to the requested set" do
-        ThinkingSphinx.should_receive(:search).once.and_return([])
-
-        ThinkingSphinx::FacetSearch.new(
-          :classes => [Person], :facets => :state
-        )
-      end
-    end
-
-    describe "empty result set for attributes" do
-      before :each do
-        ThinkingSphinx.stub!(:search => [])
-        @facets = ThinkingSphinx::FacetSearch.new(
-          :classes => [Person], :facets => :state
-        )
-      end
-
-      it "should add key as attribute" do
-        @facets.should have_key(:state)
-      end
-
-      it "should return an empty hash for the facet results" do
-        @facets[:state].should be_empty
-      end
-    end
-
-    describe "non-empty result set" do
-      before :each do
-        @person = Person.find(:first)
-        @people = [@person]
-        @people.stub!(:each_with_match).
-          and_yield(@person, {:attributes => {'@groupby' => @person.city.to_crc32, '@count' => 1}})
-        ThinkingSphinx.stub!(:search => @people)
-
-        @facets = ThinkingSphinx::FacetSearch.new(
-          :classes => [Person], :facets => :city
-        )
-      end
-
-      it "should return a hash" do
-        @facets.should be_a_kind_of(Hash)
-      end
-
-      it "should add key as attribute" do
-        @facets.keys.should include(:city)
-      end
-
-      it "should return a hash" do
-        @facets[:city].should == {@person.city => 1}
-      end
-    end
-
-    before :each do
-      @config = ThinkingSphinx::Configuration.instance
-      @config.configuration.searchd.max_matches = 10_000
-    end
-
     it "should use the system-set max_matches for limit on facet calls" do
       ThinkingSphinx.should_receive(:search) do |options|
         options[:max_matches].should  == 10_000
         options[:limit].should        == 10_000
-        []
+        search
       end
 
       ThinkingSphinx::FacetSearch.new
     end
 
     it "should use the default max-matches if there is no explicit setting" do
-      @config.configuration.searchd.max_matches = nil
+      config.configuration.searchd.max_matches = nil
       ThinkingSphinx.should_receive(:search) do |options|
         options[:max_matches].should  == 1000
         options[:limit].should        == 1000
-        []
+        search
       end
 
       ThinkingSphinx::FacetSearch.new
@@ -113,7 +53,7 @@
       ThinkingSphinx.should_receive(:search) do |options|
         options[:max_matches].should  == 10_000
         options[:limit].should        == 10_000
-        []
+        search
       end
 
       ThinkingSphinx::FacetSearch.new(
@@ -125,7 +65,7 @@
     it "should not use an explicit :page" do
       ThinkingSphinx.should_receive(:search) do |options|
         options[:page].should == 1
-        []
+        search
       end
 
       ThinkingSphinx::FacetSearch.new(:page => 3)
@@ -149,15 +89,69 @@
         }.should raise_error
       end
     end
+
+    describe ':facets option' do
+      it "should limit facets to the requested set" do
+        ThinkingSphinx.should_receive(:search).once.and_return(search)
+
+        ThinkingSphinx::FacetSearch.new(
+          :classes => [Person], :facets => :state
+        )
+      end
+    end
+
+    describe "empty result set for attributes" do
+      before :each do
+        ThinkingSphinx.stub!(:search => search)
+        @facets = ThinkingSphinx::FacetSearch.new(
+          :classes => [Person], :facets => :state
+        )
+      end
+
+      it "should add key as attribute" do
+        @facets.should have_key(:state)
+      end
+
+      it "should return an empty hash for the facet results" do
+        @facets[:state].should be_empty
+      end
+    end
+
+    describe "non-empty result set" do
+      before :each do
+        @person = Person.find(:first)
+        @people = [@person]
+        search.stub!(:empty? => false)
+        search.stub!(:each_with_match).
+          and_yield(@person, {:attributes => {'@groupby' => @person.city.to_crc32, '@count' => 1}})
+        ThinkingSphinx::Search.stub!(:bundle_searches => [search])
+
+        @facets = ThinkingSphinx::FacetSearch.new(
+          :classes => [Person], :facets => :city
+        )
+      end
+
+      it "should return a hash" do
+        @facets.should be_a_kind_of(Hash)
+      end
+
+      it "should add key as attribute" do
+        @facets.keys.should include(:city)
+      end
+
+      it "should return a hash" do
+        @facets[:city].should == {@person.city => 1}
+      end
+    end
   end
 
   describe "#for" do
     before do
       @person = Person.find(:first)
       @people = [@person]
-      @people.stub!(:each_with_match).
+      search.stub!(:each_with_match).
         and_yield(@person, {:attributes => {'@groupby' => @person.city.to_crc32, '@count' => 1}})
-      ThinkingSphinx.stub!(:search => @people)
+      ThinkingSphinx::Search.stub!(:bundle_searches => [search])
 
       @facets = ThinkingSphinx::FacetSearch.new(
         :classes => [Person], :facets => :city

spec/thinking_sphinx/search_methods_spec.rb

@@ -135,6 +135,10 @@
   end
 
   describe '.facets' do
+    before :each do
+      ThinkingSphinx::Search.stub!(:bundle_searches => [])
+    end
+
     it "should return a FacetSearch instance" do
       Alpha.facets.should be_a(ThinkingSphinx::FacetSearch)
     end

spec/thinking_sphinx/search_spec.rb

@@ -785,9 +785,10 @@
         end
 
         it "should set up the excerpter with the instances and search" do
-          ThinkingSphinx::Excerpter.should_receive(:new).with(@search, @alpha_a)
-          ThinkingSphinx::Excerpter.should_receive(:new).with(@search, @alpha_b)
-
+          [@alpha_a, @beta_b, @alpha_b, @beta_a].each do |object|
+            ThinkingSphinx::Excerpter.should_receive(:new).with(@search, object)
+          end
+
           @search.first
         end
       end
@@ -822,11 +823,6 @@
           search.first.should respond_to(:matching_fields)
         end
 
-        it "should not add matching_fields method if using a different ranking mode" do
-          search = ThinkingSphinx::Search.new :rank_mode => :bm25
-          search.first.should_not respond_to(:matching_fields)
-        end
-
         it "should not add matching_fields method if object already have one" do
           search = ThinkingSphinx::Search.new :rank_mode => :fieldmask
           search.last.matching_fields.should_not be_an(Array)