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. git-svn-id: http://svn.redmine.org/redmine/trunk@13739 e93f8b46-1217-0410-a6f0-8f06a7374b81tags/3.0.0
@@ -36,9 +36,6 @@ class SearchController < ApplicationController | |||
@project | |||
end | |||
offset = nil | |||
begin; offset = params[:offset].to_time if params[:offset]; rescue; end | |||
# quick jump to an issue | |||
if (m = @question.match(/^#?(\d+)$/)) && (issue = Issue.visible.find_by_id(m[1].to_i)) | |||
redirect_to issue_path(issue) | |||
@@ -66,34 +63,39 @@ class SearchController < ApplicationController | |||
# no more than 5 tokens to search for | |||
@tokens.slice! 5..-1 if @tokens.size > 5 | |||
@results = [] | |||
@results_by_type = Hash.new {|h,k| h[k] = 0} | |||
limit = 10 | |||
@scope.each do |s| | |||
r, c = s.singularize.camelcase.constantize.search(@tokens, projects_to_search, | |||
@result_count = 0 | |||
@result_count_by_type = Hash.new {|h,k| h[k] = 0} | |||
ranks_and_ids = [] | |||
# get all the results ranks and ids | |||
@scope.each do |scope| | |||
klass = scope.singularize.camelcase.constantize | |||
ranks_and_ids_in_scope = klass.search_result_ranks_and_ids(@tokens, User.current, projects_to_search, | |||
:all_words => @all_words, | |||
:titles_only => @titles_only, | |||
:limit => (limit+1), | |||
:offset => offset, | |||
:before => params[:previous].nil?) | |||
@results += r | |||
@results_by_type[s] += c | |||
:titles_only => @titles_only | |||
) | |||
@result_count_by_type[scope] += ranks_and_ids_in_scope.size | |||
@result_count += ranks_and_ids_in_scope.size | |||
ranks_and_ids += ranks_and_ids_in_scope.map {|r| [scope, r]} | |||
end | |||
@results = @results.sort {|a,b| b.event_datetime <=> a.event_datetime} | |||
if params[:previous].nil? | |||
@pagination_previous_date = @results[0].event_datetime if offset && @results[0] | |||
if @results.size > limit | |||
@pagination_next_date = @results[limit-1].event_datetime | |||
@results = @results[0, limit] | |||
end | |||
else | |||
@pagination_next_date = @results[-1].event_datetime if offset && @results[-1] | |||
if @results.size > limit | |||
@pagination_previous_date = @results[-(limit)].event_datetime | |||
@results = @results[-(limit), limit] | |||
end | |||
@result_pages = Paginator.new @result_count, limit, params['page'] | |||
# sort results, higher rank and id first | |||
ranks_and_ids.sort! {|a,b| b.last <=> a.last } | |||
ranks_and_ids = ranks_and_ids[@result_pages.offset, limit] || [] | |||
# load the results to display | |||
results_by_scope = Hash.new {|h,k| h[k] = []} | |||
ranks_and_ids.group_by(&:first).each do |scope, rs| | |||
klass = scope.singularize.camelcase.constantize | |||
results_by_scope[scope] += klass.search_results_from_ids(rs.map(&:last).map(&:last)) | |||
end | |||
@results = ranks_and_ids.map do |scope, r| | |||
results_by_scope[scope].detect {|record| record.id == r.last} | |||
end.compact | |||
else | |||
@question = "" | |||
end |
@@ -35,9 +35,9 @@ class Changeset < ActiveRecord::Base | |||
:url => Proc.new {|o| {:controller => 'repositories', :action => 'revision', :id => o.repository.project, :repository_id => o.repository.identifier_param, :rev => o.identifier}} | |||
acts_as_searchable :columns => 'comments', | |||
:scope => preload(:repository => :project), | |||
:preload => {:repository => :project}, | |||
:project_key => "#{Repository.table_name}.project_id", | |||
:date_column => "#{Changeset.table_name}.committed_on" | |||
:date_column => :committed_on | |||
acts_as_activity_provider :timestamp => "#{table_name}.committed_on", | |||
:author_key => :user_id, |
@@ -22,7 +22,7 @@ class Document < ActiveRecord::Base | |||
acts_as_attachable :delete_permission => :delete_documents | |||
acts_as_searchable :columns => ['title', "#{table_name}.description"], | |||
:scope => preload(:project) | |||
:preload => :project | |||
acts_as_event :title => Proc.new {|o| "#{l(:label_document)}: #{o.title}"}, | |||
:author => Proc.new {|o| o.attachments.reorder("#{Attachment.table_name}.created_on ASC").first.try(:author) }, | |||
:url => Proc.new {|o| {:controller => 'documents', :action => 'show', :id => o.id}} |
@@ -46,8 +46,7 @@ class Issue < ActiveRecord::Base | |||
acts_as_customizable | |||
acts_as_watchable | |||
acts_as_searchable :columns => ['subject', "#{table_name}.description", "#{Journal.table_name}.notes"], | |||
# sort by id so that limited eager loading doesn't break with postgresql | |||
:order_column => "#{table_name}.id", | |||
:preload => [:project, :status, :tracker], | |||
:scope => lambda { joins(:project). | |||
joins("LEFT OUTER JOIN #{Journal.table_name} ON #{Journal.table_name}.journalized_type='Issue'" + | |||
" AND #{Journal.table_name}.journalized_id = #{Issue.table_name}.id" + |
@@ -25,9 +25,9 @@ class Message < ActiveRecord::Base | |||
attr_protected :id | |||
acts_as_searchable :columns => ['subject', 'content'], | |||
:scope => preload(:board => :project), | |||
:project_key => "#{Board.table_name}.project_id", | |||
:date_column => "#{table_name}.created_on" | |||
:preload => {:board => :project}, | |||
:project_key => "#{Board.table_name}.project_id" | |||
acts_as_event :title => Proc.new {|o| "#{o.board.name}: #{o.subject}"}, | |||
:description => :content, | |||
:group => :parent, |
@@ -29,7 +29,7 @@ class News < ActiveRecord::Base | |||
acts_as_attachable :edit_permission => :manage_news, | |||
:delete_permission => :manage_news | |||
acts_as_searchable :columns => ['title', 'summary', "#{table_name}.description"], | |||
:scope => preload(:project) | |||
:preload => :project | |||
acts_as_event :url => Proc.new {|o| {:controller => 'news', :action => 'show', :id => o.id}} | |||
acts_as_activity_provider :scope => preload(:project, :author), | |||
:author_key => :author_id |
@@ -33,7 +33,8 @@ class WikiPage < ActiveRecord::Base | |||
:url => Proc.new {|o| {:controller => 'wiki', :action => 'show', :project_id => o.wiki.project, :id => o.title}} | |||
acts_as_searchable :columns => ['title', "#{WikiContent.table_name}.text"], | |||
:scope => preload(:wiki => :project).joins(:content, {:wiki => :project}), | |||
:scope => joins(:content, {:wiki => :project}), | |||
:preload => {:wiki => :project}, | |||
:permission => :view_wiki_pages, | |||
:project_key => "#{Wiki.table_name}.project_id" | |||
@@ -23,9 +23,9 @@ | |||
<% if @results %> | |||
<div id="search-results-counts"> | |||
<%= render_results_by_type(@results_by_type) unless @scope.size == 1 %> | |||
<%= render_results_by_type(@result_count_by_type) unless @scope.size == 1 %> | |||
</div> | |||
<h3><%= l(:label_result_plural) %> (<%= @results_by_type.values.sum %>)</h3> | |||
<h3><%= l(:label_result_plural) %> (<%= @result_count %>)</h3> | |||
<dl id="search-results"> | |||
<% @results.each do |e| %> | |||
<dt class="<%= e.event_type %>"> | |||
@@ -38,18 +38,9 @@ | |||
</dl> | |||
<% end %> | |||
<p class="pagination"> | |||
<% if @pagination_previous_date %> | |||
<%= link_to_content_update("\xc2\xab " + l(:label_previous), | |||
params.merge(:previous => 1, | |||
:offset => @pagination_previous_date.strftime("%Y%m%d%H%M%S"))) %> | |||
<% if @result_pages %> | |||
<p class="pagination"><%= pagination_links_full @result_pages, @result_count, :per_page_links => false %></p> | |||
<% end %> | |||
<% if @pagination_next_date %> | |||
<%= link_to_content_update(l(:label_next) + " \xc2\xbb", | |||
params.merge(:previous => nil, | |||
:offset => @pagination_next_date.strftime("%Y%m%d%H%M%S"))) %> | |||
<% end %> | |||
</p> | |||
<% html_title(l(:label_search)) -%> | |||
@@ -23,15 +23,18 @@ module Redmine | |||
end | |||
module ClassMethods | |||
# Adds the search methods to the class. | |||
# | |||
# Options: | |||
# * :columns - a column or an array of columns to search | |||
# * :project_key - project foreign key (default to project_id) | |||
# * :date_column - name of the datetime column (default to created_on) | |||
# * :sort_order - name of the column used to sort results (default to :date_column or created_on) | |||
# * :permission - permission required to search the model (default to :view_"objects") | |||
# * :date_column - name of the datetime column used to sort results (default to :created_on) | |||
# * :permission - permission required to search the model | |||
# * :scope - scope used to search results | |||
# * :preload - associations to preload when loading results for display | |||
def acts_as_searchable(options = {}) | |||
return if self.included_modules.include?(Redmine::Acts::Searchable::InstanceMethods) | |||
options.assert_valid_keys(:columns, :project_key, :date_column, :order_column, :search_custom_fields, :permission, :scope) | |||
options.assert_valid_keys(:columns, :project_key, :date_column, :permission, :scope, :preload) | |||
cattr_accessor :searchable_options | |||
self.searchable_options = options | |||
@@ -43,8 +46,7 @@ module Redmine | |||
end | |||
searchable_options[:project_key] ||= "#{table_name}.project_id" | |||
searchable_options[:date_column] ||= "#{table_name}.created_on" | |||
searchable_options[:order_column] ||= searchable_options[:date_column] | |||
searchable_options[:date_column] ||= :created_on | |||
# Should we search custom fields on this model ? | |||
searchable_options[:search_custom_fields] = !reflect_on_association(:custom_values).nil? | |||
@@ -59,23 +61,28 @@ module Redmine | |||
end | |||
module ClassMethods | |||
# Searches the model for the given tokens | |||
# projects argument can be either nil (will search all projects), a project or an array of projects | |||
# Returns the results and the results count | |||
def search(tokens, projects=nil, options={}) | |||
# Searches the model for the given tokens and user visibility. | |||
# The projects argument can be either nil (will search all projects), a project or an array of projects. | |||
# Returns an array that contains the rank and id of all results. | |||
# In current implementation, the rank is the record timestamp. | |||
# | |||
# Valid options: | |||
# * :titles_only - searches tokens in the first searchable column only | |||
# * :all_words - searches results that match all token | |||
# * :limit - maximum number of results to return | |||
# | |||
# Example: | |||
# Issue.search_result_ranks_and_ids("foo") | |||
# # => [[Tue, 26 Jun 2007 22:16:00 UTC +00:00, 69], [Mon, 08 Oct 2007 14:31:00 UTC +00:00, 123]] | |||
def search_result_ranks_and_ids(tokens, user=User.current, projects=nil, options={}) | |||
if projects.is_a?(Array) && projects.empty? | |||
# no results | |||
return [[], 0] | |||
return [] | |||
end | |||
# TODO: make user an argument | |||
user = User.current | |||
tokens = [] << tokens unless tokens.is_a?(Array) | |||
projects = [] << projects if projects.is_a?(Project) | |||
limit_options = {} | |||
limit_options[:limit] = options[:limit] if options[:limit] | |||
columns = searchable_options[:columns] | |||
columns = columns[0..0] if options[:titles_only] | |||
@@ -105,38 +112,39 @@ module Redmine | |||
if scope.is_a? Proc | |||
scope = scope.call | |||
end | |||
project_conditions = [] | |||
if searchable_options.has_key?(:permission) | |||
project_conditions << Project.allowed_to_condition(user, searchable_options[:permission] || :view_project) | |||
elsif respond_to?(:visible) | |||
if respond_to?(:visible) && !searchable_options.has_key?(:permission) | |||
scope = scope.visible(user) | |||
else | |||
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." | |||
project_conditions << Project.allowed_to_condition(user, "view_#{self.name.underscore.pluralize}".to_sym) | |||
permission = searchable_options[:permission] || :view_project | |||
scope = scope.where(Project.allowed_to_condition(user, permission)) | |||
end | |||
# TODO: use visible scope options instead | |||
project_conditions << "#{searchable_options[:project_key]} IN (#{projects.collect(&:id).join(',')})" unless projects.nil? | |||
project_conditions = project_conditions.empty? ? nil : project_conditions.join(' AND ') | |||
if projects | |||
scope = scope.where("#{searchable_options[:project_key]} IN (?)", projects.map(&:id)) | |||
end | |||
results = [] | |||
results_count = 0 | |||
scope = scope. | |||
joins(searchable_options[:include]). | |||
order("#{searchable_options[:order_column]} " + (options[:before] ? 'DESC' : 'ASC')). | |||
where(project_conditions). | |||
scope. | |||
reorder(searchable_options[:date_column] => :desc, :id => :desc). | |||
where(tokens_conditions). | |||
uniq | |||
results_count = scope.count | |||
limit(options[:limit]). | |||
uniq. | |||
pluck(searchable_options[:date_column], :id) | |||
end | |||
scope_with_limit = scope.limit(options[:limit]) | |||
if options[:offset] | |||
scope_with_limit = scope_with_limit.where("#{searchable_options[:date_column]} #{options[:before] ? '<' : '>'} ?", options[:offset]) | |||
end | |||
results = scope_with_limit.to_a | |||
# Returns search results of given ids | |||
def search_results_from_ids(ids) | |||
where(:id => ids).preload(searchable_options[:preload]).to_a | |||
end | |||
[results, results_count] | |||
# Returns search results with same arguments as search_result_ranks_and_ids | |||
def search_results(*args) | |||
ranks_and_ids = search_result_ranks_and_ids(*args) | |||
search_results_from_ids(ranks_and_ids.map(&:last)) | |||
end | |||
end | |||
end |
@@ -75,8 +75,8 @@ class SearchControllerTest < ActionController::TestCase | |||
assert_select 'dt.issue a', :text => /Add ingredients categories/ | |||
assert_select 'dd', :text => /should be classified by categories/ | |||
assert assigns(:results_by_type).is_a?(Hash) | |||
assert_equal 5, assigns(:results_by_type)['changesets'] | |||
assert assigns(:result_count_by_type).is_a?(Hash) | |||
assert_equal 5, assigns(:result_count_by_type)['changesets'] | |||
assert_select 'a', :text => 'Changesets (5)' | |||
end | |||
@@ -222,20 +222,24 @@ class SearchControllerTest < ActionController::TestCase | |||
assert_equal 1, results.size | |||
end | |||
def test_search_with_offset | |||
get :index, :q => 'coo', :offset => '20080806073000' | |||
def test_search_with_pagination | |||
issue = (0..24).map {Issue.generate! :subject => 'search_with_limited_results'}.reverse | |||
get :index, :q => 'search_with_limited_results' | |||
assert_response :success | |||
results = assigns(:results) | |||
assert results.any? | |||
assert results.map(&:event_datetime).max < '20080806T073000'.to_time | |||
end | |||
assert_equal issue[0..9], assigns(:results) | |||
def test_search_previous_with_offset | |||
get :index, :q => 'coo', :offset => '20080806073000', :previous => '1' | |||
get :index, :q => 'search_with_limited_results', :page => 2 | |||
assert_response :success | |||
results = assigns(:results) | |||
assert results.any? | |||
assert results.map(&:event_datetime).min >= '20080806T073000'.to_time | |||
assert_equal issue[10..19], assigns(:results) | |||
get :index, :q => 'search_with_limited_results', :page => 3 | |||
assert_response :success | |||
assert_equal issue[20..24], assigns(:results) | |||
get :index, :q => 'search_with_limited_results', :page => 4 | |||
assert_response :success | |||
assert_equal [], assigns(:results) | |||
end | |||
def test_search_with_invalid_project_id |
@@ -42,25 +42,25 @@ class SearchTest < ActiveSupport::TestCase | |||
def test_search_by_anonymous | |||
User.current = nil | |||
r = Issue.search(@issue_keyword).first | |||
r = Issue.search_results(@issue_keyword) | |||
assert r.include?(@issue) | |||
r = Changeset.search(@changeset_keyword).first | |||
r = Changeset.search_results(@changeset_keyword) | |||
assert r.include?(@changeset) | |||
# Removes the :view_changesets permission from Anonymous role | |||
remove_permission Role.anonymous, :view_changesets | |||
User.current = nil | |||
r = Issue.search(@issue_keyword).first | |||
r = Issue.search_results(@issue_keyword) | |||
assert r.include?(@issue) | |||
r = Changeset.search(@changeset_keyword).first | |||
r = Changeset.search_results(@changeset_keyword) | |||
assert !r.include?(@changeset) | |||
# Make the project private | |||
@project.update_attribute :is_public, false | |||
r = Issue.search(@issue_keyword).first | |||
r = Issue.search_results(@issue_keyword) | |||
assert !r.include?(@issue) | |||
r = Changeset.search(@changeset_keyword).first | |||
r = Changeset.search_results(@changeset_keyword) | |||
assert !r.include?(@changeset) | |||
end | |||
@@ -68,25 +68,25 @@ class SearchTest < ActiveSupport::TestCase | |||
User.current = User.find_by_login('rhill') | |||
assert User.current.memberships.empty? | |||
r = Issue.search(@issue_keyword).first | |||
r = Issue.search_results(@issue_keyword) | |||
assert r.include?(@issue) | |||
r = Changeset.search(@changeset_keyword).first | |||
r = Changeset.search_results(@changeset_keyword) | |||
assert r.include?(@changeset) | |||
# Removes the :view_changesets permission from Non member role | |||
remove_permission Role.non_member, :view_changesets | |||
User.current = User.find_by_login('rhill') | |||
r = Issue.search(@issue_keyword).first | |||
r = Issue.search_results(@issue_keyword) | |||
assert r.include?(@issue) | |||
r = Changeset.search(@changeset_keyword).first | |||
r = Changeset.search_results(@changeset_keyword) | |||
assert !r.include?(@changeset) | |||
# Make the project private | |||
@project.update_attribute :is_public, false | |||
r = Issue.search(@issue_keyword).first | |||
r = Issue.search_results(@issue_keyword) | |||
assert !r.include?(@issue) | |||
r = Changeset.search(@changeset_keyword).first | |||
r = Changeset.search_results(@changeset_keyword) | |||
assert !r.include?(@changeset) | |||
end | |||
@@ -94,16 +94,16 @@ class SearchTest < ActiveSupport::TestCase | |||
User.current = User.find_by_login('jsmith') | |||
assert User.current.projects.include?(@project) | |||
r = Issue.search(@issue_keyword).first | |||
r = Issue.search_results(@issue_keyword) | |||
assert r.include?(@issue) | |||
r = Changeset.search(@changeset_keyword).first | |||
r = Changeset.search_results(@changeset_keyword) | |||
assert r.include?(@changeset) | |||
# Make the project private | |||
@project.update_attribute :is_public, false | |||
r = Issue.search(@issue_keyword).first | |||
r = Issue.search_results(@issue_keyword) | |||
assert r.include?(@issue) | |||
r = Changeset.search(@changeset_keyword).first | |||
r = Changeset.search_results(@changeset_keyword) | |||
assert r.include?(@changeset) | |||
end | |||
@@ -115,26 +115,26 @@ class SearchTest < ActiveSupport::TestCase | |||
User.current = User.find_by_login('jsmith') | |||
assert User.current.projects.include?(@project) | |||
r = Issue.search(@issue_keyword).first | |||
r = Issue.search_results(@issue_keyword) | |||
assert r.include?(@issue) | |||
r = Changeset.search(@changeset_keyword).first | |||
r = Changeset.search_results(@changeset_keyword) | |||
assert !r.include?(@changeset) | |||
# Make the project private | |||
@project.update_attribute :is_public, false | |||
r = Issue.search(@issue_keyword).first | |||
r = Issue.search_results(@issue_keyword) | |||
assert r.include?(@issue) | |||
r = Changeset.search(@changeset_keyword).first | |||
r = Changeset.search_results(@changeset_keyword) | |||
assert !r.include?(@changeset) | |||
end | |||
def test_search_issue_with_multiple_hits_in_journals | |||
i = Issue.find(1) | |||
assert_equal 2, i.journals.where("notes LIKE '%notes%'").count | |||
issue = Issue.find(1) | |||
assert_equal 2, issue.journals.where("notes LIKE '%notes%'").count | |||
r = Issue.search('%notes%').first | |||
r = Issue.search_results('%notes%') | |||
assert_equal 1, r.size | |||
assert_equal i, r.first | |||
assert_equal issue, r.first | |||
end | |||
private |