diff --git a/app/models/public_feed.rb b/app/models/public_feed.rb index 060a9ac24d480d..b40e47b609f8b7 100644 --- a/app/models/public_feed.rb +++ b/app/models/public_feed.rb @@ -25,13 +25,14 @@ def get(limit, max_id = nil, since_id = nil, min_id = nil) scope.merge!(without_local_only_scope) unless allow_local_only? scope.merge!(without_replies_scope) unless with_replies? scope.merge!(without_reblogs_scope) unless with_reblogs? - scope.merge!(without_duplicate_reblogs) if with_reblogs? scope.merge!(local_only_scope) if local_only? scope.merge!(remote_only_scope) if remote_only? scope.merge!(account_filters_scope) if account? scope.merge!(media_only_scope) if media_only? scope.merge!(language_scope) if account&.chosen_languages.present? + scope.merge!(without_duplicate_reblogs) if with_reblogs? + scope.cache_ids.to_a_paginated_by_id(limit, max_id: max_id, since_id: since_id, min_id: min_id) end @@ -91,15 +92,21 @@ def without_reblogs_scope Status.without_reblogs end + def max_boost_id_scope + Status.where(<<~SQL.squish) + "statuses"."id" = ( + SELECT MAX(id) + FROM "statuses" "s2" + WHERE "s2"."reblog_of_id" = "statuses"."reblog_of_id" + #{'AND ("s2"."local" = true OR "s2"."uri" IS NULL)' if local_only?} + #{'AND "s2"."local" = false AND "s2"."uri" IS NOT NULL' if remote_only?} + ) + SQL + end + def without_duplicate_reblogs Status.where(statuses: { reblog_of_id: nil }) - .or(Status.where(<<~SQL.squish)) - "statuses"."id" = ( - SELECT MAX(id) - FROM statuses s2 - WHERE s2.reblog_of_id = statuses.reblog_of_id - ) - SQL + .or(max_boost_id_scope) end def media_only_scope diff --git a/db/migrate/20240129222920_add_index_reblog_of_id_and_status_to_statuses.rb b/db/migrate/20240129222920_add_index_reblog_of_id_and_status_to_statuses.rb new file mode 100644 index 00000000000000..e96454f4ee3a12 --- /dev/null +++ b/db/migrate/20240129222920_add_index_reblog_of_id_and_status_to_statuses.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddIndexReblogOfIdAndStatusToStatuses < ActiveRecord::Migration[7.1] + disable_ddl_transaction! + + def change + add_index :statuses, [:reblog_of_id, :id], order: { reblog_of_id: 'DESC NULLS LAST', id: 'DESC' }, algorithm: :concurrently + end +end diff --git a/db/schema.rb b/db/schema.rb index 9c7c490bb1e786..c8019927814fac 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_09_07_150100) do +ActiveRecord::Schema[7.1].define(version: 2024_01_29_222920) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -993,6 +993,7 @@ t.index ["in_reply_to_account_id"], name: "index_statuses_on_in_reply_to_account_id", where: "(in_reply_to_account_id IS NOT NULL)" t.index ["in_reply_to_id"], name: "index_statuses_on_in_reply_to_id", where: "(in_reply_to_id IS NOT NULL)" t.index ["reblog_of_id", "account_id"], name: "index_statuses_on_reblog_of_id_and_account_id" + t.index ["reblog_of_id", "id"], name: "index_statuses_on_reblog_of_id_and_id", order: { reblog_of_id: "DESC NULLS LAST", id: :desc } t.index ["uri"], name: "index_statuses_on_uri", unique: true, opclass: :text_pattern_ops, where: "(uri IS NOT NULL)" end diff --git a/spec/models/public_feed_spec.rb b/spec/models/public_feed_spec.rb index ff9078b4891f84..d5ce9f1a3f3fcf 100644 --- a/spec/models/public_feed_spec.rb +++ b/spec/models/public_feed_spec.rb @@ -35,6 +35,11 @@ context 'with with_reblogs option' do subject { described_class.new(nil, with_reblogs: true).get(20).map(&:id) } + let!(:poster) { Fabricate(:account, domain: nil) } + let!(:booster) { Fabricate(:account, domain: nil) } + let!(:second_booster) { Fabricate(:account, domain: nil) } + let!(:remote_booster) { Fabricate(:account, domain: 'example.com') } + it 'does include boosts' do status = Fabricate(:status) boost = Fabricate(:status, reblog_of_id: status.id) @@ -44,10 +49,6 @@ end it 'only includes the most recent boost' do - poster = Fabricate(:account, domain: nil) - booster = Fabricate(:account, domain: nil) - second_booster = Fabricate(:account, domain: nil) - status = Fabricate(:status, account: poster) boost = Fabricate(:status, reblog_of_id: status.id, account: poster) second_boost = Fabricate(:status, reblog_of_id: status.id, account: booster) @@ -60,20 +61,16 @@ end it 'filters duplicate boosts across pagination' do - poster = Fabricate(:account, domain: nil) - booster = Fabricate(:account, domain: nil) - status = Fabricate(:status, account: poster) - # sleep for 2ms to make sure the other posts come in a greater snowflake ID - sleep(0.002) - boost = Fabricate(:status, reblog_of_id: status.id, account: poster) + boost = Fabricate(:status, reblog_of_id: status.id, id: status.id + 1, account: poster) # sleep for 2ms to make sure the other posts come in a greater snowflake ID sleep(0.002) - (1..20).each do |_i| - Fabricate(:status, account: poster) + n_posts = 20 + (1..n_posts).each do |i| + Fabricate(:status, account: poster, id: boost.id + i) end # before a second boost, the second page should still include the original boost @@ -81,12 +78,38 @@ expect(second_page).to include(boost.id) # after a second boost, the second page should no longer include the original boost - second_boost = Fabricate(:status, reblog_of_id: status.id, account: booster) + second_boost = Fabricate(:status, reblog_of_id: status.id, id: boost.id + n_posts + 1, account: booster) second_page = described_class.new(nil, with_reblogs: true).get(20, boost.id + 1).map(&:id) expect(subject).to include(second_boost.id) expect(second_page).to_not include(boost.id) end + + context 'with local option' do + subject { described_class.new(nil, with_reblogs: true, local: true, remote: false).get(20).map(&:id) } + + it 'shows the most recent local boost when there is a more recent remote boost' do + status = Fabricate(:status, account: poster) + local_boost = Fabricate(:status, reblog_of_id: status.id, local: true, account: booster) + remote_boost = Fabricate(:status, reblog_of_id: status.id, id: local_boost.id + 1, local: false, uri: 'https://example.com/boosturl', account: remote_booster) + + expect(subject).to include(local_boost.id) + expect(subject).to_not include(remote_boost.id) + end + end + + context 'with remote option' do + subject { described_class.new(nil, with_reblogs: true, local: false, remote: true).get(20).map(&:id) } + + it 'shows the most recent remote boost when there is a more recent local boost' do + status = Fabricate(:status, account: poster) + remote_boost = Fabricate(:status, reblog_of_id: status.id, local: false, uri: 'https://example.com/boosturl', account: remote_booster) + local_boost = Fabricate(:status, reblog_of_id: status.id, id: remote_boost.id + 1, local: true, account: booster) + + expect(subject).to include(remote_boost.id) + expect(subject).to_not include(local_boost.id) + end + end end it 'filters out silenced accounts' do