Skip to content

Commit

Permalink
[#3512] Support facebook-style query params in cases where a facet pi…
Browse files Browse the repository at this point in the history
…vot is configured

each_with_object ignores the return value of the supplied block; you need to mutate
the passed object instead.

Prior to this change, the ramifications of bug are that:
* FilterField#permitted_params always includes empty hashes (the original, unmutated
  object we passed to each_with_object) for a facet pivot field.
* When Blacklight::Parameters::deep_merge_permitted_params comes across empty hashes,
  it discards a present hash in favor of the empty one.
* As the permitted params for all facet fields are merged together, the presence of
  even one facet pivot field leads us to discard all other permitted facet field keys.
* We have empty hashes, rather than hashes with permitted facet keys going into
  Blacklight::Parameters#deep_unmangle_params!
* Blacklight::Parameters#deep_unmangle_params! can find no permitted parameters that
  it needs to unmangle, so the facebook params never get unmangled, causing errors
  and unexpected behavior elsewhere in the stack.
* Facebook-style query params don't work if a facet pivot is configured.
  • Loading branch information
sandbergja committed Feb 20, 2025
1 parent cf4500c commit 3f84146
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 4 deletions.
4 changes: 2 additions & 2 deletions lib/blacklight/search_state/filter_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ def include?(item)
def permitted_params
if config.pivot
{
filters_key => config.pivot.each_with_object({}) { |key, filter| filter.merge(key => [], "-#{key}" => []) },
inclusive_filters_key => config.pivot.each_with_object({}) { |key, filter| filter.merge(key => []) }
filters_key => config.pivot.each_with_object({}) { |key, filter| filter.merge!(key => [], "-#{key}" => []) },
inclusive_filters_key => config.pivot.each_with_object({}) { |key, filter| filter.merge!(key => []) }
}
else
{
Expand Down
7 changes: 7 additions & 0 deletions spec/features/facets_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -143,4 +143,11 @@
end
end
end

describe 'facebook-style facet parameters' do
it 'can perform a search' do
visit '/?f[subject_ssim][0]=Iran.+Viza%CC%84rat-i+Kishvar'
expect(page).to have_text 'Naqdī barā-yi tamām-i fuṣūl'
end
end
end
13 changes: 12 additions & 1 deletion spec/lib/blacklight/parameters_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
let(:search_state) { Blacklight::SearchState.new(query_params, blacklight_config) }
let(:blacklight_config) { Blacklight::Configuration.new }

context 'with facebooks badly mangled query parameters' do
context 'with facebook\'s badly mangled query parameters' do
let(:query_params) do
ActionController::Parameters.new(
f: { field: { '0': 'first', '1': 'second' } },
Expand All @@ -67,6 +67,17 @@
it 'normalizes the facets to the expected format' do
expect(params.permit_search_params.to_h.with_indifferent_access).to include f: { field: %w[first second] }, f_inclusive: { field: %w[first second] }
end

context 'when several fields are configured' do
before do
blacklight_config.add_facet_field 'other_field'
blacklight_config.add_facet_field 'some_other_pivot_field', pivot: %w[abc def]
end

it 'normalizes the facets to the expected format' do
expect(params.permit_search_params.to_h.with_indifferent_access).to include f: { field: %w[first second] }, f_inclusive: { field: %w[first second] }
end
end
end

context 'with filter_search_state_fields set to false' do
Expand Down
18 changes: 18 additions & 0 deletions spec/lib/blacklight/search_state/filter_field_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -198,4 +198,22 @@
expect(search_state.filter('some_field').include?(OpenStruct.new(value: '1'))).to be true
end
end

describe '#permitted_params' do
context 'with a pivot facet' do
let(:blacklight_config) do
Blacklight::Configuration.new.configure do |config|
config.add_facet_field 'my_pivot', pivot: %w[format language_ssim]
end
end

it 'marks all the pivot fields as permitted' do
field = described_class.new blacklight_config.facet_fields['my_pivot'], search_state
expect(field.permitted_params).to eq({
f: { "-format" => [], "-language_ssim" => [], "format" => [], "language_ssim" => [] },
f_inclusive: { "format" => [], "language_ssim" => [] }
})
end
end
end
end
2 changes: 1 addition & 1 deletion spec/services/blacklight/search_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
allow(blacklight_config).to receive(:default_solr_params).and_return(qt: 'custom_request_handler')
allow(blacklight_solr).to receive(:send_and_receive) do |path, params|
expect(path).to eq 'select'
expect(params[:params]['facet.field']).to eq ["format", "{!ex=pub_date_ssim_single}pub_date_ssim", "subject_ssim", "language_ssim", "lc_1letter_ssim", "subject_geo_ssim", "subject_era_ssim"]
expect(params[:params]['facet.field']).to contain_exactly "format", "{!ex=pub_date_ssim_single}pub_date_ssim", "subject_ssim", "language_ssim", "lc_1letter_ssim", "subject_geo_ssim", "subject_era_ssim"
expect(params[:params]["facet.query"]).to eq ["pub_date_ssim:[#{5.years.ago.year} TO *]", "pub_date_ssim:[#{10.years.ago.year} TO *]", "pub_date_ssim:[#{25.years.ago.year} TO *]"]
expect(params[:params]).to include('rows' => 10, 'qt' => "custom_request_handler", 'q' => "", "f.subject_ssim.facet.limit" => 21, 'sort' => "score desc, pub_date_si desc, title_si asc")
end.and_return('response' => { 'docs' => [] })
Expand Down

0 comments on commit 3f84146

Please sign in to comment.