Project

General

Profile

« Previous | Next » 

Revision 13739

Rewrites search engine to properly paginate results (#18631).

Instead of counting and retrieving results based on their timestamps, we now load all result ids then load the appropriate results by their ids. This also brings a 2x performance improvement as we search tokens in one of the 2 queries only.

View differences:

trunk/app/controllers/search_controller.rb
36 36
        @project
37 37
      end
38 38

  
39
    offset = nil
40
    begin; offset = params[:offset].to_time if params[:offset]; rescue; end
41

  
42 39
    # quick jump to an issue
43 40
    if (m = @question.match(/^#?(\d+)$/)) && (issue = Issue.visible.find_by_id(m[1].to_i))
44 41
      redirect_to issue_path(issue)
......
66 63
      # no more than 5 tokens to search for
67 64
      @tokens.slice! 5..-1 if @tokens.size > 5
68 65

  
69
      @results = []
70
      @results_by_type = Hash.new {|h,k| h[k] = 0}
66
      limit = 10
71 67

  
72
      limit = 10
73
      @scope.each do |s|
74
        r, c = s.singularize.camelcase.constantize.search(@tokens, projects_to_search,
68
      @result_count = 0
69
      @result_count_by_type = Hash.new {|h,k| h[k] = 0}
70
      ranks_and_ids = []
71

  
72
      # get all the results ranks and ids
73
      @scope.each do |scope|
74
        klass = scope.singularize.camelcase.constantize
75
        ranks_and_ids_in_scope = klass.search_result_ranks_and_ids(@tokens, User.current, projects_to_search,
75 76
          :all_words => @all_words,
76
          :titles_only => @titles_only,
77
          :limit => (limit+1),
78
          :offset => offset,
79
          :before => params[:previous].nil?)
80
        @results += r
81
        @results_by_type[s] += c
77
          :titles_only => @titles_only
78
        )
79
        @result_count_by_type[scope] += ranks_and_ids_in_scope.size
80
        @result_count += ranks_and_ids_in_scope.size
81
        ranks_and_ids += ranks_and_ids_in_scope.map {|r| [scope, r]}
82 82
      end
83
      @results = @results.sort {|a,b| b.event_datetime <=> a.event_datetime}
84
      if params[:previous].nil?
85
        @pagination_previous_date = @results[0].event_datetime if offset && @results[0]
86
        if @results.size > limit
87
          @pagination_next_date = @results[limit-1].event_datetime
88
          @results = @results[0, limit]
89
        end
90
      else
91
        @pagination_next_date = @results[-1].event_datetime if offset && @results[-1]
92
        if @results.size > limit
93
          @pagination_previous_date = @results[-(limit)].event_datetime
94
          @results = @results[-(limit), limit]
95
        end
83
      @result_pages = Paginator.new @result_count, limit, params['page']
84

  
85
      # sort results, higher rank and id first
86
      ranks_and_ids.sort! {|a,b| b.last <=> a.last }
87
      ranks_and_ids = ranks_and_ids[@result_pages.offset, limit] || []
88

  
89
      # load the results to display
90
      results_by_scope = Hash.new {|h,k| h[k] = []}
91
      ranks_and_ids.group_by(&:first).each do |scope, rs|
92
        klass = scope.singularize.camelcase.constantize
93
        results_by_scope[scope] += klass.search_results_from_ids(rs.map(&:last).map(&:last))
96 94
      end
95

  
96
      @results = ranks_and_ids.map do |scope, r|
97
        results_by_scope[scope].detect {|record| record.id == r.last}
98
      end.compact
97 99
    else
98 100
      @question = ""
99 101
    end
trunk/app/models/changeset.rb
35 35
                :url => Proc.new {|o| {:controller => 'repositories', :action => 'revision', :id => o.repository.project, :repository_id => o.repository.identifier_param, :rev => o.identifier}}
36 36

  
37 37
  acts_as_searchable :columns => 'comments',
38
                     :scope => preload(:repository => :project),
38
                     :preload => {:repository => :project},
39 39
                     :project_key => "#{Repository.table_name}.project_id",
40
                     :date_column => "#{Changeset.table_name}.committed_on"
40
                     :date_column => :committed_on
41 41

  
42 42
  acts_as_activity_provider :timestamp => "#{table_name}.committed_on",
43 43
                            :author_key => :user_id,
trunk/app/models/document.rb
22 22
  acts_as_attachable :delete_permission => :delete_documents
23 23

  
24 24
  acts_as_searchable :columns => ['title', "#{table_name}.description"],
25
                     :scope => preload(:project)
25
                     :preload => :project
26 26
  acts_as_event :title => Proc.new {|o| "#{l(:label_document)}: #{o.title}"},
27 27
                :author => Proc.new {|o| o.attachments.reorder("#{Attachment.table_name}.created_on ASC").first.try(:author) },
28 28
                :url => Proc.new {|o| {:controller => 'documents', :action => 'show', :id => o.id}}
trunk/app/models/issue.rb
46 46
  acts_as_customizable
47 47
  acts_as_watchable
48 48
  acts_as_searchable :columns => ['subject', "#{table_name}.description", "#{Journal.table_name}.notes"],
49
                     # sort by id so that limited eager loading doesn't break with postgresql
50
                     :order_column => "#{table_name}.id",
49
                     :preload => [:project, :status, :tracker],
51 50
                     :scope => lambda { joins(:project).
52 51
                                        joins("LEFT OUTER JOIN #{Journal.table_name} ON #{Journal.table_name}.journalized_type='Issue'" + 
53 52
                                              " AND #{Journal.table_name}.journalized_id = #{Issue.table_name}.id" +
trunk/app/models/message.rb
25 25
  attr_protected :id
26 26

  
27 27
  acts_as_searchable :columns => ['subject', 'content'],
28
                     :scope => preload(:board => :project),
29
                     :project_key => "#{Board.table_name}.project_id",
30
                     :date_column => "#{table_name}.created_on"
28
                     :preload => {:board => :project},
29
                     :project_key => "#{Board.table_name}.project_id"
30

  
31 31
  acts_as_event :title => Proc.new {|o| "#{o.board.name}: #{o.subject}"},
32 32
                :description => :content,
33 33
                :group => :parent,
trunk/app/models/news.rb
29 29
  acts_as_attachable :edit_permission => :manage_news,
30 30
                     :delete_permission => :manage_news
31 31
  acts_as_searchable :columns => ['title', 'summary', "#{table_name}.description"],
32
                     :scope => preload(:project)
32
                     :preload => :project
33 33
  acts_as_event :url => Proc.new {|o| {:controller => 'news', :action => 'show', :id => o.id}}
34 34
  acts_as_activity_provider :scope => preload(:project, :author),
35 35
                            :author_key => :author_id
trunk/app/models/wiki_page.rb
33 33
                :url => Proc.new {|o| {:controller => 'wiki', :action => 'show', :project_id => o.wiki.project, :id => o.title}}
34 34

  
35 35
  acts_as_searchable :columns => ['title', "#{WikiContent.table_name}.text"],
36
                     :scope => preload(:wiki => :project).joins(:content, {:wiki => :project}),
36
                     :scope => joins(:content, {:wiki => :project}),
37
                     :preload => {:wiki => :project},
37 38
                     :permission => :view_wiki_pages,
38 39
                     :project_key => "#{Wiki.table_name}.project_id"
39 40

  
trunk/app/views/search/index.html.erb
23 23

  
24 24
<% if @results %>
25 25
    <div id="search-results-counts">
26
      <%= render_results_by_type(@results_by_type) unless @scope.size == 1 %>
26
      <%= render_results_by_type(@result_count_by_type) unless @scope.size == 1 %>
27 27
    </div>
28
    <h3><%= l(:label_result_plural) %> (<%= @results_by_type.values.sum %>)</h3>
28
    <h3><%= l(:label_result_plural) %> (<%= @result_count %>)</h3>
29 29
    <dl id="search-results">
30 30
      <% @results.each do |e| %>
31 31
        <dt class="<%= e.event_type %>">
......
38 38
    </dl>
39 39
<% end %>
40 40

  
41
<p class="pagination">
42
<% if @pagination_previous_date %>
43
<%= link_to_content_update("\xc2\xab " + l(:label_previous),
44
      params.merge(:previous => 1,
45
                   :offset => @pagination_previous_date.strftime("%Y%m%d%H%M%S"))) %>&nbsp;
41
<% if @result_pages %>
42
<p class="pagination"><%= pagination_links_full @result_pages, @result_count, :per_page_links => false %></p>
46 43
<% end %>
47
<% if @pagination_next_date %>
48
<%= link_to_content_update(l(:label_next) + " \xc2\xbb",
49
      params.merge(:previous => nil,
50
                   :offset => @pagination_next_date.strftime("%Y%m%d%H%M%S"))) %>
51
<% end %>
52
</p>
53 44

  
54 45
<% html_title(l(:label_search)) -%>
55 46

  
trunk/lib/plugins/acts_as_searchable/lib/acts_as_searchable.rb
23 23
      end
24 24

  
25 25
      module ClassMethods
26
        # Adds the search methods to the class.
27
        #
26 28
        # Options:
27 29
        # * :columns - a column or an array of columns to search
28 30
        # * :project_key - project foreign key (default to project_id)
29
        # * :date_column - name of the datetime column (default to created_on)
30
        # * :sort_order - name of the column used to sort results (default to :date_column or created_on)
31
        # * :permission - permission required to search the model (default to :view_"objects")
31
        # * :date_column - name of the datetime column used to sort results (default to :created_on)
32
        # * :permission - permission required to search the model
33
        # * :scope - scope used to search results
34
        # * :preload - associations to preload when loading results for display
32 35
        def acts_as_searchable(options = {})
33 36
          return if self.included_modules.include?(Redmine::Acts::Searchable::InstanceMethods)
34
          options.assert_valid_keys(:columns, :project_key, :date_column, :order_column, :search_custom_fields, :permission, :scope)
37
          options.assert_valid_keys(:columns, :project_key, :date_column, :permission, :scope, :preload)
35 38

  
36 39
          cattr_accessor :searchable_options
37 40
          self.searchable_options = options
......
43 46
          end
44 47

  
45 48
          searchable_options[:project_key] ||= "#{table_name}.project_id"
46
          searchable_options[:date_column] ||= "#{table_name}.created_on"
47
          searchable_options[:order_column] ||= searchable_options[:date_column]
49
          searchable_options[:date_column] ||= :created_on
48 50

  
49 51
          # Should we search custom fields on this model ?
50 52
          searchable_options[:search_custom_fields] = !reflect_on_association(:custom_values).nil?
......
59 61
        end
60 62

  
61 63
        module ClassMethods
62
          # Searches the model for the given tokens
63
          # projects argument can be either nil (will search all projects), a project or an array of projects
64
          # Returns the results and the results count
65
          def search(tokens, projects=nil, options={})
64
          # Searches the model for the given tokens and user visibility.
65
          # The projects argument can be either nil (will search all projects), a project or an array of projects.
66
          # Returns an array that contains the rank and id of all results.
67
          # In current implementation, the rank is the record timestamp.
68
          #
69
          # Valid options:
70
          # * :titles_only - searches tokens in the first searchable column only
71
          # * :all_words - searches results that match all token
72
          # * :limit - maximum number of results to return
73
          #
74
          # Example:
75
          #   Issue.search_result_ranks_and_ids("foo")
76
          #   # => [[Tue, 26 Jun 2007 22:16:00 UTC +00:00, 69], [Mon, 08 Oct 2007 14:31:00 UTC +00:00, 123]]
77
          def search_result_ranks_and_ids(tokens, user=User.current, projects=nil, options={})
66 78
            if projects.is_a?(Array) && projects.empty?
67 79
              # no results
68
              return [[], 0]
80
              return []
69 81
            end
70 82

  
71
            # TODO: make user an argument
72
            user = User.current
73 83
            tokens = [] << tokens unless tokens.is_a?(Array)
74 84
            projects = [] << projects if projects.is_a?(Project)
75 85

  
76
            limit_options = {}
77
            limit_options[:limit] = options[:limit] if options[:limit]
78

  
79 86
            columns = searchable_options[:columns]
80 87
            columns = columns[0..0] if options[:titles_only]
81 88

  
......
105 112
            if scope.is_a? Proc
106 113
              scope = scope.call
107 114
            end
108
            project_conditions = []
109
            if searchable_options.has_key?(:permission)
110
              project_conditions << Project.allowed_to_condition(user, searchable_options[:permission] || :view_project)
111
            elsif respond_to?(:visible)
115

  
116
            if respond_to?(:visible) && !searchable_options.has_key?(:permission)
112 117
              scope = scope.visible(user)
113 118
            else
114
              ActiveSupport::Deprecation.warn "acts_as_searchable with implicit :permission option is deprecated. Add a visible scope to the #{self.name} model or use explicit :permission option."
115
              project_conditions << Project.allowed_to_condition(user, "view_#{self.name.underscore.pluralize}".to_sym)
119
              permission = searchable_options[:permission] || :view_project
120
              scope = scope.where(Project.allowed_to_condition(user, permission))
116 121
            end
122

  
117 123
            # TODO: use visible scope options instead
118
            project_conditions << "#{searchable_options[:project_key]} IN (#{projects.collect(&:id).join(',')})" unless projects.nil?
119
            project_conditions = project_conditions.empty? ? nil : project_conditions.join(' AND ')
124
            if projects
125
              scope = scope.where("#{searchable_options[:project_key]} IN (?)", projects.map(&:id))
126
            end
120 127

  
121 128
            results = []
122 129
            results_count = 0
123 130

  
124
            scope = scope.
125
              joins(searchable_options[:include]).
126
              order("#{searchable_options[:order_column]} " + (options[:before] ? 'DESC' : 'ASC')).
127
              where(project_conditions).
131
            scope.
132
              reorder(searchable_options[:date_column] => :desc, :id => :desc).
128 133
              where(tokens_conditions).
129
              uniq
134
              limit(options[:limit]).
135
              uniq.
136
              pluck(searchable_options[:date_column], :id)
137
          end
130 138

  
131
            results_count = scope.count
139
          # Returns search results of given ids
140
          def search_results_from_ids(ids)
141
            where(:id => ids).preload(searchable_options[:preload]).to_a
142
          end
132 143

  
133
            scope_with_limit = scope.limit(options[:limit])
134
            if options[:offset]
135
              scope_with_limit = scope_with_limit.where("#{searchable_options[:date_column]} #{options[:before] ? '<' : '>'} ?", options[:offset])
136
            end
137
            results = scope_with_limit.to_a
138

  
139
            [results, results_count]
144
          # Returns search results with same arguments as search_result_ranks_and_ids
145
          def search_results(*args)
146
            ranks_and_ids = search_result_ranks_and_ids(*args)
147
            search_results_from_ids(ranks_and_ids.map(&:last))
140 148
          end
141 149
        end
142 150
      end
trunk/test/functional/search_controller_test.rb
75 75
    assert_select 'dt.issue a', :text => /Add ingredients categories/
76 76
    assert_select 'dd', :text => /should be classified by categories/
77 77

  
78
    assert assigns(:results_by_type).is_a?(Hash)
79
    assert_equal 5, assigns(:results_by_type)['changesets']
78
    assert assigns(:result_count_by_type).is_a?(Hash)
79
    assert_equal 5, assigns(:result_count_by_type)['changesets']
80 80
    assert_select 'a', :text => 'Changesets (5)'
81 81
  end
82 82

  
......
222 222
    assert_equal 1, results.size
223 223
  end
224 224

  
225
  def test_search_with_offset
226
    get :index, :q => 'coo', :offset => '20080806073000'
225
  def test_search_with_pagination
226
    issue = (0..24).map {Issue.generate! :subject => 'search_with_limited_results'}.reverse
227

  
228
    get :index, :q => 'search_with_limited_results'
227 229
    assert_response :success
228
    results = assigns(:results)
229
    assert results.any?
230
    assert results.map(&:event_datetime).max < '20080806T073000'.to_time
231
  end
230
    assert_equal issue[0..9], assigns(:results)
232 231

  
233
  def test_search_previous_with_offset
234
    get :index, :q => 'coo', :offset => '20080806073000', :previous => '1'
232
    get :index, :q => 'search_with_limited_results', :page => 2
235 233
    assert_response :success
236
    results = assigns(:results)
237
    assert results.any?
238
    assert results.map(&:event_datetime).min >= '20080806T073000'.to_time
234
    assert_equal issue[10..19], assigns(:results)
235

  
236
    get :index, :q => 'search_with_limited_results', :page => 3
237
    assert_response :success
238
    assert_equal issue[20..24], assigns(:results)
239

  
240
    get :index, :q => 'search_with_limited_results', :page => 4
241
    assert_response :success
242
    assert_equal [], assigns(:results)
239 243
  end
240 244

  
241 245
  def test_search_with_invalid_project_id
trunk/test/unit/search_test.rb
42 42
  def test_search_by_anonymous
43 43
    User.current = nil
44 44

  
45
    r = Issue.search(@issue_keyword).first
45
    r = Issue.search_results(@issue_keyword)
46 46
    assert r.include?(@issue)
47
    r = Changeset.search(@changeset_keyword).first
47
    r = Changeset.search_results(@changeset_keyword)
48 48
    assert r.include?(@changeset)
49 49

  
50 50
    # Removes the :view_changesets permission from Anonymous role
51 51
    remove_permission Role.anonymous, :view_changesets
52 52
    User.current = nil
53 53

  
54
    r = Issue.search(@issue_keyword).first
54
    r = Issue.search_results(@issue_keyword)
55 55
    assert r.include?(@issue)
56
    r = Changeset.search(@changeset_keyword).first
56
    r = Changeset.search_results(@changeset_keyword)
57 57
    assert !r.include?(@changeset)
58 58

  
59 59
    # Make the project private
60 60
    @project.update_attribute :is_public, false
61
    r = Issue.search(@issue_keyword).first
61
    r = Issue.search_results(@issue_keyword)
62 62
    assert !r.include?(@issue)
63
    r = Changeset.search(@changeset_keyword).first
63
    r = Changeset.search_results(@changeset_keyword)
64 64
    assert !r.include?(@changeset)
65 65
  end
66 66

  
......
68 68
    User.current = User.find_by_login('rhill')
69 69
    assert User.current.memberships.empty?
70 70

  
71
    r = Issue.search(@issue_keyword).first
71
    r = Issue.search_results(@issue_keyword)
72 72
    assert r.include?(@issue)
73
    r = Changeset.search(@changeset_keyword).first
73
    r = Changeset.search_results(@changeset_keyword)
74 74
    assert r.include?(@changeset)
75 75

  
76 76
    # Removes the :view_changesets permission from Non member role
77 77
    remove_permission Role.non_member, :view_changesets
78 78
    User.current = User.find_by_login('rhill')
79 79

  
80
    r = Issue.search(@issue_keyword).first
80
    r = Issue.search_results(@issue_keyword)
81 81
    assert r.include?(@issue)
82
    r = Changeset.search(@changeset_keyword).first
82
    r = Changeset.search_results(@changeset_keyword)
83 83
    assert !r.include?(@changeset)
84 84

  
85 85
    # Make the project private
86 86
    @project.update_attribute :is_public, false
87
    r = Issue.search(@issue_keyword).first
87
    r = Issue.search_results(@issue_keyword)
88 88
    assert !r.include?(@issue)
89
    r = Changeset.search(@changeset_keyword).first
89
    r = Changeset.search_results(@changeset_keyword)
90 90
    assert !r.include?(@changeset)
91 91
  end
92 92

  
......
94 94
    User.current = User.find_by_login('jsmith')
95 95
    assert User.current.projects.include?(@project)
96 96

  
97
    r = Issue.search(@issue_keyword).first
97
    r = Issue.search_results(@issue_keyword)
98 98
    assert r.include?(@issue)
99
    r = Changeset.search(@changeset_keyword).first
99
    r = Changeset.search_results(@changeset_keyword)
100 100
    assert r.include?(@changeset)
101 101

  
102 102
    # Make the project private
103 103
    @project.update_attribute :is_public, false
104
    r = Issue.search(@issue_keyword).first
104
    r = Issue.search_results(@issue_keyword)
105 105
    assert r.include?(@issue)
106
    r = Changeset.search(@changeset_keyword).first
106
    r = Changeset.search_results(@changeset_keyword)
107 107
    assert r.include?(@changeset)
108 108
  end
109 109

  
......
115 115
    User.current = User.find_by_login('jsmith')
116 116
    assert User.current.projects.include?(@project)
117 117

  
118
    r = Issue.search(@issue_keyword).first
118
    r = Issue.search_results(@issue_keyword)
119 119
    assert r.include?(@issue)
120
    r = Changeset.search(@changeset_keyword).first
120
    r = Changeset.search_results(@changeset_keyword)
121 121
    assert !r.include?(@changeset)
122 122

  
123 123
    # Make the project private
124 124
    @project.update_attribute :is_public, false
125
    r = Issue.search(@issue_keyword).first
125
    r = Issue.search_results(@issue_keyword)
126 126
    assert r.include?(@issue)
127
    r = Changeset.search(@changeset_keyword).first
127
    r = Changeset.search_results(@changeset_keyword)
128 128
    assert !r.include?(@changeset)
129 129
  end
130 130

  
131 131
  def test_search_issue_with_multiple_hits_in_journals
132
    i = Issue.find(1)
133
    assert_equal 2, i.journals.where("notes LIKE '%notes%'").count
132
    issue = Issue.find(1)
133
    assert_equal 2, issue.journals.where("notes LIKE '%notes%'").count
134 134

  
135
    r = Issue.search('%notes%').first
135
    r = Issue.search_results('%notes%')
136 136
    assert_equal 1, r.size
137
    assert_equal i, r.first
137
    assert_equal issue, r.first
138 138
  end
139 139

  
140 140
  private

Also available in: Unified diff