From e28f043b025a72d6726d5fe8cae581121b68c84e Mon Sep 17 00:00:00 2001 From: Jane Sandberg Date: Tue, 31 Dec 2024 15:21:26 -0800 Subject: [PATCH] Improve performance of the solr query used to generate the advanced search form Running this locally against the production solr sped the solr query up from a median 10,691 ms to a median 1,851 ms. This removes several potentially expensive parameters from the solr query we send to generate the advanced search page: * facet.query * facet.pivot * stats * stats.field * facet.field, except for the facet field values we need to retrieve to render the advanced search form --- app/controllers/catalog_controller.rb | 4 ++ app/models/advanced_form_search_builder.rb | 9 +++- .../advanced_form_search_builder_spec.rb | 50 +++++++++++++++++++ spec/requests/advanced_search_spec.rb | 17 +++++++ 4 files changed, 79 insertions(+), 1 deletion(-) diff --git a/app/controllers/catalog_controller.rb b/app/controllers/catalog_controller.rb index bf482fc2c..83ece8cf1 100644 --- a/app/controllers/catalog_controller.rb +++ b/app/controllers/catalog_controller.rb @@ -874,4 +874,8 @@ def search_service_compatibility_wrapper def solrize_boolean_params @search_state = search_state.reset(AdvancedBooleanOperators.new(search_state.params).for_boolqparser) end + + def blacklight_advanced_search_form_params + params.select { |key, _value| key == 'action' } + end end diff --git a/app/models/advanced_form_search_builder.rb b/app/models/advanced_form_search_builder.rb index adb7f5bb2..e2f1f8ffc 100644 --- a/app/models/advanced_form_search_builder.rb +++ b/app/models/advanced_form_search_builder.rb @@ -2,7 +2,7 @@ # This class is responsible for building a solr query # that renders an advanced search form class AdvancedFormSearchBuilder < SearchBuilder - self.default_processor_chain += %i[do_not_limit_languages] + self.default_processor_chain += %i[do_not_limit_languages only_request_advanced_facets] def do_not_limit_languages(solr_params) solr_params.update(solr_params) do |key, value| @@ -13,4 +13,11 @@ def do_not_limit_languages(solr_params) end end end + + def only_request_advanced_facets(solr_params) + return unless blacklight_params[:action] == 'advanced_search' + + solr_params['facet.field'] = blacklight_config.facet_fields.values.select(&:include_in_advanced_search).map(&:key) + %w[facet.pivot facet.query stats stats.field].each { |unneeded_field| solr_params.delete unneeded_field } + end end diff --git a/spec/models/advanced_form_search_builder_spec.rb b/spec/models/advanced_form_search_builder_spec.rb index 62c2d28b0..14bd8ffbd 100644 --- a/spec/models/advanced_form_search_builder_spec.rb +++ b/spec/models/advanced_form_search_builder_spec.rb @@ -29,4 +29,54 @@ end.not_to(change { solr_params }) end end + + describe '#only_request_advanced_facets' do + let(:blacklight_config) { CatalogController.blacklight_config } + + it 'does not modify anything if the action is numismatics' do + builder.with({ action: 'numismatics' }) + solr_params = { "facet.field" => %w[issue_metal_s issue_city_s] } + expect do + builder.only_request_advanced_facets(solr_params) + end.not_to(change { solr_params }) + end + + it 'removes unnecessary facet.field entries if the action is advanced_search' do + builder.with({ action: 'advanced_search' }) + solr_params = { "facet.field" => %w[genre_facet subject_era_facet geographic_facet] } + + builder.only_request_advanced_facets(solr_params) + + expect(solr_params['facet.field']).not_to include 'genre_facet' + expect(solr_params['facet.field']).not_to include 'subject_era_facet' + expect(solr_params['facet.field']).not_to include 'geographic_facet' + end + + it 'adds the needed facet.field entries if the action is advanced_search' do + builder.with({ action: 'advanced_search' }) + solr_params = { "facet.field" => %w[genre_facet subject_era_facet geographic_facet] } + + builder.only_request_advanced_facets(solr_params) + + expect(solr_params['facet.field']).to include 'language_facet' + end + + it 'removes unneeded params if the action is advanced_search' do + builder.with({ action: 'advanced_search' }) + solr_params = { + "facet.pivot" => %w[lc_1letter_facet lc_rest_facet], + "facet.query" => ['cataloged_tdt:[NOW/DAY-7DAYS TO NOW/DAY+1DAY]', 'cataloged_tdt:[NOW/DAY-14DAYS TO NOW/DAY+1DAY]'], + "stats" => true, + "stats.field" => "pub_date_start_sort" + } + + builder.only_request_advanced_facets(solr_params) + + expect(solr_params.keys).not_to include 'facet.query' + expect(solr_params.keys).not_to include 'facet.pivot' + expect(solr_params.keys).not_to include 'stats' + expect(solr_params.keys).not_to include 'stats.field' + expect(solr_params.keys).to include 'facet.field' + end + end end diff --git a/spec/requests/advanced_search_spec.rb b/spec/requests/advanced_search_spec.rb index 44cd55455..1cbabd976 100644 --- a/spec/requests/advanced_search_spec.rb +++ b/spec/requests/advanced_search_spec.rb @@ -2,6 +2,12 @@ require 'rails_helper' +RSpec::Matchers.define :request_without_facet_queries do + match do |actual| + actual[:params].keys.exclude? 'facet.query' + end +end + describe 'Orangelight advanced search', type: :request, advanced_search: true do before do stub_holding_locations @@ -11,4 +17,15 @@ get '/advanced?f[subject_facet][]=United+Nations-Decision+making' expect(response.status).to eq(200) end + + it 'does not send complex facet queries to solr when rendering advanced search form' do + # rubocop:disable RSpec/AnyInstance + expect_any_instance_of(RSolr::Client).to receive(:send_and_receive) + .with('select', request_without_facet_queries) + .once + .and_call_original + # rubocop:enable RSpec/AnyInstance + + get '/advanced' + end end