Backport #2597 (Handle RSolr timeouts with its own class)

lib/blacklight/exceptions.rb

@@ -15,6 +15,9 @@ class ExpiredSessionToken < StandardError
 
     class ECONNREFUSED < ::Errno::ECONNREFUSED; end
 
+    # NOTE: In Blacklight 8, the parent class will be Timeout::Error
+    class RepositoryTimeout < InvalidRequest; end
+
     class IconNotFound < StandardError
     end
   end

lib/blacklight/solr/repository.rb

@@ -58,30 +58,40 @@ def ping
     # @return [Blacklight::Solr::Response] the solr response object
     def send_and_receive(path, solr_params = {})
       benchmark("Solr fetch", level: :debug) do
-        res = if solr_params[:json].present?
-                connection.send_and_receive(
-                  path,
-                  data: { params: solr_params.to_hash.except(:json) }.merge(solr_params[:json]).to_json,
-                  method: :post,
-                  headers: { 'Content-Type' => 'application/json' }
-                )
-              else
-                key = blacklight_config.http_method == :post ? :data : :params
-                connection.send_and_receive(path, { key => solr_params.to_hash, method: blacklight_config.http_method })
-              end
-
+        res = connection.send_and_receive(path, build_solr_request(solr_params))
         solr_response = blacklight_config.response_model.new(res, solr_params, document_model: blacklight_config.document_model, blacklight_config: blacklight_config)
 
         Blacklight.logger&.debug("Solr query: #{blacklight_config.http_method} #{path} #{solr_params.to_hash.inspect}")
         Blacklight.logger&.debug("Solr response: #{solr_response.inspect}") if defined?(::BLACKLIGHT_VERBOSE_LOGGING) && ::BLACKLIGHT_VERBOSE_LOGGING
         solr_response
       end
+    rescue *defined_rsolr_timeout_exceptions => e
+      raise Blacklight::Exceptions::RepositoryTimeout, "Timeout connecting to Solr instance using #{connection.inspect}: #{e.inspect}"
     rescue Errno::ECONNREFUSED => e
+      # intended for and likely to be a RSolr::Error:ConnectionRefused, specifically.
       raise Blacklight::Exceptions::ECONNREFUSED, "Unable to connect to Solr instance using #{connection.inspect}: #{e.inspect}"
     rescue RSolr::Error::Http => e
       raise Blacklight::Exceptions::InvalidRequest, e.message
     end
 
+    # @return [Hash]
+    # @!visibility private
+    def build_solr_request(solr_params)
+      if solr_params[:json].present?
+        {
+          data: { params: solr_params.to_hash.except(:json) }.merge(solr_params[:json]).to_json,
+          method: :post,
+          headers: { 'Content-Type' => 'application/json' }
+        }
+      else
+        key = blacklight_config.http_method == :post ? :data : :params
+        {
+          key => solr_params.to_hash,
+          method: blacklight_config.http_method
+        }
+      end
+    end
+
     private
 
     ##
@@ -97,5 +107,19 @@ def suggester_name
     def build_connection
       RSolr.connect(connection_config.merge(adapter: connection_config[:http_adapter]))
     end
+
+    # RSolr 2.4.0+ has a RSolr::Error::Timeout that we'd like to treat specially
+    # instead of lumping into RSolr::Error::Http. Before that we can not rescue
+    # specially, so return an empty array.
+    #
+    # @return [Array<Exception>] that can be used, with a splat, as argument
+    #   to a ruby rescue
+    def defined_rsolr_timeout_exceptions
+      if defined?(RSolr::Error::Timeout)
+        [RSolr::Error::Timeout]
+      else
+        []
+      end
+    end
   end
 end

spec/models/blacklight/solr/repository_spec.rb

@@ -9,6 +9,8 @@
     CatalogController.blacklight_config.deep_copy
   end
 
+  let(:all_docs_query) { '' }
+
   let :mock_response do
     { response: { docs: [document] } }
   end
@@ -104,19 +106,30 @@
       expect(response).to be_a_kind_of Blacklight::Solr::Response
       expect(response.params).to be_a_kind_of ActiveSupport::HashWithIndifferentAccess
     end
+
+    it "calls send_and_receive with params returned from request factory method" do
+      expect(blacklight_config.http_method).to eq :get
+      input_params = { q: all_docs_query }
+      allow(subject.connection).to receive(:send_and_receive) do |path, params|
+        expect(path).to eq 'select'
+        expect(params[:method]).to eq :get
+        expect(params[:params]).to include input_params
+      end.and_return('response' => { 'docs' => [] })
+      subject.search(input_params)
+    end
   end
 
-  describe "#send_and_receive" do
+  describe "#build_solr_request" do
+    let(:input_params) { { q: all_docs_query } }
+    let(:actual_params) { subject.build_solr_request(input_params) }
+
     describe "http_method configuration" do
       describe "using default" do
         it "defaults to get" do
           expect(blacklight_config.http_method).to eq :get
-          allow(subject.connection).to 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' => [] })
-          subject.search(q: @all_docs_query)
+          expect(actual_params[:method]).to eq :get
+          expect(actual_params[:params]).to include input_params
+          expect(actual_params).not_to have_key :data
         end
       end
 
@@ -125,25 +138,20 @@
 
         it "keep value set to post" do
           expect(blacklight_config.http_method).to eq :post
-          allow(subject.connection).to 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' => [] })
-          subject.search(q: @all_docs_query)
+          expect(actual_params[:method]).to eq :post
+          expect(actual_params[:data]).to include input_params
+          expect(actual_params).not_to have_key :params
         end
       end
     end
 
     context 'with json parameters' do
+      let(:input_params) { { json: { query: { bool: {} } } } }
+
       it 'sends a post request with some json' do
-        allow(subject.connection).to receive(:send_and_receive) do |path, params|
-          expect(path).to eq 'select'
-          expect(params[:method]).to eq :post
-          expect(JSON.parse(params[:data]).with_indifferent_access).to include(query: { bool: {} })
-          expect(params[:headers]).to include({ 'Content-Type' => 'application/json' })
-        end.and_return('response' => { 'docs' => [] })
-        subject.search(json: { query: { bool: {} } })
+        expect(actual_params[:method]).to eq :post
+        expect(JSON.parse(actual_params[:data]).with_indifferent_access).to include(query: { bool: {} })
+        expect(actual_params[:headers]).to include({ 'Content-Type' => 'application/json' })
       end
     end
   end
@@ -152,7 +160,7 @@
     let (:blacklight_config) { config = Blacklight::Configuration.new; config.http_method = :post; config }
 
     it "sends a post request to solr and get a response back" do
-      response = subject.search(q: @all_docs_query)
+      response = subject.search(q: all_docs_query)
       expect(response.docs.length).to be >= 1
     end
   end

spec/services/blacklight/search_service_spec.rb

@@ -421,6 +421,14 @@
     expect { service.repository.search }.to raise_exception(/Unable to connect to Solr instance/)
   end
 
+  it "raises a Blacklight exception if RSolr raises a timeout error connecting to Solr instance" do
+    rsolr_timeout = RSolr::Error::Timeout.new(nil, nil)
+    allow(rsolr_timeout).to receive(:to_s).and_return("mocked RSolr timeout")
+
+    allow(blacklight_solr).to receive(:send_and_receive).and_raise(rsolr_timeout)
+    expect { service.repository.search }.to raise_exception(Blacklight::Exceptions::RepositoryTimeout, /Timeout connecting to Solr instance/)
+  end
+
   describe "#previous_and_next_documents_for_search" do
     let(:user_params) { { q: '', per_page: 100 } }