diff --git a/lib/blacklight/solr/request.rb b/lib/blacklight/solr/request.rb index 2b391c2fd2..627299f1c7 100644 --- a/lib/blacklight/solr/request.rb +++ b/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 diff --git a/lib/blacklight/solr_helper.rb b/lib/blacklight/solr_helper.rb index b31272511c..b71eaa6da8 100644 --- a/lib/blacklight/solr_helper.rb +++ b/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 diff --git a/spec/controllers/catalog_controller_spec.rb b/spec/controllers/catalog_controller_spec.rb index 754d9c0d41..acfedbfe5d 100644 --- a/spec/controllers/catalog_controller_spec.rb +++ b/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 diff --git a/spec/lib/blacklight/solr_helper_spec.rb b/spec/lib/blacklight/solr_helper_spec.rb index 6587d48d5a..789821f620 100644 --- a/spec/lib/blacklight/solr_helper_spec.rb +++ b/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