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
  • Loading branch information
sandbergja committed Jan 1, 2025
1 parent f4cfae3 commit e28f043
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 1 deletion.
4 changes: 4 additions & 0 deletions app/controllers/catalog_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
9 changes: 8 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,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
50 changes: 50 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,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
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 e28f043

Please sign in to comment.