Search Refactor: Let's use a class to keep track of our state rather

than passing params everywhere. Also make the private API private.

app/controllers/search_controller.rb

@@ -3,8 +3,11 @@ require_dependency 'search'
 class SearchController < ApplicationController
 
   def query
-    search_result = Search.query(params[:term], guardian, params[:type_filter], SiteSetting.min_search_term_length)
+    search = Search.new(params[:term],
-    render_json_dump(search_result.as_json)
+                        guardian: guardian,
+                        type_filter: params[:type_filter])
+
+    render_json_dump(search.execute.as_json)
   end
 
 end

lib/search.rb

@@ -1,4 +1,4 @@
-module Search
+class Search
 
   def self.per_facet
     5
@@ -8,101 +8,6 @@ module Search
     %w(topic category user)
   end
 
-  def self.user_query_sql
-    "SELECT 'user' AS type,
-                  u.username_lower AS id,
-                  '/users/' || u.username_lower AS url,
-                  u.username AS title,
-                  u.email,
-                  NULL AS color,
-                  NULL AS text_color
-    FROM users AS u
-    JOIN users_search s on s.id = u.id
-    WHERE s.search_data @@ TO_TSQUERY(:locale, :query)
-    ORDER BY CASE WHEN u.username_lower = lower(:orig) then 0 else 1 end,  last_posted_at desc
-    LIMIT :limit
-    "
-  end
-
-  def self.post_query(guardian, type, args)
-    builder = SqlBuilder.new <<SQL
-    /*select*/
-    FROM topics AS ft
-    /*join*/
-      JOIN posts_search s on s.id = p.id
-      LEFT JOIN categories c ON c.id = ft.category_id
-    /*where*/
-    ORDER BY
-            TS_RANK_CD(TO_TSVECTOR(:locale, ft.title), TO_TSQUERY(:locale, :query)) desc,
-            TS_RANK_CD(search_data, TO_TSQUERY(:locale, :query)) desc,
-            bumped_at desc
-    LIMIT :limit
-SQL
-
-    builder.select "'topic' AS type"
-    builder.select("CAST(ft.id AS VARCHAR)")
-
-    if type == :topic
-      builder.select "'/t/slug/' || ft.id AS url"
-    else
-      builder.select "'/t/slug/' || ft.id || '/' || p.post_number AS url"
-    end
-
-    builder.select "ft.title, NULL AS email, NULL AS color, NULL AS text_color"
-
-  if type == :topic
-    builder.join "posts AS p ON p.topic_id = ft.id AND p.post_number = 1"
-  else
-    builder.join "posts AS p ON p.topic_id = ft.id AND p.post_number > 1"
-  end
-
-    builder.where <<SQL
-s.search_data @@ TO_TSQUERY(:locale, :query)
-      AND ft.deleted_at IS NULL
-      AND p.deleted_at IS NULL
-      AND ft.visible
-      AND ft.archetype <> '#{Archetype.private_message}'
-SQL
-
-    add_allowed_categories(builder, guardian)
-
-    builder.exec(args)
-  end
-
-  def self.add_allowed_categories(builder, guardian)
-    allowed_categories = nil
-    allowed_categories = guardian.secure_category_ids
-    if allowed_categories.present?
-      builder.where("(c.id IS NULL OR c.secure = 'f' OR c.id in (:category_ids))", category_ids: allowed_categories)
-    else
-      builder.where("(c.id IS NULL OR c.secure = 'f')")
-    end
-  end
-
-
-  def self.category_query(guardian, args)
-    builder = SqlBuilder.new <<SQL
-    SELECT 'category' AS type,
-            c.name AS id,
-            '/category/' || c.slug AS url,
-            c.name AS title,
-            NULL AS email,
-            c.color,
-            c.text_color
-    FROM categories AS c
-    JOIN categories_search s on s.id = c.id
-    /*where*/
-    ORDER BY topics_month desc
-    LIMIT :limit
-SQL
-
-    builder.where "s.search_data @@ TO_TSQUERY(:locale, :query)"
-    add_allowed_categories(builder,guardian)
-
-    builder.exec(args)
-
-  end
-
   def self.current_locale_long
     case I18n.locale         # Currently-present in /conf/locales/* only, sorry :-( Add as needed
       when :da then 'danish'
@@ -118,143 +23,241 @@ SQL
     end
   end
 
-  # If we're searching for a single topic
+  def initialize(term, opts=nil)
-  def self.single_topic(id, guardian)
+    @term = term.to_s if term.present?
-    topic = Topic.where(id: id).first
+    @opts = opts || {}
-    return nil unless guardian.can_see?(topic)
+    @guardian = @opts[:guardian] || Guardian.new
-
-    results = [{'type' => 'topic',
-                'id' => topic.id,
-                'url' => topic.relative_url,
-                'title' => topic.title }]
-    group_db_result(results, 'topic')
   end
 
   # Query a term
-  def self.query(term, guardian, type_filter=nil, min_search_term_length=3)
+  def execute
 
-    return nil if term.blank?
+    return nil if @term.blank?
 
-    # If no guardian supplied, assume anonymous
+    # really short terms are totally pointless
-    guardian ||= Guardian.new(nil)
+    return nil if @term.length < (@opts[:min_search_term_length] || SiteSetting.min_search_term_length)
-
-    term = term.to_s
 
     # If the term is a number or url to a topic, just include that topic
-    if type_filter == 'topic'
+    if @opts[:type_filter] == 'topic'
 
       begin
-        route = Rails.application.routes.recognize_path(term)
+        route = Rails.application.routes.recognize_path(@term)
-        return single_topic(route[:topic_id], guardian) if route[:topic_id].present?
+        return single_topic(route[:topic_id]) if route[:topic_id].present?
       rescue ActionController::RoutingError
       end
 
-      return single_topic(term.to_i, guardian) if term =~ /^\d+$/
+      return single_topic(@term.to_i) if @term =~ /^\d+$/
     end
 
     # We are stripping only symbols taking place in FTS and simply sanitizing the rest.
-    sanitized_term = PG::Connection.escape_string(term.gsub(/[:()&!]/,''))
+    @term = PG::Connection.escape_string(@term.gsub(/[:()&!]/,''))
-    query_string(sanitized_term, guardian, type_filter, min_search_term_length)
-  end
 
-  # Search for a string term
+    query_string
-  def self.query_string(term, guardian, type_filter, min_search_term_length)
-
-    # really short terms are totally pointless
-    return nil if term.length < min_search_term_length
-
-    args = {orig: term,
-            query: term.split.map {|t| "#{t}:*"}.join(" & "),
-            locale: current_locale_long}
-
-    if type_filter.present?
-      raise Discourse::InvalidAccess.new("invalid type filter") unless Search.facets.include?(type_filter)
-      args.merge!(limit: Search.per_facet * Search.facets.size)
-      db_result = case type_filter.to_s
-                  when 'topic'
-                    post_query(guardian, type_filter.to_sym, args)
-                  when 'category'
-                    category_query(guardian, args)
-                  else
-                    ActiveRecord::Base.exec_sql(Search.send("#{type_filter}_query_sql"), args)
-                  end
-    else
-      args.merge!(limit: (Search.per_facet + 1))
-      db_result = []
-      db_result += ActiveRecord::Base.exec_sql(user_query_sql, args).to_a
-      db_result += category_query(guardian, args).to_a
-      db_result += post_query(guardian, :topic, args).to_a
-    end
-
-    db_result = db_result.to_a
-
-    expected_topics = 0
-    expected_topics = Search.facets.size unless type_filter.present?
-    expected_topics = Search.per_facet * Search.facets.size if type_filter == 'topic'
-
-    if expected_topics > 0
-      db_result.each do |row|
-        expected_topics -= 1 if row['type'] == 'topic'
-      end
-    end
-
-    if expected_topics > 0
-      tmp = post_query(guardian, :post, args.merge(limit: expected_topics * 3)).to_a
-
-      topic_ids = Set.new db_result.map{|r| r["id"]}
-
-      tmp.reject! do |i|
-        if topic_ids.include?(i["id"])
-          true
-        else
-          topic_ids << i["id"]
-          false
-        end
-      end
-
-      db_result += tmp[0..expected_topics-1]
-    end
-
-    group_db_result(db_result, type_filter)
   end
 
   private
 
-  # Group the results by type
+    # Search for a string term
-  def self.group_db_result(db_result, type_filter)
+    def query_string
-    grouped = {}
-    db_result.each do |row|
-      type = row.delete('type')
 
-      # Add the slug for topics
+      args = {orig: @term,
-      if type == 'topic'
+              query: @term.split.map {|t| "#{t}:*"}.join(" & "),
-        new_slug = Slug.for(row['title'])
+              locale: Search.current_locale_long}
-        new_slug = "topic" if new_slug.blank?
+
-        row['url'].gsub!('slug', new_slug)
+      type_filter = @opts[:type_filter]
+
+      if type_filter.present?
+        raise Discourse::InvalidAccess.new("invalid type filter") unless Search.facets.include?(type_filter)
+        args.merge!(limit: Search.per_facet * Search.facets.size)
+        db_result = case type_filter.to_s
+                    when 'topic'
+                      post_query(type_filter.to_sym, args)
+                    when 'category'
+                      category_query(args)
+                    when 'user'
+                      user_query(args)
+                    end
+      else
+        args.merge!(limit: (Search.per_facet + 1))
+        db_result = []
+        db_result += user_query(args).to_a
+        db_result += category_query(args).to_a
+        db_result += post_query(:topic, args).to_a
       end
 
-      # Add avatars for users
+      db_result = db_result.to_a
-      row['avatar_template'] = User.avatar_template(row['email']) if type == 'user'
 
-      # Remove attributes when we know they don't matter
+      expected_topics = 0
-      row.delete('email')
+      expected_topics = Search.facets.size unless type_filter.present?
-      row.delete('color') unless type == 'category'
+      expected_topics = Search.per_facet * Search.facets.size if type_filter == 'topic'
-      row.delete('text_color') unless type == 'category'
 
-      grouped[type] ||= []
+      if expected_topics > 0
-      grouped[type] << row
+        db_result.each do |row|
+          expected_topics -= 1 if row['type'] == 'topic'
+        end
+      end
+
+      if expected_topics > 0
+        tmp = post_query(:post, args.merge(limit: expected_topics * 3)).to_a
+
+        topic_ids = Set.new db_result.map{|r| r["id"]}
+
+        tmp.reject! do |i|
+          if topic_ids.include?(i["id"])
+            true
+          else
+            topic_ids << i["id"]
+            false
+          end
+        end
+
+        db_result += tmp[0..expected_topics-1]
+      end
+
+      group_db_result(db_result)
     end
 
-    grouped.map do |type, results|
+
-      more = type_filter.blank? && (results.size > Search.per_facet)
+    # If we're searching for a single topic
-      results = results[0..([results.length, Search.per_facet].min - 1)] if type_filter.blank?
+    def single_topic(id)
-      {
+      topic = Topic.where(id: id).first
-        type: type,
+      return nil unless @guardian.can_see?(topic)
-        name: I18n.t("search.types.#{type}"),
+
-        more: more,
+      group_db_result([{'type' => 'topic',
-        results: results
+                        'id' => topic.id,
-      }
+                        'url' => topic.relative_url,
+                        'title' => topic.title }])
+    end
+
+    def add_allowed_categories(builder)
+      allowed_categories = nil
+      allowed_categories = @guardian.secure_category_ids
+      if allowed_categories.present?
+        builder.where("(c.id IS NULL OR c.secure OR c.id in (:category_ids))", category_ids: allowed_categories)
+      else
+        builder.where("(c.id IS NULL OR (NOT c.secure))")
+      end
+    end
+
+
+    def category_query(args)
+      builder = SqlBuilder.new <<SQL
+    SELECT 'category' AS type,
+            c.name AS id,
+            '/category/' || c.slug AS url,
+            c.name AS title,
+            NULL AS email,
+            c.color,
+            c.text_color
+    FROM categories AS c
+    JOIN categories_search s on s.id = c.id
+    /*where*/
+    ORDER BY topics_month desc
+    LIMIT :limit
+SQL
+
+      builder.where "s.search_data @@ TO_TSQUERY(:locale, :query)"
+      add_allowed_categories(builder)
+
+      builder.exec(args)
+    end
+
+    def user_query(args)
+      sql = "SELECT 'user' AS type,
+                    u.username_lower AS id,
+                    '/users/' || u.username_lower AS url,
+                    u.username AS title,
+                    u.email,
+                    NULL AS color,
+                    NULL AS text_color
+            FROM users AS u
+            JOIN users_search s on s.id = u.id
+            WHERE s.search_data @@ TO_TSQUERY(:locale, :query)
+            ORDER BY CASE WHEN u.username_lower = lower(:orig) then 0 else 1 end,  last_posted_at desc
+            LIMIT :limit"
+      ActiveRecord::Base.exec_sql(sql, args)
+    end
+
+    def post_query(type, args)
+      builder = SqlBuilder.new <<SQL
+      /*select*/
+      FROM topics AS ft
+      /*join*/
+        JOIN posts_search s on s.id = p.id
+        LEFT JOIN categories c ON c.id = ft.category_id
+      /*where*/
+      ORDER BY
+              TS_RANK_CD(TO_TSVECTOR(:locale, ft.title), TO_TSQUERY(:locale, :query)) desc,
+              TS_RANK_CD(search_data, TO_TSQUERY(:locale, :query)) desc,
+              bumped_at desc
+      LIMIT :limit
+SQL
+
+      builder.select "'topic' AS type"
+      builder.select("CAST(ft.id AS VARCHAR)")
+
+      if type == :topic
+        builder.select "'/t/slug/' || ft.id AS url"
+      else
+        builder.select "'/t/slug/' || ft.id || '/' || p.post_number AS url"
+      end
+
+      builder.select "ft.title, NULL AS email, NULL AS color, NULL AS text_color"
+
+      if type == :topic
+        builder.join "posts AS p ON p.topic_id = ft.id AND p.post_number = 1"
+      else
+        builder.join "posts AS p ON p.topic_id = ft.id AND p.post_number > 1"
+      end
+
+      builder.where <<SQL
+  s.search_data @@ TO_TSQUERY(:locale, :query)
+        AND ft.deleted_at IS NULL
+        AND p.deleted_at IS NULL
+        AND ft.visible
+        AND ft.archetype <> '#{Archetype.private_message}'
+SQL
+
+      add_allowed_categories(builder)
+
+      builder.exec(args)
+    end
+
+    # Group the results by type
+    def group_db_result(db_result)
+      grouped = {}
+      db_result.each do |row|
+        type = row.delete('type')
+
+        # Add the slug for topics
+        if type == 'topic'
+          new_slug = Slug.for(row['title'])
+          new_slug = "topic" if new_slug.blank?
+          row['url'].gsub!('slug', new_slug)
+        end
+
+        # Add avatars for users
+        row['avatar_template'] = User.avatar_template(row['email']) if type == 'user'
+
+        # Remove attributes when we know they don't matter
+        row.delete('email')
+        row.delete('color') unless type == 'category'
+        row.delete('text_color') unless type == 'category'
+
+        grouped[type] ||= []
+        grouped[type] << row
+      end
+
+      grouped.map do |type, results|
+        more = @opts[:type_filter].blank? && (results.size > Search.per_facet)
+        results = results[0..([results.length, Search.per_facet].min - 1)] if @opts[:type_filter].blank?
+        {
+          type: type,
+          name: I18n.t("search.types.#{type}"),
+          more: more,
+          results: results
+        }
+      end
     end
-  end
 
 end

spec/components/search_spec.rb

@@ -72,25 +72,25 @@ describe Search do
 
   it 'returns something blank on a nil search' do
     ActiveRecord::Base.expects(:exec_sql).never
-    Search.query(nil,nil).should be_blank
+    Search.new(nil).execute.should be_blank
   end
 
   it 'does not search when the search term is too small' do
     ActiveRecord::Base.expects(:exec_sql).never
-    Search.query('evil', nil,  nil, 5).should be_blank
+    Search.new('evil', min_search_term_length: 5).execute.should be_blank
   end
 
   it 'escapes non alphanumeric characters' do
-    Search.query('foo :!$);}]>@\#\"\'', nil).should be_blank # There are at least three levels of sanitation for Search.query!
+    Search.new('foo :!$);}]>@\#\"\'').execute.should be_blank # There are at least three levels of sanitation for Search.query!
   end
 
   it 'works when given two terms with spaces' do
-    lambda { Search.query('evil trout', nil) }.should_not raise_error
+    lambda { Search.new('evil trout').execute }.should_not raise_error
   end
 
   context 'users' do
     let!(:user) { Fabricate(:user) }
-    let(:result) { first_of_type(Search.query('bruce', nil), 'user') }
+    let(:result) { first_of_type( Search.new('bruce', type_filter: 'user').execute, 'user') }
 
     it 'returns a result' do
       result.should be_present
@@ -115,7 +115,7 @@ describe Search do
 
     context 'searching the OP' do
       let!(:post) { Fabricate(:post, topic: topic, user: topic.user) }
-      let(:result) { first_of_type(Search.query('hello', nil), 'topic') }
+      let(:result) { first_of_type(Search.new('hello', type_filter: 'topic').execute, 'topic') }
 
       it 'returns a result correctly' do
         result.should be_present
@@ -125,7 +125,7 @@ describe Search do
     end
 
     context "search for a topic by id" do
-      let(:result) { first_of_type(Search.query(topic.id, nil, 'topic'), 'topic') }
+      let(:result) { first_of_type(Search.new(topic.id, type_filter: 'topic').execute, 'topic') }
 
       it 'returns the topic' do
         result.should be_present
@@ -135,7 +135,7 @@ describe Search do
     end
 
     context "search for a topic by url" do
-      let(:result) { first_of_type(Search.query(topic.relative_url, nil, 'topic'), 'topic') }
+      let(:result) { first_of_type(Search.new(topic.relative_url, type_filter: 'topic').execute, 'topic') }
 
       it 'returns the topic' do
         result.should be_present
@@ -147,7 +147,7 @@ describe Search do
     context 'security' do
       let!(:post) { Fabricate(:post, topic: topic, user: topic.user) }
       def result(current_user)
-        first_of_type(Search.query('hello', current_user), 'topic')
+        first_of_type(Search.new('hello', guardian: current_user).execute, 'topic')
       end
 
       it 'secures results correctly' do
@@ -176,7 +176,7 @@ describe Search do
                                               end
     }
     let!(:post) {Fabricate(:post, topic: cyrillic_topic, user: cyrillic_topic.user)}
-    let(:result) { first_of_type(Search.query('запись',nil), 'topic') }
+    let(:result) { first_of_type(Search.new('запись').execute, 'topic') }
 
     it 'finds something when given cyrillic query' do
       result.should be_present
@@ -187,7 +187,7 @@ describe Search do
 
     let!(:category) { Fabricate(:category) }
     def result
-      first_of_type(Search.query('amazing', nil), 'category')
+      first_of_type(Search.new('amazing').execute, 'category')
     end
 
     it 'returns the correct result' do
@@ -212,7 +212,7 @@ describe Search do
 
 
     context 'user filter' do
-      let(:results) { Search.query('amazing', nil, 'user') }
+      let(:results) { Search.new('amazing', type_filter: 'user').execute }
 
       it "returns a user result" do
         results.detect {|r| r[:type] == 'user'}.should be_present
@@ -222,7 +222,7 @@ describe Search do
     end
 
     context 'category filter' do
-      let(:results) { Search.query('amazing', nil, 'category') }
+      let(:results) { Search.new('amazing', type_filter: 'category').execute }
 
       it "returns a category result" do
         results.detect {|r| r[:type] == 'user'}.should be_blank

spec/controllers/search_controller_spec.rb

@@ -3,16 +3,24 @@ require 'spec_helper'
 describe SearchController do
 
   it 'performs the query' do
-    m = Guardian.new(nil)
+    guardian = Guardian.new
-    Guardian.stubs(:new).returns(m)
+    Guardian.stubs(:new).returns(guardian)
-    Search.expects(:query).with('test', m, nil, 3)
+
+    search = mock()
+    Search.expects(:new).with('test', guardian: guardian, type_filter: nil).returns(search)
+    search.expects(:execute)
+
     xhr :get, :query, term: 'test'
   end
 
   it 'performs the query with a filter' do
-    m = Guardian.new(nil)
+    guardian = Guardian.new
-    Guardian.stubs(:new).returns(m)
+    Guardian.stubs(:new).returns(guardian)
-    Search.expects(:query).with('test', m, 'topic', 3)
+
+    search = mock()
+    Search.expects(:new).with('test', guardian: guardian, type_filter: 'topic').returns(search)
+    search.expects(:execute)
+
     xhr :get, :query, term: 'test', type_filter: 'topic'
   end