PERF: Reduce DB queries when serializing ignore/mute information (#8629)

* PERF: Cache ignored and muted user ids in the current_user object
* PERF: Avoid DB queries when checking ignore/mute permission in guardian
This commit is contained in:
David Taylor 2020-01-02 13:04:08 +00:00 committed by GitHub
parent a9f90cdec3
commit 45c5f56ffc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 73 additions and 23 deletions

View File

@ -1118,7 +1118,7 @@ class UsersController < ApplicationController
user = fetch_user_from_params user = fetch_user_from_params
if params[:notification_level] == "ignore" if params[:notification_level] == "ignore"
guardian.ensure_can_ignore_user!(user.id) guardian.ensure_can_ignore_user!(user)
MutedUser.where(user: current_user, muted_user: user).delete_all MutedUser.where(user: current_user, muted_user: user).delete_all
ignored_user = IgnoredUser.find_by(user: current_user, ignored_user: user) ignored_user = IgnoredUser.find_by(user: current_user, ignored_user: user)
if ignored_user.present? if ignored_user.present?
@ -1127,7 +1127,7 @@ class UsersController < ApplicationController
IgnoredUser.create!(user: current_user, ignored_user: user, expiring_at: Time.parse(params[:expiring_at])) IgnoredUser.create!(user: current_user, ignored_user: user, expiring_at: Time.parse(params[:expiring_at]))
end end
elsif params[:notification_level] == "mute" elsif params[:notification_level] == "mute"
guardian.ensure_can_mute_user!(user.id) guardian.ensure_can_mute_user!(user)
IgnoredUser.where(user: current_user, ignored_user: user).delete_all IgnoredUser.where(user: current_user, ignored_user: user).delete_all
MutedUser.find_or_create_by!(user: current_user, muted_user: user) MutedUser.find_or_create_by!(user: current_user, muted_user: user)
elsif params[:notification_level] == "normal" elsif params[:notification_level] == "normal"

View File

@ -83,6 +83,9 @@ class User < ActiveRecord::Base
has_many :muted_user_records, class_name: 'MutedUser' has_many :muted_user_records, class_name: 'MutedUser'
has_many :muted_users, through: :muted_user_records has_many :muted_users, through: :muted_user_records
has_many :ignored_user_records, class_name: 'IgnoredUser'
has_many :ignored_users, through: :ignored_user_records
has_many :api_keys, dependent: :destroy has_many :api_keys, dependent: :destroy
has_many :push_subscriptions, dependent: :destroy has_many :push_subscriptions, dependent: :destroy
@ -468,9 +471,19 @@ class User < ActiveRecord::Base
@unread_total_notifications = nil @unread_total_notifications = nil
@unread_pms = nil @unread_pms = nil
@user_fields = nil @user_fields = nil
@ignored_user_ids = nil
@muted_user_ids = nil
super super
end end
def ignored_user_ids
@ignored_user_ids ||= ignored_users.pluck(:id)
end
def muted_user_ids
@muted_user_ids ||= muted_users.pluck(:id)
end
def unread_notifications_of_type(notification_type) def unread_notifications_of_type(notification_type)
# perf critical, much more efficient than AR # perf critical, much more efficient than AR
sql = <<~SQL sql = <<~SQL

View File

@ -255,19 +255,21 @@ class UserSerializer < BasicUserSerializer
end end
def ignored def ignored
IgnoredUser.where(user_id: scope.user&.id, ignored_user_id: object.id).exists? scope_ignored_user_ids = scope.user&.ignored_user_ids || []
scope_ignored_user_ids.include?(object.id)
end end
def muted def muted
MutedUser.where(user_id: scope.user&.id, muted_user_id: object.id).exists? scope_muted_user_ids = scope.user&.muted_user_ids || []
scope_muted_user_ids.include?(object.id)
end end
def can_mute_user def can_mute_user
scope.can_mute_user?(object.id) scope.can_mute_user?(object)
end end
def can_ignore_user def can_ignore_user
scope.can_ignore_user?(object.id) scope.can_ignore_user?(object)
end end
# Needed because 'send_private_message_to_user' will always return false # Needed because 'send_private_message_to_user' will always return false

View File

@ -463,10 +463,10 @@ class Guardian
UserExport.where(user_id: @user.id, created_at: (Time.zone.now.beginning_of_day..Time.zone.now.end_of_day)).count == 0 UserExport.where(user_id: @user.id, created_at: (Time.zone.now.beginning_of_day..Time.zone.now.end_of_day)).count == 0
end end
def can_mute_user?(user_id) def can_mute_user?(target_user)
can_mute_users? && can_mute_users? &&
@user.id != user_id && @user.id != target_user.id &&
User.where(id: user_id, admin: false, moderator: false).exists? !target_user.staff?
end end
def can_mute_users? def can_mute_users?
@ -474,8 +474,8 @@ class Guardian
@user.staff? || @user.trust_level >= TrustLevel.levels[:basic] @user.staff? || @user.trust_level >= TrustLevel.levels[:basic]
end end
def can_ignore_user?(user_id) def can_ignore_user?(target_user)
can_ignore_users? && @user.id != user_id && User.where(id: user_id, admin: false, moderator: false).exists? can_ignore_users? && @user.id != target_user.id && !target_user.staff?
end end
def can_ignore_users? def can_ignore_users?

View File

@ -2759,7 +2759,7 @@ describe Guardian do
context "when ignored user is the same as guardian user" do context "when ignored user is the same as guardian user" do
it 'does not allow ignoring user' do it 'does not allow ignoring user' do
expect(guardian.can_ignore_user?(trust_level_2.id)).to eq(false) expect(guardian.can_ignore_user?(trust_level_2)).to eq(false)
end end
end end
@ -2767,13 +2767,13 @@ describe Guardian do
let!(:admin) { Fabricate(:user, admin: true) } let!(:admin) { Fabricate(:user, admin: true) }
it 'does not allow ignoring user' do it 'does not allow ignoring user' do
expect(guardian.can_ignore_user?(admin.id)).to eq(false) expect(guardian.can_ignore_user?(admin)).to eq(false)
end end
end end
context "when ignored user is a normal user" do context "when ignored user is a normal user" do
it 'allows ignoring user' do it 'allows ignoring user' do
expect(guardian.can_ignore_user?(another_user.id)).to eq(true) expect(guardian.can_ignore_user?(another_user)).to eq(true)
end end
end end
@ -2782,7 +2782,7 @@ describe Guardian do
let!(:trust_level_1) { build(:user, trust_level: 1) } let!(:trust_level_1) { build(:user, trust_level: 1) }
it 'does not allow ignoring user' do it 'does not allow ignoring user' do
expect(guardian.can_ignore_user?(another_user.id)).to eq(false) expect(guardian.can_ignore_user?(another_user)).to eq(false)
end end
end end
@ -2790,7 +2790,7 @@ describe Guardian do
let(:guardian) { Guardian.new(admin) } let(:guardian) { Guardian.new(admin) }
it 'allows ignoring user' do it 'allows ignoring user' do
expect(guardian.can_ignore_user?(another_user.id)).to eq(true) expect(guardian.can_ignore_user?(another_user)).to eq(true)
end end
end end
@ -2798,7 +2798,7 @@ describe Guardian do
let(:guardian) { Guardian.new(trust_level_2) } let(:guardian) { Guardian.new(trust_level_2) }
it 'allows ignoring user' do it 'allows ignoring user' do
expect(guardian.can_ignore_user?(another_user.id)).to eq(true) expect(guardian.can_ignore_user?(another_user)).to eq(true)
end end
end end
end end
@ -2809,7 +2809,7 @@ describe Guardian do
context "when muted user is the same as guardian user" do context "when muted user is the same as guardian user" do
it 'does not allow muting user' do it 'does not allow muting user' do
expect(guardian.can_mute_user?(trust_level_1.id)).to eq(false) expect(guardian.can_mute_user?(trust_level_1)).to eq(false)
end end
end end
@ -2817,13 +2817,13 @@ describe Guardian do
let!(:admin) { Fabricate(:user, admin: true) } let!(:admin) { Fabricate(:user, admin: true) }
it 'does not allow muting user' do it 'does not allow muting user' do
expect(guardian.can_mute_user?(admin.id)).to eq(false) expect(guardian.can_mute_user?(admin)).to eq(false)
end end
end end
context "when muted user is a normal user" do context "when muted user is a normal user" do
it 'allows muting user' do it 'allows muting user' do
expect(guardian.can_mute_user?(another_user.id)).to eq(true) expect(guardian.can_mute_user?(another_user)).to eq(true)
end end
end end
@ -2832,7 +2832,7 @@ describe Guardian do
let!(:trust_level_0) { build(:user, trust_level: 0) } let!(:trust_level_0) { build(:user, trust_level: 0) }
it 'does not allow muting user' do it 'does not allow muting user' do
expect(guardian.can_mute_user?(another_user.id)).to eq(false) expect(guardian.can_mute_user?(another_user)).to eq(false)
end end
end end
@ -2840,7 +2840,7 @@ describe Guardian do
let(:guardian) { Guardian.new(admin) } let(:guardian) { Guardian.new(admin) }
it 'allows muting user' do it 'allows muting user' do
expect(guardian.can_mute_user?(another_user.id)).to eq(true) expect(guardian.can_mute_user?(another_user)).to eq(true)
end end
end end
@ -2848,7 +2848,7 @@ describe Guardian do
let(:guardian) { Guardian.new(trust_level_1) } let(:guardian) { Guardian.new(trust_level_1) }
it 'allows muting user' do it 'allows muting user' do
expect(guardian.can_mute_user?(another_user.id)).to eq(true) expect(guardian.can_mute_user?(another_user)).to eq(true)
end end
end end
end end

View File

@ -192,6 +192,41 @@ describe UserSerializer do
end end
end end
end end
describe "ignored and muted" do
fab!(:viewing_user) { Fabricate(:user) }
let(:scope) { Guardian.new(viewing_user) }
it 'returns false values for muted and ignored' do
expect(json[:ignored]).to eq(false)
expect(json[:muted]).to eq(false)
end
context 'when ignored' do
before do
Fabricate(:ignored_user, user: viewing_user, ignored_user: user)
viewing_user.reload
end
it 'returns true for ignored' do
expect(json[:ignored]).to eq(true)
expect(json[:muted]).to eq(false)
end
end
context 'when muted' do
before do
Fabricate(:muted_user, user: viewing_user, muted_user: user)
viewing_user.reload
end
it 'returns true for muted' do
expect(json[:muted]).to eq(true)
expect(json[:ignored]).to eq(false)
end
end
end
end end
context "with custom_fields" do context "with custom_fields" do