Improve enforcement of collection item limit (#39969)

app/models/collection.rb

@@ -54,7 +54,7 @@ class Collection < ApplicationRecord
             if: :remote?
   validates :language, language: { if: :local?, allow_nil: true }
   validate :tag_is_usable
-  validate :items_do_not_exceed_limit
+  validate :items_do_not_exceed_limit, if: :local?
   validate :user_does_not_exceed_limit, on: :create
 
   scope :with_items, -> { includes(:collection_items).merge(CollectionItem.with_accounts) }

app/services/activitypub/process_featured_item_service.rb

@@ -17,9 +17,10 @@ class ActivityPub::ProcessFeaturedItemService
     return if non_matching_actor_and_approval_uris?
     return if non_supported_object_type?
 
-    with_redis_lock("collection_item:#{@item_json['id']}") do
+    @collection_item = existing_item || pre_approved_item || new_item
-      @collection_item = existing_item || pre_approved_item || new_item
+    return if @collection_item.nil?
 
+    with_redis_lock("collection_item:#{@item_json['id']}") do
       @collection_item.position = position unless position.nil?
       @collection_item.update!(
         uri: @item_json['id'],
@@ -45,6 +46,8 @@ class ActivityPub::ProcessFeaturedItemService
   end
 
   def new_item
+    return if @collection.collection_items.count >= ActivityPub::ProcessFeaturedCollectionService::ITEMS_LIMIT
+
     @collection.collection_items.new(
       created_at: @item_json['published']
     )

spec/models/collection_spec.rb

@@ -53,20 +53,28 @@ RSpec.describe Collection do
     end
 
     context 'when there are more items than allowed' do
-      subject { Fabricate.build(:collection, collection_items:) }
-
       let(:collection_items) { Fabricate.build_times(described_class::MAX_ITEMS + 1, :collection_item, collection: nil) }
 
-      it { is_expected.to_not be_valid }
+      context 'when collection is local' do
+        subject { Fabricate.build(:collection, collection_items:) }
 
-      context 'when the limit is only exceeded due to `rejected` and `revoked` items' do
+        it { is_expected.to_not be_valid }
-        let(:collection_items) do
+
-          items = Fabricate.build_times(described_class::MAX_ITEMS - 2, :collection_item, collection: nil, state: :accepted)
+        context 'when the limit is only exceeded due to `rejected` and `revoked` items' do
-          items << Fabricate.build(:collection_item, collection: nil, state: :pending)
+          let(:collection_items) do
-          items << Fabricate.build(:collection_item, collection: nil, state: :rejected)
+            items = Fabricate.build_times(described_class::MAX_ITEMS - 2, :collection_item, collection: nil, state: :accepted)
-          items << Fabricate.build(:collection_item, collection: nil, state: :revoked)
+            items << Fabricate.build(:collection_item, collection: nil, state: :pending)
-          items
+            items << Fabricate.build(:collection_item, collection: nil, state: :rejected)
+            items << Fabricate.build(:collection_item, collection: nil, state: :revoked)
+            items
+          end
+
+          it { is_expected.to be_valid }
         end
+      end
+
+      context 'when collection is remote' do
+        subject { Fabricate.build(:remote_collection, collection_items:) }
 
         it { is_expected.to be_valid }
       end

spec/services/activitypub/process_featured_item_service_spec.rb

@@ -81,6 +81,17 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do
           expect(new_item.position).to eq 1
         end
       end
+
+      context "when the collection's item count is already at the limit" do
+        before do
+          stub_const('ActivityPub::ProcessFeaturedCollectionService::ITEMS_LIMIT', 3)
+          Fabricate.times(3, :collection_item, collection:)
+        end
+
+        it 'does not create an item' do
+          expect { subject.call(collection, object) }.to_not change(collection.collection_items, :count)
+        end
+      end
     end
 
     context 'when item exists' do