Merge pull request #4628 from pulibrary/move-excessive-paging

Move excessive paging out of search builder into controllers

app/controllers/catalog_controller.rb

@@ -7,9 +7,11 @@ class CatalogController < ApplicationController
   include BlacklightRangeLimit::ControllerOverride
   include Orangelight::Catalog
   include Orangelight::Stackmap
+  include Orangelight::ExcessivePaging
   include BlacklightHelper
 
   before_action :redirect_browse
+  before_action :deny_excessive_paging
 
   rescue_from Blacklight::Exceptions::RecordNotFound do
     alma_id = "99#{params[:id]}3506421"

app/controllers/concerns/orangelight/excessive_paging.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+module Orangelight
+  module ExcessivePaging
+    private
+
+      def deny_excessive_paging
+        page = params[:page].to_i || 0
+        return if page <= 1
+        return if (has_search_parameters? || advanced_search? || bookmarks_page?) && page < 1000
+        render plain: "excessive paging", status: :bad_request
+      end
+
+      def advanced_search?
+        %w[advanced numismatics].include?(params[:advanced_type]) ||
+          params[:search_field] == 'advanced' ||
+          %w[advanced numismatics].include?(action_name)
+      end
+
+      def bookmarks_page?
+        instance_of?(BookmarksController)
+      end
+  end
+end

app/models/search_builder.rb

@@ -8,7 +8,7 @@ class SearchBuilder < Blacklight::SearchBuilder
   default_processor_chain.unshift(:conditionally_configure_json_query_dsl)
 
   self.default_processor_chain += %i[parslet_trick cleanup_boolean_operators
-                                     cjk_mm wildcard_char_strip excessive_paging_error
+                                     cjk_mm wildcard_char_strip
                                      only_home_facets prepare_left_anchor_search
                                      series_title_results pul_holdings html_facets
                                      numismatics_facets numismatics_advanced
@@ -73,22 +73,6 @@ def only_home_facets(solr_parameters)
     solr_parameters['facet.pivot'] = []
   end
 
-  # Determines whether or not the user is requesting an excessively high page of results
-  # @param [ActionController::Parameters] params
-  # @return [Boolean]
-  def excessive_paging_error(_solr_parameters)
-    raise ActionController::BadRequest, "excessive paging" if excessive_paging?
-  end
-
-  # Determines whether or not the user is requesting an excessively high page of results
-  # @return [Boolean]
-  def excessive_paging?
-    page = blacklight_params[:page].to_i || 0
-    return false if page <= 1
-    return false if (search_parameters? || advanced_search? || bookmarks_page?) && page < 1000
-    true
-  end
-
   ##
   # Check if we are on an advanced search page
   # @return [Boolean]
@@ -153,8 +137,4 @@ def add_edismax(advanced_fields:)
         blacklight_config.search_fields[field]['clause_params'] = { edismax: }
       end
     end
-
-    def bookmarks_page?
-      blacklight_params[:controller] == 'bookmarks'
-    end
 end

spec/controllers/bookmarks_controller_spec.rb

@@ -48,5 +48,9 @@
       get :index, params: { page: 2 }
       expect(response).to have_http_status(:ok)
     end
+    it 'errors when paging is excessive' do
+      get :index, params: { page: 1500 }
+      expect(response).to have_http_status(:bad_request)
+    end
   end
 end

spec/controllers/catalog_controller_spec.rb

@@ -82,10 +82,26 @@
       get :index, params: { q: 'asdf', page: 2 }
       expect(response.status).to eq(200)
     end
+    it 'does not error when paging is reasonable and the search is a facet' do
+      get :index, params: { f: { format: 'anything' }, page: 2 }
+      expect(response.status).to eq(200)
+    end
+    it 'does not error when query is empty and paging is reasonable' do
+      get :index, params: { q: '', search_field: 'all_fields', page: 2 }
+      expect(response.status).to eq(200)
+    end
+    it 'does not error when paging is reasonable and query is from advanced search' do
+      get :index, params: { clause: { '0': { query: 'dog' } }, page: 2 }
+      expect(response.status).to eq(200)
+    end
     it 'errors when paging is excessive' do
       get :index, params: { q: 'asdf', page: 1500 }
       expect(response.status).to eq(400)
     end
+    it 'errors when paging is reasonable but there is no query or facet' do
+      get :index, params: { page: 3 }
+      expect(response.status).to eq(400)
+    end
   end
 
   describe 'home page' do

spec/models/search_builder_spec.rb

@@ -9,64 +9,6 @@
   let(:scope) { Blacklight::SearchService.new config: blacklight_config, search_state: state }
   let(:state) { Blacklight::SearchState.new({}, blacklight_config) }
 
-  describe '#excessive_paging' do
-    let(:excessive) { 9999 }
-    let(:reasonable) { 123 }
-
-    it 'allows reasonable paging with a search query' do
-      search_builder.blacklight_params[:page] = reasonable
-      search_builder.blacklight_params[:q] = 'anything'
-      expect(search_builder.excessive_paging?).to be false
-    end
-
-    it 'allows reasonable paging with a facet query' do
-      search_builder.blacklight_params[:page] = reasonable
-      search_builder.blacklight_params[:f] = 'anything'
-      expect(search_builder.excessive_paging?).to be false
-    end
-
-    it 'returns false if the search is empty' do
-      search_builder.blacklight_params[:page] = reasonable
-      search_builder.blacklight_params[:q] = ''
-      expect(search_builder.excessive_paging?).to be false
-    end
-
-    it 'does not allow paging without a search or facet' do
-      search_builder.blacklight_params[:page] = reasonable
-      expect(search_builder.excessive_paging?).to be true
-    end
-
-    it 'does not allow excessive paging with a search query' do
-      search_builder.blacklight_params[:page] = excessive
-      search_builder.blacklight_params[:q] = 'anything'
-      expect(search_builder.excessive_paging?).to be true
-    end
-
-    it 'does not allow excessive paging with a facet query' do
-      search_builder.blacklight_params[:page] = excessive
-      search_builder.blacklight_params[:f] = 'anything'
-      expect(search_builder.excessive_paging?).to be true
-    end
-
-    it 'allows paging for advanced search' do
-      search_builder.blacklight_params[:page] = reasonable
-      search_builder.blacklight_params[:search_field] = 'advanced'
-      expect(search_builder.excessive_paging?).to be false
-    end
-
-    it 'allows paging for bookmarks controller' do
-      search_builder.blacklight_params[:page] = reasonable
-      search_builder.blacklight_params[:controller] = 'bookmarks'
-      expect(search_builder.excessive_paging?).to be false
-    end
-
-    it 'handles query ending with empty parenthesis' do
-      search_builder.blacklight_params[:q] = 'hello world ()'
-      search_builder.parslet_trick({})
-      expect(search_builder.blacklight_params[:q].end_with?("()")).to be false
-    end
-  end
-
   describe '#cleanup_boolean_operators' do
     let(:solr_parameters) do
       { q: 'Douglas fir' }