Fix replies collection incorrectly looping (#17462)

* Refactor tests

* Add tests

* Fix replies collection incorrectly looping

app/controllers/activitypub/replies_controller.rb

@@ -63,15 +63,29 @@ class ActivityPub::RepliesController < ActivityPub::BaseController
   end
 
   def next_page
-    only_other_accounts = !(@replies&.last&.account_id == @account.id && @replies.size == DESCENDANTS_LIMIT)
+    if only_other_accounts?
+      # Only consider remote accounts
+      return nil if @replies.size < DESCENDANTS_LIMIT
 
       account_status_replies_url(
         @account,
         @status,
         page: true,
-      min_id: only_other_accounts && !only_other_accounts? ? nil : @replies&.last&.id,
+        min_id: @replies&.last&.id,
-      only_other_accounts: only_other_accounts
+        only_other_accounts: true
       )
+    else
+      # For now, we're serving only self-replies, but next page might be other accounts
+      next_only_other_accounts = @replies&.last&.account_id != @account.id || @replies.size < DESCENDANTS_LIMIT
+
+      account_status_replies_url(
+        @account,
+        @status,
+        page: true,
+        min_id: next_only_other_accounts ? nil : @replies&.last&.id,
+        only_other_accounts: next_only_other_accounts
+      )
+    end
   end
 
   def page_params

spec/controllers/activitypub/replies_controller_spec.rb

@@ -4,8 +4,9 @@ require 'rails_helper'
 
 RSpec.describe ActivityPub::RepliesController, type: :controller do
   let(:status) { Fabricate(:status, visibility: parent_visibility) }
-  let(:remote_reply_id) { nil }
+  let(:remote_account)  { Fabricate(:account, domain: 'foobar.com') }
-  let(:remote_account) { nil }
+  let(:remote_reply_id) { 'https://foobar.com/statuses/1234' }
+  let(:remote_querier) { nil }
 
   shared_examples 'cachable response' do
     it 'does not set cookies' do
@@ -23,23 +24,37 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do
     end
   end
 
-  before do
+  shared_examples 'common behavior' do
-    allow(controller).to receive(:signed_request_account).and_return(remote_account)
+    context 'when status is private' do
+      let(:parent_visibility) { :private }
 
-    Fabricate(:status, thread: status, visibility: :public)
+      it 'returns http not found' do
-    Fabricate(:status, thread: status, visibility: :public)
+        expect(response).to have_http_status(404)
-    Fabricate(:status, thread: status, visibility: :private)
+      end
-    Fabricate(:status, account: status.account, thread: status, visibility: :public)
+    end
-    Fabricate(:status, account: status.account, thread: status, visibility: :private)
+
+    context 'when status is direct' do
+      let(:parent_visibility) { :direct }
 
-    Fabricate(:status, account: remote_account, thread: status, visibility: :public, uri: remote_reply_id) if remote_reply_id
+      it 'returns http not found' do
+        expect(response).to have_http_status(404)
+      end
+    end
   end
 
-  describe 'GET #index' do
+  shared_examples 'disallowed access' do
-    context 'with no signature' do
+    context 'when status is public' do
-      subject(:response) { get :index, params: { account_username: status.account.username, status_id: status.id } }
+      let(:parent_visibility) { :public }
-      subject(:body) { body_as_json }
+
+      it 'returns http not found' do
+        expect(response).to have_http_status(404)
+      end
+    end
+
+    it_behaves_like 'common behavior'
+  end
 
+  shared_examples 'allowed access' do
     context 'when account is permanently suspended' do
       let(:parent_visibility) { :public }
 
@@ -67,6 +82,8 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do
 
     context 'when status is public' do
       let(:parent_visibility) { :public }
+      let(:json) { body_as_json }
+      let(:page_json) { json[:first] }
 
       it 'returns http success' do
         expect(response).to have_http_status(200)
@@ -78,61 +95,30 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do
 
       it_behaves_like 'cachable response'
 
-        it 'returns items with account\'s own replies' do
+      context 'without only_other_accounts' do
-          expect(body[:first]).to be_a Hash
+        it "returns items with thread author's replies" do
-          expect(body[:first][:items]).to be_an Array
+          expect(page_json).to be_a Hash
-          expect(body[:first][:items].size).to eq 1
+          expect(page_json[:items]).to be_an Array
-          expect(body[:first][:items].all? { |item| item[:to].include?(ActivityPub::TagManager::COLLECTIONS[:public]) || item[:cc].include?(ActivityPub::TagManager::COLLECTIONS[:public]) }).to be true
+          expect(page_json[:items].size).to eq 1
-        end
+          expect(page_json[:items].all? { |item| item[:to].include?(ActivityPub::TagManager::COLLECTIONS[:public]) || item[:cc].include?(ActivityPub::TagManager::COLLECTIONS[:public]) }).to be true
-      end
-
-      context 'when status is private' do
-        let(:parent_visibility) { :private }
-
-        it 'returns http not found' do
-          expect(response).to have_http_status(404)
-        end
         end
 
-      context 'when status is direct' do
+        context 'when there are few self-replies' do
-        let(:parent_visibility) { :direct }
+          it 'points next to replies from other people' do
-
+            expect(page_json).to be_a Hash
-        it 'returns http not found' do
+            expect(Addressable::URI.parse(page_json[:next]).query.split('&')).to include('only_other_accounts=true', 'page=true')
-          expect(response).to have_http_status(404)
-        end
           end
         end
 
-    context 'with signature' do
+        context 'when there are many self-replies' do
-      let(:remote_account) { Fabricate(:account, domain: 'example.com') }
-      let(:only_other_accounts) { nil }
-
-      context do
           before do
-          get :index, params: { account_username: status.account.username, status_id: status.id, only_other_accounts: only_other_accounts }
+            10.times { Fabricate(:status, account: status.account, thread: status, visibility: :public) }
           end
 
-        context 'when status is public' do
+          it 'points next to other self-replies' do
-          let(:parent_visibility) { :public }
+            expect(page_json).to be_a Hash
-
+            expect(Addressable::URI.parse(page_json[:next]).query.split('&')).to include('only_other_accounts=false', 'page=true')
-          it 'returns http success' do
-            expect(response).to have_http_status(200)
-          end
-
-          it 'returns application/activity+json' do
-            expect(response.media_type).to eq 'application/activity+json'
           end
-
-          it_behaves_like 'cachable response'
-
-          context 'without only_other_accounts' do
-            it 'returns items with account\'s own replies' do
-              json = body_as_json
-
-              expect(json[:first]).to be_a Hash
-              expect(json[:first][:items]).to be_an Array
-              expect(json[:first][:items].size).to eq 1
-              expect(json[:first][:items].all? { |item| item[:to].include?(ActivityPub::TagManager::COLLECTIONS[:public]) || item[:cc].include?(ActivityPub::TagManager::COLLECTIONS[:public]) }).to be true
         end
       end
 
@@ -140,107 +126,86 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do
         let(:only_other_accounts) { 'true' }
 
         it 'returns items with other public or unlisted replies' do
-              json = body_as_json
+          expect(page_json).to be_a Hash
-
+          expect(page_json[:items]).to be_an Array
-              expect(json[:first]).to be_a Hash
+          expect(page_json[:items].size).to eq 3
-              expect(json[:first][:items]).to be_an Array
-              expect(json[:first][:items].size).to eq 2
-              expect(json[:first][:items].all? { |item| item[:to].include?(ActivityPub::TagManager::COLLECTIONS[:public]) || item[:cc].include?(ActivityPub::TagManager::COLLECTIONS[:public]) }).to be true
         end
 
-            context 'with remote responses' do
+        it 'only inlines items that are local and public or unlisted replies' do
-              let(:remote_reply_id) { 'foo' }
+          inlined_replies = page_json[:items].select { |x| x.is_a?(Hash) }
-
+          public_collection = ActivityPub::TagManager::COLLECTIONS[:public]
-              it 'returned items are all inlined local toots or are ids' do
+          expect(inlined_replies.all? { |item| item[:to].include?(public_collection) || item[:cc].include?(public_collection) }).to be true
-                json = body_as_json
+          expect(inlined_replies.all? { |item| ActivityPub::TagManager.instance.local_uri?(item[:id]) }).to be true
-
-                expect(json[:first]).to be_a Hash
-                expect(json[:first][:items]).to be_an Array
-                expect(json[:first][:items].size).to eq 3
-                expect(json[:first][:items].all? { |item| item.is_a?(Hash) ? ActivityPub::TagManager.instance.local_uri?(item[:id]) : item.is_a?(String) }).to be true
-                expect(json[:first][:items]).to include remote_reply_id
         end
+
+        it 'uses ids for remote toots' do
+          remote_replies = page_json[:items].select { |x| !x.is_a?(Hash) }
+          expect(remote_replies.all? { |item| item.is_a?(String) && !ActivityPub::TagManager.instance.local_uri?(item) }).to be true
         end
+
+        context 'when there are few replies' do
+          it 'does not have a next page' do
+            expect(page_json).to be_a Hash
+            expect(page_json[:next]).to be_nil
           end
         end
 
-        context 'when status is private' do
+        context 'when there are many replies' do
-          let(:parent_visibility) { :private }
+          before do
+            10.times { Fabricate(:status, thread: status, visibility: :public) }
+          end
 
-          it 'returns http not found' do
+          it 'points next to other replies' do
-            expect(response).to have_http_status(404)
+            expect(page_json).to be_a Hash
+            expect(Addressable::URI.parse(page_json[:next]).query.split('&')).to include('only_other_accounts=true', 'page=true')
           end
         end
-
-        context 'when status is direct' do
-          let(:parent_visibility) { :direct }
-
-          it 'returns http not found' do
-            expect(response).to have_http_status(404)
       end
     end
+
+    it_behaves_like 'common behavior'
   end
 
-      context 'when signed request account is blocked' do
   before do
-          status.account.block!(remote_account)
+    stub_const 'ActivityPub::RepliesController::DESCENDANTS_LIMIT', 5
-          get :index, params: { account_username: status.account.username, status_id: status.id }
+    allow(controller).to receive(:signed_request_account).and_return(remote_querier)
-        end
 
-        context 'when status is public' do
+    Fabricate(:status, thread: status, visibility: :public)
-          let(:parent_visibility) { :public }
+    Fabricate(:status, thread: status, visibility: :public)
+    Fabricate(:status, thread: status, visibility: :private)
+    Fabricate(:status, account: status.account, thread: status, visibility: :public)
+    Fabricate(:status, account: status.account, thread: status, visibility: :private)
 
-          it 'returns http not found' do
+    Fabricate(:status, account: remote_account, thread: status, visibility: :public, uri: remote_reply_id)
-            expect(response).to have_http_status(404)
-          end
   end
 
-        context 'when status is private' do
+  describe 'GET #index' do
-          let(:parent_visibility) { :private }
+    subject(:response) { get :index, params: { account_username: status.account.username, status_id: status.id, only_other_accounts: only_other_accounts } }
+    let(:only_other_accounts) { nil }
 
-          it 'returns http not found' do
+    context 'with no signature' do
-            expect(response).to have_http_status(404)
+      it_behaves_like 'allowed access'
-          end
     end
 
-        context 'when status is direct' do
+    context 'with signature' do
-          let(:parent_visibility) { :direct }
+      let(:remote_querier) { Fabricate(:account, domain: 'example.com') }
 
-          it 'returns http not found' do
+      it_behaves_like 'allowed access'
-            expect(response).to have_http_status(404)
-          end
-        end
-      end
 
-      context 'when signed request account is domain blocked' do
+      context 'when signed request account is blocked' do
         before do
-          status.account.block_domain!(remote_account.domain)
+          status.account.block!(remote_querier)
-          get :index, params: { account_username: status.account.username, status_id: status.id }
         end
 
-        context 'when status is public' do
+        it_behaves_like 'disallowed access'
-          let(:parent_visibility) { :public }
-
-          it 'returns http not found' do
-            expect(response).to have_http_status(404)
       end
-        end
-
-        context 'when status is private' do
-          let(:parent_visibility) { :private }
 
-          it 'returns http not found' do
+      context 'when signed request account is domain blocked' do
-            expect(response).to have_http_status(404)
+        before do
-          end
+          status.account.block_domain!(remote_querier.domain)
         end
 
-        context 'when status is direct' do
+        it_behaves_like 'disallowed access'
-          let(:parent_visibility) { :direct }
-
-          it 'returns http not found' do
-            expect(response).to have_http_status(404)
-          end
-        end
       end
     end
   end