From a7628c1d749f2d0c2d0744e5bdeaac8145554562 Mon Sep 17 00:00:00 2001
From: Sam <sam.saffron@gmail.com>
Date: Fri, 18 Jan 2019 13:18:20 +1100
Subject: [PATCH] FIX: use ordered_posts for last post check, not the posts
 relation

The `posts` relation on `Topic` is not ordered. Using `Topic.posts.first`
is basically the same as asking for a random post, it will depend on DB
order. This breaks on Topic merge and split for example.

Additionally, a huge problem with that is that it forces active record down
a slow path. `Topic.posts.first` is extremely slow on giant topics, since
it has no default ordering it appears AR materializes the entire set prior
to doing `first`.

This commit also illustrates the importance of testing, initially I only
fixed the second instance of the problem in `post_validator.rb` but testing
revealed that the problem was repeated at the top of the file.

Longer term we should consider a larger change of default ordering the posts
relations so people do not fall down this trap anymore.
---
 lib/validators/post_validator.rb                  | 4 ++--
 spec/components/validators/post_validator_spec.rb | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/validators/post_validator.rb b/lib/validators/post_validator.rb
index 9c1e7fe3b04..b18abd9bd5b 100644
--- a/lib/validators/post_validator.rb
+++ b/lib/validators/post_validator.rb
@@ -146,7 +146,7 @@ class Validators::PostValidator < ActiveModel::Validator
     return if SiteSetting.max_consecutive_replies == 0 || post.id || post.acting_user&.staff? || private_message?(post)
 
     topic = post.topic
-    return if topic&.posts&.first&.user == post.user
+    return if topic&.ordered_posts&.first&.user == post.user
 
     last_posts_count = DB.query_single(<<~SQL, topic_id: post.topic_id, user_id: post.acting_user.id, max_replies: SiteSetting.max_consecutive_replies).first
       SELECT COUNT(*)
@@ -164,7 +164,7 @@ class Validators::PostValidator < ActiveModel::Validator
     return if last_posts_count < SiteSetting.max_consecutive_replies
 
     guardian = Guardian.new(post.acting_user)
-    if guardian.can_edit?(topic.posts.last)
+    if guardian.can_edit?(topic.ordered_posts.last)
       post.errors.add(:base, I18n.t(:max_consecutive_replies, count: SiteSetting.max_consecutive_replies))
     end
   end
diff --git a/spec/components/validators/post_validator_spec.rb b/spec/components/validators/post_validator_spec.rb
index f368daf4852..91f36ab2c65 100644
--- a/spec/components/validators/post_validator_spec.rb
+++ b/spec/components/validators/post_validator_spec.rb
@@ -239,11 +239,11 @@ describe Validators::PostValidator do
     end
 
     it "should not allow posting more than 2 consecutive replies" do
-      Post.create(user: other_user, topic: topic, raw: "post number 0")
-      Post.create(user: user, topic: topic, raw: "post number 1")
-      Post.create(user: user, topic: topic, raw: "post number 2")
+      Post.create!(user: user, topic: topic, raw: "post number 2", post_number: 2)
+      Post.create!(user: user, topic: topic, raw: "post number 3", post_number: 3)
+      Post.create!(user: other_user, topic: topic, raw: "post number 1", post_number: 1)
 
-      post = Post.new(user: user, topic: topic, raw: "post number 3")
+      post = Post.new(user: user, topic: topic, raw: "post number 4", post_number: 4)
       validator.force_edit_last_validator(post)
       expect(post.errors.count).to eq(1)
     end