Search Refactor: Remove some manual SQL, make search data tables more idomatic Rails/AR

app/models/category.rb

@@ -27,6 +27,8 @@ class Category < ActiveRecord::Base
   after_destroy :invalidate_site_cache
   after_destroy :publish_categories_list
 
+  has_one :category_search_data
+
   scope :latest, ->{ order('topic_count desc') }
 
   scope :secured, ->(guardian = nil) {

app/models/category_search_data.rb

@@ -0,0 +1,5 @@
+class CategorySearchData < ActiveRecord::Base
+  belongs_to :category
+
+  validates_presence_of :search_data
+end

app/models/post.rb

@@ -25,6 +25,8 @@ class Post < ActiveRecord::Base
   has_many :replies, through: :post_replies
   has_many :post_actions
 
+  has_one :post_search_data
+
   validates_presence_of :raw, :user_id, :topic_id
   validates :raw, stripped_length: { in: -> { SiteSetting.post_length } }
   validate :raw_quality

app/models/post_search_data.rb

@@ -0,0 +1,5 @@
+class PostSearchData < ActiveRecord::Base
+  belongs_to :post
+
+  validates_presence_of :search_data
+end

app/models/search_observer.rb

@@ -5,32 +5,33 @@ class SearchObserver < ActiveRecord::Observer
     HtmlScrubber.scrub(html)
   end
 
-  def self.update_index(table, id, idx)
-    Post.exec_sql("delete from #{table} where id = ?", id)
-    sql = "insert into #{table} (id, search_data) values (?, to_tsvector('english', ?))"
-    begin
-      Post.exec_sql(sql, id, idx)
-    rescue
-      # don't allow concurrency to mess up saving a post
+  def self.update_index(table, id, search_data)
+    table_name = "#{table}_search_data"
+    foreign_key = "#{table}_id"
+
+    # Would be nice to use AR here but not sure how to execut Postgres functions
+    # when inserting data like this.
+    rows = Post.exec_sql_row_count("UPDATE #{table_name} SET search_data = TO_TSVECTOR('english', ?) WHERE #{foreign_key} = ?", search_data, id)
+    if rows == 0
+      Post.exec_sql("INSERT INTO #{table_name} (#{foreign_key}, search_data) VALUES (?, TO_TSVECTOR('english', ?))", id, search_data)
     end
+  rescue
+    # don't allow concurrency to mess up saving a post
   end
 
   def self.update_posts_index(post_id, cooked, title, category)
-    idx = scrub_html_for_search(cooked)
-    idx << " " << title
-    idx << " " << category if category
-    update_index('posts_search', post_id, idx)
+    search_data = scrub_html_for_search(cooked) << " " << title
+    search_data << " " << category if category
+    update_index('post', post_id, search_data)
   end
 
   def self.update_users_index(user_id, username, name)
-    idx = username.dup
-    idx << " " << (name || "")
-
-    update_index('users_search', user_id, idx)
+    search_data = username.dup << " " << (name || "")
+    update_index('user', user_id, search_data)
   end
 
   def self.update_categories_index(category_id, name)
-    update_index('categories_search', category_id, name)
+    update_index('category', category_id, name)
   end
 
   def after_save(obj)

app/models/user.rb

@@ -33,6 +33,8 @@ class User < ActiveRecord::Base
   has_many :groups, through: :group_users
   has_many :secure_categories, through: :groups, source: :categories
 
+  has_one :user_search_data
+
   validates_presence_of :username
   validates_presence_of :email
   validates_uniqueness_of :email

app/models/user_search_data.rb

@@ -0,0 +1,4 @@
+class UserSearchData < ActiveRecord::Base
+  belongs_to :user
+  validates_presence_of :search_data
+end

db/migrate/20130522193615_rename_search_tables.rb

@@ -0,0 +1,19 @@
+class RenameSearchTables < ActiveRecord::Migration
+  def up
+    rename_table :users_search, :user_search_data
+    rename_column :user_search_data, :id, :user_id
+    rename_table :categories_search, :category_search_data
+    rename_column :category_search_data, :id, :category_id
+    rename_table :posts_search, :post_search_data
+    rename_column :post_search_data, :id, :post_id
+  end
+
+  def down
+    rename_table :user_search_data, :users_search
+    rename_column :users_search, :user_id, :id
+    rename_table :category_search_data, :categories_search
+    rename_column :categories_search, :category_id, :id
+    rename_table :post_search_data, :posts_search
+    rename_column :posts_search, :post_id, :id
+  end
+end

lib/search.rb

@@ -150,7 +150,7 @@ class Search
             c.color,
             c.text_color
     FROM categories AS c
-    JOIN categories_search s on s.id = c.id
+    JOIN category_search_data s on s.category_id = c.id
     /*where*/
     ORDER BY topics_month desc
     LIMIT :limit
@@ -171,7 +171,7 @@ SQL
                     NULL AS color,
                     NULL AS text_color
             FROM users AS u
-            JOIN users_search s on s.id = u.id
+            JOIN user_search_data s on s.user_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"
@@ -183,7 +183,7 @@ SQL
       /*select*/
       FROM topics AS ft
       /*join*/
-        JOIN posts_search s on s.id = p.id
+        JOIN post_search_data s on s.post_id = p.id
         LEFT JOIN categories c ON c.id = ft.category_id
       /*where*/
       ORDER BY
@@ -234,18 +234,18 @@ SQL
           new_slug = Slug.for(row['title'])
           new_slug = "topic" if new_slug.blank?
           row['url'].gsub!('slug', new_slug)
+        elsif type == 'user'
+          row['avatar_template'] = User.avatar_template(row['email'])
         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'
+        unless type == 'category'
+          row.delete('color')
+          row.delete('text_color')
+        end
 
-        grouped[type] ||= []
-        grouped[type] << row
+        grouped[type] = (grouped[type] || []) << row
       end
 
       grouped.map do |type, results|

spec/components/search_spec.rb

@@ -22,7 +22,7 @@ describe Search do
       @category = Fabricate(:category, name: 'america')
       @topic = Fabricate(:topic, title: 'sam saffron test topic', category: @category)
       @post = Fabricate(:post, topic: @topic, raw: 'this <b>fun test</b> <img src="bla" title="my image">')
-      @indexed = Topic.exec_sql("select search_data from posts_search where id = #{@post.id}").first["search_data"]
+      @indexed = @post.post_search_data.search_data
     end
     it "should include body in index" do
       @indexed.should =~ /fun/
@@ -37,7 +37,9 @@ describe Search do
     it "should pick up on title updates" do
       @topic.title = "harpi is the new title"
       @topic.save!
-      @indexed = Topic.exec_sql("select search_data from posts_search where id = #{@post.id}").first["search_data"]
+      @post.post_search_data.reload
+
+      @indexed = @post.post_search_data.search_data
 
       @indexed.should =~ /harpi/
     end
@@ -46,7 +48,7 @@ describe Search do
   context 'user indexing observer' do
     before do
       @user = Fabricate(:user, username: 'fred', name: 'bob jones')
-      @indexed = User.exec_sql("select search_data from users_search where id = #{@user.id}").first["search_data"]
+      @indexed = @user.user_search_data.search_data
     end
 
     it "should pick up on username" do
@@ -61,7 +63,7 @@ describe Search do
   context 'category indexing observer' do
     before do
       @category = Fabricate(:category, name: 'america')
-      @indexed = Topic.exec_sql("select search_data from categories_search where id = #{@category.id}").first["search_data"]
+      @indexed = @category.category_search_data.search_data
     end
 
     it "should pick up on name" do