Discource-C/discourse
1
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-02-08 04:18:23 +00:00
Code Issues Packages Projects Releases Wiki Activity

FIX: use ordered_posts for last post check, not the posts relation

Browse Source 
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.
This commit is contained in:
Sam 2019-01-18 13:18:20 +11:00
parent 8c5c4200ec
commit a7628c1d74
2 changed files with 6 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
lib/validators/post_validator.rb
View File

@ -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) return if SiteSetting.max_consecutive_replies == 0 || post.id || post.acting_user&.staff? || private_message?(post)
topic = post.topic 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 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(*) SELECT COUNT(*)
@ -164,7 +164,7 @@ class Validators::PostValidator < ActiveModel::Validator
return if last_posts_count < SiteSetting.max_consecutive_replies return if last_posts_count < SiteSetting.max_consecutive_replies
guardian = Guardian.new(post.acting_user) 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)) post.errors.add(:base, I18n.t(:max_consecutive_replies, count: SiteSetting.max_consecutive_replies))
end end
end end

8
spec/components/validators/post_validator_spec.rb
View File

@ -239,11 +239,11 @@ describe Validators::PostValidator do
end end
it "should not allow posting more than 2 consecutive replies" do 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 2", post_number: 2)
Post.create(user: user, topic: topic, raw: "post number 1") Post.create!(user: user, topic: topic, raw: "post number 3", post_number: 3)
Post.create(user: user, topic: topic, raw: "post number 2") 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) validator.force_edit_last_validator(post)
expect(post.errors.count).to eq(1) expect(post.errors.count).to eq(1)
end end