searchable: factor out metatag value parser.

Factor out the code that parses metatag values (e.g. `score:>5`) and
search URL params (e.g. `search[score]=>5`) into a RangeParser class.

Also fix a bug where, if the `search[order]=custom` param was used
without a `search[id]` param, an exception would be raised. Fix another
bug where if an invalid `search[id]` was provided, then the custom order
would be ignored and the search would be returned with the default order
instead. Now if you use `search[order]=custom` without a valid
`search[id]` param, the search will return no results.

app/logical/concerns/searchable.rb

@@ -201,7 +201,7 @@ module Searchable
   end
 
   def attribute_matches(value, field, type = :integer)
-    operator, *args = PostQueryBuilder.parse_metatag_value(value, type)
+    operator, *args = RangeParser.parse(value, type)
     relation = where_operator(field, operator, *args)
 
     # XXX Hack to make negating the equality operator work correctly on nullable columns.
@@ -215,7 +215,7 @@ module Searchable
     end
 
     relation
-  rescue PostQueryBuilder::ParseError
+  rescue RangeParser::ParseError
     none
   end
 
@@ -223,15 +223,24 @@ module Searchable
     SearchContext.new(all, params, current_user).search_attributes(attributes)
   end
 
+  # Order according to the list of IDs in the given string.
+  #
+  # Post.order_custom("1,2,3") => [post #1, post #2, post #3]
+  def order_custom(string)
+    operator, ids = RangeParser.parse(string, :integer)
+    return none unless operator == :in
+
+    in_order_of(:id, ids)
+  rescue RangeParser::ParseError
+    none
+  end
+
   def apply_default_order(params)
     if params[:order] == "custom"
-      parse_ids = PostQueryBuilder.parse_range(params[:id], :integer)
+      order_custom(params[:id])
-      if parse_ids[0] == :in
+    else
-        return in_order_of(:id, parse_ids[1])
+      default_order
-      end
     end
-
-    default_order
   end
 
   def default_order

app/logical/post_query_builder.rb

@@ -12,8 +12,6 @@ require "strscan"
 class PostQueryBuilder
   extend Memoist
 
-  class ParseError < StandardError; end
-
   # How many tags a `blah*` search should match.
   MAX_WILDCARD_TAGS = 100
 
@@ -269,7 +267,13 @@ class PostQueryBuilder
     elsif post_query.has_metatag?(:date, :age) && post_query.find_metatag(:order).in?(["id_desc", nil])
       relation = search_order(relation, "created_at_desc")
     elsif post_query.find_metatag(:order) == "custom"
-      relation = search_order_custom(relation, post_query.select_metatags(:id).map(&:value))
+      ids = post_query.select_metatags(:id).map(&:value)
+
+      if ids.size == 1
+        relation = relation.order_custom(ids.first)
+      else
+        relation = relation.none
+      end
     elsif post_query.has_metatag?(:ordfav)
       # no-op
     else
@@ -491,141 +495,4 @@ class PostQueryBuilder
 
     relation
   end
-
-  def search_order_custom(relation, id_metatags)
-    return relation.none unless id_metatags.present? && id_metatags.size == 1
-
-    operator, ids = PostQueryBuilder.parse_range(id_metatags.first, :integer)
-    return relation.none unless operator == :in
-
-    relation.in_order_of(:id, ids)
-  end
-
-  concerning :ParseMethods do
-    class_methods do
-      # Parse a simple string value into a Ruby type.
-      # @param string [String] the value to parse
-      # @param type [Symbol] the value's type
-      # @return [Object] the parsed value
-      def parse_cast(string, type)
-        case type
-        when :enum
-          string.downcase
-
-        when :integer
-          Integer(string) # raises ArgumentError if string is invalid
-
-        when :float
-          Float(string) # raises ArgumentError if string is invalid
-
-        when :md5
-          raise ParseError, "#{string} is not a valid MD5" unless string.match?(/\A[0-9a-fA-F]{32}\z/)
-          string.downcase
-
-        when :date, :datetime
-          date = Time.zone.parse(string)
-          raise ParseError, "#{string} is not a valid date" if date.nil?
-          date
-
-        when :age
-          DurationParser.parse(string).ago
-
-        when :interval
-          DurationParser.parse(string)
-
-        when :ratio
-          string = string.tr(":", "/") # "2:3" => "2/3"
-          Rational(string).to_f.round(2) # raises ArgumentError or ZeroDivisionError if string is invalid
-
-        when :filesize
-          raise ParseError, "#{string} is not a valid filesize" unless string =~ /\A(\d+(?:\.\d*)?|\d*\.\d+)([kKmM]?)[bB]?\Z/
-
-          size = Float($1)
-          unit = $2
-
-          conversion_factor = case unit
-          when /m/i
-            1024 * 1024
-          when /k/i
-            1024
-          else
-            1
-          end
-
-          (size * conversion_factor).to_i
-
-        else
-          raise NotImplementedError, "unrecognized type #{type} for #{string}"
-        end
-
-      rescue ArgumentError, ZeroDivisionError => e
-        raise ParseError, e.message
-      end
-
-      def parse_metatag_value(string, type)
-        if type == :enum
-          [:in, string.split(/[, ]+/).map { |x| parse_cast(x, type) }]
-        else
-          parse_range(string, type)
-        end
-      end
-
-      # Parse a metatag range value of the given type. For example: 1..10.
-      # @param string [String] the metatag value
-      # @param type [Symbol] the value's type
-      def parse_range(string, type)
-        range = case string
-        when /\A(.+?)\.\.\.(.+)/ # A...B
-          lo, hi = [parse_cast($1, type), parse_cast($2, type)].sort
-          [:between, (lo...hi)]
-        when /\A(.+?)\.\.(.+)/
-          lo, hi = [parse_cast($1, type), parse_cast($2, type)].sort
-          [:between, (lo..hi)]
-        when /\A<=(.+)/, /\A\.\.(.+)/
-          [:lteq, parse_cast($1, type)]
-        when /\A<(.+)/
-          [:lt, parse_cast($1, type)]
-        when /\A>=(.+)/, /\A(.+)\.\.\Z/
-          [:gteq, parse_cast($1, type)]
-        when /\A>(.+)/
-          [:gt, parse_cast($1, type)]
-        when /[, ]/
-          [:in, string.split(/[, ]+/).map {|x| parse_cast(x, type)}]
-        when "any"
-          [:not_eq, nil]
-        when "none"
-          [:eq, nil]
-        else
-          # add a 5% tolerance for float and filesize values
-          if type == :float || (type == :filesize && string =~ /[km]b?\z/i)
-            value = parse_cast(string, type)
-            [:between, (value * 0.95..value * 1.05)]
-          elsif type.in?([:date, :age])
-            value = parse_cast(string, type)
-            [:between, (value.beginning_of_day..value.end_of_day)]
-          else
-            [:eq, parse_cast(string, type)]
-          end
-        end
-
-        range = reverse_range(range) if type == :age
-        range
-      end
-
-      def reverse_range(range)
-        case range
-        in [:lteq, value]
-          [:gteq, value]
-        in [:lt, value]
-          [:gt, value]
-        in [:gteq, value]
-          [:lteq, value]
-        in [:gt, value]
-          [:lt, value]
-        else
-          range
-        end
-      end
-    end
-  end
 end

app/logical/range_parser.rb

@@ -0,0 +1,153 @@
+# frozen_string_literal: true
+
+# Parse a simple inequality expression into an operator and value. Used for parsing
+# metatag values (e.g. `score:>5`) and URL params (e.g. /comments?search[score]=>5).
+#
+# @example
+#
+#   RangeParser.parse("5")      => [:eq, 5]
+#   RangeParser.parse(">5")     => [:gt, 5]
+#   RangeParser.parse(">=5")    => [:gteq, 5]
+#   RangeParser.parse("<5")     => [:lt, 5]
+#   RangeParser.parse("<=5")    => [:lteq, 5]
+#   RangeParser.parse("5..")    => [:lteq, 5]
+#   RangeParser.parse("..5")    => [:gteq, 5]
+#   RangeParser.parse("5..10")  => [:between, (5..10)]
+#   RangeParser.parse("5...10") => [:between, (5...10)]
+#   RangeParser.parse("5,6,7")  => [:in, [5, 6, 7]]
+#   RangeParser.parse("any")    => [:not_eq, nil]
+#   RangeParser.parse("none")   => [:eq, nil]
+#
+class RangeParser
+  class ParseError < StandardError; end
+
+  attr_reader :string, :type
+
+  def self.parse(...)
+    new(...).parse
+  end
+
+  # @param string [String] The expression to parse
+  # @param type [Symbol] The type of the expression (:enum, :integer, :float, :md5, :date, :datetime, :age, :interval, :ratio, or :filesize)
+  def initialize(string, type = :integer)
+    @string = string.to_s
+    @type = type
+  end
+
+  # Parse a string expression into an operator and value.
+  # @return [(Symbol, Object)] The operator name and the value
+  def parse
+    range = case string
+    in _ if type == :enum
+      [:in, string.split(/[, ]+/).map { |x| parse_value(x) }]
+    in /\A(.+?)\.\.\.(.+)/ # A...B
+      lo, hi = [parse_value($1), parse_value($2)].sort
+      [:between, (lo...hi)]
+    in /\A(.+?)\.\.(.+)/ # A..B
+      lo, hi = [parse_value($1), parse_value($2)].sort
+      [:between, (lo..hi)]
+    in /\A<=(.+)/ | /\A\.\.(.+)/ # <=A, ..A
+      [:lteq, parse_value($1)]
+    in /\A<(.+)/ # <A
+      [:lt, parse_value($1)]
+    in /\A>=(.+)/ | /\A(.+)\.\.\z/ # >=A, A..
+      [:gteq, parse_value($1)]
+    in /\A>(.+)/ # >A
+      [:gt, parse_value($1)]
+    in /[, ]/ # A,B,C
+      [:in, string.split(/[, ]+/).map { |x| parse_value(x) }]
+    in "any"
+      [:not_eq, nil]
+    in "none"
+      [:eq, nil]
+    in _ if type == :float
+      value = parse_value(string)
+      [:between, (value * 0.95..value * 1.05)] # add a 5% tolerance for float values
+    in /[km]b?\z/i if type == :filesize
+      value = parse_value(string)
+      [:between, (value * 0.95..value * 1.05)] # add a 5% tolerance for filesize values
+    in _ if type in :date | :age
+      value = parse_value(string)
+      [:between, (value.beginning_of_day..value.end_of_day)]
+    else
+      [:eq, parse_value(string)]
+    end
+
+    range = reverse_range(range) if type == :age
+    range
+  end
+
+  def reverse_range(range)
+    case range
+    in [:lteq, value]
+      [:gteq, value]
+    in [:lt, value]
+      [:gt, value]
+    in [:gteq, value]
+      [:lteq, value]
+    in [:gt, value]
+      [:lt, value]
+    else
+      range
+    end
+  end
+
+  # Parse a simple string value into a Ruby type.
+  #
+  # @param string [String] the value to parse
+  # @return [Object] the parsed value
+  def parse_value(string)
+    case type
+    when :enum
+      string.downcase
+
+    when :integer
+      Integer(string) # raises ArgumentError if string is invalid
+
+    when :float
+      Float(string) # raises ArgumentError if string is invalid
+
+    when :md5
+      raise ParseError, "#{string} is not a valid MD5" unless string.match?(/\A[0-9a-fA-F]{32}\z/)
+      string.downcase
+
+    when :date, :datetime
+      date = Time.zone.parse(string)
+      raise ParseError, "#{string} is not a valid date" if date.nil?
+      date
+
+    when :age
+      DurationParser.parse(string).ago
+
+    when :interval
+      DurationParser.parse(string)
+
+    when :ratio
+      string = string.tr(":", "/") # "2:3" => "2/3"
+      Rational(string).to_f.round(2) # raises ArgumentError or ZeroDivisionError if string is invalid
+
+    when :filesize
+      raise ParseError, "#{string} is not a valid filesize" unless string =~ /\A(\d+(?:\.\d*)?|\d*\.\d+)([kKmM]?)[bB]?\Z/
+
+      size = Float($1)
+      unit = $2
+
+      conversion_factor = case unit
+      when /m/i
+        1024 * 1024
+      when /k/i
+        1024
+      else
+        1
+      end
+
+      (size * conversion_factor).to_i
+
+    else
+      raise NotImplementedError, "unrecognized type #{type} for #{string}"
+    end
+
+  rescue ArgumentError, ZeroDivisionError => e
+    raise ParseError, e.message
+  end
+end

test/functional/application_controller_test.rb

@@ -310,6 +310,30 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest
       assert_equal(tags.first.id, response.parsed_body.first.fetch("id"))
     end
 
+    should "support ordering by search[order]=custom" do
+      tags = create_list(:tag, 2, post_count: 42)
+      get tags_path, params: { search: { id: "#{tags[0].id},#{tags[1].id}", order: "custom" } }, as: :json
+
+      assert_response :success
+      assert_equal(tags.pluck(:id), response.parsed_body.pluck("id"))
+    end
+
+    should "return nothing if the search[order]=custom param isn't accompanied by search[id]" do
+      tags = create_list(:tag, 2, post_count: 42)
+      get tags_path, params: { search: { order: "custom" } }, as: :json
+
+      assert_response :success
+      assert_equal(0, response.parsed_body.size)
+    end
+
+    should "return nothing if the search[order]=custom param isn't accompanied by a valid search[id]" do
+      tags = create_list(:tag, 2, post_count: 42)
+      get tags_path, params: { search: { id: ">1", order: "custom" } }, as: :json
+
+      assert_response :success
+      assert_equal(0, response.parsed_body.size)
+    end
+
     should "support the expiry parameter" do
       get posts_path, as: :json, params: { expiry: "1" }