From 3c47020f93c88eca508f07e6da9c48f44d3f6cfb Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Mon, 23 Mar 2026 15:09:10 +0100 Subject: [PATCH] Allow service to update existing Collections (#38329) --- .../process_featured_collection_service.rb | 36 ++++++++------- .../process_featured_item_service.rb | 45 +++++++++++-------- .../process_featured_item_worker.rb | 4 +- ...rocess_featured_collection_service_spec.rb | 32 +++++++++++++ .../process_featured_item_service_spec.rb | 34 ++++++++++---- .../process_featured_item_worker_spec.rb | 2 +- 6 files changed, 108 insertions(+), 45 deletions(-) diff --git a/app/services/activitypub/process_featured_collection_service.rb b/app/services/activitypub/process_featured_collection_service.rb index 91c15bdce2..2ef555e6bc 100644 --- a/app/services/activitypub/process_featured_collection_service.rb +++ b/app/services/activitypub/process_featured_collection_service.rb @@ -14,21 +14,22 @@ class ActivityPub::ProcessFeaturedCollectionService return if non_matching_uri_hosts?(@account.uri, @json['id']) with_redis_lock("collection:#{@json['id']}") do - return if @account.collections.exists?(uri: @json['id']) + Collection.transaction do + @collection = @account.collections.find_or_initialize_by(uri: @json['id']) - @collection = @account.collections.create!( - local: false, - uri: @json['id'], - name: (@json['name'] || '')[0, Collection::NAME_LENGTH_HARD_LIMIT], - description_html: truncated_summary, - language:, - sensitive: @json['sensitive'], - discoverable: @json['discoverable'], - original_number_of_items: @json['totalItems'] || 0, - tag_name: @json.dig('topic', 'name') - ) + @collection.update!( + local: false, + name: (@json['name'] || '')[0, Collection::NAME_LENGTH_HARD_LIMIT], + description_html: truncated_summary, + language:, + sensitive: @json['sensitive'], + discoverable: @json['discoverable'], + original_number_of_items: @json['totalItems'] || 0, + tag_name: @json.dig('topic', 'name') + ) - process_items! if @json['totalItems'].positive? + process_items! + end @collection end @@ -46,8 +47,13 @@ class ActivityPub::ProcessFeaturedCollectionService end def process_items! - @json['orderedItems'].take(ITEMS_LIMIT).each do |item_json| - ActivityPub::ProcessFeaturedItemWorker.perform_async(@collection.id, item_json, @request_id) + uris = [] + items = @json['orderedItems'] || [] + items.take(ITEMS_LIMIT).each_with_index do |item_json, index| + uris << value_or_id(item_json) + ActivityPub::ProcessFeaturedItemWorker.perform_async(@collection.id, item_json, index, @request_id) end + uris.compact! + @collection.collection_items.where.not(uri: uris).delete_all end end diff --git a/app/services/activitypub/process_featured_item_service.rb b/app/services/activitypub/process_featured_item_service.rb index c0d2bfd206..961de802c9 100644 --- a/app/services/activitypub/process_featured_item_service.rb +++ b/app/services/activitypub/process_featured_item_service.rb @@ -5,29 +5,24 @@ class ActivityPub::ProcessFeaturedItemService include Lockable include Redisable - def call(collection, uri_or_object, request_id: nil) + def call(collection, uri_or_object, position: nil, request_id: nil) + @collection = collection @request_id = request_id - item_json = uri_or_object.is_a?(String) ? fetch_resource(uri_or_object, true) : uri_or_object - return if non_matching_uri_hosts?(collection.uri, item_json['id']) + @item_json = uri_or_object.is_a?(String) ? fetch_resource(uri_or_object, true) : uri_or_object + return if non_matching_uri_hosts?(@collection.uri, @item_json['id']) - with_redis_lock("collection_item:#{item_json['id']}") do - return if collection.collection_items.exists?(uri: item_json['id']) + with_redis_lock("collection_item:#{@item_json['id']}") do + @collection_item = existing_item || pre_approved_item || new_item - local_account = ActivityPub::TagManager.instance.uris_to_local_accounts([item_json['featuredObject']]).first + @collection_item.update!( + uri: @item_json['id'], + object_uri: value_or_id(@item_json['featuredObject']), + position: + ) - if local_account.present? - # This is a local account that has authorized this item already - @collection_item = collection.collection_items.accepted_partial(local_account).first - @collection_item&.update!(uri: item_json['id']) - else - @collection_item = collection.collection_items.create!( - uri: item_json['id'], - object_uri: item_json['featuredObject'] - ) - @approval_uri = item_json['featureAuthorization'] + @approval_uri = @item_json['featureAuthorization'] - verify_authorization! - end + verify_authorization! unless @collection_item&.account&.local? @collection_item end @@ -35,6 +30,20 @@ class ActivityPub::ProcessFeaturedItemService private + def existing_item + @collection.collection_items.find_by(uri: @item_json['id']) + end + + def pre_approved_item + # This is a local account that has authorized this item already + local_account = ActivityPub::TagManager.instance.uris_to_local_accounts([@item_json['featuredObject']]).first + @collection.collection_items.accepted_partial(local_account).first if local_account.present? + end + + def new_item + @collection.collection_items.new + end + def verify_authorization! ActivityPub::VerifyFeaturedItemService.new.call(@collection_item, @approval_uri, request_id: @request_id) rescue Mastodon::RecursionLimitExceededError, Mastodon::UnexpectedResponseError, *Mastodon::HTTP_CONNECTION_ERRORS diff --git a/app/workers/activitypub/process_featured_item_worker.rb b/app/workers/activitypub/process_featured_item_worker.rb index f50ff50b40..b29c7d7aee 100644 --- a/app/workers/activitypub/process_featured_item_worker.rb +++ b/app/workers/activitypub/process_featured_item_worker.rb @@ -6,10 +6,10 @@ class ActivityPub::ProcessFeaturedItemWorker sidekiq_options queue: 'pull', retry: 3 - def perform(collection_id, id_or_json, request_id = nil) + def perform(collection_id, id_or_json, position = nil, request_id = nil) collection = Collection.find(collection_id) - ActivityPub::ProcessFeaturedItemService.new.call(collection, id_or_json, request_id:) + ActivityPub::ProcessFeaturedItemService.new.call(collection, id_or_json, position:, request_id:) rescue ActiveRecord::RecordNotFound true end diff --git a/spec/services/activitypub/process_featured_collection_service_spec.rb b/spec/services/activitypub/process_featured_collection_service_spec.rb index 3a0fdd82f1..c68120bf0d 100644 --- a/spec/services/activitypub/process_featured_collection_service_spec.rb +++ b/spec/services/activitypub/process_featured_collection_service_spec.rb @@ -73,4 +73,36 @@ RSpec.describe ActivityPub::ProcessFeaturedCollectionService do expect(new_collection.description_html).to eq '

A list of remote actors you should follow.

' end end + + context 'when the collection already exists' do + let(:collection) { Fabricate(:remote_collection, account:, uri: base_json['id'], name: 'placeholder') } + + before do + Fabricate(:collection_item, collection:, uri: 'https://example.com/featured_items/1') + Fabricate(:collection_item, collection:, uri: 'https://example.com/featured_items/3') + end + + it 'updates the existing collection, removes the item that no longer exists and queues a jobs to fetch the other items' do + expect { subject.call(account, featured_collection_json) } + .to change(collection.collection_items, :count).by(-1) + + expect(collection.reload.name).to eq 'Good people from other servers' + expect(ActivityPub::ProcessFeaturedItemWorker).to have_enqueued_sidekiq_job.exactly(2).times + end + + context 'when the updated collection no longer contains any items' do + let(:featured_collection_json) do + base_json.merge({ + 'summary' => summary, + 'totalItems' => 0, + 'orderedItems' => nil, + }) + end + + it 'removes all items' do + expect { subject.call(account, featured_collection_json) } + .to change(collection.collection_items, :count).by(-2) + end + end + end end diff --git a/spec/services/activitypub/process_featured_item_service_spec.rb b/spec/services/activitypub/process_featured_item_service_spec.rb index 55bf89df91..6a91f6dba8 100644 --- a/spec/services/activitypub/process_featured_item_service_spec.rb +++ b/spec/services/activitypub/process_featured_item_service_spec.rb @@ -8,6 +8,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do subject { described_class.new } let(:collection) { Fabricate(:remote_collection, uri: 'https://other.example.com/collection/1') } + let(:position) { 3 } let(:featured_object_uri) { 'https://example.com/actor/1' } let(:feature_authorization_uri) { 'https://example.com/auth/1' } let(:featured_item_json) do @@ -34,7 +35,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do it 'does not create a collection item and returns `nil`' do expect do - expect(subject.call(collection, object)).to be_nil + expect(subject.call(collection, object, position:)).to be_nil end.to_not change(CollectionItem, :count) end end @@ -45,14 +46,29 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do it_behaves_like 'non-matching URIs' - it 'creates and verifies the item' do - expect { subject.call(collection, object) }.to change(collection.collection_items, :count).by(1) + context 'when item does not yet exist' do + it 'creates and verifies the item' do + expect { subject.call(collection, object, position:) }.to change(collection.collection_items, :count).by(1) - expect(stubbed_service).to have_received(:call) + expect(stubbed_service).to have_received(:call) - new_item = collection.collection_items.last - expect(new_item.object_uri).to eq 'https://example.com/actor/1' - expect(new_item.approval_uri).to be_nil + new_item = collection.collection_items.last + expect(new_item.object_uri).to eq 'https://example.com/actor/1' + expect(new_item.approval_uri).to be_nil + expect(new_item.position).to eq 3 + end + end + + context 'when item exists at a different position' do + let!(:collection_item) do + Fabricate(:collection_item, collection:, uri: featured_item_json['id'], position: 2) + end + + it 'updates the position' do + expect { subject.call(collection, object, position:) }.to_not change(collection.collection_items, :count) + + expect(collection_item.reload.position).to eq 3 + end end context 'when an item exists for a local featured account' do @@ -63,7 +79,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do let(:feature_authorization_uri) { ap_account_feature_authorization_url(collection_item.account_id, collection_item) } it 'updates the URI of the existing record' do - expect { subject.call(collection, object) }.to_not change(collection.collection_items, :count) + expect { subject.call(collection, object, position:) }.to_not change(collection.collection_items, :count) expect(collection_item.reload.uri).to eq 'https://other.example.com/featured_item/1' end end @@ -87,7 +103,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do it_behaves_like 'non-matching URIs' it 'fetches the collection item' do - expect { subject.call(collection, object) }.to change(collection.collection_items, :count).by(1) + expect { subject.call(collection, object, position:) }.to change(collection.collection_items, :count).by(1) expect(featured_item_request).to have_been_requested diff --git a/spec/workers/activitypub/process_featured_item_worker_spec.rb b/spec/workers/activitypub/process_featured_item_worker_spec.rb index 02a9edfc47..531b85407f 100644 --- a/spec/workers/activitypub/process_featured_item_worker_spec.rb +++ b/spec/workers/activitypub/process_featured_item_worker_spec.rb @@ -19,7 +19,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemWorker do it 'calls the service to process the item' do subject.perform(collection.id, object) - expect(stubbed_service).to have_received(:call).with(collection, object, request_id: nil) + expect(stubbed_service).to have_received(:call).with(collection, object, position: nil, request_id: nil) end end end