Refactor search code

This contains several improvements and bug-fixes:

* Take advantage of indexing changes in ead2f5e3 to reduce the number of
  round trips to execute a search
* Use Tire scan to include all matching threads
* Execute final sort/pagination in Mongo
* Share sorting code
* Eliminate separate presenter for search results (which removes unused
  highlighting functionality)

JIRA: FOR-607

api/search.rb

 require 'new_relic/agent/method_tracer'
 
 get "#{APIPREFIX}/search/threads" do
+  local_params = params # Necessary for params to be available inside blocks
+  sort_criteria = get_sort_criteria(local_params)
 
-  sort_key_mapper = {
-    "date" => :created_at,
-    "activity" => :last_activity_at,
-    "votes" => :votes_point,
-    "comments" => :comment_count,
-  }
-
-  sort_order_mapper = {
-    "desc" => :desc,
-    "asc" => :asc,
-  }
-
-  sort_key = sort_key_mapper[params["sort_key"]]
-  sort_order = sort_order_mapper[params["sort_order"]]
-
-  sort_keyword_valid = (!params["sort_key"] && !params["sort_order"] || sort_key && sort_order)
-
-  if !params["text"] || !sort_keyword_valid
+  if !local_params["text"] || !sort_criteria
     {}.to_json
   else
-    page = (params["page"] || DEFAULT_PAGE).to_i
-    per_page = (params["per_page"] || DEFAULT_PER_PAGE).to_i
-    # for multi commentable searching
-    params["commentable_ids"] = params["commentable_ids"].split(',') if params["commentable_ids"]
-    options = {
-      sort_key: sort_key,
-      sort_order: sort_order,
-      page: page,
-      per_page: per_page,
-    }
-
-    result_hash = CommentThread.perform_search(params, options)
-    results = result_hash[:results]
-    total_results = result_hash[:total_results]
+    page = (local_params["page"] || DEFAULT_PAGE).to_i
+    per_page = (local_params["per_page"] || DEFAULT_PER_PAGE).to_i
+
+    # Because threads and comments are currently separate unrelated documents in
+    # Elasticsearch, we must first query for all matching documents, then
+    # extract the set of thread ids, and then sort the threads by the specified
+    # criteria and paginate. For performance reasons, we currently limit the
+    # number of documents considered (ordered by update recency), which means
+    # that matching threads can be missed if the search terms are very common.
+
+    thread_ids = Set.new
+    self.class.trace_execution_scoped(["Custom/get_search_threads/es_search"]) do
+      search = Tire.search Content::ES_INDEX_NAME do
+        query do
+          match [:title, :body], local_params["text"]
+          filtered do
+            filter :term, :commentable_id => local_params["commentable_id"] if local_params["commentable_id"]
+            filter :terms, :commentable_id => local_params["commentable_ids"].split(",") if local_params["commentable_ids"]
+            filter :term, :course_id => local_params["course_id"] if local_params["course_id"]
+            if local_params["group_id"]
+              filter :or, [
+                {:not => {:exists => {:field => :group_id}}},
+                {:term => {:group_id => local_params["group_id"]}}
+              ]
+            end
+          end
+        end
+        sort do
+          by "updated_at", "desc"
+        end
+        size CommentService.config["max_deep_search_comment_count"].to_i
+      end
+      search.results.each do |content|
+        case content.type
+        when "comment_thread"
+          thread_ids.add(content.id)
+        when "comment"
+          thread_ids.add(content.comment_thread_id)
+        end
+      end
+    end
 
-    if page > results.total_pages #TODO find a better way for this
-      result_hash = CommentThread.perform_search(params, options.merge(page: results.total_pages))
-      results = result_hash[:results]
-      total_results = result_hash[:total_results]
+    results = nil
+    self.class.trace_execution_scoped(["Custom/get_search_threads/mongo_sort_page"]) do
+      results = CommentThread.
+        where(:id.in => thread_ids.to_a).
+        order_by(sort_criteria).
+        page(page).
+        per(per_page).
+        to_a
     end
+    total_results = thread_ids.size
+    num_pages = (total_results + per_page - 1) / per_page
 
     if results.length == 0
       collection = []
     else
-      pres_threads = ThreadSearchResultsPresenter.new(
+      pres_threads = ThreadListPresenter.new(
         results,
-        params[:user_id] ? user : nil,
-        params[:course_id] || results.first.course_id
+        local_params[:user_id] ? user : nil,
+        local_params[:course_id] || results.first.course_id
       )
       collection = pres_threads.to_hash
     end
 
-    num_pages = results.total_pages
-    page = [num_pages, [1, page].max].min
     json_output = nil
     self.class.trace_execution_scoped(['Custom/get_search_threads/json_serialize']) do
       json_output = {

lib/helpers.rb

@@ -142,34 +142,15 @@ helpers do
       comment_threads = comment_threads.in(commentable_id: params[:commentable_ids].split(","))
     end
 
-    sort_key_mapper = {
-      "date" => :created_at,
-      "activity" => :last_activity_at,
-      "votes" => :"votes.point",
-      "comments" => :comment_count,
-    }
-
-    sort_order_mapper = {
-      "desc" => :desc,
-      "asc" => :asc,
-    }
+    sort_criteria = get_sort_criteria(params)
 
-    sort_key = sort_key_mapper[params["sort_key"] || "date"]
-    sort_order = sort_order_mapper[params["sort_order"] || "desc"] 
-    sort_keyword_valid = (!params["sort_key"] && !params["sort_order"] || sort_key && sort_order)
-
-    if not sort_keyword_valid
+    if not sort_criteria
       {}.to_json
     else
       page = (params["page"] || DEFAULT_PAGE).to_i
       per_page = (params["per_page"] || DEFAULT_PER_PAGE).to_i
 
-      order_clause = "pinned DESC, #{sort_key} #{sort_order}"
-      if ![:created_at, :last_activity_at].include? sort_key
-        # make sort order predictable when preceding sorts are non-unique
-        order_clause = "#{order_clause}, created_at DESC"
-      end
-      comment_threads = comment_threads.order_by(order_clause)
+      comment_threads = comment_threads.order_by(sort_criteria)
       num_pages = [1, (comment_threads.count / per_page.to_f).ceil].max
       page = [num_pages, [1, page].max].min
       # actual query happens here (by doing to_a)
@@ -198,6 +179,35 @@ helpers do
     end
   end
 
+  # Given query params, return sort criteria appropriate for passing to the
+  # order_by function of a Mongoid query. Returns nil if params are not valid.
+  def get_sort_criteria(params)
+    sort_key_mapper = {
+      "date" => :created_at,
+      "activity" => :last_activity_at,
+      "votes" => :"votes.point",
+      "comments" => :comment_count,
+    }
+
+    sort_order_mapper = {
+      "desc" => :desc,
+      "asc" => :asc,
+    }
+
+    sort_key = sort_key_mapper[params["sort_key"] || "date"]
+    sort_order = sort_order_mapper[params["sort_order"] || "desc"]
+
+    if sort_key && sort_order
+      sort_criteria = [[:pinned, :desc], [sort_key, sort_order]]
+      if ![:created_at, :last_activity_at].include? sort_key
+        sort_criteria << [:created_at, :desc]
+      end
+      sort_criteria
+    else
+      nil
+    end
+  end
+
   def notifications_by_date_range_and_user_ids start_date_time, end_date_time, user_ids  
     #given a date range and a user, find all of the notifiable content
     #key by thread id, and return notification messages for each user

models/comment_thread.rb

@@ -76,107 +76,6 @@ class CommentThread < Content
     c
   end
 
-  def self.perform_search(params, options={})
-    
-    page = [1, options[:page] || 1].max
-    per_page = options[:per_page] || 20
-    sort_key = options[:sort_key]
-    sort_order = options[:sort_order]
-    
-
-#GET /api/v1/search/threads?user_id=1&recursive=False&sort_key=date&│[2013-06-28 10:16:46,104][INFO ][plugins                  ] [Glamor] loaded [], sites []                  
-#text=response&sort_order=desc&course_id=HarvardX%2FHLS1xD%2FCopyright&per_page=20&api_key=PUT_YOUR_API_KE│T1GYWxzZSZzb3J0X2tleT1kYXRlJnRleHQ9cmVzcG9uc2Umc29ydF9vcmRlcj1kZXNjJmNvdXJzZV9pZA==: initialized          
-#Y_HERE&page=1
-
-    #KChugh - Unfortunately, there's no algorithmically nice way to handle pagination with
-    #stitching together Comments and CommentThreads, because there is no determinstic relationship
-    #between the ordinality of comments and threads. 
-    #the best solution is to find all of the thread ids for matching comment hits, and union them
-    #with the comment thread query, however, Tire does not support ORing a query key with a term filter
-    
-    #so the 3rd best solution is to run two Tire searches (3 actually, one to query the comments, one to query the threads based on
-    #thread ids and the original thread search) and merge the results, uniqifying the results in the process.
-      
-    #so first, find the comment threads associated with comments that hit the query
-        
-    search = Tire::Search::Search.new Content::ES_INDEX_NAME
-
-    search.query {|query| query.match [:title, :body], params["text"]} if params["text"]
-    search.highlight({title: { number_of_fragments: 0 } } , {body: { number_of_fragments: 0 } }, options: { tag: "<highlight>" })
-    search.filter(:type, value: 'comment_thread')
-    search.filter(:term, commentable_id: params["commentable_id"]) if params["commentable_id"]
-    search.filter(:terms, commentable_id: params["commentable_ids"]) if params["commentable_ids"]
-    search.filter(:term, course_id: params["course_id"]) if params["course_id"]
-
-    if params["group_id"]
-      search.filter :or, [
-        {:not => {:exists => {:field => :group_id}}},
-        {:term => {:group_id => params["group_id"]}}
-      ]
-    end
-
-    search.sort {|sort| sort.by sort_key, sort_order} if sort_key && sort_order #TODO should have search option 'auto sort or sth'
-
-    #again, b/c there is no relationship in ordinality, we cannot paginate if it's a text query
-    results = search.results
-
-    search = Tire::Search::Search.new Content::ES_INDEX_NAME
-    search.query {|query| query.match :body, params["text"]} if params["text"]
-    search.filter(:type, value: 'comment')
-    search.filter(:term, course_id: params["course_id"]) if params["course_id"]
-    search.size CommentService.config["max_deep_search_comment_count"].to_i
-
-    #unforutnately, we cannot paginate here, b/c we don't know how the ordinality is totally
-    #unrelated to that of threads
-
-    c_results = comment_ids = comments = thread_ids = nil
-    self.class.trace_execution_scoped(['Custom/perform_search/collect_comment_search_results']) do
-      c_results = search.results
-      comment_ids = c_results.collect{|c| c.id}.uniq
-    end
-    self.class.trace_execution_scoped(['Custom/perform_search/collect_comment_thread_ids']) do
-      comments = Comment.where(:id.in => comment_ids)
-      thread_ids = comments.collect{|c| c.comment_thread_id.to_s}
-    end
-
-    #thread_ids = c_results.collect{|c| c.comment_thread_id}
-    #as soon as we can add comment thread id to the ES index, via Tire updgrade, we'll 
-    #use ES instead of mongo to collect the thread ids
-
-    #use the elasticsearch index instead to avoid DB hit
-
-    self.class.trace_execution_scoped(['Custom/perform_search/collect_unique_thread_ids']) do
-      original_thread_ids = results.collect{|r| r.id}
-
-      #now add the original search thread ids
-      thread_ids += original_thread_ids
-
-      thread_ids = thread_ids.uniq
-    end
-    
-    #now run one more search to harvest the threads and filter by group
-    search = Tire::Search::Search.new Content::ES_INDEX_NAME
-    search.filter(:type, value: 'comment_thread')
-    search.filter(:terms, :thread_id => thread_ids)
-    search.filter(:terms, commentable_id: params["commentable_ids"]) if params["commentable_ids"]
-    search.filter(:term, course_id: params["course_id"]) if params["course_id"]
-
-    search.size per_page
-    search.from per_page * (page - 1)
-
-    if params["group_id"]
-
-      search.filter :or, [
-        {:not => {:exists => {:field => :group_id}}},
-        {:term => {:group_id => params["group_id"]}}
-
-      ]
-    end
-
-    search.sort {|sort| sort.by sort_key, sort_order} if sort_key && sort_order 
-    {results: search.results, total_results: thread_ids.length}
-  end
-
   def activity_since(from_time=nil)
     if from_time
       activities.where(:created_at => {:$gte => from_time})
@@ -268,9 +167,4 @@ private
     subscriptions.delete_all
   end
 
-  class << self
-    include ::NewRelic::Agent::MethodTracer
-    add_method_tracer :perform_search, 'Custom/perform_search'
-  end
-
 end

presenters/thread_search_results.rb

-require_relative 'thread_list'
-
-class ThreadSearchResultsPresenter < ThreadListPresenter
-
-  alias :super_to_hash :to_hash
-
-  def initialize(search_results, user, course_id)
-    @search_result_map = Hash[search_results.map { |t| [t.id, t] }]
-    threads = CommentThread.where(id: {"$in" => @search_result_map.keys}).to_a
-    # reorder fetched threads to match the original result order
-    threads = Hash[threads.map { |t| [t._id.to_s, t] }].values_at *search_results.map { |t| t.id }
-    super(threads, user, course_id)
-  end
-
-  def to_hash
-    super_to_hash.each do |thread_hash|
-      thread_key = thread_hash['id'].to_s
-      highlight = @search_result_map[thread_key].highlight || {}
-      thread_hash["highlighted_body"] = (highlight[:body] || []).first || thread_hash["body"]
-      thread_hash["highlighted_title"] = (highlight[:title] || []).first || thread_hash["title"]
-    end
-  end
-
-end

spec/api/query_spec.rb

@@ -21,7 +21,7 @@ describe "app" do
         get "/api/v1/search/threads", text: random_string
         last_response.should be_ok
         threads = parse(last_response.body)['collection']
-        check_thread_result_json(nil, thread, threads.select{|t| t["id"] == thread.id.to_s}.first, true)
+        check_thread_result_json(nil, thread, threads.select{|t| t["id"] == thread.id.to_s}.first)
       end
 
     end
@@ -50,7 +50,7 @@ describe "app" do
         get "/api/v1/search/threads", text: random_string
         last_response.should be_ok
         threads = parse(last_response.body)['collection']
-        check_thread_result_json(nil, thread, threads.select{|t| t["id"] == thread.id.to_s}.first, true)
+        check_thread_result_json(nil, thread, threads.select{|t| t["id"] == thread.id.to_s}.first)
       end
     end
   end

spec/api/search_spec.rb

@@ -8,7 +8,162 @@ describe "app" do
 
     let(:author) { create_test_user(42) }
 
+    let(:course_id) { "test/course/id" }
+
+    def get_result_ids(result)
+      result["collection"].map {|t| t["id"]}
+    end
+
     describe "GET /api/v1/search/threads" do
+      def assert_empty_response
+        last_response.should be_ok
+        result = parse(last_response.body)
+        result.should == {}
+      end
+
+      it "returns an empty reuslt if text parameter is missing" do
+        get "/api/v1/search/threads", course_id: course_id
+        assert_empty_response
+      end
+
+      it "returns an empty reuslt if sort key is invalid" do
+        get "/api/v1/search/threads", course_id: course_id, text: "foobar", sort_key: "invalid", sort_order: "desc"
+        assert_empty_response
+      end
+
+      it "returns an empty reuslt if sort order is invalid" do
+        get "/api/v1/search/threads", course_id: course_id, text: "foobar", sort_key: "date", sort_order: "invalid"
+        assert_empty_response
+      end
+
+      describe "filtering works" do
+        let!(:threads) do
+          threads = (0..29).map do |i|
+            thread = make_thread(author, "text", course_id + (i % 2).to_s, "commentable" + (i % 3).to_s)
+            if i % 5 != 0
+              thread.group_id = i % 5
+              thread.save!
+            end
+            thread
+          end
+          refresh_es_index
+          threads
+        end
+
+        def assert_response_contains(expected_thread_indexes)
+          last_response.should be_ok
+          result = parse(last_response.body)
+          actual_ids = Set.new get_result_ids(result)
+          expected_ids = Set.new expected_thread_indexes.map {|i| threads[i].id.to_s}
+          actual_ids.should == expected_ids
+        end
+
+        it "by course_id" do
+          get "/api/v1/search/threads", text: "text", course_id: "test/course/id0"
+          assert_response_contains((0..29).find_all {|i| i % 2 == 0})
+        end
+
+        it "by commentable_id" do
+          get "/api/v1/search/threads", text: "text", commentable_id: "commentable0"
+          assert_response_contains((0..29).find_all {|i| i % 3 == 0})
+        end
+
+        it "by commentable_ids" do
+          get "/api/v1/search/threads", text: "text", commentable_ids: "commentable0,commentable1"
+          assert_response_contains((0..29).find_all {|i| i % 3 == 0 || i % 3 == 1})
+        end
+
+        it "by group_id" do
+          get "/api/v1/search/threads", text: "text", group_id: "1"
+          assert_response_contains((0..29).find_all {|i| i % 5 == 0 || i % 5 == 1})
+        end
+
+        it "by all filters combined" do
+          get "/api/v1/search/threads", text: "text", course_id: "test/course/id0", commentable_id: "commentable0", group_id: "1"
+          assert_response_contains([0, 6])
+        end
+      end
+
+      describe "sorting works" do
+        let!(:threads) do
+          threads = (0..5).map {|i| make_thread(author, "text", course_id, "dummy")}
+          [1, 2].map {|i| author.vote(threads[i], :up)}
+          [1, 3].map do |i|
+            threads[i].comment_count = 5
+            threads[i].save!
+          end
+          threads[4].save!
+          refresh_es_index
+          threads
+        end
+
+        def check_sort(sort_key, sort_order, expected_thread_indexes)
+          get "/api/v1/search/threads", text: "text", course_id: course_id, sort_key: sort_key, sort_order: sort_order
+          last_response.should be_ok
+          result = parse(last_response.body)
+          actual_ids = get_result_ids(result)
+          expected_ids = expected_thread_indexes.map {|i| threads[i].id.to_s}
+          actual_ids.should == expected_ids
+        end
+
+        it "by date" do
+          asc_order = [0, 1, 2, 3, 4, 5]
+          check_sort("date", "asc", asc_order)
+          check_sort("date", "desc", asc_order.reverse)
+        end
+
+        it "by activity" do
+          asc_order = [0, 2, 5, 1, 3, 4]
+          check_sort("activity", "asc", asc_order)
+          check_sort("activity", "desc", asc_order.reverse)
+        end
+
+        it "by votes" do
+          check_sort("votes", "asc", [5, 4, 3, 0, 2, 1])
+          check_sort("votes", "desc", [2, 1, 5, 4, 3, 0])
+        end
+
+        it "by comments" do
+          check_sort("comments", "asc", [5, 4, 2, 0, 3, 1])
+          check_sort("comments", "desc", [3, 1, 5, 4, 2, 0])
+        end
+
+        it "by default" do
+          check_sort(nil, nil, [5, 4, 3, 2, 1, 0])
+        end
+      end
+
+      describe "pagination" do
+        let!(:threads) do
+          threads = (1..50).map {|i| make_thread(author, "text", course_id, "dummy")}
+          refresh_es_index
+          threads
+        end
+
+        def check_pagination(per_page, num_pages)
+          result_ids = []
+          (1..(num_pages + 1)).each do |i| # Go past the end to make sure non-existent pages are empty
+            get "/api/v1/search/threads", text: "text", page: i, per_page: per_page
+            last_response.should be_ok
+            result = parse(last_response.body)
+            result_ids += get_result_ids(result)
+          end
+          result_ids.should == threads.reverse.map {|t| t.id.to_s}
+        end
+
+        it "works correctly with page size 1" do
+          check_pagination(1, 50)
+        end
+
+        it "works correctly with page size 30" do
+          check_pagination(30, 2)
+        end
+
+        it "works correctly with default page size" do
+          check_pagination(nil, 3)
+        end
+      end
+
       it "returns the correct values for total_results and num_pages" do
         course_id = "test/course/id"
         for i in 1..100 do
@@ -55,7 +210,7 @@ describe "app" do
         last_response.should be_ok
         result = parse(last_response.body)["collection"]
         result.length.should == 1
-        check_thread_result_json(nil, thread, result.first, true)
+        check_thread_result_json(nil, thread, result.first)
       end
 
       include_examples "unicode data"

spec/presenters/thread_search_result_spec.rb

-require 'spec_helper'
-
-describe ThreadSearchResultsPresenter do
-  context "#to_hash" do
-  
-    before(:each) { setup_10_threads }
-
-    # NOTE: throrough coverage of search result hash structure is presently provided in spec/presenters/thread_spec
-    def check_search_result_hash(search_result, hash)
-      hash["highlighted_body"].should == ((search_result.highlight[:body] || []).first || hash["body"])
-      hash["highlighted_title"].should == ((search_result.highlight[:title] || []).first || hash["title"])
-    end
-
-    def check_search_results_hash(search_results, hashes)
-      expected_order = search_results.map {|t| t.id}
-      actual_order = hashes.map {|h| h["id"].to_s}
-      actual_order.should == expected_order
-      hashes.each_with_index { |hash, i| check_search_result_hash(search_results[i], hash) }
-    end
-
-    it "presents search results in correct order" do
-      threads_random_order = @threads.values.shuffle
-      mock_results = threads_random_order.map do |t| 
-        double(Tire::Results::Item, :id => t._id.to_s, :highlight => {:body => ["foo"], :title => ["bar"]})
-      end
-      pres = ThreadSearchResultsPresenter.new(mock_results, nil, DFLT_COURSE_ID)
-      check_search_results_hash(mock_results, pres.to_hash)
-    end
-
-    it "presents search results with correct default highlights" do
-      threads_random_order = @threads.values.shuffle
-      mock_results = threads_random_order.map do |t| 
-        double(Tire::Results::Item, :id => t._id.to_s, :highlight => {})
-      end
-      pres = ThreadSearchResultsPresenter.new(mock_results, nil, DFLT_COURSE_ID)
-      check_search_results_hash(mock_results, pres.to_hash)
-    end
-
-  end
-end

spec/spec_helper.rb

@@ -201,14 +201,11 @@ end
 
 # this method is used to test results produced using the helper function handle_threads_query
 # which is used in multiple areas of the API
-def check_thread_result(user, thread, hash, is_search=false, is_json=false)
+def check_thread_result(user, thread, hash, is_json=false)
   expected_keys = %w(id title body course_id commentable_id created_at updated_at)
   expected_keys += %w(anonymous anonymous_to_peers at_position_list closed user_id)
   expected_keys += %w(username votes abuse_flaggers tags type group_id pinned)
   expected_keys += %w(comments_count unread_comments_count read endorsed)
-  if is_search
-    expected_keys += %w(highlighted_body highlighted_title)
-  end
   # these keys are checked separately, when desired, using check_thread_response_paging.
   actual_keys = hash.keys - ["children", "resp_skip", "resp_limit", "resp_total"]
   actual_keys.sort.should == expected_keys.sort
@@ -267,8 +264,8 @@ def check_thread_result(user, thread, hash, is_search=false, is_json=false)
   end
 end
 
-def check_thread_result_json(user, thread, json_response, is_search=false)
-  check_thread_result(user, thread, json_response, is_search, true)
+def check_thread_result_json(user, thread, json_response)
+  check_thread_result(user, thread, json_response, true)
 end
 
 def check_thread_response_paging(thread, hash, resp_skip=0, resp_limit=nil, is_json=false)