FEATURE: limit number of notifications per user to 10,000

Introduces a new site setting `max_notifications_per_user`.

Out-of-the-box this is set to 10,000. If a user exceeds this number of
notifications, we will delete the oldest notifications keeping only 10,000.

To disable this safeguard set the setting to 0.

Enforcement happens weekly.

This is in place to protect the system from pathological states where a
single user has enormous amounts of notifications causing various queries
to time out. In practice nobody looks back more than a few hundred notifications.
This commit is contained in:
Sam Saffron 2020-02-24 11:42:50 +11:00
parent f93de763b7
commit 372f6f4f22
No known key found for this signature in database
GPG Key ID: B9606168D2FFD9F5
5 changed files with 46 additions and 0 deletions

View File

@ -14,6 +14,7 @@ module Jobs
UserAuthToken.cleanup! UserAuthToken.cleanup!
Upload.reset_unknown_extensions! Upload.reset_unknown_extensions!
Email::Cleaner.delete_rejected! Email::Cleaner.delete_rejected!
Notification.purge_old!
end end
end end
end end

View File

@ -44,6 +44,25 @@ class Notification < ActiveRecord::Base
send_email unless NotificationConsolidator.new(self).consolidate! send_email unless NotificationConsolidator.new(self).consolidate!
end end
def self.purge_old!
return if SiteSetting.max_notifications_per_user == 0
DB.exec(<<~SQL, SiteSetting.max_notifications_per_user)
DELETE FROM notifications n1
USING (
SELECT * FROM (
SELECT
user_id,
id,
rank() OVER (PARTITION BY user_id ORDER BY id DESC)
FROM notifications
) AS X
WHERE rank = ?
) n2
WHERE n1.user_id = n2.user_id AND n1.id < n2.id
SQL
end
def self.ensure_consistency! def self.ensure_consistency!
DB.exec(<<~SQL, Notification.types[:private_message]) DB.exec(<<~SQL, Notification.types[:private_message])
DELETE DELETE

View File

@ -1994,6 +1994,8 @@ en:
user_selected_primary_groups: "Allow users to set their own primary group" user_selected_primary_groups: "Allow users to set their own primary group"
max_notifications_per_user: "Maximum amount of notifications per user, if this number is exceeded old notifications will be deleted. Enforced weekly. Set to 0 to disable"
user_website_domains_whitelist: "User website will be verified against these domains. Pipe-delimited list." user_website_domains_whitelist: "User website will be verified against these domains. Pipe-delimited list."
allow_profile_backgrounds: "Allow users to upload profile backgrounds." allow_profile_backgrounds: "Allow users to upload profile backgrounds."

View File

@ -592,6 +592,8 @@ users:
user_selected_primary_groups: user_selected_primary_groups:
default: false default: false
client: true client: true
max_notifications_per_user:
default: 10000
groups: groups:
enable_group_directory: enable_group_directory:

View File

@ -414,4 +414,26 @@ describe Notification do
end end
end end
end end
describe "purge_old!" do
fab!(:user) { Fabricate(:user) }
fab!(:notification1) { Fabricate(:notification, user: user) }
fab!(:notification2) { Fabricate(:notification, user: user) }
fab!(:notification3) { Fabricate(:notification, user: user) }
fab!(:notification4) { Fabricate(:notification, user: user) }
it "does nothing if set to 0" do
SiteSetting.max_notifications_per_user = 0
Notification.purge_old!
expect(Notification.where(user_id: user.id).count).to eq(4)
end
it "correctly limits" do
SiteSetting.max_notifications_per_user = 2
Notification.purge_old!
expect(Notification.where(user_id: user.id).pluck(:id)).to contain_exactly(notification4.id, notification3.id)
end
end
end end