Fix performance of account timelines (#17709)

* Fix performance of account timelines

* Various fixes and improvements

* Fix duplicate results being returned

Co-authored-by: Claire <claire.github-309c@sitedethib.com>

* Fix grouping for pinned statuses scope

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
This commit is contained in:
Eugen Rochko 2022-03-08 09:14:39 +01:00 committed by GitHub
parent 61ae6b3535
commit 8f6c67bfde
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 366 additions and 115 deletions

View file

@ -62,7 +62,7 @@ class ActivityPub::OutboxesController < ActivityPub::BaseController
return unless page_requested? return unless page_requested?
@statuses = cache_collection_paginated_by_id( @statuses = cache_collection_paginated_by_id(
@account.statuses.permitted_for(@account, signed_request_account), AccountStatusesFilter.new(@account, signed_request_account).results,
Status, Status,
LIMIT, LIMIT,
params_slice(:max_id, :min_id, :since_id) params_slice(:max_id, :min_id, :since_id)

View file

@ -22,53 +22,16 @@ class Api::V1::Accounts::StatusesController < Api::BaseController
end end
def cached_account_statuses def cached_account_statuses
statuses = truthy_param?(:pinned) ? pinned_scope : permitted_account_statuses
statuses.merge!(only_media_scope) if truthy_param?(:only_media)
statuses.merge!(no_replies_scope) if truthy_param?(:exclude_replies)
statuses.merge!(no_reblogs_scope) if truthy_param?(:exclude_reblogs)
statuses.merge!(hashtag_scope) if params[:tagged].present?
cache_collection_paginated_by_id( cache_collection_paginated_by_id(
statuses, AccountStatusesFilter.new(@account, current_account, params).results,
Status, Status,
limit_param(DEFAULT_STATUSES_LIMIT), limit_param(DEFAULT_STATUSES_LIMIT),
params_slice(:max_id, :since_id, :min_id) params_slice(:max_id, :since_id, :min_id)
) )
end end
def permitted_account_statuses
@account.statuses.permitted_for(@account, current_account)
end
def only_media_scope
Status.joins(:media_attachments).merge(@account.media_attachments.reorder(nil)).group(:id)
end
def pinned_scope
@account.pinned_statuses.permitted_for(@account, current_account)
end
def no_replies_scope
Status.without_replies
end
def no_reblogs_scope
Status.without_reblogs
end
def hashtag_scope
tag = Tag.find_normalized(params[:tagged])
if tag
Status.tagged_with(tag.id)
else
Status.none
end
end
def pagination_params(core_params) def pagination_params(core_params)
params.slice(:limit, :only_media, :exclude_replies).permit(:limit, :only_media, :exclude_replies).merge(core_params) params.slice(:limit, *AccountStatusesFilter::KEYS).permit(:limit, *AccountStatusesFilter::KEYS).merge(core_params)
end end
def insert_pagination_headers def insert_pagination_headers

View file

@ -0,0 +1,134 @@
# frozen_string_literal: true
class AccountStatusesFilter
KEYS = %i(
pinned
tagged
only_media
exclude_replies
exclude_reblogs
).freeze
attr_reader :params, :account, :current_account
def initialize(account, current_account, params = {})
@account = account
@current_account = current_account
@params = params
end
def results
scope = initial_scope
scope.merge!(pinned_scope) if pinned?
scope.merge!(only_media_scope) if only_media?
scope.merge!(no_replies_scope) if exclude_replies?
scope.merge!(no_reblogs_scope) if exclude_reblogs?
scope.merge!(hashtag_scope) if tagged?
scope
end
private
def initial_scope
if suspended?
Status.none
elsif anonymous?
account.statuses.where(visibility: %i(public unlisted))
elsif author?
account.statuses.all # NOTE: #merge! does not work without the #all
elsif blocked?
Status.none
else
filtered_scope
end
end
def filtered_scope
scope = account.statuses.left_outer_joins(:mentions)
scope.merge!(scope.where(visibility: follower? ? %i(public unlisted private) : %i(public unlisted)).or(scope.where(mentions: { account_id: current_account.id })).group(Status.arel_table[:id]))
scope.merge!(filtered_reblogs_scope) if reblogs_may_occur?
scope
end
def filtered_reblogs_scope
Status.left_outer_joins(:reblog).where(reblog_of_id: nil).or(Status.where.not(reblogs_statuses: { account_id: current_account.excluded_from_timeline_account_ids }))
end
def only_media_scope
Status.joins(:media_attachments).merge(account.media_attachments.reorder(nil)).group(Status.arel_table[:id])
end
def no_replies_scope
Status.without_replies
end
def no_reblogs_scope
Status.without_reblogs
end
def pinned_scope
account.pinned_statuses.group(Status.arel_table[:id], StatusPin.arel_table[:created_at])
end
def hashtag_scope
tag = Tag.find_normalized(params[:tagged])
if tag
Status.tagged_with(tag.id)
else
Status.none
end
end
def suspended?
account.suspended?
end
def anonymous?
current_account.nil?
end
def author?
current_account.id == account.id
end
def blocked?
account.blocking?(current_account) || (current_account.domain.present? && account.domain_blocking?(current_account.domain))
end
def follower?
current_account.following?(account)
end
def reblogs_may_occur?
!exclude_reblogs? && !only_media? && !tagged?
end
def pinned?
truthy_param?(:pinned)
end
def only_media?
truthy_param?(:only_media)
end
def exclude_replies?
truthy_param?(:exclude_replies)
end
def exclude_reblogs?
truthy_param?(:exclude_reblogs)
end
def tagged?
params[:tagged].present?
end
def truthy_param?(key)
ActiveModel::Type::Boolean.new.cast(params[key])
end
end

View file

@ -345,28 +345,6 @@ class Status < ApplicationRecord
end end
end end
def permitted_for(target_account, account)
visibility = [:public, :unlisted]
if account.nil?
where(visibility: visibility)
elsif target_account.blocking?(account) || (account.domain.present? && target_account.domain_blocking?(account.domain)) # get rid of blocked peeps
none
elsif account.id == target_account.id # author can see own stuff
all
else
# followers can see followers-only stuff, but also things they are mentioned in.
# non-followers can see everything that isn't private/direct, but can see stuff they are mentioned in.
visibility.push(:private) if account.following?(target_account)
scope = left_outer_joins(:reblog)
scope.where(visibility: visibility)
.or(scope.where(id: account.mentions.select(:status_id)))
.merge(scope.where(reblog_of_id: nil).or(scope.where.not(reblogs_statuses: { account_id: account.excluded_from_timeline_account_ids })))
end
end
def from_text(text) def from_text(text)
return [] if text.blank? return [] if text.blank?

View file

@ -0,0 +1,229 @@
# frozen_string_literal: true
require 'rails_helper'
RSpec.describe AccountStatusesFilter do
let(:account) { Fabricate(:account) }
let(:current_account) { nil }
let(:params) { {} }
subject { described_class.new(account, current_account, params) }
def status!(visibility)
Fabricate(:status, account: account, visibility: visibility)
end
def status_with_tag!(visibility, tag)
Fabricate(:status, account: account, visibility: visibility, tags: [tag])
end
def status_with_parent!(visibility)
Fabricate(:status, account: account, visibility: visibility, thread: Fabricate(:status))
end
def status_with_reblog!(visibility)
Fabricate(:status, account: account, visibility: visibility, reblog: Fabricate(:status))
end
def status_with_mention!(visibility, mentioned_account = nil)
Fabricate(:status, account: account, visibility: visibility).tap do |status|
Fabricate(:mention, status: status, account: mentioned_account || Fabricate(:account))
end
end
def status_with_media_attachment!(visibility)
Fabricate(:status, account: account, visibility: visibility).tap do |status|
Fabricate(:media_attachment, account: account, status: status)
end
end
describe '#results' do
let(:tag) { Fabricate(:tag) }
before do
status!(:public)
status!(:unlisted)
status!(:private)
status_with_parent!(:public)
status_with_reblog!(:public)
status_with_tag!(:public, tag)
status_with_mention!(:direct)
status_with_media_attachment!(:public)
end
shared_examples 'filter params' do
context 'with only_media param' do
let(:params) { { only_media: true } }
it 'returns only statuses with media' do
expect(subject.results.all?(&:with_media?)).to be true
end
end
context 'with tagged param' do
let(:params) { { tagged: tag.name } }
it 'returns only statuses with tag' do
expect(subject.results.all? { |s| s.tags.include?(tag) }).to be true
end
end
context 'with exclude_replies param' do
let(:params) { { exclude_replies: true } }
it 'returns only statuses that are not replies' do
expect(subject.results.none?(&:reply?)).to be true
end
end
context 'with exclude_reblogs param' do
let(:params) { { exclude_reblogs: true } }
it 'returns only statuses that are not reblogs' do
expect(subject.results.none?(&:reblog?)).to be true
end
end
end
context 'when accessed anonymously' do
let(:current_account) { nil }
let(:direct_status) { nil }
it 'returns only public statuses' do
expect(subject.results.pluck(:visibility).uniq).to match_array %w(unlisted public)
end
it 'returns public replies' do
expect(subject.results.pluck(:in_reply_to_id)).to_not be_empty
end
it 'returns public reblogs' do
expect(subject.results.pluck(:reblog_of_id)).to_not be_empty
end
it_behaves_like 'filter params'
end
context 'when accessed with a blocked account' do
let(:current_account) { Fabricate(:account) }
before do
account.block!(current_account)
end
it 'returns nothing' do
expect(subject.results.to_a).to be_empty
end
end
context 'when accessed by self' do
let(:current_account) { account }
it 'returns everything' do
expect(subject.results.pluck(:visibility).uniq).to match_array %w(direct private unlisted public)
end
it 'returns replies' do
expect(subject.results.pluck(:in_reply_to_id)).to_not be_empty
end
it 'returns reblogs' do
expect(subject.results.pluck(:reblog_of_id)).to_not be_empty
end
it_behaves_like 'filter params'
end
context 'when accessed by a follower' do
let(:current_account) { Fabricate(:account) }
before do
current_account.follow!(account)
end
it 'returns private statuses' do
expect(subject.results.pluck(:visibility).uniq).to match_array %w(private unlisted public)
end
it 'returns replies' do
expect(subject.results.pluck(:in_reply_to_id)).to_not be_empty
end
it 'returns reblogs' do
expect(subject.results.pluck(:reblog_of_id)).to_not be_empty
end
context 'when there is a direct status mentioning the non-follower' do
let!(:direct_status) { status_with_mention!(:direct, current_account) }
it 'returns the direct status' do
expect(subject.results.pluck(:id)).to include(direct_status.id)
end
end
it_behaves_like 'filter params'
end
context 'when accessed by a non-follower' do
let(:current_account) { Fabricate(:account) }
it 'returns only public statuses' do
expect(subject.results.pluck(:visibility).uniq).to match_array %w(unlisted public)
end
it 'returns public replies' do
expect(subject.results.pluck(:in_reply_to_id)).to_not be_empty
end
it 'returns public reblogs' do
expect(subject.results.pluck(:reblog_of_id)).to_not be_empty
end
context 'when there is a private status mentioning the non-follower' do
let!(:private_status) { status_with_mention!(:private, current_account) }
it 'returns the private status' do
expect(subject.results.pluck(:id)).to include(private_status.id)
end
end
context 'when blocking a reblogged account' do
let(:reblog) { status_with_reblog!('public') }
before do
current_account.block!(reblog.reblog.account)
end
it 'does not return reblog of blocked account' do
expect(subject.results.pluck(:id)).to_not include(reblog.id)
end
end
context 'when muting a reblogged account' do
let(:reblog) { status_with_reblog!('public') }
before do
current_account.mute!(reblog.reblog.account)
end
it 'does not return reblog of muted account' do
expect(subject.results.pluck(:id)).to_not include(reblog.id)
end
end
context 'when blocked by a reblogged account' do
let(:reblog) { status_with_reblog!('public') }
before do
reblog.reblog.account.block!(current_account)
end
it 'does not return reblog of blocked-by account' do
expect(subject.results.pluck(:id)).to_not include(reblog.id)
end
end
it_behaves_like 'filter params'
end
end
end

View file

@ -348,59 +348,6 @@ RSpec.describe Status, type: :model do
end end
end end
describe '.permitted_for' do
subject { described_class.permitted_for(target_account, account).pluck(:visibility) }
let(:target_account) { alice }
let(:account) { bob }
let!(:public_status) { Fabricate(:status, account: target_account, visibility: 'public') }
let!(:unlisted_status) { Fabricate(:status, account: target_account, visibility: 'unlisted') }
let!(:private_status) { Fabricate(:status, account: target_account, visibility: 'private') }
let!(:direct_status) do
Fabricate(:status, account: target_account, visibility: 'direct').tap do |status|
Fabricate(:mention, status: status, account: account)
end
end
let!(:other_direct_status) do
Fabricate(:status, account: target_account, visibility: 'direct').tap do |status|
Fabricate(:mention, status: status)
end
end
context 'given nil' do
let(:account) { nil }
let(:direct_status) { nil }
it { is_expected.to eq(%w(unlisted public)) }
end
context 'given blocked account' do
before do
target_account.block!(account)
end
it { is_expected.to be_empty }
end
context 'given same account' do
let(:account) { target_account }
it { is_expected.to eq(%w(direct direct private unlisted public)) }
end
context 'given followed account' do
before do
account.follow!(target_account)
end
it { is_expected.to eq(%w(direct private unlisted public)) }
end
context 'given unfollowed account' do
it { is_expected.to eq(%w(direct unlisted public)) }
end
end
describe 'before_validation' do describe 'before_validation' do
it 'sets account being replied to correctly over intermediary nodes' do it 'sets account being replied to correctly over intermediary nodes' do
first_status = Fabricate(:status, account: bob) first_status = Fabricate(:status, account: bob)