From 9981fa4466052d1f0429b5fef9a7c040490940af Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 13 May 2020 09:05:56 +1000 Subject: [PATCH] FIX: Prevent column name conflicts in reviewable code (#9753) We were getting errors like this in Reviewables in some cases: ``` ActiveRecord::StatementInvalid (PG::AmbiguousColumn: ERROR: column reference "category_id" is ambiguous LINE 4: ...TRUE) OR (reviewable_by_group_id IN (NULL))) AND (category_i... ``` The problem that was making everything go boom is that plugins can add their own custom filters for Reviewables. If one is doing an INNER JOIN on topics, which has its own category_id column, we would get the above AmbiguousColumn error. The solution here is to just make all references to the reviewable columns in the list_for and viewable_by code prefixed by the table name e.g. reviewables.category_id. --- app/models/reviewable.rb | 31 ++++++++++++++++--------------- spec/models/reviewable_spec.rb | 27 +++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 15 deletions(-) diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index 0fd3f4059bd..99c2ad62c0b 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -406,7 +406,7 @@ class Reviewable < ActiveRecord::Base def self.viewable_by(user, order: nil, preload: true) return none unless user.present? - result = self.order(order || 'score desc, created_at desc') + result = self.order(order || 'reviewables.score desc, reviewables.created_at desc') if preload result = result.includes( @@ -422,10 +422,10 @@ class Reviewable < ActiveRecord::Base group_ids = SiteSetting.enable_category_group_review? ? user.group_users.pluck(:group_id) : [] result.where( - '(reviewable_by_moderator AND :staff) OR (reviewable_by_group_id IN (:group_ids))', + '(reviewables.reviewable_by_moderator AND :staff) OR (reviewables.reviewable_by_group_id IN (:group_ids))', staff: user.staff?, group_ids: group_ids - ).where("category_id IS NULL OR category_id IN (?)", Guardian.new(user).allowed_category_ids) + ).where("reviewables.category_id IS NULL OR reviewables.category_id IN (?)", Guardian.new(user).allowed_category_ids) end def self.pending_count(user) @@ -451,13 +451,13 @@ class Reviewable < ActiveRecord::Base order = case sort_order when 'priority_asc' - 'score ASC, created_at DESC' + 'reviewables.score ASC, reviewables.created_at DESC' when 'created_at' - 'created_at DESC, score DESC' + 'reviewables.created_at DESC, reviewables.score DESC' when 'created_at_asc' - 'created_at ASC, score DESC' + 'reviewables.created_at ASC, reviewables.score DESC' else - 'score DESC, created_at DESC' + 'reviewables.score DESC, reviewables.created_at DESC' end if username.present? @@ -469,19 +469,19 @@ class Reviewable < ActiveRecord::Base result = viewable_by(user, order: order) result = by_status(result, status) - result = result.where(type: type) if type - result = result.where(category_id: category_id) if category_id - result = result.where(topic_id: topic_id) if topic_id - result = result.where("created_at >= ?", from_date) if from_date - result = result.where("created_at <= ?", to_date) if to_date + result = result.where('reviewables.type = ?', type) if type + result = result.where('reviewables.category_id = ?', category_id) if category_id + result = result.where('reviewables.topic_id = ?', topic_id) if topic_id + result = result.where("reviewables.created_at >= ?", from_date) if from_date + result = result.where("reviewables.created_at <= ?", to_date) if to_date if min_score > 0 && status == :pending && type.nil? result = result.where( - "score >= ? OR type IN (?)", + "reviewables.score >= ? OR reviewables.type IN (?)", min_score, [ReviewableQueuedPost.name, ReviewableUser.name] ) elsif min_score > 0 - result = result.where("score >= ?", min_score) + result = result.where("reviewables.score >= ?", min_score) end if !custom_filters.empty? @@ -497,7 +497,8 @@ class Reviewable < ActiveRecord::Base # If a reviewable doesn't have a target, allow us to filter on who created that reviewable. if user_id result = result.where( - "(target_created_by_id IS NULL AND created_by_id = :user_id) OR (target_created_by_id = :user_id)", + "(reviewables.target_created_by_id IS NULL AND reviewables.created_by_id = :user_id) + OR (reviewables.target_created_by_id = :user_id)", user_id: user_id ) end diff --git a/spec/models/reviewable_spec.rb b/spec/models/reviewable_spec.rb index 5fd051fc0c5..8d58d6a537c 100644 --- a/spec/models/reviewable_spec.rb +++ b/spec/models/reviewable_spec.rb @@ -479,6 +479,33 @@ RSpec.describe Reviewable, type: :model do expect(results.size).to eq(1) expect(results.first).to eq first_reviewable end + + context "when listing for a moderator with a custom filter that joins tables with same named columns" do + it "should not error" do + first_reviewable = Fabricate(:reviewable) + second_reviewable = Fabricate(:reviewable) + custom_filter = [ + :troublemaker, + Proc.new do |results, value| + results.joins(<<~SQL + INNER JOIN posts p ON p.id = target_id + INNER JOIN topics t ON t.id = p.topic_id + INNER JOIN topic_custom_fields tcf ON tcf.topic_id = t.id + INNER JOIN users u ON u.id = tcf.value::integer + SQL + ) + .where(target_type: Post.name) + .where('tcf.name = ?', 'troublemaker_user_id') + .where('u.username = ?', value) + end + ] + + Reviewable.add_custom_filter(custom_filter) + mod = Fabricate(:moderator) + results = Reviewable.list_for(mod, additional_filters: { troublemaker: 'badguy' }) + expect { results.first }.not_to raise_error + end + end end describe '.by_status' do