Merge functionality from #query_solr into #find, and restore 2-arg (p…

…ath + args) form of #find

lib/blacklight/solr/request.rb

@@ -39,7 +39,7 @@ def append_highlight_field(query)
   end
 
   def to_hash
-    reject {|key, value| ARRAY_KEYS.include?(key) && value.empty?}
+    reject {|key, value| ARRAY_KEYS.include?(key) && value.blank?}
   end
 
 end

lib/blacklight/solr_helper.rb

@@ -45,6 +45,7 @@
 #   end  
 
 module Blacklight::SolrHelper
+  extend ActiveSupport::Benchmarkable
   extend ActiveSupport::Concern
   include Blacklight::SearchFields
   include Blacklight::Facet
@@ -82,10 +83,32 @@ def force_to_utf8(value)
     value
   end
 
+  ##
+  # Execute a solr query
+  # @see [RSolr::Client#send_and_receive]
+  # @overload find(solr_path, params)
+  #   Execute a solr query at the given path with the parameters
+  #   @param [String] solr path (defaults to blacklight_config.solr_path)
+  #   @param [Hash] parameters for RSolr::Client#send_and_receive
+  # @overload find(params)
+  #   @param [Hash] parameters for RSolr::Client#send_and_receive
   def find(*args)
-    path = blacklight_config.solr_path
-    response = blacklight_solr.get(path, :params=> args[1])
-    Blacklight::SolrResponse.new(force_to_utf8(response), args[1])
+    # In later versions of Rails, the #benchmark method can do timing
+    # better for us. 
+    Blacklight::SolrHelper.benchmark("Solr fetch", level: :debug) do
+      solr_params = args.extract_options!
+      path = args.first || blacklight_config.solr_path
+      solr_params[:qt] ||= blacklight_config.qt
+      # delete these parameters, otherwise rsolr will pass them through.
+      key = blacklight_config.http_method == :post ? :data : :params
+      res = blacklight_solr.send_and_receive(path, {key=>solr_params.to_hash, method:blacklight_config.http_method})
+
+      solr_response = Blacklight::SolrResponse.new(force_to_utf8(res), solr_params)
+
+      Rails.logger.debug("Solr query: #{solr_params.inspect}")
+      Rails.logger.debug("Solr response: #{solr_response.inspect}") if defined?(::BLACKLIGHT_VERBOSE_LOGGING) and ::BLACKLIGHT_VERBOSE_LOGGING
+      solr_response
+    end
   rescue Errno::ECONNREFUSED => e
     raise Blacklight::Exceptions::ECONNREFUSED.new("Unable to connect to Solr instance using #{blacklight_solr.inspect}")
   end
@@ -394,25 +417,9 @@ def get_search_results(user_params = params || {}, extra_controller_params = {})
   # given a user query,
   # Returns a solr response object
   def query_solr(user_params = params || {}, extra_controller_params = {})
-    # In later versions of Rails, the #benchmark method can do timing
-    # better for us. 
-    bench_start = Time.now
     solr_params = self.solr_search_params(user_params).merge(extra_controller_params)
-    solr_params[:qt] ||= blacklight_config.qt
-    path = blacklight_config.solr_path
-
-    # delete these parameters, otherwise rsolr will pass them through.
-    key = blacklight_config.http_method == :post ? :data : :params
-    res = blacklight_solr.send_and_receive(path, {key=>solr_params.to_hash, method:blacklight_config.http_method})
-
-    solr_response = Blacklight::SolrResponse.new(force_to_utf8(res), solr_params)
-
-    Rails.logger.debug("Solr query: #{solr_params.inspect}")
-    Rails.logger.debug("Solr response: #{solr_response.inspect}") if defined?(::BLACKLIGHT_VERBOSE_LOGGING) and ::BLACKLIGHT_VERBOSE_LOGGING
-    Rails.logger.debug("Solr fetch: #{self.class}#query_solr (#{'%.1f' % ((Time.now.to_f - bench_start.to_f)*1000)}ms)")
-
 
-    solr_response
+    find(solr_params)
   end
 
   # returns a params hash for finding a single solr document (CatalogController #show action)
@@ -434,7 +441,7 @@ def solr_doc_params(id=nil)
   # retrieve a solr document, given the doc id
   def get_solr_response_for_doc_id(id=nil, extra_controller_params={})
     solr_params = solr_doc_params(id).merge(extra_controller_params)
-    solr_response = find((blacklight_config.document_solr_request_handler || blacklight_config.qt), solr_params)
+    solr_response = find(blacklight_config.document_solr_request_handler, solr_params)
     raise Blacklight::Exceptions::InvalidSolrID.new if solr_response.docs.empty?
     document = SolrDocument.new(solr_response.docs.first, solr_response)
     [solr_response, document]
@@ -463,7 +470,7 @@ def get_solr_response_for_field_values(field, values, extra_solr_params = {})
       :spellcheck => 'false'
     }.merge(extra_solr_params)
 
-    solr_response = find(blacklight_config.qt, self.solr_search_params().merge(solr_params) )
+    solr_response = find(self.solr_search_params().merge(solr_params) )
     document_list = solr_response.docs.collect{|doc| SolrDocument.new(doc, solr_response) }
     [solr_response,document_list]
   end
@@ -509,7 +516,7 @@ def solr_facet_params(facet_field, user_params=params || {}, extra_controller_pa
   def get_facet_field_response(facet_field, user_params = params || {}, extra_controller_params = {})
     solr_params = solr_facet_params(facet_field, user_params, extra_controller_params)
     # Make the solr call
-    find(blacklight_config.qt, solr_params)
+    find(solr_params)
   end
 
   # a solr query method
@@ -543,7 +550,7 @@ def get_single_doc_via_search(index, request_params)
     solr_params[:start] = (index - 1) # start at 0 to get 1st doc, 1 to get 2nd.    
     solr_params[:rows] = 1
     solr_params[:fl] = '*'
-    solr_response = find(blacklight_config.qt, solr_params)
+    solr_response = find(solr_params)
     SolrDocument.new(solr_response.docs.first, solr_response) unless solr_response.docs.empty?
   end
 
@@ -562,7 +569,7 @@ def get_previous_and_next_documents_for_search(index, request_params, extra_cont
 
     solr_params[:fl] = '*'
     solr_params[:facet] = false
-    solr_response = find(blacklight_config.qt, solr_params)
+    solr_response = find(solr_params)
 
     document_list = solr_response.docs.collect{|doc| SolrDocument.new(doc, solr_response) }
 
@@ -591,7 +598,7 @@ def solr_opensearch_params(field=nil)
   # where the field is the "field" argument passed in.
   def get_opensearch_response(field=nil, extra_controller_params={})
     solr_params = solr_opensearch_params().merge(extra_controller_params)
-    response = find(blacklight_config.qt, solr_params)
+    response = find(solr_params)
     a = [solr_params[:q]]
     a << response.docs.map {|doc| doc[solr_params[:fl]].to_s }
   end

spec/controllers/catalog_controller_spec.rb

@@ -509,10 +509,7 @@ def export_as_mock
       controller.stub(:has_user_authentication_provider?) { false }
         @mock_response = double()
         @mock_document = double()
-        @mock_response.stub(:docs => [], :total => 1, :facets => [], :facet_queries => {}, :facet_by_field_name => nil)
-        @mock_document = double()
-        controller.stub(:find => @mock_response, 
-                        :get_single_doc_via_search => @mock_document)
+        controller.stub(:get_single_doc_via_search => @mock_document)
     end
 
     it "should not show user util links" do

spec/lib/blacklight/solr_helper_spec.rb

@@ -40,7 +40,77 @@ def blacklight_config=(config)
     @subject_search_params = {:commit=>"search", :search_field=>"subject", :action=>"index", :"controller"=>"catalog", :"rows"=>"10", :"q"=>"wome"}
   end
 
+  describe "#find" do
+    let(:blacklight_config) { Blacklight::Configuration.new }
+    let(:response) { double }
+    let(:blacklight_solr) { double }
+
+    before do
+      double(blacklight_solr: blacklight_solr)
+    end
+
+    it "should use the configured solr path" do
+      blacklight_config.solr_path = 'xyz'
+      blacklight_solr.should_receive(:send_and_receive).with('xyz', anything).and_return("{}".to_json)
+      expect(find({})).to be_a_kind_of Blacklight::SolrResponse
+    end
+
+    it "should override the configured solr path" do
+      blacklight_config.solr_path = 'xyz'
+      blacklight_solr.should_receive(:send_and_receive).with('abc', anything).and_return("{}".to_json)
+      expect(find('abc', {})).to be_a_kind_of Blacklight::SolrResponse
+    end
+
+    it "should use a default :qt param" do
+      blacklight_config.qt = 'xyz'
+      blacklight_solr.should_receive(:send_and_receive).with('select', hash_including(params: { qt: 'xyz'})).and_return("{}".to_json)
+      expect(find({})).to be_a_kind_of Blacklight::SolrResponse
+    end
+
+    it "should use the provided :qt param" do
+      blacklight_config.qt = 'xyz'
+      blacklight_solr.should_receive(:send_and_receive).with('select', hash_including(params: { qt: 'abc'})).and_return("{}".to_json)
+      expect(find({qt: 'abc'})).to be_a_kind_of Blacklight::SolrResponse
+    end
+
+    describe "http_method configuration" do
+      describe "using default" do
+
+        it "defaults to get" do
+          expect(blacklight_config.http_method).to eq :get
+          blacklight_solr.should_receive(:send_and_receive) do |path, params|
+            expect(path).to eq 'select'
+            expect(params[:method]).to eq :get
+            expect(params[:params]).to include(:q)
+          end.and_return({'response'=>{'docs'=>[]}})
+          find(:q => @all_docs_query)
+        end
+      end
+
+      describe "setting to post" do
+        let (:blacklight_config) {config = Blacklight::Configuration.new; config.http_method=:post; config}
+
+        it "keep value set to post" do
+          expect(blacklight_config.http_method).to eq :post
+          blacklight_solr.should_receive(:send_and_receive) do |path, params|
+            expect(path).to eq 'select'
+            expect(params[:method]).to eq :post
+            expect(params[:data]).to include(:q)
+          end.and_return({'response'=>{'docs'=>[]}})
+          find(:q => @all_docs_query)
+        end
+      end
+    end
+  end
 
+  describe "http_method configuration", :integration => true do
+    let (:blacklight_config) {config = Blacklight::Configuration.new; config.http_method=:post; config}
+
+    it "should send a post request to solr and get a response back" do
+      response = find(:q => @all_docs_query)
+      expect(response.docs.length).to be >= 1
+    end
+  end
 
   # SPECS for actual search parameter generation
   describe "solr_search_params" do
@@ -954,7 +1024,7 @@ def grouped_key_for_results
 
     it "should use a provided document request handler " do
       blacklight_config.stub(:document_solr_request_handler => 'document')
-      blacklight_solr.should_receive(:get).with('select', kind_of(Hash)).and_return({'response'=>{'docs'=>[]}})
+      blacklight_solr.should_receive(:send_and_receive).with('document', kind_of(Hash)).and_return({'response'=>{'docs'=>[]}})
       expect { get_solr_response_for_doc_id(@doc_id)}.to raise_error Blacklight::Exceptions::InvalidSolrID
     end
 
@@ -1159,17 +1229,17 @@ def blacklight_config
         @mock_response.stub(:docs => [])
       end
       it "should contruct a solr query based on the field and value pair" do
-        self.should_receive(:find).with(an_instance_of(String), hash_including(:q => "field_name:(value)")).and_return(@mock_response)
+        self.should_receive(:find).with(hash_including(:q => "field_name:(value)")).and_return(@mock_response)
         get_solr_response_for_field_values('field_name', 'value')
       end
 
       it "should OR multiple values together" do
-        self.should_receive(:find).with(an_instance_of(String), hash_including(:q => "field_name:(a OR b)")).and_return(@mock_response)
+        self.should_receive(:find).with(hash_including(:q => "field_name:(a OR b)")).and_return(@mock_response)
         get_solr_response_for_field_values('field_name', ['a', 'b'])
       end
 
       it "should escape crazy identifiers" do
-        self.should_receive(:find).with(an_instance_of(String), hash_including(:q => "field_name:(\"h://\\\"\\\'\")")).and_return(@mock_response)
+        self.should_receive(:find).with(hash_including(:q => "field_name:(\"h://\\\"\\\'\")")).and_return(@mock_response)
         get_solr_response_for_field_values('field_name', 'h://"\'')
       end
     end
@@ -1182,7 +1252,7 @@ def blacklight_config
 #  more like this
 #  nearby on shelf
   it "should raise a Blacklight exception if RSolr can't connect to the Solr instance" do
-    blacklight_solr.stub(:get).and_raise(Errno::ECONNREFUSED)
+    blacklight_solr.stub(:send_and_receive).and_raise(Errno::ECONNREFUSED)
     expect { find(:a => 123) }.to raise_exception(/Unable to connect to Solr instance/)
   end
 
@@ -1241,41 +1311,5 @@ def blacklight_config
       expect(docs.first).to be_nil
     end
   end
-
-  describe "http_method configuration" do
-    describe "using default" do
-      let (:blacklight_config) {Blacklight::Configuration.new}
-
-      it "defaults to get" do
-        expect(blacklight_config.http_method).to eq :get
-        blacklight_solr.should_receive(:send_and_receive) do |path, params|
-          expect(path).to eq 'select'
-          expect(params[:method]).to eq :get
-          expect(params[:params]).to include(:q)
-        end.and_return({'response'=>{'docs'=>[]}})
-        get_search_results(:q => @all_docs_query)
-      end
-    end
-
-    describe "setting to post" do
-      let (:blacklight_config) {config = Blacklight::Configuration.new; config.http_method=:post; config}
-
-      it "keep value set to post" do
-        expect(blacklight_config.http_method).to eq :post
-        blacklight_solr.should_receive(:send_and_receive) do |path, params|
-          expect(path).to eq 'select'
-          expect(params[:method]).to eq :post
-          expect(params[:data]).to include(:q)
-        end.and_return({'response'=>{'docs'=>[]}})
-        get_search_results(:q => @all_docs_query)
-      end
-
-      it "should send a post request to solr", :integration => true do
-        response, docs = get_search_results(:q => @all_docs_query)
-        expect(docs.length).to be >= 1
-     end
-    end
-  end
-
 end