Skip to content

Commit

Permalink
Improve performance of the solr query used to generate the advanced s…
Browse files Browse the repository at this point in the history
…earch 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

Since the logic is different on the numismatics form vs advanced form, this commit
splits them into two separate search builders.
  • Loading branch information
sandbergja committed Jan 1, 2025
1 parent f4cfae3 commit e7875bd
Show file tree
Hide file tree
Showing 5 changed files with 70 additions and 2 deletions.
4 changes: 3 additions & 1 deletion app/controllers/catalog_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -841,8 +841,10 @@ def basic_response
end

def search_service_context
if %w[advanced_search numismatics].include? action_name
if action_name == 'advanced_search'
{ search_builder_class: AdvancedFormSearchBuilder }
elsif action_name == 'numismatics'
{ search_builder_class: NumismaticsFormSearchBuilder }
else
return {} unless Flipflop.multi_algorithm?
return {} unless configurable_search_builder_class # use default if none specified
Expand Down
7 changes: 6 additions & 1 deletion app/models/advanced_form_search_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand All @@ -13,4 +13,9 @@ def do_not_limit_languages(solr_params)
end
end
end

def only_request_advanced_facets(solr_params)
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
2 changes: 2 additions & 0 deletions app/models/numismatics_form_search_builder.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
class NumismaticsFormSearchBuilder < SearchBuilder
end
42 changes: 42 additions & 0 deletions spec/models/advanced_form_search_builder_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,46 @@
end.not_to(change { solr_params })
end
end

describe '#only_request_advanced_facets' do
let(:blacklight_config) { CatalogController.blacklight_config }

it 'removes unnecessary facet.field entries' 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' 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' 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
17 changes: 17 additions & 0 deletions spec/requests/advanced_search_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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

0 comments on commit e7875bd

Please sign in to comment.