Merge pull request #2687 from barmintor/7.x-delegate-normalization-to…

…-filters

7.x delegate normalization to filters

app/helpers/blacklight/render_constraints_helper_behavior.rb

@@ -84,14 +84,12 @@ def render_constraints_filters(params_or_search_state = search_state)
     Deprecation.warn(Blacklight::RenderConstraintsHelperBehavior, 'render_constraints_filters is deprecated')
     search_state = convert_to_search_state(params_or_search_state)
 
-    Deprecation.silence(Blacklight::SearchState) do
-      return "".html_safe if search_state.filter_params.blank?
+    return "".html_safe unless search_state.filters.any?
 
-      Deprecation.silence(Blacklight::RenderConstraintsHelperBehavior) do
-        safe_join(search_state.filters.map do |field|
-          render_filter_element(field.key, field.values, search_state)
-        end, "\n")
-      end
+    Deprecation.silence(Blacklight::RenderConstraintsHelperBehavior) do
+      safe_join(search_state.filters.map do |field|
+        render_filter_element(field.key, field.values, search_state)
+      end, "\n")
     end
   end
 

lib/blacklight/configuration.rb

@@ -295,6 +295,16 @@ def default_per_page
     # @return [Boolean]
     property :enable_search_bar_autofocus, default: false
 
+    BASIC_SEARCH_PARAMETERS = [:q, :qt, :page, :per_page, :search_field, :sort, :controller, :action, :'facet.page', :'facet.prefix', :'facet.sort', :rows, :format].freeze
+    ADVANCED_SEARCH_PARAMETERS = [:clause, :op].freeze
+    # List the request parameters that compose the SearchState.
+    # If you use a plugin that adds to the search state, then you can add the parameters
+    # by modifiying this field.
+    # @!attribute search_state_fields
+    # @since v8.0.0
+    # @return [Array<Symbol>]
+    property :search_state_fields, default: BASIC_SEARCH_PARAMETERS + ADVANCED_SEARCH_PARAMETERS
+
     ##
     # Create collections of solr field configurations.
     # This will create array-like accessor methods for

lib/blacklight/deprecations/search_state_normalization.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+module Blacklight
+  module Deprecations
+    module SearchStateNormalization
+      extend ActiveSupport::Concern
+
+      class_methods do
+        def facet_params_need_normalization(facet_params)
+          Deprecation.warn(self, 'Calling `facet_params_need_normalization` on the Blacklight::SearchState ' \
+          'class is deprecated and will be removed in Blacklight 8. Delegate to #needs_normalization?(value_params) on the ' \
+          'filter fields of the search state object.')
+
+          facet_params.is_a?(Hash) && facet_params.values.any? { |x| x.is_a?(Hash) }
+        end
+
+        def normalize_facet_params(facet_params)
+          Deprecation.warn(self, 'Calling `normalize_facet_params` on the Blacklight::SearchState ' \
+          'class is deprecated and will be removed in Blacklight 8. Delegate to #normalize(value_params) on the ' \
+          'filter fields of the search state object.')
+
+          facet_params.transform_values { |value| value.is_a?(Hash) ? value.values : value }
+        end
+
+        def normalize_params(untrusted_params = {})
+          Deprecation.warn(self, 'Calling `normalize_params` on the Blacklight::SearchState ' \
+          'class is deprecated and will be removed in Blacklight 8. Call #normalize_params on the ' \
+          'search state object.')
+          params = untrusted_params
+
+          if params.respond_to?(:to_unsafe_h)
+            # This is the typical (not-ActionView::TestCase) code path.
+            params = params.to_unsafe_h
+            # In Rails 5 to_unsafe_h returns a HashWithIndifferentAccess, in Rails 4 it returns Hash
+            params = params.with_indifferent_access if params.instance_of? Hash
+          elsif params.is_a? Hash
+            # This is an ActionView::TestCase workaround for Rails 4.2.
+            params = params.dup.with_indifferent_access
+          else
+            params = params.dup.to_h.with_indifferent_access
+          end
+
+          # Normalize facet parameters mangled by facebook
+          params[:f] = normalize_facet_params(params[:f]) if facet_params_need_normalization(params[:f])
+          params[:f_inclusive] = normalize_facet_params(params[:f_inclusive]) if facet_params_need_normalization(params[:f_inclusive])
+
+          params
+        end
+      end
+    end
+  end
+end

lib/blacklight/search_state.rb

@@ -1,12 +1,14 @@
 # frozen_string_literal: true
 
+require 'blacklight/deprecations/search_state_normalization'
 require 'blacklight/search_state/filter_field'
 
 module Blacklight
   # This class encapsulates the search state as represented by the query
   # parameters namely: :f, :q, :page, :per_page and, :sort
   class SearchState
     extend Deprecation
+    include Blacklight::Deprecations::SearchStateNormalization
 
     attr_reader :blacklight_config # Must be called blacklight_config, because Blacklight::Facet calls blacklight_config.
     attr_reader :params
@@ -17,18 +19,7 @@ class SearchState
 
     delegate :facet_configuration_for_field, to: :blacklight_config
 
-    # @param [ActionController::Parameters] params
-    # @param [Blacklight::Config] blacklight_config
-    # @param [ApplicationController] controller used for the routing helpers
-    def initialize(params, blacklight_config, controller = nil)
-      @params = self.class.normalize_params(params)
-      @blacklight_config = blacklight_config
-      @controller = controller
-    end
-
-    def self.normalize_params(untrusted_params = {})
-      params = untrusted_params
-
+    def self.modifiable_params(params)
       if params.respond_to?(:to_unsafe_h)
         # This is the typical (not-ActionView::TestCase) code path.
         params = params.to_unsafe_h
@@ -40,12 +31,55 @@ def self.normalize_params(untrusted_params = {})
       else
         params = params.dup.to_h.with_indifferent_access
       end
+      params
+    end
 
-      # Normalize facet parameters mangled by facebook
-      params[:f] = normalize_facet_params(params[:f]) if facet_params_need_normalization(params[:f])
-      params[:f_inclusive] = normalize_facet_params(params[:f_inclusive]) if facet_params_need_normalization(params[:f_inclusive])
+    # @param [ActionController::Parameters] params
+    # @param [Blacklight::Config] blacklight_config
+    # @param [ApplicationController] controller used for the routing helpers
+    def initialize(params, blacklight_config, controller = nil)
+      @blacklight_config = blacklight_config
+      @controller = controller
+      @params = SearchState.modifiable_params(params)
+      normalize_params! if needs_normalization?
+    end
 
-      params
+    def needs_normalization?
+      return false if params.blank?
+      return true if (params.keys.map(&:to_s) - permitted_fields.map(&:to_s)).present?
+
+      !!filters.detect { |filter| filter.values.detect { |value| filter.needs_normalization?(value) } }
+    end
+
+    def normalize_params!
+      @params = normalize_params
+    end
+
+    def normalize_params
+      return params unless needs_normalization?
+
+      base_params = params.slice(*blacklight_config.search_state_fields)
+      normal_state = blacklight_config.facet_fields.each_value.inject(reset(base_params)) do |working_state, filter_key|
+        f = filter(filter_key)
+        next working_state unless f.any?
+
+        filter_values = f.values(except: [:inclusive_filters]).inject([]) do |memo, filter_value|
+          # flatten arrays that had been mangled into integer-indexed hashes
+          memo.concat([f.normalize(filter_value)].flatten)
+        end
+        filter_values = f.values(except: [:filters, :missing]).inject(filter_values) do |memo, filter_value|
+          memo << f.normalize(filter_value)
+        end
+        filter_values.inject(working_state) do |memo, filter_value|
+          memo.filter(filter_key).add(filter_value)
+        end
+      end
+      normal_state.params
+    end
+
+    def permitted_fields
+      filter_keys = filter_fields.inject(Set.new) { |memo, filter| memo.merge [filter.filters_key, filter.inclusive_filters_key] }
+      blacklight_config.search_state_fields + filter_keys.subtract([nil, '']).to_a
     end
 
     def to_hash
@@ -129,11 +163,7 @@ def remove_query_params
     end
 
     def filters
-      @filters ||= blacklight_config.facet_fields.each_value.map do |value|
-        f = filter(value)
-
-        f if f.any?
-      end.compact
+      @filters ||= filter_fields.select(&:any?)
     end
 
     def filter(field_key_or_field)
@@ -248,16 +278,6 @@ def facet_prefix
       params[facet_request_keys[:prefix]]
     end
 
-    def self.facet_params_need_normalization(facet_params)
-      facet_params.is_a?(Hash) && facet_params.values.any? { |x| x.is_a?(Hash) }
-    end
-
-    def self.normalize_facet_params(facet_params)
-      facet_params.transform_values do |value|
-        value.is_a?(Hash) ? value.values : value
-      end
-    end
-
     private
 
     def search_field_key
@@ -279,5 +299,9 @@ def facet_request_keys
     def reset_search_params
       Parameters.sanitize(params).except(:page, :counter)
     end
+
+    def filter_fields
+      blacklight_config.facet_fields.each_value.map { |value| filter(value) }
+    end
   end
 end

lib/blacklight/search_state/filter_field.rb

@@ -14,6 +14,14 @@ class FilterField
       #   @return [Blacklight::SearchState]
       attr_reader :search_state
 
+      # @!attribute param
+      #   @return [String,Symbol]
+      attr_reader :filters_key
+
+      # @!attribute inclusive_param
+      #   @return [String,Symbol]
+      attr_reader :inclusive_filters_key
+
       # @return [String,Symbol]
       delegate :key, to: :config
 
@@ -22,6 +30,8 @@ class FilterField
       def initialize(config, search_state)
         @config = config
         @search_state = search_state
+        @filters_key = :f
+        @inclusive_filters_key = :f_inclusive
       end
 
       # @param [String,#value] item a filter item to add to the url
@@ -43,15 +53,15 @@ def add(item)
 
         url_key = key
         params = new_state.params
-        param = :f
+        param = filters_key
         value = as_url_parameter(item)
 
         if value == Blacklight::SearchState::FilterField::MISSING
           url_key = "-#{key}"
           value = Blacklight::Engine.config.blacklight.facet_missing_param
         end
 
-        param = :f_inclusive if value.is_a?(Array)
+        param = inclusive_filters_key if value.is_a?(Array)
 
         # value could be a string
         params[param] = (params[param] || {}).dup
@@ -79,15 +89,15 @@ def remove(item)
         url_key = config.key
         params = new_state.params
 
-        param = :f
+        param = filters_key
         value = as_url_parameter(item)
 
         if value == Blacklight::SearchState::FilterField::MISSING
           url_key = "-#{key}"
           value = Blacklight::Engine.config.blacklight.facet_missing_param
         end
 
-        param = :f_inclusive if value.is_a?(Array)
+        param = inclusive_filters_key if value.is_a?(Array)
 
         # need to dup the facet values too,
         # if the values aren't dup'd, then the values
@@ -112,19 +122,22 @@ def remove(item)
       end
 
       # @return [Array] an array of applied filters
-      def values
+      def values(except: [])
         params = search_state.params
-        f = Array(params.dig(:f, key))
-        f_inclusive = [params.dig(:f_inclusive, key)] if params.dig(:f_inclusive, key).present?
-        f_missing = [Blacklight::SearchState::FilterField::MISSING] if params.dig(:f, "-#{key}")&.any? { |v| v == Blacklight::Engine.config.blacklight.facet_missing_param }
+        return [] if params.blank?
+
+        f = except.include?(:filters) ? [] : [params.dig(filters_key, key)].flatten.compact
+        f_inclusive = [params.dig(:f_inclusive, key)] unless params.dig(inclusive_filters_key, key).blank? || except.include?(:inclusive_filters)
+        f_missing = [Blacklight::SearchState::FilterField::MISSING] if params.dig(filters_key, "-#{key}")&.any? { |v| v == Blacklight::Engine.config.blacklight.facet_missing_param }
+        f_missing = [] if except.include?(:missing)
 
         f + (f_inclusive || []) + (f_missing || [])
       end
       delegate :any?, to: :values
 
       # Appease rubocop rules by implementing #each_value
-      def each_value(&block)
-        values.each(&block)
+      def each_value(except: [], &block)
+        values(except: except).each(&block)
       end
 
       # @param [String,#value] item a filter to remove from the url
@@ -138,14 +151,22 @@ def include?(item)
         params = search_state.params
 
         if value.is_a?(Array)
-          (params.dig(:f_inclusive, key) || []).to_set == value.to_set
+          (params.dig(inclusive_filters_key, key) || []).to_set == value.to_set
         elsif value == Blacklight::SearchState::FilterField::MISSING
-          (params.dig(:f, "-#{key}") || []).include?(Blacklight::Engine.config.blacklight.facet_missing_param)
+          (params.dig(filters_key, "-#{key}") || []).include?(Blacklight::Engine.config.blacklight.facet_missing_param)
         else
-          (params.dig(:f, key) || []).include?(value)
+          (params.dig(filters_key, key) || []).include?(value)
         end
       end
 
+      def needs_normalization?(value_params)
+        value_params&.is_a?(Hash) && value_params != Blacklight::SearchState::FilterField::MISSING
+      end
+
+      def normalize(value_params)
+        needs_normalization?(value_params) ? value_params.values : value_params
+      end
+
       private
 
       # TODO: this code is duplicated in Blacklight::FacetsHelperBehavior

spec/components/blacklight/constraints_component_spec.rb

@@ -12,7 +12,7 @@
   end
 
   let(:blacklight_config) do
-    Blacklight::Configuration.new.tap do |config|
+    Blacklight::Configuration.new.configure do |config|
       config.add_facet_field 'some_facet'
     end
   end
@@ -43,6 +43,19 @@
   context 'with a facet' do
     let(:query_params) { { f: { some_facet: ['some value'] } } }
 
+    before do
+      controller.blacklight_config.configure do |config|
+        config.add_facet_field :some_facet
+      end
+    end
+
+    # this has to be cleaned up to prevent leaking class configuration
+    after do
+      controller.blacklight_config.configure do |config|
+        config['facet_fields'].delete('some_facet')
+      end
+    end
+
     it 'renders the query' do
       expect(rendered).to have_selector('.constraint-value > .filter-name', text: 'Some Facet').and(have_selector('.constraint-value > .filter-value', text: 'some value'))
     end

spec/components/blacklight/facet_field_list_component_spec.rb

@@ -123,7 +123,12 @@
       )
     end
 
-    let(:search_state) { Blacklight::SearchState.new(params.with_indifferent_access, Blacklight::Configuration.new) }
+    let(:blacklight_config) do
+      Blacklight::Configuration.new.configure do |config|
+        config.add_facet_field :field
+      end
+    end
+    let(:search_state) { Blacklight::SearchState.new(params.with_indifferent_access, blacklight_config) }
     let(:params) { { f_inclusive: { field: %w[a b c] } } }
 
     it 'displays the constraint above the list' do

spec/components/blacklight/facet_item_pivot_component_spec.rb

@@ -7,8 +7,14 @@
     render_inline_to_capybara_node(described_class.new(facet_item: facet_item))
   end
 
+  let(:blacklight_config) do
+    Blacklight::Configuration.new.configure do |config|
+      config.add_facet_field :z
+    end
+  end
+
   let(:search_state) do
-    Blacklight::SearchState.new({}, Blacklight::Configuration.new)
+    Blacklight::SearchState.new({}, blacklight_config)
   end
 
   let(:facet_item) do