From 85e97ecab6db67faefb64557af9b2271d2b23735 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 30 Nov 2017 03:50:05 +0100 Subject: [PATCH] Fix too many forwards (#5854) * Avoid sending explicit Undo->Announce when original deleted * Do not forward a reply back to the server that sent it * Deduplicate inboxes of rebloggers' followers for delete forwarding * Adjust test * Fix wrong class, bad SQL, wrong variable, outdated comment --- app/lib/activitypub/activity/create.rb | 2 +- app/lib/activitypub/activity/delete.rb | 7 +++++-- app/models/account.rb | 4 ++++ app/services/remove_status_service.rb | 12 +++++++++--- app/workers/activitypub/raw_distribution_worker.rb | 4 ++-- spec/lib/activitypub/activity/delete_spec.rb | 13 ++++++------- 6 files changed, 27 insertions(+), 15 deletions(-) diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index 66e4f7c5e..3e95cbd06 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -210,7 +210,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity def forward_for_reply return unless @json['signature'].present? && reply_to_local? - ActivityPub::RawDistributionWorker.perform_async(Oj.dump(@json), replied_to_status.account_id) + ActivityPub::RawDistributionWorker.perform_async(Oj.dump(@json), replied_to_status.account_id, [@account.preferred_inbox_url]) end def lock_options diff --git a/app/lib/activitypub/activity/delete.rb b/app/lib/activitypub/activity/delete.rb index 4c6afb090..d0fb49342 100644 --- a/app/lib/activitypub/activity/delete.rb +++ b/app/lib/activitypub/activity/delete.rb @@ -30,8 +30,11 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity def forward_for_reblogs(status) return if @json['signature'].blank? - ActivityPub::RawDistributionWorker.push_bulk(status.reblogs.includes(:account).references(:account).merge(Account.local).pluck(:account_id)) do |account_id| - [payload, account_id] + rebloggers_ids = status.reblogs.includes(:account).references(:account).merge(Account.local).pluck(:account_id) + inboxes = Account.where(id: ::Follow.where(target_account_id: rebloggers_ids).select(:account_id)).inboxes - [@account.preferred_inbox_url] + + ActivityPub::DeliveryWorker.push_bulk(inboxes) do |inbox_url| + [payload, rebloggers_ids.first, inbox_url] end end diff --git a/app/models/account.rb b/app/models/account.rb index f3f604eda..b6d46b2cc 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -214,6 +214,10 @@ class Account < ApplicationRecord Rails.cache.fetch("exclude_domains_for:#{id}") { domain_blocks.pluck(:domain) } end + def preferred_inbox_url + shared_inbox_url.presence || inbox_url + end + class << self def readonly_attributes super - %w(statuses_count following_count followers_count) diff --git a/app/services/remove_status_service.rb b/app/services/remove_status_service.rb index c75627205..9f603bb36 100644 --- a/app/services/remove_status_service.rb +++ b/app/services/remove_status_service.rb @@ -3,7 +3,7 @@ class RemoveStatusService < BaseService include StreamEntryRenderer - def call(status) + def call(status, options = {}) @payload = Oj.dump(event: :delete, payload: status.id.to_s) @status = status @account = status.account @@ -11,6 +11,7 @@ class RemoveStatusService < BaseService @mentions = status.mentions.includes(:account).to_a @reblogs = status.reblogs.to_a @stream_entry = status.stream_entry + @options = options remove_from_self if status.account.local? remove_from_followers @@ -22,7 +23,12 @@ class RemoveStatusService < BaseService @status.destroy! - return unless @account.local? + # There is no reason to send out Undo activities when the + # cause is that the original object has been removed, since + # original object being removed implicitly removes reblogs + # of it. The Delete activity of the original is forwarded + # separately. + return if !@account.local? || @options[:original_removed] remove_from_remote_followers remove_from_remote_affected @@ -104,7 +110,7 @@ class RemoveStatusService < BaseService # without us being able to do all the fancy stuff @reblogs.each do |reblog| - RemoveStatusService.new.call(reblog) + RemoveStatusService.new.call(reblog, original_removed: true) end end diff --git a/app/workers/activitypub/raw_distribution_worker.rb b/app/workers/activitypub/raw_distribution_worker.rb index d73466f6e..41e61132f 100644 --- a/app/workers/activitypub/raw_distribution_worker.rb +++ b/app/workers/activitypub/raw_distribution_worker.rb @@ -5,10 +5,10 @@ class ActivityPub::RawDistributionWorker sidekiq_options queue: 'push' - def perform(json, source_account_id) + def perform(json, source_account_id, exclude_inboxes = []) @account = Account.find(source_account_id) - ActivityPub::DeliveryWorker.push_bulk(inboxes) do |inbox_url| + ActivityPub::DeliveryWorker.push_bulk(inboxes - exclude_inboxes) do |inbox_url| [json, @account.id, inbox_url] end rescue ActiveRecord::RecordNotFound diff --git a/spec/lib/activitypub/activity/delete_spec.rb b/spec/lib/activitypub/activity/delete_spec.rb index 38254e31c..37b93ecf7 100644 --- a/spec/lib/activitypub/activity/delete_spec.rb +++ b/spec/lib/activitypub/activity/delete_spec.rb @@ -1,8 +1,8 @@ require 'rails_helper' RSpec.describe ActivityPub::Activity::Delete do - let(:sender) { Fabricate(:account, domain: 'example.com') } - let(:status) { Fabricate(:status, account: sender, uri: 'foobar') } + let(:sender) { Fabricate(:account, domain: 'example.com') } + let(:status) { Fabricate(:status, account: sender, uri: 'foobar') } let(:json) do { @@ -30,13 +30,13 @@ RSpec.describe ActivityPub::Activity::Delete do context 'when the status has been reblogged' do describe '#perform' do subject { described_class.new(json, sender) } - let(:reblogger) { Fabricate(:account) } - let(:follower) { Fabricate(:account, username: 'follower', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } + let!(:reblogger) { Fabricate(:account) } + let!(:follower) { Fabricate(:account, username: 'follower', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } + let!(:reblog) { Fabricate(:status, account: reblogger, reblog: status) } before do stub_request(:post, 'http://example.com/inbox').to_return(status: 200) follower.follow!(reblogger) - Fabricate(:status, account: reblogger, reblog: status) subject.perform end @@ -45,8 +45,7 @@ RSpec.describe ActivityPub::Activity::Delete do end it 'sends delete activity to followers of rebloggers' do - # one for Delete original post, and one for Undo reblog (normal delivery) - expect(a_request(:post, 'http://example.com/inbox')).to have_been_made.twice + expect(a_request(:post, 'http://example.com/inbox')).to have_been_made.once end end end