From 6ddf0432e71aea7c62f1ee7946da5538d2526a13 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 3 Dec 2018 01:32:08 +0100 Subject: [PATCH 01/13] Improve account suspension speed and completeness (#9290) - Some associations were missing from the clean-up - Some attributes were not reset on suspension - Skip federation and streaming deletes when purging a dead domain - Move account association definitions to concern --- app/models/account.rb | 59 +++------- app/models/concerns/account_associations.rb | 53 +++++++++ app/services/batched_remove_status_service.rb | 6 +- app/services/suspend_account_service.rb | 103 +++++++++++++----- app/workers/admin/suspension_worker.rb | 2 +- lib/mastodon/domains_cli.rb | 6 +- 6 files changed, 153 insertions(+), 76 deletions(-) create mode 100644 app/models/concerns/account_associations.rb diff --git a/app/models/account.rb b/app/models/account.rb index f252633064..fb089de90e 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -49,6 +49,7 @@ class Account < ApplicationRecord USERNAME_RE = /[a-z0-9_]+([a-z0-9_\.-]+[a-z0-9_]+)?/i MENTION_RE = /(?<=^|[^\/[:word:]])@((#{USERNAME_RE})(?:@[a-z0-9\.\-]+[a-z0-9]+)?)/i + include AccountAssociations include AccountAvatar include AccountFinderConcern include AccountHeader @@ -59,9 +60,6 @@ class Account < ApplicationRecord enum protocol: [:ostatus, :activitypub] - # Local users - has_one :user, inverse_of: :account - validates :username, presence: true # Remote user validations @@ -76,45 +74,6 @@ class Account < ApplicationRecord validates :note, length: { maximum: 160 }, if: -> { local? && will_save_change_to_note? } validates :fields, length: { maximum: 4 }, if: -> { local? && will_save_change_to_fields? } - # Timelines - has_many :stream_entries, inverse_of: :account, dependent: :destroy - has_many :statuses, inverse_of: :account, dependent: :destroy - has_many :favourites, inverse_of: :account, dependent: :destroy - has_many :mentions, inverse_of: :account, dependent: :destroy - has_many :notifications, inverse_of: :account, dependent: :destroy - - # Pinned statuses - has_many :status_pins, inverse_of: :account, dependent: :destroy - has_many :pinned_statuses, -> { reorder('status_pins.created_at DESC') }, through: :status_pins, class_name: 'Status', source: :status - - # Endorsements - has_many :account_pins, inverse_of: :account, dependent: :destroy - has_many :endorsed_accounts, through: :account_pins, class_name: 'Account', source: :target_account - - # Media - has_many :media_attachments, dependent: :destroy - - # PuSH subscriptions - has_many :subscriptions, dependent: :destroy - - # Report relationships - has_many :reports - has_many :targeted_reports, class_name: 'Report', foreign_key: :target_account_id - - has_many :report_notes, dependent: :destroy - has_many :custom_filters, inverse_of: :account, dependent: :destroy - - # Moderation notes - has_many :account_moderation_notes, dependent: :destroy - has_many :targeted_moderation_notes, class_name: 'AccountModerationNote', foreign_key: :target_account_id, dependent: :destroy - - # Lists - has_many :list_accounts, inverse_of: :account, dependent: :destroy - has_many :lists, through: :list_accounts - - # Account migrations - belongs_to :moved_to_account, class_name: 'Account', optional: true - scope :remote, -> { where.not(domain: nil) } scope :local, -> { where(domain: nil) } scope :expiring, ->(time) { remote.where.not(subscription_expires_at: nil).where('subscription_expires_at < ?', time) } @@ -452,6 +411,7 @@ class Account < ApplicationRecord before_create :generate_keys before_validation :normalize_domain before_validation :prepare_contents, if: :local? + before_destroy :clean_feed_manager private @@ -477,4 +437,19 @@ class Account < ApplicationRecord def emojifiable_text [note, display_name, fields.map(&:value)].join(' ') end + + def clean_feed_manager + reblog_key = FeedManager.instance.key(:home, id, 'reblogs') + reblogged_id_set = Redis.current.zrange(reblog_key, 0, -1) + + Redis.current.pipelined do + Redis.current.del(FeedManager.instance.key(:home, id)) + Redis.current.del(reblog_key) + + reblogged_id_set.each do |reblogged_id| + reblog_set_key = FeedManager.instance.key(:home, id, "reblogs:#{reblogged_id}") + Redis.current.del(reblog_set_key) + end + end + end end diff --git a/app/models/concerns/account_associations.rb b/app/models/concerns/account_associations.rb new file mode 100644 index 0000000000..0f7482fa6a --- /dev/null +++ b/app/models/concerns/account_associations.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module AccountAssociations + extend ActiveSupport::Concern + + included do + # Local users + has_one :user, inverse_of: :account, dependent: :destroy + + # Timelines + has_many :stream_entries, inverse_of: :account, dependent: :destroy + has_many :statuses, inverse_of: :account, dependent: :destroy + has_many :favourites, inverse_of: :account, dependent: :destroy + has_many :mentions, inverse_of: :account, dependent: :destroy + has_many :notifications, inverse_of: :account, dependent: :destroy + has_many :conversations, class_name: 'AccountConversation', dependent: :destroy, inverse_of: :account + + # Pinned statuses + has_many :status_pins, inverse_of: :account, dependent: :destroy + has_many :pinned_statuses, -> { reorder('status_pins.created_at DESC') }, through: :status_pins, class_name: 'Status', source: :status + + # Endorsements + has_many :account_pins, inverse_of: :account, dependent: :destroy + has_many :endorsed_accounts, through: :account_pins, class_name: 'Account', source: :target_account + + # Media + has_many :media_attachments, dependent: :destroy + + # PuSH subscriptions + has_many :subscriptions, dependent: :destroy + + # Report relationships + has_many :reports, dependent: :destroy, inverse_of: :account + has_many :targeted_reports, class_name: 'Report', foreign_key: :target_account_id, dependent: :destroy, inverse_of: :target_account + + has_many :report_notes, dependent: :destroy + has_many :custom_filters, inverse_of: :account, dependent: :destroy + + # Moderation notes + has_many :account_moderation_notes, dependent: :destroy, inverse_of: :account + has_many :targeted_moderation_notes, class_name: 'AccountModerationNote', foreign_key: :target_account_id, dependent: :destroy, inverse_of: :target_account + + # Lists (that the account is on, not owned by the account) + has_many :list_accounts, inverse_of: :account, dependent: :destroy + has_many :lists, through: :list_accounts + + # Lists (owned by the account) + has_many :owned_lists, class_name: 'List', dependent: :destroy, inverse_of: :account + + # Account migrations + belongs_to :moved_to_account, class_name: 'Account', optional: true + end +end diff --git a/app/services/batched_remove_status_service.rb b/app/services/batched_remove_status_service.rb index 75d7564950..2e61904fc7 100644 --- a/app/services/batched_remove_status_service.rb +++ b/app/services/batched_remove_status_service.rb @@ -9,7 +9,9 @@ class BatchedRemoveStatusService < BaseService # Remove statuses from home feeds # Push delete events to streaming API for home feeds and public feeds # @param [Status] statuses A preferably batched array of statuses - def call(statuses) + # @param [Hash] options + # @option [Boolean] :skip_side_effects + def call(statuses, **options) statuses = Status.where(id: statuses.map(&:id)).includes(:account, :stream_entry).flat_map { |status| [status] + status.reblogs.includes(:account, :stream_entry).to_a } @mentions = statuses.each_with_object({}) { |s, h| h[s.id] = s.active_mentions.includes(:account).to_a } @@ -26,6 +28,8 @@ class BatchedRemoveStatusService < BaseService status.destroy end + return if options[:skip_side_effects] + # Batch by source account statuses.group_by(&:account_id).each_value do |account_statuses| account = account_statuses.first.account diff --git a/app/services/suspend_account_service.rb b/app/services/suspend_account_service.rb index 8fc79b8ad6..6ab6b29019 100644 --- a/app/services/suspend_account_service.rb +++ b/app/services/suspend_account_service.rb @@ -1,6 +1,41 @@ # frozen_string_literal: true class SuspendAccountService < BaseService + ASSOCIATIONS_ON_SUSPEND = %w( + account_pins + active_relationships + block_relationships + blocked_by_relationships + conversation_mutes + conversations + custom_filters + domain_blocks + favourites + follow_requests + list_accounts + media_attachments + mute_relationships + muted_by_relationships + notifications + owned_lists + passive_relationships + report_notes + status_pins + stream_entries + subscriptions + ).freeze + + ASSOCIATIONS_ON_DESTROY = %w( + reports + targeted_moderation_notes + targeted_reports + ).freeze + + # Suspend an account and remove as much of its data as possible + # @param [Account] + # @param [Hash] options + # @option [Boolean] :including_user Remove the user record as well + # @option [Boolean] :destroy Remove the account record instead of suspending def call(account, **options) @account = account @options = options @@ -8,60 +43,66 @@ class SuspendAccountService < BaseService purge_user! purge_profile! purge_content! - unsubscribe_push_subscribers! end private def purge_user! - if @options[:remove_user] - @account.user&.destroy + return if !@account.local? || @account.user.nil? + + if @options[:including_user] + @account.user.destroy else - @account.user&.disable! + @account.user.disable! end end def purge_content! - if @account.local? - ActivityPub::DeliveryWorker.push_bulk(delivery_inboxes) do |inbox_url| - [delete_actor_json, @account.id, inbox_url] - end - end + distribute_delete_actor! if @account.local? @account.statuses.reorder(nil).find_in_batches do |statuses| - BatchedRemoveStatusService.new.call(statuses) + BatchedRemoveStatusService.new.call(statuses, skip_side_effects: @options[:destroy]) end - [ - @account.media_attachments, - @account.stream_entries, - @account.notifications, - @account.favourites, - @account.active_relationships, - @account.passive_relationships, - ].each do |association| - destroy_all(association) + associations_for_destruction.each do |association_name| + destroy_all(@account.public_send(association_name)) end + + @account.destroy if @options[:destroy] end def purge_profile! - @account.suspended = true - @account.display_name = '' - @account.note = '' - @account.statuses_count = 0 + # If the account is going to be destroyed + # there is no point wasting time updating + # its values first + + return if @options[:destroy] + + @account.silenced = false + @account.suspended = true + @account.locked = false + @account.display_name = '' + @account.note = '' + @account.fields = {} + @account.statuses_count = 0 + @account.followers_count = 0 + @account.following_count = 0 + @account.moved_to_account = nil @account.avatar.destroy @account.header.destroy @account.save! end - def unsubscribe_push_subscribers! - destroy_all(@account.subscriptions) - end - def destroy_all(association) association.in_batches.destroy_all end + def distribute_delete_actor! + ActivityPub::DeliveryWorker.push_bulk(delivery_inboxes) do |inbox_url| + [delete_actor_json, @account.id, inbox_url] + end + end + def delete_actor_json return @delete_actor_json if defined?(@delete_actor_json) @@ -77,4 +118,12 @@ class SuspendAccountService < BaseService def delivery_inboxes Account.inboxes + Relay.enabled.pluck(:inbox_url) end + + def associations_for_destruction + if @options[:destroy] + ASSOCIATIONS_ON_SUSPEND + ASSOCIATIONS_ON_DESTROY + else + ASSOCIATIONS_ON_SUSPEND + end + end end diff --git a/app/workers/admin/suspension_worker.rb b/app/workers/admin/suspension_worker.rb index e41465ccca..ae8b24d8c3 100644 --- a/app/workers/admin/suspension_worker.rb +++ b/app/workers/admin/suspension_worker.rb @@ -6,6 +6,6 @@ class Admin::SuspensionWorker sidekiq_options queue: 'pull' def perform(account_id, remove_user = false) - SuspendAccountService.new.call(Account.find(account_id), remove_user: remove_user) + SuspendAccountService.new.call(Account.find(account_id), including_user: remove_user) end end diff --git a/lib/mastodon/domains_cli.rb b/lib/mastodon/domains_cli.rb index a7a5caa115..16e2985842 100644 --- a/lib/mastodon/domains_cli.rb +++ b/lib/mastodon/domains_cli.rb @@ -22,11 +22,7 @@ module Mastodon dry_run = options[:dry_run] ? ' (DRY RUN)' : '' Account.where(domain: domain).find_each do |account| - unless options[:dry_run] - SuspendAccountService.new.call(account) - account.destroy - end - + SuspendAccountService.new.call(account, destroy: true) unless options[:dry_run] removed += 1 say('.', :green, false) end From 58a29db99d410771f62ffb9e8c2ce95e1d3cb4ae Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 3 Dec 2018 01:32:27 +0100 Subject: [PATCH 02/13] Add database statement timeout of 60s (#9382) --- config/database.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/database.yml b/config/database.yml index 82e560515c..90133881ad 100644 --- a/config/database.yml +++ b/config/database.yml @@ -3,6 +3,8 @@ default: &default pool: <%= ENV["DB_POOL"] || ENV['MAX_THREADS'] || 5 %> timeout: 5000 encoding: unicode + variables: + statement_timeout: 60000 development: <<: *default From 2b657c175f9240bda0167d91692cc7dbc3cf7518 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" Date: Tue, 4 Dec 2018 09:26:40 +0900 Subject: [PATCH 03/13] Bump pry-rails from 0.3.7 to 0.3.8 (#9418) Bumps [pry-rails](https://github.com/rweng/pry-rails) from 0.3.7 to 0.3.8. - [Release notes](https://github.com/rweng/pry-rails/releases) - [Commits](https://github.com/rweng/pry-rails/compare/v0.3.7...v0.3.8) Signed-off-by: dependabot[bot] --- Gemfile.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 269c19a95b..7983736206 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -410,13 +410,13 @@ GEM actionmailer (>= 3, < 6) premailer (~> 1.7, >= 1.7.9) private_address_check (0.5.0) - pry (0.12.0) + pry (0.12.2) coderay (~> 1.1.0) method_source (~> 0.9.0) pry-byebug (3.6.0) byebug (~> 10.0) pry (~> 0.10) - pry-rails (0.3.7) + pry-rails (0.3.8) pry (>= 0.10.4) public_suffix (3.0.3) puma (3.12.0) From 4ad6bac447fb75a44f66b0e85bb403e6bfdfd587 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" Date: Tue, 4 Dec 2018 09:26:55 +0900 Subject: [PATCH 04/13] Bump rails-controller-testing from 1.0.2 to 1.0.3 (#9417) Bumps [rails-controller-testing](https://github.com/rails/rails-controller-testing) from 1.0.2 to 1.0.3. - [Release notes](https://github.com/rails/rails-controller-testing/releases) - [Commits](https://github.com/rails/rails-controller-testing/compare/v1.0.2...v1.0.3) Signed-off-by: dependabot[bot] --- Gemfile.lock | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 7983736206..00103d0ff7 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -446,10 +446,10 @@ GEM bundler (>= 1.3.0) railties (= 5.2.1.1) sprockets-rails (>= 2.0.0) - rails-controller-testing (1.0.2) - actionpack (~> 5.x, >= 5.0.1) - actionview (~> 5.x, >= 5.0.1) - activesupport (~> 5.x) + rails-controller-testing (1.0.3) + actionpack (>= 5.0.1.x) + actionview (>= 5.0.1.x) + activesupport (>= 5.0.1.x) rails-dom-testing (2.0.3) activesupport (>= 4.2.0) nokogiri (>= 1.6) From a61ce1c947eda67e62ee2f1abcaf44ebcc60d7ec Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" Date: Tue, 4 Dec 2018 09:27:17 +0900 Subject: [PATCH 05/13] Bump capybara from 3.11.1 to 3.12.0 (#9388) Bumps [capybara](https://github.com/teamcapybara/capybara) from 3.11.1 to 3.12.0. - [Release notes](https://github.com/teamcapybara/capybara/releases) - [Changelog](https://github.com/teamcapybara/capybara/blob/master/History.md) - [Commits](https://github.com/teamcapybara/capybara/compare/3.11.1...3.12.0) Signed-off-by: dependabot[bot] --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index 6e83a52caf..fae38895ea 100644 --- a/Gemfile +++ b/Gemfile @@ -107,7 +107,7 @@ group :production, :test do end group :test do - gem 'capybara', '~> 3.11' + gem 'capybara', '~> 3.12' gem 'climate_control', '~> 0.2' gem 'faker', '~> 1.9' gem 'microformats', '~> 4.0' diff --git a/Gemfile.lock b/Gemfile.lock index 00103d0ff7..2ce4a76f6d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -126,7 +126,7 @@ GEM sshkit (~> 1.3) capistrano-yarn (2.0.2) capistrano (~> 3.0) - capybara (3.11.1) + capybara (3.12.0) addressable mini_mime (>= 0.1.3) nokogiri (~> 1.8) @@ -667,7 +667,7 @@ DEPENDENCIES capistrano-rails (~> 1.4) capistrano-rbenv (~> 2.1) capistrano-yarn (~> 2.0) - capybara (~> 3.11) + capybara (~> 3.12) charlock_holmes (~> 0.7.6) chewy (~> 5.0) cld3 (~> 3.2.0) From e88c6a5c3c188731f7784141b8835c410163cbeb Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 5 Dec 2018 02:12:29 +0100 Subject: [PATCH 06/13] Fix thread depth computation in statuses_controller (#9426) * Add test that should currently fail * Fix depth computation (will still fail if statuses have been filtered out) * Fix handling of broken threads --- app/controllers/statuses_controller.rb | 24 +++++++++++--------- spec/controllers/statuses_controller_spec.rb | 12 ++++++---- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/app/controllers/statuses_controller.rb b/app/controllers/statuses_controller.rb index 0f3fe198f4..15d59fd893 100644 --- a/app/controllers/statuses_controller.rb +++ b/app/controllers/statuses_controller.rb @@ -65,12 +65,13 @@ class StatusesController < ApplicationController private - def create_descendant_thread(depth, statuses) + def create_descendant_thread(starting_depth, statuses) + depth = starting_depth + statuses.size if depth < DESCENDANTS_DEPTH_LIMIT - { statuses: statuses } + { statuses: statuses, starting_depth: starting_depth } else next_status = statuses.pop - { statuses: statuses, next_status: next_status } + { statuses: statuses, starting_depth: starting_depth, next_status: next_status } end end @@ -101,16 +102,19 @@ class StatusesController < ApplicationController @descendant_threads = [] if descendants.present? - statuses = [descendants.first] - depth = 1 + statuses = [descendants.first] + starting_depth = 0 descendants.drop(1).each_with_index do |descendant, index| if descendants[index].id == descendant.in_reply_to_id - depth += 1 statuses << descendant else - @descendant_threads << create_descendant_thread(depth, statuses) + @descendant_threads << create_descendant_thread(starting_depth, statuses) + # The thread is broken, assume it's a reply to the root status + starting_depth = 0 + + # ... unless we can find its ancestor in one of the already-processed threads @descendant_threads.reverse_each do |descendant_thread| statuses = descendant_thread[:statuses] @@ -119,18 +123,16 @@ class StatusesController < ApplicationController end if index.present? - depth += index - statuses.size + starting_depth = descendant_thread[:starting_depth] + index + 1 break end - - depth -= statuses.size end statuses = [descendant] end end - @descendant_threads << create_descendant_thread(depth, statuses) + @descendant_threads << create_descendant_thread(starting_depth, statuses) end @max_descendant_thread_id = @descendant_threads.pop[:statuses].first.id if descendants.size >= DESCENDANTS_LIMIT diff --git a/spec/controllers/statuses_controller_spec.rb b/spec/controllers/statuses_controller_spec.rb index b4f3c5a081..1bb6636c60 100644 --- a/spec/controllers/statuses_controller_spec.rb +++ b/spec/controllers/statuses_controller_spec.rb @@ -115,14 +115,18 @@ describe StatusesController do end it 'assigns @descendant_threads for threads with :next_status key if they are hitting the depth limit' do - stub_const 'StatusesController::DESCENDANTS_DEPTH_LIMIT', 1 + stub_const 'StatusesController::DESCENDANTS_DEPTH_LIMIT', 2 status = Fabricate(:status) - child = Fabricate(:status, in_reply_to_id: status.id) + child0 = Fabricate(:status, in_reply_to_id: status.id) + child1 = Fabricate(:status, in_reply_to_id: child0.id) + child2 = Fabricate(:status, in_reply_to_id: child0.id) get :show, params: { account_username: status.account.username, id: status.id } - expect(assigns(:descendant_threads)[0][:statuses].pluck(:id)).not_to include child.id - expect(assigns(:descendant_threads)[0][:next_status].id).to eq child.id + expect(assigns(:descendant_threads)[0][:statuses].pluck(:id)).not_to include child1.id + expect(assigns(:descendant_threads)[1][:statuses].pluck(:id)).not_to include child2.id + expect(assigns(:descendant_threads)[0][:next_status].id).to eq child1.id + expect(assigns(:descendant_threads)[1][:next_status].id).to eq child2.id end it 'returns a success' do From 9897cf0701d1fb588f9b0defcbd78d0a2095230a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Ngei?= Date: Wed, 5 Dec 2018 05:08:43 +0100 Subject: [PATCH 07/13] Add visible dates for notifications in Notification column (#9423) * add RelativeTimestamp elements * style the elements properly with CSS --- .../features/notifications/components/notification.js | 11 ++++++++++- app/javascript/styles/mastodon/components.scss | 7 ++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/app/javascript/mastodon/features/notifications/components/notification.js b/app/javascript/mastodon/features/notifications/components/notification.js index 8df6830c52..e79bd1a3c3 100644 --- a/app/javascript/mastodon/features/notifications/components/notification.js +++ b/app/javascript/mastodon/features/notifications/components/notification.js @@ -3,6 +3,7 @@ import PropTypes from 'prop-types'; import ImmutablePropTypes from 'react-immutable-proptypes'; import StatusContainer from '../../../containers/status_container'; import AccountContainer from '../../../containers/account_container'; +import RelativeTimestamp from '../../../components/relative_timestamp'; import { injectIntl, FormattedMessage } from 'react-intl'; import Permalink from '../../../components/permalink'; import ImmutablePureComponent from 'react-immutable-pure-component'; @@ -87,9 +88,11 @@ class Notification extends ImmutablePureComponent { + + + -