Skip to content

Commit

Permalink
Ignore empty f[facet][] query param
Browse files Browse the repository at this point in the history
Previously this would result in a limit being applied
for a facet with an empty string value. I don't think this
is a real use case (it would be somewhat tricky to index
values in Solr this way even if you wanted to), and I
don't think this was intentional.

Seems better to ignore empty facet limit query params
like that, have them be no-ops that do not effect
the Solr query or the visible constraints. That's
what this does.

empty facet limit query params of course shouldn't happen
at all, and are probably mistakes, but could come from:
* a bug in some part of BL or plugin
* a bug in some external "deep link generating" tool
* user error manipulating the URL manually

I suggest it's better to have such a mistake be a no-op, then
to have it result in odd and disconcerting behavior as previous.
  • Loading branch information
jrochkind committed May 29, 2014
1 parent df3e5ba commit a97b1ad
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 0 deletions.
Expand Up @@ -76,6 +76,7 @@ def render_filter_element(facet, values, localized_params)
facet_config = facet_configuration_for_field(facet)

safe_join(values.map do |val|
next if val.blank? # skip empty string
render_constraint_element( blacklight_config.facet_fields[facet].label,
facet_display_value(facet, val),
:remove => search_action_path(remove_facet_params(facet, val, localized_params)),
Expand Down
1 change: 1 addition & 0 deletions lib/blacklight/request_builders.rb
Expand Up @@ -121,6 +121,7 @@ def add_facet_fq_to_solr(solr_parameters, user_params)

f_request_params.each_pair do |facet_field, value_list|
Array(value_list).each do |value|
next if value.blank? # skip empty strings
solr_parameters.append_filter_query facet_value_to_fq_string(facet_field, value)
end
end
Expand Down
14 changes: 14 additions & 0 deletions spec/helpers/render_constraints_helper_spec.rb
Expand Up @@ -29,4 +29,18 @@
end
end

describe "#render_constraints_filters" do
before do
@config = Blacklight::Configuration.new do |config|
config.add_facet_field 'type'
end
helper.stub(:blacklight_config => @config)
end

it "should render nothing for empty facet limit param" do
rendered = helper.render_constraints_filters(:f=>{'type'=>['']})
expect(rendered).to be_blank
end
end

end
8 changes: 8 additions & 0 deletions spec/lib/blacklight/solr_helper_spec.rb
Expand Up @@ -191,6 +191,14 @@ def logger
end
end

describe "for an empty facet limit param" do
it "should not add any fq to solr" do
solr_params = subject.solr_search_params(:f => {"format" => [""]})

expect(solr_params[:fq]).to be_blank
end
end

describe "with Multi Facets, No Query" do
it 'should have fq set properly' do
solr_params = subject.solr_search_params(:f => @multi_facets)
Expand Down

0 comments on commit a97b1ad

Please sign in to comment.