searchable: refactor search_attributes helper methods into class.

Move the `search_attributes` helper methods inside the Searchable
concern into a SearchContext helper class. This is so we don't have to
pass `params` and `current_user` down through a bunch of different
helper methods, and so that these private helper methods don't pollute
the object's namespace.

app/logical/concerns/searchable.rb

@@ -3,31 +3,12 @@
 module Searchable
   extend ActiveSupport::Concern
 
-  def parameter_hash?(params)
-    params.present? && params.respond_to?(:each_value)
-  end
-
-  def parameter_depth(params)
-    return 0 if params.values.empty?
-    1 + params.values.map { |v| parameter_hash?(v) ? parameter_depth(v) : 1 }.max
-  end
-
   def negate_relation
     relation = unscoped
     relation = relation.from(all.from_clause.value) if all.from_clause.value.present?
     relation.where(all.where_clause.invert.ast)
   end
 
-  # XXX hacky method to AND two relations together.
-  # XXX Replace with ActiveRecord#and (cf https://github.com/rails/rails/pull/39328)
-  def and_relation(relation)
-    q = all
-    q = q.where(relation.where_clause.ast) if relation.where_clause.present?
-    q = q.joins(relation.joins_values + q.joins_values) if relation.joins_values.present?
-    q = q.order(relation.order_values) if relation.order_values.present?
-    q
-  end
-
   # Search a table column by an Arel operator.
   #
   # @see https://github.com/rails/rails/blob/master/activerecord/lib/arel/predications.rb
@@ -189,29 +170,13 @@ module Searchable
     where("(#{tsvectors.to_sql}) @@ websearch_to_tsquery('pg_catalog.english', :query)", query: query)
   end
 
-  def search_boolean_attribute(attr, params)
-    if params[attr].present?
-      boolean_attribute_matches(attr, params[attr])
-    else
-      all
-    end
-  end
-
-  def search_inet_attribute(attr, params)
-    if params[attr].present?
-      where_inet_matches(attr, params[attr])
-    else
-      all
-    end
-  end
-
   # value: "5", ">5", "<5", ">=5", "<=5", "5..10", "5,6,7"
   def where_numeric_matches(attribute, value, type = :integer)
     range = PostQueryBuilder.parse_range(value, type)
     where_operator(attribute, *range)
   end
 
-  def boolean_attribute_matches(attribute, value)
+  def where_boolean_matches(attribute, value)
     value = value.to_s
 
     if value.truthy?
@@ -237,63 +202,91 @@ module Searchable
     end
   end
 
-  def search_attributes(params, *attributes)
+  def search_attributes(params, *attributes, current_user: CurrentUser.user)
+    SearchContext.new(all, params, current_user).search_attributes(attributes)
+  end
+
+  def apply_default_order(params)
+    if params[:order] == "custom"
+      parse_ids = PostQueryBuilder.parse_range(params[:id], :integer)
+      if parse_ids[0] == :in
+        return in_order_of(:id, parse_ids[1])
+      end
+    end
+
+    default_order
+  end
+
+  def default_order
+    order(id: :desc)
+  end
+
+  private
+
+  # A SearchContext contains private helper methods for `search_attributes`.
+  class SearchContext
+    attr_reader :relation, :params, :current_user
+
+    def initialize(relation, params, current_user)
+      @relation = relation
+      @params = params.try(:with_indifferent_access) || params.try(:to_unsafe_h)
+      @current_user = current_user
+    end
+
+    def search_attributes(attributes)
       raise ArgumentError, "max parameter depth of 10 exceeded" if parameter_depth(params) > 10
 
-    # This allows the hash keys to be either strings or symbols
+      attributes.reduce(relation) do |relation, attribute|
-    indifferent_params = params.try(:with_indifferent_access) || params.try(:to_unsafe_h)
+        search_context(relation).search_attribute(attribute)
-    raise ArgumentError, "unable to process params" if indifferent_params.nil?
-
-    attributes.reduce(all) do |relation, attribute|
-      relation.search_attribute(attribute, indifferent_params, CurrentUser.user)
       end
     end
 
-  def search_attribute(name, params, current_user)
+    def search_attribute(name)
-    if has_attribute?(name)
+      if relation.has_attribute?(name)
-      search_basic_attribute(name, params, current_user)
+        search_basic_attribute(name)
-    elsif reflections.has_key?(name.to_s)
+      elsif relation.reflections.has_key?(name.to_s)
-      search_association_attribute(name, params, current_user)
+        search_association_attribute(name)
       else
         raise ArgumentError, "#{name} is not an attribute or association"
       end
     end
 
-  def search_basic_attribute(name, params, current_user)
+    def search_basic_attribute(name)
-    column = column_for_attribute(name)
+      column = relation.column_for_attribute(name)
-    type = column.type
 
       if column.try(:array?)
-      subtype = type
         type = :array
-    elsif defined_enums.has_key?(name.to_s)
+        subtype = column.type
+      elsif relation.defined_enums.has_key?(name.to_s)
         type = :enum
+      else
+        type = column.type
       end
 
       case type
       when :string, :text
-      search_text_attribute(name, params)
+        search_text_attribute(name)
       when :uuid
-      search_uuid_attribute(name, params)
+        search_uuid_attribute(name)
       when :boolean
-      search_boolean_attribute(name, params)
+        search_boolean_attribute(name)
       when :integer, :float, :datetime, :interval
-      search_numeric_attribute(name, params, type: type)
+        search_numeric_attribute(name, type: type)
       when :inet
-      search_inet_attribute(name, params)
+        search_inet_attribute(name)
       when :enum
-      search_enum_attribute(name, params)
+        search_enum_attribute(name)
       when :jsonb
-      search_jsonb_attribute(name, params)
+        search_jsonb_attribute(name)
       when :array
-      search_array_attribute(name, subtype, params)
+        search_array_attribute(name, subtype)
       else
         raise NotImplementedError, "unhandled attribute type: #{name} (#{type})"
       end
     end
 
-  def search_numeric_attribute(attr, params, key: attr, type: :integer)
+    def search_numeric_attribute(attr, key: attr, type: :integer)
-    relation = all
+      relation = self.relation
 
       if params[key].present?
         relation = relation.where_numeric_matches(attr, params[key], type)
@@ -330,8 +323,8 @@ module Searchable
       relation
     end
 
-  def search_text_attribute(attr, params)
+    def search_text_attribute(attr)
-    relation = all
+      relation = self.relation
 
       if params[attr].present?
         relation = relation.where(attr => params[attr])
@@ -404,8 +397,8 @@ module Searchable
       relation
     end
 
-  def search_uuid_attribute(attr, params)
+    def search_uuid_attribute(attr)
-    relation = all
+      relation = self.relation
 
       if params[attr].present?
         relation = relation.where(attr => params[attr])
@@ -422,96 +415,24 @@ module Searchable
       relation
     end
 
-  def search_association_attribute(attr, params, current_user)
+    def search_boolean_attribute(attr)
-    association = reflect_on_association(attr)
+      if params[attr].present?
-    relation = all
+        relation.where_boolean_matches(attr, params[attr])
-
-    if association.polymorphic?
-      return search_polymorphic_attribute(attr, params, current_user)
-    end
-
-    if association.belongs_to?
-      relation = relation.search_attribute(association.foreign_key, params, current_user)
-    end
-
-    model = association.klass
-    if model == User && params["#{attr}_name"].present?
-      name = params["#{attr}_name"]
-      if name.include?("*")
-        relation = relation.where(attr => User.search(name_matches: name).reorder(nil))
       else
-        relation = relation.where(attr => User.find_by_name(name))
+        relation
       end
     end
 
-    if model == Post && params["#{attr}_tags_match"].present?
+    def search_inet_attribute(attr)
-      posts = Post.user_tag_match(params["#{attr}_tags_match"], current_user).reorder(nil)
+      if params[attr].present?
-
+        relation.where_inet_matches(attr, params[attr])
-      if association.through_reflection?
-        relation = relation.includes(association.through_reflection.name).where(association.through_reflection.name => { attr => posts })
       else
-        relation = relation.where(attr => posts)
-      end
-    end
-
-    if params["has_#{attr}"].to_s.truthy? || params["has_#{attr}"].to_s.falsy?
-      relation = relation.search_has_include(attr, params["has_#{attr}"].to_s.truthy?, model)
-    end
-
-    if parameter_hash?(params[attr])
-      relation = relation.includes(attr).references(attr).where(attr => model.visible(current_user).search(params[attr]).reorder(nil))
-    end
-
         relation
       end
-
-  def search_polymorphic_attribute(attr, params, current_user)
-    model_keys = ((model_types || []) & params.keys)
-    # The user can only logically specify one model at a time, so more than that should return no results
-    return none if model_keys.length > 1
-
-    relation = all
-    model_specified = false
-    model_key = model_keys[0]
-    if model_keys.length == 1 && parameter_hash?(params[model_key])
-      # Returning none here for the same reason specified above
-      return none if params["#{attr}_type"].present? && params["#{attr}_type"] != model_key
-      model_specified = true
-      model = Kernel.const_get(model_key)
-      relation = relation.where(attr => model.visible(current_user).search(params[model_key]))
     end
 
-    if params["#{attr}_id"].present?
+    def search_jsonb_attribute(name)
-      relation = relation.search_attribute("#{attr}_id", params, current_user)
+      relation = self.relation
-    end
-
-    if params["#{attr}_type"].present? && !model_specified
-      relation = relation.search_attribute("#{attr}_type", params, current_user)
-    end
-
-    relation
-  end
-
-  def search_enum_attribute(name, params)
-    relation = all
-
-    if params[name].present?
-      value = params[name].split(/[, ]+/).map(&:downcase)
-      relation = relation.where(name => value)
-    end
-
-    if params[:"#{name}_not"].present?
-      value = params[:"#{name}_not"].split(/[, ]+/).map(&:downcase)
-      relation = relation.where.not(name => value)
-    end
-
-    relation = relation.search_numeric_attribute(name, params, key: :"#{name}_id")
-
-    relation
-  end
-
-  def search_jsonb_attribute(name, params)
-    relation = all
 
       if params[name].present?
         relation = relation.where_json_contains(:metadata, params[name])
@@ -530,8 +451,26 @@ module Searchable
       relation
     end
 
-  def search_array_attribute(name, type, params)
+    def search_enum_attribute(name)
-    relation = all
+      relation = self.relation
+
+      if params[name].present?
+        value = params[name].split(/[, ]+/).map(&:downcase)
+        relation = relation.where(name => value)
+      end
+
+      if params[:"#{name}_not"].present?
+        value = params[:"#{name}_not"].split(/[, ]+/).map(&:downcase)
+        relation = relation.where.not(name => value)
+      end
+
+      relation = search_context(relation).search_numeric_attribute(name, key: :"#{name}_id")
+
+      relation
+    end
+
+    def search_array_attribute(name, type)
+      relation = self.relation
       singular_name = name.to_s.singularize
 
       if params[:"#{name}_include_any"]
@@ -589,44 +528,111 @@ module Searchable
       relation
     end
 
-  def search_has_include(name, exists, model)
+    def search_association_attribute(attr)
-    if column_names.include?("#{name}_id")
+      association = relation.reflect_on_association(attr)
-      return exists ? where.not("#{name}_id" => nil) : where("#{name}_id" => nil)
+      relation = self.relation
+
+      if association.polymorphic?
+        return search_polymorphic_attribute(attr)
       end
 
-    association = reflect_on_association(name)
+      if association.belongs_to?
+        relation = search_attribute(association.foreign_key)
+      end
+
+      model = association.klass
+      if model == User && params["#{attr}_name"].present?
+        name = params["#{attr}_name"]
+        if name.include?("*")
+          relation = relation.where(attr => User.search(name_matches: name).reorder(nil))
+        else
+          relation = relation.where(attr => User.find_by_name(name))
+        end
+      end
+
+      if model == Post && params["#{attr}_tags_match"].present?
+        posts = Post.user_tag_match(params["#{attr}_tags_match"], current_user).reorder(nil)
+
+        if association.through_reflection?
+          relation = relation.includes(association.through_reflection.name).where(association.through_reflection.name => { attr => posts })
+        else
+          relation = relation.where(attr => posts)
+        end
+      end
+
+      if params["has_#{attr}"].to_s.truthy? || params["has_#{attr}"].to_s.falsy?
+        relation = search_context(relation).search_has_include(attr, params["has_#{attr}"].to_s.truthy?, model)
+      end
+
+      if parameter_hash?(params[attr])
+        relation = relation.includes(attr).references(attr).where(attr => model.visible(current_user).search(params[attr]).reorder(nil))
+      end
+
+      relation
+    end
+
+    def search_polymorphic_attribute(attr)
+      model_keys = ((relation.model_types || []) & params.keys)
+      # The user can only logically specify one model at a time, so more than that should return no results
+      return none if model_keys.length > 1
+
+      relation = self.relation
+      model_specified = false
+      model_key = model_keys[0]
+      if model_keys.length == 1 && parameter_hash?(params[model_key])
+        # Returning none here for the same reason specified above
+        return none if params["#{attr}_type"].present? && params["#{attr}_type"] != model_key
+        model_specified = true
+        model = Kernel.const_get(model_key)
+        relation = relation.where(attr => model.visible(current_user).search(params[model_key]))
+      end
+
+      if params["#{attr}_id"].present?
+        relation = search_context(relation).search_attribute("#{attr}_id")
+      end
+
+      if params["#{attr}_type"].present? && !model_specified
+        relation = search_context(relation).search_attribute("#{attr}_type")
+      end
+
+      relation
+    end
+
+    def search_has_include(name, exists, model)
+      if relation.column_names.include?("#{name}_id")
+        return exists ? relation.where.not("#{name}_id" => nil) : relation.where("#{name}_id" => nil)
+      end
+
+      association = relation.reflect_on_association(name)
       primary_key = association.active_record_primary_key
       foreign_key = association.foreign_key
       # The belongs_to macro has its primary and foreign keys reversed
       primary_key, foreign_key = foreign_key, primary_key if association.macro == :belongs_to
-    return all if primary_key.nil? || foreign_key.nil?
+      return relation if primary_key.nil? || foreign_key.nil?
 
-    self_table = arel_table
+      self_table = relation.arel_table
       model_table = model.arel_table
       model_exists = model.model_restriction(model_table).where(model_table[foreign_key].eq(self_table[primary_key])).exists
       if exists
-      attribute_restriction(name).where(model_exists)
+        relation.attribute_restriction(name).where(model_exists)
       else
-      attribute_restriction(name).where.not(model_exists)
+        relation.attribute_restriction(name).where.not(model_exists)
       end
     end
 
-  def apply_default_order(params)
+    def parameter_depth(params)
-    if params[:order] == "custom"
+      return 0 if params.values.empty?
-      parse_ids = PostQueryBuilder.parse_range(params[:id], :integer)
+      1 + params.values.map { |v| parameter_hash?(v) ? parameter_depth(v) : 1 }.max
-      if parse_ids[0] == :in
-        return in_order_of(:id, parse_ids[1])
-      end
     end
 
-    default_order
+    def parameter_hash?(params)
+      params.present? && params.respond_to?(:each_value)
     end
 
-  def default_order
+    def search_context(relation)
-    order(id: :desc)
+      SearchContext.new(relation, params, current_user)
+    end
   end
-
-  private
 
   def qualified_column_for(attr)
     return attr if attr.to_s.include?(".")

test/unit/concerns/searchable.rb

@@ -19,7 +19,7 @@ class SearchableTest < ActiveSupport::TestCase
     context "for a nonexistent attribute" do
       should "raise an error" do
         assert_raises(ArgumentError) do
-          Post.search_attribute(:answer, 42, User.anonymous)
+          Post.search_attributes({ answer: 42 }, :answer)
         end
       end
     end