diff --git a/app/services/activitypub/fetch_all_replies_service.rb b/app/services/activitypub/fetch_all_replies_service.rb index cba476ab46c30c..292d5aad301c01 100644 --- a/app/services/activitypub/fetch_all_replies_service.rb +++ b/app/services/activitypub/fetch_all_replies_service.rb @@ -24,9 +24,15 @@ def call(collection_or_uri, allow_synchronous_requests: true, request_id: nil) def filtered_replies return if @items.nil? - # find all statuses that we *shouldn't* update the replies for, and use that as a filter - # Typically we assume this is smaller than the replies we *should* fetch, - # so we minimize the number of uris we should load here. + # Find all statuses that we *shouldn't* update the replies for, and use that as a filter. + # We don't assume that we have the statuses before they're created, + # hence the negative filter - + # "keep all these uris except the ones we already have" + # instead of + # "keep all these uris that match some conditions on existing Status objects" + # + # Typically we assume the number of replies we *shouldn't* fetch is smaller than the + # replies we *should* fetch, so we also minimize the number of uris we should load here. uris = @items.map { |item| value_or_id(item) } dont_update = Status.where(uri: uris).shouldnt_fetch_replies.pluck(:uri) diff --git a/spec/services/activitypub/fetch_all_replies_service_spec.rb b/spec/services/activitypub/fetch_all_replies_service_spec.rb new file mode 100644 index 00000000000000..5ab1536d519f9b --- /dev/null +++ b/spec/services/activitypub/fetch_all_replies_service_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe ActivityPub::FetchAllRepliesService do + subject { described_class.new } + + let(:actor) { Fabricate(:account, domain: 'example.com', uri: 'http://example.com/account') } + let(:status) { Fabricate(:status, account: actor) } + let(:collection_uri) { 'http://example.com/replies/1' } + + let(:items) do + [ + 'http://example.com/self-reply-1', + 'http://example.com/self-reply-2', + 'http://example.com/self-reply-3', + 'http://other.com/other-reply-1', + 'http://other.com/other-reply-2', + 'http://other.com/other-reply-3', + 'http://example.com/self-reply-4', + 'http://example.com/self-reply-5', + 'http://example.com/self-reply-6', + ] + end + + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + type: 'Collection', + id: collection_uri, + items: items, + }.with_indifferent_access + end + + describe '#call' do + it 'fetches more than the default maximum and from multiple domains' do + allow(FetchReplyWorker).to receive(:push_bulk) + + subject.call(payload) + + expect(FetchReplyWorker).to have_received(:push_bulk).with(%w(http://example.com/self-reply-1 http://example.com/self-reply-2 http://example.com/self-reply-3 http://other.com/other-reply-1 http://other.com/other-reply-2 http://other.com/other-reply-3 http://example.com/self-reply-4 + http://example.com/self-reply-5 http://example.com/self-reply-6)) + end + + context 'with a recent status' do + before do + Fabricate(:status, uri: 'http://example.com/self-reply-2', fetched_replies_at: 1.second.ago, local: false) + end + + it 'skips statuses that have been updated recently' do + allow(FetchReplyWorker).to receive(:push_bulk) + + subject.call(payload) + + expect(FetchReplyWorker).to have_received(:push_bulk).with(%w(http://example.com/self-reply-1 http://example.com/self-reply-3 http://other.com/other-reply-1 http://other.com/other-reply-2 http://other.com/other-reply-3 http://example.com/self-reply-4 http://example.com/self-reply-5 http://example.com/self-reply-6)) + end + end + + context 'with an old status' do + before do + Fabricate(:status, uri: 'http://other.com/other-reply-1', fetched_replies_at: 1.year.ago, created_at: 1.year.ago, account: actor) + end + + it 'updates the time that fetched statuses were last fetched' do + allow(FetchReplyWorker).to receive(:push_bulk) + + subject.call(payload) + + expect(Status.find_by(uri: 'http://other.com/other-reply-1').fetched_replies_at).to be >= 1.minute.ago + end + end + end +end