From 5db3a14388cf780364b213c63aaf97b6f444ca17 Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 14 Apr 2022 17:44:21 +0200 Subject: [PATCH] Fix loading Home TL from database not respecting `min_id` and not including DMs (#1744) * Rework tests * Add tests * Fix HomeFeed#get with min_id fetching from database * Minor code cleanup and optimizations * Add tests * Take DMs into account when fetching home TL from database * Fix not listing own DMs in Home timeline * Add tests * Please CodeClimate --- app/models/home_feed.rb | 43 +++++++++++++------- spec/models/home_feed_spec.rb | 75 ++++++++++++++++++++++++++++++----- 2 files changed, 93 insertions(+), 25 deletions(-) diff --git a/app/models/home_feed.rb b/app/models/home_feed.rb index 08f421c9c..447445092 100644 --- a/app/models/home_feed.rb +++ b/app/models/home_feed.rb @@ -16,34 +16,47 @@ class HomeFeed < Feed since_id = since_id.to_i if since_id.present? min_id = min_id.to_i if min_id.present? - statuses = from_redis(limit, max_id, since_id, min_id) + if min_id.present? + redis_min_id = fetch_min_redis_id + return from_redis(limit, max_id, since_id, min_id) if redis_min_id && min_id >= redis_min_id - return statuses if statuses.size >= limit + statuses = from_database(limit, redis_min_id, since_id, min_id) + return statuses if statuses.size >= limit - redis_min_id = from_redis(1, nil, nil, 0).first&.id if min_id.present? || since_id.present? - redis_sufficient = redis_min_id && ( - (min_id.present? && min_id >= redis_min_id) || - (since_id.present? && since_id >= redis_min_id) - ) + remaining_limit = limit - statuses.size + min_id = statuses.first.id unless statuses.empty? + from_redis(remaining_limit, max_id, since_id, min_id) + statuses + else + statuses = from_redis(limit, max_id, since_id, min_id) + return statuses if statuses.size >= limit + + if since_id.present? + redis_min_id = fetch_min_redis_id + return statuses if redis_min_id.present? && since_id >= redis_min_id + end - unless redis_sufficient remaining_limit = limit - statuses.size max_id = statuses.last.id unless statuses.empty? - statuses += from_database(remaining_limit, max_id, since_id, min_id) + statuses + from_database(remaining_limit, max_id, since_id, min_id) end - - statuses end protected def from_database(limit, max_id, since_id, min_id) - # Note that this query will not contains direct messages - Status - .where(account: [@account] + @account.following) - .where(visibility: [:public, :unlisted, :private]) + scope = Status.where(account: @account.following) + scope = scope.left_outer_joins(:mentions) + scope = scope.where(visibility: %i(public unlisted private)).or(scope.where(mentions: { account_id: @account.id })).group(Status.arel_table[:id]) + scope = scope.or(Status.where(account: @account)) + scope .to_a_paginated_by_id(limit, min_id: min_id, max_id: max_id, since_id: since_id) .reject { |status| FeedManager.instance.filter?(:home, status, @account) } .sort_by { |status| -status.id } end + + private + + def fetch_min_redis_id + redis.zrangebyscore(key, '(0', '(+inf', limit: [0, 1], with_scores: true).first&.first&.to_i + end end diff --git a/spec/models/home_feed_spec.rb b/spec/models/home_feed_spec.rb index 179459e53..565357d7b 100644 --- a/spec/models/home_feed_spec.rb +++ b/spec/models/home_feed_spec.rb @@ -1,33 +1,83 @@ require 'rails_helper' RSpec.describe HomeFeed, type: :model do - let(:account) { Fabricate(:account) } + let(:account) { Fabricate(:account) } + let(:followed) { Fabricate(:account) } + let(:other) { Fabricate(:account) } subject { described_class.new(account) } describe '#get' do before do - Fabricate(:status, account: account, id: 1) - Fabricate(:status, account: account, id: 2) - Fabricate(:status, account: account, id: 3) - Fabricate(:status, account: account, id: 10) + account.follow!(followed) + + Fabricate(:status, account: account, id: 1) + Fabricate(:status, account: account, id: 2) + status = Fabricate(:status, account: followed, id: 3) + Fabricate(:mention, account: account, status: status) + Fabricate(:status, account: account, id: 10) + Fabricate(:status, account: other, id: 11) + Fabricate(:status, account: followed, id: 12, visibility: :private) + Fabricate(:status, account: followed, id: 13, visibility: :direct) + Fabricate(:status, account: account, id: 14, visibility: :direct) + dm = Fabricate(:status, account: followed, id: 15, visibility: :direct) + Fabricate(:mention, account: account, status: dm) end context 'when feed is generated' do before do + FeedManager.instance.populate_home(account) + + # Add direct messages because populate_home does not do that Redis.current.zadd( FeedManager.instance.key(:home, account.id), - [[4, 4], [3, 3], [2, 2], [1, 1]] + [[14, 14], [15, 15]] ) end it 'gets statuses with ids in the range from redis with database' do - results = subject.get(3) + results = subject.get(5) - expect(results.map(&:id)).to eq [3, 2, 1] + expect(results.map(&:id)).to eq [15, 14, 12, 10, 3] expect(results.first.attributes.keys).to eq %w(id updated_at) end + it 'with since_id present' do + results = subject.get(5, nil, 3, nil) + expect(results.map(&:id)).to eq [15, 14, 12, 10] + end + + it 'with min_id present' do + results = subject.get(3, nil, nil, 0) + expect(results.map(&:id)).to eq [3, 2, 1] + end + end + + context 'when feed is only partial' do + before do + FeedManager.instance.populate_home(account) + + # Add direct messages because populate_home does not do that + Redis.current.zadd( + FeedManager.instance.key(:home, account.id), + [[14, 14], [15, 15]] + ) + + Redis.current.zremrangebyrank(FeedManager.instance.key(:home, account.id), 0, -2) + end + + it 'gets statuses with ids in the range from redis with database' do + results = subject.get(5) + + expect(results.map(&:id)).to eq [15, 14, 12, 10, 3] + expect(results.first.attributes.keys).to eq %w(id updated_at) + end + + it 'with since_id present' do + results = subject.get(5, nil, 3, nil) + expect(results.map(&:id)).to eq [15, 14, 12, 10] + end + it 'with min_id present' do results = subject.get(3, nil, nil, 0) expect(results.map(&:id)).to eq [3, 2, 1] @@ -40,9 +90,14 @@ RSpec.describe HomeFeed, type: :model do end it 'returns from database' do - results = subject.get(3) + results = subject.get(5) - expect(results.map(&:id)).to eq [10, 3, 2] + expect(results.map(&:id)).to eq [15, 14, 12, 10, 3] + end + + it 'with since_id present' do + results = subject.get(5, nil, 3, nil) + expect(results.map(&:id)).to eq [15, 14, 12, 10] end it 'with min_id present' do