From f8f0572ee09122dd8fda51aec899f939c6ab03bf Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Sat, 24 Feb 2018 13:40:18 +0900 Subject: [PATCH] Do not push status to feed if its reblog is already inserted (#6488) A complemental change for precompute_feed_service_spec.rb also fixes its random failure which is caused by the Snowlake randomization of the order of an original status and its reblog. --- app/lib/feed_manager.rb | 8 +++++ spec/fabricators/list_fabricator.rb | 2 +- spec/lib/feed_manager_spec.rb | 35 ++++++++++++++++++- spec/services/precompute_feed_service_spec.rb | 7 ++-- 4 files changed, 45 insertions(+), 7 deletions(-) diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb index 20b10e11376..dd78e543f75 100644 --- a/app/lib/feed_manager.rb +++ b/app/lib/feed_manager.rb @@ -220,6 +220,14 @@ class FeedManager return false end else + # A reblog may reach earlier than the original status because of the + # delay of the worker deliverying the original status, the late addition + # by merging timelines, and other reasons. + # If such a reblog already exists, just do not re-insert it into the feed. + rank = redis.zrevrank(reblog_key, status.id) + + return false unless rank.nil? + redis.zadd(timeline_key, status.id, status.id) end diff --git a/spec/fabricators/list_fabricator.rb b/spec/fabricators/list_fabricator.rb index d249c20298f..2a61b317b8c 100644 --- a/spec/fabricators/list_fabricator.rb +++ b/spec/fabricators/list_fabricator.rb @@ -1,4 +1,4 @@ Fabricator(:list) do - account nil + account title "MyString" end diff --git a/spec/lib/feed_manager_spec.rb b/spec/lib/feed_manager_spec.rb index c6bc388e057..6ead5bbd93e 100644 --- a/spec/lib/feed_manager_spec.rb +++ b/spec/lib/feed_manager_spec.rb @@ -157,7 +157,7 @@ RSpec.describe FeedManager do end end - describe '#push' do + describe '#push_to_home' do it 'trims timelines if they will have more than FeedManager::MAX_ITEMS' do account = Fabricate(:account) status = Fabricate(:status) @@ -248,6 +248,39 @@ RSpec.describe FeedManager do expect(FeedManager.instance.push_to_home(account, reblogs.last)).to be true end end + + it "does not push when the given status's reblog is already inserted" do + account = Fabricate(:account) + reblog = Fabricate(:status) + status = Fabricate(:status, reblog: reblog) + FeedManager.instance.push_to_home(account, status) + + expect(FeedManager.instance.push_to_home(account, reblog)).to eq false + end + end + + describe '#push_to_list' do + it "does not push when the given status's reblog is already inserted" do + list = Fabricate(:list) + reblog = Fabricate(:status) + status = Fabricate(:status, reblog: reblog) + FeedManager.instance.push_to_list(list, status) + + expect(FeedManager.instance.push_to_list(list, reblog)).to eq false + end + end + + describe '#merge_into_timeline' do + it "does not push source account's statuses whose reblogs are already inserted" do + account = Fabricate(:account, id: 0) + reblog = Fabricate(:status) + status = Fabricate(:status, reblog: reblog) + FeedManager.instance.push_to_home(account, status) + + FeedManager.instance.merge_into_timeline(account, reblog.account) + + expect(Redis.current.zscore("feed:home:0", reblog.id)).to eq nil + end end describe '#trim' do diff --git a/spec/services/precompute_feed_service_spec.rb b/spec/services/precompute_feed_service_spec.rb index 396a3c3fba4..43340bffc01 100644 --- a/spec/services/precompute_feed_service_spec.rb +++ b/spec/services/precompute_feed_service_spec.rb @@ -9,14 +9,11 @@ RSpec.describe PrecomputeFeedService do let(:account) { Fabricate(:account) } it 'fills a user timeline with statuses' do account = Fabricate(:account) - followed_account = Fabricate(:account) - Fabricate(:follow, account: account, target_account: followed_account) - reblog = Fabricate(:status, account: followed_account) - status = Fabricate(:status, account: account, reblog: reblog) + status = Fabricate(:status, account: account) subject.call(account) - expect(Redis.current.zscore(FeedManager.instance.key(:home, account.id), reblog.id)).to be_within(0.1).of(status.id.to_f) + expect(Redis.current.zscore(FeedManager.instance.key(:home, account.id), status.id)).to be_within(0.1).of(status.id.to_f) end it 'does not raise an error even if it could not find any status' do