PERF: optimize chat user membership cleanup when removing a single user (#29833)

This optimizes a hot path when users are being removed from one or more groups since we use the `on(:user_removed_from_group)` event to call the `Chat::AutoLeaveChannels` with a `user_id` parameter. In such case, we don't need to clear the membership of all users, just that one user.

DEBUG: Also added a "-- event = <event>" comment on top of the SQL queries used by the "AutoLeaveChannels" so they show up in the logs and hopefully facilitate any performance issues that might arise in the future.
This commit is contained in:
Régis Hanol 2024-11-20 09:21:02 +01:00 committed by GitHub
parent eb41a07afb
commit 7cf28fec7b
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 29 additions and 10 deletions

View File

@ -17,6 +17,8 @@ module Chat
attribute :group_id, :integer attribute :group_id, :integer
attribute :channel_id, :integer attribute :channel_id, :integer
attribute :category_id, :integer attribute :category_id, :integer
validates :event, presence: true
end end
step :remove_memberships step :remove_memberships
@ -34,6 +36,7 @@ module Chat
if !group_ids.include?(Group::AUTO_GROUPS[:everyone]) if !group_ids.include?(Group::AUTO_GROUPS[:everyone])
sql = <<~SQL sql = <<~SQL
-- event = #{params.event}
DELETE FROM user_chat_channel_memberships uccm DELETE FROM user_chat_channel_memberships uccm
WHERE NOT EXISTS ( WHERE NOT EXISTS (
SELECT 1 SELECT 1
@ -41,6 +44,7 @@ module Chat
WHERE gu.user_id = uccm.user_id WHERE gu.user_id = uccm.user_id
AND gu.group_id IN (:group_ids) AND gu.group_id IN (:group_ids)
) )
#{params.user_id ? "AND uccm.user_id = #{params.user_id}" : ""}
RETURNING chat_channel_id, user_id RETURNING chat_channel_id, user_id
SQL SQL
@ -54,6 +58,7 @@ module Chat
category_sql = params.category_id ? "AND c.id = #{params.category_id}" : "" category_sql = params.category_id ? "AND c.id = #{params.category_id}" : ""
sql = <<~SQL sql = <<~SQL
-- event = #{params.event}
WITH valid_permissions AS ( WITH valid_permissions AS (
SELECT gu.user_id, cg.category_id SELECT gu.user_id, cg.category_id
FROM group_users gu FROM group_users gu

View File

@ -2,9 +2,10 @@
describe "Automatic user removal from channels" do describe "Automatic user removal from channels" do
fab!(:user_1) { Fabricate(:user, trust_level: 1) } fab!(:user_1) { Fabricate(:user, trust_level: 1) }
let(:user_1_guardian) { Guardian.new(user_1) }
fab!(:user_2) { Fabricate(:user, trust_level: 3) } fab!(:user_2) { Fabricate(:user, trust_level: 3) }
fab!(:user_1_guardian) { Guardian.new(user_1) }
fab!(:secret_group) { Fabricate(:group) } fab!(:secret_group) { Fabricate(:group) }
fab!(:private_category) { Fabricate(:private_category, group: secret_group) } fab!(:private_category) { Fabricate(:private_category, group: secret_group) }
@ -145,6 +146,7 @@ describe "Automatic user removal from channels" do
context "when a user is removed from a private category group" do context "when a user is removed from a private category group" do
context "when the user is in another group that can interact with the channel" do context "when the user is in another group that can interact with the channel" do
fab!(:stealth_group) { Fabricate(:group) } fab!(:stealth_group) { Fabricate(:group) }
before do before do
CategoryGroup.create!( CategoryGroup.create!(
category: private_category, category: private_category,

View File

@ -2,7 +2,7 @@
RSpec.describe Chat::AutoLeaveChannels do RSpec.describe Chat::AutoLeaveChannels do
describe ".call" do describe ".call" do
subject(:result) { described_class.call(params: {}) } subject(:result) { described_class.call(params: { event: :generic_event }) }
let!(:previous_events) { DiscourseEvent.events.dup } let!(:previous_events) { DiscourseEvent.events.dup }
@ -26,6 +26,12 @@ RSpec.describe Chat::AutoLeaveChannels do
expect { result }.to change { ::Chat::UserChatChannelMembership.count }.from(2).to(0) expect { result }.to change { ::Chat::UserChatChannelMembership.count }.from(2).to(0)
end end
it "only removes memberships for a user when filtering by user_id" do
expect {
described_class.call(params: { event: :generic_event, user_id: uccm_1.user_id })
}.to change { ::Chat::UserChatChannelMembership.count }.from(2).to(1)
end
it "publishes automatically removed users" do it "publishes automatically removed users" do
::Chat::Action::PublishAutoRemovedUser ::Chat::Action::PublishAutoRemovedUser
.expects(:call) .expects(:call)
@ -72,7 +78,12 @@ RSpec.describe Chat::AutoLeaveChannels do
::Chat::Action::PublishAutoRemovedUser ::Chat::Action::PublishAutoRemovedUser
.expects(:call) .expects(:call)
.once .once
.with(event: nil, users_removed_map: { uccm.chat_channel_id => [uccm.user_id] }) .with(
event: :generic_event,
users_removed_map: {
uccm.chat_channel_id => [uccm.user_id],
},
)
result result
end end
@ -99,6 +110,7 @@ RSpec.describe Chat::AutoLeaveChannels do
end end
context "with another category/channel/user" do context "with another category/channel/user" do
let(:params) { { event: :generic_event } }
fab!(:user_2) { Fabricate(:user, trust_level: 1) } fab!(:user_2) { Fabricate(:user, trust_level: 1) }
fab!(:category_2) { Fabricate(:private_category, group:) } fab!(:category_2) { Fabricate(:private_category, group:) }
fab!(:chat_channel_2) { Fabricate(:chat_channel, chatable: category_2) } fab!(:chat_channel_2) { Fabricate(:chat_channel, chatable: category_2) }
@ -107,21 +119,21 @@ RSpec.describe Chat::AutoLeaveChannels do
end end
it "supports filtering by user_id" do it "supports filtering by user_id" do
expect { described_class.call(params: { user_id: user.id }) }.to change { expect { described_class.call(params: params.merge(user_id: user.id)) }.to change {
::Chat::UserChatChannelMembership.count ::Chat::UserChatChannelMembership.count
}.from(2).to(1) }.from(2).to(1)
end end
it "supports filtering by channel_id" do it "supports filtering by channel_id" do
expect { described_class.call(params: { channel_id: chat_channel.id }) }.to change { expect {
::Chat::UserChatChannelMembership.count described_class.call(params: params.merge(channel_id: chat_channel.id))
}.from(2).to(1) }.to change { ::Chat::UserChatChannelMembership.count }.from(2).to(1)
end end
it "supports filtering by category_id" do it "supports filtering by category_id" do
expect { described_class.call(params: { category_id: category.id }) }.to change { expect {
::Chat::UserChatChannelMembership.count described_class.call(params: params.merge(category_id: category.id))
}.from(2).to(1) }.to change { ::Chat::UserChatChannelMembership.count }.from(2).to(1)
end end
end end
end end