From b9a310f4b19b341f962b947d36733aaf9c603d0c Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Wed, 22 May 2013 14:36:14 -0400 Subject: [PATCH] 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 | 7 +- lib/search.rb | 415 +++++++++++---------- spec/components/search_spec.rb | 26 +- spec/controllers/search_controller_spec.rb | 20 +- 4 files changed, 241 insertions(+), 227 deletions(-) diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 2665070705e..9d43075ab5e 100644 --- a/app/controllers/search_controller.rb +++ b/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) - render_json_dump(search_result.as_json) + search = Search.new(params[:term], + guardian: guardian, + type_filter: params[:type_filter]) + + render_json_dump(search.execute.as_json) end end diff --git a/lib/search.rb b/lib/search.rb index 7331d9f2def..8f222d29db4 100644 --- a/lib/search.rb +++ b/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 < 1" - end - - builder.where < '#{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 < 'topic', - 'id' => topic.id, - 'url' => topic.relative_url, - 'title' => topic.title }] - group_db_result(results, 'topic') + def initialize(term, opts=nil) + @term = term.to_s if term.present? + @opts = opts || {} + @guardian = @opts[:guardian] || Guardian.new 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 - guardian ||= Guardian.new(nil) - - term = term.to_s + # really short terms are totally pointless + return nil if @term.length < (@opts[:min_search_term_length] || SiteSetting.min_search_term_length) # 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) - return single_topic(route[:topic_id], guardian) if route[:topic_id].present? + route = Rails.application.routes.recognize_path(@term) + 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(/[:()&!]/,'')) - query_string(sanitized_term, guardian, type_filter, min_search_term_length) - end + @term = PG::Connection.escape_string(@term.gsub(/[:()&!]/,'')) - # Search for a string term - 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) + query_string end private - # Group the results by type - def self.group_db_result(db_result, type_filter) - grouped = {} - db_result.each do |row| - type = row.delete('type') + # Search for a string term + def query_string - # 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) + args = {orig: @term, + query: @term.split.map {|t| "#{t}:*"}.join(" & "), + locale: Search.current_locale_long} + + 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 - row['avatar_template'] = User.avatar_template(row['email']) if type == 'user' + db_result = db_result.to_a - # 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' + expected_topics = 0 + expected_topics = Search.facets.size unless type_filter.present? + expected_topics = Search.per_facet * Search.facets.size if type_filter == 'topic' - grouped[type] ||= [] - grouped[type] << row + 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(: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) - results = results[0..([results.length, Search.per_facet].min - 1)] if type_filter.blank? - { - type: type, - name: I18n.t("search.types.#{type}"), - more: more, - results: results - } + + # If we're searching for a single topic + def single_topic(id) + topic = Topic.where(id: id).first + return nil unless @guardian.can_see?(topic) + + group_db_result([{'type' => 'topic', + '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 < 1" + end + + builder.where < '#{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 diff --git a/spec/components/search_spec.rb b/spec/components/search_spec.rb index 33cd841426f..96737fbb03d 100644 --- a/spec/components/search_spec.rb +++ b/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 diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb index ae6bdfc87d3..9299e97347d 100644 --- a/spec/controllers/search_controller_spec.rb +++ b/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.stubs(:new).returns(m) - Search.expects(:query).with('test', m, nil, 3) + guardian = Guardian.new + Guardian.stubs(:new).returns(guardian) + + 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.stubs(:new).returns(m) - Search.expects(:query).with('test', m, 'topic', 3) + guardian = Guardian.new + Guardian.stubs(:new).returns(guardian) + + 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