Merge pull request #8736 from gschlager/rename_reply_id_column

REFACTOR: Rename `post_replies.reply_id` column to `post_replies.reply_post_id`

app/controllers/application_controller.rb

@@ -483,7 +483,7 @@ class ApplicationController < ActionController::Base
 
   def post_ids_including_replies
     post_ids  = params[:post_ids].map(&:to_i)
-    post_ids |= PostReply.where(post_id: params[:reply_post_ids]).pluck(:reply_id) if params[:reply_post_ids]
+    post_ids |= PostReply.where(post_id: params[:reply_post_ids]).pluck(:reply_post_id) if params[:reply_post_ids]
     post_ids
   end
 

app/models/post.rb

@@ -817,10 +817,10 @@ class Post < ActiveRecord::Base
       WITH RECURSIVE breadcrumb(id, level) AS (
         SELECT :post_id, 0
         UNION
-        SELECT reply_id, level + 1
+        SELECT reply_post_id, level + 1
         FROM post_replies AS r
           JOIN breadcrumb AS b ON (r.post_id = b.id)
-        WHERE r.post_id <> r.reply_id
+        WHERE r.post_id <> r.reply_post_id
               AND b.level < :max_reply_level
       ), breadcrumb_with_count AS (
           SELECT
@@ -828,8 +828,8 @@ class Post < ActiveRecord::Base
             level,
             COUNT(*) AS count
           FROM post_replies AS r
-            JOIN breadcrumb AS b ON (r.reply_id = b.id)
+            JOIN breadcrumb AS b ON (r.reply_post_id = b.id)
-          WHERE r.reply_id <> r.post_id
+          WHERE r.reply_post_id <> r.post_id
           GROUP BY id, level
       )
       SELECT id, level
@@ -1063,7 +1063,7 @@ class Post < ActiveRecord::Base
 
   def create_reply_relationship_with(post)
     return if post.nil? || self.deleted_at.present?
-    post_reply = post.post_replies.new(reply_id: id)
+    post_reply = post.post_replies.new(reply_post_id: id)
     if post_reply.save
       if Topic.visible_post_types.include?(self.post_type)
         Post.where(id: post.id).update_all ['reply_count = reply_count + 1']

app/models/post_mover.rb

@@ -250,7 +250,7 @@ class PostMover
       FROM (
         SELECT r.post_id, mp.new_topic_id, COUNT(1) AS moved_reply_count
         FROM moved_posts mp
-               JOIN post_replies r ON (mp.old_post_id = r.reply_id)
+               JOIN post_replies r ON (mp.old_post_id = r.reply_post_id)
         GROUP BY r.post_id, mp.new_topic_id
       ) x
       WHERE x.post_id = p.id AND x.new_topic_id <> p.topic_id
@@ -275,7 +275,7 @@ class PostMover
       SET post_id = mp.new_post_id
       FROM moved_posts mp
       WHERE mp.old_post_id <> mp.new_post_id AND pr.post_id = mp.old_post_id AND
-        EXISTS (SELECT 1 FROM moved_posts mr WHERE mr.new_post_id = pr.reply_id)
+        EXISTS (SELECT 1 FROM moved_posts mr WHERE mr.new_post_id = pr.reply_post_id)
     SQL
   end
 
@@ -283,8 +283,8 @@ class PostMover
     DB.exec <<~SQL
       DELETE
       FROM post_replies pr USING moved_posts mp, posts p, posts r
-      WHERE (pr.reply_id = mp.old_post_id OR pr.post_id = mp.old_post_id) AND
+      WHERE (pr.reply_post_id = mp.old_post_id OR pr.post_id = mp.old_post_id) AND
-        p.id = pr.post_id AND r.id = pr.reply_id AND p.topic_id <> r.topic_id
+        p.id = pr.post_id AND r.id = pr.reply_post_id AND p.topic_id <> r.topic_id
     SQL
   end
 

app/models/post_reply.rb

@@ -1,10 +1,14 @@
 # frozen_string_literal: true
 
 class PostReply < ActiveRecord::Base
-  belongs_to :post
+  self.ignored_columns = %w{
-  belongs_to :reply, class_name: 'Post'
+    reply_id
+  }
 
-  validates_uniqueness_of :reply_id, scope: :post_id
+  belongs_to :post
+  belongs_to :reply, foreign_key: :reply_post_id, class_name: 'Post'
+
+  validates_uniqueness_of :reply_post_id, scope: :post_id
   validate :ensure_same_topic
 
   private
@@ -24,12 +28,12 @@ end
 # Table name: post_replies
 #
 #  post_id       :integer
-#  reply_id   :integer
 #  created_at    :datetime         not null
 #  updated_at    :datetime         not null
+#  reply_post_id :integer
 #
 # Indexes
 #
-#  index_post_replies_on_post_id_and_reply_id  (post_id,reply_id) UNIQUE
+#  index_post_replies_on_post_id_and_reply_post_id  (post_id,reply_post_id) UNIQUE
-#  index_post_replies_on_reply_id              (reply_id)
+#  index_post_replies_on_reply_post_id              (reply_post_id)
 #

db/migrate/20200116140132_rename_reply_id_column.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+class RenameReplyIdColumn < ActiveRecord::Migration[6.0]
+  def up
+    Migration::ColumnDropper.mark_readonly(:post_replies, :reply_id)
+
+    add_column :post_replies, :reply_post_id, :integer
+
+    execute <<~SQL
+      UPDATE post_replies
+      SET reply_post_id = reply_id
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/migrate/20200117141138_update_post_reply_indexes.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+class UpdatePostReplyIndexes < ActiveRecord::Migration[6.0]
+  # Adding and removing indexes concurrently doesn't work within transaction.
+  # It also needs existence checks for indexes in case the migration fails and needs to run again.
+  disable_ddl_transaction!
+
+  def up
+    if !index_exists?(:post_replies, [:post_id, :reply_post_id])
+      add_index :post_replies, [:post_id, :reply_post_id], unique: true, algorithm: :concurrently
+    end
+
+    if !index_exists?(:post_replies, [:reply_post_id])
+      add_index :post_replies, [:reply_post_id], algorithm: :concurrently
+    end
+
+    if index_exists?(:post_replies, [:post_id, :reply_id])
+      remove_index :post_replies, column: [:post_id, :reply_id], algorithm: :concurrently
+    end
+
+    if index_exists?(:post_replies, [:reply_id])
+      remove_index :post_replies, column: [:reply_id], algorithm: :concurrently
+    end
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/email/sender.rb

@@ -114,7 +114,7 @@ module Email
 
         referenced_posts = Post.includes(:incoming_email)
           .joins("INNER JOIN post_replies ON post_replies.post_id = posts.id ")
-          .where("post_replies.reply_id = ?", post_id)
+          .where("post_replies.reply_post_id = ?", post_id)
           .order(id: :desc)
 
         referenced_post_message_ids = referenced_posts.map do |referenced_post|

lib/post_destroyer.rb

@@ -307,10 +307,10 @@ class PostDestroyer
   end
 
   def remove_associated_replies
-    post_ids = PostReply.where(reply_id: @post.id).pluck(:post_id)
+    post_ids = PostReply.where(reply_post_id: @post.id).pluck(:post_id)
 
     if post_ids.present?
-      PostReply.where(reply_id: @post.id).delete_all
+      PostReply.where(reply_post_id: @post.id).delete_all
       Post.where(id: post_ids).each { |p| p.update_column :reply_count, p.replies.count }
     end
   end

lib/tasks/import.rake

@@ -45,7 +45,7 @@ def insert_post_replies
   log "Inserting post replies..."
 
   DB.exec <<-SQL
-    INSERT INTO post_replies (post_id, reply_id, created_at, updated_at)
+    INSERT INTO post_replies (post_id, reply_post_id, created_at, updated_at)
          SELECT p2.id, p.id, p.created_at, p.created_at
            FROM posts p
      INNER JOIN posts p2 ON p2.post_number = p.reply_to_post_number AND p2.topic_id = p.topic_id
@@ -300,7 +300,7 @@ def update_posts
   # WITH X AS (
   #   SELECT pr.post_id, p.user_id
   #     FROM post_replies pr
-  #     JOIN posts p ON p.id = pr.reply_id
+  #     JOIN posts p ON p.id = pr.reply_post_id
   # )
   # UPDATE posts
   #    SET reply_to_user_id = X.user_id

script/bulk_import/discourse_merger.rb

@@ -511,7 +511,7 @@ class BulkImport::DiscourseMerger < BulkImport::Base
   end
 
   def process_post_reply(post_reply)
-    post_reply['reply_id'] = post_id_from_imported_id(post_reply['reply_id']) if post_reply['reply_id']
+    post_reply['reply_post_id'] = post_id_from_imported_id(post_reply['reply_post_id']) if post_reply['reply_post_id']
     post_reply
   end
 

spec/components/post_destroyer_spec.rb

@@ -639,7 +639,7 @@ describe PostDestroyer do
     describe 'with a reply' do
 
       fab!(:reply) { Fabricate(:basic_reply, user: coding_horror, topic: post.topic) }
-      let!(:post_reply) { PostReply.create(post_id: post.id, reply_id: reply.id) }
+      let!(:post_reply) { PostReply.create(post_id: post.id, reply_post_id: reply.id) }
 
       it 'changes the post count of the topic' do
         post.reload

spec/models/post_mover_spec.rb

@@ -955,10 +955,10 @@ describe PostMover do
             expect(topic.highest_post_number).to eq(p4.post_number)
 
             # updates replies for posts moved to same topic
-            expect(PostReply.where(reply_id: p2.id).pluck(:post_id)).to contain_exactly(new_first.id)
+            expect(PostReply.where(reply_post_id: p2.id).pluck(:post_id)).to contain_exactly(new_first.id)
 
             # leaves replies to the first post of the original topic unchanged
-            expect(PostReply.where(reply_id: p3.id).pluck(:post_id)).to contain_exactly(p1.id)
+            expect(PostReply.where(reply_post_id: p3.id).pluck(:post_id)).to contain_exactly(p1.id)
           end
 
           it "preserves post actions in the new post" do

spec/requests/posts_controller_spec.rb

@@ -236,7 +236,7 @@ describe PostsController do
 
       describe "can delete replies" do
         before do
-          PostReply.create(post_id: post1.id, reply_id: post2.id)
+          PostReply.create(post_id: post1.id, reply_post_id: post2.id)
         end
 
         it "deletes the post and the reply to it" do

spec/requests/topics_controller_spec.rb

@@ -162,7 +162,7 @@ RSpec.describe TopicsController do
             user = sign_in(moderator)
             p1 = Fabricate(:post, topic: topic, user: user)
             p2 = Fabricate(:post, topic: topic, user: user, reply_to_post_number: p1.post_number)
-            PostReply.create(post_id: p1.id, reply_id: p2.id)
+            PostReply.create(post_id: p1.id, reply_post_id: p2.id)
 
             post "/t/#{topic.id}/move-posts.json", params: {
               title: 'new topic title',